Add a nullable string16 class to base. It combines a string16 + a null param...

Add a nullable string16 class to base.  It combines a string16 + a null param
in order to cover all the possible states of a WebKit string.  For strings
where the null state is not meaninfully different from the empty state, this
class should NOT be used.  There are, however, some cases where we do need to
track null.  LocalStorage is an example.  This class should be a fairly light
weight way to do so.  This change also adds implicit conversion to and from
WebStrings.

This also switches LocalStorage's IPCs over to using this new class.

BUG=17343
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=24574

[darin (slow to review), 2009-08-26 06:54:10]

http://codereview.chromium.org/174484/diff/1/5
File base/nullable_string16.h (right):

http://codereview.chromium.org/174484/diff/1/5#newcode10
Line 10: // In WebKit::WebStrings, being empty is distinguishable from being
null.
i'd leave off this first sentence.  i don't think it adds much, and it is nice
to avoid references to WebKit from base/ since WebKit could change causing this
comment to become stale.

http://codereview.chromium.org/174484/diff/1/5#newcode19
Line 19: NullableString16& operator=(const NullableString16& s);
i don't think you need to implement the copying ctor or assignment operator. 
the default ones generated by the compiler should be equivalent.

i would then just implement the other two constructors here in the header file,
avoiding the need for the .cc file.

http://codereview.chromium.org/174484/diff/1/6
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/174484/diff/1/6#newcode8
Line 8: #include "base/nullable_string16.h"
nit: alphabetize

http://codereview.chromium.org/174484/diff/1/6#newcode266
Line 266: NullableString16 key = storage_area->key(index);
nit: const NullableString16& key = ...

this way there is (for sure) no extra copy.  otherwise, i think you are relying
on the compiler to eliminate one copy.

you could also just not have the local variable.

http://codereview.chromium.org/174484/diff/1/6#newcode286
Line 286: NullableString16 value = storage_area->getItem(key);
ditto

http://codereview.chromium.org/174484/diff/1/8
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/174484/diff/1/8#newcode1591
Line 1591: // Traits for NullableString16
i think this should go in ipc/ipc_message_utils.h (alongside the one for
string16) or perhaps common_param_traits.h (if we only want to use this in
chrome land).

[jorlow, 2009-08-26 08:42:03]

New patch uploaded.

http://codereview.chromium.org/174484/diff/1/5
File base/nullable_string16.h (right):

http://codereview.chromium.org/174484/diff/1/5#newcode10
Line 10: // In WebKit::WebStrings, being empty is distinguishable from being
null.
On 2009/08/26 06:54:10, darin wrote:
> i'd leave off this first sentence.  i don't think it adds much, and it is nice
> to avoid references to WebKit from base/ since WebKit could change causing
this
> comment to become stale.

Done.

http://codereview.chromium.org/174484/diff/1/5#newcode19
Line 19: NullableString16& operator=(const NullableString16& s);
On 2009/08/26 06:54:10, darin wrote:
> i don't think you need to implement the copying ctor or assignment operator. 
> the default ones generated by the compiler should be equivalent.
> 
> i would then just implement the other two constructors here in the header
file,
> avoiding the need for the .cc file.

Done.

http://codereview.chromium.org/174484/diff/1/6
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/174484/diff/1/6#newcode8
Line 8: #include "base/nullable_string16.h"
On 2009/08/26 06:54:10, darin wrote:
> nit: alphabetize

Done.

http://codereview.chromium.org/174484/diff/1/6#newcode266
Line 266: NullableString16 key = storage_area->key(index);
On 2009/08/26 06:54:10, darin wrote:
> nit: const NullableString16& key = ...
> 
> this way there is (for sure) no extra copy.  otherwise, i think you are
relying
> on the compiler to eliminate one copy.
> 
> you could also just not have the local variable.

It'd surprise me if the compiler would do something different whether or not
there's a const there in this case, but I don't really care enough to look at
the disassembly or anything.

http://codereview.chromium.org/174484/diff/1/8
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/174484/diff/1/8#newcode1591
Line 1591: // Traits for NullableString16
On 2009/08/26 06:54:10, darin wrote:
> i think this should go in ipc/ipc_message_utils.h (alongside the one for
> string16) or perhaps common_param_traits.h (if we only want to use this in
> chrome land).

Done.

[Mark Mentovai, 2009-08-26 15:23:47]

LGTM

Weird that this is a different codereview "issue" than the one I reviewed
yesterday.  What happened?

http://codereview.chromium.org/174484/diff/1013/1021
File webkit/api/public/WebString.h (right):

http://codereview.chromium.org/174484/diff/1013/1021#newcode40
Line 40: #include <base/nullable_string16.h>
Sort.

[darin (slow to review), 2009-08-26 16:29:28]

http://codereview.chromium.org/174484/diff/1013/1016
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/174484/diff/1013/1016#newcode266
Line 266: const NullableString16 key = storage_area->key(index);
const NullableString16& <- reference is key

[jorlow, 2009-08-26 17:04:49]

I decided that it'd be best if this CL included the changes to use
NullableString16 class.  Unfortunately, I had changes touching
render_messages_inernal.h already in that svn client, so I had to move it to
another.  Sorry.

http://codereview.chromium.org/174484/diff/1013/1016
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/174484/diff/1013/1016#newcode266
Line 266: const NullableString16 key = storage_area->key(index);
On 2009/08/26 16:29:29, darin wrote:
> const NullableString16& <- reference is key

Ohhh...gotcha.

http://codereview.chromium.org/174484/diff/1013/1021
File webkit/api/public/WebString.h (right):

http://codereview.chromium.org/174484/diff/1013/1021#newcode40
Line 40: #include <base/nullable_string16.h>
On 2009/08/26 15:23:48, Mark Mentovai wrote:
> Sort.

sorry