Closed Bug 1153671 Opened 5 years ago Closed 3 years ago

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

Categories

(SeaMonkey :: General, defect)

defect
Not set

Tracking

(seamonkey2.49esr fixed)

RESOLVED FIXED
seamonkey2.50
Tracking Status
seamonkey2.49esr --- fixed

People

(Reporter: iann_bugzilla, Assigned: waffles, Mentored)

References

()

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(3 files, 15 obsolete files)

19.28 KB, patch
arai
: review+
Details | Diff | Splinter Review
3.98 KB, patch
philip.chee
: review+
Details | Diff | Splinter Review
13.64 KB, patch
arai
: review+
Details | Diff | Splinter Review
Mentor: iann_bugzilla
Whiteboard: [good first bug][lang=js][mentor=IanN]
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?
Flags: needinfo?(iann_bugzilla)
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`)
(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.
Looks like mochitests are broken on c-c at the moment, so I need to chase getting those fixed.
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?
Whiteboard: [good first bug][lang=js][mentor=IanN] → [good first bug][lang=js]
(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.
mochitests can run locally now, so any issues, just say.
Flags: needinfo?(iann_bugzilla)
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?
Flags: needinfo?(iann_bugzilla)
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.
Flags: needinfo?(iann_bugzilla)
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.
Flags: needinfo?(iann_bugzilla)
Attached patch for_each (obsolete) — Splinter Review
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.
Attachment #8607196 - Flags: review?(iann_bugzilla)
(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.
[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.
Assignee: nobody → wafflespeanut
Status: NEW → ASSIGNED
Flags: needinfo?(iann_bugzilla)
Attachment #8607196 - Flags: review?(iann_bugzilla) → review+
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.
Flags: needinfo?(iann_bugzilla)
[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 :)
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
Flags: needinfo?(iann_bugzilla) → needinfo?(wafflespeanut)
Attached patch working_tests (obsolete) — Splinter Review
Attachment #8607196 - Attachment is obsolete: true
Flags: needinfo?(wafflespeanut)
Attachment #8625734 - Flags: review?(iann_bugzilla)
15 tests seem to work. More patches on the way.
Attached patch failing_test (obsolete) — Splinter Review
Only 1 test actually fails (when ran using mochitests).
Attachment #8625736 - Flags: review?(iann_bugzilla)
Attached patch freezing_tests (obsolete) — Splinter Review
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).
Attachment #8625737 - Flags: review?(iann_bugzilla)
Attached patch common and mailnews code (obsolete) — Splinter Review
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.
Attachment #8625740 - Flags: review?(iann_bugzilla)
Attached patch sync_notification (obsolete) — Splinter Review
This is our last remaining test. We've never tested this beast :)
Attachment #8625741 - Flags: review?(iann_bugzilla)
Comment on attachment 8625734 [details] [diff] [review]
working_tests

r=me and a=me for CLOSED TREE tests only
Attachment #8625734 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 8625740 [details] [diff] [review]
common and mailnews code

r=me and a=me for CLOSED TREE
Attachment #8625740 - Attachment description: unsure_tests → common and mailnews code
Attachment #8625740 - Flags: review?(iann_bugzilla) → review+
any progress?
all patches are still applicable with simple rebase.
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`.
taking over, to disable for each on nightly.

rebased and fixed some errors.
(fixed files are in next patch)
Attachment #8625734 - Attachment is obsolete: true
Attachment #8783037 - Flags: review+
sorry, it was wrong file
Attachment #8783037 - Attachment is obsolete: true
Attachment #8783039 - Flags: review+
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`.
Attachment #8783041 - Flags: review?(iann_bugzilla)
savedFormData is an object, so we need to use `Object.values` to iterate over values.
Attachment #8783043 - Flags: review?(iann_bugzilla)
No change from previous one.
I don't see any issue in this change.
If the test still freezes, it should be different issue.
Attachment #8625736 - Attachment is obsolete: true
Attachment #8625737 - Attachment is obsolete: true
Attachment #8625736 - Flags: review?(iann_bugzilla)
Attachment #8625737 - Flags: review?(iann_bugzilla)
Attachment #8783046 - Flags: review?(iann_bugzilla)
fix for "unsure tests".
applied same fix as m-c.
Attachment #8783049 - Flags: review?(iann_bugzilla)
not changed.
I don't see any issue with it.
Attachment #8783050 - Flags: review?(iann_bugzilla)
Attachment #8625741 - Attachment is obsolete: true
Attachment #8625741 - Flags: review?(iann_bugzilla)
Depends on: 1293305
See Also: → 1319399
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?
Attachment #8783041 - Flags: review?(iann_bugzilla) → review+
Attachment #8783043 - Flags: review?(iann_bugzilla) → review+
Attachment #8783049 - Flags: review?(iann_bugzilla) → review+
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/
Attachment #8783050 - Flags: review?(iann_bugzilla) → review+
Duplicate of this bug: 1319399
Thank you for reviewing.
Updated and folded patches.
Attachment #8783039 - Attachment is obsolete: true
Attachment #8783041 - Attachment is obsolete: true
Attachment #8783043 - Attachment is obsolete: true
Attachment #8813304 - Flags: review+
Attachment #8783046 - Attachment is obsolete: true
Attachment #8783046 - Flags: review?(iann_bugzilla)
Attachment #8813306 - Flags: review?(philip.chee)
Attachment #8783048 - Attachment is obsolete: true
Attachment #8783049 - Attachment is obsolete: true
Attachment #8783050 - Attachment is obsolete: true
Attachment #8813307 - Flags: review+
Attachment #8813306 - Flags: review?(philip.chee) → review+
Thanks :D
Keywords: checkin-needed
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.
Flags: needinfo?(arai.unmht)
Keywords: checkin-needed
the patches are for comm-central.
Flags: needinfo?(arai.unmht)
Keywords: checkin-needed
got approval via IRC. I'll push them
Keywords: checkin-needed
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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.