Bug 133680

Summary: [GTK] WebKitWebView::create should receive information about the navigation action
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, bunhere, cdumez, commit-queue, gustavo, gyuyoung.kim, mrobinson, rakuco, sergio
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 133724    
Attachments:
Description Flags
Patch
none
Updated patch
none
Fix the build mrobinson: review+

Description Carlos Garcia Campos 2014-06-10 09:36:28 PDT
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.
Comment 1 Carlos Garcia Campos 2014-06-10 09:53:14 PDT
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.
Comment 2 WebKit Commit Bot 2014-06-10 09:53:48 PDT
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
Comment 3 WebKit Commit Bot 2014-06-10 09:53:59 PDT
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.
Comment 4 Martin Robinson 2014-06-23 09:32:59 PDT
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.
Comment 5 Gustavo Noronha (kov) 2014-06-23 10:46:02 PDT
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//
Comment 6 Carlos Garcia Campos 2014-06-24 06:54:14 PDT
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.
Comment 7 Carlos Garcia Campos 2014-06-24 08:13:13 PDT
Created attachment 233705 [details]
Updated patch

Fixed review comments
Comment 8 WebKit Commit Bot 2014-06-24 08:14:05 PDT
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.
Comment 9 Carlos Garcia Campos 2014-06-24 08:43:31 PDT
Created attachment 233708 [details]
Fix the build

Broke the build while fixing review comments, it should work now
Comment 10 WebKit Commit Bot 2014-06-24 08:45:59 PDT
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.
Comment 11 Martin Robinson 2014-06-24 09:06:39 PDT
(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?
Comment 12 Carlos Garcia Campos 2014-06-24 10:52:06 PDT
(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&)>
Comment 13 Carlos Garcia Campos 2014-06-27 09:17:45 PDT
Any other objection here?
Comment 14 Carlos Garcia Campos 2014-07-02 00:20:19 PDT
Committed r170702: <http://trac.webkit.org/changeset/170702>