Eliminate AutofillExternalDelegate being a WebContentsUserData

Eliminate AutofillExternalDelegate being a WebContentsUserData

This CL changes AutofillExternalDelegate from being a WebContentsUserData to
being owned by AutofillDriverImpl as part of making Autofill a layered
component.

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

[blundell, 2013-06-17 15:14:37]

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
File components/autofill/content/browser/autofill_driver_impl.h (right):

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
components/autofill/content/browser/autofill_driver_impl.h:42: bool
enable_native_ui);
I looked at internalizing this parameter, but AndroidWebview always sets it to
true, so I think it doesn't make sense to internalize.

[Ilya Sherman, 2013-06-17 21:25:12]

LGTM % nits.  Thanks!

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
File components/autofill/content/browser/autofill_driver_impl.cc (right):

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
components/autofill/content/browser/autofill_driver_impl.cc:61: new
AutofillExternalDelegate(web_contents, &autofill_manager_)));
nit: Reduce indentation by two spaces.  You might want to tweak your editor
settings so that this formatting happens automatically ;)

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
components/autofill/content/browser/autofill_driver_impl.cc:85: {
nit: Curly brace should be on the previous line.

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
components/autofill/content/browser/autofill_driver_impl.cc:87:
autofill_manager_.SetExternalDelegate(autofill_external_delegate_.get());
nit: Reduce indentation by two spaces.

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
File components/autofill/content/browser/autofill_driver_impl.h (right):

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
components/autofill/content/browser/autofill_driver_impl.h:49: return
autofill_external_delegate_.get();
nit: Reduce indentation by two spaces.

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
components/autofill/content/browser/autofill_driver_impl.h:55:
scoped_ptr<AutofillExternalDelegate> delegate);
Why is this method, as well as the one above, public?  AFAICT they're only used
by tests.

[blundell, 2013-06-17 21:31:48]

Thanks! Will address comments tomorrow, but just a quick question.

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
File components/autofill/content/browser/autofill_driver_impl.h (right):

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
components/autofill/content/browser/autofill_driver_impl.h:55:
scoped_ptr<AutofillExternalDelegate> delegate);
I had thought about this. The tests can't operate on a subclass of this class
with expanded visibility since they get the instance from the WebContents. It
seems weird to friend tests in chrome/ from the autofill component. Do you see a
possibility that I'm missing?

On 2013/06/17 21:25:12, Ilya Sherman wrote:
> Why is this method, as well as the one above, public?  AFAICT they're only
used
> by tests.

[Ilya Sherman, 2013-06-17 21:45:27]

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
File components/autofill/content/browser/autofill_driver_impl.h (right):

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
components/autofill/content/browser/autofill_driver_impl.h:55:
scoped_ptr<AutofillExternalDelegate> delegate);
Seems like tests should be able to set a different instance of this class on the
WebContents, rather than operating on the one that's already present.  At least,
I'd hope that WCUD classes are test-friendly in that way.

On 2013/06/17 21:31:48, blundell wrote:
> I had thought about this. The tests can't operate on a subclass of this class
> with expanded visibility since they get the instance from the WebContents. It
> seems weird to friend tests in chrome/ from the autofill component. Do you see
a
> possibility that I'm missing?
> 
> On 2013/06/17 21:25:12, Ilya Sherman wrote:
> > Why is this method, as well as the one above, public?  AFAICT they're only
> used
> > by tests.
>

[blundell, 2013-06-18 06:20:46]

Thanks!

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
File components/autofill/content/browser/autofill_driver_impl.cc (right):

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
components/autofill/content/browser/autofill_driver_impl.cc:61: new
AutofillExternalDelegate(web_contents, &autofill_manager_)));
This indentation of 4 spaces is correct I believe, as it's a continuation of the
previous line.

On 2013/06/17 21:25:12, Ilya Sherman wrote:
> nit: Reduce indentation by two spaces.  You might want to tweak your editor
> settings so that this formatting happens automatically ;)

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
components/autofill/content/browser/autofill_driver_impl.cc:85: {
On 2013/06/17 21:25:12, Ilya Sherman wrote:
> nit: Curly brace should be on the previous line.

Done.

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
components/autofill/content/browser/autofill_driver_impl.cc:87:
autofill_manager_.SetExternalDelegate(autofill_external_delegate_.get());
On 2013/06/17 21:25:12, Ilya Sherman wrote:
> nit: Reduce indentation by two spaces.

Done.

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
File components/autofill/content/browser/autofill_driver_impl.h (right):

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
components/autofill/content/browser/autofill_driver_impl.h:49: return
autofill_external_delegate_.get();
On 2013/06/17 21:25:12, Ilya Sherman wrote:
> nit: Reduce indentation by two spaces.

Done.

https://codereview.chromium.org/17225008/diff/1/components/autofill/content/b...
components/autofill/content/browser/autofill_driver_impl.h:55:
scoped_ptr<AutofillExternalDelegate> delegate);
There's nothing like that for WCUD/SupportsUserData as far as I can tell...I'll
push this as-is and follow up with Avi to see if there's something I can do.

On 2013/06/17 21:45:27, Ilya Sherman wrote:
> Seems like tests should be able to set a different instance of this class on
the
> WebContents, rather than operating on the one that's already present.  At
least,
> I'd hope that WCUD classes are test-friendly in that way.
> 
> On 2013/06/17 21:31:48, blundell wrote:
> > I had thought about this. The tests can't operate on a subclass of this
class
> > with expanded visibility since they get the instance from the WebContents.
It
> > seems weird to friend tests in chrome/ from the autofill component. Do you
see
> a
> > possibility that I'm missing?
> > 
> > On 2013/06/17 21:25:12, Ilya Sherman wrote:
> > > Why is this method, as well as the one above, public?  AFAICT they're only
> > used
> > > by tests.
> > 
>

[commit-bot: I haz the power, 2013-06-18 06:21:24]

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

[commit-bot: I haz the power, 2013-06-18 13:31:42]

Change committed as 206967