[Webkit-unassigned] [Bug 94315] [GTK] Add destroy notify for register_uri_scheme

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=94315


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #159050|                            |review-
               Flag|                            |




--- Comment #2 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-08-17 02:53:37 PST ---
(From update of attachment 159050)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.