Closed Bug 1122984 Opened 9 years ago Closed 9 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: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38

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