Eliminate all code related to the AutomaticProfileResetter.

Eliminate all code related to the AutomaticProfileResetter.

The component has been unused for a long time, is getting stale, and requires maintenance from time to time.

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

[engedy, 2014-05-07 17:09:50]

PTAL.

[battre, 2014-05-08 11:46:13]

https://codereview.chromium.org/271673006/diff/1/chrome/browser/prefs/browser...
File chrome/browser/prefs/browser_prefs.cc (left):

https://codereview.chromium.org/271673006/diff/1/chrome/browser/prefs/browser...
chrome/browser/prefs/browser_prefs.cc:274:
AutomaticProfileResetterFactory::RegisterPrefs(registry);
The preference should be cleared in MigrateUserPrefs().

[engedy, 2014-07-30 09:07:21]

@Dominic: please take another look at the whole thing.

@Vasilii: could you please take a look at:
  c/b/profile_resetter/*
  c/b/ui/webui/*
  c/b/r/options/*
and double check that I have not forgotten to remove references and/or code
specific to the AutomaticProfileResetter in files that are not entirely removed?

@Gabriel: could you please take a look at
c/b/prefs/chrome_pref_service_factory.cc to make sure that nothing will explode
if I remove an element?

https://codereview.chromium.org/271673006/diff/1/chrome/browser/prefs/browser...
File chrome/browser/prefs/browser_prefs.cc (left):

https://codereview.chromium.org/271673006/diff/1/chrome/browser/prefs/browser...
chrome/browser/prefs/browser_prefs.cc:274:
AutomaticProfileResetterFactory::RegisterPrefs(registry);
On 2014/05/08 11:46:14, battre wrote:
> The preference should be cleared in MigrateUserPrefs().

I have removed the respective prefs completely.

Could you please confirm that it is okay to have some values lying around in the
Preferences file that are no longer registered?

[battre, 2014-07-30 09:11:09]

https://codereview.chromium.org/271673006/diff/1/chrome/browser/prefs/browser...
File chrome/browser/prefs/browser_prefs.cc (left):

https://codereview.chromium.org/271673006/diff/1/chrome/browser/prefs/browser...
chrome/browser/prefs/browser_prefs.cc:274:
AutomaticProfileResetterFactory::RegisterPrefs(registry);
On 2014/07/30 09:07:21, engedy wrote:
> On 2014/05/08 11:46:14, battre wrote:
> > The preference should be cleared in MigrateUserPrefs().
> 
> I have removed the respective prefs completely.
> 
> Could you please confirm that it is okay to have some values lying around in
the
> Preferences file that are no longer registered?

As I said above, I think the policy is to clear it in MigrateUserPrefs() for one
milestone so that we don't add cruft to the prefs file.

[vasilii, 2014-07-30 09:49:13]

LGTM
c/b/profile_resetter/
c/b/ui/webui/options/
c/b/resources/options/

https://codereview.chromium.org/271673006/diff/20001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/resettable_settings_snapshot.cc (right):

https://codereview.chromium.org/271673006/diff/20001/chrome/browser/profile_r...
chrome/browser/profile_resetter/resettable_settings_snapshot.cc:31: // Feedback
bucket labels.
label

[engedy, 2014-07-30 11:45:11]

https://codereview.chromium.org/271673006/diff/1/chrome/browser/prefs/browser...
File chrome/browser/prefs/browser_prefs.cc (left):

https://codereview.chromium.org/271673006/diff/1/chrome/browser/prefs/browser...
chrome/browser/prefs/browser_prefs.cc:274:
AutomaticProfileResetterFactory::RegisterPrefs(registry);
On 2014/07/30 09:11:08, battre wrote:
> On 2014/07/30 09:07:21, engedy wrote:
> > On 2014/05/08 11:46:14, battre wrote:
> > > The preference should be cleared in MigrateUserPrefs().
> > 
> > I have removed the respective prefs completely.
> > 
> > Could you please confirm that it is okay to have some values lying around in
> the
> > Preferences file that are no longer registered?
> 
> As I said above, I think the policy is to clear it in MigrateUserPrefs() for
one
> milestone so that we don't add cruft to the prefs file.

Done. I have added code to clear both the local state and user preference, and
also the file that might have been stored to the profile directory.

As discussed offline, I have verified manually/locally that it works. I also
tried it out that just unregistering the preference is not sufficient: the value
will be unreachable from Chrome, but it gets written back when the JSON file is
saved to disk.

https://codereview.chromium.org/271673006/diff/20001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/resettable_settings_snapshot.cc (right):

https://codereview.chromium.org/271673006/diff/20001/chrome/browser/profile_r...
chrome/browser/profile_resetter/resettable_settings_snapshot.cc:31: // Feedback
bucket labels.
On 2014/07/30 09:49:13, vasilii wrote:
> label

Nice catch, done.

[battre, 2014-07-30 12:53:28]

LGTM

https://codereview.chromium.org/271673006/diff/120001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/271673006/diff/120001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:228: // TODO(engedy): Remove this and all
usages at one point. See crbug.com/398813.
Please specify a milestone ("at or after Mxx")

[engedy, 2014-07-30 13:52:52]

https://codereview.chromium.org/271673006/diff/120001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/271673006/diff/120001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:228: // TODO(engedy): Remove this and all
usages at one point. See crbug.com/398813.
On 2014/07/30 12:53:28, battre wrote:
> Please specify a milestone ("at or after Mxx")

Done.

[engedy, 2014-07-30 13:55:08]

On 2014/07/30 13:52:52, engedy wrote:
>
https://codereview.chromium.org/271673006/diff/120001/chrome/browser/prefs/br...
> File chrome/browser/prefs/browser_prefs.cc (right):
> 
>
https://codereview.chromium.org/271673006/diff/120001/chrome/browser/prefs/br...
> chrome/browser/prefs/browser_prefs.cc:228: // TODO(engedy): Remove this and
all
> usages at one point. See crbug.com/398813.
> On 2014/07/30 12:53:28, battre wrote:
> > Please specify a milestone ("at or after Mxx")
> 
> Done.

@Gabriel: In addition, could you please also confirm that I am not removing any
code that you might be using or want to be using in the near future? :-)

[engedy, 2014-07-30 14:52:36]

And now for a round of OWNERs' approvals. Please take a look at:

@Dan:
  c/b/resources/options/
  c/b/ui/webui/

@Elliot:
  c/b/profiles

@Scott:
  c/a/generated_resources.grd
  c/b/browser_resources.grd
  c/b/ui/profile_reset_bubble.*
  c/b/ui/views/
  c/c/chrome_constants.*

@Alexei:
  histograms.xml

[Alexei Svitkine (slow), 2014-07-30 14:59:05]

histograms.xml lgtm

[gab, 2014-07-30 16:59:25]

Nice cleanup (glad I got my CL in yesterday to re-use the layout of this old
banner in a new banner for settings hardening --
https://codereview.chromium.org/411273006/ -- spared me the lengthy layout UX
design discussions :-)!).

Comments below.

Cheers,
Gab

https://codereview.chromium.org/271673006/diff/140001/chrome/browser/prefs/ch...
File chrome/browser/prefs/chrome_pref_service_factory.cc (left):

https://codereview.chromium.org/271673006/diff/140001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:167: },
Leave this pref here as NO_ENFORCEMENT (see kKnownDisabled above).

Otherwise the old value will live in "Secure Preferences" yet
SegregatedPrefStore will try to clear it from "Preferences" (as it will not be
known as a protected preference).

I should really fix this so that SegregatedPrefStore::RemoveValue affects both
underlying stores rather than the selected store (like Get/Set do), but I need
to think about it some more, for now leaving the pref here as NO_ENFORCEMENT
with a "deprecated" comment will achieve what you want.

https://codereview.chromium.org/271673006/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/271673006/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:280: void
DeleteResetPromptMementoFile(const base::FilePath& base) {
I assume "base" is the user_data_dir or the profile_dir? Can you change the name
to make that explicit?

https://codereview.chromium.org/271673006/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:654: BrowserThread::FILE, FROM_HERE,
I think the BlockingPool() is prefered to the FILE thread these days for such
operations.

https://codereview.chromium.org/271673006/diff/140001/chrome/browser/resource...
File chrome/browser/resources/options/reset_profile_settings_overlay.js (left):

https://codereview.chromium.org/271673006/diff/140001/chrome/browser/resource...
chrome/browser/resources/options/reset_profile_settings_overlay.js:75:
AutomaticSettingsResetBanner.dismiss();
Don't remove this line, the AutomaticSettingsResetBanner is still used (it's the
post-hardening banner).

https://codereview.chromium.org/271673006/diff/140001/chrome/browser/resource...
File chrome/browser/resources/options/settings_banner.css (right):

https://codereview.chromium.org/271673006/diff/140001/chrome/browser/resource...
chrome/browser/resources/options/settings_banner.css:1: /* Copyright 2014 The
Chromium Authors. All rights reserved.
settings_banner.css and settings_banner.js were introduced in
https://codereview.chromium.org/151003004 as a common base for the
reset_profile_settings_banner and the automatic_settings_reset_banner

Now that the former is gone, settings_banner.css and settings_banner.js should
be moved to automatic_settings_reset_banner.css and
automatic_settings_reset_banner.js respectively (maybe as a follow-up CL since
this one is already quite large -- at least add TODO here in this CL for now)

[Elliot Glaysher, 2014-07-30 17:51:33]

profiles lgtm

[engedy, 2014-07-30 18:29:48]

@Gab, please take another look.

https://codereview.chromium.org/271673006/diff/140001/chrome/browser/prefs/ch...
File chrome/browser/prefs/chrome_pref_service_factory.cc (left):

https://codereview.chromium.org/271673006/diff/140001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:167: },
On 2014/07/30 16:59:25, gab wrote:
> Leave this pref here as NO_ENFORCEMENT (see kKnownDisabled above).
> 
> Otherwise the old value will live in "Secure Preferences" yet
> SegregatedPrefStore will try to clear it from "Preferences" (as it will not be
> known as a protected preference).
> 
> I should really fix this so that SegregatedPrefStore::RemoveValue affects both
> underlying stores rather than the selected store (like Get/Set do), but I need
> to think about it some more, for now leaving the pref here as NO_ENFORCEMENT
> with a "deprecated" comment will achieve what you want.

Done. Thanks!

https://codereview.chromium.org/271673006/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/271673006/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:280: void
DeleteResetPromptMementoFile(const base::FilePath& base) {
On 2014/07/30 16:59:25, gab wrote:
> I assume "base" is the user_data_dir or the profile_dir? Can you change the
name
> to make that explicit?

Done.

https://codereview.chromium.org/271673006/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:654: BrowserThread::FILE, FROM_HERE,
On 2014/07/30 16:59:25, gab wrote:
> I think the BlockingPool() is prefered to the FILE thread these days for such
> operations.

Done.

https://codereview.chromium.org/271673006/diff/140001/chrome/browser/resource...
File chrome/browser/resources/options/reset_profile_settings_overlay.js (left):

https://codereview.chromium.org/271673006/diff/140001/chrome/browser/resource...
chrome/browser/resources/options/reset_profile_settings_overlay.js:75:
AutomaticSettingsResetBanner.dismiss();
On 2014/07/30 16:59:25, gab wrote:
> Don't remove this line, the AutomaticSettingsResetBanner is still used (it's
the
> post-hardening banner).

Done.

https://codereview.chromium.org/271673006/diff/140001/chrome/browser/resource...
File chrome/browser/resources/options/settings_banner.css (right):

https://codereview.chromium.org/271673006/diff/140001/chrome/browser/resource...
chrome/browser/resources/options/settings_banner.css:1: /* Copyright 2014 The
Chromium Authors. All rights reserved.
On 2014/07/30 16:59:25, gab wrote:
> settings_banner.css and settings_banner.js were introduced in
> https://codereview.chromium.org/151003004 as a common base for the
> reset_profile_settings_banner and the automatic_settings_reset_banner
> 
> Now that the former is gone, settings_banner.css and settings_banner.js should
> be moved to automatic_settings_reset_banner.css and
> automatic_settings_reset_banner.js respectively (maybe as a follow-up CL since
> this one is already quite large -- at least add TODO here in this CL for now)

I have added a TODO, and will refactor this in a follow-up CL.

[sky, 2014-07-30 19:31:18]

LGTM

[gab, 2014-07-30 19:46:53]

lgtm w/ grammar nit (and English is not my native language so feel free to
disregard if you think it's correct as-is..!)

https://codereview.chromium.org/271673006/diff/150001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/271673006/diff/150001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:278: // has been since removed, to store
that the prompt had already been shown.
nit: s/has been since/has since been/

[engedy, 2014-07-31 08:10:50]

https://codereview.chromium.org/271673006/diff/150001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/271673006/diff/150001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:278: // has been since removed, to store
that the prompt had already been shown.
On 2014/07/30 19:46:53, gab wrote:
> nit: s/has been since/has since been/

Done.

[Dan Beam, 2014-08-01 18:28:53]

lgtm

https://chromiumcodereview.appspot.com/271673006/diff/170001/chrome/browser/r...
File chrome/browser/resources/options/settings_banner.css (right):

https://chromiumcodereview.appspot.com/271673006/diff/170001/chrome/browser/r...
chrome/browser/resources/options/settings_banner.css:5: /*
nit: /* TODO

[engedy, 2014-08-01 19:44:08]

@Gab: I have rebased this on your change, could you please take a final look?

https://codereview.chromium.org/271673006/diff/170001/chrome/browser/resource...
File chrome/browser/resources/options/settings_banner.css (right):

https://codereview.chromium.org/271673006/diff/170001/chrome/browser/resource...
chrome/browser/resources/options/settings_banner.css:5: /*
On 2014/08/01 18:28:52, Dan Beam wrote:
> nit: /* TODO

Done.

[gab, 2014-08-04 14:56:13]

lgtm, thanks!

PS: In the future, it would have been easier to review if you'd uploaded the
rebase as a separate patch set followed by your actual code changes.

[engedy, 2014-08-04 15:22:11]

On 2014/08/04 14:56:13, gab wrote:
> lgtm, thanks!
> 
> PS: In the future, it would have been easier to review if you'd uploaded the
> rebase as a separate patch set followed by your actual code changes.

Ahh, I forgot... sorry for the extra confusion.

[engedy, 2014-08-04 15:23:16]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2014-08-04 15:24:08]

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

[commit-bot: I haz the power, 2014-08-04 18:38:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[commit-bot: I haz the power, 2014-08-04 19:03:45]

Change committed as 287385