Bug 133680 – [GTK] WebKitWebView::create should receive information about the navigation action

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

In order to properly implement a popup blocker, the signal should provide information about the navigation action like the navigation type, the request, mouse button and key modifiers and whether it was triggered by a user gesture, at least.

[Carlos Garcia Campos]

Created attachment 232791 [details]
Patch

This breaks the API, as we commented in the mailing list. I hope only ephy uses this API, so it will not cause major problems for most of the users that will only have to recompile when we bump the binary version.

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[WebKit Commit Bot]

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


WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitNavigationAction.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.h"
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:74:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 25 files


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

[Martin Robinson]

Comment on attachment 232791 [details]
Patch

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

Seems okay, though I have a few  nits. I think, if possible, it'd be better to pass WebHitTestResult& everywhere instead of the internal data structure. Probably a good idea to fix up the style errors too. :)

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationActionPrivate.h:49
> +    unsigned isUserGesture : 1;

Why not use a bool here?

> Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:50
> +        WebKitNavigationAction navigation(request.get(), navigationActionData);
> +        return webkitWebViewCreateNewPage(m_webView, windowFeatures, &navigation);

Nit: Probably "action" is a better name.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:940
> +        g_cclosure_marshal_generic,

Is this really the correct marshaller for a OBJECT and an enum value?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1858
> +    // FIXME: we should use a custom ContextMenuclient at some point.

Nit: ContextMenuclient -> ContextMenuClient.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1869
> +    WebHitTestResult::Data data;
> +    data.absoluteImageURL = webHitTestResult->absoluteImageURL();
> +    data.absoluteLinkURL = webHitTestResult->absoluteLinkURL();
> +    data.absoluteMediaURL = webHitTestResult->absoluteMediaURL();
> +    data.linkLabel = webHitTestResult->linkLabel();
> +    data.linkTitle = webHitTestResult->linkTitle();
> +    data.isContentEditable = webHitTestResult->isContentEditable();
> +    data.elementBoundingBox = webHitTestResult->elementBoundingBox();
> +    data.isScrollbar = webHitTestResult->isScrollbar();
> +
> +    GRefPtr<WebKitHitTestResult> hitTestResult = adoptGRef(webkitHitTestResultCreate(data));

Wow. It'd be great to avoid all these copies! Why not just pass a const WebHitTestResult& instead of Data?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:212
> +                                                WebKitNavigationAction      *navigation);

Probably should be called action or navigation_action.

[Gustavo Noronha (kov)]

Comment on attachment 232791 [details]
Patch

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

New API looks good to me!

> Source/WebKit2/ChangeLog:10
> +        signal. In the future it could alsp be also for the

s/alsp//

[Carlos Garcia Campos]

Comment on attachment 232791 [details]
Patch

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

I'm using WebHitTestResult::Data because that's what we receive now in API:UIClient methods, WebHitTestResult is the C API wrapper, but we are not using the C API for the UI client anymore. So, instead of allocating a WebHitTestResult I prefer to use the given const reference. It's only a problem for the context menu, until we use a custom client that receives a const WebHitTestResult::Data& too. And the style issues are false positives of std::function

>> Source/WebKit2/ChangeLog:10
>> +        signal. In the future it could alsp be also for the
> 
> s/alsp//

Oops.

>> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationActionPrivate.h:49
>> +    unsigned isUserGesture : 1;
> 
> Why not use a bool here?

No reason, we can use bool as well.

>> Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:50
>> +        return webkitWebViewCreateNewPage(m_webView, windowFeatures, &navigation);
> 
> Nit: Probably "action" is a better name.

Ok.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:940
>> +        g_cclosure_marshal_generic,
> 
> Is this really the correct marshaller for a OBJECT and an enum value?

Yes, the generic marshaller can be used for any parameters

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1869
>> +    GRefPtr<WebKitHitTestResult> hitTestResult = adoptGRef(webkitHitTestResultCreate(data));
> 
> Wow. It'd be great to avoid all these copies! Why not just pass a const WebHitTestResult& instead of Data?

That's what the previous FIXME is about :-)

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:212
>> +                                                WebKitNavigationAction      *navigation);
> 
> Probably should be called action or navigation_action.

Ok.

[Carlos Garcia Campos]

Created attachment 233705 [details]
Updated patch

Fixed review comments

[WebKit Commit Bot]

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


WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitNavigationAction.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.h"
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:74:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 25 files


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

[Carlos Garcia Campos]

Created attachment 233708 [details]
Fix the build

Broke the build while fixing review comments, it should work now

[WebKit Commit Bot]

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


WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitNavigationAction.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.h"
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:74:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 25 files


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

[Martin Robinson]

(In reply to comment #10)
> Attachment 233708 [details] did not pass style-queue:
> 
> 
> WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
> WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
> WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitNavigationAction.h"
> WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.h"
> ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:63:  Extra space before ( in function call  [whitespace/parens] [4]
> ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:69:  Extra space before ( in function call  [whitespace/parens] [4]
> ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:74:  Extra space before ( in function call  [whitespace/parens] [4]
> Total errors found: 3 in 25 files

Are these false-positives?

[Carlos Garcia Campos]

(In reply to comment #11)
> (In reply to comment #10)
> > Attachment 233708 [details] [details] did not pass style-queue:
> > 
> > 
> > WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
> > WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
> > WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitNavigationAction.h"
> > WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.h"
> > ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:63:  Extra space before ( in function call  [whitespace/parens] [4]
> > ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:69:  Extra space before ( in function call  [whitespace/parens] [4]
> > ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:74:  Extra space before ( in function call  [whitespace/parens] [4]
> > Total errors found: 3 in 25 files
> 
> Are these false-positives?

Yes, the style checker thinks they are function calls, but they are std::function parameters

std::function<void ()>, std::function<void (bool) and std::function<void (const String&)>

[Carlos Garcia Campos]

Any other objection here?

[Carlos Garcia Campos]

Committed r170702: <http://trac.webkit.org/changeset/170702>