Added the AutomaticProfileResetter service.

Added the AutomaticProfileResetter service.

BUG=298036
NOTRY=True

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

[engedy, 2013-09-25 08:37:38]

Please take a look. It turns out that trying to decouple this from the
Interpreter is a huge pain from the testing perspective. So right now some tests
don't pass, as I look forward to using the real interpreter for testing.

[battre, 2013-09-25 09:43:26]

https://codereview.chromium.org/24533002/diff/1/base/prefs/pref_service.h
File base/prefs/pref_service.h (right):

https://codereview.chromium.org/24533002/diff/1/base/prefs/pref_service.h#new...
base/prefs/pref_service.h:241: // The ownership is passed to the caller.
My suggestion:

Returns a dictionary with effective preference values. Contrary to
GetPreferenceValues() the paths of registered preferences are not split on '.'
characters. If a registered preference stores a dictionary, this will use the
typical nesting on '.' characters. If "foo.bar" is a registered preference, the
result could look like this: { "foo.bar": {"a": {"b": true} } }.
The ownership is passed to the caller.

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

https://codereview.chromium.org/24533002/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:243:
scoped_ptr<AutomaticProfileResetter::EvaluationResults>
add "// static" before this line
or in my opinion preferable:
make this a static function in the anonymous namespace. This ensures that we
don't access any member variables from the wrong thread.

https://codereview.chromium.org/24533002/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:301: hash_seed_ =
hash_key;
This looks incorrect. If he passed hash_key parameter leaves its scope,
hash_seed_ points to an incorrect location as it is a StringPiece. Same below.

https://codereview.chromium.org/24533002/diff/1/chrome/browser/profile_resett...
File chrome/browser/profile_resetter/automatic_profile_resetter.h (right):

https://codereview.chromium.org/24533002/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/automatic_profile_resetter.h:18: namespace
profile_resetter {
Using namespaces is discouraged according to the most recent threads I have seen
on the mailing lists. I think in this case it is not necessary either.

[engedy, 2013-09-25 16:56:59]

Robert, could you please take a look?

Clearly, there is work to do here, but we would like to limit the number of
iterations.

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

https://codereview.chromium.org/24533002/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:301: hash_seed_ =
hash_key;
On 2013/09/25 09:43:26, battre wrote:
> This looks incorrect. If he passed hash_key parameter leaves its scope,
> hash_seed_ points to an incorrect location as it is a StringPiece. Same below.

Good point. I played around too much with StringPieces vs. std::strings...

[robertshield, 2013-09-25 20:18:12]

Looks really good, just minor style nits.

https://codereview.chromium.org/24533002/diff/1/base/prefs/pref_service.h
File base/prefs/pref_service.h (right):

https://codereview.chromium.org/24533002/diff/1/base/prefs/pref_service.h#new...
base/prefs/pref_service.h:241: // The ownership is passed to the caller.
On 2013/09/25 09:43:26, battre wrote:
> My suggestion:
> 
> Returns a dictionary with effective preference values. Contrary to
> GetPreferenceValues() the paths of registered preferences are not split on '.'
> characters. If a registered preference stores a dictionary, this will use the
> typical nesting on '.' characters. If "foo.bar" is a registered preference,
the
> result could look like this: { "foo.bar": {"a": {"b": true} } }.
> The ownership is passed to the caller.

Don't have much to add other than a big +1 on providing an example in the
comment.

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

https://codereview.chromium.org/24533002/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:64: // Mostly
trivial AutomaticProfileResetterDelegate implementation.
Nit: delete the above line, or explain how the implementation is non-trivial.
Preference for the former.

https://codereview.chromium.org/24533002/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:91: // A fake
interpreter that enables writing EvaluateConditionsOnWorkerPoolThread.
s/enables writing/used by/

Also mention that this is temporary until the real interpreter is wired up.

https://codereview.chromium.org/24533002/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:190:
kAutomaticProfileResetStudyEnabledroupName;
Looks like there is a G missing before the "roup"

https://codereview.chromium.org/24533002/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:290: else
style nit: multi line statements should be wrapped in {}

[engedy, 2013-09-27 23:27:34]

I have added the real Interpreter, and addressed comments. 

Coud you PTAL?

https://codereview.chromium.org/24533002/diff/1/base/prefs/pref_service.h
File base/prefs/pref_service.h (right):

https://codereview.chromium.org/24533002/diff/1/base/prefs/pref_service.h#new...
base/prefs/pref_service.h:241: // The ownership is passed to the caller.
On 2013/09/25 20:18:12, robertshield wrote:
> On 2013/09/25 09:43:26, battre wrote:
> > My suggestion:
> > 
> > Returns a dictionary with effective preference values. Contrary to
> > GetPreferenceValues() the paths of registered preferences are not split on
'.'
> > characters. If a registered preference stores a dictionary, this will use
the
> > typical nesting on '.' characters. If "foo.bar" is a registered preference,
> the
> > result could look like this: { "foo.bar": {"a": {"b": true} } }.
> > The ownership is passed to the caller.
> 
> Don't have much to add other than a big +1 on providing an example in the
> comment.

Done.

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

https://codereview.chromium.org/24533002/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:64: // Mostly
trivial AutomaticProfileResetterDelegate implementation.
On 2013/09/25 20:18:12, robertshield wrote:
> Nit: delete the above line, or explain how the implementation is non-trivial.
> Preference for the former.

Done.

https://codereview.chromium.org/24533002/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:91: // A fake
interpreter that enables writing EvaluateConditionsOnWorkerPoolThread.
On 2013/09/25 20:18:12, robertshield wrote:
> s/enables writing/used by/
> 
> Also mention that this is temporary until the real interpreter is wired up.

I have wired in the real thing. PTAL below.

https://codereview.chromium.org/24533002/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:190:
kAutomaticProfileResetStudyEnabledroupName;
On 2013/09/25 20:18:12, robertshield wrote:
> Looks like there is a G missing before the "roup"

Done.

https://codereview.chromium.org/24533002/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:243:
scoped_ptr<AutomaticProfileResetter::EvaluationResults>
On 2013/09/25 09:43:26, battre wrote:
> add "// static" before this line
> or in my opinion preferable:
> make this a static function in the anonymous namespace. This ensures that we
> don't access any member variables from the wrong thread.

I am not sure I understand: how would this be any safer than a static member
method?

https://codereview.chromium.org/24533002/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:290: else
On 2013/09/25 20:18:12, robertshield wrote:
> style nit: multi line statements should be wrapped in {}

Done.

https://codereview.chromium.org/24533002/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:301: hash_seed_ =
hash_key;
On 2013/09/25 16:56:59, engedy wrote:
> On 2013/09/25 09:43:26, battre wrote:
> > This looks incorrect. If he passed hash_key parameter leaves its scope,
> > hash_seed_ points to an incorrect location as it is a StringPiece. Same
below.
> 
> Good point. I played around too much with StringPieces vs. std::strings...

Done.

https://codereview.chromium.org/24533002/diff/1/chrome/browser/profile_resett...
File chrome/browser/profile_resetter/automatic_profile_resetter.h (right):

https://codereview.chromium.org/24533002/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/automatic_profile_resetter.h:18: namespace
profile_resetter {
On 2013/09/25 09:43:26, battre wrote:
> Using namespaces is discouraged according to the most recent threads I have
seen
> on the mailing lists. I think in this case it is not necessary either.

Done.

[robertshield, 2013-09-28 01:11:28]

lgtm

[battre, 2013-09-28 07:36:16]

one more feature request.

https://codereview.chromium.org/24533002/diff/10001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right):

https://codereview.chromium.org/24533002/diff/10001/chrome/browser/profile_re...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:147: } else { //
ShouldPerformDryRun()
nit: +1 space before //

https://codereview.chromium.org/24533002/diff/10001/chrome/browser/profile_re...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:237:
interpreter.Execute();
please UMA report the success status.

[engedy, 2013-10-01 14:12:19]

@Dominic: please take another look.

https://codereview.chromium.org/24533002/diff/10001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right):

https://codereview.chromium.org/24533002/diff/10001/chrome/browser/profile_re...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:147: } else { //
ShouldPerformDryRun()
On 2013/09/28 07:36:16, battre wrote:
> nit: +1 space before //

Done.

https://codereview.chromium.org/24533002/diff/10001/chrome/browser/profile_re...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:237:
interpreter.Execute();
On 2013/09/28 07:36:16, battre wrote:
> please UMA report the success status.

Done.

[battre, 2013-10-01 14:56:07]

lgtm

[engedy, 2013-10-01 15:30:36]

@Miranda: please review "profiles/chrome_browser_main_extra_parts_profiles.cc"
for OWNERS' approval.

@Jochen, @Nico: please review "browser_resources.grd" and 
"chrome_constants.{h|cc}" for OWNERS' approval.

[Miranda Callahan, 2013-10-01 15:33:45]

On 2013/10/01 15:30:36, engedy wrote:
> @Miranda: please review "profiles/chrome_browser_main_extra_parts_profiles.cc"
> for OWNERS' approval.
> 
> @Jochen, @Nico: please review "browser_resources.grd" and 
> "chrome_constants.{h|cc}" for OWNERS' approval.

Rerouting to rlp@ for profiles/*, as I am no longer on Chrome project.

[engedy, 2013-10-01 22:17:51]

Further re-routing chrome/browser/profiles/* to Elliot for OWNER's approval, as
Rachel is travelling.

[Elliot Glaysher, 2013-10-01 22:29:36]

lgtm

[Nico, 2013-10-01 22:33:18]

https://codereview.chromium.org/24533002/diff/74001/chrome/browser/browser_re...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/24533002/diff/74001/chrome/browser/browser_re...
chrome/browser/browser_resources.grd:389: <include
name="IDR_AUTOMATIC_PROFILE_RESET_HASH_SEED_DRY"
file="internal\resources\profile_reset\automatic_profile_reset_hash_seed_dry.dat"
type="BINDATA" />
Don't you need to set these in the non-_google_chrome case too? Does this build
in a Chromium build as is?

[engedy, 2013-10-01 22:52:56]

https://codereview.chromium.org/24533002/diff/74001/chrome/browser/browser_re...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/24533002/diff/74001/chrome/browser/browser_re...
chrome/browser/browser_resources.grd:389: <include
name="IDR_AUTOMATIC_PROFILE_RESET_HASH_SEED_DRY"
file="internal\resources\profile_reset\automatic_profile_reset_hash_seed_dry.dat"
type="BINDATA" />
On 2013/10/01 22:33:18, Nico wrote:
> Don't you need to set these in the non-_google_chrome case too? Does this
build
> in a Chromium build as is?

Yes, it builds and runs. Also, the Grit documentation explicitly mentions that
the resource IDs will stay the same regardless of conditional includes, and that
"this is desirable for several reasons". I think this would make a
ResourceBundle implementation that cannot handle non-included resources (by
returning "") at least strange.

[Nico, 2013-10-01 23:03:59]

Huh, I didn't know that!

lgtm

[commit-bot: I haz the power, 2013-10-02 10:34:35]

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

[commit-bot: I haz the power, 2013-10-02 10:57:45]

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

[commit-bot: I haz the power, 2013-10-02 12:30:38]

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

[commit-bot: I haz the power, 2013-10-02 12:32:25]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-10-02 12:33:52]

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

[commit-bot: I haz the power, 2013-10-02 15:00:42]

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

[commit-bot: I haz the power, 2013-10-02 16:01:06]

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

[commit-bot: I haz the power, 2013-10-02 16:07:07]

Change committed as 226473