Closed Bug 1122984 Opened 10 years ago Closed 10 years ago

Use for..of iteration instead of Array.forEach in tabbrowser.xml

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 38

People

(Reporter: dao, Assigned: sssarvjeet27, Mentored)

Details

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

Attachments

(1 file, 6 obsolete files)

bug-1122984-fix-v2.patch
4.04 KB, patch
dao
: review+
Details | Diff | Splinter Review
Look for "forEach" (without quotes) in browser/base/content/tabbrowser.xml. You should find five matches. All those cases can be simplified as follows... instead of: x.forEach(function (y) { ... }); or: Array.forEach(x, function (y) { ... }); we can do: for (let y of x) { ... } This one is a little bit tricky: http://hg.mozilla.org/mozilla-central/annotate/369a8f14ccf8/browser/base/content/tabbrowser.xml#l3657 But it can be treated like I've shown above, because it's basically equivalent to this code: tabs.tabbrowser._removingTabs.forEach(function (tab) { tabs.tabbrowser.removeTab(tab); });
hi dao could you please assign this bug to me? Also this is going to be my first contribution.
Sure, let me know if you have questions.
Assignee: nobody → sssarvjeet27
(In reply to sssarvjeet27 from comment #1) > hi dao > could you please assign this bug to me? > Also this is going to be my first contribution. Have you been able to make any progress yet?
yeah i have replaced all the 5 forEach() with for(let y of x).just had little problem with adding the patch. i`ll attach very soon.
Attached patch bug1122984.patch (obsolete) — — Splinter Review
i haven`t tested it.could you tell me how to test?
Attachment #8552550 - Flags: review?(dao)
Comment on attachment 8552550 [details] [diff] [review] bug1122984.patch This patch looks functionally correct, but there are formatting issues: >- this.mProgressListeners.forEach(function (p) { >+ for(let p of this.mProgressListners) { >+ if(aMethod in y) { >+ try{ >+ if(!p[aMethod].apply(p,aArguments)) >+ rv = false; >+ } catch(e) { >+ // don`t inhibit other listners >+ Components.utils.reportError(e); >+ } >+ } >+ } You needlessly and wrongly reindented this whole block of code. Except for the first line, it should stay entirely untouched, just like here where you did it correctly: > <method name="showOnlyTheseTabs"> > <parameter name="aTabs"/> > <body> > <![CDATA[ >- Array.forEach(this.tabs, function(tab) { >+ for (let tab of this.tab) { > if (aTabs.indexOf(tab) == -1) > this.hideTab(tab); > else > this.showTab(tab); >- }, this); >+ } > > this.tabContainer._handleTabSelect(false); > ]]> > </body> > </method>
Attachment #8552550 - Flags: review?(dao) → review-
Hi, I would like to work on this bug. It would be my first bug please guide me.
(In reply to mak from comment #7) > Hi, I would like to work on this bug. It would be my first bug please guide > me. Sorry, somebody else is already working on this one.
Attached patch bug1122984.patch (obsolete) — — Splinter Review
Attachment #8553073 - Flags: review?(dao)
ignore the above attachment in comment 9.
Attached patch bug1122984.patch (obsolete) — — Splinter Review
Attachment #8552550 - Attachment is obsolete: true
Attachment #8553073 - Attachment is obsolete: true
Attachment #8553073 - Flags: review?(dao)
Attachment #8553080 - Flags: review?(dao)
Comment on attachment 8553080 [details] [diff] [review] bug1122984.patch >- this.mProgressListeners.forEach(function (p) { >+ for (let p of this.mProgressListeners) { > if (aMethod in p) { > try { > if (!p[aMethod].apply(p, aArguments)) > rv = false; > } catch (e) { > // don't inhibit other listeners > Components.utils.reportError(e); > } > } > }); This looks a lot better than the previous patch, but the syntax is broken... you need to replace }); with } at the end of the loop. >- this.mTabsProgressListeners.forEach(function (p) { >+ for (let p of this.mTabsProgressListeners) { same here > // Remove this tab as the owner of any other tabs, since it's going away. >- Array.forEach(this.tabs, function (tab) { >+ for (let tab of this.tabs) { > if ("owner" in tab && tab.owner == aTab) > // |tab| is a child of the tab we're removing, make it an orphan > tab.owner = null; > }); same here > <method name="showOnlyTheseTabs"> > <parameter name="aTabs"/> > <body> > <![CDATA[ >- Array.forEach(this.tabs, function(tab) { >+ for (let tab of this.tabs) { > if (aTabs.indexOf(tab) == -1) > this.hideTab(tab); > else > this.showTab(tab); > }, this); same here
Attachment #8553080 - Flags: review?(dao) → review-
Attached patch bug1122984.patch (obsolete) — — Splinter Review
Attachment #8553080 - Attachment is obsolete: true
Attachment #8553145 - Flags: review?(dao)
Comment on attachment 8553145 [details] [diff] [review] bug1122984.patch > for (let p of this.mTabsProgressListeners) { > if (aMethod in p) { > try { > if (!p[aMethod].apply(p, aArguments)) > rv = false; > } catch (e) { > // don't inhibit other listeners > Components.utils.reportError(e); > } > } >- }); >+ } Unfortunately this is on top of the previous patch. We'll need all changes in one patch.
Attachment #8553145 - Flags: review?(dao)
Attached patch bug1122984.patch (obsolete) — — Splinter Review
Attachment #8553145 - Attachment is obsolete: true
Attachment #8553202 - Flags: review?(dao)
Comment on attachment 8553202 [details] [diff] [review] bug1122984.patch >diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js >--- a/browser/app/profile/firefox.js >+++ b/browser/app/profile/firefox.js >@@ -1698,17 +1698,17 @@ pref("security.cert_pinning.enforcement_ > // Override the Gecko-default value of false for Firefox. > pref("plain_text.wrap_long_lines", true); > > // If this turns true, Moz*Gesture events are not called stopPropagation() > // before content. > pref("dom.debug.propagate_gesture_events_through_content", false); > > // The request URL of the GeoLocation backend. >-pref("geo.wifi.uri", "https://www.googleapis.com/geolocation/v1/geolocate?key=%GOOGLE_API_KEY%"); >+pref("geo.wifi.uri", "https://location.services.mozilla.com/v1/geolocate?key=%MOZILLA_API_KEY%"); > > // Necko IPC security checks only needed for app isolation for cookies/cache/etc: > // currently irrelevant for desktop e10s > pref("network.disable.ipc.security", true); > > // CustomizableUI debug logging. > pref("browser.uiCustomization.debug", false); > r+ with the above part removed, because that has nothing to do with this bug :)
Attachment #8553202 - Flags: review?(dao) → review+
what does "with the above part removed" mean that i need to remove that or you have done that? also could you please check-in this patch?
(In reply to sssarvjeet27 from comment #17) > what does "with the above part removed" mean that i need to remove that Please attach a new patch with that part removed.
Attached patch bug-1122984-fix.patch (obsolete) — — Splinter Review
Attachment #8553202 - Attachment is obsolete: true
Attachment #8553676 - Flags: review?(dao)
Comment on attachment 8553676 [details] [diff] [review] bug-1122984-fix.patch >@@ -3644,19 +3644,19 @@ > return; // Ignore vertical events > > var tabs = document.getBindingParent(this); > tabs.removeAttribute("overflow"); > > if (tabs._lastTabClosedByMouse) > tabs._expandSpacerBy(this._scrollButtonDown.clientWidth); > >- tabs.tabbrowser._removingTabs.forEach(tabs.tabbrowser.removeTab, >- tabs.tabbrowser); >- >+ for (let tab of tabs.tabbrowser._removingTabs) >+ tabs.tabbrowser.removeTab(tab); >+ Oops, one last thing: In the last line (the empty one), you added lots of spaces. Please remove those. Thanks!
Attachment #8553676 - Flags: review?(dao)
Attachment #8553676 - Attachment is obsolete: true
Attachment #8554126 - Flags: review?(dao)
Comment on attachment 8554126 [details] [diff] [review] bug-1122984-fix-v2.patch Thanks!
Attachment #8554126 - Flags: review?(dao) → review+
https://hg.mozilla.org/mozilla-central/rev/bb70f0e4a567
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: