Bug 51710 – [chromium] Add a WebAutoFillClient interface that moves some functions from WebViewClient

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[John Abd-El-Malek]

[chromium] Add a WebAutoFillClient interface that moves some functions from WebViewClient

[John Abd-El-Malek]

Created attachment 77626 [details]
Patch

[John Abd-El-Malek]

Created attachment 77628 [details]
Patch

[John Abd-El-Malek]

I'm just moving functions from WebViewClient, which will allow some code cleanup in Chromium code (auto-fill, auto-complete, and password-complete features can be detached from RenderView).  The first step is to check in this interface so that RenderView can implement it, then I will check in the WebKit side change that uses the new interface, and once that's rolled, I will remove the old WebKit code.

[David Levin]

Comment on attachment 77628 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77628&action=review

A few comments. Of course Darin will have to give the r+ since this is the public api since my items are just nits, I'm leaving this in the r? state.

> WebKit/chromium/ChangeLog:5
> +        [chromium] Add a WebAutoFillClient interface that moves some functions from WebViewClient

No move appears to have occurred.

Is this part 1 of several? The ChangeLog ideally would tell me this.

> WebKit/chromium/public/WebAutoFillClient.h:46
> +    virtual void queryAutofillSuggestions(const WebNode&,

AutoFill for consistency.

> WebKit/chromium/public/WebAutoFillClient.h:77
> +    virtual void removeAutocompleteSugestion(const WebString& name,

Nit: typo Sugestion

[John Abd-El-Malek]

(In reply to comment #4)
> (From update of attachment 77628 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77628&action=review
> 
> A few comments. Of course Darin will have to give the r+ since this is the public api since my items are just nits, I'm leaving this in the r? state.
> 
> > WebKit/chromium/ChangeLog:5
> > +        [chromium] Add a WebAutoFillClient interface that moves some functions from WebViewClient
> 
> No move appears to have occurred.
> 
> Is this part 1 of several? The ChangeLog ideally would tell me this.

I figured describing this in a comment (#3) is enough.  Isn't this pattern of doing things in stages to avoid two-sided patches common?

> 
> > WebKit/chromium/public/WebAutoFillClient.h:46
> > +    virtual void queryAutofillSuggestions(const WebNode&,
> 
> AutoFill for consistency.

Done

> 
> > WebKit/chromium/public/WebAutoFillClient.h:77
> > +    virtual void removeAutocompleteSugestion(const WebString& name,
> 
> Nit: typo Sugestion

Done

[John Abd-El-Malek]

Created attachment 77820 [details]
Patch

[David Levin]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 77628 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=77628&action=review
> > 
> > A few comments. Of course Darin will have to give the r+ since this is the public api since my items are just nits, I'm leaving this in the r? state.
> > 
> > > WebKit/chromium/ChangeLog:5
> > > +        [chromium] Add a WebAutoFillClient interface that moves some functions from WebViewClient
> > 
> > No move appears to have occurred.
> > 
> > Is this part 1 of several? The ChangeLog ideally would tell me this.
> 
> I figured describing this in a comment (#3) is enough.  Isn't this pattern of doing things in stages to avoid two-sided patches common?

Semi-common. I like the ChangeLog because it makes the patch self contained and unfortunately, (personally) I sometimes don't notice the comments in the bug like this time :(, so when I was reading the patch alone it was confusing to me.

[Darin Fisher (:fishd, Google)]

Comment on attachment 77820 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77820&action=review

> WebKit/chromium/public/WebAutoFillClient.h:77
> +    virtual void removeAutocompleteSuggestion(const WebString& name,

it is the presence of these other methods that don't have AutoFill in their
name that gives me pause about naming this interface WebAutoFillClient.  i'm
struggling to come up with a better name though...

[John Abd-El-Malek]

(In reply to comment #8)
> (From update of attachment 77820 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77820&action=review
> 
> > WebKit/chromium/public/WebAutoFillClient.h:77
> > +    virtual void removeAutocompleteSuggestion(const WebString& name,
> 
> it is the presence of these other methods that don't have AutoFill in their
> name that gives me pause about naming this interface WebAutoFillClient.  i'm
> struggling to come up with a better name though...

yeah, I tried to come up with something more inclusive to auto-fill/auto-complete/password-complete but couldn't.  The same issue exists on the chromium side.  WebFillClient?  But that'd be even more confusing.  WebFormFillClient?

[Darin Fisher (:fishd, Google)]

Some questions:

1- Some of the methods are just generic editing related notifications.  WebEditingClient would be a good name for those methods.

2- Perhaps we should consider moving more of the autofill / autocomplete implementation into Chromium?  Perhaps that would lead us to create more generic interfaces in webkit?

[John Abd-El-Malek]

(In reply to comment #10)
> Some questions:
> 
> 1- Some of the methods are just generic editing related notifications.  WebEditingClient would be a good name for those methods.

hmm, so are you suggesting that we add two new interfaces, both for auto-fill related notifications, and both going to the same listener in chromium?  I could do that, but it feels unnecessary given that the only consumers of the editing notifications would still be auto-fill code.

> 
> 2- Perhaps we should consider moving more of the autofill / autocomplete implementation into Chromium?  Perhaps that would lead us to create more generic interfaces in webkit?

Perhaps, but that's outside the scope of my current cleanup (making RenderView smaller).

[John Abd-El-Malek]

(In reply to comment #11)
> (In reply to comment #10)
> > Some questions:
> > 
> > 1- Some of the methods are just generic editing related notifications.  WebEditingClient would be a good name for those methods.
> 
> hmm, so are you suggesting that we add two new interfaces, both for auto-fill related notifications, and both going to the same listener in chromium?  I could do that, but it feels unnecessary given that the only consumers of the editing notifications would still be auto-fill code.
> 
> > 
> > 2- Perhaps we should consider moving more of the autofill / autocomplete implementation into Chromium?  Perhaps that would lead us to create more generic interfaces in webkit?
> 
> Perhaps, but that's outside the scope of my current cleanup (making RenderView smaller).

I'm also not sure what you mean should be moved?  i.e. the code that's calling these functions in the interface is deep inside WebKit, and chromium code doesn't know when these events happen (i.e. WebKit\WebCore\rendering\RenderTextControlSingleLine.cpp and WebKit\WebCore\dom\InputElement.cpp are what end up calling the text editing functions),

[Darin Fisher (:fishd, Google)]

Comment on attachment 77820 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77820&action=review

This patch is a good incremental improvement.  Long term, I would like us to move most of the auto-fill / auto-complete logic out of WebKit and instead provide lower level, generic APIs to access the information and events that we need in order to support having the implementation live in chromium.  Generic APIs are usually better since they can be used to support other features in the future.

> WebKit/chromium/public/WebAutoFillClient.h:46
> +    virtual void queryAutoFillSuggestions(const WebNode&,

nit: it would be helpful to document what the callee is supposed to do in response to this method.
i assume the embedder is supposed to call some other function to pass results back to WebKit.
(i think the method is WebView::applyAutoFillSuggestions.)

[John Abd-El-Malek]

(In reply to comment #13)
> (From update of attachment 77820 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77820&action=review
> 
> This patch is a good incremental improvement.  Long term, I would like us to move most of the auto-fill / auto-complete logic out of WebKit and instead provide lower level, generic APIs to access the information and events that we need in order to support having the implementation live in chromium.  Generic APIs are usually better since they can be used to support other features in the future.

There's a chromium bug to track this btw: http://code.google.com/p/chromium/issues/detail?id=51644
> 
> > WebKit/chromium/public/WebAutoFillClient.h:46
> > +    virtual void queryAutoFillSuggestions(const WebNode&,
> 
> nit: it would be helpful to document what the callee is supposed to do in response to this method.
> i assume the embedder is supposed to call some other function to pass results back to WebKit.
> (i think the method is WebView::applyAutoFillSuggestions.)

I actually took this function out, since it's not used anymore, but hadn't updated the patch yet.  Sorry about that.

[John Abd-El-Malek]

Committed r75146: <http://trac.webkit.org/changeset/75146>