Closed Bug 898125 Opened 11 years ago Closed 9 years ago

add 'use strict' to remaining files in browser/metro


(Firefox for Metro Graveyard :: General, defect)

Windows 8.1
Not set


(Not tracked)



(Reporter: ally, Unassigned, Mentored)



(1 file)

- we still have 55 files that are not strict mode
- We can make a checklist or even a tool to grep for common things that will break in strict mode (arguments.callee, octal literals, "with" and "eval").
- has a complete list of changes in strict mode, though not all are trivial to grep for.

Let's do this in parts, based on how much refactoring is needed for each set of files.
OS: Mac OS X → Windows 8 Metro
Whiteboard: [mentor=ally][mentor=mbrubeck][lang=js]
Hello, I am a new contributor and would like to work on it? Can I get some help on how to get things started?
Certainly!  First, you'll need to check out the Firefox source code and build Metro Firefox.  See these pages for some instructions if needed:

You can run this shell command in your mozilla-central repository to find Metro files that are not yet in strict mode:

find browser/metro -name '*.js' | xargs grep -L 'use strict'

Pick a file from that list to start with.  Add "use strict"; to the top of the file, after the license header but before any lines of code.  Make sure the file doesn't contain anything that will break in strict mode.  The most common things I've found in our code are:

* arguments.callee
* octal literals (multi-digit numbers starting with '0').

I'd start with files in /browser/metro/base/tests because we should be able to find any remaining problems in those automatically; other files will need careful testing and auditing to make sure we aren't introducing new bugs.
Comment on attachment 788280 [details] [diff] [review]
Added 'use strict' on /browser/metro/base/tests/*.js

Review of attachment 788280 [details] [diff] [review]:

Look good, thanks!  Our tests still pass on my machine with this patch.  I pushed it to the Try server to make sure it passes on our automated build machines too:

If that succeeds, I'll check in this patch for you.
Attachment #788280 - Flags: review?(mbrubeck) → review+
I landed this change on our integration branch.  It will be merged to the main mozilla-central repository within the next few days.  Thanks again!

If you or anyone else wants to continue working on these changes, we'll need to work on some testing strategies to help us catch anything that breaks accidentally.  I'd be most comfortable checking in changes for just a few files at a time and doing some manual testing to make sure the code in each one is still correct.
Whiteboard: [mentor=ally][mentor=mbrubeck][lang=js] → [mentor=ally][mentor=mbrubeck][lang=js][leave open]
Mentor: paul.dally mbrubeck
Mentor: mbrubeck, paul.dally
Whiteboard: [mentor=ally][mentor=mbrubeck][lang=js][leave open] → [lang=js][leave open]
Mentor: mbrubeck, paul.dally
OS: Windows 8 Metro → Windows 8.1
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][leave open]
You need to log in before you can comment on or make changes to this bug.