I’m sure Firefox, the browser from the Mozilla Foundation needs no introduction. Even if Google Chrome is the preferred web browser for most people it would be hard to find someone who doesn’t know what Firefox is. Personally, Firefox has been my browser of choice for nearly a decade and a half so I was rather excited to add a contribution of my own to the browser that I use almost daily.
ESLint
ESLint is a code linter that checks if code is standard compliant before it is submitted to help avoid wasting time during code reviews. As Firefox’s codebase is so large that some of the older pieces of source code have not been updated to the standard so they are currently ignored by ESLint. For my bug I worked on enabling ESLint for the directories dom/abort/, dom/asmjscache/, dom/battery/, dom/broadcastchannel/ and dom/console/.
Set Up
I have development environments for all three major operating systems set up to some capacity (mostly due to virtual machines) and my first step was to decide which environment to develop with. After skimming through the documentation for getting the source code and the tools I would be using to submit the code I decided to go with MacOS.
Getting the Source Code
Mozilla uses Mercurial to manage their source code. To install Mercurial I ran:
brew install mercurialwhich installed Mercurial to my machine. Next I cloned the Firefox source code by running:
hg clone https://hg.mozilla.org/mozilla-central/This can take anywhere between 30 minutes to an hour.
As I would be implementing lint fixes I did not build the source code.
To prep myself for the submission process I downloaded ancanist, libphutil and moz-phab from their repositories. Then I added arcanist/bin and moz-phab directories to my system path to enable the arc and moz-phab commands.
ESLint Fixes
I removed the directories that I’ll be enabling ESLint for from the .eslintignore file which holds the list of directories which ESLint is supposed to ignore and I checked the number of ESLint errors I had by running
./mach eslint dom/
246 errors. Quite a lot! Before we panic let’s see how many we can eliminate using ESLint’s automatic fix command:
./mach eslint --fix dom/
33 errors now. Much more manageable. Let’s commit these changes before moving on. My particular commit command was:
hg commit -m "Bug 1508988 - Enable ESLint for dom/abort/, dom/asmjscache/, dom/battery/, dom/broadcastchannel/ and dom/console/ (automatic changes). r?standard8!"Note that during the submission process the bug ID and the reviewer (in my case standard8) will be parsed out of the commit message.
The Fixes
The errors I encountered was generally one of the following:
no-undef
- The script was a worker and the line
/* eslint-env worker */had to be added to let ESLint know - The variable was undefined so I had to define it with
let
no-shadow
The variable is already declared in an upper scope so the variable is shadowed if it’s re-declared.
- This was fixed by changing the
vardecoration toletas let is scope aware - Using a new variable for function arguments
no-redeclare
The variable was redeclared so I removed the var
After I was left with 9 errors that I wasn’t sure how to handle
Unfortunately I couldn’t search through the source code as Mozilla’s source viewer DXR was unreachable as I was working on the bug so it hampered my ability to search through the source code so at this point I was stuck. I couldn’t figure out where checkForEventListenerLeaks, jsFuns and complete came from so I couldn’t safely exclude them.
As for id, I am fairly new to Javascript and I was relatively unfamiliar with the syntax. I could recognize that a deconstructing assignment was occurring but I wasn’t sure what it was deconstructing and I wasn’t entirely sure how this should be handled.
I talked to my code reviewer Mark Banner who directed me to another source code search page, Searchfox and guided me through the process of finding jsFuns and complete which are injected into the global scope. As these variables are globally defined so they could be handled by adding the comment /* globals jsFuns:false, complete:false */.
So the loop on line 88 was destructing an array, not an object so it was important that something was there so the line was changed to:
for (let [, events] of _consoleStorage) {According to Mark, the checkForEventListenerLeaks errors occur because the Firefox ESLint setup hasn’t been taught everything about the Firefox setup and the simpliest fix is to add the line /* import-globals-from ../../events/test/event_leak_utils.js */ after the Javascript script tag.
These fixes fixed all the ESLint errors.
I committed my code in the exact same way as before except the commit message changed “automatic changes” to “manual changes”.
Submitting the Changes
To submit my changes I followed the Mozilla Phabricator User Guide and successfully posted my patches without any relative issues.
