Disable AutoImport: experiment and histogram for analysis.

chrome/browser/chrome_browser_main.cc

Index: chrome/browser/chrome_browser_main.cc
diff --git a/chrome/browser/chrome_browser_main.cc b/chrome/browser/chrome_browser_main.cc
index 262d91c8b06ff3eeefc1474964e5284c8d531fe5..c0603dd587f3dd4e3fc91d55359dd90f4d7593b9 100644
--- a/chrome/browser/chrome_browser_main.cc
+++ b/chrome/browser/chrome_browser_main.cc
@@ -33,6 +33,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/sys_info.h"
@@ -205,6 +206,7 @@
 
 #if defined(OS_WIN)
 #include "base/trace_event/trace_event_etw_export_win.h"
+#include "base/win/win_util.h"
 #include "base/win/windows_version.h"
 #include "chrome/app/file_pre_reader_win.h"
 #include "chrome/browser/chrome_browser_main_win.h"
@@ -290,6 +292,10 @@ using content::BrowserThread;
 
 namespace {
 
+// feature used to remove AutoImport on first run is default-off
+const base::Feature kDisableFirstRunAutoImport{
+    "DisableFirstRunAutoImport", base::FEATURE_DISABLED_BY_DEFAULT};

[Ilya Sherman, 2016/08/22 21:10:45]

Please define this feature in
https://cs.chromium.org/chromium/src/chrome/common/chrome_features.h

[gcomanici, 2016/08/23 16:31:59]

Done.

+
 // A provider of Geolocation services to override AccessTokenStore.
 class ChromeGeolocationDelegate : public device::GeolocationDelegate {
  public:
@@ -1798,11 +1804,24 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
   // preferences are registered, since some of the code that the importer
   // touches reads preferences.
   if (first_run::IsChromeFirstRun()) {
-    first_run::AutoImport(profile_,
-                          master_prefs_->homepage_defined,
-                          master_prefs_->do_import_items,
-                          master_prefs_->dont_import_items,
-                          master_prefs_->import_bookmarks_path);
+    // By default, autoImport is performed on first run.

[Ilya Sherman, 2016/08/22 21:10:45]

nit: Please try to be consist about how autoImport vs. "AutoImport" vs.
"auto-import" (vs. any other variants) is spelled.

+    bool perform_autoImport_on_first_run = true;

[Ilya Sherman, 2016/08/22 21:10:45]

nit: s/autoImport/auto_import

[Ilya Sherman, 2016/08/22 21:10:45]

nit: I'd drop the "_on_first_run" suffix, since this code is already within the
body of an IsChromeFirstRun() if-stmt.

[gcomanici, 2016/08/23 16:31:59]

Done.

+  // Part of a field trial that is designed to turn off the AutoImport feature
+  // for Windows users (on first run).
+  // The feature keeps running for enterprise.
+  #if defined(OS_WIN)

[Ilya Sherman, 2016/08/22 21:10:45]

Please control the platform via the field trial's server-side config, rather
than via a #define, unless you have a really compelling reason to use a #define.
 If you do, please document it here =)

[gcomanici, 2016/08/23 16:31:59]

Done.

+  base::FieldTrial* trial =
+          base::FieldTrialList::Find("DisableFirstRunAutoImport");

[Ilya Sherman, 2016/08/22 21:10:45]

There's no need to call this when using the Feature API.

[gcomanici, 2016/08/23 16:31:59]

Acknowledged.

+  perform_autoImport_on_first_run = !trial ||
+        !base::FeatureList::IsEnabled(kDisableFirstRunAutoImport) ||
+        base::win::IsEnrolledToDomain();

[Ilya Sherman, 2016/08/22 21:10:45]

Please make the IsEnrolledToDomain() check first.  That way, users who are in
enterprise domains won't be considered as "active" in the field trial, which
will reduce noise when viewing the metrics.

[gcomanici, 2016/08/23 16:31:59]

Done.

+  #endif

[Ilya Sherman, 2016/08/22 21:10:45]

The indentation looks off for these lines.  Please run git cl format over this
CL -- it's a handy tool =)  (Apologies if you've already run it, and I'm
misunderstanding something.)

[gcomanici, 2016/08/23 16:31:59]

Acknowledged.

+    if (perform_autoImport_on_first_run) {
+      first_run::AutoImport(profile_, master_prefs_->homepage_defined,
+                            master_prefs_->do_import_items,
+                            master_prefs_->dont_import_items,
+                            master_prefs_->import_bookmarks_path);
+    }
 
     // Note: this can pop the first run consent dialog on linux.
     first_run::DoPostImportTasks(profile_,