Add IPC serialization for DictionaryValue, with unit tests.

Add IPC serialization for DictionaryValue and ListValue, with unit tests.

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

[Matt Perry, 2009-05-19 02:15:03]

Actually, hold off on this review.  I found out I can't use DictionaryValue as a
param to an IPC message because it doesn't have a copy constructor.  I need to
fix this up some more.

[Matt Perry, 2009-05-19 20:09:57]

OK, this is ready for a look.  I'm using linked_ptr to hold the json values so
the copy semantics for ipc params work.

[tony, 2009-05-19 21:47:27]

Drive by question: Is serializing/deserializing to json too slow?

[tony, 2009-05-19 22:06:55]

http://codereview.chromium.org/115491/diff/1014/8
File chrome/common/ipc_message_utils.cc (right):

http://codereview.chromium.org/115491/diff/1014/8#newcode270
Line 270: WriteValue(m, subval);
Since this is recursive, you need to make sure the user can't overflow the
stack.

http://codereview.chromium.org/115491/diff/1014/8#newcode283
Line 283: WriteValue(m, subval);
Same as above.

[cpu_(ooo_6.6-7.5), 2009-05-19 23:35:40]

http://codereview.chromium.org/115491/diff/1014/8
File chrome/common/ipc_message_utils.cc (right):

http://codereview.chromium.org/115491/diff/1014/8#newcode343
Line 343: *value = val.release();
so when do we call release() ? for example see line 335, 340

http://codereview.chromium.org/115491/diff/1014/8#newcode361
Line 361: return false;
NOTREACHED() << blah 
here would be sweet too.

http://codereview.chromium.org/115491/diff/1014/8#newcode372
Line 372: bool ParamTraits< linked_ptr<DictionaryValue> >::Read(
for my education, what is the reason to use linker_ptr<Foo> for these types?
seems that you are passing a pointer to a linked_ptr... would a Foo* work?

[Matt Perry, 2009-05-19 23:46:00]

http://codereview.chromium.org/115491/diff/1014/8
File chrome/common/ipc_message_utils.cc (right):

http://codereview.chromium.org/115491/diff/1014/8#newcode343
Line 343: *value = val.release();
On 2009/05/19 23:35:40, cpu wrote:
> so when do we call release() ? for example see line 335, 340

We don't in those cases because we want the DictionaryValue to be deleted.

http://codereview.chromium.org/115491/diff/1014/8#newcode361
Line 361: return false;
On 2009/05/19 23:35:40, cpu wrote:
> NOTREACHED() << blah 
> here would be sweet too.

I can try that, but I think it might make my unit test barf (the "corrupt" case
has a type of 99).

http://codereview.chromium.org/115491/diff/1014/8#newcode372
Line 372: bool ParamTraits< linked_ptr<DictionaryValue> >::Read(
On 2009/05/19 23:35:40, cpu wrote:
> for my education, what is the reason to use linker_ptr<Foo> for these types?
> seems that you are passing a pointer to a linked_ptr... would a Foo* work?

I'm not super happy with using linked_ptr.  Here's the full story:

I wanted to use plain DictionaryValue, but to use it as a parameter in IPC
messages, it needs a copy constructor (since the message can outlive the stack
frame of the Send call).  I decided against adding a copy constructor to
DictionaryValue/ListValue.

Using a pointer type would work, but would require manual memory management at
the end points.  Message senders would need to ensure their DictionaryValue
lived long enough to be sent, yet also got cleaned up afterwards.  Message
receivers would need to manually delete the DictionaryValue after they handled
the message.

So, linked_ptr, the ugly compromise.

[cpu_(ooo_6.6-7.5), 2009-05-20 00:20:31]

Ignore the NOTREACHED then.

LGTM


On 2009/05/19 23:46:00, Matt Perry wrote:
> http://codereview.chromium.org/115491/diff/1014/8
> File chrome/common/ipc_message_utils.cc (right):
> 
> http://codereview.chromium.org/115491/diff/1014/8#newcode343
> Line 343: *value = val.release();
> On 2009/05/19 23:35:40, cpu wrote:
> > so when do we call release() ? for example see line 335, 340
> 
> We don't in those cases because we want the DictionaryValue to be deleted.
> 
> http://codereview.chromium.org/115491/diff/1014/8#newcode361
> Line 361: return false;
> On 2009/05/19 23:35:40, cpu wrote:
> > NOTREACHED() << blah 
> > here would be sweet too.
> 
> I can try that, but I think it might make my unit test barf (the "corrupt"
case
> has a type of 99).
> 
> http://codereview.chromium.org/115491/diff/1014/8#newcode372
> Line 372: bool ParamTraits< linked_ptr<DictionaryValue> >::Read(
> On 2009/05/19 23:35:40, cpu wrote:
> > for my education, what is the reason to use linker_ptr<Foo> for these types?
> > seems that you are passing a pointer to a linked_ptr... would a Foo* work?
> 
> I'm not super happy with using linked_ptr.  Here's the full story:
> 
> I wanted to use plain DictionaryValue, but to use it as a parameter in IPC
> messages, it needs a copy constructor (since the message can outlive the stack
> frame of the Send call).  I decided against adding a copy constructor to
> DictionaryValue/ListValue.
> 
> Using a pointer type would work, but would require manual memory management at
> the end points.  Message senders would need to ensure their DictionaryValue
> lived long enough to be sent, yet also got cleaned up afterwards.  Message
> receivers would need to manually delete the DictionaryValue after they handled
> the message.
> 
> So, linked_ptr, the ugly compromise.

[Matt Perry, 2009-05-20 19:58:13]

I uploaded a new snapshot to get rid of the linked_ptr usage (requires
http://codereview.chromium.org/112042 ).

http://codereview.chromium.org/115491/diff/1014/8
File chrome/common/ipc_message_utils.cc (right):

http://codereview.chromium.org/115491/diff/1014/8#newcode270
Line 270: WriteValue(m, subval);
On 2009/05/19 22:06:55, tony wrote:
> Since this is recursive, you need to make sure the user can't overflow the
> stack.

Done.

[tony, 2009-05-20 21:01:08]

LGTM.

[cpu_(ooo_6.6-7.5), 2009-05-20 21:49:17]

LGTM

On 2009/05/20 21:01:08, tony wrote:
> LGTM.