676375 - Send Tab to Device - Core API

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

This bug tracks the implementation of the core API feature to support "Send Tab to Device" (tracked in bug 507471).

This bug should not involve any UI changes.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Send tab to device API v1

This is my first pass at the API for sending a tab to a device.

Currently, a tab is defined as a string URI, so you can think of the core API as "send URI to client X."

Patch overview:

* New API: Service.sendUriToClient(uri, clientId) sends a URI to a client specified by client ID. The URI is sent via a command.
* Command APIs were changed to allow sending a command to a specific client (previously, commands were broadcasted to every client)
* When a "displayURI" command is received, an observer is notified. Eventually, gSyncUI will listen for this event and display a new tab.
* Added remoteIds data to Clients engine. This will eventually be needed by the UI component of the patch. I decided to leave it in this patch because it touches the core.
* Patch includes basic unit test, which passes.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 550512 [details] [diff] [review]
Send tab to device API v1

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

A pile of nits and style stuff, and one substantive comment.

Did not bother to address clients.js yet, because that will change.

Expect parallel comments from philikon :)

::: services/sync/modules/service.js
@@ +1871,5 @@
>          engine.resetClient();
>      }))(),
>  
>    /**
> +   * Called when a display URI command is received

Nit: punctuation.

@@ +1873,5 @@
>  
>    /**
> +   * Called when a display URI command is received
> +   *
> +   * @param uri String URI that was received

Nit: please put the description on the next line. See how prepCommand does it elsewhere in this file.

@@ +1876,5 @@
> +   *
> +   * @param uri String URI that was received
> +   * @param clientId ID of client that sent URI
> +   */
> +  displayReceivedUri: function displayReceivedUri(uri, clientId) {

s/Uri/URI/g

@@ +1893,5 @@
>      ["resetEngine", 1, "Clear temporary local data for engine"],
>      ["wipeAll", 0, "Delete all client data for all engines"],
>      ["wipeEngine", 1, "Delete all client data for engine"],
>      ["logout", 0, "Log out client"],
> +    ["displayURI", 2, "Sends/receives a URI between devices/clients"],

How about "Instruct a client to display the provided URI"?

@@ +1972,5 @@
>        return;
>      }
>  
> +    if (clientId) {
> +      this._log.debug("Sending to client " + clientId + ": " + [command, args, commandData.desc]);

This won't print as neatly as you think it might.

@@ +1983,5 @@
>    },
>  
>    /**
>     * Fetch storage info from the server.
> +   *

Whitespace change...

@@ +2036,5 @@
> +  sendUriToClient: function sendUriToClient(uri, clientId) {
> +    this._log.info("Sending URI to client: " + uri + " -> " + clientId);
> +    this.prepCommand("displayURI", [ uri, this.syncID], clientId);
> +
> +    // TODO is it correct to access "private" member here?

This is what you might call a red flag :)

IMO, no. What you've done is added functionality to Service that should probably be part of the Clients engine, or refactored separately. When Clients detects a new outgoing command, it can bump its own score.

If you look at prepCommand, all it does is poke at Clients. I'd be tempted to pull out _commands and prepCommand, clean them up, and bundle them somewhere else. That avoids you having to put sendURIToClient into Service.

::: services/sync/tests/unit/test_service_display_uri.js
@@ +6,5 @@
> +Cu.import("resource://services-sync/service.js");
> +Cu.import("resource://services-sync/log4moz.js");
> +
> +add_test(function test_send_uri_to_client() {
> +  _("verifies the semantics of sendUriToClient() work");

Nit: capitalization and punctuation. Also, a better note would be:

  _("Verifies that Service.sendURIToClient results in a client command being queued.");

because that's what the test does!

@@ +21,5 @@
> +  let remoteRecord = store.createRecord(remoteId, "clients");
> +
> +  let uri = "http://www.mozilla.org/";
> +
> +  Service.sendUriToClient(uri, remoteId);

Can we get sendURIToClient instead? That seems to have ended up as our convention for 'modern' Sync code.

@@ +35,5 @@
> +  do_check_eq(command.args[0], uri);
> +
> +  run_next_test();
> +});
> +

Missing test: what happens if you attempt to send a URI to a client ID that's no longer present in the remote set?

@@ +40,5 @@
> +add_test(function test_receive_display_uri() {
> +  _("verifies that the callback on receipt of displayURI command is invoked");
> +
> +  let uri = "http://www.mozilla.org/";
> +  let identity = Utils.makeGUID();

Not sure "identity" is the right name for this... how about "sender"?

@@ +51,5 @@
> +  Clients.localCommands = [command];
> +  let count = 0;
> +
> +  Svc.Obs.add("weave:service:uri-received", function(subject, data) {
> +    count++;

Also log, and add do_check_eqs for whatever should be in subject and data. If the notification code breaks and starts sending empty notifications, this test won't fail.

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to comment #2)
> > +  sendUriToClient: function sendUriToClient(uri, clientId) {
> > +    this._log.info("Sending URI to client: " + uri + " -> " + clientId);
> > +    this.prepCommand("displayURI", [ uri, this.syncID], clientId);
> > +
> > +    // TODO is it correct to access "private" member here?
> 
> This is what you might call a red flag :)
> 
> IMO, no. What you've done is added functionality to Service that should
> probably be part of the Clients engine, or refactored separately. When
> Clients detects a new outgoing command, it can bump its own score.
> 
> If you look at prepCommand, all it does is poke at Clients. I'd be tempted
> to pull out _commands and prepCommand, clean them up, and bundle them
> somewhere else. That avoids you having to put sendURIToClient into Service.

Amen to that. To be fair, gps *did* suggest that himself. So yeah, let's pull prepCommand out of Service and move it into Clients. Maybe it should even be factored into sendCommand?

That takes care of sending a command. The receiving a command logic sadly still is in Service. Ideally we'd pull that out, too. We can do it now or in a follow-up (I'd prefer now, but I don't feel strongly about it.)

Comment 4

[Gregory Szorc [:gps]]

(In reply to comment #2)
 
> @@ +2036,5 @@
> > +  sendUriToClient: function sendUriToClient(uri, clientId) {
> > +    this._log.info("Sending URI to client: " + uri + " -> " + clientId);
> > +    this.prepCommand("displayURI", [ uri, this.syncID], clientId);
> > +
> > +    // TODO is it correct to access "private" member here?
> 
> This is what you might call a red flag :)
> 
> IMO, no. What you've done is added functionality to Service that should
> probably be part of the Clients engine, or refactored separately. When
> Clients detects a new outgoing command, it can bump its own score.
> 
> If you look at prepCommand, all it does is poke at Clients. I'd be tempted
> to pull out _commands and prepCommand, clean them up, and bundle them
> somewhere else. That avoids you having to put sendURIToClient into Service.

This was my single biggest concern as well. I was kinda forced to put the API in Service b/c that's where existing commands code is. Talking with philikon here, it sounds like the eventual solution will be to consolidate all the commands code to the clients engine.

Unless there are objections, I'll file a bug to move command code to the clients engine then resubmit this patch on top of that.

Comment 5

[Richard Newman [:rnewman]]

(In reply to comment #4)

> Unless there are objections, I'll file a bug to move command code to the
> clients engine then resubmit this patch on top of that.

+1 from me on that. Feel free to do more than just move it, too: make it better and write good tests :)

Comment 6

[Gregory Szorc [:gps]]

Attachment: Display URI on Client Implementation

This patch implements the core "send URI for display (on device)" API in the Sync Clients engine.

Patch includes unit tests. Unit test suite has passed on my local build.

The patch has changed significantly since the last upload because the command API has been refactored. Feedback from previous review has been incorporated, where appropriate.

One "nit" question I have is regarding the formatting of the definition of Clients.sendURIToClientForDisplay(). I'm just not sure what the formatting convention is for long lines in this case.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 551526 [details] [diff] [review]
Display URI on Client Implementation

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

This looks pretty good to me. Perhaps refresh the commit message to include the word "command"; right now it kinda suggests the whole feature.

::: services/sync/modules/engines/clients.js
@@ +351,5 @@
> +   *        ID of client to send the command to. If not defined, will be sent
> +   *        to all remote clients.
> +   */
> +  sendURIToClientForDisplay:
> +    function sendURIToClientForDisplay(uri, clientId) {

Doesn't this fit on the same line? Vim tells me it's 80 chars.

@@ +353,5 @@
> +   */
> +  sendURIToClientForDisplay:
> +    function sendURIToClientForDisplay(uri, clientId) {
> +
> +    this._log.info("Sending URI to client: " + uri + " -> " + clientId);

Question: this will be called outside the scope of a Sync session. What happens wrt logOnSuccess/logOnError? Does the log info get dumped at all?

Answering this would be good before you land, if only for my peace of mind. If it doesn't go anywhere, then we either want to remove the logging, or fix the logger so it doesn't blackhole.

@@ +354,5 @@
> +  sendURIToClientForDisplay:
> +    function sendURIToClientForDisplay(uri, clientId) {
> +
> +    this._log.info("Sending URI to client: " + uri + " -> " + clientId);
> +    this.sendCommand("displayURI", [ uri, this.syncID], clientId);

Nit: space before "uri" should go.

@@ +372,5 @@
> +   *
> +   * The 'data' parameter to the callback will not be defined.
> +   *
> +   * @param uri String URI that was received
> +   * @param clientId ID of client that sent URI

Copy the way you've done it for sendURIToClientForDisplay.

 * @param clientId
 *        ID of client to ...

@@ +375,5 @@
> +   * @param uri String URI that was received
> +   * @param clientId ID of client that sent URI
> +   */
> +  _handleDisplayURI: function _handleDisplayURI(uri, clientId) {
> +    this._log.debug("Received a URI for display: " + uri + " from " + clientId);

OK, so why is this log a debug, but the sender's log an info?

I'm not fussed either way, but they should probably align.

::: services/sync/tests/unit/test_clients_engine.js
@@ +505,5 @@
> +  let remoteId = Utils.makeGUID();
> +
> +  let command = {
> +    command: "displayURI",
> +    args: [ uri, remoteId ],

Nit: cuddle [].

@@ +526,5 @@
> +  };
> +
> +  Svc.Obs.add(ev, handler);
> +
> +  do_check_true(Clients.processIncomingCommands());

\o/ on this check :)

Comment 8

[Philipp von Weitershausen [:philikon]]

(In reply to Richard Newman [:rnewman] from comment #7)
> Question: this will be called outside the scope of a Sync session. What
> happens wrt logOnSuccess/logOnError? Does the log info get dumped at all?
> 
> Answering this would be good before you land, if only for my peace of mind.
> If it doesn't go anywhere, then we either want to remove the logging, or fix
> the logger so it doesn't blackhole.

Stuff that gets logged after a sync has completed appears at the beginning of the log of the next sync. So if logOnSuccess is false, it gets discarded. Otherwise it appears at the top of that sync's log.

Comment 9

[Gregory Szorc [:gps]]

> ::: services/sync/modules/engines/clients.js
> @@ +351,5 @@
> > +   *        ID of client to send the command to. If not defined, will be sent
> > +   *        to all remote clients.
> > +   */
> > +  sendURIToClientForDisplay:
> > +    function sendURIToClientForDisplay(uri, clientId) {
> 
> Doesn't this fit on the same line? Vim tells me it's 80 chars.

It is exactly 80 characters. I've grown up with the 80-characters-includes-the-LF mentality. Your comment implies we only count printable characters here, so I'll change it.

Comment 10

[Gregory Szorc [:gps]]

Attachment: Display URI on Client Implementation, v2

This patch incorporates feedback from previous r+. Commit message reflects r+ from rnewman, so it should be ready for commit.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 551543 [details] [diff] [review]
Display URI on Client Implementation, v2

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

Nice.

Comment 12

[Richard Newman [:rnewman]]

Will land this after the current train has left the station.

Comment 13

[Richard Newman [:rnewman]]

Landed on s-c:

  http://hg.mozilla.org/services/services-central/rev/0815c4e5b498

Comment 14

[Richard Newman [:rnewman]]

http://hg.mozilla.org/mozilla-central/rev/0815c4e5b498