[Hotword] Putting preferences under search for hotword service. Putting behind a flag.

[Hotword] Putting preferences under search for hotword service. Putting behind a flag.

In order to test/see the new settings run chrome with "--force-fieldtrials=VoiceTrigger/abc/

BUG=325439, 289023
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246500

[rpetterson, 2014-01-14 00:20:59]

samarth@chromium.org: Please review changes in search -- and general hotword
consistency

jhawkins@chromium.org: Please review changes in resources/, webui/, chrome/

Also, please let me know if there are any tests that are typically added for
these sorts of preferences.

Thanks!

[James Hawkins, 2014-01-14 00:53:49]

https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.css (right):

https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.css:160: margin-left: 22px;
RTL

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

https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:131:
chrome.send('requestHotwordAvailabile');
It's not clear how this call is tied to the added listener a line below.  For
example, what happens if (hypothetically) this browser call completes
synchronously?

https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:882: * the UI for updating
hotword settings with animation.
Do we need the 'with animation' part (in the comment and the method name)?  It
seems like an implementation details.

https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:888: if (event.value.value)
Can you document what event.value.value represents?

[rpetterson, 2014-01-14 21:54:17]

https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.css (right):

https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.css:160: margin-left: 22px;
On 2014/01/14 00:53:50, James Hawkins wrote:
> RTL

What's the best way to handle this? Just "margin"?

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

https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:131:
chrome.send('requestHotwordAvailabile');
On 2014/01/14 00:53:50, James Hawkins wrote:
> It's not clear how this call is tied to the added listener a line below.  For
> example, what happens if (hypothetically) this browser call completes
> synchronously?

Good point. I've swapped the order of them. Now, when the
"requestHotwordAvailable" message is sent, it should already be listening for
the reply.

https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:882: * the UI for updating
hotword settings with animation.
On 2014/01/14 00:53:50, James Hawkins wrote:
> Do we need the 'with animation' part (in the comment and the method name)?  It
> seems like an implementation details.

Done.

https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:888: if (event.value.value)
On 2014/01/14 00:53:50, James Hawkins wrote:
> Can you document what event.value.value represents?

Done.

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

https://codereview.chromium.org/134103005/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:157: <span
i18n-content="hotwordSearchDescription"></span>
This line is actually supposed to be a light gray per the mocks. I've tried
every combination of id, class, divs, spans, p's, etc. I can think of to get it
to listen to browser_options.css, but something keeps overriding it and making
it solid black. Even doing something like:

#hotword-search-description {
  color: #333;
}

doesn't work. Any ideas?

[samarth, 2014-01-17 01:26:40]

search lgtm

https://codereview.chromium.org/134103005/diff/120001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/134103005/diff/120001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6912: +        Enable "Ok Google" to start a
voice search. <ph name="BEGIN_LINK">&lt;a target="_blank"
href="$1"&gt;</ph>Learn more<ph
name="END_LINK">&lt;/a&gt;<ex>&lt;/a&gt;</ex></ph>
I assume these string have already been approved?

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

https://codereview.chromium.org/134103005/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:157: <span
i18n-content="hotwordSearchDescription"></span>
On 2014/01/14 21:54:18, rpetterson wrote:
> This line is actually supposed to be a light gray per the mocks. I've tried
> every combination of id, class, divs, spans, p's, etc. I can think of to get
it
> to listen to browser_options.css, but something keeps overriding it and making
> it solid black. Even doing something like:
> 
> #hotword-search-description {
>   color: #333;
> }
> 
> doesn't work. Any ideas?

You're probably entering a "css specificity war".  In short, you can probably
make your style take effect by adding a more specific rule (e.g.
#hotword-search-description span {}).  Or adding a !important to the rule.  But
it's probably worth figuring out if there's a better way to restructure the
markup/css so you don't need to do that.

[James Hawkins, 2014-01-17 17:39:11]

On 2014/01/17 01:26:40, samarth wrote:
> search lgtm
> 
>
https://codereview.chromium.org/134103005/diff/120001/chrome/app/generated_re...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/134103005/diff/120001/chrome/app/generated_re...
> chrome/app/generated_resources.grd:6912: +        Enable "Ok Google" to start
a
> voice search. <ph name="BEGIN_LINK">&lt;a target="_blank"
> href="$1"&gt;</ph>Learn more<ph
> name="END_LINK">&lt;/a&gt;<ex>&lt;/a&gt;</ex></ph>
> I assume these string have already been approved?
> 
>
https://codereview.chromium.org/134103005/diff/120001/chrome/browser/resource...
> File chrome/browser/resources/options/browser_options.html (right):
> 
>
https://codereview.chromium.org/134103005/diff/120001/chrome/browser/resource...
> chrome/browser/resources/options/browser_options.html:157: <span
> i18n-content="hotwordSearchDescription"></span>
> On 2014/01/14 21:54:18, rpetterson wrote:
> > This line is actually supposed to be a light gray per the mocks. I've tried
> > every combination of id, class, divs, spans, p's, etc. I can think of to get
> it
> > to listen to browser_options.css, but something keeps overriding it and
making
> > it solid black. Even doing something like:
> > 
> > #hotword-search-description {
> >   color: #333;
> > }
> > 
> > doesn't work. Any ideas?
> 
> You're probably entering a "css specificity war".  In short, you can probably
> make your style take effect by adding a more specific rule (e.g.
> #hotword-search-description span {}).  Or adding a !important to the rule. 
But
> it's probably worth figuring out if there's a better way to restructure the
> markup/css so you don't need to do that.

Beyond that I usually just check the inspector to see which specifier is
overriding this, then figure out how to beat it in the 'war' as samarth put it
:-)

[James Hawkins, 2014-01-17 17:39:18]

https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.css (right):

https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.css:160: margin-left: 22px;
On 2014/01/14 21:54:17, rpetterson wrote:
> On 2014/01/14 00:53:50, James Hawkins wrote:
> > RTL
> 
> What's the best way to handle this? Just "margin"?

-webkit-margin-start

[rpetterson, 2014-01-17 23:53:23]

https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.css (right):

https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.css:160: margin-left: 22px;
On 2014/01/17 17:39:18, James Hawkins wrote:
> On 2014/01/14 21:54:17, rpetterson wrote:
> > On 2014/01/14 00:53:50, James Hawkins wrote:
> > > RTL
> > 
> > What's the best way to handle this? Just "margin"?
> 
> -webkit-margin-start

Done.

https://codereview.chromium.org/134103005/diff/120001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/134103005/diff/120001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6912: +        Enable "Ok Google" to start a
voice search. <ph name="BEGIN_LINK">&lt;a target="_blank"
href="$1"&gt;</ph>Learn more<ph
name="END_LINK">&lt;/a&gt;<ex>&lt;/a&gt;</ex></ph>
On 2014/01/17 01:26:41, samarth wrote:
> I assume these string have already been approved?

They are directly from the mocks I was given, but they are also behind a flag.
My understanding is that part of getting them in 2 weeks before branch is to get
the string approval.

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

https://codereview.chromium.org/134103005/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:157: <span
i18n-content="hotwordSearchDescription"></span>
On 2014/01/17 01:26:41, samarth wrote:
> On 2014/01/14 21:54:18, rpetterson wrote:
> > This line is actually supposed to be a light gray per the mocks. I've tried
> > every combination of id, class, divs, spans, p's, etc. I can think of to get
> it
> > to listen to browser_options.css, but something keeps overriding it and
making
> > it solid black. Even doing something like:
> > 
> > #hotword-search-description {
> >   color: #333;
> > }
> > 
> > doesn't work. Any ideas?
> 
> You're probably entering a "css specificity war".  In short, you can probably
> make your style take effect by adding a more specific rule (e.g.
> #hotword-search-description span {}).  Or adding a !important to the rule. 
But
> it's probably worth figuring out if there's a better way to restructure the
> markup/css so you don't need to do that.

So I actually had this right before, but I just had the wrong color in there so
I couldn't see the effect. Thanks for the tip on the inspector. It was quite
helpful for figuring this out. I've updated the CSS.

[James Hawkins, 2014-01-21 18:18:02]

LGTM with nit.

https://codereview.chromium.org/134103005/diff/200001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.css (right):

https://codereview.chromium.org/134103005/diff/200001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.css:164: color: #9B9B9B;
nit: Lower-case letters in hex.

[rpetterson, 2014-01-21 18:24:19]

https://codereview.chromium.org/134103005/diff/200001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.css (right):

https://codereview.chromium.org/134103005/diff/200001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.css:164: color: #9B9B9B;
On 2014/01/21 18:18:03, James Hawkins wrote:
> nit: Lower-case letters in hex.

Done.

[commit-bot: I haz the power, 2014-01-21 21:07:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/134103005/260001

[commit-bot: I haz the power, 2014-01-21 21:07:50]

Failed to apply patch for chrome/common/pref_names.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/common/pref_names.h
  Hunk #1 FAILED at 746.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/common/pref_names.h.rej

Patch:       chrome/common/pref_names.h
Index: chrome/common/pref_names.h
diff --git a/chrome/common/pref_names.h b/chrome/common/pref_names.h
index
81bcd4dfdcce967986bb4e3e726f134a23ca6d73..458a89989388904431ff967ac15a7d80e37978e4
100644
--- a/chrome/common/pref_names.h
+++ b/chrome/common/pref_names.h
@@ -746,6 +746,7 @@ extern const char kVideoCaptureAllowedUrls[];
 
 extern const char kHotwordSearchEnabled[];
 extern const char kHotwordSearchIncognitoEnabled[];
+extern const char kHotwordSearchTimeoutEnabled[];
 
 #if defined(OS_ANDROID)
 extern const char kProtectedMediaIdentifierEnabled[];

[rpetterson, 2014-01-22 18:47:00]

On 2014/01/21 21:07:50, I haz the power (commit-bot) wrote:
> Failed to apply patch for chrome/common/pref_names.h:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
>   patching file chrome/common/pref_names.h
>   Hunk #1 FAILED at 746.
>   1 out of 1 hunk FAILED -- saving rejects to file
> chrome/common/pref_names.h.rej
> 
> Patch:       chrome/common/pref_names.h
> Index: chrome/common/pref_names.h
> diff --git a/chrome/common/pref_names.h b/chrome/common/pref_names.h
> index
>
81bcd4dfdcce967986bb4e3e726f134a23ca6d73..458a89989388904431ff967ac15a7d80e37978e4
> 100644
> --- a/chrome/common/pref_names.h
> +++ b/chrome/common/pref_names.h
> @@ -746,6 +746,7 @@ extern const char kVideoCaptureAllowedUrls[];
>  
>  extern const char kHotwordSearchEnabled[];
>  extern const char kHotwordSearchIncognitoEnabled[];
> +extern const char kHotwordSearchTimeoutEnabled[];
>  
>  #if defined(OS_ANDROID)
>  extern const char kProtectedMediaIdentifierEnabled[];

I'm having an issue getting this patch to apply properly to pref_names.h/cc
(I've managed to trigger both). Any reviewers have an idea what I'm doing wrong?
The diffs look fine to me . . .

[James Hawkins, 2014-01-22 18:48:34]

On 2014/01/22 18:47:00, rpetterson wrote:
> On 2014/01/21 21:07:50, I haz the power (commit-bot) wrote:
> > Failed to apply patch for chrome/common/pref_names.h:
> > While running patch -p1 --forward --force --no-backup-if-mismatch;
> >   patching file chrome/common/pref_names.h
> >   Hunk #1 FAILED at 746.
> >   1 out of 1 hunk FAILED -- saving rejects to file
> > chrome/common/pref_names.h.rej
> > 
> > Patch:       chrome/common/pref_names.h
> > Index: chrome/common/pref_names.h
> > diff --git a/chrome/common/pref_names.h b/chrome/common/pref_names.h
> > index
> >
>
81bcd4dfdcce967986bb4e3e726f134a23ca6d73..458a89989388904431ff967ac15a7d80e37978e4
> > 100644
> > --- a/chrome/common/pref_names.h
> > +++ b/chrome/common/pref_names.h
> > @@ -746,6 +746,7 @@ extern const char kVideoCaptureAllowedUrls[];
> >  
> >  extern const char kHotwordSearchEnabled[];
> >  extern const char kHotwordSearchIncognitoEnabled[];
> > +extern const char kHotwordSearchTimeoutEnabled[];
> >  
> >  #if defined(OS_ANDROID)
> >  extern const char kProtectedMediaIdentifierEnabled[];
> 
> I'm having an issue getting this patch to apply properly to pref_names.h/cc
> (I've managed to trigger both). Any reviewers have an idea what I'm doing
wrong?
> The diffs look fine to me . . .

The diff above looks different than patch set 15.  Try committing that patch
set?

[rpetterson, 2014-01-22 18:49:55]

On 2014/01/22 18:48:34, James Hawkins wrote:
> On 2014/01/22 18:47:00, rpetterson wrote:
> > On 2014/01/21 21:07:50, I haz the power (commit-bot) wrote:
> > > Failed to apply patch for chrome/common/pref_names.h:
> > > While running patch -p1 --forward --force --no-backup-if-mismatch;
> > >   patching file chrome/common/pref_names.h
> > >   Hunk #1 FAILED at 746.
> > >   1 out of 1 hunk FAILED -- saving rejects to file
> > > chrome/common/pref_names.h.rej
> > > 
> > > Patch:       chrome/common/pref_names.h
> > > Index: chrome/common/pref_names.h
> > > diff --git a/chrome/common/pref_names.h b/chrome/common/pref_names.h
> > > index
> > >
> >
>
81bcd4dfdcce967986bb4e3e726f134a23ca6d73..458a89989388904431ff967ac15a7d80e37978e4
> > > 100644
> > > --- a/chrome/common/pref_names.h
> > > +++ b/chrome/common/pref_names.h
> > > @@ -746,6 +746,7 @@ extern const char kVideoCaptureAllowedUrls[];
> > >  
> > >  extern const char kHotwordSearchEnabled[];
> > >  extern const char kHotwordSearchIncognitoEnabled[];
> > > +extern const char kHotwordSearchTimeoutEnabled[];
> > >  
> > >  #if defined(OS_ANDROID)
> > >  extern const char kProtectedMediaIdentifierEnabled[];
> > 
> > I'm having an issue getting this patch to apply properly to pref_names.h/cc
> > (I've managed to trigger both). Any reviewers have an idea what I'm doing
> wrong?
> > The diffs look fine to me . . .
> 
> The diff above looks different than patch set 15.  Try committing that patch
> set?

The try bots are giving me the same error so I can't get a green run.

[commit-bot: I haz the power, 2014-01-23 00:10:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/134103005/820001

[commit-bot: I haz the power, 2014-01-23 03:06:25]

Change committed as 246500