Bug 94315 – [GTK] Add destroy notify for register_uri_scheme

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Jesse van den Kieboom]

For introspection to work correctly, a destroy notify needs to be added to register_uri_scheme so that bindings know when to finalize the userdata. I've added a patch which adds the destroy notify and calls it when the uri handler is finalized. I really don't know much about webkit so it might well be that this doesn't work correctly (I don't know if those objects in HashMap get copied or not), but it's a start :)

[Jesse van den Kieboom]

Created attachment 159050 [details]
Add destroy notify

[Carlos Garcia Campos]

Comment on attachment 159050 [details]
Add destroy notify

View in context: https://bugs.webkit.org/attachment.cgi?id=159050&action=review

Thank you very much for the patch. Please read this http://www.webkit.org/coding/contributing.html Patches should include a ChangeLog and you should set the r flag to ? to ask for a review.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:68
> +        if (destroyNotify)
> +        {
> +            destroyNotify(userData);
> +        }

Don't see use braces for one-line ifs, see http://www.webkit.org/coding/coding-style.html

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:394
> + * @destroy_notify: destroy notify for @user_data

I prefer to call this something like user_data_destroy_func

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:568
> -    WebKitURISchemeHandler handler = context->priv->uriSchemeHandlers.get(webkit_uri_scheme_request_get_scheme(request));
> +    WebKitURISchemeHandler const &handler = context->priv->uriSchemeHandlers.get(webkit_uri_scheme_request_get_scheme(request));

Unfortunately this is not enough, when the handler is set to the HashMap it's also copied, so the destructor will be called in that case too. So, we have several options here:

 a) Make WebKitURISchemeHandler a refcounted class instead of a struct. 
 b) Ierate all handlers in webkitWebContextFinalize to call destroy_notify function

probably a) is easier, you need to convert it into a class deriving from RefCounted

class WebKitURISchemeHandler: public RefCounted<WebKitURISchemeHandler> {

and then change the HashMap definition to:

typedef HashMap<String, RefPtr<WebKitURISchemeHandler> > URISchemeHandlerMap;

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:165
> -        webkit_web_context_register_uri_scheme(webkit_web_context_get_default(), scheme, uriSchemeRequestCallback, this);
> +        webkit_web_context_register_uri_scheme(webkit_web_context_get_default(), scheme, uriSchemeRequestCallback, this, 0);

Ideally the destroy notify should also be tested in the unit tests, but in this case it's not so easy, because the web context is destroyed when all tests have finished.

[Carlos Garcia Campos]

Jesse, are you working on a new patch for this? Since it's an api change I would like to land this patch before the release.

[Carlos Garcia Campos]

Created attachment 159644 [details]
Updated patch

[WebKit Review Bot]

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

[Philippe Normand]

Comment on attachment 159644 [details]
Updated patch

LGTM

[Philippe Normand]

Comment on attachment 159644 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159644&action=review

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:168
> -        webkit_web_context_register_uri_scheme(webkit_web_context_get_default(), scheme, uriSchemeRequestCallback, this);
> +        webkit_web_context_register_uri_scheme(webkit_web_context_get_default(), scheme, uriSchemeRequestCallback, this, 0);

I think Carlos wanted to have the test checking the destroy notify callback.

[Carlos Garcia Campos]

(In reply to comment #7)
> (From update of attachment 159644 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159644&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:168
> > -        webkit_web_context_register_uri_scheme(webkit_web_context_get_default(), scheme, uriSchemeRequestCallback, this);
> > +        webkit_web_context_register_uri_scheme(webkit_web_context_get_default(), scheme, uriSchemeRequestCallback, this, 0);
> 
> I think Carlos wanted to have the test checking the destroy notify callback.

Yes, but unfortunately it's not possible:

"Ideally the destroy notify should also be tested in the unit tests, but in this case it's not so easy, because the web context is destroyed when all tests have finished."

[Philippe Normand]

Comment on attachment 159644 [details]
Updated patch

Please wait EWS before landing :)

[Carlos Garcia Campos]

(In reply to comment #9)
> (From update of attachment 159644 [details])
> Please wait EWS before landing :)

Sure, thanks reviewing!

[Carlos Garcia Campos]

Committed r126152: <http://trac.webkit.org/changeset/126152>