[Webkit-unassigned] [Bug 44759] [EFL] Add custom network resource handler

[bugzilla-daemon at webkit.org]

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


Lucas De Marchi <demarchi at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #100153|review?                     |review-, commit-queue-
               Flag|                            |




--- Comment #27 from Lucas De Marchi <demarchi at webkit.org>  2011-08-29 08:15:49 PST ---
(From update of attachment 100153)
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_context.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

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