| 1 | | These are JavaScript coding styles used in the [https://trac.webkit.org/browser/trunk/Source/WebInspectorUI/ Source/WebInspectorUI/UserInterface] folder. |
| 2 | | |
| 3 | | (Note: Few if any of these guidelines are checked by `check-webkit-style`. There's a tracking bug for that: [https://bugs.webkit.org/show_bug.cgi?id=125045]) |
| 4 | | |
| 5 | | == Non-code Style |
| 6 | | For user-facing strings, we follow the macOS Human Interface Guidelines. |
| 7 | | * [https://developer.apple.com/library/content/documentation/UserExperience/Conceptual/OSXHIGuidelines/Assistance.html Guidelines for help tags] (aka tooltips) |
| 8 | | * [https://developer.apple.com/library/content/documentation/UserExperience/Conceptual/OSXHIGuidelines/TerminologyWording.html#//apple_ref/doc/uid/20000957-CH15-SW4 Guidelines for UI labels] |
| 9 | | * [https://developer.apple.com/library/content/documentation/UserExperience/Conceptual/OSXHIGuidelines/Keyboard.html#//apple_ref/doc/uid/20000957-CH84-SW1 Guidelines for keyboard shortcuts] |
| 10 | | |
| 11 | | == Localization |
| 12 | | |
| 13 | | * Include a unique key and comment when dealing with arbitrary strings. Use `@` in the key to denote where in the interface the string can be found. (ex: `WI.UIString("Frames")` vs `WI.UIString("Frames", "Frames @ Execution Context Picker", "Title for list of HTML subframe JavaScript execution contexts")`). |
| 14 | | |
| 15 | | == Tokens, spacing, indentation, syntax |
| 16 | | |
| 17 | | * No trailing whitespace. |
| 18 | | * Indent with 4 spaces. |
| 19 | | * Double quoted strings; use template strings if a bunch of interpolation is required. |
| 20 | | * The `{` after a named, non-inlined function goes on the next line. Anywhere else, the `{` stays on the same line. |
| 21 | | * Style for object literals is: `{key1: value1, key2: value2}`. When key and variable names coincide, use the syntax `{key}` rather than `{key: key}`. If the object is complex enough, each `key: value,` should be on its own line. |
| 22 | | * Always include a trailing comma for object literals. |
| 23 | | * Add new lines before and after different tasks performed in the same function. |
| 24 | | * Else-blocks should share a line with leading `}`. |
| 25 | | * Long promise chains should place `.then()` blocks on a new line. |
| 26 | | * Calling a constructor with no arguments should have no parenthesis `()`. (ex: `var map = new Map;`) |
| 27 | | * Put anonymous functions inline if they are not used as a subroutine. |
| 28 | | * Prefer `let` to `var`, unless the variable is not used in a block scoping manner. Be careful when using `let` with `case` switches, as [https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let#Temporal_dead_zone_and_errors_with_let all switch cases share the same block by default]. Only use `const` for values that will not change between executions (i.e. actual constants). |
| 29 | | * Use arrow functions when possible, unless it makes code less readable. See below for examples. |
| 30 | | * For default parameters, add a space around the default assignment: `function foo(isGood = false)` |
| 31 | | * Trivial public getters can be made a single line and moved to the top of the list of getters in a class, unless there is a corresponding setter. |
| 32 | | |
| 33 | | == Naming things |
| 34 | | |
| 35 | | * Avoid using the "on" prefix where possible. The `_onFoo` methods can just be `_foo` or `_handleFoo` (preferred for event listeners). |
| 36 | | * New class names should use the name of the base class as a suffix. (ex: `TimelinesContentView` < `ContentView`). Exceptions: classes extending `WI.Object` (unless they are a represented object). |
| 37 | | * Spell out `identifier` instead of `id` if not doing so would result in a name ending with capitalized `Id`. For example, just `this.id` is fine, but `this.breakpointId` should be `this.breakpointIdentifier`. |
| 38 | | * An object's events live on the `Event` property of the constructor. Event names are properties on the `Event` object, and property values duplicate the event name, but are lowercased, hyphenated, and prefixed with the constructor name. See the skeleton example below. |
| 39 | | * When serializing a function to be evaluated in a different execution context, such as from inspector to inspected page or layout test to inspector, make it obvious where the function is going to be evaluated. For example, if a function will be sent from Inspector context to inspected page context, the name `inspectedPage_node_getFlowInfo()` signifies that the function will be evaluated in the inspected page, with `this` bound to a node, and it performs the action `getFlowInfo`. |
| 40 | | |
| 41 | | == API preferences |
| 42 | | |
| 43 | | * Use `Map` and `Set` collections instead of plain objects if the key values are unknown or not monotonic (i.e., frequently added then removed). |
| 44 | | * Use `hsla()` over hex or `rgba()` for colors in CSS. |
| 45 | | * Use `for..of` syntax when performing actions on each element. Use `forEach` when chaining methods in a functional style. Use a classical for loop when doing index math. |
| 46 | | * When using `forEach` or `map`, use an arrow function or supply the `this`-object as the optional second parameter rather than binding it. |
| 47 | | * In promise chains, use arrow functions for lexical `this`, rather than assigning `const instance = this;' or `.bind`ing every function's `this`-argument. |
| 48 | | * Use destructuring assignment when digging values out of a JSON object or "args" object. |
| 49 | | * Use default parameters when it makes sense. |
| 50 | | * Use `super` to make calls to base class (possibly overridden) methods. |
| 51 | | |
| 52 | | == Layering and abstractions |
| 53 | | |
| 54 | | * Firewall the protocol inside the Manager classes. JSON objects received from the protocol are called "payload" in the code. The payload is usually deconstructed at the Managers level and passes down as smart objects inheriting from `WI.Object`. |
| 55 | | * Avoid accessing *View classes from *Manager or *Object classes. This is a layering violation that prevents writing tests for models. |
| 56 | | * Avoid storing DOM elements in *Manager or *Object classes. (see above.) |
| 57 | | * In the backend, avoid using Inspector TypeBuilders outside of InspectorAgent classes. We want to isolate protocol considerations from other functionality in JavaScriptCore and WebCore. |
| 58 | | |
| 59 | | == Understanding and Using Promises |
| 60 | | |
| 61 | | [http://blog.parse.com/2013/01/29/whats-so-great-about-javascript-promises/ What's so great about Promises?] [http://domenic.me/2012/10/14/youre-missing-the-point-of-promises/ The point of promises is to give us back functional composition] and error bubbling in the async world. They do this by saying that your functions should return a promise, which can do one of two things: |
| 62 | | |
| 63 | | 1. Become __fulfilled__ by a **value** |
| 64 | | 2. Become __rejected__ with an **Error instance** or by throwing an exception |
| 65 | | |
| 66 | | A promise that is eiher fulfilled or rejected is said to be __settled__. A promise that has not settled is said to be __pending__. |
| 67 | | |
| 68 | | And, if you have a correctly implemented `then()` function, then fulfillment and rejection will compose just like their synchronous counterparts, with fulfillments flowing up a compositional chain, but being interrupted at any time by a rejection that is only handled by someone who declares they are ready to handle it. |
| 69 | | |
| 70 | | === Promise Gotchas |
| 71 | | |
| 72 | | (Summarized from [http://making.change.org/post/69613524472/promises-and-error-handling change.org Blog] and [http://taoofcode.net/promise-anti-patterns/ The Art of Code Blog]) |
| 73 | | |
| 74 | | * Don't nest promises to perform multiple async operations; instead, chain them or use `Promise.all()`. |
| 75 | | * Beware of storing or returning promise values that are not from the end of a chain. Each `.then()` returns a new promise value, so return the last promise. |
| 76 | | * Use `Promise.all()` with `map()` to process an array of asynchronous work in parallel. Use `Promise.all()` with `reduce()` to sequence an array asynchronous work. |
| 77 | | * If a result may be a promise or an actual value, wrap the value in a promise, e.g., `Promise.resolve(val)` |
| 78 | | * Use `.catch()` at the end of a chain to perform error handling. '''Most promise chains should have a catch block to avoid dropping errors'''. |
| 79 | | * To reject a promise, throw an `Error` instance or call the `reject` callback with an `Error` instance. |
| 80 | | * A `.catch()` block is considered resolved if it does not re-throw an `Error` instance. Re-throw if you want to log an error message and allow other parts of a chain (i.e, an API client) to handle an error condition. |
| 81 | | * Don't directly pass a promise's `resolve` function to `Object.addEventListener`, as it will leak the promise if the event never fires. Instead, use a single-fire `WI.EventListener` object defined outside of the promise chain and connect it inside a `.then()` body. Inside the `.catch` block, disconnect the `EventListener` if necessary. |
| 82 | | * For APIs that return promises, document what the fulfilled value will be, if any. Example: `createSession() // --> (sessionId)` |
| 83 | | |
| 84 | | == Arrow Functions |
| 85 | | |
| 86 | | Arrow functions simplify a common use of anonymous functions by providing a shorter syntax, lexical binding of `this` and `arguments`, and implicit return. While this new syntax enables new levels of terse code, we must take care to keep our code readable. |
| 87 | | |
| 88 | | === Implicit return |
| 89 | | |
| 90 | | Arrow functions with one expression have an implicit return. All of these are equivalent (modulo `this` binding, arguments, constructor usage, etc.): |
| 91 | | |
| 92 | | {{{ |
| 93 | | 1 let foo = val => val; |
| 94 | | 2 let foo = (val) => val |
| 95 | | 3 let foo = (val) => val; |
| 96 | | 4 let foo = (val) => { return value++; }; |
| 97 | | 5 let foo = (val) => { |
| 98 | | return value++; |
| 99 | | }; |
| 100 | | 6 let foo = function doStuff(val) { return value++; }; |
| 101 | | 7 let foo = function doStuff(val) { |
| 102 | | return value++; |
| 103 | | }; |
| | 1 | {{{#!html |
| | 2 | <div style="padding: 1em; background: #fff8c4; border-left: 4px solid #f5c842; margin: 1em 0;"> |
| | 3 | <strong>⚠ This page has moved.</strong> The current documentation is at |
| | 4 | <a href="https://docs.webkit.org/Deep%20Dive/Web%20Inspector/StyleGuide.html">docs.webkit.org/Deep Dive/Web Inspector/StyleGuide</a>. |
| | 5 | </div> |
| 105 | | |
| 106 | | Never use option (1), because it is a special case that only applies when the function has one argument, reducing predictability. |
| 107 | | |
| 108 | | In cases where the return value is used and the single expression is a constant ("foo"), a variable (foo), a member (this.foo), or evaluates to a Promise, use option (2). Never use braces though, because implicit return only works if there are no braces around the single expression. |
| 109 | | |
| 110 | | In cases where the expression computes a value (a + 42) or performs a side effect (++a), prefer option 5. |
| 111 | | In some sense, curly braces are a signpost to the effect of "careful, we do actual work here". |
| 112 | | |
| 113 | | If the implicit return is not used (4, 5, 6, 7), always put the function body on new lines from the `{` and `}` (as demonstrated in 5 and 7). |
| 114 | | |
| 115 | | GOOD: |
| 116 | | |
| 117 | | {{{ |
| 118 | | setTimeout(() => { |
| 119 | | testRunner.notifyDone(); |
| 120 | | }, 0); |
| 121 | | }}} |
| 122 | | |
| 123 | | BAD: |
| 124 | | |
| 125 | | {{{ |
| 126 | | // return value not implicitly returned |
| 127 | | |
| 128 | | setTimeout(() => { |
| 129 | | testRunner.notifyDone() |
| 130 | | }, 0); |
| 131 | | }}} |
| 132 | | |
| 133 | | |
| 134 | | {{{ |
| 135 | | // implicit return value not used |
| 136 | | |
| 137 | | setTimeout(() => testRunner.notifyDone(), 0); |
| 138 | | }}} |
| 139 | | |
| 140 | | === When not to arrow |
| 141 | | |
| 142 | | When assigning a function to a subclass prototype (in the old way of setting up classes), always use the normal function syntax, to avoid breaking subclasses who use a different 'this' binding. Note that arrow functions are NOT acceptable for assigning functions to singleton objects like `WI`, since the captured lexical `this` is typically the global object. |
| 143 | | |
| 144 | | GOOD: |
| 145 | | |
| 146 | | {{{ |
| 147 | | Base.prototype.compute = function(a, b, c) { |
| 148 | | // ... |
| 149 | | }; |
| 150 | | |
| 151 | | Foo.prototype.compute = function(a, b, c) { |
| 152 | | Base.prototype.compute.call(this, a, b, c); |
| 153 | | }; |
| 154 | | |
| 155 | | WI.UIString = function(format, args) { |
| 156 | | // ... |
| 157 | | }; |
| 158 | | }}} |
| 159 | | |
| 160 | | BAD: |
| 161 | | |
| 162 | | {{{ |
| 163 | | // `this` will be `window` |
| 164 | | |
| 165 | | Base.prototype.compute = (a, b, c) => { |
| 166 | | // ... |
| 167 | | }; |
| 168 | | |
| 169 | | Foo.prototype.compute = (a, b, c) => { |
| 170 | | Base.prototype.compute.call(this, a, b, c); |
| 171 | | }; |
| 172 | | |
| 173 | | WI.UIString = (format, args) => { |
| 174 | | // ... |
| 175 | | }; |
| 176 | | }}} |
| 177 | | |
| 178 | | Also use the normal function syntax when naming an anonymous function improves readability of the code. In this case, use Function.prototype.bind or assign the arrow function into a local variable first. |
| 179 | | |
| 180 | | GOOD: |
| 181 | | |
| 182 | | {{{ |
| 183 | | Promise.resolve().then( |
| 184 | | function resolved(value) { ... }, |
| 185 | | function rejected(value) { ... } |
| 186 | | ); |
| 187 | | }}} |
| 188 | | |
| 189 | | BAD: |
| 190 | | |
| 191 | | {{{ |
| 192 | | Promise.resolve().then( |
| 193 | | (value) => { ... }, |
| 194 | | (value) => { ... } |
| 195 | | ); |
| 196 | | }}} |
| 197 | | |
| 198 | | |
| 199 | | == New class skeleton |
| 200 | | |
| 201 | | New Inspector object classes use ES6 class syntax and should have the following format: |
| 202 | | |
| 203 | | {{{ |
| 204 | | WI.NewObjectType = class NewObjectType extends WI.Object |
| 205 | | { |
| 206 | | constructor(type, param) |
| 207 | | { |
| 208 | | console.assert(param instanceof WI.ExpectedType); |
| 209 | | |
| 210 | | super(); |
| 211 | | |
| 212 | | this._type = type; |
| 213 | | this._propertyName = param; |
| 214 | | } |
| 215 | | |
| 216 | | // Static |
| 217 | | |
| 218 | | static computeBestWidth(things) |
| 219 | | { |
| 220 | | // ... |
| 221 | | return 3.14159; |
| 222 | | } |
| 223 | | |
| 224 | | // Public |
| 225 | | |
| 226 | | get type() { return this._type; } |
| 227 | | |
| 228 | | get propertyName() |
| 229 | | { |
| 230 | | return this._propertyName; |
| 231 | | } |
| 232 | | |
| 233 | | set propertyName(value) |
| 234 | | { |
| 235 | | this._propertyName = value; |
| 236 | | this.dispatchEventToListeners(WI.NewObjectType.Event.PropertyWasChanged); |
| 237 | | } |
| 238 | | |
| 239 | | publicMethod() |
| 240 | | { |
| 241 | | /* public methods called outside the class */ |
| 242 | | } |
| 243 | | |
| 244 | | // Protected |
| 245 | | |
| 246 | | protectedMethod(event) |
| 247 | | { |
| 248 | | /* delegate methods and overrides */ |
| 249 | | } |
| 250 | | |
| 251 | | // Private |
| 252 | | |
| 253 | | _privateMethod() |
| 254 | | { |
| 255 | | /* private methods are underscore prefixed */ |
| 256 | | } |
| 257 | | }; |
| 258 | | |
| 259 | | WI.NewObjectType.Event = { |
| 260 | | PropertyWasChanged: "new-object-type-property-was-changed", |
| 261 | | }; |
| 262 | | |
| 263 | | }}} |
| 264 | | |
| 265 | | == CSS |
| 266 | | |
| 267 | | === z-index |
| 268 | | |
| 269 | | Z-index variables are defined in [https://trac.webkit.org/browser/trunk/Source/WebInspectorUI/UserInterface/Views/Variables.css Variables.css]. Usage example: |
| 270 | | |
| 271 | | {{{ |
| 272 | | .popover { |
| 273 | | z-index: var(--z-index-popover); |
| 274 | | } |
| 275 | | }}} |
| 276 | | |
| 277 | | Read more about the rationale in [https://bugs.webkit.org/show_bug.cgi?id=151978 Bug 151978]. |
