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

VERIFIED FIXED in mozilla14

Status

Cloud Services
Firefox Sync: Backend
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: emtwo, Assigned: emtwo)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa!])

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 1

6 years ago
Created attachment 609551 [details] [diff] [review]
V1: Proposed patch to pass title as parameter for display.

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

Updated

6 years ago
Attachment #609551 - Flags: review?(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!
Attachment #609551 - Flags: review?(gps) → review+
(Assignee)

Comment 3

6 years ago
Created attachment 609590 [details]
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!
Attachment #609551 - Attachment is obsolete: true
https://hg.mozilla.org/services/services-central/rev/978f7c2e98f5
Whiteboard: [fixed in services]
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.
Blocks: 732147
Status: NEW → ASSIGNED
Whiteboard: [fixed in services] → [fixed in services][qa+]
version 0.4 submitted for approval.

Comment 7

6 years ago
(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?
*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
(In reply to Tony Chung [:tchung] from comment #7)

> Thanks richard.  Tracy, you got this?

Yes. When will inbound merge to nightly?
(Assignee)

Comment 10

6 years ago
In reply to comment 8: eek. The tests passed for me :S Did you compile?
Status: ASSIGNED → NEW
(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.
Status: NEW → ASSIGNED
(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*
Created attachment 610250 [details] [diff] [review]
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.
Attachment #610250 - Flags: review?(rnewman)
Attachment #610250 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/services/services-central/rev/79d0a46d47b8
https://hg.mozilla.org/mozilla-central/rev/978f7c2e98f5
https://hg.mozilla.org/mozilla-central/rev/79d0a46d47b8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services][qa+] → [qa+]
Target Milestone: --- → mozilla14
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?)
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.