Abstract WebContentsObserver from Autofill shared code

Abstract WebContentsObserver from Autofill shared code

Introduces the AutofillDriver, whose purpose is to abstract communication from the renderer and the external world to autofill browser process code. This CL concretely uses AutofillDriverImpl to eliminate AutofillManager and
AutocompleteHistoryManager having to be WebContentsObservers. It also eliminates AutofillManager being a WebContentsUserData, instead changing it to be owned by and accessed from the AutofillDriverImpl.

TBR=benm,yfriedman,avi
TEST=No visible behavior in the change of autofill.

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

[blundell, 2013-06-04 17:30:48]

I have not yet introduced DEPS restrictions or added a README in this CL.

With respect to introducing DEPS restrictions, do you think that it makes sense
to move files into their anticipated final structure now-ish or to wait until
the work is farther along? The former would more easily enable introducing sane
DEPS restrictions -- in the structure that the code is in now, there are files
that in the long term shouldn't be using content sitting next to files that will
be able to continue to use content.

https://codereview.chromium.org/16286020/diff/2001/components/autofill/browse...
File components/autofill/browser/autocomplete_history_manager.cc (left):

https://codereview.chromium.org/16286020/diff/2001/components/autofill/browse...
components/autofill/browser/autocomplete_history_manager.cc:66:
IPC_MESSAGE_HANDLER(AutofillHostMsg_RemoveAutocompleteEntry,
I moved the reception of this message into AutofillManager, which I saw already
had a RemoveAutocompleteEntry() method that forwarded to
AutocompleteHistoryManager. Any pointers on how to test this change would be
appreciated -- I could not find any tests that failed even when I commented out
the handling of this message entirely.

https://codereview.chromium.org/16286020/diff/2001/components/autofill/browse...
File components/autofill/browser/autocomplete_history_manager.cc (right):

https://codereview.chromium.org/16286020/diff/2001/components/autofill/browse...
components/autofill/browser/autocomplete_history_manager.cc:200: } else if
(send_IPC_) {
This change is to preserve the behavior of the ExternalDelegate test --
AutocompleteHistoryManager no longer has a Send() method inherited from
WebContentsObserver.

https://codereview.chromium.org/16286020/diff/2001/components/autofill/browse...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/16286020/diff/2001/components/autofill/browse...
components/autofill/browser/autofill_manager.cc:790: // web_contents_ will be
NULL when the interactive autocomplete is closed due
Hmm, I missed this comment. I had thought that replacing usages of
WebContentsObserver's web_contents() method by holding on to the WebContents*
passed in to the constructor would be a safe change to make, but this comment
makes me pause.

https://codereview.chromium.org/16286020/diff/2001/components/autofill/conten...
File components/autofill/content/browser/autofill_driver.cc (right):

https://codereview.chromium.org/16286020/diff/2001/components/autofill/conten...
components/autofill/content/browser/autofill_driver.cc:55:
AutofillManager::CreateForWebContentsAndDelegate(web_contents,
Follow-up work will expose these instances directly to consumers via accessors.

[Evan Stade, 2013-06-04 18:06:36]

moving the files early seems fine to me.

https://codereview.chromium.org/16286020/diff/2001/components/autofill/browse...
File components/autofill/browser/autocomplete_history_manager.cc (left):

https://codereview.chromium.org/16286020/diff/2001/components/autofill/browse...
components/autofill/browser/autocomplete_history_manager.cc:66:
IPC_MESSAGE_HANDLER(AutofillHostMsg_RemoveAutocompleteEntry,
On 2013/06/04 17:30:48, blundell wrote:
> I moved the reception of this message into AutofillManager, which I saw
already
> had a RemoveAutocompleteEntry() method that forwarded to
> AutocompleteHistoryManager. Any pointers on how to test this change would be
> appreciated -- I could not find any tests that failed even when I commented
out
> the handling of this message entirely.

Ilya would know for sure but my guess is that it's only used in the old-style
renderer-side autofill implentation. So you'd have to disable the native
autofill ui (in about:flags) to test this manually. But on the other hand it's
probably going away soon.

https://codereview.chromium.org/16286020/diff/2001/components/autofill/browse...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/16286020/diff/2001/components/autofill/browse...
components/autofill/browser/autofill_manager.cc:790: // web_contents_ will be
NULL when the interactive autocomplete is closed due
On 2013/06/04 17:30:48, blundell wrote:
> Hmm, I missed this comment. I had thought that replacing usages of
> WebContentsObserver's web_contents() method by holding on to the WebContents*
> passed in to the constructor would be a safe change to make, but this comment
> makes me pause.

so I guess you need to pipe through destruction notifications from the driver?

[Ilya Sherman, 2013-06-05 10:50:02]

https://codereview.chromium.org/16286020/diff/2001/components/autofill/browse...
File components/autofill/browser/autocomplete_history_manager.cc (left):

https://codereview.chromium.org/16286020/diff/2001/components/autofill/browse...
components/autofill/browser/autocomplete_history_manager.cc:66:
IPC_MESSAGE_HANDLER(AutofillHostMsg_RemoveAutocompleteEntry,
On 2013/06/04 18:06:36, Evan Stade wrote:
> On 2013/06/04 17:30:48, blundell wrote:
> > I moved the reception of this message into AutofillManager, which I saw
> already
> > had a RemoveAutocompleteEntry() method that forwarded to
> > AutocompleteHistoryManager. Any pointers on how to test this change would be
> > appreciated -- I could not find any tests that failed even when I commented
> out
> > the handling of this message entirely.
> 
> Ilya would know for sure but my guess is that it's only used in the old-style
> renderer-side autofill implentation. So you'd have to disable the native
> autofill ui (in about:flags) to test this manually. But on the other hand it's
> probably going away soon.

Yes, Evan is correct -- this IPC message would only ever be generated by the
deprecated UI, support for which should be safe to rip out at this point.

https://codereview.chromium.org/16286020/diff/9001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_tab_contents.cc (right):

https://codereview.chromium.org/16286020/diff/9001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_tab_contents.cc:121: native_autofill_ui_enabled);
Rather than passing in the boolean, could you just query the commandline state
within the AutofillDriver?

https://codereview.chromium.org/16286020/diff/9001/components/autofill/browse...
File components/autofill/browser/autocomplete_history_manager.cc (right):

https://codereview.chromium.org/16286020/diff/9001/components/autofill/browse...
components/autofill/browser/autocomplete_history_manager.cc:207:
autofill_unique_ids_));
Hmm, why isn't this being handled by the AutofillDriver now?

https://codereview.chromium.org/16286020/diff/9001/components/autofill/browse...
File components/autofill/browser/autocomplete_history_manager.h (right):

https://codereview.chromium.org/16286020/diff/9001/components/autofill/browse...
components/autofill/browser/autocomplete_history_manager.h:59:
FRIEND_TEST_ALL_PREFIXES(AutocompleteHistoryManagerTest, ExternalDelegate);
Why is this needed?

https://codereview.chromium.org/16286020/diff/9001/components/autofill/browse...
components/autofill/browser/autocomplete_history_manager.h:65: 
nit: Spurious blank line.

https://codereview.chromium.org/16286020/diff/9001/components/autofill/browse...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/16286020/diff/9001/components/autofill/browse...
components/autofill/browser/autofill_manager.cc:209: :
web_contents_(web_contents),
This doesn't seem safe, as the WebContentsObserver makes sure to update the
state of web_contents_, whereas no such updating is happening within the
AutofillManager class.  It would likely be more appropriate for the
AutofillManager class to query the AutofillDriver for its WebContents.

https://codereview.chromium.org/16286020/diff/9001/components/autofill/conten...
File components/autofill/content/browser/autofill_driver.cc (right):

https://codereview.chromium.org/16286020/diff/9001/components/autofill/conten...
components/autofill/content/browser/autofill_driver.cc:16: const char*
kAutofillDriverWebContentsUserDataKey =
nit: const char kAutofillDriverWebContentsUserDataKey[].

[blundell, 2013-06-11 15:35:46]

Thanks for the reviews. Most significant change in this iteration is having
AutofillDriver be an interface in the shared code in order to provide
driver-level context to the shared code (currently, it provides an accessor for
the current WebContents; in the long term, it will provide information such as
"am I currently off the record"). This change introduced significant noise into
the tests.

https://codereview.chromium.org/16286020/diff/2001/components/autofill/browse...
File components/autofill/browser/autocomplete_history_manager.cc (left):

https://codereview.chromium.org/16286020/diff/2001/components/autofill/browse...
components/autofill/browser/autocomplete_history_manager.cc:66:
IPC_MESSAGE_HANDLER(AutofillHostMsg_RemoveAutocompleteEntry,
Are you saying (a) that it's OK to just not handle this message at this point,
or (b) that the handling of this message will be removed in any case when
components/autofill/renderer/autofill_agent goes away? If the former, I'd rather
isolate that change into a different CL, as this CL is not intended to produce
any behavioral changes.

On 2013/06/05 10:50:02, Ilya Sherman wrote:
> On 2013/06/04 18:06:36, Evan Stade wrote:
> > On 2013/06/04 17:30:48, blundell wrote:
> > > I moved the reception of this message into AutofillManager, which I saw
> > already
> > > had a RemoveAutocompleteEntry() method that forwarded to
> > > AutocompleteHistoryManager. Any pointers on how to test this change would
be
> > > appreciated -- I could not find any tests that failed even when I
commented
> > out
> > > the handling of this message entirely.
> > 
> > Ilya would know for sure but my guess is that it's only used in the
old-style
> > renderer-side autofill implentation. So you'd have to disable the native
> > autofill ui (in about:flags) to test this manually. But on the other hand
it's
> > probably going away soon.
> 
> Yes, Evan is correct -- this IPC message would only ever be generated by the
> deprecated UI, support for which should be safe to rip out at this point.

https://codereview.chromium.org/16286020/diff/2001/components/autofill/browse...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/16286020/diff/2001/components/autofill/browse...
components/autofill/browser/autofill_manager.cc:790: // web_contents_ will be
NULL when the interactive autocomplete is closed due
Added in an interface whereby the shared code can obtain the WebContents
instance from the driver, as that seems less fragile.

On 2013/06/04 18:06:36, Evan Stade wrote:
> On 2013/06/04 17:30:48, blundell wrote:
> > Hmm, I missed this comment. I had thought that replacing usages of
> > WebContentsObserver's web_contents() method by holding on to the
WebContents*
> > passed in to the constructor would be a safe change to make, but this
comment
> > makes me pause.
> 
> so I guess you need to pipe through destruction notifications from the driver?

https://codereview.chromium.org/16286020/diff/9001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_tab_contents.cc (right):

https://codereview.chromium.org/16286020/diff/9001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_tab_contents.cc:121: native_autofill_ui_enabled);
On 2013/06/05 10:50:02, Ilya Sherman wrote:
> Rather than passing in the boolean, could you just query the commandline state
> within the AutofillDriver?

Done.

https://codereview.chromium.org/16286020/diff/9001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_tab_contents.cc:121: native_autofill_ui_enabled);
On 2013/06/05 10:50:02, Ilya Sherman wrote:
> Rather than passing in the boolean, could you just query the commandline state
> within the AutofillDriver?

Done.

https://codereview.chromium.org/16286020/diff/9001/components/autofill/browse...
File components/autofill/browser/autocomplete_history_manager.cc (right):

https://codereview.chromium.org/16286020/diff/9001/components/autofill/browse...
components/autofill/browser/autocomplete_history_manager.cc:207:
autofill_unique_ids_));
The goal of this CL is just to remove the need of AutocompleteHistoryManager and
AutofillManager to be WebContentsObservers. A follow-up CL will introduce
AutofillRendererBridge to abstract IPC out of the shared code.

On 2013/06/05 10:50:02, Ilya Sherman wrote:
> Hmm, why isn't this being handled by the AutofillDriver now?

https://codereview.chromium.org/16286020/diff/9001/components/autofill/browse...
File components/autofill/browser/autocomplete_history_manager.h (right):

https://codereview.chromium.org/16286020/diff/9001/components/autofill/browse...
components/autofill/browser/autocomplete_history_manager.h:59:
FRIEND_TEST_ALL_PREFIXES(AutocompleteHistoryManagerTest, ExternalDelegate);
To disable sending IPC. Previously a custom subclass of
AutocompleteHistoryManager was used in the test for this purpose.

On 2013/06/05 10:50:02, Ilya Sherman wrote:
> Why is this needed?

https://codereview.chromium.org/16286020/diff/9001/components/autofill/browse...
components/autofill/browser/autocomplete_history_manager.h:65: 
On 2013/06/05 10:50:02, Ilya Sherman wrote:
> nit: Spurious blank line.

Done.

https://codereview.chromium.org/16286020/diff/9001/components/autofill/browse...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/16286020/diff/9001/components/autofill/browse...
components/autofill/browser/autofill_manager.cc:209: :
web_contents_(web_contents),
Yes, you're right. Made AutofillDriver an interface declared in the shared code
to handle this.

On 2013/06/05 10:50:02, Ilya Sherman wrote:
> This doesn't seem safe, as the WebContentsObserver makes sure to update the
> state of web_contents_, whereas no such updating is happening within the
> AutofillManager class.  It would likely be more appropriate for the
> AutofillManager class to query the AutofillDriver for its WebContents.

https://codereview.chromium.org/16286020/diff/9001/components/autofill/conten...
File components/autofill/content/browser/autofill_driver.cc (right):

https://codereview.chromium.org/16286020/diff/9001/components/autofill/conten...
components/autofill/content/browser/autofill_driver.cc:16: const char*
kAutofillDriverWebContentsUserDataKey =
On 2013/06/05 10:50:02, Ilya Sherman wrote:
> nit: const char kAutofillDriverWebContentsUserDataKey[].

I looked into doing this but it got messy as some of the tests manually set the
external delegate on the AutofillManager (e.g., to a test external delegate). In
the interest of keeping this CL from growing too humongous, I'll tackle this
change in the follow-up CL as part of moving ownership of the
AutofillManager/AutofillExternalDelegate instances to the driver (i.e.,
eliminating them being WCUD's).

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
File components/autofill/browser/autofill_driver.h (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autofill_driver.h:16: class AutofillDriver {
I envision this interface being the one that will provide information such as
whether the BrowserContext is currently off-the-record, etc. I wrestled with
what to name it -- I think that something like AutofillContext most accurately
conveys its function, but then it gets confusing between which interfaces are
implemented in the driver vs. the embedder of the component. Do you have
opinions?

[blundell, 2013-06-11 21:19:27]

https://codereview.chromium.org/16286020/diff/9001/components/autofill/conten...
File components/autofill/content/browser/autofill_driver.cc (right):

https://codereview.chromium.org/16286020/diff/9001/components/autofill/conten...
components/autofill/content/browser/autofill_driver.cc:16: const char*
kAutofillDriverWebContentsUserDataKey =
This comment was meant to address your suggestion to move the logic for
determining whether the native UI was enabled into the driver (which I did *not*
do for the reasons mentioned below). Sorry for the confusion!

On 2013/06/11 15:35:47, blundell wrote:
> On 2013/06/05 10:50:02, Ilya Sherman wrote:
> > nit: const char kAutofillDriverWebContentsUserDataKey[].
> 
> I looked into doing this but it got messy as some of the tests manually set
the
> external delegate on the AutofillManager (e.g., to a test external delegate).
In
> the interest of keeping this CL from growing too humongous, I'll tackle this
> change in the follow-up CL as part of moving ownership of the
> AutofillManager/AutofillExternalDelegate instances to the driver (i.e.,
> eliminating them being WCUD's).

[Ilya Sherman, 2013-06-12 00:07:45]

https://codereview.chromium.org/16286020/diff/2001/components/autofill/browse...
File components/autofill/browser/autocomplete_history_manager.cc (left):

https://codereview.chromium.org/16286020/diff/2001/components/autofill/browse...
components/autofill/browser/autocomplete_history_manager.cc:66:
IPC_MESSAGE_HANDLER(AutofillHostMsg_RemoveAutocompleteEntry,
(b), though not all of autofill_agent will be going away -- just the parts
handling the popup UI.

On 2013/06/11 15:35:47, blundell wrote:
> Are you saying (a) that it's OK to just not handle this message at this point,
> or (b) that the handling of this message will be removed in any case when
> components/autofill/renderer/autofill_agent goes away? If the former, I'd
rather
> isolate that change into a different CL, as this CL is not intended to produce
> any behavioral changes.
> 
> On 2013/06/05 10:50:02, Ilya Sherman wrote:
> > On 2013/06/04 18:06:36, Evan Stade wrote:
> > > On 2013/06/04 17:30:48, blundell wrote:
> > > > I moved the reception of this message into AutofillManager, which I saw
> > > already
> > > > had a RemoveAutocompleteEntry() method that forwarded to
> > > > AutocompleteHistoryManager. Any pointers on how to test this change
would
> be
> > > > appreciated -- I could not find any tests that failed even when I
> commented
> > > out
> > > > the handling of this message entirely.
> > > 
> > > Ilya would know for sure but my guess is that it's only used in the
> old-style
> > > renderer-side autofill implentation. So you'd have to disable the native
> > > autofill ui (in about:flags) to test this manually. But on the other hand
> it's
> > > probably going away soon.
> > 
> > Yes, Evan is correct -- this IPC message would only ever be generated by the
> > deprecated UI, support for which should be safe to rip out at this point.
>

https://codereview.chromium.org/16286020/diff/9001/components/autofill/browse...
File components/autofill/browser/autocomplete_history_manager.h (right):

https://codereview.chromium.org/16286020/diff/9001/components/autofill/browse...
components/autofill/browser/autocomplete_history_manager.h:59:
FRIEND_TEST_ALL_PREFIXES(AutocompleteHistoryManagerTest, ExternalDelegate);
Using a custom subclass is preferable, as it exposes a narrower section of the
private API to tests.

On 2013/06/11 15:35:47, blundell wrote:
> To disable sending IPC. Previously a custom subclass of
> AutocompleteHistoryManager was used in the test for this purpose.
> 
> On 2013/06/05 10:50:02, Ilya Sherman wrote:
> > Why is this needed?
>

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
File components/autofill/browser/autocomplete_history_manager.cc (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.cc:47: AutofillDriver*
driver)
nit: Looks like this fits on the previous line.

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
File components/autofill/browser/autocomplete_history_manager.h (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.h:90: bool send_IPC_;
nit: If you keep this variable, it should use hacker_case, i.e. "ipc" should be
all lowercase.  But, it doesn't look like the variable is used at all.

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
File components/autofill/browser/autofill_driver.h (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autofill_driver.h:14: // Interface that provides
access to the driver-level context in which autofill
nit: "autofill" -> "Autofill" (feature name)

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autofill_driver.h:16: class AutofillDriver {
On 2013/06/11 15:35:47, blundell wrote:
> I envision this interface being the one that will provide information such as
> whether the BrowserContext is currently off-the-record, etc. I wrestled with
> what to name it -- I think that something like AutofillContext most accurately
> conveys its function, but then it gets confusing between which interfaces are
> implemented in the driver vs. the embedder of the component. Do you have
> opinions?

AutofillDriver sounds fine to me.  If the interface starts to get quite large,
we might want to split off an AutofillContext interface from it.

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autofill_driver.h:21: virtual ~AutofillDriver() {}
nit: Destructor belongs above other methods.

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
File components/autofill/browser/autofill_manager.h (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:87: content::WebContents*
contents,
nit: Does the WebContents still need to be passed in, or can we simply pass in
the driver?

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:348: AutofillDriver* driver_;
nit: Please document this variable, including its lifetime expectation (i.e.
that it is expected to outlive this class).

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
File components/autofill/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:841: */
Why is this test commented out?

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
File components/autofill/browser/test_autofill_driver.cc (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/test_autofill_driver.cc:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
ultra nit: Please omit the "(c)"

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/test_autofill_driver.cc:11: }
nit: Please move the closing curly brace to the previous line, to match the
destructor.

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
File components/autofill/browser/test_autofill_driver.h (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/test_autofill_driver.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
ultra nit: Please omit the "(c)"

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/test_autofill_driver.h:22:
TestAutofillDriver(content::WebContents* web_contents);
nit: Please mark this as explicit.

https://codereview.chromium.org/16286020/diff/44001/components/autofill/conte...
File components/autofill/content/browser/autofill_driver_impl.h (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/conte...
components/autofill/content/browser/autofill_driver_impl.h:42: bool
enable_native_UI);
nit: Please name this variable using hacker_case, i.e. enable_native_ui

https://codereview.chromium.org/16286020/diff/44001/components/autofill/conte...
components/autofill/content/browser/autofill_driver_impl.h:64:
AutofillExternalDelegate* autofill_external_delegate_;
Please document these, including comments about the lifetime and ownership of
the memory.

[blundell, 2013-06-12 16:29:36]

Thanks for the thorough review. Your comment re: commenting the fact that
AutofillDriver needing to outlive AutofillManager made me realize that in the
current implementation it wasn't guaranteed to (since they're both WCUD's, which
are destroyed in an arbitrary order). As a result, I went ahead and made the
change of having AutofillManager be owned by AutofillDriverImpl in this CL.

I left AutofillExternalDelegate alone for this CL because changing
AutofillExternalDelegate from being owned by the client of autofill to being
owned by the AutofillDriverImpl will take a little finesse, and there was
already no ordering requirement there as AutofillManager and
AutofillExternalDelegate were both WCUD's.

https://codereview.chromium.org/16286020/diff/9001/components/autofill/browse...
File components/autofill/browser/autocomplete_history_manager.h (right):

https://codereview.chromium.org/16286020/diff/9001/components/autofill/browse...
components/autofill/browser/autocomplete_history_manager.h:59:
FRIEND_TEST_ALL_PREFIXES(AutocompleteHistoryManagerTest, ExternalDelegate);
On 2013/06/12 00:07:46, Ilya Sherman wrote:
> Using a custom subclass is preferable, as it exposes a narrower section of the
> private API to tests.
> 
> On 2013/06/11 15:35:47, blundell wrote:
> > To disable sending IPC. Previously a custom subclass of
> > AutocompleteHistoryManager was used in the test for this purpose.
> > 
> > On 2013/06/05 10:50:02, Ilya Sherman wrote:
> > > Why is this needed?
> > 
> 

Done.

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
File components/autofill/browser/autocomplete_history_manager.cc (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.cc:47: AutofillDriver*
driver)
On 2013/06/12 00:07:46, Ilya Sherman wrote:
> nit: Looks like this fits on the previous line.

Done.

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.cc:203: if
(web_contents) {
This is where send_ipc_ should have been used (and is now being used).

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
File components/autofill/browser/autocomplete_history_manager.h (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.h:90: bool send_IPC_;
Done. It was a bug that the variable wasn't used; thanks for pointing that out.

On 2013/06/12 00:07:46, Ilya Sherman wrote:
> nit: If you keep this variable, it should use hacker_case, i.e. "ipc" should
be
> all lowercase.  But, it doesn't look like the variable is used at all.

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
File components/autofill/browser/autofill_driver.h (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autofill_driver.h:14: // Interface that provides
access to the driver-level context in which autofill
On 2013/06/12 00:07:46, Ilya Sherman wrote:
> nit: "autofill" -> "Autofill" (feature name)

Done.

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autofill_driver.h:16: class AutofillDriver {
SGTM.

On 2013/06/12 00:07:46, Ilya Sherman wrote:
> On 2013/06/11 15:35:47, blundell wrote:
> > I envision this interface being the one that will provide information such
as
> > whether the BrowserContext is currently off-the-record, etc. I wrestled with
> > what to name it -- I think that something like AutofillContext most
accurately
> > conveys its function, but then it gets confusing between which interfaces
are
> > implemented in the driver vs. the embedder of the component. Do you have
> > opinions?
> 
> AutofillDriver sounds fine to me.  If the interface starts to get quite large,
> we might want to split off an AutofillContext interface from it.

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autofill_driver.h:21: virtual ~AutofillDriver() {}
On 2013/06/12 00:07:46, Ilya Sherman wrote:
> nit: Destructor belongs above other methods.

Done.

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
File components/autofill/browser/autofill_manager.h (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:87: content::WebContents*
contents,
Eliminated this method as part of making the AutofillManager owned by the
AutofillDriver instead of by the WebContents.

On 2013/06/12 00:07:46, Ilya Sherman wrote:
> nit: Does the WebContents still need to be passed in, or can we simply pass in
> the driver?

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:348: AutofillDriver* driver_;
Done. Additionally, your comment made me realize that leaving the
AutofillManager as a WebContentsUserData was problematic, as WebContentsUserData
objects are destroyed in an arbitrary order. I've thus moved the ownership of
the AutofillManager to the AutofillDriverImpl in this CL instead of doing it in
a follow-up CL as I had originally planned.

On 2013/06/12 00:07:46, Ilya Sherman wrote:
> nit: Please document this variable, including its lifetime expectation (i.e.
> that it is expected to outlive this class).

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
File components/autofill/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:841: */
Cruft from debugging! Thanks.

On 2013/06/12 00:07:46, Ilya Sherman wrote:
> Why is this test commented out?

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
File components/autofill/browser/test_autofill_driver.cc (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/test_autofill_driver.cc:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2013/06/12 00:07:46, Ilya Sherman wrote:
> ultra nit: Please omit the "(c)"

Done.

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/test_autofill_driver.cc:11: }
On 2013/06/12 00:07:46, Ilya Sherman wrote:
> nit: Please move the closing curly brace to the previous line, to match the
> destructor.

Done.

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
File components/autofill/browser/test_autofill_driver.h (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/test_autofill_driver.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2013/06/12 00:07:46, Ilya Sherman wrote:
> ultra nit: Please omit the "(c)"

Done.

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/test_autofill_driver.h:22:
TestAutofillDriver(content::WebContents* web_contents);
On 2013/06/12 00:07:46, Ilya Sherman wrote:
> nit: Please mark this as explicit.

Done.

https://codereview.chromium.org/16286020/diff/44001/components/autofill/conte...
File components/autofill/content/browser/autofill_driver_impl.h (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/conte...
components/autofill/content/browser/autofill_driver_impl.h:42: bool
enable_native_UI);
On 2013/06/12 00:07:46, Ilya Sherman wrote:
> nit: Please name this variable using hacker_case, i.e. enable_native_ui

Done.

https://codereview.chromium.org/16286020/diff/44001/components/autofill/conte...
components/autofill/content/browser/autofill_driver_impl.h:64:
AutofillExternalDelegate* autofill_external_delegate_;
Done. Removed the autofill_external_delegate_ instance as the driver doesn't
need it at this point. I will handle moving ownership of
AutofillExternalDelegate in a follow-up CL.

On 2013/06/12 00:07:46, Ilya Sherman wrote:
> Please document these, including comments about the lifetime and ownership of
> the memory.

[blundell, 2013-06-13 20:19:15]

Ping :).

[Ilya Sherman, 2013-06-14 02:44:37]

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:206:
delegate->GetAutocheckoutWhitelistManager();
Where does this happen now?

https://codereview.chromium.org/16286020/diff/57001/components/autofill/brows...
File components/autofill/browser/autocomplete_history_manager.h (right):

https://codereview.chromium.org/16286020/diff/57001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.h:65: bool send_ipc_;
nit: Please keep this variable private, and add a protected setter for it.  Per
the style guide, all member variables should be private.

https://codereview.chromium.org/16286020/diff/57001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.h:72: AutofillDriver*
driver_;
nit: Please add documentation for this variable, even though the class isn't
totally consistent about doing that for the existing variables.

https://codereview.chromium.org/16286020/diff/57001/components/autofill/brows...
File components/autofill/browser/autocomplete_history_manager_unittest.cc
(right):

https://codereview.chromium.org/16286020/diff/57001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager_unittest.cc:236: }
nit: You can write this as "using AutocompleteHistoryManager::SendSuggestions;"

https://codereview.chromium.org/16286020/diff/57001/components/autofill/brows...
File components/autofill/browser/autofill_driver.h (right):

https://codereview.chromium.org/16286020/diff/57001/components/autofill/brows...
components/autofill/browser/autofill_driver.h:18: virtual ~AutofillDriver() {}
optional nit: IMO, it would be nice to leave a blank line after this one.

https://codereview.chromium.org/16286020/diff/57001/components/autofill/conte...
File components/autofill/content/browser/autofill_driver_impl.h (right):

https://codereview.chromium.org/16286020/diff/57001/components/autofill/conte...
components/autofill/content/browser/autofill_driver_impl.h:27: class
AutofillManager;
nit: This is currently both forward-declared and #included.  Please pick just
one :)

https://codereview.chromium.org/16286020/diff/57001/components/autofill/conte...
components/autofill/content/browser/autofill_driver_impl.h:47: AutofillManager*
autofill_manager() { return autofill_manager_.get(); }
nit: Please leave a blank line above this one, as it's not part of implementing
the AutofillDriver interface.

https://codereview.chromium.org/16286020/diff/57001/components/autofill/conte...
components/autofill/content/browser/autofill_driver_impl.h:66:
scoped_ptr<AutofillManager> autofill_manager_;
Any reason not to allocate this as a direct member, rather than as an owned
pointer?

[blundell, 2013-06-14 09:54:08]

Thanks! Had to rebase -- the relevant diff is between PS 7 and PS 8.

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/16286020/diff/44001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:206:
delegate->GetAutocheckoutWhitelistManager();
Great catch, thanks. Moved to
AutofillDriverImpl::CreateForWebContentsAndDelegate().

On 2013/06/14 02:44:37, Ilya Sherman wrote:
> Where does this happen now?

https://codereview.chromium.org/16286020/diff/57001/components/autofill/brows...
File components/autofill/browser/autocomplete_history_manager.h (right):

https://codereview.chromium.org/16286020/diff/57001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.h:65: bool send_ipc_;
On 2013/06/14 02:44:37, Ilya Sherman wrote:
> nit: Please keep this variable private, and add a protected setter for it. 
Per
> the style guide, all member variables should be private.

Done.

https://codereview.chromium.org/16286020/diff/57001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.h:72: AutofillDriver*
driver_;
On 2013/06/14 02:44:37, Ilya Sherman wrote:
> nit: Please add documentation for this variable, even though the class isn't
> totally consistent about doing that for the existing variables.

Done.

https://codereview.chromium.org/16286020/diff/57001/components/autofill/brows...
File components/autofill/browser/autocomplete_history_manager_unittest.cc
(right):

https://codereview.chromium.org/16286020/diff/57001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager_unittest.cc:236: }
This doesn't work as SendSuggestions() is a member function in
AutocompleteHistoryManager.

On 2013/06/14 02:44:37, Ilya Sherman wrote:
> nit: You can write this as "using
AutocompleteHistoryManager::SendSuggestions;"

https://codereview.chromium.org/16286020/diff/57001/components/autofill/brows...
File components/autofill/browser/autofill_driver.h (right):

https://codereview.chromium.org/16286020/diff/57001/components/autofill/brows...
components/autofill/browser/autofill_driver.h:18: virtual ~AutofillDriver() {}
On 2013/06/14 02:44:37, Ilya Sherman wrote:
> optional nit: IMO, it would be nice to leave a blank line after this one.

Done.

https://codereview.chromium.org/16286020/diff/57001/components/autofill/conte...
File components/autofill/content/browser/autofill_driver_impl.h (right):

https://codereview.chromium.org/16286020/diff/57001/components/autofill/conte...
components/autofill/content/browser/autofill_driver_impl.h:27: class
AutofillManager;
On 2013/06/14 02:44:37, Ilya Sherman wrote:
> nit: This is currently both forward-declared and #included.  Please pick just
> one :)

Done.

https://codereview.chromium.org/16286020/diff/57001/components/autofill/conte...
components/autofill/content/browser/autofill_driver_impl.h:47: AutofillManager*
autofill_manager() { return autofill_manager_.get(); }
On 2013/06/14 02:44:37, Ilya Sherman wrote:
> nit: Please leave a blank line above this one, as it's not part of
implementing
> the AutofillDriver interface.

Done.

https://codereview.chromium.org/16286020/diff/57001/components/autofill/conte...
components/autofill/content/browser/autofill_driver_impl.h:66:
scoped_ptr<AutofillManager> autofill_manager_;
Nope! Done.

On 2013/06/14 02:44:37, Ilya Sherman wrote:
> Any reason not to allocate this as a direct member, rather than as an owned
> pointer?

[Ilya Sherman, 2013-06-14 23:22:23]

Thanks!  LGTM % nits.

https://codereview.chromium.org/16286020/diff/57001/components/autofill/brows...
File components/autofill/browser/autocomplete_history_manager_unittest.cc
(right):

https://codereview.chromium.org/16286020/diff/57001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager_unittest.cc:236: }
I'm pretty sure this should work.  See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
for an example.

On 2013/06/14 09:54:08, blundell wrote:
> This doesn't work as SendSuggestions() is a member function in
> AutocompleteHistoryManager.
> 
> On 2013/06/14 02:44:37, Ilya Sherman wrote:
> > nit: You can write this as "using
> AutocompleteHistoryManager::SendSuggestions;"
>

https://codereview.chromium.org/16286020/diff/93001/components/autofill/brows...
File components/autofill/browser/autocomplete_history_manager.h (right):

https://codereview.chromium.org/16286020/diff/93001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.h:92: // Sets whether
IPC is sent.
nit: Omit "Sets", so just "Whether IPC is sent."

[blundell, 2013-06-15 12:52:32]

Thanks!

Evan, I'm going to land this to enable progress on follow-up work, but please
let me know if you have any further concerns and I'll address them in a
follow-up CL.

https://codereview.chromium.org/16286020/diff/57001/components/autofill/brows...
File components/autofill/browser/autocomplete_history_manager_unittest.cc
(right):

https://codereview.chromium.org/16286020/diff/57001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager_unittest.cc:236: }
Thanks for the pointer - nifty! I had misunderstood your suggestion. Done.

On 2013/06/14 23:22:23, Ilya Sherman wrote:
> I'm pretty sure this should work.  See
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> for an example.
> 
> On 2013/06/14 09:54:08, blundell wrote:
> > This doesn't work as SendSuggestions() is a member function in
> > AutocompleteHistoryManager.
> > 
> > On 2013/06/14 02:44:37, Ilya Sherman wrote:
> > > nit: You can write this as "using
> > AutocompleteHistoryManager::SendSuggestions;"
> > 
>

https://codereview.chromium.org/16286020/diff/93001/components/autofill/brows...
File components/autofill/browser/autocomplete_history_manager.h (right):

https://codereview.chromium.org/16286020/diff/93001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.h:92: // Sets whether
IPC is sent.
On 2013/06/14 23:22:24, Ilya Sherman wrote:
> nit: Omit "Sets", so just "Whether IPC is sent."

Done.

[commit-bot: I haz the power, 2013-06-15 12:52:58]

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

[commit-bot: I haz the power, 2013-06-15 13:01:44]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-06-15 16:27:12]

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

[commit-bot: I haz the power, 2013-06-15 16:38:50]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-06-15 21:23:41]

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

[blundell, 2013-06-15 21:26:25]

benm: android_webview
yfriedman: chrome/browser/android
avi: chrome/browser/ui/browser_tab_contents.cc

[commit-bot: I haz the power, 2013-06-16 03:06:43]

Change committed as 206623

[Avi (use Gerrit), 2013-06-17 23:48:36]

browser tab contents LGTM

I assume a similar change doesn't need to be made for the android code.

[blundell, 2013-06-18 08:09:03]

On 2013/06/17 23:48:36, Avi wrote:
> browser tab contents LGTM
> 
> I assume a similar change doesn't need to be made for the android code.

I made the change both for android_webview/ and chrome/browser/tab_android; is
there another usage on Android that I'm missing?

[Avi (use Gerrit), 2013-06-18 14:24:48]

On 2013/06/18 08:09:03, blundell wrote:
> On 2013/06/17 23:48:36, Avi wrote:
> > browser tab contents LGTM
> > 
> > I assume a similar change doesn't need to be made for the android code.
> 
> I made the change both for android_webview/ and chrome/browser/tab_android; is
> there another usage on Android that I'm missing?

I missed tab_android; don't mind me.

[Yaron, 2013-06-18 15:58:18]

tab_android lgtm