Begin abstracting sending of IPC from autofill core code.

Begin abstracting sending of IPC from autofill core code.

This CL introduces the pattern via which sending of IPC from autofill core code
will instead be abstracted through AutofillDriver. Concretely, it abstracts the sending of the AutofillMsg_FormDataFilled message.

BUG=247015
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209375

[blundell, 2013-06-24 20:25:13]

Hi Ilya,

This is a work-in-progress; in particular, I need to fix up the tests. However,
I'd like to see what you think of the approach. I had initially proposed a
separate AutofillRendererBridge interface for this purpose, but I'm not
convinced it's worth it at this point.

https://codereview.chromium.org/17572015/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/17572015/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_manager.cc:946: bool
AutofillManager::GetHost(RenderViewHost** host) const {
I will rename this method and change its signature once I fix up all the
callsites.

https://codereview.chromium.org/17572015/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_manager.cc:959: *host =
driver_->GetWebContents()->GetRenderViewHost();
This is just in place until I change all callsites of this method to not need
|host|.

[Ilya Sherman, 2013-06-25 01:22:33]

General approach LG, thanks :)

https://codereview.chromium.org/17572015/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/autofill_driver.h (right):

https://codereview.chromium.org/17572015/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_driver.h:28: virtual bool
CanSendToRenderer() = 0;
Would it make more sense for SendFormDataToRenderer() to return a boolean, or
perhaps for the client to simply fire-and-forget, without needing to worry about
this at all?  Or are there times when the client would want to know whether it's
possible to communicate with the renderer, but then not actually go ahead and
send a message over?

[blundell, 2013-06-25 16:11:03]

Hi Ilya,

Thanks! This is now complete with the exception of autofill_manager_unittest.cc.
There are several ways that I could change the tests in that file -- I chose one
way and changed one test as an example. I'd like to make sure you're down with
that approach before I do the rest, as there are many tests that will change and
I'd rather only do the work once.

https://codereview.chromium.org/17572015/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/autofill_driver.h (right):

https://codereview.chromium.org/17572015/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_driver.h:28: virtual bool
CanSendToRenderer() = 0;
I noticed a couple places where there's a short-circuit out of a method, most
significantly in AutofillManager::OnFormsSeen(). But you're right that most of
the time the core code doesn't care. Changed SendFormDataToRenderer() to be a
no-op if the renderer isn't available.

On 2013/06/25 01:22:33, Ilya Sherman wrote:
> Would it make more sense for SendFormDataToRenderer() to return a boolean, or
> perhaps for the client to simply fire-and-forget, without needing to worry
about
> this at all?  Or are there times when the client would want to know whether
it's
> possible to communicate with the renderer, but then not actually go ahead and
> send a message over?

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
components/autofill/core/browser/autofill_manager_unittest.cc:729: void
FillAutofillFormDataAndSaveResults(int input_query_id,
This was the cleanest approach I found to the tests. Other approaches I
considered:
- Creating custom GMock actions that execute the ExpectFilled* methods. In
addition to being cumbersome, these actions only support up to 10 parameters,
which is a no go for ExpectedFilledForm().
- Creating "expected value" FormData's and matching the arguments of the
SendFormDataToRenderer() calls against these expected values. This would result
in test failures due to a FormData mismatch spitting out just a "Expected <huge
object X> but received <huge object Y>", which seemed like a severe regression
compared to the specificity of error output there is now.

[blundell, 2013-06-26 19:05:00]

On 2013/06/25 16:11:03, blundell wrote:
> Hi Ilya,
> 
> Thanks! This is now complete with the exception of
autofill_manager_unittest.cc.
> There are several ways that I could change the tests in that file -- I chose
one
> way and changed one test as an example. I'd like to make sure you're down with
> that approach before I do the rest, as there are many tests that will change
and
> I'd rather only do the work once.
> 
>
https://codereview.chromium.org/17572015/diff/1/components/autofill/core/brow...
> File components/autofill/core/browser/autofill_driver.h (right):
> 
>
https://codereview.chromium.org/17572015/diff/1/components/autofill/core/brow...
> components/autofill/core/browser/autofill_driver.h:28: virtual bool
> CanSendToRenderer() = 0;
> I noticed a couple places where there's a short-circuit out of a method, most
> significantly in AutofillManager::OnFormsSeen(). But you're right that most of
> the time the core code doesn't care. Changed SendFormDataToRenderer() to be a
> no-op if the renderer isn't available.
> 
> On 2013/06/25 01:22:33, Ilya Sherman wrote:
> > Would it make more sense for SendFormDataToRenderer() to return a boolean,
or
> > perhaps for the client to simply fire-and-forget, without needing to worry
> about
> > this at all?  Or are there times when the client would want to know whether
> it's
> > possible to communicate with the renderer, but then not actually go ahead
and
> > send a message over?
> 
>
https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
> File components/autofill/core/browser/autofill_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
> components/autofill/core/browser/autofill_manager_unittest.cc:729: void
> FillAutofillFormDataAndSaveResults(int input_query_id,
> This was the cleanest approach I found to the tests. Other approaches I
> considered:
> - Creating custom GMock actions that execute the ExpectFilled* methods. In
> addition to being cumbersome, these actions only support up to 10 parameters,
> which is a no go for ExpectedFilledForm().
> - Creating "expected value" FormData's and matching the arguments of the
> SendFormDataToRenderer() calls against these expected values. This would
result
> in test failures due to a FormData mismatch spitting out just a "Expected
<huge
> object X> but received <huge object Y>", which seemed like a severe regression
> compared to the specificity of error output there is now.

Ping :).

[Ilya Sherman, 2013-06-26 22:54:58]

https://codereview.chromium.org/17572015/diff/8002/components/autofill/conten...
File components/autofill/content/browser/autofill_driver_impl.h (right):

https://codereview.chromium.org/17572015/diff/8002/components/autofill/conten...
components/autofill/content/browser/autofill_driver_impl.h:48: OVERRIDE;
nit: IMO, it would be slightly cleaner to wrap the second parameter, like so:

  virtual void SendFormDataToRenderer(int query_id,
                                      const FormData& data) OVERRIDE;

https://codereview.chromium.org/17572015/diff/8002/components/autofill/conten...
File components/autofill/content/browser/autofill_driver_impl_unittest.cc
(right):

https://codereview.chromium.org/17572015/diff/8002/components/autofill/conten...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:81: bool
GetAutofillFormDataFilledMessage(int* page_id, FormData* results) {
nit: Docs, please.

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
File components/autofill/core/browser/autofill_common_test.cc (right):

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
components/autofill/core/browser/autofill_common_test.cc:41: // for building up
more complex test forms.
nit: This comment is duplicated from the header.

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
File components/autofill/core/browser/autofill_driver.h (right):

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
components/autofill/core/browser/autofill_driver.h:31: // original request for
the data. Is a no-op if the renderer is not currently
nit: "Is a no-op" -> "This method is a no-op"

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
components/autofill/core/browser/autofill_manager_unittest.cc:729: void
FillAutofillFormDataAndSaveResults(int input_query_id,
nit: Docs, please.

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
components/autofill/core/browser/autofill_manager_unittest.cc:729: void
FillAutofillFormDataAndSaveResults(int input_query_id,
On 2013/06/25 16:11:04, blundell wrote:
> This was the cleanest approach I found to the tests. Other approaches I
> considered:
> - Creating custom GMock actions that execute the ExpectFilled* methods. In
> addition to being cumbersome, these actions only support up to 10 parameters,
> which is a no go for ExpectedFilledForm().
> - Creating "expected value" FormData's and matching the arguments of the
> SendFormDataToRenderer() calls against these expected values. This would
result
> in test failures due to a FormData mismatch spitting out just a "Expected
<huge
> object X> but received <huge object Y>", which seemed like a severe regression
> compared to the specificity of error output there is now.

The approach you're using now seems pretty good to me :)

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
File components/autofill/core/browser/test_autofill_driver.h (right):

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
components/autofill/core/browser/test_autofill_driver.h:32: const FormData&
data));
Please stub out the method rather than mocking it out.  Tests that need mocks
can add the mocks for themselves; but in general, we try to limit the use of
gmock as much as we can without making tests harder or more verbose to write.

[blundell, 2013-06-27 21:58:26]

Thanks! Fixed up all tests as well as addressing comments. The
autofill_manager_unittest churn is unfortunate.

https://codereview.chromium.org/17572015/diff/8002/components/autofill/conten...
File components/autofill/content/browser/autofill_driver_impl.h (right):

https://codereview.chromium.org/17572015/diff/8002/components/autofill/conten...
components/autofill/content/browser/autofill_driver_impl.h:48: OVERRIDE;
On 2013/06/26 22:54:58, Ilya Sherman wrote:
> nit: IMO, it would be slightly cleaner to wrap the second parameter, like so:
> 
>   virtual void SendFormDataToRenderer(int query_id,
>                                       const FormData& data) OVERRIDE;

Done.

https://codereview.chromium.org/17572015/diff/8002/components/autofill/conten...
File components/autofill/content/browser/autofill_driver_impl_unittest.cc
(right):

https://codereview.chromium.org/17572015/diff/8002/components/autofill/conten...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:81: bool
GetAutofillFormDataFilledMessage(int* page_id, FormData* results) {
On 2013/06/26 22:54:58, Ilya Sherman wrote:
> nit: Docs, please.

Done.

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
File components/autofill/core/browser/autofill_common_test.cc (right):

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
components/autofill/core/browser/autofill_common_test.cc:41: // for building up
more complex test forms.
On 2013/06/26 22:54:58, Ilya Sherman wrote:
> nit: This comment is duplicated from the header.

Done.

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
File components/autofill/core/browser/autofill_driver.h (right):

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
components/autofill/core/browser/autofill_driver.h:31: // original request for
the data. Is a no-op if the renderer is not currently
On 2013/06/26 22:54:58, Ilya Sherman wrote:
> nit: "Is a no-op" -> "This method is a no-op"

Done.

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
components/autofill/core/browser/autofill_manager_unittest.cc:729: void
FillAutofillFormDataAndSaveResults(int input_query_id,
On 2013/06/26 22:54:58, Ilya Sherman wrote:
> nit: Docs, please.

Done.

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
File components/autofill/core/browser/test_autofill_driver.h (right):

https://codereview.chromium.org/17572015/diff/8002/components/autofill/core/b...
components/autofill/core/browser/test_autofill_driver.h:32: const FormData&
data));
On 2013/06/26 22:54:58, Ilya Sherman wrote:
> Please stub out the method rather than mocking it out.  Tests that need mocks
> can add the mocks for themselves; but in general, we try to limit the use of
> gmock as much as we can without making tests harder or more verbose to write.

Done.

[Ilya Sherman, 2013-06-27 23:13:02]

LGTM, thanks.

https://codereview.chromium.org/17572015/diff/42001/components/autofill/conte...
File components/autofill/content/browser/autofill_driver_impl_unittest.cc
(right):

https://codereview.chromium.org/17572015/diff/42001/components/autofill/conte...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:82: // IPC
messages. If none is present, returns false.  Otherwise, Extracts the
nit: "Extracts" -> "extracts"

[blundell, 2013-06-28 08:25:58]

https://codereview.chromium.org/17572015/diff/42001/components/autofill/conte...
File components/autofill/content/browser/autofill_driver_impl_unittest.cc
(right):

https://codereview.chromium.org/17572015/diff/42001/components/autofill/conte...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:82: // IPC
messages. If none is present, returns false.  Otherwise, Extracts the
On 2013/06/27 23:13:02, Ilya Sherman wrote:
> nit: "Extracts" -> "extracts"

Done.

[commit-bot: I haz the power, 2013-06-28 17:36:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/17572015/58001

[commit-bot: I haz the power, 2013-06-28 17:36:58]

Failed to apply patch for
components/autofill/content/browser/autofill_driver_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file components/autofill/content/browser/autofill_driver_impl.cc
  Hunk #1 FAILED at 9.
  Hunk #2 succeeded at 81 (offset 13 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
components/autofill/content/browser/autofill_driver_impl.cc.rej

Patch:       components/autofill/content/browser/autofill_driver_impl.cc
Index: components/autofill/content/browser/autofill_driver_impl.cc
diff --git a/components/autofill/content/browser/autofill_driver_impl.cc
b/components/autofill/content/browser/autofill_driver_impl.cc
index
095822cb32c9a9e884a0f55bd845026818be2dfa..808ea4f5a53adad1e75402ff350d54b8d20cb0eb
100644
--- a/components/autofill/content/browser/autofill_driver_impl.cc
+++ b/components/autofill/content/browser/autofill_driver_impl.cc
@@ -9,6 +9,7 @@
 #include "components/autofill/core/browser/autofill_manager_delegate.h"
 #include "components/autofill/core/common/autofill_messages.h"
 #include "content/public/browser/navigation_details.h"
+#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/frame_navigate_params.h"
 #include "ipc/ipc_message_macros.h"
@@ -68,6 +69,19 @@ content::WebContents* AutofillDriverImpl::GetWebContents() {
   return web_contents();
 }
 
+bool AutofillDriverImpl::RendererIsAvailable() {
+  return (web_contents()->GetRenderViewHost() != NULL);
+}
+
+void AutofillDriverImpl::SendFormDataToRenderer(int query_id,
+                                                const FormData& data) {
+  if (!RendererIsAvailable())
+    return;
+  content::RenderViewHost* host = web_contents()->GetRenderViewHost();
+  host->Send(
+      new AutofillMsg_FormDataFilled(host->GetRoutingID(), query_id, data));
+}
+
 bool AutofillDriverImpl::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(AutofillDriverImpl, message)

[commit-bot: I haz the power, 2013-06-30 16:59:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/17572015/63001

[commit-bot: I haz the power, 2013-07-01 07:18:00]

Change committed as 209375