Replace the use of CLD_DATA_FROM* and CLD-related ifdefs with runtime checks.

Replace the use of CLD_DATA_FROM* and CLD-related ifdefs with runtime checks.

This, along with other changes described in crbug/367239, allows refactoring
of the build logic to allow per-target customization of the CLD data source.
This, in turn, allows chrome_shell to be built with statically-linked CLD even
if other things (such as the full browser) are not.

BUG=367239
TBR=toyoshim@chromium.org

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

[Andrew Hayden (chromium.org), 2014-07-29 15:12:27]

Depends upon:
1. [Landed] https://codereview.chromium.org/422843009
2. [Trying] https://codereview.chromium.org/422253002
3. [Review] https://codereview.chromium.org/424053002

[jochen (gone - plz use gerrit), 2014-07-29 15:30:01]

lgtm

https://codereview.chromium.org/424123003/diff/1/chrome/browser/component_upd...
File chrome/browser/component_updater/cld_component_installer.cc (right):

https://codereview.chromium.org/424123003/diff/1/chrome/browser/component_upd...
chrome/browser/component_updater/cld_component_installer.cc:112: LOG(ERROR) <<
"Wrong CLD data source: " <<
why not LOG(FATAL)?

[Andrew Hayden (chromium.org), 2014-07-29 15:52:33]

On 2014/07/29 15:30:01, jochen wrote:
> lgtm
> 
>
https://codereview.chromium.org/424123003/diff/1/chrome/browser/component_upd...
> File chrome/browser/component_updater/cld_component_installer.cc (right):
> 
>
https://codereview.chromium.org/424123003/diff/1/chrome/browser/component_upd...
> chrome/browser/component_updater/cld_component_installer.cc:112: LOG(ERROR) <<
> "Wrong CLD data source: " <<
> why not LOG(FATAL)?

Language detection isn't critical to Chrome running, so it's not truly fatal; it
just means that you'll never see the translate bar, because things are
misconfigured. By returning early, we abort the registration process; so the CLD
data will never be loaded, language detection will gracefully fail every time,
and life carries on.

[Sorin Jianu, 2014-07-29 18:04:48]

Thank you Andrew!

https://codereview.chromium.org/424123003/diff/1/chrome/browser/component_upd...
File chrome/browser/component_updater/cld_component_installer.cc (right):

https://codereview.chromium.org/424123003/diff/1/chrome/browser/component_upd...
chrome/browser/component_updater/cld_component_installer.cc:109: // Make sure we
don't start up if the CLD data source isn't compatible.
I wonder if the new code can be refactored and implemented in the CLD module, so
the code in the component updater here looks like:

void RegisterCldComponent(ComponentUpdateService* cus) {
 if (!translate::CldDataSource::CanRegisterForUpdates())
   return false;
 ...
}

The idea is that we want to avoid seeing implementation details such as the name
of the data source. Plus, we can do extra validation, record UMA stats, etc if
the check is implemented on the CLD side.

https://codereview.chromium.org/424123003/diff/1/chrome/browser/component_upd...
chrome/browser/component_updater/cld_component_installer.cc:110: if
(translate::CldDataSource::GetName() != "component") {
Do we have concerns re: hard coded literal "component" ?

[Andrew Hayden (chromium.org), 2014-07-29 20:47:38]

On 2014/07/29 18:04:48, Sorin Jianu wrote:
> Thank you Andrew!
> 
>
https://codereview.chromium.org/424123003/diff/1/chrome/browser/component_upd...
> File chrome/browser/component_updater/cld_component_installer.cc (right):
> 
>
https://codereview.chromium.org/424123003/diff/1/chrome/browser/component_upd...
> chrome/browser/component_updater/cld_component_installer.cc:109: // Make sure
we
> don't start up if the CLD data source isn't compatible.
> I wonder if the new code can be refactored and implemented in the CLD module,
so
> the code in the component updater here looks like:
> 
> void RegisterCldComponent(ComponentUpdateService* cus) {
>  if (!translate::CldDataSource::CanRegisterForUpdates())
>    return false;
>  ...
> }
> 
> The idea is that we want to avoid seeing implementation details such as the
name
> of the data source. Plus, we can do extra validation, record UMA stats, etc if
> the check is implemented on the CLD side.
> 
>
https://codereview.chromium.org/424123003/diff/1/chrome/browser/component_upd...
> chrome/browser/component_updater/cld_component_installer.cc:110: if
> (translate::CldDataSource::GetName() != "component") {
> Do we have concerns re: hard coded literal "component" ?

Hmmmmmmmm. It's a great question. This boils down, I think, to this question:
where does the ugly stuff live? It's nice to centralize the ugly (in the CLD
data source implementation), but also very brittle; if a product build wants to
provide a custom CLD data source, then they have to implement a bunch of
nonsensical APIs (CanRegisterForUpdates? No... NeedsStandaloneDataFile? No...
IsFlavourOfTheDay? No...). I can see some decent arguments either way, but
someone somewhere has to pay the price for the customization.

Ultimately I agree with you; it's probably cleaner to implement it in the data
sources. For the sake of my sanity and the scope creep in crbug/367239, would
you be ok with me doing that work in a followup CL? I feel like this change
as-is already moves us in the right direction and improves the situation, so are
we ok with incremental steps forward?

Re: the hardcoded string. I think your suggestion takes care of that nicely, so
how about we get that cleaned up at the same time?

[Andrew Hayden (chromium.org), 2014-07-29 22:03:24]

On 2014/07/29 20:47:38, Andrew Hayden wrote:
> On 2014/07/29 18:04:48, Sorin Jianu wrote:
> > Thank you Andrew!
> > 
> >
>
https://codereview.chromium.org/424123003/diff/1/chrome/browser/component_upd...
> > File chrome/browser/component_updater/cld_component_installer.cc (right):
> > 
> >
>
https://codereview.chromium.org/424123003/diff/1/chrome/browser/component_upd...
> > chrome/browser/component_updater/cld_component_installer.cc:109: // Make
sure
> we
> > don't start up if the CLD data source isn't compatible.
> > I wonder if the new code can be refactored and implemented in the CLD
module,
> so
> > the code in the component updater here looks like:
> > 
> > void RegisterCldComponent(ComponentUpdateService* cus) {
> >  if (!translate::CldDataSource::CanRegisterForUpdates())
> >    return false;
> >  ...
> > }
> > 
> > The idea is that we want to avoid seeing implementation details such as the
> name
> > of the data source. Plus, we can do extra validation, record UMA stats, etc
if
> > the check is implemented on the CLD side.
> > 
> >
>
https://codereview.chromium.org/424123003/diff/1/chrome/browser/component_upd...
> > chrome/browser/component_updater/cld_component_installer.cc:110: if
> > (translate::CldDataSource::GetName() != "component") {
> > Do we have concerns re: hard coded literal "component" ?
> 
> Hmmmmmmmm. It's a great question. This boils down, I think, to this question:
> where does the ugly stuff live? It's nice to centralize the ugly (in the CLD
> data source implementation), but also very brittle; if a product build wants
to
> provide a custom CLD data source, then they have to implement a bunch of
> nonsensical APIs (CanRegisterForUpdates? No... NeedsStandaloneDataFile? No...
> IsFlavourOfTheDay? No...). I can see some decent arguments either way, but
> someone somewhere has to pay the price for the customization.
> 
> Ultimately I agree with you; it's probably cleaner to implement it in the data
> sources. For the sake of my sanity and the scope creep in crbug/367239, would
> you be ok with me doing that work in a followup CL? I feel like this change
> as-is already moves us in the right direction and improves the situation, so
are
> we ok with incremental steps forward?
> 
> Re: the hardcoded string. I think your suggestion takes care of that nicely,
so
> how about we get that cleaned up at the same time?

I've recanted my evil ways and implemented your suggestion PTAL.

[Andrew Hayden (chromium.org), 2014-07-29 22:47:34]

TBR'ing toyoshim@, per his request (he doesn't have time for a few days)

[Sorin Jianu, 2014-07-29 23:12:54]

lgtm component updater files

https://codereview.chromium.org/424123003/diff/60001/components/translate/con...
File components/translate/content/common/component_cld_data_source.cc (right):

https://codereview.chromium.org/424123003/diff/60001/components/translate/con...
components/translate/content/common/component_cld_data_source.cc:9: std::string
CldDataSource::GetName() {
These lines don't have to be indented right here and in similar files.

[Andrew Hayden (chromium.org), 2014-07-29 23:18:18]

On 2014/07/29 23:12:54, Sorin Jianu wrote:
> lgtm component updater files
> 
>
https://codereview.chromium.org/424123003/diff/60001/components/translate/con...
> File components/translate/content/common/component_cld_data_source.cc (right):
> 
>
https://codereview.chromium.org/424123003/diff/60001/components/translate/con...
> components/translate/content/common/component_cld_data_source.cc:9:
std::string
> CldDataSource::GetName() {
> These lines don't have to be indented right here and in similar files.

Whoops, silly me and my extra spaces. I'll fix and land after
https://codereview.chromium.org/424053002/. Thanks.

[Andrew Hayden (chromium.org), 2014-07-29 23:45:56]

On 2014/07/29 23:18:18, Andrew Hayden wrote:
> On 2014/07/29 23:12:54, Sorin Jianu wrote:
> > lgtm component updater files
> > 
> >
>
https://codereview.chromium.org/424123003/diff/60001/components/translate/con...
> > File components/translate/content/common/component_cld_data_source.cc
(right):
> > 
> >
>
https://codereview.chromium.org/424123003/diff/60001/components/translate/con...
> > components/translate/content/common/component_cld_data_source.cc:9:
> std::string
> > CldDataSource::GetName() {
> > These lines don't have to be indented right here and in similar files.
> 
> Whoops, silly me and my extra spaces. I'll fix and land after
> https://codereview.chromium.org/424053002/. Thanks.

Done.

[Andrew Hayden (chromium.org), 2014-07-30 07:25:52]

The CQ bit was checked by andrewhayden@chromium.org

[commit-bot: I haz the power, 2014-07-30 07:28:10]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/424123003/10...

[commit-bot: I haz the power, 2014-07-30 11:56:34]

Change committed as 286487