Bug 99174 – [EFL][WK2] Encapsulate ref counting for Ewk objects in a parent class

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

[Mikhail Pozdnyakov]

Have a proposal to encapsulate ref counting for Ewk objects in a parent class, this also would allow us to use smart pointers internally
instead of manual refing and derefing.

[Mikhail Pozdnyakov]

Created attachment 168416 [details]
patch

[Gyuyoung Kim]

Comment on attachment 168416 [details]
patch

Looks great. BTW, is "Source/WebKit2/UIProcess/API/efl" place best place for this util class ? As you know, similar util classes are placed to Source/WTF/wtf. Can't we move this to Source/WTF/wtf/efl ?

[Kenneth Rohde Christiansen]

Comment on attachment 168416 [details]
patch

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

> Source/WebKit2/ChangeLog:10
> +        Added EwkRefCounted which is encapsulating ref counting for Ewk objects (supposed to be 
> +        parent class for them). Also added typedefs for EwkRefCounted-based smart pointer types.
> +        Applied the new approach for Ewk_Navigation_Data and Ewk_Url_Request objects as an example.

:-) finally

> Source/WebKit2/UIProcess/API/efl/EwkRefCounted.h:36
> +    EwkRefCounted() { relaxAdoptionRequirement(); } // Do not require adoption as both plain and smart pointers may co-exist.

Is that smart? and without this we could just use RefCounted directly

> Source/WebKit2/UIProcess/API/efl/ewk_context_history_client.cpp:58
> +    EwkRefPtr navigationDataEwk = ewk_navigation_data_new(navigationData);

Isn't it a problem now that you could do RefPtr here? as it inherits from RefCounted? Sounds like something we want to avoid working? or we want to stick to actually using RefPtr instead

[Mikhail Pozdnyakov]

(In reply to comment #3)
> (From update of attachment 168416 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168416&action=review
> 
> > Source/WebKit2/ChangeLog:10
> > +        Added EwkRefCounted which is encapsulating ref counting for Ewk objects (supposed to be 
> > +        parent class for them). Also added typedefs for EwkRefCounted-based smart pointer types.
> > +        Applied the new approach for Ewk_Navigation_Data and Ewk_Url_Request objects as an example.
> 
> :-) finally
> 
> > Source/WebKit2/UIProcess/API/efl/EwkRefCounted.h:36
> > +    EwkRefCounted() { relaxAdoptionRequirement(); } // Do not require adoption as both plain and smart pointers may co-exist.
> 
> Is that smart? and without this we could just use RefCounted directly
My idea was that we use RefPtr internally and keep manual ref counting for external usage, so we have to relax adoption requirement here.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context_history_client.cpp:58
> > +    EwkRefPtr navigationDataEwk = ewk_navigation_data_new(navigationData);
> 
> Isn't it a problem now that you could do RefPtr here? as it inherits from RefCounted? Sounds like something we want to avoid working? or we want to stick to actually using RefPtr instead
Why do you think it's a problem (did not get your point :( )

[Mikhail Pozdnyakov]

(In reply to comment #2)
> (From update of attachment 168416 [details])
Can't we move this to Source/WTF/wtf/efl ?

Would not do it, as area of this class usage is quite narrow: EFL WK2 API objects, so would leave it in WebKit2/UIProcess/API/efl.

[Chris Dumez]

Comment on attachment 168416 [details]
patch

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

> Source/WebKit2/UIProcess/API/efl/EwkRefCounted.h:44
> +EwkAPIType* toEwkAPI(EwkRefCounted* refCounted)

If we used things like RefPtr<Ewk_Navigation_Data> instead of EwkRefPtr, we could use RefPtr::get() instead of toEwkAPI(). It would be more consistent with the rest of WebKit too.

[Mikhail Pozdnyakov]

(In reply to comment #6)
> (From update of attachment 168416 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168416&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkRefCounted.h:44
> > +EwkAPIType* toEwkAPI(EwkRefCounted* refCounted)
> 
> If we used things like RefPtr<Ewk_Navigation_Data> instead of EwkRefPtr, we could use RefPtr::get() instead of toEwkAPI(). It would be more consistent with the rest of WebKit too.

Absolutely, the reason why it's not done like that from the beginning is that structures used to be defined in .ccp files so it was impossible, but since I've moved declaration to private header we off course should follow standard way.

[Mikhail Pozdnyakov]

Created attachment 168659 [details]
patch v2

Took comments from Kenneth and Chris into consideration.

[Chris Dumez]

Comment on attachment 168659 [details]
patch v2

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

I love this change, great idea.

> Source/WebKit2/UIProcess/API/efl/ewk_context_history_client.cpp:58
> +    RefPtr<Ewk_Navigation_Data> navigationDataEwk = ewk_navigation_data_new(navigationData);

Shouldn't it be navigationDataEwk = adoptRef(ewk_navigation_data_new(navigationData))?

> Source/WebKit2/UIProcess/API/efl/ewk_navigation_data_private.h:51
> +        request = ewk_url_request_new(requestWK.get());

shouldn't it be adoptRef(ewk_url_request_new(requestWK.get()) ?

[Kenneth Rohde Christiansen]

Comment on attachment 168659 [details]
patch v2

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

> Source/WebKit2/UIProcess/API/efl/EWKRefCounted.h:35
> +    // Idea is to use RefPtr internally and keep manual ref counting for external usage, so we relax adoption requirement here.

The idea*

>> Source/WebKit2/UIProcess/API/efl/ewk_navigation_data_private.h:51
>> +        request = ewk_url_request_new(requestWK.get());
> 
> shouldn't it be adoptRef(ewk_url_request_new(requestWK.get()) ?

That is what you get from relaxing the requirements. In what case don't we have to adopt it first?

[Mikhail Pozdnyakov]

(In reply to comment #10)
> (From update of attachment 168659 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168659&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EWKRefCounted.h:35
> > +    // Idea is to use RefPtr internally and keep manual ref counting for external usage, so we relax adoption requirement here.
> 
> The idea*
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_navigation_data_private.h:51
> >> +        request = ewk_url_request_new(requestWK.get());
> > 
> > shouldn't it be adoptRef(ewk_url_request_new(requestWK.get()) ?
> 
> That is what you get from relaxing the requirements. In what case don't we have to adopt it first?

If the client has created an instance(might have reffed it several times) and then gave it to us we should not adopt it.

[Mikhail Pozdnyakov]

Created attachment 168666 [details]
patch v3

Added adoption (hate myself for being absentminded :( ).

[Kenneth Rohde Christiansen]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 168659 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=168659&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/EWKRefCounted.h:35
> > > +    // Idea is to use RefPtr internally and keep manual ref counting for external usage, so we relax adoption requirement here.
> > 
> > The idea*
> > 
> > >> Source/WebKit2/UIProcess/API/efl/ewk_navigation_data_private.h:51
> > >> +        request = ewk_url_request_new(requestWK.get());
> > > 
> > > shouldn't it be adoptRef(ewk_url_request_new(requestWK.get()) ?
> > 
> > That is what you get from relaxing the requirements. In what case don't we have to adopt it first?
> 
> If the client has created an instance(might have reffed it several times) and then gave it to us we should not adopt it.

But then the adopt case wouldnt be checked anyway right? And when does that actually happen? coduln't we have a relaxAdoptionRule() for that specific case?

[Kenneth Rohde Christiansen]

> But then the adopt case wouldnt be checked anyway right? And when does that actually happen? coduln't we have a relaxAdoptionRule() for that specific case?

Which we already have relaxAdoptationRequirement()

[Mikhail Pozdnyakov]

Created attachment 168677 [details]
patch v4

Actually we do not even need some extra class, RefCounted is sufficient. The requirement of adoption can be satisfied as long as we keep dealing with smart pointers only, in case external client provides us an object having already refs as a row pointer relaxAdoptionRequirement() can be set explicitly in the specific place.

[Mikhail Pozdnyakov]

Comment on attachment 168677 [details]
patch v4

some garbage left..

[Mikhail Pozdnyakov]

Created attachment 168679 [details]
patch

Removed some garbage (unneeded commented #include)

[Kenneth Rohde Christiansen]

Comment on attachment 168679 [details]
patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_url_request_private.h:40
> +    WKEinaSharedString first_party;

why not firstParty?

[Mikhail Pozdnyakov]

(In reply to comment #18)
> (From update of attachment 168679 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168679&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_url_request_private.h:40
> > +    WKEinaSharedString first_party;
> 
> why not firstParty?

Well, just moved the existing code from .cpp file.. Anyway, not a big deal to correct

[Mikhail Pozdnyakov]

Created attachment 168693 [details]
to be landed

corrected also ewk_url_request data members names.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 168693 [details]
to be landed

Clearing flags on attachment: 168693

Committed r131298: <http://trac.webkit.org/changeset/131298>

[Raphael Kubo da Costa (:rakuco)]

All reviewed patches have been landed.  Closing bug.