Last Comment Bug 595020 - Group name isn't displayed in title before TabView is loaded the first time
: Group name isn't displayed in title before TabView is loaded the first time
Status: VERIFIED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: P3 normal
: Firefox 7
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
: 623312 (view as bug list)
Depends on: 655197 655269
Blocks: 608387 660175
  Show dependency treegraph
 
Reported: 2010-09-09 16:50 PDT by Aza Raskin [:aza]
Modified: 2016-04-12 14:00 PDT (History)
10 users (show)
ehsan: in‑testsuite+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (4.07 KB, patch)
2010-10-20 17:14 PDT, Ian Gilman [:iangilman]
no flags Details | Diff | Splinter Review
patch v2 (requires 600665) (317.65 KB, patch)
2010-10-29 14:10 PDT, Ian Gilman [:iangilman]
seanedunn: feedback+
Details | Diff | Splinter Review
patch v3 (4.99 KB, patch)
2010-12-22 14:59 PST, Ian Gilman [:iangilman]
no flags Details | Diff | Splinter Review
patch v4 (5.17 KB, patch)
2011-01-07 15:48 PST, Ian Gilman [:iangilman]
no flags Details | Diff | Splinter Review
patch v5 (9.74 KB, patch)
2011-04-21 13:12 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v6 (9.69 KB, patch)
2011-04-30 04:34 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
Details | Diff | Splinter Review
patch for checkin (6.32 KB, patch)
2011-05-03 11:37 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review

Description Aza Raskin [:aza] 2010-09-09 16:50:00 PDT
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.
Comment 1 Aza Raskin [:aza] 2010-09-09 16:50:20 PDT
This bug was fathered from https://bugzilla.mozilla.org/show_bug.cgi?id=587099#c6
Comment 2 Michael Yoshitaka Erlewine [:mitcho] 2010-09-16 18:54:05 PDT
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.
Comment 3 Ian Gilman [:iangilman] 2010-09-28 11:57:06 PDT
Note that the fix for this will likely be related to the fix for bug 599852.
Comment 4 Ian Gilman [:iangilman] 2010-10-20 14:28:17 PDT
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.
Comment 5 Ian Gilman [:iangilman] 2010-10-20 17:14:34 PDT
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?
Comment 6 Ian Gilman [:iangilman] 2010-10-29 14:10:27 PDT
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.
Comment 7 Sean Dunn 2010-11-08 12:37:17 PST
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?
Comment 8 Kevin Hanes 2010-12-10 11:42:20 PST
Moving to b9
Comment 9 Ian Gilman [:iangilman] 2010-12-22 14:59:57 PST
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.
Comment 10 Dão Gottwald [:dao] 2010-12-23 01:59:30 PST
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?)
Comment 11 Ian Gilman [:iangilman] 2011-01-06 09:43:11 PST
*** Bug 623312 has been marked as a duplicate of this bug. ***
Comment 12 Ian Gilman [:iangilman] 2011-01-07 15:44:03 PST
This patch no longer depends on bug 600665.
Comment 13 Ian Gilman [:iangilman] 2011-01-07 15:48:46 PST
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.
Comment 14 Kevin Hanes 2011-01-10 10:07:31 PST
bugspam (moving b9 to b10)
Comment 15 Kevin Hanes 2011-01-10 10:10:17 PST
bugspam (removing b9)
Comment 16 Michael Yoshitaka Erlewine [:mitcho] 2011-01-20 16:44:38 PST
Dāo, could you review at your convenience if you get bored of blockers? ;)
Comment 17 Kevin Hanes 2011-01-21 14:29:27 PST
Non blocking and languishing. Moving off our official b11 timeline. Wouldn't object if it came through, however.
Comment 18 Ian Gilman [:iangilman] 2011-04-01 10:22:22 PDT
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).
Comment 19 Tim Taubert [:ttaubert] 2011-04-06 11:56:46 PDT
Taking. Dāo are you the right one to review this?
Comment 20 Zéfling 2011-04-10 07:32:21 PDT
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.
Comment 21 Dão Gottwald [:dao] 2011-04-10 07:40:39 PDT
(In reply to comment #19)
> Taking. Dāo are you the right one to review this?

I can review this.
Comment 22 Tim Taubert [:ttaubert] 2011-04-21 13:12:52 PDT
Created attachment 527632 [details] [diff] [review]
patch v5
Comment 23 Dão Gottwald [:dao] 2011-04-29 04:48:43 PDT
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() : "";
Comment 24 Tim Taubert [:ttaubert] 2011-04-30 04:34:01 PDT
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.
Comment 25 Dão Gottwald [:dao] 2011-05-02 08:01:17 PDT
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.
Comment 26 Tim Taubert [:ttaubert] 2011-05-03 11:37:09 PDT
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
Comment 27 Boris Zbarsky [:bz] 2011-05-03 16:37:38 PDT
http://hg.mozilla.org/projects/cedar/rev/11d2a7ed963a
Comment 28 Boris Zbarsky [:bz] 2011-05-04 13:52:47 PDT
http://hg.mozilla.org/mozilla-central/rev/11d2a7ed963a
Comment 29 Tim Taubert [:ttaubert] 2011-05-06 08:50:50 PDT
Backed out because it caused intermittent bug 655197.

http://hg.mozilla.org/mozilla-central/rev/76bec6bdc88c
Comment 30 Tim Taubert [:ttaubert] 2011-05-06 08:57:32 PDT
The problem was that it revealed a pretty basic error that hasn't been noticed, yet: bug 655269.
Comment 31 Tim Taubert [:ttaubert] 2011-05-27 02:22:23 PDT
bugspam
Comment 32 Tim Taubert [:ttaubert] 2011-05-27 02:27:18 PDT
bugspam
Comment 34 Boris Zbarsky [:bz] 2011-06-09 11:57:02 PDT
Backed out due to mochitest-other orange.
Comment 37 George Carstoiu 2011-06-16 05:14:10 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.