As all the ref-counting EWK classes are inherited from WTF::RefCounted it is possible now to have generic REF/UNREF functions in the public API instead of having separate pair of such functions for every object.
Created attachment 171634 [details] patch
Comment on attachment 171634 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=171634&action=review Still not sure how I feel about this change, but here is some initial feedback: > Source/WebKit2/PlatformEfl.cmake:61 > + UIProcess/API/efl/ewk_ref_counted.cpp You need to add "ewk_ref_counted.h" to the installable headers below in the same file. > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:62 > + if (const EwkBackForwardListItem* impl = ewkCastToType<EwkBackForwardListItem>(item)) Maybe we could have a macro. Something like EWK_GET_IMPL_OR_RETURN(item, 0); > Source/WebKit2/UIProcess/API/efl/ewk_ref_counted.h:50 > +EAPI Ewk_Ref_Counted *ewk_ref_counted_ref(Ewk_Ref_Counted *object); Why not simply ewk_ref() / ewk_unref()? > Source/WebKit2/UIProcess/API/efl/ewk_ref_counted_private.h:34 > + virtual const char* instClassName() const = 0; "inst"? What does it stand for? We usually try to avoid abbreviations.
The referencing count files(ewk_ref_counted.h | cpp) are placed to Source/WTF/wtf. Why these files should be place to WebKit2's UIProcess side ? If we can move these files to there, I think we can use this functionality in all EFL places.
(In reply to comment #3) > The referencing count files(ewk_ref_counted.h | cpp) are placed to Source/WTF/wtf. Why these files should be place to WebKit2's UIProcess side ? If we can move these files to there, I think we can use this functionality in all EFL places. AFAIK, they need to be in the API folder because they are part of the public API (e.g. ewe_ref_counted_ref() is in ewk_ref_counted.h).
(In reply to comment #4) > (In reply to comment #3) > > The referencing count files(ewk_ref_counted.h | cpp) are placed to Source/WTF/wtf. Why these files should be place to WebKit2's UIProcess side ? If we can move these files to there, I think we can use this functionality in all EFL places. > > AFAIK, they need to be in the API folder because they are part of the public API (e.g. ewe_ref_counted_ref() is in ewk_ref_counted.h). I think we can try this. Anyway, this might to be new item on other bug later.
Comment on attachment 171634 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=171634&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_back_forward_list.cpp:89 > static inline void freeEinaList(Eina_List* list) > { > void* data = 0; > EINA_LIST_FREE(list, data) > - ewk_back_forward_list_item_unref(static_cast<Ewk_Back_Forward_List_Item*>(data)); > + ewk_ref_counted_unref(static_cast<Ewk_Back_Forward_List_Item*>(data)); > } > > TEST_F(EWK2UnitTestBase, ewk_back_forward_list_current_item_get) Would be nice if we could have more unit tests for these functions.
(In reply to comment #6) > (From update of attachment 171634 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171634&action=review > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_back_forward_list.cpp:89 > > static inline void freeEinaList(Eina_List* list) > > { > > void* data = 0; > > EINA_LIST_FREE(list, data) > > - ewk_back_forward_list_item_unref(static_cast<Ewk_Back_Forward_List_Item*>(data)); > > + ewk_ref_counted_unref(static_cast<Ewk_Back_Forward_List_Item*>(data)); > > } > > > > TEST_F(EWK2UnitTestBase, ewk_back_forward_list_current_item_get) > > Would be nice if we could have more unit tests for these functions. we will have these functions checked for every ref counted object we have
Created attachment 171840 [details] patch v2 Used macro as proposed by Chris, used Ewk_Object as proposed by Kenneth. Thanks for feedback.
Comment on attachment 171840 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=171840&action=review > Source/WebKit2/UIProcess/API/efl/ewk_object_private.h:62 > +template <class T> > +inline const T* ewkObjectCast(const Ewk_Object* object) > +{ > + return ewkObjectCastCheck<T>(object) ? static_cast<const T*>(object) : 0; > +} as all casts are of the style some_cast maybe we should do the same? ewk_object_cast() it might even make sense to expose to C++ developers
(In reply to comment #9) > (From update of attachment 171840 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171840&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_object_private.h:62 > > +template <class T> > > +inline const T* ewkObjectCast(const Ewk_Object* object) > > +{ > > + return ewkObjectCastCheck<T>(object) ? static_cast<const T*>(object) : 0; > > +} > > as all casts are of the style some_cast maybe we should do the same? > > ewk_object_cast() it might even make sense to expose to C++ developers no problem :)
Created attachment 171852 [details] patch v3 Took Kenneth feedback into consideration.
Comment on attachment 171852 [details] patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=171852&action=review > Source/WebKit2/ChangeLog:28 > + * UIProcess/API/efl/ewk_back_forward_list_private.h: ewk_object seems to be missing from the changelog? > Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp:62 > + EWK_OBJ_CONST_IMPL_GET_OR_RETURN_VAL(EwkBackForwardListItem, item, impl, 0); Do we really need to distinguish CONST and non-CONST macros? We end up with 4 macros this way. Also do we really need to use Yoda style for internal macros? How about EWK_OBJ_GET_IMPL_OR_RETURN_VAL() > Source/WebKit2/UIProcess/API/efl/ewk_object_private.h:85 > +#define EWK_OBJ_CONST_IMPL_GET_OR_RETURN_VAL(ImplClass, object, impl, ...) \ Doesn't VA_ARGS work with 0 arguments? Then we would need only 2 macros.
Comment on attachment 171852 [details] patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=171852&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_object_private.h:85 >> +#define EWK_OBJ_CONST_IMPL_GET_OR_RETURN_VAL(ImplClass, object, impl, ...) \ > > Doesn't VA_ARGS work with 0 arguments? Then we would need only 2 macros. Actually, in Ewk_view, we don't differentiate and we need only one macro (leveraging VA_ARGS): EWK_VIEW_IMPL_GET_OR_RETURN() No need for EWK_VIEW_IMPL_GET_OR_RETURN_VAL(). Sadly, this macro uses Yoda style too :(
(In reply to comment #13) > (From update of attachment 171852 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171852&action=review > > >> Source/WebKit2/UIProcess/API/efl/ewk_object_private.h:85 > >> +#define EWK_OBJ_CONST_IMPL_GET_OR_RETURN_VAL(ImplClass, object, impl, ...) \ > > > > Doesn't VA_ARGS work with 0 arguments? Then we would need only 2 macros. > > Actually, in Ewk_view, we don't differentiate and we need only one macro (leveraging VA_ARGS): > EWK_VIEW_IMPL_GET_OR_RETURN() > > No need for EWK_VIEW_IMPL_GET_OR_RETURN_VAL(). > > Sadly, this macro uses Yoda style too :( It's consistent with yoda style from other macros there, but they all can be changed off course
Created attachment 171864 [details] patch v4 Changed ewk_object_cast semantics, removed all extra macros. Thank you Kenneth and Chris!
Comment on attachment 171864 [details] patch v4 Looks better now. Thank you.
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 171634 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=171634&action=review > > > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_back_forward_list.cpp:89 > > > static inline void freeEinaList(Eina_List* list) > > > { > > > void* data = 0; > > > EINA_LIST_FREE(list, data) > > > - ewk_back_forward_list_item_unref(static_cast<Ewk_Back_Forward_List_Item*>(data)); > > > + ewk_ref_counted_unref(static_cast<Ewk_Back_Forward_List_Item*>(data)); > > > } > > > > > > TEST_F(EWK2UnitTestBase, ewk_back_forward_list_current_item_get) > > > > Would be nice if we could have more unit tests for these functions. > > we will have these functions checked for every ref counted object we have I would write specific unit tests just for that. WTF has its own unit tests, but some may argue that is tested all around WebKit code. The point of having separated unit tests is: it will make it easier to isolate a problem, check for leaks, measure performance, etc.
(In reply to comment #17) > (In reply to comment #7) > > (In reply to comment #6) > > > (From update of attachment 171634 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=171634&action=review > > > > > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_back_forward_list.cpp:89 > > > > static inline void freeEinaList(Eina_List* list) > > > > { > > > > void* data = 0; > > > > EINA_LIST_FREE(list, data) > > > > - ewk_back_forward_list_item_unref(static_cast<Ewk_Back_Forward_List_Item*>(data)); > > > > + ewk_ref_counted_unref(static_cast<Ewk_Back_Forward_List_Item*>(data)); > > > > } > > > > > > > > TEST_F(EWK2UnitTestBase, ewk_back_forward_list_current_item_get) > > > > > > Would be nice if we could have more unit tests for these functions. > > > > we will have these functions checked for every ref counted object we have > > I would write specific unit tests just for that. WTF has its own unit tests, but some may argue that is tested all around WebKit code. > > The point of having separated unit tests is: it will make it easier to isolate a problem, check for leaks, measure performance, etc. Ewk_Object is abstract class and it cannot live apart from an inherited object , so I guess those inherited objects should be tested including ref/unref functions.
Comment on attachment 171864 [details] patch v4 I think we can do the tests separate from this
Comment on attachment 171864 [details] patch v4 Clearing flags on attachment: 171864 Committed r133278: <http://trac.webkit.org/changeset/133278>
All reviewed patches have been landed. Closing bug.
Bug for unit tests: https://bugs.webkit.org/show_bug.cgi?id=101037