Fine tuned availability of hotword plugin.

Fine tuned availability of hotword plugin.

This CL adds several changes:
- enables the JS handler to turn on and off the hotword recognizer
- adds a pref 'hotword.app_list_enabled' to customize the plugin usage
- synchronization logic, so if the user explicitly turns off
  hotword.search_enabled, hotword.app_list_enabled goes off too
- extract the language check of HotwordService as a static method
  and let StartPageHandler check it, because our hotword
  "Ok, Google" is US-English only

BUG=341710, 341779
R=xiyuan@chromium.org, rlp@chromium.org, estade@chromium.org
TBR=samarth@chromium.org
TEST=manually

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251142

[xiyuan, 2014-02-08 04:21:27]

LGTM

[rpetterson, 2014-02-09 18:27:18]

https://codereview.chromium.org/158143002/diff/30001/chrome/browser/profiles/...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(right):

https://codereview.chromium.org/158143002/diff/30001/chrome/browser/profiles/...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:339: #if
defined(ENABLE_APP_LIST)
Services should be listed here alphabetically so this should go near the top.

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

https://codereview.chromium.org/158143002/diff/30001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:92: return
DoesLanguageSupportHotword(profile_);
The if statement is no longer necessary. Just

return !group.empty() && group !=
hotword_internal::kHotwordFieldTrialDisabledGroupName &&
DoesLanguageSupportHotword(profile_);

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

https://codereview.chromium.org/158143002/diff/30001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.h:24: static bool
DoesLanguageSupportHotword(Profile* profile);
Please rename to DoesHotwordSupportLanguage

[Jun Mukai, 2014-02-10 20:50:37]

https://codereview.chromium.org/158143002/diff/30001/chrome/browser/profiles/...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(right):

https://codereview.chromium.org/158143002/diff/30001/chrome/browser/profiles/...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:339: #if
defined(ENABLE_APP_LIST)
On 2014/02/09 18:27:19, rpetterson wrote:
> Services should be listed here alphabetically so this should go near the top.

Done.

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

https://codereview.chromium.org/158143002/diff/30001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:92: return
DoesLanguageSupportHotword(profile_);
On 2014/02/09 18:27:19, rpetterson wrote:
> The if statement is no longer necessary. Just
> 
> return !group.empty() && group !=
> hotword_internal::kHotwordFieldTrialDisabledGroupName &&
> DoesLanguageSupportHotword(profile_);

Done.

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

https://codereview.chromium.org/158143002/diff/30001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.h:24: static bool
DoesLanguageSupportHotword(Profile* profile);
On 2014/02/09 18:27:19, rpetterson wrote:
> Please rename to DoesHotwordSupportLanguage

Done.

[rpetterson, 2014-02-10 21:03:45]

lgtm

[Jun Mukai, 2014-02-10 21:12:26]

estade, could you review changes on c/b/resources/options and
c/b/ui/webui/options?

[Jun Mukai, 2014-02-11 18:26:44]

On 2014/02/10 21:12:26, Jun Mukai wrote:
> estade, could you review changes on c/b/resources/options and
> c/b/ui/webui/options?

Evan, do you have any chance to see this?

[Evan Stade, 2014-02-11 18:54:11]

https://codereview.chromium.org/158143002/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/158143002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:361: <label
for="hotword-app-list-enable"
even though there are lots of crappy examples in this file of label outside of
an input, you should actually put the label aroudn the input and span (and then
you don't need for). The problem with outside label + for is that you can click
in between the checkbox and text and nothing happens.

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

https://codereview.chromium.org/158143002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:880:
web_ui()->CallJavascriptFunction("BrowserOptions.showAppListHotword");
you should do this in GetLocalizedValues (like the sync setup) and not call a JS
function, but set some variable for the page to read. This way it doesn't jiggle
after resources load but before page init.

[Jun Mukai, 2014-02-11 19:27:37]

https://codereview.chromium.org/158143002/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/158143002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:361: <label
for="hotword-app-list-enable"
On 2014/02/11 18:54:12, Evan Stade wrote:
> even though there are lots of crappy examples in this file of label outside of
> an input, you should actually put the label aroudn the input and span (and
then
> you don't need for). The problem with outside label + for is that you can
click
> in between the checkbox and text and nothing happens.

Done.

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

https://codereview.chromium.org/158143002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:880:
web_ui()->CallJavascriptFunction("BrowserOptions.showAppListHotword");
On 2014/02/11 18:54:12, Evan Stade wrote:
> you should do this in GetLocalizedValues (like the sync setup) and not call a
JS
> function, but set some variable for the page to read. This way it doesn't
jiggle
> after resources load but before page init.

Done.

[Jun Mukai, 2014-02-12 17:43:07]

Evan, I think I've fixed the code. Anything else I should fix?

[Evan Stade, 2014-02-12 23:01:48]

https://codereview.chromium.org/158143002/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/158143002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:361: <label
for="hotword-app-list-enable"
On 2014/02/11 19:27:38, Jun Mukai wrote:
> On 2014/02/11 18:54:12, Evan Stade wrote:
> > even though there are lots of crappy examples in this file of label outside
of
> > an input, you should actually put the label aroudn the input and span (and
> then
> > you don't need for). The problem with outside label + for is that you can
> click
> > in between the checkbox and text and nothing happens.
> 
> Done.

this is not done.  It should look like this:

<label><input><span></span></label>

https://codereview.chromium.org/158143002/diff/330001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/158143002/diff/330001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:234:
!loadTimeData.getBoolean('appListHotwordShown');
call this shouldShowAppListHotword

[Jun Mukai, 2014-02-13 00:08:28]

https://codereview.chromium.org/158143002/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/158143002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:361: <label
for="hotword-app-list-enable"
On 2014/02/12 23:01:49, Evan Stade wrote:
> On 2014/02/11 19:27:38, Jun Mukai wrote:
> > On 2014/02/11 18:54:12, Evan Stade wrote:
> > > even though there are lots of crappy examples in this file of label
outside
> of
> > > an input, you should actually put the label aroudn the input and span (and
> > then
> > > you don't need for). The problem with outside label + for is that you can
> > click
> > > in between the checkbox and text and nothing happens.
> > 
> > Done.
> 
> this is not done.  It should look like this:
> 
> <label><input><span></span></label>

Fixed.  Sorry for my poor understanding.

https://codereview.chromium.org/158143002/diff/330001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/158143002/diff/330001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:234:
!loadTimeData.getBoolean('appListHotwordShown');
On 2014/02/12 23:01:49, Evan Stade wrote:
> call this shouldShowAppListHotword

Done.

[Evan Stade, 2014-02-13 00:15:43]

webui lgtm

[Jun Mukai, 2014-02-13 00:35:22]

TBR=samarth for chrome/browser/search/hotword_service*. Already got lgtm from
rlp.

[Jun Mukai, 2014-02-13 00:35:43]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-02-13 00:38:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/158143002/650001

[commit-bot: I haz the power, 2014-02-13 02:21:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 02:21:51]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, browser_tests, cacheinvalidation_unittests,
cc_unittests, check_deps, check_deps2git, chrome_elf_unittests,
chromedriver_unittests, components_unittests, compositor_unittests,
content_browsertests, content_unittests, crypto_unittests, events_unittests,
google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests,
jingle_unittests, media_unittests, mini_installer_test, nacl_integration,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Jun Mukai, 2014-02-13 18:34:12]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-02-13 18:35:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/158143002/650001

[commit-bot: I haz the power, 2014-02-13 19:58:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/158143002/650001

[commit-bot: I haz the power, 2014-02-13 21:58:29]

Change committed as 251142