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: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 99321 | ||||||||||||||||
Attachments: |
|
Description
Mikhail Pozdnyakov
2012-10-12 06:57:10 PDT
Created attachment 168416 [details]
patch
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 ?
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 (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 :( ) (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. 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. (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. Created attachment 168659 [details]
patch v2
Took comments from Kenneth and Chris into consideration.
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()) ? 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? (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. Created attachment 168666 [details]
patch v3
Added adoption (hate myself for being absentminded :( ).
(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?
> 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()
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.
Comment on attachment 168677 [details]
patch v4
some garbage left..
Created attachment 168679 [details]
patch
Removed some garbage (unneeded commented #include)
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? (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 Created attachment 168693 [details]
to be landed
corrected also ewk_url_request data members names.
Comment on attachment 168693 [details] to be landed Clearing flags on attachment: 168693 Committed r131298: <http://trac.webkit.org/changeset/131298> All reviewed patches have been landed. Closing bug. |