Move LanguageUsageMetrics and TranslateBrowserMetrics to components

Move LanguageUsageMetrics and TranslateBrowserMetrics to components

LanguageUsageMetrics is moved to its own component because it is used
both in the translate component and directly in the browser.

TranslateBrowserMetrics is moved to the translate component.

BUG=331509
TBR=jochen

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

[blundell, 2014-01-09 09:53:08]

LGTM with nits. +joi@.

https://codereview.chromium.org/131203002/diff/30001/components/components_te...
File components/components_tests.gyp (right):

https://codereview.chromium.org/131203002/diff/30001/components/components_te...
components/components_tests.gyp:158: ['include', '^auto_login_parser/'],
Can you add the language_usage_metrics unittest here?

https://codereview.chromium.org/131203002/diff/30001/components/language_usag...
File components/language_usage_metrics/language_usage_metrics.h (left):

https://codereview.chromium.org/131203002/diff/30001/components/language_usag...
components/language_usage_metrics/language_usage_metrics.h:14: // Methods to
record language usage as UMA histograms.
This should be in the language_usage_metrics namespace (I know you're holding
off on the translate one until that componentization is complete; no reason to
hold off on this one).

[Jói, 2014-01-09 10:44:14]

LGTM

https://codereview.chromium.org/131203002/diff/30001/components/language_usag...
File components/language_usage_metrics/language_usage_metrics.h (right):

https://codereview.chromium.org/131203002/diff/30001/components/language_usag...
components/language_usage_metrics/language_usage_metrics.h:15: class
LanguageUsageMetrics {
Code in a component located in //components/xyz should be in namespace xyz, so
this should be in namespace language_usage_metrics.

I don't mind if you defer that change to a follow-up, but I would recommend
doing it right after.

https://codereview.chromium.org/131203002/diff/30001/components/translate/cor...
File components/translate/core/browser/translate_browser_metrics.h (right):

https://codereview.chromium.org/131203002/diff/30001/components/translate/cor...
components/translate/core/browser/translate_browser_metrics.h:10: namespace
TranslateBrowserMetrics {
As per my other comment, this code should be in namespace translate (and thus
you could rename this inner namespace from TranslateBrowserMetrics to just
BrowserMetrics) but I don't mind if you do this in a follow-up.

[blundell, 2014-01-09 10:47:04]

https://codereview.chromium.org/131203002/diff/30001/components/translate/cor...
File components/translate/core/browser/translate_browser_metrics.h (right):

https://codereview.chromium.org/131203002/diff/30001/components/translate/cor...
components/translate/core/browser/translate_browser_metrics.h:10: namespace
TranslateBrowserMetrics {
Jói: For iterative refactorings, we were planning on introducing the namespace
at the conclusion of the refactoring in order to avoid churn on files that have
not yet moved into the component but will do so in succeeding CLs. Does that
sound OK to you as a general approach?

On 2014/01/09 10:44:14, Jói wrote:
> As per my other comment, this code should be in namespace translate (and thus
> you could rename this inner namespace from TranslateBrowserMetrics to just
> BrowserMetrics) but I don't mind if you do this in a follow-up.

[Jói, 2014-01-09 10:50:31]

Fine by me.

On Thu, Jan 9, 2014 at 10:47 AM,  <blundell@chromium.org> wrote:
>
>
https://codereview.chromium.org/131203002/diff/30001/components/translate/cor...
> File components/translate/core/browser/translate_browser_metrics.h
> (right):
>
>
https://codereview.chromium.org/131203002/diff/30001/components/translate/cor...
> components/translate/core/browser/translate_browser_metrics.h:10:
> namespace TranslateBrowserMetrics {
> Jói: For iterative refactorings, we were planning on introducing the
> namespace at the conclusion of the refactoring in order to avoid churn
> on files that have not yet moved into the component but will do so in
> succeeding CLs. Does that sound OK to you as a general approach?
>
>
> On 2014/01/09 10:44:14, Jói wrote:
>>
>> As per my other comment, this code should be in namespace translate
>
> (and thus
>>
>> you could rename this inner namespace from TranslateBrowserMetrics to
>
> just
>>
>> BrowserMetrics) but I don't mind if you do this in a follow-up.
>
>
> https://codereview.chromium.org/131203002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[droger, 2014-01-09 11:52:27]

+mad for translate
+tfarina for BUILD files

https://codereview.chromium.org/131203002/diff/30001/components/language_usag...
File components/language_usage_metrics/language_usage_metrics.h (right):

https://codereview.chromium.org/131203002/diff/30001/components/language_usag...
components/language_usage_metrics/language_usage_metrics.h:15: class
LanguageUsageMetrics {
On 2014/01/09 10:44:14, Jói wrote:
> Code in a component located in //components/xyz should be in namespace xyz, so
> this should be in namespace language_usage_metrics.
> 
> I don't mind if you defer that change to a follow-up, but I would recommend
> doing it right after.

Done.

https://codereview.chromium.org/131203002/diff/30001/components/translate/cor...
File components/translate/core/browser/translate_browser_metrics.h (right):

https://codereview.chromium.org/131203002/diff/30001/components/translate/cor...
components/translate/core/browser/translate_browser_metrics.h:10: namespace
TranslateBrowserMetrics {
On 2014/01/09 10:47:04, blundell wrote:
> Jói: For iterative refactorings, we were planning on introducing the namespace
> at the conclusion of the refactoring in order to avoid churn on files that
have
> not yet moved into the component but will do so in succeeding CLs. Does that
> sound OK to you as a general approach?
> 
> On 2014/01/09 10:44:14, Jói wrote:
> > As per my other comment, this code should be in namespace translate (and
thus
> > you could rename this inner namespace from TranslateBrowserMetrics to just
> > BrowserMetrics) but I don't mind if you do this in a follow-up.
> 

As blundell said, I'll do that later. I'm tracking this as
http://crbug.com/331782

[tfarina, 2014-01-09 12:49:25]

BUILD.gn lgtm.

[MAD, 2014-01-09 13:38:32]

LGTM...

[droger, 2014-01-09 13:56:30]

TBR=jochen

[droger, 2014-01-09 13:56:49]

TBR=jochen

[commit-bot: I haz the power, 2014-01-09 13:57:23]

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

[commit-bot: I haz the power, 2014-01-09 13:57:39]

Failed to apply patch for components/components_tests.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file components/components_tests.gyp
  Hunk #2 succeeded at 50 with fuzz 2 (offset 1 line).
  Hunk #3 FAILED at 102.
  Hunk #4 succeeded at 159 with fuzz 2 (offset 5 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
components/components_tests.gyp.rej

Patch:       components/components_tests.gyp
Index: components/components_tests.gyp
diff --git a/components/components_tests.gyp b/components/components_tests.gyp
index
dfa8472ff88d0ff627f61f32f86f76c1578ad449..28f70f0fa07df8171bdd2f3c6f5ca01be0e63c61
100644
--- a/components/components_tests.gyp
+++ b/components/components_tests.gyp
@@ -41,6 +41,7 @@
             'json_schema/json_schema_validator_unittest.cc',
             'json_schema/json_schema_validator_unittest_base.cc',
             'json_schema/json_schema_validator_unittest_base.h',
+            'language_usage_metrics/language_usage_metrics_unittest.cc',
            
'navigation_interception/intercept_navigation_resource_throttle_unittest.cc',
             'precache/content/precache_manager_unittest.cc',
             'precache/core/precache_database_unittest.cc',
@@ -48,6 +49,7 @@
             'precache/core/precache_url_table_unittest.cc',
             'sessions/serialized_navigation_entry_unittest.cc',
             'test/run_all_unittests.cc',
+            'translate/core/browser/translate_browser_metrics_unittest.cc',
             'translate/core/common/translate_metrics_unittest.cc',
             'translate/core/common/translate_util_unittest.cc',
             'translate/language_detection/language_detection_util_unittest.cc',
@@ -100,10 +102,14 @@
             # Dependencies of json_schema
             'components.gyp:json_schema',
 
+            # Dependencies of language_usage_metrics
+            'components.gyp:language_usage_metrics',
+
             # Dependencies of precache/core
             'components.gyp:precache_core',
 
             # Dependencies of translate.
+            'components.gyp:translate_core_browser',
             'components.gyp:translate_core_common',
             'components.gyp:translate_language_detection',
 
@@ -152,6 +158,7 @@
                 ['include', '^auto_login_parser/'],
                 ['include', '^autofill/'],
                 ['include', '^dom_distiller/'],
+                ['include', '^language_usage_metrics/'],
                 ['include', '^precache/core/'],
                 ['include', '^translate/'],
               ],

[jochen (gone - plz use gerrit), 2014-01-09 13:58:02]

On 2014/01/09 13:56:49, droger wrote:
> TBR=jochen

which part are you tbr'ing me for?

[droger, 2014-01-09 13:59:03]

On 2014/01/09 13:58:02, jochen wrote:
> On 2014/01/09 13:56:49, droger wrote:
> > TBR=jochen
> 
> which part are you tbr'ing me for?

Sorry, I forgot that information. It's for:
    BUILD.gn
    chrome/browser/DEPS
    chrome/browser/chrome_browser_main.cc
    chrome/browser/language_usage_metrics.cc
    chrome/browser/language_usage_metrics.h
    chrome/browser/language_usage_metrics_unittest.cc

[jochen (gone - plz use gerrit), 2014-01-09 14:02:02]

lgtm

[commit-bot: I haz the power, 2014-01-09 14:55:21]

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

[commit-bot: I haz the power, 2014-01-09 19:05:09]

Change committed as 243946

[jam, 2016-06-28 17:30:11]

Sorry I know this is an old cl, but I'm wondering:

"LanguageUsageMetrics is moved to its own component because it is used both in
the translate component and directly in the browser."

what's wrong with making the chrome/browser code include files in
components/translate to get this?

[blundell, 2016-06-28 19:01:55]

On 2016/06/28 17:30:11, jam wrote:
> Sorry I know this is an old cl, but I'm wondering:
> 
> "LanguageUsageMetrics is moved to its own component because it is used both in
> the translate component and directly in the browser."
> 
> what's wrong with making the chrome/browser code include files in
> components/translate to get this?

I had to page this back in :). IIRC, the issue wasn't a technical one but rather
that LanguageUsageMetrics seemed conceptually not part of the translate feature
(e.g., it wasn't under //chrome/browser/translate before, and its usage in
//chrome/browser doesn't seem to be related to translation persay). A different
wording could be "LanguageUsageMetrics is also componentized in order to be made
available to the translate component".