[webkit-reviews] review denied: [Bug 44759] [EFL] Add custom network resource handler : [Attachment 100153] Patch

[bugzilla-daemon at webkit.org]

Lucas De Marchi <demarchi at webkit.org> has denied Flavio Ceolin
<flavio.ceolin at profusion.mobi>'s request for review:
Bug 44759: [EFL] Add custom network resource handler
https://bugs.webkit.org/show_bug.cgi?id=44759

Attachment 100153: Patch
https://bugs.webkit.org/attachment.cgi?id=100153&action=review

------- Additional Comments from Lucas De Marchi <demarchi at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=100153&action=review


> Source/WebKit/efl/ewk/ewk_protocol_handler.cpp:32
> +/**
> + * Register a protocol handler.
> + *
> + * @param protocols the protocols that will be handled.
> + * @return @c EINA_TRUE if success, @c EINA_FALSE if not.
> + */

Please, move this doc to the header as recently changed for the EFL port.

> Source/WebKit/efl/ewk/ewk_protocol_handler.cpp:47
> +
> +/**
> + * Remove protocol handler.
> + *
> + * @return @c EINA_TRUE if success, @c EINA_FALSE if not.
> + */

Same here.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:52
> +    } else
> +	   EINA_LOG_CRIT("Could not init custom protocol handler, priv ==
NULL.");

We usually do this with a macro finished with _OR_RETURN. This way you can
remove the checks for priv in all the other functions. If you don't want/need a
new macro, you can use EINA_SAFETY_ON_NULL_RETURN, but I'm not sure if this
check for priv being NULL is a safety check or this path is allowed.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:62
> +    EwkProtocolHandlerPrivate* priv	= G_TYPE_INSTANCE_GET_PRIVATE(obj,
EWK_TYPE_CUSTOM_PROTOCOL_HANDLER,
> +									       
 EwkProtocolHandlerPrivate);
> +    if (priv)
> +	   free(priv->mime);
> +
> +    G_OBJECT_CLASS(ewk_custom_protocol_handler_parent_class)->finalize(obj);


Do you still have to call this finalize if priv is NULL? Otherwise the previous
comment fits here too.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:67
> +    return TRUE;

What is this??

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:79
> +    void* buf = 0;
> +    char* mime = 0;
> +    size_t bytesRead = 0;
> +    SoupURI* uri = 0;
> +    EwkProtocolHandlerPrivate* priv = 0;
> +    WebCore::ResourceHandle* resource = 0;
> +    const WebCore::FrameNetworkingContextEfl* frameContext = 0;
> +    const WebCore::FrameLoaderClientEfl* frameLoaderClient = 0;

No need to initialize everything to 0. If the variable is being set afterwards
as a return of a function, initializing them here will just add noise to static
code analyzers.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:81
> +    resource =
static_cast<WebCore::ResourceHandle*>(g_object_get_data(G_OBJECT(request),
"webkit-resource"));

e.g. here.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:83
> +	   frameContext =
static_cast<WebCore::FrameNetworkingContextEfl*>(resource->getInternal()->m_con
text.get());

this is ok, but add a blank line here

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:85
> +	   frameLoaderClient =
static_cast<WebCore::FrameLoaderClientEfl*>(frameContext->coreFrame()->loader()
->client());

this is ok

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:87
> +    uri = soup_request_get_uri(request);

and here.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:90
> +    priv  =
G_TYPE_INSTANCE_GET_PRIVATE(reinterpret_cast<EwkCustomProtocolHandler*>(request
),
> +					   EWK_TYPE_CUSTOM_PROTOCOL_HANDLER,
> +					   EwkProtocolHandlerPrivate);

Couldn't you move this up and using EINA_SAFETY_ON_NULL_RETURN or the like?

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:93
> +    if (!(resource && frameContext && frameLoaderClient && priv && uri))
> +	   return 0;

Isn't this a candidate for eina safety?

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:99
> +    if (uri->path[0] == '/')
> +	   buf =
ewk_view_protocol_handler_resource_get(frameLoaderClient->webView(),
> +							&bytesRead, &mime,
uri->path + 1); // The path is always initialized with /.
> +    else
> +	   buf =
ewk_view_protocol_handler_resource_get(frameLoaderClient->webView(),
&bytesRead, &mime, uri->host);

buf doesn't need to be initialized too.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:143
> +    int protocols_size;
> +    SoupSession* session = WebCore::ResourceHandle::defaultSession();
> +    SoupSessionFeature* requester = 0;

same here

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:146
> +    if (protocols_size <= 0 || protocols[protocols_size]) // This array must
be null terminated.

g_strv_length returns guint, so it's not possible to be less than zero.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:172
> +    SoupSession* session = WebCore::ResourceHandle::defaultSession();
> +    SoupSessionFeature* requester = 0;

same here too

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.h:57
> +GType ewk_custom_protocol_handler_get_type();
> +
> +Eina_Bool ewk_custom_protocol_handler_soup_set(const char** protocols);
> +
> +Eina_Bool ewk_custom_protocol_handler_soup_all_unset();

EAPI?

> Source/WebKit/efl/ewk/ewk_view.cpp:4462
>  /**
> + * Register a new protocol handler for handling an specific protocol
(scheme).
> + *
> + * @param o view.
> + * @param protocols the protocols that will be handled.
> + * @param handler the function that will be executed for the protocols
> + * @param ctxt the handler context
> + * @return @c EINA_TRUE if success, @c EINA_FALSE if not.
> + */

Need to be moved to the header

> Source/WebKit/efl/ewk/ewk_view.cpp:4482
> +/**
> + * Remove the custom protocol handler.
> + *
> + * @param o view.
> + * @return @c EINA_TRUE if success, @c EINA_FALSE if not.
> + */

ditto