1000237 - Standalone UI for link clickers needs "call being processed" visual notification

Status: VERIFIED FIXED

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

Comment 1

[Romain Testard [:RT]]

"Call being processed" is an indication that the call has been triggered by the client but does not inform the user that the other party is being notified.
An other notification will inform the user that the other party is being notified - "call ringing" addressed on bug 1015074

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

As far as wording for the UI, maybe "Connecting..." or "Calling..." is a phrase that people are already used to seeing in this sort of context?

Comment 3

[Romain Testard [:RT]]

Yes, this was only meant to explain the type of message to display and not the wording.
The wording should come from Darrin's mock-ups, please refer to https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/ on section 10 where "Connecting..." is used.

Comment 4

[Romain Testard [:RT]]

Attachment: "Connecting" notification

Comment 5

[:shell escalante]

stand-alone UI: https://people.mozilla.org/~dhenein/labs/loop-link-spec/#connecting

Comment 6

[:shell escalante]

new stand-alone UI: https://people.mozilla.org/~dhenein/labs/loop-link-spec/#connecting

Comment 7

[Romain Testard [:RT]]

Please note that as per discussion with Arcadio yesterday we should not be using the "Firefox Hello" brand for now (likely we'll use it when we get to beta). For Nightly and Aurora we should replace "Hello" by "WebRTC" wherever "Hello" appears in the UX and also use the Firefox logo instead of the speech bubble for now.

Comment 8

[:shell escalante]

the other bug covers this - it's duplicated smaller subset of 1015074

Comment 9

[Romain Testard [:RT]]

This bug is about notifying the caller that we're trying to reach the callee whereas https://bugzilla.mozilla.org/show_bug.cgi?id=1015074 is about notifying the caller that the callee is being notifie.
I don't think these are duplicates - they are separate user stories which can be implemented separately.
Or are you saying these should both be implemented under the same bug? In this case we need to change user stories on https://bugzilla.mozilla.org/show_bug.cgi?id=1015074

Comment 10

[Mark Banner (:standard8)]

This should be kept separate from bug 1015074, they are related, and probably could be done at the same time, but need to be kept separate for now.

Comment 11

[Mark Banner (:standard8)]

Shell says these are ok to be separate, we'll probably implement them together though.

Comment 12

[Mark Banner (:standard8)]

Now that bug 1022594 is almost landed:

(Commenting on User Story)
> * Currently, user clicks start & call information is obtained, followed by
> showing media
> * As call information is being obtained, display the "Connecting" view
> ** Note: The media should still be obtained in this connecting state. It
> should not be delayed.

In WebappRouter#_setupWebSocketAndCallView should navigate to a new route, or update the existing start call view to display ongoing and cancel buttons (rather than the start button).

Depends what makes most sense from a view/react/router base.

> * Connect to the websocket & send hello (if not already sent)

This would already be done if doing the changes suggested above

> * When a messageType "progress" from the server is received with state
> "alerting", switch to the next view (currently the media view).

In WebappRouter#_handleWebSocketProgress handle the "alerting" state for progress, and navigate to "call/ongoing/" + loopToken (the existing route) when it is received.

Comment 13

[Mark Banner (:standard8)]

Attachment: Standalone UI for link clickers needs "call being processed" visual notification.

This is an outline for this bug. Some of it isn't finishes yet, e.g. some of the tests and fixing up the CSS properly.

Mainly I'm waiting on the uplift bug landing, but maybe also bug 1035348 (depends how close it gets), before fixing this up completely ready for review.

Hence, I'm looking for feedback on the general structure and new react views.

Comment 14

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

Comment on attachment 8484100 [details] [diff] [review]
Standalone UI for link clickers needs "call being processed" visual notification.

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

Looks pretty good. My only concern is that we keep adding complexity to the router, we should find a decent strategy to decouple business logic from the router and components. This work shouldn't be part of this very patch though.

::: browser/components/loop/content/shared/js/models.js
@@ -287,5 @@
> -    _clearPendingCallTimer: function() {
> -      if (this._pendingCallTimer) {
> -        clearTimeout(this._pendingCallTimer);
> -      }
> -    },

Code removal ftw. So long, bloat!

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +115,1 @@
>            <div className="loop-logo" title="Firefox WebRTC! logo"></div>

Nit: while not part of this patch, it feels odd to have static branding text in there.

@@ +139,5 @@
> +    getInitialState: function() {
> +      return {
> +        callState: "connecting"
> +      }
> +    },

We set the initiate the initial state against any passed prop to ease creating a component instance in a given state using props; that could be:

getInitialProps: function() {
  return {callState: "connecting"};
},

getInitialState: function() {
  return {callState: this.props.callState};
}

So we could build a PendingConversationView this way in the UI showcase:

<PendingConversationView callState="ringing"/>

@@ +148,5 @@
> +    },
> +
> +    componentDidMount: function() {
> +      this.props.websocket.listenTo(this.props.websocket, "progress:alerting",
> +                                    this._handleRingingProgress);

Note: This would possibly greatly benefit from using the event dispatcher approach suggested in bug 1062126; let's don't bother too much for now.

@@ +158,5 @@
> +
> +    render: function() {
> +      var callState = __("call_progress_" + this.state.callState + "_description");
> +      var btnClassCancelCall = "btn btn-large btn-accept " +
> +                              loop.shared.utils.getTargetPlatform();

I'm surprised this is still there, I think this has been removed by bug 1048938.

@@ +160,5 @@
> +      var callState = __("call_progress_" + this.state.callState + "_description");
> +      var btnClassCancelCall = "btn btn-large btn-accept " +
> +                              loop.shared.utils.getTargetPlatform();
> +      return (
> +        /* jshint ignore:start */

You what would be great? To use jsxhint[0] so we could get rid of these annoying rules, plus getting JSX transpiled code validation as well. While you're in the area, please ensure a bug is filed for this.

[0]: https://www.npmjs.org/package/jsxhint

@@ +504,5 @@
> +            case "timeout":
> +              this._handleCallTimeout();
> +              break;
> +          }
> +          break;

Nested switch statements won't scale if we add more states/reasons… Could we find something more maintainable?

_handleCallRejected and _handleCallTimeout share a lot, we could possibly merge them into a new _handleCallTermination(reason) method, wdyt?

@@ +512,5 @@
> +
> +    /**
> +     * Handles a call moving to the connecting stage.
> +     */
> +    _handleCallConnecting: function() {

Note: I still feel like this router does too much; we'll probably want to extract call establishment & communication in an external controller later. Or to use the AppEventDispatcher approach suggested in bug 1062126.

This is probably okay for now.

@@ +611,5 @@
> +      }
> +      this._setupWebSocketAndCallView(loopToken);
> +      this.loadReactComponent(PendingConversationView({
> +        websocket: this._websocket
> +      }));

I wish we could have:

var ws = this._setupWebSocketAndCallView(loopToken);

this.loadReactComponent(PendingConversationView({
  websocket: ws
}));

::: browser/components/loop/ui/ui-showcase.jsx
@@ +174,5 @@
>  
> +          <Section name="PendingConversationView">
> +            <Example summary="Pending conversation view" dashed="true">
> +              <div className="standalone">
> +                <PendingConversationView />

Ideally we'd want two different examples showing the different states here.

Comment 15

[Mark Banner (:standard8)]

Attachment: Standalone UI for link clickers needs "call being processed" visual notification. v2

Updated patch, still need to fix the styles for the cancel button and check a few things on the server side for consistency.

Comment 16

[Mark Banner (:standard8)]

Attachment: Standalone UI for link clickers needs "call being processed" visual notification.

Update patch, I think its ready for review now. There's still some intermittent server issues (bug 1062266), but hopefully should be testable even without those fixed.

This patch is implementing:
- Connecting and Ringing displays (this bug, and bug 1000237)
- Cancel button
- Removes the old timeout of the webapp, in favour of the server generated timeouts

It doesn't do any changes on the desktop side, i.e. notification of cancellation of the call, or other termination, won't show anything visually on the desktop client - it can be seen across the websocket by setting "loop.debug.websocket" to true and looking for 'WS ' notifications in the browser console.

I've noticed one issue, which is when the call times out, there's no error notification display (even though we try to). This is a regression from an earlier patch, and should be fixed by bug 1046959.

Comment 17

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

Comment on attachment 8487828 [details] [diff] [review]
Standalone UI for link clickers needs "call being processed" visual notification.

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

Looks very good, thanks. r=me with comments addressed.

::: browser/components/loop/content/shared/js/websocket.js
@@ +147,5 @@
>          event: "media-up"
>        });
>      },
>  
> +    cancel: function() {

Missing jsdocs.

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +223,5 @@
> +                <span className="standalone-call-btn-text">
> +                  {mozL10n.get("initiate_call_cancel_button")}
> +                </span>
> +              </button>
> +              <div className="flex-padding-1"></div>

Still concerned with this pattern (non-semantic empty elements used for decoration), but I won't fight anymore :)

::: browser/components/loop/standalone/content/l10n/data.ini
@@ +1,1 @@
>  @import url(loop.en-US.properties)

Nit: I'd favor keeping an empty line at the bottom of the file, see http://robots.thoughtbot.com/no-newline-at-end-of-file.

::: browser/components/loop/test/standalone/webapp_test.js
@@ +165,2 @@
>                callId: "Hello",
>                url: "http://progress.example.com",

While you're around, we should use invalid urls here.

@@ +237,5 @@
>              });
>  
> +            it("should display an error message if the reason is not 'cancel'",
> +              function() {
> +                sandbox.stub(notifications, "errorL10n");

Nit: I think you can stub this in beforeEach().

@@ +245,5 @@
> +                  reason: "reject"
> +                });
> +
> +                sinon.assert.calledOnce(router._notifications.errorL10n);
> +                sinon.assert.calledWithExactly(router._notifications.errorL10n,

Nit: sinon.assert.calledWithExactly(notifications.errorL10n,…) is probably better because that's what's actually stubbed above.

@@ +258,5 @@
> +                  state: "terminated",
> +                  reason: "cancel"
> +                });
> +
> +                sinon.assert.notCalled(router._notifications.errorL10n);

Same remark as above.

@@ +264,5 @@
> +          });
> +
> +          describe("state: connecting", function() {
> +            beforeEach(function() {
> +              conversation.set({"loopToken": "fakeToken"});

Nit: you can probably set this within the only test in this section.

Comment 18

[Mark Banner (:standard8)]

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

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3932a0c7f8e5

Comment 20

[Mark Banner (:standard8)]

This should definitely get some functional testing, and preferably, some looking for unexpected failure testing as well.

The fix is currently deployed on the dev server - https://wiki.mozilla.org/CloudServices/Loop/Deploy#Dev - set your loop.server pref to "https://loop-dev.stage.mozaws.net" (note: no trailing slash.

Note that currently, due to other bugs (bug 1000240, bug 1046959), call failures are not called out on the UI - it just returns to the home screen.

Outline of things to test (there may be a few more clicks/steps required for permissions prompts etc):

- Call a browser that generated a url, but is now shut-down
=> Standalone client should display "Connecting..." for 10 seconds, then revert to the home screen.

- Call a browser that generated a url, but don't accept the call
=> Standalone client should display "Connecting...", then
=> when the conversation window on the second browser pops up, the standalone client changes to "Ringing..."
=> 30 seconds after displaying "Ringing..." the standalone client reverts to the home screen.

- Call a browser that generated a url, and accept the call
=> Standalone client displays "Connecting...", "Ringing..." and then displays the conversation UI.
=> Desktop client connects appropriately as well.

As I said earlier, it'd be useful to get some other folks testing this and looking out for any unexpected timeout lengths/actions.

It should work with nightly, aurora or beta as the desktop client side.

Comment 21

[u279076]

Paul, can you please take a crack at this?

Comment 22

[Paul Silaghi, QA [:pauly]]

I'm testing this with 2 profiles on the same machine, and the call is terminated after couple of seconds.
I see this in the error console:
"OT.exception :: title: Unable to Publish (1500) msg: Session.publish :: The selected voice or video devices are unavailable. Verify that the chosen devices are not in use by another application."
Is this expected ?

Comment 23

[Paul Silaghi, QA [:pauly]]

Issue #2:
1. Load the call URL
2. Press Start
3. While "Ringing", go to offline mode (on the caller's side)
Actual results:
Ringing forever, 'cancel button' no longer works

Comment 24

[Paul Silaghi, QA [:pauly]]

Moreover, reloading (F5) in offline mode displays:
"Incompatible Browser
The audio and video components of WebRTC! are powered by WebRTC.
Please try this link in a WebRTC-enabled browser, such as Firefox."

Comment 25

[Mark Banner (:standard8)]

Paul: Please can you come into #loop on irc and discuss with me? I want to check some details as to what's going on, and it'll be easier to do that there.

Comment 26

[Mark Banner (:standard8)]

(In reply to Paul Silaghi, QA [:pauly] from comment #23)
> Issue #2:
> 1. Load the call URL
> 2. Press Start
> 3. While "Ringing", go to offline mode (on the caller's side)
> Actual results:
> Ringing forever, 'cancel button' no longer works

This is bug 1067519 with also needing bug 1065974 for the most ideal UX.

Comment 27

[Mark Banner (:standard8)]

(In reply to Paul Silaghi, QA [:pauly] from comment #22)
> I'm testing this with 2 profiles on the same machine, and the call is
> terminated after couple of seconds.
> I see this in the error console:
> "OT.exception :: title: Unable to Publish (1500) msg: Session.publish :: The
> selected voice or video devices are unavailable. Verify that the chosen
> devices are not in use by another application."
> Is this expected ?

As discussed on irc, this appears to be a limitation of windows - you can't access more than one device from different processes at the same time, e.g.:

http://superuser.com/questions/741429/running-webcam-with-multiple-applications
http://superuser.com/questions/741429/running-webcam-with-multiple-applications

(In reply to Paul Silaghi, QA [:pauly] from comment #24)
> Moreover, reloading (F5) in offline mode displays:
> "Incompatible Browser
> The audio and video components of WebRTC! are powered by WebRTC.
> Please try this link in a WebRTC-enabled browser, such as Firefox."

Please file a new bug for this, it will either need an SDK fix, or a fix in the loop-client (or maybe both).

Comment 28

[Paul Silaghi, QA [:pauly]]

Thanks Mark.
Verified fixed this bug on 35.0a1 (2014-09-17) Win 7, OS X 10.9.5, Ubuntu 12.10.

(In reply to Mark Banner (:standard8) from comment #27)
> Please file a new bug for this
filed bug 1069321

Comment 29

[Maire Reavy [:mreavy]]

This is for the standalone app (it doesn't ride the trains, no uplift required)

Comment 30

[:shell escalante]

Marked as [no-loop-uplift] in the whiteboard, so we know this is one of the bugs coming in the timeframe, but doesn't require the triaging to get uplifted to Aurora since it's the stand-alone client.

Comment 31

[u279076]

Untracking as needing verification pre-uplift.

Comment 32

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

Comment on attachment 8487828 [details] [diff] [review]
Standalone UI for link clickers needs "call being processed" visual notification.

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 33

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/a016dfb71871

Comment 34

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8487828 [details] [diff] [review]
Standalone UI for link clickers needs "call being processed" visual notification.

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.

Comment 35

[u279076]

Paul, this has been uplifted to Aurora. Can you please test this to confirm it's working as expected?

Comment 36

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 34 beta 1 Win 7