Modularize Compact Language Detector 2 (CLD2) data sources

This patch defines three mechanisms for acquiring CLD2's data:

1. Static linking (the way it is done today)
2. Standalone dynamic data file bundled with the application
3. Dynamic data file downloaded via the Component Updater

This change does NOT switch any implementation to a different cld data source.
Android/iOS remain pinned to CLD1, and all platforms continue to statically
link the data with cld2_data_source="static".

This change has several important side effects:
1. The gyp variables "cld2_dynamic" and "cld2_is_component" have been removed.
   There is now a single variable, "cld2_data_source", that defines which
   implementation is to be used.
2. cld_component_installer.[h,cc] and cld_component_installer_unittest.cc are
   now conditionally built if and only if cld2_data_source=="component" and
   have direct dependencies upon components/translate/content/browser.
3. Almost all preprocesser checks for the CLD data source have been removed,
   greatly simplifying code flow. The logic previously gated by these checks
   has been split into separate implementation classes whose inclusion is
   controlled by the cld2_data_source gyp variable.

For more information, refer to crbug/383769.

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

[Andrew Hayden (chromium.org), 2014-06-12 02:54:52]

Work in progress, but feel free to take a look. Style nits are premature at this
point, as there is still much to be woven in.

Patchset 1 attacks the major architectural issues, solving the annoyances of
per-acquisition-model IPC and back-end implementation in the browser. Patchset
2, I expect, will add in the renderer side of things, and then in a third or
later patchset we can replace what exists today with this stuff.

[Andrew Hayden (chromium.org), 2014-06-12 12:41:08]

Still not ready for nits, but renderer and browser implementations are now
theoretically ready to replace the existing use in translate. Next patch set
will actually attempt to replace the current use, and will flesh out the factory
methods used to create the objects.

[Andrew Hayden (chromium.org), 2014-06-14 23:51:29]

not LGTM

https://codereview.chromium.org/333603002/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/333603002/diff/40001/build/common.gypi#newcod...
build/common.gypi:686: 'cld_version%': 2,
Don't ship this yet.

[Andrew Hayden (chromium.org), 2014-06-15 00:17:40]

Need to define destructors for the cld_data_provider classes in order to comply
with gcc requirements, else there's an error about missing the vtable for these
classes.

[jam, 2014-06-16 05:57:52]

this shouldn't be changing code in content, since translate is a feature in
chrome. see http://www.chromium.org/developers/content-module

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

jam@ (and Takashi): Right, sorry. How about
chrome/[renderer|browser|common]/translate or should this be part of the
component stuff in components/translate/[renderer|browser|common]?

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

This is almost ready, sans relocation of code and getting browser tests running
again.

[droger, 2014-06-16 13:09:58]

This code should go in the translate component if possible.

Here is my suggestion:

renderer_cld_data_provider and data_file_renderer_cld_data_provider should go in
components/translate/content/renderer

browser_cld_data_provider and data_file_browser_cld_data_provider should go in
components/translate/content/browser

data_file_cld_data_provider_messages should go in 
components/translate/content/common. You can use the translate_messages file
that is already here, but it is probably better to put your messages in a new
file.

About iOS:
as discussed offline, it seems that the logic to read the file and pass its
content to the CLD library are be needed on iOS if we want to use CLD2.
This means that this file handling logic should be split in a different fine,
probably in components/translate/core/language_detection (but in core/ in any
case).
It looks like this can be done in a later step though.

[Andrew Hayden (chromium.org), 2014-06-17 11:37:03]

I got a note back from Takashi as well saying he is in agreement. I'll make this
happen.

[Andrew Hayden (chromium.org), 2014-06-18 15:58:53]

Are these locations acceptable to all?

[droger, 2014-06-19 08:38:23]

File structure looks fine to me.

https://codereview.chromium.org/333603002/diff/80001/chrome/renderer/DEPS
File chrome/renderer/DEPS (right):

https://codereview.chromium.org/333603002/diff/80001/chrome/renderer/DEPS#new...
chrome/renderer/DEPS:15: "+content/common",
This looks wrong.

https://codereview.chromium.org/333603002/diff/80001/components/translate.gypi
File components/translate.gypi (right):

https://codereview.chromium.org/333603002/diff/80001/components/translate.gyp...
components/translate.gypi:168: ['cld_version==2', {
why not
['cld_version==2' and 'cld2_dynamic==1', {

[Andrew Hayden (chromium.org), 2014-06-19 09:27:11]

Thanks, droger@.

https://codereview.chromium.org/333603002/diff/80001/chrome/renderer/DEPS
File chrome/renderer/DEPS (right):

https://codereview.chromium.org/333603002/diff/80001/chrome/renderer/DEPS#new...
chrome/renderer/DEPS:15: "+content/common",
On 2014/06/19 08:38:22, droger wrote:
> This looks wrong.

Yeah, leftover cruft. I'm ditching this and fixing the DEPS that are broken
right now.

https://codereview.chromium.org/333603002/diff/80001/components/translate.gypi
File components/translate.gypi (right):

https://codereview.chromium.org/333603002/diff/80001/components/translate.gyp...
components/translate.gypi:168: ['cld_version==2', {
On 2014/06/19 08:38:23, droger wrote:
> why not
> ['cld_version==2' and 'cld2_dynamic==1', {

Because I forgot the gyp syntax for 'and' (oh right, it's not &&). I'll change
this around, but the next review after this is submitted is going to clean up
the variables and replace them with an enumerated choice of values:
CLD_DATA_ACQUISITION=[component_updater|bundled_file|static_link|custom]

So no big deal, probably.

[Takashi Toyoshima, 2014-06-19 10:20:35]

Locations look fine with me.

Quick review comments follow;

https://codereview.chromium.org/333603002/diff/80001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/333603002/diff/80001/build/common.gypi#newcod...
build/common.gypi:686: 'cld_version%': 2,
this part should be reviewed by miguelg@

https://codereview.chromium.org/333603002/diff/80001/chrome/browser/translate...
File chrome/browser/translate/chrome_translate_client.cc (right):

https://codereview.chromium.org/333603002/diff/80001/chrome/browser/translate...
chrome/browser/translate/chrome_translate_client.cc:58: cld_data_provider_
(content::BrowserCldDataProviderFor(
no space after cld_data_provider_.

https://codereview.chromium.org/333603002/diff/80001/chrome/browser/translate...
File chrome/browser/translate/chrome_translate_client.h (left):

https://codereview.chromium.org/333603002/diff/80001/chrome/browser/translate...
chrome/browser/translate/chrome_translate_client.h:176: // Necessary for binding
the callback to HandleCLDDataRequest on the blocking
Why do you remove these comments for WeakPtrFactory?

https://codereview.chromium.org/333603002/diff/80001/chrome/renderer/translat...
File chrome/renderer/translate/translate_helper.cc (right):

https://codereview.chromium.org/333603002/diff/80001/chrome/renderer/translat...
chrome/renderer/translate/translate_helper.cc:78: cld_data_provider_
(content::RendererCldDataProviderFor(this)),
no space after cld_data_provider_

https://codereview.chromium.org/333603002/diff/80001/chrome/renderer/translat...
chrome/renderer/translate/translate_helper.cc:84: {
Now you can and should merge this line 84 to line 83.

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

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/browser_cld_data_provider.h:5: #ifndef
CONTENT_PUBLIC_BROWSER_BROWSER_CLD_DATA_PROVIDER_H_
These macro names should be updated to follow the path change.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/browser_cld_data_provider.h:8: #include
"content/public/browser/render_view_host.h"
Forward declaration is enough for content::RenderViewHost.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/browser_cld_data_provider.h:10: #include
"ipc/ipc_message.h"
Forward declaration is enough for IPC::Message.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/browser_cld_data_provider.h:25: //
(Inherited from IPC::Listener)
'<class name> implementation:' is a common style for this kind of comments.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/browser_cld_data_provider.h:30: virtual
bool OnMessageReceived(const IPC::Message&);
OVERRIDE

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/browser_cld_data_provider.h:49: // Static
factory function defined by the implementation that produces a new
Did you build your CL with GYP_DEFINES+="component=shared_library"?
My default build configuration contains it, and I can not build this CL for
undefined symbols for Browser|RendererCldDataProviderFor.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/browser_cld_data_provider.h:51:
BrowserCldDataProvider* BrowserCldDataProviderFor(
CreateBrowser...() is better name since it starts with verb, and easy to
understand that an object will be created and returned by this function.

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

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/data_file_browser_cld_data_provider.h:6:
#define CHROME_BROWSER_TRANSLATE_DATA_FILE_BROWSER_CLD_DATA_PROVIDER_H_
macro name

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/data_file_browser_cld_data_provider.h:17:
DataFileBrowserCldDataProvider(content::RenderViewHost*);
explicit

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/data_file_browser_cld_data_provider.h:21:
private:
single space before 'private'

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/data_file_browser_cld_data_provider.h:28:
base::WeakPtrFactory<DataFileBrowserCldDataProvider> weak_pointer_factory_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/data_file_browser_cld_data_provider.h:31:
// Implementation of the static factory method from BrowserCldDataProvider,
Any reason to have this implementation in the header file?

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/data_file_browser_cld_data_provider.h:33:
BrowserCldDataProvider* BrowserCldDataProviderFor(
CreateBrowser...() is better name since it starts with verb, and easy to
understand that an object will be created and returned by this function.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
File
components/translate/content/renderer/data_file_renderer_cld_data_provider.cc
(right):

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/renderer/data_file_renderer_cld_data_provider.cc:21:
}  // namespace
remove line 21 - 23

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
File
components/translate/content/renderer/data_file_renderer_cld_data_provider.h
(right):

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:5:
#ifndef CHROME_RENDERER_TRANSLATE_DATA_FILE_RENDERER_CLD_DATA_PROVIDER_H_
macro name

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:21:
DataFileRendererCldDataProvider(content::RenderViewObserver*);
explicit

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:43:
static base::LazyInstance<CLDMmapWrapper>::Leaky s_cld_mmap_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:48:
RendererCldDataProvider* RendererCldDataProviderFor(
ditto, any reason to implement this in a header?

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
File components/translate/content/renderer/renderer_cld_data_provider.h (right):

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/renderer/renderer_cld_data_provider.h:6: #define
CONTENT_PUBLIC_RENDERER_RENDERER_CLD_DATA_PROVIDER_H_
macro name

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/renderer/renderer_cld_data_provider.h:59:
RendererCldDataProvider* RendererCldDataProviderFor(
CreateRendererCldDataProviderFor()

[Andrew Hayden (chromium.org), 2014-06-19 12:37:37]

I'll get to Takashi's comments in just a moment. I've finished cleaning up the
refactoring that was required due to the move. Most notably, there was an
inversion of control required: previously, the browser would query the component
installer to get the latest data file. This can't happen now that we're properly
componentized; the data_file_browser_cld_data_provider impl can't talk to
/chrome/*. And it shouldn't. The cleanup that was required was to expose a
getter/setter pair for the data file path. This cleans things up even more in
the component, removing the last of the ifdef's there. The component is now, I
think, totally agnostic to whether the component installer downloads CLD or
whether it's bundled as a standalone file. This is a Good Thing.

So, on to Takashi's comments. This code is now free of presubmit warnings, and
compiles and links cleanly on Android. There are still some things to fix, like
that damn virtual destructor... <grumble>... but I'm working on it. I consider
this about 95% done now, so please feel free to review "for real".

[Andrew Hayden (chromium.org), 2014-06-19 13:40:20]

All of Takashi's comments addressed. Destructors added.

https://codereview.chromium.org/333603002/diff/80001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/333603002/diff/80001/build/common.gypi#newcod...
build/common.gypi:686: 'cld_version%': 2,
On 2014/06/19 10:20:34, Takashi Toyoshima (chromium) wrote:
> this part should be reviewed by miguelg@

This won't be done as part of this change. I'll revert this particular bit, it's
just a pain to keep reverting and reapplying this config section to test on
Android :)

https://codereview.chromium.org/333603002/diff/80001/chrome/browser/translate...
File chrome/browser/translate/chrome_translate_client.cc (right):

https://codereview.chromium.org/333603002/diff/80001/chrome/browser/translate...
chrome/browser/translate/chrome_translate_client.cc:58: cld_data_provider_
(content::BrowserCldDataProviderFor(
On 2014/06/19 10:20:34, Takashi Toyoshima (chromium) wrote:
> no space after cld_data_provider_.

Done.

https://codereview.chromium.org/333603002/diff/80001/chrome/browser/translate...
File chrome/browser/translate/chrome_translate_client.h (left):

https://codereview.chromium.org/333603002/diff/80001/chrome/browser/translate...
chrome/browser/translate/chrome_translate_client.h:176: // Necessary for binding
the callback to HandleCLDDataRequest on the blocking
On 2014/06/19 10:20:34, Takashi Toyoshima (chromium) wrote:
> Why do you remove these comments for WeakPtrFactory?

Because this factory was here before HandleCLDDataRequest got added, and because
HandleCLDDataRequest no longer exists. I haven't looked into what else is using
it, but it is used in other parts of the code.

https://codereview.chromium.org/333603002/diff/80001/chrome/renderer/DEPS
File chrome/renderer/DEPS (right):

https://codereview.chromium.org/333603002/diff/80001/chrome/renderer/DEPS#new...
chrome/renderer/DEPS:15: "+content/common",
On 2014/06/19 09:27:11, Andrew Hayden wrote:
> On 2014/06/19 08:38:22, droger wrote:
> > This looks wrong.
> 
> Yeah, leftover cruft. I'm ditching this and fixing the DEPS that are broken
> right now.

Done.

https://codereview.chromium.org/333603002/diff/80001/chrome/renderer/translat...
File chrome/renderer/translate/translate_helper.cc (right):

https://codereview.chromium.org/333603002/diff/80001/chrome/renderer/translat...
chrome/renderer/translate/translate_helper.cc:78: cld_data_provider_
(content::RendererCldDataProviderFor(this)),
On 2014/06/19 10:20:34, Takashi Toyoshima (chromium) wrote:
> no space after cld_data_provider_

Done.

https://codereview.chromium.org/333603002/diff/80001/chrome/renderer/translat...
chrome/renderer/translate/translate_helper.cc:84: {
On 2014/06/19 10:20:34, Takashi Toyoshima (chromium) wrote:
> Now you can and should merge this line 84 to line 83.

Done.

https://codereview.chromium.org/333603002/diff/80001/components/translate.gypi
File components/translate.gypi (right):

https://codereview.chromium.org/333603002/diff/80001/components/translate.gyp...
components/translate.gypi:168: ['cld_version==2', {
On 2014/06/19 09:27:11, Andrew Hayden wrote:
> On 2014/06/19 08:38:23, droger wrote:
> > why not
> > ['cld_version==2' and 'cld2_dynamic==1', {
> 
> Because I forgot the gyp syntax for 'and' (oh right, it's not &&). I'll change
> this around, but the next review after this is submitted is going to clean up
> the variables and replace them with an enumerated choice of values:
> CLD_DATA_ACQUISITION=[component_updater|bundled_file|static_link|custom]
> 
> So no big deal, probably.

Done.

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

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/browser_cld_data_provider.h:5: #ifndef
CONTENT_PUBLIC_BROWSER_BROWSER_CLD_DATA_PROVIDER_H_
On 2014/06/19 10:20:34, Takashi Toyoshima (chromium) wrote:
> These macro names should be updated to follow the path change.

Done.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/browser_cld_data_provider.h:8: #include
"content/public/browser/render_view_host.h"
On 2014/06/19 10:20:34, Takashi Toyoshima (chromium) wrote:
> Forward declaration is enough for content::RenderViewHost.

Done.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/browser_cld_data_provider.h:10: #include
"ipc/ipc_message.h"
On 2014/06/19 10:20:34, Takashi Toyoshima (chromium) wrote:
> Forward declaration is enough for IPC::Message.

Done.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/browser_cld_data_provider.h:25: //
(Inherited from IPC::Listener)
On 2014/06/19 10:20:34, Takashi Toyoshima (chromium) wrote:
> '<class name> implementation:' is a common style for this kind of comments.

Done.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/browser_cld_data_provider.h:30: virtual
bool OnMessageReceived(const IPC::Message&);
On 2014/06/19 10:20:34, Takashi Toyoshima (chromium) wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/browser_cld_data_provider.h:49: // Static
factory function defined by the implementation that produces a new
On 2014/06/19 10:20:34, Takashi Toyoshima (chromium) wrote:
> Did you build your CL with GYP_DEFINES+="component=shared_library"?
> My default build configuration contains it, and I can not build this CL for
> undefined symbols for Browser|RendererCldDataProviderFor.

No, I was just building for my local vanilla Android checkout. I'll make sure
there is nothing left undefined on any systems before submitting. Thanks!

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/browser_cld_data_provider.h:51:
BrowserCldDataProvider* BrowserCldDataProviderFor(
On 2014/06/19 10:20:34, Takashi Toyoshima (chromium) wrote:
> CreateBrowser...() is better name since it starts with verb, and easy to
> understand that an object will be created and returned by this function.

Done.

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

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/data_file_browser_cld_data_provider.h:6:
#define CHROME_BROWSER_TRANSLATE_DATA_FILE_BROWSER_CLD_DATA_PROVIDER_H_
On 2014/06/19 10:20:35, Takashi Toyoshima (chromium) wrote:
> macro name

Done.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/data_file_browser_cld_data_provider.h:17:
DataFileBrowserCldDataProvider(content::RenderViewHost*);
On 2014/06/19 10:20:35, Takashi Toyoshima (chromium) wrote:
> explicit

Done.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/data_file_browser_cld_data_provider.h:21:
private:
On 2014/06/19 10:20:35, Takashi Toyoshima (chromium) wrote:
> single space before 'private'

Done.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/data_file_browser_cld_data_provider.h:28:
base::WeakPtrFactory<DataFileBrowserCldDataProvider> weak_pointer_factory_;
On 2014/06/19 10:20:34, Takashi Toyoshima (chromium) wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/data_file_browser_cld_data_provider.h:31:
// Implementation of the static factory method from BrowserCldDataProvider,
On 2014/06/19 10:20:35, Takashi Toyoshima (chromium) wrote:
> Any reason to have this implementation in the header file?

I figured it was an ok place for it, but I was of course wrong. The file can be
included in multiple libraries (component, browser) and the result is multiple
definitions. So.... I've moved to the .cc file.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/browser/data_file_browser_cld_data_provider.h:33:
BrowserCldDataProvider* BrowserCldDataProviderFor(
On 2014/06/19 10:20:35, Takashi Toyoshima (chromium) wrote:
> CreateBrowser...() is better name since it starts with verb, and easy to
> understand that an object will be created and returned by this function.

Done.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
File
components/translate/content/renderer/data_file_renderer_cld_data_provider.cc
(right):

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/renderer/data_file_renderer_cld_data_provider.cc:21:
}  // namespace
On 2014/06/19 10:20:35, Takashi Toyoshima (chromium) wrote:
> remove line 21 - 23

Whoops. Done.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
File
components/translate/content/renderer/data_file_renderer_cld_data_provider.h
(right):

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:5:
#ifndef CHROME_RENDERER_TRANSLATE_DATA_FILE_RENDERER_CLD_DATA_PROVIDER_H_
On 2014/06/19 10:20:35, Takashi Toyoshima (chromium) wrote:
> macro name

Done.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:21:
DataFileRendererCldDataProvider(content::RenderViewObserver*);
On 2014/06/19 10:20:35, Takashi Toyoshima (chromium) wrote:
> explicit

Done.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:43:
static base::LazyInstance<CLDMmapWrapper>::Leaky s_cld_mmap_;
On 2014/06/19 10:20:35, Takashi Toyoshima (chromium) wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:48:
RendererCldDataProvider* RendererCldDataProviderFor(
On 2014/06/19 10:20:35, Takashi Toyoshima (chromium) wrote:
> ditto, any reason to implement this in a header?

Same response, moved to .cc file.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
File components/translate/content/renderer/renderer_cld_data_provider.h (right):

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/renderer/renderer_cld_data_provider.h:6: #define
CONTENT_PUBLIC_RENDERER_RENDERER_CLD_DATA_PROVIDER_H_
On 2014/06/19 10:20:35, Takashi Toyoshima (chromium) wrote:
> macro name

Done.

https://codereview.chromium.org/333603002/diff/80001/components/translate/con...
components/translate/content/renderer/renderer_cld_data_provider.h:59:
RendererCldDataProvider* RendererCldDataProviderFor(
On 2014/06/19 10:20:35, Takashi Toyoshima (chromium) wrote:
> CreateRendererCldDataProviderFor()

Done.

[Andrew Hayden (chromium.org), 2014-06-19 13:45:25]

I'm now working on getting browser tests running again under the new framework.
I do not expect any significant changes to the existing patchset here, just
minor tweaks to the browser test utilities to use the new methods.

[droger, 2014-06-19 16:13:19]

https://codereview.chromium.org/333603002/diff/120001/chrome/browser/componen...
File chrome/browser/component_updater/DEPS (right):

https://codereview.chromium.org/333603002/diff/120001/chrome/browser/componen...
chrome/browser/component_updater/DEPS:2:
"+components/translate/content/browser",
this is already covered by chrome/browser/DEPS

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

https://codereview.chromium.org/333603002/diff/120001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.cc:48: namespace content {
The namespace should be translate, not content.

https://codereview.chromium.org/333603002/diff/120001/chrome/renderer/transla...
File chrome/renderer/translate/translate_helper.h (right):

https://codereview.chromium.org/333603002/diff/120001/chrome/renderer/transla...
chrome/renderer/translate/translate_helper.h:25: namespace content {
should be namespace translate.

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

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider.h:22: //  
content/public/renderer/renderer_cld_data_provider.h
update the comment.

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider.h:27: class
BrowserCldDataProvider : public IPC::Listener {
This should be in translate namespace

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

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
components/translate/content/browser/data_file_browser_cld_data_provider.cc:46:
const base::FilePath& path) {
This fits in one line.

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

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
components/translate/content/browser/data_file_browser_cld_data_provider.h:14:
namespace content {
namespace translate

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
components/translate/content/browser/data_file_browser_cld_data_provider.h:21:
bool OnMessageReceived(const IPC::Message&);
virtual and OVERRIDE?

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
File
components/translate/content/renderer/data_file_renderer_cld_data_provider.h
(right):

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:16:
namespace content {
namespace translate

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:23:
bool OnMessageReceived(const IPC::Message&);
virtual and OVERRIDE?

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

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
components/translate/content/renderer/renderer_cld_data_provider.h:12: class
Message;
no leading whitespace

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
components/translate/content/renderer/renderer_cld_data_provider.h:23: //  
content/public/browser/browser_cld_data_provider.h
update the comment.

[Andrew Hayden (chromium.org), 2014-06-19 19:49:30]

All comments so far are now addressed, and I've added the static-compiled
(no-op) implementations that we're going to need shortly. Having these avoids
some #ifdef's in source files and keeps logic consistent throughout the
application.

https://codereview.chromium.org/333603002/diff/120001/chrome/browser/componen...
File chrome/browser/component_updater/DEPS (right):

https://codereview.chromium.org/333603002/diff/120001/chrome/browser/componen...
chrome/browser/component_updater/DEPS:2:
"+components/translate/content/browser",
On 2014/06/19 16:13:18, droger wrote:
> this is already covered by chrome/browser/DEPS

Done.

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

https://codereview.chromium.org/333603002/diff/120001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.cc:48: namespace content {
On 2014/06/19 16:13:18, droger wrote:
> The namespace should be translate, not content.

Whoops. Cleanup on aisle 12! <sweep> <sweep>

Done. Embarassing. Obviously, fixed throughout the entire review...

https://codereview.chromium.org/333603002/diff/120001/chrome/renderer/transla...
File chrome/renderer/translate/translate_helper.h (right):

https://codereview.chromium.org/333603002/diff/120001/chrome/renderer/transla...
chrome/renderer/translate/translate_helper.h:25: namespace content {
On 2014/06/19 16:13:18, droger wrote:
> should be namespace translate.

Done.

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

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider.h:22: //  
content/public/renderer/renderer_cld_data_provider.h
On 2014/06/19 16:13:18, droger wrote:
> update the comment.

Done.

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
components/translate/content/browser/browser_cld_data_provider.h:27: class
BrowserCldDataProvider : public IPC::Listener {
On 2014/06/19 16:13:19, droger wrote:
> This should be in translate namespace

Done.

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

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
components/translate/content/browser/data_file_browser_cld_data_provider.cc:46:
const base::FilePath& path) {
On 2014/06/19 16:13:19, droger wrote:
> This fits in one line.

Done.

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

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
components/translate/content/browser/data_file_browser_cld_data_provider.h:14:
namespace content {
On 2014/06/19 16:13:19, droger wrote:
> namespace translate

Done.

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
components/translate/content/browser/data_file_browser_cld_data_provider.h:21:
bool OnMessageReceived(const IPC::Message&);
On 2014/06/19 16:13:19, droger wrote:
> virtual and OVERRIDE?

I don't currently plan to have any more subclasses, so I've left out virtual
here. OVERRIDE, though, is added.

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
File
components/translate/content/renderer/data_file_renderer_cld_data_provider.h
(right):

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:16:
namespace content {
On 2014/06/19 16:13:19, droger wrote:
> namespace translate

Done.

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:23:
bool OnMessageReceived(const IPC::Message&);
On 2014/06/19 16:13:19, droger wrote:
> virtual and OVERRIDE?

As above, leaving virtual out but OVERRIDE is in.

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

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
components/translate/content/renderer/renderer_cld_data_provider.h:12: class
Message;
On 2014/06/19 16:13:19, droger wrote:
> no leading whitespace

Done.

https://codereview.chromium.org/333603002/diff/120001/components/translate/co...
components/translate/content/renderer/renderer_cld_data_provider.h:23: //  
content/public/browser/browser_cld_data_provider.h
On 2014/06/19 16:13:19, droger wrote:
> update the comment.

Done.

[Andrew Hayden (chromium.org), 2014-06-19 21:16:55]

Stand by, browser tests need more work. The current implementation of
translate_browser_test_utils has EXACTLY the same problem as the existing code:
it's not possible to swap the implementation out via GYP. That needs to be fixed
as well.

[Andrew Hayden (chromium.org), 2014-06-20 17:22:23]

OK, I think this is finally, FINALLY complete. All the unit tests and browser
tests are working in all 3 modes - static, standalone and component; the CLD
stuff is cleanly and thoroughly componentized, and gyp is happy.

We're ready to rock.  File assignments below, distilled from git cl owners:

palmer@:
ipc/ipc_message_start.h.

toyoshim@:
  chrome/browser/translate/chrome_translate_client.cc
  chrome/browser/translate/chrome_translate_client.h
  chrome/browser/translate/scoped_cld_component_data_harness.cc
  chrome/browser/translate/scoped_cld_component_data_harness.h
  chrome/browser/translate/scoped_cld_data_harness.cc
  chrome/browser/translate/scoped_cld_data_harness.h
  chrome/browser/translate/scoped_cld_standalone_data_harness.cc
  chrome/browser/translate/scoped_cld_standalone_data_harness.h
  chrome/browser/translate/scoped_cld_static_data_harness.cc
  chrome/browser/translate/scoped_cld_static_data_harness.h
  chrome/browser/translate/translate_browser_test_utils.cc
  chrome/browser/translate/translate_browser_test_utils.h
  chrome/browser/translate/translate_manager_browsertest.cc
  chrome/renderer/translate/translate_helper.cc
  chrome/renderer/translate/translate_helper.h
  components/translate.gypi
  components/translate/content/browser/browser_cld_data_provider.h
  components/translate/content/browser/data_file_browser_cld_data_provider.cc
  components/translate/content/browser/data_file_browser_cld_data_provider.h
  components/translate/content/browser/static_browser_cld_data_provider.cc
  components/translate/content/browser/static_browser_cld_data_provider.h
  components/translate/content/common/data_file_cld_data_provider_messages.cc
  components/translate/content/common/data_file_cld_data_provider_messages.h
  components/translate/content/common/translate_messages.h
  components/translate/content/renderer/DEPS
  components/translate/content/renderer/data_file_renderer_cld_data_provider.cc
  components/translate/content/renderer/data_file_renderer_cld_data_provider.h
  components/translate/content/renderer/renderer_cld_data_provider.h
  components/translate/content/renderer/static_renderer_cld_data_provider.cc
  components/translate/content/renderer/static_renderer_cld_data_provider.h
  components/translate/content/test/fake_data_file_browser_cld_data_provider.cc
  components/translate/content/test/fake_data_file_browser_cld_data_provider.h

jam@: (and anything else you want to look at, but you deferred to toyoshim@ et
al for "*translate*" in email)
  chrome/browser/chrome_browser_main.cc
  chrome/browser/component_updater/cld_component_installer.cc
  chrome/browser/component_updater/cld_component_installer.h
  chrome/browser/component_updater/test/cld_component_installer_unittest.cc
  chrome/browser/policy/policy_browsertest.cc
  chrome/browser/ui/browser_browsertest.cc
  chrome/browser/ui/views/translate/translate_bubble_view_browsertest.cc
  chrome/common/chrome_constants.cc
  chrome/common/chrome_constants.h
  chrome/renderer/DEPS

[jam, 2014-06-20 17:50:17]

On 2014/06/20 17:22:23, Andrew Hayden wrote:
> OK, I think this is finally, FINALLY complete. All the unit tests and browser
> tests are working in all 3 modes - static, standalone and component; the CLD
> stuff is cleanly and thoroughly componentized, and gyp is happy.
> 
> We're ready to rock.  File assignments below, distilled from git cl owners:
> 
> palmer@:
> ipc/ipc_message_start.h.
> 
> toyoshim@:
>   chrome/browser/translate/chrome_translate_client.cc
>   chrome/browser/translate/chrome_translate_client.h
>   chrome/browser/translate/scoped_cld_component_data_harness.cc
>   chrome/browser/translate/scoped_cld_component_data_harness.h
>   chrome/browser/translate/scoped_cld_data_harness.cc
>   chrome/browser/translate/scoped_cld_data_harness.h
>   chrome/browser/translate/scoped_cld_standalone_data_harness.cc
>   chrome/browser/translate/scoped_cld_standalone_data_harness.h
>   chrome/browser/translate/scoped_cld_static_data_harness.cc
>   chrome/browser/translate/scoped_cld_static_data_harness.h
>   chrome/browser/translate/translate_browser_test_utils.cc
>   chrome/browser/translate/translate_browser_test_utils.h
>   chrome/browser/translate/translate_manager_browsertest.cc
>   chrome/renderer/translate/translate_helper.cc
>   chrome/renderer/translate/translate_helper.h
>   components/translate.gypi
>   components/translate/content/browser/browser_cld_data_provider.h
>   components/translate/content/browser/data_file_browser_cld_data_provider.cc
>   components/translate/content/browser/data_file_browser_cld_data_provider.h
>   components/translate/content/browser/static_browser_cld_data_provider.cc
>   components/translate/content/browser/static_browser_cld_data_provider.h
>   components/translate/content/common/data_file_cld_data_provider_messages.cc
>   components/translate/content/common/data_file_cld_data_provider_messages.h
>   components/translate/content/common/translate_messages.h
>   components/translate/content/renderer/DEPS
>  
components/translate/content/renderer/data_file_renderer_cld_data_provider.cc
>   components/translate/content/renderer/data_file_renderer_cld_data_provider.h
>   components/translate/content/renderer/renderer_cld_data_provider.h
>   components/translate/content/renderer/static_renderer_cld_data_provider.cc
>   components/translate/content/renderer/static_renderer_cld_data_provider.h
>  
components/translate/content/test/fake_data_file_browser_cld_data_provider.cc
>   components/translate/content/test/fake_data_file_browser_cld_data_provider.h
> 
> jam@: (and anything else you want to look at, but you deferred to toyoshim@ et
> al for "*translate*" in email)
>   chrome/browser/chrome_browser_main.cc
>   chrome/browser/component_updater/cld_component_installer.cc
>   chrome/browser/component_updater/cld_component_installer.h
>   chrome/browser/component_updater/test/cld_component_installer_unittest.cc
>   chrome/browser/policy/policy_browsertest.cc
>   chrome/browser/ui/browser_browsertest.cc
>   chrome/browser/ui/views/translate/translate_bubble_view_browsertest.cc
>   chrome/common/chrome_constants.cc
>   chrome/common/chrome_constants.h
>   chrome/renderer/DEPS

lgtm for these files

please add a helpful cl description that describes the motivation and an
overview of the design changes.

[palmer, 2014-06-20 18:00:17]

IPC security LGTM.

[Andrew Hayden (chromium.org), 2014-06-20 18:14:51]

On 2014/06/20 17:50:17, jam wrote:
> lgtm for these files
> 
> please add a helpful cl description that describes the motivation and an
> overview of the design changes.

There's a longwinded version in the linked bug. I will try to distill something
more concise.

[Andrew Hayden (chromium.org), 2014-06-20 18:34:22]

Next up, unit_tests apparently builds differently on android than it does on
linux, leading to duplicate symbol definitions from the "fakes" in
components/translate/content/test. I'm trying to figure out why it builds ok on
linux but not on Android.

[droger, 2014-06-20 19:32:56]

On 2014/06/20 18:34:22, Andrew Hayden wrote:
> Next up, unit_tests apparently builds differently on android than it does on
> linux, leading to duplicate symbol definitions from the "fakes" in
> components/translate/content/test. I'm trying to figure out why it builds ok
on
> linux but not on Android.

I think I know what happens with android. 
There is an infobar_android file in the component which defines the constant and
probably duplicates the symbols.
I think the solution is to move this file back to chrome, like other platforms
do.

[Andrew Hayden (chromium.org), 2014-06-20 19:47:37]

On 2014/06/20 19:32:56, droger wrote:
> I think I know what happens with android. 
> There is an infobar_android file in the component which defines the constant
and
> probably duplicates the symbols.
> I think the solution is to move this file back to chrome, like other platforms
> do.

Actually I think I was just being stupid. I unconditionally included the
cld_component_installer_unittest.cc file, but that shouldn't be: it should only
be included in a build where CLD's data is being provided via the component
updater, otherwise it doesn't make sense to have it because it relies upon
methods in the data_file_browser_cld_data_provider implementation. We could
guard that part with and ifdef as an alternative. This is new behavior that
results from the inversion of control, having the cld_component_installer make
the call to configure the cld_data_provider. Previously we could run the unit
test all the time because the component installer was passive, and didn't depend
upon the translate stuff at all. That's no longer the case.

I think what was happening here was something like this:
If the cld_data_source was static, as it was for the trybots, there's an attempt
to use a static function that is not defined. Thus, error. Trying to include the
'fake' added another problem: duplicate symbol definition in both
fake_data_file_browser_cld_data_provider.h and the original, which both defined
the same class. Whoops. The linux compiler didn't complain about that one
(probably because they were similar "enough"), but the android toolchain did.
Thus, also error.

We can't use a fake after all, because this static method must be declared
exactly once. There's no way to get the components_translate_browser.a file to
build without the provider implementation while still getting it from the
unit_tests side. The workaround would be to create a stubbable "configurator"
class that could be set in the cld_component_installer whose job is to glue the
cld_component_installer impl to the cld_data_provider impl. It looks like such a
class would just be useless complexity, since there don't seem to be serious
problems including the real data_file_browser_cld_data_provider implementation.
Yuck. If you don't follow that, don't worry - it's taken me two hours to wrap my
head around it.

If you feel strongly about stubbing the "glue" between the component installer
and the data provider, I'm willing to write it - I'm just not sure it's
necessary. Conditionally including the unit test feels a little hacky, but I
don't have a better way other than the glue, which feels worse than
conditionally enabling. The unit test will run any time that someone tries to
enable the component updater logic in a build, so it will still serve its
purpose as a gate; it just won't catch breakage earlier than someone trying to
turn it on, which is unfortunate.

Anyhow, this patchset links on both Android and Linux. Crossing fingers for
bots, while I run through the monstrous process of rebuilding all six combos
(Android|Linux, standalone|static|component) on my local machine.

[Andrew Hayden (chromium.org), 2014-06-20 21:30:16]

I've updated the patch description. jam@: is this more to your liking?

[Andrew Hayden (chromium.org), 2014-06-21 07:40:28]

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
File chrome/renderer/translate/translate_helper.cc (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
chrome/renderer/translate/translate_helper.cc:101: // TODO(andrewhayden):
Refactor translate_manager.cc's IsTranslatableURL to
This is biting us: the chromeos bots are failing with this message:
error: 'file_manager' has not been declared

I'll add a temporary workaround for now, but as soon as this refactor is landed
we should get this take care of.

[droger, 2014-06-23 08:31:31]

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
File chrome/renderer/translate/translate_helper.cc (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
chrome/renderer/translate/translate_helper.cc:101: // TODO(andrewhayden):
Refactor translate_manager.cc's IsTranslatableURL to
On 2014/06/21 07:40:28, Andrew Hayden wrote:
> This is biting us: the chromeos bots are failing with this message:
> error: 'file_manager' has not been declared
> 
> I'll add a temporary workaround for now, but as soon as this refactor is
landed
> we should get this take care of.

I don't think this can go in components/translate/core/:
- it depends on content/, and core/ cannot depend on content/
- I don't know if components can depend on extensions, but in any case core/
cannot.

If you want to move it I'd think this should be better in
chrome/common/translate/, or maybe in components/translate/content/common, if
it's ok to depend on extensions there.

[Andrew Hayden (chromium.org), 2014-06-23 08:48:38]

On 2014/06/23 08:31:31, droger wrote:
> I don't think this can go in components/translate/core/:
> - it depends on content/, and core/ cannot depend on content/
> - I don't know if components can depend on extensions, but in any case core/
> cannot.
> 
> If you want to move it I'd think this should be better in
> chrome/common/translate/, or maybe in components/translate/content/common, if
> it's ok to depend on extensions there.

Thanks! I spoke with Takashi about this offline. We think we can get away with
just not translating extensions. I've uploaded patchset 19 which doesn't have
the special case for chromeos, and just doesn't bother preparing translations
for extensions. I think this should be ok; if not we can add the #ifdef back.

[Takashi Toyoshima, 2014-06-23 08:50:02]

I didn't finish reviewing on files under components, but I'd publish it.

https://codereview.chromium.org/333603002/diff/360001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/333603002/diff/360001/build/common.gypi#newco...
build/common.gypi:2694: 'defines': ['CLD2_DYNAMIC_MODE'],
Would it be safe to move this CLD2_DYNAMIC_MODE definition inside cld_2.gyp?

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

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:184: 
Maybe here is a better place to insert #if defined(CLD_DATA_FROM_COMPONENT)
block.
Here ifdef macros are almost ordered by dictionary order except for OS_*.

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

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:32:
base::LazyInstance<base::Lock> cld_file_lock = LAZY_INSTANCE_INITIALIZER;
These two LazyInstance objects looks not needed. See my comment on
SetLatestCldDataFile() below.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:34: const
base::FilePath::CharType kCLDDataFilename[] =
You are changing other CLD* to Cld*, so kCldDataFilename looks better to be
consistent with others.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:125: cld_file.Get()
= path;
Do you really need to have a copy of path inside this class? It seems that
|cld_file| is not used in any other places.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:131:
CldComponentInstallerTraits::GetCldDataFileName() {
no two spaces.

Recently, another reviewer tell me 'git cl format' that automatically fixes
style nits. It's really useful.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
File chrome/browser/component_updater/cld_component_installer.h (left):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.h:64: // Returns the
path to the latest CLD data file into the specified path object,
It would be better to keep this original comment even if the new place is for
private methods.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
File chrome/browser/component_updater/test/cld_component_installer_unittest.cc
(left):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:46:
ASSERT_TRUE(component_updater::GetLatestCldDataFile().empty());
I'm not sure it is worth doing, but this check can be moved inside SetUp() just
before calling SetLatestCldDataFile().

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
File chrome/browser/component_updater/test/cld_component_installer_unittest.cc
(right):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
Now this file is compiled only when cld2_data_source is "component".
If your module is independent from others enough, you may be able to run these
unit tests even with other configurations. Is it difficult? If possible, it
would be helpful to avoid someone mistakenly breaks "component" mode.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:44: 
following members can be protected.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:45:
const base::FilePath::CharType* cld_data_file_name;
cld_data_file_name_

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:47:
component_updater::CldComponentInstallerTraits traits;
traits_

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:48: };
DISALLOW_...

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:67:
const base::FilePath data_file =
no line break

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
File chrome/browser/translate/chrome_translate_client.cc (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.cc:58: // If you change this,
also update translate_browser_test_utils.cc accordingly!
Same TODO for cld_component_installer.cc and
scoped_cld_standalone_data_harness.cc?

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.cc:59: const
base::FilePath::CharType kCLDDataFilename[] =
kCldDataFilename would be better name.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.cc:61: bool
s_cld_file_path_initialized_ = false;
g_cld_file_path_initialized. Also empty line before this declaration will
improve readability.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.cc:62: void InitCldFilePath() {
empty line is needed before this function.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
File chrome/browser/translate/scoped_cld_component_data_harness.cc (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_component_data_harness.cc:18: // This comes
from cld_component_installer.cc
You can remove a following duplicated constant string by calling
CldComponentInstallerTraits::GetCldDataFileName().

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_component_data_harness.cc:96:
scoped_ptr<ScopedCldDataHarness>
no line break here

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
File chrome/browser/translate/scoped_cld_data_harness.cc (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_data_harness.cc:11: namespace  {
remove one space after 'namespace'

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_data_harness.cc:23:
ScopedCldDataHarness::GetTestDataSourceCrxVersion() {
remove two spaces

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
File chrome/browser/translate/scoped_cld_data_harness.h (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_data_harness.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
I feel now just CldDataHarness is more natural to name this class. WDYT? Ditto
on other derived classes.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_data_harness.h:57: protected:
Can you add some useful comments on following methods?
The first method name implies that it is for a derived class, and implementation
of the second method contains "cld2_component". It is a little confusing at a
glance.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
File chrome/browser/translate/scoped_cld_standalone_data_harness.cc (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_standalone_data_harness.cc:73: << " to " <<
target_file.value();
The first '<<' should be aligned with the first '<<' in the previous line.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_standalone_data_harness.cc:78:
scoped_ptr<ScopedCldDataHarness>
no line break here

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
File chrome/browser/translate/scoped_cld_standalone_data_harness.h (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_standalone_data_harness.h:14:
ScopedCldStandaloneDataHarness()
';' is missing at the end of line

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
File chrome/browser/translate/scoped_cld_static_data_harness.h (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_static_data_harness.h:19: 
remove this empty line

https://codereview.chromium.org/333603002/diff/360001/chrome/common/chrome_co...
File chrome/common/chrome_constants.h (left):

https://codereview.chromium.org/333603002/diff/360001/chrome/common/chrome_co...
chrome/common/chrome_constants.h:62: extern const base::FilePath::CharType
kCLDDataFilename[];
Just a note for future CLs.
Now this CL is too big, +1386 lines, -709 lines, and contains too many files.
Personally speaking, it would be better to split this kind of independent
cleanups into a separate CL for effective reviews.

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
File chrome/renderer/translate/translate_helper.cc (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
chrome/renderer/translate/translate_helper.cc:69: bool cld_callback_set = false;
g_cld_callback_set

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
chrome/renderer/translate/translate_helper.cc:114: if
(url.SchemeIs(extensions::kExtensionScheme) &&
I guess other native extensions should not be translated on Chrome OS, but only
FileManager app is listed now.

Even on other platforms, Chrome Apps does not provide infobar service, and all
features that need infobar already didn't work. Translate feature is one of
them. I think we can stop supporting all extensions::kExtensionScheme.

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
chrome/renderer/translate/translate_helper.cc:377: if (!cld_callback_set) {
Hum.... setting a callback inside this message handler looks a little tricky.
Can you set it just before calling cld_data_provider_->SendCldDataRequest()?

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
chrome/renderer/translate/translate_helper.cc:587: // Else, make an asynchronous
request to get the data we need.
As a commented above, you may be able to set the callback here. It would be
readable for me.

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
File chrome/renderer/translate/translate_helper.h (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
chrome/renderer/translate/translate_helper.h:164: // Do not ask for CLD data any
more.
Now that following methods and members are free from the ifdef block, we may
want to follow the C++ style guide. It means that methods would be better to be
defined before variables.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

https://codereview.chromium.org/333603002/diff/360001/components/translate/co...
File components/translate/content/browser/data_file_browser_cld_data_provider.cc
(right):

https://codereview.chromium.org/333603002/diff/360001/components/translate/co...
components/translate/content/browser/data_file_browser_cld_data_provider.cc:25:
// The data file,  cached as long as the process stays alive.
wrong indent (namespace does not require two spaces indent)

https://codereview.chromium.org/333603002/diff/360001/components/translate/co...
components/translate/content/browser/data_file_browser_cld_data_provider.cc:26:
// We also track the offset at which the data starts, and its length.
It would be better to name following variables as g_foo_bar.

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

https://codereview.chromium.org/333603002/diff/360001/components/translate/co...
components/translate/content/browser/data_file_browser_cld_data_provider.h:47:
weak_pointer_factory_;
four spaces indent (two more spaces)

[Takashi Toyoshima, 2014-06-23 09:22:58]

Comments for components/

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

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
components/translate/content/browser/data_file_browser_cld_data_provider.h:45:
content::RenderViewHost* render_view_host;
render_view_host_

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
File
components/translate/content/renderer/data_file_renderer_cld_data_provider.cc
(right):

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
components/translate/content/renderer/data_file_renderer_cld_data_provider.cc:103:
|| data_length > max_int32) { // max signed 32 bit integer
'||' should be placed at the end of previous line.

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
components/translate/content/renderer/data_file_renderer_cld_data_provider.cc:105:
<< data_offset << ", data_length=" << data_length
The first '<<' in each line should be aligned between line 104:106.

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
File
components/translate/content/renderer/data_file_renderer_cld_data_provider.h
(right):

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:33:
const uint64 data_offset,
indent nits on line 33 and 34.

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:35:
content::RenderViewObserver* render_view_observer;
render_view_observer_

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:36:
base::Callback<void(void)> cld_available_callback;
cld_available_callback_

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:37:
// A struct that contains the pointer to the CLD mmap. Used so that we can
Following definitions can be moved to .cc file in anonymous namespace.
Also, do you really need to define struct CLDMmapWrapper? It looks just a
base::MemoryMappedFile*.

[Andrew Hayden (chromium.org), 2014-06-23 13:20:36]

Uploaded patchset.

https://codereview.chromium.org/333603002/diff/360001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/333603002/diff/360001/build/common.gypi#newco...
build/common.gypi:2694: 'defines': ['CLD2_DYNAMIC_MODE'],
On 2014/06/23 08:49:59, Takashi Toyoshima (chromium) wrote:
> Would it be safe to move this CLD2_DYNAMIC_MODE definition inside cld_2.gyp?

Almost, but not quite. The forward declarations of "loadDataFromRawAddress" and
"isDataLoaded" methods in CLD2 header files are guarded by the preprocessor
check, and these files are included in both the cld_2 compile as well as the
translate component compile. The linker complains that it can't locate the
symbols if the define is only done in cld_2.

To fix this we'd have to expose the methods in CLD2 directly. Not necessarily a
bad idea - we could have the ifdefs inside the methods instead, and declare them
unconditionally. I'll ping Dick Sites about this, but let's skip this change for
now unless you strongly object. Agree it would be nicer to do it that way.

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

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:184: 
On 2014/06/23 08:49:59, Takashi Toyoshima (chromium) wrote:
> Maybe here is a better place to insert #if defined(CLD_DATA_FROM_COMPONENT)
> block.
> Here ifdef macros are almost ordered by dictionary order except for OS_*.

Done.

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

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:32:
base::LazyInstance<base::Lock> cld_file_lock = LAZY_INSTANCE_INITIALIZER;
On 2014/06/23 08:49:59, Takashi Toyoshima (chromium) wrote:
> These two LazyInstance objects looks not needed. See my comment on
> SetLatestCldDataFile() below.

Yes, you're right. Now that this class configures the CLD data provider, we no
longer need to hold state here.

I've removed the variables and the lock, as well as the getter method.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:34: const
base::FilePath::CharType kCLDDataFilename[] =
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> You are changing other CLD* to Cld*, so kCldDataFilename looks better to be
> consistent with others.

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:125: cld_file.Get()
= path;
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> Do you really need to have a copy of path inside this class? It seems that
> |cld_file| is not used in any other places.

Good catch, as described above I've removed all of this leftover logic.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:131:
CldComponentInstallerTraits::GetCldDataFileName() {
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> no two spaces.
> 
> Recently, another reviewer tell me 'git cl format' that automatically fixes
> style nits. It's really useful.

Yeah, I always forget this. I'll do a final formatting pass.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
File chrome/browser/component_updater/cld_component_installer.h (left):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.h:64: // Returns the
path to the latest CLD data file into the specified path object,
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> It would be better to keep this original comment even if the new place is for
> private methods.

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
File chrome/browser/component_updater/test/cld_component_installer_unittest.cc
(left):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:46:
ASSERT_TRUE(component_updater::GetLatestCldDataFile().empty());
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> I'm not sure it is worth doing, but this check can be moved inside SetUp()
just
> before calling SetLatestCldDataFile().

Added equivalent check to setup.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
File chrome/browser/component_updater/test/cld_component_installer_unittest.cc
(right):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> Now this file is compiled only when cld2_data_source is "component".
> If your module is independent from others enough, you may be able to run these
> unit tests even with other configurations. Is it difficult? If possible, it
> would be helpful to avoid someone mistakenly breaks "component" mode.

It's not independent anymore. Now that we've inverted control and made the
component installer responsible for configuring the code in
components/translate, there's no simple way to have this be isolated from that
code. We could add fakes or mocks and stub out functionality in the component
installer, but I feel like the ROI there isn't worth it. We should strive to get
the component updater stuff into all the desktop platforms instead, I think,
which will get all of dynamic mode turned on in many bot pipelines.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:44: 
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> following members can be protected.

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:45:
const base::FilePath::CharType* cld_data_file_name;
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> cld_data_file_name_

Now that I've removed the getter from cld_component_installer, I've just made
this a constant.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:47:
component_updater::CldComponentInstallerTraits traits;
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> traits_

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:48: };
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> DISALLOW_...

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:67:
const base::FilePath data_file =
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> no line break

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
File chrome/browser/translate/chrome_translate_client.cc (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.cc:58: // If you change this,
also update translate_browser_test_utils.cc accordingly!
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> Same TODO for cld_component_installer.cc and
> scoped_cld_standalone_data_harness.cc?

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.cc:59: const
base::FilePath::CharType kCLDDataFilename[] =
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> kCldDataFilename would be better name.

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.cc:61: bool
s_cld_file_path_initialized_ = false;
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> g_cld_file_path_initialized. Also empty line before this declaration will
> improve readability.

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.cc:62: void InitCldFilePath() {
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> empty line is needed before this function.

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
File chrome/browser/translate/scoped_cld_component_data_harness.cc (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_component_data_harness.cc:18: // This comes
from cld_component_installer.cc
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> You can remove a following duplicated constant string by calling
> CldComponentInstallerTraits::GetCldDataFileName().

I've removed that method completely, as it was leftover from before we inverted
control to have the component installer configure the data source. So, still
needed. Comment updated a bit.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_component_data_harness.cc:96:
scoped_ptr<ScopedCldDataHarness>
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> no line break here

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
File chrome/browser/translate/scoped_cld_data_harness.cc (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_data_harness.cc:11: namespace  {
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> remove one space after 'namespace'

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_data_harness.cc:23:
ScopedCldDataHarness::GetTestDataSourceCrxVersion() {
On 2014/06/23 08:50:00, Takashi Toyoshima (chromium) wrote:
> remove two spaces

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
File chrome/browser/translate/scoped_cld_data_harness.h (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_data_harness.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2014/06/23 08:50:01, Takashi Toyoshima (chromium) wrote:
> I feel now just CldDataHarness is more natural to name this class. WDYT? Ditto
> on other derived classes.

I like the word "scoped" because it alerts callers to the fact that this object
has a nontrivial destructor, but I suppose "harness" conveys something similar.
I'll rename, but in a second patchset for cleanliness.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_data_harness.h:57: protected:
On 2014/06/23 08:50:01, Takashi Toyoshima (chromium) wrote:
> Can you add some useful comments on following methods?
> The first method name implies that it is for a derived class, and
implementation
> of the second method contains "cld2_component". It is a little confusing at a
> glance.

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
File chrome/browser/translate/scoped_cld_standalone_data_harness.cc (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_standalone_data_harness.cc:73: << " to " <<
target_file.value();
On 2014/06/23 08:50:01, Takashi Toyoshima (chromium) wrote:
> The first '<<' should be aligned with the first '<<' in the previous line.

Sorry, this is due to refactoring DLOG(INFO) -> VLOG(1).

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_standalone_data_harness.cc:78:
scoped_ptr<ScopedCldDataHarness>
On 2014/06/23 08:50:01, Takashi Toyoshima (chromium) wrote:
> no line break here

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
File chrome/browser/translate/scoped_cld_standalone_data_harness.h (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_standalone_data_harness.h:14:
ScopedCldStandaloneDataHarness()
On 2014/06/23 08:50:01, Takashi Toyoshima (chromium) wrote:
> ';' is missing at the end of line
I must have missed this when I was making the compilers happy for whatever
platform demanded the declaration of this constructor. Sorry! Determining
correctness across all files on all configs is error-prone because of the need
to twiddle the data source and then build for multiple platforms with multiple
configs. This slipped by me somehow.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
File chrome/browser/translate/scoped_cld_static_data_harness.h (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/translat...
chrome/browser/translate/scoped_cld_static_data_harness.h:19: 
On 2014/06/23 08:50:01, Takashi Toyoshima (chromium) wrote:
> remove this empty line

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/common/chrome_co...
File chrome/common/chrome_constants.h (left):

https://codereview.chromium.org/333603002/diff/360001/chrome/common/chrome_co...
chrome/common/chrome_constants.h:62: extern const base::FilePath::CharType
kCLDDataFilename[];
On 2014/06/23 08:50:01, Takashi Toyoshima (chromium) wrote:
> Just a note for future CLs.
> Now this CL is too big, +1386 lines, -709 lines, and contains too many files.
> Personally speaking, it would be better to split this kind of independent
> cleanups into a separate CL for effective reviews.

It has become a very large code review because of the expansion into unit tests
and browser tests, which both required nontrivial changes and bloated the review
significantly. Unfortunately there isn't a good way around it, since we need the
unit tests and browser tests functioning in-sync with the code.

I think the real size issue here comes from the fact that we ended up putting
all the code into /components/translate at the same time as refactoring the
access pattern. That's made a lot of changes necessary, like inverting control
in the cld_component_installer and slurping things out of /chrome/common into
/components/translate/content/common.

I do try to keep the size down, but in this case I'm not sure what we could have
done to really decrease the complexity :(

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
File chrome/renderer/translate/translate_helper.cc (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
chrome/renderer/translate/translate_helper.cc:69: bool cld_callback_set = false;
On 2014/06/23 08:50:01, Takashi Toyoshima (chromium) wrote:
> g_cld_callback_set

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
chrome/renderer/translate/translate_helper.cc:101: // TODO(andrewhayden):
Refactor translate_manager.cc's IsTranslatableURL to
On 2014/06/23 08:31:31, droger wrote:
> On 2014/06/21 07:40:28, Andrew Hayden wrote:
> > This is biting us: the chromeos bots are failing with this message:
> > error: 'file_manager' has not been declared
> > 
> > I'll add a temporary workaround for now, but as soon as this refactor is
> landed
> > we should get this take care of.
> 
> I don't think this can go in components/translate/core/:
> - it depends on content/, and core/ cannot depend on content/
> - I don't know if components can depend on extensions, but in any case core/
> cannot.
> 
> If you want to move it I'd think this should be better in
> chrome/common/translate/, or maybe in components/translate/content/common, if
> it's ok to depend on extensions there.

Talked with Takashi about this, see below.

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
chrome/renderer/translate/translate_helper.cc:114: if
(url.SchemeIs(extensions::kExtensionScheme) &&
On 2014/06/23 08:50:01, Takashi Toyoshima (chromium) wrote:
> I guess other native extensions should not be translated on Chrome OS, but
only
> FileManager app is listed now.
> 
> Even on other platforms, Chrome Apps does not provide infobar service, and all
> features that need infobar already didn't work. Translate feature is one of
> them. I think we can stop supporting all extensions::kExtensionScheme.

Agreed after offline discussion, I've removed the offending bits.

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
chrome/renderer/translate/translate_helper.cc:377: if (!cld_callback_set) {
On 2014/06/23 08:50:01, Takashi Toyoshima (chromium) wrote:
> Hum.... setting a callback inside this message handler looks a little tricky.
> Can you set it just before calling cld_data_provider_->SendCldDataRequest()?

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
chrome/renderer/translate/translate_helper.cc:587: // Else, make an asynchronous
request to get the data we need.
On 2014/06/23 08:50:01, Takashi Toyoshima (chromium) wrote:
> As a commented above, you may be able to set the callback here. It would be
> readable for me.

Done.

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
File chrome/renderer/translate/translate_helper.h (right):

https://codereview.chromium.org/333603002/diff/360001/chrome/renderer/transla...
chrome/renderer/translate/translate_helper.h:164: // Do not ask for CLD data any
more.
On 2014/06/23 08:50:01, Takashi Toyoshima (chromium) wrote:
> Now that following methods and members are free from the ifdef block, we may
> want to follow the C++ style guide. It means that methods would be better to
be
> defined before variables.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Yes, good catch. Done. Die, ifdefs, die!

https://codereview.chromium.org/333603002/diff/360001/components/translate/co...
File components/translate/content/browser/data_file_browser_cld_data_provider.cc
(right):

https://codereview.chromium.org/333603002/diff/360001/components/translate/co...
components/translate/content/browser/data_file_browser_cld_data_provider.cc:25:
// The data file,  cached as long as the process stays alive.
On 2014/06/23 08:50:01, Takashi Toyoshima (chromium) wrote:
> wrong indent (namespace does not require two spaces indent)

Done.

https://codereview.chromium.org/333603002/diff/360001/components/translate/co...
components/translate/content/browser/data_file_browser_cld_data_provider.cc:26:
// We also track the offset at which the data starts, and its length.
On 2014/06/23 08:50:01, Takashi Toyoshima (chromium) wrote:
> It would be better to name following variables as g_foo_bar.

Done.

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

https://codereview.chromium.org/333603002/diff/360001/components/translate/co...
components/translate/content/browser/data_file_browser_cld_data_provider.h:47:
weak_pointer_factory_;
On 2014/06/23 08:50:01, Takashi Toyoshima (chromium) wrote:
> four spaces indent (two more spaces)

Done.

[Andrew Hayden (chromium.org), 2014-06-23 13:54:43]

The last bits of toyohsim@'s non-rename-related comments addressed.

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

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
components/translate/content/browser/data_file_browser_cld_data_provider.h:45:
content::RenderViewHost* render_view_host;
On 2014/06/23 09:22:58, Takashi Toyoshima (chromium) wrote:
> render_view_host_

Done.

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
File
components/translate/content/renderer/data_file_renderer_cld_data_provider.cc
(right):

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
components/translate/content/renderer/data_file_renderer_cld_data_provider.cc:103:
|| data_length > max_int32) { // max signed 32 bit integer
On 2014/06/23 09:22:58, Takashi Toyoshima (chromium) wrote:
> '||' should be placed at the end of previous line.

Done.

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
components/translate/content/renderer/data_file_renderer_cld_data_provider.cc:105:
<< data_offset << ", data_length=" << data_length
On 2014/06/23 09:22:58, Takashi Toyoshima (chromium) wrote:
> The first '<<' in each line should be aligned between line 104:106.

Done.

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
File
components/translate/content/renderer/data_file_renderer_cld_data_provider.h
(right):

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:33:
const uint64 data_offset,
On 2014/06/23 09:22:58, Takashi Toyoshima (chromium) wrote:
> indent nits on line 33 and 34.

Done.

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:35:
content::RenderViewObserver* render_view_observer;
On 2014/06/23 09:22:58, Takashi Toyoshima (chromium) wrote:
> render_view_observer_

Done.

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:36:
base::Callback<void(void)> cld_available_callback;
On 2014/06/23 09:22:58, Takashi Toyoshima (chromium) wrote:
> cld_available_callback_

Done.

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:37:
// A struct that contains the pointer to the CLD mmap. Used so that we can
On 2014/06/23 09:22:58, Takashi Toyoshima (chromium) wrote:
> Following definitions can be moved to .cc file in anonymous namespace.
> Also, do you really need to define struct CLDMmapWrapper? It looks just a
> base::MemoryMappedFile*.

LazyInstance::Leaky is necessary to avoid having the code get detected as a
memory leak. The pattern with the holder class is what I *think* the standard
use case is, and is what we have been using up till now anyways. Is there a
different pattern you'd like to see here?

[Andrew Hayden (chromium.org), 2014-06-23 14:39:54]

All comments, including renames and git cl format, are now addressed.

[Takashi Toyoshima, 2014-06-24 02:09:48]

lgtm with one nit pick.
Thank you for great works.

https://codereview.chromium.org/333603002/diff/360001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/333603002/diff/360001/build/common.gypi#newco...
build/common.gypi:2694: 'defines': ['CLD2_DYNAMIC_MODE'],
OK. Current approach sounds reasonable.

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
File chrome/browser/component_updater/test/cld_component_installer_unittest.cc
(right):

https://codereview.chromium.org/333603002/diff/360001/chrome/browser/componen...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
I see.

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
File
components/translate/content/renderer/data_file_renderer_cld_data_provider.h
(right):

https://codereview.chromium.org/333603002/diff/380001/components/translate/co...
components/translate/content/renderer/data_file_renderer_cld_data_provider.h:37:
// A struct that contains the pointer to the CLD mmap. Used so that we can
Ah, I see. Thank you for your explanation!

https://codereview.chromium.org/333603002/diff/460001/components/translate/co...
File components/translate/content/browser/data_file_browser_cld_data_provider.cc
(right):

https://codereview.chromium.org/333603002/diff/460001/components/translate/co...
components/translate/content/browser/data_file_browser_cld_data_provider.cc:33:
base::LazyInstance<base::Lock> s_file_lock_;
g_file_lock?
comments should be updated. ditto on other four variables above.

[Andrew Hayden (chromium.org), 2014-06-24 09:20:26]

All nits addressed, time to CQ.

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

The CQ bit was checked by andrewhayden@chromium.org

[commit-bot: I haz the power, 2014-06-24 09:22:42]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/333603002/53...

[Andrew Hayden (chromium.org), 2014-06-24 09:27:46]

The CQ bit was unchecked by andrewhayden@chromium.org

[Andrew Hayden (chromium.org), 2014-06-24 09:30:23]

On 2014/06/24 09:27:46, Andrew Hayden wrote:
> The CQ bit was unchecked by mailto:andrewhayden@chromium.org

Patch failed to apply. Rebased, resubmitting.

[Andrew Hayden (chromium.org), 2014-06-24 09:30:39]

The CQ bit was checked by andrewhayden@chromium.org

[commit-bot: I haz the power, 2014-06-24 09:31:45]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/333603002/55...

[commit-bot: I haz the power, 2014-06-24 10:27:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/20773)

[commit-bot: I haz the power, 2014-06-24 10:29:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-24 10:29:57]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/20793)

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

Merge conflict from: https://codereview.chromium.org/326383005. I'm working on a
pinned revision, let me sync against tip of tree and get this moving.

[Andrew Hayden (chromium.org), 2014-06-24 11:09:25]

The CQ bit was checked by andrewhayden@chromium.org

[commit-bot: I haz the power, 2014-06-24 11:11:03]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/333603002/57...

[commit-bot: I haz the power, 2014-06-24 14:09:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-24 15:14:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-24 15:14:57]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Andrew Hayden (chromium.org), 2014-06-24 15:33:37]

The CQ bit was checked by andrewhayden@chromium.org

[commit-bot: I haz the power, 2014-06-24 15:34:15]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/333603002/57...

[commit-bot: I haz the power, 2014-06-24 15:54:42]

Change committed as 279415