summaryrefslogtreecommitdiffstats
path: root/remote/doc/marionette/CodeStyle.md
blob: abd70e09c3b31e556bfdb8a0c52a7e7513f0509b (plain)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
# Style guide

Like other projects, we also have some guidelines to keep to the code.
For the overall Marionette project, a few rough rules are:

* Make your code readable and sensible, and don’t try to be
  clever.  Prefer simple and easy solutions over more convoluted
  and foreign syntax.

* Fixing style violations whilst working on a real change as a
  preparatory clean-up step is good, but otherwise avoid useless
  code churn for the sake of conforming to the style guide.

* Code is mutable and not written in stone.  Nothing that
  is checked in is sacred and we encourage change to make
  remote/marionette a pleasant ecosystem to work in.

## JavaScript

Marionette is written in JavaScript and ships
as part of Firefox.  We have access to all the latest ECMAScript
features currently in development, usually before it ships in the
wild and we try to make use of new features when appropriate,
especially when they move us off legacy internal replacements
(such as Promise.jsm and Task.jsm).

One of the peculiarities of working on JavaScript code that ships as
part of a runtime platform is, that unlike in a regular web document,
we share a single global state with the rest of Firefox.  This means
we have to be responsible and not leak resources unnecessarily.

JS code in Gecko is organised into _modules_ carrying _.js_ or _.jsm_
file extensions.  Depending on the area of Gecko you’re working on,
you may find they have different techniques for exporting symbols,
varying indentation and code style, as well as varying linting
requirements.

To export symbols to other Marionette modules, remember to assign
your exported symbols to the shared global `this`:

    const EXPORTED_SYMBOLS = ["PollPromise", "TimedPromise"];

When importing symbols in Marionette code, try to be specific about
what you need:

    const { TimedPromise } = ChromeUtils.import(
      "chrome://remote/content/marionette/sync.js"
    );

We prefer object assignment shorthands when redefining names,
for example when you use functionality from the `Components` global:

    const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;

When using symbols by their own name, the assignment name can be
omitted:

    const {TYPE_ONE_SHOT, TYPE_REPEATING_SLACK} = Ci.nsITimer;

In addition to the default [Mozilla eslint rules], we have [our
own specialisations] that are stricter and enforce more security.
A few notable examples are that we disallow fallthrough `case`
statements unless they are explicitly grouped together:

    switch (x) {
      case "foo":
        doSomething();

      case "bar":  // <-- disallowed!
        doSomethingElse();
        break;

      case "baz":
      case "bah":  // <-- allowed (-:
        doCrazyThings();
    }

We disallow the use of `var`, for which we always prefer `let` and
`const` as replacements.  Do be aware that `const` does not mean
that the variable is immutable: just that it cannot be reassigned.
We require all lines to end with semicolons, disallow construction
of plain `new Object()`, require variable names to be camel-cased,
and complain about unused variables.

For purely aesthetic reasons we indent our code with two spaces,
which includes switch-statement `case`s, and limit the maximum
line length to 78 columns.  When you need to wrap a statement to
the next line, the second line is indented with four spaces, like this:

    throw new TypeError(
        "Expected an element or WindowProxy, " +
        pprint`got: ${el}`);

This is not normally something you have to think to deeply about as
it is enforced by the [linter].  The linter also has an automatic
mode that fixes and formats certain classes of style violations.

If you find yourself struggling to fit a long statement on one line,
this is usually an indication that it is too long and should be
split into multiple lines.  This is also a helpful tip to make the
code easier to read.  Assigning transitive values to descriptive
variable names can serve as self-documentation:

    let location = event.target.documentURI || event.target.location.href;
    log.debug(`Received DOM event ${event.type} for ${location}`);

On the topic of variable naming the opinions are as many as programmers
writing code, but it is often helpful to keep the input and output
arguments to functions descriptive (longer), and let transitive
internal values to be described more succinctly:

    /** Prettifies instance of Error and its stacktrace to a string. */
    function stringify(error) {
      try {
        let s = error.toString();
        if ("stack" in error) {
          s += "\n" + error.stack;
        }
        return s;
      } catch (e) {
        return "<unprintable error>";
      }
    }

When we can, we try to extract the relevant object properties in
the arguments to an event handler or a function:

    const responseListener = ({name, target, json, data}) => { … };

Instead of:

    const responseListener = msg => {
      let name = msg.name;
      let target = msg.target;
      let json = msg.json;
      let data = msg.data;
      …
    };

All source files should have `"use strict";` as the first directive
so that the file is parsed in [strict mode].

Every source code file that ships as part of the Firefox bundle
must also have a [copying header], such as this:

    /* This Source Code Form is subject to the terms of the Mozilla Public
     * License, v. 2.0. If a copy of the MPL was not distributed with this file,
     * You can obtain one at http://mozilla.org/MPL/2.0/. */

New xpcshell test files _should not_ have a license header as all
new Mozilla tests should be in the [public domain] so that they can
easily be shared with other browser vendors.  We want to re-license
existing tests covered by the [MPL] so that they can be shared.
We very much welcome your help in doing version control archeology
to make this happen!

The practical details of working on the Marionette code is outlined
in [Contributing.md], but generally you do not have to re-build
Firefox when changing code.  Any change to remote/marionette/*.js
will be picked up on restarting Firefox.  The only notable exception
is remote/components/Marionette.jsm, which does require
a re-build.

[strict mode]: https://developer.mozilla.org/docs/Web/JavaScript/Reference/Strict_mode
[Mozilla eslint rules]: https://searchfox.org/mozilla-central/source/.eslintrc.js
[our own specialisations]: https://searchfox.org/mozilla-central/source/remote/marionette/.eslintrc.js
[linter]: #linting
[copying header]: https://www.mozilla.org/en-US/MPL/headers/
[public domain]: https://creativecommons.org/publicdomain/zero/1.0/
[MPL]: https://www.mozilla.org/en-US/MPL/2.0/
[Contributing.md]: ./Contributing.md

## Python

TODO

## Documentation

We keep our documentation in-tree under [remote/doc/marionette]
and [testing/geckodriver/doc].  Updates and minor changes to
documentation should ideally not be scrutinised to the same degree
as code changes to encourage frequent updates so that the documentation
does not go stale.  To that end, documentation changes with `r=me`
from module peers are permitted.

Use fmt(1) or an equivalent editor specific mechanism (such as Meta-Q
in Emacs) to format paragraphs at a maximum width of 75 columns
with a goal of roughly 65.  This is equivalent to `fmt -w 75 -g 65`,
which happens to be the default on BSD and macOS.

We endeavour to document all _public APIs_ of the Marionette component.
These include public functions—or command implementations—on
the `GeckoDriver` class, as well as all exported symbols from
other modules.  Documentation for non-exported symbols is not required.

[remote/doc/marionette]: https://searchfox.org/mozilla-central/source/remote/marionette/doc
[testing/geckodriver/doc]: https://searchfox.org/mozilla-central/source/testing/geckodriver/doc

## Linting

Marionette consists mostly of JavaScript (server) and Python (client,
harness, test runner) code.  We lint our code with [mozlint],
which harmonises the output from [eslint] and [ruff].

To run the linter with a sensible output:

    % ./mach lint -funix remote/marionette

For certain classes of style violations the eslint linter has
an automatic mode for fixing and formatting your code.  This is
particularly useful to keep to whitespace and indentation rules:

    % ./mach eslint --fix remote/marionette

The linter is also run as a try job (shorthand `ES`) which means
any style violations will automatically block a patch from landing
(if using Autoland) or cause your changeset to be backed out (if
landing directly on mozilla-inbound).

If you use git(1) you can [enable automatic linting] before you push
to a remote through a pre-push (or pre-commit) hook.  This will
run the linters on the changed files before a push and abort if
there are any problems.  This is convenient for avoiding a try run
failing due to a stupid linting issue.

[mozlint]: /code-quality/lint/mozlint.rst
[eslint]: /code-quality/lint/linters/eslint.rst
[ruff]: /code-quality/lint/linters/ruff.rst
[enable automatic linting]: /code-quality/lint/usage.rst#using-a-vcs-hook