Bug 172603

Summary: [WK2] Add runBeforeUnloadConfirmPanel WKUIDelegate SPI; support onbeforeunload confirm panel in MiniBrowser
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebKit2Assignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, andersca, beidson, berto, buildbot, cgarcia, gustavo, mcatanzaro, mitz, sam, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Mac   
OS: All   
Attachments:
Description Flags
Test case
none
Patch and unit tests
none
Patch and unit tests
none
Patch and unit tests
none
Patch and unit tests
none
Patch and unit tests
none
Patch and unit tests
none
Patch and unit tests beidson: review+

Description Daniel Bates 2017-05-25 12:36:38 PDT
We should support showing an onbeforeunload confirm panel in a WebKit2 MiniBrowser window just as we do when using a WebKit1 MiniBrowser window.
Comment 1 Daniel Bates 2017-05-25 12:44:25 PDT
Created attachment 311266 [details]
Test case
Comment 2 Daniel Bates 2017-05-25 12:53:17 PDT
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.
Comment 3 Build Bot 2017-05-25 12:54:27 PDT
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.
Comment 4 Daniel Bates 2017-05-25 12:55:41 PDT
Created attachment 311269 [details]
Patch and unit tests

Remote inadvertent change to LayoutTests/fast/events/before-unload-adopt-subframe-to-outside.html.
Comment 5 Build Bot 2017-05-25 12:57:17 PDT
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.
Comment 6 Simon Fraser (smfr) 2017-05-25 13:20:06 PDT
The patch seems way more extensive than the bug title suggests. Do you want to break up the patch, or retitle the bug?
Comment 7 Daniel Bates 2017-05-25 14:56:49 PDT
(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.
Comment 8 Daniel Bates 2017-05-25 14:59:37 PDT
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.
Comment 9 Build Bot 2017-05-25 15:02:12 PDT
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.
Comment 10 Daniel Bates 2017-05-25 15:07:12 PDT
(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.
Comment 11 Daniel Bates 2017-05-25 21:26:42 PDT
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?).
Comment 12 Build Bot 2017-05-25 21:29:18 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 13 Build Bot 2017-05-25 21:29:32 PDT
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.
Comment 14 Brady Eidson 2017-05-26 10:04:10 PDT
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
Comment 15 Daniel Bates 2017-05-26 12:59:02 PDT
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.
Comment 16 Build Bot 2017-05-26 13:00:37 PDT
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 17 mitz 2017-05-26 13:23:37 PDT
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.
Comment 18 Daniel Bates 2017-05-26 14:12:09 PDT
(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.
Comment 19 Daniel Bates 2017-05-26 14:43:26 PDT
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.
Comment 20 Build Bot 2017-05-26 14:45:14 PDT
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.
Comment 21 Daniel Bates 2017-05-26 16:46:52 PDT
Created attachment 311396 [details]
Patch and unit tests
Comment 22 Build Bot 2017-05-26 16:49:49 PDT
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.
Comment 23 Brady Eidson 2017-05-26 20:08:05 PDT
Comment on attachment 311396 [details]
Patch and unit tests

Ahhhhhhhh so much better than the one with C-API!
Comment 24 Radar WebKit Bug Importer 2017-05-30 13:59:56 PDT
<rdar://problem/32471306>
Comment 25 Daniel Bates 2017-05-30 14:02:17 PDT
Committed r217571: <http://trac.webkit.org/changeset/217571>