Closed Bug 1097742 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect
Points:
5

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
mozilla36
Iteration:
37.1
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
Blocking Flags:
backlog Fx35+

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Assignee: nobody → standard8
Iteration: --- → 36.3
Points: --- → 3
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).
Attachment #8522330 - Flags: ui-review?(sfranks)
Attachment #8522330 - Flags: review?(nperriault)
Attachment #8522330 - Flags: feedback?(Mnovak)
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 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?
Attachment #8522330 - Flags: review?(nperriault) → review-
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).
Flags: needinfo?(Mnovak)
(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.
Flags: needinfo?(Mnovak)
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.
Attachment #8522330 - Flags: ui-review?(sfranks)
Attachment #8522330 - Flags: feedback?(Mnovak)
Updated for review comments and feedback.
Attachment #8522330 - Attachment is obsolete: true
Attachment #8522450 - Flags: review?(nperriault)
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.
Attachment #8522450 - Flags: review?(nperriault) → review-
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!
Attachment #8522507 - Flags: review?(nperriault) → review+
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.
backlog: --- → Fx35+
Points: 3 → 5
Keywords: leave-open
Target Milestone: --- → mozilla36
Thanks Jaws.
Priority: -- → P1
Is this "fixed" now?
(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.
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).
Attachment #8528359 - Flags: review?(nperriault)
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?
Attachment #8528359 - Flags: review?(nperriault) → review+
Whiteboard: [checkin needed: part 2]
Attachment #8522507 - Attachment description: Part 1 Handle access being denied to media, and prevent the sdk prompts from showing in Loop Rooms. v3 → [checked in] Part 1 Handle access being denied to media, and prevent the sdk prompts from showing in Loop Rooms. v3
https://hg.mozilla.org/integration/fx-team/rev/ff0d0c2d2484
Iteration: 36.3 → 37.1
Keywords: checkin-needed
Whiteboard: [checkin needed: part 2]
https://hg.mozilla.org/mozilla-central/rev/ff0d0c2d2484
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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.
Attachment #8522507 - Flags: approval-mozilla-aurora?
Flags: qe-verify-
Attachment #8522507 - Flags: approval-mozilla-aurora?
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).
Attachment #8528624 - Flags: approval-mozilla-beta?
Attachment #8528624 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Iteration: 37.1 → 36.3
Iteration: 36.3 → 37.1
You need to log in before you can comment on or make changes to this bug.