Disable AutoImport: experiment and histogram for analysis.

chrome/browser/importer/profile_writer.cc

Index: chrome/browser/importer/profile_writer.cc
diff --git a/chrome/browser/importer/profile_writer.cc b/chrome/browser/importer/profile_writer.cc
index 642bbf31567c2c7bcaf189d08041fd459c3a7396..07ebc98f5648d370601d5fc67ac1336f02320707 100644
--- a/chrome/browser/importer/profile_writer.cc
+++ b/chrome/browser/importer/profile_writer.cc
@@ -10,6 +10,7 @@
 #include <set>
 #include <string>
 
+#include "base/metrics/histogram.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -18,6 +19,7 @@
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
+#include "chrome/browser/first_run/first_run.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
 #include "chrome/browser/profiles/profile.h"
@@ -36,7 +38,6 @@
 #include "components/search_engines/template_url_service.h"
 
 #if defined(OS_WIN)
-#include "chrome/browser/web_data_service_factory.h"
 #include "components/password_manager/core/browser/webdata/password_web_data_service_win.h"
 #endif
 
@@ -65,7 +66,7 @@ base::string16 GenerateUniqueFolderName(BookmarkModel* model,
   // Otherwise iterate until we find a unique name.
   for (size_t i = 1; i <= existing_folder_names.size(); ++i) {
     base::string16 name = folder_name + base::ASCIIToUTF16(" (") +
-        base::SizeTToString16(i) + base::ASCIIToUTF16(")");
+                          base::SizeTToString16(i) + base::ASCIIToUTF16(")");
     if (existing_folder_names.find(name) == existing_folder_names.end())
       return name;
   }
@@ -92,14 +93,16 @@ bool ProfileWriter::TemplateURLServiceIsLoaded() const {
 }
 
 void ProfileWriter::AddPasswordForm(const autofill::PasswordForm& form) {
-  PasswordStoreFactory::GetForProfile(
-      profile_, ServiceAccessType::EXPLICIT_ACCESS)->AddLogin(form);
+  PasswordStoreFactory::GetForProfile(profile_,

[manzagop (departed), 2016/08/18 15:59:56]

Did git cl format do this? Typically you don't want to reformat lines you don't
change to avoid claming the "blame". Perhaps it's because of the line endings
conversions that happened.

I wouldn't worry about it this time.

Actually, looking below, there's quite a few lines. It seems you've only added
l.114-117, so you could checkout the file from master and paste your changes
back in, to get a cleaner diff.

[gcomanici, 2016/08/19 16:04:31]

Done.

+                                      ServiceAccessType::EXPLICIT_ACCESS)
+      ->AddLogin(form);
 }
 
 #if defined(OS_WIN)
 void ProfileWriter::AddIE7PasswordInfo(const IE7PasswordInfo& info) {
   WebDataServiceFactory::GetPasswordWebDataForProfile(
-      profile_, ServiceAccessType::EXPLICIT_ACCESS)->AddIE7Login(info);
+      profile_, ServiceAccessType::EXPLICIT_ACCESS)
+      ->AddIE7Login(info);
 }
 #endif
 
@@ -108,6 +111,10 @@ void ProfileWriter::AddHistoryPage(const history::URLRows& page,
   HistoryServiceFactory::GetForProfile(profile_,
                                        ServiceAccessType::EXPLICIT_ACCESS)
       ->AddPagesWithDetails(page, visit_source);
+  // Measure the size of the history page after AutoImport on first run.
+  if (first_run::IsChromeFirstRun()) {
+    UMA_HISTOGRAM_COUNTS("Import.SizeImportedHistory.AutoImport", page.size());

[manzagop (departed), 2016/08/18 15:59:56]

Is this called once per importer? Eg if I import both from firefox and IE. Also,
do we care about differentiating which importers are providing actual value (ie
visit_source)? Does the Import.ImporterType. histogram give you enough info.

Basically, depending on the channel, there's a high turn around time to get the
data, so you want the metrics to be right from the get go.

[gcomanici, 2016/08/18 19:40:00]

This is a very good point. I tried to see if there is an easy way to aggregate
the size of all imported histories. From what I gather that is not an easy task.

Import.ImporterType suggests that 80% of the time people import from IE and 15%
from EDGE (which is not in the enum history::VisitSourse - I assume it gets
labelled as IE). 

Do you think it is correct to restrict the metric to IE-imports only? If you are
OK with this, I will take care to modify the UMA details and the description
accordingly.

[manzagop (departed), 2016/08/18 20:35:29]

This sounds like a discussion for the bug, where you'll find the relevant
stakeholders (eg PMs). ;)

[gcomanici, 2016/08/19 16:04:31]

Acknowledged.

+  }
 }
 
 void ProfileWriter::AddHomepage(const GURL& home_page) {
@@ -176,10 +183,9 @@ void ProfileWriter::AddBookmarks(
       // to the bar.  The first time we do so, create the folder.
       if (!top_level_folder) {
         base::string16 name =
-            GenerateUniqueFolderName(model,top_level_folder_name);
-        top_level_folder = model->AddFolder(bookmark_bar,
-                                            bookmark_bar->child_count(),
-                                            name);
+            GenerateUniqueFolderName(model, top_level_folder_name);
+        top_level_folder =
+            model->AddFolder(bookmark_bar, bookmark_bar->child_count(), name);
       }
       parent = top_level_folder;
     }
@@ -214,12 +220,9 @@ void ProfileWriter::AddBookmarks(
     if (bookmark->is_folder) {
       model->AddFolder(parent, parent->child_count(), bookmark->title);
     } else {
-      model->AddURLWithCreationTimeAndMetaInfo(parent,
-                                               parent->child_count(),
-                                               bookmark->title,
-                                               bookmark->url,
-                                               bookmark->creation_time,
-                                               NULL);
+      model->AddURLWithCreationTimeAndMetaInfo(parent, parent->child_count(),
+                                               bookmark->title, bookmark->url,
+                                               bookmark->creation_time, NULL);
     }
   }
 
@@ -273,10 +276,9 @@ static std::string BuildHostPathKey(const TemplateURL* t_url,
     return HostPathKeyForURL(GURL(t_url->url()));
 
   if (t_url->url_ref().SupportsReplacement(search_terms_data)) {
-    return HostPathKeyForURL(GURL(
-        t_url->url_ref().ReplaceSearchTerms(
-            TemplateURLRef::SearchTermsArgs(base::ASCIIToUTF16("x")),
-            search_terms_data)));
+    return HostPathKeyForURL(GURL(t_url->url_ref().ReplaceSearchTerms(
+        TemplateURLRef::SearchTermsArgs(base::ASCIIToUTF16("x")),
+        search_terms_data)));
   }
   return std::string();
 }
@@ -288,13 +290,12 @@ static void BuildHostPathMap(TemplateURLService* model,
   TemplateURLService::TemplateURLVector template_urls =
       model->GetTemplateURLs();
   for (size_t i = 0; i < template_urls.size(); ++i) {
-    const std::string host_path = BuildHostPathKey(
-        template_urls[i], model->search_terms_data(), false);
+    const std::string host_path =
+        BuildHostPathKey(template_urls[i], model->search_terms_data(), false);
     if (!host_path.empty()) {
       const TemplateURL* existing_turl = (*host_path_map)[host_path];
-      if (!existing_turl ||
-          (template_urls[i]->show_in_default_list() &&
-           !existing_turl->show_in_default_list())) {
+      if (!existing_turl || (template_urls[i]->show_in_default_list() &&
+                             !existing_turl->show_in_default_list())) {
         // If there are multiple TemplateURLs with the same host+path, favor
         // those shown in the default list.  If there are multiple potential
         // defaults, favor the first one, which should be the more commonly used
@@ -327,14 +328,14 @@ void ProfileWriter::AddKeywords(ScopedVector<TemplateURL> template_urls,
     // sure the search engines we provide aren't replaced by those from the
     // imported browser.
     if (unique_on_host_and_path &&
-        (host_path_map.find(BuildHostPathKey(
-            *i, model->search_terms_data(), true)) != host_path_map.end()))
+        (host_path_map.find(BuildHostPathKey(*i, model->search_terms_data(),
+                                             true)) != host_path_map.end()))
       continue;
 
     // Only add valid TemplateURLs to the model.
     if ((*i)->url_ref().IsValid(model->search_terms_data())) {
       model->Add(*i);  // Takes ownership.
-      *i = NULL;  // Prevent the vector from deleting *i later.
+      *i = NULL;       // Prevent the vector from deleting *i later.
     }
   }
 }