Closed
Bug 898125
Opened 11 years ago
Closed 9 years ago
add 'use strict' to remaining files in browser/metro
Categories
(Firefox for Metro Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ally, Unassigned, Mentored)
Details
Attachments
(1 file)
3.13 KB,
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
mbrubeck
- 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").
- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions_and_function_scope/Strict_mode 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.
Reporter | ||
Updated•11 years ago
|
OS: Mac OS X → Windows 8 Metro
Whiteboard: [mentor=ally][mentor=mbrubeck][lang=js]
Comment 1•11 years ago
|
||
Hello, I am a new contributor and would like to work on it? Can I get some help on how to get things started?
Comment 2•11 years ago
|
||
Certainly! First, you'll need to check out the Firefox source code and build Metro Firefox. See these pages for some instructions if needed:
https://developer.mozilla.org/en-US/docs/Introduction
https://wiki.mozilla.org/Firefox/Windows_8_Integration#Building_Locally
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 3•11 years ago
|
||
Attachment #788280 -
Flags: review?(mbrubeck)
Comment 4•11 years ago
|
||
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:
https://tbpl.mozilla.org/?tree=Try&rev=f8d14b2b6715
If that succeeds, I'll check in this patch for you.
Attachment #788280 -
Flags: review?(mbrubeck) → review+
Comment 5•11 years ago
|
||
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!
https://hg.mozilla.org/integration/fx-team/rev/34fef1cab94c
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]
Comment 6•11 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Mentor: paul.dally mbrubeck
Mentor: mbrubeck, paul.dally
Whiteboard: [mentor=ally][mentor=mbrubeck][lang=js][leave open] → [lang=js][leave open]
Updated•10 years ago
|
Mentor: mbrubeck, paul.dally
Assignee | ||
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
Updated•9 years ago
|
Status: NEW → RESOLVED
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.
Description
•