Continuation of the work done in bug 740237 to convert loops in the Add-ons Manager frontend and backend code to use for...of loops. for..of is a new ES6 feature for iterating over arrays and array-like objects: https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...of See bug 740237 and bug 737792 for some fine examples.

Have you had a chance to work on this, Marcos? Anything I can help with?

These are the only changes I thought were possible for those candidates remaining in the extensions/content folder. I'm getting errors running the tests on my end, but I don't know why. Might just be on my end, so I attached the patch file.

(In reply to Marcos S. from comment #2) > I'm getting errors running the tests on my end, but I don't know why. What kind of errors? Also, are you remembering to not switch to another application/window, and not touch your keyboard or mouse while the tests are running?

I get errors running the XPC Shell tests. I made sure to have no windows open and made sure to not use keyboard/mouse while the test ran.

(In reply to Marcos S. from comment #4) > Created attachment 633773 [details] > Part 1, v1 - XPCShell tests output > > I get errors running the XPC Shell tests. I made sure to have no windows > open and made sure to not use keyboard/mouse while the test ran. Ah, I assumed you meant browser-chrome tests. Changes in /toolkit/mozapps/extensions/content can't affect XPCShell tests, since they don't open the UI. Which also means you can use your computer while running XPCshell tests, unlike other types of tests. What I think it happening is that there's a test addon that has been left in ./dist/bin/extensions/ (in your objdir). Probably from a previous test run that failed, or was killed. I get this occasionally too, it's frustrating :\ Just deleting that extra addon file should fix the tests - it should be a file named addon4@tests.mozilla.org.xpi.

Comment on attachment 633407 [details] [diff] [review] Part 1, v1 - extensions/content Review of attachment 633407 [details] [diff] [review]: ----------------------------------------------------------------- There's also some cases where we use the following pattern, which should also be converted (can be in another patch): array.forEach(function(arrayItem) { do_something(); }); (Converting those should speed up the code a tiny bit too.) ::: toolkit/mozapps/extensions/content/extensions.js @@ +2651,5 @@ > !gViewController.commands.cmd_showItemPreferences.isEnabled(aAddon); > > var gridRows = document.querySelectorAll("#detail-grid rows row"); > + var first = true; > + for (var gridRow of gridRows) { Use 'let' here. (Pretty much always want to use 'let', unless it breaks something.) ::: toolkit/mozapps/extensions/content/xpinstallConfirm.js @@ +28,5 @@ > > var itemList = document.getElementById("itemList"); > > var numItemsToInstall = args.installs.length; > + for (var installs of args.installs) { Use 'let' here. Also, the variable should be named 'install', since it represents just one install.

Patch containing all changes so far since there aren't many of them. I don't know why the Chrome tests are failing for me, but I have attached the log file as well.

The Chrome tests log pertaining to the second patch.

Comment on attachment 639585 [details] [diff] [review] Part 1, v2 (This is more like Part 1 v2, since it covers the files in the original patch for Part 1)

Comment on attachment 639585 [details] [diff] [review] Part 1, v2 Review of attachment 639585 [details] [diff] [review]: ----------------------------------------------------------------- Great - thanks Macros :) The problems with the tests are because the xpinstallConfirm dialog's code is old and crusty :( You'll notice that there's no other use of 'let' in xpinstallConfirm.js - it's not using the newer language features of JS. So in xpinstallConfirm.xul, look for where xpinstallConfirm.js is loaded, and add: type="application/javascript" to the <script> tag. Have you had a chance to look at the cases of .forEach( ? ::: toolkit/mozapps/extensions/test/browser/browser_bug557956.js @@ +162,5 @@ > } > > function get_list_names(aList) { > var items = []; > + for (let aListItem of aList.childNodes) We prefix variable names with 'a' only if they're function arguments. So 'aListItem' here should be just 'listItem'. @@ +312,5 @@ > is(items[1], "Addon8 2.0", "Should have seen update for addon8"); > is(items[2], "Addon9 2.0", "Should have seen update for addon9"); > > // Unheck the ones we don't want to install > + for (listItem of list.childNodes) { Missing 'let' statement before 'listItem'.

Macros: Any progress since we last spoke?

I'm having unsuccessful builds with this patch even though it's very similar to Part 1, v2.

Comment on attachment 647380 [details] The warning I get in the console when building Part 1, v3. Nothing to worry about in this screenshot, that's expected stuff.

Comment on attachment 647383 [details] This is the build output/log for Part 1, v3. This is after performing a "clobber build" as well as redownloading a fresh repository. Hmm, I don't know why this would happen. Could you get on IRC and ask in #developers if anyone knows?

Comment on attachment 647379 [details] [diff] [review] Part 1, v3 Review of attachment 647379 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/AddonRepository.jsm @@ +1027,4 @@ > let name = self._getDescendantTextContent(aAuthorNode, "name"); > let link = self._getDescendantTextContent(aAuthorNode, "link"); > if (name == null || link == null) > return; Since this is now a normal block, instead of a function, you need to change this return; to a continue; so it continues with the next iteration, rather than completely exiting _parseAddon(). With that fixed, I see all tests passing. Also, variable names in the format aWhatever are for function parameters. So aAuthorNode should be renamed to authorNode. Both of these issues also apply to previewNodes below.

Updated version of Part 1, v3 to fix variable names and replace "return;" with "continue;"

Comment on attachment 648931 [details] [diff] [review] Part 1, v4 Review of attachment 648931 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/AddonRepository.jsm @@ +1029,2 @@ > if (name == null || link == null) > + ocntinue; Typo!

Typo fixed from Part 1, v4

Comment on attachment 649207 [details] [diff] [review] Part 1, v5 Review of attachment 649207 [details] [diff] [review]: ----------------------------------------------------------------- All good!

Pushed Part 1 the fx-team branch, to avoid accumulating bitrot: https://hg.mozilla.org/integration/fx-team/rev/16a8b66f1503 (Note to whoever merges: Please leave this bug open, we're not done here yet.)

Looks like something went wrong: all M1s are orange on https://tbpl.mozilla.org/?tree=Fx-Team&rev=16a8b66f1503

Sorry, had to back out for mochitest-1 failures in test_bug292789.html: https://tbpl.mozilla.org/?tree=Fx-Team&rev=16a8b66f1503 { 1241 ERROR TEST-UNEXPECTED-FAIL | /tests/caps/tests/mochitest/test_bug292789.html | an unexpected uncaught JS exception reported through window.onerror - Script error. at resource://gre/chrome/toolkit/content/mozapps/xpinstall/xpinstallConfirm.js:0 1242 ERROR TEST-UNEXPECTED-FAIL | /tests/caps/tests/mochitest/test_bug292789.html | xpinstallConfirm.js has not moved unexpectedly - got undefined, expected object } https://hg.mozilla.org/integration/fx-team/rev/a15f73561f76

Well that was unexpected. And I ended up fixing this as I investigated, so here we are. test_bug292789.html loads /toolkit/mozapps/extensions/xpinstallConfirm.js - which in this bug we're changing to use newer JS features, so loading it as an old JS version now fails. This makes that test load it as JS 1.8.

(In reply to Blair McBride (:Unfocused) from comment #25) > This makes that test load it as JS 1.8. Forgot to mention: Changed the <script> from being dynamically created to being static because apparently that's the only way the "type" attribute will affect the JS version.

> - <script src="chrome://mozapps/content/xpinstall/xpinstallConfirm.js"></script> > + <script src="chrome://mozapps/content/xpinstall/xpinstallConfirm.js" type="application/javascript;version=1.8"></script> Nit, since you're changing this line anyway, put the type first?

Comment on attachment 649616 [details] [diff] [review] Test fix, v1 Bouncing review in hopes of seeing a r+ sooner. (Assume I've done comment 27.)

Comment on attachment 649616 [details] [diff] [review] Test fix, v1 r=dveditz

Comment on attachment 653217 [details] [diff] [review] Part 2, v1 - extensions/content Part 2 consists of changes of forEach to for..of

Test fix: https://hg.mozilla.org/integration/fx-team/rev/866eba872722 Part 1: https://hg.mozilla.org/integration/fx-team/rev/67c042561a37

Comment on attachment 653217 [details] [diff] [review] Part 2, v1 - extensions/content Review of attachment 653217 [details] [diff] [review]: ----------------------------------------------------------------- Since you're converting from using a function to just a normal loop, the variable names should be updated too. The "a" at the start of "aAddon" is a convention to show that that variable was passed into the function, so should be rewritten now as just "addon". Also, there are a few cases where there's just one line in the loop - usually we don't use { } brackets in those cases (they were needed for the functions, but not anymore). ::: toolkit/mozapps/extensions/content/extensions.js @@ +1410,5 @@ > items.push(listitem.mAddon); > listitem = listitem.nextSibling; > } > > + for (let aAddon of items) { aAddon.uninstall(); } Split this into two lines.

https://hg.mozilla.org/mozilla-central/rev/866eba872722 https://hg.mozilla.org/mozilla-central/rev/67c042561a37

Fixes variable names and braces issue of Part 2, v1.

Comment on attachment 653591 [details] [diff] [review] Part 2a, v2 - extensions/content Review of attachment 653591 [details] [diff] [review]: ----------------------------------------------------------------- There's a few cases here where the original code was something like this: myArray.forEach(function(aItem) { if (condition) return; ... }); You need to convert that return so it only breaks out of the loop now, not the entire function.

Comment on attachment 653591 [details] [diff] [review] Part 2a, v2 - extensions/content Part 2 pertains to forEach conversions. Part 2a refers to changes in the extensions/content folder.

Do you have updated patches you could upload here, Marcos? I saw on IRC you were having issues with tests again.

I was playing around with the return; to continue statements. It seems some of the tests within the Chrome Tests weren't stopping due to an infinite loop. Though when I changed the continue;'s to break;'s, the infinite looping stopped. There might be other issues I didn't catch while reviewing the changes. I ran into the same issues with Part 2b, v1.

Comment on attachment 658356 [details] [diff] [review] Part 2a, v3 - extensions/content Review of attachment 658356 [details] [diff] [review]: ----------------------------------------------------------------- You should consider making a small set of changes, running the tests, and if they pass make some more changes. It makes it much easier to track down why tests are failing. ::: toolkit/mozapps/extensions/content/extensions.js @@ +278,5 @@ > _installListeners: [], > > initialize: function gEM_initialize() { > var self = this; > + let events = ["onEnabling", "onEnabled", "onDisabling", "onDisabled", "onUninstalling", We usually use const for variables like these. eg, https://hg.mozilla.org/mozilla-central/file/e5bffe88e184/toolkit/mozapps/extensions/content/extensions.js#l1242 So this would end up being named something like ADDON_EVENTS, and the one below INSTALL_EVENTS. (Same applies to all 4 cases of these in this patch.) @@ +284,5 @@ > + "onUpdateAvailable", "onUpdateFinished", "onCompatibilityUpdateAvailable", > + "onPropertyChanged"]; > + for (let event of events) { > + self[event] = function initialize_delegateAddonEvent(...aArgs) { > + self.delegateAddonEvent(event, aArgs); This (and the other array of events below) is what is causing a lot of your issues. This is a subtle issue that has to do with how JavaScript scoping and closures work. When you have code like this: for (let event of events) { self[event] = function someFunc() { doSomething(event); } } Setting |object[item]| works as you'd expect - the value of |item| changes for each iteration of the for-of loop. However, since the loop is re-using the |item| variable, you have to be careful with how you use it in closures (ie, re-using it in the someFunc function). Since someFunc() is called sometime *after* the loop finishes, the value of |item| has changed - to whatever value it had in the last iteration of the loop. To get around that, you can use a new variable that is always re-created for every iteration of the loop: for (let evt of events) { let event = evt; self[event] = function someFunc() { doSomething(event); } } @@ +845,1 @@ > AddonManager.UPDATE_WHEN_USER_REQUESTED); Nit: Fix the indentation of the second line. @@ +1790,3 @@ > try { > if (!Services.prefs.getBoolPref(prefName)) > + break; This should be a continue @@ +2070,2 @@ > if (score == 0) > + break; This needs to be a continue - this is what is breaking browser_bug557943.js, browser_bug562797.js, maybe others. @@ +2744,5 @@ > if (pending != AddonManager.PENDING_NONE) { > this.node.removeAttribute("notification"); > > var pending = null; > + let ops = ["enable", "disable", "install", "uninstall", "upgrade"]; PENDING_OPERATIONS @@ +3037,5 @@ > var elements = []; > let threshold = Date.now() - UPDATES_RECENT_TIMESPAN; > + for (let addon of aAddonsList) { > + if (!addon.updateDate || addon.updateDate.getTime() < threshold) > + break; Needs to be a continue @@ +3077,5 @@ > var elements = []; > > + for (let install of aInstallsList) { > + if (!self.isManualUpdate(install)) > + break; Needs to be a continue

Comment on attachment 658357 [details] [diff] [review] Part 2b, v1 - extensions/test/browser/ Review of attachment 658357 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/test/browser/browser_bug567127.js @@ +83,5 @@ > function test_confirmation(aWindow, aExpectedURLs) { > var list = aWindow.document.getElementById("itemList"); > is(list.childNodes.length, aExpectedURLs.length, "Should be the right number of installs"); > > + for (let url of aExpectedURLs) { Same problem here as in browser_dragdrop.js (see below for an explanation). ::: toolkit/mozapps/extensions/test/browser/browser_checkAddonCompatibility.js @@ +21,3 @@ > // Ignore plugins. > if (a.type == "plugin") > + break; This needs to be a continue ::: toolkit/mozapps/extensions/test/browser/browser_dragdrop.js @@ +109,5 @@ > var node = list.firstChild; > while (node) { > + if (node.url == url) { > + ok(true, "Should have seen " + url + " in the list"); > + continue; This is causing the infinite loop because this while() is now a nested loop inside the for-of loop - previously the "return" worked to break out of the while loop *and* continue on the next iteration of the forEach(). Now it only affects the while() loop. To fix it, this block of code should be rewritten to be something like for (let url of expected) { let found = false; for (let node of list.children) { if (node.url == url) { found = true; break; } } ok(found, "whatever"); } ::: toolkit/mozapps/extensions/test/browser/browser_select_selection.js @@ +104,5 @@ > Services.prefs.setBoolPref("extensions.installedDistroAddon.test15@tests.mozilla.org", true); > > + for (let pos in ADDONS) { > + let addonItem = ADDONS[pos]; > + var addon = new MockAddon("test" + pos + "@tests.mozilla.org", This test is failing due to the same reason you were having trouble with the events arrays in extensions.js, due to how scoping and closures work in JS. To fix it, just change the |var addon| to |let addon| - that will make it so the findUpdates function will keep the value of |addon| that it expects, since it will be a new variable for each iteration of the loop. ::: toolkit/mozapps/extensions/test/browser/head.js @@ +71,5 @@ > > +for (let pref of gRestorePrefs) { > + if (!Services.prefs.prefHasUserValue(pref.name)) { > + pref.type = "clear"; > + break; This needs to be continue @@ +123,5 @@ > // it returns so this will complete before the next test start. > AddonManager.getAllInstalls(function(aInstalls) { > + for (let install of aInstalls) { > + if (install instanceof MockInstall) > + break; Ditto

All tests passed on my end.

Comment on attachment 661937 [details] [diff] [review] Part 2b, v2 - extensions/test/browser/ Review of attachment 661937 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Glad you got the tests running smoothly again on your end :)

I ran into errors running the Chrome Tests on one machine while I ran into zero (minus the errors from a bug found earlier) to two errors running on another machine. I'm not sure if the patch is at fault or my setup.

Comment on attachment 663649 [details] [diff] [review] Part 2a, v4 - extensions/content Review of attachment 663649 [details] [diff] [review]: ----------------------------------------------------------------- Tests are passing for me. You might want to try doing a clobber build - deleting your obj dir, and rebuilding again. On your other machine, if you're only sometimes getting a failure, it's probably an existing intermittent failure. Just one nit, which I'll fix up when I land this: ::: toolkit/mozapps/extensions/content/extensions.js @@ +280,5 @@ > initialize: function gEM_initialize() { > var self = this; > + const ADDON_EVENTS = ["onEnabling", "onEnabled", "onDisabling", "onDisabled", "onUninstalling", > + "onUninstalled", "onInstalled", "onOperationCancelled", > + "onUpdateAvailable", "onUpdateFinished", "onCompatibilityUpdateAvailable", Nit: indenting/wrapping. Ideally, the lines should be no more than 80 characters long, and the wrapped lines should start lines up with the relevant part of the first line (in this case, the first character of the first element of the array).

https://hg.mozilla.org/integration/fx-team/rev/4eb96cc64a6e https://hg.mozilla.org/integration/fx-team/rev/cf12bddd0b41

https://hg.mozilla.org/mozilla-central/rev/4eb96cc64a6e https://hg.mozilla.org/mozilla-central/rev/cf12bddd0b41

Calling this closed, and opened bug 830592 for a new owner to handle anything leftover.