1020876 - Route desktop client XHRs though the mozLoop API to share hawk implementation with MozLoopService

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mark Banner (:standard8)]

Once bug 1020859 is complete, we should also route the desktop client XHRs through the mozLoop API. This is for two reasons:

1) It allows the hawk credentials implementation to re-use all the gecko code, and not require separate content-space libraries.

2) It will allow us to use the HawkClient library which automatically handles clock skew and not need to manually handle that in content space.

Note that only calls to the loop server should go through the mozLoop API.

Comment 1

[Mark Banner (:standard8)]

I'll also see if I can do tests for MozLoopAPI in this one.

Comment 2

[Mark Banner (:standard8)]

Attachment: WIP Route desktop client XHRs though the mozLoop API to share hawk implementation with MozLoopService

This is functional, there's a few XXX's and other things for me to tidy up before it lands.

Dan: Can you have a look at the resultant client.js and see what you think about splitting it out into separate files - one for desktop, the other for standalone.

The current functions are very much based on mozLoop being available or not (hence the second type of failure handler I've added in the patch), and by splitting them, I could easily remove the desktop dependency on getting the server url, which would no longer be required (as the mozLoop functions handle it).

The only other way I could think it may make sense to client.js, is if we wrote another abstraction layer which simulates mozLoop for the standlone side. My main concern is that it would seem to be a potentially unnecessary layer of abstraction and could confuse the mozLoop use for privs versus non-privs stuff.


Adam: Presumable in removing the hawk libraries we'd need to remove them from about:license as well, as we added them?

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

I think what we agreed in during our discussion was to split client.js into two files, since almost nothing is shared, and to switch to dependency injection of mozLoop for testing.  Mark, feel free to correct if I misunderstood.

Comment 4

[Mark Banner (:standard8)]

Attachment: Route desktop client XHRs though the mozLoop API to share hawk implementation with MozLoopService

Ok, I think this is now ready for review.

Tim, can you review the non-content parts? Dan will review the content parts.

Adam: as before, do we need to remove the licenses for these libs from about:license?

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8445135 [details] [diff] [review]
Route desktop client XHRs though the mozLoop API to share hawk implementation with MozLoopService

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

r=dmose on the content parts, once you've addressed the issues mentioned here as well as the few we talked about.

::: browser/components/loop/MozLoopAPI.jsm
@@ +142,5 @@
> +     * Performs a hawk based request to the loop server.
> +     *
> +     * Callback parameters:
> +     *  - {Object|null} null if success. Otherwise an object which contains
> +     *    the code, errno, error and message body relating to the failure.

Please put an example here rather than written text.

::: browser/components/loop/content/js/client.js
@@ +111,2 @@
>  
> +        // XXX split this out into two functions for better readability

Can probably kill the above comment.

::: browser/components/loop/test/desktop-local/client_test.js
@@ +70,3 @@
>        });
>  
> +      it("should call the callback with the url", function() {

Please change the "should" to be more clear that this is only the success case.

@@ +89,4 @@
>            "expiresAt": 60
>          };
>  
> +        mozLoop.hawkRequest.callsArgWith(3, null,

Maybe use hawkRequestStub intermediate var to make this a bit easier to read?

@@ +96,3 @@
>  
>          // expiresAt is in hours, and noteCallUrlExpiry wants seconds.
> +        sinon.assert.calledWithExactly(mozLoop.noteCallUrlExpiry,

Maybe asssert calledOnce also?

@@ +99,5 @@
>            60 * 60 * 60);
>        });
>  
>        it("should send an error when the request fails", function() {
> +        mozLoop.hawkRequest.callsArgWith(3, fakeErrorResMozLoop);

Ditch the MozLoop on the end of the var name, please.

@@ +154,1 @@
>          sinon.assert.calledWithMatch(callback, sinon.match(function(err) {

Presumably this wants a calledOnce as well.

@@ +164,1 @@
>          sinon.assert.calledWithMatch(callback, sinon.match(function(err) {

calledOnce here too would be nice.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8445135 [details] [diff] [review]
Route desktop client XHRs though the mozLoop API to share hawk implementation with MozLoopService

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

(Didn't take a closer look at the tests so far.)

::: browser/components/loop/MozLoopAPI.jsm
@@ +157,5 @@
> +      configurable: true,
> +      writable: true,
> +      value: function(path, method, payloadObj, callback) {
> +        // We translate from a promise to a callback, as we can't pass promises from
> +        // Promise.jsm across the priv versus unpriv boundary.

Does the same restriction apply to DOMPromises?

@@ +164,5 @@
> +        }, (error) => {
> +          let errorObj = Cu.createObjectIn(targetWindow);
> +          ["code", "errno", "error", "message"].forEach(function(property) {
> +            errorObj[property] = error[property];
> +          });

Could |Cu.cloneInto(error, errorObj)| work instead of iterating and setting all properties manually?

::: browser/components/loop/content/js/client.js
@@ +15,3 @@
>     */
>    function Client(settings) {
>      settings = settings || {};

function Client(settings = {}) {

@@ +87,2 @@
>          cb(err);
>        }.bind(this));

.bind(this) isn't really needed here.

@@ +165,5 @@
>          throw new Error("missing required parameter version");
>        }
>  
> +      this.mozLoop.hawkRequest("/calls?version=" + version, "GET", null,
> +                               (error, responseText) => {

I understand the we can't pass promises through the boundary but why isn't there a small wrapper here that turns it back into a promise? Having to use callbacks here feels like leaking an implementation detail.

::: browser/components/loop/standalone/content/js/standaloneClient.js
@@ +13,5 @@
> +   *
> +   * @param {Object} settings Settings object.
> +   */
> +  function StandaloneClient(settings) {
> +    settings = settings || {};

function StandaloneClient(settings = {}) {

@@ +15,5 @@
> +   */
> +  function StandaloneClient(settings) {
> +    settings = settings || {};
> +    if (!settings.hasOwnProperty("baseServerUrl") ||
> +        !settings.baseServerUrl) {

Isn't checking (!settings.baseServerUrl) sufficient?

@@ +45,5 @@
> +      });
> +
> +      if (properties.length <= 1) {
> +        return data[properties[0]];
> +      }

Accessing |properties[0]| when |properties.length < 1| doesn't seem like a good idea...

@@ +97,5 @@
> +      var req = $.ajax({
> +        url:         this.settings.baseServerUrl + "/calls/" + loopToken,
> +        method:      "POST",
> +        contentType: "application/json",
> +        data:        JSON.stringify({}),

data: '"{}"', (do we really need to that? can we just not pass |data|?)

@@ +101,5 @@
> +        data:        JSON.stringify({}),
> +        dataType:    "json"
> +      });
> +
> +      req.done(function(sessionData) {

req.done(sessionData => {
  ...
});

We can get rid of |.bind(this)| below that way.

@@ +105,5 @@
> +      req.done(function(sessionData) {
> +        try {
> +          cb(null, this._validate(sessionData, [
> +            "sessionId", "sessionToken", "apiKey"
> +          ]));

These required properties are fixed, they should be put in a |const| at the top of the file with a proper description.

Comment 7

[Mark Banner (:standard8)]

(In reply to Tim Taubert [:ttaubert] (away July 7th-18th) from comment #6)
> ::: browser/components/loop/MozLoopAPI.jsm
> @@ +157,5 @@
> > +      configurable: true,
> > +      writable: true,
> > +      value: function(path, method, payloadObj, callback) {
> > +        // We translate from a promise to a callback, as we can't pass promises from
> > +        // Promise.jsm across the priv versus unpriv boundary.
> 
> Does the same restriction apply to DOMPromises?

I'm looking into this.

> @@ +164,5 @@
> > +        }, (error) => {
> > +          let errorObj = Cu.createObjectIn(targetWindow);
> > +          ["code", "errno", "error", "message"].forEach(function(property) {
> > +            errorObj[property] = error[property];
> > +          });
> 
> Could |Cu.cloneInto(error, errorObj)| work instead of iterating and setting
> all properties manually?

Nice (slightly different syntax required though).

> ::: browser/components/loop/standalone/content/js/standaloneClient.js
> @@ +13,5 @@
> > +   *
> > +   * @param {Object} settings Settings object.
> > +   */
> > +  function StandaloneClient(settings) {
> > +    settings = settings || {};
> 
> function StandaloneClient(settings = {}) {

This is an ecma 6 construct, so whilst we can use it for Firefox code (e.g. client.js), we can't use it for the standalone code as that needs to run in other browsers.


> @@ +101,5 @@
> > +        data:        JSON.stringify({}),
> > +        dataType:    "json"
> > +      });
> > +
> > +      req.done(function(sessionData) {
> 
> req.done(sessionData => {
>   ...
> });
> 
> We can get rid of |.bind(this)| below that way.

Again, we're in standalone here, so we can't use ecma 6.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Mark Banner (:standard8) from comment #7)
> This is an ecma 6 construct, so whilst we can use it for Firefox code (e.g.
> client.js), we can't use it for the standalone code as that needs to run in
> other browsers.

Ah, I didn't know that was a requirement. Please ignore then.

Comment 9

[Mark Banner (:standard8)]

Attachment: Route desktop client XHRs though the mozLoop API to share hawk implementation with MozLoopService v2

This is the updated patch with the relevant changes.

Re returning promises from the MozLoopAPI - I experimented a bit with doing so, and we can do this for at least the desktop side of the code.

However, there's a few issues with getting our test infrastructure (mocha & chai) to work with promises - although there's meant to be a module that helps that.

Additionally, there's another (ensureRegistered) function on the api that could also do with switching to promises, and I think it makes sense to change these both together (and fix tests). So I'd like to propose that we do that switch in a follow-up bug later.

Comment 10

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to Mark Banner (:standard8) from comment #9)
> However, there's a few issues with getting our test infrastructure (mocha &
> chai) to work with promises - although there's meant to be a module that
> helps that.

For the records Mocha supports promises; quoting http://visionmedia.github.io/mocha/#asynchronous-code:

> Alternately, instead of using the done() callback, you can return a promise.

They also use https://github.com/domenic/chai-as-promised/ to extend the Chai expectation API.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8447968 [details] [diff] [review]
Route desktop client XHRs though the mozLoop API to share hawk implementation with MozLoopService v2

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

LGTM. I mostly skimmed the tests but they look like they do the right things. Following up with some Promise improvements seems reasonable.

::: browser/components/loop/content/js/client.js
@@ +92,2 @@
>          cb(err);
> +      });

Nit: this.mozLoop.ensureRegistered(cb);

Comment 12

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/694ac12a5922

I've raised bug 1033967 on switching the APIs to promises, and bug 1033965 on removing the licenses we no longer need.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/694ac12a5922