Add error indicators to always-on andno-dsp hotword checkboxes.

Add error indicators to always-on andno-dsp hotword checkboxes.

Also shows the indicator regardless of pref status.
(For always-on, the opt-in process requires that there
are no errors, so if something goes wrong it is better that the user know why.)

Screenshot in attached bug.

BUG=390086
Committed: https://crrev.com/96d45c708e9cc6f6f97c1b1cba395b5fc59728c6
Cr-Commit-Position: refs/heads/master@{#302710}

[kcarattini, 2014-11-03 03:05:34]

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

[kcarattini, 2014-11-03 03:05:53]

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

[rpetterson, 2014-11-03 18:14:16]

overall lgtm with some nits

https://codereview.chromium.org/693393004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/hotword_search_setting_indicator.css
(right):

https://codereview.chromium.org/693393004/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/hotword_search_setting_indicator.css:11:
#hotword-always-on-search-setting-indicator > div {
wondering if there's a way to combine these three with css magic. Dan might have
an idea.

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

https://codereview.chromium.org/693393004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1630: std::string
fcn_name;
what does "fcn" stand for? maybe a name that makes it clearer what it is?

[kcarattini, 2014-11-03 23:39:24]

https://codereview.chromium.org/693393004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/hotword_search_setting_indicator.css
(right):

https://codereview.chromium.org/693393004/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/hotword_search_setting_indicator.css:11:
#hotword-always-on-search-setting-indicator > div {
On 2014/11/03 18:14:16, rpetterson wrote:
> wondering if there's a way to combine these three with css magic. Dan might
have
> an idea.

Yeah, using a class gets overridden by the controlled-setting-indicator, I
think.

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

https://codereview.chromium.org/693393004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1630: std::string
fcn_name;
On 2014/11/03 18:14:16, rpetterson wrote:
> what does "fcn" stand for? maybe a name that makes it clearer what it is?

Done.

[Dan Beam, 2014-11-04 01:59:25]

https://codereview.chromium.org/693393004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/hotword_search_setting_indicator.css
(right):

https://codereview.chromium.org/693393004/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/hotword_search_setting_indicator.css:11:
#hotword-always-on-search-setting-indicator > div {
On 2014/11/03 23:39:24, kcarattini wrote:
> On 2014/11/03 18:14:16, rpetterson wrote:
> > wondering if there's a way to combine these three with css magic. Dan might
> have
> > an idea.
> 
> Yeah, using a class gets overridden by the controlled-setting-indicator, I
> think.

read up on CSS specificity:
https://developer.mozilla.org/en-US/docs/Web/CSS/Specificity

if a class simply wont do it for you, here's an alternative:

#hotword-search-setting-indicator > div,
#hotword-always-on-search-setting-indicator > div,
#hotword-no-dsp-search-setting-indicator > div {
  background: url(chrome://theme/IDR_WARNING) left top no-repeat;
  background-size: 18px;
}

[Dan Beam, 2014-11-04 02:03:46]

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

https://codereview.chromium.org/693393004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1134: * @param {string}
section_id The id of the section to display.
jsVarsLikeThis, opt_varsIsSillyButPartOfTheStyleGuide

https://codereview.chromium.org/693393004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1136: * @param {string}
opt_error The error message to display.
this should only be opt_ if the type is string= (i.e. the argument could be
undefined).

if it can be undefined, change to

  @param {string=} opt_error

if it's not, change to

  @param {string} error

and do the same for showHotwordSection_ and showHotwordAlwaysOnSection_ and
showHotwordNoDspSection_

https://codereview.chromium.org/693393004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1149: * @param {string}
opt_error The error message to display.
string=

https://codereview.chromium.org/693393004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1153:
this.showHotwordCheckboxAndIndicator_('hotword-search',
nit: wrap first arg, e.g.

  this.showHotwordCheckboxAndIndicator_(
      'hotword-search',
      'hotword-search-setting-indicator',
      opt_error);

https://codereview.chromium.org/693393004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1177:
showHotwordNoDSPSection_: function(opt_error) {
nit: showHotwordNoDspSection_ (e.g. HttpUrl instead of HTTPURL)

[kcarattini, 2014-11-04 06:01:21]

https://codereview.chromium.org/693393004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/hotword_search_setting_indicator.css
(right):

https://codereview.chromium.org/693393004/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/hotword_search_setting_indicator.css:11:
#hotword-always-on-search-setting-indicator > div {
On 2014/11/04 01:59:24, Dan Beam wrote:
> On 2014/11/03 23:39:24, kcarattini wrote:
> > On 2014/11/03 18:14:16, rpetterson wrote:
> > > wondering if there's a way to combine these three with css magic. Dan
might
> > have
> > > an idea.
> > 
> > Yeah, using a class gets overridden by the controlled-setting-indicator, I
> > think.
> 
> read up on CSS specificity:
> https://developer.mozilla.org/en-US/docs/Web/CSS/Specificity
> 
> if a class simply wont do it for you, here's an alternative:
> 
> #hotword-search-setting-indicator > div,
> #hotword-always-on-search-setting-indicator > div,
> #hotword-no-dsp-search-setting-indicator > div {
>   background: url(chrome://theme/IDR_WARNING) left top no-repeat;
>   background-size: 18px;
> }

Done.

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

https://codereview.chromium.org/693393004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1134: * @param {string}
section_id The id of the section to display.
On 2014/11/04 02:03:46, Dan Beam wrote:
> jsVarsLikeThis, opt_varsIsSillyButPartOfTheStyleGuide

Done.

https://codereview.chromium.org/693393004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1136: * @param {string}
opt_error The error message to display.
On 2014/11/04 02:03:45, Dan Beam wrote:
> this should only be opt_ if the type is string= (i.e. the argument could be
> undefined).
> 
> if it can be undefined, change to
> 
>   @param {string=} opt_error
> 
> if it's not, change to
> 
>   @param {string} error
> 
> and do the same for showHotwordSection_ and showHotwordAlwaysOnSection_ and
> showHotwordNoDspSection_

Done.

https://codereview.chromium.org/693393004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1149: * @param {string}
opt_error The error message to display.
On 2014/11/04 02:03:46, Dan Beam wrote:
> string=

Done.

https://codereview.chromium.org/693393004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1153:
this.showHotwordCheckboxAndIndicator_('hotword-search',
On 2014/11/04 02:03:46, Dan Beam wrote:
> nit: wrap first arg, e.g.
> 
>   this.showHotwordCheckboxAndIndicator_(
>       'hotword-search',
>       'hotword-search-setting-indicator',
>       opt_error);

Done.

https://codereview.chromium.org/693393004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1177:
showHotwordNoDSPSection_: function(opt_error) {
On 2014/11/04 02:03:46, Dan Beam wrote:
> nit: showHotwordNoDspSection_ (e.g. HttpUrl instead of HTTPURL)

Done.

[Dan Beam, 2014-11-04 18:24:28]

lgtm

https://codereview.chromium.org/693393004/diff/80001/chrome/browser/resources...
File chrome/browser/resources/options/hotword_search_setting_indicator.css
(right):

https://codereview.chromium.org/693393004/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/hotword_search_setting_indicator.css:10:
background: url('chrome://theme/IDR_WARNING') left top no-repeat;
nit: url(chrome://theme/IDR_WARNING) (no quotes required)

[kcarattini, 2014-11-04 22:43:34]

https://codereview.chromium.org/693393004/diff/80001/chrome/browser/resources...
File chrome/browser/resources/options/hotword_search_setting_indicator.css
(right):

https://codereview.chromium.org/693393004/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/hotword_search_setting_indicator.css:10:
background: url('chrome://theme/IDR_WARNING') left top no-repeat;
On 2014/11/04 18:24:27, Dan Beam wrote:
> nit: url(chrome://theme/IDR_WARNING) (no quotes required)

Done.

[kcarattini, 2014-11-04 22:43:44]

The CQ bit was checked by kcarattini@chromium.org

[commit-bot: I haz the power, 2014-11-04 22:46:13]

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

[commit-bot: I haz the power, 2014-11-04 23:45:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-04 23:46:03]

Patchset 6 (id:??) landed as
https://crrev.com/96d45c708e9cc6f6f97c1b1cba395b5fc59728c6
Cr-Commit-Position: refs/heads/master@{#302710}