Enable Autocomplete feature for chromium webview

Enable Autocomplete feature for chromium webview

Webview allows enabling/disabling autocomplete per instance which is different than chromium browser which does it per profile. Address this using external_delegate.

BUG=b/6335434

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

[sgurun-gerrit only, 2013-05-17 18:30:56]

Hey Ben,

Can you take a look at this CL (which is to enable autocomplete feature) and let
me know what you think? I am especially interested in what you think about the
changes in 
autofill_manager: see how autofill_manager assumes a personal_data_manager is
available at the construction time, which we won't provide. (there are also
other unnecessary pieces like metric_logger and autocheckoutmanager). would it
make sense to refactor this code further or would this be ok?

[benm (inactive), 2013-05-20 11:42:29]

Hmm, this seems a little bit fragile, and I think we'll get broken often here as
the only implementation not using a personal_data_manager.

I wonder whether the better thing to do is to separate more clearly the
difference between autocomplete and autofill. WebView would only ever use
AutoComplete and never trigger AutoFill code. So
AutoFillManager::IsAutoFillEnabled() would return false for Aw, of course we'd
need to find the places currently checking IsAutoFillEnabled() for AutoComplete
functionality and create an IsAutoCompleteEnabled function to check there I
guess. 

Does that sound reasonable?

[sgurun-gerrit only, 2013-05-20 21:50:44]

Let me give this a try. Sounds like a reasonable path. This means we will add a
new pref, I think.

[sgurun-gerrit only, 2013-05-22 20:06:23]

Hey Ben, 

I made a few more changes especially regarding how we enable it. The main
problem I addressed is the logic that enables autofill per webcontents (to be
compatible with webview classic). I used the external_delegate for this (Joth's
suggestion) which actually I think fit pretty good in this case. 

I had some concerns about personal_data_manager being null, but it can already
be null in OTR case, so I think using the external delegate address this case
well too.

PTAL, thanks,

[sgurun-gerrit only, 2013-05-23 00:09:30]

after thinking about it a little more, this solution seems to have a fundamental
bug such that enabling autocomplete will for webview will still fail because the
personal_data_ is still null. I thought about adding a new field to
external_delegate to control autocomplete independent of OTR functionality but
then this makes the external delegate even more complicated and then there are
corner cases (for example in OnQueryFormFieldAutofill) where the if's that
control these cases become quite fragile. 

I think the solution is making autofillmanager more modular.

[sgurun-gerrit only, 2013-05-23 01:36:32]

Hey Ben, I made the personal_data_ optional and I think addressed the issue in
previous patch. PTAL, thanks

[benm (inactive), 2013-05-23 12:02:21]

This approach seems viable, but I don't feel confident enough in Autofill under
the hood to say it's completely sound - please ask the autofill folks for their
opinion. The fact that personal_data_manager_ is already NULL in the OTR case
relieves my original concerns somewhat.

Thanks!

PS: I will be OOO next week (back June 3rd) so you might like to ask another
android_webview reviewer to take a look so that you aren't held up.

https://codereview.chromium.org/15097004/diff/31001/android_webview/browser/a...
File android_webview/browser/aw_autofill_external_delegate.cc (right):

https://codereview.chromium.org/15097004/diff/31001/android_webview/browser/a...
android_webview/browser/aw_autofill_external_delegate.cc:18: if
(FromWebContents(web_contents))
DCHECK?

https://codereview.chromium.org/15097004/diff/31001/android_webview/native/aw...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/15097004/diff/31001/android_webview/native/aw...
android_webview/native/aw_contents.cc:188:
AwAutofillExternalDelegate::CreateForWebContentsAndManager(
nit: move this consctruction into the setter below?

https://codereview.chromium.org/15097004/diff/31001/android_webview/renderer/...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/15097004/diff/31001/android_webview/renderer/...
android_webview/renderer/aw_content_renderer_client.cc:53: new
autofill::AutofillAgent(render_view, password_autofill_agent);
leaving these dangling looks a bit strange. save to members to clean up in dtor?

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

https://codereview.chromium.org/15097004/diff/31001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.cc:130: if
(external_delegate_ && external_delegate_->ShouldIgnoreFormData())
In what situation is external_delegate_ not installed? Seems like this could be
a behavior change for clients that do not set it.

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

https://codereview.chromium.org/15097004/diff/31001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:403: 
nit: remove?

https://codereview.chromium.org/15097004/diff/31001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:796: 
nit: remove

https://codereview.chromium.org/15097004/diff/31001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:879: 
nit: remove

[sgurun-gerrit only, 2013-05-23 17:10:00]

@isherman PTAL, thank

https://codereview.chromium.org/15097004/diff/31001/android_webview/browser/a...
File android_webview/browser/aw_autofill_external_delegate.cc (right):

https://codereview.chromium.org/15097004/diff/31001/android_webview/browser/a...
android_webview/browser/aw_autofill_external_delegate.cc:18: if
(FromWebContents(web_contents))
On 2013/05/23 12:02:21, benm wrote:
> DCHECK?

Done.

https://codereview.chromium.org/15097004/diff/31001/android_webview/native/aw...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/15097004/diff/31001/android_webview/native/aw...
android_webview/native/aw_contents.cc:188:
AwAutofillExternalDelegate::CreateForWebContentsAndManager(
On 2013/05/23 12:02:21, benm wrote:
> nit: move this consctruction into the setter below?

not sure if I understand right (I did not have a morning coffee yet)
autofill_manager->SetExternalDelegate(
AwAutofillExternalDelegate::CreateForWebContentsAndManager(
      web_contents,
      autofill_manager));
but createforwebcontentsbla is a void function.

https://codereview.chromium.org/15097004/diff/31001/android_webview/renderer/...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/15097004/diff/31001/android_webview/renderer/...
android_webview/renderer/aw_content_renderer_client.cc:53: new
autofill::AutofillAgent(render_view, password_autofill_agent);
I also find it weird and unusual. However an autofillagent is a
renderviewobserver and goes away when the renderview goes away. (this seems like
the pattern, browser also does the same)


On 2013/05/23 12:02:21, benm wrote:
> leaving these dangling looks a bit strange. save to members to clean up in
dtor?

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

https://codereview.chromium.org/15097004/diff/31001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.cc:130: if
(external_delegate_ && external_delegate_->ShouldIgnoreFormData())
This is a fair point. I did a code search and found that it is always installed.
I would love to hear what autofill folks have to say about it.

On 2013/05/23 12:02:21, benm wrote:
> In what situation is external_delegate_ not installed? Seems like this could
be
> a behavior change for clients that do not set it.

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

https://codereview.chromium.org/15097004/diff/31001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:403: 
On 2013/05/23 12:02:21, benm wrote:
> nit: remove?

Done.

https://codereview.chromium.org/15097004/diff/31001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:796: 
On 2013/05/23 12:02:21, benm wrote:
> nit: remove

Done.

https://codereview.chromium.org/15097004/diff/31001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:879: 
On 2013/05/23 12:02:21, benm wrote:
> nit: remove

Done.

[sgurun-gerrit only, 2013-05-30 14:48:50]

Ping, @isherman can you take a look at the components/ part?

[sgurun-gerrit only, 2013-06-03 15:59:04]

On 2013/05/30 14:48:50, sgurun wrote:
> Ping, @isherman can you take a look at the components/ part?

Ping. @isherman, if you prefer not reviewing this CL, can you suggest who can?
thanks,

[Ilya Sherman, 2013-06-03 19:15:27]

On 2013/06/03 15:59:04, sgurun wrote:
> On 2013/05/30 14:48:50, sgurun wrote:
> > Ping, @isherman can you take a look at the components/ part?
> 
> Ping. @isherman, if you prefer not reviewing this CL, can you suggest who can?
> thanks,

Apologies for the delay -- I was out on vacation and then build sheriff, so I'm
very behind on code reviews.  I'm aiming to get to this one be EOD today.

[sgurun-gerrit only, 2013-06-03 21:33:45]

On 2013/06/03 19:15:27, Ilya Sherman wrote:
> On 2013/06/03 15:59:04, sgurun wrote:
> > On 2013/05/30 14:48:50, sgurun wrote:
> > > Ping, @isherman can you take a look at the components/ part?
> > 
> > Ping. @isherman, if you prefer not reviewing this CL, can you suggest who
can?
> > thanks,
> 
> Apologies for the delay -- I was out on vacation and then build sheriff, so
I'm
> very behind on code reviews.  I'm aiming to get to this one be EOD today.

did not know you were on vacation. Thanks for the update!

[Ilya Sherman, 2013-06-03 23:22:18]

At a high level, I'm worried that checking for non-NULLness of the
personal_data_manager_ everywhere seems really fragile, especially since nothing
will visibly regress (AFAICT) if these checks are later refactored away.  It
would be best to add test coverage for all of these code paths.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
File components/autofill/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_external_delegate.h:109: virtual bool
ShouldIgnoreFormData();
The ExternalDelegate class is for things related to the popup UI.  Please add
this to a different location, e.g. the AutofillManagerDelegate class.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
File components/autofill/browser/autofill_manager.cc (left):

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:428: 
nit: Spurious diff.

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

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:326: if (external_delegate_ &&
external_delegate_->ShouldIgnoreFormData())
Should this check just be part of the IsAutofillEnabled() method?

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:488: if (personal_data_) {
Please move this check into the wrapped method.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:496: }
nit: Please add a newline after this one.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:505: int query_id,
This parameter doesn't seem to be used.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:712: 
nit: Spurious newline.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:714: return;
Why is this code reachable?

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:920: const CreditCard*
imported_credit_card;
nit: Please keep this just above line 925.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:1026: return false;
Why is this code reachable?

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:1046: return false;
Why is this code reachable?

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:1197: 
nit: Spurious newline.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:1199: return;
nit: Please add a newline after this line.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:1199: return;
Why is this code reachable?

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:1223: 
nit: Spurious newline.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:1225: return;
Why is this code reachable?

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

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:239: bool display_warning);
nit: Add a newline after this line.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:240: void
OnQueryFormFieldAutofillPersonalData(
Please name this method something more like "GetAutofillSuggestions()", and add
documentation.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:363: // May be NULL for OTR and
android webview.
nit: "Android WebView".  Ditto below.

[sgurun-gerrit only, 2013-06-13 01:13:31]

Hi Ilya,

Many thanks for your review. PTAL,

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
File components/autofill/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_external_delegate.h:109: virtual bool
ShouldIgnoreFormData();
On 2013/06/03 23:22:19, Ilya Sherman wrote:
> The ExternalDelegate class is for things related to the popup UI.  Please add
> this to a different location, e.g. the AutofillManagerDelegate class.

Done.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
File components/autofill/browser/autofill_manager.cc (left):

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:428: 
On 2013/06/03 23:22:19, Ilya Sherman wrote:
> nit: Spurious diff.

Done.

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

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:326: if (external_delegate_ &&
external_delegate_->ShouldIgnoreFormData())
Agreed. much less error prone.

On 2013/06/03 23:22:19, Ilya Sherman wrote:
> Should this check just be part of the IsAutofillEnabled() method?

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:488: if (personal_data_) {
Done. Actually this seems redundant since wrapped method checks it via getHost.

On 2013/06/03 23:22:19, Ilya Sherman wrote:
> Please move this check into the wrapped method.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:496: }
On 2013/06/03 23:22:19, Ilya Sherman wrote:
> nit: Please add a newline after this one.

Done.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:505: int query_id,
On 2013/06/03 23:22:19, Ilya Sherman wrote:
> This parameter doesn't seem to be used.

Done.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:712: 
On 2013/06/03 23:22:19, Ilya Sherman wrote:
> nit: Spurious newline.

Done.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:714: return;
On 2013/06/03 23:22:19, Ilya Sherman wrote:
> Why is this code reachable?

should not be reachable. removing.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:920: const CreditCard*
imported_credit_card;
On 2013/06/03 23:22:19, Ilya Sherman wrote:
> nit: Please keep this just above line 925.

I think this code should also be unreachable?

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:1026: return false;
it is eventually reachable via OnQueryFormFieldAutofill()
On 2013/06/03 23:22:19, Ilya Sherman wrote:
> Why is this code reachable?

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:1046: return false;
On 2013/06/03 23:22:19, Ilya Sherman wrote:
> Why is this code reachable?

Done.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:1197: 
On 2013/06/03 23:22:19, Ilya Sherman wrote:
> nit: Spurious newline.

Done.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:1199: return;
On 2013/06/03 23:22:19, Ilya Sherman wrote:
> nit: Please add a newline after this line.

Done.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:1199: return;
On 2013/06/03 23:22:19, Ilya Sherman wrote:
> nit: Please add a newline after this line.

Done.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:1223: 
On 2013/06/03 23:22:19, Ilya Sherman wrote:
> nit: Spurious newline.

Done.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:1225: return;
removed.

On 2013/06/03 23:22:19, Ilya Sherman wrote:
> Why is this code reachable?

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

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:239: bool display_warning);
On 2013/06/03 23:22:19, Ilya Sherman wrote:
> nit: Add a newline after this line.

Done.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:240: void
OnQueryFormFieldAutofillPersonalData(
On 2013/06/03 23:22:19, Ilya Sherman wrote:
> Please name this method something more like "GetAutofillSuggestions()", and
add
> documentation.

Done.

https://codereview.chromium.org/15097004/diff/38001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:363: // May be NULL for OTR and
android webview.
On 2013/06/03 23:22:19, Ilya Sherman wrote:
> nit: "Android WebView".  Ditto below.

Done.

[sgurun-gerrit only, 2013-06-14 23:25:22]

ping! (I know too much is going on in autofill component but can you give this a
look too).

[Ilya Sherman, 2013-06-15 00:28:08]

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
File components/autofill/browser/autocomplete_history_manager.cc (left):

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.cc:131: return;
Please either keep these if-stmts, or replace them with DCHECKs.

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.cc:131: return;
I don't follow why this check was removed -- there's nothing in the
AutofillManager code that seems to guarantee non-OTR mode prior to calling this
method.

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
File components/autofill/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_external_delegate.cc:10: #include
"content/public/browser/browser_context.h"
nit: No longer needed

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
File components/autofill/browser/autofill_manager.cc (left):

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:339: return false;
I think this check is still needed.

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

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:336:
autocomplete_history_manager_.OnFormSubmitted(form);
Does this re-ordering still make sense given the updated definition of
IsAutofillEnabled()?

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:503: &unique_ids);
nit: This all fits on a single line.

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:503: &unique_ids);
What's the purpose of decomposing out this method?

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:890: &&
!manager_delegate_->ShouldIgnoreFormData() ;
nit: The "&&" should go on the previous line.

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:912: 
nit: Spurious diff.

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:1033: 
nit: Spurious diff.

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

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:160: // Returns the value of the
AutofillEnabled pref.
Please update this comment.

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:364: // May be NULL for OTR and
Android Webview.
I don't think it's true that personal_data_ is null in Incognito mode, as we
still offer Autofill suggestions in Incognito mode.

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:376: // Handles autocheckout
flows. NULL for Android Webview.
What does it mean for this non-pointer value to be NULL?

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
File components/autofill/browser/autofill_manager_delegate.h (right):

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager_delegate.h:132: virtual bool
ShouldIgnoreFormData() = 0;
IMO this comment and method name are insufficiently clear.  What does it mean to
ignore form data?  I *think* this method means something like "Should fancy
(non-autocomplete) Autofill be enabled?", but I'm not 100% sure.

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

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:2709: // works as
expected.
I don't see any code to trigger a form submission in this test...

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:2726: // suggestions
and the warning; these will be culled by the renderer.
What is the warning referred to in this comment?

[sgurun-gerrit only, 2013-06-17 21:20:32]

@ilya, 

I think it is becoming more obvious that autocomplete should be enabled
independent of autofill. So I changed the meaning of ShouldIgnoreFormData which
I added in the previous patch, and named it as IsAutocompleteEnabled().
I am now passing manager_delegate to Autocomplete_history_manager so I can use
this check. Autofill preference is disabled, so I removed null checks that I
added too.

PTAL, Thanks.

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
File components/autofill/browser/autocomplete_history_manager.cc (left):

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.cc:131: return;
On 2013/06/15 00:28:09, Ilya Sherman wrote:
> Please either keep these if-stmts, or replace them with DCHECKs.

Done.

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.cc:131: return;
On 2013/06/15 00:28:09, Ilya Sherman wrote:
> I don't follow why this check was removed -- there's nothing in the
> AutofillManager code that seems to guarantee non-OTR mode prior to calling
this
> method.

Done.

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
File components/autofill/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_external_delegate.cc:10: #include
"content/public/browser/browser_context.h"
On 2013/06/15 00:28:09, Ilya Sherman wrote:
> nit: No longer needed

Done.

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
File components/autofill/browser/autofill_manager.cc (left):

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:339: return false;
On 2013/06/15 00:28:09, Ilya Sherman wrote:
> I think this check is still needed.

Done.

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

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:336:
autocomplete_history_manager_.OnFormSubmitted(form);
There seems multiple issues with enabling/disabling the logic as you pointed
out. I have removed ShouldIgnoreFormDAta and introduced IsAutocompleteEnabled()
which I think would be cleaner and solve the problem in a different way.

On 2013/06/15 00:28:09, Ilya Sherman wrote:
> Does this re-ordering still make sense given the updated definition of
> IsAutofillEnabled()?

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:503: &unique_ids);
On 2013/06/15 00:28:09, Ilya Sherman wrote:
> What's the purpose of decomposing out this method?

Done.

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:503: &unique_ids);
On 2013/06/15 00:28:09, Ilya Sherman wrote:
> nit: This all fits on a single line.

Done.

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:890: &&
!manager_delegate_->ShouldIgnoreFormData() ;
On 2013/06/15 00:28:09, Ilya Sherman wrote:
> nit: The "&&" should go on the previous line.

Done.

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:912: 
On 2013/06/15 00:28:09, Ilya Sherman wrote:
> nit: Spurious diff.

Done.

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:1033: 
On 2013/06/15 00:28:09, Ilya Sherman wrote:
> nit: Spurious diff.

Done.

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

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:160: // Returns the value of the
AutofillEnabled pref.
On 2013/06/15 00:28:09, Ilya Sherman wrote:
> Please update this comment.

Done.

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:364: // May be NULL for OTR and
Android Webview.
himm, perhaps the comment is not true anymore then. I will verify.

On 2013/06/15 00:28:09, Ilya Sherman wrote:
> I don't think it's true that personal_data_ is null in Incognito mode, as we
> still offer Autofill suggestions in Incognito mode.

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:376: // Handles autocheckout
flows. NULL for Android Webview.
yeah. this is a runaway leftover comment. I was thinking of making this a
pointer since it does not apply to android webview, but then decided not to mess
with it.
On 2013/06/15 00:28:09, Ilya Sherman wrote:
> What does it mean for this non-pointer value to be NULL?

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
File components/autofill/browser/autofill_manager_delegate.h (right):

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager_delegate.h:132: virtual bool
ShouldIgnoreFormData() = 0;
Himm, I guess a cleaner approach would be introducing a IsAutoCompleteEnabled()
method to control autocomplete independent of autofill.

On 2013/06/15 00:28:09, Ilya Sherman wrote:
> IMO this comment and method name are insufficiently clear.  What does it mean
to
> ignore form data?  I *think* this method means something like "Should fancy
> (non-autocomplete) Autofill be enabled?", but I'm not 100% sure.

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

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:2709: // works as
expected.
I actually looked into it. I could not find any sane way of testing if
autocomplete_history_manager's OnFormSubmitted is called, which would nicely
complement this test. Any suggestions?


On 2013/06/15 00:28:09, Ilya Sherman wrote:
> I don't see any code to trigger a form submission in this test...

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:2726: // suggestions
and the warning; these will be culled by the renderer.
inherited from copy/paste. does not make sense, removing.

On 2013/06/15 00:28:09, Ilya Sherman wrote:
> What is the warning referred to in this comment?

[Ilya Sherman, 2013-06-18 05:56:24]

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

https://codereview.chromium.org/15097004/diff/56001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:2709: // works as
expected.
Presumably you could install a test AutocompleteHistoryManager that has the
method mocked out -- have you tried that and found that it doesn't work?

On 2013/06/17 21:20:32, sgurun wrote:
> I actually looked into it. I could not find any sane way of testing if
> autocomplete_history_manager's OnFormSubmitted is called, which would nicely
> complement this test. Any suggestions?
> 
> 
> On 2013/06/15 00:28:09, Ilya Sherman wrote:
> > I don't see any code to trigger a form submission in this test...
>

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
File components/autofill/browser/autocomplete_history_manager.cc (left):

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.cc:128: return;
Why did you remove this check?

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

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.h:33:
autofill::AutofillManagerDelegate* const delegate);
nit: It's quite contrary to existing style that I've seen to mark pointer values
(note: not pointed-to values) as const for params.

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.h:76: BooleanPrefMember
autofill_enabled_;
Can this be removed?

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.h:91:
autofill::AutofillManagerDelegate* const manager_delegate_;
(this const is fine and dandy, though :)

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

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:355: // May be NULL for OTR and
Android Webview.
nit: Please revert all the changes in this file besides the typo fix.

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
File components/autofill/browser/autofill_manager_delegate.h (right):

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
components/autofill/browser/autofill_manager_delegate.h:131: // Whether the
autocomplete feature of the autofill should be disabled.
nit: "the autofill" -> "Autofill" or "the Autofill feature"

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
components/autofill/browser/autofill_manager_delegate.h:131: // Whether the
autocomplete feature of the autofill should be disabled.
nit: "should be disabled" -> "should be enabled" to match the method name.

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
components/autofill/browser/autofill_manager_delegate.h:132: virtual bool
IsAutoCompleteEnabled() = 0;
nit: "AutoComplete" -> "Autocomplete", to match the class name
"AutocompleteHistoryManager"

[sgurun-gerrit only, 2013-06-19 00:16:16]

Ilya, Thanks, PTAL.

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
File components/autofill/browser/autocomplete_history_manager.cc (left):

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.cc:128: return;
weird. I have 
 if (!manager_delegate_->IsAutocompleteEnabled())
    return;
in my local file. I guess I forgot uploading it.
good catch.

On 2013/06/18 05:56:25, Ilya Sherman wrote:
> Why did you remove this check?

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

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.h:33:
autofill::AutofillManagerDelegate* const delegate);
On 2013/06/18 05:56:25, Ilya Sherman wrote:
> nit: It's quite contrary to existing style that I've seen to mark pointer
values
> (note: not pointed-to values) as const for params.

Done.

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.h:76: BooleanPrefMember
autofill_enabled_;
On 2013/06/18 05:56:25, Ilya Sherman wrote:
> Can this be removed?

Done.

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.h:91:
autofill::AutofillManagerDelegate* const manager_delegate_;
On 2013/06/18 05:56:25, Ilya Sherman wrote:
> (this const is fine and dandy, though :)

Done.

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

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:355: // May be NULL for OTR and
Android Webview.
On 2013/06/18 05:56:25, Ilya Sherman wrote:
> nit: Please revert all the changes in this file besides the typo fix.

Done.

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
File components/autofill/browser/autofill_manager_delegate.h (right):

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
components/autofill/browser/autofill_manager_delegate.h:131: // Whether the
autocomplete feature of the autofill should be disabled.
On 2013/06/18 05:56:25, Ilya Sherman wrote:
> nit: "should be disabled" -> "should be enabled" to match the method name.

Done.

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
components/autofill/browser/autofill_manager_delegate.h:131: // Whether the
autocomplete feature of the autofill should be disabled.
On 2013/06/18 05:56:25, Ilya Sherman wrote:
> nit: "the autofill" -> "Autofill" or "the Autofill feature"

Done.

https://codereview.chromium.org/15097004/diff/67001/components/autofill/brows...
components/autofill/browser/autofill_manager_delegate.h:132: virtual bool
IsAutoCompleteEnabled() = 0;
On 2013/06/18 05:56:25, Ilya Sherman wrote:
> nit: "AutoComplete" -> "Autocomplete", to match the class name
> "AutocompleteHistoryManager"

Done.

[Ilya Sherman, 2013-06-19 00:38:27]

Thanks, looking good.  Just a bunch of nits:

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

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.cc:48:
autofill::AutofillManagerDelegate* manager_delegate)
nit: This code is already in the autofill:: namespace, so no need to include the
namespace prefix here.

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

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.h:32:
autofill::AutofillManagerDelegate* delegate);
nit: This declaration is already in the autofill:: namespace, so no need to
include the namespace prefix here.

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.h:90:
autofill::AutofillManagerDelegate* const manager_delegate_;
nit: Please leave a blank line above this member variable, and add docs.

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

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager_unittest.cc:228:
explicit AutocompleteHistoryManagerNoIPC(AutofillDriver* driver,
nit: No longer a need for explicit here.

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

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:206: new
AutocompleteHistoryManager(driver,delegate));
nit: Please include a space after the comma.

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:206: new
AutocompleteHistoryManager(driver,delegate));
nit: Why did this get moved out of the initializer list?

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:981: new
AutocompleteHistoryManager(driver, delegate));
nit: Why did this get moved out of the initializer list?

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

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:348: // May be NULL. NULL
indicates OTR.
nit: Please revert the diff to this line.

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
File components/autofill/browser/autofill_manager_delegate.h (right):

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager_delegate.h:133: // Whether the
autocomplete feature of Autofill should be enabled.
nit: "Autocomplete" (feature names are capitalized)

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

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:488:
form_submissions(0) { }
nit: No space between the curlies.

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:490: void
OnFormSubmitted(const FormData& form) {
nit: virtual, OVERRIDE

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:490: void
OnFormSubmitted(const FormData& form) {
nit: This seems like a good function to just mock out using Gmock.

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:491:
form_submissions++;
nit: ++form_submissions;

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:492: }
nit: Leave a blank line below this one.

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:495: }
nit: Leave a blank line below this one.

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:2739: new
MockAutocompleteHistoryManager(autofill_driver_.get(), &delegate);
nit: Please declare this as a scoped_ptr.

[sgurun-gerrit only, 2013-06-19 17:56:22]

PTAL, thanks

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

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.cc:48:
autofill::AutofillManagerDelegate* manager_delegate)
On 2013/06/19 00:38:27, Ilya Sherman wrote:
> nit: This code is already in the autofill:: namespace, so no need to include
the
> namespace prefix here.

Done.

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

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.h:32:
autofill::AutofillManagerDelegate* delegate);
On 2013/06/19 00:38:27, Ilya Sherman wrote:
> nit: This declaration is already in the autofill:: namespace, so no need to
> include the namespace prefix here.

Done.

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager.h:90:
autofill::AutofillManagerDelegate* const manager_delegate_;
On 2013/06/19 00:38:27, Ilya Sherman wrote:
> nit: Please leave a blank line above this member variable, and add docs.

Done.

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

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autocomplete_history_manager_unittest.cc:228:
explicit AutocompleteHistoryManagerNoIPC(AutofillDriver* driver,
On 2013/06/19 00:38:27, Ilya Sherman wrote:
> nit: No longer a need for explicit here.

Done.

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

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:206: new
AutocompleteHistoryManager(driver,delegate));
On 2013/06/19 00:38:27, Ilya Sherman wrote:
> nit: Please include a space after the comma.

Done.

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:206: new
AutocompleteHistoryManager(driver,delegate));
On 2013/06/19 00:38:27, Ilya Sherman wrote:
> nit: Why did this get moved out of the initializer list?

Done.

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:981: new
AutocompleteHistoryManager(driver, delegate));
On 2013/06/19 00:38:27, Ilya Sherman wrote:
> nit: Why did this get moved out of the initializer list?

Done.

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

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:348: // May be NULL. NULL
indicates OTR.
On 2013/06/19 00:38:27, Ilya Sherman wrote:
> nit: Please revert the diff to this line.

Done.

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
File components/autofill/browser/autofill_manager_delegate.h (right):

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager_delegate.h:133: // Whether the
autocomplete feature of Autofill should be enabled.
On 2013/06/19 00:38:27, Ilya Sherman wrote:
> nit: "Autocomplete" (feature names are capitalized)

Done.

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

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:488:
form_submissions(0) { }
On 2013/06/19 00:38:27, Ilya Sherman wrote:
> nit: No space between the curlies.

Done.

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:490: void
OnFormSubmitted(const FormData& form) {
On 2013/06/19 00:38:27, Ilya Sherman wrote:
> nit: virtual, OVERRIDE

Done.

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:490: void
OnFormSubmitted(const FormData& form) {
On 2013/06/19 00:38:27, Ilya Sherman wrote:
> nit: virtual, OVERRIDE

Done.

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:491:
form_submissions++;
On 2013/06/19 00:38:27, Ilya Sherman wrote:
> nit: ++form_submissions;

Done.

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:492: }
On 2013/06/19 00:38:27, Ilya Sherman wrote:
> nit: Leave a blank line below this one.

Done.

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:495: }
On 2013/06/19 00:38:27, Ilya Sherman wrote:
> nit: Leave a blank line below this one.

Done.

https://codereview.chromium.org/15097004/diff/84001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:2739: new
MockAutocompleteHistoryManager(autofill_driver_.get(), &delegate);
On 2013/06/19 00:38:27, Ilya Sherman wrote:
> nit: Please declare this as a scoped_ptr.

Done.

[Ilya Sherman, 2013-06-19 23:19:54]

Thanks!  LGTM with a final two comments:

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

https://codereview.chromium.org/15097004/diff/94001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:2737:
autocomplete_history_manager.get());
Pretty sure this needs to be .Pass(), or else you'll have a double-free.

https://codereview.chromium.org/15097004/diff/94001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:2745:
OnFormSubmitted(_)).Times(1);
I'm pretty sure this is supposed to go *before* the code that's expected to
trigger it.

[sgurun-gerrit only, 2013-06-20 15:18:44]

many thanks,

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

https://codereview.chromium.org/15097004/diff/94001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:2737:
autocomplete_history_manager.get());
You are right. thanks.

On 2013/06/19 23:19:55, Ilya Sherman wrote:
> Pretty sure this needs to be .Pass(), or else you'll have a double-free.

https://codereview.chromium.org/15097004/diff/94001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:2745:
OnFormSubmitted(_)).Times(1);
On 2013/06/19 23:19:55, Ilya Sherman wrote:
> I'm pretty sure this is supposed to go *before* the code that's expected to
> trigger it.

Done.

[sgurun-gerrit only, 2013-06-20 17:12:22]

benm, need aw review

[benm (inactive), 2013-06-20 18:13:41]

basically all looking good!

Thanks for the reviews Ilya!

https://chromiumcodereview.appspot.com/15097004/diff/111001/android_webview/b...
File android_webview/browser/aw_browser_context.cc (right):

https://chromiumcodereview.appspot.com/15097004/diff/111001/android_webview/b...
android_webview/browser/aw_browser_context.cc:143:
autofill::prefs::kAutofillEnabled, false);
Maybe worth a comment to note that we use AutoFill machinery for AutoComplete
functionality, but don't use the full Autofill profile stuff, which is why we
force false here

https://chromiumcodereview.appspot.com/15097004/diff/111001/android_webview/r...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/15097004/diff/111001/android_webview/r...
android_webview/renderer/aw_content_renderer_client.cc:51: new
autofill::PasswordAutofillAgent(render_view);
I was trying to figure out whether we could get away with not providing a
password_autofill_agent in the WebView. PasswordAutofillAgent is a
WeakPtrFactory, and the comment in AutoFillAgent's header suggests it should be
a WeakPtr but then stores it as a raw pointer. 

Would it be worth (maybe in a follow up) making AutofillAgent carry a WeakPtr
(and updating the .cc to check it before use) then we can get rid of this
instance here.

[sgurun-gerrit only, 2013-06-20 18:50:29]

https://chromiumcodereview.appspot.com/15097004/diff/111001/android_webview/b...
File android_webview/browser/aw_browser_context.cc (right):

https://chromiumcodereview.appspot.com/15097004/diff/111001/android_webview/b...
android_webview/browser/aw_browser_context.cc:143:
autofill::prefs::kAutofillEnabled, false);
On 2013/06/20 18:13:42, benm wrote:
> Maybe worth a comment to note that we use AutoFill machinery for AutoComplete
> functionality, but don't use the full Autofill profile stuff, which is why we
> force false here

Done.

https://chromiumcodereview.appspot.com/15097004/diff/111001/android_webview/r...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/15097004/diff/111001/android_webview/r...
android_webview/renderer/aw_content_renderer_client.cc:51: new
autofill::PasswordAutofillAgent(render_view);
On 2013/06/20 18:13:42, benm wrote:
> I was trying to figure out whether we could get away with not providing a
> password_autofill_agent in the WebView. PasswordAutofillAgent is a
> WeakPtrFactory, and the comment in AutoFillAgent's header suggests it should
be
> a WeakPtr but then stores it as a raw pointer. 
> 
> Would it be worth (maybe in a follow up) making AutofillAgent carry a WeakPtr
> (and updating the .cc to check it before use) then we can get rid of this
> instance here.

Done.

[benm (inactive), 2013-06-20 18:52:44]

lgtm

[commit-bot: I haz the power, 2013-06-20 18:57:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sgurun@chromium.org/15097004/99022

[commit-bot: I haz the power, 2013-06-21 14:30:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sgurun@chromium.org/15097004/99022

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sgurun@chromium.org/15097004/139002

[commit-bot: I haz the power, 2013-06-21 21:50:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sgurun@chromium.org/15097004/157022

[commit-bot: I haz the power, 2013-06-22 00:57:49]

Change committed as 207984