Full Text Bug Listing

Mikhail Pozdnyakov

Reported 2012-08-06 00:55:28 PDT

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

Comment 1 2012-08-07 02:01:12 PDT

Created attachment 156895 [details] patch

Kenneth Rohde Christiansen

Comment 2 2012-08-07 02:50:27 PDT

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 3 2012-08-07 03:09:30 PDT

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

Comment 4 2012-08-07 03:21:30 PDT

Created attachment 156903 [details] patch v2 Kenneth, thanks for review!

Chris Dumez

Comment 5 2012-08-07 03:27:40 PDT

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

Comment 6 2012-08-07 03:29:08 PDT

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 7 2012-08-07 03:33:22 PDT

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 8 2012-08-07 03:34:12 PDT

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

Comment 9 2012-08-07 03:35:51 PDT

(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

Comment 10 2012-08-07 03:37:54 PDT

(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 11 2012-08-07 03:39:20 PDT

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

Comment 12 2012-08-07 03:42:19 PDT

(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

Comment 13 2012-08-07 03:44:33 PDT

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

Comment 14 2012-08-07 03:45:52 PDT

(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

Comment 15 2012-08-07 03:51:08 PDT

(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

Comment 16 2012-08-07 03:56:20 PDT

(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

Comment 17 2012-08-07 03:57:05 PDT

(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

Comment 18 2012-08-07 04:27:59 PDT

(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

Comment 19 2012-08-07 04:29:44 PDT

(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

Comment 20 2012-08-07 04:33:37 PDT

(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

Comment 21 2012-08-07 04:35:03 PDT

(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

Comment 22 2012-08-07 04:41:38 PDT

Created attachment 156912 [details] patch v3 Considered comments from Kenneth and Chris. Thanks.

Kenneth Rohde Christiansen

Comment 23 2012-08-08 05:26:08 PDT

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 24 2012-08-08 05:36:16 PDT

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

Comment 25 2012-08-08 05:39:18 PDT

(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 26 2012-08-08 05:43:46 PDT

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

Comment 27 2012-08-08 05:45:59 PDT

(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

Comment 28 2012-08-08 05:47:05 PDT

> > 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

Comment 29 2012-08-08 05:48:08 PDT

(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

Comment 30 2012-08-08 06:27:04 PDT

Created attachment 157201 [details] patch v4 Kenneth, thanks for your patience.

Kenneth Rohde Christiansen

Comment 31 2012-08-08 06:36:21 PDT

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

Comment 32 2012-08-08 06:45:49 PDT

Created attachment 157205 [details] patch v5

WebKit Review Bot

Comment 33 2012-08-08 07:41:10 PDT

Comment on attachment 157205 [details] patch v5 Clearing flags on attachment: 157205 Committed r125035: <http://trac.webkit.org/changeset/125035>

WebKit Review Bot

Comment 34 2012-08-08 07:41:19 PDT

All reviewed patches have been landed. Closing bug.

Gyuyoung Kim

Comment 35 2012-08-15 18:40:24 PDT

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

Comment 36 2012-08-16 01:34:56 PDT

(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

Comment 37 2012-08-16 02:53:11 PDT

(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

Comment 38 2012-08-16 02:56:25 PDT

(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

Comment 39 2012-08-16 03:08:34 PDT

(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

Attachments
patch (6.98 KB, patch) 2012-08-07 02:01 PDT, Mikhail Pozdnyakov	no flags	Details Formatted Diff Diff
patch v2 (7.76 KB, patch) 2012-08-07 03:21 PDT, Mikhail Pozdnyakov	no flags	Details Formatted Diff Diff
patch v3 (9.22 KB, patch) 2012-08-07 04:41 PDT, Mikhail Pozdnyakov	no flags	Details Formatted Diff Diff
patch v4 (9.81 KB, patch) 2012-08-08 06:27 PDT, Mikhail Pozdnyakov	no flags	Details Formatted Diff Diff
patch v5 (9.80 KB, patch) 2012-08-08 06:45 PDT, Mikhail Pozdnyakov	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Bug 93229