[WK2] Add Navigator Content Utils API support for WebKit2

• Component: WebKit2
• Status: RESOLVED INVALID

[Mikhail Pozdnyakov]

WK2 is missing Protocol Handler API.

[Mikhail Pozdnyakov]

Created attachment 156332 [details]
preliminary patch (do not set any flags)

Register Protocol Handler implementation proposal

[Mikhail Pozdnyakov]

Created attachment 157168 [details]
patch

[Gustavo Noronha (kov)]

Comment on attachment 157168 [details]
patch

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

[Gyuyoung Kim]

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.

[Mikhail Pozdnyakov]

Created attachment 157215 [details]
patch v2

Fixed build failure on GTK. Gyuyoung, thanks for review!

[Chris Dumez]

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

[Gustavo Noronha (kov)]

Comment on attachment 157215 [details]
patch v2

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

[Chris Dumez]

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.

[Mikhail Pozdnyakov]

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

[Mikhail Pozdnyakov]

(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

[Mikhail Pozdnyakov]

Created attachment 157300 [details]
patch v3

Chris, thank you for the review!

[Gyuyoung Kim]

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.

[Mikhail Pozdnyakov]

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

[Mikhail Pozdnyakov]

Created attachment 158093 [details]
patch v4

Corrected indentation.

[Chris Dumez]

Comment on attachment 158093 [details]
patch v4

LGTM.

[Gyuyoung Kim]

LGTM too. Thanks.

[Kenneth Rohde Christiansen]

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?

[Mikhail Pozdnyakov]

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

[Kenneth Rohde Christiansen]

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

[Mikhail Pozdnyakov]

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

[Gyuyoung Kim]

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

[Mikhail Pozdnyakov]

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

[Simon Hausmann]

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.

[Mikhail Pozdnyakov]

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

[Mikhail Pozdnyakov]

Comment on attachment 158093 [details]
patch v4

Need rebasing after bug94920 landed.

[Mikhail Pozdnyakov]

Created attachment 160687 [details]
patch v5

Rebased.

[Mikhail Pozdnyakov]

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.

[Raphael Kubo da Costa (:rakuco)]

Anything left to get this in? Simon, Kenneth?

[Sanghyun Park]

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

[Sanghyun Park]

Created attachment 207839 [details]
Patch

[Chris Dumez]

Comment on attachment 207839 [details]
Patch

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

> Source/WebKit2/ChangeLog:1
> [phone]  Sanghyun Park  <[e-mail]>

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.

[Sanghyun Park]

(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
> > [phone]  Sanghyun Park  <[e-mail]>
> 
> 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.

[Sanghyun Park]

Created attachment 207853 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 207853 [details]
Patch

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

> Source/WebKit2/ChangeLog:1
> [phone]  Sanghyun Park  <[e-mail]>

Or, you can add original author name as below,

Mikhail Pozdnyakov <[e-mail]>, Sanghyun Park <[e-mail]>

[Sanghyun Park]

(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
> > [phone]  Sanghyun Park  <[e-mail]>
> 
> Or, you can add original author name as below,
> 
> Mikhail Pozdnyakov <[e-mail]>, Sanghyun Park <[e-mail]>

I will refer this, too. Thank you.

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

Created attachment 252847 [details]
Patch

[Gyuyoung Kim]

Created attachment 252943 [details]
Patch

[WebKit Commit Bot]

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.

[Gyuyoung Kim]

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 ?

[Gyuyoung Kim]

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

[Sam Weinig]

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.

[Gyuyoung Kim]

Created attachment 256069 [details]
Patch

[WebKit Commit Bot]

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.

[Gyuyoung Kim]

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

[Darin Adler]

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?

[Sam Weinig]

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

[Gyuyoung Kim]

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

[Gyuyoung Kim]

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

[Gyuyoung Kim]

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

[Gyuyoung Kim]

Created attachment 258348 [details]
Patch

[Brady Eidson]

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)

[Anne van Kesteren]

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