Bug 92749

Created attachment 156332 [details] preliminary patch (do not set any flags) Register Protocol Handler implementation proposal

Created attachment 157168 [details] patch

Comment on attachment 157168 [details] patch Attachment 157168 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13458218

Comment on attachment 157168 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=157168&action=review > Source/WebKit2/UIProcess/WebRegisterProtocolHandlerProxy.messages.in:33 > +#endif Missing // ENABLE(REGISTER_PROTOCOL_HANDLER). > Source/WebKit2/WebProcess/WebCoreSupport/WebRegisterProtocolHandlerClient.cpp:69 > + Nit : unneeded line. > Source/WebKit2/WebProcess/WebCoreSupport/WebRegisterProtocolHandlerClient.cpp:70 > +#endif // ENABLE(CUSTOM_SCHEME_HANDLER) Nit : It looks an empty line is needed.

Created attachment 157215 [details] patch v2 Fixed build failure on GTK. Gyuyoung, thanks for review!

Comment on attachment 157215 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=157215&action=review Looks like you're missing the integration with WebContext. You need to add some code in WebContext.cpp and WebContext.h to instantiate your new proxy and properly direct messages to it. > Source/WebKit2/UIProcess/API/C/WKRegisterProtocolHandler.cpp:29 > +#include "WKAPICast.h" This can be in the #if ENABLE(REGISTER_PROTOCOL_HANDLER) as well > Source/WebKit2/UIProcess/API/C/WKRegisterProtocolHandler.cpp:35 > +using namespace WebKit; Ditto

Comment on attachment 157215 [details] patch v2 Attachment 157215 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13461195

Comment on attachment 157215 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=157215&action=review > Source/WebKit2/GNUmakefile.list.am:1120 > + Source/WebKit2/WebProcess/WebCoreSupport/WebRegisterProtocolHandlerClient.cpp \ You need to add DerivedSources/WebKit2/WebRegisterProtocolHandlerMessageReceiver.cpp to webkit2_built_sources as well.

Comment on attachment 157215 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=157215&action=review >> Source/WebKit2/GNUmakefile.list.am:1120 >> + Source/WebKit2/WebProcess/WebCoreSupport/WebRegisterProtocolHandlerClient.cpp \ > > You need to add DerivedSources/WebKit2/WebRegisterProtocolHandlerMessageReceiver.cpp to webkit2_built_sources as well. think this is done already..

(In reply to comment #9) > (From update of attachment 157215 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157215&action=review > > >> Source/WebKit2/GNUmakefile.list.am:1120 > >> + Source/WebKit2/WebProcess/WebCoreSupport/WebRegisterProtocolHandlerClient.cpp \ > > > > You need to add DerivedSources/WebKit2/WebRegisterProtocolHandlerMessageReceiver.cpp to webkit2_built_sources as well. > > think this is done already.. found a typo: WebRegisterProtocolHandlerMessageReceiver.cpp -> WebRegisterProtocolHandlerProxyMessageReceiver.cpp

Created attachment 157300 [details] patch v3 Chris, thank you for the review!

Comment on attachment 157300 [details] patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=157300&action=review > Source/WebKit2/GNUmakefile.list.am:210 > + DerivedSources/WebKit2/WebRegisterProtocolHandlerProxyMessageReceiver.cpp \ Nit : wrong indentation. > Source/WebKit2/GNUmakefile.list.am:906 > + Source/WebKit2/UIProcess/WebRegisterProtocolHandlerProvider.cpp \ ditto. > Source/WebKit2/UIProcess/API/C/WKRegisterProtocolHandler.cpp:29 > +#if ENABLE(REGISTER_PROTOCOL_HANDLER) It looks we don't need to add this macro to here.

> > Source/WebKit2/UIProcess/API/C/WKRegisterProtocolHandler.cpp:29 > > +#if ENABLE(REGISTER_PROTOCOL_HANDLER) > > It looks we don't need to add this macro to here. This seems to be completely opposite to what was recommended in Comment #6 by Chris :) Do style guidelines tell anything about this?(I haven't found anything about it).

Created attachment 158093 [details] patch v4 Corrected indentation.

Comment on attachment 158093 [details] patch v4 LGTM.

LGTM too. Thanks.

Comment on attachment 158093 [details] patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=158093&action=review > Source/WebKit2/CMakeLists.txt:276 > + UIProcess/WebRegisterProtocolHandlerProvider.cpp I find the name a big confusing. It sounds like a web-register and then it sounds like a function name instead of a class name. I think we can do better. It feels really out of place with the other class names > Source/WebKit2/ChangeLog:8 > + Added support of Register Protocol Handler API. Maybe link to http://www.w3.org/TR/html5/system-state-and-capabilities.html#custom-handlers Also what about content handlers? > Source/WebKit2/UIProcess/API/C/WKRegisterProtocolHandler.h:46 > +struct WKRegisterProtocolHandlerProvider { Why is this a provider and not a Client?

Comment on attachment 158093 [details] patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=158093&action=review >> Source/WebKit2/CMakeLists.txt:276 >> + UIProcess/WebRegisterProtocolHandlerProvider.cpp > > I find the name a big confusing. It sounds like a web-register and then it sounds like a function name instead of a class name. I think we can do better. > > It feels really out of place with the other class names We currently have WebRegisterProtocolHandlerClient in WebProcess.. So I was following the naming that was already there.. >> Source/WebKit2/ChangeLog:8 >> + Added support of Register Protocol Handler API. > > Maybe link to http://www.w3.org/TR/html5/system-state-and-capabilities.html#custom-handlers > > Also what about content handlers? Sure, link will be useful. As for content handlers, WebKit does not expose JS API for it yet, think it is out of scope of this patch.. >> Source/WebKit2/UIProcess/API/C/WKRegisterProtocolHandler.h:46 >> +struct WKRegisterProtocolHandlerProvider { > > Why is this a provider and not a Client? Similar objects are called "providers" in Vibration API, Battery Status API and Geolocation API

(In reply to comment #18) > (From update of attachment 158093 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158093&action=review > > >> Source/WebKit2/CMakeLists.txt:276 > >> + UIProcess/WebRegisterProtocolHandlerProvider.cpp > > > > I find the name a big confusing. It sounds like a web-register and then it sounds like a function name instead of a class name. I think we can do better. > > > > It feels really out of place with the other class names > > We currently have WebRegisterProtocolHandlerClient in WebProcess.. So I was following the naming that was already there.. OK then it is not your fault :-) I am just wondering why it is not called WebProtocolHandlerProvider or so. Why is the Register important? Maybe you cna find out who created the WebRegisterProtocolHandlerClient and ask. > >> Source/WebKit2/ChangeLog:8 > >> + Added support of Register Protocol Handler API. > > > > Maybe link to http://www.w3.org/TR/html5/system-state-and-capabilities.html#custom-handlers > > > > Also what about content handlers? > > Sure, link will be useful. As for content handlers, WebKit does not expose JS API for it yet, think it is out of scope of this patch.. OK :-) > > >> Source/WebKit2/UIProcess/API/C/WKRegisterProtocolHandler.h:46 > >> +struct WKRegisterProtocolHandlerProvider { > > > > Why is this a provider and not a Client? > > Similar objects are called "providers" in Vibration API, Battery Status API and Geolocation API OK, if it is like that in WebKit2 then fine :-)

Sam (or anyone), could you please take a look? it's been pending for a while..:(

I wonder if this patch is able to unskip tests related to protocol handler in WK2 layout test. http://trac.webkit.org/browser/trunk/LayoutTests/platform/efl-wk2/TestExpectations#L45

(In reply to comment #21) > I wonder if this patch is able to unskip tests related to protocol handler in WK2 layout test. > > http://trac.webkit.org/browser/trunk/LayoutTests/platform/efl-wk2/TestExpectations#L45 The dependent bug 92726 will.

Comment on attachment 158093 [details] patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=158093&action=review > Source/WebKit2/UIProcess/API/C/WKRegisterProtocolHandler.h:38 > + kWKCustomHandlersNew, > + kWKCustomHandlersRegistered, > + kWKCustomHandlersDeclined The use of "Custom" in the name here seems "lonely" as it doesn not appear anywhere else in the API. How about kWKProtocolHandlerRegistrationStatus and kWKProtocolHandlerNew, kWKProtocolHandlerRegistered as well as kWKProtocolHandlerDenied? > Source/WebKit2/UIProcess/API/C/WKRegisterProtocolHandler.h:59 > +WK_EXPORT void WKRegisterProtocolHandlerSetProvider(WKRegisterProtocolHandlerRef registerProtocolHandlerRef, const WKRegisterProtocolHandlerProvider* provider); I don't think it is very common to have the primary name of an API start with a verb. I think a better fitting name would be for example "ProtocolHandlerRegistry" and therefore WKSetProtocolHandlerRegistryProvider. > Source/WebKit2/UIProcess/WebContext.cpp:149 > + , m_registerProtocolHandlerProxy(WebRegisterProtocolHandlerProxy::create(this)) This is an example of why I think having a verb as the first word in the name sounds strange. m_registerProtocolHandlerProxy sounds more like a function name than a noun, hence my suggestion of for example m_protocolHandlerRegistryProxy or maybe just simply m_customProtocolHandlerProxy.

(In reply to comment #23) > (From update of attachment 158093 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158093&action=review > I don't think it is very common to have the primary name of an API start with a verb. I think a better fitting name would be for example "ProtocolHandlerRegistry" and therefore WKSetProtocolHandlerRegistryProvider. > > > Source/WebKit2/UIProcess/WebContext.cpp:149 > > + , m_registerProtocolHandlerProxy(WebRegisterProtocolHandlerProxy::create(this)) > > This is an example of why I think having a verb as the first word in the name sounds strange. m_registerProtocolHandlerProxy sounds more like a function name than a noun, hence my suggestion of for example > m_protocolHandlerRegistryProxy or maybe just simply m_customProtocolHandlerProxy. As I mentioned in the comment #18 such naming is already in WebKit, so have to be consistent. However I absolutely agree that the API should be renamed according to specs (http://www.w3.org/TR/html5/system-state-and-capabilities.html#custom-handlers), filed a separate bug for it https://bugs.webkit.org/show_bug.cgi?id=94920. I do not think it should be a blocker for this bug, as this bug just creates WK2 IPC infrastructure on top of the existing API.

Comment on attachment 158093 [details] patch v4 Need rebasing after bug94920 landed.

Created attachment 160687 [details] patch v5 Rebased.

Here is a good description of how protocol handlers can be used and why they are needed: https://developer.mozilla.org/en-US/docs/Web-based_protocol_handlers protocol handler API is supported by Chromium and Firefox currently and I strongly believe that any WK2 client will benefit from it as well. Protocol handler should be applied to all the pages and that is why the proposed implementation is in WebContext.

Anything left to get this in? Simon, Kenneth?

Is there any progress on this? If there is no progress, I want to follow up this patch.

Created attachment 207839 [details] Patch

Comment on attachment 207839 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207839&action=review > Source/WebKit2/ChangeLog:1 > +2013-07-31 Sanghyun Park <sh919.park@samsung.com> It is not acceptable not to mention the original author of the patch. > Source/WebKit2/UIProcess/API/C/WKNavigatorContentUtils.cpp:2 > + * Copyright (C) 2013 Samsung Electronics. All rights reserved. It is not acceptable either to strip / replace Intel's copyright as they wrote the initial patch.

(In reply to comment #31) > (From update of attachment 207839 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207839&action=review > > > Source/WebKit2/ChangeLog:1 > > +2013-07-31 Sanghyun Park <sh919.park@samsung.com> > > It is not acceptable not to mention the original author of the patch. > > > Source/WebKit2/UIProcess/API/C/WKNavigatorContentUtils.cpp:2 > > + * Copyright (C) 2013 Samsung Electronics. All rights reserved. > > It is not acceptable either to strip / replace Intel's copyright as they wrote the initial patch. Thank you for review. I'll fix these.

Created attachment 207853 [details] Patch

Comment on attachment 207853 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207853&action=review > Source/WebKit2/ChangeLog:1 > +2013-07-31 Sanghyun Park <sh919.park@samsung.com> Or, you can add original author name as below, Mikhail Pozdnyakov <mikhail.pozdnyakov@intel.com>, Sanghyun Park <sh919.park@samsung.com>

(In reply to comment #34) > (From update of attachment 207853 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207853&action=review > > > Source/WebKit2/ChangeLog:1 > > +2013-07-31 Sanghyun Park <sh919.park@samsung.com> > > Or, you can add original author name as below, > > Mikhail Pozdnyakov <mikhail.pozdnyakov@intel.com>, Sanghyun Park <sh919.park@samsung.com> I will refer this, too. Thank you.

Comment on attachment 207853 [details] Patch I think this patch should be rebased against latest trunk. Please update this patch. If you're not interested in this patch anymore, I would like to take over this patch.

Created attachment 252847 [details] Patch

Created attachment 252943 [details] Patch

Attachment 252943 [details] did not pass style-queue: ERROR: Source/WebKit2/WebProcess/WebCoreSupport/WebNavigatorContentUtilsClient.h:34: Do not use 'using namespace WebCore;'. [build/using_namespace] [4] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

This feature has been supported by Chromium and Mozilla. So I would like to support this feature for WebKit as well. Could anyone take a look ?

CC'ing Darin, I wonder if you can take a look this patch.

Support for navigator.registerProtocolHandler/unregisterProtocolHandler is not something we want to support in WebKit2 at this time as we are not confident it is a good Web API. This might be a good conversation for webkit-dev.

Created attachment 256069 [details] Patch

Attachment 256069 [details] did not pass style-queue: ERROR: Source/WebKit2/WebProcess/WebCoreSupport/WebNavigatorContentUtilsClient.h:34: Do not use 'using namespace WebCore;'. [build/using_namespace] [4] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #42) > Support for navigator.registerProtocolHandler/unregisterProtocolHandler is > not something we want to support in WebKit2 at this time as we are not > confident it is a good Web API. This might be a good conversation for > webkit-dev. Though I headed up this API implementation on webkit-dev, I didn't get clear reason yet why we shouldn't support this feature now. Instead, some people wanted to support this feature.

I know you waited a long time and got no reply from Sam. But I still think we need to wait for Sam’s reply. Sam, is that right?

(In reply to comment #46) > I know you waited a long time and got no reply from Sam. But I still think > we need to wait for Sam’s reply. Sam, is that right? Mea culpa. I meant to respond a long time ago on this topic and had a draft sitting ready to go that I though I had sent. I have now sent it to webkit-dev.

(In reply to comment #47) > (In reply to comment #46) > > I know you waited a long time and got no reply from Sam. But I still think > > we need to wait for Sam’s reply. Sam, is that right? > > Mea culpa. I meant to respond a long time ago on this topic and had a draft > sitting ready to go that I though I had sent. I have now sent it to > webkit-dev. As I replied on the thread, I think this feature won't complicate custom protocol. This just uses it. https://lists.webkit.org/pipermail/webkit-dev/2015-July/027524.html I plan to use this feature as below scenario. I would like to listen your opinion about it. 1. Custom scheme is registered by "registeredProtocolHandler()" in JS 2. The registered scheme will be filtering in WebCore. (If unsupported scheme is requested, security error happens.) 3. Filtered scheme will be passed to application side (of course, which is web browser or similar things) 4. The application will register the passed custom scheme and a callback(to call the native embedding application) to WK2's network using "custom protocol handler feature", which was implemented in WebKit2."

(In reply to comment #48) > (In reply to comment #47) > > (In reply to comment #46) > > > I know you waited a long time and got no reply from Sam. But I still think > > > we need to wait for Sam’s reply. Sam, is that right? > > > > Mea culpa. I meant to respond a long time ago on this topic and had a draft > > sitting ready to go that I though I had sent. I have now sent it to > > webkit-dev. > > As I replied on the thread, I think this feature won't complicate custom > protocol. This just uses it. > > https://lists.webkit.org/pipermail/webkit-dev/2015-July/027524.html > > > I plan to use this feature as below scenario. I would like to listen your > opinion about it. > > 1. Custom scheme is registered by "registeredProtocolHandler()" in JS > 2. The registered scheme will be filtering in WebCore. (If unsupported > scheme is requested, security error happens.) > 3. Filtered scheme will be passed to application side (of course, which is > web browser or similar things) > 4. The application will register the passed custom scheme and a callback(to > call the native embedding application) to WK2's network > using "custom protocol handler feature", which was implemented in > WebKit2." Sam, any comment ?

(In reply to comment #48) > I plan to use this feature as below scenario. I would like to listen your > opinion about it. > > 1. Custom scheme is registered by "registeredProtocolHandler()" in JS > 2. The registered scheme will be filtering in WebCore. (If unsupported > scheme is requested, security error happens.) > 3. Filtered scheme will be passed to application side (of course, which is > web browser or similar things) > 4. The application will register the passed custom scheme and a callback(to > call the native embedding application) to WK2's network > using "custom protocol handler feature", which was implemented in > WebKit2." Sam, I spent 2 months to talk about this feature though, I'm still waiting for your reply. I think this scenario won't generate your concern. Could you please give me any your comment against my suggestion ?

Created attachment 258348 [details] Patch

Comment on attachment 258348 [details] Patch This patch has been pending review since 2015 with no recent activity. It seems unlikely that it would even still apply to trunk in its current form. Clearing from the review queue. Feel free to update and resubmit if the patch is still relevant. (Additional editorial note: We probably shouldn't bother enhancing the C-API anymore)

If there is an initiative to add this API again it would benefit from a clean approach so let's close this.