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

RESOLVED FIXED in 33 Sprint 2- 7/7

Status

Hello (Loop)
Client
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
33 Sprint 2- 7/7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [p=2][qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → standard8
(Assignee)

Comment 1

4 years ago
I'll also see if I can do tests for MozLoopAPI in this one.
(Assignee)

Updated

4 years ago
Target Milestone: mozilla33 → 33 Sprint 2- 7/7
(Assignee)

Comment 2

4 years ago
Created attachment 8444396 [details] [diff] [review]
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?
Attachment #8444396 - Flags: feedback?(dmose)
Attachment #8444396 - Flags: feedback?(adam)
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.

Updated

4 years ago
Attachment #8444396 - Flags: feedback?(dmose)
(Assignee)

Comment 4

4 years ago
Created attachment 8445135 [details] [diff] [review]
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?
Attachment #8444396 - Attachment is obsolete: true
Attachment #8444396 - Flags: feedback?(adam)
Attachment #8445135 - Flags: review?(ttaubert)
Attachment #8445135 - Flags: review?(dmose)
Attachment #8445135 - Flags: feedback?(adam)
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.
Attachment #8445135 - Flags: review?(dmose) → review+
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.
Attachment #8445135 - Flags: review?(ttaubert)
(Assignee)

Comment 7

4 years ago
(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.
(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.
(Assignee)

Comment 9

4 years ago
Created attachment 8447968 [details] [diff] [review]
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.
Attachment #8447968 - Flags: review?(ttaubert)
(Assignee)

Updated

4 years ago
Attachment #8445135 - Attachment is obsolete: true
Attachment #8445135 - Flags: feedback?(adam)
(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.
(Assignee)

Updated

4 years ago
Blocks: 1032741
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);
Attachment #8447968 - Flags: review?(ttaubert) → review+
(Assignee)

Updated

4 years ago
Blocks: 1033965
(Assignee)

Comment 12

4 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/694ac12a5922
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
QA Contact: anthony.s.hughes
Whiteboard: [p=2] → [p=2][qa-]

Updated

4 years ago
Blocks: 1059754
You need to log in before you can comment on or make changes to this bug.