| Summary: | [GTK] Add webkit_web_view_try_close stay event | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jan-Michael Brummer <jan.brummer> | ||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||
| Status: | NEW --- | ||||||
| Severity: | Normal | CC: | berto, cgarcia, ews-watchlist, gustavo, mcatanzaro | ||||
| Priority: | P2 | ||||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Jan-Michael Brummer
2021-08-23 12:37:29 PDT
Created attachment 436225 [details]
Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API Comment on attachment 436225 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436225&action=review API looks good. We can bikeshed about the naming. I think I would call it: WebKitWebView::close-rejected, UIClient::closeRejected, webkitWebViewCloseRejected. But your naming is fine too. I wonder if others have suggestions? r- for now because it needs an API test. > Source/WebKit/ChangeLog:8 > + No new tests (OOPS!). You'll need to replace this line with an explanation of why the new API is required. > Source/WebKit/UIProcess/API/APIUIClient.h:89 > + virtual void stayOnPage(WebKit::WebPageProxy*) { } Technically we need an owner to approve this line of the diff, but that shouldn't be a problem since it's only one line and obviously OK. Comment on attachment 436225 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436225&action=review >> Source/WebKit/UIProcess/API/APIUIClient.h:89 >> + virtual void stayOnPage(WebKit::WebPageProxy*) { } > > Technically we need an owner to approve this line of the diff, but that shouldn't be a problem since it's only one line and obviously OK. This name sounds like the user needs to take an action, but it's just a notification. I would use something like didNotClose, didCancelClose or something like that. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:176 > + STAY_ON_PAGE, And same for the glib api signal. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1685 > + * Emitted when try closing a #WebKitWebView is aborted. We should document here why close can fail. And you also need to update the webkit_web_view_try_close() doc that says that nothing happens when user cancels the close. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2530 > + g_signal_emit(webView, signals[STAY_ON_PAGE], 0, NULL); NULL -> nullptr |