Bug 93229 – [EFL] EFL Webkit needs a class wrapping eina stringshare

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

EFL WebKit is currently using eina_stringshare to manage strings in a shared pull, however its EFL C API turns to be not quite convenient comparing to a C++ wrapper class.

[Mikhail Pozdnyakov]

Created attachment 156895 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 156895 [details]
patch

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

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:32
> +    :m_string(eina_stringshare_ref(other.m_string))

space missing before m_

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:37
> +    :m_string(eina_stringshare_add(str))

same here

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:43
> +    if (m_string)

when will that happen? why not assert in the ctor instead?

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:34
> +class WK_EXPORT WKEinaSharedString {

maybe it should be made non inheritable

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:36
> +    WKEinaSharedString() : m_string(0) { }

ok I see now. It might still make sense to assert in the other ctors

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:38
> +    WKEinaSharedString(const char* str);

I think think it might make sense to make a WKEinaSharedString(WKStringRef) or so

[Mikhail Pozdnyakov]

Comment on attachment 156895 [details]
patch

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

>> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:34
>> +class WK_EXPORT WKEinaSharedString {
> 
> maybe it should be made non inheritable

How to make it non inheritable? The only way that comes to my mind is to inherit it virtually from a friend class with private default constructor :)

[Mikhail Pozdnyakov]

Created attachment 156903 [details]
patch v2

Kenneth, thanks for review!

[Chris Dumez]

Comment on attachment 156903 [details]
patch v2

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

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:60
> +    ASSERT(str);

Eina stringshare accepts null

[Chris Dumez]

It would be nice to use it in the same patch to see how convenient it is. Personally I'm not convinced yet.

[Mikhail Pozdnyakov]

Comment on attachment 156903 [details]
patch v2

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

>> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:60
>> +    ASSERT(str);
> 
> Eina stringshare accepts null

right, but do we want "einaSharedString = 0;" ?

[Kenneth Rohde Christiansen]

Comment on attachment 156903 [details]
patch v2

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

>> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:60
>> +    ASSERT(str);
> 
> Eina stringshare accepts null

and the _del doesn't?

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:36
> +    WKEinaSharedString() : m_string(0) { }

ALWAYS_INLINE WKEinaSharedString() ? RefPtr does this

[Mikhail Pozdnyakov]

(In reply to comment #6)
> It would be nice to use it in the same patch to see how convenient it is. Personally I'm not convinced yet.

Please see example here: https://bugs.webkit.org/attachment.cgi?id=156900&action=prettypatch

[Kenneth Rohde Christiansen]

(In reply to comment #6)
> It would be nice to use it in the same patch to see how convenient it is. Personally I'm not convinced yet.

It basically provides the same as OwnPtr in that it takes care of freeing for you which in complex code and refactorings easily can be forgotten. But anyway I agree with you and think it would be nice to include it

[Mikhail Pozdnyakov]

Comment on attachment 156903 [details]
patch v2

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

>>>> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:60
>>>> +    ASSERT(str);
>>> 
>>> Eina stringshare accepts null
>> 
>> right, but do we want "einaSharedString = 0;" ?
> 
> and the _del doesn't?

From eina_stringshare_del docs "Note that if the given pointer is not shared or NULL, bad things will happen, likely a segmentation fault. " :)

[Mikhail Pozdnyakov]

(In reply to comment #10)
> (In reply to comment #6)
> > It would be nice to use it in the same patch to see how convenient it is. Personally I'm not convinced yet.
> 
> It basically provides the same as OwnPtr in that it takes care of freeing for you which in complex code and refactorings easily can be forgotten. But anyway I agree with you and think it would be nice to include it

Shall I use WKEinaSharedString class through all the ewk_ classes in same patch? (Can do it but I'm a bit afraid that the patch becomes huge)

[Chris Dumez]

Eina string share replace accepts null and it is used in several places in our code. The fact that you need to do a null check before calling del() is unrelated to the matter.

[Chris Dumez]

(In reply to comment #7)
> (From update of attachment 156903 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156903&action=review
> 
> >> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:60
> >> +    ASSERT(str);
> > 
> > Eina stringshare accepts null
> 
> right, but do we want "einaSharedString = 0;" ?

Why not. I think of your class as OwnPtr. It accepts 0.

[Chris Dumez]

(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #6)
> > > It would be nice to use it in the same patch to see how convenient it is. Personally I'm not convinced yet.
> > 
> > It basically provides the same as OwnPtr in that it takes care of freeing for you which in complex code and refactorings easily can be forgotten. But anyway I agree with you and think it would be nice to include it
> 
> Shall I use WKEinaSharedString class through all the ewk_ classes in same patch? (Can do it but I'm a bit afraid that the patch becomes huge)

Kenneth should decide but I propose to update just the back forward list code for now?

[Mikhail Pozdnyakov]

(In reply to comment #14)
> (In reply to comment #7)
> > (From update of attachment 156903 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=156903&action=review
> > 
> > >> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:60
> > >> +    ASSERT(str);
> > > 
> > > Eina stringshare accepts null
> > 
> > right, but do we want "einaSharedString = 0;" ?
> 
> Why not. I think of your class as OwnPtr. It accepts 0.

That's a string class at the same time and such semantics would look weird IMHO.
I'd rather add empty() function or write einaSharedString = "";

[Mikhail Pozdnyakov]

(In reply to comment #15)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > (In reply to comment #6)
> > > > It would be nice to use it in the same patch to see how convenient it is. Personally I'm not convinced yet.
> > > 
> > > It basically provides the same as OwnPtr in that it takes care of freeing for you which in complex code and refactorings easily can be forgotten. But anyway I agree with you and think it would be nice to include it
> > 
> > Shall I use WKEinaSharedString class through all the ewk_ classes in same patch? (Can do it but I'm a bit afraid that the patch becomes huge)
> 
> Kenneth should decide but I propose to update just the back forward list code for now?

Agree with you, that I will do for sure

[Chris Dumez]

(In reply to comment #16)
> (In reply to comment #14)
> > (In reply to comment #7)
> > > (From update of attachment 156903 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=156903&action=review
> > > 
> > > >> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:60
> > > >> +    ASSERT(str);
> > > > 
> > > > Eina stringshare accepts null
> > > 
> > > right, but do we want "einaSharedString = 0;" ?
> > 
> > Why not. I think of your class as OwnPtr. It accepts 0.
> 
> That's a string class at the same time and such semantics would look weird IMHO.
> I'd rather add empty() function or write einaSharedString = "";

Well, having isNull(), isEmpty() and reset() methods is a good idea. I still think supporting 0 is a good idea because it is used in EFL port. We use char* for in / out values and they may be null. If you don't support 0, this will result in extra code and checks when refactoring.

[Chris Dumez]

(In reply to comment #18)
> (In reply to comment #16)
> > (In reply to comment #14)
> > > (In reply to comment #7)
> > > > (From update of attachment 156903 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=156903&action=review
> > > > 
> > > > >> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:60
> > > > >> +    ASSERT(str);
> > > > > 
> > > > > Eina stringshare accepts null
> > > > 
> > > > right, but do we want "einaSharedString = 0;" ?
> > > 
> > > Why not. I think of your class as OwnPtr. It accepts 0.
> > 
> > That's a string class at the same time and such semantics would look weird IMHO.
> > I'd rather add empty() function or write einaSharedString = "";
> 
> Well, having isNull(), isEmpty() and reset() methods is a good idea. I still think supporting 0 is a good idea because it is used in EFL port. We use char* for in / out values and they may be null. If you don't support 0, this will result in extra code and checks when refactoring.

BTW, using "" instead of 0 has a different meaning. Empty string is different than null.

[Mikhail Pozdnyakov]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #16)
> > > (In reply to comment #14)
> > > > (In reply to comment #7)
> > > > > (From update of attachment 156903 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=156903&action=review
> > > > > 
> > > > > >> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:60
> > > > > >> +    ASSERT(str);
> > > > > > 
> > > > > > Eina stringshare accepts null
> > > > > 
> > > > > right, but do we want "einaSharedString = 0;" ?
> > > > 
> > > > Why not. I think of your class as OwnPtr. It accepts 0.
> > > 
> > > That's a string class at the same time and such semantics would look weird IMHO.
> > > I'd rather add empty() function or write einaSharedString = "";
> > 
> > Well, having isNull(), isEmpty() and reset() methods is a good idea. I still think supporting 0 is a good idea because it is used in EFL port. We use char* for in / out values and they may be null. If you don't support 0, this will result in extra code and checks when refactoring.
> 
> BTW, using "" instead of 0 has a different meaning. Empty string is different than null.

Yeah, that's just in case you want make smth empty..

[Mikhail Pozdnyakov]

(In reply to comment #18)
> (In reply to comment #16)
> > (In reply to comment #14)
> > > (In reply to comment #7)
> > > > (From update of attachment 156903 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=156903&action=review
> > > > 
> > > > >> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:60
> > > > >> +    ASSERT(str);
> > > > > 
> > > > > Eina stringshare accepts null
> > > > 
> > > > right, but do we want "einaSharedString = 0;" ?
> > > 
> > > Why not. I think of your class as OwnPtr. It accepts 0.
> > 
> > That's a string class at the same time and such semantics would look weird IMHO.
> > I'd rather add empty() function or write einaSharedString = "";
> 
> Well, having isNull(), isEmpty() and reset() methods is a good idea. I still think supporting 0 is a good idea because it is used in EFL port. We use char* for in / out values and they may be null. If you don't support 0, this will result in extra code and checks when refactoring.

Ok, let's keep it so far

[Mikhail Pozdnyakov]

Created attachment 156912 [details]
patch v3

Considered comments from Kenneth and Chris. Thanks.

[Kenneth Rohde Christiansen]

Comment on attachment 156912 [details]
patch v3

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

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:41
> +    ALWAYS_INLINE WKEinaSharedString(WKAdoptTag, WKRefType strRef)

Is the WKAdoptTag supposed to be ignored?

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:52
> +    ALWAYS_INLINE WKEinaSharedString(WKRefType strRef)

WKRefType could be something that doesn't have ->string(), no? Maybe it is better to specialize it to only accept WKStringRef and WKURLRef ?

[Mikhail Pozdnyakov]

Comment on attachment 156912 [details]
patch v3

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

>> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:41
>> +    ALWAYS_INLINE WKEinaSharedString(WKAdoptTag, WKRefType strRef)
> 
> Is the WKAdoptTag supposed to be ignored?

Yes, at least WKRetainPtr ignores it.

>> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:52
>> +    ALWAYS_INLINE WKEinaSharedString(WKRefType strRef)
> 
> WKRefType could be something that doesn't have ->string(), no? Maybe it is better to specialize it to only accept WKStringRef and WKURLRef ?

I guess we can have other WKRefType having ->string() in future (for instance StringWithDirection ;-) ). Specialization will mean repeating of existing code basically.. I would say if we at some point have WKRefType having ->someWeirdStringGetter() we will specialize for that type.

[Kenneth Rohde Christiansen]

(In reply to comment #24)
> (From update of attachment 156912 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156912&action=review
> 
> >> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:41
> >> +    ALWAYS_INLINE WKEinaSharedString(WKAdoptTag, WKRefType strRef)
> > 
> > Is the WKAdoptTag supposed to be ignored?
> 
> Yes, at least WKRetainPtr ignores it.
> 
> >> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:52
> >> +    ALWAYS_INLINE WKEinaSharedString(WKRefType strRef)
> > 
> > WKRefType could be something that doesn't have ->string(), no? Maybe it is better to specialize it to only accept WKStringRef and WKURLRef ?

Then we could ASSERT if it is not AdoptWK right?

> 
> I guess we can have other WKRefType having ->string() in future (for instance StringWithDirection ;-) ). Specialization will mean repeating of existing code basically.. I would say if we at some point have WKRefType having ->someWeirdStringGetter() we will specialize for that type.

But are there any WKRefType not having the string() method?

[Mikhail Pozdnyakov]

Comment on attachment 156912 [details]
patch v3

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

>>>> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:41
>>>> +    ALWAYS_INLINE WKEinaSharedString(WKAdoptTag, WKRefType strRef)
>>> 
>>> Is the WKAdoptTag supposed to be ignored?
>> 
>> Yes, at least WKRetainPtr ignores it.
> 
> Then we could ASSERT if it is not AdoptWK right?

enum WKAdoptTag { AdoptWK }; -> guess, it has to be AdoptWK

[Mikhail Pozdnyakov]

(In reply to comment #25)
> (In reply to comment #24)
> > (From update of attachment 156912 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=156912&action=review
> > 
> > >> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:41
> > >> +    ALWAYS_INLINE WKEinaSharedString(WKAdoptTag, WKRefType strRef)
> > > 
> > > Is the WKAdoptTag supposed to be ignored?
> > 
> > Yes, at least WKRetainPtr ignores it.
> > 
> > >> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:52
> > >> +    ALWAYS_INLINE WKEinaSharedString(WKRefType strRef)
> > > 
> > > WKRefType could be something that doesn't have ->string(), no? Maybe it is better to specialize it to only accept WKStringRef and WKURLRef ?
> 
> Then we could ASSERT if it is not AdoptWK right?
> 
> > 
> > I guess we can have other WKRefType having ->string() in future (for instance StringWithDirection ;-) ). Specialization will mean repeating of existing code basically.. I would say if we at some point have WKRefType having ->someWeirdStringGetter() we will specialize for that type.
> 
> But are there any WKRefType not having the string() method?
Think we have plenty of them, but if the Client puts inappropriate WKRefType to function, there will be compiler error..

[Kenneth Rohde Christiansen]

> > But are there any WKRefType not having the string() method?
> Think we have plenty of them, but if the Client puts inappropriate WKRefType to function, there will be compiler error..

Sure I just find it more safe to open this up for classes we trust and where we know it makes sense.

[Kenneth Rohde Christiansen]

(In reply to comment #26)
> (From update of attachment 156912 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156912&action=review
> 
> >>>> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:41
> >>>> +    ALWAYS_INLINE WKEinaSharedString(WKAdoptTag, WKRefType strRef)
> >>> 
> >>> Is the WKAdoptTag supposed to be ignored?
> >> 
> >> Yes, at least WKRetainPtr ignores it.
> > 
> > Then we could ASSERT if it is not AdoptWK right?
> 
> enum WKAdoptTag { AdoptWK }; -> guess, it has to be AdoptWK

It one is added at some point, you could still ASSERT. It is an enum so basically a number

[Mikhail Pozdnyakov]

Created attachment 157201 [details]
patch v4

Kenneth, thanks for your patience.

[Kenneth Rohde Christiansen]

Comment on attachment 157201 [details]
patch v4

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

Very nice

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:44
> +        if (adopt)
> +            WKRelease(strRef); // Have stored a copy into eina_stringshare, do not need adopted strRef.

What about the comment now?

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:61
> +    : m_string(fromRefType(stringRef, true /*adopt*/))

Don't we usually do /* adopt */ true? and not the other way around

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:63
> +    ASSERT(adoptTag == AdoptWK);

// Guard for future enum changes.

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:82
> +
> +

unneeded double newline

[Mikhail Pozdnyakov]

Created attachment 157205 [details]
patch v5

[WebKit Review Bot]

Comment on attachment 157205 [details]
patch v5

Clearing flags on attachment: 157205

Committed r125035: <http://trac.webkit.org/changeset/125035>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Gyuyoung Kim]

Mikhail, thank you for your great patch. :)  As I said webkit-efl mailing list, could you add this WKEinaSharedString usage to EFL WebKit coding style wiki page ?

http://trac.webkit.org/wiki/EFLWebKitCodingStyle

[Kenneth Rohde Christiansen]

(In reply to comment #35)
> Mikhail, thank you for your great patch. :)  As I said webkit-efl mailing list, could you add this WKEinaSharedString usage to EFL WebKit coding style wiki page ?
> 
> http://trac.webkit.org/wiki/EFLWebKitCodingStyle

What list is that? can you add me ? :-)

[Gyuyoung Kim]

(In reply to comment #36)
> (In reply to comment #35)
> > Mikhail, thank you for your great patch. :)  As I said webkit-efl mailing list, could you add this WKEinaSharedString usage to EFL WebKit coding style wiki page ?
> > 
> > http://trac.webkit.org/wiki/EFLWebKitCodingStyle
> 
> What list is that? can you add me ? :-)

As you know, EFL port had efl specific coding style in last year. So, many reviewer couldn't review patches for EFL port because of EFL specific coding style. EFL port decided to use WebKit style except for some specific cases. To guide changed EFL port coding style, I made the wiki page. In my humble opinion, everybody can change it after getting agreements from WebKit EFL folks. :-)

[Kenneth Rohde Christiansen]

(In reply to comment #37)
> (In reply to comment #36)
> > (In reply to comment #35)
> > > Mikhail, thank you for your great patch. :)  As I said webkit-efl mailing list, could you add this WKEinaSharedString usage to EFL WebKit coding style wiki page ?
> > > 
> > > http://trac.webkit.org/wiki/EFLWebKitCodingStyle
> > 
> > What list is that? can you add me ? :-)
> 
> As you know, EFL port had efl specific coding style in last year. So, many reviewer couldn't review patches for EFL port because of EFL specific coding style. EFL port decided to use WebKit style except for some specific cases. To guide changed EFL port coding style, I made the wiki page. In my humble opinion, everybody can change it after getting agreements from WebKit EFL folks. :-)

I was talking about your mailing list, if you could add me.

[Chris Dumez]

(In reply to comment #38)
> (In reply to comment #37)
> > (In reply to comment #36)
> > > (In reply to comment #35)
> > > > Mikhail, thank you for your great patch. :)  As I said webkit-efl mailing list, could you add this WKEinaSharedString usage to EFL WebKit coding style wiki page ?
> > > > 
> > > > http://trac.webkit.org/wiki/EFLWebKitCodingStyle
> > > 
> > > What list is that? can you add me ? :-)
> > 
> > As you know, EFL port had efl specific coding style in last year. So, many reviewer couldn't review patches for EFL port because of EFL specific coding style. EFL port decided to use WebKit style except for some specific cases. To guide changed EFL port coding style, I made the wiki page. In my humble opinion, everybody can change it after getting agreements from WebKit EFL folks. :-)
> 
> I was talking about your mailing list, if you could add me.

I thought you had already subscribed (didn't you comment about licensing there not so long ago?) Anyway, here is the link:
http://lists.webkit.org/mailman/listinfo/webkit-efl