Last Comment Bug 656913 - Search results show wrong (old) tab title
: Search results show wrong (old) tab title
Status: VERIFIED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 7
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on:
Blocks: 660175
  Show dependency treegraph
 
Reported: 2011-05-13 07:30 PDT by Vlad [QA]
Modified: 2016-04-12 14:00 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Panorama search bug (120.19 KB, image/jpeg)
2011-05-13 07:30 PDT, Vlad [QA]
no flags Details
v1 (4.73 KB, patch)
2011-06-03 01:55 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Splinter Review
v2 (4.81 KB, patch)
2011-06-03 11:18 PDT, Raymond Lee [:raymondlee]
ian: review+
Details | Diff | Splinter Review
Patch for checkin (5.06 KB, patch)
2011-06-06 20:39 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Vlad [QA] 2011-05-13 07:30:06 PDT
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
Comment 1 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-05-27 02:24:28 PDT
bugspam
Comment 2 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-05-27 02:29:29 PDT
bugspam
Comment 3 Raymond Lee [:raymondlee] 2011-06-03 01:55:59 PDT
Created attachment 537111 [details] [diff] [review]
v1
Comment 4 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-03 04:01:37 PDT
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 :)
Comment 5 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-03 04:12:37 PDT
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.
Comment 6 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-03 04:17:29 PDT
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?
Comment 7 Raymond Lee [:raymondlee] 2011-06-03 11:03:54 PDT
(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.
Comment 8 Raymond Lee [:raymondlee] 2011-06-03 11:18:54 PDT
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()
Comment 9 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-06 04:50:29 PDT
(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 10 Ian Gilman [:iangilman] 2011-06-06 09:58:55 PDT
Comment on attachment 537188 [details] [diff] [review]
v2

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

Looks good.
Comment 11 Raymond Lee [:raymondlee] 2011-06-06 20:39:31 PDT
Created attachment 537723 [details] [diff] [review]
Patch for checkin
Comment 12 Raymond Lee [:raymondlee] 2011-06-07 02:02:55 PDT
Passed Try
http://tbpl.mozilla.org/?tree=Try&rev=ddd50c053895
Comment 13 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-07 02:31:20 PDT
http://hg.mozilla.org/mozilla-central/rev/f2cb2a8f5814
Comment 14 George Carstoiu 2011-06-10 02:19:19 PDT
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.

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