595236 - Match Tabs From Other Windows in Panorama Search

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P1)

Description

[Aza Raskin [:aza]]

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.

Comment 1

[Aza Raskin [:aza]]

Attachment: Patch v1

Comment 2

[Aza Raskin [:aza]]

How it looks in practice: http://img.skitch.com/20100910-bp4c5ein3hge8yrc264hj1k6n.png

Comment 3

[Aza Raskin [:aza]]

Attachment: Patch v1.1

Forgot the remove firebug lite which I was using for debugging purposes. It is now removed from the patch.

Comment 4

[Ian Gilman [:iangilman]]

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

Comment 5

[Ian Gilman [:iangilman]]

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.

Comment 6

[Dietrich Ayala (: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.

Comment 7

[Ian Gilman [:iangilman]]

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

Comment 8

[Aza Raskin [:aza]]

Attachment: Patch v2

Still requires test cases.

Comment 9

[Ian Gilman [:iangilman]]

Filed follow-up bug 596073 to discuss TabUtils.

Comment 10

[(no longer active)]

Attachment: Tests - WIP

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

Comment 11

[Sean Dunn]

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?

Comment 12

[Sean Dunn]

Attachment: Tests -WIP

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

Comment 13

[Dietrich Ayala (:dietrich)]

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

Comment 14

[(no longer active)]

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.

Comment 15

[Aza Raskin [:aza]]

Attachment: Version 2.1

I had previously uploaded the wrong version of the patch that didn't have the change to .faviconURLOf(). Fixed now.

Comment 16

[Sean Dunn]

Attachment: v1 (combined patch and tests)

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.

Comment 17

[(no longer active)]

Comment on attachment 475303 [details] [diff] [review]
v1 (combined patch and tests)

Looks good to me.

Comment 18

[Sean Dunn]

Attachment: v3 (combined patch and tests)

Resubmitting with correct version# (v3)

Comment 19

[Sean Dunn]

Attachment: v3 (combined patch and tests)

(resubmitting again because I love to forget to click the patch box)

Comment 20

[Sean Dunn]

Attachment: v4 (combined patch and tests with Aza's latest)

Everything should be properly folded together now.

Comment 21

[Ian Gilman [:iangilman]]

pushed to try

Comment 22

[Sean Dunn]

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.

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Bug 597071 is now reviewed, so these can both land!

Comment 24

[Sean Dunn]

Sent to try with 597071

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 26

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/mozilla-central/rev/1dd70e6dfe61

Comment 27

[Tracy Walker [:tracy]]

Attachment: bad display of tabs from other windows on Windows OS's

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.

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Looks like the CSS isn't in the right place for winstripe or, I presume, gnomestripe

Comment 29

[(no longer active)]

I'll post a new patch here in a few minutes.

Comment 30

[(no longer active)]

Attachment: Port the theme changes to winstripe as well

I tested this locally (FWIW).

Comment 31

[Dão Gottwald [: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 >.

Comment 32

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/6a2ce1ab7d88

Comment 33

[Jonathan Kew [:jfkthame]]

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?

Comment 34

[Mike Beltzner [:beltzner, not reading bugmail]]

Reopening so Dao can answer comment 33.

Comment 35

[Dão Gottwald [:dao]]

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

Comment 36

[Tracy Walker [:tracy]]

verified with recent nightly trunk builds

Comment 37

[Siddhartha Dugar [:sdrocking]]

Search results disappear on maximizing window but stay while restoring.

Comment 38

[Tracy Walker [:tracy]]

test case https://litmus.mozilla.org/show_test.cgi?id=13771 added to litmus BFT's