Moving AutofillAgent Logic into Browser

Moving AutofillAgent Logic into Browser

This is the first attempt at moving some of the Autofill logic out of the 
autofillagent in the renderer and into the external delegate in the browser.
I just focused on OnSuggestionsReturned, so the browser side should be able to 
correcrtly create the list of suggestions to show. There are still features that
need to be moved, such as the ability to select an item, which I will address in
future cls.

BUG=51644
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111240

[Ilya Sherman, 2011-11-17 02:24:37]

I think it might be better to hold off on migrating this logic until there is at
least a basic popup UI.  There's a lot of subtlety in the details here, and much
of that might need to be updated to better match the new UI.

That said, if you'd like to get this in earlier and iterate from there, here are
a bunch of comments:

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_external_delegate.cc:21: autofill_query_id_(0)
{
Don't forget to initialize other member variables, e.g.
|has_shown_autofill_popup_for_current_edit_|

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_external_delegate.cc:37: autofill_query_id_ =
query_id;
Why not simply pass these values into OnSuggestionsReturned()?

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_external_delegate.cc:53: return;
This case shouldn't be reachable, since this communication occurs within a
single task in the browser process.

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_external_delegate.cc:124: }
nit: No need for curly braces since this is a one-line if-stmt.

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_external_delegate.h (right):

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_external_delegate.h:23: // this logic.
nit: Please add a link to http://crbug.com/51644

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_external_delegate.h:41: bool display_warning);
nit: Please describe the role of |display_warning| in the method comment.

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_external_delegate.h:98: int
suggestions_options_index_;
I'm not sure that we'll need these indices for the new UI -- I'm hopeful that we
can structure the code in such a way that the presence or absence of these items
can easily be deduced from the popup directly.

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_manager.cc:455: display_warning);
nit: Please add curly braces since this method body no longer fits on a single
line.

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

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_manager.h:67: //
AutofillDownloadManager::Observer:
nit: Please remove this line -- these are not AutofillDownloadManager::Observer
methods.

http://codereview.chromium.org/8488011/diff/1/chrome/common/autofill_messages.h
File chrome/common/autofill_messages.h (right):

http://codereview.chromium.org/8488011/diff/1/chrome/common/autofill_messages...
chrome/common/autofill_messages.h:162:
IPC_MESSAGE_ROUTED0(AutofillHostMsg_SetDidEndTextFieldEditing)
nit: This should probably be named "DidEndTextFieldEditing", without the "Set".

http://codereview.chromium.org/8488011/diff/1/webkit/glue/form_field.h
File webkit/glue/form_field.h (right):

http://codereview.chromium.org/8488011/diff/1/webkit/glue/form_field.h#newcode35
webkit/glue/form_field.h:35: bool autocomplete;
nit: Perhaps "should_autocomplete"?

[csharp, 2011-11-18 18:15:08]

I like the idea of getting this part in first, since it helps me see a bit more
where exactly the hooks for the view should go.

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_external_delegate.cc:21: autofill_query_id_(0)
{
On 2011/11/17 02:24:37, Ilya Sherman wrote:
> Don't forget to initialize other member variables, e.g.
> |has_shown_autofill_popup_for_current_edit_|

Done.

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_external_delegate.cc:37: autofill_query_id_ =
query_id;
On 2011/11/17 02:24:37, Ilya Sherman wrote:
> Why not simply pass these values into OnSuggestionsReturned()?

I don't think there would be an easy way to do that. OnSuggestionsReturned is
called from AutocompleteHistoryManager, which currently does see any of these
values, so we'd need to add a call to AutocompleteHistoryManager to enable it to
pass this information.

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_external_delegate.cc:53: return;
On 2011/11/17 02:24:37, Ilya Sherman wrote:
> This case shouldn't be reachable, since this communication occurs within a
> single task in the browser process.

I'm not sure that this all occurs in a single task (I could be wrong). In
looking through the code I noticed that between OnQuery and
OnSuggestionsReturned the AutocompleteHistoryManager calls
web_data_service->GetFormValuesForElementNames, which seems to be an
asynchronous call, so wouldn't it be possible to have another call to OnQuery
before we first reach OnSuggestionsReturned?

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_external_delegate.cc:124: }
On 2011/11/17 02:24:37, Ilya Sherman wrote:
> nit: No need for curly braces since this is a one-line if-stmt.

Done.

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_external_delegate.h (right):

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_external_delegate.h:23: // this logic.
On 2011/11/17 02:24:37, Ilya Sherman wrote:
> nit: Please add a link to http://crbug.com/51644

Done.

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_external_delegate.h:41: bool display_warning);
On 2011/11/17 02:24:37, Ilya Sherman wrote:
> nit: Please describe the role of |display_warning| in the method comment.

Done.

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_external_delegate.h:98: int
suggestions_options_index_;
Good point, I've taken them out.

On 2011/11/17 02:24:37, Ilya Sherman wrote:
> I'm not sure that we'll need these indices for the new UI -- I'm hopeful that
we
> can structure the code in such a way that the presence or absence of these
items
> can easily be deduced from the popup directly.

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_manager.cc:455: display_warning);
On 2011/11/17 02:24:37, Ilya Sherman wrote:
> nit: Please add curly braces since this method body no longer fits on a single
> line.

Done.

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

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_manager.h:67: //
AutofillDownloadManager::Observer:
On 2011/11/17 02:24:37, Ilya Sherman wrote:
> nit: Please remove this line -- these are not
AutofillDownloadManager::Observer
> methods.

Done.

http://codereview.chromium.org/8488011/diff/1/chrome/common/autofill_messages.h
File chrome/common/autofill_messages.h (right):

http://codereview.chromium.org/8488011/diff/1/chrome/common/autofill_messages...
chrome/common/autofill_messages.h:162:
IPC_MESSAGE_ROUTED0(AutofillHostMsg_SetDidEndTextFieldEditing)
On 2011/11/17 02:24:37, Ilya Sherman wrote:
> nit: This should probably be named "DidEndTextFieldEditing", without the
"Set".

Done.

http://codereview.chromium.org/8488011/diff/1/webkit/glue/form_field.h
File webkit/glue/form_field.h (right):

http://codereview.chromium.org/8488011/diff/1/webkit/glue/form_field.h#newcode35
webkit/glue/form_field.h:35: bool autocomplete;
On 2011/11/17 02:24:37, Ilya Sherman wrote:
> nit: Perhaps "should_autocomplete"?

Done.

[Ilya Sherman, 2011-11-18 23:16:04]

LGTM with nits, thanks :)

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_external_delegate.cc:37: autofill_query_id_ =
query_id;
On 2011/11/18 18:15:10, csharp wrote:
> On 2011/11/17 02:24:37, Ilya Sherman wrote:
> > Why not simply pass these values into OnSuggestionsReturned()?
> 
> I don't think there would be an easy way to do that. OnSuggestionsReturned is
> called from AutocompleteHistoryManager, which currently does see any of these
> values, so we'd need to add a call to AutocompleteHistoryManager to enable it
to
> pass this information.

Fair enough -- I keep forgetting that the AutocompleteHistoryManager code runs
asynchronously.

http://codereview.chromium.org/8488011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_external_delegate.cc:53: return;
On 2011/11/18 18:15:10, csharp wrote:
> On 2011/11/17 02:24:37, Ilya Sherman wrote:
> > This case shouldn't be reachable, since this communication occurs within a
> > single task in the browser process.
> 
> I'm not sure that this all occurs in a single task (I could be wrong). In
> looking through the code I noticed that between OnQuery and
> OnSuggestionsReturned the AutocompleteHistoryManager calls
> web_data_service->GetFormValuesForElementNames, which seems to be an
> asynchronous call, so wouldn't it be possible to have another call to OnQuery
> before we first reach OnSuggestionsReturned? 
> 

Yep, you're right -- again, I forgot that the AutocompleteHistoryManager code
runs asynchronously.

http://codereview.chromium.org/8488011/diff/9001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

http://codereview.chromium.org/8488011/diff/9001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate.cc:22:
autofill_query_field_(),
nit: No need to explicitly invoke the default constructor.

http://codereview.chromium.org/8488011/diff/9001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_external_delegate.h (right):

http://codereview.chromium.org/8488011/diff/9001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate.h:43: // warnings (such as
autofill is disabled, but had suggestions).
nit: I would elaborate on this explanation for |display_warning| to include the
motivation for why it might be set to false.  The reason we have a UI for
warnings in the first place is to help explain to users why Autofill is
disabled, so that the feature doesn't just feel broken.  On the other hand,
sometimes Autofill is disabled because websites want to show their own popup in
place of ours, and it's a really poor user-experience to have two competing
popups.  To account for this, we have some simple heuristics to try to detect
when it's safe to show warnings, and when it isn't.  See http://crbug.com/64286
for a bit more backstory on this.

http://codereview.chromium.org/8488011/diff/9001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate.h:48: bool display_warning);
nit: Let's name this |display_warning_if_disabled|, to match the member
variable.

http://codereview.chromium.org/8488011/diff/9001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate.h:71: void
SetDidEndTextFieldEditing();
nit: Here as well, I would omit the "Set" in the name, since there's no member
variable "did_end_text_field_editing_" whose value is being set.

http://codereview.chromium.org/8488011/diff/9001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate.h:101: // Should we display a
warning if autofill is disabled?
nit: "autofill" -> "Autofill"

[Ilya Sherman, 2011-11-18 23:16:07]

LGTM with nits, thanks :)

[csharp, 2011-11-21 14:42:14]

http://codereview.chromium.org/8488011/diff/9001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

http://codereview.chromium.org/8488011/diff/9001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate.cc:22:
autofill_query_field_(),
On 2011/11/18 23:16:04, Ilya Sherman wrote:
> nit: No need to explicitly invoke the default constructor.

Done.

http://codereview.chromium.org/8488011/diff/9001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_external_delegate.h (right):

http://codereview.chromium.org/8488011/diff/9001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate.h:43: // warnings (such as
autofill is disabled, but had suggestions).
On 2011/11/18 23:16:04, Ilya Sherman wrote:
> nit: I would elaborate on this explanation for |display_warning| to include
the
> motivation for why it might be set to false.  The reason we have a UI for
> warnings in the first place is to help explain to users why Autofill is
> disabled, so that the feature doesn't just feel broken.  On the other hand,
> sometimes Autofill is disabled because websites want to show their own popup
in
> place of ours, and it's a really poor user-experience to have two competing
> popups.  To account for this, we have some simple heuristics to try to detect
> when it's safe to show warnings, and when it isn't.  See
http://crbug.com/64286
> for a bit more backstory on this.

Done.

http://codereview.chromium.org/8488011/diff/9001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate.h:48: bool display_warning);
On 2011/11/18 23:16:04, Ilya Sherman wrote:
> nit: Let's name this |display_warning_if_disabled|, to match the member
> variable.

Done.

http://codereview.chromium.org/8488011/diff/9001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate.h:71: void
SetDidEndTextFieldEditing();
On 2011/11/18 23:16:04, Ilya Sherman wrote:
> nit: Here as well, I would omit the "Set" in the name, since there's no member
> variable "did_end_text_field_editing_" whose value is being set.

Done.

http://codereview.chromium.org/8488011/diff/9001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate.h:101: // Should we display a
warning if autofill is disabled?
On 2011/11/18 23:16:04, Ilya Sherman wrote:
> nit: "autofill" -> "Autofill"

Done.

[commit-bot: I haz the power, 2011-11-21 14:42:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/8488011/11001

[commit-bot: I haz the power, 2011-11-21 14:42:34]

Presubmit check for 8488011-11001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
webkit/glue/form_field.h,webkit/glue/form_field.cc

Presubmit checks took 2.0s to calculate.

[csharp, 2011-11-21 14:48:39]

Hey tony@,

I've made some changes to webkit/glue/form_field and need a LGTM from you to
submit it.

Thanks

[tony, 2011-11-21 21:28:48]

LGTM

[commit-bot: I haz the power, 2011-11-21 23:04:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/8488011/11001

[commit-bot: I haz the power, 2011-11-22 00:20:35]

Try job failure for 8488011-11001 (retry) on linux_rel for steps "unit_tests,
browser_tests".
It's a second try, previously, steps "unit_tests, browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-11-22 00:28:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/8488011/11001

[commit-bot: I haz the power, 2011-11-22 02:02:23]

Try job failure for 8488011-11001 (retry) on linux_rel for steps "unit_tests,
browser_tests".
It's a second try, previously, steps "unit_tests, browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Ilya Sherman, 2011-11-22 02:04:31]

Ah, Chris, these seem to be legitimate test failures.  Please take a look at the
commit queue output.

[csharp, 2011-11-22 15:57:37]

Ok, the problem should be fixed. I hadn't initialized the new boolean values in
form_field and that it to try and send invalid booleans in the IPC calls, which
seems to have caused the crashes.

[commit-bot: I haz the power, 2011-11-22 15:58:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/8488011/20001

[csharp, 2011-11-22 17:49:26]

Ok, some of the browser_tests are still failing, but I'm not sure if it is
because of my code or if they just aren't working right now.

InProcessBrowserTest.Empty, omniboxApiTest.Basic,
omniboxApiTest.PopupStaysClosed seem to have failed each time, but I can't seem
to reproduce on my local machines. I'm not sure if it would have passed the try
bot since I stopped the commit once I noticed it was the same tests failing.

isherman@, any ideas on how I should proceed?

[Ilya Sherman, 2011-11-22 21:04:50]

On 2011/11/22 17:49:26, csharp wrote:
> Ok, some of the browser_tests are still failing, but I'm not sure if it is
> because of my code or if they just aren't working right now.
> 
> InProcessBrowserTest.Empty, omniboxApiTest.Basic,
> omniboxApiTest.PopupStaysClosed seem to have failed each time, but I can't
seem
> to reproduce on my local machines. I'm not sure if it would have passed the
try
> bot since I stopped the commit once I noticed it was the same tests failing.

These should be safe to ignore.  In general, for try bot results, an orange
background means "probably flaky failure, safe to ignore"; a red background
means "might be a genuine failure, take a closer look".  I'll re-start the the
commit process -- hopefully should go through all fine and dandy this time :)

[commit-bot: I haz the power, 2011-11-22 21:05:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/8488011/20001

[commit-bot: I haz the power, 2011-11-22 22:49:13]

Change committed as 111240