Closed Bug 737792 Opened 9 years ago Closed 8 years ago

Use for..of loops in browser.js

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 17

People

(Reporter: dao, Assigned: Kwan)

References

Details

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

Attachments

(7 files, 4 obsolete files)

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).
Assignee: nobody → moz-ian
Attached patch Patch for already nearly for of (obsolete) — Splinter Review
This patch handles the areas that are basically already doing this anyway by instantly setting a variable to var[i]
Comment on attachment 608148 [details] [diff] [review]
Patch for already nearly for of

oops, forgot to do the final check for prior declarations, sorry
Attachment #608148 - Attachment is obsolete: true
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) {
Attached patch Patch for already nearly for of (obsolete) — Splinter Review
This patch handles the areas that are basically already doing this anyway by instantly setting a variable to var[i]
Attached patch Patch for 2-line changes (obsolete) — Splinter Review
This patch is for loops that only use the variable once
Address code simplification nit and a similar instance
Attachment #608153 - Attachment is obsolete: true
Remove unneeded variable creation as in change to previous patch
Attachment #608155 - Attachment is obsolete: true
These are changes that are longer than two lines but still pretty simple
More complex multi-line changes
This deals with the for...in changes, of which there only seemed to be a couple that were changable
This deals with the for each...in change
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
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.
(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 on attachment 608187 [details] [diff] [review]
Patch for already nearly for of

perfect!
Attachment #608187 - Flags: review+
Attachment #608192 - Flags: review+
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.
Attachment #608193 - Flags: review+
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.
Attachment #608194 - Flags: review+
Attachment #608196 - Flags: review+
Attachment #608198 - Flags: review+
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
Attachment #608200 - Flags: review+
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.
> '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.
> 'styleSheets' isn't iterable either,

bug 738196
(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'...
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
Depends on: 738193, 738196
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.
(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.
(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?
(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...
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.
Attached patch Patch of safe changes (obsolete) — Splinter Review
This should be all the safe changes. Going to request a try run on IRC to make sure nothing is broken.
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 on attachment 613111 [details] [diff] [review]
Patch of safe changes

Passed try (with some noise from intermittent oranges).
Attachment #613111 - Flags: review?(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 on attachment 613111 [details] [diff] [review]
Patch of safe changes

see previous comment
Attachment #613111 - Flags: review?(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?
Target Milestone: Firefox 14 → ---
Attachment #613111 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/8f9cac092f38
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
You need to log in before you can comment on or make changes to this bug.