Bug 100751 – [EFL][WK2] Common ref and unref functions for EFL WK2 objects

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Mikhail Pozdnyakov]

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.

[Mikhail Pozdnyakov]

Created attachment 171634 [details]
patch

[Chris Dumez]

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.

[Gyuyoung Kim]

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.

[Chris Dumez]

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

[Gyuyoung Kim]

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

[Thiago Marcos P. Santos]

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.

[Mikhail Pozdnyakov]

(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

[Mikhail Pozdnyakov]

Created attachment 171840 [details]
patch v2

Used macro as proposed by Chris, used Ewk_Object as proposed by Kenneth. Thanks for feedback.

[Kenneth Rohde Christiansen]

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

[Mikhail Pozdnyakov]

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

[Mikhail Pozdnyakov]

Created attachment 171852 [details]
patch v3

Took Kenneth feedback into consideration.

[Chris Dumez]

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.

[Chris Dumez]

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

[Mikhail Pozdnyakov]

(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

[Mikhail Pozdnyakov]

Created attachment 171864 [details]
patch v4

Changed ewk_object_cast semantics, removed all extra macros. Thank you Kenneth and Chris!

[Chris Dumez]

Comment on attachment 171864 [details]
patch v4

Looks better now. Thank you.

[Thiago Marcos P. Santos]

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

[Mikhail Pozdnyakov]

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

[Kenneth Rohde Christiansen]

Comment on attachment 171864 [details]
patch v4

I think we can do the tests separate from this

[WebKit Review Bot]

Comment on attachment 171864 [details]
patch v4

Clearing flags on attachment: 171864

Committed r133278: <http://trac.webkit.org/changeset/133278>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mikhail Pozdnyakov]

Bug for unit tests:
https://bugs.webkit.org/show_bug.cgi?id=101037