737792 - Use for..of loops in browser.js

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

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 browser.js.

Useful queries:
http://mxr.mozilla.org/mozilla-central/search?string=for+%28&find=base%2Fcontent%2Fbrowser.js&filter=\%2B\%2B
http://mxr.mozilla.org/mozilla-central/search?string=for+%28&find=base%2Fcontent%2Fbrowser.js&filter=+in+
http://mxr.mozilla.org/mozilla-central/search?string=for+each+%28&find=base%2Fcontent%2Fbrowser.js
http://mxr.mozilla.org/mozilla-central/search?string=.forEach%28&find=base%2Fcontent%2Fbrowser.js

Some of these instances might not make sense to be rewritten. This needs to be decided case-by-case.

Comment 1

[Ian Moody [:Kwan] (UTC+0)]

Just commenting to say I'm taking this, and I've already got patches for all the ones in the first list that make sense (to me).

Comment 2

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Patch for already nearly for of

This patch handles the areas that are basically already doing this anyway by instantly setting a variable to var[i]

Comment 3

[Ian Moody [:Kwan] (UTC+0)]

Comment on attachment 608148 [details] [diff] [review]
Patch for already nearly for of

oops, forgot to do the final check for prior declarations, sorry

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 608148 [details] [diff] [review]
Patch for already nearly for of

> function switchToTabHavingURI(aURI, aOpenNew) {
>   // This will switch to the tab in aWindow having aURI, if present.
>   function switchIfURIInWindow(aWindow) {
>     let browsers = aWindow.gBrowser.browsers;
>-    for (let i = 0; i < browsers.length; i++) {
>-      let browser = browsers[i];
>+    for (let browser of browsers) {

nit: for (let browser of aWindow.gBrowser.browsers) {

Comment 5

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Patch for already nearly for of

This patch handles the areas that are basically already doing this anyway by instantly setting a variable to var[i]

Comment 6

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Patch for 2-line changes

This patch is for loops that only use the variable once

Comment 7

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Patch for already nearly for of

Address code simplification nit and a similar instance

Comment 8

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Patch for 2-line changes

Remove unneeded variable creation as in change to previous patch

Comment 9

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Patch for fairly simple changes

These are changes that are longer than two lines but still pretty simple

Comment 10

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Patch for more complex changes

More complex multi-line changes

Comment 11

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Patch for for...in changes

This deals with the for...in changes, of which there only seemed to be a couple that were changable

Comment 12

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Patch for for each...in change

This deals with the for each...in change

Comment 13

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Patch for forEach method changes

This deals with the forEach methods, and might be missing some right now since I'm not sure how aggressive to be with ones that set a thisArg

Comment 14

[Ian Moody [:Kwan] (UTC+0)]

Another query:
http://mxr.mozilla.org/mozilla-central/search?string=for+%28&find=base%2Fcontent%2Fbrowser.js&filter=\-\-

Unless I'm mistaken only the middle one can't be changed.

Comment 15

[Ian Moody [:Kwan] (UTC+0)]

(In reply to Ian Moody (:Kwan) from comment #14)
> Another query:
> http://mxr.mozilla.org/mozilla-central/
> search?string=for+%28&find=base%2Fcontent%2Fbrowser.js&filter=\%2D\%2D
> 
> Unless I'm mistaken only the middle one can't be changed.

Nevermind, the last one definitely doesn't work, just horks the toolbar menu.  The first one doesn't seem to do anything bad, but maybe I'm just testing the wrong place.  Either way I'll just leave these ones.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 608187 [details] [diff] [review]
Patch for already nearly for of

perfect!

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 608193 [details] [diff] [review]
Patch for fairly simple changes

>   updateStatusField: function () {
>     var text, type, types = ["overLink"];
>     if (this._busyUI)
>       types.push("status");
>     types.push("jsStatus", "jsDefaultStatus", "defaultStatus");
>-    for (let i = 0; !text && i < types.length; i++) {
>-      type = types[i];
>+    for (type of types) {
>+      if (text)
>+        break;
>       text = this[type];
>     }

this makes more sense:

      text = this[type];
      if (text)
        break;

> function WindowIsClosing()
> {
>   if (TabView.isVisible()) {
>     TabView.hide();
>     return false;
>   }
> 
>-  var reallyClose = closeWindow(false, warnAboutClosingWindow);
>-  if (!reallyClose)
>+  if (!closeWindow(false, warnAboutClosingWindow))
>     return false;
> 
>-  var numBrowsers = gBrowser.browsers.length;
>-  for (let i = 0; reallyClose && i < numBrowsers; ++i) {
>-    let ds = gBrowser.browsers[i].docShell;
>+  for (let browser of gBrowser.browsers) {
>+    let ds = browser.docShell;
> 
>     if (ds.contentViewer && !ds.contentViewer.permitUnload())
>-      reallyClose = false;
>-  }
>-
>-  return reallyClose;
>+      return false;
>+  }
> }

need to keep returning 'true' at the end of the function

r+ with these changes -- I'll make them before landing this.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 608194 [details] [diff] [review]
Patch for more complex changes

>+            el.setAttribute("saved-iconsize",
>+                                el.getAttribute("iconsize"));

>+          el.setAttribute("saved-context",
>+                              el.getAttribute("context"));

nit: Indentation is off. Also, these fit on one line each.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 608200 [details] [diff] [review]
Patch for forEach method changes

>-      installInfo.installs.forEach(function(aInstall) {
>+      for (let install of installInfo.installs) {
>         var host = (installInfo.originatingURI instanceof Ci.nsIStandardURL) &&
>                    installInfo.originatingURI.host;

>-        var error = (host || aInstall.error == 0) ? "addonError" : "addonLocalError";

>+        var error = (host || install.error == 0) ? "addonError" : "addonLocalError";

>-  toolbarNodes.forEach(function(toolbar) {
>+  for (let toolbar of toolbarNodes) {
>     var toolbarName = toolbar.getAttribute("toolbarname");

'var' should be changed to 'let' in these cases

Comment 20

[Dão Gottwald [:dao]]

I found some problems when testing this. 'frames' is just an alias for 'window', which isn't really iterable. 'styleSheets' isn't iterable either, but it should be. I'll file a bug on this. 'options' is iterable but contains bogus values. I'll file a bug on this as well.

For now I'll revert those changes and land the rest.

Comment 21

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/378712300ad2

Comment 22

[Dão Gottwald [:dao]]

> 'options' is iterable but contains bogus values.

It's possible that this is just sometimes broken but wasn't really a problem with this patch. Filed as bug 738193.

Comment 23

[Dão Gottwald [:dao]]

> 'styleSheets' isn't iterable either,

bug 738196

Comment 24

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #22)
> > 'options' is iterable but contains bogus values.
> 
> It's possible that this is just sometimes broken but wasn't really a problem
> with this patch. Filed as bug 738193.

Actually I think it does affect this patch, and I may have to back it out since I only reverted the 'options' case but not 'childNodes'...

Comment 25

[Dão Gottwald [:dao]]

Yeah, the childNodes change in FillInHTMLTooltip is affected. browser_bug329212.js caught this.

browser_tabview_bug589324.js also exposes a second problem: "an unexpected uncaught JS exception reported through window.onerror - i is not defined at chrome://browser/content/browser.js:12091"

-> backed out

Comment 26

[Ian Moody [:Kwan] (UTC+0)]

Ah I think this has also thrown up another small bug in browser_bug329212.js, half the failure messages are empty, which appears to be down to misplaced brackets in the ok()s.  Will fix that too.

Comment 27

[Ian Moody [:Kwan] (UTC+0)]

(In reply to Ian Moody (:Kwan) from comment #26)
> Ah I think this has also thrown up another small bug in
> browser_bug329212.js, half the failure messages are empty, which appears to
> be down to misplaced brackets in the ok()s.  Will fix that too.

Filed as Bug 738233.

Comment 28

[Ian Moody [:Kwan] (UTC+0)]

(In reply to Dão Gottwald [:dao] from comment #25)> 
> browser_tabview_bug589324.js also exposes a second problem: "an unexpected
> uncaught JS exception reported through window.onerror - i is not defined at
> chrome://browser/content/browser.js:12091"

While I find the error message confusing and not relatable to any code, the line number would seem to indicate the index I missed in this line:
aWindow.gBrowser.tabContainer.selectedIndex = i;
oops, sorry.

Looking with the DOM Inspector it seems like browser._tPos should replace the i, is that correct?

Comment 29

[Dão Gottwald [:dao]]

(In reply to Ian Moody (:Kwan) from comment #28)
> While I find the error message confusing and not relatable to any code, the
> line number would seem to indicate the index I missed in this line:
> aWindow.gBrowser.tabContainer.selectedIndex = i;
> oops, sorry.
> 
> Looking with the DOM Inspector it seems like browser._tPos should replace
> the i, is that correct?

browsers don't have a _tPos property. You could iterate over gBrowser.tabs rather than gBrowser.browsers, but you could also just leave this loop alone...

Comment 30

[Dão Gottwald [:dao]]

Ian, do you want to create a patch with the changes that are good to land? We could file new bugs for the changes blocked by bug 738193 and bug 738196.

Comment 31

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Patch of safe changes

This should be all the safe changes. Going to request a try run on IRC to make sure nothing is broken.

Comment 32

[Mozilla RelEng Bot]

Try run for 163b63a532de is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=163b63a532de
Results (out of 164 total builds):
    success: 136
    warnings: 28
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-163b63a532de

Comment 33

[Ian Moody [:Kwan] (UTC+0)]

Comment on attachment 613111 [details] [diff] [review]
Patch of safe changes

Passed try (with some noise from intermittent oranges).

Comment 34

[Dão Gottwald [:dao]]

Comment on attachment 613111 [details] [diff] [review]
Patch of safe changes

Why did you revert the BrowserCustomizeToolbar and switchToTabHavingURI changes? I think they should work.

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 613111 [details] [diff] [review]
Patch of safe changes

see previous comment

Comment 36

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #34)
> Why did you revert the BrowserCustomizeToolbar and switchToTabHavingURI
> changes? I think they should work.

Ian, are you going to address this?

Comment 37

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #21)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/378712300ad2

Re-landed with comment 29 addressed.

http://hg.mozilla.org/integration/mozilla-inbound/rev/8f9cac092f38

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8f9cac092f38