Add a new "Configure" mechanism to CLD browser-side data interfaces.

Add a new "Configure" mechanism to CLD browser-side data interfaces.

This allows test code to be included all the time, regardless of whether a
non-static CLD build is in use. It also decouples sources from needing to
include an implementation-specific header to configure CLD, which eliminates
much of the need for the "#if defined" statements in code. Coupled with
the new CldDataSource (see related bug), this will allow a significant
refactoring of the build logic which will make it possible to build
chrome_shell and other targets with target-specific CLD configurations.

BUG=367239
TBR=toyoshim@chromium.org

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

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

For posterity the CLD data source work is here:
https://codereview.chromium.org/422843009

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

Exposed a bug in default_component_installer.h, fix is here:

https://codereview.chromium.org/422253002/

[simonb1, 2014-07-29 13:13:22]

lgtm

[Sorin Jianu, 2014-07-29 17:56:56]

Thank you Andrew!

https://codereview.chromium.org/424053002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/cld_component_installer.h (right):

https://codereview.chromium.org/424053002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/cld_component_installer.h:63: // Allows testing
code to detect the file path that was ultimately set in
Can this code be for production, in other words, why do we have to special case
it for test only? In there an issue with exposing an accessor for the latest
data file?

[jam, 2014-07-29 19:51:27]

https://codereview.chromium.org/424053002/diff/80001/components/translate/con...
File components/translate/content/browser/browser_cld_data_provider.h (right):

https://codereview.chromium.org/424053002/diff/80001/components/translate/con...
components/translate/content/browser/browser_cld_data_provider.h:80: // ensure
that the correct type of object is passed.
this seems a bit odd and fragile. Can an abstract interface be used instead?

[Andrew Hayden (chromium.org), 2014-07-29 19:51:55]

https://codereview.chromium.org/424053002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/cld_component_installer.h (right):

https://codereview.chromium.org/424053002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/cld_component_installer.h:63: // Allows testing
code to detect the file path that was ultimately set in
On 2014/07/29 17:56:56, Sorin Jianu wrote:
> Can this code be for production, in other words, why do we have to special
case
> it for test only? In there an issue with exposing an accessor for the latest
> data file?

There is no issue, but there is also no need. It's a one-way thing, you set it
and forget it. The only thing that would need this info is that code in
components/translate, which by definition should not have dependencies on THIS
code.

If we do have a need for it, I'm happy to remove the _ForTest suffix.

[Andrew Hayden (chromium.org), 2014-07-29 19:59:01]

On 2014/07/29 19:51:27, jam wrote:
>
https://codereview.chromium.org/424053002/diff/80001/components/translate/con...
> File components/translate/content/browser/browser_cld_data_provider.h (right):
> 
>
https://codereview.chromium.org/424053002/diff/80001/components/translate/con...
> components/translate/content/browser/browser_cld_data_provider.h:80: // ensure
> that the correct type of object is passed.
> this seems a bit odd and fragile. Can an abstract interface be used instead?

Sadly, I have not been able to come up with anything more graceful. See the
related bug for a long-winded description of why. It boils down to this: the
implementation of the provider can be arbitrary, may require arbitrary
configuration information, and may only be available very late (at link time).
If I were to add an abstract interface, all it would do is push the problem down
one layer (void* somewhere inside of it).

I definitely agree it is suboptimal, but getting rid of the "#if defined"
statements and the conditional includes in gyp files seems worth it to me. It
gets rid of a lot of unnecessary build complexity. I've tried to comment the
implementation heavily to make sure that it's clear, and I will definitely
update the docs here when done:

http://www.chromium.org/developers/how-tos/compact-language-detector-cld-data...

I know it's not ideal, but do you have any objections?

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

On 2014/07/29 19:59:01, Andrew Hayden wrote:
> On 2014/07/29 19:51:27, jam wrote:
> >
>
https://codereview.chromium.org/424053002/diff/80001/components/translate/con...
> > File components/translate/content/browser/browser_cld_data_provider.h
(right):
> > 
> >
>
https://codereview.chromium.org/424053002/diff/80001/components/translate/con...
> > components/translate/content/browser/browser_cld_data_provider.h:80: //
ensure
> > that the correct type of object is passed.
> > this seems a bit odd and fragile. Can an abstract interface be used instead?
> 
> Sadly, I have not been able to come up with anything more graceful. See the
> related bug for a long-winded description of why. It boils down to this: the
> implementation of the provider can be arbitrary, may require arbitrary
> configuration information, and may only be available very late (at link time).
> If I were to add an abstract interface, all it would do is push the problem
down
> one layer (void* somewhere inside of it).
> 
> I definitely agree it is suboptimal, but getting rid of the "#if defined"
> statements and the conditional includes in gyp files seems worth it to me. It
> gets rid of a lot of unnecessary build complexity. I've tried to comment the
> implementation heavily to make sure that it's clear, and I will definitely
> update the docs here when done:
> 
>
http://www.chromium.org/developers/how-tos/compact-language-detector-cld-data...
> 
> I know it's not ideal, but do you have any objections?

I've thought about this a lot and I think I'm coming to the conclusion we should
just call Configure "ConfigureDataFilePath" and make it take a base::FilePath.
The static impl can just do nothing, as it does for the generic Configure. If we
need a more generic configuration mechanism, we can do it later; non-Chromium
implementations can just link against their own implementation if they need to,
although it's less than ideal.

[jam, 2014-07-29 22:40:37]

lgtm

[Sorin Jianu, 2014-07-29 22:42:36]

Thank you Andrew!

https://codereview.chromium.org/424053002/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/cld_component_installer.cc (right):

https://codereview.chromium.org/424053002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:31:
base::LazyInstance<base::FilePath>::Leaky g_latest_cld_data_file_for_test =
Is this still test code?

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

On 2014/07/29 22:42:36, Sorin Jianu wrote:
> Thank you Andrew!
> 
>
https://codereview.chromium.org/424053002/diff/100001/chrome/browser/componen...
> File chrome/browser/component_updater/cld_component_installer.cc (right):
> 
>
https://codereview.chromium.org/424053002/diff/100001/chrome/browser/componen...
> chrome/browser/component_updater/cld_component_installer.cc:31:
> base::LazyInstance<base::FilePath>::Leaky g_latest_cld_data_file_for_test =
> Is this still test code?

No. Fixed. :)

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

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

[Sorin Jianu, 2014-07-29 23:08:06]

lgtm component updater.

https://codereview.chromium.org/424053002/diff/120001/chrome/browser/componen...
File chrome/browser/component_updater/cld_component_installer.cc (right):

https://codereview.chromium.org/424053002/diff/120001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:128: return
*g_latest_cld_data_file.Pointer();
Seems the lazy instance has a member Type& Get() which does the *Pointer. Maybe
use that?

https://codereview.chromium.org/424053002/diff/120001/chrome/browser/componen...
File chrome/browser/component_updater/cld_component_installer.h (right):

https://codereview.chromium.org/424053002/diff/120001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.h:63: // Allows
detection of the file path that was most recently set in
Nit: maybe just say, "Returns the file path...." unless the word "detection"
matters here.

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

On 2014/07/29 23:08:06, Sorin Jianu wrote:
> lgtm component updater.
> 
>
https://codereview.chromium.org/424053002/diff/120001/chrome/browser/componen...
> File chrome/browser/component_updater/cld_component_installer.cc (right):
> 
>
https://codereview.chromium.org/424053002/diff/120001/chrome/browser/componen...
> chrome/browser/component_updater/cld_component_installer.cc:128: return
> *g_latest_cld_data_file.Pointer();
> Seems the lazy instance has a member Type& Get() which does the *Pointer.
Maybe
> use that?
> 
>
https://codereview.chromium.org/424053002/diff/120001/chrome/browser/componen...
> File chrome/browser/component_updater/cld_component_installer.h (right):
> 
>
https://codereview.chromium.org/424053002/diff/120001/chrome/browser/componen...
> chrome/browser/component_updater/cld_component_installer.h:63: // Allows
> detection of the file path that was most recently set in
> Nit: maybe just say, "Returns the file path...." unless the word "detection"
> matters here.

Done.

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

The CQ bit was checked by andrewhayden@chromium.org

[commit-bot: I haz the power, 2014-07-29 23:20:33]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/424053002/14...

[commit-bot: I haz the power, 2014-07-30 07:13:47]

Change committed as 286430