Issue 8353025: External autofill delegates.

Issue 8353025: External autofill delegates. (Closed)

Created:
9 years, 2 months ago by John Grabowski

Modified:
9 years, 1 month ago

Reviewers:
jam, Ilya Sherman, dhollowa

CC:
chromium-reviews, GeorgeY, Avi (use Gerrit), creis+watch_chromium.org, ajwong+watch_chromium.org, jam, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, dyu1, brettw-cc_chromium.org, csharp

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

Description

External autofill delegates. BUG= TEST= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107839

Patch Set 1 #

Patch Set 2 : initial set #

Patch Set 3 : Carnitasify #

Total comments: 21

Patch Set 4 : feedback #

Total comments: 12

Patch Set 5 : latest feedback #

Patch Set 6 : add final TODOs #

Patch Set 7 : rebase against TOT #

Total comments: 1

Created: 9 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+322 lines, -3 lines)			Patch
M	chrome/browser/autocomplete_history_manager.h	View	1 2 3	5 chunks	+18 lines, -0 lines	0 comments	Download
M	chrome/browser/autocomplete_history_manager.cc	View		3 chunks	+12 lines, -1 line	0 comments	Download
M	chrome/browser/autocomplete_history_manager_unittest.cc	View	1 2 3 4 5 6	2 chunks	+53 lines, -0 lines	0 comments	Download
A	chrome/browser/autofill/autofill_external_delegate.h	View	1 2 3 4 5	1 chunk	+64 lines, -0 lines	0 comments	Download
A	chrome/browser/autofill/autofill_external_delegate.cc	View	1 2 3	1 chunk	+37 lines, -0 lines	0 comments	Download
M	chrome/browser/autofill/autofill_manager.h	View	1 2 3 4 5 6	5 chunks	+27 lines, -0 lines	0 comments	Download
M	chrome/browser/autofill/autofill_manager.cc	View	1 2 3 4 5 6	4 chunks	+8 lines, -2 lines	0 comments	Download
M	chrome/browser/autofill/autofill_manager_unittest.cc	View	1 2 3 4 5 6	5 chunks	+69 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/tab_contents/tab_contents_wrapper.h	View	1 2 3 4 5 6	2 chunks	+2 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/tab_contents/tab_contents_wrapper.cc	View	1 2 3 4 5 6	2 chunks	+10 lines, -0 lines	0 comments	Download
M	chrome/chrome_browser.gypi	View	1 2 3 4 5 6	1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/common/autofill_messages.h	View	1 2 3 4 5 6	1 chunk	+4 lines, -0 lines	0 comments	Download
M	chrome/common/chrome_switches.h	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/common/chrome_switches.cc	View	1 2 3 4 5 6	1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/renderer/autofill/autofill_agent.h	View	1 2 3 4 5 6	1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/renderer/autofill/autofill_agent.cc	View	1 2 3 4 5 6	2 chunks	+8 lines, -0 lines	0 comments	Download
M	content/content_common.gypi	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	1 comment	Download

Messages

Total messages: 16 (0 generated)

Expand Messages | Collapse Messages

Ilya Sherman

Is this ready for review, or are you planning to make some further changes first?

9 years, 2 months ago (2011-10-20 22:55:03 UTC) #1

John Grabowski

Not ready for review Adding unit tests jrg On Thu, Oct 20, 2011 at 3:55 ...

9 years, 2 months ago (2011-10-20 22:55:44 UTC) #2

Ilya Sherman

http://codereview.chromium.org/8353025/diff/6001/chrome/browser/autocomplete_history_manager.h File chrome/browser/autocomplete_history_manager.h (right): http://codereview.chromium.org/8353025/diff/6001/chrome/browser/autocomplete_history_manager.h#newcode60 chrome/browser/autocomplete_history_manager.h:60: FRIEND_TEST(AutofillManagerTest, TestTabContents); nit: These should probably be FRIEND_TEST_ALL_PREFIXES http://codereview.chromium.org/8353025/diff/6001/chrome/browser/autocomplete_history_manager_unittest.cc ...

9 years, 1 month ago (2011-10-26 11:09:57 UTC) #4

dhollowa

http://codereview.chromium.org/8353025/diff/6001/chrome/browser/autocomplete_history_manager_unittest.cc File chrome/browser/autocomplete_history_manager_unittest.cc (right): http://codereview.chromium.org/8353025/diff/6001/chrome/browser/autocomplete_history_manager_unittest.cc#newcode146 chrome/browser/autocomplete_history_manager_unittest.cc:146: MOCK_METHOD5(OnSuggestionsReturned, void( nit: indent seems off here. What about ...

9 years, 1 month ago (2011-10-26 16:14:22 UTC) #5

John Grabowski

Feedback applied except where noted. Thx for the scrutiny. http://codereview.chromium.org/8353025/diff/6001/chrome/browser/autofill/autofill_manager_unittest.cc File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/8353025/diff/6001/chrome/browser/autofill/autofill_manager_unittest.cc#newcode2878 chrome/browser/autofill/autofill_manager_unittest.cc:2878: ...

9 years, 1 month ago (2011-10-27 03:05:59 UTC) #6

Feedback applied except where noted.
Thx for the scrutiny.

http://codereview.chromium.org/8353025/diff/6001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

http://codereview.chromium.org/8353025/diff/6001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_manager_unittest.cc:2878:
autofill_manager_->SetExternalDelegate(NULL);
On 2011/10/26 11:09:58, Ilya Sherman wrote:
> nit: Is this line really necessary for the test?

If not, autofill_manager_ will have a pointer to a stack variable that just went
out of scope.

http://codereview.chromium.org/8353025/diff/6001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/8353025/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.h:362: void
SelectAutofillSuggestionAtIndex(int listIndex);
On 2011/10/26 11:09:58, Ilya Sherman wrote:
> We should not add a dependency on Autofill to content/.  Can this method be
> defined on the AutofillExternalDelegate itself; or otherwise on the
> AutofillManager class?

Yes.  Looks like I didn't finish the Carnitas-ification.
I took a stab at doing this the right way but would appreciate feedback.

http://codereview.chromium.org/8353025/diff/6001/content/renderer/autofill_di...
File content/renderer/autofill_dispatcher.cc (right):

http://codereview.chromium.org/8353025/diff/6001/content/renderer/autofill_di...
content/renderer/autofill_dispatcher.cc:30: //
render_view()->webview()->selectAutofillSuggestionAtIndex(listIndex);
On 2011/10/26 11:09:58, Ilya Sherman wrote:
> What will |selectAutofillSuggestionAtIndex| do?  I would prefer not to add
code
> to WebKit to support further Autofill functionality unless we really have to. 
> The task Chris Sharp is working on right now is to move all of the Autofill
> functionality out of WebKit entirely ;)

Briefly, at this time WebKit owns the list of suggestions.
Step one is to move display of these suggestions out of WebKit.
(This CL is the Chrome side of that work).
Step two is to move knowledge of the suggestion list out of WebKit as well.
I am working with Chris on the WebKit side; what I currently have is only part
way there.

WebKit and Chrome will still need to communicate.  Although Chrome already knows
the bounding box of the current text input field (which would set the position
of the autofill window on-screen), I don't think Chrome has a "fill input field
with text X" mechanism.  Thus, "out of WebKit entirely" is a bit too strong I
think.

Ilya Sherman

http://codereview.chromium.org/8353025/diff/6001/chrome/browser/autofill/autofill_manager_unittest.cc File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/8353025/diff/6001/chrome/browser/autofill/autofill_manager_unittest.cc#newcode2878 chrome/browser/autofill/autofill_manager_unittest.cc:2878: autofill_manager_->SetExternalDelegate(NULL); On 2011/10/27 03:05:59, John Grabowski wrote: > On ...

9 years, 1 month ago (2011-10-27 11:01:54 UTC) #7

http://codereview.chromium.org/8353025/diff/6001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

http://codereview.chromium.org/8353025/diff/6001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_manager_unittest.cc:2878:
autofill_manager_->SetExternalDelegate(NULL);
On 2011/10/27 03:05:59, John Grabowski wrote:
> On 2011/10/26 11:09:58, Ilya Sherman wrote:
> > nit: Is this line really necessary for the test?
> 
> If not, autofill_manager_ will have a pointer to a stack variable that just
went
> out of scope.

Sure, but it's not like NULL is especially much so in scope, either ;)  It
doesn't matter too much either way -- I was mostly just curious if this was
reflecting something that non-test clients should be doing as well.

http://codereview.chromium.org/8353025/diff/6001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/8353025/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.h:362: void
SelectAutofillSuggestionAtIndex(int listIndex);
On 2011/10/27 03:05:59, John Grabowski wrote:
> On 2011/10/26 11:09:58, Ilya Sherman wrote:
> > We should not add a dependency on Autofill to content/.  Can this method be
> > defined on the AutofillExternalDelegate itself; or otherwise on the
> > AutofillManager class?
> 
> Yes.  Looks like I didn't finish the Carnitas-ification.
> I took a stab at doing this the right way but would appreciate feedback.

Thanks, the Carnitas-ification looks good with the latest patch set :)

http://codereview.chromium.org/8353025/diff/6001/content/renderer/autofill_di...
File content/renderer/autofill_dispatcher.cc (right):

http://codereview.chromium.org/8353025/diff/6001/content/renderer/autofill_di...
content/renderer/autofill_dispatcher.cc:30: //
render_view()->webview()->selectAutofillSuggestionAtIndex(listIndex);
On 2011/10/27 03:05:59, John Grabowski wrote:
> On 2011/10/26 11:09:58, Ilya Sherman wrote:
> > What will |selectAutofillSuggestionAtIndex| do?  I would prefer not to add
> code
> > to WebKit to support further Autofill functionality unless we really have
to. 
> > The task Chris Sharp is working on right now is to move all of the Autofill
> > functionality out of WebKit entirely ;)
> 
> Briefly, at this time WebKit owns the list of suggestions.
> Step one is to move display of these suggestions out of WebKit.
> (This CL is the Chrome side of that work).
> Step two is to move knowledge of the suggestion list out of WebKit as well.
> I am working with Chris on the WebKit side; what I currently have is only part
> way there.

Hmm, ok.  I think it might be easier to avoid adding new Autofill methods to
WebKit at all; but as long as they are merely transitional, I'm ok with it. 
When you do add the method to WebKit, please mark it clearly as a transitional
API.

> WebKit and Chrome will still need to communicate.  Although Chrome already
knows
> the bounding box of the current text input field (which would set the position
> of the autofill window on-screen), I don't think Chrome has a "fill input
field
> with text X" mechanism.  Thus, "out of WebKit entirely" is a bit too strong I
> think.

Sure, the Chrome renderer will still need to talk to WebKit in order to fill the
form -- that's currently handled in chrome/renderer/autofill, and I don't expect
it to go away.  I just meant that WebKit should not have any particular
knowledge of Autofill as a feature, much like content/ should not.

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_external_delegate.cc:19: RenderViewHost* host =
tab_contents_wrapper_->render_view_host();
nit: How about just storing the RenderViewHost as a member variable, rather than
the TabContentsWrapper?

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_external_delegate.h (right):

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_external_delegate.h:22: // display and
selection.
nit: For consistency with the style in adjacent files, please capitalize
Autocomplete and Autofill (throughout).

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_external_delegate.h:29: void
SelectAutofillSuggestionAtIndex(int listIndex);
nit: Should this perhaps be protected?  Where is this method invoked?

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_external_delegate.h:29: void
SelectAutofillSuggestionAtIndex(int listIndex);
Also, Autofill has two selection-like states, which we sometimes call "select"
and "accept".  The prior occurs when you hover over a list item with your mouse,
or highlight it via the keyboard, and causes a preview of the autofilled form to
be shown in the renderer.  The latter occurs when you click on a list item, or
accept it by pressing Enter, and causes the form to actually be autofilled.  As
far as I can tell, this code is currently only set up to support one of those
gestures -- you'll probably want to add a parallel method that handles the
other.

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_manager.h:87: }
nit: Can this be passed into the constructor rather than being initialized
separately, or might a single AutofillManager be associated with multiple
AutofillExternalDelegates over its lifetime?

John Grabowski

More better diffs send up. PTAL. http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/autofill_external_delegate.cc File chrome/browser/autofill/autofill_external_delegate.cc (right): http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/autofill_external_delegate.cc#newcode19 chrome/browser/autofill/autofill_external_delegate.cc:19: RenderViewHost* host = ...

9 years, 1 month ago (2011-10-27 17:42:20 UTC) #8

More better diffs send up.  PTAL.

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_external_delegate.cc:19: RenderViewHost* host =
tab_contents_wrapper_->render_view_host();
On 2011/10/27 11:01:55, Ilya Sherman wrote:
> nit: How about just storing the RenderViewHost as a member variable, rather
than
> the TabContentsWrapper?

discussed offline

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_external_delegate.h (right):

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_external_delegate.h:22: // display and
selection.
On 2011/10/27 11:01:55, Ilya Sherman wrote:
> nit: For consistency with the style in adjacent files, please capitalize
> Autocomplete and Autofill (throughout).

Done (throughout)

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_external_delegate.h:29: void
SelectAutofillSuggestionAtIndex(int listIndex);
On 2011/10/27 11:01:55, Ilya Sherman wrote:
> nit: Should this perhaps be protected?  Where is this method invoked?

This method would be invoked by the platform-specific code that displays the
list of suggestions.  Prerequisites for landing my version have themselves not
landed in Chromium.

Presumably, when the remaining autofill selection is pulled from WebKit to
Chromium, this method will go away.  We won't need to tell WebKit we selected
anything; we'll just fill in the suggestion in the Browser.  We are not at that
point yet.

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_external_delegate.h:29: void
SelectAutofillSuggestionAtIndex(int listIndex);
On 2011/10/27 11:01:55, Ilya Sherman wrote:
> Also, Autofill has two selection-like states, which we sometimes call "select"
> and "accept".  The prior occurs when you hover over a list item with your
mouse,
> or highlight it via the keyboard, and causes a preview of the autofilled form
to
> be shown in the renderer.  The latter occurs when you click on a list item, or
> accept it by pressing Enter, and causes the form to actually be autofilled. 
As
> far as I can tell, this code is currently only set up to support one of those
> gestures -- you'll probably want to add a parallel method that handles the
> other.

My platform does not have hover capacity.  This is something Chris and I can
collaborate on.

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_manager.h:87: }
On 2011/10/27 11:01:55, Ilya Sherman wrote:
> nit: Can this be passed into the constructor rather than being initialized
> separately, or might a single AutofillManager be associated with multiple
> AutofillExternalDelegates over its lifetime?

One platform's likely implementation of the external delegate has a weak pointer
to the manager, so no.  That may change in the future.  Would you like a TODO?

Ilya Sherman

LGTM. Thanks for your patience :) http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/autofill_external_delegate.h File chrome/browser/autofill/autofill_external_delegate.h (right): http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/autofill_external_delegate.h#newcode29 chrome/browser/autofill/autofill_external_delegate.h:29: void SelectAutofillSuggestionAtIndex(int listIndex); ...

9 years, 1 month ago (2011-10-27 21:17:23 UTC) #9

LGTM.  Thanks for your patience :)

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_external_delegate.h (right):

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_external_delegate.h:29: void
SelectAutofillSuggestionAtIndex(int listIndex);
On 2011/10/27 17:42:21, John Grabowski wrote:
> On 2011/10/27 11:01:55, Ilya Sherman wrote:
> > Also, Autofill has two selection-like states, which we sometimes call
"select"
> > and "accept".  The prior occurs when you hover over a list item with your
> mouse,
> > or highlight it via the keyboard, and causes a preview of the autofilled
form
> to
> > be shown in the renderer.  The latter occurs when you click on a list item,
or
> > accept it by pressing Enter, and causes the form to actually be autofilled. 
> As
> > far as I can tell, this code is currently only set up to support one of
those
> > gestures -- you'll probably want to add a parallel method that handles the
> > other.
> 
> My platform does not have hover capacity.  This is something Chris and I can
> collaborate on.

Ok.  Please add a TODO here just as a reminder for the other platforms.

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/8353025/diff/12001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_manager.h:87: }
On 2011/10/27 17:42:21, John Grabowski wrote:
> On 2011/10/27 11:01:55, Ilya Sherman wrote:
> > nit: Can this be passed into the constructor rather than being initialized
> > separately, or might a single AutofillManager be associated with multiple
> > AutofillExternalDelegates over its lifetime?
> 
> One platform's likely implementation of the external delegate has a weak
pointer
> to the manager, so no.  That may change in the future.  Would you like a TODO?
> 

Sure, let's add a TODO just to keep this in the back of our minds as this code
evolves.

Ilya Sherman

Quick heads up: you'll probably need to rebase to catch up to http://codereview.chromium.org/8351027/

9 years, 1 month ago (2011-10-28 20:03:15 UTC) #11

John Grabowski

On 2011/10/28 20:03:15, Ilya Sherman wrote: > Quick heads up: you'll probably need to rebase ...

9 years, 1 month ago (2011-10-28 20:49:10 UTC) #12

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jrg@chromium.org/8353025/19003

9 years, 1 month ago (2011-10-28 23:16:14 UTC) #13

jam

http://codereview.chromium.org/8353025/diff/19003/content/content_common.gypi File content/content_common.gypi (right): http://codereview.chromium.org/8353025/diff/19003/content/content_common.gypi#newcode62 content/content_common.gypi:62: 'common/autofill_messages.h', this is in the wrong place.. this belongs ...

9 years, 1 month ago (2011-10-29 01:48:03 UTC) #15

John Grabowski

9 years, 1 month ago (2011-10-31 16:52:14 UTC) #16

On 2011/10/29 01:48:03, John Abd-El-Malek wrote:
> http://codereview.chromium.org/8353025/diff/19003/content/content_common.gypi
> File content/content_common.gypi (right):
> 
>
http://codereview.chromium.org/8353025/diff/19003/content/content_common.gypi...
> content/content_common.gypi:62: 'common/autofill_messages.h',
> this is in the wrong place.. this belongs in chrome_common.gypi

I will follow up in new CL

Expand Messages | Collapse Messages