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.
Created attachment 156895 [details] patch
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
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 :)
Created attachment 156903 [details] patch v2 Kenneth, thanks for review!
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
It would be nice to use it in the same patch to see how convenient it is. Personally I'm not convinced yet.
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;" ?
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
(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
(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
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. " :)
(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)
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.
(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.
(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?
(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 = "";
(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
(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.
(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.
(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..
(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
Created attachment 156912 [details] patch v3 Considered comments from Kenneth and Chris. Thanks.
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 ?
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.
(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?
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
(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..
> > 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.
(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
Created attachment 157201 [details] patch v4 Kenneth, thanks for your patience.
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
Created attachment 157205 [details] patch v5
Comment on attachment 157205 [details] patch v5 Clearing flags on attachment: 157205 Committed r125035: <http://trac.webkit.org/changeset/125035>
All reviewed patches have been landed. Closing bug.
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
(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 ? :-)
(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. :-)
(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.
(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