Send TranslateEventProtos to UMA.

Send TranslateEventProtos to UMA

Cache TranslateEvents and send them to UMA through the metrics provider.

BUG=653700, 634961
Committed: https://crrev.com/b257fe9351c22aaae756a8527442193ef412d6cf
Cr-Commit-Position: refs/heads/master@{#424878}

[hamelphi, 2016-10-07 18:42:46]

Description was changed from

==========
Use translate_ranker_metrics_provider to send TranslateEventProtos to UMA.
BUG=

patch from issue 2394643002 at patchset 20001
(http://crrev.com/2394643002#ps20001)
==========

to

==========
Use translate_ranker_metrics_provider to send TranslateEventProtos to UMA.
BUG=653700

patch from issue 2394643002 at patchset 20001
(http://crrev.com/2394643002#ps20001)
==========

[hamelphi, 2016-10-07 21:25:36]

Description was changed from

==========
Use translate_ranker_metrics_provider to send TranslateEventProtos to UMA.
BUG=653700

patch from issue 2394643002 at patchset 20001
(http://crrev.com/2394643002#ps20001)
==========

to

==========
Use translate_ranker_metrics_provider to send TranslateEventProtos to UMA.
BUG=653700, 634961
==========

[hamelphi, 2016-10-07 21:53:47]

The CQ bit was checked by hamelphi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 21:54:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hamelphi, 2016-10-07 21:54:26]

hamelphi@chromium.org changed reviewers:
+ asvitkine@chromium.org, rogerm@chromium.org

[hamelphi, 2016-10-07 22:00:17]

hamelphi@chromium.org changed reviewers:
+ groby@chromium.org

[groby-ooo-7-16, 2016-10-10 02:47:26]

RS LGTM for translate, but I defer to Alexei on metrics handling.

[hamelphi, 2016-10-11 17:13:29]

The CQ bit was checked by hamelphi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 17:13:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 17:29:31]

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

[commit-bot: I haz the power, 2016-10-11 17:29:32]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[hamelphi, 2016-10-11 20:35:47]

Please hold on the review. I am refactoring to fix a segmentation fault in some
tests.

[hamelphi, 2016-10-12 15:14:13]

The CQ bit was checked by hamelphi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 15:14:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hamelphi, 2016-10-12 15:14:31]

PTAL

[commit-bot: I haz the power, 2016-10-12 15:17:46]

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

[commit-bot: I haz the power, 2016-10-12 15:17:47]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alexei Svitkine (slow), 2016-10-12 15:21:59]

LGTM % comment

Please put a blank line after the first sentence of your CL description. Also, I
think it's best to keep the first sentence below 80 chars - maybe no need to
spell out translate_ranker_metrics_provider.

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

https://codereview.chromium.org/2395253002/diff/120001/chrome/browser/transla...
chrome/browser/translate/chrome_translate_client.cc:174: if
(translate::TranslateRanker::IsEnabled()) {
Nit: Can this code be simplified to remove this, if GetInstance() returns null
when it's disabled? Or are both needed to avoid some DCHECK?

Same for other places where you check both. Imho, if you document semantics of
GetInstance() to return null when not enabled, I think you should remove the
extra IsEnabled() to simplify the code.

https://codereview.chromium.org/2395253002/diff/120001/components/translate/c...
File components/translate/core/browser/translate_ranker_unittest.cc (right):

https://codereview.chromium.org/2395253002/diff/120001/components/translate/c...
components/translate/core/browser/translate_ranker_unittest.cc:202: 
Nit: Remove extra empty line.

[hamelphi, 2016-10-12 15:47:15]

The CQ bit was checked by hamelphi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 15:47:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hamelphi, 2016-10-12 16:17:22]

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

https://codereview.chromium.org/2395253002/diff/120001/chrome/browser/transla...
chrome/browser/translate/chrome_translate_client.cc:174: if
(translate::TranslateRanker::IsEnabled()) {
On 2016/10/12 15:21:58, Alexei Svitkine (slow) wrote:
> Nit: Can this code be simplified to remove this, if GetInstance() returns null
> when it's disabled? Or are both needed to avoid some DCHECK?
> 
> Same for other places where you check both. Imho, if you document semantics of
> GetInstance() to return null when not enabled, I think you should remove the
> extra IsEnabled() to simplify the code.

I was just chatting about this with Roger. IsEnabled is related to whether or
not we should query or enforce the Ranker, which is not directly related to
logging, we may want to log stuff without querying the ranker for some reason.
What we need here is another flag for TranslateRanker "IsLoggingEnabled" to
control the logging of TranslateEvents independently of querying ranker and
enforcing it. Added it.

https://codereview.chromium.org/2395253002/diff/120001/components/translate/c...
File components/translate/core/browser/translate_ranker_unittest.cc (right):

https://codereview.chromium.org/2395253002/diff/120001/components/translate/c...
components/translate/core/browser/translate_ranker_unittest.cc:202: 
On 2016/10/12 15:21:58, Alexei Svitkine (slow) wrote:
> Nit: Remove extra empty line.

Done.

[hamelphi, 2016-10-12 16:19:00]

Description was changed from

==========
Use translate_ranker_metrics_provider to send TranslateEventProtos to UMA.
BUG=653700, 634961
==========

to

==========
Send TranslateEventProtos to UMA

Cache TranslateEvents and send them to UMA through the metrics provider.

BUG=653700, 634961
==========

[hamelphi, 2016-10-12 16:19:29]

The CQ bit was checked by hamelphi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 16:19:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 17:53:45]

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

[commit-bot: I haz the power, 2016-10-12 17:53:46]

Dry run: This issue passed the CQ dry run.

[Roger McFarlane (Chromium), 2016-10-12 18:18:28]

Sweet! There's some code that isn't needed.

Otherwise... LGTM.

https://codereview.chromium.org/2395253002/diff/180001/chrome/browser/transla...
File chrome/browser/translate/chrome_translate_client.cc (right):

https://codereview.chromium.org/2395253002/diff/180001/chrome/browser/transla...
chrome/browser/translate/chrome_translate_client.cc:171: // TODO(rogerm): This
code can be moved into TranslateManager.
This can be removed. TranslateManager is is the intended client of this code,
but it can directly call
TranslterRanker::GetInstance()->RecordTranslateEvent(...), which already acts
based on IsLoggingEnabled().

https://codereview.chromium.org/2395253002/diff/180001/chrome/browser/transla...
File chrome/browser/translate/chrome_translate_client.h (right):

https://codereview.chromium.org/2395253002/diff/180001/chrome/browser/transla...
chrome/browser/translate/chrome_translate_client.h:92: bool
RecordTranslateEvent(
this entry point isn't needed. The TranslateManager has direct access to the
TranslateRanker, so doesn't need to go through the client.

[hamelphi, 2016-10-12 21:06:09]

https://codereview.chromium.org/2395253002/diff/180001/chrome/browser/transla...
File chrome/browser/translate/chrome_translate_client.cc (right):

https://codereview.chromium.org/2395253002/diff/180001/chrome/browser/transla...
chrome/browser/translate/chrome_translate_client.cc:171: // TODO(rogerm): This
code can be moved into TranslateManager.
On 2016/10/12 18:18:28, Roger McFarlane (Chromium) wrote:
> This can be removed. TranslateManager is is the intended client of this code,
> but it can directly call
> TranslterRanker::GetInstance()->RecordTranslateEvent(...), which already acts
> based on IsLoggingEnabled().

Agreed. Removed.

https://codereview.chromium.org/2395253002/diff/180001/chrome/browser/transla...
File chrome/browser/translate/chrome_translate_client.h (right):

https://codereview.chromium.org/2395253002/diff/180001/chrome/browser/transla...
chrome/browser/translate/chrome_translate_client.h:92: bool
RecordTranslateEvent(
On 2016/10/12 18:18:28, Roger McFarlane (Chromium) wrote:
> this entry point isn't needed. The TranslateManager has direct access to the
> TranslateRanker, so doesn't need to go through the client.

Done.

[hamelphi, 2016-10-12 21:07:02]

The CQ bit was checked by hamelphi@chromium.org

[hamelphi, 2016-10-12 21:07:03]

The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org,
asvitkine@chromium.org, rogerm@chromium.org
Link to the patchset: https://codereview.chromium.org/2395253002/#ps200001
(title: "Remove Record interface from client.")

[commit-bot: I haz the power, 2016-10-12 21:07:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 22:08:16]

Description was changed from

==========
Send TranslateEventProtos to UMA

Cache TranslateEvents and send them to UMA through the metrics provider.

BUG=653700, 634961
==========

to

==========
Send TranslateEventProtos to UMA

Cache TranslateEvents and send them to UMA through the metrics provider.

BUG=653700, 634961
==========

[commit-bot: I haz the power, 2016-10-12 22:08:17]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-10-12 22:11:00]

Description was changed from

==========
Send TranslateEventProtos to UMA

Cache TranslateEvents and send them to UMA through the metrics provider.

BUG=653700, 634961
==========

to

==========
Send TranslateEventProtos to UMA

Cache TranslateEvents and send them to UMA through the metrics provider.

BUG=653700, 634961

Committed: https://crrev.com/b257fe9351c22aaae756a8527442193ef412d6cf
Cr-Commit-Position: refs/heads/master@{#424878}
==========

[commit-bot: I haz the power, 2016-10-12 22:11:01]

Patchset 11 (id:??) landed as
https://crrev.com/b257fe9351c22aaae756a8527442193ef412d6cf
Cr-Commit-Position: refs/heads/master@{#424878}