Last Comment Bug 609080 - Invalid titles in win7 taskbar
: Invalid titles in win7 taskbar
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: x86_64 Windows 7
: P3 normal
: Future
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on: 581726 682996
Blocks: 586556
  Show dependency treegraph
 
Reported: 2010-11-02 12:21 PDT by gadjo
Modified: 2016-04-12 14:00 PDT (History)
10 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Visualization of the bug (192.80 KB, image/jpeg)
2010-11-02 12:22 PDT, gadjo
no flags Details
WIP (2.50 KB, patch)
2011-08-23 22:30 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
WIP2 (2.87 KB, patch)
2011-08-25 00:54 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v1 (8.54 KB, patch)
2011-08-29 11:08 PDT, Raymond Lee [:raymondlee]
ttaubert: review-
Details | Diff | Splinter Review
v2 (7.16 KB, patch)
2011-09-07 09:23 PDT, Raymond Lee [:raymondlee]
dao+bmo: feedback-
Details | Diff | Splinter Review

Description gadjo 2010-11-02 12:21:49 PDT
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 2010-11-02 12:22:27 PDT
Created attachment 487652 [details]
Visualization of the bug
Comment 2 Kevin Hanes 2010-11-03 10:02:22 PDT
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 2010-11-10 23:37:21 PST
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) 2010-12-13 02:36:18 PST
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 2011-01-10 10:07:25 PST
bugspam (moving b9 to b10)
Comment 6 Kevin Hanes 2011-01-10 10:10:10 PST
bugspam (removing b9)
Comment 7 Kevin Hanes 2011-01-12 16:44:11 PST
Punt
Comment 8 Dão Gottwald [:dao] 2011-01-13 00:34:06 PST
Kevin, you're wrongly changing the OS field with regularity.
Comment 9 Kevin Hanes 2011-01-13 10:18:10 PST
(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] 2011-02-05 10:09:22 PST
This should be fixed by bug 581726, I just need to get it landed.
Comment 11 gadjo 2011-02-06 14:52:17 PST
Doesn't seem to be fixed in 2011-02-06 nightly
Comment 12 Raymond Lee [:raymondlee] 2011-08-23 22:30:11 PDT
Created attachment 555327 [details] [diff] [review]
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] 2011-08-25 00:54:39 PDT
Created attachment 555672 [details] [diff] [review]
WIP2
Comment 15 Tim Taubert [:ttaubert] 2011-09-06 03:18:52 PDT
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] 2011-09-07 09:23:40 PDT
Created attachment 558842 [details] [diff] [review]
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] 2011-09-07 10:22:34 PDT
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] 2011-09-08 05:25:19 PDT
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] 2011-10-18 14:11:58 PDT
Fixed by bug 682996.

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