Bug 128177 - [EFL] Switch to CUSTOM_PROTOCOLS
Summary: [EFL] Switch to CUSTOM_PROTOCOLS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 136705 (view as bug list)
Depends on:
Blocks: 128169
  Show dependency treegraph
 
Reported: 2014-02-04 05:25 PST by Carlos Garcia Campos
Modified: 2014-10-07 23:37 PDT (History)
8 users (show)

See Also:


Attachments
Patch proposal to fix ewk_context_url_scheme_register() ewebkit2 API (25.51 KB, patch)
2014-09-30 09:53 PDT, Pascal Jacquemart
no flags Details | Formatted Diff | Diff
Patch proposal to fix ewk_context_url_scheme_register() ewebkit2 API (v2) (25.45 KB, patch)
2014-10-01 08:00 PDT, Pascal Jacquemart
cgarcia: commit-queue-
Details | Formatted Diff | Diff
After Carlos review (25.50 KB, patch)
2014-10-01 09:47 PDT, Pascal Jacquemart
no flags Details | Formatted Diff | Diff
After Gyuyoung review (24.17 KB, patch)
2014-10-02 03:59 PDT, Pascal Jacquemart
no flags Details | Formatted Diff | Diff
After Gyuyoung review (v2) (23.50 KB, patch)
2014-10-02 05:46 PDT, Pascal Jacquemart
no flags Details | Formatted Diff | Diff
After Gyuyoung review (v3) (23.33 KB, patch)
2014-10-03 10:38 PDT, Pascal Jacquemart
gyuyoung.kim: review+
Details | Formatted Diff | Diff
After final review (23.35 KB, patch)
2014-10-07 01:43 PDT, Pascal Jacquemart
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2014-02-04 05:25:19 PST
There's a soup implementation already.
Comment 1 Carlos Garcia Campos 2014-09-11 00:02:43 PDT
*** Bug 136705 has been marked as a duplicate of this bug. ***
Comment 2 Pascal Jacquemart 2014-09-30 09:53:18 PDT
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
Comment 3 Gyuyoung Kim 2014-09-30 17:16:08 PDT
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.
Comment 4 Pascal Jacquemart 2014-10-01 03:57:59 PDT
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)
Comment 5 Anton Obzhirov 2014-10-01 07:44:53 PDT
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.
Comment 6 Pascal Jacquemart 2014-10-01 08:00:43 PDT
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
Comment 7 Pascal Jacquemart 2014-10-01 08:37:19 PDT
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
Comment 8 Carlos Garcia Campos 2014-10-01 08:43:01 PDT
(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.
Comment 9 Carlos Garcia Campos 2014-10-01 09:01:56 PDT
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.
Comment 10 Carlos Garcia Campos 2014-10-01 09:04:20 PDT
(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.
Comment 11 Pascal Jacquemart 2014-10-01 09:38:56 PDT
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
Comment 12 Pascal Jacquemart 2014-10-01 09:47:40 PDT
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
Comment 13 Gyuyoung Kim 2014-10-01 17:28:45 PDT
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().
Comment 14 Gyuyoung Kim 2014-10-01 17:36:08 PDT
One more thing. Set obsolete to expired your patches.
Comment 15 Pascal Jacquemart 2014-10-02 03:47:33 PDT
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
Comment 16 Pascal Jacquemart 2014-10-02 03:59:36 PDT
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")
Comment 17 Pascal Jacquemart 2014-10-02 05:46:03 PDT
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
Comment 18 Pascal Jacquemart 2014-10-03 10:38:27 PDT
Created attachment 239211 [details]
After Gyuyoung review (v3)

Cleanup in RequestManagerClientEfl.h
Comment 19 Gyuyoung Kim 2014-10-06 22:56:27 PDT
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.
Comment 20 Gyuyoung Kim 2014-10-06 23:01:29 PDT
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.
Comment 21 Carlos Garcia Campos 2014-10-06 23:12:12 PDT
(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
Comment 22 Gyuyoung Kim 2014-10-06 23:19:05 PDT
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.
Comment 23 Gyuyoung Kim 2014-10-06 23:20:22 PDT
Comment on attachment 239211 [details]
After Gyuyoung review (v3)

Oops. Need to fix unused param first. Please fix it.
Comment 24 Pascal Jacquemart 2014-10-07 01:43:18 PDT
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?
Comment 25 WebKit Commit Bot 2014-10-07 23:37:41 PDT
Comment on attachment 239401 [details]
After final review

Clearing flags on attachment: 239401

Committed r174419: <http://trac.webkit.org/changeset/174419>
Comment 26 WebKit Commit Bot 2014-10-07 23:37:49 PDT
All reviewed patches have been landed.  Closing bug.