[webkit-reviews] review denied: [Bug 94315] [GTK] Add destroy notify for register_uri_scheme : [Attachment 159050] Add destroy notify

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied  review:
Bug 94315: [GTK] Add destroy notify for register_uri_scheme
https://bugs.webkit.org/show_bug.cgi?id=94315

Attachment 159050: Add destroy notify
https://bugs.webkit.org/attachment.cgi?id=159050&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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(reque
st));
> +    WebKitURISchemeHandler const &handler =
context->priv->uriSchemeHandlers.get(webkit_uri_scheme_request_get_scheme(reque
st));

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.