1097742 - Standalone Rooms shouldn't join the room until after user media has been accepted

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

Currently the standalone rooms join the room as soon as "Join" is pressed - really we should wait until after the gUM prompt, to avoid confusing the desktop user as to why someone is in the room but not showing media.

Comment 1

[Mark Banner (:standard8)]

Attachment: Part 1 Handle access being denied to media, and prevent the sdk prompts from showing in Loop Rooms.

Here's the first part - some tidy up to disable the existing prompts that the SDK puts up when media fails or hasn't been granted yet. Additionally, we put up an error if the media is denied.

Sevaan/Matej: Requesting feedback on the strings. I need to land this today/early tomorrow, but as it is standalone UI, these strings won't be translated straight away. If you have feedback about the content and they need to change, then I'll do a patch to update them later (on another bug if necessary).

Comment 2

[Matej Novak [:matej]]

Could someone pull the strings for me? I don't know what to look at in all that code (sorry, I'm just a marketing guy).

Comment 3

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8522330 [details] [diff] [review]
Part 1 Handle access being denied to media, and prevent the sdk prompts from showing in Loop Rooms.

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

Looks good, though I'd like to see some constants added for server codes.

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +120,4 @@
>        this.setStoreState({
>          error: actionData.error,
> +        failureReason: actionData.error.errno === 105 ||
> +          actionData.error.errno === 111 ? "notFound" : "unknown",

Really, we should create some SERVER_CODES object somewhere, so we could do things like:

actionData.error.errno === SERVER_CODES.EXPIRED

@@ +287,5 @@
>        // could be a success case, but there's no way we should be intentionally
>        // sending that and still have the window open.
> +      this.setStoreState({
> +        failureReason: actionData.reason
> +      });

I'd expect the failureReason value being reseted sometimes, or maybe should we rename it to latestFailureReason?

::: browser/components/loop/content/shared/js/otSdkDriver.js
@@ +111,5 @@
>        if (this.publisher) {
> +        this.publisher.off("accessAllowed", this._onPublishComplete.bind(this));
> +        this.publisher.off("accessDenied", this._onPublishDenied.bind(this));
> +        this.publisher.off("accessDialogOpened",
> +          this._onAccessDialogOpened.bind(this));

Nit: Couldn't we just unregister all the listeners for each event name at once? eg. this.publisher.off("accessAllowed") should be enough.

@@ +226,5 @@
> +      // This prevents the SDK's "access denied" dialog showing.
> +      event.preventDefault();
> +
> +      this.dispatcher.dispatch(new sharedActions.ConnectionFailure({
> +        reason: "mediaDenied"

Nit: I wonder if we shouldn't create some FAILURE_REASONS object, so we could do:

reason: FAILURE_REASONS.MEDIA_DENIED

::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
@@ +39,5 @@
>        );
>      },
>  
> +    /**
> +     * Returns an appropriate string according to the failureReason.

Nit: @return String

@@ +82,5 @@
>              </div>
>            );
> +        case ROOM_STATES.FAILED:
> +          return (
> +            <div>

Do we need this empty div?

::: browser/components/loop/test/shared/activeRoomStore_test.js
@@ +107,5 @@
>      });
> +
> +    it("should set the failureReason to `notFound` on server errno 105",
> +      function() {
> +        fakeError.errno = 105;

Using a SERVER_CODES constant would shine here.

::: browser/components/loop/test/shared/otSdkDriver_test.js
@@ +297,5 @@
> +      });
> +    });
> +
> +    describe("accessDenied", function() {
> +      it("should prevent the default action", function() {

Nit: s/default action/default event behavior?

Comment 4

[Mark Banner (:standard8)]

Matej, they are:

rooms_unavailable_notification_message=Sorry, this room is not available. It may be expired or you have an invalid URL.
rooms_media_denied_message=We could not get access to your microphone or camera. Please reload the page to try again.

(in loop.en-US.properties).

Comment 5

[Matej Novak [:matej]]

(In reply to Mark Banner (:standard8) from comment #4)
> Matej, they are:

Thanks. Really appreciate it.

> rooms_unavailable_notification_message=Sorry, this room is not available. It
> may be expired or you have an invalid URL.

I thought we weren't explicitly calling them "rooms." I would say something like:

Sorry, you cannot join this conversation. The link may be expired or invalid.

> rooms_media_denied_message=We could not get access to your microphone or
> camera. Please reload the page to try again.

This one looks good to me.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8522330 [details] [diff] [review]
Part 1 Handle access being denied to media, and prevent the sdk prompts from showing in Loop Rooms.

Great, thanks.

Comment 7

[Mark Banner (:standard8)]

Attachment: Part 1 Handle access being denied to media, and prevent the sdk prompts from showing in Loop Rooms.

Updated for review comments and feedback.

Comment 8

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8522450 [details] [diff] [review]
Part 1 Handle access being denied to media, and prevent the sdk prompts from showing in Loop Rooms.

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

Looks better; we're not far off now :)

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +128,5 @@
>        this.setStoreState({
>          error: actionData.error,
> +        failureReason: actionData.error.errno === SERVER_CODES.INVALID_TOKEN ||
> +          actionData.error.errno === SERVER_CODES.EXPIRED ?
> +          FAILURE_REASONS.EXPIRED_OR_INVALID : FAILURE_REASONS.UNKNOWN,

As per our discussion on IRC, this part could be made a bit more legible. More importantly, failureReason can be set to `true` when actionData.error.errno === SERVER_CODES.INVALID_TOKEN.

::: browser/components/loop/content/shared/js/utils.js
@@ +21,5 @@
> +    MEDIA_DENIED: "fr-media-denied",
> +    COULD_NOT_CONNECT: "fr-could-not-connect",
> +    NETWORK_DISCONNECTED: "fr-network-disconnected",
> +    EXPIRED_OR_INVALID: "fr-expired-or-invalid",
> +    UNKNOWN: "fr-unknown"

Nit: I'd rather see using a "reason" prefix, eg. "reason-unknown".

::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
@@ +85,5 @@
> +        case ROOM_STATES.FAILED:
> +          return (
> +              <p className="failed-room-message">
> +                {this._getFailureString()}
> +              </p>

Nit: Indentation issue.

Comment 9

[Mark Banner (:standard8)]

Attachment: [checked in] Part 1 Handle access being denied to media, and prevent the sdk prompts from showing in Loop Rooms. v3

Updated for review comments.

Comment 10

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8522507 [details] [diff] [review]
[checked in] Part 1 Handle access being denied to media, and prevent the sdk prompts from showing in Loop Rooms. v3

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

Ship it!

Comment 11

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/bd6aae177d9c

Marking as leave-open for part 2, which will do the actual delaying of joining until gUM has been accepted.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

There is a syntax error here, due to the comma missing:
https://hg.mozilla.org/integration/fx-team/annotate/5fb37ccaf137/browser/components/loop/test/desktop-local/roomViews_test.js#l68

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Fixed the syntax error, https://hg.mozilla.org/integration/fx-team/rev/7924cb65d21f

Comment 14

[Mark Banner (:standard8)]

Thanks Jaws.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd6aae177d9c
https://hg.mozilla.org/mozilla-central/rev/7924cb65d21f

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9fe7dd3fcb20
https://hg.mozilla.org/releases/mozilla-aurora/rev/00e65cb7f780

Comment 17

[u279076]

Is this "fixed" now?

Comment 18

[Mark Banner (:standard8)]

No, see comment 11.

Comment 19

[u279076]

(In reply to Mark Banner (:standard8) from comment #18)
> No, see comment 11.

Okay, thanks. I really wish we'd branch these types of bugs so they're easier to follow. Multi-part bugs are really hard to grok.

Comment 20

[Mark Banner (:standard8)]

Attachment: Part 2 Standalone Rooms shouldn't join the room until after user media has been accepted.

This inserts a new state "MEDIA" where we obtain the permissions. The denied flow hasn't changed. If the user accepts, then we join the room on the servers.

The only strange bit I found was that because we're initialising the sdk publisher so early, it didn't seem to be sizing the video correctly. I've added in the calls to updateVideoContainer and also forced one to happen when the room is joined. This seems to get it working correctly (and for when the tab is resized).

Comment 21

[Adam Roach [:abr]]

Comment on attachment 8528359 [details] [diff] [review]
Part 2 Standalone Rooms shouldn't join the room until after user media has been accepted.

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

Looks good. I have a handful of nits; r+ with those resolved.

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +28,5 @@
>      GATHER: "room-gather",
>      // The store has got the room data
>      READY: "room-ready",
> +    // Obtaining media from the user
> +    MEDIA: "room-media",

This name is a bit confusing when trying to read the resulting code. Maybe MEDIA_WAIT: "room-media-wait"?

@@ +264,5 @@
>        }
>  
> +      this.setStoreState({roomState: ROOM_STATES.MEDIA});
> +    },
> +

Missing a documentation block here.

::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
@@ +72,5 @@
> +          var msg = mozL10n.get("call_progress_getting_media_description",
> +                                {clientShortname: mozL10n.get("clientShortname2")});
> +          return (
> +            <p className="prompt-media-message">
> +              {msg}

Is there a companion bug to Bug 1047040 that puts the graphics in place to let the user know exactly what is expected of them? Is so, cite it here. If not, open one, and cite it here.

@@ +239,5 @@
> +      /**
> +       * OT inserts inline styles into the markup. Using a listener for
> +       * resize events helps us trigger a full width/height on the element
> +       * so that they update to the correct dimensions.
> +       * XXX: this should be factored as a mixin.

Open a bug, put the bug # here.

@@ +262,5 @@
>       */
>      componentWillUpdate: function(nextProps, nextState) {
>        if (this.state.roomState !== ROOM_STATES.JOINED &&
>            nextState.roomState === ROOM_STATES.JOINED) {
> +        // XXX This forces the video size to update - creating the publisher

I'm not sure why we have the XXX marking here. Is there further action indicated? If so, open a bug and cite it. If this isn't (and won't be) actionable, take out the "XXX".

@@ +269,5 @@
> +        this.updateVideoContainer();
> +      }
> +
> +      if (this.state.roomState !== ROOM_STATES.MEDIA &&
> +          nextState.roomState === ROOM_STATES.MEDIA) {

Given that these events happen in the other order (waiting for media and *then* joined), I have a slight preference for reversing the order they appear here (feel free to ignore if you have some reason for keeping them in this order)

::: browser/components/loop/test/shared/activeRoomStore_test.js
@@ +300,5 @@
> +      expect(store.getStoreState().roomState).eql(ROOM_STATES.MEDIA);
> +    });
> +  });
> +
> +  describe("gotMediaPermission", function() {

describe("#gotMediaPermission"...

::: browser/components/loop/test/standalone/standaloneRoomViews_test.js
@@ +64,5 @@
>  
>            expectActionDispatched(view);
>          });
>  
> +      it("should dispatch a `SetupStreamElements` action on MEDIA state rejoined",

s/rejoined/re-entered/

@@ +74,4 @@
>  
>            expectActionDispatched(view);
>          });
>      });

We're no longer testing the JOINED action -- shouldn't we at least do something like mock out updateVideoContainer() and confirm that it's called on JOINED?

Comment 22

[Mark Banner (:standard8)]

Attachment: Part 2 Standalone Rooms shouldn't join the room until after user media has been accepted.

Updated patch for review comments.

Comment 23

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/ff0d0c2d2484

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ff0d0c2d2484

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8522507 [details] [diff] [review]
[checked in] Part 1 Handle access being denied to media, and prevent the sdk prompts from showing in Loop Rooms. v3

Approval Request Comment
[Feature/regressing bug #]: Rooms

[User impact if declined]: Merge annoyances as this touches 3 shared files

[Describe test coverage new/current, TBPL]: Includes tests.  On central.  Manually tested.

[Risks and why]: Important usability fix (ask for camera/mic *before* connecting to the room!) for standalone.  Very low risk; touches a few shared files mostly to add some feedback as to state.  Would cause annoyances to landing other updates and bug fixes if not taken.

[String/UUID change made/needed]: none.

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8528624 [details] [diff] [review]
Part 2 Standalone Rooms shouldn't join the room until after user media has been accepted.

Move request to beta (and set on the correct patch).

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0f1716aab02b