739320 - Send a title along with a URI sent in clients.js

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Comment 1

[Marina Samuel [:emtwo]]

Attachment: V1: Proposed patch to pass title as parameter for display.

* Note: this patch also makes sure the sender localID is used instead of syncID

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 609551 [details] [diff] [review]
V1: Proposed patch to pass title as parameter for display.

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

r+ with nits addressed.

::: services/sync/modules/engines/clients.js
@@ +199,5 @@
>      resetEngine: { args: 1, desc: "Clear temporary local data for engine" },
>      wipeAll:     { args: 0, desc: "Delete all client data for all engines" },
>      wipeEngine:  { args: 1, desc: "Delete all client data for engine" },
>      logout:      { args: 0, desc: "Log out client" },
> +    displayURI:  { args: 3, desc: "Instruct a client to display a URI" }

nit: while here, please add a trailing comma

@@ +355,3 @@
>     */
> +  sendURIToClientForDisplay: function sendURIToClientForDisplay(uri, clientId, title) {
> +    this._log.info("Sending URI to client: " + uri + " -> " + clientId + " with title '" + title + "'");

nit: wrap long line

Also, perhaps include the title inside parens e.g.

  Sending URI to client: https://www.mozilla.org/ -> foobar (Mozilla Home Page)

@@ +355,4 @@
>     */
> +  sendURIToClientForDisplay: function sendURIToClientForDisplay(uri, clientId, title) {
> +    this._log.info("Sending URI to client: " + uri + " -> " + clientId + " with title '" + title + "'");
> +    this.sendCommand("displayURI", [uri, this.localID, title], clientId);

Good catch on the ID bit!

Comment 3

[Marina Samuel [:emtwo]]

Attachment: V2: Patch to pass title as parameter for display.

gps: I don't have commit access, so if you wouldn't mind, could you commit this for me? Thanks!

Comment 4

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/978f7c2e98f5

Comment 5

[Richard Newman [:rnewman]]

QA: you'll need to watch for Bug 732147 to land in Nightly, then test with the add-on.

https://addons.mozilla.org/en-US/firefox/addon/send-tab-to-device/

gps, would you please roll up a new release of that? 0.3 won't have the change.

Comment 6

[Gregory Szorc [:gps]]

version 0.4 submitted for approval.

Comment 7

[Tony Chung [:tchung]]

(In reply to Richard Newman [:rnewman] from comment #5)
> QA: you'll need to watch for Bug 732147 to land in Nightly, then test with
> the add-on.
> 
> https://addons.mozilla.org/en-US/firefox/addon/send-tab-to-device/
> 
> gps, would you please roll up a new release of that? 0.3 won't have the
> change.

Thanks richard.  Tracy, you got this?

Comment 8

[Richard Newman [:rnewman]]

*cough*

TEST-PASS | /Users/rnewman/moz/hg/services-central/obj-ff-dbg/_tests/xpcshell/services/sync/tests/unit/test_clients_engine.js | [test_send_uri_to_client_for_display : 491] displayURI == displayURI

TEST-UNEXPECTED-FAIL | /Users/rnewman/moz/hg/services-central/obj-ff-dbg/_tests/xpcshell/services/sync/tests/unit/test_clients_engine.js | 2 == 3 - See following stack:

JS frame :: /Users/rnewman/moz/hg/services-central/testing/xpcshell/head.js :: do_throw :: line 462
JS frame :: /Users/rnewman/moz/hg/services-central/testing/xpcshell/head.js :: _do_check_eq :: line 556
JS frame :: /Users/rnewman/moz/hg/services-central/testing/xpcshell/head.js :: do_check_eq :: line 577
JS frame :: /Users/rnewman/moz/hg/services-central/obj-ff-dbg/_tests/xpcshell/services/sync/tests/unit/test_clients_engine.js :: test_send_uri_to_client_for_display :: line 492
JS frame :: /Users/rnewman/moz/hg/services-central/testing/xpcshell/head.js :: _run_next_test :: line 891
JS frame :: /Users/rnewman/moz/hg/services-central/testing/xpcshell/head.js :: <TOP_LEVEL> :: line 429

Comment 9

[Tracy Walker [:tracy]]

(In reply to Tony Chung [:tchung] from comment #7)

> Thanks richard.  Tracy, you got this?

Yes. When will inbound merge to nightly?

Comment 10

[Marina Samuel [:emtwo]]

In reply to comment 8: eek. The tests passed for me :S Did you compile?

Comment 11

[Richard Newman [:rnewman]]

(In reply to Tracy Walker [:tracy] from comment #9)

> Yes. When will inbound merge to nightly?

When a sheriff does it. There's no fixed schedule.

Comment 12

[Richard Newman [:rnewman]]

(In reply to Marina Samuel [:emtwo] from comment #10)
> In reply to comment 8: eek. The tests passed for me :S Did you compile?

Several times… and yet now it works. *shrug*

Comment 13

[Gregory Szorc [:gps]]

Attachment: Followup: Handle title when receiving command

This should have been included with the original patch. I missed it when I did the review :( I'm making up for my mistake by doing the patch myself instead of passing it off to Marina.

Comment 14

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/79d0a46d47b8

Comment 15

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/978f7c2e98f5
https://hg.mozilla.org/mozilla-central/rev/79d0a46d47b8

Comment 16

[Tracy Walker [:tracy]]

works with recently nightly and the StoD extension.  Doesn't actually open a new tab on the other device as advertised on AMO, but the tab is in the Synced Tabs list.  

(so Sent to Device just triggers a sync?)