Closed Bug 1022594 Opened 11 years ago Closed 10 years ago

Desktop client needs ability to decline an incoming call

Categories

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

defect

Tracking

(firefox33 verified, firefox34 verified)

VERIFIED FIXED
mozilla34
Tracking Status
firefox33 --- verified
firefox34 --- verified

People

(Reporter: RT, Assigned: standard8)

References

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 files, 9 obsolete files)

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
dmosedale
: review+
Details | Diff | Splinter Review
45.35 KB, patch
Details | Diff | Splinter Review
53.26 KB, patch
Details | Diff | Splinter Review
No description provided.
User Story: (updated)
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.
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.
Assignee: nobody → dmose
Target Milestone: mozilla33 → 33 Sprint 3- 7/21
Assignee: dmose → standard8
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.
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).
Going to be afk, so putting this back in the pool for now.
Assignee: standard8 → nobody
Target Milestone: 33 Sprint 3- 7/21 → mozilla34
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)
Can do, we're probably going to want this for other standalone items soon.
Assignee: nobody → standard8
Flags: needinfo?(standard8)
Depends on: 1045643
Blocks: 1045643
No longer depends on: 1045643
Blocks: 1045569
Blocks: 1035348
Blocks: 1000237
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+
Attachment #8457502 - Attachment is obsolete: true
Attachment #8458165 - Attachment is obsolete: true
Updated to address earlier comments. Still to do: tests, review function names.
Attachment #8466174 - Attachment is obsolete: true
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+
(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.
Whiteboard: p=1 → [p=1][leave open]
No longer blocks: 1035348
See Also: → 1035348
No longer blocks: 1045569
See Also: → 1045569
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+
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.
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.
Depends on: 1050314
Attached patch Websocket with promises (obsolete) — Splinter Review
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)
Target Milestone: mozilla34 → 34 Sprint 2- 8/18
Status: NEW → ASSIGNED
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+
Attached patch Websocket with promises v2 (obsolete) — Splinter Review
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+
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.
Attachment #8470885 - Attachment is obsolete: true
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)
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-
(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.
Attachment #8473686 - Attachment is obsolete: true
(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.
Attachment #8473895 - Flags: review?(dmose) → review-
Attachment #8473895 - Flags: review- → review+
Whiteboard: [p=1][leave open] → [p=1]
Target Milestone: 34 Sprint 2- 8/18 → mozilla34
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Verified fixed in today's Nightly.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: anthony.s.hughes
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.
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?
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+
"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.
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
(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.

Attachment

General

Created:
Updated:
Size: