608407 - GroupItems.getActiveOrphanTab should use UI.getActiveTabRef

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Ian Gilman [:iangilman]]

We shouldn't ever have an "active orphan tab" that's not the globally selected "active tab", so we shouldn't be keeping a separate record of it. The getActiveOrphanTab routine should just get the active tab and return null if it has a parent. For that matter perhaps the routine should be moved into UI.

Comment 1

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 2

[Kevin Hanes]

bugspam (removing b9)

Comment 3

[Michael Yoshitaka Erlewine [:mitcho]]

Punting to the future.

Comment 4

[Kevin Hanes]

bugspam

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v1

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

Comment 6

[Ian Gilman [:iangilman]]

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

Please flag me for review once there's an F+ :)

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

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

Looks good to me, nice cleanup!

Two nits:

>   _updateTabBar: function GroupItems__updateTabBar() {
>     if (!window.UI)
>       return; // called too soon
>-      
>-    if (!this._activeGroupItem && !this._activeOrphanTab) {
>+
>+
>+    if (!this._activeGroupItem && !UI.getActiveOrphanTab()) {
>       Utils.assert(false, "There must be something to show in the tab bar!");
>       return;
>     }
> 
>     let tabItems = this._activeGroupItem == null ?
>-      [this._activeOrphanTab] : this._activeGroupItem._children;
>+      [UI.getActiveOrphanTab()] : this._activeGroupItem._children;
>     gBrowser.showOnlyTheseTabs(tabItems.map(function(item) item.tab));
>   },

Maybe we can use a variable here to store the result of UI.getActiveOrphanTab()? So we can save some look-ups at the bottom where the function gets called again.

>   // ----------
>+  // Function: getActiveTab
>+  // Returns the currently active orphan tab as a <TabItem>
>+  getActiveOrphanTab: function UI_getActiveOrphanTab() {
>+    return (this._activeTab && !this._activeTab.parent) ? this._activeTab : null;
>+  },

The doc comment tells the wrong function name.

F+ with that addressed :)

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: v2

(In reply to comment #7)
> Comment on attachment 523558 [details] [diff] [review]
> v1
> 
> Looks good to me, nice cleanup!
> 
> Two nits:
> 
> >   _updateTabBar: function GroupItems__updateTabBar() {
> >     if (!window.UI)
> >       return; // called too soon
> >-      
> >-    if (!this._activeGroupItem && !this._activeOrphanTab) {
> >+
> >+
> >+    if (!this._activeGroupItem && !UI.getActiveOrphanTab()) {
> >       Utils.assert(false, "There must be something to show in the tab bar!");
> >       return;
> >     }
> > 
> >     let tabItems = this._activeGroupItem == null ?
> >-      [this._activeOrphanTab] : this._activeGroupItem._children;
> >+      [UI.getActiveOrphanTab()] : this._activeGroupItem._children;
> >     gBrowser.showOnlyTheseTabs(tabItems.map(function(item) item.tab));
> >   },
> 
> Maybe we can use a variable here to store the result of
> UI.getActiveOrphanTab()? So we can save some look-ups at the bottom where the
> function gets called again.
> 

Fixed

> >   // ----------
> >+  // Function: getActiveTab
> >+  // Returns the currently active orphan tab as a <TabItem>
> >+  getActiveOrphanTab: function UI_getActiveOrphanTab() {
> >+    return (this._activeTab && !this._activeTab.parent) ? this._activeTab : null;
> >+  },
> 
> The doc comment tells the wrong function name.


> 
> F+ with that addressed :)

Comment 9

[Raymond Lee [:raymondlee]]

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

Comment 10

[Ian Gilman [:iangilman]]

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

Looks great! Doesn't need a test, as it's a code refactor.

Comment 11

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Comment 12

[(no longer active)]

http://hg.mozilla.org/projects/cedar/rev/9bed272173d8

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/9bed272173d8