Bug 73638 – [EFL] Implement 'registerProtocolHandler' function

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Dongwoo Joshua Im (dwim)]

Custom Scheme Handler should be supported on EFL port.
(http://dev.w3.org/html5/spec/Overview.html#custom-handlers)

I'm preparing the patch currently,
I will upload the patch soon.

Thanks!

[Dongwoo Joshua Im (dwim)]

Created attachment 131987 [details]
Patch

This is the patch of the implementation of the 'registerProtocolHandler' on EFL port.

[Gyuyoung Kim]

Comment on attachment 131987 [details]
Patch

Looks fine. By the way, I wonder whether this patch is able to unskip test cases in EFL Skipped file.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #2)
> (From update of attachment 131987 [details])
> Looks fine. By the way, I wonder whether this patch is able to unskip test cases in EFL Skipped file.

The test case of this feature is "LayoutTests/fast/dom/register-protocol-handler.html".
This test case is not include in the efl/Skipped file currently.

[Gyuyoung Kim]

Comment on attachment 131987 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_custom_handlers.cpp:46
> +bool ewk_custom_handlers_register_protocol_handler(Evas_Object* ewkView, const char* scheme, const char* baseUrl, const char* url, const char* title)

By the way, this file uses ewkView though this file is not ewk_view_xxx file. As you know, ewk files use only ewkView object in ewk_view files. I think we need to discuss how to process this structure. In my humble opinion, there are three methods.

 1. Fire a signal in ChromeClientEfl.
     : Bug 73544 submitted a patch to send a signal in WebCoreSupport.
 2. Move only this function to ewk_view.cpp
 3. This file has an own structure, which has a view object dependency.

It seems to me 1 is most simple method for now.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #4)
> (From update of attachment 131987 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131987&action=review
> 
> > Source/WebKit/efl/ewk/ewk_custom_handlers.cpp:46
> > +bool ewk_custom_handlers_register_protocol_handler(Evas_Object* ewkView, const char* scheme, const char* baseUrl, const char* url, const char* title)
> 
> By the way, this file uses ewkView though this file is not ewk_view_xxx file. As you know, ewk files use only ewkView object in ewk_view files. I think we need to discuss how to process this structure. In my humble opinion, there are three methods.
> 
>  1. Fire a signal in ChromeClientEfl.
>      : Bug 73544 submitted a patch to send a signal in WebCoreSupport.

If it is ok, I prefer this way, as well.
Call a smart_callback in the ChromeClientEfl.cpp file.


>  2. Move only this function to ewk_view.cpp

5 more functions of Custom Handlers will be added.
I think ewk_view.cpp is too big already.


>  3. This file has an own structure, which has a view object dependency.

I think this is possible way also.
I can add the ewkView object into the structure, Ewk_Custom_Handlers_Data.


> 
> It seems to me 1 is most simple method for now.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 131987 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_custom_handlers.cpp:29
> +    Ewk_Custom_Handlers_Data* data = new Ewk_Custom_Handlers_Data();

Please use "new Foo" instead of "new Foo()".

>>> Source/WebKit/efl/ewk/ewk_custom_handlers.cpp:46
>>> +bool ewk_custom_handlers_register_protocol_handler(Evas_Object* ewkView, const char* scheme, const char* baseUrl, const char* url, const char* title)
>> 
>> By the way, this file uses ewkView though this file is not ewk_view_xxx file. As you know, ewk files use only ewkView object in ewk_view files. I think we need to discuss how to process this structure. In my humble opinion, there are three methods.
>> 
>>  1. Fire a signal in ChromeClientEfl.
>>      : Bug 73544 submitted a patch to send a signal in WebCoreSupport.
>>  2. Move only this function to ewk_view.cpp
>>  3. This file has an own structure, which has a view object dependency.
>> 
>> It seems to me 1 is most simple method for now.
> 
> If it is ok, I prefer this way, as well.
> Call a smart_callback in the ChromeClientEfl.cpp file.

That sounds good to me as well. This we can even get rid of this file and just do everything in ChromeClientEfl::registerProtocolHandler.

> Source/WebKit/efl/ewk/ewk_custom_handlers.cpp:50
> +    evas_object_smart_callback_call(ewkView, "protocolhandler,register", static_cast<void*>(data));

Please document this signal in ewk_view.h.

> Source/WebKit/efl/ewk/ewk_custom_handlers.h:36
> +struct _Ewk_Custom_Handlers_Data {
> +    const char *target;
> +    const char *base_url;
> +    const char *url;
> +    const char *title;
> +};

Would be good to have some documentation on what these fields mean and how to use these new features.

[Gyuyoung Kim]

(In reply to comment #6)

>> That sounds good to me as well. This we can even get rid of this file and just do everything in ChromeClientEfl::registerProtocolHandler.

As mentioned in comment #5, five functions will be added to this feature. If application doesn't need to set for this feature, I think we can implement this feature to WebCoreSupport. However, we need to consider when application needs to set something for this feature. 

For example, in this case, _Ewk_Custom_Handlers_Data can has ewkView reference. If  this is permitted, we can split some features into new files. How do you think about this ?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #7)
> (In reply to comment #6)
> 
> >> That sounds good to me as well. This we can even get rid of this file and just do everything in ChromeClientEfl::registerProtocolHandler.
> 
> As mentioned in comment #5, five functions will be added to this feature. If application doesn't need to set for this feature, I think we can implement this feature to WebCoreSupport. However, we need to consider when application needs to set something for this feature. 
> 
> For example, in this case, _Ewk_Custom_Handlers_Data can has ewkView reference. If  this is permitted, we can split some features into new files. How do you think about this ?

Damn, I had forgotten about that. I guess we won't be able to get rid of that cpp file then.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #7)
> (In reply to comment #6)
> 
> >> That sounds good to me as well. This we can even get rid of this file and just do everything in ChromeClientEfl::registerProtocolHandler.
> 
> As mentioned in comment #5, five functions will be added to this feature. If application doesn't need to set for this feature, I think we can implement this feature to WebCoreSupport. However, we need to consider when application needs to set something for this feature. 
> 
> For example, in this case, _Ewk_Custom_Handlers_Data can has ewkView reference. If  this is permitted, we can split some features into new files. How do you think about this ?


Then, I can add a member into the _Ewk_Custom_Handlers_Data structure like below.

struct _Ewk_Custom_Handlers_Data {
    const char *target;
    const char *base_url;
    const char *url;
    const char *title;
    Evas_Object* ewkView;
};

[Dongwoo Joshua Im (dwim)]

Created attachment 132556 [details]
Patch

[Dongwoo Joshua Im (dwim)]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > 
> > >> That sounds good to me as well. This we can even get rid of this file and just do everything in ChromeClientEfl::registerProtocolHandler.
> > 
> > As mentioned in comment #5, five functions will be added to this feature. If application doesn't need to set for this feature, I think we can implement this feature to WebCoreSupport. However, we need to consider when application needs to set something for this feature. 
> > 
> > For example, in this case, _Ewk_Custom_Handlers_Data can has ewkView reference. If  this is permitted, we can split some features into new files. How do you think about this ?
> 
> Damn, I had forgotten about that. I guess we won't be able to get rid of that cpp file then.

Please review the new patch I've uploaded.

Thanks!

[Dongwoo Joshua Im (dwim)]

Created attachment 135299 [details]
Patch

Fix typo.

[Dongwoo Joshua Im (dwim)]

Created attachment 135307 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 135307 [details]
Patch

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

> Source/WebKit/ChangeLog:8
> +        The registerProtocolHandler() method allows Web sites to registera themselves

Typo ? registera -> register ?

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:600
> +    data->o = ewkView;

It looks this is good solution to split ewk_custom_handler.cpp from ewk_view.cpp. I would like to know kubo's opinion about this.

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:608
> +void customHandlersDataDelete(Ewk_Custom_Handlers_Data* data)

It seems to me it is good to use non-member function as static or to use it as private member function.

> Source/WebKit/efl/ewk/ewk_view.h:290
> +    Evas_Object *o;

In my opinion, you need to add description for each field.

[Dongwoo Joshua Im (dwim)]

Created attachment 135467 [details]
Patch

I've fixed the patch regarding Gyuyoung's comments.

[Gyuyoung Kim]

Comment on attachment 135467 [details]
Patch

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

Almost looks good to me except for view object maintenance. However, I think this implementation is not bad.

> Source/WebKit/efl/ewk/ewk_custom_handlers.cpp:25
> +

If there is description for this internal function, In my opinion, it is better to understand this function.

[Gyuyoung Kim]

Comment on attachment 135467 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_custom_handlers.cpp:29
> +    evas_object_smart_callback_call(data->o, "protocolhandler,register", static_cast<void*>(data));

Oops, I'm sorry. It looks you don't write this signal to signal list in ewk_view.h

> Source/WebKit/efl/ewk/ewk_view.h:289
> +struct _Ewk_Custom_Handlers_Data {

I would like to confirm this struct again. Will you add new API to set this data by application ? Because, as you know, if there is no API to set protocol handler, we don't need to open this struct.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #17)
> (From update of attachment 135467 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135467&action=review
> 
> > Source/WebKit/efl/ewk/ewk_custom_handlers.cpp:29
> > +    evas_object_smart_callback_call(data->o, "protocolhandler,register", static_cast<void*>(data));
> 
> Oops, I'm sorry. It looks you don't write this signal to signal list in ewk_view.h
> 

Oh.. should I add that in the ewk_view.h? I will. Thanks.


> > Source/WebKit/efl/ewk/ewk_view.h:289
> > +struct _Ewk_Custom_Handlers_Data {
> 
> I would like to confirm this struct again. Will you add new API to set this data by application ? Because, as you know, if there is no API to set protocol handler, we don't need to open this struct.


Application will not set any value to the members of this structure.
Application will only "read" the value.
Is setter needed anyway?


By the way, I didn't make getter, as well.
I open this structure, so I think the application can get the value by access to the structure without getter.

Do you want me to make getter?

[Gyuyoung Kim]

Comment on attachment 135467 [details]
Patch

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

>>> Source/WebKit/efl/ewk/ewk_view.h:289
>>> +struct _Ewk_Custom_Handlers_Data {
>> 
>> I would like to confirm this struct again. Will you add new API to set this data by application ? Because, as you know, if there is no API to set protocol handler, we don't need to open this struct.
> 
> Application will not set any value to the members of this structure.
> Application will only "read" the value.
> Is setter needed anyway?
> 
> 
> By the way, I didn't make getter, as well.
> I open this structure, so I think the application can get the value by access to the structure without getter.
> 
> Do you want me to make getter?

As we discuss on private channel, I think you don't need to add set / get APIs for now. In future, let's discuss set/get APIs again when you add new APIs related to setting options.

[Dongwoo Joshua Im (dwim)]

Created attachment 135498 [details]
Patch

Fix regarding Gyuyoung's comment.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 135498 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:621
> +    Ewk_Custom_Handlers_Data* data = customHandlersDataCreate(m_view, scheme.utf8().data(), baseURL.utf8().data(), url.utf8().data(), title.utf8().data());
> +    ewk_custom_handlers_register_protocol_handler(data);
> +    customHandlersDataDelete(data);

Since the functions creating and destroying the data are only used in this method, I think they could be all squashed here instead of being implemented separately above. If you do that, I think it even makes sense not to use eina_stringshare at all and reduce the code even more.

> Source/WebKit/efl/ewk/ewk_custom_handlers.cpp:31
> +bool ewk_custom_handlers_register_protocol_handler(Ewk_Custom_Handlers_Data* data)
> +{
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(data->o, false);
> +    evas_object_smart_callback_call(data->o, "protocolhandler,register", data);
> +    return true;
> +}

As we have been discussing in other bugs (such as bug 74921), at least for now it is recommended to put functions which act on a given object together with others. This function simply emits a signal on an ewk_view, so it can clearly be implemented as a private ewk_view function, like the others already defined that also emit ewk_view signals.

This way, not only can you get rid of this whole file, but you can also remove the "o" parameter from the Ewk_Custom_Handlers_Data struct.

> Source/WebKit/efl/ewk/ewk_view.h:63
> + *  - "protocolhandler,register", Ewk_Custom_Handlers_Data: add a handler url for the given protocol.

Nit: we usually describe the signals in a sort of passive voice. You'd normally say something like "a URL handler for the given protocol has been added [or registered]" here.

> Source/WebKit/efl/ewk/ewk_view.h:290
> +struct _Ewk_Custom_Handlers_Data {

We usually use singular forms for struct names.

> Source/WebKit/efl/ewk/ewk_view.h:291
> +    Evas_Object *o; /**< Reference to the view object. */

"o" is not a very descriptive name for this object.

> Source/WebKit/efl/ewk/ewk_view.h:295
> +    const char *target; /**< Reference to the protocols that will be handled. */
> +    const char *base_url; /**< Reference to the resolved url if the url is relative url. */
> +    const char *url; /**< Reference to the url which will handle the given protocol. */
> +    const char *title; /**< Reference to the descriptive title of the handler. */

It'd be useful to give an example for each field. For example: "Reference to the descriptive title of the handler (eg. 'Foo Bar')"

> Source/cmake/OptionsEfl.cmake:95
> +WEBKIT_FEATURE(ENABLE_REGISTER_PROTOCOL_HANDLER "Enable the custom protocol handler" DEFAULT ON)

You'll need to rebase this part; the features are defined in WebKitFeatures.cmake and have their default values overridden in Options${PORT}.cmake.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #21)
> (From update of attachment 135498 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135498&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:621
> > +    Ewk_Custom_Handlers_Data* data = customHandlersDataCreate(m_view, scheme.utf8().data(), baseURL.utf8().data(), url.utf8().data(), title.utf8().data());
> > +    ewk_custom_handlers_register_protocol_handler(data);
> > +    customHandlersDataDelete(data);
> 
> Since the functions creating and destroying the data are only used in this method, I think they could be all squashed here instead of being implemented separately above. If you do that, I think it even makes sense not to use eina_stringshare at all and reduce the code even more.
> 

Only registerProtocolHandler function is implemented in the WebKit currently,
so customHandlersDataCreate & customHandlersDataDelete functions would seem unnecessary.

But, as you know, I'm planning to implement 5 more functions which related to registerProtocolHandler,
and they will use customHandlersDataCreate & customHandlersDataDelete fuctions.
That's why I implement these two functions.

I'll implement the 5 functions just after this patch is merged into the WebKit trunk.


> > Source/WebKit/efl/ewk/ewk_custom_handlers.cpp:31
> > +bool ewk_custom_handlers_register_protocol_handler(Ewk_Custom_Handlers_Data* data)
> > +{
> > +    EINA_SAFETY_ON_NULL_RETURN_VAL(data->o, false);
> > +    evas_object_smart_callback_call(data->o, "protocolhandler,register", data);
> > +    return true;
> > +}
> 
> As we have been discussing in other bugs (such as bug 74921), at least for now it is recommended to put functions which act on a given object together with others. This function simply emits a signal on an ewk_view, so it can clearly be implemented as a private ewk_view function, like the others already defined that also emit ewk_view signals.
> 
> This way, not only can you get rid of this whole file, but you can also remove the "o" parameter from the Ewk_Custom_Handlers_Data struct.
> 

As we discussed in this bug, (and also in this comment)
I'll implement the 5 more functions which related to registerProtocolHandler.

That's why you suggested me to make new file rather than implement in the ewk_private.cpp.

If you think it is reasonable that 6 more functions are added into the ewk_private.cpp,
I'll move it to the file.



> > Source/WebKit/efl/ewk/ewk_view.h:63
> > + *  - "protocolhandler,register", Ewk_Custom_Handlers_Data: add a handler url for the given protocol.
> 
> Nit: we usually describe the signals in a sort of passive voice. You'd normally say something like "a URL handler for the given protocol has been added [or registered]" here.
> 
> > Source/WebKit/efl/ewk/ewk_view.h:290
> > +struct _Ewk_Custom_Handlers_Data {
> 
> We usually use singular forms for struct names.
> 

OK. I'll fix this.


> > Source/WebKit/efl/ewk/ewk_view.h:291
> > +    Evas_Object *o; /**< Reference to the view object. */
> 
> "o" is not a very descriptive name for this object.
> 

OK. I'll fix this.


> > Source/WebKit/efl/ewk/ewk_view.h:295
> > +    const char *target; /**< Reference to the protocols that will be handled. */
> > +    const char *base_url; /**< Reference to the resolved url if the url is relative url. */
> > +    const char *url; /**< Reference to the url which will handle the given protocol. */
> > +    const char *title; /**< Reference to the descriptive title of the handler. */
> 
> It'd be useful to give an example for each field. For example: "Reference to the descriptive title of the handler (eg. 'Foo Bar')"
> 

OK, I try to make it.


> > Source/cmake/OptionsEfl.cmake:95
> > +WEBKIT_FEATURE(ENABLE_REGISTER_PROTOCOL_HANDLER "Enable the custom protocol handler" DEFAULT ON)
> 
> You'll need to rebase this part; the features are defined in WebKitFeatures.cmake and have their default values overridden in Options${PORT}.cmake.

OK. I'll fix this.

[Dongwoo Joshua Im (dwim)]

Created attachment 140691 [details]
Patch

I've fixed the patch regarding Raphael's comment.

[WebKit Review Bot]

Attachment 140691 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/CMakeLists.tx..." exit_code: 1
Source/WebKit/efl/ewk/ewk_view.h:318:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit/efl/ewk/ewk_view.h:318:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/efl/ewk/ewk_view.h:319:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit/efl/ewk/ewk_view.h:319:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 4 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dongwoo Joshua Im (dwim)]

Created attachment 140807 [details]
Patch

[Dongwoo Joshua Im (dwim)]

Created attachment 140831 [details]
Patch

I add LayoutTests/platform/efl/fast/dom/register-protocol-handler-expected.txt in the patch.

Please see the discussion at the bug, https://bugs.webkit.org/show_bug.cgi?id=85874.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 140831 [details]
Patch

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

Something that's not related to a specific part of the patch: what kind of functions do you intend to add? Doesn't it make sense to add them all in this patch?

> Source/WebKit/efl/ChangeLog:20
> +        * ewk/ewk_custom_handlers.cpp: Added.

If the function name prefix in this file is "ewk_custom_handler" (singular), the file should be called "ewk_custom_handler.cpp" (singular).

> Source/WebKit/efl/ewk/ewk_custom_handlers.cpp:29
> +    evas_object_smart_callback_call(data->ewkView, "protocolhandler,register", data);

I'm a bit confused about the behavior here -- is the intention of the code at this point to simply notify API users that this has happened or is this actively part of the process of registering a new protocol handler? In case it's the latter, then it probably makes sense to add a virtual function to the Ewk_View_Smart_Class structure and call it instead of emitting a signal.

> Source/WebKit/efl/ewk/ewk_view.h:73
> + *  - "protocolhandler,register", Ewk_Custom_Handler_Data: add a handler url for the given protocol.

Nit: we generally use the present perfect form for the verbs in signals ("displayed", "set") instead of the infinitive. After all, this is something that has already happened.

> Source/WebKit/efl/ewk/ewk_view.h:317
> +    const char *target; /**< Reference to the protocols that will be handled.  (eg. mailto, irc) */

Isn't this a single protocol instead of protocols (plural)? Shouldn't the variable be just called "protocol"?

> Source/WebKit/efl/ewk/ewk_view.h:318
> +    const char *base_url; /**< Reference to the resolved url if the url is relative url. (eg. "https://www.currenthost.com/") */

If the URL isn't relative, is this 0 or an empty string?

[Dongwoo Joshua Im (dwim)]

(In reply to comment #27)
> (From update of attachment 140831 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140831&action=review
> 

Ok.. I see what you are confused..


There is registerProtocolHandler function is in the WebKit currently.
This patch is implementing the EFL port of the function.

This API works like this.
 1. Web app try to register a protocol handler.
 2. WebKit pass the information to the application who is using the WebKit at that time.
 3. Application save the information into DB or whatever.
 4. When the protocol is shown, application redirect the url regarding the information.

So, WebKit only need to pass the right information to the application.



And, 2 more functions are added at the W3C spec.
isRegisteredProtocolHandler, and unregisterProtocolHandler.

And, there is one more custom handler at rhe W3C spec, which is Custom Content Handler.
There are three APIs : registerContentHandler, isRegisteredContentHandler, and unregisterContentHandler.



I'm planning to implement those new APIs in the WebCore and EFL port.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #27)
> (From update of attachment 140831 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140831&action=review
> 
> Something that's not related to a specific part of the patch: what kind of functions do you intend to add? Doesn't it make sense to add them all in this patch?
> 

I leave a comment above.


> > Source/WebKit/efl/ChangeLog:20
> > +        * ewk/ewk_custom_handlers.cpp: Added.
> 
> If the function name prefix in this file is "ewk_custom_handler" (singular), the file should be called "ewk_custom_handler.cpp" (singular).
> 

Because I want to add Content Handler in this file alse.

I will just make it as singular form, 
and I will change it as plural when I add Content Handler.


> > Source/WebKit/efl/ewk/ewk_custom_handlers.cpp:29
> > +    evas_object_smart_callback_call(data->ewkView, "protocolhandler,register", data);
> 
> I'm a bit confused about the behavior here -- is the intention of the code at this point to simply notify API users that this has happened or is this actively part of the process of registering a new protocol handler? In case it's the latter, then it probably makes sense to add a virtual function to the Ewk_View_Smart_Class structure and call it instead of emitting a signal.
> 

Yes. notify to the application.
As I know, that's what WebKit supposed to do all about.

I simply explained about this at the comment above.


> > Source/WebKit/efl/ewk/ewk_view.h:73
> > + *  - "protocolhandler,register", Ewk_Custom_Handler_Data: add a handler url for the given protocol.
> 
> Nit: we generally use the present perfect form for the verbs in signals ("displayed", "set") instead of the infinitive. After all, this is something that has already happened.
> 

Registering a handler will be happened after an application get this signal.
I think "register" is right form in this case.

But, if it is a coding style of the EFL port, I will changed this  "protocolhandler,registered"


> > Source/WebKit/efl/ewk/ewk_view.h:317
> > +    const char *target; /**< Reference to the protocols that will be handled.  (eg. mailto, irc) */
> 
> Isn't this a single protocol instead of protocols (plural)? Shouldn't the variable be just called "protocol"?
> 

1. Single protocol. I will remove one of them.

2. I use the name "target" because I want to use this attribute for both protocol and content.

  I will change the name of this attribute as protocol, 
  and I will re-change it when I add Content Handler.


> > Source/WebKit/efl/ewk/ewk_view.h:318
> > +    const char *base_url; /**< Reference to the resolved url if the url is relative url. (eg. "https://www.currenthost.com/") */
> 
> If the URL isn't relative, is this 0 or an empty string?

This is the return value of Document::baseURL().
This has value whether the url is relative or not.

[Dongwoo Joshua Im (dwim)]

Created attachment 140871 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

First of all, sorry for repeating some comments -- we've been discussing this for so long that I really forgot what I had already said :-)

(In reply to comment #28)
> This API works like this.
>  1. Web app try to register a protocol handler.
>  2. WebKit pass the information to the application who is using the WebKit at that time.
>  3. Application save the information into DB or whatever.
>  4. When the protocol is shown, application redirect the url regarding the information.
> 
> So, WebKit only need to pass the right information to the application.

(In reply to comment #29)
> > > Source/WebKit/efl/ewk/ewk_custom_handlers.cpp:29
> > > +    evas_object_smart_callback_call(data->ewkView, "protocolhandler,register", data);
> > I'm a bit confused about the behavior here -- is the intention of the code at this point to simply notify API users that this has happened or is this actively part of the process of registering a new protocol handler? In case it's the latter, then it probably makes sense to add a virtual function to the Ewk_View_Smart_Class structure and call it instead of emitting a signal.

> Yes. notify to the application.
> As I know, that's what WebKit supposed to do all about.

> > > Source/WebKit/efl/ewk/ewk_view.h:73
> > > + *  - "protocolhandler,register", Ewk_Custom_Handler_Data: add a handler url for the given protocol.
> > Nit: we generally use the present perfect form for the verbs in signals ("displayed", "set") instead of the infinitive. After all, this is something that has already happened.

> Registering a handler will be happened after an application get this signal.
> I think "register" is right form in this case.

> But, if it is a coding style of the EFL port, I will changed this  "protocolhandler,registered"

Let us clear this part up before the others. What I am asking here is whether the application is part of the protocol registration process or if it is simply notified that the registration happened. In your comment #28 and the first block of comment #29 I quoted, you seem to say that it is the latter; in the last block I quoted, you say that the handler registration will only have happened after the application is notified. I am not sure if you mean this from the point of view of the application (it will only know about the whole protocol registration thing going on in WebKit after it catches the signal indeed) or from WebKit's point of view (in which case synchronously calling a function defined in Ewk_View_Smart_Class is a vital part of the registration process, for example because it needs some return value from the application code).

If the application really simply needs to be notified that this event happened, using "registered" really makes much more sense than "register"; if it's the opposite, you need to change your code and should not be emitting a signal anyway.

[Raphael Kubo da Costa (:rakuco)]

Now on to the rest.

(In reply to comment #29)
> (In reply to comment #27)
> > > Source/WebKit/efl/ewk/ewk_view.h:317
> > > +    const char *target; /**< Reference to the protocols that will be handled.  (eg. mailto, irc) */
> > Isn't this a single protocol instead of protocols (plural)? Shouldn't the variable be just called "protocol"?
> 1. Single protocol. I will remove one of them.
> 2. I use the name "target" because I want to use this attribute for both protocol and content.
>   I will change the name of this attribute as protocol, 
>   and I will re-change it when I add Content Handler.

In the end you chose to use "scheme" as the variable name, which is OK, but you still say "protocols" (plural) in the documentation.

> > > Source/WebKit/efl/ewk/ewk_view.h:318
> > > +    const char *base_url; /**< Reference to the resolved url if the url is relative url. (eg. "https://www.currenthost.com/") */
> > If the URL isn't relative, is this 0 or an empty string?
> This is the return value of Document::baseURL().
> This has value whether the url is relative or not.

This does not answer my question -- I am asking if the value of this variable will be 0 (NULL) or an empty, allocated string if the URL is not relative. This should be properly documented.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #31)
> First of all, sorry for repeating some comments -- we've been discussing this for so long that I really forgot what I had already said :-)
> 
> (In reply to comment #28)
> > This API works like this.
> >  1. Web app try to register a protocol handler.
> >  2. WebKit pass the information to the application who is using the WebKit at that time.
> >  3. Application save the information into DB or whatever.
> >  4. When the protocol is shown, application redirect the url regarding the information.
> > 
> > So, WebKit only need to pass the right information to the application.
> 
> (In reply to comment #29)
> > > > Source/WebKit/efl/ewk/ewk_custom_handlers.cpp:29
> > > > +    evas_object_smart_callback_call(data->ewkView, "protocolhandler,register", data);
> > > I'm a bit confused about the behavior here -- is the intention of the code at this point to simply notify API users that this has happened or is this actively part of the process of registering a new protocol handler? In case it's the latter, then it probably makes sense to add a virtual function to the Ewk_View_Smart_Class structure and call it instead of emitting a signal.
> 
> > Yes. notify to the application.
> > As I know, that's what WebKit supposed to do all about.
> 
> > > > Source/WebKit/efl/ewk/ewk_view.h:73
> > > > + *  - "protocolhandler,register", Ewk_Custom_Handler_Data: add a handler url for the given protocol.
> > > Nit: we generally use the present perfect form for the verbs in signals ("displayed", "set") instead of the infinitive. After all, this is something that has already happened.
> 
> > Registering a handler will be happened after an application get this signal.
> > I think "register" is right form in this case.
> 
> > But, if it is a coding style of the EFL port, I will changed this  "protocolhandler,registered"
> 
> Let us clear this part up before the others. What I am asking here is whether the application is part of the protocol registration process or if it is simply notified that the registration happened. In your comment #28 and the first block of comment #29 I quoted, you seem to say that it is the latter; in the last block I quoted, you say that the handler registration will only have happened after the application is notified. I am not sure if you mean this from the point of view of the application (it will only know about the whole protocol registration thing going on in WebKit after it catches the signal indeed) or from WebKit's point of view (in which case synchronously calling a function defined in Ewk_View_Smart_Class is a vital part of the registration process, for example because it needs some return value from the application code).
> 
> If the application really simply needs to be notified that this event happened, using "registered" really makes much more sense than "register"; if it's the opposite, you need to change your code and should not be emitting a signal anyway.


Regarding comment #28 I've wrote, 
 * WebKit pass the information to the application who is using the WebKit at that time.
 * Application save the information into DB or whatever.


So, the answer of your question is this.

Q: What I am asking here is whether the application is part of the protocol registration process or if it is simply notified that the registration happened.

A: WebKit simply notify to the application that registration is needed, 
and then, the application register the handler and keep it in itself by DB or whatever.

So, the "actual" action, registering, is happened in the application.
That's why I want to 'register' rather than 'registered' in WebKit.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #32)
> Now on to the rest.
> 
> (In reply to comment #29)
> > (In reply to comment #27)
> > > > Source/WebKit/efl/ewk/ewk_view.h:317
> > > > +    const char *target; /**< Reference to the protocols that will be handled.  (eg. mailto, irc) */
> > > Isn't this a single protocol instead of protocols (plural)? Shouldn't the variable be just called "protocol"?
> > 1. Single protocol. I will remove one of them.
> > 2. I use the name "target" because I want to use this attribute for both protocol and content.
> >   I will change the name of this attribute as protocol, 
> >   and I will re-change it when I add Content Handler.
> 
> In the end you chose to use "scheme" as the variable name, which is OK, but you still say "protocols" (plural) in the documentation.
>

Oh. I will use 'scheme' in the comment. That will be better. Thanks!


> > > > Source/WebKit/efl/ewk/ewk_view.h:318
> > > > +    const char *base_url; /**< Reference to the resolved url if the url is relative url. (eg. "https://www.currenthost.com/") */
> > > If the URL isn't relative, is this 0 or an empty string?
> > This is the return value of Document::baseURL().
> > This has value whether the url is relative or not.
> 
> This does not answer my question -- I am asking if the value of this variable will be 0 (NULL) or an empty, allocated string if the URL is not relative. This should be properly documented.

I don't think this could be empty when any web page is loaded.
This attribute set by Document::setURL function.

[Dongwoo Joshua Im (dwim)]

Created attachment 142663 [details]
Patch

I've fixed the patch regarding to the Rakuco's comments.

And, I've moved the prototype of ewk_custom_handler_register_protocol_handler into the ewk_view_private.h file.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 142663 [details]
Patch

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

Looks almost there to me, there are some minor details left.

> Source/WebKit/efl/ewk/ewk_view.h:73
> + *  - "protocolhandler,register", Ewk_Custom_Handler_Data: add a handler url for the given protocol.

It looks like you forgot to change this to "registered"? You could use another signal name if you think it makes more sense, such as "protocolhandler,registration,requested".

> Source/WebKit/efl/ewk/ewk_view.h:318
> +    const char *scheme; /**< Reference to the scheme that will be handled.  (eg. mailto) */

Nitpick: there are two spaces between '.' and '('. Please enclose "mailto" within quotes.

> Source/WebKit/efl/ewk/ewk_view.h:319
> +    const char *base_url; /**< Reference to the resolved url if the url is relative url. (eg. "https://www.currenthost.com/") */

Please mention that this member contains an empty string if the URL is not relative.

> Source/WebKit/efl/ewk/ewk_view.h:321
> +    const char *title; /**< Reference to the descriptive title of the handler. (eg. Example Email) */

Nitpick: please enclose "Example Email" withing quotes.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #36)
> (From update of attachment 142663 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142663&action=review
> 
> Looks almost there to me, there are some minor details left.
> 
> > Source/WebKit/efl/ewk/ewk_view.h:73
> > + *  - "protocolhandler,register", Ewk_Custom_Handler_Data: add a handler url for the given protocol.
> 
> It looks like you forgot to change this to "registered"? You could use another signal name if you think it makes more sense, such as "protocolhandler,registration,requested".
> 

Actually, I didn't want to change the name as 'registered', because of the reason I explained the comment above somewhere.

BTW, your recommendation is pretty nice.
I can use that kind of name for it. ;)

> > Source/WebKit/efl/ewk/ewk_view.h:318
> > +    const char *scheme; /**< Reference to the scheme that will be handled.  (eg. mailto) */
> 
> Nitpick: there are two spaces between '.' and '('. Please enclose "mailto" within quotes.
> 

OK.

> > Source/WebKit/efl/ewk/ewk_view.h:319
> > +    const char *base_url; /**< Reference to the resolved url if the url is relative url. (eg. "https://www.currenthost.com/") */
> 
> Please mention that this member contains an empty string if the URL is not relative.
> 

Do you really think it could have an empty string?
I think it would have a string all the time, even though it could be ignored when the given URL is not relative thing.

I will give you an exact answer after I study about it little bit more.


> > Source/WebKit/efl/ewk/ewk_view.h:321
> > +    const char *title; /**< Reference to the descriptive title of the handler. (eg. Example Email) */
> 
> Nitpick: please enclose "Example Email" withing quotes.

OK.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #37)
> Do you really think it could have an empty string?
> I think it would have a string all the time, even though it could be ignored when the given URL is not relative thing.
> 
> I will give you an exact answer after I study about it little bit more.

I'm not sure, that was a guess; I'll leave it up to you to check this behavior :-)

[Dongwoo Joshua Im (dwim)]

Created attachment 142958 [details]
Patch

I've fixed the patch according to the rakuco's comments.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #38)
> (In reply to comment #37)
> > Do you really think it could have an empty string?
> > I think it would have a string all the time, even though it could be ignored when the given URL is not relative thing.
> > 
> > I will give you an exact answer after I study about it little bit more.
> 
> I'm not sure, that was a guess; I'll leave it up to you to check this behavior :-)

I've add a comment that it could be an empty string.


I'm still thinking it should have a value when any web page is loaded, 
but it could be empty in some cases which I cannot imagine.

[Raphael Kubo da Costa (:rakuco)]

Sorry for the noise. So checking how navigator.registerProtocolHandler and the WebCore implementation work, baseURL can't be empty, since it's the base URL of the site calling navigator.registerProtocolHandler.

[Dongwoo Joshua Im (dwim)]

Created attachment 143166 [details]
Patch

I've fixed regarding the rakuco's comment.

Thanks for the all the comments and discussion, rakuco and Gyuyoung!

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 143166 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_view.h:320
> +    const char *base_url; /**< Reference to the resolved url if the url is relative url. (eg. "https://www.currenthost.com/") */
> +    const char *url; /**< Reference to the url which will handle the given protocol. (eg. "https://www.example.com/?uri=%s") */

One last thing regarding these two comments: I don't think base_url has anything to do with relative URLs, and url itself may not be a full URL.

Using the example from <http://www.w3.org/TR/html5/system-state-and-capabilities.html#custom-handlers>, if a user visits http://www.example.com/foo/bar.html and this page calls navigator.registerProtocolHandler('application/x-soup', 'soup?url=%s', 'SoupWeb'):
 - base_url will be "http://www.example.com/"
 - url will be "soup?url=%s"

[Dongwoo Joshua Im (dwim)]

(In reply to comment #43)
> (From update of attachment 143166 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143166&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.h:320
> > +    const char *base_url; /**< Reference to the resolved url if the url is relative url. (eg. "https://www.currenthost.com/") */
> > +    const char *url; /**< Reference to the url which will handle the given protocol. (eg. "https://www.example.com/?uri=%s") */
> 
> One last thing regarding these two comments: I don't think base_url has anything to do with relative URLs, and url itself may not be a full URL.
> 
> Using the example from <http://www.w3.org/TR/html5/system-state-and-capabilities.html#custom-handlers>, if a user visits http://www.example.com/foo/bar.html and this page calls navigator.registerProtocolHandler('application/x-soup', 'soup?url=%s', 'SoupWeb'):
>  - base_url will be "http://www.example.com/"
>  - url will be "soup?url=%s"

As I know, url could be relative url, or not.

The url is a releative url in the example that you mentioned. 

When I wrote the comments, I was thinking the url is absolute path of url, not relative.

Do you want me to change the comment regarding your example?

[Dongwoo Joshua Im (dwim)]

(In reply to comment #44)
> (In reply to comment #43)
> > (From update of attachment 143166 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=143166&action=review
> > 
> > > Source/WebKit/efl/ewk/ewk_view.h:320
> > > +    const char *base_url; /**< Reference to the resolved url if the url is relative url. (eg. "https://www.currenthost.com/") */
> > > +    const char *url; /**< Reference to the url which will handle the given protocol. (eg. "https://www.example.com/?uri=%s") */
> > 
> > One last thing regarding these two comments: I don't think base_url has anything to do with relative URLs, and url itself may not be a full URL.
> > 
> > Using the example from <http://www.w3.org/TR/html5/system-state-and-capabilities.html#custom-handlers>, if a user visits http://www.example.com/foo/bar.html and this page calls navigator.registerProtocolHandler('application/x-soup', 'soup?url=%s', 'SoupWeb'):
> >  - base_url will be "http://www.example.com/"
> >  - url will be "soup?url=%s"
> 
> As I know, url could be relative url, or not.
> 

OK.

Regarding the discussion on the irc,
I will use the scheme, url, and title which are in the W3C spec.

I will upload the new patch soon.
> The url is a releative url in the example that you mentioned. 
> 
> When I wrote the comments, I was thinking the url is absolute path of url, not relative.
> 
> Do you want me to change the comment regarding your example?

[Dongwoo Joshua Im (dwim)]

Created attachment 143187 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 143187 [details]
Patch

Looks fine to me, thanks for the effort you've put into this.

[WebKit Review Bot]

Comment on attachment 143187 [details]
Patch

Clearing flags on attachment: 143187

Committed r118123: <http://trac.webkit.org/changeset/118123>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dominik Röttsches (drott)]

Reopening - still fails on bots, it needs an additional isEfl() in webkit-build.

[Dongwoo Joshua Im (dwim)]

Created attachment 143546 [details]
Patch

ENABLE_REGISTER_PROTOCOL_HANDLER option will turn on as default on EFL port by this patch.

[WebKit Review Bot]

Comment on attachment 143546 [details]
Patch

Clearing flags on attachment: 143546

Committed r118170: <http://trac.webkit.org/changeset/118170>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.