Abstracted AcceptPasswordAutofillSuggestion IPC out of core Autofill code.

Abstracted AcceptPasswordAutofillSuggestion IPC out of core Autofill code.

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

[blundell, 2013-11-20 20:32:42]

Looking good, looping in isherman@.

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
File components/autofill/core/browser/autofill_driver.h (right):

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
components/autofill/core/browser/autofill_driver.h:81: // Tells the renderer to
accept password autofill suggestion for |value|.
the password autofill suggestion

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
File components/autofill/core/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
components/autofill/core/browser/autofill_external_delegate.h:24: namespace
content {
Kill.

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
File components/autofill/core/browser/password_autofill_manager_unittest.cc
(right):

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
components/autofill/core/browser/password_autofill_manager_unittest.cc:72: //
TODO(jeanfrancoisg): Add unittest checking that AutofillDriver's
I think that you could just make a MockAutofillDriver that subclasses
TestAutofillDriver and passes in NULL to the TestAutofillDriver constructor,
since the only thing that you want to do is expect that the
RenderShouldAcceptPasswordAutofillSuggestion() method is called.

[Ilya Sherman, 2013-11-21 10:31:56]

LGTM % comments.  Thanks.

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
File components/autofill/core/browser/autofill_driver.h (right):

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
components/autofill/core/browser/autofill_driver.h:81: // Tells the renderer to
accept password autofill suggestion for |value|.
nit: Might be helpful to rename |value| to something with more semantic context.
 If I'm remembering correctly, the value is a username, so perhaps we should
name the variable |username|?

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
components/autofill/core/browser/autofill_external_delegate.cc:21:
content::WebContents* web_contents,
Can this parameter be removed now?  Or are you leaving that for a future CL?

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
File components/autofill/core/browser/password_autofill_manager.h (right):

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
components/autofill/core/browser/password_autofill_manager.h:64: // outlive this
object.
nit: "|this|"

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
File components/autofill/core/browser/test_autofill_driver.cc (right):

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
components/autofill/core/browser/test_autofill_driver.cc:57: const
base::string16& value) {
nit: Indent this line two spaces fewer, as on line 53.

[jif-google, 2013-11-21 16:46:01]

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
File components/autofill/core/browser/autofill_driver.h (right):

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
components/autofill/core/browser/autofill_driver.h:81: // Tells the renderer to
accept password autofill suggestion for |value|.
On 2013/11/20 20:32:43, blundell wrote:
> the password autofill suggestion

Done.

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
components/autofill/core/browser/autofill_driver.h:81: // Tells the renderer to
accept password autofill suggestion for |value|.
On 2013/11/21 10:31:56, Ilya Sherman wrote:
> nit: Might be helpful to rename |value| to something with more semantic
context.
>  If I'm remembering correctly, the value is a username, so perhaps we should
> name the variable |username|?

Renamed |value| to |username| in several methods (
*AutofillDriver*::RendererShouldAcceptPasswordAutofillSuggestion,
PasswordAutofillAgent::DidAcceptAutofillSuggestion, and
PasswordAutofillManager::DidAcceptAutofillSuggestion ).

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
components/autofill/core/browser/autofill_external_delegate.cc:21:
content::WebContents* web_contents,
On 2013/11/21 10:31:56, Ilya Sherman wrote:
> Can this parameter be removed now?  Or are you leaving that for a future CL?

Yes.

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
File components/autofill/core/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
components/autofill/core/browser/autofill_external_delegate.h:24: namespace
content {
On 2013/11/20 20:32:43, blundell wrote:
> Kill.

I erroneously removed this forward declaration in
https://codereview.chromium.org/69293007/. It was only compiling because that
forward declaration was also declared in password_autofill_manager.h, which is
included here.

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
File components/autofill/core/browser/password_autofill_manager.h (right):

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
components/autofill/core/browser/password_autofill_manager.h:64: // outlive this
object.
On 2013/11/21 10:31:56, Ilya Sherman wrote:
> nit: "|this|"

Done.

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
File components/autofill/core/browser/password_autofill_manager_unittest.cc
(right):

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
components/autofill/core/browser/password_autofill_manager_unittest.cc:72: //
TODO(jeanfrancoisg): Add unittest checking that AutofillDriver's
On 2013/11/20 20:32:43, blundell wrote:
> I think that you could just make a MockAutofillDriver that subclasses
> TestAutofillDriver and passes in NULL to the TestAutofillDriver constructor,
> since the only thing that you want to do is expect that the
> RenderShouldAcceptPasswordAutofillSuggestion() method is called.

Done.

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
File components/autofill/core/browser/test_autofill_driver.cc (right):

https://codereview.chromium.org/79103002/diff/30001/components/autofill/core/...
components/autofill/core/browser/test_autofill_driver.cc:57: const
base::string16& value) {
On 2013/11/21 10:31:56, Ilya Sherman wrote:
> nit: Indent this line two spaces fewer, as on line 53.

Done.

[blundell, 2013-11-21 17:08:44]

LGTM

Ilya should take another pass since the CL has grown so much

https://codereview.chromium.org/79103002/diff/480001/components/autofill/cont...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/79103002/diff/480001/components/autofill/cont...
components/autofill/content/renderer/password_autofill_agent.cc:299: // Set the
incoming |value| in the text field and |FillUserNameAndPassword|
username

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
File components/autofill/core/browser/password_autofill_manager.h (right):

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager.h:64: // outlive
|this| object.
s/ object//

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
File components/autofill/core/browser/password_autofill_manager_unittest.cc
(right):

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager_unittest.cc:73:
base::MessageLoop message_loop_;
Comment on why this is needed (i.e., because the SWP used by TestAutofillManager
expects there to be a current MessageLoop).

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager_unittest.cc:74:
scoped_ptr<MockAutofillDriver> autofill_driver_;
Nit: Do these have to be scoped_ptrs rather than objects that are initialized in
the test's constructor?

[Ilya Sherman, 2013-11-22 00:26:28]

https://codereview.chromium.org/79103002/diff/480001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right):

https://codereview.chromium.org/79103002/diff/480001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:41:
autofill_driver) {}
nit: Looks like this can fit on the previous line.

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
File components/autofill/core/browser/password_autofill_manager.cc (right):

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager.cc:15:
AutofillDriver* autofill_driver) : autofill_driver_(autofill_driver) {
nit: Please add DCHECK(autofill_driver) here, since it's currently implicitly
assumed that autofill_driver is non-null; and there's no code in the constructor
that explicitly exercises that assumption.

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
File components/autofill/core/browser/password_autofill_manager.h (right):

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager.h:65: AutofillDriver*
autofill_driver_;  // weak
nit: Can this be declared as "AutofillDriver* const autofill_driver_;  // weak"?
 (i.e., is the pointer value const?)

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
File components/autofill/core/browser/password_autofill_manager_unittest.cc
(right):

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager_unittest.cc:27:
void(const base::string16&));
nit: Please leave a blank line after this one.

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager_unittest.cc:30: };
nit: Please tuck this into an anonymous namespace.

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager_unittest.cc:37: 
nit: Spurious blank line.

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager_unittest.cc:55:
password_autofill_manager_.get()->AddPasswordFormMapping(username_field_,
nit: No need for ".get()" to dereference a smart pointer; just the "->" arrow is
sufficient.

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager_unittest.cc:74:
scoped_ptr<MockAutofillDriver> autofill_driver_;
On 2013/11/21 17:08:45, blundell wrote:
> Nit: Do these have to be scoped_ptrs rather than objects that are initialized
in
> the test's constructor?

+1

[jif-google, 2013-11-22 12:21:43]

Thanks for the reviews. I've addressed all your comments.

https://codereview.chromium.org/79103002/diff/480001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right):

https://codereview.chromium.org/79103002/diff/480001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:41:
autofill_driver) {}
On 2013/11/22 00:26:29, Ilya Sherman wrote:
> nit: Looks like this can fit on the previous line.

Done.

https://codereview.chromium.org/79103002/diff/480001/components/autofill/cont...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/79103002/diff/480001/components/autofill/cont...
components/autofill/content/renderer/password_autofill_agent.cc:299: // Set the
incoming |value| in the text field and |FillUserNameAndPassword|
On 2013/11/21 17:08:45, blundell wrote:
> username

Done.

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
File components/autofill/core/browser/password_autofill_manager.cc (right):

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager.cc:15:
AutofillDriver* autofill_driver) : autofill_driver_(autofill_driver) {
On 2013/11/22 00:26:29, Ilya Sherman wrote:
> nit: Please add DCHECK(autofill_driver) here, since it's currently implicitly
> assumed that autofill_driver is non-null; and there's no code in the
constructor
> that explicitly exercises that assumption.

Done.

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
File components/autofill/core/browser/password_autofill_manager.h (right):

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager.h:64: // outlive
|this| object.
On 2013/11/21 17:08:45, blundell wrote:
> s/ object//

Done.

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager.h:65: AutofillDriver*
autofill_driver_;  // weak
On 2013/11/22 00:26:29, Ilya Sherman wrote:
> nit: Can this be declared as "AutofillDriver* const autofill_driver_;  //
weak"?
>  (i.e., is the pointer value const?)

Yes.

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
File components/autofill/core/browser/password_autofill_manager_unittest.cc
(right):

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager_unittest.cc:27:
void(const base::string16&));
On 2013/11/22 00:26:29, Ilya Sherman wrote:
> nit: Please leave a blank line after this one.

Done.

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager_unittest.cc:30: };
On 2013/11/22 00:26:29, Ilya Sherman wrote:
> nit: Please tuck this into an anonymous namespace.

Done.

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager_unittest.cc:37: 
On 2013/11/22 00:26:29, Ilya Sherman wrote:
> nit: Spurious blank line.

Done.

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager_unittest.cc:55:
password_autofill_manager_.get()->AddPasswordFormMapping(username_field_,
On 2013/11/22 00:26:29, Ilya Sherman wrote:
> nit: No need for ".get()" to dereference a smart pointer; just the "->" arrow
is
> sufficient.

Done.

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager_unittest.cc:73:
base::MessageLoop message_loop_;
On 2013/11/21 17:08:45, blundell wrote:
> Comment on why this is needed (i.e., because the SWP used by
TestAutofillManager
> expects there to be a current MessageLoop).

Done.

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager_unittest.cc:74:
scoped_ptr<MockAutofillDriver> autofill_driver_;
On 2013/11/21 17:08:45, blundell wrote:
> Nit: Do these have to be scoped_ptrs rather than objects that are initialized
in
> the test's constructor?

No. Fixed.

https://codereview.chromium.org/79103002/diff/480001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager_unittest.cc:74:
scoped_ptr<MockAutofillDriver> autofill_driver_;
On 2013/11/22 00:26:29, Ilya Sherman wrote:
> On 2013/11/21 17:08:45, blundell wrote:
> > Nit: Do these have to be scoped_ptrs rather than objects that are
initialized
> in
> > the test's constructor?
> 
> +1

Done.

[commit-bot: I haz the power, 2013-11-22 13:33:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeanfrancoisg@chromium.org/79103002/56...

[commit-bot: I haz the power, 2013-11-22 15:34:36]

Change committed as 236768