Disable AutoImport: experiment and histogram for analysis.

chrome/browser/importer/profile_writer.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/importer/profile_writer.h"
 
 #include <stddef.h>
 
 #include <map>
 #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"
 #include "base/threading/thread.h"
 #include "build/build_config.h"
 #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"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/web_data_service_factory.h"
 #include "chrome/common/importer/imported_bookmark_entry.h"
 #include "chrome/common/pref_names.h"
 #include "components/autofill/core/browser/webdata/autofill_webdata_service.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/common/bookmark_pref_names.h"
 #include "components/favicon/core/favicon_service.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/password_manager/core/browser/password_store.h"
 #include "components/prefs/pref_service.h"
 #include "components/search_engines/template_url.h"
 #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
 
 using bookmarks::BookmarkModel;
 using bookmarks::BookmarkNode;
 
 namespace {
 
 // Generates a unique folder name. If |folder_name| is not unique, then this
 // repeatedly tests for '|folder_name| + (i)' until a unique name is found.
 base::string16 GenerateUniqueFolderName(BookmarkModel* model,
                                         const base::string16& folder_name) {
   // Build a set containing the bookmark bar folder names.
   std::set<base::string16> existing_folder_names;
   const BookmarkNode* bookmark_bar = model->bookmark_bar_node();
   for (int i = 0; i < bookmark_bar->child_count(); ++i) {
     const BookmarkNode* node = bookmark_bar->GetChild(i);
     if (node->is_folder())
       existing_folder_names.insert(node->GetTitle());
   }
 
   // If the given name is unique, use it.
   if (existing_folder_names.find(folder_name) == existing_folder_names.end())
     return folder_name;
 
   // 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;
   }
 
   NOTREACHED();
   return folder_name;
 }
 
 // Shows the bookmarks toolbar.
 void ShowBookmarkBar(Profile* profile) {
   profile->GetPrefs()->SetBoolean(bookmarks::prefs::kShowBookmarkBar, true);
 }
 
 }  // namespace
 
 ProfileWriter::ProfileWriter(Profile* profile) : profile_(profile) {}
 
 bool ProfileWriter::BookmarkModelIsLoaded() const {
   return BookmarkModelFactory::GetForBrowserContext(profile_)->loaded();
 }
 
 bool ProfileWriter::TemplateURLServiceIsLoaded() const {
   return TemplateURLServiceFactory::GetForProfile(profile_)->loaded();
 }
 
 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
 
 void ProfileWriter::AddHistoryPage(const history::URLRows& page,
                                    history::VisitSource visit_source) {
   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) {
   DCHECK(profile_);
 
   PrefService* prefs = profile_->GetPrefs();
   // NOTE: We set the kHomePage value, but keep the NewTab page as the homepage.
   const PrefService::Preference* pref = prefs->FindPreference(prefs::kHomePage);
   if (pref && !pref->IsManaged()) {
     prefs->SetString(prefs::kHomePage, home_page.spec());
@@ skipping 48 matching lines @@
 
     const BookmarkNode* parent = NULL;
     if (import_to_top_level && (add_all_to_top_level || bookmark->in_toolbar)) {
       // Add directly to the bookmarks bar.
       parent = bookmark_bar;
     } else {
       // Add to a folder that will contain all the imported bookmarks not added
       // 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;
     }
 
     // Ensure any enclosing folders are present in the model.  The bookmark's
     // enclosing folder structure should be
     //   path[0] > path[1] > ... > path[size() - 1]
     for (std::vector<base::string16>::const_iterator folder_name =
              bookmark->path.begin();
          folder_name != bookmark->path.end(); ++folder_name) {
@@ skipping 14 matching lines @@
       }
       if (!child)
         child = model->AddFolder(parent, parent->child_count(), *folder_name);
       parent = child;
     }
 
     folders_added_to.insert(parent);
     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);
     }
   }
 
   // In order to keep the imported-to folders from appearing in the 'recently
   // added to' combobox, reset their modified times.
   for (std::set<const BookmarkNode*>::const_iterator i =
            folders_added_to.begin();
        i != folders_added_to.end(); ++i) {
     model->ResetDateFolderModified(*i);
   }
@@ skipping 33 matching lines @@
 // matches our prepopulate data. IE's URL for Live Search is something like
 // 'http://...{Language}...'. As {Language} is not a valid OSDD parameter value
 // the TemplateURL is invalid.
 static std::string BuildHostPathKey(const TemplateURL* t_url,
                                     const SearchTermsData& search_terms_data,
                                     bool try_url_if_invalid) {
   if (try_url_if_invalid && !t_url->url_ref().IsValid(search_terms_data))
     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();
 }
 
 // Builds a set that contains an entry of the host+path for each TemplateURL in
 // the TemplateURLService that has a valid search url.
 static void BuildHostPathMap(TemplateURLService* model,
                              HostPathMap* host_path_map) {
   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
         // one.
         (*host_path_map)[host_path] = template_urls[i];
       }
     }  // else case, TemplateURL doesn't have a search url, doesn't support
        // replacement, or doesn't have valid GURL. Ignore it.
   }
 }
@@ skipping 12 matching lines @@
     // TemplateURL with this keyword, don't import it again.
     if (model->GetTemplateURLForKeyword((*i)->keyword()) != NULL)
       continue;
 
     // For search engines if there is already a keyword with the same
     // host+path, we don't import it. This is done to avoid both duplicate
     // search providers (such as two Googles, or two Yahoos) as well as making
     // 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.
     }
   }
 }
 
 void ProfileWriter::AddAutofillFormDataEntries(
     const std::vector<autofill::AutofillEntry>& autofill_entries) {
   scoped_refptr<autofill::AutofillWebDataService> web_data_service =
       WebDataServiceFactory::GetAutofillWebDataForProfile(
           profile_, ServiceAccessType::EXPLICIT_ACCESS);
   if (web_data_service.get())
     web_data_service->UpdateAutofillEntries(autofill_entries);
 }
 
 ProfileWriter::~ProfileWriter() {}