1065201 - Desktop client 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 desktop client UI required for FF34 MC. 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 - sound played on caller end only): https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-outgoing
* 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 - sound played on caller end only): https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-outgoing
* Call connected:
     - Sound file: https://www.dropbox.com/sh/0fvz9cead5bt8zj/AAAbsYf6TGikHqe4CrbTL3nka/C1-Connect-Up.wav?dl=0 
     - UX (play sound once the call connects - sound played on both the callee and the caller sides): https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-active
* 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 - sound played on the caller end only): https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#link-prompt
* 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 - sound played on both the caller and the callee sides): https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#feedback
* Incoming call notification:
     - Sound file: https://www.dropbox.com/sh/0fvz9cead5bt8zj/AAAxehbnSWO-6njIG12I90nca/Ringtone-Full.wav?dl=0 
     - UX (Play sound file upon incoming call notification screen as a result of a direct call or a URL call-back - sound played on the callee side only): https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-incoming

Comment 2

[Mark Banner (:standard8)]

Once the work in bug 1065203 is completed, this work should be able to proceed.

From various discussions and implementation attempts so far:

- Loading the chrome uris directly for audio files on the desktop side doesn't work due to security permissions.
- It would be nicer to play the sounds from the content space rather than chrome.
- A suggestion was to implement a function in mozLoopAPI that would return a data uri for the sound. The data uri would then be played.

Comment 3

[Mark Banner (:standard8)]

I forgot, Romain had expressed an interest in fixing this after bug 1065203, so tentatively assigning him for now.

Comment 4

[Romain Gauthier [:tOkeshu]]

As discussed, we will hardcode the sounds as data URIs in the loop content.

Comment 5

[Romain Gauthier [:tOkeshu]]

Attachment: Bug 1065201: Desktop new sounds

New sounds for Desktop. I could not test the failure case correctly because of a problem with Firefox Accounts, so I encourage you to try the patch by calling someone and make the call fail on purpose (incoming and outgoing calls).

Comment 6

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

Comment on attachment 8513594 [details] [diff] [review]
Bug 1065201: Desktop new sounds

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

Looks okay but I have a feeling we could generate a single sound file instead of two (see comments below). F+ until we discuss this further.

::: browser/components/loop/encode-sounds.js
@@ +1,1 @@
> +#! /usr/bin/env node

Nit: With this defined you can get rid of the extension in the filename, like build-jsx.

@@ +17,5 @@
> +}, {});
> +
> +var template = fs.readFileSync('sounds.js.template').toString();
> +var desktop = util.format(template, JSON.stringify(encodedSounds));
> +var standalone = util.format(template, JSON.stringify(soundsMap));

How about generating a single shared file usable in both environments, which contents would be like:

var sounds = {
  connecting: {
    standalone: "shared/sounds/connecting.ogg", 
    desktop:    "data:audio/ogg;base64,T2dnUwACAAAAAAAAAA…"
  },
  // …
};

That'd mean keeping the _isDesktop() check to find which sound uri to use, but I'd personally would find this approach a bit cleaner and maintainable.

@@ +20,5 @@
> +var desktop = util.format(template, JSON.stringify(encodedSounds));
> +var standalone = util.format(template, JSON.stringify(soundsMap));
> +
> +fs.writeFileSync('content/js/sounds.js', desktop);
> +fs.writeFileSync('standalone/content/js/sounds.js', standalone);

Could you please document how this script is to be used somewhere? The README seems to be the right place to do so for now.

::: browser/components/loop/sounds.js.template
@@ +1,5 @@
> +var loop = loop || {};
> +loop.sounds = (function() {
> +  var sounds = %s;
> +  return sounds;
> +})();

Nit: I'm uneasy seeing this listed at the root of the browser component directory; do we really need a separate template file here? I could see it being defined in the script file itself.

Comment 7

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

Now I think about it, we could even decide to use data uris everywhere (dunno about frontend performances though).

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8513594 [details] [diff] [review]
Bug 1065201: Desktop new sounds

So I during my talk with tOkeshu, we decided to move this bug to implement the following:

 * Leave the .ogg files where they are in shared/
 * Implement `MozLoopAPI::getSoundBlob(soundFile, callback)`, which will do the following:
    - Immediately returns a Blob from the in-memory cache of sounds that were
      requested before. This is an early return, no processing needs to happen
      anymore.
    - Read the appropriate sound file from the omni-jar using OS.File[1] or
      using an XHR that returns a TypedArray, if the OS.File approach turns out
      to be problematic and/ or difficult to accomplish.
    - Invoke the callback when the previous operation fails like:
      `callback(cloneValueInto(error, targetWindow));return;`
    - Invoke the callback upon success like:
      `callback(null, cloneValueInto(blob, targetWindow));`
    - Note: to be able to use the Blob constructor, you'll need to import it:
      `Cu.importGlobalProperties(["Blob"]);`
 * The callee may then play this sounds by passing an object URL to the Audio element: `audio.src = URL.createObjectURL(blob)`.

Since bug 1088617 we're able to use blob: URLs in about: pages, so we should be good here. Please note that that bug is tracking Fx 36, so if we want to use this in Fx 35, we'll need to uplift the patch in there too.

Let me know if I missed anything!

[1] https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File_for_the_main_thread

Comment 9

[Mike de Boer [:mikedeboer]]

Of course I forgot two details:

 * Remove the `startAlerting` and `stopAlerting` functions from MozLoopAPI.jsm and the `loop.ringtone` pref. We assessed that this is causing an unnecessary split in the code to deal with sounds and currently the pref is not useful as there's no UI to customize the ringtone. Content can now play the ringtone without help from chrome.
 * We _do_ want to stop ringing when window is hidden, so we'll need to dispatch an event on the targetWindow to forward the 'visibilitychange' event to content. This will be similar to the implementation of the 'LoopStatusChanged' event.

Comment 10

[Romain Gauthier [:tOkeshu]]

Attachment: WIP Desktop new sounds

Comment 11

[Romain Gauthier [:tOkeshu]]

Attachment: Bug 1065201: Desktop new sounds

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8516022 [details] [diff] [review]
Bug 1065201: Desktop new sounds

I'm happy for Mike to review this.

Comment 13

[Adam Roach [:abr]]

Comment on attachment 8516022 [details] [diff] [review]
Bug 1065201: Desktop new sounds

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

r- on the patch: this is a regression, as it removes an important part of the alerting experience.

::: browser/components/loop/MozLoopAPI.jsm
@@ -463,5 @@
> -      enumerable: true,
> -      writable: true,
> -      value: function() {
> -        let chromeWindow = getChromeWindow(targetWindow);
> -        chromeWindow.getAttention();

The "startAlerting" method cannot go away unless you find some other way to trigger "getAttention()".

Per the conversation Mike de Boer and I had in #loop earlier today, we think the best way forward is to leave playing the ringing tone in chrome (along with the other alerting functions, which will expand in the future), rather than trying to unify it with the rest of the sound handling.

Comment 14

[Adam Roach [:abr]]

Comment on attachment 8516022 [details] [diff] [review]
Bug 1065201: Desktop new sounds

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

Note that I'm not doing a full review here -- I just wanted to weigh in on a couple of the areas that are trickier to get right than the rest. The CSP is another such place (and I'm wondering how this got tested, as we're presumably testing with DEBUG defined...)

::: browser/app/profile/firefox.js
@@ +1629,5 @@
>  pref("loop.debug.sdk", false);
>  #ifdef DEBUG
>  pref("loop.CSP", "default-src 'self' about: file: chrome: http://localhost:*; img-src 'self' data: http://www.gravatar.com/ about: file: chrome:; font-src 'none'; connect-src wss://*.tokbox.com https://*.opentok.com https://*.tokbox.com wss://*.mozilla.com https://*.mozilla.org wss://*.mozaws.net http://localhost:* ws://localhost:*");
>  #else
> +pref("loop.CSP", "default-src 'self' about: file: chrome:; img-src 'self' data: http://www.gravatar.com/ about: file: chrome:; font-src 'none'; connect-src wss://*.tokbox.com https://*.opentok.com https://*.tokbox.com wss://*.mozilla.com https://*.mozilla.org wss://*.mozaws.net; media-src blob:;");

"media-sec blob:" needs to be added to loop.CSP for debug as well. Additionally, the trailing semicolon here is superfluous.

Comment 15

[Mike de Boer [:mikedeboer]]

Comment on attachment 8516022 [details] [diff] [review]
Bug 1065201: Desktop new sounds

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

Like Adam said, we'll need to keep the startAlerting and stopAlerting function. I'm sure we'll find a way to live with the duality here.

::: browser/components/loop/MozLoopAPI.jsm
@@ +13,5 @@
>  Cu.import("resource:///modules/loop/MozLoopService.jsm");
>  Cu.import("resource:///modules/loop/LoopRooms.jsm");
>  Cu.import("resource:///modules/loop/LoopContacts.jsm");
> +Cu.importGlobalProperties(["Blob"]);
> +Cu.importGlobalProperties(["XMLHttpRequest"]);

Please use the XPCOM component, so remove this line.

@@ +654,5 @@
> +        var url = `chrome://browser/content/loop/shared/sounds/${name}.ogg`;
> +
> +        xhr.open('GET', url, true);
> +        xhr.responseType = "arraybuffer";
> +        xhr.onload = function() {

What you're missing here is the error handling. Please check the response code and pass it to the callback:

```js
if (xhr.status < 400) {
  // success!
  callback(null, cloneValueInto(blob, targetWindow));
} else {
  callback(cloneValueInto(new Error(xhr.status + " " + xhr.statusText), targetWindow));
}
```

@@ +655,5 @@
> +
> +        xhr.open('GET', url, true);
> +        xhr.responseType = "arraybuffer";
> +        xhr.onload = function() {
> +          var blob = new Blob([xhr.response], {type: 'audio/ogg'});

nit: no single quotes, please.

::: browser/components/loop/content/shared/js/mixins.js
@@ +147,2 @@
>       */
> +    play: function(name, options) {

There's only one option; let's call it `loop` and document it as a Boolean.

@@ +163,5 @@
> +        rootObject.navigator.mozLoop.getAudioBlob(name, callback);
> +        return;
> +      }
> +
> +      var url = 'shared/sounds/' + name + '.ogg';

nit: no single quotes, please.

@@ +168,5 @@
> +      this._audioXHR = new XMLHttpRequest();
> +      this._audioXHR.open('GET', url, true);
> +      this._audioXHR.responseType = "arraybuffer";
> +      this._audioXHR.onload = function() {
> +        var type = this._audioXHR.getResponseHeader('Content-Type');

nit: again, single quotes.

::: browser/components/loop/test/desktop-local/conversationViews_test.js
@@ +52,5 @@
> +          platform: "test"
> +        };
> +      },
> +      getAudioBlob: sinon.spy(function(name, callback) {
> +        callback(new Blob([new ArrayBuffer(10)], {type: 'audio/ogg'}));

please make this `callback(null, new Blob([new ArrayBuffer(10)], {type: "audio/ogg"});`

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ +51,5 @@
>            platform: "test"
>          };
> +      },
> +      getAudioBlob: sinon.spy(function(name, callback) {
> +        callback(new Blob([new ArrayBuffer(10)], {type: 'audio/ogg'}));

please make this `callback(null, new Blob([new ArrayBuffer(10)], {type: "audio/ogg"});`

@@ +647,5 @@
>            });
>        });
>  
>        describe("session:network-disconnected", function() {
> +

nit: please remove this newline.

::: browser/components/loop/test/shared/views_test.js
@@ +26,5 @@
> +    fakeAudioXHR = {
> +      open: sinon.spy(),
> +      send: function() {},
> +      getResponseHeader: function(header) {
> +        if (header === 'Content-Type')

nit: single quotes.

@@ +27,5 @@
> +      open: sinon.spy(),
> +      send: function() {},
> +      getResponseHeader: function(header) {
> +        if (header === 'Content-Type')
> +          return 'audio/ogg';

nit: single quotes.

@@ +399,5 @@
> +          beforeEach(function() {
> +            mozLoop = navigator.mozLoop;
> +            navigator.mozLoop = {
> +              getAudioBlob: sinon.spy(function(name, callback) {
> +                callback(new Blob([new ArrayBuffer(10)], {type: 'audio/ogg'}));

please make this `callback(null, new Blob([new ArrayBuffer(10)], {type: "audio/ogg"});`

@@ +426,2 @@
>          describe("for both (standalone and desktop)", function() {
> +          beforeEahc(function() {

Hmm, did you run these tests? This typo would yield a JS error.

::: browser/components/loop/test/standalone/webapp_test.js
@@ +27,5 @@
> +    fakeAudioXHR = {
> +      open: sinon.spy(),
> +      send: function() {},
> +      getResponseHeader: function(header) {
> +        if (header === 'Content-Type')

nit: single quotes

@@ +28,5 @@
> +      open: sinon.spy(),
> +      send: function() {},
> +      getResponseHeader: function(header) {
> +        if (header === 'Content-Type')
> +          return 'audio/ogg';

nit: single quotes.

Comment 16

[Romain Gauthier [:tOkeshu]]

Attachment: Bug 1065201: Desktop new sounds

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8517403 [details] [diff] [review]
Bug 1065201: Desktop new sounds

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

Can you do an `hg|git mv browser/components/loop/content/shared/sounds/Firefox-Long.ogg browser/components/loop/content/shared/sounds/ringtone.ogg`? This way we'll preserve blame. Or is the new ringtone a different sound altogether? If so, please disregard this comment ;-)

::: browser/components/loop/MozLoopAPI.jsm
@@ +689,5 @@
> +    getAudioBlob: {
> +      enumerable: true,
> +      writable: true,
> +      value: function(name, callback) {
> +        let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].

nit: please write this as:

```js
let request = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
                .createInstance(Ci.nsIXMLHttpRequest);
```

@@ +706,5 @@
> +          let blob = new Blob([xhr.response], {type: "audio/ogg"});
> +          callback(null, cloneValueInto(blob, targetWindow));
> +        };
> +
> +        xhr.send(null);

nit: sending `null` explicitly is not necessary.

::: browser/components/loop/content/js/conversationViews.jsx
@@ +149,5 @@
>        };
>      },
>  
> +    componentDidMount: function() {
> +      this.play("ringtone", {loop: true});

I *think* we want to leave the ringtone out of this.

::: browser/components/loop/content/shared/js/mixins.js
@@ +174,5 @@
> +      }.bind(this));
> +    },
> +
> +    _getAudioBlob: function(name, callback) {
> +      if (!this._isLoopDesktop()) {

This basically means that your code below will never run for the standalone. I think that is not what you want.

@@ +184,5 @@
> +        return;
> +      }
> +
> +      var url = "shared/sounds/" + name + ".ogg";
> +      this._audioXHR = new XMLHttpRequest();

Why `this._audioXHR`? You can just keep everything in scope...
When you change this, please name the var `request`.

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ -543,5 @@
>        onAfterFeedbackReceived: React.PropTypes.func.isRequired
>      },
>  
> -    componentDidMount: function() {
> -      this.play("terminated");

What happened here? hg-git fubar?

I think you want this added, not removed.

::: browser/components/loop/test/desktop-local/conversationViews_test.js
@@ +50,5 @@
> +          version: "42",
> +          channel: "test",
> +          platform: "test"
> +        };
> +      },

Where does all this above come from?

@@ +150,5 @@
>  
>    describe("PendingConversationView", function() {
> +    beforeEach(function() {
> +      sandbox.stub(window, "XMLHttpRequest").returns(fakeAudioXHR);
> +    });

Do we still need this? We're not using XHR audio retrieval on desktop...

@@ -413,5 @@
>  
>      beforeEach(function() {
> -      navigator.mozLoop = {
> -        getLoopCharPref: function() { return "fake"; },
> -        appVersionInfo: sinon.spy()

You can add these to the stub you're adding above.

@@ +463,5 @@
>          dispatcher: dispatcher,
>          client: {},
>          sdkDriver: {}
>        });
> +      sandbox.stub(window, "XMLHttpRequest").returns(fakeAudioXHR);

I don't think we need this on desktop.

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ +844,5 @@
>             sinon.assert.calledWithExactly(model.set, "selectedCallType", "audio");
>             sinon.assert.calledOnce(model.trigger);
>             sinon.assert.calledWithExactly(model.trigger, "accept");
>           });
> +

nit: superfluous newline

::: browser/components/loop/test/shared/views_test.js
@@ +393,5 @@
> +             });
> +        });
> +
> +        describe("for desktop", function() {
> +          var mozLoop;

please call this `origMozLoop` to make sure this used for caching

@@ +422,3 @@
>                 expect(fakeAudio.loop).to.not.equal(true);
>               });
> +

nit: superfluous newline.

Comment 18

[Romain Gauthier [:tOkeshu]]

(In reply to Mike de Boer [:mikedeboer] from comment #17)
> Comment on attachment 8517403 [details] [diff] [review]
> Bug 1065201: Desktop new sounds
> 
> Review of attachment 8517403 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you do an `hg|git mv
> browser/components/loop/content/shared/sounds/Firefox-Long.ogg
> browser/components/loop/content/shared/sounds/ringtone.ogg`? This way we'll
> preserve blame. Or is the new ringtone a different sound altogether? If so,
> please disregard this comment ;-)
> 

Firefox-Long.ogg and ringtone.ogg are different files.

> ::: browser/components/loop/content/js/conversationViews.jsx
> @@ +149,5 @@
> >        };
> >      },
> >  
> > +    componentDidMount: function() {
> > +      this.play("ringtone", {loop: true});
> 
> I *think* we want to leave the ringtone out of this.

Well, for the standalone UI we still want the ringtone to be played.

> ::: browser/components/loop/content/shared/js/mixins.js
> @@ +174,5 @@
> > +      }.bind(this));
> > +    },
> > +
> > +    _getAudioBlob: function(name, callback) {
> > +      if (!this._isLoopDesktop()) {
> 
> This basically means that your code below will never run for the standalone.
> I think that is not what you want.

Yep my bad

> @@ +184,5 @@
> > +        return;
> > +      }
> > +
> > +      var url = "shared/sounds/" + name + ".ogg";
> > +      this._audioXHR = new XMLHttpRequest();
> 
> Why `this._audioXHR`? You can just keep everything in scope...
> When you change this, please name the var `request`.

I actually forgot to handle the abort case.
We may have a race condition where the component get unmount before the sound is retrieved.
In this case you don't want the sound to be played.

> ::: browser/components/loop/standalone/content/js/webapp.jsx
> @@ -543,5 @@
> >        onAfterFeedbackReceived: React.PropTypes.func.isRequired
> >      },
> >  
> > -    componentDidMount: function() {
> > -      this.play("terminated");
> 
> What happened here? hg-git fubar?
> 
> I think you want this added, not removed.

This has been actually moved to the shared FeedbackView.

> ::: browser/components/loop/test/desktop-local/conversationViews_test.js
> @@ +50,5 @@
> > +          version: "42",
> > +          channel: "test",
> > +          platform: "test"
> > +        };
> > +      },
> 
> Where does all this above come from?
> 

It's needed for some test. It fails otherwise.

> @@ -413,5 @@
> >  
> >      beforeEach(function() {
> > -      navigator.mozLoop = {
> > -        getLoopCharPref: function() { return "fake"; },
> > -        appVersionInfo: sinon.spy()
> 
> You can add these to the stub you're adding above.

I did not understand this comment since this code is removed.

Comment 19

[Romain Gauthier [:tOkeshu]]

Attachment: Bug 1065201: Desktop new sounds

Comment 20

[Mike de Boer [:mikedeboer]]

Comment on attachment 8517578 [details] [diff] [review]
Bug 1065201: Desktop new sounds

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

Ship it!

::: browser/components/loop/test/desktop-local/conversationViews_test.js
@@ +35,5 @@
> +      send: function() {},
> +      abort: function() {},
> +      getResponseHeader: function(header) {
> +        if (header === 'Content-Type')
> +          return 'audio/ogg';

nit: please use double quotes.

@@ +53,5 @@
> +          platform: "test"
> +        };
> +      },
> +      getAudioBlob: sinon.spy(function(name, callback) {
> +        callback(null, new Blob([new ArrayBuffer(10)], {type: 'audio/ogg'}));

nit: please use double quotes.

Comment 21

[Romain Gauthier [:tOkeshu]]

Attachment: Bug 1065201: Desktop new sounds

Comment 22

[Mike de Boer [:mikedeboer]]

Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/31a2e1b8b32b

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/31a2e1b8b32b

Comment 24

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

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

Comment 25

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

Comment on attachment 8518125 [details] [diff] [review]
Bug 1065201: Desktop new sounds

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