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.
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.
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
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 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 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 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.
Created attachment 233705 [details] Updated patch Fixed review comments
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.
Created attachment 233708 [details] Fix the build Broke the build while fixing review comments, it should work now
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.
(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?
(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&)>
Any other objection here?
Committed r170702: <http://trac.webkit.org/changeset/170702>