Desktop client should notify the server of acceptance and media-up during incoming call requests, Standalone client should notify the server of media-up during outgoing calls

RESOLVED FIXED in Firefox 33

Status

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

People

(Reporter: standard8, Assigned: standard8)

Tracking

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

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed)

Details

(Whiteboard: p=1)

User Story

Building on bug 1022594, the desktop client should notify the server via the websocket when:

- The user clicks accept.
- The media is up (local stream published, remote stream subscribed)

These are both sending a message of type "action" with either "accept" or "media-up"

The Standalone client should notify the server via the websocket when:

- The media is up (local stream published, remote stream subscribed)


There are no user-visible effects of this bug, but they can be monitored by the websocket debugging.

Attachments

(4 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
This should be in 34, as bug 1000237 needs it.
No longer blocks: 1022594
User Story: (updated)
Depends on: 1022594
Priority: -- → P1
Target Milestone: --- → mozilla34

Updated

4 years ago
Whiteboard: p=1
(Assignee)

Comment 2

4 years ago
Approximately, what to do here:

1) Define functions in websocket.js for sending accept and media-up messages
2) In the ConversationRouter#accept, call the accept function on the websocket
3) In the ConversationView once both local stream is published, and receiving remote stream has been initiated, find a way to trigger the media-up message on the websocket.
4) Ensure unit tests are fixed and implemented.

Note that part 3, should be easy to find a way for sending media-up for the standalone client as well - hence incorporating the standalone part into this bug.
User Story: (updated)
Summary: Desktop client should notify the server of acceptance and media-up during incoming call requests → Desktop client should notify the server of acceptance and media-up during incoming call requests, Standalone client should notify the server of media-up during outgoing calls
(Assignee)

Comment 3

4 years ago
Created attachment 8473823 [details] [diff] [review]
WIP Notify the server when the connection is accepted.

This is a WIP that gets the 'accept' notification being sent to the server. I verified it got to the other side. The main part remaining is to fix the unit tests as they weren't quite working.

Obviously, the second part for media-up needs implementing as well, as described previously.
(Assignee)

Updated

3 years ago
Assignee: nobody → standard8
(Assignee)

Comment 4

3 years ago
Created attachment 8479886 [details] [diff] [review]
Part 1 - Notify the Loop server when the desktop client accepts the call, so that it can update the call status.

Ok, splitting this into two parts. Here's the simple acceptance part with updated tests. As mentioned previously, there's no user-visible impact of this yet, but it can be seen on the standalone side, by tweaking this._debugWebSocket to true in CallConnectionWebSocket and watching for the debug messages (bug 1000237 will be the first user of this notification, but that's more involved work, hence the separation).
Attachment #8473823 - Attachment is obsolete: true
Attachment #8479886 - Flags: review?(nperriault)
Comment on attachment 8479886 [details] [diff] [review]
Part 1 - Notify the Loop server when the desktop client accepts the call, so that it can update the call status.

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

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

Comment 6

3 years ago
Created attachment 8480533 [details] [diff] [review]
Part 2 - Notify the Loop server when the client has local media up and remote media being received, so that it can update the call connection status.

This is what's necessary to get the media-up notifications sent from both sides (standalone & desktop). With it, we get the media-up notifications successfully sent, and the server sends back completed and closes the sockets as expected.

I've got to fix tests yet, but looking for some feedback as to the general structure - I'm not too sure about handling the model subscribe/publish events in the routers, but it seemed to make more sense than passing the websocket to the model based on our previous discussions.
Attachment #8480533 - Flags: feedback?(nperriault)
Comment on attachment 8480533 [details] [diff] [review]
Part 2 - Notify the Loop server when the client has local media up and remote media being received, so that it can update the call connection status.

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

::: browser/components/loop/content/shared/js/views.jsx
@@ +314,5 @@
>            video: {enabled: false}
>          });
>        }.bind(this));
>  
> +      this.props.model.publish(this.publisher);

Just a thought: while it was previously the case, this couples even further the model with the SDK… It's probably okay for now, but at some point we'll probably want to create an abstraction on top of the SDK API to possibly allow for other adapters. Not now though.

::: browser/components/loop/content/shared/js/websocket.js
@@ +143,5 @@
> +     */
> +    mediaConnected: function() {
> +      this._send({
> +        messageType: "action",
> +        event: "media-up"

Nit: How about "media-connected" to match the method name?

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +416,5 @@
> +     */
> +    _checkConnected: function() {
> +      if (this._conversation.streamsConnected()) {
> +        this._websocket.mediaConnected();
> +      }

What if that test fails? The other party would never be notified, right? Do we handle timeouts in such case already, or is it planned?

@@ +505,5 @@
>          client: this._client
>        });
>        this._conversation.once("call:outgoing:setup", this.setupOutgoingCall, this);
> +      this._conversation.once("change:publishedStream", this._checkConnected, this);
> +      this._conversation.once("change:subscribedStream", this._checkConnected, this);

I agree with your comment that this seems odd to be performed within the router, though I have no better suggestion in mind without much heavier refactoring atm so it's probably good to go for now.
Attachment #8480533 - Flags: feedback?(nperriault) → feedback+
(Assignee)

Comment 8

3 years ago
(In reply to Nicolas Perriault (:NiKo`) from comment #7)
> ::: browser/components/loop/content/shared/js/websocket.js
> @@ +143,5 @@
> > +     */
> > +    mediaConnected: function() {
> > +      this._send({
> > +        messageType: "action",
> > +        event: "media-up"
> 
> Nit: How about "media-connected" to match the method name?

media-up is the name on the websocket as defined by the protocol. I went with mediaConnected for the function name as I thought it was slightly more descriptive, though I could change it back to mediaUp.


> > +    _checkConnected: function() {
> > +      if (this._conversation.streamsConnected()) {
> > +        this._websocket.mediaConnected();
> > +      }
> 
> What if that test fails? The other party would never be notified, right? Do
> we handle timeouts in such case already, or is it planned?

The server will handle timeout of connections via the websocket protocol, however, this check is mainly for ensuring we've both published and subscribed to streams, not just one of them (the current setup will give us two events that call this function, it seemed the easiest way without doing separate checks & event throwing).
(Assignee)

Comment 9

3 years ago
Created attachment 8480900 [details] [diff] [review]
Part 2 - Notify the Loop server when the client has local media up and remote media being received, so that it can update the call connection status. v2

Updated patch, fixed some of the comments and fixed and added tests.
Attachment #8480533 - Attachment is obsolete: true
Attachment #8480900 - Flags: review?(nperriault)
Comment on attachment 8480900 [details] [diff] [review]
Part 2 - Notify the Loop server when the client has local media up and remote media being received, so that it can update the call connection status. v2

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

LGTM with comments.

::: browser/components/loop/content/js/conversation.jsx
@@ +247,5 @@
> +      // sending the media up message.
> +      if (this._conversation.streamsConnected()) {
> +        this._websocket.mediaUp();
> +      }
> +    },

This code is duplicated from webapp.jsx, it should probably be shared. Regarding the schedule, let's just add a comment and/or file a bug about it for now.
Attachment #8480900 - Flags: review?(nperriault) → review+
(Assignee)

Comment 11

3 years ago
(In reply to Nicolas Perriault (:NiKo`) from comment #10)
> This code is duplicated from webapp.jsx, it should probably be shared.
> Regarding the schedule, let's just add a comment and/or file a bug about it
> for now.

Filed bug 1060298, as this seems like a larger issue than just the couple of bits of copying.
Mark, does this need testing or is testsuite coverage sufficient?
Flags: qe-verify?
(Assignee)

Comment 15

3 years ago
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #14)
> Mark, does this need testing or is testsuite coverage sufficient?

This isn't really testable by itself until bug 1000237 lands, at which stage we can test it in more detail, and hopefully add functional tests.
(Assignee)

Comment 16

3 years ago
Created attachment 8485116 [details] [diff] [review]
Beta port - part 1

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 1022594.
Attachment #8485116 - Flags: approval-mozilla-beta?
(Assignee)

Comment 17

3 years ago
Created attachment 8485117 [details] [diff] [review]
Beta port - part 2
Attachment #8485117 - Flags: approval-mozilla-beta?
I'm going to flag this as qe-verify+ but note that we're blocked on testing until bug 1000237 lands. I think we'll want to verify this is working when it's ready to test but testing it will likely be too complex for a smoketest.

Mark, while we're waiting, can you provide me with a basic set of instructions on how test this with websocket debugging?
Flags: qe-verify? → qe-verify+
Whiteboard: p=1 → p=1 [qablocked: bug 1000237]
Comment on attachment 8485116 [details] [diff] [review]
Beta port - part 1

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 have already approved related bug 
1022594 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 #8485116 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8485117 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This slipped off my radar but since there are no visible user effects from this bug I'm untracking for testing. Please needinfo me if you think this needs QE attention.
Flags: qe-verify+ → qe-verify-
QA Contact: anthony.s.hughes
Whiteboard: p=1 [qablocked: bug 1000237] → p=1
You need to log in before you can comment on or make changes to this bug.