Bug 100506

Created attachment 171106 [details] patch

Created attachment 171107 [details] rebased

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

I am not sure what I think about this yet

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.

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

Comment on attachment 171107 [details] rebased do not like weak arg typing in my approach, preparing smth better ;)

Created attachment 171135 [details] patch v2 Another approach with better callback arguments type checking.

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

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

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

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

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

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

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

Created attachment 171177 [details] patch v3 removed extra space

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

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

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

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?

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

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

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 ?

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

Created attachment 171397 [details] patch v4 Have only one macro now.

Comment on attachment 171397 [details] patch v4 Any more comments Chris?

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.

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?

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

Created attachment 171405 [details] patch v5 Arranged callbacks in alphabetical order, substituted assertion that people did not like (read Sutter! :-) )

Comment on attachment 171405 [details] patch v5 ouch, not everything is sorted :(

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

Comment on attachment 171405 [details] patch v5 Clearing flags on attachment: 171405 Committed r132887: <http://trac.webkit.org/changeset/132887>

All reviewed patches have been landed. Closing bug.

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