[chromium-dev] Re: base::string16 / WebCore::String incompatibility
On Fri, Jul 10, 2009 at 5:04 PM, Jeremy Orlowjor...@chromium.org 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 bre...@chromium.org wrote: On Fri, Jul 10, 2009 at 5:04 PM, Jeremy Orlowjor...@chromium.org 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 da...@chromium.org wrote: On Mon, Jul 13, 2009 at 8:20 AM, Brett Wilson bre...@chromium.org wrote: On Fri, Jul 10, 2009 at 5:04 PM, Jeremy Orlowjor...@chromium.org 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 Orlowjor...@chromium.org wrote: On Mon, Jul 13, 2009 at 8:59 AM, Darin Fisher da...@chromium.org wrote: On Mon, Jul 13, 2009 at 8:20 AM, Brett Wilson bre...@chromium.org wrote: On Fri, Jul 10, 2009 at 5:04 PM, Jeremy Orlowjor...@chromium.org 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 bre...@chromium.org wrote: On Mon, Jul 13, 2009 at 10:05 AM, Jeremy Orlowjor...@chromium.org wrote: On Mon, Jul 13, 2009 at 8:59 AM, Darin Fisher da...@chromium.org wrote: On Mon, Jul 13, 2009 at 8:20 AM, Brett Wilson bre...@chromium.org wrote: On Fri, Jul 10, 2009 at 5:04 PM, Jeremy Orlowjor...@chromium.org 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 Orlowjor...@chromium.org wrote: On Mon, Jul 13, 2009 at 10:08 AM, Brett Wilson bre...@chromium.org 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 sh...@chromium.org wrote: On Mon, Jul 13, 2009 at 10:10 AM, Jeremy Orlowjor...@chromium.org wrote: On Mon, Jul 13, 2009 at 10:08 AM, Brett Wilson bre...@chromium.org 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 12:29 PM, Jeremy Orlow jor...@chromium.org wrote: On Mon, Jul 13, 2009 at 12:20 PM, David Levin le...@google.com wrote: On Mon, Jul 13, 2009 at 8:59 AM, Darin Fisher da...@chromium.org 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 da...@chromium.org wrote: On Mon, Jul 13, 2009 at 12:29 PM, Jeremy Orlow jor...@chromium.orgwrote: On Mon, Jul 13, 2009 at 12:20 PM, David Levin le...@google.com wrote: On Mon, Jul 13, 2009 at 8:59 AM, Darin Fisher da...@chromium.orgwrote: 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 jor...@chromium.org wrote: On Mon, Jul 13, 2009 at 12:54 PM, Darin Fisher da...@chromium.org wrote: On Mon, Jul 13, 2009 at 12:29 PM, Jeremy Orlow jor...@chromium.orgwrote: On Mon, Jul 13, 2009 at 12:20 PM, David Levin le...@google.com wrote: On Mon, Jul 13, 2009 at 8:59 AM, Darin Fisher da...@chromium.orgwrote: 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 -~--~~~~--~~--~--~---
