OOP import on Windows.

Gets rid of the import process on all platforms -- replaced by a utility process which communicates with a ProfileWriter back in the browser process to write to the profile (this uses the ExternalProcessImporterHost machinery written 2+ years ago by mirandac@ for import on Mac and still the state of the art today).

Gets rid of all issues regarding profile contention and races to profile creation between the browser and import processes on first run (example of issues this has previously caused: http://crbug.com/171475, http://crbug.com/174591, http://crbug.com/180459).

Makes bookmarks file import use the same mechanism on all platforms (this means bookmarks file import is now in-process on Linux which still uses ImporterHost (instead of ExternalProcessImporterHost) -- Linux used to use the import process solely for bookmarks file import -- but the work to switch Linux to ExternalProcessImporterHost should be very minimal after this CL and I intend to do it in a follow-up CL to unify the import flow cross-platform once and for all!).

Do not use the out-of-process import for Google Toolbar (this was already the case prior to this CL).
To make the Google Toolbar importer work out-of-process, we would have to augment the import IPC drastically to support the web auth flow required by this importer (it requires to login to import the google.com/bookmarks favorites).

This, as a side-effect, brings silent bookmarks file import from master_preferences to Mac (long standing issue 48880).

Also fixes issue 231710 (or at least removes the condition causing the bug by making the ImportLockDialog go away on first run on Windows -- as should already have been the case).

Also addresses issue 178083 since the early message loop spinning was caused by ImportSettingsWin which was called too early on Windows (actually resulting in running the full import twice on Windows!) -- via PreCreateThreadsImpl()-->ProcessMasterPreferences()-->SetImportPreferencesAndLaunchImport()-->ImportSettingsWin()... This whole flow is removed in this CL :).

This improves first run speed in a debug build from 4901ms to 1477ms, a 332% improvement!!!! (tested by instrumenting a first run browser test, the delta is between the time the test is constructed and the time the test case is called (which happens after the browser has been initialized and import has occurred)).

This supersedes https://codereview.chromium.org/12463030/ (which won't be committed because this fix is so much better).

BUG=219419, 22142, 56816, 178083, 178051, 48880, 232241, 231710, 223462, 87657, 236225

Review URL: https://chromiumcodereview.appspot.com/12670013

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@201837 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 40 insertions, 25 deletions

diff --git a/chrome/browser/importer/external_process_importer_bridge.cc b/chrome/browser/importer/external_process_importer_bridge.cc
index a2ac59b..6271210c 100644
--- a/chrome/browser/importer/external_process_importer_bridge.cc
+++ b/chrome/browser/importer/external_process_importer_bridge.cc
@@ -21,12 +21,14 @@
 #endif
 
 namespace {
+
 // Rather than sending all import items over IPC at once we chunk them into
 // separate requests.  This avoids the case of a large import causing
 // oversized IPC messages.
 const int kNumBookmarksToSend = 100;
 const int kNumHistoryRowsToSend = 100;
 const int kNumFaviconsToSend = 100;
+
 }
 
 ExternalProcessImporterBridge::ExternalProcessImporterBridge(
@@ -46,18 +48,23 @@ void ExternalProcessImporterBridge::AddBookmarks(
   Send(new ProfileImportProcessHostMsg_NotifyBookmarksImportStart(
       first_folder_name, bookmarks.size()));
 
-  std::vector<ImportedBookmarkEntry>::const_iterator it;
-  for (it = bookmarks.begin(); it < bookmarks.end();
-       it = it + kNumBookmarksToSend) {
+  // |bookmarks_left| is required for the checks below as Windows has a
+  // Debug bounds-check which prevents pushing an iterator beyond its end()
+  // (i.e., |it + 2 < s.end()| crashes in debug mode if |i + 1 == s.end()|).
+  int bookmarks_left = bookmarks.end() - bookmarks.begin();
+  for (std::vector<ImportedBookmarkEntry>::const_iterator it =
+           bookmarks.begin(); it < bookmarks.end();) {
     std::vector<ImportedBookmarkEntry> bookmark_group;
     std::vector<ImportedBookmarkEntry>::const_iterator end_group =
-        it + kNumBookmarksToSend < bookmarks.end() ?
-        it + kNumBookmarksToSend : bookmarks.end();
+        it + std::min(bookmarks_left, kNumBookmarksToSend);
     bookmark_group.assign(it, end_group);
 
     Send(new ProfileImportProcessHostMsg_NotifyBookmarksImportGroup(
         bookmark_group));
+    bookmarks_left -= end_group - it;
+    it = end_group;
   }
+  DCHECK_EQ(0, bookmarks_left);
 }
 
 void ExternalProcessImporterBridge::AddHomePage(const GURL& home_page) {
@@ -76,17 +83,23 @@ void ExternalProcessImporterBridge::SetFavicons(
   Send(new ProfileImportProcessHostMsg_NotifyFaviconsImportStart(
     favicons.size()));
 
-  std::vector<ImportedFaviconUsage>::const_iterator it;
-  for (it = favicons.begin(); it < favicons.end();
-       it = it + kNumFaviconsToSend) {
+  // |favicons_left| is required for the checks below as Windows has a
+  // Debug bounds-check which prevents pushing an iterator beyond its end()
+  // (i.e., |it + 2 < s.end()| crashes in debug mode if |i + 1 == s.end()|).
+  int favicons_left = favicons.end() - favicons.begin();
+  for (std::vector<ImportedFaviconUsage>::const_iterator it =
+           favicons.begin(); it < favicons.end();) {
     std::vector<ImportedFaviconUsage> favicons_group;
     std::vector<ImportedFaviconUsage>::const_iterator end_group =
-        std::min(it + kNumFaviconsToSend, favicons.end());
+        it + std::min(favicons_left, kNumFaviconsToSend);
     favicons_group.assign(it, end_group);
 
-  Send(new ProfileImportProcessHostMsg_NotifyFaviconsImportGroup(
-      favicons_group));
+    Send(new ProfileImportProcessHostMsg_NotifyFaviconsImportGroup(
+        favicons_group));
+    favicons_left -= end_group - it;
+    it = end_group;
   }
+  DCHECK_EQ(0, favicons_left);
 }
 
 void ExternalProcessImporterBridge::SetHistoryItems(
@@ -94,18 +107,22 @@ void ExternalProcessImporterBridge::SetHistoryItems(
     history::VisitSource visit_source) {
   Send(new ProfileImportProcessHostMsg_NotifyHistoryImportStart(rows.size()));
 
-  history::URLRows::const_iterator it;
-  for (it = rows.begin(); it < rows.end();
-       it = it + kNumHistoryRowsToSend) {
+  // |rows_left| is required for the checks below as Windows has a
+  // Debug bounds-check which prevents pushing an iterator beyond its end()
+  // (i.e., |it + 2 < s.end()| crashes in debug mode if |i + 1 == s.end()|).
+  int rows_left = rows.end() - rows.begin();
+  for (history::URLRows::const_iterator it = rows.begin(); it < rows.end();) {
     history::URLRows row_group;
     history::URLRows::const_iterator end_group =
-        it + kNumHistoryRowsToSend < rows.end() ?
-        it + kNumHistoryRowsToSend : rows.end();
+        it + std::min(rows_left, kNumHistoryRowsToSend);
     row_group.assign(it, end_group);
 
-    Send(new ProfileImportProcessHostMsg_NotifyHistoryImportGroup(row_group,
-         visit_source));
+    Send(new ProfileImportProcessHostMsg_NotifyHistoryImportGroup(
+        row_group, visit_source));
+    rows_left -= end_group - it;
+    it = end_group;
   }
+  DCHECK_EQ(0, rows_left);
 }
 
 void ExternalProcessImporterBridge::SetKeywords(
diff --git a/chrome/browser/importer/firefox_importer_browsertest.cc b/chrome/browser/importer/firefox_importer_browsertest.cc
index 1a6dbec..fc87222 100644
--- a/chrome/browser/importer/firefox_importer_browsertest.cc
+++ b/chrome/browser/importer/firefox_importer_browsertest.cc
@@ -163,7 +163,6 @@ class Firefox3Observer : public ProfileWriter,
 
   virtual void AddBookmarks(const std::vector<ImportedBookmarkEntry>& bookmarks,
                             const string16& top_level_folder_name) OVERRIDE {
-
     ASSERT_LE(bookmark_count_ + bookmarks.size(),
               arraysize(kFirefox3Bookmarks));
     // Importer should import the FF favorites the same as the list, in the same
@@ -269,9 +268,9 @@ class FirefoxProfileImporterBrowserTest : public InProcessBrowserTest {
       items = items | importer::SEARCH_ENGINES;
 
     // Deletes itself.
-    // TODO(gab): Use ExternalProcessImporterHost on both Windows and Linux.
+    // TODO(gab): Use ExternalProcessImporterHost on Linux as well.
     ImporterHost* host;
-#if defined(OS_MACOSX)
+#if defined(OS_MACOSX) || defined(OS_WIN)
     host = new ExternalProcessImporterHost;
 #else
     host = new ImporterHost;
diff --git a/chrome/browser/importer/ie_importer_browsertest_win.cc b/chrome/browser/importer/ie_importer_browsertest_win.cc
index 5b51894..ac04365 100644
--- a/chrome/browser/importer/ie_importer_browsertest_win.cc
+++ b/chrome/browser/importer/ie_importer_browsertest_win.cc
@@ -30,6 +30,7 @@
 #include "base/win/windows_version.h"
 #include "chrome/browser/bookmarks/imported_bookmark_entry.h"
 #include "chrome/browser/favicon/imported_favicon_usage.h"
+#include "chrome/browser/importer/external_process_importer_host.h"
 #include "chrome/browser/importer/ie_importer.h"
 #include "chrome/browser/importer/ie_importer_utils_win.h"
 #include "chrome/browser/importer/ie_importer_test_registry_overrider_win.h"
@@ -484,8 +485,7 @@ IN_PROC_BROWSER_TEST_F(IEImporterBrowserTest, IEImporter) {
 
   // Starts to import the above settings.
   // Deletes itself.
-  // TODO(gab): Use ExternalProcessImporterHost on Windows.
-  ImporterHost* host = new ImporterHost;
+  ImporterHost* host = new ExternalProcessImporterHost;
   TestObserver* observer = new TestObserver();
   host->SetObserver(observer);
 
@@ -561,8 +561,7 @@ IN_PROC_BROWSER_TEST_F(IEImporterBrowserTest,
 
     // Starts to import the above settings.
     // Deletes itself.
-    // TODO(gab): Use ExternalProcessImporterHost on Windows.
-    ImporterHost* host = new ImporterHost;
+    ImporterHost* host = new ExternalProcessImporterHost;
     MalformedFavoritesRegistryTestObserver* observer =
         new MalformedFavoritesRegistryTestObserver();
     host->SetObserver(observer);