609080 - Invalid titles in win7 taskbar

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[gadjo]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101101 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101101 Firefox/4.0b8pre

After hovering Fx icon in win7 task bar tabs thumbnails are displayed. Thumbnails titles consist of panorama group name and page title, however the first part (group name) sometimes doesn't match the real group name

Reproducible: Always

Steps to Reproduce:
1. Open some tabs in two panorama groups
2. Give names to both panorama groups
3. Hover on Fx icon in task bar
Actual Results:  
Some tabs have in the title the name of the group which they don't belong to

Expected Results:  
Group name in thumbnails titles should match the panorama group name

Comment 1

[gadjo]

Attachment: Visualization of the bug

Comment 2

[Kevin Hanes]

We've got a number of Aero Peek related questions to answer. Moving this bug to be more in line with those.

Comment 3

[aravindm]

Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101110 Firefox/4.0b8pre

Able to reproduce. When hovering on the taskbar thumbnails group names does not match the panorama group name.

Comment 4

[Maniac Vlad Florin (:vladmaniac)]

This issue reappeared on Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101212 Firefox/4.0b8pre . Due to this, setting this issue to NEW

Comment 5

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 6

[Kevin Hanes]

bugspam (removing b9)

Comment 7

[Kevin Hanes]

Punt

Comment 8

[Dão Gottwald [:dao]]

Kevin, you're wrongly changing the OS field with regularity.

Comment 9

[Kevin Hanes]

(In reply to comment #8)
> Kevin, you're wrongly changing the OS field with regularity.

Odd... I'm not manually setting it when changing other fields. I'll keep an eye out. Sorry, and thank you.

Comment 10

[Jim Mathies [:jimm]]

This should be fixed by bug 581726, I just need to get it landed.

Comment 11

[gadjo]

Doesn't seem to be fixed in 2011-02-06 nightly

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: WIP

This is probably caused by getWindowTitleForBrowser().  The group name is not returned correctly.

http://mxr.mozilla.org/mozilla-central/source/browser/components/wintaskbar/WindowsPreviewPerTab.jsm#295

Comment 13

[Raymond Lee [:raymondlee]]

Attachment: WIP2

Comment 14

[Raymond Lee [:raymondlee]]

Attachment: v1

You can try the build here
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-832857be9f35/

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

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

Review of attachment 556615 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work finding the cause of this error! Please address those issues and ask me and Dão for review again.

::: browser/base/content/browser-tabview.js
@@ +200,5 @@
>        self._isFrameLoading = false;
>        self._window = self._iframe.contentWindow;
>        self._setBrowserKeyHandlers();
> +#ifdef XP_WIN
> +      self._updateTaskbar();

Do we need an extra function for this? If we include it here we don't even need that this._window check.

@@ +270,5 @@
> +  // Function: getGroupNameForTab
> +  // Gets the group name for the given xul:tab.
> +  getGroupNameForTab: function TabView_getGroupNameForTab(tab) {
> +    if (!this._window)
> +      return this._lastSessionGroupName;

This does only work for getActiveGroupName() because we're saving the active group's name on shutdown. If this is used by TaskbarPreviews before Panorama is loaded we'll get into the same trouble again. But we don't have any group names if we're not loaded, yet. Maybe we don't need them - I don't know exactly when TaskbarPreview ask for the tab's title.

@@ +377,5 @@
> +  // Updates the task bar.
> +  _updateTaskbar: function TabView__updateTaskbar() {
> +    if (this._window)
> +      this._window.GroupItems.updateTaskbarPreviews();
> +  },

(see above)

::: browser/base/content/tabbrowser.xml
@@ +813,5 @@
> +              if (index != -1) {
> +                let groupName = TabView.getGroupNameForTab(this.tabs[index]);
> +                if (groupName)
> +                  newTitle = groupName + sep + newTitle;
> +              }

Looks good to me but we should ask a browser peer.

::: browser/base/content/tabview/groupitems.js
@@ +184,5 @@
>        self.$titleShield.show();
>        if (self.getTitle())
>          gTabView.firstUseExperienced = true;
>        self.save();
> +#ifdef XP_WIN

You could make GroupItems.updateTaskbarPreviews() a no-op for non-Win systems by surrounding its code with #ifdef/#endif. So we'd have less clutter in the code when reading.

@@ +2594,5 @@
>        this._updateTabBar();
>      else if (shouldShowTabView)
>        UI.showTabView();
> +
> +#ifdef XP_WIN

(see above).

@@ +2595,5 @@
>      else if (shouldShowTabView)
>        UI.showTabView();
> +
> +#ifdef XP_WIN
> +    if ("gTaskbarTabGroup" in gWindow) // AeroPeek available

We don't need that double check here as that is done in updateTaskbarTabPreview() already.

@@ +2674,5 @@
> +  // Function: updateTaskbarPreviews
> +  // Updates taskbar previews for the given groupItem.
> +  updateTaskbarPreviews: function GroupItems_updateTaskbarPreviews(groupItem) {
> +    if ("gTaskbarTabGroup" in gWindow) { // AeroPeek available
> +      let updatePreviews = function updatePreviews(group) {

We don't need that "let updatePreviews" here. For strict mode compat you'd need to move that function definition out of the if() block.

@@ +2684,5 @@
> +        updatePreviews(groupItem);
> +      } else {
> +        this.groupItems.forEach(function(groupItem) {
> +          updatePreviews(groupItem);
> +        });

Better: this.groupItems.forEach(updatePreviews);

::: browser/base/content/tabview/tabitems.js
@@ +1168,5 @@
> +#ifdef XP_WIN
> +  // ----------
> +  // Function: updateTaskbarTabPreview
> +  // Updates tab preview in the taskbar for the given xul:tab.
> +  updateTaskbarTabPreview: function TabItems_updateTaskbarTabPreview(xulTab) {

I think we should attach this function ("updateTaskbarPreview") to the TabItem itself.

@@ +1170,5 @@
> +  // Function: updateTaskbarTabPreview
> +  // Updates tab preview in the taskbar for the given xul:tab.
> +  updateTaskbarTabPreview: function TabItems_updateTaskbarTabPreview(xulTab) {
> +    let preview = gWindow.gTaskbarTabGroup.previewFromTab(xulTab);
> +    preview.controller.wrappedJSObject.updateTitleAndTooltip();

I don't know if that's the right API to for the TaskbarPreview to update. Let's just ask Dão for review.

Comment 16

[Raymond Lee [:raymondlee]]

Attachment: v2

> ::: browser/base/content/browser-tabview.js
> @@ +200,5 @@
> >        self._isFrameLoading = false;
> >        self._window = self._iframe.contentWindow;
> >        self._setBrowserKeyHandlers();
> > +#ifdef XP_WIN
> > +      self._updateTaskbar();
> 
> Do we need an extra function for this? If we include it here we don't even
> need that this._window check.

Replaced it

> 
> @@ +270,5 @@
> > +  // Function: getGroupNameForTab
> > +  // Gets the group name for the given xul:tab.
> > +  getGroupNameForTab: function TabView_getGroupNameForTab(tab) {
> > +    if (!this._window)
> > +      return this._lastSessionGroupName;
> 
> This does only work for getActiveGroupName() because we're saving the active
> group's name on shutdown. If this is used by TaskbarPreviews before Panorama
> is loaded we'll get into the same trouble again. But we don't have any group
> names if we're not loaded, yet. Maybe we don't need them - I don't know
> exactly when TaskbarPreview ask for the tab's title.

getWindowTitleForBrowser() is called by the TaskbarPreview. Check out the link
http://mxr.mozilla.org/mozilla-central/source/browser/components/wintaskbar/WindowsPreviewPerTab.jsm#296

Yes, we still have the problem before Panorama is loaded, since we would display the saved active group's name on shutdown.  Two possible solutions:
1) Don't add the saved active group's name to tab title before Panorama is loaded so both window titles and titles in TaskbarPreview don't have the group name
2) Pass additional param to the getWindowTitleForBrowser() when it's called by TaskbarPreview code, so we know when not to add the group name to tab titles

> 
> @@ +377,5 @@
> > +  // Updates the task bar.
> > +  _updateTaskbar: function TabView__updateTaskbar() {
> > +    if (this._window)
> > +      this._window.GroupItems.updateTaskbarPreviews();
> > +  },
> 
> (see above)
> 
> ::: browser/base/content/tabbrowser.xml
> @@ +813,5 @@
> > +              if (index != -1) {
> > +                let groupName = TabView.getGroupNameForTab(this.tabs[index]);
> > +                if (groupName)
> > +                  newTitle = groupName + sep + newTitle;
> > +              }
> 
> Looks good to me but we should ask a browser peer.
> 
> ::: browser/base/content/tabview/groupitems.js
> @@ +184,5 @@
> >        self.$titleShield.show();
> >        if (self.getTitle())
> >          gTabView.firstUseExperienced = true;
> >        self.save();
> > +#ifdef XP_WIN
> 
> You could make GroupItems.updateTaskbarPreviews() a no-op for non-Win
> systems by surrounding its code with #ifdef/#endif. So we'd have less
> clutter in the code when reading.

Done

> 
> @@ +2594,5 @@
> >        this._updateTabBar();
> >      else if (shouldShowTabView)
> >        UI.showTabView();
> > +
> > +#ifdef XP_WIN
> 
> (see above).
> 
> @@ +2595,5 @@
> >      else if (shouldShowTabView)
> >        UI.showTabView();
> > +
> > +#ifdef XP_WIN
> > +    if ("gTaskbarTabGroup" in gWindow) // AeroPeek available
> 
> We don't need that double check here as that is done in
> updateTaskbarTabPreview() already.
> 
> @@ +2674,5 @@
> > +  // Function: updateTaskbarPreviews
> > +  // Updates taskbar previews for the given groupItem.
> > +  updateTaskbarPreviews: function GroupItems_updateTaskbarPreviews(groupItem) {
> > +    if ("gTaskbarTabGroup" in gWindow) { // AeroPeek available
> > +      let updatePreviews = function updatePreviews(group) {
> 
> We don't need that "let updatePreviews" here. For strict mode compat you'd
> need to move that function definition out of the if() block.

Moved

> 
> @@ +2684,5 @@
> > +        updatePreviews(groupItem);
> > +      } else {
> > +        this.groupItems.forEach(function(groupItem) {
> > +          updatePreviews(groupItem);
> > +        });
> 
> Better: this.groupItems.forEach(updatePreviews);

Fixed

> 
> ::: browser/base/content/tabview/tabitems.js
> @@ +1168,5 @@
> > +#ifdef XP_WIN
> > +  // ----------
> > +  // Function: updateTaskbarTabPreview
> > +  // Updates tab preview in the taskbar for the given xul:tab.
> > +  updateTaskbarTabPreview: function TabItems_updateTaskbarTabPreview(xulTab) {
> 
> I think we should attach this function ("updateTaskbarPreview") to the
> TabItem itself.

Moved

> 
> @@ +1170,5 @@
> > +  // Function: updateTaskbarTabPreview
> > +  // Updates tab preview in the taskbar for the given xul:tab.
> > +  updateTaskbarTabPreview: function TabItems_updateTaskbarTabPreview(xulTab) {
> > +    let preview = gWindow.gTaskbarTabGroup.previewFromTab(xulTab);
> > +    preview.controller.wrappedJSObject.updateTitleAndTooltip();
> 
> I don't know if that's the right API to for the TaskbarPreview to update.
> Let's just ask Dão for review.

Comment 17

[Dão Gottwald [:dao]]

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

I think we should get rid of this code by fixing bug 682996.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

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

Review of attachment 558842 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Dão Gottwald [:dao] from comment #17)
> I think we should get rid of this code by fixing bug 682996.

Though I'd also favor that solution we should wait for some feedback from the UX team about the change in bug 682996. Anyway, this patch fixes the described problem but we should put it on hold until we know what to do.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Fixed by bug 682996.