1153671 - JavaScript 1.6's for-each-in loops are deprecated

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Ian Neal]

According to https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/for_each...in, for each in loops are now deprecated.

We should use for of loops instead.

https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/for...of

Related Firefox bug: Bug 791348

For reference - http://mxr.mozilla.org/comm-central/search?string=for+each+%28&find=suite%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

Comment 1

[Ravi Shankar [:waffles]]

Hey Ian, I think I can work on this bug. So, we gotta replace the `for-each` lines in all those 47 files with `for-of` loops, right?

Comment 2

[Ravi Shankar [:waffles]]

Oops, and I forgot to ask something. Is there a way to test the files once I do my revision? (like the one we do with `./mach xpcshell-test`)

Comment 3

[Ian Neal]

(In reply to Ravi Shankar [:waffles] from comment #1)
> Hey Ian, I think I can work on this bug. So, we gotta replace the `for-each`
> lines in all those 47 files with `for-of` loops, right?

Yes. I will confirm the best way to test soon.

Comment 4

[Ian Neal]

Looks like mochitests are broken on c-c at the moment, so I need to chase getting those fixed.

Comment 5

[Ravi Shankar [:waffles]]

Ah, okay. No problem. In the meantime, can I go ahead, work on the bug and submit my patches, or should I wait till they've been fixed?

Comment 6

[Ian Neal]

(In reply to Ravi Shankar [:waffles] from comment #5)
> Ah, okay. No problem. In the meantime, can I go ahead, work on the bug and
> submit my patches, or should I wait till they've been fixed?

It is fine to work on the bug and submit patches.

Comment 7

[Ian Neal]

mochitests can run locally now, so any issues, just say.

Comment 8

[Ravi Shankar [:waffles]]

Okay, I need some help. How shall I run mochitests locally? The docs (https://developer.mozilla.org/en/docs/Mochitest#Running_tests_on_comm-central_%28Thunderbird.2C_SeaMonkey%29) say that we gotta use "pymake" for testing (which I don't really know). For instance, I gotta test this JS file (http://mxr.mozilla.org/comm-central/source/suite/browser/test/browser/browser_bug581947.js) which is the first one which needs replacement. How should I test it?

Comment 9

[Ian Neal]

It should be something like:
TEST_PATH=suite/browser/test/browser/browser_bug581947.js make -C $(OBJDIR) mochitest-browser-chrome
where you have to replace $(OBJDIR) with the object directory.

You need to check the failures of each set of tests before and after your patch. If the fails before and after are the same, then you have not regressed anything.

Some existing failures will already have bugs raised but some won't, so, for the two in browser_bug581947.js, I have raised bugs against SeaMonkey::Testing Infrastructure (bug 1165264 and bug 1165312). If you spot any that have not been raised then please mention in here, and you have the chance, create a bug for each one.

Comment 10

[Ravi Shankar [:waffles]]

Here's a list of files which I was unable to test (due to the issues below). Is there any other way to test them?

- suite/common/downloads/tests/chrome/*.xul
- suite/common/tests/chrome/test_idcheck.xul
- suite/common/sync/syncNotification.xml
- suite/common/tests/chrome/test_idcheck.xul

^^^ Well, I dunno how I should go about testing XUL and XML files

- suite/common/pref/pref-applications.js
- suite/common/search/engineManager.js
- suite/common/src/nsSessionStore.js
- suite/common/sync/syncQuota.js
- suite/common/sync/syncSetup.js
- suite/mailnews/*.js

^^^ These files didn't go through the test. They always log something like, "Running 0 tests, leaving". Maybe there's a different test for them (I guess?) Log here: https://pastebin.mozilla.org/8833852

- suite/common/tests/browser/browser_354894.js

^^^ This file's log after patch has ~100 additional lines compared to the log before patch. Here's a "diff" of both the test logs - https://pastebin.mozilla.org/8833857. Both logs have the same number of failures though (18 FAIL - before & after the patch). I'm just curious why it got extra lines.

- suite/common/tests/browser/browser_456342.js

^^^ This file's log not only gets larger, but also lags behind (i.e) the test takes too long to finish (Sea Monkey window is stays still for about a minute). And, it shows an additional FAILED test.

All the other files showed the same errors before & after the patch.

Comment 11

[Ravi Shankar [:waffles]]

Attachment: for_each

Here's my current patch, where I've simply modified all the occurrences of `for each... in` with `for... of` in all those 47 files.

Comment 12

[Ian Neal]

(In reply to Ravi Shankar [:waffles] from comment #10)
> Here's a list of files which I was unable to test (due to the issues below).
> Is there any other way to test them?
> 
> - suite/common/downloads/tests/chrome/*.xul
> - suite/common/tests/chrome/test_idcheck.xul
Those that are in tests/chrome you would do with "mochitest-chrome" rather than "mochitest-browser-chrome"

Those that are not in tests directory you could test by starting SeaMonkey and using that part of the code.

I'll have a look at those other two tests over the next 7 days.

Comment 13

[Ravi Shankar [:waffles]]

[UPDATED LIST] - Ian, you don't have to worry about the previous comments. I'll update the list every now & then and notify you of the results :)

 - suite/common/downloads/tests/chrome/test_cleanup_search.xul
 - suite/common/downloads/tests/chrome/test_open_properties.xul
 - suite/common/downloads/tests/chrome/test_search_clearlist.xul

^^^ The mochitests were frozen while testing these files (that I had to close the windows manually after 2 minutes). I guess that the tests don't close the windows automatically after testing. They're simply staying there, doing nothing. With this on the way, whole directory testing can't be done. Only individual file tests are possible.

 - suite/common/sync/syncNotification.xml

^^^ This is still there. I dunno how to test this XML.

 - suite/common/pref/pref-applications.js
 - suite/common/search/engineManager.js
 - suite/common/src/nsSessionStore.js
 - suite/common/sync/syncQuota.js
 - suite/common/sync/syncSetup.js
 - suite/mailnews/mailWindowOverlay.js
 - suite/mailnews/mailViewList.js
 - suite/mailnews/mailWindow.js
 - suite/mailnews/msgHdrViewOverlay.js
 - suite/mailnews/msgViewNavigation.js

^^^ I haven't tested these yet. Will do it soon :)

 - suite/common/tests/browser/browser_354894.js

^^^ This file's test log after patch has ~100 additional lines compared to the log before patch. Both logs have the same number of failures though (18 FAIL - before & after the patch). I'm just curious why it got extra lines.

 - suite/common/tests/browser/browser_456342.js

^^^ This file's log not only gets larger, but also lags behind (i.e) the test takes too long to finish (Sea Monkey window stays still). And, it showed an additional FAILED test.

All the other files either showed the same errors before & after the patch, or showed no errors at all.

Comment 14

[Ravi Shankar [:waffles]]

With some help from @RattyAway and some fiddling around, these are some conclusions:

 - suite/common/sync/syncNotification.xml - has a lot of broken code and so I should probably neglect it for now.
 - suite/common/tests/browser/browser_354894.js - has no problem and so we don't have to worry
 - suite/common/tests/browser/browser_456342.js - for...of cannot be replaced in this file since the log shows that `savedFormData` is not an iterator. I tried modifying it (with this as reference: http://hg.mozilla.org/mozilla-central/diff/35b6230f10c1/browser/components/sessionstore/test/browser_456342.js#l1.48), but still no luck.

Comment 15

[Ravi Shankar [:waffles]]

[UPDATE #2]

 - suite/common/downloads/tests/chrome/test_cleanup_search.xul
 - suite/common/downloads/tests/chrome/test_open_properties.xul
 - suite/common/downloads/tests/chrome/test_search_clearlist.xul

^^^ (See comment #13) These are still there (freezing the mochitests)

 - suite/common/sync/syncNotification.xml

^^^ This is still there. I dunno I should go about testing this XML, and it's broken code (from comment #14)

 - suite/common/tests/browser/browser_456342.js

^^^ (See comment #14) This file has got problems with iterating through `savedFormData` as it's not an iterator.

The other files are fine. And,

 - suite/common/pref/pref-applications.js
 - suite/common/search/engineManager.js
 - suite/common/src/nsSessionStore.js
 - suite/common/sync/syncQuota.js
 - suite/common/sync/syncSetup.js
 - suite/mailnews/mailWindowOverlay.js
 - suite/mailnews/mailViewList.js
 - suite/mailnews/mailWindow.js
 - suite/mailnews/msgHdrViewOverlay.js
 - suite/mailnews/msgViewNavigation.js

^^^ I've tested these non-testable JS files by fiddling around the related options in Seamonkey (Helper apps, search engines, session restore, Sync, Mail, IRC, etc.) looking for errors in the console. They seem to work fine. Looking forward to your suggestions :)

Comment 16

[Ian Neal]

So we can start getting this checked in please could you split the patch into a number of parts:
1/ Tests that work
2/ Main code that works
3/ Tests that don't work
4/ syncNotification.xml

Comment 17

[Ravi Shankar [:waffles]]

Attachment: working_tests

Comment 18

[Ravi Shankar [:waffles]]

15 tests seem to work. More patches on the way.

Comment 19

[Ravi Shankar [:waffles]]

Attachment: failing_test

Only 1 test actually fails (when ran using mochitests).

Comment 20

[Ravi Shankar [:waffles]]

Attachment: freezing_tests

These tests freeze when ran using mochitest-chrome. They just get stuck along the way and don't seem to close Seamonkey (as they're supposed to do).

Comment 21

[Ravi Shankar [:waffles]]

Attachment: common and mailnews code

As you know, these tests require interaction with the binary. I didn't test them again (as it consumes more time). But the last time I tested them, they seemed to work just fine.

Comment 22

[Ravi Shankar [:waffles]]

Attachment: sync_notification

This is our last remaining test. We've never tested this beast :)

Comment 23

[Ian Neal]

Comment on attachment 8625734 [details] [diff] [review]
working_tests

r=me and a=me for CLOSED TREE tests only

Comment 24

[Ian Neal]

Comment on attachment 8625740 [details] [diff] [review]
common and mailnews code

r=me and a=me for CLOSED TREE

Comment 25

[Tooru Fujisawa [:arai]]

any progress?
all patches are still applicable with simple rebase.

Comment 26

[Tooru Fujisawa [:arai]]

Comment on attachment 8625734 [details] [diff] [review]
working_tests

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

::: suite/common/downloads/tests/chrome/test_delete_key_removes.xul
@@ +125,5 @@
>      "INSERT INTO moz_downloads (name, source, target, startTime, endTime, " +
>        "state, currBytes, maxBytes, preferredAction, autoResume) " +
>      "VALUES (:name, :source, :target, :startTime, :endTime, :state, " +
>        ":currBytes, :maxBytes, :preferredAction, :autoResume)");
> +  for each (var dl of DownloadData) {

remove `each`.

Comment 27

[Tooru Fujisawa [:arai]]

Attachment: JavaScript 1.6's for-each-in loops are deprecated; r=Ian (working tests)

taking over, to disable for each on nightly.

rebased and fixed some errors.
(fixed files are in next patch)

Comment 28

[Tooru Fujisawa [:arai]]

Attachment: JavaScript 1.6's for-each-in loops are deprecated; r=Ian (working tests)

sorry, it was wrong file

Comment 29

[Tooru Fujisawa [:arai]]

Attachment: JavaScript 1.6's for-each-in loops are deprecated; r=Ian (working tests, fixed files)

syntactic fix for test_delete_key_removes.xul (for each -> for)

in test_idcheck.xul, window.gLoadedWindows is object, and the code checks if the object has any key.
so replaced it to `Object.keys(window.gLoadedWindows).length == 0`.

Comment 30

[Tooru Fujisawa [:arai]]

Attachment: JavaScript 1.6's for-each-in loops are deprecated; r=Ian (failing test fixed)

savedFormData is an object, so we need to use `Object.values` to iterate over values.

Comment 31

[Tooru Fujisawa [:arai]]

Attachment: JavaScript 1.6's for-each-in loops are deprecated; r=Ian (freezing tests)

No change from previous one.
I don't see any issue in this change.
If the test still freezes, it should be different issue.

Comment 32

[Tooru Fujisawa [:arai]]

Attachment: JavaScript 1.6's for-each-in loops are deprecated; r=Ian (unsure tests)

Comment 33

[Tooru Fujisawa [:arai]]

Attachment: JavaScript 1.6's for-each-in loops are deprecated; r=Ian (unsure tests, fixed files)

fix for "unsure tests".
applied same fix as m-c.

Comment 34

[Tooru Fujisawa [:arai]]

Attachment: JavaScript 1.6's for-each-in loops are deprecated; r=Ian (sync_notification.xml)

not changed.
I don't see any issue with it.

Comment 35

[Philip Chee]

Comment on attachment 8783041 [details] [diff] [review]
JavaScript 1.6's for-each-in loops are deprecated; r=Ian (working tests, fixed files)

> -  for each (var dl in DownloadData) {
> +  for (var dl of DownloadData) {
for (let dl ...)

> -  for each (var dl in DownloadData) {
> +  for (var dl of DownloadData) {
for (let dl ...)
>      for (var prop in dl)
s/var/let/
Can we use for...of here?

Comment 36

[Philip Chee]

Comment on attachment 8783050 [details] [diff] [review]
JavaScript 1.6's for-each-in loops are deprecated; r=Ian (sync_notification.xml)

> -          for each (var notification in notifications) {
> +          for (var notification of notifications) {
s/var/let/

Comment 38

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Remove non-standard for-each-in from tests. r=Ian,Ratty

Thank you for reviewing.
Updated and folded patches.

Comment 39

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Remove non-standard for-each-in from remaining tests. r?Ratty

Comment 40

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Remove non-standard for-each-in from suite/. r=Ian,Ratty

Comment 41

[Tooru Fujisawa [:arai]]

Thanks :D

Comment 42

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Remove non-standard for-each-in from suite/. r=Ian,Ratty

Comment 43

[Iris Hsiao [:ihsiao]]

adding 1153671 to series file
renamed 1153671 -> 01-Bug_1153671___Part_1__Remove_non_standar.patch
applying 01-Bug_1153671___Part_1__Remove_non_standar.patch
unable to find 'suite/browser/test/browser/browser_bug581947.js' for patching
1 out of 1 hunks FAILED -- saving rejects to file suite/browser/test/browser/browser_bug581947.js.rej
unable to find 'suite/common/downloads/tests/browser/browser_nsISuiteDownloadManagerUI.js' for patching
1 out of 1 hunks FAILED -- saving rejects to file suite/common/downloads/tests/browser/browser_nsISuiteDownloadManagerUI.js.rej
unable to find 'suite/common/downloads/tests/chrome/test_action_keys_respect_focus.xul' for patching
1 out of 1 hunks FAILED -- saving rejects to file suite/common/downloads/tests/chrome/test_action_keys_respect_focus.xul.rej
unable to find 'suite/common/downloads/tests/chrome/test_basic_functionality.xul' for patching
2 out of 2 hunks FAILED -- saving rejects to file suite/common/downloads/tests/chrome/test_basic_functionality.xul.rej
unable to find 'suite/common/downloads/tests/chrome/test_clear_button_disabled.xul' for patching
2 out of 2 hunks FAILED -- saving rejects to file suite/common/downloads/tests/chrome/test_clear_button_disabled.xul.rej
unable to find 'suite/common/downloads/tests/chrome/test_delete_key_removes.xul' for patching
1 out of 1 hunks FAILED -- saving rejects to file suite/common/downloads/tests/chrome/test_delete_key_removes.xul.rej
unable to find 'suite/common/downloads/tests/chrome/test_drag.xul' for patching
1 out of 1 hunks FAILED -- saving rejects to file suite/common/downloads/tests/chrome/test_drag.xul.rej
unable to find 'suite/common/downloads/tests/chrome/test_multi_select.xul' for patching
2 out of 2 hunks FAILED -- saving rejects to file suite/common/downloads/tests/chrome/test_multi_select.xul.rej
unable to find 'suite/common/downloads/tests/chrome/test_multiword_search.xul' for patching
1 out of 1 hunks FAILED -- saving rejects to file suite/common/downloads/tests/chrome/test_multiword_search.xul.rej
unable to find 'suite/common/downloads/tests/chrome/test_removeDownload_updates_ui.xul' for patching
1 out of 1 hunks FAILED -- saving rejects to file suite/common/downloads/tests/chrome/test_removeDownload_updates_ui.xul.rej
unable to find 'suite/common/downloads/tests/chrome/test_search_keys.xul' for patching
1 out of 1 hunks FAILED -- saving rejects to file suite/common/downloads/tests/chrome/test_search_keys.xul.rej
unable to find 'suite/common/downloads/tests/chrome/test_select_all.xul' for patching
1 out of 1 hunks FAILED -- saving rejects to file suite/common/downloads/tests/chrome/test_select_all.xul.rej
unable to find 'suite/common/tests/browser/browser_354894.js' for patching
2 out of 2 hunks FAILED -- saving rejects to file suite/common/tests/browser/browser_354894.js.rej
unable to find 'suite/common/tests/browser/browser_456342.js' for patching
1 out of 1 hunks FAILED -- saving rejects to file suite/common/tests/browser/browser_456342.js.rej
unable to find 'suite/common/tests/browser/browser_615394-SSWindowState_events.js' for patching
1 out of 1 hunks FAILED -- saving rejects to file suite/common/tests/browser/browser_615394-SSWindowState_events.js.rej
unable to find 'suite/common/tests/chrome/test_idcheck.xul' for patching
2 out of 2 hunks FAILED -- saving rejects to file suite/common/tests/chrome/test_idcheck.xul.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 01-Bug_1153671___Part_1__Remove_non_standar.patch

Please help to fix the error, thanks.

Comment 44

[Tooru Fujisawa [:arai]]

the patches are for comm-central.

Comment 45

[Tooru Fujisawa [:arai]]

got approval via IRC. I'll push them

Comment 46

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/comm-central/rev/b039b691a388341b5f887e4341251cee55882d02
Bug 1153671 - Part 1: Remove non-standard for-each-in from tests. r=Ian,Ratty a=ewong

https://hg.mozilla.org/comm-central/rev/ff12858adf3e6c0aa8748848e427be9e532e4463
Bug 1153671 - Part 2: Remove non-standard for-each-in from remaining tests. r=Ratty a=ewong

https://hg.mozilla.org/comm-central/rev/e81f45a36c43a28b387bb089b4c7ab4e7f05d171
Bug 1153671 - Part 3: Remove non-standard for-each-in from suite/. r=Ian,Ratty a=ewong

Comment 47

[Frank-Rainer Grahl (:frg)]

Part 3 only Target 2.49.5
https://hg.mozilla.org/releases/comm-esr52/pushloghtml?changeset=6b30146aa4117616f2570fe192eaad1938fa5416