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

chrome/browser/first_run/first_run.cc

 // Copyright (c) 2012 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.
 
 #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"
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
@@ skipping 173 matching lines @@
 
 void DoDelayedInstallExtensionsIfNeeded(
     installer::MasterPreferences* install_prefs) {
   base::DictionaryValue* extensions = 0;
   if (install_prefs->GetExtensionsBlock(&extensions)) {
     DVLOG(1) << "Extensions block found in master preferences";
     DoDelayedInstallExtensions();
   }
 }
 
-// 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
 // always block until done.
 void ImportFromSourceProfile(const importer::SourceProfile& source_profile,
                              Profile* target_profile,
                              uint16_t items_to_import) {
   // Deletes itself.
   ExternalProcessImporterHost* importer_host =
       new ExternalProcessImporterHost;
@@ skipping 30 matching lines @@
 #endif
   source_profile.source_path = base::FilePath(import_bookmarks_path_str);
 
   ImportFromSourceProfile(source_profile, profile, importer::FAVORITES);
   g_auto_import_state |= first_run::AUTO_IMPORT_BOOKMARKS_FILE_IMPORTED;
 }
 
 // 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;
 }
 
 GURL UrlFromString(const std::string& in) {
   return GURL(in);
 }
 
 void ConvertStringVectorToGURLVector(
     const std::vector<std::string>& src,
@@ skipping 152 matching lines @@
       if (g_browser_process->local_state()->GetBoolean(
           prefs::kDefaultBrowserSettingEnabled)) {
         shell_integration::SetAsDefaultBrowser();
       }
     } else if (make_chrome_default_for_user) {
       shell_integration::SetAsDefaultBrowser();
     }
   }
 }
 
+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 {
 namespace internal {
 
 void SetupMasterPrefsFromInstallPrefs(
     const installer::MasterPreferences& install_prefs,
     MasterPrefs* out_prefs) {
   ConvertStringVectorToGURLVector(
       install_prefs.GetFirstRunTabs(), &out_prefs->new_tabs);
 
   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(
           installer::master_preferences::kMakeChromeDefaultForUser,
           &value) && value) {
     out_prefs->make_chrome_default_for_user = true;
   }
 
   if (install_prefs.GetBool(
           installer::master_preferences::kSuppressFirstRunDefaultBrowserPrompt,
@@ skipping 52 matching lines @@
                                      bool no_first_run) {
   return (force_first_run || (!has_sentinel && !no_first_run))
              ? FIRST_RUN_TRUE
              : FIRST_RUN_FALSE;
 }
 
 }  // namespace internal
 
 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) {
 }
 
 MasterPrefs::~MasterPrefs() {}
 
 bool IsChromeFirstRun() {
   if (g_first_run == internal::FIRST_RUN_UNKNOWN) {
     const base::CommandLine* command_line =
@@ skipping 106 matching lines @@
   std::unique_ptr<installer::MasterPreferences> install_prefs(
       LoadMasterPrefs());
 
   // Default value in case master preferences is missing or corrupt, or
   // ping_delay is missing.
   out_prefs->ping_delay = 90;
   if (install_prefs.get()) {
     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.";
     }
 
     DoDelayedInstallExtensionsIfNeeded(install_prefs.get());
 
     internal::SetupMasterPrefsFromInstallPrefs(*install_prefs, out_prefs);
   }
 
   return FIRST_RUN_PROCEED;
 }
 
 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);
+  g_auto_import_state |= AUTO_IMPORT_CALLED;
 
-  // 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();
+  // 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 :) ).

 
-  // 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;
-      }
-    }
-
-    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);
+  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) {
   // Only set default browser after import as auto import relies on the current
   // default browser to know what to import from.
   ProcessDefaultBrowserPolicy(make_chrome_default_for_user);
 
   // Display the first run bubble if there is a default search provider.
   TemplateURLService* template_url =
       TemplateURLServiceFactory::GetForProfile(profile);
   if (template_url && template_url->GetDefaultSearchProvider())
     FirstRunBubbleLauncher::ShowFirstRunBubbleSoon();
   SetShouldShowWelcomePage();
   SetShouldDoPersonalDataManagerFirstRun();
 
   internal::DoPostImportPlatformSpecificTasks(profile);
 }
 
 uint16_t auto_import_state() {
   return g_auto_import_state;
 }
 
 }  // namespace first_run