Hotword Audio Verification app: control the hotword settings

Hotword Audio Verification app: control the hotword settings

Adds a function to the hotword private API to allow the Hotword Audio
Verification app to set the always-on-hotword setting.

Gives control over the always-on-hotword pref to the app from the settings page.

This cl is dependent on https://codereview.chromium.org/528113003/

BUG=390086
Committed: https://crrev.com/e9273647058dda9eb46331ec7f73c424d7dfbbbd
Cr-Commit-Position: refs/heads/master@{#296661}

[kcarattini, 2014-09-11 06:56:44]

kcarattini@chromium.org changed reviewers:
+ benwells@chromium.org, rlp@chromium.org

[rpetterson, 2014-09-11 20:14:18]

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/hotword_private/hotword_private_api.cc
(right):

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/hotword_private/hotword_private_api.cc:104:
scoped_ptr<api::hotword_private::SetEnabled::Params> params(
This should be:
scoped_ptr<api::hotword_private::SetHotwordAlwaysOnSearchEnabled::Params> params

And if you could also fix it in SetAudioLogging (which was my bad) that would be
super. :)

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/hotword_private/hotword_private_api.h
(right):

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/hotword_private/hotword_private_api.h:72: class
HotwordPrivateSetHotwordAlwaysOnSearchEnabledFunction
Perhaps just SetAlwaysOnSearchEnabled? I think the second Hotword is redundant
since it's in the hotword private api. That would make it a bit more manageable
in length as well.

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/hotword_private/hotword_private_apitest.cc
(right):

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/hotword_private/hotword_private_apitest.cc:163:
ExtensionTestMessageListener listenerTrue("ready", false);
Do you mean true here instead of false?

https://codereview.chromium.org/559303002/diff/1/chrome/browser/resources/hot...
File chrome/browser/resources/hotword_audio_verification/main.js (right):

https://codereview.chromium.org/559303002/diff/1/chrome/browser/resources/hot...
chrome/browser/resources/hotword_audio_verification/main.js:45:
flow.advanceStep.bind(flow));
Based on the discussion with Hwi, should this move to the $('training') button
below?

https://codereview.chromium.org/559303002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/559303002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:228:
$('hotword-always-on-search-checkbox').checked = false;
Does setting checked to false actually update the preference so that it's not
set? I'm not as familiar with exactly when checkboxes update the preference
based their handlers.

https://codereview.chromium.org/559303002/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/559303002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/browser_options_handler.cc:1627:
DCHECK(profile->GetPrefs()->GetBoolean(
I would do all these DCHECKs in the similar if statement below unless you plan
on acting on the state of the preference.

https://codereview.chromium.org/559303002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/browser_options_handler.cc:1636: if (retrain) {
Would the following be clearer?

LaunchMode launch_mode = HotwordService::HOTWORD_AND_AUDIO_HISTORY;
if (retrain)
  launch_mode = HotwordService::SPEECH_TRAINING;
else if (prefs::kHotwordAudioLoggingEnabled)
  launch_mode = HotwordService::HOTWORD_ONLY;
hotword_service->LaunchHotwordAudioVerificationApp(launch_mode);

https://codereview.chromium.org/559303002/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/hotword_private.idl (right):

https://codereview.chromium.org/559303002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/hotword_private.idl:70: static void
setHotwordAlwaysOnSearchEnabled(boolean state,
I think you got this function and the one above mixed up.

[kcarattini, 2014-09-12 01:02:06]

kcarattini@chromium.org changed reviewers:
- benwells@chromium.org

[kcarattini, 2014-09-12 01:02:08]

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/hotword_private/hotword_private_api.cc
(right):

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/hotword_private/hotword_private_api.cc:104:
scoped_ptr<api::hotword_private::SetEnabled::Params> params(
On 2014/09/11 20:14:17, rpetterson wrote:
> This should be:
> scoped_ptr<api::hotword_private::SetHotwordAlwaysOnSearchEnabled::Params>
params
> 
> And if you could also fix it in SetAudioLogging (which was my bad) that would
be
> super. :)

Done.

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/hotword_private/hotword_private_api.h
(right):

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/hotword_private/hotword_private_api.h:72: class
HotwordPrivateSetHotwordAlwaysOnSearchEnabledFunction
On 2014/09/11 20:14:17, rpetterson wrote:
> Perhaps just SetAlwaysOnSearchEnabled? I think the second Hotword is redundant
> since it's in the hotword private api. That would make it a bit more
manageable
> in length as well.

The name of the pref being changed in prefs.h is kHotwordAlwaysOnSearchEnabled,
and I like the consistency of having the function name match the pref name. Let
me know if you feel strongly about this.

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/hotword_private/hotword_private_apitest.cc
(right):

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/hotword_private/hotword_private_apitest.cc:163:
ExtensionTestMessageListener listenerTrue("ready", false);
On 2014/09/11 20:14:17, rpetterson wrote:
> Do you mean true here instead of false?

I don't think I need to wait for a reply here because I'm using a different
listener in the second case. The AudioLogging test does the same thing. Let me
know if that's incorrect.

https://codereview.chromium.org/559303002/diff/1/chrome/browser/resources/hot...
File chrome/browser/resources/hotword_audio_verification/main.js (right):

https://codereview.chromium.org/559303002/diff/1/chrome/browser/resources/hot...
chrome/browser/resources/hotword_audio_verification/main.js:45:
flow.advanceStep.bind(flow));
On 2014/09/11 20:14:17, rpetterson wrote:
> Based on the discussion with Hwi, should this move to the $('training') button
> below?

Done.

https://codereview.chromium.org/559303002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/559303002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:228:
$('hotword-always-on-search-checkbox').checked = false;
On 2014/09/11 20:14:17, rpetterson wrote:
> Does setting checked to false actually update the preference so that it's not
> set? I'm not as familiar with exactly when checkboxes update the preference
> based their handlers.

My understanding is that this function is called after the checkbox is checked
but before the pref is updated. Returning true stops the pref from being set.
See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

https://codereview.chromium.org/559303002/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/559303002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/browser_options_handler.cc:1627:
DCHECK(profile->GetPrefs()->GetBoolean(
On 2014/09/11 20:14:17, rpetterson wrote:
> I would do all these DCHECKs in the similar if statement below unless you plan
> on acting on the state of the preference.

Done.

https://codereview.chromium.org/559303002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/browser_options_handler.cc:1636: if (retrain) {
On 2014/09/11 20:14:17, rpetterson wrote:
> Would the following be clearer?
> 
> LaunchMode launch_mode = HotwordService::HOTWORD_AND_AUDIO_HISTORY;
> if (retrain)
>   launch_mode = HotwordService::SPEECH_TRAINING;
> else if (prefs::kHotwordAudioLoggingEnabled)
>   launch_mode = HotwordService::HOTWORD_ONLY;
> hotword_service->LaunchHotwordAudioVerificationApp(launch_mode);

Done.

https://codereview.chromium.org/559303002/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/hotword_private.idl (right):

https://codereview.chromium.org/559303002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/hotword_private.idl:70: static void
setHotwordAlwaysOnSearchEnabled(boolean state,
On 2014/09/11 20:14:17, rpetterson wrote:
> I think you got this function and the one above mixed up.

Done. Thanks for catching that.

[kcarattini, 2014-09-12 01:09:07]

kcarattini@chromium.org changed reviewers:
+ dbeam@chromium.org, kalman@chromium.org, mpearson@chromium.org

[kcarattini, 2014-09-12 01:09:08]

kalman@chromium.org: Please review changes in 
chrome/browser/extensions/api/...
chrome/common/extensions/api/...

mpearson@chromium.org: Please review changes in 
tools/metrics/histograms/histograms.xml
extensions/browser/extension_function_histogram_value.h

dbeam@chromium.org: Please review changes in 
chrome/browser/resources/...
chrome/browser/ui/...

[kcarattini, 2014-09-12 01:10:19]

kcarattini@chromium.org changed reviewers:
- kalman@chromium.org

[kcarattini, 2014-09-12 01:11:27]

kcarattini@chromium.org changed reviewers:
+ asargent@chromium.org

[kcarattini, 2014-09-12 01:11:27]

asargent@chromium.org: Please review changes in 

chrome/browser/extensions/api/...
chrome/common/extensions/api/...

since kalman@ is OOO

[benwells, 2014-09-12 02:58:12]

benwells@chromium.org changed reviewers:
+ benwells@chromium.org

[benwells, 2014-09-12 02:58:13]

Kendra: I took some time and had a look. This will be my last review before my
vacation though :)

bits I am owner for lgtm

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/hotword_private/hotword_private_apitest.cc
(right):

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/hotword_private/hotword_private_apitest.cc:163:
ExtensionTestMessageListener listenerTrue("ready", false);
On 2014/09/12 01:02:07, kcarattini wrote:
> On 2014/09/11 20:14:17, rpetterson wrote:
> > Do you mean true here instead of false?
> 
> I don't think I need to wait for a reply here because I'm using a different
> listener in the second case. The AudioLogging test does the same thing. Let me
> know if that's incorrect.
> 

This is correct. The boolean argument is to say whether this listener sends a
reply. You only need true if you do something like listenerTrue.reply('blah') at
some point.

Maybe it's a bit confusing as you have true in the name and false in the name
later. If you want you can reuse the same listener (as it listens for the same
thing) and call reset() on it after the first test.

[benwells, 2014-09-12 02:58:56]

On 2014/09/12 02:58:13, benwells (OOO until October 7) wrote:
> Kendra: I took some time and had a look. This will be my last review before my
> vacation though :)
> 
> bits I am owner for lgtm

asargent: so this means you don't need to look (unless you want to, of course!)

> 
>
https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
> File chrome/browser/extensions/api/hotword_private/hotword_private_apitest.cc
> (right):
> 
>
https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
> chrome/browser/extensions/api/hotword_private/hotword_private_apitest.cc:163:
> ExtensionTestMessageListener listenerTrue("ready", false);
> On 2014/09/12 01:02:07, kcarattini wrote:
> > On 2014/09/11 20:14:17, rpetterson wrote:
> > > Do you mean true here instead of false?
> > 
> > I don't think I need to wait for a reply here because I'm using a different
> > listener in the second case. The AudioLogging test does the same thing. Let
me
> > know if that's incorrect.
> > 
> 
> This is correct. The boolean argument is to say whether this listener sends a
> reply. You only need true if you do something like listenerTrue.reply('blah')
at
> some point.
> 
> Maybe it's a bit confusing as you have true in the name and false in the name
> later. If you want you can reuse the same listener (as it listens for the same
> thing) and call reset() on it after the first test.

[rpetterson, 2014-09-12 03:03:33]

LGTM

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/hotword_private/hotword_private_api.h
(right):

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/hotword_private/hotword_private_api.h:72: class
HotwordPrivateSetHotwordAlwaysOnSearchEnabledFunction
On 2014/09/12 01:02:07, kcarattini wrote:
> On 2014/09/11 20:14:17, rpetterson wrote:
> > Perhaps just SetAlwaysOnSearchEnabled? I think the second Hotword is
redundant
> > since it's in the hotword private api. That would make it a bit more
> manageable
> > in length as well.
> 
> The name of the pref being changed in prefs.h is
kHotwordAlwaysOnSearchEnabled,
> and I like the consistency of having the function name match the pref name.
Let
> me know if you feel strongly about this.

We only use SetEnabled for the pref call kHotwordSearchEnabled :) But I do not
feel strongly. It just seemed rather long and I was offering a suggestion.

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/hotword_private/hotword_private_apitest.cc
(right):

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/hotword_private/hotword_private_apitest.cc:163:
ExtensionTestMessageListener listenerTrue("ready", false);
On 2014/09/12 02:58:13, benwells (OOO until October 7) wrote:
> On 2014/09/12 01:02:07, kcarattini wrote:
> > On 2014/09/11 20:14:17, rpetterson wrote:
> > > Do you mean true here instead of false?
> > 
> > I don't think I need to wait for a reply here because I'm using a different
> > listener in the second case. The AudioLogging test does the same thing. Let
me
> > know if that's incorrect.
> > 
> 
> This is correct. The boolean argument is to say whether this listener sends a
> reply. You only need true if you do something like listenerTrue.reply('blah')
at
> some point.
> 
> Maybe it's a bit confusing as you have true in the name and false in the name
> later. If you want you can reuse the same listener (as it listens for the same
> thing) and call reset() on it after the first test.

Got it. Thanks for the clarification!

[kcarattini, 2014-09-12 05:17:55]

kcarattini@chromium.org changed reviewers:
- asargent@chromium.org

[kcarattini, 2014-09-12 05:17:56]

Thanks, Ben! I thought you'd gone already.
Thanks, Rachel!

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/hotword_private/hotword_private_api.h
(right):

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/hotword_private/hotword_private_api.h:72: class
HotwordPrivateSetHotwordAlwaysOnSearchEnabledFunction
On 2014/09/12 03:03:32, rpetterson wrote:
> On 2014/09/12 01:02:07, kcarattini wrote:
> > On 2014/09/11 20:14:17, rpetterson wrote:
> > > Perhaps just SetAlwaysOnSearchEnabled? I think the second Hotword is
> redundant
> > > since it's in the hotword private api. That would make it a bit more
> > manageable
> > > in length as well.
> > 
> > The name of the pref being changed in prefs.h is
> kHotwordAlwaysOnSearchEnabled,
> > and I like the consistency of having the function name match the pref name.
> Let
> > me know if you feel strongly about this.
> 
> We only use SetEnabled for the pref call kHotwordSearchEnabled :) But I do not
> feel strongly. It just seemed rather long and I was offering a suggestion.

Acknowledged.

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/hotword_private/hotword_private_apitest.cc
(right):

https://codereview.chromium.org/559303002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/hotword_private/hotword_private_apitest.cc:163:
ExtensionTestMessageListener listenerTrue("ready", false);
On 2014/09/12 02:58:13, benwells (OOO until October 7) wrote:
> On 2014/09/12 01:02:07, kcarattini wrote:
> > On 2014/09/11 20:14:17, rpetterson wrote:
> > > Do you mean true here instead of false?
> > 
> > I don't think I need to wait for a reply here because I'm using a different
> > listener in the second case. The AudioLogging test does the same thing. Let
me
> > know if that's incorrect.
> > 
> 
> This is correct. The boolean argument is to say whether this listener sends a
> reply. You only need true if you do something like listenerTrue.reply('blah')
at
> some point.
> 
> Maybe it's a bit confusing as you have true in the name and false in the name
> later. If you want you can reuse the same listener (as it listens for the same
> thing) and call reset() on it after the first test.

Done.

[Mark P, 2014-09-12 16:49:12]

histograms.xml lgtm

[kcarattini, 2014-09-23 02:50:47]

Ping to dbeam@

dbeam@chromium.org: Please review changes in 
chrome/browser/resources/...
chrome/browser/ui/...

[Dan Beam, 2014-09-23 05:53:39]

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/hotword_audio_verification/main.js (right):

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:50: if
(chrome.hotwordPrivate) {
do you expect this to fail ever?

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:51: if
(chrome.hotwordPrivate.setAudioLoggingEnabled) {
nit: no curlies

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:219: if
(($('hotword-always-on-search-checkbox').checked)) {
remove extra parens

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:219: if
(($('hotword-always-on-search-checkbox').checked)) {
nit:

  if (!$('hotword-always-on-search-checkbox').checked)
    return false;

  $('hotword-always-on-search-checkbox').checked = false;
  chrome.send('launchHotwordAudioVerificationApp', [false]);
  return true;

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:221:
chrome.send('launchHotwordAudioVerificationApp', [Boolean(false)]);
just false... no Boolean() required

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1637: if
(!hotword_service) {
when will we hit this case?

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1639: }
nit: no curlies

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1642:
args->GetBoolean(0, &retrain);
unless this is optional,

  bool success = args->GetBoolean(0, &retrain);
  DCHECK(success);

[kcarattini, 2014-09-23 06:46:16]

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/hotword_audio_verification/main.js (right):

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:50: if
(chrome.hotwordPrivate) {
On 2014/09/23 05:53:38, Dan Beam wrote:
> do you expect this to fail ever?

No. Changed to assert.

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:51: if
(chrome.hotwordPrivate.setAudioLoggingEnabled) {
On 2014/09/23 05:53:38, Dan Beam wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:219: if
(($('hotword-always-on-search-checkbox').checked)) {
On 2014/09/23 05:53:38, Dan Beam wrote:
> remove extra parens

Done.

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:219: if
(($('hotword-always-on-search-checkbox').checked)) {
On 2014/09/23 05:53:38, Dan Beam wrote:
> nit:
> 
>   if (!$('hotword-always-on-search-checkbox').checked)
>     return false;
> 
>   $('hotword-always-on-search-checkbox').checked = false;
>   chrome.send('launchHotwordAudioVerificationApp', [false]);
>   return true;

Done.

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:221:
chrome.send('launchHotwordAudioVerificationApp', [Boolean(false)]);
On 2014/09/23 05:53:38, Dan Beam wrote:
> just false... no Boolean() required

Done.

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1637: if
(!hotword_service) {
On 2014/09/23 05:53:39, Dan Beam wrote:
> when will we hit this case?

I think the hotword service should exist for all profiles. rlp@ -- can you
confirm this? Changed to DCHECK.

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1639: }
On 2014/09/23 05:53:39, Dan Beam wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1642:
args->GetBoolean(0, &retrain);
On 2014/09/23 05:53:39, Dan Beam wrote:
> unless this is optional,
> 
>   bool success = args->GetBoolean(0, &retrain);
>   DCHECK(success);

Done.

[Dan Beam, 2014-09-23 18:45:18]

lgtm, but wait for rlp@'s input on whether the hotword service always exists (I
remember vaguely it might not during tests or something?).

https://codereview.chromium.org/559303002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/hotword_audio_verification/main.js (right):

https://codereview.chromium.org/559303002/diff/100001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/main.js:50:
assert(chrome.hotwordPrivate);
can you just remove this as chrome.hotwordPrivate.* would exploded on the next
line anyways

https://codereview.chromium.org/559303002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/559303002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1637:
DCHECK(hotword_service);
^ can you move |hotword_service|'s initialization lower (right before it's
used)?

[rpetterson, 2014-09-23 19:02:22]

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/559303002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1637: if
(!hotword_service) {
On 2014/09/23 06:46:16, kcarattini wrote:
> On 2014/09/23 05:53:39, Dan Beam wrote:
> > when will we hit this case?
> 
> I think the hotword service should exist for all profiles. rlp@ -- can you
> confirm this? Changed to DCHECK.

No, not for incognito (and currently guest). Better to do if (!hotword) return
as you had before.

[kcarattini, 2014-09-23 23:56:50]

Thanks!

https://codereview.chromium.org/559303002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/hotword_audio_verification/main.js (right):

https://codereview.chromium.org/559303002/diff/100001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/main.js:50:
assert(chrome.hotwordPrivate);
On 2014/09/23 18:45:18, Dan Beam wrote:
> can you just remove this as chrome.hotwordPrivate.* would exploded on the next
> line anyways

Done.

https://codereview.chromium.org/559303002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/559303002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1637:
DCHECK(hotword_service);
On 2014/09/23 18:45:18, Dan Beam wrote:
> ^ can you move |hotword_service|'s initialization lower (right before it's
> used)?

Done.

[kcarattini, 2014-09-25 04:46:00]

The CQ bit was checked by kcarattini@chromium.org

[commit-bot: I haz the power, 2014-09-25 04:46:25]

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

[commit-bot: I haz the power, 2014-09-25 05:56:09]

Committed patchset #8 (id:140001) as 7558d3679d2df347c4cae959dab0426ee805cfe4

[commit-bot: I haz the power, 2014-09-25 05:57:26]

Patchset 8 (id:??) landed as
https://crrev.com/e9273647058dda9eb46331ec7f73c424d7dfbbbd
Cr-Commit-Position: refs/heads/master@{#296661}