Hotword Audio Verification App: Adds to hotwordPrivate API

Hotword Audio Verification App: Adds to hotwordPrivate API

Adds a function to hotwordPrivate API for the Hotword Audio Verification app with. This function allows the app to check which mode it should launch in.

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

[kcarattini, 2014-09-09 01:37:20]

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

[kcarattini, 2014-09-09 01:45:38]

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

[kcarattini, 2014-09-09 01:45:38]

benwells@ please review for OWNERS approval in
chrome/browser/extensions/api/...
chrome/common/extensions/...
extensions/common/permissions/api_permission.h

[rpetterson, 2014-09-09 20:07:32]

General comment/question, why create a separate private api with just one
function? Why not add it to the existing hotword private api?

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:483: } else if (pref_name ==
std::string(prefs::kHotwordAudioLoggingEnabled)){
Does this function (SetHotwordAudioVerificationLaunchState) get called
elsewhere? If not, then this condition will never be met since above we DCHECK
for pref_name == std::string(prefs::kHotwordAlwaysOnSearchEnabled.

[benwells, 2014-09-09 20:57:31]

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/hotword_audio_verification_private/hotword_audio_verification_private_api.cc
(right):

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/hotword_audio_verification_private/hotword_audio_verification_private_api.cc:18:
// TODO(kcarattini): Do better error handling.
Error handling is pretty easy. You just set error_ to be a string and return
false, and the rest happens. E.g.
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/hotword_audio_verification_private/hotword_audio_verification_private_api.cc:22:
result.launch_mode =
You need {} as this is a multi-line body.

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/hotword_audio_verification_private/hotword_audio_verification_private_api.h
(right):

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/hotword_audio_verification_private/hotword_audio_verification_private_api.h:12:
#include "extensions/browser/browser_context_keyed_api_factory.h"
I don't think all (most?) of these includes are needed. Can you trim them down?

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/hotword_audio_verification/flow.js (right):

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/flow.js:29: Flow.FLOWS_ = [
Was there a reason to put this on Flow instead of being a global const?

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/flow.js:58:
Flow.prototype.getFlowForSetting_ = function(state) {
I think this function should be called startFlowForSetting, or something similar
that communicates the flow is going to be initiated.

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:455: DCHECK_EQ(pref_name,
std::string(prefs::kHotwordAlwaysOnSearchEnabled));
The indenting was correct before.

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:467: // TODO(kcarattini): Check if the
app is already running. If so, bring it
This TODO is valid but should be in the app itself, as that's where you'd do
this. That is, you always send onLaunched. In the app's onLaunched handler it
can detect if it has a window visible and can focus the window if it does.

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:483: } else if (pref_name ==
std::string(prefs::kHotwordAudioLoggingEnabled)){
On 2014/09/09 20:07:32, rpetterson wrote:
> Does this function (SetHotwordAudioVerificationLaunchState) get called
> elsewhere? If not, then this condition will never be met since above we DCHECK
> for pref_name == std::string(prefs::kHotwordAlwaysOnSearchEnabled.

I think it would be clearer if this function didn't exist, but the caller just
set hotword_audio_verification_launch_state to whatever it thought was
necessary. It is confusing setting this value based on a pref_name.

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.h (right):

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.h:150: // Stores a bit of state for the
Hotword Audio Verification App.
Super nit: a bit? Meaning 0 or 1?

https://codereview.chromium.org/528113003/diff/40001/extensions/browser/exten...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/528113003/diff/40001/extensions/browser/exten...
extensions/browser/extension_function_histogram_value.h:953: //
tools/metrics/histograms/histograms.xml.
Make sure you update tools/metrics/histograms/histograms.xml

[kcarattini, 2014-09-10 03:05:40]

Thank you both for your comments. As we discussed, I will rework this cl so that
the Hotword Audio Verification app uses the existing hotwordPrivate API instead
of adding a new one.

[kcarattini, 2014-09-10 08:14:29]

I've now extended the hotwordPrivate API, as we discussed. Please have another
look.

Thanks,
Kendra

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/hotword_audio_verification_private/hotword_audio_verification_private_api.cc
(right):

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/hotword_audio_verification_private/hotword_audio_verification_private_api.cc:18:
// TODO(kcarattini): Do better error handling.
On 2014/09/09 20:57:30, benwells wrote:
> Error handling is pretty easy. You just set error_ to be a string and return
> false, and the rest happens. E.g.
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

Done. I'm not sure what you mean by "the rest happens". Can the return value be
used in flow.js when this is an async function?

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/hotword_audio_verification_private/hotword_audio_verification_private_api.cc:22:
result.launch_mode =
On 2014/09/09 20:57:30, benwells wrote:
> You need {} as this is a multi-line body.

Done.

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/hotword_audio_verification_private/hotword_audio_verification_private_api.h
(right):

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/hotword_audio_verification_private/hotword_audio_verification_private_api.h:12:
#include "extensions/browser/browser_context_keyed_api_factory.h"
On 2014/09/09 20:57:30, benwells wrote:
> I don't think all (most?) of these includes are needed. Can you trim them
down?

Merged this file into hotword_private_api.h

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/hotword_audio_verification/flow.js (right):

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/flow.js:29: Flow.FLOWS_ = [
On 2014/09/09 20:57:30, benwells wrote:
> Was there a reason to put this on Flow instead of being a global const?

No, it was done while I was debugging. Moved back to a global variable.

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/flow.js:58:
Flow.prototype.getFlowForSetting_ = function(state) {
On 2014/09/09 20:57:30, benwells wrote:
> I think this function should be called startFlowForSetting, or something
similar
> that communicates the flow is going to be initiated.

Done.

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:455: DCHECK_EQ(pref_name,
std::string(prefs::kHotwordAlwaysOnSearchEnabled));
On 2014/09/09 20:57:30, benwells wrote:
> The indenting was correct before.

Done.

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:467: // TODO(kcarattini): Check if the
app is already running. If so, bring it
On 2014/09/09 20:57:31, benwells wrote:
> This TODO is valid but should be in the app itself, as that's where you'd do
> this. That is, you always send onLaunched. In the app's onLaunched handler it
> can detect if it has a window visible and can focus the window if it does.

Done.

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:483: } else if (pref_name ==
std::string(prefs::kHotwordAudioLoggingEnabled)){
On 2014/09/09 20:57:31, benwells wrote:
> On 2014/09/09 20:07:32, rpetterson wrote:
> > Does this function (SetHotwordAudioVerificationLaunchState) get called
> > elsewhere? If not, then this condition will never be met since above we
DCHECK
> > for pref_name == std::string(prefs::kHotwordAlwaysOnSearchEnabled.
> 
> I think it would be clearer if this function didn't exist, but the caller just
> set hotword_audio_verification_launch_state to whatever it thought was
> necessary. It is confusing setting this value based on a pref_name.

Refactored this. Please let me know if it makes more sense. The caller of the
launch function might be external to hotword_service.

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:483: } else if (pref_name ==
std::string(prefs::kHotwordAudioLoggingEnabled)){
On 2014/09/09 20:07:32, rpetterson wrote:
> Does this function (SetHotwordAudioVerificationLaunchState) get called
> elsewhere? If not, then this condition will never be met since above we DCHECK
> for pref_name == std::string(prefs::kHotwordAlwaysOnSearchEnabled.

Yes, it will be called when the user retrains the model, and from anywhere we
might put a promo link.

The logic I had here doesn't make sense given the latest mocks, so I am removing
all of this and adding a TODO to follow up in a separate cl.

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.h (right):

https://codereview.chromium.org/528113003/diff/40001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.h:150: // Stores a bit of state for the
Hotword Audio Verification App.
On 2014/09/09 20:57:31, benwells wrote:
> Super nit: a bit? Meaning 0 or 1?

Done.

https://codereview.chromium.org/528113003/diff/40001/extensions/browser/exten...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/528113003/diff/40001/extensions/browser/exten...
extensions/browser/extension_function_histogram_value.h:953: //
tools/metrics/histograms/histograms.xml.
On 2014/09/09 20:57:31, benwells wrote:
> Make sure you update tools/metrics/histograms/histograms.xml

Done.

[kcarattini, 2014-09-10 08:17:47]

kcarattini@chromium.org changed reviewers:
+ jhawkins@chromium.org, mpearson@chromium.org

[kcarattini, 2014-09-10 08:17:48]

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

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

[Mark P, 2014-09-10 16:21:40]

histograms.xml lgtm

[rpetterson, 2014-09-10 23:04:23]

https://codereview.chromium.org/528113003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/hotword_private/hotword_private_api.cc
(right):

https://codereview.chromium.org/528113003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/hotword_private/hotword_private_api.cc:151: error_
= hotword_private_constants::kHotwordServiceUnavailable;
This should probably be defined in resources.grd rather than above so that it
gets translated properly.

https://codereview.chromium.org/528113003/diff/60001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/528113003/diff/60001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:473: OpenApplication(AppLaunchParams(
Why don't you check if the pref is enabled any more?
OnHotwordAlwaysOnSearchEnabledChanged can be triggered for turning the setting
off as well and we wouldn't want that to pop up the app.

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

https://codereview.chromium.org/528113003/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/hotword_private.idl:37: long launchMode;
Are you planning on adding more items to this State? Maybe list them as TODOs?

[kcarattini, 2014-09-11 00:08:30]

https://codereview.chromium.org/528113003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/hotword_private/hotword_private_api.cc
(right):

https://codereview.chromium.org/528113003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/hotword_private/hotword_private_api.cc:151: error_
= hotword_private_constants::kHotwordServiceUnavailable;
On 2014/09/10 23:04:22, rpetterson wrote:
> This should probably be defined in resources.grd rather than above so that it
> gets translated properly.

Does this get exposed to the user? I thought it was an internal error message
(maybe Ben can clarify). This was how it was done in the example Ben sent to me.

https://codereview.chromium.org/528113003/diff/60001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/528113003/diff/60001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:473: OpenApplication(AppLaunchParams(
On 2014/09/10 23:04:23, rpetterson wrote:
> Why don't you check if the pref is enabled any more?
> OnHotwordAlwaysOnSearchEnabledChanged can be triggered for turning the setting
> off as well and we wouldn't want that to pop up the app.

I don't think this is the final behaviour we want at all, actually, and I plan
to fix that in a follow-up cl (added a TODO). I think what we want is to manage
the enabling of the pref, rather than react to it, by overriding the default
pref behaviour and intercepting the checkbox click before the pref is enabled
(and control the enabled state through the app).

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

https://codereview.chromium.org/528113003/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/hotword_private.idl:37: long launchMode;
On 2014/09/10 23:04:23, rpetterson wrote:
> Are you planning on adding more items to this State? Maybe list them as TODOs?

Done.

[rpetterson, 2014-09-11 17:43:54]

LGTM

https://codereview.chromium.org/528113003/diff/60001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/528113003/diff/60001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:473: OpenApplication(AppLaunchParams(
On 2014/09/11 00:08:30, kcarattini wrote:
> On 2014/09/10 23:04:23, rpetterson wrote:
> > Why don't you check if the pref is enabled any more?
> > OnHotwordAlwaysOnSearchEnabledChanged can be triggered for turning the
setting
> > off as well and we wouldn't want that to pop up the app.
> 
> I don't think this is the final behaviour we want at all, actually, and I plan
> to fix that in a follow-up cl (added a TODO). I think what we want is to
manage
> the enabling of the pref, rather than react to it, by overriding the default
> pref behaviour and intercepting the checkbox click before the pref is enabled
> (and control the enabled state through the app).

Got it. So only enable it if they've done the verification and otherwise disable
it?

[benwells, 2014-09-11 19:27:41]

lgtm

https://codereview.chromium.org/528113003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/hotword_private/hotword_private_api.cc
(right):

https://codereview.chromium.org/528113003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/hotword_private/hotword_private_api.cc:151: error_
= hotword_private_constants::kHotwordServiceUnavailable;
On 2014/09/11 00:08:29, kcarattini wrote:
> On 2014/09/10 23:04:22, rpetterson wrote:
> > This should probably be defined in resources.grd rather than above so that
it
> > gets translated properly.
> 
> Does this get exposed to the user? I thought it was an internal error message
> (maybe Ben can clarify). This was how it was done in the example Ben sent to
me.

That's basically right - these are developer facing, not user facing, and are
not translated.

[rpetterson, 2014-09-11 19:33:32]

https://codereview.chromium.org/528113003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/hotword_private/hotword_private_api.cc
(right):

https://codereview.chromium.org/528113003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/hotword_private/hotword_private_api.cc:151: error_
= hotword_private_constants::kHotwordServiceUnavailable;
On 2014/09/11 19:27:40, benwells (OOO until October 7) wrote:
> On 2014/09/11 00:08:29, kcarattini wrote:
> > On 2014/09/10 23:04:22, rpetterson wrote:
> > > This should probably be defined in resources.grd rather than above so that
> it
> > > gets translated properly.
> > 
> > Does this get exposed to the user? I thought it was an internal error
message
> > (maybe Ben can clarify). This was how it was done in the example Ben sent to
> me.
> 
> That's basically right - these are developer facing, not user facing, and are
> not translated.

Good to know, thanks!

[kcarattini, 2014-09-11 23:32:51]

James, can you please review files in chrome/browser/resources/...

https://codereview.chromium.org/528113003/diff/60001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/528113003/diff/60001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:473: OpenApplication(AppLaunchParams(
On 2014/09/11 17:43:54, rpetterson wrote:
> On 2014/09/11 00:08:30, kcarattini wrote:
> > On 2014/09/10 23:04:23, rpetterson wrote:
> > > Why don't you check if the pref is enabled any more?
> > > OnHotwordAlwaysOnSearchEnabledChanged can be triggered for turning the
> setting
> > > off as well and we wouldn't want that to pop up the app.
> > 
> > I don't think this is the final behaviour we want at all, actually, and I
plan
> > to fix that in a follow-up cl (added a TODO). I think what we want is to
> manage
> > the enabling of the pref, rather than react to it, by overriding the default
> > pref behaviour and intercepting the checkbox click before the pref is
enabled
> > (and control the enabled state through the app).
> 
> Got it. So only enable it if they've done the verification and otherwise
disable
> it?

Yes, that's the idea.

[James Hawkins, 2014-09-12 20:11:56]

https://codereview.chromium.org/528113003/diff/120001/chrome/browser/resource...
File chrome/browser/resources/hotword_audio_verification/flow.js (right):

https://codereview.chromium.org/528113003/diff/120001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/flow.js:49: if
(chrome.hotwordPrivate && chrome.hotwordPrivate.getLaunchState) {
nit: No braces for single line blocks.

https://codereview.chromium.org/528113003/diff/120001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/flow.js:61: if
(state.launchMode >= 0 && state.launchMode < FLOWS.length) {
Shouldn't we assert these conditions are true?  When are they expected to be
untrue?

[kcarattini, 2014-09-23 03:01:49]

Thanks, please have another look.

Kendra

https://codereview.chromium.org/528113003/diff/120001/chrome/browser/resource...
File chrome/browser/resources/hotword_audio_verification/flow.js (right):

https://codereview.chromium.org/528113003/diff/120001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/flow.js:49: if
(chrome.hotwordPrivate && chrome.hotwordPrivate.getLaunchState) {
On 2014/09/12 20:11:56, James Hawkins wrote:
> nit: No braces for single line blocks.

Done.

https://codereview.chromium.org/528113003/diff/120001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/flow.js:61: if
(state.launchMode >= 0 && state.launchMode < FLOWS.length) {
On 2014/09/12 20:11:56, James Hawkins wrote:
> Shouldn't we assert these conditions are true?  When are they expected to be
> untrue?

Done.

[chromium-reviews, 2014-09-24 23:44:24]

Ping to jhawkins@

Kendra

On Tue, Sep 23, 2014 at 1:01 PM, <kcarattini@chromium.org> wrote:

> Thanks, please have another look.
>
> Kendra
>
>
> https://codereview.chromium.org/528113003/diff/120001/
> chrome/browser/resources/hotword_audio_verification/flow.js
> File chrome/browser/resources/hotword_audio_verification/flow.js
> (right):
>
> https://codereview.chromium.org/528113003/diff/120001/
> chrome/browser/resources/hotword_audio_verification/flow.js#newcode49
> chrome/browser/resources/hotword_audio_verification/flow.js:49: if
> (chrome.hotwordPrivate && chrome.hotwordPrivate.getLaunchState) {
> On 2014/09/12 20:11:56, James Hawkins wrote:
>
>> nit: No braces for single line blocks.
>>
>
> Done.
>
> https://codereview.chromium.org/528113003/diff/120001/
> chrome/browser/resources/hotword_audio_verification/flow.js#newcode61
> chrome/browser/resources/hotword_audio_verification/flow.js:61: if
> (state.launchMode >= 0 && state.launchMode < FLOWS.length) {
> On 2014/09/12 20:11:56, James Hawkins wrote:
>
>> Shouldn't we assert these conditions are true?  When are they expected
>>
> to be
>
>> untrue?
>>
>
> Done.
>
> https://codereview.chromium.org/528113003/
>

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

[James Hawkins, 2014-09-25 00:53:31]

lgtm

[kcarattini, 2014-09-25 03:07:10]

The CQ bit was checked by kcarattini@chromium.org

[commit-bot: I haz the power, 2014-09-25 03:07:50]

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

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

Committed patchset #10 (id:180001) as f71d68c28a207229d22f670b4f321286812dda4c

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

Patchset 10 (id:??) landed as
https://crrev.com/558d71f8a7a13239f1af39a5f69161d364dba5e0
Cr-Commit-Position: refs/heads/master@{#296641}