Bug 229416 – [GTK] Add webkit_web_view_try_close stay event

• Status: NEW
• Component: New Bugs

[Jan-Michael Brummer]

[GTK] Add webkit_web_view_try_close stay event

[Jan-Michael Brummer]

Created attachment 436225 [details]
Patch

[EWS Watchlist]

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

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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