763171 - Display received tabs

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

Android Sync landed support for a "send tab to device" feature. It is still missing features we had originally targeted, but is better than nothing. I think it is silly for Android to have this feature but not desktop, especially since a real use case is "I want to view this on a larger screen."

I think we should implement support for displaying received tabs. Sending tabs we can leave out for now. Or, we can possibly get that in for Firefox 16 as well. Either way, the sending support is tracked in bug 677372 (or somewhere else in the bug 507471 umbrella).

Receiving support is very simple. More importantly, it doesn't involve any UX (correct, rnewman?).

Comment 1

[Paul [pwd]]

Can this not already be done? I've been sending tabs between devices using https://addons.mozilla.org/en-US/firefox/addon/send-tab-to-device/ for a while.

Comment 2

[Gregory Szorc [:gps]]

(In reply to Paul [sabret00the] from comment #1)
> Can this not already be done? I've been sending tabs between devices using
> https://addons.mozilla.org/en-US/firefox/addon/send-tab-to-device/ for a
> while.

Yes, the core Sync API supports the "open tab" command. But, it isn't hooked up to anything in the Firefox core. The add-on you mentioned simply provides that plumbing. IMO it is silly for this feature to just work on Android but require an add-on on desktop. So, the proposal is to "uplift" the receiving/display tab to Firefox core.

Comment 3

[Richard Newman [:rnewman]]

(In reply to Paul [sabret00the] from comment #1)
> Can this not already be done? I've been sending tabs between devices using
> https://addons.mozilla.org/en-US/firefox/addon/send-tab-to-device/ for a
> while.

Y'know Greg wrote that addon, right?

(In reply to Gregory Szorc [:gps] from comment #2)
> So, the proposal is to "uplift" the
> receiving/display tab to Firefox core.

Seems reasonable to me. We can tweak behavior later, but having *something* work is important.

Marking this as blocking Bug 740193, just to capture the usage dependency.

Comment 4

[Paul [pwd]]

(In reply to Richard Newman [:rnewman] from comment #3)
> Y'know Greg wrote that addon, right?

I totally missed that. D'oh!

Comment 5

[Gregory Szorc [:gps]]

Attachment: Open tab for received URIs

This gets the job done.

Do we not have mochitests covering the UI aspects of Sync? If so, where are they? If we don't, I should probably at least get TPS coverage.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 632867 [details] [diff] [review]
Open tab for received URIs

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

r+ with safety question addressed.

::: browser/base/content/browser-syncui.js
@@ +404,5 @@
> +    if (!gBrowser) {
> +      return;
> +    }
> +
> +    gBrowser.addTab(data.wrappedJSObject.object.uri);

Are you confident in the safety of this? This is going to bomb if `data` isn't a wrapped object…

I'd be inclined to code defensively here, because you're just passing through the body of an observer notification.

Comment 7

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/61aec3b7bae5

Added a try..catch with a Cu.reportError, just in case.

I'll file a follow-up to figure out automated test coverage.

For now, QA can verify by sending a tab with Fennec. Desktop should open it on the next sync. There is no way to send tabs from desktop yet.

I'll also need to update my "Send Tab to Device" extension on AMO so it detects Firefox 16 and doesn't perform a redundant tab open.

Comment 8

[Tracy Walker [:tracy]]

I can verify this works on Mac. But on Win XP, the sent tab is not displayed.

Comment 9

[Tracy Walker [:tracy]]

nix last comment about Windows.  I had the wrong build. Got the correct s-c build and this works.

Comment 10

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/61aec3b7bae5

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Neil points out on IRC that this code seems to run in every window. So if you have multiple windows open, the sent tab will open in all of them. Was that tested?

Comment 12

[Gregory Szorc [:gps]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #11)
> Neil points out on IRC that this code seems to run in every window. So if
> you have multiple windows open, the sent tab will open in all of them. Was
> that tested?

Probably not. Will submit follow-up patch for Firefox 16.

Comment 13

[Gregory Szorc [:gps]]

Attachment: Part 2: Move display URI logic to nsBrowserGlue

This patch moves the logic for displaying received tabs to nsBrowserGlue.js. I've tested this with multiple windows open and confirmed that the tab is only opened in 1 window now.

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 637702 [details] [diff] [review]
Part 2: Move display URI logic to nsBrowserGlue

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

Looks sane to me.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 637702 [details] [diff] [review]
Part 2: Move display URI logic to nsBrowserGlue

>+  _onDisplaySyncURI: function _onDisplaySyncURI(data) {
>+    let browser = this.getMostRecentBrowserWindow().gBrowser;

gBrowser is more accurately referred to as "tabbrowser" (using "browser" is an unfortunate mistake much of our existing code makes - it's confusing because a tabbrowser contains several browsers).

>+    if (!browser) {

You can safely assume this will never happen (getMostRecentBrowserWindow() won't return closed windows).

>+    try {
>+      browser.addTab(data.wrappedJSObject.object.uri);

Should the tab be selected? I'm not really familiar with the desired UX for this feature. It might be simpler to just use this.getMostRecentBrowserWindow().browserDOMWindow.openURI(makeURI(url), null, Ci.nsIBrowserDOMWindow.OPEN_NEWTAB, 0);, which should obey the tab prefs that affect external link openings.

Comment 16

[Gregory Szorc [:gps]]

Attachment: Part 2: Move display URI logic to nsBrowserGlue, v2

I addressed Gavin's feedback. I also answered Gavin's questions as code comments. Comments are good.

Comment 17

[Gregory Szorc [:gps]]

Attachment: Part 2: Move display URI logic to nsBrowserGlue, v3

Forgot a qref. Sorry for the bugspam.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 637939 [details] [diff] [review]
Part 2: Move display URI logic to nsBrowserGlue, v3

Seems like we could test (part of) this with a browser-chrome test that just fires off the notification and checks the results. I assume you've tested the real-life functionality end-to-end, because I haven't!

Comment 19

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/9d0bf5dddfc4

Wiping qa+ whiteboard because this needs reverified.

I've tested this real-life functionality end-to-end.

Bug 767663 is on file to implement automated tests. I've never done a mochitest before (shocking, I know). I guess now I get to learn!

Comment 20

[Tracy Walker [:tracy]]

looks good on s-c build from this morning.  Sharing tab via Fx Sync then shows the tab only in the current desktop window. Pushed tab also doesn't steal focus, as expected.

Comment 21

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/9d0bf5dddfc4