Hotword: Decouple sometimes and always on prefs when disabling. They can both exist independently.

Hotword: Decouple sometimes and always on prefs when disabling. They can both exist independently.

BUG=

Committed: https://crrev.com/eead91c09a21092975619b0c8363d2c604744804
Cr-Commit-Position: refs/heads/master@{#320896}

[kcarattini, 2015-03-17 02:31:10]

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

[Anand Mistry (off Chromium), 2015-03-17 03:05:40]

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

https://codereview.chromium.org/1011963002/diff/1/chrome/browser/search/hotwo...
chrome/browser/search/hotword_service.cc:790: if
(profile_->GetPrefs()->HasPrefPath(prefs::kHotwordSearchEnabled))
Get rid of these two HasPrefPath checks. IsFooEnabled() implies the Foo
preference exists and is set to true. No point in checking for existence again.

[kcarattini, 2015-03-17 04:07:39]

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

https://codereview.chromium.org/1011963002/diff/1/chrome/browser/search/hotwo...
chrome/browser/search/hotword_service.cc:790: if
(profile_->GetPrefs()->HasPrefPath(prefs::kHotwordSearchEnabled))
On 2015/03/17 03:05:40, Anand Mistry wrote:
> Get rid of these two HasPrefPath checks. IsFooEnabled() implies the Foo
> preference exists and is set to true. No point in checking for existence
again.

Done.

[Anand Mistry (off Chromium), 2015-03-17 04:10:39]

lgtm

[kcarattini, 2015-03-17 04:12:31]

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

[kcarattini, 2015-03-17 04:12:32]

thestig@, please review for OWNERS approval.

[Lei Zhang, 2015-03-17 04:55:11]

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

https://codereview.chromium.org/1011963002/diff/20001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:788: void
HotwordService::DisableHotwordPreferences() {
Isn't this the same as:

if (IsAlwaysOnEnabled()) {
  ...
  return;
}
if (IsSometimesOnEnabled()) {
  ...
}

[kcarattini, 2015-03-17 05:07:40]

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

https://codereview.chromium.org/1011963002/diff/20001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:788: void
HotwordService::DisableHotwordPreferences() {
On 2015/03/17 04:55:11, Lei Zhang wrote:
> Isn't this the same as:
> 
> if (IsAlwaysOnEnabled()) {
>   ...
>   return;
> }
> if (IsSometimesOnEnabled()) {
>   ...
> }

Well, the whole point of decoupling was to make them more independent. So both
if statements should be executed regardless of the outcome of the other.

[Lei Zhang, 2015-03-17 05:37:13]

lgtm

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

https://codereview.chromium.org/1011963002/diff/20001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:788: void
HotwordService::DisableHotwordPreferences() {
On 2015/03/17 05:07:40, kcarattini wrote:
> On 2015/03/17 04:55:11, Lei Zhang wrote:
> > Isn't this the same as:
> > 
> > if (IsAlwaysOnEnabled()) {
> >   ...
> >   return;
> > }
> > if (IsSometimesOnEnabled()) {
> >   ...
> > }
> 
> Well, the whole point of decoupling was to make them more independent. So both
> if statements should be executed regardless of the outcome of the other.

Oh, nevermind. I read the CL wrong.

[kcarattini, 2015-03-17 09:18:21]

The CQ bit was checked by kcarattini@chromium.org

[commit-bot: I haz the power, 2015-03-17 09:18:36]

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

[commit-bot: I haz the power, 2015-03-17 11:36:34]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-17 11:37:18]

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