Bug 172603 – [WK2] Add runBeforeUnloadConfirmPanel WKUIDelegate SPI; support onbeforeunload confirm panel in MiniBrowser

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Daniel Bates]

We should support showing an onbeforeunload confirm panel in a WebKit2 MiniBrowser window just as we do when using a WebKit1 MiniBrowser window.

[Daniel Bates]

Created attachment 311266 [details]
Test case

[Daniel Bates]

Created attachment 311267 [details]
Patch and unit tests

I deprecated the existing v5 WKPageRunBeforeUnloadConfirmPanelCallback and introduced a new callback that takes a WKSecurityOriginRef. I chose to have the new callback take a WKSecurityOriginRef as opposed to removing the WKFrameRef argument and taking a WKFrameHandleRef that represents both the WKFrameRef and WKSecurityOriginRef so as to make the arguments to the new callback more closely match the arguments taken by other JavaScript dialog callbacks (e.g. WKPageRunJavaScriptAlertCallback) and I was unclear about the future of the C API now that we have the Modern WebKit2 API (Cocoa). Let me know if it is preferred to have the new callback take a WKFrameHandleRef instead of both a WKFrameRef and WKSecurityOriginRef.

[Build Bot]

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


ERROR: Tools/MiniBrowser/mac/WK2BrowserWindowController.m:528:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/MiniBrowser/mac/WK2BrowserWindowController.m:528:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:322:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:336:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/APIUIClient.h:118:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/C/WKPage.cpp:2063:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 7 in 20 files


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

[Daniel Bates]

Created attachment 311269 [details]
Patch and unit tests

Remote inadvertent change to LayoutTests/fast/events/before-unload-adopt-subframe-to-outside.html.

[Build Bot]

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


ERROR: Tools/MiniBrowser/mac/WK2BrowserWindowController.m:528:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/MiniBrowser/mac/WK2BrowserWindowController.m:528:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:322:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:336:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/APIUIClient.h:118:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/C/WKPage.cpp:2063:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 7 in 19 files


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

[Simon Fraser (smfr)]

The patch seems way more extensive than the bug title suggests. Do you want to break up the patch, or retitle the bug?

[Daniel Bates]

(In reply to Simon Fraser (smfr) from comment #6)
> The patch seems way more extensive than the bug title suggests. Do you want
> to break up the patch, or retitle the bug?

I chose to retitle the bug as I am unclear how best to break up the patch in a meaningful way.

[Daniel Bates]

Created attachment 311298 [details]
Patch and unit tests

Updated patch to disable unit the runBeforeUnloadConfirmPanel tests on iOS as we do not have the infrasturcture to simulate a tap/click at the moment. Also, updated ChangeLog entries in the patch to reflect the updated title.

[Build Bot]

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


ERROR: Tools/MiniBrowser/mac/WK2BrowserWindowController.m:528:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/MiniBrowser/mac/WK2BrowserWindowController.m:528:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:322:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:336:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/APIUIClient.h:118:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/C/WKPage.cpp:2063:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 7 in 19 files


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

[Daniel Bates]

(In reply to Daniel Bates from comment #8)
> Updated patch to disable unit the runBeforeUnloadConfirmPanel tests on iOS
> as we do not have the infrasturcture to simulate a tap/click at the moment.

Filed bug #172614 to add such infrastructure for iOS. Will update the comment in the unit tests to refer to this bug before landing.

[Daniel Bates]

Created attachment 311334 [details]
Patch and unit tests

Updated patch to fix the GTK EWS, updated comments in unit test Tools/TestWebKitAPI/Tests/WebKit2Cocoa/ModalAlerts.mm to reference bug #172614. Removed PLATFORM(MAC)-guarded code from Tools/TestWebKitAPI/Tests/WebKit2/ModalAlertsSPI.cpp as this test does not run on iOS (since we do not enable the WebKit C SPI on iOS) and GTK does not seem to run many WebKit2 C SPI tests (why?).

[Build 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

[Build Bot]

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


ERROR: Tools/MiniBrowser/mac/WK2BrowserWindowController.m:528:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/MiniBrowser/mac/WK2BrowserWindowController.m:528:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:322:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:336:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:89:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/APIUIClient.h:118:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/C/WKPage.cpp:2063:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 8 in 20 files


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

[Brady Eidson]

Comment on attachment 311334 [details]
Patch and unit tests

Unless there's a particular need to update the C-API, we can skip it for now; The only important API going forward is Cocoa

[Daniel Bates]

Created attachment 311367 [details]
Patch and unit tests

Removed WebKit2 C SPI change as we do not have any clients that would make use of it.

[Build Bot]

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


ERROR: Tools/MiniBrowser/mac/WK2BrowserWindowController.m:528:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/MiniBrowser/mac/WK2BrowserWindowController.m:528:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:322:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:336:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:89:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/APIUIClient.h:118:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/C/WKPage.cpp:2063:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 8 in 16 files


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

Comment on attachment 311367 [details]
Patch and unit tests

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

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:73
> +- (void)webView:(WKWebView *)webView runBeforeUnloadConfirmPanelWithMessage:(NSString *)message initiatedByFrame:(WKFrameInfo *)frame completionHandler:(void (^)(BOOL result))completionHandler;

Private delegate methods should be underscore-prefixed.

This method declaration is missing an availability attribute.

[Daniel Bates]

(In reply to mitz from comment #17)
> Comment on attachment 311367 [details]
> Patch and unit tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311367&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:73
> > +- (void)webView:(WKWebView *)webView runBeforeUnloadConfirmPanelWithMessage:(NSString *)message initiatedByFrame:(WKFrameInfo *)frame completionHandler:(void (^)(BOOL result))completionHandler;
> 
> Private delegate methods should be underscore-prefixed.
> 
> This method declaration is missing an availability attribute.

You're right! Will fix.

[Daniel Bates]

Created attachment 311377 [details]
Patch and unit tests

Updated patch to prefix WKUIDelegate SPI with an underscore and add WK_API_AVAILABLE() annotation as pointed out by Dan Bernstein in comment #17.

[Build Bot]

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


ERROR: Tools/MiniBrowser/mac/WK2BrowserWindowController.m:528:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/MiniBrowser/mac/WK2BrowserWindowController.m:528:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:322:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:336:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:89:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/APIUIClient.h:118:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/C/WKPage.cpp:2063:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 8 in 16 files


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

[Daniel Bates]

Created attachment 311396 [details]
Patch and unit tests

[Build Bot]

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


ERROR: Tools/MiniBrowser/mac/WK2BrowserWindowController.m:528:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/MiniBrowser/mac/WK2BrowserWindowController.m:528:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:322:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:336:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:89:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/APIUIClient.h:118:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/C/WKPage.cpp:2063:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 8 in 16 files


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

[Brady Eidson]

Comment on attachment 311396 [details]
Patch and unit tests

Ahhhhhhhh so much better than the one with C-API!

[Radar WebKit Bug Importer]

<rdar://problem/32471306>

[Daniel Bates]

Committed r217571: <http://trac.webkit.org/changeset/217571>