gecko-dev/remote/marionette/doc/CodeStyle.md

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 XPCOM flavoured 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} = Cu.import("chrome://remote/content/marionette/sync.js", {});

The linter will complain if you import a named symbol that is not in use. If however you need to import every symbol, you can:

const wait = {};
Cu.import("chrome://remote/content/marionette/sync.js", wait);

wait.sleep(42);
await wait.TimedPromise(…);

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 cases, 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.js, which does require a re-build.

Python

TODO

Documentation

We keep our documentation in-tree under remote/marionette/doc 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.

The API documentation can be regenerated to remote/marionette/doc/internals so:

The API documentation uses jsdoc and is generated to https://firefox-source-docs.mozilla.org/testing/marionette/internals on Taskcluster. You may also build the documentation locally:

% ./mach 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 flake8.

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.