Bug 100506 – [EFL][WK2] Simplify signal emitting API in EwkViewImpl

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

Currently have class method per signal, should be one universal method accepting signal name + list of const strings containing supported ewk_view signals.

[Mikhail Pozdnyakov]

Created attachment 171106 [details]
patch

[Mikhail Pozdnyakov]

Created attachment 171107 [details]
rebased

[Kenneth Rohde Christiansen]

Comment on attachment 171107 [details]
rebased

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

> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:42
> +    callSmartCallback<callbackType>(const_cast<char*>(arg.utf8().data()));

it's all very smart :-)

> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:53
> +DECLARE_EWK_VIEW_CALLBACK(EwkViewImpl::DownloadJobCancelled, "download,cancelled")
> +DECLARE_EWK_VIEW_CALLBACK(EwkViewImpl::DownloadJobFailed, "download,failed")

I kind of love the Qt signals :-) Does this have to be EWK_VIEW specific?

What about documentation now?

> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:84
> +// Special case.

That is a bit vague

> Source/WebKit2/UIProcess/efl/DownloadManagerEfl.cpp:53
> +    download->viewImpl()->callSmartCallback<EwkViewImpl::DownloadJobRequested>(download);

viewImpl()->emit<EwkViewImpl::DownloadJobRequested>(download); ?

> Source/WebKit2/UIProcess/efl/DownloadManagerEfl.cpp:93
> +    download->viewImpl()->callSmartCallback<EwkViewImpl::DownloadJobFailed>(&downloadError);

So there is no type safety for arguments?

> Source/WebKit2/UIProcess/efl/ResourceLoadClientEfl.cpp:115
> +    Ewk_Resource_Load_Error resourceLoadError = {resource.get(), ewkError.get()};

why not space after { ?

[Kenneth Rohde Christiansen]

I am not sure what I think about this yet

[Chris Dumez]

Comment on attachment 171107 [details]
rebased

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

>> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:53
>> +DECLARE_EWK_VIEW_CALLBACK(EwkViewImpl::DownloadJobFailed, "download,failed")
> 
> I kind of love the Qt signals :-) Does this have to be EWK_VIEW specific?
> 
> What about documentation now?

the view is the only evas smart object we have so it is the only one able to use smart callbacks at the moment. BTW, EFL can only use smart callbacks on UI widgets (i.e. Evas Object).

The documentation is still in ewk_view.h, this is merely the private implementation.

> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:86
> +inline void EwkViewImpl::callSmartCallback<EwkViewImpl::URLChange>(void*)

I don't think this should be a special case. We should probably split the URL update and the callback call into 2 functions.

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:104
> +    enum Callback {

Why aren't those in the new header as well? Maybe we could have them in a EwkViewCallback namespace?

>> Source/WebKit2/UIProcess/efl/DownloadManagerEfl.cpp:53
>> +    download->viewImpl()->callSmartCallback<EwkViewImpl::DownloadJobRequested>(download);
> 
> viewImpl()->emit<EwkViewImpl::DownloadJobRequested>(download); ?

callSmartCallback is a bit long indeed but I don't think "emit" applies well here. It is really a synchronous callback function call, not a signal emitting processed in the event loop (like Qt).
How about "download->viewImpl()->call<EwkViewImpl::DownloadJobRequestedCb>(download);" ? it would be shorter at least.

>> Source/WebKit2/UIProcess/efl/DownloadManagerEfl.cpp:93
>> +    download->viewImpl()->callSmartCallback<EwkViewImpl::DownloadJobFailed>(&downloadError);
> 
> So there is no type safety for arguments?

I guess we are going to need a bit more code if we want to make sure the right type is passed for the right callback. However, I think it would be nice and it is worth investigating.

[Kenneth Rohde Christiansen]

We could continue using inform. call() is pretty generic

[Mikhail Pozdnyakov]

Comment on attachment 171107 [details]
rebased

do not like weak arg typing in my approach, preparing smth better ;)

[Mikhail Pozdnyakov]

Created attachment 171135 [details]
patch v2

Another approach with better callback arguments type checking.

[Ryuan Choi]

(In reply to comment #8)
> Created an attachment (id=171135) [details]
> patch v2
> 
> Another approach with better callback arguments type checking.

I need more consideration about it.

But, should we consider Evas_Smart_Cb_Description and callbacks of smart class ?
You can find it in WebKit/efl/ewk/ewk_view.cpp.

[Kenneth Rohde Christiansen]

Comment on attachment 171135 [details]
patch v2

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

> Source/WebKit2/UIProcess/efl/DownloadManagerEfl.cpp:119
> +    download->viewImpl()->smartCallback<DownloadJobFinished>().call(download);

I don't like the name smartCallback, what about smartHandler or even eventHandler

->smartHandler<DownloadJobFinished>().call(download);

[Chris Dumez]

(In reply to comment #10)
> (From update of attachment 171135 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171135&action=review
> 
> > Source/WebKit2/UIProcess/efl/DownloadManagerEfl.cpp:119
> > +    download->viewImpl()->smartCallback<DownloadJobFinished>().call(download);
> 
> I don't like the name smartCallback, what about smartHandler or even eventHandler
> 
> ->smartHandler<DownloadJobFinished>().call(download);

Well, they are called smart callbacks in EFL:
http://docs.enlightenment.org/auto/evas/group__Evas__Smart__Object__Group.html

I think using the same name makes it clear what they are for (EFL) developers.

[Chris Dumez]

Comment on attachment 171135 [details]
patch v2

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

> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:142
> +#define DECLARE_EWK_VIEW_SIGNAL(callbackType, string)         \

Would we use the same DECLARE_EWK_VIEW_CALLBACK() macro but make the last argument optional (e.g. using ", ..." and __VA_ARGS__?)

> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:163
> +DECLARE_EWK_VIEW_CALLBACK(LoadProgress, "load,progress",  double*);

Extra space here

[Mikhail Pozdnyakov]

(In reply to comment #9)

> But, should we consider Evas_Smart_Cb_Description and callbacks of smart class ?
> You can find it in WebKit/efl/ewk/ewk_view.cpp.

Think we definitely should use Evas_Smart_Cb_Description for public API users, which is however out of this bug scope.
Actually I don't see how Evas_Smart_Cb_Description can be helpful here (for private API).

[Mikhail Pozdnyakov]

(In reply to comment #12)
> (From update of attachment 171135 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171135&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:142
> > +#define DECLARE_EWK_VIEW_SIGNAL(callbackType, string)         \
> 
> Would we use the same DECLARE_EWK_VIEW_CALLBACK() macro but make the last argument optional (e.g. using ", ..." and __VA_ARGS__?)

Unfortunately 2nd macro is needed, we cannot skip type in typedef + 
hasArguments() should return false in case when no arguments is expected.

[Mikhail Pozdnyakov]

(In reply to comment #10)
> I don't like the name smartCallback, what about smartHandler or even eventHandler
> 
> ->smartHandler<DownloadJobFinished>().call(download);

don't like it either, but would agree with Chris that is sounds more familiar to EFL developers, so would keep it.

[Mikhail Pozdnyakov]

Created attachment 171177 [details]
patch v3

removed extra space

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #4)
> I am not sure what I think about this yet

Same here; I still fail to see how much benefit this kind of change brings. I've always disliked the amount of small methods/functions which just call evas_object_smart_callback_call() we have in WK1 and WK2, but whenever I thought about this, just calling evas_object_smart_callback_call() directly instead of calling another function to do that sounded good enough.

[Mikhail Pozdnyakov]

(In reply to comment #17)
> (In reply to comment #4)
> > I am not sure what I think about this yet
> 
> Same here; I still fail to see how much benefit this kind of change brings. I've always disliked the amount of small methods/functions which just call evas_object_smart_callback_call() we have in WK1 and WK2, but whenever I thought about this, just calling evas_object_smart_callback_call() directly instead of calling another function to do that sounded good enough.

was also considering this.. but came with other approach as it gives more strictness for both callback name and parameter's type.

[Chris Dumez]

(In reply to comment #17)
> (In reply to comment #4)
> > I am not sure what I think about this yet
> 
> Same here; I still fail to see how much benefit this kind of change brings. I've always disliked the amount of small methods/functions which just call evas_object_smart_callback_call() we have in WK1 and WK2, but whenever I thought about this, just calling evas_object_smart_callback_call() directly instead of calling another function to do that sounded good enough.

I see several advantages:
- We can validate the callback argument type
- There is no need to specify the signal name (string) manually everytime you call. This means that there is less risk in making a typo in the signal name. Also, if you need to change a signal name, you only need to do it in one place instead of updating all the callers

I think Mikhail's proposal makes it hard for anyone to use the API incorrectly, which is a sign of a good API IMHO. With Mikhail's approach, adding a new signal is also easy (Just 2 lines to add in 1 header).

Unfortunately, the code involved in making this approach work is a bit complex and obscure. I wish we could make it simpler (and less code) somehow.

[Kenneth Rohde Christiansen]

Comment on attachment 171177 [details]
patch v3

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

> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:53
> +namespace EwkViewCallbacks {
> +
> +enum CallbackType {
> +    DownloadJobCancelled,
> +    DownloadJobFailed,

Would it be possible somehow to let the macros declare this? igeuss not

> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:128
> +#define DECLARE_EWK_VIEW_CALLBACK(callbackType, string, type) \

Will we ever need callbacks on something different from the view?

> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:142
> +#define DECLARE_EWK_VIEW_SIGNAL(callbackType, string)         \

Could these be templates instead? then we would even support overloading.

declareCallback<ViewVisibilityChange, Evas_Object*>("view,visible,change")
declareCallback<ProcessCrashed>("process,crashed") ?

We could even hide it with __VA_LIST__

> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:165
> +DECLARE_EWK_VIEW_CALLBACK(ProvisionalLoadFailed, "load,provisional,failed", Ewk_Error*);
> +DECLARE_EWK_VIEW_SIGNAL(ProvisionalLoadRedirect, "load,provisional,redirect");

EFL differentiates between signal and callback?

[Mikhail Pozdnyakov]

(In reply to comment #20)
> (From update of attachment 171177 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171177&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:53
> > +namespace EwkViewCallbacks {
> > +
> > +enum CallbackType {
> > +    DownloadJobCancelled,
> > +    DownloadJobFailed,
> 
> Would it be possible somehow to let the macros declare this? igeuss not
do not see how it would be possible :(

> 
> > Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:128
> > +#define DECLARE_EWK_VIEW_CALLBACK(callbackType, string, type) \
> 
> Will we ever need callbacks on something different from the view?

Only view is evas object, so not I guess
> 
> > Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:142
> > +#define DECLARE_EWK_VIEW_SIGNAL(callbackType, string)         \
> 
> Could these be templates instead? then we would even support overloading.
we need to declare template class it's impossible to do from a function.

> 
> declareCallback<ViewVisibilityChange, Evas_Object*>("view,visible,change")
> declareCallback<ProcessCrashed>("process,crashed") ?
> 
> We could even hide it with __VA_LIST__
> 
> > Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:165
> > +DECLARE_EWK_VIEW_CALLBACK(ProvisionalLoadFailed, "load,provisional,failed", Ewk_Error*);
> > +DECLARE_EWK_VIEW_SIGNAL(ProvisionalLoadRedirect, "load,provisional,redirect");
> 
> EFL differentiates between signal and callback?

I just called callback w/o parameters signal to differentiate :)

[Chris Dumez]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 171177 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=171177&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:53
> > > +namespace EwkViewCallbacks {
> > > +
> > > +enum CallbackType {
> > > +    DownloadJobCancelled,
> > > +    DownloadJobFailed,
> > 
> > Would it be possible somehow to let the macros declare this? igeuss not
> do not see how it would be possible :(
> 
> > 
> > > Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:128
> > > +#define DECLARE_EWK_VIEW_CALLBACK(callbackType, string, type) \
> > 
> > Will we ever need callbacks on something different from the view?
> 
> Only view is evas object, so not I guess
> > 
> > > Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:142
> > > +#define DECLARE_EWK_VIEW_SIGNAL(callbackType, string)         \
> > 
> > Could these be templates instead? then we would even support overloading.
> we need to declare template class it's impossible to do from a function.
> 
> > 
> > declareCallback<ViewVisibilityChange, Evas_Object*>("view,visible,change")
> > declareCallback<ProcessCrashed>("process,crashed") ?
> > 
> > We could even hide it with __VA_LIST__
> > 
> > > Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:165
> > > +DECLARE_EWK_VIEW_CALLBACK(ProvisionalLoadFailed, "load,provisional,failed", Ewk_Error*);
> > > +DECLARE_EWK_VIEW_SIGNAL(ProvisionalLoadRedirect, "load,provisional,redirect");
> > 
> > EFL differentiates between signal and callback?
> 
> I just called callback w/o parameters signal to differentiate :)


Actually, EFL does not make the distinction, and this is not really a signal so I don't like this name. This is the reason why I asked if we could use the same macro for both.

[Gyuyoung Kim]

Comment on attachment 171177 [details]
patch v3

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

>>> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:165
>>> +DECLARE_EWK_VIEW_SIGNAL(ProvisionalLoadRedirect, "load,provisional,redirect");
>> 
>> EFL differentiates between signal and callback?
> 
> I just called callback w/o parameters signal to differentiate :)

I don't know why you have to define _CALLBACK and _SIGNAL macros. Can't we send signal with null parameter via _CALLBACK macro ?

[Mikhail Pozdnyakov]

(In reply to comment #23)
> (From update of attachment 171177 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171177&action=review
> 
> >>> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:165
> >>> +DECLARE_EWK_VIEW_SIGNAL(ProvisionalLoadRedirect, "load,provisional,redirect");
> >> 
> >> EFL differentiates between signal and callback?
> > 
> > I just called callback w/o parameters signal to differentiate :)
> 
> I don't know why you have to define _CALLBACK and _SIGNAL macros. Can't we send signal with null parameter via _CALLBACK macro ?

did it in order to not allow clients pass any argument where they are not expected.

[Mikhail Pozdnyakov]

Created attachment 171397 [details]
patch v4

Have only one macro now.

[Kenneth Rohde Christiansen]

Comment on attachment 171397 [details]
patch v4

Any more comments Chris?

[Gyuyoung Kim]

Comment on attachment 171397 [details]
patch v4

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

> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:121
> +            ASSERT(!"should not pass arguments for this callback!");

What does this ASSERT mean? Can't you use DGB or CRIT macro ?

> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:161
> +DECLARE_EWK_VIEW_CALLBACK(NavigationPolicyDecision, "policy,decision,navigation", Ewk_Navigation_Policy_Decision);

I prefer to place signal list by alphabetical order. How do you place these signal by alphabetical order? BTW, I think it would be good if you keep signal which uses ENABLE(XXX) macro as now.

[Alexander Shalamov]

Comment on attachment 171397 [details]
patch v4

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

> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:29
> +#include "WebPageProxy.h"

Is this still needed?

[Chris Dumez]

Comment on attachment 171397 [details]
patch v4

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

>> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:121
>> +            ASSERT(!"should not pass arguments for this callback!");
> 
> What does this ASSERT mean? Can't you use DGB or CRIT macro ?

Agreed. This looks weird. ASSERT_NOT_REACHED()?

>> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:161
>> +DECLARE_EWK_VIEW_CALLBACK(NavigationPolicyDecision, "policy,decision,navigation", Ewk_Navigation_Policy_Decision);
> 
> I prefer to place signal list by alphabetical order. How do you place these signal by alphabetical order? BTW, I think it would be good if you keep signal which uses ENABLE(XXX) macro as now.

Yes, it would be nice to sort by signal name like in ewk_view.h

[Mikhail Pozdnyakov]

Created attachment 171405 [details]
patch v5

Arranged callbacks in alphabetical order, substituted assertion that people did not like (read Sutter! :-) )

[Mikhail Pozdnyakov]

Comment on attachment 171405 [details]
patch v5

ouch, not everything is sorted :(

[Mikhail Pozdnyakov]

(In reply to comment #31)
> (From update of attachment 171405 [details])
> ouch, not everything is sorted :(

Well, enum members are not sorted but signal names are sorted like it's done in in ewk_view.h, so think it's correct. 
Putting cq? again, sorry for hassle.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 171405 [details]
patch v5

Clearing flags on attachment: 171405

Committed r132887: <http://trac.webkit.org/changeset/132887>

[Raphael Kubo da Costa (:rakuco)]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

This patch seems to be causing some segfaults in API tests. I'm looking into it to see if it's an easy fix.