Closed Bug 763171 Opened 8 years ago Closed 8 years ago

Display received tabs

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: gps, Assigned: gps)

References

(Blocks 1 open bug)

Details

(Whiteboard: [str in comment 7])

Attachments

(2 files, 2 obsolete files)

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?).
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.
(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.
(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.
Blocks: 740193
(In reply to Richard Newman [:rnewman] from comment #3)
> Y'know Greg wrote that addon, right?

I totally missed that. D'oh!
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.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #632867 - Flags: review?(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.
Attachment #632867 - Flags: review?(rnewman) → review+
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.
Whiteboard: [fixed in services]
Target Milestone: --- → mozilla16
Blocks: 767663
Blocks: 767664
Whiteboard: [fixed in services] → [fixed in services][str in comment 7][qa+]
I can verify this works on Mac. But on Win XP, the sent tab is not displayed.
nix last comment about Windows.  I had the wrong build. Got the correct s-c build and this works.
Whiteboard: [fixed in services][str in comment 7][qa+] → [verified in services][str in comment 7][qa+]
https://hg.mozilla.org/mozilla-central/rev/61aec3b7bae5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [verified in services][str in comment 7][qa+] → [str in comment 7][qa+]
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?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #637702 - Flags: review?(gavin.sharp)
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 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.
I addressed Gavin's feedback. I also answered Gavin's questions as code comments. Comments are good.
Attachment #637702 - Attachment is obsolete: true
Attachment #637702 - Flags: review?(gavin.sharp)
Attachment #637938 - Flags: review?(gavin.sharp)
Forgot a qref. Sorry for the bugspam.
Attachment #637938 - Attachment is obsolete: true
Attachment #637938 - Flags: review?(gavin.sharp)
Attachment #637939 - Flags: review?(gavin.sharp)
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!
Attachment #637939 - Flags: review?(gavin.sharp) → review+
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!
Status: REOPENED → ASSIGNED
Whiteboard: [str in comment 7][qa+] → [str in comment 7][fixed in services]
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.
Whiteboard: [str in comment 7][fixed in services] → [str in comment 7][verified in services]
https://hg.mozilla.org/mozilla-central/rev/9d0bf5dddfc4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: [str in comment 7][verified in services] → [str in comment 7]
Depends on: 836172
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.