Move language detection to a component

Move language detection to a component

Language detection is used from the renderer on most platform, but from the
browser on iOS. This CL moves it from chrome/common/ to a new "translate"
component, which allows to track and address more cleanly dependencies issues.

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

[droger, 2013-10-01 17:08:26]

blundell: Could you do an overall pass on the CL and issues around
componentization.

[blundell, 2013-10-02 10:01:15]

Please add a README to components/translate that is similar to the README in
components/webdata and also explains what is going on with language_detection:
that it is conceptually common code but is used only from the renderer on all
platforms except iOS to avoid its dependency on CLD bloating the browser DLL on
Windows.

https://codereview.chromium.org/25531002/diff/46001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/25531002/diff/46001/build/all.gyp#newcode15
build/all.gyp:15: '../components/components.gyp:*',
As discussed offline, I think that this change would be best off in its own
follow-up CL.

https://codereview.chromium.org/25531002/diff/46001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/25531002/diff/46001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:1457:
translate::kTranslateSecurityOrigin,
Hmm, now that I look at this, I think that this should follow the existing
pattern (see autofill::switches:: and extensions::switches:: above), and I can
send an email to chromium-dev to see which pattern people like better and then
change the pattern for all of them at once if that's the way we decide.

https://codereview.chromium.org/25531002/diff/46001/chrome/browser/ui/webui/t...
File chrome/browser/ui/webui/translate_internals/translate_internals_ui.cc
(right):

https://codereview.chromium.org/25531002/diff/46001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_ui.cc:64:
std::string cld_version = translate::GetCLDVersion();
Add a TODO to replace this call via the hack that jamesr@ suggested?

https://codereview.chromium.org/25531002/diff/46001/chrome/chrome_renderer.gypi
File chrome/chrome_renderer.gypi (left):

https://codereview.chromium.org/25531002/diff/46001/chrome/chrome_renderer.gy...
chrome/chrome_renderer.gypi:408: ['enable_printing==0', {
Do you know what this diff is?

https://codereview.chromium.org/25531002/diff/46001/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/25531002/diff/46001/components/components.gyp...
components/components.gyp:23: 'translate.gypi',
Same comment about moving this to a follow-up CL.

https://codereview.chromium.org/25531002/diff/46001/components/components_tes...
File components/components_tests.gypi (right):

https://codereview.chromium.org/25531002/diff/46001/components/components_tes...
components/components_tests.gypi:8: 'target_name': 'components_unittests',
Same comment.

https://codereview.chromium.org/25531002/diff/46001/components/translate/comm...
File components/translate/common/translate_metrics.cc (right):

https://codereview.chromium.org/25531002/diff/46001/components/translate/comm...
components/translate/common/translate_metrics.cc:67: translate::LANGUAGE_MAX);
nit: you can get rid of the usages of translate:: when you're in the translate
namespace.

https://codereview.chromium.org/25531002/diff/46001/components/translate/lang...
File components/translate/language_detection/language_detection_util.cc (right):

https://codereview.chromium.org/25531002/diff/46001/components/translate/lang...
components/translate/language_detection/language_detection_util.cc:55: //
chrome/common/translate, and implement them as std::set<>.
Path reference is outdated.

https://codereview.chromium.org/25531002/diff/23052/components/translate/lang...
File components/translate/language_detection/DEPS (right):

https://codereview.chromium.org/25531002/diff/23052/components/translate/lang...
components/translate/language_detection/DEPS:3: "-content",
This should be moved down one level, as at this point it's true of the entire
translate component.

https://codereview.chromium.org/25531002/diff/24001/components/translate/comm...
File components/translate/common/translate_util.h (right):

https://codereview.chromium.org/25531002/diff/24001/components/translate/comm...
components/translate/common/translate_util.h:21: void
ToChromeLanguageSynonym(std::string* language);
Given that this code is now in a component, it would make more sense to me to
have this function named something like |ToChromiumLanguageSynonym| at this
point. But I'm not a domain expert; maybe one of the translate OWNERs can
advise.

[blundell, 2013-10-02 10:01:17]

Please add a README to components/translate that is similar to the README in
components/webdata and also explains what is going on with language_detection:
that it is conceptually common code but is used only from the renderer on all
platforms except iOS to avoid its dependency on CLD bloating the browser DLL on
Windows.

https://codereview.chromium.org/25531002/diff/46001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/25531002/diff/46001/build/all.gyp#newcode15
build/all.gyp:15: '../components/components.gyp:*',
As discussed offline, I think that this change would be best off in its own
follow-up CL.

https://codereview.chromium.org/25531002/diff/46001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/25531002/diff/46001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:1457:
translate::kTranslateSecurityOrigin,
Hmm, now that I look at this, I think that this should follow the existing
pattern (see autofill::switches:: and extensions::switches:: above), and I can
send an email to chromium-dev to see which pattern people like better and then
change the pattern for all of them at once if that's the way we decide.

https://codereview.chromium.org/25531002/diff/46001/chrome/browser/ui/webui/t...
File chrome/browser/ui/webui/translate_internals/translate_internals_ui.cc
(right):

https://codereview.chromium.org/25531002/diff/46001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_ui.cc:64:
std::string cld_version = translate::GetCLDVersion();
Add a TODO to replace this call via the hack that jamesr@ suggested?

https://codereview.chromium.org/25531002/diff/46001/chrome/chrome_renderer.gypi
File chrome/chrome_renderer.gypi (left):

https://codereview.chromium.org/25531002/diff/46001/chrome/chrome_renderer.gy...
chrome/chrome_renderer.gypi:408: ['enable_printing==0', {
Do you know what this diff is?

https://codereview.chromium.org/25531002/diff/46001/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/25531002/diff/46001/components/components.gyp...
components/components.gyp:23: 'translate.gypi',
Same comment about moving this to a follow-up CL.

https://codereview.chromium.org/25531002/diff/46001/components/components_tes...
File components/components_tests.gypi (right):

https://codereview.chromium.org/25531002/diff/46001/components/components_tes...
components/components_tests.gypi:8: 'target_name': 'components_unittests',
Same comment.

https://codereview.chromium.org/25531002/diff/46001/components/translate/comm...
File components/translate/common/translate_metrics.cc (right):

https://codereview.chromium.org/25531002/diff/46001/components/translate/comm...
components/translate/common/translate_metrics.cc:67: translate::LANGUAGE_MAX);
nit: you can get rid of the usages of translate:: when you're in the translate
namespace.

https://codereview.chromium.org/25531002/diff/46001/components/translate/lang...
File components/translate/language_detection/language_detection_util.cc (right):

https://codereview.chromium.org/25531002/diff/46001/components/translate/lang...
components/translate/language_detection/language_detection_util.cc:55: //
chrome/common/translate, and implement them as std::set<>.
Path reference is outdated.

https://codereview.chromium.org/25531002/diff/23052/components/translate/lang...
File components/translate/language_detection/DEPS (right):

https://codereview.chromium.org/25531002/diff/23052/components/translate/lang...
components/translate/language_detection/DEPS:3: "-content",
This should be moved down one level, as at this point it's true of the entire
translate component.

https://codereview.chromium.org/25531002/diff/24001/components/translate/comm...
File components/translate/common/translate_util.h (right):

https://codereview.chromium.org/25531002/diff/24001/components/translate/comm...
components/translate/common/translate_util.h:21: void
ToChromeLanguageSynonym(std::string* language);
Given that this code is now in a component, it would make more sense to me to
have this function named something like |ToChromiumLanguageSynonym| at this
point. But I'm not a domain expert; maybe one of the translate OWNERs can
advise.

[droger, 2013-10-02 10:59:41]

Thanks for the review.

https://codereview.chromium.org/25531002/diff/46001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/25531002/diff/46001/build/all.gyp#newcode15
build/all.gyp:15: '../components/components.gyp:*',
On 2013/10/02 10:01:17, blundell wrote:
> As discussed offline, I think that this change would be best off in its own
> follow-up CL.

Done.

https://codereview.chromium.org/25531002/diff/46001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/25531002/diff/46001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:1457:
translate::kTranslateSecurityOrigin,
On 2013/10/02 10:01:17, blundell wrote:
> Hmm, now that I look at this, I think that this should follow the existing
> pattern (see autofill::switches:: and extensions::switches:: above), and I can
> send an email to chromium-dev to see which pattern people like better and then
> change the pattern for all of them at once if that's the way we decide.

Done.

https://codereview.chromium.org/25531002/diff/46001/chrome/browser/ui/webui/t...
File chrome/browser/ui/webui/translate_internals/translate_internals_ui.cc
(right):

https://codereview.chromium.org/25531002/diff/46001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_ui.cc:64:
std::string cld_version = translate::GetCLDVersion();
On 2013/10/02 10:01:17, blundell wrote:
> Add a TODO to replace this call via the hack that jamesr@ suggested?

Done.

https://codereview.chromium.org/25531002/diff/46001/chrome/chrome_renderer.gypi
File chrome/chrome_renderer.gypi (left):

https://codereview.chromium.org/25531002/diff/46001/chrome/chrome_renderer.gy...
chrome/chrome_renderer.gypi:408: ['enable_printing==0', {
On 2013/10/02 10:01:17, blundell wrote:
> Do you know what this diff is?

There was a tab character.

https://codereview.chromium.org/25531002/diff/46001/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/25531002/diff/46001/components/components.gyp...
components/components.gyp:23: 'translate.gypi',
On 2013/10/02 10:01:17, blundell wrote:
> Same comment about moving this to a follow-up CL.

Done, assuming you're talking about the dom_distiller change.

https://codereview.chromium.org/25531002/diff/46001/components/components_tes...
File components/components_tests.gypi (right):

https://codereview.chromium.org/25531002/diff/46001/components/components_tes...
components/components_tests.gypi:8: 'target_name': 'components_unittests',
On 2013/10/02 10:01:17, blundell wrote:
> Same comment.

Done.

https://codereview.chromium.org/25531002/diff/46001/components/translate/comm...
File components/translate/common/translate_metrics.cc (right):

https://codereview.chromium.org/25531002/diff/46001/components/translate/comm...
components/translate/common/translate_metrics.cc:67: translate::LANGUAGE_MAX);
On 2013/10/02 10:01:17, blundell wrote:
> nit: you can get rid of the usages of translate:: when you're in the translate
> namespace.

Done.

https://codereview.chromium.org/25531002/diff/46001/components/translate/lang...
File components/translate/language_detection/language_detection_util.cc (right):

https://codereview.chromium.org/25531002/diff/46001/components/translate/lang...
components/translate/language_detection/language_detection_util.cc:55: //
chrome/common/translate, and implement them as std::set<>.
On 2013/10/02 10:01:17, blundell wrote:
> Path reference is outdated.
Changed the path to translate/common, but I'm not sure if it's still relevant.

https://codereview.chromium.org/25531002/diff/23052/components/translate/lang...
File components/translate/language_detection/DEPS (right):

https://codereview.chromium.org/25531002/diff/23052/components/translate/lang...
components/translate/language_detection/DEPS:3: "-content",
On 2013/10/02 10:01:17, blundell wrote:
> This should be moved down one level, as at this point it's true of the entire
> translate component.

Done.

[blundell, 2013-10-02 11:02:21]

Looks good with the exception of the question re: ChromeLanguageSynonym that
hopefully the translate owners can provide input on.

https://codereview.chromium.org/25531002/diff/24001/chrome/browser/translate/...
File chrome/browser/translate/translate_browsertest.cc (right):

https://codereview.chromium.org/25531002/diff/24001/chrome/browser/translate/...
chrome/browser/translate/translate_browsertest.cc:59:
command_line->AppendSwitchASCII(translate::switches::kTranslateSecurityOrigin,
Is this > 80 chars now?

[droger, 2013-10-02 11:09:11]

+joi for components
+hajimehoshi for translate

https://codereview.chromium.org/25531002/diff/24001/chrome/browser/translate/...
File chrome/browser/translate/translate_browsertest.cc (right):

https://codereview.chromium.org/25531002/diff/24001/chrome/browser/translate/...
chrome/browser/translate/translate_browsertest.cc:59:
command_line->AppendSwitchASCII(translate::switches::kTranslateSecurityOrigin,
On 2013/10/02 11:02:22, blundell wrote:
> Is this > 80 chars now?
Sorry, fixed.

[droger, 2013-10-02 11:09:33]

+joi for components
+hajimehoshi for translate

[Jói, 2013-10-02 12:12:10]

https://codereview.chromium.org/25531002/diff/76001/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/25531002/diff/76001/components/components.gyp...
components/components.gyp:24: 'translate.gypi',
Please add //components/translate/OWNERS and you can add the same people in
per-file entries for translate.gypi

https://codereview.chromium.org/25531002/diff/76001/components/translate.gypi
File components/translate.gypi (right):

https://codereview.chromium.org/25531002/diff/76001/components/translate.gypi...
components/translate.gypi:1: # Copyright (c) 2013 The Chromium Authors. All
rights reserved.
nit: I don't think we use the (c) any more.

https://codereview.chromium.org/25531002/diff/76001/components/translate/README
File components/translate/README (right):

https://codereview.chromium.org/25531002/diff/76001/components/translate/READ...
components/translate/README:4: - WebData is not allowed to depend on content/,
because it is used by iOS.
WebData? (here and line below)

https://codereview.chromium.org/25531002/diff/76001/components/translate/comm...
File components/translate/common/translate_metrics_unittest.cc (right):

https://codereview.chromium.org/25531002/diff/76001/components/translate/comm...
components/translate/common/translate_metrics_unittest.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
Any chance to get this file move recognized, e.g. --similarity=30 ?  I guess
maybe not because of the namespace change to translate:: but could you check if
you haven't already?

[droger, 2013-10-02 12:33:11]

Thanks for the comments.

https://codereview.chromium.org/25531002/diff/76001/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/25531002/diff/76001/components/components.gyp...
components/components.gyp:24: 'translate.gypi',
On 2013/10/02 12:12:11, Jói wrote:
> Please add //components/translate/OWNERS and you can add the same people in
> per-file entries for translate.gypi

Done.

https://codereview.chromium.org/25531002/diff/76001/components/translate.gypi
File components/translate.gypi (right):

https://codereview.chromium.org/25531002/diff/76001/components/translate.gypi...
components/translate.gypi:1: # Copyright (c) 2013 The Chromium Authors. All
rights reserved.
On 2013/10/02 12:12:11, Jói wrote:
> nit: I don't think we use the (c) any more.

Done.

https://codereview.chromium.org/25531002/diff/76001/components/translate/README
File components/translate/README (right):

https://codereview.chromium.org/25531002/diff/76001/components/translate/READ...
components/translate/README:4: - WebData is not allowed to depend on content/,
because it is used by iOS.
On 2013/10/02 12:12:11, Jói wrote:
> WebData? (here and line below)

Done.

https://codereview.chromium.org/25531002/diff/76001/components/translate/comm...
File components/translate/common/translate_metrics_unittest.cc (right):

https://codereview.chromium.org/25531002/diff/76001/components/translate/comm...
components/translate/common/translate_metrics_unittest.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
On 2013/10/02 12:12:11, Jói wrote:
> Any chance to get this file move recognized, e.g. --similarity=30 ?  I guess
> maybe not because of the namespace change to translate:: but could you check
if
> you haven't already?

It worked!

[Jói, 2013-10-02 12:38:30]

Thanks, //components LGTM.

[hajimehoshi, 2013-10-03 05:13:18]

lgtm

[droger, 2013-10-03 08:56:19]

+jochen as OWNER of chrome/browser and chrome/common

[droger, 2013-10-03 08:56:30]

+jochen as OWNER of chrome/browser and chrome/common

[jochen (gone - plz use gerrit), 2013-10-04 08:16:37]

lgtm

[commit-bot: I haz the power, 2013-10-04 08:39:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/25531002/141001

[commit-bot: I haz the power, 2013-10-04 16:03:13]

Change committed as 227015