[chromium-dev] Re: base::string16 / WebCore::String incompatibility
On Fri, Jul 10, 2009 at 5:04 PM, Jeremy Orlow wrote: > WebCore::String has the interesting property of differentiating between an > empty string and a null string. In string16, however, there is no such thing > as null. > The LocalStorage implementation I'm working on proxies data between the > rednerer processes and browser process. Some of this data is in the form of > WebCore::Strings that need to differentiate between empty and null. > I could add a boolean to all IPC messages where the string could be null and > then add explicit checking (rather than using the elegant and implicit type > conversions that normally happen from WebCore::String <-> WebKit::WebString > <-> base::string16) but that seems pretty ugly. I think a better solution is > adding a new type to base, adding serializing code to > common/ipc_message_utils, and adding implicit type conversions between > WebKit::WebString and this new type. > First of all, is that a good idea? Second of all, what would be a good name > for it? NullableString16? What is happening with Darin's WebCore interface stuff. I remember he was planning on having some kind of WebString that could be passed to IPC. Is this still the case, and can we use that instead? Brett --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: base::string16 / WebCore::String incompatibility
On Mon, Jul 13, 2009 at 8:20 AM, Brett Wilson wrote: > On Fri, Jul 10, 2009 at 5:04 PM, Jeremy Orlow wrote: > > WebCore::String has the interesting property of differentiating between > an > > empty string and a null string. In string16, however, there is no such > thing > > as null. > > The LocalStorage implementation I'm working on proxies data between the > > rednerer processes and browser process. Some of this data is in the form > of > > WebCore::Strings that need to differentiate between empty and null. > > I could add a boolean to all IPC messages where the string could be null > and > > then add explicit checking (rather than using the elegant and implicit > type > > conversions that normally happen from WebCore::String <-> > WebKit::WebString > > <-> base::string16) but that seems pretty ugly. I think a better solution > is > > adding a new type to base, adding serializing code to > > common/ipc_message_utils, and adding implicit type conversions between > > WebKit::WebString and this new type. > > First of all, is that a good idea? Second of all, what would be a good > name > > for it? NullableString16? > > What is happening with Darin's WebCore interface stuff. I remember he > was planning on having some kind of WebString that could be passed to > IPC. Is this still the case, and can we use that instead? > > Brett > WebString exists (has for many moons now). It is just a wrapper for StringImpl, the same way WebCore::String is. It is not threadsafe due to the reference counting done for StringImpl. That makes it a very bad candidate for serializing via Chrome IPC. Someone might naively handle such an IPC on the IO thread, and then PostTask the WebString to another thread. We could try to solve that using assertions, but I decided instead to avoid serializing non-POD WebKit API types over Chrome IPC. For reference: http://src.chromium.org/viewvc/chrome/trunk/src/webkit/api/public/WebString.h?view=markup -Darin --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: base::string16 / WebCore::String incompatibility
On Mon, Jul 13, 2009 at 8:59 AM, Darin Fisher wrote: > On Mon, Jul 13, 2009 at 8:20 AM, Brett Wilson wrote: > >> On Fri, Jul 10, 2009 at 5:04 PM, Jeremy Orlow wrote: >> > WebCore::String has the interesting property of differentiating between >> an >> > empty string and a null string. In string16, however, there is no such >> thing >> > as null. >> > The LocalStorage implementation I'm working on proxies data between the >> > rednerer processes and browser process. Some of this data is in the form >> of >> > WebCore::Strings that need to differentiate between empty and null. >> > I could add a boolean to all IPC messages where the string could be null >> and >> > then add explicit checking (rather than using the elegant and implicit >> type >> > conversions that normally happen from WebCore::String <-> >> WebKit::WebString >> > <-> base::string16) but that seems pretty ugly. I think a better >> solution is >> > adding a new type to base, adding serializing code to >> > common/ipc_message_utils, and adding implicit type conversions between >> > WebKit::WebString and this new type. >> > First of all, is that a good idea? Second of all, what would be a good >> name >> > for it? NullableString16? >> >> What is happening with Darin's WebCore interface stuff. I remember he >> was planning on having some kind of WebString that could be passed to >> IPC. Is this still the case, and can we use that instead? >> >> Brett >> > > WebString exists (has for many moons now). It is just a wrapper for > StringImpl, > the same way WebCore::String is. It is not threadsafe due to the reference > counting done for StringImpl. That makes it a very bad candidate for > serializing > via Chrome IPC. Someone might naively handle such an IPC on the IO thread, > and then PostTask the WebString to another thread. We could try to solve > that > using assertions, but I decided instead to avoid serializing non-POD WebKit > API > types over Chrome IPC. > > For reference: > > http://src.chromium.org/viewvc/chrome/trunk/src/webkit/api/public/WebString.h?view=markup > > -Darin > I guess what I'm proposing is that we have a string data type that can be safely passed across IPC but that has all the possible states a WebCore::String does. I believe adding the null state is the only thing missing. In my code, I've worked around this by passing a boolean along with the string16 which states whether the string is null. This will work, but since AppCache, Database, and maybe others will soon be passing WebStrings through the IPC layer to their backends in the browser process, I'd be surprised if this doesn't come up again. J --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: base::string16 / WebCore::String incompatibility
On Mon, Jul 13, 2009 at 10:05 AM, Jeremy Orlow wrote: > On Mon, Jul 13, 2009 at 8:59 AM, Darin Fisher wrote: >> >> On Mon, Jul 13, 2009 at 8:20 AM, Brett Wilson wrote: >>> >>> On Fri, Jul 10, 2009 at 5:04 PM, Jeremy Orlow wrote: >>> > WebCore::String has the interesting property of differentiating between >>> > an >>> > empty string and a null string. In string16, however, there is no such >>> > thing >>> > as null. >>> > The LocalStorage implementation I'm working on proxies data between the >>> > rednerer processes and browser process. Some of this data is in the >>> > form of >>> > WebCore::Strings that need to differentiate between empty and null. >>> > I could add a boolean to all IPC messages where the string could be >>> > null and >>> > then add explicit checking (rather than using the elegant and implicit >>> > type >>> > conversions that normally happen from WebCore::String <-> >>> > WebKit::WebString >>> > <-> base::string16) but that seems pretty ugly. I think a better >>> > solution is >>> > adding a new type to base, adding serializing code to >>> > common/ipc_message_utils, and adding implicit type conversions between >>> > WebKit::WebString and this new type. >>> > First of all, is that a good idea? Second of all, what would be a good >>> > name >>> > for it? NullableString16? >>> >>> What is happening with Darin's WebCore interface stuff. I remember he >>> was planning on having some kind of WebString that could be passed to >>> IPC. Is this still the case, and can we use that instead? >>> >>> Brett >> >> WebString exists (has for many moons now). It is just a wrapper for >> StringImpl, >> the same way WebCore::String is. It is not threadsafe due to the >> reference >> counting done for StringImpl. That makes it a very bad candidate for >> serializing >> via Chrome IPC. Someone might naively handle such an IPC on the IO >> thread, >> and then PostTask the WebString to another thread. We could try to solve >> that >> using assertions, but I decided instead to avoid serializing non-POD >> WebKit API >> types over Chrome IPC. >> For reference: >> >> http://src.chromium.org/viewvc/chrome/trunk/src/webkit/api/public/WebString.h?view=markup >> -Darin > > I guess what I'm proposing is that we have a string data type that can be > safely passed across IPC but that has all the possible states a > WebCore::String does. I believe adding the null state is the only thing > missing. > In my code, I've worked around this by passing a boolean along with the > string16 which states whether the string is null. This will work, but since > AppCache, Database, and maybe others will soon be passing WebStrings through > the IPC layer to their backends in the browser process, I'd be surprised if > this doesn't come up again. Yes, I think NullableString16 is better than passing a separate bool. Brett --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: base::string16 / WebCore::String incompatibility
On Mon, Jul 13, 2009 at 10:08 AM, Brett Wilson wrote: > > On Mon, Jul 13, 2009 at 10:05 AM, Jeremy Orlow wrote: > > On Mon, Jul 13, 2009 at 8:59 AM, Darin Fisher > wrote: > >> > >> On Mon, Jul 13, 2009 at 8:20 AM, Brett Wilson > wrote: > >>> > >>> On Fri, Jul 10, 2009 at 5:04 PM, Jeremy Orlow > wrote: > >>> > WebCore::String has the interesting property of differentiating > between > >>> > an > >>> > empty string and a null string. In string16, however, there is no > such > >>> > thing > >>> > as null. > >>> > The LocalStorage implementation I'm working on proxies data between > the > >>> > rednerer processes and browser process. Some of this data is in the > >>> > form of > >>> > WebCore::Strings that need to differentiate between empty and null. > >>> > I could add a boolean to all IPC messages where the string could be > >>> > null and > >>> > then add explicit checking (rather than using the elegant and > implicit > >>> > type > >>> > conversions that normally happen from WebCore::String <-> > >>> > WebKit::WebString > >>> > <-> base::string16) but that seems pretty ugly. I think a better > >>> > solution is > >>> > adding a new type to base, adding serializing code to > >>> > common/ipc_message_utils, and adding implicit type conversions > between > >>> > WebKit::WebString and this new type. > >>> > First of all, is that a good idea? Second of all, what would be a > good > >>> > name > >>> > for it? NullableString16? > >>> > >>> What is happening with Darin's WebCore interface stuff. I remember he > >>> was planning on having some kind of WebString that could be passed to > >>> IPC. Is this still the case, and can we use that instead? > >>> > >>> Brett > >> > >> WebString exists (has for many moons now). It is just a wrapper for > >> StringImpl, > >> the same way WebCore::String is. It is not threadsafe due to the > >> reference > >> counting done for StringImpl. That makes it a very bad candidate for > >> serializing > >> via Chrome IPC. Someone might naively handle such an IPC on the IO > >> thread, > >> and then PostTask the WebString to another thread. We could try to > solve > >> that > >> using assertions, but I decided instead to avoid serializing non-POD > >> WebKit API > >> types over Chrome IPC. > >> For reference: > >> > >> > http://src.chromium.org/viewvc/chrome/trunk/src/webkit/api/public/WebString.h?view=markup > >> -Darin > > > > I guess what I'm proposing is that we have a string data type that can be > > safely passed across IPC but that has all the possible states a > > WebCore::String does. I believe adding the null state is the only thing > > missing. > > In my code, I've worked around this by passing a boolean along with the > > string16 which states whether the string is null. This will work, but > since > > AppCache, Database, and maybe others will soon be passing WebStrings > through > > the IPC layer to their backends in the browser process, I'd > be surprised if > > this doesn't come up again. > > Yes, I think NullableString16 is better than passing a separate bool. Can anyone think of a better name for it? --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: base::string16 / WebCore::String incompatibility
On Mon, Jul 13, 2009 at 10:10 AM, Jeremy Orlow wrote: > On Mon, Jul 13, 2009 at 10:08 AM, Brett Wilson wrote: >> Yes, I think NullableString16 is better than passing a separate bool. > > Can anyone think of a better name for it? What's wrong with NullableString16? Writing string code which assumes the input is non-NULL is a really common source of bugs. We shouldn't encourage people to pass null-capable fields unless the API really really does want actual null-capable fields. This isn't something you can address generally by encapsulating it in the wrapper, because the NULL has meaning (it IS a separate bool, regardless of how you dress it up). -scott --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: base::string16 / WebCore::String incompatibility
All right. NullableString16 it is. :-) And, in case it's not clear, this will only be for strings that have a valid null state. The common case should still be using string16. J On Mon, Jul 13, 2009 at 10:23 AM, Scott Hess wrote: > On Mon, Jul 13, 2009 at 10:10 AM, Jeremy Orlow wrote: > > On Mon, Jul 13, 2009 at 10:08 AM, Brett Wilson > wrote: > >> Yes, I think NullableString16 is better than passing a separate bool. > > > > Can anyone think of a better name for it? > > What's wrong with NullableString16? Writing string code which assumes > the input is non-NULL is a really common source of bugs. We shouldn't > encourage people to pass null-capable fields unless the API really > really does want actual null-capable fields. This isn't something you > can address generally by encapsulating it in the wrapper, because the > NULL has meaning (it IS a separate bool, regardless of how you dress > it up). > > -scott > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: base::string16 / WebCore::String incompatibility
On Mon, Jul 13, 2009 at 8:59 AM, Darin Fisher wrote: > > > WebString exists (has for many moons now). It is just a wrapper for > StringImpl, > the same way WebCore::String is. It is not threadsafe due to the reference > counting done for StringImpl. > fwiw, I've checked in all the mechanics to do cheaply use StringImpl's across threads. You have to call a method (to be named) and then you'll get back a new StringImpl which uses the *same underlying buffer *and can be passed to another thread. (Right now, webkit code calls StringImpl::copy() for this purpose but that allocates a new buffer and copies the contents there. I can't modify that method due to the fact that it is currently a threadsafe call and that is relied on in some places in webkit.) Let me know if you want/need more details about it. Dave That makes it a very bad candidate for serializing > via Chrome IPC. Someone might naively handle such an IPC on the IO thread, > and then PostTask the WebString to another thread. We could try to solve > that > using assertions, but I decided instead to avoid serializing non-POD WebKit > API > types over Chrome IPC. > > For reference: > > http://src.chromium.org/viewvc/chrome/trunk/src/webkit/api/public/WebString.h?view=markup > > -Darin > > > > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: base::string16 / WebCore::String incompatibility
On Mon, Jul 13, 2009 at 12:20 PM, David Levin wrote: > > > > On Mon, Jul 13, 2009 at 8:59 AM, Darin Fisher wrote: > >> >> WebString exists (has for many moons now). It is just a wrapper for >> StringImpl, >> the same way WebCore::String is. It is not threadsafe due to the >> reference >> counting done for StringImpl. >> > > fwiw, I've checked in all the mechanics to do cheaply use StringImpl's > across threads. > > You have to call a method (to be named) and then you'll get back a new > StringImpl which uses the *same underlying buffer *and can be passed to > another thread. (Right now, webkit code calls StringImpl::copy() for this > purpose but that allocates a new buffer and copies the contents there. I > can't modify that method due to the fact that it is currently a threadsafe > call and that is relied on in some places in webkit.) > > Let me know if you want/need more details about it. > > Dave > Would it be better to revisit allowing WebStrings to be transported across the IPC barrier (with this new code) or should we go the NullableString16 route? I'm now leaning towards the latter because the common case is that we _don't_ want to allow null strings. Also it will be similar to string16, so it should be more familiar to Chromium developers. Wrapping WebStrings might be a bit faster though, since it won't require an additional string copy from string16 to and from StringImpl. > > > That makes it a very bad candidate for serializing >> via Chrome IPC. Someone might naively handle such an IPC on the IO >> thread, >> and then PostTask the WebString to another thread. We could try to solve >> that >> using assertions, but I decided instead to avoid serializing non-POD >> WebKit API >> types over Chrome IPC. >> >> For reference: >> >> http://src.chromium.org/viewvc/chrome/trunk/src/webkit/api/public/WebString.h?view=markup >> >> -Darin >> >> >> > > > > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: base::string16 / WebCore::String incompatibility
On Mon, Jul 13, 2009 at 12:29 PM, Jeremy Orlow wrote: > On Mon, Jul 13, 2009 at 12:20 PM, David Levin wrote: > >> >> >> >> On Mon, Jul 13, 2009 at 8:59 AM, Darin Fisher wrote: >> >>> >>> WebString exists (has for many moons now). It is just a wrapper for >>> StringImpl, >>> the same way WebCore::String is. It is not threadsafe due to the >>> reference >>> counting done for StringImpl. >>> >> >> fwiw, I've checked in all the mechanics to do cheaply use StringImpl's >> across threads. >> >> You have to call a method (to be named) and then you'll get back a new >> StringImpl which uses the *same underlying buffer *and can be passed to >> another thread. (Right now, webkit code calls StringImpl::copy() for this >> purpose but that allocates a new buffer and copies the contents there. I >> can't modify that method due to the fact that it is currently a threadsafe >> call and that is relied on in some places in webkit.) >> >> Let me know if you want/need more details about it. >> >> Dave >> > > Would it be better to revisit allowing WebStrings to be transported across > the IPC barrier (with this new code) or should we go the NullableString16 > route? I'm now leaning towards the latter because the common case is that > we _don't_ want to allow null strings. Also it will be similar to string16, > so it should be more familiar to Chromium developers. Wrapping WebStrings > might be a bit faster though, since it won't require an additional string > copy from string16 to and from StringImpl. > Hmm... I'd probably go with NullableString16. If we find the copy overhead to matter, then we could always optimize later. Another issue: Do we want to add conversion methods on WebString? My first reaction is to try to avoid it... -Darin --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: base::string16 / WebCore::String incompatibility
On Mon, Jul 13, 2009 at 12:54 PM, Darin Fisher wrote: > On Mon, Jul 13, 2009 at 12:29 PM, Jeremy Orlow wrote: > >> On Mon, Jul 13, 2009 at 12:20 PM, David Levin wrote: >> >>> >>> >>> >>> On Mon, Jul 13, 2009 at 8:59 AM, Darin Fisher wrote: >>> WebString exists (has for many moons now). It is just a wrapper for StringImpl, the same way WebCore::String is. It is not threadsafe due to the reference counting done for StringImpl. >>> >>> fwiw, I've checked in all the mechanics to do cheaply use StringImpl's >>> across threads. >>> >>> You have to call a method (to be named) and then you'll get back a new >>> StringImpl which uses the *same underlying buffer *and can be passed to >>> another thread. (Right now, webkit code calls StringImpl::copy() for this >>> purpose but that allocates a new buffer and copies the contents there. I >>> can't modify that method due to the fact that it is currently a threadsafe >>> call and that is relied on in some places in webkit.) >>> >>> Let me know if you want/need more details about it. >>> >>> Dave >>> >> >> Would it be better to revisit allowing WebStrings to be transported across >> the IPC barrier (with this new code) or should we go the NullableString16 >> route? I'm now leaning towards the latter because the common case is that >> we _don't_ want to allow null strings. Also it will be similar to string16, >> so it should be more familiar to Chromium developers. Wrapping WebStrings >> might be a bit faster though, since it won't require an additional string >> copy from string16 to and from StringImpl. >> > > > Hmm... > > I'd probably go with NullableString16. If we find the copy overhead to > matter, then we could always optimize later. > > Another issue: Do we want to add conversion methods on WebString? My > first reaction is to try to avoid it... > Hm. I actually thought we should have implicit conversion methods like we do for string16. What's the problem with doing so besides a little bit of bloat? If we're not doing implicit conversion, I'm actually not sure if there's a point to adding NullableString16's since the code will be equally ugly as explicitly having a boolean. J --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: base::string16 / WebCore::String incompatibility
On Mon, Jul 13, 2009 at 12:59 PM, Jeremy Orlow wrote: > On Mon, Jul 13, 2009 at 12:54 PM, Darin Fisher wrote: > >> On Mon, Jul 13, 2009 at 12:29 PM, Jeremy Orlow wrote: >> >>> On Mon, Jul 13, 2009 at 12:20 PM, David Levin wrote: >>> On Mon, Jul 13, 2009 at 8:59 AM, Darin Fisher wrote: > > WebString exists (has for many moons now). It is just a wrapper for > StringImpl, > the same way WebCore::String is. It is not threadsafe due to the > reference > counting done for StringImpl. > fwiw, I've checked in all the mechanics to do cheaply use StringImpl's across threads. You have to call a method (to be named) and then you'll get back a new StringImpl which uses the *same underlying buffer *and can be passed to another thread. (Right now, webkit code calls StringImpl::copy() for this purpose but that allocates a new buffer and copies the contents there. I can't modify that method due to the fact that it is currently a threadsafe call and that is relied on in some places in webkit.) Let me know if you want/need more details about it. Dave >>> >>> Would it be better to revisit allowing WebStrings to be transported >>> across the IPC barrier (with this new code) or should we go the >>> NullableString16 route? I'm now leaning towards the latter because the >>> common case is that we _don't_ want to allow null strings. Also it will be >>> similar to string16, so it should be more familiar to Chromium developers. >>> Wrapping WebStrings might be a bit faster though, since it won't require an >>> additional string copy from string16 to and from StringImpl. >>> >> >> >> Hmm... >> >> I'd probably go with NullableString16. If we find the copy overhead to >> matter, then we could always optimize later. >> >> Another issue: Do we want to add conversion methods on WebString? My >> first reaction is to try to avoid it... >> > > Hm. I actually thought we should have implicit conversion methods like we > do for string16. What's the problem with doing so besides a little bit of > bloat? > > If we're not doing implicit conversion, I'm actually not sure if there's a > point to adding NullableString16's since the code will be equally ugly as > explicitly having a boolean. > > J > Yeah, that's a good point. Implicit conversion methods it is then. -Darin --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
