Hotword: Check for a speaker model before launching the opt-in flow

Hotword: Check for a speaker model before launching the opt-in flow

This is the first part of a multi-cl feature. I'll add the api functions in a followup cl to avoid having huge cls.

BUG=438525
Committed: https://crrev.com/b9f0bed7d0ea7b22848661fb2c8dd5dd266166b7
Cr-Commit-Position: refs/heads/master@{#321924}

[kcarattini, 2015-03-19 02:50:49]

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

[kcarattini, 2015-03-19 02:50:49]

jhawkins@ please review
chrome/browser/ui/webui/options/browser_options_handler.cc

[James Hawkins, 2015-03-19 16:07:48]

On 2015/03/19 02:50:49, kcarattini wrote:
> jhawkins@ please review
> chrome/browser/ui/webui/options/browser_options_handler.cc

Can you add me to the bug?  I can't access it.

[kcarattini, 2015-03-19 22:40:54]

On 2015/03/19 16:07:48, James Hawkins wrote:
> On 2015/03/19 02:50:49, kcarattini wrote:
> > jhawkins@ please review
> > chrome/browser/ui/webui/options/browser_options_handler.cc
> 
> Can you add me to the bug?  I can't access it.

You should be cced on the bug now.

[James Hawkins, 2015-03-19 22:57:07]

https://codereview.chromium.org/1020473003/diff/1/chrome/browser/search/hotwo...
File chrome/browser/search/hotword_service.h (right):

https://codereview.chromium.org/1020473003/diff/1/chrome/browser/search/hotwo...
chrome/browser/search/hotword_service.h:151: void
MaybeLaunchHotwordAudioVerificationApp(const LaunchMode& launch_mode);
I highly suggest not naming methods MaybeBlah.  It's incredibly confusing, as
it's not clear under what circumstances the method will do Blah without reading
through the method implementation.

It's even more confusing given that there's already a method
LaunchHotwordAudioVerificationApp.

The solution to this issue is to move the logic higher up.

[Anand Mistry (off Chromium), 2015-03-20 01:37:55]

https://codereview.chromium.org/1020473003/diff/1/chrome/browser/search/hotwo...
File chrome/browser/search/hotword_service.h (right):

https://codereview.chromium.org/1020473003/diff/1/chrome/browser/search/hotwo...
chrome/browser/search/hotword_service.h:151: void
MaybeLaunchHotwordAudioVerificationApp(const LaunchMode& launch_mode);
On 2015/03/19 22:57:07, James Hawkins wrote:
> I highly suggest not naming methods MaybeBlah.  It's incredibly confusing, as
> it's not clear under what circumstances the method will do Blah without
reading
> through the method implementation.
> 
> It's even more confusing given that there's already a method
> LaunchHotwordAudioVerificationApp.
> 
> The solution to this issue is to move the logic higher up.

Another option might be to name the function after the action that it's supposed
to handle. Like UserTriesToTurnOnAlwaysOnHotwording(), but not in so many words.

[kcarattini, 2015-03-20 04:45:01]

https://codereview.chromium.org/1020473003/diff/1/chrome/browser/search/hotwo...
File chrome/browser/search/hotword_service.h (right):

https://codereview.chromium.org/1020473003/diff/1/chrome/browser/search/hotwo...
chrome/browser/search/hotword_service.h:151: void
MaybeLaunchHotwordAudioVerificationApp(const LaunchMode& launch_mode);
On 2015/03/20 01:37:55, Anand Mistry wrote:
> On 2015/03/19 22:57:07, James Hawkins wrote:
> > I highly suggest not naming methods MaybeBlah.  It's incredibly confusing,
as
> > it's not clear under what circumstances the method will do Blah without
> reading
> > through the method implementation.
> > 
> > It's even more confusing given that there's already a method
> > LaunchHotwordAudioVerificationApp.
> > 
> > The solution to this issue is to move the logic higher up.
> 
> Another option might be to name the function after the action that it's
supposed
> to handle. Like UserTriesToTurnOnAlwaysOnHotwording(), but not in so many
words.

Renamed to OptIntoHotwording

[James Hawkins, 2015-03-20 16:21:08]

lgtm

[Anand Mistry (off Chromium), 2015-03-23 03:12:24]

https://codereview.chromium.org/1020473003/diff/20001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.h (right):

https://codereview.chromium.org/1020473003/diff/20001/chrome/browser/search/h...
chrome/browser/search/hotword_service.h:151: void OptIntoHotwording(const
LaunchMode& launch_mode);
Why is LaunchMode passed as a reference when it's an enum value?

[James Hawkins, 2015-03-23 14:53:40]

https://codereview.chromium.org/1020473003/diff/20001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.h (right):

https://codereview.chromium.org/1020473003/diff/20001/chrome/browser/search/h...
chrome/browser/search/hotword_service.h:151: void OptIntoHotwording(const
LaunchMode& launch_mode);
On 2015/03/23 03:12:24, Anand Mistry wrote:
> Why is LaunchMode passed as a reference when it's an enum value?

It's safe in that we don't make the assumption that the enum value is of a
simple (small) type.

[Anand Mistry (off Chromium), 2015-03-23 23:31:51]

lgtm

https://codereview.chromium.org/1020473003/diff/20001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.h (right):

https://codereview.chromium.org/1020473003/diff/20001/chrome/browser/search/h...
chrome/browser/search/hotword_service.h:151: void OptIntoHotwording(const
LaunchMode& launch_mode);
On 2015/03/23 14:53:40, James Hawkins wrote:
> On 2015/03/23 03:12:24, Anand Mistry wrote:
> > Why is LaunchMode passed as a reference when it's an enum value?
> 
> It's safe in that we don't make the assumption that the enum value is of a
> simple (small) type.

C++ defines the underlying type of an enum to be int, unless you specify a
different one, which C++11 allows. To me, passing an enum as a reference seems
completely wrong. Possible and perfectly valid, but feels very wrong.

But now I'm just arguing. So Kendra, feel free to ignore my ranting.

[kcarattini, 2015-03-24 02:16:02]

The CQ bit was checked by kcarattini@chromium.org

[commit-bot: I haz the power, 2015-03-24 02:16:15]

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

[commit-bot: I haz the power, 2015-03-24 03:06:35]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-24 03:07:41]

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