Bug 44759 – [EFL] Add custom network resource handler

• Component: New Bugs
• Status: RESOLVED FIXED

[Flavio Ceolin]

[EFL] Add custom network resource handler

[Flavio Ceolin]

Created attachment 65700 [details]
Patch

[WebKit Review Bot]

Attachment 65700 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:17:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:20:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:21:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:22:  Line contains tab character.  [whitespace/tab] [5]
WebKit/efl/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebKit/efl/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
WebKit/efl/ChangeLog:18:  Line contains tab character.  [whitespace/tab] [5]
WebKit/efl/ChangeLog:32:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 11 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 65700 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=65700&action=prettypatch

> WebCore/loader/FrameLoader.cpp:3523
> +#if PLATFORM(EFL)
> +bool FrameLoaderClient::shouldHandleScheme(const String& scheme)
> +{
> +    return false;
> +}
> +
> +void* FrameLoaderClient::handleScheme(const KURL url, String& mime, size_t* bytesRead)
> +{
> +    return 0;
> +}
> +#endif
These implementations are misplaced.  Why are you putting FrameLoaderClient impls in FrameLoader.cpp?

> WebCore/loader/FrameLoaderClient.h:271
> +#if PLATFORM(EFL)
> +        virtual bool shouldHandleScheme(const String&);
> +        virtual void* handleScheme(const KURL, String&, size_t*);
> +#endif
Why would these functions be EFL-specific?  I don't think these make sense here.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:131
> +#if PLATFORM(EFL)
> +static bool startPersonalHandler(ResourceHandle* handle, KURL url);
> +#endif
Adding EFL ifdefs to ResourceHandleSoup isn't the right approach.

[Flavio Ceolin]

(In reply to comment #3)
> (From update of attachment 65700 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=65700&action=prettypatch
> 
> > WebCore/loader/FrameLoader.cpp:3523
> > +#if PLATFORM(EFL)
> > +bool FrameLoaderClient::shouldHandleScheme(const String& scheme)
> > +{
> > +    return false;
> > +}
> > +
> > +void* FrameLoaderClient::handleScheme(const KURL url, String& mime, size_t* bytesRead)
> > +{
> > +    return 0;
> > +}
> > +#endif
> These implementations are misplaced.  Why are you putting FrameLoaderClient >impls in FrameLoader.cpp?

Because the base class FrameLoaderClient was defined in this file,  moreover there is no file named FrameLoaderClient.cpp. I can do these methods inline inside FrameLoaderClient.h, do you think is it better ?

> 
> > WebCore/loader/FrameLoaderClient.h:271
> > +#if PLATFORM(EFL)
> > +        virtual bool shouldHandleScheme(const String&);
> > +        virtual void* handleScheme(const KURL, String&, size_t*);
> > +#endif
> Why would these functions be EFL-specific?  I don't think these make sense >here.

The others ports don't need it, they can do that using their own network library

> 
> > WebCore/platform/network/soup/ResourceHandleSoup.cpp:131
> > +#if PLATFORM(EFL)
> > +static bool startPersonalHandler(ResourceHandle* handle, KURL url);
> > +#endif
> Adding EFL ifdefs to ResourceHandleSoup isn't the right approach.

Interesting, Do you think is better without ifdefs ?

Thanks for the advices.

Regards, Ceolin

[Antonio Gomes]

> Because the base class FrameLoaderClient was defined in this file,  moreover there is no file named FrameLoaderClient.cpp. I can do these methods inline inside FrameLoaderClient.h, do you think is it better ?

WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.* ?

> > > WebCore/loader/FrameLoaderClient.h:271
> > > +#if PLATFORM(EFL)
> > > +        virtual bool shouldHandleScheme(const String&);
> > > +        virtual void* handleScheme(const KURL, String&, size_t*);
> > > +#endif
> > Why would these functions be EFL-specific?  I don't think these make sense >here.
> 
> The others ports don't need it, they can do that using their own network library
> 
> > 
> > > WebCore/platform/network/soup/ResourceHandleSoup.cpp:131
> > > +#if PLATFORM(EFL)
> > > +static bool startPersonalHandler(ResourceHandle* handle, KURL url);
> > > +#endif
> > Adding EFL ifdefs to ResourceHandleSoup isn't the right approach.
> 

Personal x Custom (?)

[Flavio Ceolin]

(In reply to comment #5)
> > Because the base class FrameLoaderClient was defined in this file,  moreover there is no file named FrameLoaderClient.cpp. I can do these methods inline inside FrameLoaderClient.h, do you think is it better ?
> 
> WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.* ?
> 
If i don't implement those methods in the FrameLoader.cpp I have some undefined references as you can see below:

WebCore/libwebcore_efl.so.0.1.0: undefined reference to `WebCore::FrameLoaderClient::shouldHandleScheme(WTF::String const&)'
WebCore/libwebcore_efl.so.0.1.0: undefined reference to `WebCore::FrameLoaderClient::handleScheme(WebCore::KURL, WTF::String&, unsigned int*)'

> > > > WebCore/loader/FrameLoaderClient.h:27
> > > > +#if PLATFORM(EFL)
> > > > +        virtual bool shouldHandleScheme(const String&);
> > > > +        virtual void* handleScheme(const KURL, String&, size_t*);
> > > > +#endif
> > > Why would these functions be EFL-specific?  I don't think these make sense >here.
> > 
> > The others ports don't need it, they can do that using their own network library
> > 
> > > 
> > > > WebCore/platform/network/soup/ResourceHandleSoup.cpp:131
> > > > +#if PLATFORM(EFL)
> > > > +static bool startPersonalHandler(ResourceHandle* handle, KURL url);
> > > > +#endif
> > > Adding EFL ifdefs to ResourceHandleSoup isn't the right approach.
> > 
> 
> Personal x Custom (?)

I think "custom" is better than "personal". I'm going to change the patch.

[Flavio Ceolin]

(In reply to comment #6)
> (In reply to comment #5)
> > > Because the base class FrameLoaderClient was defined in this file,  moreover there is no file named FrameLoaderClient.cpp. I can do these methods inline inside FrameLoaderClient.h, do you think is it better ?
> > 
> > WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.* ?

If i don't implement these methods in the FrameLoader.cpp some undefined 
references occurs as you can see below:

WebCore/libwebcore_efl.so.0.1.0: undefined reference to `WebCore::FrameLoaderClient::shouldHandleScheme(WTF::String const&)'
WebCore/libwebcore_efl.so.0.1.0: undefined reference to `WebCore::FrameLoaderClient::handleScheme(WebCore::KURL, WTF::String&, unsigned int*)'
> 
> > > > > WebCore/loader/FrameLoaderClient.h:27
> > > > > +#if PLATFORM(EFL)
> > > > > +        virtual bool shouldHandleScheme(const String&);
> > > > > +        virtual void* handleScheme(const KURL, String&, size_t*);
> > > > > +#endif
> > > > Why would these functions be EFL-specific?  I don't think these make sense >here.
> > > 
> > > The others ports don't need it, they can do that using their own network library
> > > 
> > > > 
> > > > > WebCore/platform/network/soup/ResourceHandleSoup.cpp:131
> > > > > +#if PLATFORM(EFL)
> > > > > +static bool startPersonalHandler(ResourceHandle* handle, KURL url);
> > > > > +#endif
> > > > Adding EFL ifdefs to ResourceHandleSoup isn't the right approach.
> > > 
> > 
> > Personal x Custom (?)

I think "custom" is better than "personal". I'm going to change the patch.

[Flavio Ceolin]

Created attachment 66764 [details]
Patch

[Flavio Ceolin]

Please, could someone take a look in this patch.

BR, Ceolin

[Leandro Pereira]

It's been a while since this patch is awaiting review. Could someone please take a look at it? It probably needs to be rebased to current revision, but it won't change much.

[Adam Barth]

Comment on attachment 66764 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66764&action=review

> WebCore/loader/FrameLoader.cpp:3546
> +#if PLATFORM(EFL)
> +bool FrameLoaderClient::shouldHandleScheme(const String& scheme)
> +{
> +    return false;
> +}
> +
> +void* FrameLoaderClient::handleScheme(const KURL url, String& mime, size_t* bytesRead)
> +{
> +    return 0;
> +}
> +#endif

FrameLoaderClient functions should not be implemented in FrameLoader.cpp.  They should be implemented in the client.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:611
> +#if PLATFORM(EFL)
> +    if (frame->loader()->client()->shouldHandleScheme(protocol)) {
> +        if (startCustomHandler(this, url))
> +            return true;
> +    }
> +#endif

Why is this EFL specific?  The point of clients is to allow for different behavior between ports without #ifs.

[Adam Barth]

Ok, so basically you ignored my comments the first time around and I had the exact same comments looking at the patch fresh.  :(

[Flavio Ceolin]

(In reply to comment #12)
> Ok, so basically you ignored my comments the first time around and I had the exact same comments looking at the patch fresh.  :(


I answered your comments, please take a look in the comment #4.
About not implement the functions in the FrameLoader.cpp, what do you think 
is better, put these methods inline inside the FrameLoaderClient.h or just 
in the FrameLoaderClient of the port ? 

Thank you for the advice, again :)

[Adam Barth]

> I answered your comments, please take a look in the comment #4.
> About not implement the functions in the FrameLoader.cpp, what do you think 
> is better, put these methods inline inside the FrameLoaderClient.h or just 
> in the FrameLoaderClient of the port ? 

One way to answer this question is to look at how we handle all the other FrameLoaderClient methods as well as the other FooClients:

http://trac.webkit.org/browser/trunk/WebCore/loader/EmptyClients.h

[Flavio Ceolin]

(In reply to comment #14)
> > I answered your comments, please take a look in the comment #4.
> > About not implement the functions in the FrameLoader.cpp, what do you think 
> > is better, put these methods inline inside the FrameLoaderClient.h or just 
> > in the FrameLoaderClient of the port ? 
> 
> One way to answer this question is to look at how we handle all the other FrameLoaderClient methods as well as the other FooClients:
> 
> http://trac.webkit.org/browser/trunk/WebCore/loader/EmptyClients.h

Thank you, I'll see and then fix the patch.

[Flavio Ceolin]

Created attachment 96774 [details]
Patch

This new patch contains the fixes suggested in the previous comments. Although, the #if PLATFORM(EFL) in the ResourceHandleSoup.cpp persists because the FrameLoaderClient doesn't has the necessary methods (just the FrameLoaderClientEfl). Why ? Because the others ports don't need it, they can do that using their own network library, so no makes sense create the stubs for these methods in the FrameLoaderClient base class.

[Collabora GTK+ EWS bot]

Comment on attachment 96774 [details]
Patch

Attachment 96774 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8828296

[WebKit Review Bot]

Attachment 96774 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:141:  The parameter name "handle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:141:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Flavio Ceolin]

Created attachment 96789 [details]
Patch

Just fixed.

BR, Flávio Ceolin

[Martin Robinson]

This functionality is now built into libsoup. I think it would be better to use that than reimplementing it yourself. CCing Sergio who was instrumental in adding it to libsoup.

[Flavio Ceolin]

(In reply to comment #20)
> This functionality is now built into libsoup. I think it would be better to use that than reimplementing it yourself. CCing Sergio who was instrumental in adding it to libsoup.

Cool, Is it already in soup upstream ?

[Martin Robinson]

(In reply to comment #21)
> (In reply to comment #20)
> > This functionality is now built into libsoup. I think it would be better to use that than reimplementing it yourself. CCing Sergio who was instrumental in adding it to libsoup.
> 
> Cool, Is it already in soup upstream ?

Yes. You can use the SoupRequest API to do this. Take a look at EPHY_TYPE_REQUEST_ABOUT in Epiphany: 

http://git.gnome.org/browse/epiphany/tree/embed/ephy-embed-single.c#n544
http://git.gnome.org/browse/epiphany/tree/lib/ephy-request-about.c

[Flavio Ceolin]

Created attachment 100128 [details]
patch

Hi folks, 

This new patch uses the built in libsoup feature to implements the mentioned functionality.

Please, could someone review this patch.

BR, Flávio Ceolin.

[Martin Robinson]

Comment on attachment 100128 [details]
patch

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.

[Gustavo Sverzut Barbieri]

Hi Martin,

I'm the one that created this port (based on an initial try, but I almost rewrote it) and as I'm a core EFL developer, I followed EFL style for EFL related parts (ie: C parts). So Flavio is on the right path, including EFL convention of short-descriptive names if the scope is narrow (such as "ret").

I believe it is fair to have Flavio to change comments to be finished with a trailing period and to use C++ casts instead of traditional C (although I'm used to the later and so there are couple of them in our code).

The "//" in C header file won't matter as EFL uses it as well, so if one have EFL installed (required by this port) he will have a compiler that works with that ;-)

Last but not least I believe the "const char **" cast is required due: http://c-faq.com/ansi/constmismatch.html but worth checking.

All in all I'd give an informal "r+" after the minor issues are fixed (trailing ".", c++ casts)

[Flavio Ceolin]

Created attachment 100153 [details]
Patch

Thanks Martin and Gustavo,

This new patch contains the modifications suggested by you.
The code style in the efl files is mixed, in the same file you
can see more than one style. I really tried to follow the style of the file that I edited.

BR, Flavio Ceolin

[Lucas De Marchi]

Comment on attachment 100153 [details]
Patch

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

[Lucas De Marchi]

Comment on attachment 100153 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100153&action=review

>> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.h:57
>> +Eina_Bool ewk_custom_protocol_handler_soup_all_unset();
> 
> EAPI?

Disregard this comment. This header is not installed. You should make the ewk_protocol_handler.h by CMake though.

[Grzegorz Czajkowski]

Please add @brief and @file doxygen commands to ewk_protocol_handler.h after license. It's required to include documentation of the file to HTML doc.

[Flavio Ceolin]

(In reply to comment #29)
> Please add @brief and @file doxygen commands to ewk_protocol_handler.h after license. It's required to include documentation of the file to HTML doc.

Sure, I'll do that. Thanks for advice.

Flávio Ceolin

[Flavio Ceolin]

Created attachment 105627 [details]
Patch

A new patch with the suggested fixes.

[Lucas De Marchi]

Comment on attachment 105627 [details]
Patch

Looks fine now. Informal r+.

[Martin Robinson]

Comment on attachment 105627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105627&action=review

Looks good, but I have a few style nits. With another iteration and the unoffocial r+, I think this patch is fine.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:42
> +static unsigned custom_protocol_added_count = 0;

Please either use camelCase everywhere or this_thing. WebKit style is to use camelCase.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:57
> +                                                                              EwkProtocolHandlerPrivate);

The indent looks off here.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:71
> +    void* buf;

I'd prefer buffer to buf.

> Source/WebKit/efl/ewk/ewk_protocol_handler_soup.cpp:145
> +    guint protocols_size;
> +    SoupSession* session = WebCore::ResourceHandle::defaultSession();

It's odd that you use camelCase above bug something_else here.

> Source/WebKit/efl/ewk/ewk_view.cpp:94
> +    struct  {

Extra space after struct.

> Source/WebKit/efl/ewk/ewk_view.cpp:96
> +        void* ctxt;
> +        Ewk_View_Resource_Handler_Cb func;

"context" and "function" In Webkit we do not typically abbreviate variable names.

> Source/WebKit/efl/ewk/ewk_view.cpp:3618
> +Eina_Bool ewk_view_protocol_handler_set(Evas_Object* o, const char** protocols, Ewk_View_Resource_Handler_Cb handler, void* ctxt)

Ditto.

> Source/WebKit/efl/ewk/ewk_view.h:2227
> +                                             void* ctxt);

ditto.

[Flavio Ceolin]

Created attachment 107055 [details]
patch

First of all, thank you for the review.
I done all the suggested things, please take a look if it's everything ok now.

BR, Flávio Ceolin.

[WebKit Review Bot]

Comment on attachment 107055 [details]
patch

Clearing flags on attachment: 107055

Committed r94965: <http://trac.webkit.org/changeset/94965>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.