The default bug view has changed. See this FAQ.

Group name isn't displayed in title before TabView is loaded the first time

VERIFIED FIXED in Firefox 7

Status

Firefox Graveyard
Panorama
P3
normal
VERIFIED FIXED
7 years ago
a year ago

People

(Reporter: aza, Assigned: ttaubert)

Tracking

Trunk
Firefox 7
Dependency tree / graph
Bug Flags:
in-testsuite +

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

7 years ago
What happens when TabView has not loaded? Then it's possible for group names to not show up in the Title, even if we're part of a group. We could call TabView.init() here if we haven't set it up yet, but this would necessarily force TabView to load on Firefox startup, which may cause a Ts regression. Another approach is to save the last active group name in session store.
(Reporter)

Comment 1

7 years ago
This bug was fathered from https://bugzilla.mozilla.org/show_bug.cgi?id=587099#c6
I think this relates to issues in a couple other bugs as well... the bottom line is that we need a way to get group information from sessionstore/build it before we setup the tabview iframe. We need the abstract groups model.
Priority: -- → P2
Assignee: nobody → ian
Note that the fix for this will likely be related to the fix for bug 599852.
Blocks: 597043
Priority: P2 → P3
As discussed in bug 599852, I suggest we don't try for abstracting out the groups model before ff4.0. Therefore, I'm going to go with Aza's second suggestion above:

> Another approach is to save the last active group name in session store.
Status: NEW → ASSIGNED
Created attachment 484903 [details] [diff] [review]
patch v1

While I was in there I took the liberty of fixing another issue: the group title didn't show if you were on an app tab. 

I'm not quite sure how to test this bug. Everything of note happens at start up and shut down of the app. Perhaps it's minor enough we don't need a test? Otherwise, maybe litmus?
Attachment #484903 - Flags: feedback?(aza)
Created attachment 487022 [details] [diff] [review]
patch v2 (requires 600665)

Ok, I've added a test for the title updating with app tabs (and also fixed up that code a bit). This patch now depends on bug 600665. I don't plan on adding a test for the other aspect of the bug, unless someone has a good suggestion how to do so.
Attachment #484903 - Attachment is obsolete: true
Attachment #487022 - Flags: feedback?(seanedunn)
Attachment #484903 - Flags: feedback?(aza)
Depends on: 600665
Blocks: 608387

Comment 7

7 years ago
Comment on attachment 487022 [details] [diff] [review]
patch v2 (requires 600665)

Looks good, but why is all the test code removal part of this bug?
Attachment #487022 - Flags: feedback?(seanedunn) → feedback+

Comment 8

6 years ago
Moving to b9
Blocks: 598154

Updated

6 years ago
No longer blocks: 597043
Created attachment 499399 [details] [diff] [review]
patch v3

(In reply to comment #7)
> Looks good, but why is all the test code removal part of this bug?

Ack! Stupid rebase bug. Fixed.
Attachment #487022 - Attachment is obsolete: true
Attachment #499399 - Flags: review?(dao)
Comment on attachment 499399 [details] [diff] [review]
patch v3

>+      this._lastSessionGroupName = 
>+          this._sessionstore.getWindowValue(window, this._lastSessionGroupNameID);

Remove trailing space on the first line, reduce indentation on the second line by two spaces.

>   observe: function TabView_observe(subject, topic, data) {
>     if (topic == "quit-application-requested") {
>       let data = (this.isVisible() ? "true" : "false");
>       this._sessionstore.setWindowValue(window, this._visibilityID, data);
>+
>+      data = this.getActiveGroupName(); 
>+      this._sessionstore.setWindowValue(window, this._lastSessionGroupNameID, data);
>     }

What does setWindowValue do here if getActiveGroupName() returns null?

>   getActiveGroupName: function Tabview_getActiveGroupName() {
>+    if (!this._window)
>+      return this._lastSessionGroupName;
>+
>     // We get the active group this way, instead of querying
>     // GroupItems.getActiveGroupItem() because the tabSelect event
>     // will not have happened by the time the browser tries to
>     // update the title.
>+    let groupItem = null;
>     let activeTab = window.gBrowser.selectedTab;
>-    if (activeTab.tabItem && activeTab.tabItem.parent){
>-      let groupName = activeTab.tabItem.parent.getTitle();
>+    if (activeTab.pinned) {
>+      // It's an app tab, so it won't have a .tabItem. However, its .parent
>+      // will already be set as the active group (see UI.goToTabRef). 
>+      groupItem = this._window.GroupItems.getActiveGroupItem();
>+    } else {
>+      groupItem = activeTab.tabItem.parent;
>+    }
>+
>+    if (groupItem) {

Can this be null here? (Why?)
Duplicate of this bug: 623312
This patch no longer depends on bug 600665.
No longer depends on: 600665
Created attachment 502123 [details] [diff] [review]
patch v4

(In reply to comment #10)
> Remove trailing space on the first line, reduce indentation on the second line
> by two spaces.

Done. 

> What does setWindowValue do here if getActiveGroupName() returns null?

Good point; changed to "";

> Can this be null here? (Why?)

Yes, if it's an orphan tab. Added comment to this effect.
Attachment #499399 - Attachment is obsolete: true
Attachment #502123 - Flags: review?(dao)
Attachment #499399 - Flags: review?(dao)

Comment 14

6 years ago
bugspam (moving b9 to b10)
Blocks: 608028

Comment 15

6 years ago
bugspam (removing b9)
No longer blocks: 598154
Dāo, could you review at your convenience if you get bored of blockers? ;)
Assignee: ian → nobody
Blocks: 627096
No longer blocks: 608028

Comment 17

6 years ago
Non blocking and languishing. Moving off our official b11 timeline. Wouldn't object if it came through, however.
Target Milestone: --- → Future
No longer blocks: 627096
Let's get this landed now that Fx4 is out. Tim, can you take it over? It's probably going to need to be unrotted, and you should figure out who should review it (it's been on Dao's plate for months, and he hasn't gotten to it presumably because it wasn't crucial for Fx4, so maybe now he can get to it, or maybe he's not the right one for it anyway).
Assignee: nobody → tim.taubert
Blocks: 603789
Target Milestone: Future → ---
(Assignee)

Comment 19

6 years ago
Taking. Dāo are you the right one to review this?

Comment 20

6 years ago
It's a same problem, on start this line does not work:
> gBrowser.tabs[0]._tabViewTabItem.parent.getTitle();

Before TabView is loaded a first time, no problem.
(In reply to comment #19)
> Taking. Dāo are you the right one to review this?

I can review this.
(Assignee)

Updated

6 years ago
Depends on: 650573
(Assignee)

Comment 22

6 years ago
Created attachment 527632 [details] [diff] [review]
patch v5
Attachment #502123 - Attachment is obsolete: true
Attachment #527632 - Flags: review?(dao)
Attachment #502123 - Flags: review?(dao)
(Assignee)

Updated

6 years ago
No longer blocks: 603789
(Assignee)

Updated

6 years ago
Blocks: 653099
Comment on attachment 527632 [details] [diff] [review]
patch v5

Review of attachment 527632 [details] [diff] [review]:

::: browser/base/content/browser-tabview.js
@@ +72,5 @@
+    if (!this._sessionStore)
+      this._sessionStore = Cc["@mozilla.org/browser/sessionstore;1"]
+        .getService(Ci.nsISessionStore);
+
+    return this._sessionStore;

You don't need _sessionStore this way:

delete this.sessionStore;
return this.sessionStore = ...;

@@ +89,5 @@
         this._setBrowserKeyHandlers();
 
       // ___ visibility
+      let data = this.sessionStore.getWindowValue(window,
+        this.VISIBILITY_IDENTIFIER);

This looks like it fits nicely on one line.

@@ +225,5 @@
+    if (activeTab.pinned) {
+      // It's an app tab, so it won't have a .tabItem. However, its .parent
+      // will already be set as the active group. 
+      groupItem = this._window.GroupItems.getActiveGroupItem();
+    } else if (activeTabItem) {

When would activeTabItem be null here?

@@ +233,5 @@
+    // groupItem may still be null, if the active tab is an orphan.
+    if (groupItem)
+      return groupItem.getTitle();
+
+    return "";

return groupItem ? groupItem.getTitle() : "";
(Assignee)

Comment 24

6 years ago
Created attachment 529279 [details] [diff] [review]
patch v6

(In reply to comment #23)
> You don't need _sessionStore this way:
> 
> delete this.sessionStore;
> return this.sessionStore = ...;

Cool, fixed.

> +      let data = this.sessionStore.getWindowValue(window,
> +        this.VISIBILITY_IDENTIFIER);
> 
> This looks like it fits nicely on one line.

Fixed.

> @@ +225,5 @@
> +    if (activeTab.pinned) {
> +      // It's an app tab, so it won't have a .tabItem. However, its .parent
> +      // will already be set as the active group. 
> +      groupItem = this._window.GroupItems.getActiveGroupItem();
> +    } else if (activeTabItem) {
> 
> When would activeTabItem be null here?

Last time I saw some errors about activeTabItem being undefined but they were likely related to bug 651311. Removed that check as it's unnecessary.

> return groupItem ? groupItem.getTitle() : "";

Fixed.
Attachment #527632 - Attachment is obsolete: true
Attachment #529279 - Flags: review?(dao)
Attachment #527632 - Flags: review?(dao)
Comment on attachment 529279 [details] [diff] [review]
patch v6

>diff --git a/browser/base/content/browser-tabview.js b/browser/base/content/browser-tabview.js
>--- a/browser/base/content/browser-tabview.js
>+++ b/browser/base/content/browser-tabview.js

>+  get sessionStore() {
>+    delete this.sessionStore;
>+    return this.sessionStore = Cc["@mozilla.org/browser/sessionstore;1"]
>+                               .getService(Ci.nsISessionStore);
>+  },
>+
>   // ----------
>   init: function TabView_init() {
...
>       // ___ visibility
>-      let sessionstore =
>-        Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
>-      let data = sessionstore.getWindowValue(window, this.VISIBILITY_IDENTIFIER);
>+      let data = this.sessionStore.getWindowValue(window, this.VISIBILITY_IDENTIFIER);

This change seems unnecessary, since the sessionStore service isn't used outside of this method. r=me with this reverted or explained.
Attachment #529279 - Flags: review?(dao) → review+
(Assignee)

Comment 26

6 years ago
Created attachment 529781 [details] [diff] [review]
patch for checkin

(In reply to comment #25)
> This change seems unnecessary, since the sessionStore service isn't used
> outside of this method. r=me with this reverted or explained.

It's indeed unnecessary. I think that was some legacy change I forgot to get rid of as the patch evolved.

> >   getActiveGroupName: function Tabview_getActiveGroupName() {
> >+    if (!this._window)
> >+      return this._lastSessionGroupName;
> >+
> >     // We get the active group this way, instead of querying
> >     // GroupItems.getActiveGroupItem() because the tabSelect event
> >     // will not have happened by the time the browser tries to
> >     // update the title.
> >+    let groupItem = null;
> >     let activeTab = window.gBrowser.selectedTab;
> >-    if (activeTab.tabItem && activeTab.tabItem.parent){
> >-      let groupName = activeTab.tabItem.parent.getTitle();
> >+    if (activeTab.pinned) {
> >+      // It's an app tab, so it won't have a .tabItem. However, its .parent
> >+      // will already be set as the active group (see UI.goToTabRef). 
> >+      groupItem = this._window.GroupItems.getActiveGroupItem();
> >+    } else {
> >+      groupItem = activeTab.tabItem.parent;
> >+    }
> >+
> >+    if (groupItem) {
> 
> Can this be null here? (Why?)

I re-inserted the null check here because the patch failed on try constantly:

http://tbpl.mozilla.org/?tree=Try&pusher=tim.taubert@gmx.de&rev=969fe1ce8e83

This test fails because the tab gets unlinked (._tabViewTabItem gets removed) and after that the session store triggers a call to TabView.getActiveGroupName(). Now it passes try:

http://tbpl.mozilla.org/?tree=Try&pusher=tim.taubert@gmx.de&rev=d6f93d5f8818
Attachment #529279 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
No longer depends on: 650573
http://hg.mozilla.org/projects/cedar/rev/11d2a7ed963a
Keywords: checkin-needed
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → Firefox 6
http://hg.mozilla.org/mozilla-central/rev/11d2a7ed963a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
(Assignee)

Comment 29

6 years ago
Backed out because it caused intermittent bug 655197.

http://hg.mozilla.org/mozilla-central/rev/76bec6bdc88c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

6 years ago
Depends on: 655269
(Assignee)

Comment 30

6 years ago
The problem was that it revealed a pretty basic error that hasn't been noticed, yet: bug 655269.
Depends on: 655197
(Assignee)

Comment 31

6 years ago
bugspam
No longer blocks: 653099
(Assignee)

Comment 32

6 years ago
bugspam
Blocks: 660175
(Assignee)

Comment 33

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/59c84742872f
Whiteboard: [inbound]
Target Milestone: Firefox 6 → ---
Backed out due to mochitest-other orange.
Whiteboard: [inbound]
(Assignee)

Comment 35

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/3a5d39bd32a6
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/3a5d39bd32a6
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 7

Comment 37

6 years ago
Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110615 Firefox/7.0a1

Verified on Ubuntu 11.04 x86, Mac OS X 10.6, Win 7 x86, WinXP using the following steps to reproduce:
 1. Enter Panorama and named a group
 2. Restarted Firefox
 3. Checked title bar

Issue no longer present - changing status to Verified.

Updated

6 years ago
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.