[Hotword] Set up hotwording to be externally loaded component extension. Only load if the field tri…

[Hotword] Set up hotwording to be externally loaded component extension. Only load if the field trial is correct and if they use "en" language since it is only available for English at this time.

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

[rpetterson, 2014-01-29 01:24:17]

Thanks for taking a look!

[rpetterson, 2014-01-29 22:26:19]

Ping Antony -- we're hoping to get this in by Friday for M34.

[asargent_no_longer_on_chrome, 2014-01-30 00:22:56]

lgtm

https://codereview.chromium.org/141013015/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/external_component_loader.cc (right):

https://codereview.chromium.org/141013015/diff/20001/chrome/browser/extension...
chrome/browser/extensions/external_component_loader.cc:71: #endif
optional: would it make sense to have a common method (somewhere in
chrome/browser/ perhaps) that abstracts getting the locale, so each call site
doesn't have to have their code littered with #ifdef's?

https://codereview.chromium.org/141013015/diff/20001/chrome/browser/extension...
chrome/browser/extensions/external_component_loader.cc:77:
!(strcmp(normalized_locale.c_str(), "en_US"));
optional: consider using base::EqualsASCII for the first condition, and folding
the second and third into one call to base::LowerCaseEqualsASCII (unless "EN_*"
should be disallowed?)

https://codereview.chromium.org/141013015/diff/20001/chrome/browser/extension...
chrome/browser/extensions/external_component_loader.cc:78: if (group != "" &&
group != "Disabled" && locale_available) {
optional: consider using 

  !group.empty() 

instead of 

  group != ""

Also, would it make sense to check the group first, and then only check the
locale if we're in the experiment group? I suppose the time to fetch and compare
the locale is pretty fast but this code runs on the startup path so early exits
anywhere we can get them help in aggregate.

[rpetterson, 2014-01-30 00:46:09]

Thank you!

https://codereview.chromium.org/141013015/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/external_component_loader.cc (right):

https://codereview.chromium.org/141013015/diff/20001/chrome/browser/extension...
chrome/browser/extensions/external_component_loader.cc:71: #endif
On 2014/01/30 00:22:56, Antony Sargent wrote:
> optional: would it make sense to have a common method (somewhere in
> chrome/browser/ perhaps) that abstracts getting the locale, so each call site
> doesn't have to have their code littered with #ifdef's? 

Yes, I actually have plans to do this in another CL since we'll need it
elsewhere. So this will get cleaned up then.

https://codereview.chromium.org/141013015/diff/20001/chrome/browser/extension...
chrome/browser/extensions/external_component_loader.cc:77:
!(strcmp(normalized_locale.c_str(), "en_US"));
On 2014/01/30 00:22:56, Antony Sargent wrote:
> optional: consider using base::EqualsASCII for the first condition, and
folding
> the second and third into one call to base::LowerCaseEqualsASCII (unless
"EN_*"
> should be disallowed?)
> 

That's correct. We only want the ones listed at this time, not ones like
"en-au".

https://codereview.chromium.org/141013015/diff/20001/chrome/browser/extension...
chrome/browser/extensions/external_component_loader.cc:78: if (group != "" &&
group != "Disabled" && locale_available) {
On 2014/01/30 00:22:56, Antony Sargent wrote:
> optional: consider using 
> 
>   !group.empty() 
> 
> instead of 
> 
>   group != ""
> 
> Also, would it make sense to check the group first, and then only check the
> locale if we're in the experiment group? I suppose the time to fetch and
compare
> the locale is pretty fast but this code runs on the startup path so early
exits
> anywhere we can get them help in aggregate. 

Done.

[commit-bot: I haz the power, 2014-01-30 07:24:36]

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

[commit-bot: I haz the power, 2014-01-30 10:33:12]

Change committed as 247923