740237 - Use for...of loops in Add-ons Manager frontend and backend code

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

We should use this in Add-ons Manager frontend and backend code.

See bug 737792 for some fine examples.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

The relevant code is under /toolkit/mozapps/extensions/ and /toolkit/mozapps/extensions/content/

Comment 2

[Matt Brubeck (:mbrubeck)]

jphan1 is interested in working on this bug.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

jphan1: Have you made any progress on this bug? Is there anything I can help with or answer?

Comment 4

[Marcos S.]

jphan1 and I have been working on another bug. We should be able to get started on this very soon (a week). To clarify, just how big/small is the scope of this bug fix?

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Ok, great :)

The scope will be roughly the same as for bug 737792. The changes themselves should be relatively simple - have a look at the patches in bug 737792 for an idea of the type of changes needed. But it is a lot of small changes - about 80 in the backend code (ie, /toolkit/mozapps/extensions/) and around 20 in the frontend code (ie, /toolkit/mozapps/extensions/content/).

Comment 6

[Marcos S.]

Our patch should be post by tomorrow midnight.

Comment 7

[Marcos S.]

Would it be worth converting this loop to for..of? In other words, how aggressive should we be with what to convert to for..of?

1273     for (let i = 0; i < this.providers.length; i++) {
1274       if (callProvider(this.providers[i], "supportsMimetype", false, aMimetype) &&
1275           callProvider(this.providers[i], "isInstallEnabled"))
1276         return true;
1277     }
1278     return false;
1279   }

Comment 8

[Marcos S.]

Attachment: Changed the loops which would obviously benefit from for..of

jphan9 and I only made changes to for loops that seemed to obviously benefit from for..of.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 618937 [details] [diff] [review]
Changed the loops which would obviously benefit from for..of

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

(Apologies for the delay, I had to take some time off due to illness.)

This is great :) I hadn't expected to see all the changes in the tests too, so that's a nice surprise. This is making the code *much* nicer.

(In reply to Marcos S. from comment #7)
> Would it be worth converting this loop to for..of? In other words, how
> aggressive should we be with what to convert to for..of?

Yes. Any code that loops over an array (or anything array-like) should be considered for conversion to a for..of loop. I think that applies to the majority of existing for-loops in this code - in other words, be aggressive :) I remember seeing many more candidates for conversion in the main files (extensions.js, XPIProvider.jsm, etc) - about 20 in the frontend code (toolkit/mozapps/extensions/content), about 80 in the backend code (toolkit/mozapps/extensions), but I didn't look at the tests.

Some general notes:
* We use 2-spaces for indentation, not tabs as you've used here. https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide is a useful guide, though the indentation is the only style issue I noticed.
* Have you run the tests to confirm nothing broke accidentally? https://developer.mozilla.org/en/Browser_chrome_tests and https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests are the relevant test suites, and those pages have instructions on how to run them. I can also submit the patch to the TryServer (https://wiki.mozilla.org/Try) for you, which will run all the tests (but there will be a delay).
* There's a few cases here where the for..of loops are trying to iterate over a function - which won't work. For most of them, the function just maps to a data structure - you need to find that data structure and iterate over that. However, some of these cases should just not be converted.
* Should also look at extensions.xml - I remember seeing several candidates in there too.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +8295,5 @@
>     *         The key that contains the ID to path mapping
>     */
>    _readAddons: function RegInstallLocation__readAddons(aKey) {
> +
> +	for (let id of aKey.getValueName) {

This won't work, since aKey.getValueName is a function, not a data structure. This seems like a good candidate for not converting.

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +736,5 @@
>  
>    _processItemNodes: function(itemNodes, prefix, handler) {
>      var result = [];
>      var itemName = prefix + "Item";
> +	for (let blocklistElement of itemNodes.item) {

This won't work - itemNodes.item is a function, not a data structure. However, itemNodes is a data structure, and acts like an array.

Same goes for the other cases in this file where the original code was using .item(i)

::: toolkit/mozapps/extensions/test/browser/browser_bug562992.js
@@ +88,5 @@
>        let list = gManagerWindow.document.getElementById("addon-list");
>  
>        // To help prevent future breakage, don't assume the item is the only one
>        // in the list, or that it's first in the list.  Find it by name.
> +	  for (let item of list.getItemAtIndex) {

Same again, need to iterate over a data structure, in his case the children of list.

Comment 10

[Marcos S.]

Attachment: Version 2 of the first patch.

Undid inappropriate changes from the very first patch but also added many more changes.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 621914 [details] [diff] [review]
Version 2 of the first patch.

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

This is better, but the changes still break the Addons Manager quite noticeably. Have you run Firefox with these changes, and opened the Addons Manager to see if anything has broken? It's quite easy to build Firefox - https://developer.mozilla.org/En/Simple_Firefox_build

Also, the automated tests are quite broken too. As I mentioned in comment 9, you should try running these tests to see if any of them start failing. 

To run the XPCShell tests (https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests) for just the Addons Manager:
  make -C toolkit/mozapps/extensions/test/ xpcshell-tests

And to run the Browser-chrome tests (https://developer.mozilla.org/en/Browser_chrome_tests) for just the Addons Manager:
  TEST_PATH=toolkit/mozapps/extensions/test/ make mochitest-browser-chrome

This is assuming that you have a command line open at the directory where you've built Firefox. If you're still having issues building and/or running the tests, please join the #introduction channel on IRC (https://wiki.mozilla.org/IRC). I'm 'Unfocused' on IRC, but anyone will be able to help.

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +129,5 @@
>            // average ratings can be non-whole numbers, round them so they
>            // match to their closest star
>            rating = Math.round(rating);
> +          for (let star of stars)
> +            star.setAttribute("on", rating > i);

'i' is needed by this line - this is what's breaking the details view (the view that shows when you click the "more" link on an addon item).

@@ +140,5 @@
>            if (this.showRating != "user")
>              return;
>            var stars = this.stars;
> +          for (let star of stars)
> +            star.setAttribute("on", i <= (aScore -1));

Same.

::: toolkit/mozapps/extensions/content/xpinstallConfirm.js
@@ +59,5 @@
>    } catch (ex) { }
>    
>    var itemList = document.getElementById("itemList");
>    
> +  for (let installArg of args.installs) {

You've removed the numItemsToInstall variable here, but it's used later in this code (after the loop).

Although, this loop seems to just be broken, but I'm not sure why. It may just be a bug in for-of loops, since they're quite new - if you can't get it working (you can test by running the XPCShell tests), just revert back to the old code and let me know.

::: toolkit/mozapps/extensions/test/browser/browser_bug562890.js
@@ +27,5 @@
>    }]);
>    
>    open_manager("addons://list/extension", function(aManager) {
>      var addonList = aManager.document.getElementById("addon-list");
> +	for (let addonItem of addonList.childNodes) {

The purpose of this loop is to put the right item into the addonItem variable, which is used later in the test (outside of this loop). Because you've converted it to use 'let' instead of 'var', that variable isn't available outside of this loop. Using 'var' should fix this.

@@ +37,5 @@
>                                                                     "anonid",
>                                                                     "preferences-btn");
>      is(prefsBtn.hidden, true, "Prefs button should be hidden for addon with no optionsURL set")
>  
> +	for (let addonItem of addonList.childNodes)

This is missing an opening { bracket, resulting in a syntax error.

Additionally, this is used the same way as the loop I mentioned above. Removing 'let' should fix it.

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js
@@ +1085,5 @@
>  
>      readStringValue: function(aName) {
> +	  for (let currentValue of this.values) {
> +		if (currentValue.name == aName) {
> +		  return currentValue.value;

The if statement here has an opening { bracket, but no closing } bracket - causing a syntax error when running XPCShell tests.

Comment 12

[Marcos S.]

Attachment: Version 3 of a first patch.

I ran the xpc shell tests, but got 3 errors related to files we did not touch at all. The chrome tests also gave me errors I could not fix (even by reverting the changes). 

I'm not sure if it's just on my end or not, so I'm posting this patch to see if you also get the errors to give me to pointers on how I can fix it.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 622958 [details] [diff] [review]
Version 3 of a first patch.

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

Thanks Macros - you're getting there :)

I think fixing the issues below might fix more than one test. If you still get some test failures after fixing these issues, could you list which tests are failing? Also, which operating system you're running the tests on.

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2316,5 @@
>      this.removeInstall(aInstall);
>    },
>  
>    removeInstall: function(aInstall) {
> +	for (item of this._listBox.childNodes) {

Missing 'let' before 'item'.

This is what appears to be breaking the browser_install.js test.

::: toolkit/mozapps/extensions/test/browser/browser_bug562890.js
@@ +27,5 @@
>    }]);
>    
>    open_manager("addons://list/extension", function(aManager) {
>      var addonList = aManager.document.getElementById("addon-list");
> +    for (let addonItem of addonList.childNodes) {

As I mentioned in comment 11, this change (and the other change in this file) is breaking this test.

Using 'let' means 'addonItem' is only available within this loop. But the original code uses that variable outside the loop. Using 'var' instead of 'let' here, and removing 'let' in the loop you changed later in the file, will make that variable available to the whole function - and should fix the test.

Comment 14

[Marcos S.]

Attachment: Patch

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Marcos: I ran the test suites with your patch applied, but didn't see any of the failures you were getting. So I've pushed your patch to the TryServer just to be sure - that will run the test suites just like if your patch was committed to mozilla-central.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 625528 [details] [diff] [review]
Patch

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

Sorry, I meant to get to this much earlier.

I see you also fixed up the tabs-vs-spaces - fantastic work :) And all the tests are passing for me, and on Try as well.

What name would you like me to use as the patch author when I land this on mozilla-central? Just "Marcos S"? What about jphan?

Comment 17

[jphan9]

Either one is fine we both work on it

Comment 18

[Marcos S.]

If you can, Marcos Santiago and Jimmy Phan. Thanks. I think there are still changes I can make. It'll be easier to see what left once our changes are through.

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Landed on the Fx-team branch (with both of you as authors), which should get merged into mozilla-central within a day.

https://hg.mozilla.org/integration/fx-team/rev/254a734ccaf3

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Marcos S. from comment #18)
> I think there are still
> changes I can make. It'll be easier to see what left once our changes are
> through.

That would be great! It'll be easier to track further work in a separate bug (and you guys get to have your name on another bug), so I've filed bug 758950 to continue on with this work, and assigned it to you.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/254a734ccaf3