Last Comment Bug 737792 - Use for..of loops in browser.js
: Use for..of loops in browser.js
Status: RESOLVED FIXED
[good first bug][mentor=dao][lang=js]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Ian Moody [:Kwan]
:
Mentors:
Depends on: 738193 738196
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-21 03:19 PDT by Dão Gottwald [:dao]
Modified: 2012-08-21 19:09 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for already nearly for of (5.07 KB, patch)
2012-03-21 17:02 PDT, Ian Moody [:Kwan]
no flags Details | Diff | Review
Patch for already nearly for of (6.04 KB, patch)
2012-03-21 17:14 PDT, Ian Moody [:Kwan]
no flags Details | Diff | Review
Patch for 2-line changes (7.91 KB, patch)
2012-03-21 17:17 PDT, Ian Moody [:Kwan]
no flags Details | Diff | Review
Patch for already nearly for of (6.09 KB, patch)
2012-03-21 19:12 PDT, Ian Moody [:Kwan]
dao+bmo: review+
Details | Diff | Review
Patch for 2-line changes (8.01 KB, patch)
2012-03-21 19:20 PDT, Ian Moody [:Kwan]
dao+bmo: review+
Details | Diff | Review
Patch for fairly simple changes (6.75 KB, patch)
2012-03-21 19:23 PDT, Ian Moody [:Kwan]
dao+bmo: review+
Details | Diff | Review
Patch for more complex changes (6.63 KB, patch)
2012-03-21 19:25 PDT, Ian Moody [:Kwan]
dao+bmo: review+
Details | Diff | Review
Patch for for...in changes (2.03 KB, patch)
2012-03-21 19:27 PDT, Ian Moody [:Kwan]
dao+bmo: review+
Details | Diff | Review
Patch for for each...in change (1.16 KB, patch)
2012-03-21 19:29 PDT, Ian Moody [:Kwan]
dao+bmo: review+
Details | Diff | Review
Patch for forEach method changes (8.63 KB, patch)
2012-03-21 19:31 PDT, Ian Moody [:Kwan]
dao+bmo: review+
Details | Diff | Review
Patch of safe changes (30.31 KB, patch)
2012-04-07 08:27 PDT, Ian Moody [:Kwan]
no flags Details | Diff | Review

Comment 1 Ian Moody [:Kwan] 2012-03-21 15:15:09 PDT
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] 2012-03-21 17:02:53 PDT
Created attachment 608148 [details] [diff] [review]
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] 2012-03-21 17:06:32 PDT
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] 2012-03-21 17:08:37 PDT
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] 2012-03-21 17:14:17 PDT
Created attachment 608153 [details] [diff] [review]
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] 2012-03-21 17:17:59 PDT
Created attachment 608155 [details] [diff] [review]
Patch for 2-line changes

This patch is for loops that only use the variable once
Comment 7 Ian Moody [:Kwan] 2012-03-21 19:12:44 PDT
Created attachment 608187 [details] [diff] [review]
Patch for already nearly for of

Address code simplification nit and a similar instance
Comment 8 Ian Moody [:Kwan] 2012-03-21 19:20:09 PDT
Created attachment 608192 [details] [diff] [review]
Patch for 2-line changes

Remove unneeded variable creation as in change to previous patch
Comment 9 Ian Moody [:Kwan] 2012-03-21 19:23:23 PDT
Created attachment 608193 [details] [diff] [review]
Patch for fairly simple changes

These are changes that are longer than two lines but still pretty simple
Comment 10 Ian Moody [:Kwan] 2012-03-21 19:25:02 PDT
Created attachment 608194 [details] [diff] [review]
Patch for more complex changes

More complex multi-line changes
Comment 11 Ian Moody [:Kwan] 2012-03-21 19:27:35 PDT
Created attachment 608196 [details] [diff] [review]
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] 2012-03-21 19:29:11 PDT
Created attachment 608198 [details] [diff] [review]
Patch for for each...in change

This deals with the for each...in change
Comment 13 Ian Moody [:Kwan] 2012-03-21 19:31:18 PDT
Created attachment 608200 [details] [diff] [review]
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] 2012-03-21 19:36:02 PDT
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] 2012-03-21 20:49:25 PDT
(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] 2012-03-22 02:28:57 PDT
Comment on attachment 608187 [details] [diff] [review]
Patch for already nearly for of

perfect!
Comment 17 Dão Gottwald [:dao] 2012-03-22 02:43:37 PDT
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] 2012-03-22 02:47:06 PDT
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] 2012-03-22 03:00:50 PDT
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] 2012-03-22 03:34:59 PDT
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 22 Dão Gottwald [:dao] 2012-03-22 04:04:02 PDT
> '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] 2012-03-22 04:07:10 PDT
> 'styleSheets' isn't iterable either,

bug 738196
Comment 24 Dão Gottwald [:dao] 2012-03-22 04:11:21 PDT
(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] 2012-03-22 04:25:08 PDT
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] 2012-03-22 05:25:04 PDT
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] 2012-03-22 06:32:58 PDT
(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] 2012-03-22 10:23:48 PDT
(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] 2012-03-27 23:59:36 PDT
(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] 2012-04-07 05:09:05 PDT
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] 2012-04-07 08:27:42 PDT
Created attachment 613111 [details] [diff] [review]
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 2012-04-07 13:05:21 PDT
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/mbrubeck@mozilla.com-163b63a532de
Comment 33 Ian Moody [:Kwan] 2012-04-07 15:54:13 PDT
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] 2012-04-09 13:43:53 PDT
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] 2012-04-16 04:20:44 PDT
Comment on attachment 613111 [details] [diff] [review]
Patch of safe changes

see previous comment
Comment 36 Dão Gottwald [:dao] 2012-05-29 03:45:50 PDT
(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] 2012-08-21 10:00:57 PDT
(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] 2012-08-21 19:09:22 PDT
https://hg.mozilla.org/mozilla-central/rev/8f9cac092f38

Note You need to log in before you can comment on or make changes to this bug.