The desktop client should return a reason of "busy" if: * Another incoming call request is being processed but not answered yet * The desktop client is on a call The callee won't see any notification - the ability to be notified of an incoming call whilst on a call will be addressed post MVP.. The caller will see a notification as per https://bugzilla.mozilla.org/show_bug.cgi?id=1000186 Currently these types of incoming calls are silently lost.

Adam: the user story I'm proposing here is going to require two gets to the server for the same info. I'm not convinced that is the best thing. Either we need to split up the GET /calls into two (the call with progressUrl & then the full call info), or we see if we can find some other way of passing the full call information from the loop service to the conversation window (query params?).

I think this would be a good bug to move up in priority if its solution is not too involved.

(In reply to Mark Banner (:standard8) from comment #1) > Adam: the user story I'm proposing here is going to require two gets to the > server for the same info. I'm not convinced that is the best thing. > > Either we need to split up the GET /calls into two (the call with > progressUrl & then the full call info), or we see if we can find some other > way of passing the full call information from the loop service to the > conversation window (query params?). It would seem a bit odd to change the network API significantly just to accommodate the model we've chosen for our privilege isolation. I think we should solve this by having the service perform the GET /calls, and then store the corresponding information under a key (callId?) that is then passed in as the parameter to openChatWindow (rather than the version). The conversation window would then do something like "navagator.mozLoopAPI.getCallInfo(callId)" to get the entire structure. Does that make sense?

Oh, I forgot to mention -- in addition to the service needing to know about call end events to know whether to reply to a call as busy, we also need to use these events to erase the stored call information I talk about above.

create separate (blocking) bug for get on calls from conversation window into Mozloop service. this bug is busy notification.

(In reply to sescalante from comment #5) > create separate (blocking) bug for get on calls from conversation window > into Mozloop service. this bug is busy notification. Split out to bug 1035348

I discussed this with Maire yesterday, and we decided this should probably be addressed in Fx 34, as currently there is the possibility for the user to have multiple concurrent calls (or at least in a call & receive another incoming call), which we don't want at this time.

Work in progress - must be applied after bug 1035348

Work in progress - must be applied after bug 1035348

WIP update

Comment on attachment 8482386 [details] [diff] [review] LoopService reject with reason = busy an incoming call when busy Review of attachment 8482386 [details] [diff] [review]: ----------------------------------------------------------------- I've not tested this patch, as there's a few bits of bitrot unfortunately, however, I like the idea of where it is going, and I think this does what we want. ::: browser/components/loop/content/js/conversation.jsx @@ +171,5 @@ > /** > * @override {loop.shared.router.BaseConversationRouter.endCall} > */ > endCall: function() { > + navigator.mozLoop.releaseCallData(this._conversation.get("callId")); Might need to ensure we do this on window close as well, i.e. if the user clicks the 'x'.

bitrot fixes

I'll try and look at this patch tomorrow.

Added releaseCallData checks to functions tests.

Comment on attachment 8486172 [details] [diff] [review] LoopService reject with reason = busy an incoming call when busy Review of attachment 8486172 [details] [diff] [review]: ----------------------------------------------------------------- There's a few comments below that need addressing. What I'd really also like is to get an xpcshell test to test the busy notifications. I think you should be able to do this by using a combination of what's in test_looppush_initialize and some of the other tests (e.g. test_loopservice_dnd). If you need help or more ideas, let me know. ::: browser/components/loop/MozLoopService.jsm @@ +110,5 @@ > + (reason => {console.warn("MozLoopService::callProgessSocket - ", reason);}); > + > + // Allow _websocket to be set for testing. > + if (!this._websocket && !Services.io.offline) { > + this._websocket = Cc["@mozilla.org/network/protocol;1?name=wss"] This needs the protocol type to be based on the scheme of the url passed in. Currently, I can't test this as-is on localhost as it tries to use the secure socket. I would suggest moving the newURI call to before this section, and then using uri.scheme to get the right protocol type. @@ +206,5 @@ > + try { > + this._websocket.sendMsg(JSON.stringify(aMsg)); > + } > + catch (error) { > + console.error("MozLoopService::_send - ", error); Should this be calling this._onError? @@ +532,5 @@ > + // This instance of CallProgressSocket should stay alive until the underlying > + // websocket is closed since it is passed to the websocket as the nsIWebSocketListener. > + callProgress.connect(() => {callProgress.sendBusy();}); > + } > + catch (error) {} Is there a reason not to log the error here? What's the expected failure case? ::: browser/components/loop/content/js/conversation.jsx @@ +175,5 @@ > /** > * @override {loop.shared.router.BaseConversationRouter.endCall} > */ > endCall: function() { > + navigator.mozLoop.releaseCallData(this._conversation.get("callId")); At the moment, this isn't happening if I simply close the window using the 'x' - the service thinks we're still in the busy state. I suspect we need some form of onclose handler.

> > @@ +532,5 @@ > > + // This instance of CallProgressSocket should stay alive until the underlying > > + // websocket is closed since it is passed to the websocket as the nsIWebSocketListener. > > + callProgress.connect(() => {callProgress.sendBusy();}); > > + } > > + catch (error) {} > > Is there a reason not to log the error here? What's the expected failure > case? > Any expected problem should be handled by the CallProgressSocket instance. So, this catch should not be necessary.

> > ::: browser/components/loop/content/js/conversation.jsx > @@ +175,5 @@ > > /** > > * @override {loop.shared.router.BaseConversationRouter.endCall} > > */ > > endCall: function() { > > + navigator.mozLoop.releaseCallData(this._conversation.get("callId")); > > At the moment, this isn't happening if I simply close the window using the > 'x' - the service thinks we're still in the busy state. I suspect we need > some form of onclose handler. I added an unload event handler to make sure closing the chat window still clears the call.

Fixed: chat window direct close issue, websocket uri method hard coded to wss, and nits. Additional tests to follow.

fix bit-rot

bit rot fixes

Moved tests to a follow on bug (bug 1070065) to allow QA testing of the full busy/reject notification chain.

Added error message for required argument for CallProgressSocket.connect()

Comment on attachment 8492301 [details] [diff] [review] LoopService reject with reason = busy an incoming call when busy I am continuing to work on the busy/reject unit test in a separate bug to allow check-in of the functional work. Taking more time than expected to merge with the new version of the tests.

fixed typo

Comment on attachment 8492342 [details] [diff] [review] LoopService reject with reason = busy an incoming call when busy Review of attachment 8492342 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. I skimmed the patch and looked over the changes to address the last review comments. ::: browser/components/loop/MozLoopService.jsm @@ +113,5 @@ > + this._onError = onError || > + (reason => {console.warn("MozLoopService::callProgessSocket - ", reason);}); > + > + if (!onSuccess) { > + this._onError("missing onSucces argument"); Nit: missing an "s" in onSuccess here

Paul, can you please test this in the latest Nightly and the Try-server build here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rjesup@wgate.com-f9eb2cbac352 We should also make sure this gets a Moztrap test for Loop 34/35.

(In reply to Romain Testard [:RT] from comment #0) > The caller will see a notification as per > https://bugzilla.mozilla.org/show_bug.cgi?id=1000186 > Currently these types of incoming calls are silently lost. So, I don't see any 'busy' notification right now, the call simply ends, but I guess that's expected due to the above, right ?

This bug adds functionality in the MozLoopService client that will send back a reject/busy call progress message on the call progress websocket. That notification would then be passed on to the calling client.

So, is there a way to verify this ?

Since the call progress websocket is over TLS, I captured the http to websocket upgrade and that a packet was sent to the Loop server.

The only way you've really got at the moment from the UI, is to see that when you try a call, it returns to the main screen straight away. There's a non-UI possibility - on the link clicker end, open a web console, and enter: localStorage.setItem("debug.websocket", true); When you then try connecting a call, you should find that there's a progress message with terminated in it and a reason of "busy". To reset that "pref", do: localStorage.removeItem("debug.websocket"); (In reply to Paul Silaghi, QA [:pauly] from comment #32) > (In reply to Romain Testard [:RT] from comment #0) > > The caller will see a notification as per > > https://bugzilla.mozilla.org/show_bug.cgi?id=1000186 > > Currently these types of incoming calls are silently lost. > So, I don't see any 'busy' notification right now, the call simply ends, but > I guess that's expected due to the above, right ? We don't have exact UX yet. When bug 1000240 gets deployed, then I think you'll see the fact that "something went wrong".

Thanks Mark. Verified fixed FF 35.0a1 (2014-10-03) and the try build

Comment on attachment 8492342 [details] [diff] [review] LoopService reject with reason = busy an incoming call when busy Approval Request Comment Uplift request for patches staged and tested on Fig

Comment on attachment 8492342 [details] [diff] [review] LoopService reject with reason = busy an incoming call when busy 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.

Mark, I've added a draft smoketest to Moztrap. Can you please review it for accuracy and completeness? https://moztrap.mozilla.org/manage/case/14888/

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #41) > Mark, I've added a draft smoketest to Moztrap. Can you please review it for > accuracy and completeness? > > https://moztrap.mozilla.org/manage/case/14888/ Yes, that looks good, thanks.