Bug 96201 – [EFL][WK2] WKEinaSharedString needs a function to adopt eina stringshare.

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

[Byungwoo Lee]

WKEinaSharedString class needs some mechanism like adoptRef and PassRefPtr.
Without this, calling additional eina_stringshare_del() function will be needed in some case for preventing memory leak.

[Byungwoo Lee]

Created attachment 163001 [details]
Patch

[Simon Hausmann]

Comment on attachment 163001 [details]
Patch

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

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:73
> +WK_EXPORT WKEinaSharedString adoptEinaStringShare(const char* string);

Why not make it an overloaded constructor instead? WKEinaSharedString(AdoptTag, const char*) ? Seems more consistent with other adoption patterns in WebKit.

[Gyuyoung Kim]

Comment on attachment 163001 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        WKEinaSharedString needs a to adopt eina stringshare directly.

s/needs a to/needs to/g ?

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:67
> +    // adopting constructor

// adopting -> // Adopting

[Byungwoo Lee]

(In reply to comment #2)
> (From update of attachment 163001 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163001&action=review
> 
> > Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:73
> > +WK_EXPORT WKEinaSharedString adoptEinaStringShare(const char* string);
> 
> Why not make it an overloaded constructor instead? WKEinaSharedString(AdoptTag, const char*) ? Seems more consistent with other adoption patterns in WebKit.


Yes, I considered using WKAdoptTag initially, but that seems to be designed to adopt data from api object type (WKXXX) that is related with APIObject and Opaque types.
(This seems to be a reason that the adopt tag name is 'AdoptWK' - adopting WK api object)

But the data that this function tries to adopt is not WK api object type, but eina stringshare which is provided by efl platform and has it's own ref, unref function - eina_stringshare_add, eina_stringshare_del)
And Wrapper class for the eina stringshare is that WKEinaSharedString.
So I decided not to use AdoptWK tag but add a function for adopt.
(I referenced the adoptRef() function, and the function also defined as a friend function of PassRefPtr)

If there is any way that I can apply the adoption pattern with AdoptTag, please share it. It will be very helpful.

Thank you :)

[Byungwoo Lee]

(In reply to comment #3)
> (From update of attachment 163001 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163001&action=review
> 
> > Source/WebKit2/ChangeLog:8
> > +        WKEinaSharedString needs a to adopt eina stringshare directly.
> 
> s/needs a to/needs to/g ?
Oops, typo. I'll fix it.
> 
> > Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:67
> > +    // adopting constructor
> 
> // adopting -> // Adopting
Ok~ I'll fix it.

[Simon Hausmann]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 163001 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=163001&action=review
> > 
> > > Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:73
> > > +WK_EXPORT WKEinaSharedString adoptEinaStringShare(const char* string);
> > 
> > Why not make it an overloaded constructor instead? WKEinaSharedString(AdoptTag, const char*) ? Seems more consistent with other adoption patterns in WebKit.
> 
> 
> Yes, I considered using WKAdoptTag initially, but that seems to be designed to adopt data from api object type (WKXXX) that is related with APIObject and Opaque types.
> (This seems to be a reason that the adopt tag name is 'AdoptWK' - adopting WK api object)
> 
> But the data that this function tries to adopt is not WK api object type, but eina stringshare which is provided by efl platform and has it's own ref, unref function - eina_stringshare_add, eina_stringshare_del)
> And Wrapper class for the eina stringshare is that WKEinaSharedString.
> So I decided not to use AdoptWK tag but add a function for adopt.
> (I referenced the adoptRef() function, and the function also defined as a friend function of PassRefPtr)
> 
> If there is any way that I can apply the adoption pattern with AdoptTag, please share it. It will be very helpful.

Don't worry, I doubt anybody is going to complain if you decide to use the pattern in a slightly different way in _your_ API :)

That said, it's completely up to you. This is a question of aesthetics, i.e. which approach you find more readable on the caller side and which approach creates a cleaner implementation.

You could also consider using a new tag, like AdoptCF and AdoptNS are used in WTF for the adoption of CoreFoundation and NextStep objects that follow the same reference counting pattern.

Anyway, just saying that it's up to you - whichever you like better.

[Byungwoo Lee]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > (From update of attachment 163001 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=163001&action=review
> > > 
> > > > Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:73
> > > > +WK_EXPORT WKEinaSharedString adoptEinaStringShare(const char* string);
> > > 
> > > Why not make it an overloaded constructor instead? WKEinaSharedString(AdoptTag, const char*) ? Seems more consistent with other adoption patterns in WebKit.
> > 
> > 
> > Yes, I considered using WKAdoptTag initially, but that seems to be designed to adopt data from api object type (WKXXX) that is related with APIObject and Opaque types.
> > (This seems to be a reason that the adopt tag name is 'AdoptWK' - adopting WK api object)
> > 
> > But the data that this function tries to adopt is not WK api object type, but eina stringshare which is provided by efl platform and has it's own ref, unref function - eina_stringshare_add, eina_stringshare_del)
> > And Wrapper class for the eina stringshare is that WKEinaSharedString.
> > So I decided not to use AdoptWK tag but add a function for adopt.
> > (I referenced the adoptRef() function, and the function also defined as a friend function of PassRefPtr)
> > 
> > If there is any way that I can apply the adoption pattern with AdoptTag, please share it. It will be very helpful.
> 
> Don't worry, I doubt anybody is going to complain if you decide to use the pattern in a slightly different way in _your_ API :)
> 
> That said, it's completely up to you. This is a question of aesthetics, i.e. which approach you find more readable on the caller side and which approach creates a cleaner implementation.
> 
> You could also consider using a new tag, like AdoptCF and AdoptNS are used in WTF for the adoption of CoreFoundation and NextStep objects that follow the same reference counting pattern.
> 
> Anyway, just saying that it's up to you - whichever you like better.

Thank you for the kind explanation.
A thought about creating a new tag like below, but I'm not sure whether this is acceptable.


Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h
...
+ enum EinaAdoptTag { AdoptEinaStringShare };
...
+ WKEinaSharedString(EinaAdoptTag, const char* string);



Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp
*WKEinaSharedString::WKEinaSharedString(EinaAdoptTag adoptTag, const char* string)
+    : m_string(string)
+{
+    ASSERT(adoptTag == AdoptEinaStringShare); // Guard for future enum chang
+}


How about this approach? I think this is more simple and matches with other adoptation.

[Mikhail Pozdnyakov]

Comment on attachment 163001 [details]
Patch

At first WKEinaSharedString class contains already WKEinaSharedString(WKAdoptTag, WKStringRef); so there is no need to inject friend functions 
and private constructors, you can follow existing pattern.
Secondly, WKAdoptTag is quite OK to use here IMHO, and it will look much more readable than just 'bool'.
Thirdly, adoption is needed for the strings which have been already shared, right? So why don't you point that out? We could use 
'Eina_Stringshare *' type for the argument.

Informal R-

[Byungwoo Lee]

(In reply to comment #8)
> (From update of attachment 163001 [details])
> At first WKEinaSharedString class contains already WKEinaSharedString(WKAdoptTag, WKStringRef); so there is no need to inject friend functions 
> and private constructors, you can follow existing pattern.
This patch is come from the patch 95672 for some requirement that handles eina stringhshare data that run_javascript_prompt() callback returns.
(I think that the callback cannot return WKStringRef because it is the smart class member function)
In the application side, eina stringshare will be returned by the callback.
For preventing memory leak, adopt function for eina stringshare is needed.

> Secondly, WKAdoptTag is quite OK to use here IMHO, and it will look much more readable than just 'bool'.
Is it ok to use WKAdoptTag (adoptWK) for eina_stringshare even if it is not a WK api object?

> Thirdly, adoption is needed for the strings which have been already shared, right? So why don't you point that out? We could use 
> 'Eina_Stringshare *' type for the argument.
Thanks for pointing this. It will be more clear than const char*

[Mikhail Pozdnyakov]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 163001 [details] [details])
> > At first WKEinaSharedString class contains already WKEinaSharedString(WKAdoptTag, WKStringRef); so there is no need to inject friend functions 
> > and private constructors, you can follow existing pattern.
> This patch is come from the patch 95672 for some requirement that handles eina stringhshare data that run_javascript_prompt() callback returns.
> (I think that the callback cannot return WKStringRef because it is the smart class member function)
> In the application side, eina stringshare will be returned by the callback.
> For preventing memory leak, adopt function for eina stringshare is needed.
> 
I just meant that it should be just (one more) adopt constructor and not adopt function.

> > Secondly, WKAdoptTag is quite OK to use here IMHO, and it will look much more readable than just 'bool'.
> Is it ok to use WKAdoptTag (adoptWK) for eina_stringshare even if it is not a WK api object?

I think yes, unless we want to introduce our own tag, I would use existing
one..

[Mikhail Pozdnyakov]

> > Is it ok to use WKAdoptTag (adoptWK) for eina_stringshare even if it is not a WK api object?
> 
> I think yes, unless we want to introduce our own tag, I would use existing
> one..
Another option is to add a new static member like 'static WKEinaSharedString fromEinaStringShare(Eina_Stringshare*);'

[Byungwoo Lee]

(In reply to comment #11)
> > > Is it ok to use WKAdoptTag (adoptWK) for eina_stringshare even if it is not a WK api object?
> > 
> > I think yes, unless we want to introduce our own tag, I would use existing
> > one..
I think using WKAdoptTag for eina_stringshare can make human mistakes or misunderstanding, because some related template functions and class (e.g. WKRetainPtr, adoptWK()...) are also opened by cpp apis.

If a constructor uses WKAdoptTag for adopting eina_stringshare, api user can try to use adoptWK() function for eina_stringshare, because it already used in the header file.

How about your opinion?

> Another option is to add a new static member like 'static WKEinaSharedString fromEinaStringShare(Eina_Stringshare*);'
Ok, I think this is the issue about static function and friend function.

I also thought about this, and followed the way that adoptPtr() or adoptRef() is designed because it is slightly easy for api users.

With static function,
create sharedstring by adoption will be as below
WKEinaSharedString string = WKEinaSharedString::fromEinaStringShare(eina_stringshare_add(testString))

And with friend function,
create sharedstring by adoption will be as below
WKEinaSharedString string = adoptEinaStringShare(eina_stringshare_add(testString))

after we added below line in the header file.
+ using WebKit::adoptEinaStringShare(...);

I prefer the second personally (for an api user), but If there is some reason that this should be a static function, I'll change it.

Thanks for your comments :)

[Mikhail Pozdnyakov]

(In reply to comment #12)
> (In reply to comment #11)
> > > > Is it ok to use WKAdoptTag (adoptWK) for eina_stringshare even if it is not a WK api object?
> > > 
> > > I think yes, unless we want to introduce our own tag, I would use existing
> > > one..
> I think using WKAdoptTag for eina_stringshare can make human mistakes or misunderstanding, because some related template functions and class (e.g. WKRetainPtr, adoptWK()...) are also opened by cpp apis.
> 
> If a constructor uses WKAdoptTag for adopting eina_stringshare, api user can try to use adoptWK() function for eina_stringshare, because it already used in the header file.
> 
> How about your opinion?
> 
> > Another option is to add a new static member like 'static WKEinaSharedString fromEinaStringShare(Eina_Stringshare*);'
> Ok, I think this is the issue about static function and friend function.
> 
> I also thought about this, and followed the way that adoptPtr() or adoptRef() is designed because it is slightly easy for api users.
> 
> With static function,
> create sharedstring by adoption will be as below
> WKEinaSharedString string = WKEinaSharedString::fromEinaStringShare(eina_stringshare_add(testString))
> 
> And with friend function,
> create sharedstring by adoption will be as below
> WKEinaSharedString string = adoptEinaStringShare(eina_stringshare_add(testString))
> 
> after we added below line in the header file.
> + using WebKit::adoptEinaStringShare(...);
> 
> I prefer the second personally (for an api user), but If there is some reason that this should be a static function, I'll change it.
> 
> Thanks for your comments :)
I would personally go with static function approach, as it looks more explicit for me and produces better-looking code.. But let's see what reviewers will tell. We could think also about how to to make the method name shorter, what about WKEinaSharedString::adopt() ?

[Byungwoo Lee]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > > > Is it ok to use WKAdoptTag (adoptWK) for eina_stringshare even if it is not a WK api object?
> > > > 
> > > > I think yes, unless we want to introduce our own tag, I would use existing
> > > > one..
> > I think using WKAdoptTag for eina_stringshare can make human mistakes or misunderstanding, because some related template functions and class (e.g. WKRetainPtr, adoptWK()...) are also opened by cpp apis.
> > 
> > If a constructor uses WKAdoptTag for adopting eina_stringshare, api user can try to use adoptWK() function for eina_stringshare, because it already used in the header file.
> > 
> > How about your opinion?
> > 
> > > Another option is to add a new static member like 'static WKEinaSharedString fromEinaStringShare(Eina_Stringshare*);'
> > Ok, I think this is the issue about static function and friend function.
> > 
> > I also thought about this, and followed the way that adoptPtr() or adoptRef() is designed because it is slightly easy for api users.
> > 
> > With static function,
> > create sharedstring by adoption will be as below
> > WKEinaSharedString string = WKEinaSharedString::fromEinaStringShare(eina_stringshare_add(testString))
> > 
> > And with friend function,
> > create sharedstring by adoption will be as below
> > WKEinaSharedString string = adoptEinaStringShare(eina_stringshare_add(testString))
> > 
> > after we added below line in the header file.
> > + using WebKit::adoptEinaStringShare(...);
> > 
> > I prefer the second personally (for an api user), but If there is some reason that this should be a static function, I'll change it.
> > 
> > Thanks for your comments :)
> I would personally go with static function approach, as it looks more explicit for me and produces better-looking code.. But let's see what reviewers will tell. We could think also about how to to make the method name shorter, what about WKEinaSharedString::adopt() ?

Ok, it can be more explicit as you said, and short function name will be good for using.
I'll apply it. Thanks :)

[Kenneth Rohde Christiansen]

> Ok, it can be more explicit as you said, and short function name will be good for using.
> I'll apply it. Thanks :)

adoptEinaStringShare()
WKEinaSharedString::adopt()

Are pretty much the same length and the latter doesn't require you to add any 'using' lines.

[Byungwoo Lee]

(In reply to comment #15)
> > Ok, it can be more explicit as you said, and short function name will be good for using.
> > I'll apply it. Thanks :)
> 
> adoptEinaStringShare()
> WKEinaSharedString::adopt()
> 
> Are pretty much the same length and the latter doesn't require you to add any 'using' lines.

Yes, I'm applying it. Thanks for point other good point :)

[Jinwoo Song]

BTW, the class name, WKEinaSharedString seems to be out of sync  with EFL's Eina_StringShare.
http://docs.enlightenment.org/auto/eina/group__Eina__Stringshare__Group.html

Is there anyone who knows the reason? IMO, WKEinaStringShare looks more consistent.

[Mikhail Pozdnyakov]

(In reply to comment #17)
> BTW, the class name, WKEinaSharedString seems to be out of sync  with EFL's Eina_StringShare.
> http://docs.enlightenment.org/auto/eina/group__Eina__Stringshare__Group.html
> 
> Is there anyone who knows the reason? IMO, WKEinaStringShare looks more consistent.

The existing name better reflects purpose of this class. WKEinaSharedString represents 'string' rather than 'share'.

[Byungwoo Lee]

Created attachment 163110 [details]
Patch

[Mikhail Pozdnyakov]

Comment on attachment 163110 [details]
Patch

LGTM. Thanks.

[Kenneth Rohde Christiansen]

Comment on attachment 163110 [details]
Patch

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

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:114
> +{
> +    WKEinaSharedString sharedString;
> +    sharedString.m_string = static_cast<const char*>(string);
> +    return sharedString;
> +}

So that will copy and deallocate the local one. I suppose the copy constructor is effective :-)

[Byungwoo Lee]

(In reply to comment #21)
> (From update of attachment 163110 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163110&action=review
> 
> > Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:114
> > +{
> > +    WKEinaSharedString sharedString;
> > +    sharedString.m_string = static_cast<const char*>(string);
> > +    return sharedString;
> > +}
> 
> So that will copy and deallocate the local one. I suppose the copy constructor is effective :-)
Ok~ then I'll add some private constructor that just adopt eina_stringshare, and use it.
But I have a question.
On the previous patch, I defined the private constructor like below.
+    // Aopting constructor
+    WKEinaSharedString(Eina_Stringshare* string, bool) : m_string(string) { }

I referenced the adopting constructor of PassRefPtr.
Additional bool parameter seems to have some purpose for preventing some mistakes.
Should I follow this way for WKEinaSharedString?

[Kenneth Rohde Christiansen]

(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 163110 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=163110&action=review
> > 
> > > Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:114
> > > +{
> > > +    WKEinaSharedString sharedString;
> > > +    sharedString.m_string = static_cast<const char*>(string);
> > > +    return sharedString;
> > > +}
> > 
> > So that will copy and deallocate the local one. I suppose the copy constructor is effective :-)
> Ok~ then I'll add some private constructor that just adopt eina_stringshare, and use it.

The code is fine, I just wanted you to verify that the copying is efficient. Or if not, implement an efficient copy constructor. I guess the class only has one pointer (m_string?) so it is quite efficient

[Byungwoo Lee]

> The code is fine, I just wanted you to verify that the copying is efficient. Or if not, implement an efficient copy constructor. I guess the class only has one pointer (m_string?) so it is quite efficient

I also think that copy constructor will be good and efficient if the coding style is ok.
Thank you for your guide :)

[Kenneth Rohde Christiansen]

Comment on attachment 163110 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_eina_shared_string.cpp:87
> +    WKEinaSharedString string(WKEinaSharedString::adopt(eina_stringshare_add(testString)));
> +    checkString(string, testString);
> +
> +    string = WKEinaSharedString::adopt(eina_stringshare_add(anotherTestString));
> +    checkString(string, anotherTestString);
> +
> +    string = string;
> +    checkString(string, anotherTestString);

Why not test that the first one got free'd?

[Byungwoo Lee]

(In reply to comment #25)
> (From update of attachment 163110 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163110&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_eina_shared_string.cpp:87
> > +    WKEinaSharedString string(WKEinaSharedString::adopt(eina_stringshare_add(testString)));
> > +    checkString(string, testString);
> > +
> > +    string = WKEinaSharedString::adopt(eina_stringshare_add(anotherTestString));
> > +    checkString(string, anotherTestString);
> > +
> > +    string = string;
> > +    checkString(string, anotherTestString);
> 
> Why not test that the first one got free'd?
Yes, I want to test it, but I think there is no way to verify whether the first one got free'd or not, because there is no api to get the reference count for the eina_stringshare, and accessing free'd eina_stringshare can make segmentation fault according to the eina_stringshare.h

The thing that we can get from this test case is that,
eina_stringshare_del() function in WKEinaSharedString destructor will not make a segmentation fault even if we created it by adopting eina_stringshare directly (without eina_stringshare_ref()).

With this, I can think that the eina_stringshare that adopted by WKEinaSharedString will be deallocated by it's destructor.

[Byungwoo Lee]

I'm applying the last comment from kenneth about coping constructor.

[Byungwoo Lee]

Created attachment 163299 [details]
Patch

[WebKit Review Bot]

Comment on attachment 163299 [details]
Patch

Rejecting attachment 163299 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 163110 [details]
Patch

Clearing flags on attachment: 163110

Committed r128160: <http://trac.webkit.org/changeset/128160>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.