Convert profile import IPCs to Mojo

chrome/browser/importer/external_process_importer_client.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/external_process_importer_client.h"
 
 #include "base/bind.h"
 #include "base/strings/string_number_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/importer/external_process_importer_host.h"
 #include "chrome/browser/importer/in_process_importer_bridge.h"
 #include "chrome/common/importer/firefox_importer_utils.h"
 #include "chrome/common/importer/imported_bookmark_entry.h"
-#include "chrome/common/importer/profile_import_process_messages.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/strings/grit/components_strings.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/utility_process_host.h"
+#include "services/service_manager/public/cpp/interface_provider.h"
 #include "ui/base/l10n/l10n_util.h"
 
 using content::BrowserThread;
 using content::UtilityProcessHost;
 
 ExternalProcessImporterClient::ExternalProcessImporterClient(
     base::WeakPtr<ExternalProcessImporterHost> importer_host,
     const importer::SourceProfile& source_profile,
     uint16_t items,
     InProcessImporterBridge* bridge)
     : total_bookmarks_count_(0),
       total_history_rows_count_(0),
       total_favicons_count_(0),
       process_importer_host_(importer_host),
       source_profile_(source_profile),
       items_(items),
       bridge_(bridge),
-      cancelled_(false) {
+      cancelled_(false),
+      binding_(this) {
   process_importer_host_->NotifyImportStarted();
 }
 
 void ExternalProcessImporterClient::Start() {
   AddRef();  // balanced in Cleanup.
   BrowserThread::ID thread_id;
   CHECK(BrowserThread::GetCurrentThreadIdentifier(&thread_id));
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&ExternalProcessImporterClient::StartProcessOnIOThread,
                  this,
                  thread_id));
 }
 
 void ExternalProcessImporterClient::Cancel() {
   if (cancelled_)
     return;
 
   cancelled_ = true;
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(
-          &ExternalProcessImporterClient::CancelImportProcessOnIOThread,
-          this));
+  if (utility_process_host_.get())
+    profile_import_->CancelImport();
+  CloseMojoHandles();
   Release();
 }
 
 void ExternalProcessImporterClient::OnProcessCrashed(int exit_code) {
   DLOG(ERROR) << __func__;
   if (cancelled_)
     return;
 
   // If the host is still around, cancel the import; otherwise it means the
   // import was already cancelled or completed and this message can be dropped.
   if (process_importer_host_.get())
     process_importer_host_->Cancel();
 }
 
 bool ExternalProcessImporterClient::OnMessageReceived(
     const IPC::Message& message) {
-  bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP(ExternalProcessImporterClient, message)
-    // Notification messages about the state of the import process.
-    IPC_MESSAGE_HANDLER(ProfileImportProcessHostMsg_Import_Started,
-                        OnImportStart)
-    IPC_MESSAGE_HANDLER(ProfileImportProcessHostMsg_Import_Finished,
-                        OnImportFinished)
-    IPC_MESSAGE_HANDLER(ProfileImportProcessHostMsg_ImportItem_Started,
-                        OnImportItemStart)
-    IPC_MESSAGE_HANDLER(ProfileImportProcessHostMsg_ImportItem_Finished,
-                        OnImportItemFinished)
-    // Data messages containing items to be written to the user profile.
-    IPC_MESSAGE_HANDLER(ProfileImportProcessHostMsg_NotifyHistoryImportStart,
-                        OnHistoryImportStart)
-    IPC_MESSAGE_HANDLER(ProfileImportProcessHostMsg_NotifyHistoryImportGroup,
-                        OnHistoryImportGroup)
-    IPC_MESSAGE_HANDLER(ProfileImportProcessHostMsg_NotifyHomePageImportReady,
-                        OnHomePageImportReady)
-    IPC_MESSAGE_HANDLER(ProfileImportProcessHostMsg_NotifyBookmarksImportStart,
-                        OnBookmarksImportStart)
-    IPC_MESSAGE_HANDLER(ProfileImportProcessHostMsg_NotifyBookmarksImportGroup,
-                        OnBookmarksImportGroup)
-    IPC_MESSAGE_HANDLER(ProfileImportProcessHostMsg_NotifyFaviconsImportStart,
-                        OnFaviconsImportStart)
-    IPC_MESSAGE_HANDLER(ProfileImportProcessHostMsg_NotifyFaviconsImportGroup,
-                        OnFaviconsImportGroup)
-    IPC_MESSAGE_HANDLER(ProfileImportProcessHostMsg_NotifyPasswordFormReady,
-                        OnPasswordFormImportReady)
-    IPC_MESSAGE_HANDLER(ProfileImportProcessHostMsg_NotifyKeywordsReady,
-                        OnKeywordsImportReady)
-    IPC_MESSAGE_HANDLER(ProfileImportProcessHostMsg_NotifyFirefoxSearchEngData,
-                        OnFirefoxSearchEngineDataReceived)
-    IPC_MESSAGE_HANDLER(ProfileImportProcessHostMsg_AutofillFormDataImportStart,
-                        OnAutofillFormDataImportStart)
-    IPC_MESSAGE_HANDLER(ProfileImportProcessHostMsg_AutofillFormDataImportGroup,
-                        OnAutofillFormDataImportGroup)
-#if defined(OS_WIN)
-    IPC_MESSAGE_HANDLER(ProfileImportProcessHostMsg_NotifyIE7PasswordInfo,
-                        OnIE7PasswordReceived)
-#endif
-    IPC_MESSAGE_UNHANDLED(handled = false)
-  IPC_END_MESSAGE_MAP()
-  return handled;
+  return false;

[gab, 2016/11/07 20:07:31]

Why is this method still needed?

[tibell, 2016/11/07 23:34:23]

It's a (unfortunately) a pure virtual method in
content::UtilityProcessHostClient, which we subclass to get e.g.
OnProcessCrashed. Ideally it shouldn't be pure virtual in the super class.

[gab, 2016/11/08 13:34:24]

I see, does every mojo host need this to get OnProcessCrashed? Surprised the
mojo messages don't implicitly include one for that as well?

If it's common, it'd be nice to clean it up to at least not be pure virtual in a
follow-up CL.

 }
 
 void ExternalProcessImporterClient::OnImportStart() {
   if (cancelled_)
     return;
 
   bridge_->NotifyStarted();
 }
 
 void ExternalProcessImporterClient::OnImportFinished(
     bool succeeded, const std::string& error_msg) {
   if (cancelled_)
     return;
 
   if (!succeeded)
     LOG(WARNING) << "Import failed.  Error: " << error_msg;
   Cleanup();
 }
 
-void ExternalProcessImporterClient::OnImportItemStart(int item_data) {
+void ExternalProcessImporterClient::OnImportItemStart(
+    importer::ImportItem item_data) {
   if (cancelled_)
     return;
 
   bridge_->NotifyItemStarted(static_cast<importer::ImportItem>(item_data));

[Sam McNally, 2016/11/07 06:31:07]

Remove the cast.

[gab, 2016/11/07 20:07:31]

and also rename the parameter to |import_item| since it's no longer "raw" data

[tibell, 2016/11/07 23:34:23]

Done.

[tibell, 2016/11/07 23:34:23]

Done.

 }
 
-void ExternalProcessImporterClient::OnImportItemFinished(int item_data) {
+void ExternalProcessImporterClient::OnImportItemFinished(
+    importer::ImportItem item_data) {
   if (cancelled_)
     return;
 
   importer::ImportItem import_item =
       static_cast<importer::ImportItem>(item_data);

[gab, 2016/11/07 20:07:32]

no cast here too and rename param

[tibell, 2016/11/07 23:34:23]

Done.

   bridge_->NotifyItemEnded(import_item);
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&ExternalProcessImporterClient::NotifyItemFinishedOnIOThread,
-                 this,
-                 import_item));
+  profile_import_->ReportImportItemFinished(import_item);
 }
 
 void ExternalProcessImporterClient::OnHistoryImportStart(
-    size_t total_history_rows_count) {
+    int total_history_rows_count) {
   if (cancelled_)
     return;
 
   total_history_rows_count_ = total_history_rows_count;
   history_rows_.reserve(total_history_rows_count);
 }
 
 void ExternalProcessImporterClient::OnHistoryImportGroup(
     const std::vector<ImporterURLRow>& history_rows_group,
     int visit_source) {
@@ skipping 10 matching lines @@
 void ExternalProcessImporterClient::OnHomePageImportReady(
     const GURL& home_page) {
   if (cancelled_)
     return;
 
   bridge_->AddHomePage(home_page);
 }
 
 void ExternalProcessImporterClient::OnBookmarksImportStart(
     const base::string16& first_folder_name,
-    size_t total_bookmarks_count) {
+    int total_bookmarks_count) {
   if (cancelled_)
     return;
 
   bookmarks_first_folder_name_ = first_folder_name;
   total_bookmarks_count_ = total_bookmarks_count;
   bookmarks_.reserve(total_bookmarks_count);
 }
 
 void ExternalProcessImporterClient::OnBookmarksImportGroup(
     const std::vector<ImportedBookmarkEntry>& bookmarks_group) {
   if (cancelled_)
     return;
 
   // Collect sets of bookmarks from importer process until we have reached
   // total_bookmarks_count_:
   bookmarks_.insert(bookmarks_.end(), bookmarks_group.begin(),
                     bookmarks_group.end());
   if (bookmarks_.size() >= total_bookmarks_count_)
     bridge_->AddBookmarks(bookmarks_, bookmarks_first_folder_name_);
 }
 
 void ExternalProcessImporterClient::OnFaviconsImportStart(
-    size_t total_favicons_count) {
+    int total_favicons_count) {
   if (cancelled_)
     return;
 
   total_favicons_count_ = total_favicons_count;
   favicons_.reserve(total_favicons_count);
 }
 
 void ExternalProcessImporterClient::OnFaviconsImportGroup(
     const favicon_base::FaviconUsageDataList& favicons_group) {
   if (cancelled_)
@@ skipping 15 matching lines @@
 
 void ExternalProcessImporterClient::OnKeywordsImportReady(
     const std::vector<importer::SearchEngineInfo>& search_engines,
     bool unique_on_host_and_path) {
   if (cancelled_)
     return;
   bridge_->SetKeywords(search_engines, unique_on_host_and_path);
 }
 
 void ExternalProcessImporterClient::OnFirefoxSearchEngineDataReceived(
-    const std::vector<std::string> search_engine_data) {
+    const std::vector<std::string>& search_engine_data) {
   if (cancelled_)
     return;
   bridge_->SetFirefoxSearchEnginesXMLData(search_engine_data);
 }
 
 void ExternalProcessImporterClient::OnAutofillFormDataImportStart(
-    size_t total_autofill_form_data_entry_count) {
+    int total_autofill_form_data_entry_count) {
   if (cancelled_)
     return;
 
   total_autofill_form_data_entry_count_ = total_autofill_form_data_entry_count;
   autofill_form_data_.reserve(total_autofill_form_data_entry_count);
 }
 
 void ExternalProcessImporterClient::OnAutofillFormDataImportGroup(
     const std::vector<ImporterAutofillFormDataEntry>&
         autofill_form_data_entry_group) {
   if (cancelled_)
     return;
 
   autofill_form_data_.insert(autofill_form_data_.end(),
                              autofill_form_data_entry_group.begin(),
                              autofill_form_data_entry_group.end());
   if (autofill_form_data_.size() >= total_autofill_form_data_entry_count_)
     bridge_->SetAutofillFormData(autofill_form_data_);
 }
 
-#if defined(OS_WIN)
 void ExternalProcessImporterClient::OnIE7PasswordReceived(
     const importer::ImporterIE7PasswordInfo& importer_password_info) {
+#if defined(OS_WIN)
   if (cancelled_)
     return;
   bridge_->AddIE7PasswordInfo(importer_password_info);
+#else
+  NOTREACHED();
+#endif
 }
-#endif
 
 ExternalProcessImporterClient::~ExternalProcessImporterClient() {}
 
 void ExternalProcessImporterClient::Cleanup() {
   if (cancelled_)
     return;
 
   if (process_importer_host_.get())
     process_importer_host_->NotifyImportEnded();
+  CloseMojoHandles();
   Release();
 }
 
-void ExternalProcessImporterClient::CancelImportProcessOnIOThread() {
-  if (utility_process_host_.get())
-    utility_process_host_->Send(new ProfileImportProcessMsg_CancelImport());
-}
-
-void ExternalProcessImporterClient::NotifyItemFinishedOnIOThread(
-    importer::ImportItem import_item) {
-  utility_process_host_->Send(
-      new ProfileImportProcessMsg_ReportImportItemFinished(import_item));
-}
-
 void ExternalProcessImporterClient::StartProcessOnIOThread(
     BrowserThread::ID thread_id) {
   utility_process_host_ =
       UtilityProcessHost::Create(
           this, BrowserThread::GetTaskRunnerForThread(thread_id).get())
           ->AsWeakPtr();
   utility_process_host_->SetName(l10n_util::GetStringUTF16(
       IDS_UTILITY_PROCESS_PROFILE_IMPORTER_NAME));
   utility_process_host_->DisableSandbox();
 
 #if defined(OS_MACOSX)
   base::EnvironmentMap env;
   std::string dylib_path = GetFirefoxDylibPath().value();
   if (!dylib_path.empty())
     env["DYLD_FALLBACK_LIBRARY_PATH"] = dylib_path;
   utility_process_host_->SetEnv(env);
 #endif
 
+  utility_process_host_->Start();
+  chrome::mojom::ProfileImportPtr profile_import;
+  utility_process_host_->GetRemoteInterfaces()->GetInterface(&profile_import);
+
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,

[gab, 2016/11/07 20:07:31]

Does it matter that this on the UI thread? Is this because it happens to be the
owner today in practice? If so, can we avoid taking a dependency on owner thread
and instead take a SequencedTaskRunner to be stored in a
const scoped_refptr<SequencedTaskRunner> owning_sequence_;
member which in practice will be the UI thread but could be any other sequence
later?

[tibell, 2016/11/07 23:34:23]

I opted for Sam's suggestion below, which means we're not doing this post at all
anymore (which means that we're no longer being explicit about what thread we're
running on i.e. just as the code worked before). I'm punting on moving this
class to SequencedTaskRunner, although it sounds like it could be a good
independent refactoring.

+      base::Bind(&ExternalProcessImporterClient::SetupMojoOnUIThread, this,
+                 base::Passed(profile_import.PassInterface())));
+}
+
+void ExternalProcessImporterClient::SetupMojoOnUIThread(

[Sam McNally, 2016/11/07 06:31:07]

Have you considered doing this in Start() and passing a
chrome::mojom::ProfileImportRequest to the IO thread?

[tibell, 2016/11/07 23:34:23]

Done.

+    chrome::mojom::ProfileImportPtrInfo profile_import_ptr_info) {

[gab, 2016/11/07 20:07:32]

DCHECK(owning_sequence_->RunsTasksOnCurrentThread());

[tibell, 2016/11/07 23:34:23]

Done.

+  profile_import_.Bind(std::move(profile_import_ptr_info));
+  auto observer_ptr = binding_.CreateInterfacePtrAndBind();
+
   // Dictionary of all localized strings that could be needed by the importer
   // in the external process.
   base::DictionaryValue localized_strings;
   localized_strings.SetString(
       base::IntToString(IDS_BOOKMARK_GROUP),
       l10n_util::GetStringUTF8(IDS_BOOKMARK_GROUP));
   localized_strings.SetString(
       base::IntToString(IDS_BOOKMARK_GROUP_FROM_FIREFOX),
       l10n_util::GetStringUTF8(IDS_BOOKMARK_GROUP_FROM_FIREFOX));
   localized_strings.SetString(
       base::IntToString(IDS_BOOKMARK_GROUP_FROM_SAFARI),
       l10n_util::GetStringUTF8(IDS_BOOKMARK_GROUP_FROM_SAFARI));
   localized_strings.SetString(
       base::IntToString(IDS_IMPORT_FROM_FIREFOX),
       l10n_util::GetStringUTF8(IDS_IMPORT_FROM_FIREFOX));
   localized_strings.SetString(
       base::IntToString(IDS_IMPORT_FROM_ICEWEASEL),
       l10n_util::GetStringUTF8(IDS_IMPORT_FROM_ICEWEASEL));
   localized_strings.SetString(
       base::IntToString(IDS_IMPORT_FROM_SAFARI),
       l10n_util::GetStringUTF8(IDS_IMPORT_FROM_SAFARI));
   localized_strings.SetString(
       base::IntToString(IDS_BOOKMARK_BAR_FOLDER_NAME),
       l10n_util::GetStringUTF8(IDS_BOOKMARK_BAR_FOLDER_NAME));
 
-  utility_process_host_->Send(new ProfileImportProcessMsg_StartImport(
-      source_profile_, items_, localized_strings));
+  profile_import_->StartImport(source_profile_, items_, localized_strings,
+                               std::move(observer_ptr));

[gab, 2016/11/07 20:07:32]

#include <utility>

[tibell, 2016/11/07 23:34:23]

Done.

 }
+
+void ExternalProcessImporterClient::CloseMojoHandles() {

[gab, 2016/11/07 20:07:32]

DCHECK runs on proper sequence/thread?

[tibell, 2016/11/07 23:34:23]

Fortunately Mojo itself checks this (which is convenient for us, as we don't
need to track which thread these things were created on.)

+  profile_import_.reset();
+  binding_.Close();
+}