Bug 52169 – [chromium] Call WebAutoFillClient instead of WebViewClient for autofill related callbacks

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

[John Abd-El-Malek]

[chromium] Call WebAutoFillClient instead of WebViewClient for autofill related callbacks

[John Abd-El-Malek]

Created attachment 78442 [details]
Patch

[WebKit Review Bot]

Attachment 78442 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit/chromium/ChangeLog', u'WebKit/chromium/public/WebView.h', u'WebKit/chromium/public/WebViewClient.h', u'WebKit/chromium/src/AutoFillPopupMenuClient.cpp', u'WebKit/chromium/src/EditorClientImpl.cpp', u'WebKit/chromium/src/WebViewImpl.cpp', u'WebKit/chromium/src/WebViewImpl.h', u'WebKit/chromium/src/WebWorkerBase.cpp']" exit_code: 1
WebKit/chromium/src/WebViewImpl.h:370:  The parameter name "client" adds no information, so it should be removed.  [readability/parameter_name] [5]
WebKit/chromium/src/WebViewImpl.h:372:  The parameter name "autoFillClient" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Abd-El-Malek]

hey guys, this is part of the autofill cleanup in RenderView.  It does touch one function in the WebKit API (WebView::create), but I don't think it needs Darin's approval since he had already approved the interface that's used there.

[WebKit Review Bot]

Attachment 78442 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7410129

[John Abd-El-Malek]

(In reply to comment #4)
> Attachment 78442 [details] did not build on chromium:
> Build output: http://queues.webkit.org/results/7410129

build failure is because it's using an old version of Chrome.  that specific code now has an ifdef for WEBKIT_HAS_WEB_AUTO_FILL_CLIENT

[James Robinson]

It's using the version specified in WebKit/chromium/DEPS.  You'll have to roll that as well in order to keep the build.webkit.org bots compiling.

[John Abd-El-Malek]

(In reply to comment #6)
> It's using the version specified in WebKit/chromium/DEPS.  You'll have to roll that as well in order to keep the build.webkit.org bots compiling.

thanks, I didn't know about that. updating

[James Robinson]

Comment on attachment 78442 [details]
Patch

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

Please update the chromium_rev line in WebKit/chromium/DEPS so the builds don't break.

> WebKit/chromium/src/WebViewImpl.h:372
> +    WebViewImpl(WebViewClient* client,
> +                WebDevToolsAgentClient* devToolsClient,
> +                WebAutoFillClient* autoFillClient);

WebKit style is to leave out the parameter names for params like this.

[John Abd-El-Malek]

Comment on attachment 78442 [details]
Patch

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

>> WebKit/chromium/src/WebViewImpl.h:372
>> +                WebAutoFillClient* autoFillClient);
> 
> WebKit style is to leave out the parameter names for params like this.

right, I was trying to be consistent with the two other existing parameters though.  Should I change them as well?

[James Robinson]

(In reply to comment #9)
> (From update of attachment 78442 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78442&action=review
> 
> >> WebKit/chromium/src/WebViewImpl.h:372
> >> +                WebAutoFillClient* autoFillClient);
> > 
> > WebKit style is to leave out the parameter names for params like this.
> 
> right, I was trying to be consistent with the two other existing parameters though.  Should I change them as well?

Yeah - nuke 'em all so the style checker stays quiet.

[John Abd-El-Malek]

Created attachment 78449 [details]
Patch

[WebKit Review Bot]

Attachment 78449 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7384102

[James Robinson]

Comment on attachment 78449 [details]
Patch

Looks like you have to edit TestShell.cpp as well.

[John Abd-El-Malek]

(In reply to comment #13)
> (From update of attachment 78449 [details])
> Looks like you have to edit TestShell.cpp as well.

yep, on it.  I didn't know that many places used WebView::create, including some more in Chrome :)

[John Abd-El-Malek]

Created attachment 78452 [details]
Patch

[WebKit Review Bot]

Attachment 78452 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7374117

[John Abd-El-Malek]

(In reply to comment #16)
> Attachment 78452 [details] did not build on chromium:
> Build output: http://queues.webkit.org/results/7374117

this one is not my fault! looks like a buildbot error

[John Abd-El-Malek]

Created attachment 78464 [details]
Patch

[John Abd-El-Malek]

(In reply to comment #17)
> (In reply to comment #16)
> > Attachment 78452 [details] [details] did not build on chromium:
> > Build output: http://queues.webkit.org/results/7374117
> 
> this one is not my fault! looks like a buildbot error

James explained the problem, fixed

[James Robinson]

I think this looks all right, gonna wait for the cr-linux bot to cycle just to be sure ui.gyp doesn't have any other nasty dependencies.

[James Robinson]

Comment on attachment 78464 [details]
Patch

R=me

[John Abd-El-Malek]

Committed r75448: <http://trac.webkit.org/changeset/75448>