596833 - Refinement for bug 595943: No need to iterate over all tabs, just the ones at the beginning

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P4)

Description

[Ian Gilman [:iangilman]]

>+++ b/browser/base/content/tabview/ui.js
...
>+          // Don't return to TabView if there are any app tabs
>+          for (let a = 0; a < gBrowser.tabs.length; a++) {
>+            let theTab = gBrowser.tabs[a]; 
>+            if (theTab.pinned && gBrowser._removingTabs.indexOf(theTab) == -1) 
>+              return;
>+          }

I don't think you need to iterate over the whole list of tabs... AFAIK AppTabs
are always at the beginning, so you could break early.

        for (let a = 0; a < gBrowser.tabs.length; a++) {
          let theTab = gBrowser.tabs[a];
          if (!theTab.pinned)
              break;
          if (gBrowser._removingTabs.indexOf(theTab) == -1) 
            return;
        }

Comment 1

[Ian Gilman [:iangilman]]

Not a big deal, but might as well take care of it. Raymond, would you mind taking this on as well?

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: v1

Sent it to try and waiting for the results.
http://tbpl.mozilla.org/?tree=MozillaTry&rev=146cac2a9075

Comment 3

[Raymond Lee [:raymondlee]]

Passed Try!

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 523957 [details] [diff] [review]
v1

I personally don't really like the break and return statements combined - it's pretty hard to read (and get what it's doing).

I'd prefer the following:

>-for (let a = 0; a < gBrowser.tabs.length; a++) {
>+for (let a = 0; a < gBrowser._numPinnedTabs; a++) {

Would you concur with that solution? We could also ask Ian to throw in his two cents :)

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v2

Taking tim's suggestion.

Comment 6

[Ian Gilman [:iangilman]]

Comment on attachment 524338 [details] [diff] [review]
v2

Looks great! Thank you.

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Comment 8

[Raymond Lee [:raymondlee]]

(In reply to comment #7)
> Created attachment 524576 [details] [diff] [review]
> Patch for checkin

Passed Try!
http://tbpl.mozilla.org/?tree=MozillaTry&rev=56785418a284

Comment 9

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/d90464774dfe