Convert profile import IPCs to Mojo

chrome/browser/importer/external_process_importer_client.cc

Index: chrome/browser/importer/external_process_importer_client.cc
diff --git a/chrome/browser/importer/external_process_importer_client.cc b/chrome/browser/importer/external_process_importer_client.cc
index 325691bf5f862668efe04109905bbbc81a0a07cc..4896ab43dcdaab9460923757698b1b7917ea2d84 100644
--- a/chrome/browser/importer/external_process_importer_client.cc
+++ b/chrome/browser/importer/external_process_importer_client.cc
@@ -12,11 +12,11 @@
 #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;
@@ -34,7 +34,8 @@ ExternalProcessImporterClient::ExternalProcessImporterClient(
       source_profile_(source_profile),
       items_(items),
       bridge_(bridge),
-      cancelled_(false) {
+      cancelled_(false),
+      binding_(this) {
   process_importer_host_->NotifyImportStarted();
 }
 
@@ -54,11 +55,9 @@ void ExternalProcessImporterClient::Cancel() {
     return;
 
   cancelled_ = true;
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(
-          &ExternalProcessImporterClient::CancelImportProcessOnIOThread,
-          this));
+  if (utility_process_host_.get())
+    profile_import_->CancelImport();
+  CloseMojoHandles();
   Release();
 }
 
@@ -75,49 +74,7 @@ void ExternalProcessImporterClient::OnProcessCrashed(int exit_code) {
 
 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() {
@@ -137,29 +94,27 @@ void ExternalProcessImporterClient::OnImportFinished(
   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;
 
@@ -190,7 +145,7 @@ void ExternalProcessImporterClient::OnHomePageImportReady(
 
 void ExternalProcessImporterClient::OnBookmarksImportStart(
     const base::string16& first_folder_name,
-    size_t total_bookmarks_count) {
+    int total_bookmarks_count) {
   if (cancelled_)
     return;
 
@@ -213,7 +168,7 @@ void ExternalProcessImporterClient::OnBookmarksImportGroup(
 }
 
 void ExternalProcessImporterClient::OnFaviconsImportStart(
-    size_t total_favicons_count) {
+    int total_favicons_count) {
   if (cancelled_)
     return;
 
@@ -249,14 +204,14 @@ void ExternalProcessImporterClient::OnKeywordsImportReady(
 }
 
 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;
 
@@ -277,14 +232,16 @@ void ExternalProcessImporterClient::OnAutofillFormDataImportGroup(
     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
+}
 
 ExternalProcessImporterClient::~ExternalProcessImporterClient() {}
 
@@ -294,20 +251,10 @@ void ExternalProcessImporterClient::Cleanup() {
 
   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_ =
@@ -326,6 +273,21 @@ void ExternalProcessImporterClient::StartProcessOnIOThread(
   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;
@@ -351,6 +313,11 @@ void ExternalProcessImporterClient::StartProcessOnIOThread(
       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();
 }