Closed Bug 608407 Opened 9 years ago Closed 9 years ago

GroupItems.getActiveOrphanTab should use UI.getActiveTabRef

Categories

(Firefox Graveyard :: Panorama, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 5

People

(Reporter: iangilman, Assigned: raymondlee)

References

Details

(Whiteboard: [qa-][cleanup][good first bug])

Attachments

(1 file, 2 obsolete files)

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.
Depends on: 600665
bugspam (moving b9 to b10)
Blocks: 608028
bugspam (removing b9)
No longer blocks: 598154
Punting to the future.
No longer blocks: 608028
Whiteboard: [qa-][cleanup][good first bug]
Target Milestone: --- → Future
bugspam
Target Milestone: Future → ---
Attached patch v1 (obsolete) — Splinter Review
Sent it to try and waiting for the results
http://tbpl.mozilla.org/?tree=MozillaTry&rev=8978085e7f73
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #523558 - Flags: review?(ian)
Attachment #523558 - Flags: feedback?(tim.taubert)
Comment on attachment 523558 [details] [diff] [review]
v1

Please flag me for review once there's an F+ :)
Attachment #523558 - Flags: review?(ian)
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 :)
Attachment #523558 - Flags: feedback?(tim.taubert) → feedback+
Attached patch v2 (obsolete) — Splinter Review
(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 :)
Attachment #523558 - Attachment is obsolete: true
Attachment #523932 - Flags: review?(ian)
Comment on attachment 523932 [details] [diff] [review]
v2

Looks great! Doesn't need a test, as it's a code refactor.
Attachment #523932 - Flags: review?(ian) → review+
Attachment #523932 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/projects/cedar/rev/9bed272173d8
Keywords: checkin-needed
Whiteboard: [qa-][cleanup][good first bug] → [qa-][cleanup][good first bug][fixed-in-cedar]
Target Milestone: --- → Firefox4.2
http://hg.mozilla.org/mozilla-central/rev/9bed272173d8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [qa-][cleanup][good first bug][fixed-in-cedar] → [qa-][cleanup][good first bug]
Target Milestone: Firefox5 → Firefox 5
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.