Closed
Bug 1032700
Opened 10 years ago
Closed 10 years ago
Desktop client needs to reject incoming calls with a reason of "busy" if already on a call.
Categories
(Hello (Loop) :: Client, enhancement, P1)
Hello (Loop)
Client
Tracking
(firefox34 verified, firefox35 verified)
People
(Reporter: RT, Assigned: pkerr)
References
Details
(Whiteboard: p=2[loop-uplift][loop-inccall1][fig:verified])
User Story
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 Rough Impl (after bug 1035348 is implemented): For the purposes of this bug, it is suggested that an "active" call is one that is in the state from where the conversation window is opened, until the call is ended or it is closed. - MozLoopService will need to know when calls have finished, this could be via an event that MozLoopService listens to. - For each call notification, check if a conversation window is "active" -- if not, open a new conversation window. -- if there is, open a websocket to the server, and reject the call with busy.
Attachments
(1 file, 11 obsolete files)
20.26 KB,
patch
|
MattN
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Comment 1•10 years ago
|
||
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?).
User Story: (updated)
Flags: needinfo?(adam)
Comment 2•10 years ago
|
||
I think this would be a good bug to move up in priority if its solution is not too involved.
Target Milestone: mozilla34 → mozilla33
Comment 3•10 years ago
|
||
(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?
Flags: needinfo?(adam)
Comment 4•10 years ago
|
||
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.
Comment 5•10 years ago
|
||
create separate (blocking) bug for get on calls from conversation window into Mozloop service. this bug is busy notification.
Whiteboard: p=2
Comment 6•10 years ago
|
||
(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
Updated•10 years ago
|
User Story: (updated)
Updated•10 years ago
|
Priority: P2 → P3
Target Milestone: mozilla33 → mozilla34
Updated•10 years ago
|
Target Milestone: mozilla34 → mozilla35
Comment 8•10 years ago
|
||
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.
Target Milestone: mozilla35 → mozilla34
Updated•10 years ago
|
Priority: P3 → P1
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → pkerr
Updated•10 years ago
|
Status: NEW → ASSIGNED
Updated•10 years ago
|
Target Milestone: mozilla34 → 34 Sprint 3- 9/1
Assignee | ||
Comment 9•10 years ago
|
||
Work in progress - must be applied after bug 1035348
Assignee | ||
Comment 10•10 years ago
|
||
Work in progress - must be applied after bug 1035348
Assignee | ||
Updated•10 years ago
|
Attachment #8482026 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
WIP update
Assignee | ||
Updated•10 years ago
|
Attachment #8482030 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8482371 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8482386 -
Flags: review?(standard8)
Comment 13•10 years ago
|
||
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'.
Attachment #8482386 -
Flags: review?(standard8) → feedback+
Assignee | ||
Comment 14•10 years ago
|
||
bitrot fixes
Assignee | ||
Updated•10 years ago
|
Attachment #8482386 -
Attachment is obsolete: true
Assignee | ||
Comment 15•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8483784 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8485198 -
Flags: review?(standard8)
Comment 16•10 years ago
|
||
I'll try and look at this patch tomorrow.
Assignee | ||
Comment 17•10 years ago
|
||
Added releaseCallData checks to functions tests.
Assignee | ||
Updated•10 years ago
|
Attachment #8485198 -
Attachment is obsolete: true
Attachment #8485198 -
Flags: review?(standard8)
Assignee | ||
Updated•10 years ago
|
Attachment #8486172 -
Flags: review?(standard8)
Updated•10 years ago
|
Whiteboard: p=2 → p=2[loop-uplift]
Updated•10 years ago
|
Iteration: --- → 35.1
Comment 18•10 years ago
|
||
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.
Attachment #8486172 -
Flags: review?(standard8)
Attachment #8486172 -
Flags: review-
Attachment #8486172 -
Flags: feedback+
Updated•10 years ago
|
Updated•10 years ago
|
Whiteboard: p=2[loop-uplift] → p=2[loop-uplift][loop-inccall1]
Assignee | ||
Comment 19•10 years ago
|
||
>
> @@ +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.
Assignee | ||
Comment 20•10 years ago
|
||
>
> ::: 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.
Assignee | ||
Comment 21•10 years ago
|
||
Fixed: chat window direct close issue, websocket uri method hard coded to wss, and nits. Additional tests to follow.
Assignee | ||
Updated•10 years ago
|
Attachment #8486172 -
Attachment is obsolete: true
Assignee | ||
Comment 22•10 years ago
|
||
fix bit-rot
Assignee | ||
Updated•10 years ago
|
Attachment #8488908 -
Attachment is obsolete: true
Assignee | ||
Comment 23•10 years ago
|
||
bit rot fixes
Assignee | ||
Updated•10 years ago
|
Attachment #8490153 -
Attachment is obsolete: true
Assignee | ||
Comment 24•10 years ago
|
||
Moved tests to a follow on bug (bug 1070065) to allow QA testing of the full busy/reject notification chain.
Assignee | ||
Comment 25•10 years ago
|
||
Added error message for required argument for CallProgressSocket.connect()
Assignee | ||
Updated•10 years ago
|
Attachment #8492219 -
Attachment is obsolete: true
Assignee | ||
Comment 26•10 years ago
|
||
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.
Attachment #8492301 -
Flags: review?(standard8)
Assignee | ||
Updated•10 years ago
|
Attachment #8492301 -
Flags: review?(standard8) → review?(MattN+bmo)
Assignee | ||
Comment 27•10 years ago
|
||
fixed typo
Assignee | ||
Updated•10 years ago
|
Attachment #8492301 -
Attachment is obsolete: true
Attachment #8492301 -
Flags: review?(MattN+bmo)
Assignee | ||
Updated•10 years ago
|
Attachment #8492342 -
Flags: review?(MattN+bmo)
Updated•10 years ago
|
Target Milestone: 34 Sprint 3- 9/1 → ---
Comment 28•10 years ago
|
||
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
Attachment #8492342 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 29•10 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: p=2[loop-uplift][loop-inccall1] → p=2[loop-uplift][loop-inccall1][fixed-in-fx-team]
Comment 30•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: p=2[loop-uplift][loop-inccall1][fixed-in-fx-team] → p=2[loop-uplift][loop-inccall1]
Target Milestone: --- → mozilla35
Comment 31•10 years ago
|
||
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.
Flags: needinfo?(paul.silaghi)
QA Contact: anthony.s.hughes → paul.silaghi
Whiteboard: p=2[loop-uplift][loop-inccall1] → p=2[loop-uplift][loop-inccall1][fig:verifyme]
Comment 32•10 years ago
|
||
(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 ?
Flags: needinfo?(paul.silaghi) → needinfo?(rtestard)
Assignee | ||
Comment 33•10 years ago
|
||
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.
Assignee | ||
Comment 35•10 years ago
|
||
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.
Flags: needinfo?(pkerr)
Comment 36•10 years ago
|
||
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".
Comment 37•10 years ago
|
||
Thanks Mark.
Verified fixed FF 35.0a1 (2014-10-03) and the try build
Status: RESOLVED → VERIFIED
status-firefox35:
--- → verified
Flags: needinfo?(rtestard)
Whiteboard: p=2[loop-uplift][loop-inccall1][fig:verifyme] → p=2[loop-uplift][loop-inccall1][fig:verified]
Comment 38•10 years ago
|
||
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
Attachment #8492342 -
Flags: approval-mozilla-aurora?
Comment 39•10 years ago
|
||
Updated•10 years ago
|
status-firefox34:
--- → verified
Comment 40•10 years ago
|
||
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.
Attachment #8492342 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 41•10 years ago
|
||
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/
Flags: needinfo?(standard8)
Flags: in-moztrap?
Flags: in-moztrap+
Comment 42•10 years ago
|
||
(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.
Flags: needinfo?(standard8)
You need to log in
before you can comment on or make changes to this bug.
Description
•