Added the ability to specify AutomaticProfileResetter's evaluation program through field trials.

Added the ability to specify AutomaticProfileResetter's evaluation program through field trials.

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

[engedy, 2013-11-04 12:47:57]

Please take a look.

Changes to the unit test code are somewhat ugly in my opinion. Please do let me
know if you have any good solutions for that.

[vabr (Chromium), 2013-11-04 14:16:40]

LGTM with one optional comment.

Cheers,
Vaclav

https://codereview.chromium.org/50883003/diff/1/chrome/browser/profile_resett...
File chrome/browser/profile_resetter/automatic_profile_resetter_unittest.cc
(right):

https://codereview.chromium.org/50883003/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/automatic_profile_resetter_unittest.cc:477:
ASSERT_EQ(base::TimeDelta::FromSeconds(55),
optional: For better reading and maintainability, 55 should be replaced by
kEvaluationFlowDelayInSeconds (along with making this constant visible in this
file).

[engedy, 2013-11-04 16:19:37]

I have added one more unit test, please take another look.

https://codereview.chromium.org/50883003/diff/1/chrome/browser/profile_resett...
File chrome/browser/profile_resetter/automatic_profile_resetter_unittest.cc
(right):

https://codereview.chromium.org/50883003/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/automatic_profile_resetter_unittest.cc:477:
ASSERT_EQ(base::TimeDelta::FromSeconds(55),
On 2013/11/04 14:16:40, vabr (Chromium) wrote:
> optional: For better reading and maintainability, 55 should be replaced by
> kEvaluationFlowDelayInSeconds (along with making this constant visible in this
> file).

Yes, there is actually a couple more of these magic constants I want to handle
better. Let's do this in a follow-up CL!

[vabr (Chromium), 2013-11-04 16:28:38]

Still LGTM.
Took me a while to figure out why the hell 2 almost identical tests :), but then
I finally managed to read their name. Makes sense.
Cheers,
Vaclav

On 2013/11/04 16:19:37, engedy wrote:
> I have added one more unit test, please take another look.
> 
>
https://codereview.chromium.org/50883003/diff/1/chrome/browser/profile_resett...
> File chrome/browser/profile_resetter/automatic_profile_resetter_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/50883003/diff/1/chrome/browser/profile_resett...
> chrome/browser/profile_resetter/automatic_profile_resetter_unittest.cc:477:
> ASSERT_EQ(base::TimeDelta::FromSeconds(55),
> On 2013/11/04 14:16:40, vabr (Chromium) wrote:
> > optional: For better reading and maintainability, 55 should be replaced by
> > kEvaluationFlowDelayInSeconds (along with making this constant visible in
this
> > file).
> 
> Yes, there is actually a couple more of these magic constants I want to handle
> better. Let's do this in a follow-up CL!

SGTM

[commit-bot: I haz the power, 2013-11-04 17:42:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/engedy@chromium.org/50883003/150002

[commit-bot: I haz the power, 2013-11-04 23:22:06]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-11-05 09:56:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/engedy@chromium.org/50883003/150002

[commit-bot: I haz the power, 2013-11-05 12:24:19]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-11-05 13:04:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/engedy@chromium.org/50883003/150002

[commit-bot: I haz the power, 2013-11-05 16:10:30]

Change committed as 232995

[Alexei Svitkine (slow), 2013-11-06 15:24:56]

Logic is fine, though could be made more efficient (see my suggestion below).
(Up to you whether you want to address this in a follow-up or not.)

It would have been good to include an owner of the API in the original review.

https://codereview.chromium.org/50883003/diff/150002/chrome/browser/profile_r...
File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right):

https://codereview.chromium.org/50883003/diff/150002/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:213: if
(DoesExperimentOverrideProgramAndHashSeed()) {
I would have made this call:

GetOverrideProgramAndHashSeedFromParams(&program_, &hash_seed_)

(that still returns true/false if it succeeded).

That way, you don't have to query the Variations API 3 different times (each
call involves grabbing a lock and copying a map).

[engedy, 2013-11-06 16:54:58]

I have prepared a follow-up CL to address this:
https://codereview.chromium.org/61883005/.

https://codereview.chromium.org/50883003/diff/150002/chrome/browser/profile_r...
File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right):

https://codereview.chromium.org/50883003/diff/150002/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:213: if
(DoesExperimentOverrideProgramAndHashSeed()) {
On 2013/11/06 15:24:56, Alexei Svitkine wrote:
> I would have made this call:
> 
> GetOverrideProgramAndHashSeedFromParams(&program_, &hash_seed_)
> 
> (that still returns true/false if it succeeded).
> 
> That way, you don't have to query the Variations API 3 different times (each
> call involves grabbing a lock and copying a map).

Done.