Closed Bug 595236 Opened 9 years ago Closed 9 years ago

Match Tabs From Other Windows in Panorama Search

Categories

(Firefox Graveyard :: Panorama, defect, P1)

defect

Tracking

(blocking2.0 beta7+)

VERIFIED FIXED
Firefox 4.0b7
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: aza, Assigned: seanedunn)

References

Details

Attachments

(3 files, 9 obsolete files)

Because we were not able to get all tabs from all windows for Firefox 4, and because the research from the Test Pilot team shows that multiple-window browsing is used by the majority of users, this bug adds a stop-gap measure:

Panorama is supposed to be the one place where you go to look for your tab. Without all-tabs we are left in a world with a window>group>tab hierarchy. Finding a tab now becomes a difficult thing if it is hiding in another window. While you can use the tab-matching in the awesome bar, we'd prefer to allow the mental model of "always find it in Panorama". Thus, we need a feature which allows you to see tab matches from other windows while performing a search in Panorama.
Assignee: nobody → aza
blocking2.0: --- → ?
Priority: -- → P1
Target Milestone: --- → Firefox 4.0b6
Blocks: 593902
Attached file Patch v1 (obsolete) —
Attachment #474120 - Flags: feedback?(ian)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Forgot the remove firebug lite which I was using for debugging purposes. It is now removed from the patch.
Attachment #474120 - Attachment is obsolete: true
Attachment #474122 - Flags: feedback?(ian)
Attachment #474120 - Flags: feedback?(ian)
Attachment #474122 - Attachment is patch: true
Comment on attachment 474122 [details] [diff] [review]
Patch v1.1

Looks good to me. 

I could see TabUtils being useful elsewhere in Tab Candy... perhaps at some point we can migrate it to utils.jsm
Attachment #474122 - Flags: feedback?(ian) → feedback+
Attachment #474122 - Flags: review?(dietrich)
Flags: in-litmus?
blocking2.0: ? → beta6+
Attachment #474122 - Flags: review?(dietrich) → review?(dolske)
Whiteboard: [has patch][needs review dolske]
Blocks: 594094
Comment on attachment 474122 [details] [diff] [review]
Patch v1.1

Reverting review request to dietrich. Dolske, please let us know if you have any comments.
Attachment #474122 - Flags: review?(dolske) → review?(dietrich)
Whiteboard: [has patch][needs review dolske] → [has patch][needs review dietrich]
Comment on attachment 474122 [details] [diff] [review]
Patch v1.1


> // ##########
>+// Class: TabUtils
>+// 
>+// A collection of helper functions for dealing with both
>+// <TabItem>s and <xul:tab>s without having to worry which
>+// one is which.

i'm not a fan of this approach at all; it lets a million branching codepaths bloom. we can live with it for now, but please file a followup on using TabItem everywhere instead.

>+  // ---------
>+  // Function: favURLOf
>+  // Given a <TabItem> or a <xul:tab> returns the URL of tab's favicon.
>+  favURLOf: function TabUtils_favURLOf(tab) {
>+    return tab.image != undefined ? tab.image : tab.favEl.src;
>+  },

expand to faviconURLOf.

the code changes are ok, outside of the comments above. however, r- for lack of tests.
Attachment #474122 - Flags: review?(dietrich) → review-
Whiteboard: [has patch][needs review dietrich] → [has patch]
(In reply to comment #6)
> > // ##########
> >+// Class: TabUtils
> >+// 
> >+// A collection of helper functions for dealing with both
> >+// <TabItem>s and <xul:tab>s without having to worry which
> >+// one is which.
> 
> i'm not a fan of this approach at all; it lets a million branching codepaths
> bloom. we can live with it for now, but please file a followup on using TabItem
> everywhere instead.

Actually, now that we support app tabs, they don't have TabItems associated with them; this means it's possible to need to deal with tabs that don't have TabItems. It doesn't make sense for app tabs to have TabItems associated with them; most of the TabItem code is about the UI specifics of normal tabs, none of which applies to app tabs. 

Perhaps we need some other sort of class that handles the various Panorama needs for tabs that are common to both normal tabs and app tabs.
Attached patch Patch v2 (obsolete) — Splinter Review
Still requires test cases.
Attachment #474122 - Attachment is obsolete: true
Filed follow-up bug 596073 to discuss TabUtils.
Attached patch Tests - WIP (obsolete) — Splinter Review
I wrote these tests for this bug.  The test is failing like this:

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_multiwindow_search.js | Match something when the whole title exists - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_multiwindow_search.js | Match something when a part of title exists - Got 1, expected 2

I'm not sure if this is a problem in the original patch or a problem with the test...
Ehsan,

In your test you have the following block:

 newWindows.forEach(function(win) {
    tabNames.push(win.gBrowser.selectedBrowser.contentDocument.title);
 });

This doesn't get all the tabs inside those windows. Is that what the test is supposed to test on when it does a search?
Attached patch Tests -WIP (obsolete) — Splinter Review
Modified Ehsan's test to query all tabs in other windows, call matchedTabsFromOtherWindows(), and changed the test to only expect one "New Tab" tab, since the tabs that exist for this test are "New Tab", "data:text/html", and "mochitest index /".
Attachment #474917 - Attachment is obsolete: true
Attachment #475010 - Flags: review?(dietrich)
Attachment #475010 - Flags: feedback?(ehsan)
Comment on attachment 475010 [details] [diff] [review]
Tests -WIP

get feedback+ from ehsan first, and then request review of a single unified patch (code changes + test).
Attachment #475010 - Flags: review?(dietrich)
Comment on attachment 475010 [details] [diff] [review]
Tests -WIP

This test breaks browser_tabview_search.js, which is the test run right after it in the suite.
Attachment #475010 - Flags: feedback?(ehsan) → feedback-
Attached patch Version 2.1 (obsolete) — Splinter Review
I had previously uploaded the wrong version of the patch that didn't have the change to .faviconURLOf(). Fixed now.
Attachment #474866 - Attachment is obsolete: true
Attached patch v1 (combined patch and tests) (obsolete) — Splinter Review
Hangs were caused by tests not properly closing the search button before continuing with next test. Also, now properly cleaning up when windows close using window watcher observer.
Attachment #475010 - Attachment is obsolete: true
Attachment #475252 - Attachment is obsolete: true
Attachment #475303 - Flags: feedback?(ehsan)
Comment on attachment 475303 [details] [diff] [review]
v1 (combined patch and tests)

Looks good to me.
Attachment #475303 - Flags: feedback?(ehsan) → feedback+
Attached file v3 (combined patch and tests) (obsolete) —
Resubmitting with correct version# (v3)
Attachment #475303 - Attachment is obsolete: true
Attached patch v3 (combined patch and tests) (obsolete) — Splinter Review
(resubmitting again because I love to forget to click the patch box)
Attachment #475309 - Attachment is obsolete: true
Everything should be properly folded together now.
Attachment #475310 - Attachment is obsolete: true
Attachment #475327 - Flags: review?(dietrich)
Attachment #475327 - Flags: review?(dietrich) → review+
pushed to try
Blocks: 597043
Assignee: aza → seanedunn
Depends on: 597071
This bug is currently dependent upon 597071, which is causing our tests to fail since we open windows before the private browsing mochitests are run.
Whiteboard: [has patch] → [has patch][needs 597071 to land]
Bug 597071 is now reviewed, so these can both land!
Whiteboard: [has patch][needs 597071 to land] → [has patch][needs 597071 to land][can land]
Attachment #475327 - Flags: approval2.0?
Attachment #475327 - Flags: approval2.0?
Sent to try with 597071
Were the try results ok?  Bug 597071 suggests they might not have been?

I added checkin-needed, but if it shouldn't be landed yet, please remove both checkin-needed and [can land].
Pushed http://hg.mozilla.org/mozilla-central/rev/1dd70e6dfe61
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][needs 597071 to land][can land]
Depends on: 598691
this bug isn't fixed on Windows OS's (Win 7 and XP tested). There isn't a strip at the bottom of the view as there is on Mac. Instead it's just text and the favicon in the upper left of the view.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Looks like the CSS isn't in the right place for winstripe or, I presume, gnomestripe
Whiteboard: [needs new patch]
I'll post a new patch here in a few minutes.
I tested this locally (FWIW).
Attachment #480691 - Flags: review?(dao)
Whiteboard: [needs new patch] → [needs review dao]
Comment on attachment 480691 [details] [diff] [review]
Port the theme changes to winstripe as well

-moz-box-shadow is obsolete, use box-shadow instead. Also, add whitespace before {, after : and around >.
Attachment #480691 - Flags: review?(dao) → review+
Whiteboard: [needs review dao]
http://hg.mozilla.org/mozilla-central/rev/6a2ce1ab7d88
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
I noticed http://hg.mozilla.org/mozilla-central/rev/4e12a34e5398 today, which appears to be a followup to this. Should it be landed on the b7 relbranch as well as m-c default, as this bug is a beta7+ blocker?
Reopening so Dao can answer comment 33.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #33)
> I noticed http://hg.mozilla.org/mozilla-central/rev/4e12a34e5398 today, which
> appears to be a followup to this. Should it be landed on the b7 relbranch as
> well as m-c default, as this bug is a beta7+ blocker?

It's not needed for beta 7 since -moz-box-shadow continues to work as an alias for box-shadow.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
verified with recent nightly trunk builds
Status: RESOLVED → VERIFIED
Search results disappear on maximizing window but stay while restoring.
test case https://litmus.mozilla.org/show_test.cgi?id=13771 added to litmus BFT's
Flags: in-litmus? → in-litmus+
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.