diff options
Diffstat (limited to 'devtools/docs/contributing')
30 files changed, 1577 insertions, 0 deletions
diff --git a/devtools/docs/contributing/code-reviews-checklist.md b/devtools/docs/contributing/code-reviews-checklist.md new file mode 100644 index 0000000000..3b63f92bf2 --- /dev/null +++ b/devtools/docs/contributing/code-reviews-checklist.md @@ -0,0 +1,59 @@ +# Code reviews checklist + +This checklist is primarily aimed at reviewers, as it lists important points to check while reviewing a patch. + +It can also be useful for patch authors: if the changes comply with these guidelines, then it's more likely the review will be approved. + +## Bug status and patch file + +* Bug status is assigned, and assignee is correctly set. +* Commit title and message follow [the conventions](https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions). +* Commit message says [what is being changed and why](http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/commits.html#write-detailed-commit-messages). +* Patch applies locally to current sources with no merge required. +* Check that every new file introduced by the patch has the proper Mozilla license header: https://www.mozilla.org/en-US/MPL/headers/ + +## Manual testing + +* Verify: + * if it's a new feature, the patch implements it. + * if it's a fix, the patch fixes the bug it addresses. +* Report any problems you find in the global review comment. +* Decide if any of those problems should block landing the change, or if they can be filed as follow-up bugs instead, to be fixed later. + +## Automated testing + +* Run new/modified tests, [with and without e10s](../tests/writing-tests.md#electrolysis). +* Watch out for tests that pass but log exceptions or end before protocol requests are handled. +* Watch out for slow/long tests: suggest many small tests rather than single long tests. +* Watch out for new tests written as integration tests instead of as unit tests: unit tests should be the preferred option, when possible. + +## Code review + +* Code changes: + * Review only what was changed by the contributor. + * Code formatting follows [our ESLint rules](eslint.md) and [coding standards](./coding-standards.md). + * Code is properly commented, JSDoc is updated, new "public" methods all have JSDoc, see the [comment guidelines](./javascript.md#comments). + * If Promise code was added/modified, the right promise syntax is used and rejections are handled. See [asynchronous code](./javascript.md#asynchronous-code). + * If a CSS file is added/modified, it follows [the CSS guidelines](./css.md). + * If a React or Redux module is added/modified, it follows the [React/Redux guidelines](./javascript.md#react--redux). + * If DevTools server code that should run in a worker is added/modified then it shouldn't use Services +* Test changes: + * The feature or bug is [tested by new tests, or a modification of existing tests](../tests/writing-tests.md). + * [Test logging](../tests/writing-tests.md#logs-and-comments) is sufficient to help investigating test failures/timeouts. + * [Test is e10s compliant](../tests/writing-tests.md#e10s-electrolysis) (doesn't try to access web content from the parent process, etc…). + * Tests are [clean and maintainable](../tests/writing-tests.md#writing-clean-maintainable-test-code). + * A try push has started (or even better, is green already). +* User facing changes: + * If any user-facing interfaces are added/modified, double-check the changes with the UX mockups or specs, if available. If there's any confusion, need-info the UX designer.<!--TODO this needs updating with the new process--> + * If a user facing string has been added, it is localized and follows [the localization guidelines](../files/adding-files.md#localization-l10n). + * If a user-facing string has changed meaning, [the key has been updated](https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings). + * If a new image is added, it is a SVG image or there is a reason for not using a SVG. + * If a SVG is added/modified, it follows [the SVG guidelines](../frontend/svgs.md). + * If a documented feature has been modified, the keyword `dev-doc-needed` is present on the bug. + +## Finalize the review + +* R+: the code should land as soon as possible. +* R+ with comments: there are some comments, but they are minor enough, or don't require a new review once addressed, trust the author. +* R cancel / R- / F+: there is something wrong with the code, and a new review is required. + diff --git a/devtools/docs/contributing/code-reviews-find-reviewer.md b/devtools/docs/contributing/code-reviews-find-reviewer.md new file mode 100644 index 0000000000..fc1a8d3c8e --- /dev/null +++ b/devtools/docs/contributing/code-reviews-find-reviewer.md @@ -0,0 +1,7 @@ +# Finding suitable reviewers + +The code in DevTools is divided into modules (because there's just so much code it's impossible for one person to know *everything* off the top of their head). + +Each module has one or more "owners", who know a lot about it. And they're the people you want to ask to review your code 😀 + +You might already know who is the reviewer (perhaps the person that is mentoring the bug, perhaps your team mate), but in case of doubt, you can [consult this list of people and modules](https://firefox-dev.tools/#people-and-modules). diff --git a/devtools/docs/contributing/code-reviews-setup.md b/devtools/docs/contributing/code-reviews-setup.md new file mode 100644 index 0000000000..8813e58ddb --- /dev/null +++ b/devtools/docs/contributing/code-reviews-setup.md @@ -0,0 +1,27 @@ +# Set up for code reviews + +There are two things you need to do before you can get a code review, although you only need to do this once 😃 + +## Set up to get code reviews in Phabricator + +We use an online tool called Phabricator for code reviews. To create an account in Phabricator, you first need the Bugzilla account that you created earlier. If you don't have one, [create it now](../getting-started/bugzilla.md). + +--- + +⚠️ *IMPORTANT:* ⚠️️️ + +It's helpful to have the same user name in both Bugzilla and Phabricator, so that people always know how to find you. + +Bugzilla's `Real name` field can be edited after the fact, but you cannot change Phabricator's username once the account has been created. + +If you added an `:ircnickname` in your Bugzilla's `Real name`, Phabricator will use that to pre-fill the username field when you create the account. **Please double check you like the proposed username, and make any corrections before you register**. + +--- + +Once you understand the above, please [create a Phabricator account](https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#creating-an-account). + + + +## Set up to send code for review + +In order to push your commit to Phabricator, you need to install [moz-phab](https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#using-moz-phab). diff --git a/devtools/docs/contributing/code-reviews.md b/devtools/docs/contributing/code-reviews.md new file mode 100644 index 0000000000..9ddb03b5bb --- /dev/null +++ b/devtools/docs/contributing/code-reviews.md @@ -0,0 +1,104 @@ +# Code reviews + +A review is required before code is added to Mozilla's repository. In addition, contrary to what you might have seen in other open source projects, code is not pushed directly to the repository. Instead, once the code is approved by reviewers, they will _request_ the code to be _landed_ on the repository. + +All this can be done somehow manually, but we have infrastructure in place to help us with reviews and landing code. + +Learn about how to get started with getting your code reviewed and landed in our [setting up](./code-reviews-setup.md) guide. + +And read on to learn about why and how we do code reviews. + +## Why do we do code reviews? + +### Quality + +Doing code reviews helps with **correctness**. It gives us a chance to check that the code: + +- fixes the problem, +- doesn't have bugs, +- doesn't regress other things, +- covers edge cases. + +A review is also a chance for the reviewer to check that the right **test coverage** is here, that the change doesn't have **performance** problems, that it **simplifies hard to understand code** and that it comes with **code comments** and **adequate documentation**. + +### Learning and sharing knowledge + +While going through the process of doing a code review, **both the reviewer and the reviewee** will be learning something (a new part of the code, a new efficient way to write code, tests, localization, etc.). + +Making the code easy to understand by the reviewer helps everybody. + +Doing reviews also helps people working on DevTools feel more comfortable and learn about new parts of the codebase. + +It helps build consensus around ideas and practices. + +### Maintenance + +Doing reviews gives us an opportunity to check we are willing to maintain and support the new code that being introduced for a feature or bug fix. + +### Code consistency + +It is also a great opportunity for us to check that whatever new code is being written is consistent with what already exists in the code base. + +### Shared responsibility + +Approving a code review means that you are the second person who thinks this change is correct and a good idea. Doing this makes you responsible for the code change just as much as the author. + +It is the entire DevTools group who owns the code, not just the author. We write code as a group, not as individuals, because on the long-term it's the group that maintains it. + +Having a review discussion shares the ownership of the code, because authors and reviewers all put a little bit of themselves in it, and the code that results isn't just the result of one person's work + +## What should be avoided in code reviews? + +- Style nits that a linter should ideally handle. +- Requests that are outside of the scope of the bug. + +## What are some good practices for code reviews? + +### Use empathetic language. + +More reading: + +- [Mindful Communication in Code Reviews](http://amyciavolino.com/assets/MindfulCommunicationInCodeReviews.pdf) +- [How to Do Code Reviews Like a Human](https://mtlynch.io/human-code-reviews-1/) + +### Prefer smaller commits over large monolithic commits. + +It makes it easier for the reviewer to provide a quality review. It's also easier to get consensus on the change if that change is small. Finally, it's easier to understand the risk, and find regressions. + +### Be explicit + +Specifically, be explicit about required versus optional changes. + +Reviews are conversations, and the reviewee should feel comfortable on discussing and pushing back on changes before making them. + +### Be quick + +As a reviewer, strive to answer in a couple of days, or at least under a week. + +If that is not possible, please update the bug to let the requester know when you'll get to the review. Or forward it to someone else who has more time. + +### Ask for help + +Know when you're out of your comfort zone as a reviewer, and don't hesitate to ask for an extra review to someone else. + +It's fine, you can't know everything, and the more people participate, the more everybody learns, and the more bugs we avoid. + +## How do we communicate? + +First and foremost, like in any Mozilla-run platforms or events, please abide by [the Community Participation Guidelines](https://www.mozilla.org/en-US/about/governance/policies/participation/). + +Maintainers should **lead by example through their tone and language**. We want to build an inclusive and safe environment for working together. + +As a reviewer, **double-check your comments**. Just because you're a reviewer and think you have a better solution doesn't mean that's true. Assume **the code author has spent more time thinking about this part of the code than you have** (if you're the reviewer) and might actually be right, even if you originally thought something was wrong. It doesn't take long to look up the code and double-check. + +**Being inclusive** is highly important. We shouldn't make any assumptions about the person requesting a review, or about the person you're asking a review from, and always provide as much information as required, in a way that is as inclusive as possible. + +The bug will live forever online, and many people will read it long after the author and reviewers are done. + +Talking over video/IRC/Slack is a great way to get buy-in and share responsibility over a solution. It is also helpful to resolve disagreement and work through issues in a more relaxed environment. + +**Do not criticize** the reviewee, talk about the quality of the code on its own, and not directly how the reviewee wrote the code. + +Take the time to thank and point out good code changes. + +**Using "please" and “what do you think?”** goes a long way in making others feel like colleagues, and not subordinates. diff --git a/devtools/docs/contributing/coding-standards.md b/devtools/docs/contributing/coding-standards.md new file mode 100644 index 0000000000..553f1f622c --- /dev/null +++ b/devtools/docs/contributing/coding-standards.md @@ -0,0 +1,10 @@ +# Coding standards + +Our code base is quite large, and a lot of different people contribute to it all the time. Therefore, it's important to share standards to keep the code consistent and written in a predictable style. This also helps avoid common mistakes. + +We have pages defining standards, best practices and tips for various languages used in our tools: + +* [JavaScript](./javascript.md) +* [CSS](./css.md) +* [SVG](../frontend/svgs.md) + diff --git a/devtools/docs/contributing/css.md b/devtools/docs/contributing/css.md new file mode 100644 index 0000000000..26ab276d58 --- /dev/null +++ b/devtools/docs/contributing/css.md @@ -0,0 +1,132 @@ +# CSS + +This page is for information about CSS used by DevTools. Wondering about the Dev Edition theme? See this page for more information about the [Developer Edition theme](https://wiki.mozilla.org/DevTools/Developer_Edition_Theme). + +## Basics + +The CSS code is in `devtools/client/themes`. + +Here are some basic tips that can optimize reviews if you are changing CSS: + +* Avoid `!important` but if you have to use it, make sure it's obvious why you're using it (maybe with a comment). +* Avoid magic numbers, prefer automatic sizing. +* Avoid platforms specific styles, put everything in the `shared` directory. +* Avoid preprocessor variables, use CSS variables instead. +* Avoid setting styles in JavaScript. It's generally better to set a class and then specify the styles in CSS +* `classList` is generally better than `className`. There's less chance of over-writing an existing class. + +### Boilerplate + +Make sure each file starts with the standard copyright header (see [License Boilerplate](https://www.mozilla.org/MPL/headers/)). + +### Testing + +CSS changes should generally be similar across platforms since they used a shared implementation, but there can still be differences worth checking out. Check major changes on Windows, OS X and Ubuntu. + +## Formatting + +We use 2-spaces indentation for the CSS. + +In general the formatting looks like this: + +```css +selector, +alternate-selector { + property: value; + other-property: other-value; +} +``` +<!--TODO: add examples for long shorthand properties, and multi-valued properties (background, font-family, ...)--> +Also: + +* Omit units on 0 values. + * Example: Use `margin: 0;`, not `margin: 0px;`. +* Add a space after each comma, **except** within color functions. + * Example: `linear-gradient(to bottom, black 1px, rgba(255,255,255,0.2) 1px)`. +* Always add a space before ` !important`. +* Assume `="true"` in attribute selectors. + * Example: Use `option[checked]`, not `option[checked="true"]`. +* Use longhand versions of properties so it's clear what you're changing. + * Example: Use `border-color: red`, not `border: red;`. + +Naming standards for class names: + +* `lower-case-with-dashes` is the most common. +* But `camelCase` is also used sometimes. Try to follow the style of existing or related code. + +## Light and Dark theme support + +DevTools supports 2 different themes: the dark theme and the light theme. In order to support them, there are 2 class names available (`theme-dark` and `theme-light`). + +* Use [pre-defined CSS variables](https://searchfox.org/mozilla-central/source/devtools/client/themes/variables.css) instead of hardcoding colors when possible. +* If you need to support themes and the pre-defined variables don't fit, define a variable with your custom colors at the beginning of the CSS file. This avoids selector duplication in the code. + +Example: + +```css +.theme-light { + --some-variable-name: <color-for-light-theme>; +} +.theme-dark { + --some-variable-name: <color-for-dark-theme>; +} +#myElement { + background-color: var(--some-variable-name); +} +``` + +## HDPI support + +It's recommended to use SVG since it keeps the CSS clean when supporting multiple resolutions. However, if only 1x and 2x PNG assets are available, you can use this `@media` query to target higher density displays (HDPI): `@media (min-resolution: 1.1dppx)`. <!--TODO an example would be good here--> + +## Performance + +* Read [Writing Efficient CSS](https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Writing_efficient_CSS). +* Use an iframe where possible so your rules are scoped to the smallest possible set of nodes.<!--TODO: is this still true? and also refine exactly when it is appropriate to use an iframe. Examples might help--> +* If your CSS is used in `browser.xhtml`, you need to take special care with performance: + * Descendent selectors should be avoided. + * If possible, find ways to use **only** id selectors, class selectors and selector groups. + +## Localization + +### Text Direction +* For margins, padding and borders, use `inline-start`/`inline-end` rather than `left`/`right`. + * Example: Use `margin-inline-start: 3px;` not `margin-left: 3px`. +* For RTL-aware positioning (left/right), use `inset-inline-start/end`. +* When there is no special RTL-aware property (eg. `float: left|right`) available, use the pseudo `:-moz-locale-dir(ltr|rtl)` (for XUL files) or `:dir(ltr|rtl)` (for HTML files). +* Remember that while a tab content's scrollbar still shows on the right in RTL, an overflow scrollbar will show on the left. +* Write `padding: 0 3px 4px;` instead of `padding: 0 3px 4px 3px;`. This makes it more obvious that the padding is symmetrical (so RTL won't be an issue). + +### RTL support for html modules + +By default, new HTML modules support only left-to-right (LTR) and do not reuse the current direction of the browser. + +To enable right-to-left (RTL) support in a module, set the `[dir]` attribute on the document element of the module: +* Example: `<html xmlns="http://www.w3.org/1999/xhtml" dir="">`. + +The appropriate value for the `dir` attribute will then be set when the toolbox loads this module. + +### Testing + +The recommended workflow to test RTL on DevTools is to use the [Force RTL extension](https://addons.mozilla.org/en-US/firefox/addon/force-rtl/). After changing the direction using Force RTL, you should restart DevTools to make sure all modules apply the new direction. A future version of Force RTL will be able to update dynamically all DevTools documents.<!--TODO: update when the fate of this addon/webextension is known--> + +## Toggles + +Sometimes you have a style that you want to turn on and off. For example a tree twisty (a expand-collapse arrow), a tab background, etc. + +The Mozilla way is to perform the toggle using an attribute rather than a class: + +```css +.tree-node { + background-image: url(right-arrow.svg); +} +.tree-node[open] { + background-image: url(down-arrow.svg); +} +``` + +## Tips + +* Use `:empty` to match a node that doesn't have children. +* Usually, if `margin` or `padding` has 4 values, something is wrong. If the left and right values are asymmetrical, you're supposed to use `-start` and `-end`. If the values are symmetrical, use only 3 values (see localization section). + diff --git a/devtools/docs/contributing/eslint-atom-settings.png b/devtools/docs/contributing/eslint-atom-settings.png Binary files differnew file mode 100644 index 0000000000..1108c206dc --- /dev/null +++ b/devtools/docs/contributing/eslint-atom-settings.png diff --git a/devtools/docs/contributing/eslint-atom.png b/devtools/docs/contributing/eslint-atom.png Binary files differnew file mode 100644 index 0000000000..52350a31ec --- /dev/null +++ b/devtools/docs/contributing/eslint-atom.png diff --git a/devtools/docs/contributing/eslint-sublimetext3.png b/devtools/docs/contributing/eslint-sublimetext3.png Binary files differnew file mode 100644 index 0000000000..8a76f06cda --- /dev/null +++ b/devtools/docs/contributing/eslint-sublimetext3.png diff --git a/devtools/docs/contributing/eslint-vscode.png b/devtools/docs/contributing/eslint-vscode.png Binary files differnew file mode 100644 index 0000000000..41a58fe9d4 --- /dev/null +++ b/devtools/docs/contributing/eslint-vscode.png diff --git a/devtools/docs/contributing/eslint.md b/devtools/docs/contributing/eslint.md new file mode 100644 index 0000000000..73f50c75de --- /dev/null +++ b/devtools/docs/contributing/eslint.md @@ -0,0 +1,158 @@ +# Using ESLint in DevTools +<!--TODO paths, executables and everything here should be reviewed when we go to GitHub--> + +The main rule set is in `devtools/.eslintrc`. It is meant to be used with ESLint 3.5.0. + +Note that the file `.eslintignore` at the root of the repository contains a list of paths to ignore. This is because a lot of the code isn't ESLint compliant yet. We're in the process of making code free of ESLint warnings and errors, but this takes time. In the meantime, make sure the file or folder you are running ESLint on isn't ignored. + +## Installing + +From the root of the project type: + +`./mach eslint --setup` + +ESLint, `eslint-plugin-html`, `eslint-plugin-mozilla` and `eslint-plugin-react` will be automatically downloaded and installed. + +## Running ESLint + +### From the command line + +The preferred way of running ESLint from the command line is by using `mach` like this: + +```bash +./mach eslint path/to/directory +``` + +This ensures that ESLint runs with the same configuration that our CI environment (see the next section). + +### In continuous integration + +Relying only on people to run ESLint isn't enough to guarantee new warnings or errors aren't introduced in the code. Therefore, ESLint also runs automatically in our Continuous Integration environment. + +This means that every time a commit is pushed to one of the repositories, a job runs ESLint on the whole code. + +If you are pushing a patch to the [`try` repository](https://wiki.mozilla.org/ReleaseEngineering/TryServer) to run the tests, then you can also tell it to run the ESLint job and therefore verify that you did not introduce new errors. + +If you build on all platforms, then the ESLint job will run by default, but if you selected a few platforms only in your [trysyntax](https://wiki.mozilla.org/Build:TryChooser), then you need to also add `eslint-gecko` as a target platform for ESLint to run. + +### Running ESLint in SublimeText + +SublimeText is a popular code editor and it supports ESLint via a couple of plugins. Here are some pointers to get started: + +* make sure you have [SublimeText 3](http://www.sublimetext.com/3), the linter plugin doesn't work with ST2, +* install [SublimeLinter 3](http://www.sublimelinter.com/en/latest/installation.html), this is a framework for linters that supports, among others, ESLint. Installing SublimeLinter via [Package Control](https://packagecontrol.io/installation) is the easiest way) +* with SublimeLinter installed, you can now [install the specific ESLint plugin](https://github.com/roadhump/SublimeLinter-eslint#linter-installation). The installation instructions provide details about how to install node, npm, eslint which are required). +* make sure to configure SublimeLinter with the `--no-ignore` option so that errors are also shown for source files that are ignored. To do this, open the SublimeLinter user configuration at: Preferences / Package Settings / SublimeLinter / Settings - User, and add `"args": "--no-ignore"` to the eslint linter object. + +You will also need to point SublimeLinter at the local eslint installation by setting the path to whatever `./mach eslint --setup` gives you when you run it (include a trailing slash but remove the eslint binary filename) e.g. + +NOTE: Your local eslint binary is at /some-project-path/tools/lint/eslint/node_modules/.bin/eslint + +``` + "paths": { + "linux": [], + "osx": [ + "/some-project-path/tools/lint/eslint/node_modules/.bin" + ], + "windows": [ + "C:\\some-project-path\\tools\\lint\\eslint\\node_modules\\.bin" + ] + }, +``` + +Once done, open the mozilla project in SublimeText and open any JS file in the `/devtools` directory. You can then trigger the linter via the contextual menu (right click on the file itself) or with a keyboard shortcut (ctrl+option+L on Mac). + +You should see errors and warnings in the gutter as shown in the screenshot below. You can also see all errors listed with ctrl+option+A, and individual errors by clicking in the gutter marker. + + + +### Running ESLint in Emacs + +* First, install the flycheck package (flymake doesn't support ESLint yet). You can get flycheck from the [marmalade](https://marmalade-repo.org/) or [melpa-stable](http://stable.melpa.org/#/) repositories. + +* Tell flycheck to disable jslint, and enable flycheck in your javascript mode. Some care is needed to find the eslint installed in the source tree. This snippet assumes the built-in javascript mode, but with minor changes (just the name of the hook) should work fine with js2-mode as well: +```lisp +(defun my-js-mode-hacks () + (setq-local mode-name "JS") + ;; Set this locally so that the head.js rule continues to work + ;; properly. In particular for a mochitest we want to preserve the + ;; "browser_" prefix. + (when (buffer-file-name) + (let ((base (file-name-nondirectory (buffer-file-name)))) + (when (string-match "^\\([a-z]+_\\)" base) + (setq-local flycheck-temp-prefix (match-string 1 base)))) + (let ((base-dir (locate-dominating-file (buffer-file-name) + ".eslintignore"))) + (when base-dir + (let ((eslint (expand-file-name + "tools/lint/eslint/node_modules/.bin/eslint" base-dir))) + (when (file-exists-p eslint) + (setq-local flycheck-javascript-eslint-executable eslint)))))) + (flycheck-mode 1)) +(require 'flycheck) +(setq-default flycheck-disabled-checkers + (append flycheck-disabled-checkers + '(javascript-jshint))) +(add-hook 'js-mode-hook #'my-js-mode-hacks) +``` + +* flycheck puts its bindings on `C-c !` by default, so use `C-c ! C-h` to see what is available. There are key bindings to list all the errors and to move through the errors, among other things. +* To make sure flycheck is finding eslint, open a .js file and run `M-x flycheck-verify-setup`. It should show the path to your eslint installation. + +### Running ESLint in Atom + +From the root of the project type: + +`./mach eslint --setup` + +Install the [linter-eslint](https://atom.io/packages/linter-eslint) package v.8.00 or above. Then go to the package settings and enable the following options: + + + +Once done, you should see errors and warnings as shown in the screenshot below. + + + +### Running ESLint in ViM + +If you don't use Syntastic yet, the instructions here should get you started: https://wiki.mozilla.org/WebExtensions/Hacking#Vim + +Alternatively, if you do use Syntastic already, add this to your `.vimrc` to get ESLint working where the path contains `mozilla-central` (adjust the path to reflect the one in your computer): + +```vim + autocmd FileType javascript,html + \ if stridx(expand("%:p"), "/mozilla-central/") != -1 | + \ let b:syntastic_checkers = ['eslint'] | + \ let b:syntastic_eslint_exec = '/path/to/mozilla-central/tools/lint/eslint/node_modules/.bin/eslint' | + \ let b:syntastic_html_eslint_args = ['--plugin', 'html'] | + \ endif +``` + +You probably need to close and reopen ViM for the changes to take effect. Then, open any file and try to edit it to cause an error, then save it. If all goes well, you will get some distinctive arrows pointing to the error. Hovering with the mouse will produce a sort of tooltip with more information about the error. + +### Running ESLint in Visual Studio Code + +From the root of the project type: + +`./mach eslint --setup` + +Install the [dbaeumer.vscode-eslint](https://marketplace.visualstudio.com/items?itemName=dbaeumer.vscode-eslint) package. Then go to the package settings and set the following option: + +`"eslint.nodePath": "tools/lint/eslint/node_modules/.bin"` + +Once done, you should see errors and warnings as shown in the screenshot below: + + + +### Fixing ESLint Errors + +This should help you write eslint-clean code: + +* When moving or refactoring a piece of code, consider this as an opportunity to remove all ESlint errors from this piece of code. In fact, it may even be a good opportunity to remove all ESLint errors from the entire file. +* When doing ESLint-only changes, please do them in a separate patch from the actual functionality changes or bug fix. This helps make the review easier, and isolate the actual changes when looking at the source history. +* When cleaning an entire file or folder from ESLint errors, do not forget to remove the corresponding entry from the `.eslintignore` file. +* When writing new code, from scratch, please make it ESLint compliant from the start. This is a lot easier than having to revisit it later. +* ESLint also runs on `<script>` tags in HTML files, so if you create new HTML test files for mochitests for example, make sure that JavaScript code in those files is free of ESLint errors. +* Depending on how a dependency is loaded into a file, the symbols this dependency exports might not be considered as defined by ESLint. For instance, using `Cu.import("some.jsm")` doesn't explicitly say which symbols are now available in the scope of the file, and so using those symbols will be consider by ESLint as using undefined variables. When this happens, please avoid using the `/* globals ... */` ESLint comment (which tells it that these variables are defined). Instead, please use `/* import-globals-from relative/path/to/file.js */`. This way, you won't have a list of variables to maintain manually, the globals are going to be imported dynamically instead. +* In test files (xpcshell and mochitest), all globals from the corresponding `head.js` file are imported automatically, so you don't need to define them using a `/* globals ... */` comment or a `/* import-globals-from head.js */` comment. + diff --git a/devtools/docs/contributing/filing-good-bugs.md b/devtools/docs/contributing/filing-good-bugs.md new file mode 100644 index 0000000000..c57979dff7 --- /dev/null +++ b/devtools/docs/contributing/filing-good-bugs.md @@ -0,0 +1,11 @@ +# Filing good bugs + +Getting started working on a bug can be hard, specially if you lack context. + +This guide is meant to provide a list of steps to provide the necessary information to resolve a bug. + +* Use a descriptive title. Avoid jargon and abbreviations where possible, they make it hard for other people to find existing bugs, and to understand them. +* Explain the problem in depth and provide the steps to reproduce. Be as specific as possible, and include things such as operating system and version if reporting a bug. +* If you can, list files and lines of code that may need to be modified. Ideally provide a patch for getting started. +* If applicable, provide a test case or document that can be used to test the bug is solved. For example, if the bug title was "HTML inspector fails when inspecting a page with one million of nodes", you would provide an HTML document with one million of nodes, and we could use it to test the implementation, and make sure you're looking at the same thing we're looking at. You could use services like jsfiddle, codepen or jsbin to share your test cases. Other people use GitHub, or their own web server. +* If it's a bug that new contributors can work on, add the keyword `good-first-bug`. diff --git a/devtools/docs/contributing/find-bugs.md b/devtools/docs/contributing/find-bugs.md new file mode 100644 index 0000000000..b756904844 --- /dev/null +++ b/devtools/docs/contributing/find-bugs.md @@ -0,0 +1,6 @@ +# Find bugs to work on + +* Choose something from [the list of existing bugs](https://bugs.firefox-dev.tools/?easy&tool=all). You can filter by tools (e.g. only `Console` bugs), and also by good bugs for beginners. +* Or if you would like to work on something that is not listed there, [file a bug in Bugzilla](https://bugzilla.mozilla.org/enter_bug.cgi?product=DevTools) (you'll need the Bugzilla account [you created earlier](../getting-started/bugzilla.md)) and ask for it to be assigned to you. Please also try to initiate a conversation in the bug first, to ensure that you don't work on something that will not be accepted (for example, if you think you found a bug, but the feature worked that way by design). + +<!-- TODO: mention finding potential work that is captured as a TODO or FIXME comments, but doesn't have an associated filed bug --> diff --git a/devtools/docs/contributing/fixing-bugs.md b/devtools/docs/contributing/fixing-bugs.md new file mode 100644 index 0000000000..d2df3e377d --- /dev/null +++ b/devtools/docs/contributing/fixing-bugs.md @@ -0,0 +1,170 @@ +# How to fix a bug + +## Make sure you understand what needs to be done + +If you're not quite sure of this, please add a comment requesting more information in the bug itself. It is absolutely fine to also talk to someone via other means (e.g. irc, email, in the office kitchen...), but it is good to come back to the bug and add the extra information, just in case you stop working on the bug at some point and someone else has to pick work where you left it. + +## Find out where are the files that need changing + +In an ideal world, the bug has this information from the start, but very often it doesn't. + +If you're not yet familiar with the codebase, the [files and directories overview](../files/README.md) might help you find where things are. + +If you know what you're looking for (e.g. a string that has a typo and needs correcting, or a function name that needs modifying), you can use a source code search engine: + +* [Searchfox](http://searchfox.org/mozilla-central/source) + +It is a good idea to [add smart keyword searches](https://support.mozilla.org/en-US/kb/how-search-from-address-bar) for DXR to search faster. + +You can also use your operating system's command line. For example, let's search for occurrences of `TODO` in the code base. + +Within your command line prompt, `cd` to the `devtools` directory: + +```bash +cd ~/mozilla-central/devtools # use your actual folder name! +grep -r 'TODO' . +``` + +This will list all the places in the DevTools code that contain the `TODO` string. If there are too many instances, you can combine the above with `less`, to scroll and paginate the output of `grep`: + +```bash +grep -r 'TODO' . | less +``` + +Press `q` to exit. + +If after all of this you still can't find your bearings, add a comment in the bug asking for more information. + +## How do you know that you have found what you were looking for? + +There are a few options to confirm that you found the right files: + +### If this is about changing a string... + +Edit the file(s), and change the string (e.g. fix a typo). Rebuild, and run again: + +```bash +./mach build +./mach run +``` +Then go to the panel that displays the string you wanted to change. + +Does the typo still occur? Or is the string being displayed the correct one now? + +### If this is about changing JavaScript code... + +If you think you found the right file to edit, add some logging statement on a place of the code which is likely to be executed (for example, on a class constructor): + +```javascript +// For front-end code +console.log('hello friends\n'); + +// Sometimes it's hard to spot your output. Emojis can help here really well. +console.log('👗👗👗', 'This is your logged output!'); +``` + +Or... + +```javascript +// For server code +dump('hello friends\n'); +``` + +TIP: Whether to use one or another depends on the type of bug you're working on, but if you've just started in DevTools, it's highly likely you'll take a front-end bug first. + +Then rebuild and run again: + +```bash +./mach build +./mach run +``` + +Go to the panel or initiate the action that is likely to result on the code being executed, and pay close attention to the output in your console. + +Can you see `hello friends`? Then you found the file that you were looking for. + +It's possible that you'll get a lot of other messages you don't care about now, but we can use `grep` to filter: + +```bash +./mach run | grep hello friends +``` + +This will only show lines that contain `hello friends`. If you get an empty output after trying to get the code to execute, maybe this isn't the right file, or maybe you didn't trigger the action. + +And that means it's time to ask for more information in the bug or from your colleagues. Tell them what you tried, so they don't have to figure that out themselves (saves everyone some time!). + +### If this is about changing CSS code... + +If you think you have found the right file and the right CSS selector, you could try to edit the file to insert some outrageously colourful rule (for example, a really thick bright blue border), then restart DevTools to see if the changes show up. + +```css +border: 4px solid blue; +``` + +## NEXT: do whatever needs doing + +This will always depend on the specific bug you're working on, so it's hard to provide guidance here. + +The key aspect here is that if you have questions, you should not hesitate to ask. Ask your mentor, your manager, or [get in touch](https://firefox-dev.tools/#getting-in-touch). **You should just not get stuck**. + +Some people find it difficult to recognise or even admit they're in such a situation, so some ways to describe 'stuck' could be: + +* you've tried everything you could think of, nothing works, and do not know what else to do. +* you have various ideas for things that can be done, and do not know which one to go for. +* you have not learned anything new about the problem in the last one or two days. +* you're starting to think about abandoning this bug and doing something else instead. +* you don't know what to do, but are afraid of asking for help. + +If you think *any* of the above describes you, ask for help! + +Another tip you can use if you're afraid that you're wasting people's time is to timebox. For example, give yourself 2 hours to investigate. If you can't figure anything after that time has elapsed, stop and ask for help. It might be that you needed a key piece of information that was missing in the bug description, or you misunderstood something, or maybe even that you found a bug and that's why things didn't work even if they should! This is why it's important to call for help sooner rather than later. + +### Useful references + +#### Coding standards + +If it's your first time contributing, the documentation on [coding standards](./coding-standards.md) might have answers to questions such as what style of code to use, how to name new files (if you have to add any), tools we use to run automated checks, etc. + +#### Specialised guides + +We also have a few guides explaining how to work on specific types of problems, for example: [investigating performance issues](./performance.md), or [writing efficient React code](./react-performance-tips.md). Please have a look at the sidebar or use the search box on top of the sidebar to see if there's something written about the type of bug you're working on. + +If not, maybe you'll be able to contribute with one by the time you fix your bug! + +#### MDN + +[MDN Web Docs](http://developer.mozilla.org/) (also known as *MDN*) has a lot of information about HTML, CSS, JS, DOM, Web APIs, Gecko-specific APIs, and more. + +## Run tests + +We have several types of automated tests to help us when developing. + +Some, like the linting tests, address coding style; others address functionality, such as unit and integration tests. This page has more [details on types of tests and how to run them](../tests/). + +You might want to run the unit and integration types of tests quite frequently, to confirm you're not breaking anything else. Depending on what you're doing, it might be even possible to run just one test file which addresses the specific change you're implementing: + +```bash +./mach test devtools/path/to/test.js +``` + +Sometimes you might want to run a number of tests which are related to the bug you're fixing: + +```bash +./mach test devtools/path/to/test-thing-*.js +``` + +At the beginning, it is entirely possible that you have no idea of where the tests are for the thing you're working on. Please ask for help! You will eventually learn your way around. + +It is good etiquette to ensure the tests pass locally before asking for a code review. This includes linting tests. To run them, please [configure your system to run ESlint](./eslint.md), and then you can execute: + +```bash +./mach eslint devtools/path/to/files/you/changed +``` + +Our tool for code review will run the linter automatically as well, but if you run this locally you'll get instant feedback, and avoid having to send an updated commit again. + +## Time for a review + +When you think you have fixed the bug, first let's celebrate a bit! Yay! Well done 😀 + +And now it's time to [get your code reviewed](./code-reviews.md). diff --git a/devtools/docs/contributing/javascript.md b/devtools/docs/contributing/javascript.md new file mode 100644 index 0000000000..f912a1a9da --- /dev/null +++ b/devtools/docs/contributing/javascript.md @@ -0,0 +1,89 @@ +# JavaScript coding standards + +Probably the best piece of advice is **to be consistent with the rest of the code in the file**. + +We use [ESLint](http://eslint.org/) to analyse JavaScript files automatically, either from within a code editor or from the command line. Here's [our guide to install and configure it](./eslint.md). + +For quick reference, here are some of the main code style rules: + +* file references to browser globals such as `window` and `document` need `/* eslint-env browser */` at the top of the file, +* lines should be 90 characters maximum, +* indent with 2 spaces (no tabs!), +* `camelCasePlease`, +* don't open braces on the next line, +* don't name function expressions: `let o = { doSomething: function doSomething() {} };`, +* use a space before opening paren for anonymous functions, but don't use one for named functions: + * anonymous functions: `function () {}` + * named functions: `function foo() {}` + * anonymous generators: `function* () {}` + * named generators: `function* foo() {}` +* aim for short functions, 24 lines max (ESLint has a rule that checks for function complexity too), +* `aArguments aAre the aDevil` (don't use them please), +* `"use strict";` globally per module, +* `semicolons; // use them`, +* no comma-first, +* consider using async/await for nice-looking asynchronous code instead of formatting endless `.then` chains, +* use ES6 syntax: + * `function setBreakpoint({url, line, column}) { ... }`, + * `(...args) => { }` rest args are awesome, no need for `arguments`, + * `for..of` loops, +* don't use non-standard SpiderMonkey-only syntax: + * no `for each` loops, + * no `function () implicitReturnVal`, + * getters / setters require { }, +* only import specific, explicitly-declared symbols into your namespace: + * `const { foo, bar } = require("foo/bar");`, + * `const { foo, bar } = ChromeUtils.import("...");`, +* use Maps, Sets, WeakMaps when possible, +* use [template strings](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals) whenever possible to avoid concatenation, allow multi-line strings, and interpolation. + + +## Comments + +Commenting code is important, but bad comments can hurt too, so it's important to have a few rules in mind when commenting: + +* If the code can be rewritten to be made more readable, then that should be preferred over writing an explanation as a comment. +* Instead of writing a comment to explain the meaning of a poorly chosen variable name, then rename that variable. +* Avoid long separator comments like `// ****************** another section below **********`. They are often a sign that you should split a file in multiple files. +* Line comments go above the code they are commenting, not at the end of the line. +* Sentences in comments start with a capital letter and end with a period. +* Watch out for typos. +* Obsolete copy/pasted code hurts, make sure you update comments inside copy/pasted code blocks. +* A global comment at the very top of a file explaining what the file is about and the major types/classes/functions it contains is a good idea for quickly browsing code. +* If you are forced to employ some kind of hack in your code, and there's no way around it, then add a comment that explains the hack and why it is needed. The reviewer is going to ask for one anyway. +* Bullet points in comments should use stars aligned with the first comment to format each point +```javascript +// headline comment +// * bullet point 1 +// * bullet point 2 +``` + +## Asynchronous code + +A lot of code in DevTools is asynchronous, because a lot of it relies on connecting to the DevTools server and getting information from there in an asynchronous fashion. + +It's easy to make mistakes with asynchronous code, so here are a few guidelines that should help: + +* Prefer [promises](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) over callbacks. +* Use the `new Promise(() => {})` syntax. +* Don't forget to catch rejections by defining a rejection handler: `promise.then(() => console.log("resolved"), () => console.log("rejected"));` or `promise.catch(() => console.log("rejected"));`. +* Make use of [`Tasks` and generator functions](https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm) to make asynchronous code look synchronous. <!--TODO async/await as in the TODO above?--> + +## React & Redux + +There are React-specific code style rules in the .eslintrc file. + +### Components + +* Default to creating components as [stateless function components](https://facebook.github.io/react/docs/reusable-components.html#stateless-functions). +* If you need local state or lifecycle methods, use `React.createClass` instead of functions. +* Use React.DOM to create native elements. Assign it to a variable named `dom`, and use it like `dom.div({}, dom.span({}))`. You may also destructure specific elements directly: `const { div, ul } = React.DOM`. + +### PropTypes + +* Use [PropTypes](https://facebook.github.io/react/docs/reusable-components.html#prop-validation) to define the expected properties of your component. Each directly accessed property (or child of a property) should have a corresponding PropType. +* Use `isRequired` for any required properties. +* Place the propTypes definition at the top of the component. If using a stateless function component, place it above the declaration of the function. +* Where the children property is used, consider [validating the children](http://www.mattzabriskie.com/blog/react-validating-children). + + diff --git a/devtools/docs/contributing/landing-code.md b/devtools/docs/contributing/landing-code.md new file mode 100644 index 0000000000..4f9f1d0fa8 --- /dev/null +++ b/devtools/docs/contributing/landing-code.md @@ -0,0 +1,20 @@ +# Landing code (i.e. getting code into Mozilla's repository) + +Code changes (patches) in Mozilla are not 'merged' in a sequential way, as it's the fashion in other popular projects. Here, the patches will be *applied* on top of the latest code, and will stay there if + +1. the patch applies cleanly, without conflicts +2. the patch doesn't cause 'bustage' (i.e. breaks the build) + +Therefore, it's good to try and do smaller changes rather than bigger, specially if you're modifying files that many other people are working on simultaneously, to avoid conflicts and your patch being rejected. Otherwise you might need to rebase from the latest changes, try to write your changes on top of it, and submit this new diff. + +Leaving potential conflicts aside, a patch can make its way into the repository in two ways: + +## From Phabricator + +Once a review has been approved, someone with enough privileges can request the code be merged, using the [Lando](https://moz-conduit.readthedocs.io/en/latest/lando-user.html) interface. These 'privileges' are "commit level access 3". You get these once you have successfully contributed with a number of patches. See [levelling up](./levelling-up.md) for more details. + +If you don't have the privileges, you can also ask your mentor to land the code. In fact, they might even initiate that for you once the code review is approved. + +To request the landing, add the *Check-in Needed* Tags to the review (*Edit Revision*). + +Someone looks at review marked with `Check-in Needed` a few times a day, verifies they have an associated **approved** review, and then attempts to land them. diff --git a/devtools/docs/contributing/levelling-up.md b/devtools/docs/contributing/levelling-up.md new file mode 100644 index 0000000000..fc0c8bab01 --- /dev/null +++ b/devtools/docs/contributing/levelling-up.md @@ -0,0 +1,23 @@ +# Levelling up + +Once you've fixed a few bugs, it's quite likely that we'll ask you to complete one or more of the following steps. They will grant you access to useful Mozilla infrastructure for testing or landing code automatically, so you can be more autonomous and able to contribute with less supervision. + +If you're curious, the [Mozilla Commit Access Policy](https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/) page explains what does each level mean. + +## Request commit access level 1 + +Once this is granted, you can use [the `Try` server](https://wiki.mozilla.org/ReleaseEngineering/TryServer) for running automated tests of your code. + +To request this, [follow the instructions here](https://www.mozilla.org/en-US/about/governance/policies/commit/) to file a bug with the title "Commit Access (Level 1) for ${your name}", and continue with steps such as adding the public SSH key, etc (it's not enough with just filing the bug!). + +The person that asked you to request this can also vouch for you. + +## Request commit access level 3 + +After you landed a few bugs, that are more advanced than the `good-first-bugs` and you feel confident about your contributions as well as the process, you can request level 3 access. Once this is granted, you will get access to Lando, the autoland feature. + +To request this, [follow the instructions here](https://www.mozilla.org/en-US/about/governance/policies/commit/) to file a bug with the title "Commit Access (Level 3) for ${your name}". + +The person that asked you to request this can also vouch for you, and at this point you might already know a few other people who will vouch for you. You need two vouchers. + +If you reach this level, well done! That's amazing, and we're glad to have you on board 😄 diff --git a/devtools/docs/contributing/making-prs.md b/devtools/docs/contributing/making-prs.md new file mode 100644 index 0000000000..fdaf827d15 --- /dev/null +++ b/devtools/docs/contributing/making-prs.md @@ -0,0 +1,82 @@ +# Sending your code for review (also known as "sending patches") + +First, commit your changes. For example: + +```bash +hg add /path/to/file/changed +hg commit -m "Bug 1234567 - [devtools] Implement feature XYZ. r=name,name2!" +``` + + The commit message explained in detail: + - `Bug 1234567` - The number of the bug in bugzilla. + - `- [devtools] Implement feature XYZ.` - The commit message, with a "devtools" prefix to quickly identify DevTools changesets. + - `r=name` - The short form to request a review. Enter the name you found using the + instructions in the [previous step](./code-reviews-find-reviewer.md). + - `,name2!` - You can have more than one reviewer. The `!` makes the review a *blocking* review (Patch can not land without accepted review). + +Then create a revision in Phabricator using `moz-phab`: + +```bash +moz-phab submit +``` + +A revision will be created including that information and the difference in code between your changes and the point in the repository where you based your work (this difference is sometimes called "a patch", as it's what you'd need to apply on the repository to get to the final state). + +If you click on the provided URL for the revision, it'll bring you to Phabricator's interface, which the reviewer will visit as well in order to review the code. They will look at your changes and decide if they need any additional work, based on whether the changes do fix what the bug describes or not. To get a better idea of the types of things they will look at and verify, read the [code reviews checklist](./code-reviews-checklist.md). + +For more information on using moz-phab, you can run: + +```bash +moz-phab -h +``` + +or to get information on a specific command (here `submit`): + +```bash +moz-phab submit -h +``` + +The reviewer might suggest you do additional changes. For example, they might recommend using a helper that already exists (but you were not aware of), or they might recommend renaming things to make things clearer. Or they might recommend you do *less* things (e.g. if you changed other things that are out of scope for the bug). Or they might simply ask questions if things aren't clear. You can also ask questions if the comments are unclear or if you're unsure about parts of the code you're interacting with. Something that looks very obvious to one person might confuse others. + +Hence, you might need to go back to the code and do some edits to address the issues and recommendations. Once you have done this, you must update the existing commit: + +```bash +hg commit --amend +``` + +And submit the change again: + +```bash +moz-phab submit +``` + +You might have to go through this cycle of submitting changes and getting it reviewed several times, depending on the complexity of the bug. + +Once your code fixes the bug, and there are no more blocking issues, the reviewer will approve the changes, and the code can be landed in the repository now. + + +# Squashing commits + +Sometimes you may be asked to squash your commits. Squashing means merging multiple commits into one in case you created multiple commits while working on a bug. Squashing bugs is easy! + +We will use the histedit extension for squashing commits in Mercurial. You can check if this extension is enabled in your Mercurial installation following these steps: + +* Open `.hgrc` (Linux/OSX) or `Mercurial.ini` (Windows) –this is the default configuration file of Mercurial– located in your home directory, using your favourite editor. +* Then add `histedit= ` under the `[extensions]` list present in file, if not present already. + +Then, run the following command: + +`hg histedit` + +You will see something like this on your terminal: + +``` +pick 3bd22d1cc59a 0 "First-Commit-Message" +pick 81c4d40e57d3 1 "Second-Commit-Message" +``` + +These lines represent your commits. Suppose we want to merge `81c4d40e57d3` to `3bd22d1cc59a`. Then replace **pick** in front of `81c4d40e57d3` with **fold** (or simply 'f'). Save the changes. + +You will see that `81c4d40e57d3` has been combined with `3bd22d1cc59a`. You can verify this using the `hg log` command. + +You can fold as many commits you want, and they will be combined with the first commit above them which does not use fold. diff --git a/devtools/docs/contributing/performance.md b/devtools/docs/contributing/performance.md new file mode 100644 index 0000000000..1b0f374260 --- /dev/null +++ b/devtools/docs/contributing/performance.md @@ -0,0 +1,152 @@ +# Writing efficient code + +When debugging a page, tools get to slow down the website because of the added instrumentation. +While working on Developer Tools we should strive to be the less impactful. +First, because it is painful to work with laggy UI, but also because some tools record timings. +For example, the network monitor records HTTP request timings. +If the tools are slowing down Firefox significantly, it will make these measurements be wrong. + +To be efficient while working on performance, you should always focus on one precise user scenario. +It could be: +* a bug report where someone reports a precise interaction being slow, +* or you could be trying to improve overall tools performance by looking at the most common usages. +The important point here is to have some steps to reproduce, that you can redo manually in order to record a profile. +And also, it is even better if you can replay via a test script. Test script that you can save as a new performance test. + +## Don't guess — profile. + +The very first thing to do is to record a profile while reproducing the scenario. + +Here's the Firefox documentation for [how to install the profiler and record a profile](https://developer.mozilla.org/docs/Mozilla/Performance/Reporting_a_Performance_Problem) and also [how to interpret the profiles](https://developer.mozilla.org/docs/Mozilla/Performance/Profiling_with_the_Built-in_Profiler#Understanding_Profiles) + +There are some peculiarities about DevTools architecture that are worth knowing about when looking at a profile: + +### Tweak profiler default settings + +The default buffer size (9MB) is too small. If you don't increase it, you may easily miss data and only see last couple of seconds of your recording. +To increase the buffer size, click on the profiler add-on icon, in the Firefox toolbar, and set it to 360MB, like this: + + <img src="performance/profiler-buffer-size.png" alt="Profiler buffer size" style="width: 300px" /> + +The other setting worth mentioning for DevTools debugging is the `Interval` +The profiler records only samples, based on this `Interval`. +If you want to see more fine-grained stack traces, you may reduce this interval to 0.1ms, +but do that only if you really need it, as it will make Firefox much even slower when recording, +and the measured times will be even slower. + +### The DevTools UI runs on the parent process + +When you are debugging tool front-ends (e.g. panels), always ensure you select the `Main Thread` line. +It should have a light blue background like this: + + <img src="performance/profiler-main-thread.png" alt="Select main process" style="width: 300px" /> + +Otherwise, the vast majority of DevTools backend (DevToolsServer, actors, ...) lives in content processes. +So if you are debugging them, you should select one of the `Content` lines. + +### Most of the DevTools codebase is in Javascript + +In the call tree, it is easier to filter by `JS`, via this menu list: + <img src="performance/profiler-filter-js.png" alt="JS Filtering" style="width: 200px" /> + +But note that you may have to switch back to `Combined` in order to understand why some particular Javascript method is slow. + +### Handy filter strings for DevTools: + + * `require` + Helps highlighting the cost of module loading +  + * DevTools uses two kind of URLs: + * `chrome://devtools/` for all panel documents. Filter with this to see the cost of all panel documents: +  + * `resource://devtools/` for all javascript modules. Filter with this to see the cost of all modules: +  + +### Record durations manually + +Sometimes it is handy to focus on a very precise piece of code and record its time manually. +For example when you identified one slow running method and think you can speed it up. +It saves your from having to: record the profile, wait for the profiler to display and search for the precise method durations. + +#### Print durations in your Terminal and in the Browser Console + +You can use the [`Performance`](https://developer.mozilla.org/docs/Web/API/Performance) API, like this: +``` +let start = window.performance.now(); + +// Run the code you want to measure + +// Once it is done, do: +console.log("my function took", window.performance.now() - start, "ms"); +``` + +#### Use markers + +The Performance API also allows recording markers, like this: +``` +window.performance.mark("my-function-start"); + +// Run the code you want to measure + +// Once it is done, do: +window.performance.measure("my-function", "my-function-start"); +``` + +This marker will appear in the `Marker Chart` section in [profiler.firefox.com](https://profiler.firefox.com), in the `UserTiming` lines: +  + +You can double click on it to make [profiler.firefox.com](https://profiler.firefox.com) display the record during this precise moment in time, +and the call tree will only display what was executed during this measurement. + +### Prototype quickly + +Sometimes the best way to find what is slow is to comment blocks of code out +and uncomment them one by one until you identify the culprit. And then focus on it. + +There are few things worse than spending a long time refactoring the piece of code that was not slow to begin with! + +## Assess your improvement. + +Once you have a patch that you think improves the performance, you have to assess whether it actually improves it. + +### Record another profile + +Compare the two profiles, without and with your patch. +Then see if the call tree reports a significant difference: +* A function call completely disappears in the new profile, with your fix. + For example you were loading a big module, and you got a frame for `require("my/big/module")` call, and no longer see it. +* The same function call takes xxx ms less with your patch. + +This [lazy loading of modules in netmonitor](https://bugzilla.mozilla.org/show_bug.cgi?id=1420289) is a good example. +Without this patch, App.js was loading in 91ms and was loading MonitorPanel.js and StatisticsPanel.js as dependencies: +  + +With the patch, App.js loads in 47ms and only loads MonitorPanel.js: +  + +It highlights that: + * we no longer load StatisticsPanel, + * App is faster to load. + +### Run performance tests + +See if any subtest reports a improvement. Ensure that the improvement makes any sense. +For example, if the test is 50% faster, maybe you broke the performance test. +This might happen if the test no longer waits for all the operations to finish executing before completing. + +To push your current patch to try, execute: +```bash +./mach try fuzzy --query "'linux64-shippable/ 'damp" --rebuild 5 +``` +It will print in your Terminal a link to perfherder like this one: +[https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=9bef6cb13c43bbce21d40ffaea595e082a4c28db](https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=9bef6cb13c43bbce21d40ffaea595e082a4c28db) +Running performance tests takes time, so you should open it 30 minutes up to 2 hours later to see your results. +See [Performance tests (DAMP)](../tests/performance-tests.md) for more information about PerfHerder/try. + +Let's look at how to interpret an actual real-life [set of perfherder results](https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=9bef6cb13c43bbce21d40ffaea595e082a4c28db&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1&selectedTimeRange=172800): + + + +These results are related to [lazy loading of modules in netmonitor](https://bugzilla.mozilla.org/show_bug.cgi?id=1420289). +It is interesting to see that this patch is a trade-off. It makes netmonitor opening significantly faster, by preventing loading many modules during its opening. +But it makes the page reload a bit slower as some modules that used to be loaded during netmonitor open, now have to be loaded during page reload. diff --git a/devtools/docs/contributing/performance/perfherder-results.png b/devtools/docs/contributing/performance/perfherder-results.png Binary files differnew file mode 100644 index 0000000000..8f35a47c8c --- /dev/null +++ b/devtools/docs/contributing/performance/perfherder-results.png diff --git a/devtools/docs/contributing/performance/profiler-buffer-size.png b/devtools/docs/contributing/performance/profiler-buffer-size.png Binary files differnew file mode 100644 index 0000000000..626fcc5c3f --- /dev/null +++ b/devtools/docs/contributing/performance/profiler-buffer-size.png diff --git a/devtools/docs/contributing/performance/profiler-chrome-url.png b/devtools/docs/contributing/performance/profiler-chrome-url.png Binary files differnew file mode 100644 index 0000000000..fa6209f2fd --- /dev/null +++ b/devtools/docs/contributing/performance/profiler-chrome-url.png diff --git a/devtools/docs/contributing/performance/profiler-custom-markers.png b/devtools/docs/contributing/performance/profiler-custom-markers.png Binary files differnew file mode 100644 index 0000000000..be40060a8d --- /dev/null +++ b/devtools/docs/contributing/performance/profiler-custom-markers.png diff --git a/devtools/docs/contributing/performance/profiler-filter-js.png b/devtools/docs/contributing/performance/profiler-filter-js.png Binary files differnew file mode 100644 index 0000000000..e59242aea8 --- /dev/null +++ b/devtools/docs/contributing/performance/profiler-filter-js.png diff --git a/devtools/docs/contributing/performance/profiler-filter-require.png b/devtools/docs/contributing/performance/profiler-filter-require.png Binary files differnew file mode 100644 index 0000000000..44c89b0907 --- /dev/null +++ b/devtools/docs/contributing/performance/profiler-filter-require.png diff --git a/devtools/docs/contributing/performance/profiler-main-thread.png b/devtools/docs/contributing/performance/profiler-main-thread.png Binary files differnew file mode 100644 index 0000000000..f7e8f5541f --- /dev/null +++ b/devtools/docs/contributing/performance/profiler-main-thread.png diff --git a/devtools/docs/contributing/performance/profiler-netmon-open-fixed.png b/devtools/docs/contributing/performance/profiler-netmon-open-fixed.png Binary files differnew file mode 100644 index 0000000000..007a923713 --- /dev/null +++ b/devtools/docs/contributing/performance/profiler-netmon-open-fixed.png diff --git a/devtools/docs/contributing/performance/profiler-netmonitor-open.png b/devtools/docs/contributing/performance/profiler-netmonitor-open.png Binary files differnew file mode 100644 index 0000000000..31c78845f5 --- /dev/null +++ b/devtools/docs/contributing/performance/profiler-netmonitor-open.png diff --git a/devtools/docs/contributing/performance/profiler-resource-url.png b/devtools/docs/contributing/performance/profiler-resource-url.png Binary files differnew file mode 100644 index 0000000000..d4631f5317 --- /dev/null +++ b/devtools/docs/contributing/performance/profiler-resource-url.png diff --git a/devtools/docs/contributing/react-performance-tips.md b/devtools/docs/contributing/react-performance-tips.md new file mode 100644 index 0000000000..db6f920895 --- /dev/null +++ b/devtools/docs/contributing/react-performance-tips.md @@ -0,0 +1,527 @@ +# Writing efficient React code + +In this article we'll discuss about the various component types we can use, as +well as discuss some tips to make your React application faster. + +## TL;DR tips + +* Prefer props and state immutability and use `PureComponent` components as a default +* As a convention, the object reference should change **if and only if** the inner data + changes. + * Be careful to never use new instance of functions as props to a Component (it's fine to use + them as props to a DOM element). + * Be careful to not update a reference if the inner data doesn't change. +* [Always measure before optimizing](./performance.md) to have a real impact on + performance. And always measure _after_ optimizing too, to prove your change + had a real impact. + +## How React renders normal components + +### What's a normal component? +As a start let's discuss about how React renders normal plain components, that +don't use `shouldComponentUpdate`. What we call plain components here are either: +* classes that extend [`Component`](https://reactjs.org/docs/react-component.html) + ```jsx + class Application extends React.Component { + render() { + return <div>{this.props.content}</div>; + } + } + ``` +* normal functions that take some `props` as parameter and return some JSX. We + call these functions either Stateless Components or Functional Components. + This is important to understand that these Stateless Components are _not_ + especially optimized in React. + ```jsx + function Application(props) { + return <div>{props.content}</div>; + } + ``` + These functions are equivalent to classes extending `Component`. In + the rest of the article we'll especially focus on the latter. Unless otherwise + stated everything about classes extending `Component` is also true for + Stateless/Functional Components. + +#### Notes on the use of JSX +Because we don't use a build step in mozilla-central yet, some of our +tools don't use JSX and use [factories](https://reactjs.org/docs/react-api.html#createfactory) +instead: +```javascript +class Application extends React.Component { + render() { + return dom.div(null, this.props.content); + } +} +``` + +We'll use JSX in this documentation for more clarity but this is strictly +equivalent. You can read more on [React documentation](https://reactjs.org/docs/react-without-jsx.html). + +### The first render +There's only one way to start a React application and trigger a first render: +calling `ReactDOM.render`: + +```jsx +ReactDOM.render( + <Application content='Hello World!'/>, + document.getElementById('root') +); +``` + +React will call that component's `render` method, and then recursively call +every child's `render` method, generating a rendering tree and then a virtual +DOM tree. It will then render actual DOM elements to the specified container. + +### Subsequent rerenders + +There are several ways to trigger a rerender: +1. We call `ReactDOM.render` again with the same component. + ```jsx + ReactDOM.render( + <Application content='Good Bye, Cruel World!'/>, + document.getElementById('root') + ); + ``` +2. One component's state changes, through the use of [`setState`](https://reactjs.org/docs/react-component.html#setstate). + If the application is using Redux, this is how Redux-connected components + trigger updates too. +3. One component's props change. But note that this can't happen by itself, this + is always a consequence of the case 1 or 2 in one of its parents. So we'll + ignore this case for this chapter. + +When one of these happens, just like the initial render, React will call that +component's `render` method, and then recursively call every child's `render` +method, but this time possibly with changed props compared to the previous render. + +These recursive calls produce a new rendering tree. That's where React uses an +algorithm called _virtual diffing_ or +[_reconciliation_](https://reactjs.org/docs/reconciliation.html) to find the +minimal set of updates to apply to the DOM. This is good because the less +updates to the DOM the less work the browser has to do to reflow and repaint the +application. + +### Main sources of performance issues + +From this explanation we can gather that the main performance issues can +come from: +1. triggering the render process **too frequently**, +2. **expensive** render methods, +3. the reconciliation algorithm itself. The algorithm is O(n) according to React + authors, which means the processing duration increases linearly with **the number + of elements in the tree** we compare. So a larger tree means a longer time to + process. + +Let's dive more into each one of these issues. + +#### Do not render too often + +A rerender will happen after calling `setState` to change the +local state. + +Everything that's in the state should be used in `render`. +Anything in the state that's not used in `render` shouldn't be in the state, but +rather in an instance variable. This way you won't trigger an update if you +change some internal state that you don't want to reflect in the UI. + +If you call `setState` from an event handler you may call it too often. +This is usually not a problem because React is smart enough to merge close +setState calls and trigger a rerender only once per frame. Yet if your `render` +is expensive (see below as well) this could lead to problems and you may want to +use `setTimeout` or other similar techniques to throttle the renders. + +#### Keep `render` methods as lean as possible + +When rendering a list, it's very common that we'll map this list to a list of +components. This can be costly and we might want to cut this list in several +chunks of items or to +[virtualize this list](https://reactjs.org/docs/optimizing-performance.html#virtualize-long-lists). +Although this is not always possible or easy. + +Do not do heavy computations in your `render` methods. Rather do them before +setting the state, and set the state to the result of these computations. +Ideally `render` should be a direct mirror of the component's props and state. + +Note that this rule also applies to the other methods called as part of the +rendering process: `componentWillUpdate` and `componentDidUpdate`. In +`componentDidUpdate` especially avoid synchronous reflows by getting DOM +measurements, and do not call `setState` as this would trigger yet another +update. + +#### Help the reconciliation algorithm be efficient + +The smaller the tree is, the faster the algorithm is. So it's +useful to limit the changes to a subtree of the full tree. Note that the use of +`shouldComponentUpdate` or `PureComponent` alleviates this issue by cutting off +entire branches from the rendering tree, [we discuss this in more details +below](shouldcomponentupdate-and-purecomponent-avoiding-renders-altogether). + +Try to change the state as close as possible to where your UI +should change (close in the components tree). + +Do not forget to [set `key` attributes when rendering a list of +things](https://reactjs.org/docs/lists-and-keys.html), which shouldn't be the +array's indices but something that identifies the item in a predictable, unique +and stable way. This helps the algorithm +a lot by skipping parts that likely haven't changed. + +### More documentation + +The React documentation has [a very well documented page](https://reactjs.org/docs/implementation-notes.html#mounting-as-a-recursive-process) +explaining the whole render and rerender process. + +## `shouldComponentUpdate` and `PureComponent`: avoiding renders altogether + +React has an optimized algorithm to apply changes. But the fastest algorithm is +an algorithm that isn't executed at all. + +[React's own documentation about performance](https://reactjs.org/docs/optimizing-performance.html#shouldcomponentupdate-in-action) +is quite complete on this subject. + +### Avoiding rerenders with `shouldComponentUpdate` + +As the first step of a rerender process, React calls your component's +[`shouldComponentUpdate`](https://reactjs.org/docs/react-component.html#shouldcomponentupdate) +method with 2 parameters: the new props, and the new +state. If this method returns false, then React will skip the render process for this +component, **and its whole subtree**. + +```jsx +class ComplexPanel extends React.Component { + // Note: this syntax, new but supported by Babel, automatically binds the + // method with the object instance. + onClick = () => { + this.setState({ detailsOpen: true }); + } + + // Return false to avoid a render + shouldComponentUpdate(nextProps, nextState) { + // Note: this works only if `summary` and `content` are primitive data + // (eg: string, number) or immutable data + // (keep reading to know more about this) + return nextProps.summary !== this.props.summary + || nextProps.content !== this.props.content + || nextState.detailsOpen !== this.state.detailsOpen; + } + + render() { + return ( + <div> + <ComplexSummary summary={this.props.summary} onClick={this.onClick}/> + {this.state.detailsOpen + ? <ComplexContent content={this.props.content} /> + : null} + </div> + ); + } +} +``` + +__This is a very efficient way to improve your application speed__, because this +avoids everything: both calling render methods for this component _and_ the +whole subtree, and the reconciliation phase for this subtree. + +Note that just like the `render` method, `shouldComponentUpdate` is called once +per render cycle, so it needs to be very lean and return as fast as possible. So +it should execute some cheap comparisons only. + +### `PureComponent` and immutability + +A very common implementation of `shouldComponentUpdate` is provided by React's +[`PureComponent`](https://reactjs.org/docs/react-api.html#reactpurecomponent): +it will shallowly check the new props and states for reference equality. + +```jsx +class ComplexPanel extends React.PureComponent { + // Note: this syntax, new but supported by Babel, automatically binds the + // method with the object instance. + onClick = () => { + // Running this repeatidly won't render more than once. + this.setState({ detailsOpen: true }); + } + + render() { + return ( + <div> + <ComplexSummary summary={this.props.summary} onClick={this.onClick}/> + {this.state.detailsOpen + ? <ComplexContent content={this.props.content} /> + : null} + </div> + ); + } +} +``` + +This has a very important consequence: for non-primitive props and states, that is +objects and arrays that can be mutated without changing the reference itself, +PureComponent's inherited `shouldComponentUpdate` will yield wrong results and will +skip renders where it shouldn't. + +So you're left with one of these two options: +* either implement your own `shouldComponentUpdate` in a `Component` +* or (__preferred__) decide to make all your data structure immutable. + +The latter is recommended because: +* It's much simpler to think about. +* It's much faster to check for equality in `shouldComponentUpdate` and in other + places (like Redux' selectors). + +Note you could technically implement your own `shouldComponentUpdate` in a +`PureComponent` but this is quite useless because `PureComponent` is nothing +more than `Component` with a default implementation for `shouldComponentUpdate`. + +### About immutability +#### What it doesn't mean +It doesn't mean you need to enforce the immutability using a library like +[Immutable](https://github.com/facebook/immutable-js). + +#### What it means +It means that once a structure exists, you don't mutate it. + +**Every time some data changes, the object reference must change as well**. This +means a new object or a new array needs to be created. This gives the nice +reverse guarantee: if the object reference has changed, the data has changed. + +It's good to go one step further to get a **strict equivalence**: if the data +doesn't change, the object reference mustn't change. This isn't necessary for +your app to work, but this is a lot better for performance as this avoids +spurious rerenders. + +Keep reading to learn how to proceed. + +#### Keep your state objects simple + +Updating your immutable state objects can be difficult if the objects used are +complex. That's why it's a good idea to keep the objects simple, especially keep +them not nested, so that you don't need to use a library like +[immutability-helper](https://github.com/kolodny/immutability-helper), +[updeep](https://github.com/substantial/updeep), or even +[Immutable](https://github.com/facebook/immutable-js). Be especially careful +with Immutable as it's easy to create performance problems by misusing +its API. + +If you're using Redux ([see below as well](#a-few-words-about-redux)) this +advice applies to your individual reducers as well, even if Redux tools make +it easy to have a nested/combined state. + +#### How to update an object + +Updating an object is quite easy. + +You must not change/add/delete inner properties directly: + +```javascript +// Note that in the following examples we use the callback version +// of `setState` everywhere, because we build the new state from +// the current state. + +// Please don't do this as this will likely induce bugs. +this.setState(state => { + state.stateObject.details = details; + return state; +}); + +// This is wrong too: `stateObject` is still mutated. +this.setState(({ stateObject }) => { + stateObject.details = details; + return { stateObject }; +}); +``` + +Instead **you must create a new object** for this property. In this example +we'll use the object spread operator, already implemented in Firefox, Chrome and Babel. + +However here we take care to return the same object if it doesn't need an update. The +comparison happens inside the callback because it depends on the state as +well. This is a good thing to do so that the shallow equality check doesn't +return false if nothing changes. + +```javascript +// Updating one property in the state +this.setState(({ stateObject }) => ({ + stateObject: stateObject.content === newContent + ? stateObject + : { ...stateObject, content: newContent }, +}); + +// This is very similar if 2 properties need an update: +this.setState(({ stateObject1, stateObject2 }) => ({ + stateObject1: stateObject1.content === newContent + ? stateObject1 + : { ...stateObject1, content: newContent }, + stateObject2: stateObject2.details === newDetails + ? stateObject2 + : { ...stateObject2, details: newDetails }, +}); + +// Or if one of the properties needs to update 2 of it's own properties: +this.setState(({ stateObject }) => ({ + stateObject: stateObject.content === newContent && stateObject.details === newDetails + ? stateObject + : { ...stateObject, content: newContent, details: newDetails }, +}); +``` + +Note that this isn't about the returned `state` object, but its properties. +The returned object is always merged into the current state, and React creates +a new component's state object at each update cycle. + +#### How to update an array +Updating an array is easy too. + +You must avoid methods that mutate the array like push/splice/pop/shift and you +must not change directly an item. + +```javascript +// Please don't do this as this will likely induce bugs. +this.setState(({ stateArray }) => { + stateArray.push(newItem); // This is wrong + stateArray[1] = newItem; // This is wrong too + return { stateArray }; +}); +``` + +Instead here again you need to **create a new array instance**. + +```javascript +// Adding an element is easy. +this.setState(({ stateArray }) => ({ + stateArray: [...stateArray, newElement], +})); + +this.setState(({ stateArray }) => { + // Removing an element is more involved. + const newArray = stateArray.filter(element => element !== removeElement); + // or + const newArray = [...stateArray.slice(0, index), ...stateArray.slice(index + 1)]; + // or do what you want on a new clone: + const newArray = stateArray.slice(); + return { + // Because we want to keep the old array if removeElement isn't in the + // filtered array, we compare the lengths. + // We still start a render phase because we call `setState`, but thanks to + // PureComponent's shouldComponentUpdate implementation we won't actually render. + stateArray: newArray.length === stateArray.length ? stateArray : newArray, + }; + + // You can also return a falsy value to avoid the render cycle at all: + return newArray.length === stateArray.length + ? null + : { stateArray: newArray }; +}); +``` + +#### How to update Maps and Sets +The process is very similar for Maps and Sets. Here is a quick example: + +```javascript +// For a Set +this.setState(({ stateSet }) => { + if (!stateSet.has(value)) { + stateSet = new Set(stateSet); + stateSet.add(value); + } + return { stateSet }; +}); + +// For a Map +this.setState(({ stateMap }) => { + if (stateMap.get(key) !== value) { + stateMap = new Map(stateMap); + stateMap.set(key, value); + } + return { stateMap }; +})); +``` + +#### How to update primitive values + +Obviously, with primitive types like boolean, number or string, that are +comparable with the operator `===`, it's much easier: + +```javascript +this.setState({ + stateString: "new string", + stateNumber: 42, + stateBool: false, +}); +``` + +Note that we don't use the callback version of `setState` here. That's because +for primitive values we don't need to use the previous state to generate a new +state. + +#### A few words about Redux + +When working with Redux, the rules stay the same, except all of this +happens in your reducers instead of in your components. With Redux comes the +function [`combineReducers`](https://redux.js.org/docs/api/combineReducers.html) +that obeys all the rules we outlined before while making it possible to have a +nested state. + +### `shouldComponentUpdate` or `PureComponent`? + +It is highly recommended to go the full **PureComponent + immutability** route, +instead of writing custom `shouldComponentUpdate` implementations for +components. This is more generic, more maintainable, less error-prone, faster. + +Of course all rules have exceptions and you're free to implement a +`shouldComponentUpdate` method if you have specific cases to take care of. + +### Some gotchas with `PureComponent` + +Because `PureComponent` shallowly checks props and state, you need to take care +to not create a new reference for something that's otherwise identical. Some +common cases are: + +* Using a new instance for a prop at each render cycle. Especially, do not use + a bound function or an anonymous function (both classic functions or + arrow functions) as a prop: + + ```jsx + render() { + return <MyComponent onUpdate={() => this.update()} />; + } + ``` + + Each time the `render` method runs, a new function will be created, and in + `MyComponent`'s `shouldComponentUpdate` the shallow check will always fail + defeating its purpose. + +* Using another reference for the same data. One very common example is the empty + array: if you use a new `[]` for each render, you won't skip render. A solution + is to reuse a common instance. Be careful as this can very well be hidden + within some complicated Redux reducers. + +* A similar issue can arise if you use sets or maps. If you add an element in a + `Set` that's already in there, you don't need to return a new `Set` as it will be + identical. + +* Be careful with array's methods, especially `map` or `filter`, as they always + return a new array. So even with the same inputs (same input array, same + function), you'll get a new output, even if it contains the same data. If + you're using Redux, [reselect](https://github.com/reactjs/reselect) is + recommended. + [memoize-immutable](https://github.com/memoize-immutable/memoize-immutable) + can be useful in some cases too. + +## Diagnosing performance issues with some tooling + +[You can read about it in the dedicated +page](./performance.md#diagnosing-performance-issues-in-react-based-applications). + +## Breaking the rules: always measure first + +You should generally follow these rules because they bring a consistent +performance in most cases. + +However you may have specific cases that will need that you break the rules. In +that case the first thing to do is to **measure** using a profiler so that you +know where your problem are. + +Then and only then you can decide to break the rules by using some mutable state +and/or custom `shouldComponentUpdate` implementation. + +And remember to measure again after you did your changes, to check and prove +that your changes actually made an impact. Ideally you should always give links +to profiles when requesting a review for a performance patch. |
