Update AutoImport to import nothing by default (in absence of policy and master_prefs).

chrome/browser/first_run/first_run.cc

Index: chrome/browser/first_run/first_run.cc
diff --git a/chrome/browser/first_run/first_run.cc b/chrome/browser/first_run/first_run.cc
index a13c299c3ac9743173c7146daf6fbc8d08dcc7da..2027776665dcb039b83de07c58d33c745151cf61 100644
--- a/chrome/browser/first_run/first_run.cc
+++ b/chrome/browser/first_run/first_run.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/first_run/first_run.h"
 
 #include <algorithm>
+#include <memory>

[grt (UTC plus 2), 2017/02/23 14:05:48]

nit: still needed for std::unique_ptr

[gab, 2017/02/23 20:37:40]

Hmm, right, and this is adding it..?!

[grt (UTC plus 2), 2017/02/23 21:19:01]

Wow. My brain swapped adding and removing. Guess I didn't get enough sleep last
night...

 #include <utility>
 
 #include "base/command_line.h"
@@ -198,56 +199,6 @@ void DoDelayedInstallExtensionsIfNeeded(
   }
 }
 
-// Sets the |items| bitfield according to whether the import data specified by
-// |import_type| should be be auto imported or not.
-void SetImportItem(PrefService* user_prefs,
-                   const char* pref_path,
-                   int import_items,
-                   int dont_import_items,
-                   importer::ImportItem import_type,
-                   int* items) {
-  // Work out whether an item is to be imported according to what is specified
-  // in master preferences.
-  bool should_import = false;
-  bool master_pref_set =
-      ((import_items | dont_import_items) & import_type) != 0;
-  bool master_pref = ((import_items & ~dont_import_items) & import_type) != 0;
-
-  if (import_type == importer::HISTORY ||
-      (import_type != importer::FAVORITES &&
-       first_run::internal::IsOrganicFirstRun())) {
-    // History is always imported unless turned off in master_preferences.
-    // Search engines and home page are imported in organic builds only
-    // unless turned off in master_preferences.
-    should_import = !master_pref_set || master_pref;
-  } else {
-    // Bookmarks are never imported, unless turned on in master_preferences.
-    // Search engine and home page import behaviour is similar in non organic
-    // builds.
-    should_import = master_pref_set && master_pref;
-  }
-
-  // If an import policy is set, import items according to policy. If no master
-  // preference is set, but a corresponding recommended policy is set, import
-  // item according to recommended policy. If both a master preference and a
-  // recommended policy is set, the master preference wins. If neither
-  // recommended nor managed policies are set, import item according to what we
-  // worked out above.
-  if (master_pref_set)
-    user_prefs->SetBoolean(pref_path, should_import);
-
-  if (!user_prefs->FindPreference(pref_path)->IsDefaultValue()) {
-    if (user_prefs->GetBoolean(pref_path))
-      *items |= import_type;
-  } else {
-    // no policy (recommended or managed) is set
-    if (should_import)
-      *items |= import_type;
-  }
-
-  user_prefs->ClearPref(pref_path);
-}
-
 // Launches the import, via |importer_host|, from |source_profile| into
 // |target_profile| for the items specified in the |items_to_import| bitfield.
 // This may be done in a separate process depending on the platform, but it will
@@ -298,19 +249,16 @@ void ImportFromFile(Profile* profile,
 // Imports settings from the first profile in |importer_list|.
 void ImportSettings(Profile* profile,
                     std::unique_ptr<ImporterList> importer_list,
-                    int items_to_import) {
+                    uint16_t items_to_import) {
+  DCHECK(items_to_import);
   const importer::SourceProfile& source_profile =
       importer_list->GetSourceProfileAt(0);
-  // If no items to import then skip entirely.
-  if (!items_to_import)
-    return;
 
   // Ensure that importers aren't requested to import items that they do not
   // support. If there is no overlap, skip.
   items_to_import &= source_profile.services_supported;
-  if (items_to_import) {
+  if (items_to_import)
     ImportFromSourceProfile(source_profile, profile, items_to_import);
-  }
 
   g_auto_import_state |= first_run::AUTO_IMPORT_PROFILE_IMPORTED;
 }
@@ -483,6 +431,38 @@ void ProcessDefaultBrowserPolicy(bool make_chrome_default_for_user) {
   }
 }
 
+void MapLegacyDistroPrefs(base::DictionaryValue* master_prefs_dict) {

[grt (UTC plus 2), 2017/02/23 14:05:48]

MasterPreferences::EnforceLegacyPreferences does something kinda like this for
other prefs. can these two similar operations happen in the same place? i'd
almost lean toward removing the installer::master_preferences constants and
using inline literals in this mapping to make it impossible for someone to
accidentally use them in the future.

[gab, 2017/02/23 20:37:40]

Done.

+  const base::DictionaryValue* distro_dict;
+  if (!master_prefs_dict->GetDictionary(
+          installer::master_preferences::kDistroDict, &distro_dict)) {
+    return;
+  }
+
+  struct LegacyDistroPrefMapping {

[grt (UTC plus 2), 2017/02/23 14:05:48]

  static constexpr struct {... (see comment below)

[gab, 2017/02/23 20:37:40]

Done.

+    const char* old_distro_pref_path;
+    const char* modern_pref_path;
+  } static constexpr kLegacyDistroPrefMappings[] = {
+      {installer::master_preferences::kDistroImportHistoryPref,
+       prefs::kImportHistory},
+      {installer::master_preferences::kDistroImportHomePagePref,
+       prefs::kImportHomepage},
+      {installer::master_preferences::kDistroImportSearchPref,
+       prefs::kImportSearchEngine},
+      {installer::master_preferences::kDistroImportBookmarksPref,
+       prefs::kImportBookmarks},
+  };
+
+  for (const auto& mapping : kLegacyDistroPrefMappings) {
+    const base::Value* value;
+    if (distro_dict->Get(mapping.old_distro_pref_path, &value)) {
+      bool bool_value = false;
+      bool success = value->GetAsBoolean(&bool_value);
+      DCHECK(success) << "This method assumes boolean mappings";
+      master_prefs_dict->SetBoolean(mapping.modern_pref_path, bool_value);
+    }
+  }
+}
+
 }  // namespace
 
 namespace first_run {
@@ -497,57 +477,16 @@ void SetupMasterPrefsFromInstallPrefs(
   install_prefs.GetInt(installer::master_preferences::kDistroPingDelay,
                        &out_prefs->ping_delay);
 
-  bool value = false;
-  if (install_prefs.GetBool(
-          installer::master_preferences::kDistroImportSearchPref, &value)) {
-    if (value) {
-      out_prefs->do_import_items |= importer::SEARCH_ENGINES;
-    } else {
-      out_prefs->dont_import_items |= importer::SEARCH_ENGINES;
-    }
-  }
-
   // If we're suppressing the first-run bubble, set that preference now.
   // Otherwise, wait until the user has completed first run to set it, so the
   // user is guaranteed to see the bubble iff they have completed the first run
   // process.
+  bool value = false;
   if (install_prefs.GetBool(
           installer::master_preferences::kDistroSuppressFirstRunBubble,
-          &value) && value)
+          &value) &&
+      value) {
     SetShowFirstRunBubblePref(FIRST_RUN_BUBBLE_SUPPRESS);
-
-  if (install_prefs.GetBool(
-          installer::master_preferences::kDistroImportHistoryPref,
-          &value)) {
-    if (value) {
-      out_prefs->do_import_items |= importer::HISTORY;
-    } else {
-      out_prefs->dont_import_items |= importer::HISTORY;
-    }
-  }
-
-  std::string not_used;
-  out_prefs->homepage_defined = install_prefs.GetString(
-      prefs::kHomePage, &not_used);
-
-  if (install_prefs.GetBool(
-          installer::master_preferences::kDistroImportHomePagePref,
-          &value)) {
-    if (value) {
-      out_prefs->do_import_items |= importer::HOME_PAGE;
-    } else {
-      out_prefs->dont_import_items |= importer::HOME_PAGE;
-    }
-  }
-
-  // Bookmarks are never imported unless specifically turned on.
-  if (install_prefs.GetBool(
-          installer::master_preferences::kDistroImportBookmarksPref,
-          &value)) {
-    if (value)
-      out_prefs->do_import_items |= importer::FAVORITES;
-    else
-      out_prefs->dont_import_items |= importer::FAVORITES;
   }
 
   if (install_prefs.GetBool(
@@ -620,9 +559,6 @@ FirstRunState DetermineFirstRunState(bool has_sentinel,
 
 MasterPrefs::MasterPrefs()
     : ping_delay(0),
-      homepage_defined(false),
-      do_import_items(0),
-      dont_import_items(0),
       make_chrome_default_for_user(false),
       suppress_first_run_default_browser_prompt(false),
       welcome_page_on_os_upgrade_enabled(true) {
@@ -749,9 +685,13 @@ ProcessMasterPreferencesResult ProcessMasterPreferences(
     if (!internal::ShowPostInstallEULAIfNeeded(install_prefs.get()))
       return EULA_EXIT_NOW;
 
+    std::unique_ptr<base::DictionaryValue> master_prefs_dict =
+        install_prefs->master_dictionary().CreateDeepCopy();
+    MapLegacyDistroPrefs(master_prefs_dict.get());
+
     if (!chrome_prefs::InitializePrefsFromMasterPrefs(
             profiles::GetDefaultProfileDir(user_data_dir),
-            install_prefs->master_dictionary())) {
+            std::move(master_prefs_dict))) {
       DLOG(ERROR) << "Failed to initialize from master_preferences.";
     }
 
@@ -765,84 +705,54 @@ ProcessMasterPreferencesResult ProcessMasterPreferences(
 
 void AutoImport(
     Profile* profile,
-    bool homepage_defined,
-    int import_items,
-    int dont_import_items,
     const std::string& import_bookmarks_path) {
-  base::FilePath local_state_path;
-  PathService::Get(chrome::FILE_LOCAL_STATE, &local_state_path);
-  bool local_state_file_exists = base::PathExists(local_state_path);
-
-  // It may be possible to do the if block below asynchronously. In which case,
-  // get rid of this RunLoop. http://crbug.com/366116.
-  base::RunLoop run_loop;
-  std::unique_ptr<ImporterList> importer_list(new ImporterList());
-  importer_list->DetectSourceProfiles(
-      g_browser_process->GetApplicationLocale(),
-      false,  // include_interactive_profiles?
-      run_loop.QuitClosure());
-  run_loop.Run();
-
-  // Do import if there is an available profile for us to import.
-  if (importer_list->count() > 0) {
-    if (internal::IsOrganicFirstRun()) {
-      // Home page is imported in organic builds only unless turned off or
-      // defined in master_preferences.
-      if (homepage_defined) {
-        dont_import_items |= importer::HOME_PAGE;
-        if (import_items & importer::HOME_PAGE)
-          import_items &= ~importer::HOME_PAGE;
-      }
-      // Search engines are not imported automatically in organic builds if the
-      // user already has a user preferences directory.
-      if (local_state_file_exists) {
-        dont_import_items |= importer::SEARCH_ENGINES;
-        if (import_items & importer::SEARCH_ENGINES)
-          import_items &= ~importer::SEARCH_ENGINES;
-      }
-    }
+  g_auto_import_state |= AUTO_IMPORT_CALLED;
 
-    PrefService* user_prefs = profile->GetPrefs();
-    int items = 0;
-
-    SetImportItem(user_prefs,
-                  prefs::kImportHistory,
-                  import_items,
-                  dont_import_items,
-                  importer::HISTORY,
-                  &items);
-    SetImportItem(user_prefs,
-                  prefs::kImportHomepage,
-                  import_items,
-                  dont_import_items,
-                  importer::HOME_PAGE,
-                  &items);
-    SetImportItem(user_prefs,
-                  prefs::kImportSearchEngine,
-                  import_items,
-                  dont_import_items,
-                  importer::SEARCH_ENGINES,
-                  &items);
-    SetImportItem(user_prefs,
-                  prefs::kImportBookmarks,
-                  import_items,
-                  dont_import_items,
-                  importer::FAVORITES,
-                  &items);
-
-    importer::LogImporterUseToMetrics(
-        "AutoImport", importer_list->GetSourceProfileAt(0).importer_type);
-
-    ImportSettings(profile, std::move(importer_list), items);
+  // Use |profile|'s PrefService to determine what to import. It will reflect in
+  // order:
+  //  1) Policies.
+  //  2) Master preferences (used to initialize user prefs in
+  //     ProcessMasterPreferences()).
+  //  3) Recommended policies.
+  //  4) Registered default.
+  PrefService* prefs = profile->GetPrefs();
+  uint16_t items_to_import = 0;
+  struct ImportItems {
+    const char* pref_path;
+    importer::ImportItem bit;
+  } static constexpr kImportItems[] = {

[grt (UTC plus 2), 2017/02/23 14:05:48]

nit: "static constexpr" should come first, and this type doesn't really need a
name, so this could become:
  static constexpr struct {
    ....
  } kImportItems[] = {

[gab, 2017/02/23 20:37:40]

Done.

+      {prefs::kImportHistory, importer::HISTORY},

[grt (UTC plus 2), 2017/02/23 14:05:48]

the pedant in me would like to see this sorted either by pref name or by the
position of the ImportItem bit. :-)

[gab, 2017/02/23 20:37:40]

Done.

+      {prefs::kImportHomepage, importer::HOME_PAGE},
+      {prefs::kImportSearchEngine, importer::SEARCH_ENGINES},
+      {prefs::kImportBookmarks, importer::FAVORITES},
+  };

[grt (UTC plus 2), 2017/02/23 14:05:48]

prefs::kImportAutofillFormData and prefs::kImportSavedPasswords are notably
absent. why? is fixing https://crbug.com/601697 now as simple as adding
{prefs::kImportSavedPasswords, importer::PASSWORDS} here? if i'm not mistaken,
adding both will suddenly make the GP settings work as documented, right?

https://www.chromium.org/administrators/policy-list-3#ImportAutofillFormData
https://www.chromium.org/administrators/policy-list-3#ImportSavedPasswords

[gab, 2017/02/23 20:37:40]

Interesting, I guess it probably is =D (though in practice it will depend on
that importer's support for these ImportItems).

Done.

One thing to note: I've long thought we should split first run only
master_preferences from prefs. On the other hand, policies only work with real
prefs (so it's kind of weird to have master_preferences only things -- i.e.
kDistro*). And the prefs::kImport* values happen to be used at runtime after
first_run in a few corner UIs so I figured they might as well all use the same
thing (those used to not be directly seeded by master_preferences but I don't
think it's wrong that they now be).

Also, while writing this CL I realized we never even register any distro prefs
(so I just went ahead and added logic to strip the kDistroDict from
master_preferences before mapping them to Preferences -- as well as cleaning it
from existing Preferences :) ).

+
+  for (const auto& import_item : kImportItems) {
+    if (prefs->GetBoolean(import_item.pref_path))
+      items_to_import |= import_item.bit;
   }
 
-  if (!import_bookmarks_path.empty()) {
-    ImportFromFile(profile, import_bookmarks_path);
+  if (items_to_import) {
+    // It may be possible to do the if block below asynchronously. In which
+    // case, get rid of this RunLoop. http://crbug.com/366116.
+    base::RunLoop run_loop;
+    std::unique_ptr<ImporterList> importer_list(new ImporterList());

[grt (UTC plus 2), 2017/02/23 14:05:48]

nit:
    auto importer_list = base::MakeUnique<ImporterList>();

[gab, 2017/02/23 20:37:40]

this was cut-paste from above but why not :)

+    importer_list->DetectSourceProfiles(
+        g_browser_process->GetApplicationLocale(),
+        false,  // include_interactive_profiles?
+        run_loop.QuitClosure());
+    run_loop.Run();
+
+    if (importer_list->count() > 0) {
+      importer::LogImporterUseToMetrics(
+          "AutoImport", importer_list->GetSourceProfileAt(0).importer_type);
+
+      ImportSettings(profile, std::move(importer_list), items_to_import);
+    }
   }
 
-  content::RecordAction(UserMetricsAction("FirstRunDef_Accept"));
-
-  g_auto_import_state |= AUTO_IMPORT_CALLED;
+  if (!import_bookmarks_path.empty())
+    ImportFromFile(profile, import_bookmarks_path);
 }
 
 void DoPostImportTasks(Profile* profile, bool make_chrome_default_for_user) {