Post-cleanup settings reset.

chrome/browser/ui/startup/startup_browser_creator.cc

Index: chrome/browser/ui/startup/startup_browser_creator.cc
diff --git a/chrome/browser/ui/startup/startup_browser_creator.cc b/chrome/browser/ui/startup/startup_browser_creator.cc
index 994ee56a84c3b536e269df86b019f223e1604019..efb913d43c45309b0733a3281b1917d60c4d0e09 100644
--- a/chrome/browser/ui/startup/startup_browser_creator.cc
+++ b/chrome/browser/ui/startup/startup_browser_creator.cc
@@ -105,6 +105,7 @@
 #if defined(OS_WIN)
 #include "base/win/windows_version.h"
 #include "chrome/browser/metrics/jumplist_metrics_win.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h"
 #endif
 
 #if BUILDFLAG(ENABLE_PRINT_PREVIEW)
@@ -382,6 +383,15 @@ bool StartupBrowserCreator::LaunchBrowser(
 
   profile_launch_observer.Get().AddLaunched(profile);
 
+#if defined(OS_WIN)

[Peter Kasting, 2017/06/14 17:39:16]

Is this OS_WIN because we only ever want it to be on Windows, or because it's
only implemented on Windows for now?

In either case, but especially the latter, you may want to consider implementing
this such that it compiles on all platforms, and IsEnabled() returns false on
non-Windows.  This avoids #ifs.

[tmartino, 2017/06/14 21:48:35]

+1 regardless of where this code lives

[ftirelo, 2017/06/15 03:41:23]

Done.

[ftirelo, 2017/06/15 03:41:24]

Good idea. Done.

+  // Reset settings for the current profile if it's tagged to be reset after a
+  // complete run of the Chrome Cleanup tool.
+  if (safe_browsing::PostCleanupSettingsResetter::IsEnabled()) {
+    safe_browsing::PostCleanupSettingsResetter().ResetTaggedProfiles(

[Peter Kasting, 2017/06/14 17:39:16]

It feels as if this isn't really part of "launching the browser" (what this
function is about), but rather is just a thing that we want to happen near the
time of launching the browser.

Looking at callers of this function, I wonder if we really want this to happen
in FindOrCreateNewWindowForProfile().  It seems like we mostly care about
ProcessCmdLineImpl().  In turn we only care about some of the callers of that.

This is my way of saying "I'm not sure this is the right location to hook".  I
would think we'd want to hoist this somewhere else.  I'm not really sure because
I've never deeply looked into the startup sequence (and it's super complicated).
 Have you asked tmartino about this?  That's who I'd look to for expertise.

[tmartino, 2017/06/14 21:48:35]

Spoke offline with ftirelo and alito. The major themes that came up were:

1. We actually need to do this reset *before* launching a browser. Preferences
affected by the reset are used for, e.g., opening a home page on certain
profiles, and we definitely want to reset before determining what content is
shown at startup.

2. The fact that this doesn't have to do with "launching the browser" is
correct; in fact, it probably doesn't belong in startup_browser_creator at all.
It's a relatively free-standing data-cleanup operation on profiles that needs to
happen early in the startup process.

3. We're not super concerned about performance hits for wherever this lives. We
really don't care what the performance is in the rare case that we do hit a
triggered profile (the user is already expecting atypical one-off things to
happen on the next launch anyway) and it should be very inexpensive to check for
the trigger the rest of the time.

As a result, I think this probably belongs somewhere in chrome_browser_main or
chrome_browser_main_win. It'd be nice if there was a bit more structure to those
files, and a bit more obvious of a place for this to live in that
process...bonus points to anyone who wants to carve one out :)

[ftirelo, 2017/06/15 03:41:23]

Done.

[ftirelo, 2017/06/15 03:41:24]

Done.

+        {profile}, base::Bind([] {}), nullptr /* use default delegate */);

[Peter Kasting, 2017/06/14 17:39:16]

Nit: Can you just use OnceClosure() for the penultimate arg?  That seems
clearer.

[ftirelo, 2017/06/15 03:41:23]

Unfortunately, OnceClosure() will create an unbound callback, so that the
following lines will crash:
  auto c = base::OnceClosure();
  std::move(c).Run();

[Peter Kasting, 2017/06/15 06:28:19]

Can we fix that?  I thought the basic (non-Once) callback/closure objects could
be safely constructed and used (and would do nothing) in this kind of case.  It
seems surprising that OnceCallback doesn't allow this, and it seems like if we
use it more, more people are going to need ugly boilerplate like this.

Doesn't have to be done in this CL, but it would be nice to figure this out
separately.

[ftirelo, 2017/06/15 17:06:36]

I'm not sure of the consequences of the change and what assumptions that would
break, but it's worth a try. I will try a stab at that in a follow up CL.

+  }
+#endif
+
 #if defined(OS_CHROMEOS)
   chromeos::ProfileHelper::Get()->ProfileStartup(profile, process_startup);
 #endif