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

RESOLVED FIXED

Status

Firefox for Metro
General
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: ally, Unassigned, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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

4 years ago
OS: Mac OS X → Windows 8 Metro
Whiteboard: [mentor=ally][mentor=mbrubeck][lang=js]

Comment 1

4 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?
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

4 years ago
Created attachment 788280 [details] [diff] [review]
Added 'use strict' on /browser/metro/base/tests/*.js
Attachment #788280 - Flags: review?(mbrubeck)
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+
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]
https://hg.mozilla.org/mozilla-central/rev/34fef1cab94c
(Assignee)

Updated

4 years ago
Mentor: paul.dally@themutualgroup.com mbrubeck@mozilla.com
Mentor: mbrubeck@mozilla.com, paul.dally@themutualgroup.com
Whiteboard: [mentor=ally][mentor=mbrubeck][lang=js][leave open] → [lang=js][leave open]

Updated

4 years ago
Mentor: mbrubeck@mozilla.com, paul.dally@themutualgroup.com
(Assignee)

Updated

3 years ago
OS: Windows 8 Metro → Windows 8.1
Status: NEW → RESOLVED
Last Resolved: 2 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.