Bug 229416 - [GTK] Add webkit_web_view_try_close stay event
Summary: [GTK] Add webkit_web_view_try_close stay event
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-08-23 12:37 PDT by Jan-Michael Brummer
Modified: 2021-08-23 23:39 PDT (History)
5 users (show)

See Also:


Attachments
Patch (6.38 KB, patch)
2021-08-23 12:37 PDT, Jan-Michael Brummer
mcatanzaro: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan-Michael Brummer 2021-08-23 12:37:29 PDT
[GTK] Add webkit_web_view_try_close stay event
Comment 1 Jan-Michael Brummer 2021-08-23 12:37:56 PDT
Created attachment 436225 [details]
Patch
Comment 2 EWS Watchlist 2021-08-23 12:38:43 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 https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Michael Catanzaro 2021-08-23 12:50:24 PDT
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 4 Carlos Garcia Campos 2021-08-23 23:39:42 PDT
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