Play a chime when the hotword is triggered.

Play a chime when the hotword is triggered.

BUG=397019
Committed: https://crrev.com/7590f797b76fb7c6df196f394623e840917b20da
Cr-Commit-Position: refs/heads/master@{#296343}

[Anand Mistry (off Chromium), 2014-09-16 07:42:46]

amistry@chromium.org changed reviewers:
+ dbeam@chromium.org, rlp@chromium.org

[rpetterson, 2014-09-16 07:55:46]

lgtm

https://codereview.chromium.org/545373005/diff/1/chrome/browser/resources/hot...
File chrome/browser/resources/hotword/state_manager.js (right):

https://codereview.chromium.org/545373005/diff/1/chrome/browser/resources/hot...
chrome/browser/resources/hotword/state_manager.js:47: * Hotword trigger audio
notification... a.k.a The Chime (tm)
nit: end with period.

[Dan Beam, 2014-09-16 07:57:00]

where did you get this wav and how is it licensed?

[chromium-reviews, 2014-09-16 08:15:52]

From the current hotword extension, in google3 (not third-party).

On 16 September 2014 17:57, <dbeam@chromium.org> wrote:

> where did you get this wav and how is it licensed?
>
> https://codereview.chromium.org/545373005/
>



-- 
Anand K. Mistry
Software Engineer
*Google* Australia

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Anand Mistry (off Chromium), 2014-09-23 02:59:53]

So, I'm moving forward under the assumption we can't open-source the chime. In
that case, I'm pulling it from the shared module instead. I've deleted the chime
from this change.

On 2014/09/16 08:15:52, chromium-reviews wrote:
> From the current hotword extension, in google3 (not third-party).
> 
> On 16 September 2014 17:57, <mailto:dbeam@chromium.org> wrote:
> 
> > where did you get this wav and how is it licensed?
> >
> > https://codereview.chromium.org/545373005/
> >
> 
> 
> 
> -- 
> Anand K. Mistry
> Software Engineer
> *Google* Australia
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Dan Beam, 2014-09-23 03:43:13]

https://codereview.chromium.org/545373005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/hotword/state_manager.js (right):

https://codereview.chromium.org/545373005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword/state_manager.js:58:
document.body.appendChild(this.chime_);
what happens if this fails to load?

https://codereview.chromium.org/545373005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword/state_manager.js:242: this.chime_.play();
can this handle being called before the sound stops playing or if not loaded
yet?

[Anand Mistry (off Chromium), 2014-09-24 01:35:02]

https://codereview.chromium.org/545373005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/hotword/state_manager.js (right):

https://codereview.chromium.org/545373005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword/state_manager.js:58:
document.body.appendChild(this.chime_);
On 2014/09/23 03:43:13, Dan Beam wrote:
> what happens if this fails to load?

There'll be a warning on the console for this extension that the file couldn't
be found. But since this extension is hidden from the user unless they pass
--show-component-extension-options, it shouldn't be user visible. Calling play()
appears to just do nothing.

https://codereview.chromium.org/545373005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword/state_manager.js:242: this.chime_.play();
On 2014/09/23 03:43:13, Dan Beam wrote:
> can this handle being called before the sound stops playing or if not loaded
> yet?

The sound is shorter than 1 second. I don't think it's reasonable to try an
handle playing if it's already playing. As for not loaded, the file either
doesn't exist, which should only happen when the shared module isn't installed
and hence this code path won't be invoked, or the URL was slow to be loaded. I'm
not sure if it's worth trying to handle that.

[Dan Beam, 2014-09-24 02:02:17]

lgtm

[Anand Mistry (off Chromium), 2014-09-24 03:12:57]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2014-09-24 03:13:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/545373005/40001

[commit-bot: I haz the power, 2014-09-24 03:51:54]

Committed patchset #3 (id:40001) as ea2a4b0f0dd9a7fc325dc5ae5084366757c379cc

[commit-bot: I haz the power, 2014-09-24 03:52:40]

Patchset 3 (id:??) landed as
https://crrev.com/7590f797b76fb7c6df196f394623e840917b20da
Cr-Commit-Position: refs/heads/master@{#296343}