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

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson 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 100128: patch
https://bugs.webkit.org/attachment.cgi?id=100128&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.