[Autofill] Migrate ContentAutofillDriver<-->AutofillAgent IPCs to mojo.

components/autofill/content/browser/content_autofill_driver_unittest.cc

Index: components/autofill/content/browser/content_autofill_driver_unittest.cc
diff --git a/components/autofill/content/browser/content_autofill_driver_unittest.cc b/components/autofill/content/browser/content_autofill_driver_unittest.cc
index bc88e12f2ad08d02ce69f5bbc61d3807b397319c..94d869a4930cfd2a6a5a56b586cb1b8aa490c09e 100644
--- a/components/autofill/content/browser/content_autofill_driver_unittest.cc
+++ b/components/autofill/content/browser/content_autofill_driver_unittest.cc
@@ -13,6 +13,8 @@
 #include <vector>
 
 #include "base/command_line.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/autofill/content/common/autofill_messages.h"
 #include "components/autofill/core/browser/autofill_external_delegate.h"
@@ -26,9 +28,11 @@
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/frame_navigate_params.h"
+#include "content/public/common/service_registry.h"
 #include "content/public/test/mock_render_process_host.h"
 #include "content/public/test/test_renderer_host.h"
 #include "ipc/ipc_test_sink.h"
+#include "mojo/public/cpp/bindings/binding_set.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -40,6 +44,13 @@ const char kAppLocale[] = "en-US";
 const AutofillManager::AutofillDownloadManagerState kDownloadState =
     AutofillManager::DISABLE_AUTOFILL_DOWNLOAD_MANAGER;
 
+void RunAllPendingTasks() {
+  base::RunLoop run_loop;
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE, base::MessageLoop::QuitWhenIdleClosure());

[yzshen1, 2016/05/24 16:12:10]

QuitWhenIdleClosure is deprecated according to the comments in message_loop.h.

You might want to use corresponding RunLoop method.

[leonhsl(Using Gerrit), 2016/06/23 10:00:12]

Done.

+  run_loop.Run();
+}
+
 }  // namespace
 
 class MockAutofillManager : public AutofillManager {
@@ -69,7 +80,8 @@ class TestContentAutofillDriver : public ContentAutofillDriver {
   using ContentAutofillDriver::DidNavigateFrame;
 };
 
-class ContentAutofillDriverTest : public content::RenderViewHostTestHarness {
+class ContentAutofillDriverTest : public content::RenderViewHostTestHarness,
+                                  public mojom::AutofillAgent {

[vabr (Chromium), 2016/05/24 08:08:20]

Please aggregate a test version of an mojom::AutofillAgent into
ContentAutofillDriverTest as a data member instead of inheriting from it. You
need the fixture to provide a mojom::AutofillAgent, not to be one. Splitting the
AutofillAgent implementation into a separate class (in the anonymous namespace)
will make the fixture smaller and easier to understand. It will also make it
easier to locate the mojom::AutofillAgent-specific bits.

[leonhsl(Using Gerrit), 2016/06/23 10:00:12]

Done.

  public:
   void SetUp() override {
     content::RenderViewHostTestHarness::SetUp();
@@ -77,6 +89,16 @@ class ContentAutofillDriverTest : public content::RenderViewHostTestHarness {
     test_autofill_client_.reset(new TestAutofillClient());
     driver_.reset(new TestContentAutofillDriver(web_contents()->GetMainFrame(),
                                                 test_autofill_client_.get()));
+
+    id_sent_ = -1;

[vabr (Chromium), 2016/05/24 08:08:20]

Please initialise this in the constructor or via a non-static class member
initializer in the class definition. This will ensure that the data member has a
sane value through the whole existence of the text fixture.

[leonhsl(Using Gerrit), 2016/06/23 10:00:12]

Done.

+    form_sent_.reset();

[vabr (Chromium), 2016/05/24 08:08:20]

No need for this, it is null since construction, and you are already relying on
SetUp being called before anything else touching |form_sent_|.

[leonhsl(Using Gerrit), 2016/06/23 10:00:12]

Done.

+
+    content::ServiceRegistry* registry =
+        web_contents()->GetMainFrame()->GetServiceRegistry();
+    registry->AddServiceOverrideForTesting(
+        mojom::AutofillAgent::Name_,
+        base::Bind(&ContentAutofillDriverTest::BindFakeAutofillAgent,
+                   base::Unretained(this)));
   }
 
   void TearDown() override {
@@ -86,25 +108,35 @@ class ContentAutofillDriverTest : public content::RenderViewHostTestHarness {
     content::RenderViewHostTestHarness::TearDown();
   }
 
+  void BindFakeAutofillAgent(mojo::ScopedMessagePipeHandle handle) {
+    bindings_.AddBinding(
+        this, mojo::MakeRequest<mojom::AutofillAgent>(std::move(handle)));
+  }
+
  protected:
-  // Searches for an |AutofillMsg_FillForm| message in the queue of sent IPC
-  // messages. If none is present, returns false. Otherwise, extracts the first
-  // |AutofillMsg_FillForm| message, fills the output parameters with the values
-  // of the message's parameters, and clears the queue of sent messages.
+  // mojom::AutofillAgent methods:
+  void FirstUserGestureObservedInTab() override {}
+
+  void FillForm(int32_t id,
+                const FormData& form,
+                const FillFormCallback& callback) override {
+    id_sent_ = id;
+    form_sent_.reset(new FormData(form));
+    callback.Run(form, 0);
+  }
+
+  // Returns the id and formdata received via
+  // mojo interface method mojom::AutofillAgent::FillForm().
   bool GetAutofillFillFormMessage(int* page_id, FormData* results) {
-    const uint32_t kMsgID = AutofillMsg_FillForm::ID;
-    const IPC::Message* message =
-        process()->sink().GetFirstMessageMatching(kMsgID);
-    if (!message)
+    if (id_sent_ == -1)
       return false;
-    std::tuple<int, FormData> autofill_param;
-    if (!AutofillMsg_FillForm::Read(message, &autofill_param))
+    if (!form_sent_)
       return false;
+
     if (page_id)
-      *page_id = std::get<0>(autofill_param);
+      *page_id = id_sent_;
     if (results)
-      *results = std::get<1>(autofill_param);
-    process()->sink().ClearMessages();
+      *results = *form_sent_;
     return true;
   }
 
@@ -199,6 +231,10 @@ class ContentAutofillDriverTest : public content::RenderViewHostTestHarness {
 
   std::unique_ptr<TestAutofillClient> test_autofill_client_;
   std::unique_ptr<TestContentAutofillDriver> driver_;
+  mojo::BindingSet<mojom::AutofillAgent> bindings_;
+
+  int32_t id_sent_;

[vabr (Chromium), 2016/05/24 08:08:20]

nit: Please comment about what |id_sent_| and |form_sent_| are used for.

[leonhsl(Using Gerrit), 2016/06/23 10:00:12]

Done.

+  std::unique_ptr<FormData> form_sent_;
 };
 
 TEST_F(ContentAutofillDriverTest, GetURLRequestContext) {
@@ -236,6 +272,8 @@ TEST_F(ContentAutofillDriverTest, FormDataSentToRenderer_FillForm) {
   driver_->SendFormDataToRenderer(

[yzshen1, 2016/05/24 16:12:10]

I am not familiar with auto fill code. Does this call somehow make a mojo call
of mojom::AutofillAgent::FillForm() which will be handled by line 120?

It is potentially flaky to use RunAllPendingTasks() below. There is no guarantee
that the message loop stays busy between you make a mojo call of FillFrom() and
receive this call, even if they are on the same thread.

The safer way is to signal run loop to exit from
mojom::AutofillAgent::FillForm() implementation.

[leonhsl(Using Gerrit), 2016/06/23 10:00:12]

Done.

       input_page_id, AutofillDriver::FORM_DATA_ACTION_FILL, input_form_data);
 
+  RunAllPendingTasks();
+
   int output_page_id = 0;
   FormData output_form_data;
   EXPECT_FALSE(
@@ -252,6 +290,8 @@ TEST_F(ContentAutofillDriverTest, FormDataSentToRenderer_PreviewForm) {
   driver_->SendFormDataToRenderer(
       input_page_id, AutofillDriver::FORM_DATA_ACTION_PREVIEW, input_form_data);
 
+  RunAllPendingTasks();
+
   int output_page_id = 0;
   FormData output_form_data;
   EXPECT_FALSE(GetAutofillFillFormMessage(&output_page_id, &output_form_data));