Desktop client needs ability to decline an incoming call

VERIFIED FIXED in Firefox 33

Status

Hello (Loop)
Client
P1
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: RT, Assigned: standard8)

Tracking

unspecified
mozilla34
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox33 verified, firefox34 verified)

Details

(Whiteboard: [p=1])

User Story

As a FF browser user (signed-in or not) receiving an incoming call notification, I can decline the incoming call so that the caller knows I don't want to answer.

Attachments

(6 attachments, 9 obsolete attachments)

57.14 KB, patch
standard8
: review+
Details | Diff | Splinter Review
1.09 KB, patch
NiKo
: review+
Details | Diff | Splinter Review
12.33 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
59.52 KB, patch
dmose
: review+
Details | Diff | Splinter Review
45.35 KB, patch
Details | Diff | Splinter Review
53.26 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)
(Reporter)

Updated

4 years ago
User Story: (updated)

Comment 1

4 years ago
XHR delete on caller party side in Fx browser.  currently sending from content

another bug already exist to move all XHR globally to Moz APIs.
Whiteboard: p=1
Should the declined client getting notified about the declining?
If so, how?

Note: most modern real time communications clients offer the user actually two choices: 'ignore' which stops the local ringer and might hide the incoming call notification (but the caller does not get notified and rings forever or until the call gets diverted to voicemail) and 'reject'/'decline' which notifies the caller about the call rejection.
(Reporter)

Comment 3

4 years ago
Yes as per the user story here, the caller should get notified that the call failed as the callee declines the incoming call request.
This matches https://bugzilla.mozilla.org/show_bug.cgi?id=1000240 which is about "call failed" notification for the caller with the following user story (please note that the caller won't know if the "call failed" is a result of call declined, callee busy, or callee not available): As a WebRTC browser user calling a reachable user who refused my call, I get notified that the callee could not be reached so that I can try again.

"Ignore" may get implemented at some point later but given this is MVP we should restrict the scope to decline only for now.

Updated

4 years ago
Assignee: nobody → dmose
Target Milestone: mozilla33 → 33 Sprint 3- 7/21

Updated

4 years ago
Assignee: dmose → standard8
(Assignee)

Comment 4

4 years ago
Created attachment 8457502 [details] [diff] [review]
Part 1. Change Loop's incoming call handling to get the call details before displaying the incoming call UI.

Here's the first step for this bug - change the incoming call handling so that we will get the details of the call from the server before the user can click accept.

We need this so that we can get the websocket url & connect to the server and tell it about the fact we're alerting, so the other end knows it is ringing even though the user hasn't clicked accept yet (additionally it'll tell us if the caller aborts the call as well). We're also going to need it to get the url details & names to display on the incoming call notification.

The only slight liberty I've taken is not displaying anything until we get the message back from the server. Bug 1035348 is going to further move the call to the server to the back-end code, so that by the time the conversation window opens, we'll already have the information from the server, and hence there might be one brief call into the mozLoopAPI to get that info. Hence once bug 1035348 any delay in displaying the UI is going to be really minimal.

Part 2 will hook up the actual websocket, this was enough of a change that I thought it was worth a separate patch.
Attachment #8457502 - Flags: review?(nperriault)
Comment on attachment 8457502 [details] [diff] [review]
Part 1. Change Loop's incoming call handling to get the call details before displaying the incoming call UI.

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

Some concerns with the semantics and router hooks, but it feels like refactoring all this would be far beyond the scope of this bug. Your move.

::: browser/components/loop/content/js/conversation.js
@@ +114,5 @@
>        this.navigate("call/ended", {trigger: true});
>      },
>  
> +    onSessionReady: function() {
> +      this.loadView(new IncomingCallView({model: this._conversation}));

I'm concerned with the semantics here. Displaying an incoming call view when the session is ready seems odd; it feels like we could be missing other cases where that hook could be called. A proper solution might be to just rename the hook and/or the associated model event.

@@ +129,5 @@
> +      this._conversation.initiate({
> +        client: new loop.Client(),
> +        loopVersion: loopVersion,
> +        outgoing: false
> +      });

It feels to me we'd really need a ConversationModel#incoming() method… "initiate" is a bit vague and probably confusing to the reader.

@@ +143,5 @@
>       * Accepts an incoming call.
>       */
>      accept: function() {
>        window.navigator.mozLoop.stopAlerting();
> +      this.startCall();

I'm wondering if calling directly this.navigate("call/ongoing", {trigger: true}); wouldn't be more explicit… and challenges the usefulness of startCall, at all…

::: browser/components/loop/content/shared/js/models.js
@@ +145,5 @@
>          options.client.requestCallInfo(this.get("loopToken"), options.callType,
>            handleResult.bind(this));
>        }
>        else {
> +        options.client.requestCallsInfo(options.loopVersion,

Model's `loopVersion` attribute seems to be no longer used and should be probably removed.

::: browser/components/loop/content/shared/js/router.js
@@ +156,5 @@
>      endCall: function() {},
>  
>      /**
> +     * Session is ready. By default it starts the call, but this may be
> +     * overriden.

This sole sentence feels like this is possibly doing too many different things… Maybe removing the default behavior and explicitly declare it wherever appropriate would be more maintainable on the long run.
(Assignee)

Comment 6

4 years ago
Created attachment 8458165 [details] [diff] [review]
WIP Part 2. Add a websocket to Loop's call-setup code to report to the other side when a call is rejected.

Here's the WIP I had of part 2. There's a lot of XXX's about things that need to be considered and handled. The main idea was to encaspulate the websocket functionality in a separate object. With the aim of giving it updates from the views, and letting the model listen to changes from it. It may need some tweaks to that.

Additionally, I'm a bit concerned about server responses or lack of, on the websocket and how we actually handle those (some may want to be follow-ups, some not).
(Assignee)

Comment 7

4 years ago
Going to be afk, so putting this back in the pool for now.
Assignee: standard8 → nobody

Updated

4 years ago
Target Milestone: 33 Sprint 3- 7/21 → mozilla34
Duplicate of this bug: 1042818
(Assignee)

Comment 9

3 years ago
Comment on attachment 8457502 [details] [diff] [review]
Part 1. Change Loop's incoming call handling to get the call details before displaying the incoming call UI.

Cancelling review request until we either address the comments, or take this forward further.
Attachment #8457502 - Flags: review?(nperriault)
Hey Mark -- Are you planning to pick this back up for this sprint?  This is one of the bugs that Chad and RT would like soon -- and then uplifted, if we think that's feasible.
Flags: needinfo?(standard8)
(Assignee)

Comment 11

3 years ago
Can do, we're probably going to want this for other standalone items soon.
Assignee: nobody → standard8
Flags: needinfo?(standard8)
(Assignee)

Updated

3 years ago
Depends on: 1045643
(Assignee)

Updated

3 years ago
Blocks: 1045643
No longer depends on: 1045643
(Assignee)

Updated

3 years ago
Blocks: 1045569
(Assignee)

Updated

3 years ago
Blocks: 1035348
(Assignee)

Updated

3 years ago
Blocks: 1000237
(Assignee)

Comment 12

3 years ago
Created attachment 8466174 [details] [diff] [review]
POC Revised Part 1 Change Loop's incoming call handling to get the call details before displaying the incoming call UI.

We discussed this offline, and came to the conclusion that the routers should be doing more of the work to get the call information rather than the model. This simplifies the model and makes it more reflecting what it needs to - just the data relating to the call.

This is a very rough proof of concept, there's quite a few XXX's and some namings need to be altered. This does, however, achieve the work to get the incoming call information before displaying the accept/reject buttons as well.

Looking for some early feedback before I tidy this further.
Attachment #8466174 - Flags: feedback?(nperriault)
Comment on attachment 8466174 [details] [diff] [review]
POC Revised Part 1 Change Loop's incoming call handling to get the call details before displaying the incoming call UI.

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

This is an improvement, though minor impl. design decisions to eventually reconsider here.

::: browser/components/loop/content/js/conversation.js
@@ +24,5 @@
> +  /**
> +   * Client for xhr requests
> +   * @type {loop.Client}
> +   */
> +  var client;

Instead of setting this to a global, I'd rather see it passed as a dependency to the router (I know we discussed this a bunch already, so no big deal and up to you).

::: browser/components/loop/content/shared/js/models.js
@@ +87,3 @@
>       */
> +    incoming: function() {
> +      this.trigger("startCall");

How about "call:incoming"?

@@ +106,3 @@
>  
> +      this.setSessionData(sessionData);
> +      this.trigger("startCall");

How about "call:outgoing"?

::: browser/components/loop/standalone/content/js/webapp.js
@@ +154,2 @@
>        this.disableForm();
> +      this.trigger("outgoing:startcall");

I'd rather see this triggered from the model.

@@ +282,4 @@
>          model: this._conversation,
>          notifier: this._notifier
> +      });
> +      startView.once("outgoing:startcall", this.startCall2, this);

While this is an original approach, I think events related to conversation state should be triggered from the conversation model.

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +28,5 @@
>    /**
> +   * Client for xhr requests
> +   * @type {loop.StandaloneClient}
> +   */
> +  var client;

Same remark as before with a globally shared client.
Attachment #8466174 - Flags: feedback?(nperriault) → feedback+
(Assignee)

Updated

3 years ago
Attachment #8457502 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8458165 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
Created attachment 8467262 [details] [diff] [review]
Part 1 Change Loop's incoming call handling to get the call details before displaying the incoming call UI. v3

Updated to address earlier comments. Still to do: tests, review function names.
Attachment #8466174 - Attachment is obsolete: true
(Assignee)

Comment 15

3 years ago
Created attachment 8467713 [details] [diff] [review]
Part 1 Change Loop's incoming call handling to get the call details before displaying the incoming call UI.

Updated per previous discussion and now fixed tests as well. This definitely improves what we've got, although I think there's room for further improvement as we touch other areas.

This is ready to go in and land, and should help unblock other work.
Attachment #8467262 - Attachment is obsolete: true
Attachment #8467713 - Flags: review?(nperriault)
Comment on attachment 8467713 [details] [diff] [review]
Part 1 Change Loop's incoming call handling to get the call details before displaying the incoming call UI.

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

LGTM, this patch definitely improves the code flow, brings a better separation of concern between the model and the server client and improves the code semantics (though some renaming with anything outgoing call related is possibly wanted).

::: browser/components/loop/content/js/conversation.jsx
@@ +169,5 @@
> +      });
> +      this._conversation.once("call:incoming", this.startCall, this);
> +      this._client.requestCallsInfo(loopVersion, (err, sessionData) => {
> +        if (err) {
> +          console.log("Failed to get the sessionData", err);

Nit: console.error

@@ +191,5 @@
>      /**
>       * Accepts an incoming call.
>       */
>      accept: function() {
> +      window.navigator.mozLoop.stopAlerting();

Nit: why prefixing with window? navigator is a global too, as it's attached to the window object. Also, whatever way we want to use it, we should do so consistently across Loop content code.

@@ +218,2 @@
>          // XXX The conversation window will be closed when this cb is triggered
>          // figure out if there is a better way to report the error to the user

While in the area, could you please file a bug about this and refer to it in the comment? Thanks.

@@ +270,4 @@
>      router = new ConversationRouter({
> +      client: client,
> +      conversation: new loop.shared.models.ConversationModel({},
> +        {sdk: OT}),

Actually while changing this and as it's been reportedly confusing lately, I'd go with:

conversation: new loop.shared.models.ConversationModel(
  {},       // Model attributes
  {sdk: OT} // Model dependencies
);

::: browser/components/loop/content/shared/js/models.js
@@ +88,5 @@
> +    /**
> +     * Used to indicate that an outgoing call should start any necessary
> +     * set-up.
> +     */
> +    setupCall: function() {

This should be setupOutgoingCall really.

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +173,5 @@
>  
>      /**
>       * Initiates the call.
>       */
>      _initiate: function() {

_initiateOutgoingCall.

@@ +178,2 @@
>        this.setState({disableCallButton: true});
> +      this.props.model.setupCall();

setupOutgoingCall()

@@ +273,5 @@
>      /**
> +     * Starts the set up of a call, obtaining the required information from the
> +     * server.
> +     */
> +    setupCall: function() {

setupOutgoingCall

::: browser/components/loop/test/standalone/webapp_test.js
@@ +185,5 @@
> +            sinon.match(function(value) {
> +              return React.addons.TestUtils.isDescriptorOfType(
> +                value, loop.webapp.StartConversationView);
> +            }));
> +        });

Nit: any specific reason these two tests have been moved further down the list?
Attachment #8467713 - Flags: review?(nperriault) → review+
(Assignee)

Comment 17

3 years ago
(In reply to Nicolas Perriault (:NiKo`) from comment #16)
> > +      window.navigator.mozLoop.stopAlerting();
> 
> Nit: why prefixing with window? navigator is a global too, as it's attached
> to the window object. Also, whatever way we want to use it, we should do so
> consistently across Loop content code.

This was just moving of existing code, I've changed it.

> ::: browser/components/loop/test/standalone/webapp_test.js
> @@ +185,5 @@
> > +            sinon.match(function(value) {
> > +              return React.addons.TestUtils.isDescriptorOfType(
> > +                value, loop.webapp.StartConversationView);
> > +            }));
> > +        });
> 
> Nit: any specific reason these two tests have been moved further down the
> list?

Nope, looks like a mistake, I've reverted it.
(Assignee)

Comment 18

3 years ago
Created attachment 8467790 [details] [diff] [review]
[checked in] Part 1 Change Loop's incoming call handling to get the call details before displaying the incoming call UI.

Updated patch for review comments.
Attachment #8467713 - Attachment is obsolete: true
Attachment #8467790 - Flags: review+
(Assignee)

Comment 19

3 years ago
Part 1 landed: https://hg.mozilla.org/integration/fx-team/rev/9632c9041c1d
Whiteboard: p=1 → [p=1][leave open]
(Assignee)

Updated

3 years ago
No longer blocks: 1035348
See Also: → bug 1035348
(Assignee)

Updated

3 years ago
No longer blocks: 1045569
See Also: → bug 1045569
(Assignee)

Comment 20

3 years ago
Created attachment 8467854 [details] [diff] [review]
[checked in] Part 1 follow-up. Fix random failure in browser/components/loop/test/standalone/index.html - use fake timers to prevent timers kicking in when we don't want them.

When part 1 landed, we saw a random failure almost straight away:

08:37:21    ERROR -  TEST-UNEXPECTED-FAIL | test_standalone_all.py TestDesktopUnits.test_units | AssertionError: build/tests/marionette/tests/browser/components/loop/test/standalone/index.html: 1 failure(s) encountered:
08:37:21    ERROR -  TEST-UNEXPECTED-FAIL | build/tests/marionette/tests/browser/components/loop/test/standalone/index.html | "before each" hook - TypeError: this.session is undefined (http://localhost:41435/build/tests/marionette/tests/browser/components/loop/content/shared/js/models.js:167)
08:37:21     INFO -  TypeError: this.session is undefined (http://localhost:41435/build/tests/marionette/tests/browser/components/loop/content/shared/js/models.js:167)
08:37:21     INFO -  process.on/global.onerror@http://localhost:41435/build/tests/marionette/tests/browser/components/loop/test/shared/vendor/mocha-1.17.1.js:5708:10
08:37:21     INFO -  Traceback (most recent call last):
08:37:21     INFO -    File "/builds/slave/test/build/tests/marionette/marionette/marionette_test.py", line 171, in run
08:37:21     INFO -      testMethod()
08:37:21     INFO -    File "/builds/slave/test/build/tests/marionette/tests/browser/components/loop/test/standalone/test_standalone_all.py", line 16, in test_units
08:37:21     INFO -      self.check_page("index.html")
08:37:21     INFO -    File "/builds/slave/test/build/tests/marionette/tests/browser/components/loop/test/shared/frontend_tester.py", line 106, in check_page
08:37:21     INFO -      raise AssertionError(self.get_failure_details(page))

The issue is that the standalone tests are now calling coversation.outgoing - which sets a timeout for the pending call. However, we're not using fake timers, so the timeout is firing at some random time later on. For most builds where the tests run quickly, this obviously isn't an issue, however a slower builder will fail occasionally.
Attachment #8467854 - Flags: review?(nperriault)
Comment on attachment 8467854 [details] [diff] [review]
[checked in] Part 1 follow-up. Fix random failure in browser/components/loop/test/standalone/index.html - use fake timers to prevent timers kicking in when we don't want them.

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

LGTM
Attachment #8467854 - Flags: review?(nperriault) → review+
(Assignee)

Updated

3 years ago
Attachment #8467790 - Attachment description: Part 1 Change Loop's incoming call handling to get the call details before displaying the incoming call UI. → [checked in] Part 1 Change Loop's incoming call handling to get the call details before displaying the incoming call UI.
(Assignee)

Comment 22

3 years ago
Comment on attachment 8467854 [details] [diff] [review]
[checked in] Part 1 follow-up. Fix random failure in browser/components/loop/test/standalone/index.html - use fake timers to prevent timers kicking in when we don't want them.

https://hg.mozilla.org/integration/fx-team/rev/d46a6ba2b40a
Attachment #8467854 - Attachment description: Part 1 follow-up. Fix random failure in browser/components/loop/test/standalone/index.html - use fake timers to prevent timers kicking in when we don't want them. → [checked in] Part 1 follow-up. Fix random failure in browser/components/loop/test/standalone/index.html - use fake timers to prevent timers kicking in when we don't want them.
https://hg.mozilla.org/mozilla-central/rev/9632c9041c1d
https://hg.mozilla.org/mozilla-central/rev/d46a6ba2b40a
(Assignee)

Updated

3 years ago
Depends on: 1050314
(Assignee)

Comment 24

3 years ago
Created attachment 8470885 [details] [diff] [review]
Websocket with promises

This is a revised part 2 implementation, based on using Promises. Whilst not every browser has promises	yet, all browsers that do support WebRTC do, so I think we'd be safe. If not, we could fallback to jQuery's promises implementation.

The bit I'm mainly looking for feedback on is the mixture of promises and events in websockets.js. I think it makes sense to use promises for expecting responses, but we'll need an event for the messages that the server sends.

This is just a wip at this stage, so I need to go back over various code, fix function names and add docs, error handling, tests etc. It does, however, work in FF and Chrome. You can monitor the console and see the websocket being connected, and then the decline message being transmitted across via the server.
Attachment #8470885 - Flags: feedback?(mdeboer)
Attachment #8470885 - Flags: feedback?(dmose)
Comment on attachment 8470885 [details] [diff] [review]
Websocket with promises

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

I've only really looked over the content side of that code.  In general, it seems reasonable.  I don't have so much experience with promises that I feel like I have a good handle on how it'll scale as the code grows more complex, but it's hard to imagine it being worse than pure callbacks or pure events, and I'd somewhat expect the clearer added structure to help.  It also looks like the big players who might get WebRTC before too long (Safari, IE) have promises under development, and if the timing turns out to be off, we should be able to polyfill.  So from my end, looks good.  I'll be interested to hear Mike's thoughts...

::: browser/components/loop/standalone/content/js/webapp.js
@@ +322,5 @@
>          });
> +        this._websocket.connect().then(function() {
> +          this.navigate("call/ongoing/" + loopToken, {
> +            trigger: true
> +          });

FWIW, in terms of code style, I suspect that separating out and naming the success and failure functions will make the code more readable because it's more explicitly split into named chunks.
Attachment #8470885 - Flags: feedback?(dmose) → feedback+
Comment on attachment 8470885 [details] [diff] [review]
Websocket with promises

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

::: browser/components/loop/content/js/conversation.jsx
@@ +193,5 @@
> +          }));
> +        }, () => {
> +          // XXX Not the ideal response, but bug 1047410 will be replacing
> +          // this by better "call failed" UI.
> +          this._notifier.errorL10n("cannot_start_call_session_not_ready");

Maybe it's a good idea to move this call to _notifier in a stub function that will be properly implemented in bug 1047410?

@@ +218,5 @@
> +        // ensure that we completely send the message.
> +        window.close();
> +      }, function() {
> +        // XXX how to handle errors here?
> +        console.log("Error declining call");

Well, for starters we could (re-)use the pref-based logging I propose in websockets.js

::: browser/components/loop/content/shared/js/websockets.js
@@ +5,5 @@
> +/* global loop:true */
> +
> +var loop = loop || {};
> +loop.shared = loop.shared || {};
> +loop.CallConnectionWebSocket = (function() {

Is this Websocket really only used for call connections?

I can imagine more use cases, just like you, otherwise you wouldn't have put it in shared ;)

@@ +37,5 @@
> +      };
> +    },
> +
> +    resolvePromise: function() {
> +      // XXX should this error?

It should at least show an error message in the console, like the `savePromise` function above does.

@@ +61,5 @@
> +      var promise = new Promise(
> +        function(resolve, reject) {
> +          this.savePromise(resolve, reject);
> +
> +          this.socket = new WebSocket(this.options.url);

Is this a singleton? Are you supposed to be able to call connect() multiple times? If this _is_ a singleton, we best rename this file to 'websocket.js'.

@@ +74,5 @@
> +
> +    send: function(data) {
> +      // XXX Maybe useful to turn this into some sort of proper debug logging function
> +      // controlled via pref?
> +      console.log("sending", data);

Yes! Maybe add a pref called `loop.debug.websocket` and use that here?

@@ +88,5 @@
> +    decline: function() {
> +      var promise = new Promise(
> +        function(resolve, reject) {
> +          this.savePromise(resolve, reject);
> +          this.expectingTerminated = true;

Tracking state like this looks fragile to me. What I expected was a solid RPC mechanism where the
1) the client would generate an ID for the message it's sending
2) the server would send the ID back for each message that's related to it (ACK, RES, etc)

This would result in an `rpc` method on this class which would return a Promise that is also tacked in a look-up table. This Promise will be resolved/ rejected in `onmessage`, which can find it in the lut. This'd also make timeout handling more specific, instead of tacking a timeout ref to `CallConnectionWebSocket`. I'd like to prevent race conditions before they happen...

@@ +95,5 @@
> +
> +      return promise;
> +    },
> +
> +    onopen: function() {

We are free to use camelCase here ;)

@@ +108,5 @@
> +    onmessage: function(event) {
> +      clearTimeout(this.responseTimer);
> +      delete this.responseTimer;
> +
> +      var msg = JSON.parse(event.data);

JSON.parse can throw.

@@ +111,5 @@
> +
> +      var msg = JSON.parse(event.data);
> +
> +      // XXX Logging function until bug 885508 is fixed?
> +      console.log(event.data);

I think this makes another case for introducing a pref...

@@ +127,5 @@
> +      }
> +    },
> +
> +    onerror: function() {
> +      console.log("websocket error");

This function would then reject _all_ promises in the lut

@@ +132,5 @@
> +      this.rejectPromise();
> +    },
> +
> +    onclose: function() {
> +      console.log("websocket closed");

Likewise.
Attachment #8470885 - Flags: feedback?(mdeboer)

Updated

3 years ago
Target Milestone: mozilla34 → 34 Sprint 2- 8/18
Status: NEW → ASSIGNED
(Assignee)

Comment 27

3 years ago
Created attachment 8472973 [details] [diff] [review]
[checked in] Part 1a Add a getLoopBoolPref function to the MozLoopAPI.

For enabling console logging when debugging the websocket, we really need a bool pref function on the API.

I separated this out from part 2 to make it easier for review, but I'll land both together.
Attachment #8472973 - Flags: review?(mdeboer)
Comment on attachment 8472973 [details] [diff] [review]
[checked in] Part 1a Add a getLoopBoolPref function to the MozLoopAPI.

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

hmm, assuming the move/ renaming of files went OK, r=me!

And thanks for making get/set pref tests one test file.
Attachment #8472973 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 29

3 years ago
Created attachment 8473249 [details] [diff] [review]
Websocket with promises v2

Updated patch for comments.

I've simplified the promise model. For now we just do a promise for the connection, which does really make sense there. I've taken out the use of timers for now, I will file a follow-up bug tomorrow to look at the timer issue and RPC model. It may mean more work later, but I think getting a base in place will help us work out what's really needed as we move forward.

I'm still writing and fixing the tests at the moment, so there may be a few more code changes, but I think the majority of the code should be fairly stable now.
Attachment #8473249 - Flags: feedback?(dmose)
Comment on attachment 8473249 [details] [diff] [review]
Websocket with promises v2

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

websockets.js definitely is cleaner this way.  That said, I'm still uncomfortable about the idea of landing this without timeout handling. 
 
One possibility would be to simply do this in two separate patches, but land them both when the second is done.  I'm not sure whether that's practical, however. 

I think we shouild, at the very least, figure out what the actual user-facing failure modes for the various clients are going to be in practice and somehow capture those in bugzilla so that they don't get lost.  It would also be good to write some it("should....") unit test descriptions without bodies so that at least we have defined timeout behavior -- both for the called and calling code.

I'm inclined to also want to handle the fact that WebSocket.send() may throw.
Attachment #8473249 - Flags: feedback?(dmose) → feedback+
(Assignee)

Comment 31

3 years ago
Comment on attachment 8472973 [details] [diff] [review]
[checked in] Part 1a Add a getLoopBoolPref function to the MozLoopAPI.

I decided to get this landed to make it easier to review the main patch. Also so I could fix up the hg rename issues before going away:

https://hg.mozilla.org/integration/fx-team/rev/f3d5c2aec04a
Attachment #8472973 - Attachment description: Part 1a Add a getLoopBoolPref function to the MozLoopAPI. → [checked in] Part 1a Add a getLoopBoolPref function to the MozLoopAPI.
(Assignee)

Updated

3 years ago
Attachment #8470885 - Attachment is obsolete: true
(Assignee)

Comment 32

3 years ago
Created attachment 8473683 [details] [diff] [review]
Part 2. Desktop client needs ability to decline an incoming call - set up a basic websocket protocol and use for both desktop and standalone UI.

This is now ready for full review. I put back in the websocket connection timeout, just in case something does go wrong. I've also completed the test updates.

I also added in detection of rejection by the standalone client. Although this is part of bug 1046959, it doesn't do the correct UI, but it does give a reasonable message, and allow us to prove that call rejection is working end-to-end (the standalone UI will now change to "Your call did not go through." as soon as the decline button is pressed at the desktop end). Bug 1046959 can be used to give the correct message for this (and more) cases.

Having an incomplete websocket protocol doesn't matter for call rejection as it is a client to client (via server) initiated message only - the server shouldn't initiate it by itself.
Attachment #8473249 - Attachment is obsolete: true
Attachment #8473683 - Flags: review?(dmose)
(Assignee)

Comment 33

3 years ago
Created attachment 8473686 [details] [diff] [review]
Part 2. Desktop client needs ability to decline an incoming call - set up a basic websocket protocol and use for both desktop and standalone UI.

Removed an obsolete XXX comment.
Attachment #8473683 - Attachment is obsolete: true
Attachment #8473683 - Flags: review?(dmose)
Attachment #8473686 - Flags: review?(dmose)
Comment on attachment 8473686 [details] [diff] [review]
Part 2. Desktop client needs ability to decline an incoming call - set up a basic websocket protocol and use for both desktop and standalone UI.

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

Thanks for the hard work!  This is looking good.  The majority of the comments are small stuff that I don't mind addressing on landing.  If it's possible to address the terminology questions and write (at least bodyless) it("should..."); statements on what we want to test w.r.t. websocket timeouts before you go, that'd be great.

::: browser/components/loop/content/js/conversation.js
@@ +238,5 @@
> +     * Declines a call and handles closing of the window.
> +     */
> +    _declineCall: function() {
> +      this._websocket.decline();
> +      // XXX Ideally we'd do this after ensuring we'd go a response from

s/go/gotten/

::: browser/components/loop/content/js/conversation.jsx
@@ +214,5 @@
> +        url: this._conversation.get("progressURL"),
> +        websocketToken: this._conversation.get("websocketToken"),
> +        callId: this._conversation.get("callId"),
> +      });
> +      this._websocket.promiseConnect().then(() => {

I expect this will break the ui-showcase in chrome (once I get it working).  Can we just use a normal function here, and where the other arrow functions are used?

@@ +238,5 @@
> +     * Declines a call and handles closing of the window.
> +     */
> +    _declineCall: function() {
> +      this._websocket.decline();
> +      // XXX Ideally we'd do this after ensuring we'd go a response from

s/go/gotten/

@@ +241,5 @@
> +      this._websocket.decline();
> +      // XXX Ideally we'd do this after ensuring we'd go a response from
> +      // the websocket. However, that's quite difficult to ensure at the moment
> +      // so we'll add it later.
> +      setTimeout(window.close, 0);

It's not obvious what the setTimeout is for.  The comment implies it might be in the hopes of winning a race more often.  It would be nice to make the comment more explicit.

::: browser/components/loop/content/shared/js/models.js
@@ +26,5 @@
>        sessionToken: undefined,     // OT session token
>        apiKey:       undefined,     // OT api key
> +      callId:       undefined,     // The callId on the server
> +      progressURL:  undefined,     // The websocket url to use for progress
> +      websocketToken: undefined,   // The token to use for websocket auth

The fact that setOutgoingSessionData has to use toString(16) makes me think it's worth making this comment explicit about the type to be used here.

::: browser/components/loop/content/shared/js/websocket.js
@@ +11,5 @@
> +  // Response timeout is 5 seconds as per API.
> +  var kResponseTimeout = 5000;
> +
> +  /**
> +   * Handles a websocket specifically for call connections.

s/call connections/a call connection/

@@ +13,5 @@
> +
> +  /**
> +   * Handles a websocket specifically for call connections.
> +   *
> +   * This is a singleton, and is expected to be created once for each

I'm not sure singleton is the word you want here, since at some point we'll all multiple calls and that will no longer be true. Just saying that there should be one for each call connection seems sufficient.

@@ +50,5 @@
> +    /**
> +     * Start the connection to the websocket.
> +     *
> +     * @return {Promise} A promise that resolves when the connection is complete,
> +     *          or rejects otherwise.

It looks to me like we have some terminology confusion here, where "connection completion" sometimes refers to the client having successfully opened a websocket connection to the websocket server, and sometimes to the client having successfully started a call with another client.  This is making it hard to reason about the code.  I think this is pretty important to fix and then re-review at least this part of the patch.  I.e. the comments above, I think, want to say that the promise resolves or rejects once the websocket server connection is open, and avoid using the word complete entirely.

@@ +139,5 @@
> +     * the server has determined the connection is terminated or connected.
> +     *
> +     * @return True if the last received state is terminated or connected.
> +     */
> +    get stateIsCompleted() {

Does this really want to be to be a public API right now?  If so, I suspect we want at least a spin-off bug with a little more unit testing around this.  If not, prefix it with an underscore.

::: browser/components/loop/content/shared/libs/sdk.js
@@ +16,5 @@
>      version: 'v2.2.7.2',         // The current version (eg. v2.0.4) (This is replaced by gradle)
>      build: '9425efe',    // The current build hash (This is replaced by gradle)
>  
>      // Whether or not to turn on debug logging by default
> +    debug: false,

Based on bugmail, I'm not totally sure whether you're intending to land this as part of this bug, but I entirely support doing so.  :-)

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ +215,5 @@
>            });
>  
> +          it("should call #_setupWebSocketAndCallView", function() {
> +
> +            router.incoming(42);

Is there some reason not to stick with "fakeVersion"?  It gives more context to the code reader...

::: browser/components/loop/test/shared/models_test.js
@@ +25,5 @@
> +      sessionId:      "sessionId",
> +      sessionToken:   "sessionToken",
> +      apiKey:         "apiKey",
> +      callType:       "callType",
> +      websocketToken: 123

The fact that we're not including all of the types that were added elsewhere in the patch but the tests are passing makes me slightly concerned that we're not testing the right thing here.  If what's actually happening is that some stuff is optional and some isn't, and we want to make that explicitly checked in the code in the future, a spinoff bug would be nice.

::: browser/components/loop/test/shared/websocket_test.js
@@ +49,5 @@
> +    });
> +  });
> +
> +  describe("constructed", function() {
> +    var websocket;

Given that this is not actually an object constructed from WebSocket(), please rename this to callConnectionSocket or callWebSocket or something else which is less confusing.

@@ +55,5 @@
> +    beforeEach(function() {
> +      websocket = new loop.CallConnectionWebSocket({
> +        url: "wss://fake/",
> +        callId: "callId",
> +        websocketToken: "7b"

Please define these params as variables, and just use the variables in the verification so that we get the advantage of JS compiler identifier checking in preventing DRY errors?

@@ +79,5 @@
> +          done();
> +        });
> +      });
> +
> +      it("should reject the promise if the connection errors", function(done) {

Shouldn't we define an error that the promise will be rejected with in this case and test for it?

@@ +89,5 @@
> +          done();
> +        });
> +      });
> +
> +      it("should reject the promise if the connection closes", function(done) {

Seems like we want to define and test for a specific rejection error, no?

@@ +117,5 @@
> +        function(done) {
> +          var promise = websocket.promiseConnect();
> +
> +          dummySocket.onmessage({
> +            data: '{"messageType":"hello","state":"init"}'

Space after the comma for readability?

@@ +149,5 @@
> +        websocket.promiseConnect();
> +      });
> +
> +      describe("Progress", function() {
> +        it("should trigger a progress event", function() {

"should trigger a progress event on the callConnection"? (or whatever you decide to rename websocket to)

@@ +212,5 @@
> +        });
> +      });
> +    });
> +  });
> +});

We need to test timeout behavior here somewhere.
Attachment #8473686 - Flags: review?(dmose) → review-
https://hg.mozilla.org/mozilla-central/rev/f3d5c2aec04a
(Assignee)

Comment 36

3 years ago
(In reply to Dan Mosedale (:dmose - needinfo? me for responses) from comment #34)
> ::: browser/components/loop/test/shared/models_test.js
> @@ +25,5 @@
> > +      sessionId:      "sessionId",
> > +      sessionToken:   "sessionToken",
> > +      apiKey:         "apiKey",
> > +      callType:       "callType",
> > +      websocketToken: 123
> 
> The fact that we're not including all of the types that were added elsewhere
> in the patch but the tests are passing makes me slightly concerned that
> we're not testing the right thing here.  If what's actually happening is
> that some stuff is optional and some isn't, and we want to make that
> explicitly checked in the code in the future, a spinoff bug would be nice.

Will file.

> @@ +212,5 @@
> > +        });
> > +      });
> > +    });
> > +  });
> > +});
> 
> We need to test timeout behavior here somewhere.

That's already tested in "should reject the promise if connection is not completed in 5 seconds" under the promiseConnect section.
(Assignee)

Comment 37

3 years ago
Created attachment 8473895 [details] [diff] [review]
[checked in] Part 2. Desktop client needs ability to decline an incoming call - set up a basic websocket protocol and use for both desktop and standalone UI.

Updated for comments.
Attachment #8473895 - Flags: review?(dmose)
(Assignee)

Updated

3 years ago
Attachment #8473686 - Attachment is obsolete: true
(Assignee)

Comment 38

3 years ago
(In reply to Mark Banner (:standard8) (away until 25th August) from comment #36)
> (In reply to Dan Mosedale (:dmose - needinfo? me for responses) from comment
> #34)
> > ::: browser/components/loop/test/shared/models_test.js
> > @@ +25,5 @@
> > > +      sessionId:      "sessionId",
> > > +      sessionToken:   "sessionToken",
> > > +      apiKey:         "apiKey",
> > > +      callType:       "callType",
> > > +      websocketToken: 123
> > 
> > The fact that we're not including all of the types that were added elsewhere
> > in the patch but the tests are passing makes me slightly concerned that
> > we're not testing the right thing here.  If what's actually happening is
> > that some stuff is optional and some isn't, and we want to make that
> > explicitly checked in the code in the future, a spinoff bug would be nice.
> 
> Will file.

Bug 1054518.
Comment on attachment 8473895 [details] [diff] [review]
[checked in] Part 2. Desktop client needs ability to decline an incoming call - set up a basic websocket protocol and use for both desktop and standalone UI.

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

Looks good, thanks!  r=dmose, with two tweaks

::: browser/components/loop/content/js/conversation.jsx
@@ +241,5 @@
> +      this._websocket.decline();
> +      // XXX Ideally we'd wait to close the window until after we have a response
> +      // from the server, to know that everything has completed successfully.
> +      // However, that's quite difficult to ensure at the moment so we'll add it
> +      // later.

This still doesn't seem like it's explicit about what function the setTimeout is serving.

::: browser/components/loop/content/shared/js/websocket.js
@@ +48,5 @@
> +  CallConnectionWebSocket.prototype = {
> +    /**
> +     * Start the connection to the websocket.
> +     *
> +     * @return {Promise} A promise that resolves when the when the websocket

Looks like "when the" is doubled there.

Updated

3 years ago
Attachment #8473895 - Flags: review?(dmose) → review-

Updated

3 years ago
Attachment #8473895 - Flags: review- → review+
(Assignee)

Comment 40

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/0d13e179eacb
Whiteboard: [p=1][leave open] → [p=1]
Target Milestone: 34 Sprint 2- 8/18 → mozilla34
https://hg.mozilla.org/mozilla-central/rev/0d13e179eacb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Verified fixed in today's Nightly.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: anthony.s.hughes
(Assignee)

Updated

3 years ago
Attachment #8473895 - Attachment description: Part 2. Desktop client needs ability to decline an incoming call - set up a basic websocket protocol and use for both desktop and standalone UI. → [checked in] Part 2. Desktop client needs ability to decline an incoming call - set up a basic websocket protocol and use for both desktop and standalone UI.
(Assignee)

Comment 43

3 years ago
Created attachment 8485114 [details] [diff] [review]
Beta port - Part 1 and bustage fix

Approval Request Comment
[Feature/regressing bug #]: bug 1034041 and related bugs, primarily when bug 1000237 is read to land.
[User impact if declined]: When bug 1000237 lands, if this is not fixed, then beta users won't be able to use Loop to generate links - the calls from the standalone client will fail, as the necessary information isn't being relayed to the loop server.
[Describe test coverage new/current, TBPL]: Currently on central and aurora, has unit tests. I've also manually tested against the patch that is in progress on bug 1000237 to ensure compatibility.
[Risks and why]: Risks only to Loop, if we don't do it, then Loop will stop working for beta users (or anyone who toggles the pref).
[String/UUID change made/needed]: None

Needs both part 1 & 2 on this bug, and both parts of bug 1045643.
Attachment #8485114 - Flags: approval-mozilla-beta?
(Assignee)

Comment 44

3 years ago
Created attachment 8485115 [details] [diff] [review]
Beta port - Part 1a & Part 2
Attachment #8485115 - Flags: approval-mozilla-beta?
Comment on attachment 8485114 [details] [diff] [review]
Beta port - Part 1 and bustage fix

I reviewed this change with mreavy. This change is almost entirely isolated to the loop client. Without this change, the client will be non functional on beta after bug 1000237 lands later this week. I'm going to approve this bug and related bug 1045643 for beta. Given that loop will be preffed off in beta in the next week, we should be very selective with any further loop related changes on beta.
Attachment #8485114 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8485115 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/e0ad01b2e26e
https://hg.mozilla.org/releases/mozilla-beta/rev/062929c9ff5d
status-firefox33: --- → fixed
status-firefox34: --- → fixed
"Your call did not go through" message displayed when the client declines the call.
Verified fixed FF 33b2 Win 7, OS X 10.9.5, Ubuntu 12.10.
Verified FF 34 in comment 42.
status-firefox33: fixed → verified
status-firefox34: fixed → verified
Start call, then end the call (press 'Exit') before the callee answers --> infinite incoming call to the callee. Is there a bug on file for this ?
Flags: needinfo?(standard8)
QA Contact: anthony.s.hughes → paul.silaghi
(Assignee)

Comment 49

3 years ago
(In reply to Paul Silaghi, QA [:pauly] from comment #48)
> Start call, then end the call (press 'Exit') before the callee answers -->
> infinite incoming call to the callee. Is there a bug on file for this ?

Bug 1067519
Flags: needinfo?(standard8)
You need to log in before you can comment on or make changes to this bug.