Ian Gilman [:iangilman] Reporter
	Description • 14 years ago

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.

	Kevin Hanes
	Comment 1 • 14 years ago

bugspam (moving b9 to b10)

	Kevin Hanes
	Comment 2 • 14 years ago

bugspam (removing b9)

	Michael Yoshitaka Erlewine [:mitcho]
	Comment 3 • 14 years ago

Punting to the future.

	Kevin Hanes
	Comment 4 • 14 years ago

bugspam

	Raymond Lee [:raymondlee] Assignee
	Comment 5 • 14 years ago

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

	Ian Gilman [:iangilman] Reporter
	Comment 6 • 14 years ago

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

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

	Tim Taubert [:ttaubert] (inactive)
	Comment 7 • 14 years ago

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 :)

	Raymond Lee [:raymondlee] Assignee
	Comment 8 • 14 years ago

(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 :)

	Raymond Lee [:raymondlee] Assignee
	Comment 9 • 14 years ago

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

	Ian Gilman [:iangilman] Reporter
	Comment 10 • 14 years ago

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

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

	(no longer active)
	Comment 12 • 14 years ago

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

	(no longer active)
	Comment 13 • 14 years ago

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