Send UMA recording state to TranslateRanker.

Send UMA recording state to TranslateRanker.

This will prevent the accumulation of translate events when UMA recording is not enabled.

Also:
 - clear the event_cache when recording is disabled.
 - Change 'return' to 'continue' in translate_ranker_metrics_provider in case only a subset of browser states don't have a valid ranker.

BUG=730032
Review-Url: https://codereview.chromium.org/2930433004
Cr-Commit-Position: refs/heads/master@{#478719}
Committed: https://chromium.googlesource.com/chromium/src/+/a33125bccd08d77590b3ee8907d85ca5a042c78a

[hamelphi, 2017-06-06 18:39:08]

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

[commit-bot: I haz the power, 2017-06-06 18:40:12]

Dry run: CQ is trying da patch.

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

[hamelphi, 2017-06-06 18:42:37]

Description was changed from

==========
Send UMA recording state to TranslateRanker.

This will prevent the accumualtion of translate events when UMA recording is not
enabled.

BUG=730032
==========

to

==========
Send UMA recording state to TranslateRanker.

This will prevent the accumulation of translate events when UMA recording is not
enabled.

Also:
 - clear the event_cache when recording is disabled.
 - Change 'return' to 'continue' in translate_ranker_metrics_provider in case
only a subset of browser states don't have a valid ranker.

BUG=730032
==========

[hamelphi, 2017-06-06 18:46:47]

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

[Roger McFarlane (Chromium), 2017-06-06 19:28:28]

LGTM with nits

https://codereview.chromium.org/2930433004/diff/40001/chrome/browser/translat...
File chrome/browser/translate/translate_ranker_metrics_provider.cc (right):

https://codereview.chromium.org/2930433004/diff/40001/chrome/browser/translat...
chrome/browser/translate/translate_ranker_metrics_provider.cc:24: continue;
doh! good catch!

https://codereview.chromium.org/2930433004/diff/40001/chrome/browser/translat...
chrome/browser/translate/translate_ranker_metrics_provider.cc:43:
ranker->EnableLogging(logging_enabled);
optional:

if (ranker)
  ranker->EnableLogging(...);

https://codereview.chromium.org/2930433004/diff/40001/chrome/browser/translat...
File chrome/browser/translate/translate_ranker_metrics_provider.h (right):

https://codereview.chromium.org/2930433004/diff/40001/chrome/browser/translat...
chrome/browser/translate/translate_ranker_metrics_provider.h:17: void
OnRecordingEnabled() override;
Should these be down next to ProvideGeneralMetrics(...)?

https://codereview.chromium.org/2930433004/diff/40001/components/translate/co...
File components/translate/core/browser/translate_ranker_impl.cc (right):

https://codereview.chromium.org/2930433004/diff/40001/components/translate/co...
components/translate/core/browser/translate_ranker_impl.cc:186: if (!value)
if (!is_logging_enabled_)

https://codereview.chromium.org/2930433004/diff/40001/components/translate/co...
File components/translate/core/browser/translate_ranker_impl_unittest.cc
(right):

https://codereview.chromium.org/2930433004/diff/40001/components/translate/co...
components/translate/core/browser/translate_ranker_impl_unittest.cc:357: 
before and after EnableLogging(), can we assert the cache size?

https://codereview.chromium.org/2930433004/diff/40001/ios/chrome/browser/tran...
File ios/chrome/browser/translate/translate_ranker_metrics_provider.cc (right):

https://codereview.chromium.org/2930433004/diff/40001/ios/chrome/browser/tran...
ios/chrome/browser/translate/translate_ranker_metrics_provider.cc:26:
TranslateRankerFactory::GetInstance()->GetForBrowserState(state);
TranslateRankerFactory::GetForBrowserState(...)

https://codereview.chromium.org/2930433004/diff/40001/ios/chrome/browser/tran...
ios/chrome/browser/translate/translate_ranker_metrics_provider.cc:44:
TranslateRankerFactory::GetInstance()->GetForBrowserState(state);
TranslateRankerFactory::GetForBrowserState(...)

[commit-bot: I haz the power, 2017-06-06 20:01:16]

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

[commit-bot: I haz the power, 2017-06-06 20:01:17]

Dry run: This issue passed the CQ dry run.

[hamelphi, 2017-06-06 20:28:06]

https://codereview.chromium.org/2930433004/diff/40001/chrome/browser/translat...
File chrome/browser/translate/translate_ranker_metrics_provider.cc (right):

https://codereview.chromium.org/2930433004/diff/40001/chrome/browser/translat...
chrome/browser/translate/translate_ranker_metrics_provider.cc:24: continue;
On 2017/06/06 19:28:27, Roger McFarlane (Chromium) wrote:
> doh! good catch!

Acknowledged.

https://codereview.chromium.org/2930433004/diff/40001/chrome/browser/translat...
chrome/browser/translate/translate_ranker_metrics_provider.cc:43:
ranker->EnableLogging(logging_enabled);
On 2017/06/06 19:28:27, Roger McFarlane (Chromium) wrote:
> optional:
> 
> if (ranker)
>   ranker->EnableLogging(...);
> 

Done.

https://codereview.chromium.org/2930433004/diff/40001/chrome/browser/translat...
File chrome/browser/translate/translate_ranker_metrics_provider.h (right):

https://codereview.chromium.org/2930433004/diff/40001/chrome/browser/translat...
chrome/browser/translate/translate_ranker_metrics_provider.h:17: void
OnRecordingEnabled() override;
On 2017/06/06 19:28:27, Roger McFarlane (Chromium) wrote:
> Should these be down next to ProvideGeneralMetrics(...)?

Actually, I don't think ProvideGeneralMetrics should be private. MetricsProvider
declares it as public. so I moved it up here.

https://codereview.chromium.org/2930433004/diff/40001/components/translate/co...
File components/translate/core/browser/translate_ranker_impl.cc (right):

https://codereview.chromium.org/2930433004/diff/40001/components/translate/co...
components/translate/core/browser/translate_ranker_impl.cc:186: if (!value)
On 2017/06/06 19:28:27, Roger McFarlane (Chromium) wrote:
> if (!is_logging_enabled_)

Done.

https://codereview.chromium.org/2930433004/diff/40001/components/translate/co...
File components/translate/core/browser/translate_ranker_impl_unittest.cc
(right):

https://codereview.chromium.org/2930433004/diff/40001/components/translate/co...
components/translate/core/browser/translate_ranker_impl_unittest.cc:357: 
On 2017/06/06 19:28:27, Roger McFarlane (Chromium) wrote:
> before and after EnableLogging(), can we assert the cache size?

Not easily, since the cache is private. However, RecordTranslateEvent and
FlushTranslateEvents are tested a few lines above.

I don't think it warrants exposing the cache just for testing.

https://codereview.chromium.org/2930433004/diff/40001/ios/chrome/browser/tran...
File ios/chrome/browser/translate/translate_ranker_metrics_provider.cc (right):

https://codereview.chromium.org/2930433004/diff/40001/ios/chrome/browser/tran...
ios/chrome/browser/translate/translate_ranker_metrics_provider.cc:26:
TranslateRankerFactory::GetInstance()->GetForBrowserState(state);
On 2017/06/06 19:28:27, Roger McFarlane (Chromium) wrote:
> TranslateRankerFactory::GetForBrowserState(...)

Done.

https://codereview.chromium.org/2930433004/diff/40001/ios/chrome/browser/tran...
ios/chrome/browser/translate/translate_ranker_metrics_provider.cc:44:
TranslateRankerFactory::GetInstance()->GetForBrowserState(state);
On 2017/06/06 19:28:27, Roger McFarlane (Chromium) wrote:
> TranslateRankerFactory::GetForBrowserState(...)

Done.

[hamelphi, 2017-06-06 20:42:58]

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

[hamelphi, 2017-06-06 20:42:58]

+groby as reviewer for translate OWNER approval

[groby-ooo-7-16, 2017-06-07 16:27:22]

ios parts only rubber stamping, rest LGTM % comments on event_cache_ clearing.

https://codereview.chromium.org/2930433004/diff/60001/components/translate/co...
File components/translate/core/browser/translate_ranker_impl.cc (right):

https://codereview.chromium.org/2930433004/diff/60001/components/translate/co...
components/translate/core/browser/translate_ranker_impl.cc:186: if
(!is_logging_enabled_)
Don't you want to clear on every edge transition? As a fallback in case the
event_cache_ got populated due to a bug?

https://codereview.chromium.org/2930433004/diff/60001/components/translate/co...
File components/translate/core/browser/translate_ranker_impl_unittest.cc
(right):

https://codereview.chromium.org/2930433004/diff/60001/components/translate/co...
components/translate/core/browser/translate_ranker_impl_unittest.cc:357: 
I'd check for 0 flushed events here, too - otherwise the final EXPECT conflates
testing for cache clearing, and testing that enable/disable works at all.

(In fact, I'd prefer a separate test for cache clearing on enable/disable)

[Alexei Svitkine (slow), 2017-06-07 16:45:28]

https://codereview.chromium.org/2930433004/diff/60001/chrome/browser/translat...
File chrome/browser/translate/translate_ranker_metrics_provider.cc (right):

https://codereview.chromium.org/2930433004/diff/60001/chrome/browser/translat...
chrome/browser/translate/translate_ranker_metrics_provider.cc:38: for (Profile*
profile : loaded_profiles) {
So this will update the rankers for existing opened profiles. How about when a
new profile gets loaded?

I think you'd want to listen to profile creation events and update the ranking
state there.

Alternatively, it may be better to just have a global on/off switch - and then
you don't need to worry about profile lifecycles.

[hamelphi, 2017-06-08 20:27:58]

https://codereview.chromium.org/2930433004/diff/60001/chrome/browser/translat...
File chrome/browser/translate/translate_ranker_metrics_provider.cc (right):

https://codereview.chromium.org/2930433004/diff/60001/chrome/browser/translat...
chrome/browser/translate/translate_ranker_metrics_provider.cc:38: for (Profile*
profile : loaded_profiles) {
On 2017/06/07 16:45:28, Alexei Svitkine (very slow) wrote:
> So this will update the rankers for existing opened profiles. How about when a
> new profile gets loaded?
> 
> I think you'd want to listen to profile creation events and update the ranking
> state there.
> 
> Alternatively, it may be better to just have a global on/off switch - and then
> you don't need to worry about profile lifecycles.

Who would own this switch? Ranker or UMA? I'd rather avoid using a global
variable in TranslateRanker if possible. If the metrics recording state
(https://cs.chromium.org/chromium/src/components/metrics_services_manager/metr...
maybe?) would be available globally, we could pass it as an argument to the
TranslateRanker constructor. However, my guess is that exposing this state
globally is probably not desirable.

How about we set the default to Disabled when we Initialize a Ranker object and
we only update the logging state on a profile once we receive OnRecordingEnabled
or ProvideGeneralMetrics. If I read the code correctly, OnRecordingEnabled is
called at startup, so we should not drop logging for profiles loaded at startup
(though I am not entirely sure if metrics is started after profiles are loaded).
We might lose logging on the first few TranslateEvents for profiles loaded
post-startup, but they should be enabled on the first call to
providegeneralmetrics. That sounds acceptable to me. What do you think?

https://codereview.chromium.org/2930433004/diff/60001/components/translate/co...
File components/translate/core/browser/translate_ranker_impl.cc (right):

https://codereview.chromium.org/2930433004/diff/60001/components/translate/co...
components/translate/core/browser/translate_ranker_impl.cc:186: if
(!is_logging_enabled_)
On 2017/06/07 16:27:21, groby wrote:
> Don't you want to clear on every edge transition? As a fallback in case the
> event_cache_ got populated due to a bug?

Good point. Done.

https://codereview.chromium.org/2930433004/diff/60001/components/translate/co...
File components/translate/core/browser/translate_ranker_impl_unittest.cc
(right):

https://codereview.chromium.org/2930433004/diff/60001/components/translate/co...
components/translate/core/browser/translate_ranker_impl_unittest.cc:357: 
On 2017/06/07 16:27:22, groby wrote:
> I'd check for 0 flushed events here, too - otherwise the final EXPECT
conflates
> testing for cache clearing, and testing that enable/disable works at all.
> 
> (In fact, I'd prefer a separate test for cache clearing on enable/disable)

Done. Added explicit tests for cache.

[Alexei Svitkine (slow), 2017-06-08 23:33:54]

lgtm

[hamelphi, 2017-06-12 14:31:26]

The CQ bit was checked by hamelphi@chromium.org

[hamelphi, 2017-06-12 14:31:26]

The patchset sent to the CQ was uploaded after l-g-t-m from rogerm@chromium.org,
groby@chromium.org
Link to the patchset: https://codereview.chromium.org/2930433004/#ps80001
(title: "Set default state to disabled.")

[commit-bot: I haz the power, 2017-06-12 14:31:41]

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, 2017-06-12 14:55:42]

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

[commit-bot: I haz the power, 2017-06-12 14:55:43]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[hamelphi, 2017-06-12 15:01:48]

The CQ bit was checked by hamelphi@chromium.org

[commit-bot: I haz the power, 2017-06-12 15:02:12]

CQ is trying da patch.

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

[hamelphi, 2017-06-12 15:08:40]

The CQ bit was unchecked by hamelphi@chromium.org

[hamelphi, 2017-06-12 15:09:04]

The CQ bit was checked by hamelphi@chromium.org

[hamelphi, 2017-06-12 15:09:05]

The patchset sent to the CQ was uploaded after l-g-t-m from rogerm@chromium.org,
asvitkine@chromium.org, groby@chromium.org
Link to the patchset: https://codereview.chromium.org/2930433004/#ps100001
(title: "Update IOS metrics provider.")

[commit-bot: I haz the power, 2017-06-12 15:09:22]

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, 2017-06-12 15:38:38]

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

[commit-bot: I haz the power, 2017-06-12 15:38:39]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hamelphi, 2017-06-12 17:00:52]

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

[commit-bot: I haz the power, 2017-06-12 17:01:16]

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, 2017-06-12 18:08:51]

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

[commit-bot: I haz the power, 2017-06-12 18:08:52]

Dry run: This issue passed the CQ dry run.

[hamelphi, 2017-06-12 19:35:35]

The CQ bit was checked by hamelphi@chromium.org

[hamelphi, 2017-06-12 19:35:35]

The patchset sent to the CQ was uploaded after l-g-t-m from rogerm@chromium.org,
asvitkine@chromium.org, groby@chromium.org
Link to the patchset: https://codereview.chromium.org/2930433004/#ps120001
(title: "Enable Logging in tests that use Flushing.")

[commit-bot: I haz the power, 2017-06-12 19:36:17]

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, 2017-06-12 19:41:00]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497296135264850,
"parent_rev": "2be632d5bef8b0a00525366005c6c51a439b4f7b", "commit_rev":
"a33125bccd08d77590b3ee8907d85ca5a042c78a"}

[commit-bot: I haz the power, 2017-06-12 19:41:12]

Description was changed from

==========
Send UMA recording state to TranslateRanker.

This will prevent the accumulation of translate events when UMA recording is not
enabled.

Also:
 - clear the event_cache when recording is disabled.
 - Change 'return' to 'continue' in translate_ranker_metrics_provider in case
only a subset of browser states don't have a valid ranker.

BUG=730032
==========

to

==========
Send UMA recording state to TranslateRanker.

This will prevent the accumulation of translate events when UMA recording is not
enabled.

Also:
 - clear the event_cache when recording is disabled.
 - Change 'return' to 'continue' in translate_ranker_metrics_provider in case
only a subset of browser states don't have a valid ranker.

BUG=730032

Review-Url: https://codereview.chromium.org/2930433004
Cr-Commit-Position: refs/heads/master@{#478719}
Committed:
https://chromium.googlesource.com/chromium/src/+/a33125bccd08d77590b3ee8907d8...
==========

[commit-bot: I haz the power, 2017-06-12 19:41:13]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/a33125bccd08d77590b3ee8907d8...