1035348 - Make the "GET /calls" server call happen in the MozLoopService rather than the conversation window

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

For bug 1032700 we need the get of the current calls information to happen in the MozLoopService and then relevant information to be passed to the conversation window.

Comment 1

[Maire Reavy [:mreavy]]

This goes with Bug 1032700 which was moved to Fx 35

Comment 2

[Mark Banner (:standard8)]

Bug 1032700 just got moved back to 34.

Comment 3

[Mark Banner (:standard8)]

This should depend on bug 1022594, as I'm doing some work there to make the current call url data request happen before "accept" is called, which will make implementing this much easier.

Comment 4

[Mark Banner (:standard8)]

Bug 1022594 part 1 has just landed, so this is clear to move forward.

Comment 5

[Paul Kerr [:pkerr]]

Attachment: WIP Move GET/calls to MozLoopService

Work in progress

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8481870 [details] [diff] [review]
WIP Move GET/calls to MozLoopService

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

::: browser/components/loop/MozLoopAPI.jsm
@@ +133,5 @@
> +    getCallData: {
> +      enumerable: true,
> +      writable: true,
> +      value: function(loopCallId) {
> +        return JSON.stringify(MozLoopService.getCallData(loopCallId));

I just took a brief look. If you're having trouble getting objects across the mozLoop boundary, you need (and should really need to use anyway) the Cu.cloneInto function that's also used elsewhere in this file. That would save the JSON.stringify/parse translations.

Comment 7

[Paul Kerr [:pkerr]]

Attachment: WIP Move GET/calls to MozLoopService

WIP with suggested change to API data handling

Comment 8

[Paul Kerr [:pkerr]]

Attachment: Move GET/calls to MozLoopService

Comment 9

[Paul Kerr [:pkerr]]

Follow on patch needed to fix xpcshell test failure.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8482385 [details] [diff] [review]
Move GET/calls to MozLoopService

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

This looks good. I'll give this f+ until we get test fixes though.

Note that you'll also need to fix the unit (aka Marionette) tests - there's a readme in the loop directory about how to run them, but feel free to ping me if you need help.

::: browser/components/loop/MozLoopService.jsm
@@ +300,5 @@
>      // call, they'll have had enough time to see the terms of service. See
>      // bug 1046039 for background.
>      Services.prefs.setCharPref("loop.seenToS", "seen");
>  
> +    /* Request the information on the new call(s) associated with this version. */

nit: prefer // form of comments for in-function comments.

Comment 11

[Paul Kerr [:pkerr]]

Attachment: Move GET/calls to MozLoopService

bitrot fixes

Comment 12

[Paul Kerr [:pkerr]]

Attachment: Part 1: Move GET/calls to MozLoopService

Comment 13

[Paul Kerr [:pkerr]]

Attachment: Part 2: fix unit tests. 1) Added hawkRequest stub to allow completion of inbound notification setup where necessary in tests. (These tests will now only run for a debug build) 2) Removed tests of client.requestCallsInfo() which has been removed

Rework of loop tests. For the unit tests, I needed to put back some #ifdef/ifndef DEBUG bits in two places to allow the fake return of a hawkRequest. This is because the request for calls/?=version has moved from the chat window and into MozLoopService.jsm. These internal bits are only exposed in a debug build and the test as skipped for a non-debug build.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8485196 [details] [diff] [review]
Part 2: fix unit tests. 1) Added hawkRequest stub to allow completion of inbound notification setup where necessary in tests. (These tests will now only run for a debug build) 2) Removed tests of client.requestCallsInfo() which has been removed

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

::: browser/components/loop/MozLoopService.jsm
@@ +623,5 @@
>        deferred.reject("Invalid token data");
>      }
>    },
>  };
> +#ifndef DEBUG

Have you seen bug 1061756 ? I wonder if we can work around it the same way here, I'd rather avoid debug-only if possible.

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ -170,5 @@
>  
>            expect(conversation.get("loopVersion")).to.equal("fakeVersion");
>          });
>  
> -        it("should call requestCallsInfo on the client",

These tests should be replaced by the equivalents with navigator.mozLoop.getCallData stubbed.

i.e. "should call navigator.mozLoop.getCallData"
"should display an error if getCallData returns an error"
describe("getCallData successful", ...

etc.

@@ -187,5 @@
> -
> -            sinon.assert.calledOnce(notifier.errorL10n);
> -          });
> -
> -        describe("requestCallsInfo successful", function() {

There's a mismatch of braces here, and that causes a syntax error which, unfortunately, means that the conversation tests are not run.

::: browser/components/loop/test/xpcshell/head.js
@@ +26,5 @@
>  
>  // Ensure loop is always enabled for tests
>  Services.prefs.setBoolPref("loop.enabled", true);
>  
> +function hawkGetCallsRequest(uri, protocol) {

So far, we've been doing this type of thing with the fake loop server, e.g.

http://hg.mozilla.org/mozilla-central/annotate/892768985915/browser/components/loop/test/xpcshell/test_loopservice_registration.js#l56

Is there a reason we can't do that here as well?

::: browser/components/loop/test/xpcshell/xpcshell.ini
@@ +4,5 @@
>  firefox-appdir = browser
>  
>  [test_looppush_initialize.js]
>  [test_loopservice_dnd.js]
> +run-if = debug == true 

nit: whitespace at end of line (if this line stays).

Comment 15

[Paul Kerr [:pkerr]]

(In reply to Mark Banner (:standard8) from comment #14)
> Comment on attachment 8485196 [details] [diff] [review]
> Part 2: fix unit tests. 1) Added hawkRequest stub to allow completion of
> inbound notification setup where necessary in tests. (These tests will now
> only run for a debug build) 2) Removed tests of client.requestCallsInfo()
> which has been removed
> 
> Review of attachment 8485196 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/loop/MozLoopService.jsm
> @@ +623,5 @@
> >        deferred.reject("Invalid token data");
> >      }
> >    },
> >  };
> > +#ifndef DEBUG
> 
> Have you seen bug 1061756 ? I wonder if we can work around it the same way
> here, I'd rather avoid debug-only if possible.
> 
Thanks for the pointer. Yes, I believe this can relieve the need for the ifdef's.

Comment 16

[Paul Kerr [:pkerr]]

Attachment: Part 1: Move GET/calls to MozLoopService

small bug fix

Comment 17

[Paul Kerr [:pkerr]]

Attachment: Part 2: fix unit tests. 1) Added hawkRequest stub to allow completion of inbound notification setup where necessary in tests. (These tests will now only run for a debug build) 2) Removed tests of client.requestCallsInfo() which has been removed

1) Reworked units tests to not rely on DEBUG ifdefs. 2) Added new tests for the MozLoopService API getCallData call in functional tests.

Comment 18

[Paul Kerr [:pkerr]]

Attachment: Part 2: fix tests. 1) Added hawkRequest stub to allow completion of inbound notification setup where necessary in unit tests. 2) Replaced tests of client.requestCallsInfo() with mozLoop.getCallData

1) Reworked units tests to not rely on DEBUG ifdefs. 2) Added new tests for the MozLoopService API getCallData call in functional tests.

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 8486170 [details] [diff] [review]
Part 1: Move GET/calls to MozLoopService

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

::: browser/components/loop/content/js/conversation.jsx
@@ +205,1 @@
>            console.error("Failed to get the sessionData", err);

The tests were failing, and I noticed that its due to this change - err is no longer defined. Lets replace this by

console.error("Failed to get the call data");

Then we'll at least know where the notification came from.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 8486174 [details] [diff] [review]
Part 2: fix tests. 1) Added hawkRequest stub to allow completion of inbound notification setup where necessary in unit tests. 2) Replaced tests of client.requestCallsInfo() with mozLoop.getCallData

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

Ok, these changes look good with the previous comment about fixing part 1. I don't see the functional tests being added in this patch, so I'm not quite sure what you're referring to there.

If you are adding them, could you land parts 1 & 2, and do the functional tests in a follow-up patch? I've got bugs that I'm working on that I want to depend on these two as it makes them simpler.

Comment 21

[Paul Kerr [:pkerr]]

Attachment: Part 1: Move GET/calls to MozLoopService

small bug fix

Comment 22

[Paul Kerr [:pkerr]]

(In reply to Mark Banner (:standard8) from comment #20)
> Comment on attachment 8486174 [details] [diff] [review]
> Part 2: fix tests. 1) Added hawkRequest stub to allow completion of inbound
> notification setup where necessary in unit tests. 2) Replaced tests of
> client.requestCallsInfo() with mozLoop.getCallData
> 
> Review of attachment 8486174 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, these changes look good with the previous comment about fixing part 1. I
> don't see the functional tests being added in this patch, so I'm not quite
> sure what you're referring to there.
> 
> If you are adding them, could you land parts 1 & 2, and do the functional
> tests in a follow-up patch? I've got bugs that I'm working on that I want to
> depend on these two as it makes them simpler.

Sorry Mark, bad nomenclature on my part. I modified marionette tests which I referred to as function tests, in addition to xpcshell tests. I have not more tests pending.

Comment 23

[Mark Banner (:standard8)]

Ok, that's fine.

Comment 24

[Paul Kerr [:pkerr]]

Carrying forward r+ for part 1 patch.

Comment 25

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/4b9b045ef203
https://hg.mozilla.org/integration/fx-team/rev/a0b9b573cbb6

Comment 26

[Mark Banner (:standard8)]

Oh forgot to mention, I fixed a few bits of bitrot in the patches that were pushed.

Comment 27

[Mark Banner (:standard8)]

Follow-up fix as bug 1065052 changed the parameters to hawkRequest, and landed just before this:

https://hg.mozilla.org/integration/fx-team/rev/0f8b752951d9

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4b9b045ef203
https://hg.mozilla.org/mozilla-central/rev/a0b9b573cbb6
https://hg.mozilla.org/mozilla-central/rev/0f8b752951d9

Comment 29

[u279076]

Paul, does this need manual testing?

Comment 30

[Paul Kerr [:pkerr]]

Anthony, the unit test should be sufficient. Plus, this change is not meant to be observable in the loop client behavior.

Comment 31

[u279076]

Thanks Paul.

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8486396 [details] [diff] [review]
Part 1: Move GET/calls to MozLoopService

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/185c8c55c7e8
https://hg.mozilla.org/releases/mozilla-aurora/rev/2d8503a6be1f
https://hg.mozilla.org/releases/mozilla-aurora/rev/aacb6173aedb

Comment 34

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8486396 [details] [diff] [review]
Part 1: Move GET/calls to MozLoopService

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.