Bug 128177

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

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 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 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 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.

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

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

(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 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.

(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 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

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 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().

One more thing. Set obsolete to expired your patches.

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

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")

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

Created attachment 239211 [details] After Gyuyoung review (v3) Cleanup in RequestManagerClientEfl.h

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 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.

(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 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 on attachment 239211 [details] After Gyuyoung review (v3) Oops. Need to fix unused param first. Please fix it.

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 on attachment 239401 [details] After final review Clearing flags on attachment: 239401 Committed r174419: <http://trac.webkit.org/changeset/174419>

All reviewed patches have been landed. Closing bug.