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

bugspam

bugspam

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

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?

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

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

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

Passed Try http://tbpl.mozilla.org/?tree=Try&rev=ddd50c053895

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.