Aza Raskin [:aza] Reporter
	Description • 14 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.

	Aza Raskin [:aza] Reporter
	Comment 1 • 14 years ago

This bug was fathered from https://bugzilla.mozilla.org/show_bug.cgi?id=587099#c6

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

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.

	Ian Gilman [:iangilman]
	Comment 3 • 14 years ago

Note that the fix for this will likely be related to the fix for bug 599852.

	Ian Gilman [:iangilman]
	Comment 4 • 14 years ago

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.

	Ian Gilman [:iangilman]
	Comment 5 • 14 years ago

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?

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

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.

	Sean Dunn
	Comment 7 • 14 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?

	Kevin Hanes
	Comment 8 • 14 years ago

Moving to b9

	Ian Gilman [:iangilman]
	Comment 9 • 14 years ago

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

Ack! Stupid rebase bug. Fixed.

	Dão Gottwald [:dao]
	Comment 10 • 14 years ago

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

	Ian Gilman [:iangilman]
	Comment 12 • 14 years ago

This patch no longer depends on bug 600665.

	Ian Gilman [:iangilman]
	Comment 13 • 14 years ago

(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.

	Kevin Hanes
	Comment 14 • 14 years ago

bugspam (moving b9 to b10)

	Kevin Hanes
	Comment 15 • 14 years ago

bugspam (removing b9)

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

Dāo, could you review at your convenience if you get bored of blockers? ;)

	Kevin Hanes
	Comment 17 • 14 years ago

Non blocking and languishing. Moving off our official b11 timeline. Wouldn't object if it came through, however.

	Ian Gilman [:iangilman]
	Comment 18 • 14 years ago

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

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

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

	Zéfling
	Comment 20 • 14 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.

	Dão Gottwald [:dao]
	Comment 21 • 14 years ago

(In reply to comment #19)
> Taking. Dāo are you the right one to review this?

I can review this.

	Dão Gottwald [:dao]
	Comment 23 • 14 years ago

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

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

(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.

	Dão Gottwald [:dao]
	Comment 25 • 14 years ago

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.

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

(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

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 14 years ago

http://hg.mozilla.org/projects/cedar/rev/11d2a7ed963a

	Boris Zbarsky [:bzbarsky]
	Comment 28 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/11d2a7ed963a

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

Backed out because it caused intermittent bug 655197.

http://hg.mozilla.org/mozilla-central/rev/76bec6bdc88c

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

The problem was that it revealed a pretty basic error that hasn't been noticed, yet: bug 655269.

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

bugspam

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

bugspam

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 33 • 13 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/59c84742872f

	Boris Zbarsky [:bzbarsky]
	Comment 34 • 13 years ago

Backed out due to mochitest-other orange.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 35 • 13 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/3a5d39bd32a6

	(no longer active)
	Comment 36 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/3a5d39bd32a6

	George Carstoiu
	Comment 37 • 13 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.