Bug 128177 – [EFL] Switch to CUSTOM_PROTOCOLS

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Carlos Garcia Campos]

There's a soup implementation already.

[Carlos Garcia Campos]

*** Bug 136705 has been marked as a duplicate of this bug. ***

[Pascal Jacquemart]

Created attachment 238932 [details]
Patch proposal to fix ewk_context_url_scheme_register() ewebkit2 API


Two remaining issues so far:

- Inside the scheme callback ewk_url_scheme_request_path_get(request) returns an empty path ?!
  > It seems I messed up something with WKURLCopyPath() and eina strings, but what?

- m_uriSchemeRequests map should be freed once the request is completed (or cancelled)
  > GTK port is holding a reference to the WebKitWebContext inside each request handle
  > Any other idea because I don't like this much?

All your comments so far would be very welcome

[Gyuyoung Kim]

Comment on attachment 238932 [details]
Patch proposal to fix ewk_context_url_scheme_register() ewebkit2 API

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

> ChangeLog:9
> +        using CustomProtocols implementation from Carlos Garcia Campos

Revision number is more helpful than patch owner name.

> Source/WebKit2/ChangeLog:9
> +        using CustomProtocols implementation from Carlos Garcia Campos

ditto.

> Source/WebKit2/PlatformEfl.cmake:229
> +    "${WEBKIT2_DIR}/Shared/Network/CustomProtocols/soup"

Wrong alphabet order.

> Source/cmake/OptionsEfl.cmake:311
> +add_definitions(-DENABLE_CUSTOM_PROTOCOLS=1)

Please add CUSTOM_PROTOCOLS definitions to WebKitFeatures.cmake.

> Tools/MiniBrowser/efl/main.c:75
> +static void about_url_scheme_request_cb(Ewk_Url_Scheme_Request *request, void *user_data)

*about* looks like a redundant prefix.

> Tools/MiniBrowser/efl/main.c:84
> +        // Todo

Use //FIXME:

> Tools/MiniBrowser/efl/main.c:87
> +        // Todo

ditto.

> Tools/MiniBrowser/efl/main.c:90
> +        // Todo

ditto.

[Pascal Jacquemart]

Comment on attachment 238932 [details]
Patch proposal to fix ewk_context_url_scheme_register() ewebkit2 API

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

>> Source/cmake/OptionsEfl.cmake:311
>> +add_definitions(-DENABLE_CUSTOM_PROTOCOLS=1)
> 
> Please add CUSTOM_PROTOCOLS definitions to WebKitFeatures.cmake.

Are you sure?
In my understanding CUSTOM_PROTOCOLS is already the default on MAC and GTK
After this patch EFL won't compile without CUSTOM_PROTOCOLS
And as soon as EFL switch to CUSTOM_PROTOCOLS the flag itself will be removed from WebKit (as being the default for all ports)

>> Tools/MiniBrowser/efl/main.c:75
>> +static void about_url_scheme_request_cb(Ewk_Url_Scheme_Request *request, void *user_data)
> 
> *about* looks like a redundant prefix.

I think it is perfectly fine
- It's actually a copy paste from "ewk_context.h" (ewk_context_url_scheme_register() API documentation)
- This "about" indicates which new scheme we are implementing (the I guess each new scheme callback would have its own prefix)
- It is consistent with GTK aboutURISchemeRequestCallback() naming

> Tools/MiniBrowser/efl/main.c:81
> +    path = ewk_url_scheme_request_path_get(request);

Now I realize this ewk_url_scheme_request_path_get() is working fine
But the test scenario itself should be considered:
  Tools/Scripts/run-launcher --efl minibrowser-about:plugins
    -> callback not called because function has_scheme() checks for "://" in the url, otherwise assume "http" scheme
  Tools/Scripts/run-launcher --efl minibrowser-about://plugins
    -> callback is called but ewk_url_scheme_request_path_get() returns an empty string, which is the expected behaviour
  Tools/Scripts/run-launcher --efl minibrowser-about:///plugins
    -> callback is called, path is "///plugins", this is correct but the subsequent tests will fail ("///" start is unexpected)

[Anton Obzhirov]

Comment on attachment 238932 [details]
Patch proposal to fix ewk_context_url_scheme_register() ewebkit2 API

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

> Source/WebKit2/UIProcess/API/efl/ewk_url_scheme_request.cpp:42
> +    WKURLRef url = toCopiedURLAPI(urlRequest->resourceRequest().url());

These 2 lines can be compressed in one and moved up - m_url(WKEinaSharedString(toCopiedURLAPI(urlRequest->resourceRequest().url())))

> Source/WebKit2/UIProcess/efl/RequestManagerClientEfl.cpp:86
> +    RefPtr<WebKitURISchemeHandler> handler = adoptRef(new WebKitURISchemeHandler(callback, userData));

I think you shouldn't pass raw pointer here. The ref pointer goes out of scope in this function.

[Pascal Jacquemart]

Created attachment 239027 [details]
 Patch proposal to fix ewk_context_url_scheme_register() ewebkit2 API (v2)

Changelog contains matching revision of original CUSTOM_PROTOCOLS implementation from Carlos + right bug number
Fixed sample use of ewk_context_url_scheme_register() within minibrowser
Fixed inefficient use of RefPtr
Fixed leaking requests

[Pascal Jacquemart]

Hi Carlos,

I am afraid we really need your feedback on this, since you implemented the original custom protocol mechanism.

Hopefully the EFL API is quite simpler in comparison to Gtk. The code is quite straightforward then. In short we can register a new scheme with ewk_context_url_scheme_register() and from the callback, we expect a single call to ewk_url_scheme_request_finish() to provide all the data. 
As far as I know, there is no way to unregister a scheme, nor to provide an error status to cancel the request (we just call finish without any data)

I am confident since the original EFL API test for ewk_context_url_scheme_register() is now working again (although the test itself remains unchanged)

Thanks,                 Pascal

[Carlos Garcia Campos]

(In reply to comment #7)
> Hi Carlos,
> 
> I am afraid we really need your feedback on this, since you implemented the original custom protocol mechanism.
> 
> Hopefully the EFL API is quite simpler in comparison to Gtk. The code is quite straightforward then. In short we can register a new scheme with ewk_context_url_scheme_register() and from the callback, we expect a single call to ewk_url_scheme_request_finish() to provide all the data. 
> As far as I know, there is no way to unregister a scheme, nor to provide an error status to cancel the request (we just call finish without any data)

There's WebSoupCustomProtocolRequestManager::didFailWithError()

> I am confident since the original EFL API test for ewk_context_url_scheme_register() is now working again (although the test itself remains unchanged)

Sounds good, you shouldn't need to change the test nor the API.

[Carlos Garcia Campos]

Comment on attachment 239027 [details]
 Patch proposal to fix ewk_context_url_scheme_register() ewebkit2 API (v2)

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

> Source/WebKit2/UIProcess/API/efl/ewk_url_scheme_request.cpp:80
> +    WebCore::ResourceResponse response(WebCore::URL(WebCore::URL(), String::fromUTF8(m_url)),
> +        String::fromUTF8(mimeType), contentLength, emptyString());
> +
> +    toImpl(m_wkRequestManager.get())->didReceiveResponse(m_requestID, response);
> +    toImpl(m_wkRequestManager.get())->didLoadData(m_requestID, toImpl(wkData.get()));
> +    toImpl(m_wkRequestManager.get())->didFinishLoading(m_requestID);
> +    toImpl(m_wkRequestManager.get())->stopLoading(m_requestID);

So, you send the whole data in a single chunk to the networking process?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:119
>      ewk_context_url_scheme_register(ewk_view_context_get(webView()), "fooscheme", schemeRequestCallback, 0);

0 -> nullptr

> Source/WebKit2/UIProcess/Network/CustomProtocols/soup/WebSoupCustomProtocolRequestManager.cpp:36
>  #endif
>  
> +#if PLATFORM(EFL)

#elif ?

> Source/WebKit2/UIProcess/efl/RequestManagerClientEfl.h:45
> +        : m_callback(0)
> +        , m_userData(0)

nullptr

> Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:38
> +#if ENABLE(CUSTOM_PROTOCOLS)
> +#include "WebSoupCustomProtocolRequestManager.h"
> +#else
>  #include "WebSoupRequestManagerProxy.h"
> +#endif

Is this really possible? You removed WebSoupRequestManagerProxy from the makefile no?

> Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:100
> +#if ENABLE(CUSTOM_PROTOCOLS)
> +    parameters.urlSchemesRegisteredForCustomProtocols = supplement<WebSoupCustomProtocolRequestManager>()->registeredSchemesForCustomProtocols();
> +#else
>      parameters.urlSchemesRegistered = supplement<WebSoupRequestManagerProxy>()->registeredURISchemes();
> +#endif

Ditto.

> Source/WebKit2/UIProcess/soup/WebContextSoup.cpp:46
> +#else
> +    parameters.urlSchemesRegistered = supplement<WebSoupRequestManagerProxy>()->registeredURISchemes();

Ditto.

[Carlos Garcia Campos]

(In reply to comment #7)
> Hopefully the EFL API is quite simpler in comparison to Gtk. The code is quite straightforward then. In short we can register a new scheme with ewk_context_url_scheme_register() and from the callback, we expect a single call to ewk_url_scheme_request_finish() to provide all the data. 
> As far as I know, there is no way to unregister a scheme, nor to provide an error status to cancel the request (we just call finish without any data)

CustomProtocolManager::unregisterScheme() is currently unimplemented, feel free to implement it if you need it.

[Pascal Jacquemart]

Comment on attachment 239027 [details]
 Patch proposal to fix ewk_context_url_scheme_register() ewebkit2 API (v2)

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

>> Source/WebKit2/UIProcess/API/efl/ewk_url_scheme_request.cpp:80
>> +    toImpl(m_wkRequestManager.get())->stopLoading(m_requestID);
> 
> So, you send the whole data in a single chunk to the networking process?

Yes! Because EFL only have this ewk_url_scheme_request_finish() API to feed the data. Only once then but still it could be done asynchronously
The funny thing here is to call stopLoading() after didFinishLoading() but this is the simplest way I found to actually remove the requests from the RequestManagerClientEfl's HashMap. In this case the stopLoading callback is fired and I can proceed with the clean-up

>> Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:38
>> +#endif
> 
> Is this really possible? You removed WebSoupRequestManagerProxy from the makefile no?

Yes I know it's a step forward with no real return, let's make the clean-up for these files

[Pascal Jacquemart]

Created attachment 239030 [details]
After Carlos review

Thanks Carlos for the feedback
Now I think I would let an EFL guru to land the patch

[Gyuyoung Kim]

Comment on attachment 239030 [details]
After Carlos review

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

> Source/WebKit2/UIProcess/efl/RequestManagerClientEfl.h:47
> +    }

Add a new line.

> Tools/MiniBrowser/efl/main.c:75
> +static void about_url_scheme_request_cb(Ewk_Url_Scheme_Request *request, void *user_data)

> I think it is perfectly fine
> - It's actually a copy paste from "ewk_context.h" (ewk_context_url_scheme_register() API documentation)
> - This "about" indicates which new scheme we are implementing (the I guess each new scheme callback would have its own prefix)
> - It is consistent with GTK aboutURISchemeRequestCallback() naming

EFL MiniBrowser is EFL simple application, not WebKit library. So we have used EFL coding style and naming so far. If you see other functions in main.c, you can find EFL style naming. If this is WebKit side implementation, I agree with your opinion. However, MiniBrowser is EFL simple application. But, in this case, it looks not critical. I'm OK, if you want to keep it.

> Tools/MiniBrowser/efl/main.c:83
> +        // FIXME:

Unneeded line break. Below is enough.

// FIXME: Initialize contents bla bar..

> Tools/MiniBrowser/efl/main.c:2382
> +    ewk_context_url_scheme_register(context, MINIBROWSER_ABOUT_SCHEME, about_url_scheme_request_cb, NULL);

Should you call this function here ? Basically we have initialized ewk APIs in window_create().

[Gyuyoung Kim]

One more thing. Set obsolete to expired your patches.

[Pascal Jacquemart]

Comment on attachment 239030 [details]
After Carlos review

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

>> Tools/MiniBrowser/efl/main.c:75
>> +static void about_url_scheme_request_cb(Ewk_Url_Scheme_Request *request, void *user_data)
> 
> 

Well finally I think I will drop these changes in MiniBrowser
In fact it has been really useful for tests purposes but:
- I now realize won't be able to retrieve this kind of data anyway (about memory, plugins, ...)
- We cannot really provide WebKit version either as per GTK
- I would try to avoid leaving some FIXME in my patches

[Pascal Jacquemart]

Created attachment 239101 [details]
After Gyuyoung review

Dropped the changes in EFL Minibrowser because they won't bring any useful feature after all
Updated the sample code documenting ewk_context_url_scheme_register() API because the "about" scheme is probably the only scheme that won't actually work from here (as conflicting with "about:blank")

[Pascal Jacquemart]

Created attachment 239104 [details]
 After Gyuyoung review (v2)

Sorry for the spam
Oddly previous patch was removing a blank line in ewk_context_private.h whereas this file should remain unchanged

[Pascal Jacquemart]

Created attachment 239211 [details]
After Gyuyoung review (v3)

Cleanup in RequestManagerClientEfl.h

[Gyuyoung Kim]

Comment on attachment 239211 [details]
After Gyuyoung review (v3)

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

LGTM. I think GTK port already uses CUSTOM_PROTOCOLS on soup backend by default. EFL port needs to follow it as well. However, someone might want to have final look before landing.

> Source/WebKit2/PlatformEfl.cmake:-172
> -    UIProcess/soup/WebSoupRequestManagerClient.cpp

If EFL port doesn't use it as well, I think nobody uses this file, right ? Then we have to remove it.

> Source/WebKit2/PlatformEfl.cmake:-173
> -    UIProcess/soup/WebSoupRequestManagerProxy.cpp

ditto.

> Source/WebKit2/PlatformEfl.cmake:-206
> -    WebProcess/soup/WebSoupRequestManager.cpp

ditto.

> Source/WebKit2/PlatformEfl.cmake:-212
> -    UIProcess/soup/WebSoupRequestManagerProxy.messages.in

ditto.

> Source/WebKit2/PlatformEfl.cmake:-215
> -

ditto.

[Gyuyoung Kim]

Comment on attachment 239211 [details]
After Gyuyoung review (v3)

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

> Source/WebKit2/UIProcess/efl/RequestManagerClientEfl.cpp:69
> +void RequestManagerClientEfl::stopLoading(WKSoupCustomProtocolRequestManagerRef manager, uint64_t customProtocolID, const void* clientInfo)

nit: *manager* is not used here. It will cause unused param build warning.

[Carlos Garcia Campos]

(In reply to comment #19)
> (From update of attachment 239211 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239211&action=review
> 
> LGTM. I think GTK port already uses CUSTOM_PROTOCOLS on soup backend by default. EFL port needs to follow it as well. However, someone might want to have final look before landing.
> 
> > Source/WebKit2/PlatformEfl.cmake:-172
> > -    UIProcess/soup/WebSoupRequestManagerClient.cpp
> 
> If EFL port doesn't use it as well, I think nobody uses this file, right ? Then we have to remove it.

Yes, see https://bugs.webkit.org/show_bug.cgi?id=128169

[Gyuyoung Kim]

Comment on attachment 239211 [details]
After Gyuyoung review (v3)

It looks any EFL folk isn't interested in this change now. cq=me as well.

[Gyuyoung Kim]

Comment on attachment 239211 [details]
After Gyuyoung review (v3)

Oops. Need to fix unused param first. Please fix it.

[Pascal Jacquemart]

Created attachment 239401 [details]
After final review

Fixed unused parameter in RequestManagerClientEfl::stopLoading() + Cleanup empty destructors

I agree to proceed with the cleanup for no more used files (Bug 128169)
Also I wonder if the flag CUSTOM_PROTOCOLS could be fully removed?

[WebKit Commit Bot]

Comment on attachment 239401 [details]
After final review

Clearing flags on attachment: 239401

Committed r174419: <http://trac.webkit.org/changeset/174419>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.