Refactor language detection logic to allow non-static CLD data sources.

Refactor language detection logic to allow non-static CLD data sources.

This change further refines the approach taken in setting the implementation
used for accessing compact language detector at runtime. It is based upon the
pattern used in content_client.h, though adapted for this specific use case.
Fundamentally, there is a switch from having exactly one linked-in class define
a global method to instead having the high-level targets (like the content
shell, chrome shell, and so on) be responsible for setting the implementation
that is desired. This eliminates a lot of the ugly GYP/GN logic, but requires
work for each high-level target.

BUG=367239
Committed: https://crrev.com/14d91355848a733b18396d74c97d6b945260b899
Cr-Commit-Position: refs/heads/master@{#303669}

[Andrew Hayden (chromium.org), 2014-09-26 10:22:49]

This seems to be in fair shape, after ignoring it for a month. Some minor
self-comments. Still TODO: Documentation on how to customize the data source,
and removal of the GYP stuff that was previously put in place to allow
customization.

https://codereview.chromium.org/461633002/diff/100001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/461633002/diff/100001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1473: // Maintainers: Customize the
default data source here as desired.
Should have a note here for embedders, too, explaining how to change it outside
of the Chromium code base.

https://codereview.chromium.org/461633002/diff/100001/components/translate.gypi
File components/translate.gypi (right):

https://codereview.chromium.org/461633002/diff/100001/components/translate.gy...
components/translate.gypi:147:
#'translate/content/browser/data_file_browser_cld_data_provider.cc',
Just include these all the time?

https://codereview.chromium.org/461633002/diff/100001/components/translate.gy...
components/translate.gypi:175:
#'translate/content/common/data_file_cld_data_provider_messages.cc',
Just include these all the time?

https://codereview.chromium.org/461633002/diff/100001/components/translate.gy...
components/translate.gypi:205:
#'translate/content/renderer/data_file_renderer_cld_data_provider.h',
Just include these all the time?

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
File components/translate/content/browser/browser_cld_data_provider.h (right):

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider.h:48: virtual
void OnCldDataRequest();
Now that this isn't a pure virtual (same with SendCldDataResponse, below),
document the default behavior as being empty.

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
File components/translate/content/browser/browser_cld_data_provider_factory.h
(right):

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider_factory.h:16:
class BrowserCldDataProviderFactory {
Needs comments.

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
File
components/translate/content/browser/static_browser_cld_data_provider_factory.h
(right):

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
components/translate/content/browser/static_browser_cld_data_provider_factory.h:13:
class StaticBrowserCldDataProviderFactory :
Needs comments. And no, rietveld, this isn't a rename.

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
File components/translate/content/renderer/renderer_cld_data_provider.h (right):

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
components/translate/content/renderer/renderer_cld_data_provider.h:46: virtual
void SendCldDataRequest();
Document the default behavior of all the functions that are no longer
pure-virtual.

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
File components/translate/content/renderer/translate_helper.cc (right):

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
components/translate/content/renderer/translate_helper.cc:101: // Note:
Customize the data source and provider here, as desired.
This comment should look like the other one for maintainers. Call out
maintainers specifically and remind them that this is the DEFAULT, which
embedders can override.

[Andrew Hayden (chromium.org), 2014-09-26 10:48:50]

Test code also needs to be updated. Doing away with cld2_data_source means that
there's the same problem in the test harnesses as there was when this effort
started: one class has to be linked at compile time to implement
GetCldDataHarness(). A factory needs to be put in place instead, and the test
harness will have to pick the factory to use.

[Andrew Hayden (chromium.org), 2014-09-29 15:25:23]

Factories for testing code have been created (see new files
cld_data_harness_factory.*) and the tests have been switched to use them. This
should allow each platform to safely customize the CLD data provider and having
that provider get used at test time. It is also necessary for any embedder
wishing to run the tests with a custom implementation. Although it's not the
most beautiful thing I've ever written, I feel like the approach is reasonably
clean.

This is very close to ready now; browser_tests is building and linking, so
things have progressed nicely. We should be through with the biggest parts of
the change now; I've cleaned up the GN and Gyp files as well, and am going to
try and get this back to a running state that will pass the bots so we can get
this taken care of before trying to actually switch the implementations away
from static linking.

[picksi1, 2014-09-29 15:59:06]

picksi@chromium.org changed reviewers:
+ picksi@chromium.org

[picksi1, 2014-09-29 15:59:08]

I don't know about functional issues, but I've made some readability
suggestions.

https://codereview.chromium.org/461633002/diff/120001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/461633002/diff/120001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1474:
translate::CldDataSource::Set(translate::CldDataSource::STATIC(), false);
What does the 'false' here mean? Can we rename the function, or turn the false
into a (e.g.) named enum so it is clear what it's doing.

https://codereview.chromium.org/461633002/diff/120001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1478: }
see comment about about 'false'.

https://codereview.chromium.org/461633002/diff/120001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness.cc (right):

https://codereview.chromium.org/461633002/diff/120001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness.cc:26:
translate::CldDataSource::Set(translate::CldDataSource::STATIC(), true);
see comment about 'false' earlier!

https://codereview.chromium.org/461633002/diff/120001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness_factory.cc (right):

https://codereview.chromium.org/461633002/diff/120001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.cc:15: 
Is this the preferred chromium way of implementing instances/singletons? There
are funky static-variables inside private constructor functions that can be used
to guarantee that singletons stay the only instance of a class.

https://codereview.chromium.org/461633002/diff/120001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.cc:18: public:
This looks like it is derived from a test class, which feels wrong. Is there a
common base class that can be used for both this class and the test class?

https://codereview.chromium.org/461633002/diff/120001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.cc:27:
test::CldDataHarnessFactory* value;
Is the name 'value' correct here? Looks like it should be called
'harnessFactory' or something?

https://codereview.chromium.org/461633002/diff/120001/components/translate/co...
File components/translate/content/browser/browser_cld_data_provider_factory.cc
(right):

https://codereview.chromium.org/461633002/diff/120001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider_factory.cc:12: 
This looks familiar! Is there a common pattern here & in the earlier use that
can be pulled out somehow?

https://codereview.chromium.org/461633002/diff/120001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider_factory.cc:46:
g_instance = instance;
Now I understand what the bool does! It might be better to have two functions,
one called 'SetFactoryInstance' (?) and one called 'SetFactoryInstanceIfNull'(?)
to be explicit.

Also might be more readable/future proof to use IsInitialized() instead of
checking g_instance directly.

https://codereview.chromium.org/461633002/diff/120001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider_factory.cc:55:
return g_wrapped_default.Get().value;
Can we lazily initialize (to a default?) in here?

https://codereview.chromium.org/461633002/diff/120001/components/translate/co...
File components/translate/content/common/cld_data_source.cc (right):

https://codereview.chromium.org/461633002/diff/120001/components/translate/co...
components/translate/content/common/cld_data_source.cc:13: 
Definite Deja Vu now!

https://codereview.chromium.org/461633002/diff/120001/components/translate/co...
components/translate/content/common/cld_data_source.cc:18: }
Can we turn <Bool>,<Bool> into either 2 enums or (if it makes sense) into a
single 4-value enum, to make it clear what is going on here. e.g.
value = new translate::CldDataSource("none", FEED_THE_DOG, LIGHTS_ON);

or 

value = new translate::CldDataSource("none", ILLUMINATED_DOG_FEEDING);

Also 'value' is probably not the best name here :o).

https://codereview.chromium.org/461633002/diff/120001/components/translate/co...
components/translate/content/common/cld_data_source.cc:99: return NONE();
Can the constructor default this to NONE?

[Andrew Hayden (chromium.org), 2014-09-30 15:32:21]

John, I think we're far enough along now that it would be worth you taking a
look at this. The change has gotten unfortunately large, but perhaps I can split
it into smaller bits if we can find clean incremental ways to do so.

I've got 'all' building under Android and Linux.

Still TODO:
1. Hook up factory-setter in the various test code runners; currently everything
will be static-CLD by default, which is correct but certainly accidental.
2. Get this working on Mac, Windows and iOS

Thanks.

[Andrew Hayden (chromium.org), 2014-10-06 12:33:09]

I believe we are now at compile+link on all platforms using GYP (GN still needs
some love). There are some failing tests on ChromeOS that need to be tracked
down; it seems to be picking up the dynamic configuration. Perhaps chromevox
doesn't mean what I think it means.

[Andrew Hayden (chromium.org), 2014-10-06 13:12:58]

andrewhayden@chromium.org changed reviewers:
+ toyoshim@chromium.org

[Andrew Hayden (chromium.org), 2014-10-09 14:16:05]

As of patchset 22, compilation and linking seem to be good on all bots for both
GYP and GN.

Patchset 23 fixes a lifecycle problem in translate_helper.cc which was triggered
by unit tests; destroying the translate_helper would delete the global
RendererCldDataProvider for the *process*, which would subsequently result in a
use-after-free. Hopefully we will get to real test failures now.

[Andrew Hayden (chromium.org), 2014-10-14 14:52:59]

Patchset 27 looks golden, all the well-behaved bots seem happy. I think this is
ready for a real review. Simon, I haven't forgotten your comments and will
address them shortly; thanks!

I would love to split this up into two or three less-monolithic chunks but am
somewhat at a loss on how to do so reasonably. The interconnections between the
build files, data sources and uses of the data sources make this challenging. I
am open to suggestions, and apologize for the complexity here.

[Takashi Toyoshima, 2014-10-15 08:45:07]

This is the first quick review. But, yes, it's a large CL. I could not
understand details for now. I'll go deeper in the next review cycle.

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness.cc (right):

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness.cc:51: /* static */
scoped_ptr<CldDataHarness> CldDataHarness::NONE() {
This style is not used in chromium.

// static
scoped_ptr<CldDataHarness> CldDataHarness::CreateNoneCldDataHarness() {
  ...
}

will be better.

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness.h (right):

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness.h:77: static
scoped_ptr<CldDataHarness> NONE();
This kind of name conversion looks non-standard of Chromium I think.
How about CreateFooBarHarness() or something?

Also, do you have any reason not to use the factory class directly?

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness_factory.cc (right):

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.cc:105: /* static */
CldDataHarnessFactory* CldDataHarnessFactory::NONE() {
// static
CldDataHarnessFactory* CldDataHarnessFactory::...() {
  ...
}

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness_factory.h (right):

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.h:44: static
CldDataHarnessFactory* COMPONENT();
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File components/translate/content/browser/browser_cld_utils.cc (right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/browser/browser_cld_utils.cc:13: /* static */ void
BrowserCldUtils::ConfigureDefaultDataProvider() {
// static
void BrowserCldUtils::...

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File components/translate/content/renderer/renderer_cld_data_provider.cc
(right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/renderer/renderer_cld_data_provider.cc:9:
translate::RendererCldDataProvider* g_instance = NULL;
empty line between line 8 and 9.

[jam, 2014-10-21 16:49:42]

I only looked at src/chrome

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness_factory.h (right):

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.h:32: static
CldDataHarnessFactory* NONE();
this name doesn't confirm to google style guide

what's the point of having these methods. i don't see the callers?

[jochen (gone - plz use gerrit), 2014-10-23 13:57:36]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2014-10-23 13:57:40]

overall looks good

please address all the style nits and remove dead code like ::NONE() (even after
renaming it :) )

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness.cc (right):

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness.cc:28: };
disallow copy/assing + virtual dtor

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness_factory.cc (right):

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.cc:21: }
nit. disallow copy & assign, please also add virtual dtor()

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.cc:22: };
nit space after this line (and everywhere)

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.cc:27:
test::CldDataHarnessFactory* value;
i don't understand why you need this pattern. Just
LazyInstance<ComponentCldDataHarnessFactory>::Leaky should be enough, no?

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File components/translate/content/browser/browser_cld_data_provider.cc (right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider.cc:13: return
false; // Message not handled
nit. for EOL comments, two spaces before //

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File components/translate/content/browser/browser_cld_data_provider.h (right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider.h:49: virtual
void OnCldDataRequest() {};
nit, no ; at EOL

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider.h:57: };
disallow copy/assign

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File components/translate/content/browser/browser_cld_data_provider_factory.h
(right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider_factory.h:20:
virtual BrowserCldDataProvider* NewProvider(
nit. empty line above this one

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider_factory.h:38: };
disallow copy/assign

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File components/translate/content/browser/browser_cld_utils.h (right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/browser/browser_cld_utils.h:33: ~BrowserCldUtils()
{};
use a namespace instead of a class (and if it's a class, disallow copy/assign,
no ; at EOL)

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File components/translate/content/browser/data_file_browser_cld_data_provider.h
(right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/browser/data_file_browser_cld_data_provider.h:17: }
 // namespace content
// namespace content not required for forward decl

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File
components/translate/content/browser/static_browser_cld_data_provider_factory.h
(right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/browser/static_browser_cld_data_provider_factory.h:16:
explicit StaticBrowserCldDataProviderFactory();
nit. no explicit

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File components/translate/content/common/cld_data_source.h (right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/common/cld_data_source.h:19:
CldDataSource(std::string name,
consider using an enum instead of two bools, as one combination is invalid
(true, false)?

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/common/cld_data_source.h:109: };
disallow copy/assign

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File components/translate/content/renderer/renderer_cld_data_provider.h (right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/renderer/renderer_cld_data_provider.h:80: };
disallow copy/assign

[Andrew Hayden (chromium.org), 2014-10-28 15:18:40]

Patchset 30 addresses my own initial round of comments (proceeding in temporal
comment order).

https://codereview.chromium.org/461633002/diff/100001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/461633002/diff/100001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1473: // Maintainers: Customize the
default data source here as desired.
On 2014/09/26 10:22:49, Andrew Hayden wrote:
> Should have a note here for embedders, too, explaining how to change it
outside
> of the Chromium code base.

This is gone now.

https://codereview.chromium.org/461633002/diff/100001/components/translate.gypi
File components/translate.gypi (right):

https://codereview.chromium.org/461633002/diff/100001/components/translate.gy...
components/translate.gypi:147:
#'translate/content/browser/data_file_browser_cld_data_provider.cc',
On 2014/09/26 10:22:49, Andrew Hayden wrote:
> Just include these all the time?

Done.

https://codereview.chromium.org/461633002/diff/100001/components/translate.gy...
components/translate.gypi:175:
#'translate/content/common/data_file_cld_data_provider_messages.cc',
On 2014/09/26 10:22:49, Andrew Hayden wrote:
> Just include these all the time?

Done.

https://codereview.chromium.org/461633002/diff/100001/components/translate.gy...
components/translate.gypi:205:
#'translate/content/renderer/data_file_renderer_cld_data_provider.h',
On 2014/09/26 10:22:49, Andrew Hayden wrote:
> Just include these all the time?

Done.

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
File components/translate/content/browser/browser_cld_data_provider.h (right):

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider.h:48: virtual
void OnCldDataRequest();
On 2014/09/26 10:22:49, Andrew Hayden wrote:
> Now that this isn't a pure virtual (same with SendCldDataResponse, below),
> document the default behavior as being empty.

Done.

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
File components/translate/content/browser/browser_cld_data_provider_factory.h
(right):

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider_factory.h:16:
class BrowserCldDataProviderFactory {
On 2014/09/26 10:22:49, Andrew Hayden wrote:
> Needs comments.

Done.

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
File
components/translate/content/browser/static_browser_cld_data_provider_factory.h
(right):

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
components/translate/content/browser/static_browser_cld_data_provider_factory.h:13:
class StaticBrowserCldDataProviderFactory :
On 2014/09/26 10:22:49, Andrew Hayden wrote:
> Needs comments. And no, rietveld, this isn't a rename.

I have deleted this file completely.

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
File components/translate/content/renderer/renderer_cld_data_provider.h (right):

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
components/translate/content/renderer/renderer_cld_data_provider.h:46: virtual
void SendCldDataRequest();
On 2014/09/26 10:22:49, Andrew Hayden wrote:
> Document the default behavior of all the functions that are no longer
> pure-virtual.

Done.

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
File components/translate/content/renderer/translate_helper.cc (right):

https://codereview.chromium.org/461633002/diff/100001/components/translate/co...
components/translate/content/renderer/translate_helper.cc:101: // Note:
Customize the data source and provider here, as desired.
On 2014/09/26 10:22:49, Andrew Hayden wrote:
> This comment should look like the other one for maintainers. Call out
> maintainers specifically and remind them that this is the DEFAULT, which
> embedders can override.

Done.

[Andrew Hayden (chromium.org), 2014-10-30 14:25:15]

Simon's comments from patchset 7 addressed in patchset 33

https://codereview.chromium.org/461633002/diff/120001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/461633002/diff/120001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1474:
translate::CldDataSource::Set(translate::CldDataSource::STATIC(), false);
On 2014/09/29 15:59:08, picksi1 wrote:
> What does the 'false' here mean? Can we rename the function, or turn the false
> into a (e.g.) named enum so it is clear what it's doing.

I'm changing to two methods: Set and SetDefault. Hopefully this will clear
things up.

https://codereview.chromium.org/461633002/diff/120001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1478: }
On 2014/09/29 15:59:08, picksi1 wrote:
> see comment about about 'false'.

Similarly, this is now cleaned up. Now we explicitly check to see if this is the
component-updater data source.

https://codereview.chromium.org/461633002/diff/120001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness.cc (right):

https://codereview.chromium.org/461633002/diff/120001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness.cc:26:
translate::CldDataSource::Set(translate::CldDataSource::STATIC(), true);
On 2014/09/29 15:59:08, picksi1 wrote:
> see comment about 'false' earlier!

Fixed!

https://codereview.chromium.org/461633002/diff/120001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness_factory.cc (right):

https://codereview.chromium.org/461633002/diff/120001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.cc:15: 
On 2014/09/29 15:59:08, picksi1 wrote:
> Is this the preferred chromium way of implementing instances/singletons? There
> are funky static-variables inside private constructor functions that can be
used
> to guarantee that singletons stay the only instance of a class.

Yes. However, I am going to cheat heavily and work around this tiresome
boilerplate by exploiting the singletons to do pointer comparison against
"this". I think you'll find the result far easier to read.

https://codereview.chromium.org/461633002/diff/120001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.cc:18: public:
On 2014/09/29 15:59:08, picksi1 wrote:
> This looks like it is derived from a test class, which feels wrong. Is there a
> common base class that can be used for both this class and the test class?

I think you'll like the new version much better.

https://codereview.chromium.org/461633002/diff/120001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.cc:27:
test::CldDataHarnessFactory* value;
On 2014/09/29 15:59:08, picksi1 wrote:
> Is the name 'value' correct here? Looks like it should be called
> 'harnessFactory' or something?

Idiomatic, but now deleted.

https://codereview.chromium.org/461633002/diff/120001/components/translate/co...
File components/translate/content/browser/browser_cld_data_provider_factory.cc
(right):

https://codereview.chromium.org/461633002/diff/120001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider_factory.cc:12: 
On 2014/09/29 15:59:08, picksi1 wrote:
> This looks familiar! Is there a common pattern here & in the earlier use that
> can be pulled out somehow?

Similarly, this is now cleaned up.

https://codereview.chromium.org/461633002/diff/120001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider_factory.cc:46:
g_instance = instance;
On 2014/09/29 15:59:08, picksi1 wrote:
> Now I understand what the bool does! It might be better to have two functions,
> one called 'SetFactoryInstance' (?) and one called
'SetFactoryInstanceIfNull'(?)
> to be explicit.
> 
> Also might be more readable/future proof to use IsInitialized() instead of
> checking g_instance directly.

Done

https://codereview.chromium.org/461633002/diff/120001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider_factory.cc:55:
return g_wrapped_default.Get().value;
On 2014/09/29 15:59:08, picksi1 wrote:
> Can we lazily initialize (to a default?) in here?

That's what we're doing. Cleaner now.

https://codereview.chromium.org/461633002/diff/120001/components/translate/co...
File components/translate/content/common/cld_data_source.cc (right):

https://codereview.chromium.org/461633002/diff/120001/components/translate/co...
components/translate/content/common/cld_data_source.cc:13: 
On 2014/09/29 15:59:08, picksi1 wrote:
> Definite Deja Vu now!

Done.

https://codereview.chromium.org/461633002/diff/120001/components/translate/co...
components/translate/content/common/cld_data_source.cc:18: }
On 2014/09/29 15:59:08, picksi1 wrote:
> Can we turn <Bool>,<Bool> into either 2 enums or (if it makes sense) into a
> single 4-value enum, to make it clear what is going on here. e.g.
> value = new translate::CldDataSource("none", FEED_THE_DOG, LIGHTS_ON);
> 
> or 
> 
> value = new translate::CldDataSource("none", ILLUMINATED_DOG_FEEDING);
> 
> Also 'value' is probably not the best name here :o).

Done.

https://codereview.chromium.org/461633002/diff/120001/components/translate/co...
components/translate/content/common/cld_data_source.cc:99: return NONE();
On 2014/09/29 15:59:08, picksi1 wrote:
> Can the constructor default this to NONE?

Obviated.

[Andrew Hayden (chromium.org), 2014-10-30 14:27:31]

We've gone from [+1054 lines, -461 lines] to [+985 lines, -612 lines], which is
definitely moving in the right direction :)

[Andrew Hayden (chromium.org), 2014-10-30 16:56:32]

All remaining comments from jochen@ and toyoshim@ addressed in patchset 34.

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness.cc (right):

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness.cc:28: };
On 2014/10/23 13:57:38, jochen (OOO) wrote:
> disallow copy/assing + virtual dtor

Done.

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness.cc:51: /* static */
scoped_ptr<CldDataHarness> CldDataHarness::NONE() {
On 2014/10/15 08:45:07, Takashi Toyoshima (chromium) wrote:
> This style is not used in chromium.
> 
> // static
> scoped_ptr<CldDataHarness> CldDataHarness::CreateNoneCldDataHarness() {
>   ...
> }
> 
> will be better.

Done across all the classes I'm touching. There are still several instances in
Chromium, the funny part being that there are a few where the /* static */ is on
its own line :) But none of those others ones are mine <whistles>

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness.h (right):

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness.h:77: static
scoped_ptr<CldDataHarness> NONE();
On 2014/10/15 08:45:07, Takashi Toyoshima (chromium) wrote:
> This kind of name conversion looks non-standard of Chromium I think.
> How about CreateFooBarHarness() or something?
> 
> Also, do you have any reason not to use the factory class directly?

This has been greatly cleaned up. I think you'll find it much easier to read now
and more compliant.

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness_factory.cc (right):

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.cc:21: }
On 2014/10/23 13:57:39, jochen (OOO) wrote:
> nit. disallow copy & assign, please also add virtual dtor()

These classes have been purged. With fire.

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.cc:22: };
On 2014/10/23 13:57:39, jochen (OOO) wrote:
> nit space after this line (and everywhere)

Done.

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.cc:27:
test::CldDataHarnessFactory* value;
On 2014/10/23 13:57:39, jochen (OOO) wrote:
> i don't understand why you need this pattern. Just
> LazyInstance<ComponentCldDataHarnessFactory>::Leaky should be enough, no?

Yep!

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.cc:105: /* static */
CldDataHarnessFactory* CldDataHarnessFactory::NONE() {
On 2014/10/15 08:45:07, Takashi Toyoshima (chromium) wrote:
> // static
> CldDataHarnessFactory* CldDataHarnessFactory::...() {
>   ...
> }

Done everywhere

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness_factory.h (right):

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.h:32: static
CldDataHarnessFactory* NONE();
On 2014/10/21 16:49:42, jam wrote:
> this name doesn't confirm to google style guide
> 
> what's the point of having these methods. i don't see the callers?

I fixed the names and got rid of "NONE". The point of these methods is that
high-level test runners should choose a harness that is appropriate, and
embedders may have to inject their own factory to provide a harness that works
with their unique data source. I'll get around to the test code hookup in the
next patchset.

https://codereview.chromium.org/461633002/diff/520001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.h:44: static
CldDataHarnessFactory* COMPONENT();
On 2014/10/15 08:45:07, Takashi Toyoshima (chromium) wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File components/translate/content/browser/browser_cld_data_provider.cc (right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider.cc:13: return
false; // Message not handled
On 2014/10/23 13:57:39, jochen (OOO) wrote:
> nit. for EOL comments, two spaces before //

Surprisingly few of these, fixed the ones I found with git gs "; //"

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File components/translate/content/browser/browser_cld_data_provider.h (right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider.h:49: virtual
void OnCldDataRequest() {};
On 2014/10/23 13:57:39, jochen (OOO) wrote:
> nit, no ; at EOL

Applied everywhere, I think.

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider.h:57: };
On 2014/10/23 13:57:39, jochen (OOO) wrote:
> disallow copy/assign

Done.

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File components/translate/content/browser/browser_cld_data_provider_factory.h
(right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider_factory.h:20:
virtual BrowserCldDataProvider* NewProvider(
On 2014/10/23 13:57:39, jochen (OOO) wrote:
> nit. empty line above this one

Done and comments added too.

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider_factory.h:38: };
On 2014/10/23 13:57:39, jochen (OOO) wrote:
> disallow copy/assign

Done.

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File components/translate/content/browser/browser_cld_utils.cc (right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/browser/browser_cld_utils.cc:13: /* static */ void
BrowserCldUtils::ConfigureDefaultDataProvider() {
On 2014/10/15 08:45:07, Takashi Toyoshima (chromium) wrote:
> // static
> void BrowserCldUtils::...

Done.

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File components/translate/content/browser/browser_cld_utils.h (right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/browser/browser_cld_utils.h:33: ~BrowserCldUtils()
{};
On 2014/10/23 13:57:39, jochen (OOO) wrote:
> use a namespace instead of a class (and if it's a class, disallow copy/assign,
> no ; at EOL)

I'm fine with changing this to a namespace and a simple function therein; Let me
know how strongly you feel about it.

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File components/translate/content/browser/data_file_browser_cld_data_provider.h
(right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/browser/data_file_browser_cld_data_provider.h:17: }
 // namespace content
On 2014/10/23 13:57:39, jochen (OOO) wrote:
> // namespace content not required for forward decl

Acknowledged.

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File
components/translate/content/browser/static_browser_cld_data_provider_factory.h
(right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/browser/static_browser_cld_data_provider_factory.h:16:
explicit StaticBrowserCldDataProviderFactory();
On 2014/10/23 13:57:39, jochen (OOO) wrote:
> nit. no explicit

Deleted the class!

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File components/translate/content/common/cld_data_source.h (right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/common/cld_data_source.h:19:
CldDataSource(std::string name,
On 2014/10/23 13:57:39, jochen (OOO) wrote:
> consider using an enum instead of two bools, as one combination is invalid
> (true, false)?

Totally rebuilt this, now using singletons with identity comparisons (the
pointers) instead.

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/common/cld_data_source.h:109: };
On 2014/10/23 13:57:39, jochen (OOO) wrote:
> disallow copy/assign

Acknowledged.

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File components/translate/content/renderer/renderer_cld_data_provider.cc
(right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/renderer/renderer_cld_data_provider.cc:9:
translate::RendererCldDataProvider* g_instance = NULL;
On 2014/10/15 08:45:07, Takashi Toyoshima (chromium) wrote:
> empty line between line 8 and 9.

Done.

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
File components/translate/content/renderer/renderer_cld_data_provider.h (right):

https://codereview.chromium.org/461633002/diff/520001/components/translate/co...
components/translate/content/renderer/renderer_cld_data_provider.h:80: };
On 2014/10/23 13:57:39, jochen (OOO) wrote:
> disallow copy/assign

Done.

[Andrew Hayden (chromium.org), 2014-10-30 17:34:26]

OK, folks, this is ready for another round of review. I do still need to add the
hooks to high-level test runner code to set the right test harness factory, but
everything should work with the static defaults.

PTAL.

[picksi1, 2014-10-30 17:37:32]

Looking good! All I have are a couple of minor observations/comments...

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

https://codereview.chromium.org/461633002/diff/660001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:114: // This is a
serious build-time configuration error.
The above 2 lines appear three times. Can we wrap them into a single function
e.g.

if (translate::CldDataSource::IsIncompatible()) {
....
}

https://codereview.chromium.org/461633002/diff/660001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi (right):

https://codereview.chromium.org/461633002/diff/660001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:34: [
'cld_version==2', {
Earlier in the code there's code that checks for 0 or 2 (e.g.)

 if (cld_version == 0 || cld_version == 2)

Do these 2 checks need to be 'harmonized' or are they different things?

https://codereview.chromium.org/461633002/diff/660001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:39: }],
Is a relative path here OK? Other paths seem to have absolute paths.

https://codereview.chromium.org/461633002/diff/660001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/461633002/diff/660001/chrome/chrome.gyp#newco...
chrome/chrome.gyp:48: }],
as above, does this also need to have a check for cld_version 0?

[Andrew Hayden (chromium.org), 2014-10-30 17:51:13]

Thanks for the comments, Simon. I'll get to these tomorrow morning.

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

https://codereview.chromium.org/461633002/diff/660001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:114: // This is a
serious build-time configuration error.
On 2014/10/30 17:37:32, picksi1 wrote:
> The above 2 lines appear three times. Can we wrap them into a single function
> e.g.
> 
> if (translate::CldDataSource::IsIncompatible()) {
> ....
> }

I don't think it's worth it. It can't just be
translate::CldDataSource::IsIncompatible() because that question can't be
answered without context: compatible *with what*? The answer here is compatible
with the component updater, but there is only one such data source and it is the
ComponentCldDataSource. Which is why I did away with the booleans we previously
passed to the CldDataSource constructor along with all those pointless private
classes.

I don't particularly mind adding another method here, I just don't think it will
actually clean anything up. There's not likely to be any more code that ever
needs this particular check, since all of the logic starts here.

https://codereview.chromium.org/461633002/diff/660001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi (right):

https://codereview.chromium.org/461633002/diff/660001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:34: [
'cld_version==2', {
On 2014/10/30 17:37:32, picksi1 wrote:
> Earlier in the code there's code that checks for 0 or 2 (e.g.)
> 
>  if (cld_version == 0 || cld_version == 2)
> 
> Do these 2 checks need to be 'harmonized' or are they different things?
>     
> 

It should all be 0 or 2, for consistency. As soon as we get Android onto CLD2,
we can get rid of CLD1 along with all of these checks, so it's a minor problem.
This builds and runs on all platforms acceptably. But yes... I will change it
for consistency.... good catch.

https://codereview.chromium.org/461633002/diff/660001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:39: }],
On 2014/10/30 17:37:32, picksi1 wrote:
> Is a relative path here OK? Other paths seem to have absolute paths.

Also a good catch, this should be using (DEPTH) just like the ones above.
Thanks.

https://codereview.chromium.org/461633002/diff/660001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/461633002/diff/660001/chrome/chrome.gyp#newco...
chrome/chrome.gyp:48: }],
On 2014/10/30 17:37:32, picksi1 wrote:
> as above, does this also need to have a check for cld_version 0?

It should, yes.

[jam, 2014-10-31 00:13:37]

src/chrome lgtm

[Andrew Hayden (chromium.org), 2014-10-31 11:38:07]

Simon's comments in patchset 35. No recompilation resulted, that's good. :)

https://codereview.chromium.org/461633002/diff/660001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi (right):

https://codereview.chromium.org/461633002/diff/660001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:34: [
'cld_version==2', {
On 2014/10/30 17:51:13, Andrew Hayden wrote:
> On 2014/10/30 17:37:32, picksi1 wrote:
> > Earlier in the code there's code that checks for 0 or 2 (e.g.)
> > 
> >  if (cld_version == 0 || cld_version == 2)
> > 
> > Do these 2 checks need to be 'harmonized' or are they different things?
> >     
> > 
> 
> It should all be 0 or 2, for consistency. As soon as we get Android onto CLD2,
> we can get rid of CLD1 along with all of these checks, so it's a minor
problem.
> This builds and runs on all platforms acceptably. But yes... I will change it
> for consistency.... good catch.

Done everywhere.

https://codereview.chromium.org/461633002/diff/660001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:39: }],
On 2014/10/30 17:51:13, Andrew Hayden wrote:
> On 2014/10/30 17:37:32, picksi1 wrote:
> > Is a relative path here OK? Other paths seem to have absolute paths.
> 
> Also a good catch, this should be using (DEPTH) just like the ones above.
> Thanks.

Done everywhere.

https://codereview.chromium.org/461633002/diff/660001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/461633002/diff/660001/chrome/chrome.gyp#newco...
chrome/chrome.gyp:48: }],
On 2014/10/30 17:51:13, Andrew Hayden wrote:
> On 2014/10/30 17:37:32, picksi1 wrote:
> > as above, does this also need to have a check for cld_version 0?
> 
> It should, yes.

Done.

[Andrew Hayden (chromium.org), 2014-10-31 11:40:50]

andrewhayden@chromium.org changed reviewers:
+ brettw@chromium.org

[Andrew Hayden (chromium.org), 2014-10-31 11:43:13]

OK, still need ell-gee-tee-ems from:

toyoshim@: components/translate/*
brettw@: features.gni

Optional further reviews:
jochen@: Please take another look to ensure your comments have been
satisfactorily addressed.

[Andrew Hayden (chromium.org), 2014-10-31 12:39:04]

The CQ bit was checked by andrewhayden@chromium.org

[commit-bot: I haz the power, 2014-10-31 12:39:13]

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

[Andrew Hayden (chromium.org), 2014-10-31 12:42:00]

The CQ bit was unchecked by andrewhayden@chromium.org

[Andrew Hayden (chromium.org), 2014-10-31 16:13:07]

On 2014/10/31 12:39:13, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/461633002/720001

For posterity the CQ bit was checked accidentally while trying to click on a bot
label, and has been unchecked.

[brettw, 2014-10-31 16:37:27]

features.gni lgtm, yay!

[Takashi Toyoshima, 2014-11-06 14:34:29]

Hey, sorry for late. As I talked offline, I've been ooo, but now I'm back.
I still have some high level questions.

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

https://codereview.chromium.org/461633002/diff/740001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:113:
translate::CldDataSource::GetComponentDataSource()) {
Can you have a browser test that can detect this serious configuration error?
That will be useful to protect your code from unexpected changes by other
developers who just does a cleanup or something. Build file does not need a
strict review, and developers often think it's ok if all tries pass the build.

I have a sad experience that my feature was unexpectedly disabled by another
developer's change for build configuration. TODO is ok.

https://codereview.chromium.org/461633002/diff/740001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness_factory.cc (right):

https://codereview.chromium.org/461633002/diff/740001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.cc:27:
scoped_ptr<CldDataHarness> CldDataHarnessFactory::CreateCldDataHarness() {
I may miss something, but this design looks redundant. Why can we implement them
simply like;

class Factory {
  virtual Instance* Create() { instance_->Create(); }

  void SetDefaultFactory(Factory* factory) { instance_.reset(factory); }  // or
  void SetStandardFactoryAsDefault() { instance_.reset(new StandardFactory); }

  scoped_ptr<Factory> instance_;
}

class StandardFactory : public Factory {
  virtual Instance* Create() { return new StandardInstance; }
}

If needed, you can add static methods to Factory, and make instance_ global
variable.

https://codereview.chromium.org/461633002/diff/740001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness_factory.h (right):

https://codereview.chromium.org/461633002/diff/740001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.h:27: virtual
scoped_ptr<CldDataHarness> CreateCldDataHarness();
I feel Create() is enough good name for this since it's trivial to create
CldDataHarness here.

https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
File components/translate/content/browser/browser_cld_data_provider_factory.h
(right):

https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider_factory.h:34:
virtual BrowserCldDataProvider* NewProvider(
NewProvider() does not sound a consistent name with other factory classes. More
common name is better so that other developers are not surprised.

https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
File components/translate/content/common/cld_data_source.h (right):

https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
components/translate/content/common/cld_data_source.h:17: class CldDataSource {
See also my comments on CldDataHarnessFactory.

https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
components/translate/content/common/cld_data_source.h:62: static void
SetDefault(CldDataSource* data_source);
Do you really need both SetDefault() and Set()?

https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
components/translate/content/common/cld_data_source.h:75: static CldDataSource*
Get();
Can you avoid using these getters for raw object pointers?
I think you just use it like CldDataSource::Get()->GetName(). They can be
replaced with CldDataSource::GetUsingDataSourceName() or something.

https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
components/translate/content/common/cld_data_source.h:78: static CldDataSource*
GetStaticDataSource();
Also, if your use case is only "Get() == GetStaticDataSource()" and so on, it's
safe to add "bool IsUsingDataSourceStatic()".

[Takashi Toyoshima, 2014-11-07 05:55:34]

I chatted with Andrew, and now agreed the original design that jam@ is already
agreed in chrome/.
I looked components/translate again with respect for the design in chrome/,
there are no big questions or requests anymore.

It's almost LG.

https://codereview.chromium.org/461633002/diff/760001/components/translate/co...
File components/translate/content/browser/browser_cld_data_provider.h (right):

https://codereview.chromium.org/461633002/diff/760001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider.h:35:
~BrowserCldDataProvider() {}
Should be this destructor virtual? Please add override.

https://codereview.chromium.org/461633002/diff/760001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider.h:43: virtual
bool OnMessageReceived(const IPC::Message&) override;
IIUC, the latest style requires using only override, and removing virtual from
inheritances. Please check other files, too.
FYI https://codereview.chromium.org/684613002/

[Andrew Hayden (chromium.org), 2014-11-10 14:06:35]

Addressed all of these in the next patchset, except for removing raw pointer
access from CldDataSource, TBD in next patchset + 1....

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

https://codereview.chromium.org/461633002/diff/740001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:113:
translate::CldDataSource::GetComponentDataSource()) {
On 2014/11/06 14:34:28, Takashi Toyoshima (chromium) wrote:
> Can you have a browser test that can detect this serious configuration error?
> That will be useful to protect your code from unexpected changes by other
> developers who just does a cleanup or something. Build file does not need a
> strict review, and developers often think it's ok if all tries pass the build.
> 
> I have a sad experience that my feature was unexpectedly disabled by another
> developer's change for build configuration. TODO is ok.

Sadly, there's not a way to have a test for this because the high-level test
targets themselves have to configure which data source they need. This should
now be near-impossible, though, since the code that invokes RegisterCldComponent
now does a specific "if it's the CLD component, call RegisterCldComponent" (see
chrome_browser_main.cc, line 427 in this patchset).

Looking at this again, though, we can make things better. The reason for
chrome_browser_main.cc to make the decision on whether or not to call this
method was to avoid adding the Component Updater code when it wasn't necessary
to build; but I've abandoned that approach because of its unnecessary
complexity, so we could just move this check right inside RegisterCldComponent
and call it unconditionally. I'll do that, and then this will be clearer.

https://codereview.chromium.org/461633002/diff/740001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness_factory.cc (right):

https://codereview.chromium.org/461633002/diff/740001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.cc:27:
scoped_ptr<CldDataHarness> CldDataHarnessFactory::CreateCldDataHarness() {
On 2014/11/06 14:34:28, Takashi Toyoshima (chromium) wrote:
> I may miss something, but this design looks redundant. Why can we implement
them
> simply like;
> 
> class Factory {
>   virtual Instance* Create() { instance_->Create(); }
> 
>   void SetDefaultFactory(Factory* factory) { instance_.reset(factory); }  //
or
>   void SetStandardFactoryAsDefault() { instance_.reset(new StandardFactory); }
> 
>   scoped_ptr<Factory> instance_;
> }
> 
> class StandardFactory : public Factory {
>   virtual Instance* Create() { return new StandardInstance; }
> }
> 
> If needed, you can add static methods to Factory, and make instance_ global
> variable.

Tricky question.
Each platform needs to be able to set its default differently, so
SetStandardFactoryAsDefault would only work if we had platform-specific #ifdefs
inside the method to customize the choice of the "standard" factory for each
platform.

The lazy instances up above here are basically lies. All of the factory
implementations for Chromium (i.e., open-source) are trivial, so instead of
actually having three separate *implementations* of the "Factory" class we just
have three lazily-initialized *instances* that cheat by looking at "this" to see
what they should do.

I actually used to have something that looked more like what you're requesting:
see patchset 29's version of this file. I was asked to simplify where possible
and get rid of pointless classes, so this was my solution. If patchset 29
doesn't look like what you're asking for, then I am misinterpreting and please
tell me how you'd like to see patchet 29's design for this file evolved.

I will add some comments clarifying this to the code, because I think that it is
confusing as it is written.

https://codereview.chromium.org/461633002/diff/740001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness_factory.h (right):

https://codereview.chromium.org/461633002/diff/740001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.h:27: virtual
scoped_ptr<CldDataHarness> CreateCldDataHarness();
On 2014/11/06 14:34:29, Takashi Toyoshima (chromium) wrote:
> I feel Create() is enough good name for this since it's trivial to create
> CldDataHarness here.

Please see my comments in browser_cld_data_provider_factory.h; basically, it
looks like the established pattern - if there is one - is that FooFactory.h
declares CreateFoo(), so I'm sticking with that.

https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
File components/translate/content/browser/browser_cld_data_provider_factory.h
(right):

https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider_factory.h:33: //
TODO(andrewhayden) scoped_ptr?
This still needs to be done, and it seems to make sense to hand it off as a
scoped_ptr, so do it!

https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider_factory.h:34:
virtual BrowserCldDataProvider* NewProvider(
On 2014/11/06 14:34:29, Takashi Toyoshima (chromium) wrote:
> NewProvider() does not sound a consistent name with other factory classes.
More
> common name is better so that other developers are not surprised.

I took a look at cs.chromium.org and tried to find some factories for reference:
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/stre...
https://code.google.com/p/chromium/codesearch#chromium/src/ash/accelerators/f...
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/sess...
https://code.google.com/p/chromium/codesearch#chromium/src/sync/internal_api/...

It looks like there is some consensus that FooFactory.h declares a method called
either just "Create()" or "CreateFoo()", with "CreateFoo()" being dominant. I
will go with that naming scheme, but I think this goes against your comment in
CldDataHarnessFactory.h. As you have pointed out, consistency is good; so I will
stick with the CreateFoo() pattern unless you strongly object otherwise.

https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
File components/translate/content/common/cld_data_source.h (right):

https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
components/translate/content/common/cld_data_source.h:17: class CldDataSource {
On 2014/11/06 14:34:29, Takashi Toyoshima (chromium) wrote:
> See also my comments on CldDataHarnessFactory.

Acknowledged.

https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
components/translate/content/common/cld_data_source.h:62: static void
SetDefault(CldDataSource* data_source);
On 2014/11/06 14:34:29, Takashi Toyoshima (chromium) wrote:
> Do you really need both SetDefault() and Set()?

Yes, for the same reasons we need them everywhere else. The embedder needs to be
able to override the default, but each platform needs to be able to configure a
default on its own.

https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
components/translate/content/common/cld_data_source.h:75: static CldDataSource*
Get();
On 2014/11/06 14:34:29, Takashi Toyoshima (chromium) wrote:
> Can you avoid using these getters for raw object pointers?
> I think you just use it like CldDataSource::Get()->GetName(). They can be
> replaced with CldDataSource::GetUsingDataSourceName() or something.

It would be nice, yes, but there would be tradeoffs. GetName(),
SetCldDataFilePath(), GetCldDataFilePath() all are instance methods right now.
To completely hide the pointer, we would have to make them static and they would
internally reference the global instance and set the data upon it. I'm willing
to make the change, but I'm not sure it will actually look much better.
Obviously we have to switch to using Is[type]DataSource like you suggest below.
I'll do this in a separate patchset, stand by.

[Andrew Hayden (chromium.org), 2014-11-10 15:42:38]

>
https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
> components/translate/content/common/cld_data_source.h:75: static
CldDataSource*
> Get();
> On 2014/11/06 14:34:29, Takashi Toyoshima (chromium) wrote:
> > Can you avoid using these getters for raw object pointers?
> > I think you just use it like CldDataSource::Get()->GetName(). They can be
> > replaced with CldDataSource::GetUsingDataSourceName() or something.
> 
> It would be nice, yes, but there would be tradeoffs. GetName(),
> SetCldDataFilePath(), GetCldDataFilePath() all are instance methods right now.
> To completely hide the pointer, we would have to make them static and they
would
> internally reference the global instance and set the data upon it. I'm willing
> to make the change, but I'm not sure it will actually look much better.
> Obviously we have to switch to using Is[type]DataSource like you suggest
below.
> I'll do this in a separate patchset, stand by.

I talked this over with another engineer and tried a few times to do the
refactoring cleanly, but I think the alternatives are all harder to follow than
the existing code. I think the alternative that is the cleanest would be to add
three new methods to replace the existing "Get[*]DataSource":

SetStaticDataSourceAsDefault()
SetComponentDataSourceAsDefault()
SetStandaloneDataSourceAsDefault()

With this done could delete the existing "Get[*]DataSource" methods and replace
them with "bool IsUsing[*]DataSource()" variants. GetName(), SetCldDataFilePath,
and GetCldDataFilePath would all end up being static as described previously,
avoiding the need to retrieve the raw pointer; however, the functionality must
still be implementable by embedders, so new protected-level instance methods
would have to be added (e.g., GetNameImpl(), SetCldDataFilePathImpl(),
GetCldDataFilePathImpl()). For consistency, we'd want to apply the same
technique to CldDataHarnessFactory.

I don't really like this solution because it makes things unnecessarily complex
in order to hide access to a single pointer that is used in very few places in
the code. I propose a compromise: I'll add IsUsing[*]DataSource methods and use
those in the rest of the code to reduce the ugliness by an order of magnitude;
if you feel this isn't far enough, please further describe your concerns about
the pointer being exposed.

Thanks for all the constructive feedback so far!

[Andrew Hayden (chromium.org), 2014-11-10 16:13:40]

On 2014/11/07 05:55:34, Takashi Toyoshima (chromium) wrote:
> I chatted with Andrew, and now agreed the original design that jam@ is already
> agreed in chrome/.
> I looked components/translate again with respect for the design in chrome/,
> there are no big questions or requests anymore.
> 
> It's almost LG.
> 
>
https://codereview.chromium.org/461633002/diff/760001/components/translate/co...
> File components/translate/content/browser/browser_cld_data_provider.h (right):
> 
>
https://codereview.chromium.org/461633002/diff/760001/components/translate/co...
> components/translate/content/browser/browser_cld_data_provider.h:35:
> ~BrowserCldDataProvider() {}
> Should be this destructor virtual? Please add override.
> 
>
https://codereview.chromium.org/461633002/diff/760001/components/translate/co...
> components/translate/content/browser/browser_cld_data_provider.h:43: virtual
> bool OnMessageReceived(const IPC::Message&) override;
> IIUC, the latest style requires using only override, and removing virtual from
> inheritances. Please check other files, too.
> FYI https://codereview.chromium.org/684613002/

I believe that I have fixed all of these conformance issues. I did a manual pass
through all of the .h and .cc files I've touched and tried to update them
properly. I also reached out to dcheng@ to see if his tools were capable of
automating this but sadly it requires a lot of work to get those tools running
against a changeset, so I took the error-prone eyes-on approach. Please let me
know if you see any other non-conformance issues. Thanks for the link, too, this
explained exactly what I needed to know.

[Andrew Hayden (chromium.org), 2014-11-10 16:14:29]

Takashi@: I believe all your comments are addressed in patchset 45, could you
please take another look through the /components/translate tree? Thanks!

[Takashi Toyoshima, 2014-11-11 07:16:36]

Now I understand underlying difficulty, and agreed your replies. Thank you for
your hard work.
LGTM.

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

https://codereview.chromium.org/461633002/diff/740001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:113:
translate::CldDataSource::GetComponentDataSource()) {
OK, it sounds reasonable.

https://codereview.chromium.org/461633002/diff/740001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness_factory.cc (right):

https://codereview.chromium.org/461633002/diff/740001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.cc:27:
scoped_ptr<CldDataHarness> CldDataHarnessFactory::CreateCldDataHarness() {
Thank you for additional comments.
So, now I understand why you decide to do like this. I agreed.

https://codereview.chromium.org/461633002/diff/740001/chrome/browser/translat...
File chrome/browser/translate/cld_data_harness_factory.h (right):

https://codereview.chromium.org/461633002/diff/740001/chrome/browser/translat...
chrome/browser/translate/cld_data_harness_factory.h:27: virtual
scoped_ptr<CldDataHarness> CreateCldDataHarness();
I see.

https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
File components/translate/content/browser/browser_cld_data_provider_factory.h
(right):

https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider_factory.h:34:
virtual BrowserCldDataProvider* NewProvider(
I agreed to use CreateFoo() pattern.

https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
File components/translate/content/common/cld_data_source.h (right):

https://codereview.chromium.org/461633002/diff/740001/components/translate/co...
components/translate/content/common/cld_data_source.h:62: static void
SetDefault(CldDataSource* data_source);
Now I understand. Thanks.

[Andrew Hayden (chromium.org), 2014-11-11 15:52:05]

Patchsets 45, 46, 47, and 48 fix up the situation to let browser tests and unit
tests run correctly with the stricter sanity DCHECK/NOTREACHED macros I have in
place to prevent misconfiguration. These are each very minor tweaks that don't
change any of the design or functionality, so I'm going to go ahead and hit
submit as we're already on patchset 48 of a large review and Takashi has already
indicated willingness to proceed with later corrections as necessary. I will
ping jam@ and toyoshim@ offline for any further comments.

Thanks to everyone who has reviewed and helped out.

[Andrew Hayden (chromium.org), 2014-11-11 15:52:37]

The CQ bit was checked by andrewhayden@chromium.org

[commit-bot: I haz the power, 2014-11-11 15:53:44]

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

[commit-bot: I haz the power, 2014-11-11 17:47:25]

Committed patchset #48 (id:940001)

[commit-bot: I haz the power, 2014-11-11 17:48:08]

Patchset 48 (id:??) landed as
https://crrev.com/14d91355848a733b18396d74c97d6b945260b899
Cr-Commit-Position: refs/heads/master@{#303669}

[gone, 2014-11-11 19:29:42]

On 2014/11/11 17:48:08, I haz the power (commit-bot) wrote:
> Patchset 48 (id:??) landed as
> https://crrev.com/14d91355848a733b18396d74c97d6b945260b899
> Cr-Commit-Position: refs/heads/master@{#303669}

Anyone available to help diagnose why this has broken Chrome's downstream tree? 
We can't roll because of a linker error and I'm hesitant to revert this.

[gone, 2014-11-11 19:30:04]

On 2014/11/11 19:29:42, dfalcantara wrote:
> On 2014/11/11 17:48:08, I haz the power (commit-bot) wrote:
> > Patchset 48 (id:??) landed as
> > https://crrev.com/14d91355848a733b18396d74c97d6b945260b899
> > Cr-Commit-Position: refs/heads/master@{#303669}
> 
> Anyone available to help diagnose why this has broken Chrome's downstream
tree? 
> We can't roll because of a linker error and I'm hesitant to revert this.

*Chrome for Android's downstream tree

[gone, 2014-11-11 19:42:09]

dfalcantara@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[gone, 2014-11-11 19:42:12]

Is there any reason why the .gn file defines cld_version as 1 but the .gypi file
defines it as 2 on android?

[Andrew Hayden (chromium.org), 2014-11-11 20:00:53]

Yes, I messed up. build/common.gypi:750, android should still be using v1 of
cld2. Sorry about that. Working with Dan online to get this fixed now.

[Andrew Hayden (chromium.org), 2014-11-11 20:12:19]

Fix: https://codereview.chromium.org/698483003