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

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #100128|review?                     |review-
               Flag|                            |




--- Comment #24 from Martin Robinson <mrobinson at webkit.org>  2011-07-08 11:33:10 PST ---
(From update of attachment 100128)
View in context: https://bugs.webkit.org/attachment.cgi?id=100128&action=review

I like this approach a lot better. Still I'm concerned at the mix of WebKit and non-WebKit coding style in the new code. Why isn't the same coding style used everywhere? What's the EFL policy for WebKit layer files? I'd prefer that we used the same style everywhere in WebKit.

I can't comment on the API design decision (an EFL person should sign off on that), but they seem sane enough.

> Source/WebKit/efl/ewk/ewk_protocol_handler.h:37
> +#endif // ewk_protocol_handler_h

I find it really odd that this file is written specifically for C, but has a C++ style comment. Won't that break in a strict C compiler?

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:34
> +#include "FrameLoaderClientEfl.h"
> +#include "FrameNetworkingContextEfl.h"
> +#include "ResourceHandle.h"
> +#include "ResourceHandleClient.h"
> +#include "ResourceHandleInternal.h"
> +
> +#include "ewk_private.h"
> +
> +#include <glib-object.h>
> +#include <glib.h>
> +#include <libsoup/soup-requester.h>
> +#include <libsoup/soup.h>

This block of includes does not follow typical WebKit style.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:44
> +static int added_count = 0;

I think this this variable name should more specifically describe what it is counting.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:74
> +{
> +

Extra newline here.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:82
> +    const WebCore::FrameLoaderClientEfl* frameLoaderClient = 0;

There's no need to declare these variables before they are used.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:100
> +                                                     &bytesRead, &mime, uri->path + 1); // The path always init with /

init -> "initialized with" This comment is missing a period.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:113
> +    EwkProtocolHandlerPrivate* priv  = G_TYPE_INSTANCE_GET_PRIVATE((EwkCustomProtocolHandler *)request,

Please use a C++ or GObject style cast here.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:121
> +    EwkProtocolHandlerPrivate* priv  = G_TYPE_INSTANCE_GET_PRIVATE((EwkCustomProtocolHandler *)request,

Ditto.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:129
> +    GObjectClass* gobject_class = G_OBJECT_CLASS(custom_protocol_handler_class);

This variable name would be gobjectClass if you were following WebKit style. Why aren't you following WebKit-style in this patch?

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:133
> +    request_class->schemes = (const char**)schemes;

I'm surprised this cast is necessary.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:148
> +    protocols_size = g_strv_length((gchar**)protocols);

Please use a C++ style cast here.

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

Should read "This array must be null terminated."

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:164
> +    schemes = (char**)g_strdupv((gchar**)protocols);

Please use a C++ style cast when casting protocols. The return value of g_strdupv does not need to be cast.

> Source/WebKit/efl/ewk/ewk_view.cpp:4487
> +    Eina_Bool ret = ewk_custom_protocol_handler_all_unset();

Please use full words for variable names.

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