Bug 99174

Summary:

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

Product:

WebKit

Reporter:

Mikhail Pozdnyakov <mikhail.pozdnyakov>

Component:

WebKit EFL

Assignee:

Mikhail Pozdnyakov <mikhail.pozdnyakov>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, gyuyoung.kim, gyuyoung.kim, kenneth, lucas.de.marchi, rakuco, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

99321

Attachments:

Description	Flags
patch	none
patch v2	kenneth: commit-queue-
patch v3	none
patch v4	none
patch	kenneth: review+
to be landed	none

Mikhail Pozdnyakov

Reported 2012-10-12 06:57:10 PDT

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.

Attachments
patch (11.68 KB, patch) 2012-10-12 07:09 PDT, Mikhail Pozdnyakov	no flags	Details Formatted Diff Diff
patch v2 (11.06 KB, patch) 2012-10-15 02:50 PDT, Mikhail Pozdnyakov	kenneth: commit-queue-	Details Formatted Diff Diff
patch v3 (11.06 KB, patch) 2012-10-15 03:20 PDT, Mikhail Pozdnyakov	no flags	Details Formatted Diff Diff
patch v4 (15.61 KB, patch) 2012-10-15 04:39 PDT, Mikhail Pozdnyakov	no flags	Details Formatted Diff Diff
patch (15.55 KB, patch) 2012-10-15 04:43 PDT, Mikhail Pozdnyakov	kenneth: review+	Details Formatted Diff Diff
to be landed (16.05 KB, patch) 2012-10-15 05:58 PDT, Mikhail Pozdnyakov	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Mikhail Pozdnyakov

Comment 1 2012-10-12 07:09:47 PDT

Created attachment 168416 [details] patch

Gyuyoung Kim

Comment 2 2012-10-13 04:42:58 PDT

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 3 2012-10-15 01:42:51 PDT

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

Comment 4 2012-10-15 01:53:19 PDT

(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

Comment 5 2012-10-15 02:12:27 PDT

(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 6 2012-10-15 02:39:37 PDT

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

Comment 7 2012-10-15 02:44:12 PDT

(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

Comment 8 2012-10-15 02:50:38 PDT

Created attachment 168659 [details] patch v2 Took comments from Kenneth and Chris into consideration.

Chris Dumez

Comment 9 2012-10-15 02:55:10 PDT

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 10 2012-10-15 03:00:02 PDT

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

Comment 11 2012-10-15 03:15:40 PDT

(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

Comment 12 2012-10-15 03:20:19 PDT

Created attachment 168666 [details] patch v3 Added adoption (hate myself for being absentminded :( ).

Kenneth Rohde Christiansen

Comment 13 2012-10-15 03:21:05 PDT

(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

Comment 14 2012-10-15 03:21:53 PDT

> 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

Comment 15 2012-10-15 04:39:34 PDT

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 16 2012-10-15 04:41:10 PDT

Comment on attachment 168677 [details] patch v4 some garbage left..

Mikhail Pozdnyakov

Comment 17 2012-10-15 04:43:30 PDT

Created attachment 168679 [details] patch Removed some garbage (unneeded commented #include)

Kenneth Rohde Christiansen

Comment 18 2012-10-15 05:40:34 PDT

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

Comment 19 2012-10-15 05:51:24 PDT

(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

Comment 20 2012-10-15 05:58:57 PDT

Created attachment 168693 [details] to be landed corrected also ewk_url_request data members names.

Raphael Kubo da Costa (:rakuco)

Comment 21 2012-10-15 06:19:31 PDT

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)

Comment 22 2012-10-15 06:19:41 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.