Closed Bug 1000237 Opened 6 years ago Closed 5 years ago

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

Categories

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

defect

Tracking

(firefox34 verified, firefox35 verified)

VERIFIED FIXED
mozilla35
Iteration:
35.1
Tracking Status
firefox34 --- verified
firefox35 --- verified

People

(Reporter: RT, Assigned: standard8)

References

()

Details

(Whiteboard: [p=1][standalone-uplift][fig:wontverify])

User Story

As a WebRTC browser URL clicker but not on Firefox, I get notified that my call is being processed.

Rough Impl:

This depends on bug 1022594 for the initial websocket implementation. It also requires bug 1045643 to be implemented for the standalone side to work correctly.

* 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.
* Connect to the websocket & send hello (if not already sent)
* When a messageType "progress" from the server is received with state "alerting", switch to the next view (currently the media view).

Bug 1015074 will implement the ringing phase.

Attachments

(2 files, 2 obsolete files)

No description provided.
User Story: (updated)
Blocks: 994274
No longer blocks: 1000127
Blocks: 1000778
No longer blocks: 994274
User Story: (updated)
Priority: -- → P1
Whiteboard: [p=?, s=UI32]
Target Milestone: --- → mozilla32
Target Milestone: mozilla32 → mozilla33
Whiteboard: [p=?, s=UI32] → [p=?]
"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
Summary: Standalone UI for link clickers needs "call being processed" visual and audio notification → Standalone UI for link clickers needs "call being processed" visual notification
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?
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.
Assignee: nobody → dhenein
Status: NEW → ASSIGNED
Whiteboard: [p=?] → p=1 s=33.3 [qa-]
stand-alone UI: https://people.mozilla.org/~dhenein/labs/loop-link-spec/#connecting
Assignee: dhenein → nobody
Whiteboard: p=1 s=33.3 [qa-] → p=?
Depends on: 1039224
Whiteboard: p=? → --do_not_change-- [mozilla33 carry over]
Target Milestone: mozilla33 → mozilla34
Whiteboard: --do_not_change-- [mozilla33 carry over] → [mozilla33 carry over]
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.
Blocks: 1039224
No longer depends on: 1039224
Depends on: 1045643
Whiteboard: [mozilla33 carry over] → [p=1]
the other bug covers this - it's duplicated smaller subset of 1015074
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1015074
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
Status: RESOLVED → REOPENED
Flags: needinfo?(sescalante)
Resolution: DUPLICATE → ---
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.
Blocks: 1015074
User Story: (updated)
Depends on: 1022594
User Story: (updated)
Shell says these are ok to be separate, we'll probably implement them together though.
Status: REOPENED → NEW
Flags: needinfo?(sescalante)
Blocks: 1034041
Blocks: 1000240
Blocks: 1000228
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.
Assignee: nobody → standard8
Depends on: 1062266
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.
Attachment #8484100 - Flags: feedback?(nperriault)
Depends on: 1035348, 1048938
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.
Attachment #8484100 - Flags: feedback?(nperriault) → feedback+
No longer depends on: 1048938
Iteration: --- → 35.1
Whiteboard: [p=1] → [p=1][loop-uplift]
Target Milestone: mozilla34 → mozilla35
Updated patch, still need to fix the styles for the cancel button and check a few things on the server side for consistency.
Attachment #8484100 - Attachment is obsolete: true
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.
Attachment #8487789 - Attachment is obsolete: true
Attachment #8487828 - Flags: review?(nperriault)
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.
Attachment #8487828 - Flags: review?(nperriault) → review+
https://hg.mozilla.org/mozilla-central/rev/3932a0c7f8e5
Status: NEW → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
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.
Flags: qe-verify?
Flags: needinfo?(anthony.s.hughes)
Paul, can you please take a crack at this?
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(paul.silaghi)
Flags: needinfo?(anthony.s.hughes)
QA Contact: paul.silaghi
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 ?
Flags: needinfo?(paul.silaghi) → needinfo?(standard8)
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
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."
Blocks: 1067591
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.
(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.
(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).
Flags: needinfo?(standard8)
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
Status: RESOLVED → VERIFIED
This is for the standalone app (it doesn't ride the trains, no uplift required)
Whiteboard: [p=1][loop-uplift] → [p=1]
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.
Whiteboard: [p=1] → [p=1][no-loop-uplift]
Whiteboard: [p=1][no-loop-uplift] → [p=1][standalone-uplift]
Untracking as needing verification pre-uplift.
Whiteboard: [p=1][standalone-uplift] → [p=1][standalone-uplift][figverify-]
Whiteboard: [p=1][standalone-uplift][figverify-] → [p=1][standalone-uplift][fig:wontverify]
Depends on: 1074823
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
Attachment #8487828 - Flags: approval-mozilla-aurora?
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.
Attachment #8487828 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Paul, this has been uplifted to Aurora. Can you please test this to confirm it's working as expected?
Flags: needinfo?(paul.silaghi)
Verified fixed FF 34 beta 1 Win 7
Flags: needinfo?(paul.silaghi)
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.