Post-cleanup settings reset.

chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_SAFE_BROWSING_CHROME_CLEANER_SETTINGS_RESETTER_WIN_H_
+#define CHROME_BROWSER_SAFE_BROWSING_CHROME_CLEANER_SETTINGS_RESETTER_WIN_H_
+
+#include <memory>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "chrome/browser/safe_browsing/settings_reset_prompt/default_settings_fetcher.h"
+
+class Profile;
+class ProfileResetter;
+
+namespace user_prefs {
+class PrefRegistrySyncable;
+}  // namespace user_prefs
+
+namespace safe_browsing {
+
+class PostCleanupSettingsResetter {
+ public:
+  class Delegate {
+   public:
+    Delegate();
+    virtual ~Delegate();
+
+    virtual void FetchDefaultSettings(
+        DefaultSettingsFetcher::SettingsCallback callback);
+
+    virtual std::unique_ptr<ProfileResetter> GetProfileResetter(
+        Profile* profile);
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Delegate);
+  };
+
+  // Registers the settings reset pending tracked preference.
+  static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
+
+  // Returns true if post-cleanup settings reset is enabled.
+  static bool IsEnabled();
+
+  // Factory method for this class. Returns null if post-cleanup settings reset
+  // is disabled. Ownership belongs to the caller that can either directly
+  // delete it or post a ReleaseClosure() task once the object is no longer
+  // needed.
+  static PostCleanupSettingsResetter* Create();

[alito, 2017/06/09 01:52:07]

since ownership is being passed to caller, is there any reason for not returning
a std::unique_ptr?

[alito, 2017/06/09 01:52:07]

Please move this so it is the first function in the class (as per
https://google.github.io/styleguide/cppguide.html#Declaration_Order). Also move
its definition in the .cc file to match the order here.

[ftirelo, 2017/06/09 21:24:55]

Done.

[ftirelo, 2017/06/09 21:24:55]

Done.

+
+  // Returns a closure that releases the current object and runs |continuation|.
+  base::OnceClosure ReleaseWithContinuationClosure(

[alito, 2017/06/09 01:52:07]

It seems to me that neither TagForResetting nor ResetTaggedProfiles require that
the PostCleanupSettingsResetter object be kept alive after the functions have
returned. So are these release closures really needed? AFAICT, the closure
passed to ResetTaggedProfiles will be called after all profiles have been reset
even if the PostCleanupSettingsResetter object is not around any more.

[ftirelo, 2017/06/09 21:24:55]

Good point. Added some comments to make sure we don't change that assumption in
the future.

+      base::OnceClosure continuation);
+
+  // Returns a closure that releases the current object. Shorthand for
+  // ReleaseAndContinueClosure() when no additional code needs to be run once
+  // the current object is deleted.
+  base::OnceClosure ReleaseClosure();
+
+  // Tags |profile| to have its settings reset once the current cleanup
+  // finishes, by means of ResetPostCleanupSettingsIfTagged(). If a cleanup

[alito, 2017/06/09 01:52:07]

nit: ResetPostCleanupSettingsIfTagged no longer exists.

[ftirelo, 2017/06/09 21:24:55]

Done.

+  // doesn't require a reboot and the browser is still running once the cleanup
+  // finishes, then settings will be reset as soon as the cleaner process
+  // returns; if the user closes the browser while the cleanup is ongoing, then
+  // settings will be reset on the first browser restart that happens after the

[alito, 2017/06/09 01:52:07]

Please update the comment to reflect the fact that reset only happens when the
browser is started with the tagged profile. Same thing for the next sentence
just after this one.

In fact, I would suggest moving these more global explanations of how this class
is supposed to be used to the top of the class declaration instead of next to
its individual members.

[ftirelo, 2017/06/09 21:24:55]

Done.

+  // cleaner process ends. If the cleanup requires a reboot, then settings will
+  // be reset on the first browser restart that happens after reboot.
+  void TagForResetting(Profile* profile);
+
+  // Resets settings for the profiles in |profiles| there are tagged for
+  // resetting and a cleanup has completed. If a cleanup doesn't require a

[alito, 2017/06/09 01:52:07]

nit: "and a cleanup" -> "if cleanup"

[alito, 2017/06/09 01:52:07]

The part of the comment that starts with "If a cleanup doesn't require a reboot"
is not explaining what the function does, but how it is used currently by
clients. This can easily get out of date. I think it is enough to just explain
what the function is supposed to do and its contract with its clients.

[ftirelo, 2017/06/09 21:24:55]

Done.

[ftirelo, 2017/06/09 21:24:55]

Done.

+  // reboot and the session that started the cleanup is still alive, then
+  // the settings for all open profiles will be reset at once. Otherwise,
+  // for each profile that was open when the cleanup started, this will be
+  // called the next a browser is launched for the profile.
+  void ResetTaggedProfiles(
+      std::vector<Profile*> profiles,
+      base::OnceClosure continuation,
+      std::unique_ptr<PostCleanupSettingsResetter::Delegate> delegate =
+          nullptr);
+
+ protected:
+  PostCleanupSettingsResetter();
+
+  bool CopyProfilesToReset(const std::vector<Profile*>& profiles,

[alito, 2017/06/09 01:52:07]

Please move this to the unnamed namespace in the .cc file.

[ftirelo, 2017/06/09 21:24:55]

Done.

+                           std::vector<Profile*>* profiles_to_reset);
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PostCleanupSettingsResetter);
+};
+
+}  // namespace safe_browsing
+
+#endif  // CHROME_BROWSER_SAFE_BROWSING_CHROME_CLEANER_SETTINGS_RESETTER_WIN_H_