Standalone link clicker UI needs new sounds

RESOLVED FIXED in Firefox 35

Status

defect
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: RT, Assigned: rgauthier)

Tracking

unspecified
mozilla36
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

(Whiteboard: [standalone-uplift])

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

5 years ago
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

Updated

5 years ago
Priority: -- → P2
Whiteboard: [loop-uplift]
Target Milestone: mozilla34 → mozilla35
This is a standalone bug (no uplift required).
Whiteboard: [loop-uplift]
Whiteboard: [standalone-uplift]
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.
Blocks: 1015077

Updated

5 years ago
backlog: --- → Fx35+
Assignee

Updated

5 years ago
Assignee: nobody → rgauthier

Updated

5 years ago
Target Milestone: mozilla35 → ---
: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.
Assignee

Comment 4

5 years ago
Here is a patch introducing an AudioMixin, new sounds (in ogg format) as well as tests.
Attachment #8504605 - Attachment is obsolete: true
Attachment #8510411 - Attachment is obsolete: true
Attachment #8511056 - Flags: review?(standard8)
Attachment #8511056 - Flags: review?(nperriault)
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.
Attachment #8511056 - Flags: review?(nperriault) → review+
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).
Attachment #8511056 - Flags: review?(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.
Blocks: 1000778
Duplicate of this bug: 1015074
Blocks: 1065201
Assignee

Comment 9

5 years ago
Attachment #8511056 - Attachment is obsolete: true
Attachment #8511966 - Flags: review?(standard8)
Attachment #8511966 - Flags: review?(nperriault)
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).
Attachment #8511966 - Flags: review?(nperriault) → review+
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.
Attachment #8511966 - Flags: review?(standard8) → feedback+
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
Target Milestone: --- → mozilla36

Updated

5 years ago
Priority: P2 → P1
https://hg.mozilla.org/mozilla-central/rev/c842e694f6b7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Flags: qe-verify-

Updated

5 years ago
Iteration: --- → 36.1
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
Attachment #8511966 - Flags: approval-mozilla-aurora?
Attachment #8511966 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.