Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Use for..of loops in browser.js

RESOLVED FIXED in Firefox 17

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dao, Assigned: Kwan)

Tracking

Trunk
Firefox 17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(7 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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).

Updated

5 years ago
Assignee: nobody → moz-ian
(Assignee)

Comment 2

5 years ago
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]
(Assignee)

Comment 3

5 years ago
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
(Reporter)

Comment 4

5 years ago
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) {
(Assignee)

Comment 5

5 years ago
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]
(Assignee)

Comment 6

5 years ago
Created attachment 608155 [details] [diff] [review]
Patch for 2-line changes

This patch is for loops that only use the variable once
(Assignee)

Comment 7

5 years ago
Created attachment 608187 [details] [diff] [review]
Patch for already nearly for of

Address code simplification nit and a similar instance
(Assignee)

Updated

5 years ago
Attachment #608153 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Created attachment 608192 [details] [diff] [review]
Patch for 2-line changes

Remove unneeded variable creation as in change to previous patch
(Assignee)

Updated

5 years ago
Attachment #608155 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
Created attachment 608193 [details] [diff] [review]
Patch for fairly simple changes

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

Comment 10

5 years ago
Created attachment 608194 [details] [diff] [review]
Patch for more complex changes

More complex multi-line changes
(Assignee)

Comment 11

5 years ago
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
(Assignee)

Comment 12

5 years ago
Created attachment 608198 [details] [diff] [review]
Patch for for each...in change

This deals with the for each...in change
(Assignee)

Comment 13

5 years ago
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
(Assignee)

Comment 14

5 years ago
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.
(Assignee)

Comment 15

5 years ago
(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.
(Reporter)

Comment 16

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

perfect!
Attachment #608187 - Flags: review+
(Reporter)

Updated

5 years ago
Attachment #608192 - Flags: review+
(Reporter)

Comment 17

5 years ago
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+
(Reporter)

Comment 18

5 years ago
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+
(Reporter)

Updated

5 years ago
Attachment #608196 - Flags: review+
(Reporter)

Updated

5 years ago
Attachment #608198 - Flags: review+
(Reporter)

Comment 19

5 years ago
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+
(Reporter)

Comment 20

5 years ago
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.
(Reporter)

Comment 21

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/378712300ad2
Target Milestone: --- → Firefox 14
(Reporter)

Comment 22

5 years ago
> '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.
(Reporter)

Comment 23

5 years ago
> 'styleSheets' isn't iterable either,

bug 738196
(Reporter)

Comment 24

5 years ago
(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'...
(Reporter)

Comment 25

5 years ago
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
(Reporter)

Updated

5 years ago
Depends on: 738193, 738196
(Assignee)

Comment 26

5 years ago
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.
(Assignee)

Comment 27

5 years ago
(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.
(Assignee)

Comment 28

5 years ago
(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?
(Reporter)

Comment 29

5 years ago
(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...
(Reporter)

Comment 30

5 years ago
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.
(Assignee)

Comment 31

5 years ago
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

5 years ago
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
(Assignee)

Comment 33

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

Passed try (with some noise from intermittent oranges).
Attachment #613111 - Flags: review?(dao)
(Reporter)

Comment 34

5 years ago
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.
(Reporter)

Comment 35

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

see previous comment
Attachment #613111 - Flags: review?(dao)
(Reporter)

Comment 36

5 years ago
(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 → ---
(Reporter)

Comment 37

5 years ago
(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
(Reporter)

Updated

5 years ago
Attachment #613111 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/8f9cac092f38
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
You need to log in before you can comment on or make changes to this bug.