The default bug view has changed. See this FAQ.

Search results show wrong (old) tab title

VERIFIED FIXED in Firefox 7

Status

Firefox Graveyard
Panorama
VERIFIED FIXED
6 years ago
a year ago

People

(Reporter: Vlad [QA], Assigned: raymondlee)

Tracking

Trunk
Firefox 7

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 532221 [details]
Panorama search bug

Build identifier: Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110512 Firefox/6.0a1

Panorama Search function does not bring up the expected result.

First, use an empty testing profile. (Don't install any addons into it)
http://support.mozilla.com/kb/Basic+Troubleshooting#w_8-make-a-new-profile

Steps to reproduce:
1. Open two tabs (aol.com, cnn.com).
2. Enter Panorama and rename the group (TestGroup).
4. Exit Panorama 
5. Delete one tab, say cnn.com
6. In the focus tab (aol.com), navigate to yahoo.com
7. Open a new window (ctrl+n).
8. Enter Panorama in the newly opened window and search for yahoo.com

Actual results:

Searching for yahoo.com brings up another search result (aol.com) instead of yahoo.com

Expected results:

Searching for yahoo.com should bring up the yahoo result.

Regression range 

Works for me on:
Mozilla/5.0 (Windows NT 5.1; rv:2.0b10pre) Gecko/20110117 Firefox/4.0b10pre
http://hg.mozilla.org/mozilla-central/rev/66addc5c30ca


Reproducible on:
Mozilla/5.0 (Windows NT 5.1; rv:2.0b10pre) Gecko/20110118 Firefox/4.0b10pre
http://hg.mozilla.org/mozilla-central/rev/eb105fe0e41c

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=66addc5c30ca&tochange=eb105fe0e41c
Blocks: 653099
OS: Windows 7 → All
Hardware: x86 → All
Summary: Panorama Search function does not bring up the expected result. → Search results show wrong (old) tab title
bugspam
No longer blocks: 653099
bugspam
Blocks: 660175
(Assignee)

Comment 3

6 years ago
Created attachment 537111 [details] [diff] [review]
v1
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #537111 - Flags: feedback?(tim.taubert)
Comment on attachment 537111 [details] [diff] [review]
v1

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

The test does not fail when the patch is not applied, so it seems to not really test the described issue.

::: browser/base/content/tabview/search.js
@@ +209,5 @@
>    // does not match the the search term.
>    _filterForUnmatches: function TabMatcher__filterForUnmatches(tabs) {
>      var self = this;
>      return tabs.filter(function(tab) {
> +      let name = tab.$tabTitle[0].textContent;

What exactly was/is the source of the bug? Why does that fix it?

@@ +232,5 @@
>        // This function gets tabs from other windows, not from the current window
>        if (win != gWindow) {
> +        Array.forEach(win.gBrowser.tabs, function(tab) {
> +          allTabs.push(tab);
> +        });

Why not just use "allTabs.push.apply(allTabs, win.gBrowser.tabs)"? And thanks for the cleanup here :)
Attachment #537111 - Flags: feedback?(tim.taubert) → feedback-
Comment on attachment 537111 [details] [diff] [review]
v1

My bad, I get it now. The cleanup part was actually part of the fix. So we're accessing the actual tab.label first and only if that is null we access our possibly not up-to-date tabItem title.

The test fails without the patch - so all good, sorry.
Attachment #537111 - Flags: feedback- → feedback+
Thinking about it a bit more this actually feels like a symptom fix. Shouldn't our tabItem label be up-to-date so we can use that for the filtering?
(Assignee)

Comment 7

6 years ago
(In reply to comment #6)
> Thinking about it a bit more this actually feels like a symptom fix.
> Shouldn't our tabItem label be up-to-date so we can use that for the
> filtering?

I don't think that it's worth to update tabItem label and fav icon, just for this search feature.  One use case: user goes into Panorama and back to the normal browser UI, he browses around using several tabs and will never go back to Panorama in this browsing session, then he opens a second window and does a search.  Therefore, it may not make sense to update tabItems' labels and fav icons in the first window in this case. 

Also, why we need to get the tabItems' labels and fav icons from Panorama instead of from tabs in different windows?  They have exactly the same labels and fav icons and also in some windows Panorama may not be initialized so tabItems are not available.
(Assignee)

Comment 8

6 years ago
Created attachment 537188 [details] [diff] [review]
v2

(In reply to comment #4)
> Comment on attachment 537111 [details] [diff] [review] [review]
> v1
> > 
> @@ +232,5 @@
> >        // This function gets tabs from other windows, not from the current window
> >        if (win != gWindow) {
> > +        Array.forEach(win.gBrowser.tabs, function(tab) {
> > +          allTabs.push(tab);
> > +        });
> 
> Why not just use "allTabs.push.apply(allTabs, win.gBrowser.tabs)"? And
> thanks for the cleanup here :)

Updated to use allTabs.push.apply()
Attachment #537111 - Attachment is obsolete: true
Attachment #537188 - Flags: review?(ian)
(In reply to comment #7)
> I don't think that it's worth to update tabItem label and fav icon, just for
> this search feature.  One use case: user goes into Panorama and back to the
> normal browser UI, he browses around using several tabs and will never go
> back to Panorama in this browsing session, then he opens a second window and
> does a search.  Therefore, it may not make sense to update tabItems' labels
> and fav icons in the first window in this case.

Yeah, I get it now. We're only updating when returning to Panorama so the tab label isn't up-to-date.
Comment on attachment 537188 [details] [diff] [review]
v2

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

Looks good.
Attachment #537188 - Flags: review?(ian) → review+
(Assignee)

Comment 11

6 years ago
Created attachment 537723 [details] [diff] [review]
Patch for checkin
Attachment #537188 - Attachment is obsolete: true
(Assignee)

Comment 12

6 years ago
Passed Try
http://tbpl.mozilla.org/?tree=Try&rev=ddd50c053895
http://hg.mozilla.org/mozilla-central/rev/f2cb2a8f5814
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7

Comment 14

6 years ago
Verified issue on  Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110609 Firefox/7.0a1 and also on WinXP, Win7 and Ubuntu 11.04 x86 using the steps to reproduce from the Description - problem no longer exists.

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