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

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Allison Naaktgeboren :ally]

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.

Comment 1

[Nootan Ghimire]

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

[Matt Brubeck (:mbrubeck)]

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

[Nootan Ghimire]

Attachment: Added 'use strict' on /browser/metro/base/tests/*.js

Comment 4

[Matt Brubeck (: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.

Comment 5

[Matt Brubeck (:mbrubeck)]

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.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/34fef1cab94c