Closed Bug 1083499 Opened 5 years ago Closed 5 years ago

Remove use of destructuring for-in (JS1.7-only language extension) from Add-on SDK and chrome JS

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

Attachments

(3 files)

No description provided.
Replace JS1.7's nonstandard destructuring for…in loops in mochitest/server.js with regular for…in loops. JS1.8 changed the behavior of key/value destructuring of arrays and we would like to remove SpiderMonkey's special case for JS1.7 (bug 1083498).

https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/1.8#Changes_in_destructuring_for..in
Assignee: nobody → cpeterson
Status: NEW → ASSIGNED
Attachment #8582206 - Flags: review?(jmaher)
Replace JS1.7's nonstandard destructuring for…in loops in layout/analysis/simple-match.js with regular for…in loops. Also remove unused variable `matches`, replace nonstandard expression closure `identity` with a real function (bug 1083459), and fix some comment typos.

JS1.8 changed the behavior of key/value destructuring of arrays and we would like to remove SpiderMonkey's special case for JS1.7 (bug 1083498).

https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/1.8#Changes_in_destructuring_for..in

I suspect that no one has used this script (simple-match.js and pixel-conversion.js) since it landed in 2009 (bug 495002) because it would have been broken by the destructuring for…in loop change in JS1.8. Should we just remove these scripts in https://hg.mozilla.org/mozilla-central/file/840cfd5bc971/layout/analysis?
Attachment #8582209 - Flags: review?(dholbert)
Depends on: 495002
Comment on attachment 8582209 [details] [diff] [review]
replace-layout-analysis-loop.patch

This seems fine.

I always forget the details of these JS looping techniques, but to sanity-check, I tested a simplified version of the loop with & without the change -- they both behave the same (and have the variables set correctly) when inside of a script tag like:
 <script type="application/javascript;version=1.7">

BUT, the old version is indeed broken when inside of a script tag with a 1.8 version-number:
 <script type="application/javascript;version=1.8">

(This is the first I've heard of these scripts, and I'm also wondering whether they serve any purpose now. I'd check with the author or reviewer before removing them; but I'm happy to sign off on this minor cleanup.)
Attachment #8582209 - Flags: review?(dholbert) → review+
Comment on attachment 8582209 [details] [diff] [review]
replace-layout-analysis-loop.patch

Daniel, you wrote the layout/analysis/pixel-conversion.js and simple-match.js scripts for some CSSPixelsToDevPixels and DevPixelsToCSSPixels source transformations. Is there any need to keep these scripts in mozilla-central? They are broken in JS1.8 and JS1.8.5, js shell's default JS dialect, because they depend on some JS1.7-specific quirks of for…in loops.
Attachment #8582209 - Flags: feedback?(db48x)
Thanks, Joel!

replace-mochitest-server-loops.patch
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab32e77962e2
Keywords: leave-open
Remove unused layout/analysis/pixel-conversion.js and simple-match.js scripts.

Daniel, how about we just remove these scripts that are broken in JS1.8 and not referenced by any scripts in mozilla-central?
Attachment #8583571 - Flags: review?(dholbert)
Comment on attachment 8583571 [details] [diff] [review]
remove-unused-layout-scripts.patch

See end of comment 3 -- I'd like input from someone who was involved w/ these scripts being added before yanking them out.

(It's not too surprising that they're not used in the tree; IIUC, they exist to help patch-authors do tree-wide refactoring.  Though I'm not sure they've ever been for that used since they were added.)
Attachment #8583571 - Flags: review?(dholbert)
Comment on attachment 8583571 [details] [diff] [review]
remove-unused-layout-scripts.patch

Remove unused layout/analysis/pixel-conversion.js and simple-match.js scripts.

These scripts were added in bug 495002 to find code that could use the CSSPixelsToDevPixels() and DevPixelsToCSSPixels() conversion functions. Is there any need to keep these scripts in mozilla-central? They are broken in js shell's default JS dialect (JS1.8.5) because they depend on JS1.7-specific behavior of for…in loops (and even that JS1.7-specific behavior has been removed in Nightly 40 by bug 1083498).
Attachment #8583571 - Flags: review?(roc)
Comment on attachment 8583571 [details] [diff] [review]
remove-unused-layout-scripts.patch

Review of attachment 8583571 [details] [diff] [review]:
-----------------------------------------------------------------

Oops, misfire
Attachment #8583571 - Flags: review- → review+
Attachment #8582209 - Flags: feedback?(db48x)
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/7ba5b502e7b7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.