Summary: | [WK2] Add runBeforeUnloadConfirmPanel WKUIDelegate SPI; support onbeforeunload confirm panel in MiniBrowser | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||||||||||||||
Component: | WebKit2 | Assignee: | 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
Daniel Bates
2017-05-25 12:36:38 PDT
Created attachment 311266 [details]
Test case
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.
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.
Created attachment 311269 [details]
Patch and unit tests
Remote inadvertent change to LayoutTests/fast/events/before-unload-adopt-subframe-to-outside.html.
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.
The patch seems way more extensive than the bug title suggests. Do you want to break up the patch, or retitle the bug? (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. 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.
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.
(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. 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?). 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 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 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
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.
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. (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. 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. 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.
Created attachment 311396 [details]
Patch and unit tests
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 on attachment 311396 [details]
Patch and unit tests
Ahhhhhhhh so much better than the one with C-API!
Committed r217571: <http://trac.webkit.org/changeset/217571> |