Introduce AutofillClient and use it to get rid of PasswordManager dependency.

Introduce AutofillClient and use it to get rid of PasswordManager dependency.

Also, this switches use of InfoBarTabService and PrefServiceBase in
AutofillManager from calling their ::ForXyz methods, to retrieving
them via AutofillClient.

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

[Jói, 2012-08-21 16:30:46]

Erik,

I think you will be happy to see this, as it is a step in the direction of
injecting dependencies and removing all knowledge of the concept of a tab and a
profile from Autofill.

It seemed easiest to introduce an AutofillClient delegate interface to get rid
of the PasswordManager dependency, so I added it with naming such as content
established before (ContentClient, ContentBrowserClient, ContentRendererClient),
and a code location beside the ChromeContentBrowserClient.  I do wonder if we
should carve out a new location for such component client implementations (e.g.
chrome/browser/clients) or have it live in chrome/browser/tab_contents (in terms
of which it is mostly implemented), will loop Ben in once you're happy with the
rest of the review.

We still need the ::ForXyz functions on the interfaces for some pieces of code
that haven't been worked on yet.  At some point we could probably rename those
functions ::ForXyzForTesting and then eventually perhaps drop them.

Cheers,
Jói

http://codereview.chromium.org/10837363/diff/1/chrome/browser/plugin_infobar_...
File chrome/browser/plugin_infobar_delegates.cc (right):

http://codereview.chromium.org/10837363/diff/1/chrome/browser/plugin_infobar_...
chrome/browser/plugin_infobar_delegates.cc:178: :
PluginInfoBarDelegate(InfoBarTabService::ForTab(observer->tab_contents()),
This was just a formatting fix I stumbled on as I was switching from ::ForTab
calls where possible.

[erikwright (departed), 2012-08-21 18:11:00]

LGTM.

I would rename ChromeAutofillClient to TabContentsAutofillClient or
TabAutofillClient.

Reasoning is that usually the Chrome* implementation of something is really
tightly coupled to the entire browser, with the result that I was quite
surprised that a ChromeAutofillClient could be safely used in a unit test (until
I read the implementation).

[Ilya Sherman, 2012-08-22 03:38:03]

http://codereview.chromium.org/10837363/diff/1/chrome/browser/autofill/api/au...
File chrome/browser/autofill/api/autofill_client.h (right):

http://codereview.chromium.org/10837363/diff/1/chrome/browser/autofill/api/au...
chrome/browser/autofill/api/autofill_client.h:21: class AutofillClient {
Hmm, this seems like it is more of a host than a client.  What do you think of
naming this something more like AutofillHost?

http://codereview.chromium.org/10837363/diff/1/chrome/browser/autofill/autofi...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/10837363/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager.h:178: autofill::AutofillClient*
client_;
nit: Please make this private, and expose accessors if needed for testing.

http://codereview.chromium.org/10837363/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager.h:178: autofill::AutofillClient*
client_;
nit: Can this be a const member?

http://codereview.chromium.org/10837363/diff/1/chrome/browser/chrome_autofill...
File chrome/browser/chrome_autofill_client.h (right):

http://codereview.chromium.org/10837363/diff/1/chrome/browser/chrome_autofill...
chrome/browser/chrome_autofill_client.h:22: virtual bool
IsSavingPasswordsEnabled() OVERRIDE;
nit: Should this method be const?

http://codereview.chromium.org/10837363/diff/1/chrome/browser/chrome_autofill...
chrome/browser/chrome_autofill_client.h:25: TabContents* tab_;
nit: Can this be a const-member, i.e. TabContents* const tab_; ?

http://codereview.chromium.org/10837363/diff/1/chrome/browser/ui/tab_contents...
File chrome/browser/ui/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/10837363/diff/1/chrome/browser/ui/tab_contents...
chrome/browser/ui/tab_contents/tab_contents.h:272:
scoped_ptr<ChromeAutofillClient> autofill_client_;
Why not have the AutofillManager own the client?

[Ilya Sherman, 2012-08-22 03:39:09]

http://codereview.chromium.org/10837363/diff/1/chrome/browser/ui/tab_contents...
File chrome/browser/ui/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/10837363/diff/1/chrome/browser/ui/tab_contents...
chrome/browser/ui/tab_contents/tab_contents.h:272:
scoped_ptr<ChromeAutofillClient> autofill_client_;
On 2012/08/22 03:38:03, Ilya Sherman wrote:
> Why not have the AutofillManager own the client?

Or, alternately, have the client (host?) own the manager.  It just seems odd to
have both be members of TabContents.

[Jói, 2012-08-22 16:11:34]

The AutofillClient is in fact a Delegate.  I'm going to rename it
AutofillManagerDelegate.  It should follow the conventions of typical
delegates, e.g. this should explain why the AutofillManager should not
own the delegate (since you might want to implement it in the class
that owns the AutofillManager).

Personally I don't think the ChromeAutofillManagerDelegate (née
ChromeAutofillClient) should own the AutofillManager.  That feels to
me like a poor separation of concerns, e.g. since it implies a 1:1
relationship, which for now is true but wouldn't necessarily need to
be so.

I'm going to work on a change to rename the delegate interface to
autofill::AutofillManagerDelegate.  I think given that the current
implementation in Chrome is tied to a Tab, it makes sense to follow
Erik's suggestion to name it TabAutofillManagerDelegate, and make it
live under chrome/browser/tab_contents.

Cheers,
Jói


On Wed, Aug 22, 2012 at 3:39 AM,  <isherman@chromium.org> wrote:
>
>
http://codereview.chromium.org/10837363/diff/1/chrome/browser/ui/tab_contents...
> File chrome/browser/ui/tab_contents/tab_contents.h (right):
>
>
http://codereview.chromium.org/10837363/diff/1/chrome/browser/ui/tab_contents...
> chrome/browser/ui/tab_contents/tab_contents.h:272:
> scoped_ptr<ChromeAutofillClient> autofill_client_;
> On 2012/08/22 03:38:03, Ilya Sherman wrote:
>>
>> Why not have the AutofillManager own the client?
>
>
> Or, alternately, have the client (host?) own the manager.  It just seems
> odd to have both be members of TabContents.
>
> http://codereview.chromium.org/10837363/

[Jói, 2012-08-22 17:15:53]

PTAL, and see previous email on the question of naming and ownership structures.

http://codereview.chromium.org/10837363/diff/1/chrome/browser/autofill/autofi...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/10837363/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager.h:178: autofill::AutofillClient*
client_;
On 2012/08/22 03:38:03, Ilya Sherman wrote:
> nit: Please make this private, and expose accessors if needed for testing.

Any subclass would seem to need this, so I think it makes sense to leave this as
protected.

http://codereview.chromium.org/10837363/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager.h:178: autofill::AutofillClient*
client_;
On 2012/08/22 03:38:03, Ilya Sherman wrote:
> nit: Can this be a const member?

Done.

http://codereview.chromium.org/10837363/diff/1/chrome/browser/chrome_autofill...
File chrome/browser/chrome_autofill_client.h (right):

http://codereview.chromium.org/10837363/diff/1/chrome/browser/chrome_autofill...
chrome/browser/chrome_autofill_client.h:22: virtual bool
IsSavingPasswordsEnabled() OVERRIDE;
On 2012/08/22 03:38:03, Ilya Sherman wrote:
> nit: Should this method be const?

Done.

http://codereview.chromium.org/10837363/diff/1/chrome/browser/chrome_autofill...
chrome/browser/chrome_autofill_client.h:25: TabContents* tab_;
On 2012/08/22 03:38:03, Ilya Sherman wrote:
> nit: Can this be a const-member, i.e. TabContents* const tab_; ?

Done.

[tfarina, 2012-08-22 17:24:02]

On Wednesday, August 22, 2012, wrote:

> PTAL, and see previous email on the question of naming and ownership
> structures.
>
>
> http://codereview.chromium.**org/10837363/diff/1/chrome/**
>
browser/autofill/autofill_**manager.h<http://codereview.chromium.org/10837363/diff/1/chrome/browser/autofill/autofill_manager.h>
> File chrome/browser/autofill/**autofill_manager.h (right):
>
> http://codereview.chromium.**org/10837363/diff/1/chrome/**
>
browser/autofill/autofill_**manager.h#newcode178<http://codereview.chromium.org/10837363/diff/1/chrome/browser/autofill/autofill_manager.h#newcode178>
> chrome/browser/autofill/**autofill_manager.h:178:
> autofill::AutofillClient* client_;
> On 2012/08/22 03:38:03, Ilya Sherman wrote:
>
>> nit: Please make this private, and expose accessors if needed for
>>
> testing.
>
> Any subclass would seem to need this, so I think it makes sense to leave
> this as protected.
>
> The style guide says we should avoid protected data members and provide
accessors.

http://codereview.chromium.**org/10837363/diff/1/chrome/**
>
browser/autofill/autofill_**manager.h#newcode178<http://codereview.chromium.org/10837363/diff/1/chrome/browser/autofill/autofill_manager.h#newcode178>
> chrome/browser/autofill/**autofill_manager.h:178:
> autofill::AutofillClient* client_;
> On 2012/08/22 03:38:03, Ilya Sherman wrote:
>
>> nit: Can this be a const member?
>>
>
> Done.
>
> http://codereview.chromium.**org/10837363/diff/1/chrome/**
>
browser/chrome_autofill_**client.h<http://codereview.chromium.org/10837363/diff/1/chrome/browser/chrome_autofill_client.h>
> File chrome/browser/chrome_**autofill_client.h (right):
>
> http://codereview.chromium.**org/10837363/diff/1/chrome/**
>
browser/chrome_autofill_**client.h#newcode22<http://codereview.chromium.org/10837363/diff/1/chrome/browser/chrome_autofill_client.h#newcode22>
> chrome/browser/chrome_**autofill_client.h:22: virtual bool
> IsSavingPasswordsEnabled() OVERRIDE;
> On 2012/08/22 03:38:03, Ilya Sherman wrote:
>
>> nit: Should this method be const?
>>
>
> Done.
>
> http://codereview.chromium.**org/10837363/diff/1/chrome/**
>
browser/chrome_autofill_**client.h#newcode25<http://codereview.chromium.org/10837363/diff/1/chrome/browser/chrome_autofill_client.h#newcode25>
> chrome/browser/chrome_**autofill_client.h:25: TabContents* tab_;
> On 2012/08/22 03:38:03, Ilya Sherman wrote:
>
>> nit: Can this be a const-member, i.e. TabContents* const tab_; ?
>>
>
> Done.
>
>
http://codereview.chromium.**org/10837363/<http://codereview.chromium.org/108...
>
> --
> You received this message because you are subscribed to the Google Groups
> "browser-components-watch" group.
> To post to this group, send email to browser-components-watch@chromium.org
> .
> To unsubscribe from this group, send email to
> browser-components-watch+unsubscribe@chromium.org.
> For more options, visit https://groups.google.com/a/**
>
chromium.org/groups/opt_out<https://groups.google.com/a/chromium.org/groups/opt_out>
> .
>
>
>

-- 
Thiago

[erikwright (departed), 2012-08-22 19:35:07]

LEBTM.

http://codereview.chromium.org/10837363/diff/10003/chrome/browser/tab_content...
File chrome/browser/tab_contents/tab_autofill_manager_delegate.h (right):

http://codereview.chromium.org/10837363/diff/10003/chrome/browser/tab_content...
chrome/browser/tab_contents/tab_autofill_manager_delegate.h:13: // Chrome
implementation of AutofillManagerClient
fix classname in comment.

[Ilya Sherman, 2012-08-22 21:47:58]

LGTM (with nits).

http://codereview.chromium.org/10837363/diff/1/chrome/browser/autofill/autofi...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/10837363/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager.h:178: autofill::AutofillClient*
client_;
On 2012/08/22 17:15:53, Jói wrote:
> On 2012/08/22 03:38:03, Ilya Sherman wrote:
> > nit: Please make this private, and expose accessors if needed for testing.
> 
> Any subclass would seem to need this, so I think it makes sense to leave this
as
> protected.

A subclass would presumably only need a getter.  I'm pretty sure that the style
I've seen in all the other Chromium code I've encountered is not to directly
expose members to subclasses.

http://codereview.chromium.org/10837363/diff/10003/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_external_delegate_unittest.cc (right):

http://codereview.chromium.org/10837363/diff/10003/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_external_delegate_unittest.cc:70:
delegate_(tab_contents) {
nit: Indent one more space

http://codereview.chromium.org/10837363/diff/10003/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/10837363/diff/10003/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:177:
autofill::AutofillManagerDelegate* const delegate_;
Note that there is also a member named |external_delegate_|, so this name is
somewhat confusing in context.  It would be nice to rationalize the two
somehow...

[Jói, 2012-08-23 09:50:40]

+Avi:  chrome/browser/tab_contents/OWNERS approval.

Cheers,
Jói

http://codereview.chromium.org/10837363/diff/1/chrome/browser/autofill/autofi...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/10837363/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager.h:178: autofill::AutofillClient*
client_;
On 2012/08/22 21:47:59, Ilya Sherman wrote:
> On 2012/08/22 17:15:53, Jói wrote:
> > On 2012/08/22 03:38:03, Ilya Sherman wrote:
> > > nit: Please make this private, and expose accessors if needed for testing.
> > 
> > Any subclass would seem to need this, so I think it makes sense to leave
this
> as
> > protected.
> 
> A subclass would presumably only need a getter.  I'm pretty sure that the
style
> I've seen in all the other Chromium code I've encountered is not to directly
> expose members to subclasses.

Right now, the subclasses don't use the member (my mistake when responding
earlier - I think they did in an in-progress version before I sent for review)
so I just made it private and skipped the accessor.

http://codereview.chromium.org/10837363/diff/10003/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_external_delegate_unittest.cc (right):

http://codereview.chromium.org/10837363/diff/10003/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_external_delegate_unittest.cc:70:
delegate_(tab_contents) {
On 2012/08/22 21:47:59, Ilya Sherman wrote:
> nit: Indent one more space

Done.

http://codereview.chromium.org/10837363/diff/10003/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/10837363/diff/10003/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:177:
autofill::AutofillManagerDelegate* const delegate_;
On 2012/08/22 21:47:59, Ilya Sherman wrote:
> Note that there is also a member named |external_delegate_|, so this name is
> somewhat confusing in context.  It would be nice to rationalize the two
> somehow...

Renamed this one manager_delegate_.

http://codereview.chromium.org/10837363/diff/10003/chrome/browser/tab_content...
File chrome/browser/tab_contents/tab_autofill_manager_delegate.h (right):

http://codereview.chromium.org/10837363/diff/10003/chrome/browser/tab_content...
chrome/browser/tab_contents/tab_autofill_manager_delegate.h:13: // Chrome
implementation of AutofillManagerClient
On 2012/08/22 19:35:07, erikwright wrote:
> fix classname in comment.

Done.

[Jói, 2012-08-23 11:42:26]

FYI:  I'm starting to think TabAutofillManagerDelegate belongs in
chrome/browser/ui/tab_contents rather than
chrome/browser/tab_contents, as my next change to it will add a number
of UI things (use of Browser and chrome::ShowXyzPage and such).

Avi, do you concur?

Cheers,
Jói



On Thu, Aug 23, 2012 at 9:50 AM,  <joi@chromium.org> wrote:
> +Avi:  chrome/browser/tab_contents/OWNERS approval.
>
> Cheers,
> Jói
>
>
>
>
>
http://codereview.chromium.org/10837363/diff/1/chrome/browser/autofill/autofi...
> File chrome/browser/autofill/autofill_manager.h (right):
>
>
http://codereview.chromium.org/10837363/diff/1/chrome/browser/autofill/autofi...
> chrome/browser/autofill/autofill_manager.h:178:
> autofill::AutofillClient* client_;
> On 2012/08/22 21:47:59, Ilya Sherman wrote:
>>
>> On 2012/08/22 17:15:53, Jói wrote:
>> > On 2012/08/22 03:38:03, Ilya Sherman wrote:
>> > > nit: Please make this private, and expose accessors if needed for
>
> testing.
>>
>> >
>> > Any subclass would seem to need this, so I think it makes sense to
>
> leave this
>>
>> as
>> > protected.
>
>
>> A subclass would presumably only need a getter.  I'm pretty sure that
>
> the style
>>
>> I've seen in all the other Chromium code I've encountered is not to
>
> directly
>>
>> expose members to subclasses.
>
>
> Right now, the subclasses don't use the member (my mistake when
> responding earlier - I think they did in an in-progress version before I
> sent for review) so I just made it private and skipped the accessor.
>
>
>
http://codereview.chromium.org/10837363/diff/10003/chrome/browser/autofill/au...
> File chrome/browser/autofill/autofill_external_delegate_unittest.cc
> (right):
>
>
http://codereview.chromium.org/10837363/diff/10003/chrome/browser/autofill/au...
> chrome/browser/autofill/autofill_external_delegate_unittest.cc:70:
> delegate_(tab_contents) {
> On 2012/08/22 21:47:59, Ilya Sherman wrote:
>>
>> nit: Indent one more space
>
>
> Done.
>
>
>
http://codereview.chromium.org/10837363/diff/10003/chrome/browser/autofill/au...
> File chrome/browser/autofill/autofill_manager.h (right):
>
>
http://codereview.chromium.org/10837363/diff/10003/chrome/browser/autofill/au...
> chrome/browser/autofill/autofill_manager.h:177:
> autofill::AutofillManagerDelegate* const delegate_;
> On 2012/08/22 21:47:59, Ilya Sherman wrote:
>>
>> Note that there is also a member named |external_delegate_|, so this
>
> name is
>>
>> somewhat confusing in context.  It would be nice to rationalize the
>
> two
>>
>> somehow...
>
>
> Renamed this one manager_delegate_.
>
>
>
http://codereview.chromium.org/10837363/diff/10003/chrome/browser/tab_content...
> File chrome/browser/tab_contents/tab_autofill_manager_delegate.h
> (right):
>
>
http://codereview.chromium.org/10837363/diff/10003/chrome/browser/tab_content...
> chrome/browser/tab_contents/tab_autofill_manager_delegate.h:13: //
> Chrome implementation of AutofillManagerClient
> On 2012/08/22 19:35:07, erikwright wrote:
>>
>> fix classname in comment.
>
>
> Done.
>
> http://codereview.chromium.org/10837363/

[Avi (use Gerrit), 2012-08-23 16:45:51]

On 2012/08/23 11:42:26, Jói wrote:
> FYI:  I'm starting to think TabAutofillManagerDelegate belongs in
> chrome/browser/ui/tab_contents rather than
> chrome/browser/tab_contents

As opposed to chrome/browser/ui/autofill?

I'd like to kill the tab_contents directory since it's becoming a catch-all. I
like the idea of keeping classes organized by function rather than throwing
everything tab-related into the same directory.

[Jói, 2012-08-23 21:26:47]

> As opposed to chrome/browser/ui/autofill?

That makes sense actually, a directory for UI stuff for Autofill that
is not specific to a port (e.g. we already have
chrome/browser/ui/gtk/autofill).  If nobody objects, that's what I'll
do.

Cheers,
Jói


On Thu, Aug 23, 2012 at 4:45 PM,  <avi@chromium.org> wrote:
> On 2012/08/23 11:42:26, Jói wrote:
>>
>> FYI:  I'm starting to think TabAutofillManagerDelegate belongs in
>> chrome/browser/ui/tab_contents rather than
>> chrome/browser/tab_contents
>
>
> As opposed to chrome/browser/ui/autofill?
>
> I'd like to kill the tab_contents directory since it's becoming a catch-all.
> I
> like the idea of keeping classes organized by function rather than throwing
> everything tab-related into the same directory.
>
> http://codereview.chromium.org/10837363/

[erikwright (departed), 2012-08-23 21:37:43]

Hmm. If we take that approach, what about things that would previously have
belonged in chrome/browser/tab_contents?

i.e., if there are delegates/adapters that are implemented by the browser
in order to host a component, but they are not UI-level, where will they
go? chrome/browser/autofill? Oops - can't do that...

Ideas?

On Thu, Aug 23, 2012 at 2:26 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> > As opposed to chrome/browser/ui/autofill?
>
> That makes sense actually, a directory for UI stuff for Autofill that
> is not specific to a port (e.g. we already have
> chrome/browser/ui/gtk/autofill).  If nobody objects, that's what I'll
> do.
>
> Cheers,
> Jói
>
>
> On Thu, Aug 23, 2012 at 4:45 PM,  <avi@chromium.org> wrote:
> > On 2012/08/23 11:42:26, Jói wrote:
> >>
> >> FYI:  I'm starting to think TabAutofillManagerDelegate belongs in
> >> chrome/browser/ui/tab_contents rather than
> >> chrome/browser/tab_contents
> >
> >
> > As opposed to chrome/browser/ui/autofill?
> >
> > I'd like to kill the tab_contents directory since it's becoming a
> catch-all.
> > I
> > like the idea of keeping classes organized by function rather than
> throwing
> > everything tab-related into the same directory.
> >
> > http://codereview.chromium.org/10837363/
>

[Avi (use Gerrit), 2012-08-23 22:17:09]

I'm not saying that _nothing_ will go there, but given that the concept of a
"tab" is an inherently UI-centric concept, I'm not sure what qualifies as a
non-ui level "tab" item. Can you provide examples?

(And this is straying a bit far from the review at hand; do you have an
objection to chrome/browser/ui/autofill ?)

[erikwright (departed), 2012-08-23 22:19:05]

No objection. In the worst case we will do a bit more moving around in the
future. Still L and SGTM.

On Thu, Aug 23, 2012 at 3:17 PM, <avi@chromium.org> wrote:

> I'm not saying that _nothing_ will go there, but given that the concept of
> a
> "tab" is an inherently UI-centric concept, I'm not sure what qualifies as a
> non-ui level "tab" item. Can you provide examples?
>
> (And this is straying a bit far from the review at hand; do you have an
> objection to chrome/browser/ui/autofill ?)
>
>
>
http://codereview.chromium.**org/10837363/<http://codereview.chromium.org/108...
>
> --
> You received this message because you are subscribed to the Google Groups
> "browser-components-watch" group.
> To post to this group, send email to browser-components-watch@**
> chromium.org <browser-components-watch@chromium.org>.
> To unsubscribe from this group, send email to browser-components-watch+**
> unsubscribe@chromium.org<browser-components-watch%2Bunsubscribe@chromium.org>
> .
> For more options, visit https://groups.google.com/a/**
>
chromium.org/groups/opt_out<https://groups.google.com/a/chromium.org/groups/opt_out>
> .
>
>
>

[Jói, 2012-08-29 18:30:32]

+ben for chrome/browser/ui/OWNERS approval

Ben, just need your OK for the new directory as suggested by Avi,
chrome/browser/ui/autofill for non-port-specific Autofill UI code.

Cheers,
Jói

[Ben Goodger (Google), 2012-08-29 19:58:23]

lgtm

[commit-bot: I haz the power, 2012-08-29 20:06:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10837363/27002