[Hotword] Ok Google options disappear if search provider is changed from

[Hotword] Ok Google options disappear if search provider is changed from
Google to other.

This also resets the pref to off if they go away from Google.

BUG=437889
Committed: https://crrev.com/a8649e0005039bc3c35db388ca62f4a774a9a914
Cr-Commit-Position: refs/heads/master@{#307041}

[rpetterson, 2014-12-04 19:51:23]

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

[rpetterson, 2014-12-04 19:51:23]

dbeam: browser_options*

amistry: hotword_service*

[Dan Beam, 2014-12-04 20:44:07]

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

https://codereview.chromium.org/780703004/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1203: hideHotwordSection_:
function() {
nit:

  setHotwordSectionVisible: function(visible) {

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

https://codereview.chromium.org/780703004/diff/20001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:748: if
(profile_->GetPrefs()->HasPrefPath(prefs::kHotwordSearchEnabled))
why do you need to check HasPrefPath?

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

https://codereview.chromium.org/780703004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1209:
template_url_service_->search_terms_data())) {
looks confusingly like it's inside the conditional, add more spaces

https://codereview.chromium.org/780703004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1212:
HandleRequestHotwordAvailable(nullptr);
how does the hotword section get unhidden?

https://codereview.chromium.org/780703004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1218:
HotwordServiceFactory::GetForProfile(Profile::FromWebUI(web_ui()));
4\s indent on continuations

https://codereview.chromium.org/780703004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1219: if
(hotword_service)
how often is |hotword_service| null here?

[rpetterson, 2014-12-04 23:20:29]

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

https://codereview.chromium.org/780703004/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1203: hideHotwordSection_:
function() {
On 2014/12/04 20:44:07, Dan Beam wrote:
> nit:
> 
>   setHotwordSectionVisible: function(visible) {

Done.

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

https://codereview.chromium.org/780703004/diff/20001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:748: if
(profile_->GetPrefs()->HasPrefPath(prefs::kHotwordSearchEnabled))
On 2014/12/04 20:44:07, Dan Beam wrote:
> why do you need to check HasPrefPath?

We have some special cases for if the user has not set the preference (e.g.,
promos) so if the preference doesn't currently exist, we'd like to maintain that
state.

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

https://codereview.chromium.org/780703004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1209:
template_url_service_->search_terms_data())) {
On 2014/12/04 20:44:07, Dan Beam wrote:
> looks confusingly like it's inside the conditional, add more spaces

Done.

https://codereview.chromium.org/780703004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1212:
HandleRequestHotwordAvailable(nullptr);
On 2014/12/04 20:44:07, Dan Beam wrote:
> how does the hotword section get unhidden?

That function -- HandleRequestHotwordAvailable -- deals with the checking based
on flags and preference values to determine which hotword section is shown.
Ultimately it calls one of BrowserOptions.showHotwordAlwaysOnSection,
BrowserOptions.showHotwordNoDspSection or BrowserOptions.showHotwordSection (as
well as BrowserOptions.showAudioHistorySection if applicable). The args
parameter is not used which is why I am passing nullptr.

https://codereview.chromium.org/780703004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1218:
HotwordServiceFactory::GetForProfile(Profile::FromWebUI(web_ui()));
On 2014/12/04 20:44:07, Dan Beam wrote:
> 4\s indent on continuations

Argh. Thank you. I have to figure out a way to get VS to do that automatically .
. . :-/

https://codereview.chromium.org/780703004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1219: if
(hotword_service)
On 2014/12/04 20:44:07, Dan Beam wrote:
> how often is |hotword_service| null here?

Mostly in some tests but also any platform/situation that doesn't enable
extensions (Android?). It should never be null in practice for desktop or CrOS.

[Dan Beam, 2014-12-04 23:33:07]

lgtm though I originally had you change from hide to setVisible so this code
could be re-used (up to you if it's only ever called with false to hide...)

[Anand Mistry (off Chromium), 2014-12-05 03:56:51]

LGTM as-is, but I'd like to hear your thoughts about the google.com case.

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

https://codereview.chromium.org/780703004/diff/60001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:747: if (IsSometimesOnEnabled()) {
This will also disable hotwording if the user navigates to google.com. Is this
really what we want? From the user's perspective, it's understandable that
hotwording isn't going to work on non-google searches, but if they explicitly go
to google.com, why wouldn't that work?

[rpetterson, 2014-12-05 04:57:16]

On 2014/12/04 23:33:07, Dan Beam wrote:
> lgtm though I originally had you change from hide to setVisible so this code
> could be re-used (up to you if it's only ever called with false to hide...)

I'll leave it as is since it is more useful in this state.

[rpetterson, 2014-12-05 04:57:33]

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

https://codereview.chromium.org/780703004/diff/60001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:747: if (IsSometimesOnEnabled()) {
On 2014/12/05 03:56:51, Anand Mistry wrote:
> This will also disable hotwording if the user navigates to http://google.com.
Is this
> really what we want? From the user's perspective, it's understandable that
> hotwording isn't going to work on non-google searches, but if they explicitly
go
> to http://google.com, why wouldn't that work?

So if I understand correctly, you're saying only make it disappear for
always-on, but not sometimes on? I think that's reasonable as well.

+somas so he can chime in

[Anand Mistry (off Chromium), 2014-12-05 05:23:52]

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

https://codereview.chromium.org/780703004/diff/60001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:747: if (IsSometimesOnEnabled()) {
On 2014/12/05 04:57:33, rpetterson wrote:
> On 2014/12/05 03:56:51, Anand Mistry wrote:
> > This will also disable hotwording if the user navigates to
http://google.com.
> Is this
> > really what we want? From the user's perspective, it's understandable that
> > hotwording isn't going to work on non-google searches, but if they
explicitly
> go
> > to http://google.com, why wouldn't that work?
> 
> So if I understand correctly, you're saying only make it disappear for
> always-on, but not sometimes on? I think that's reasonable as well.
> 
> +somas so he can chime in

I think it's more nuanced than that. I think it should be disabled for the
launcher search, which means always-on and the launcher integration with
sometimes-on. NTP doesn't matter since you don't get an in-page search box. But
a user can visit google.com.

But now this is getting complicated, so disabling everything is a good
compromise (hence the lgtm). I'm just asking what the UX should be in an ideal
world based on reasonable user expectations.

[rpetterson, 2014-12-05 05:29:10]

On 2014/12/05 05:23:52, Anand Mistry wrote:
>
https://codereview.chromium.org/780703004/diff/60001/chrome/browser/search/ho...
> File chrome/browser/search/hotword_service.cc (right):
> 
>
https://codereview.chromium.org/780703004/diff/60001/chrome/browser/search/ho...
> chrome/browser/search/hotword_service.cc:747: if (IsSometimesOnEnabled()) {
> On 2014/12/05 04:57:33, rpetterson wrote:
> > On 2014/12/05 03:56:51, Anand Mistry wrote:
> > > This will also disable hotwording if the user navigates to
> http://google.com.
> > Is this
> > > really what we want? From the user's perspective, it's understandable that
> > > hotwording isn't going to work on non-google searches, but if they
> explicitly
> > go
> > > to http://google.com, why wouldn't that work?
> > 
> > So if I understand correctly, you're saying only make it disappear for
> > always-on, but not sometimes on? I think that's reasonable as well.
> > 
> > +somas so he can chime in
> 
> I think it's more nuanced than that. I think it should be disabled for the
> launcher search, which means always-on and the launcher integration with
> sometimes-on. NTP doesn't matter since you don't get an in-page search box.
But
> a user can visit http://google.com.
> 
> But now this is getting complicated, so disabling everything is a good
> compromise (hence the lgtm). I'm just asking what the UX should be in an ideal
> world based on reasonable user expectations.

Agreed. Then we're looking at changing which extension they're using which is
far more complicated. I think I'll get this checked in and we can alter it in
the future.

[rpetterson, 2014-12-05 17:02:47]

The CQ bit was checked by rlp@chromium.org

[commit-bot: I haz the power, 2014-12-05 17:03:24]

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

[commit-bot: I haz the power, 2014-12-05 17:54:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-12-05 17:55:11]

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