1065203 - Standalone link clicker UI needs new sounds

Status: RESOLVED FIXED

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

Description

[Romain Testard [:RT]]

This bug lists all sound amendments required on the link clicker UI. Here are the sound files with reference to the UX matching the time these sounds need to be played
* Connecting:
     - Sound file: https://www.dropbox.com/sh/0fvz9cead5bt8zj/AAAsLfYFAN5oYKcM1YHwVMLta/Call%20Progress%204%20-%20Personality.wav?dl=0
     - UX reference (Connecting stage): https://people.mozilla.org/~dhenein/labs/loop-link-spec/#connecting
* Ringback:
     - Sound file: https://www.dropbox.com/sh/0fvz9cead5bt8zj/AADJS41VZmL2wbqjxgmIpa0Xa/Ringback-1.wav?dl=0
https://bugzilla.mozilla.org/show_bug.cgi?id=1015077
     - UX (Ringing stage): https://people.mozilla.org/~dhenein/labs/loop-link-spec/#connecting
* Call connected:
     - Sound file: https://www.dropbox.com/sh/0fvz9cead5bt8zj/AAAbsYf6TGikHqe4CrbTL3nka/C1-Connect-Up.wav?dl=0 
     - UX (play sound once the call connects): https://people.mozilla.org/~dhenein/labs/loop-link-spec/#video-call
* Call failed:
     - Sound file: https://www.dropbox.com/sh/0fvz9cead5bt8zj/AACr5DNQprNBz8IUUc6VSvt8a/C1-Failed.wav?dl=0Callee
     - UX (play sound when an outgoing call fails to connect to the other peer and the UX prompts the user to share a call-back link): https://people.mozilla.org/~dhenein/labs/loop-link-spec/#call-failed
* Call terminated:
     - Sound file: https://www.dropbox.com/sh/0fvz9cead5bt8zj/AAB1IQu3AladsKfZhBsC3oA-a/C1-Disconnect-Down.wav?dl=0 
     - UX (Play sound when the call terminates and the user gets displayed the feedback form: https://people.mozilla.org/~dhenein/labs/loop-link-spec/#call-terminated

Comment 1

[Maire Reavy [:mreavy]]

This is a standalone bug (no uplift required).

Comment 2

[Mark Banner (:standard8)]

Attachment: Example mixin for adding sounds (originally intended for desktop, not-working)

This is something that I was working on for desktop for bug 1065201 - but didn't work due to security permission issues. The idea here though was for a mixin to control playing of the sounds, which should be useful for this bug.

It also has the sound files already converted and added.

One note: I haven't put anything in for continuous looping yet - i.e. the ringing sound probably needs it, but the other ones won't.

Comment 3

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

Attachment: Added audio notifications for Loop standalone.

:tOkeshu, please find attached the WiP patch we were talking about on IRC. Next thing to do imho, extract the sound playing logic out of the mixin in some Audio{Library|Store} which could be unit tested much more easily than trying to stub an applied mixin.

Comment 4

[Romain Gauthier [:tOkeshu]]

Attachment: Bug 1065203: New sound notifications

Here is a patch introducing an AudioMixin, new sounds (in ogg format) as well as tests.

Comment 5

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

Comment on attachment 8511056 [details] [diff] [review]
Bug 1065203: New sound notifications

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

Looks great! r=me with comments addressed.

::: browser/components/loop/content/shared/js/mixins.js
@@ +114,5 @@
> +     * @param {String} filename The filename to play (excluding the extension).
> +     */
> +    play: function(filename, options) {
> +      if (this._isLoopDesktop()) {
> +        // XXX We need navigator.mozLoop.playSound(name)

Please file a bug about it and reference the bug id in the comment.

@@ +133,5 @@
> +     */
> +    _ensureAudioStopped: function() {
> +      if (this.audio) {
> +        this.audio.pause();
> +        this.audio.removeAttribute("src")

Nit: Missing trailing semicolon.

::: browser/components/loop/content/shared/js/views.jsx
@@ +184,5 @@
>        if (this.props.initiate) {
>          this.listenTo(this.props.model, "session:connected",
>                                          this.startPublishing);
> +        this.listenTo(this.props.model, "session:connected",
> +                                        this.play.bind(this, "connected"));

How about defining a _onSessionConnected event handler and keep a single subscription, where we could both startPublishing() and play the sound? I'd find it more legible, even if a bit more verbose.

::: browser/components/loop/test/shared/views_test.js
@@ +167,5 @@
>        return TestUtils.renderIntoDocument(sharedViews.ConversationView(props));
>      }
>  
>      beforeEach(function() {
> +      audio = {

s/audio/fakeAudio please.

@@ +369,5 @@
> +
> +            sinon.assert.calledOnce(window.Audio);
> +            sinon.assert.calledWithExactly(window.Audio, "shared/sounds/connected.ogg");
> +            expect(audio.loop).to.not.equal(true);
> +          });

Maybe we should ensure to setup this test is executed in a standalone context (when _isDesktop() is executed).

::: browser/components/loop/test/standalone/webapp_test.js
@@ +565,5 @@
>          websocketToken: "7b"
>        });
>  
>        sinon.stub(websocket, "cancel");
> +      audio = {

s/audio/fakeAudio please.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8511056 [details] [diff] [review]
Bug 1065203: New sound notifications

I think you're missing the call failed notification - this is specified in comment 0 (the ogg file was in my original patch on this bug).

Comment 7

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #6)
> Comment on attachment 8511056 [details] [diff] [review]
> Bug 1065203: New sound notifications
> 
> I think you're missing the call failed notification - this is specified in
> comment 0 (the ogg file was in my original patch on this bug).

As discussed on irc, this is happening in bug 1075509.

Comment 9

[Romain Gauthier [:tOkeshu]]

Attachment: Bug 1065203: New sound notifications

Comment 10

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

Comment on attachment 8511966 [details] [diff] [review]
Bug 1065203: New sound notifications

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

LGTM, just ensure the UI showcase doesn't break with these changes (it is atm on fx-team, so you'll probably have to wait for the fix to land, or to fix it along this patch).

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8511966 [details] [diff] [review]
Bug 1065203: New sound notifications

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

I've not looked at this in detail - trusting NiKo's review. I like the general idea though.

The ui-showcase isn't a problem as it uses a fake mozloop, so this code is never activated, we might just need to update the fake version when we do the desktop impl.

Comment 12

[Mark Banner (:standard8)]

Note: I committed a version with views.js updated as it hadn't been regenerated. It tested fine locally though.

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

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 14

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/5548849a1878

Comment 15

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

Comment on attachment 8511966 [details] [diff] [review]
Bug 1065203: New sound notifications

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only