Add dbus::PopDataAsValue

Add dbus::PopDataAsValue

BUG=chromium-os:16557
TEST=dbus_unittests --gtest_filter="ValuesUtilTest.*"


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

[satorux1, 2012-03-15 23:29:04]

http://codereview.chromium.org/9702094/diff/2001/dbus/values_util.h
File dbus/values_util.h (right):

http://codereview.chromium.org/9702094/diff/2001/dbus/values_util.h#newcode20
dbus/values_util.h:20: base::Value* PopDataAsValue(MessageReader* reader);
Hmm, this looks like over engineering. 

I thought we wanted to add PopStringToVariantDictionary()  in MessageReader?

[hashimoto, 2012-03-15 23:38:18]

http://codereview.chromium.org/9702094/diff/2001/dbus/values_util.h
File dbus/values_util.h (right):

http://codereview.chromium.org/9702094/diff/2001/dbus/values_util.h#newcode20
dbus/values_util.h:20: base::Value* PopDataAsValue(MessageReader* reader);
On 2012/03/15 23:29:04, satorux1 wrote:
> Hmm, this looks like over engineering. 
> 
> I thought we wanted to add PopStringToVariantDictionary()  in MessageReader?

I thought having a complete tool to pop data as base::Value is good.
But it's true that no one needs to pop a struct or an array as base::Value now…

[hashimoto, 2012-03-16 02:45:52]

http://codereview.chromium.org/9702094/diff/2001/dbus/values_util.h
File dbus/values_util.h (right):

http://codereview.chromium.org/9702094/diff/2001/dbus/values_util.h#newcode20
dbus/values_util.h:20: base::Value* PopDataAsValue(MessageReader* reader);
On 2012/03/15 23:38:18, hashimoto wrote:
> On 2012/03/15 23:29:04, satorux1 wrote:
> > Hmm, this looks like over engineering. 
> > 
> > I thought we wanted to add PopStringToVariantDictionary()  in MessageReader?
> 
> I thought having a complete tool to pop data as base::Value is good.
> But it's true that no one needs to pop a struct or an array as base::Value
now…

I once tried to write a simpler function which can pop string-to-variant
dictionaries only.
But the situation turns out to be more complicated.

Looking at Shill's code, Shill looks like outputting string-to-variant
dictionaries where the variants are containing either basic types, string array,
string-to-string map and string-to-variant.
(http://git.chromium.org/gitweb/?p=chromiumos/platform/shill.git;a=blob;f=dbus...)

It means that even if we decide to have a simple PopStringToVariantDictionary
function, it should be able to pop a complex string-to-variant dictionary with
basic types, array and dictionary (string-to-string and string-to-variant).
Since the function should be able to pop such complex data, the difference
between PopStringToVariantDictionary and PopDataAsValue is relatively small.

So I began to think PopStringToVariantDictionary is not so simpler than
PopDataAsValue and we better have PopDataAsValue, not
PopStringToVariantDictionary.
What do you think?

[satorux1, 2012-03-16 16:23:59]

http://codereview.chromium.org/9702094/diff/2001/dbus/values_util.h
File dbus/values_util.h (right):

http://codereview.chromium.org/9702094/diff/2001/dbus/values_util.h#newcode20
dbus/values_util.h:20: base::Value* PopDataAsValue(MessageReader* reader);
On 2012/03/16 02:45:52, hashimoto wrote:
> On 2012/03/15 23:38:18, hashimoto wrote:
> > On 2012/03/15 23:29:04, satorux1 wrote:
> > > Hmm, this looks like over engineering. 
> > > 
> > > I thought we wanted to add PopStringToVariantDictionary()  in
MessageReader?
> > 
> > I thought having a complete tool to pop data as base::Value is good.
> > But it's true that no one needs to pop a struct or an array as base::Value
> now…
> 
> I once tried to write a simpler function which can pop string-to-variant
> dictionaries only.
> But the situation turns out to be more complicated.
> 
> Looking at Shill's code, Shill looks like outputting string-to-variant
> dictionaries where the variants are containing either basic types, string
array,
> string-to-string map and string-to-variant.
>
(http://git.chromium.org/gitweb/?p=chromiumos/platform/shill.git;a=blob;f=dbus...)
> 
> It means that even if we decide to have a simple PopStringToVariantDictionary
> function, it should be able to pop a complex string-to-variant dictionary with
> basic types, array and dictionary (string-to-string and string-to-variant).
> Since the function should be able to pop such complex data, the difference
> between PopStringToVariantDictionary and PopDataAsValue is relatively small.
> 
> So I began to think PopStringToVariantDictionary is not so simpler than
> PopDataAsValue and we better have PopDataAsValue, not
> PopStringToVariantDictionary.
> What do you think?

Makes sense.

http://codereview.chromium.org/9702094/diff/1002/dbus/values_util.cc
File dbus/values_util.cc (right):

http://codereview.chromium.org/9702094/diff/1002/dbus/values_util.cc#newcode38
dbus/values_util.cc:38: DCHECK(reader->GetDataType() == Message::DICT_ENTRY);
DCHECK_EQ

http://codereview.chromium.org/9702094/diff/1002/dbus/values_util.cc#newcode49
dbus/values_util.cc:49: // If the type of keys is not STRING, convert it to
string.
Why? I thought DictionaryValue can have non-strings as keys.

[hashimoto, 2012-03-16 16:55:26]

http://codereview.chromium.org/9702094/diff/1002/dbus/values_util.cc
File dbus/values_util.cc (right):

http://codereview.chromium.org/9702094/diff/1002/dbus/values_util.cc#newcode38
dbus/values_util.cc:38: DCHECK(reader->GetDataType() == Message::DICT_ENTRY);
On 2012/03/16 16:23:59, satorux1 wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/9702094/diff/1002/dbus/values_util.cc#newcode49
dbus/values_util.cc:49: // If the type of keys is not STRING, convert it to
string.
On 2012/03/16 16:23:59, satorux1 wrote:
> Why? I thought DictionaryValue can have non-strings as keys.
DictionaryValue's class comment says its keys are string, it there some trick to
use non-strings as keys?
http://code.google.com/codesearch#OAMlx_jo-ck/src/base/values.h&exact_package...

[stevenjb, 2012-03-16 17:51:59]

http://codereview.chromium.org/9702094/diff/1002/dbus/values_util.cc
File dbus/values_util.cc (right):

http://codereview.chromium.org/9702094/diff/1002/dbus/values_util.cc#newcode49
dbus/values_util.cc:49: // If the type of keys is not STRING, convert it to
string.
On 2012/03/16 16:55:26, hashimoto wrote:
> On 2012/03/16 16:23:59, satorux1 wrote:
> > Why? I thought DictionaryValue can have non-strings as keys.
> DictionaryValue's class comment says its keys are string, it there some trick
to
> use non-strings as keys?
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/base/values.h&exact_package...

No, this is correct, DictionaryValue keys are always strings:
typedef std::map<std::string, Value*> ValueMap;

Using JSOMWriter struck me as odd at first, but I can't think of anything better
to use since base::Value doesn't have anything similar. Might be worth adding a
comment to that effect.

http://codereview.chromium.org/9702094/diff/4/dbus/values_util.cc
File dbus/values_util.cc (right):

http://codereview.chromium.org/9702094/diff/4/dbus/values_util.cc#newcode17
dbus/values_util.cc:17: // Returns if |value| is exactly representable by double
or not.
nit: s/if/whether/

http://codereview.chromium.org/9702094/diff/4/dbus/values_util.cc#newcode112
dbus/values_util.cc:112: "|value| is not exactly representable by double";
Print key/value here for debugging if this ever comes up.

http://codereview.chromium.org/9702094/diff/4/dbus/values_util.cc#newcode121
dbus/values_util.cc:121: "|value| is not exactly representable by double";
Same.

[hashimoto, 2012-03-16 18:19:44]

http://codereview.chromium.org/9702094/diff/1002/dbus/values_util.cc
File dbus/values_util.cc (right):

http://codereview.chromium.org/9702094/diff/1002/dbus/values_util.cc#newcode49
dbus/values_util.cc:49: // If the type of keys is not STRING, convert it to
string.
On 2012/03/16 17:51:59, stevenjb (chromium) wrote:
> On 2012/03/16 16:55:26, hashimoto wrote:
> > On 2012/03/16 16:23:59, satorux1 wrote:
> > > Why? I thought DictionaryValue can have non-strings as keys.
> > DictionaryValue's class comment says its keys are string, it there some
trick
> to
> > use non-strings as keys?
> >
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/base/values.h&exact_package...
> 
> No, this is correct, DictionaryValue keys are always strings:
> typedef std::map<std::string, Value*> ValueMap;
> 
> Using JSOMWriter struck me as odd at first, but I can't think of anything
better
> to use since base::Value doesn't have anything similar. Might be worth adding
a
> comment to that effect.
> 

Done.

http://codereview.chromium.org/9702094/diff/4/dbus/values_util.cc
File dbus/values_util.cc (right):

http://codereview.chromium.org/9702094/diff/4/dbus/values_util.cc#newcode17
dbus/values_util.cc:17: // Returns if |value| is exactly representable by double
or not.
On 2012/03/16 17:51:59, stevenjb (chromium) wrote:
> nit: s/if/whether/

Done.

http://codereview.chromium.org/9702094/diff/4/dbus/values_util.cc#newcode112
dbus/values_util.cc:112: "|value| is not exactly representable by double";
On 2012/03/16 17:51:59, stevenjb (chromium) wrote:
> Print key/value here for debugging if this ever comes up.

Done.

http://codereview.chromium.org/9702094/diff/4/dbus/values_util.cc#newcode121
dbus/values_util.cc:121: "|value| is not exactly representable by double";
On 2012/03/16 17:51:59, stevenjb (chromium) wrote:
> Same.

Done.

[satorux1, 2012-03-16 18:28:11]

looking good!

http://codereview.chromium.org/9702094/diff/1002/dbus/values_util.cc
File dbus/values_util.cc (right):

http://codereview.chromium.org/9702094/diff/1002/dbus/values_util.cc#newcode49
dbus/values_util.cc:49: // If the type of keys is not STRING, convert it to
string.
On 2012/03/16 16:55:26, hashimoto wrote:
> On 2012/03/16 16:23:59, satorux1 wrote:
> > Why? I thought DictionaryValue can have non-strings as keys.
> DictionaryValue's class comment says its keys are string, it there some trick
to
> use non-strings as keys?
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/base/values.h&exact_package...

Oh I see. Then it's likely JSON's spec. Please document about the behavior in
the function comment.

http://codereview.chromium.org/9702094/diff/3015/dbus/values_util_unittest.cc
File dbus/values_util_unittest.cc (right):

http://codereview.chromium.org/9702094/diff/3015/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:255: }
we might want to test a dictionary with non-string keys

http://codereview.chromium.org/9702094/diff/3015/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:261: double keys[length];
I may be wrong, but this is probably not allowed in C++. please use std::vector.

http://codereview.chromium.org/9702094/diff/3015/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:292: EXPECT_TRUE(value->Equals(&dictionary_value));
do we have a test for int64 that cannot be precisely represented in dobule?

[stevenjb, 2012-03-16 18:37:41]

http://codereview.chromium.org/9702094/diff/3015/dbus/values_util_unittest.cc
File dbus/values_util_unittest.cc (right):

http://codereview.chromium.org/9702094/diff/3015/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:259: static const int32 kValues[] = {0, 1, 1, 2, 3,
5, 8, 13, 21};
Avoid static here, it's not necessary and could cause problems.

http://codereview.chromium.org/9702094/diff/3015/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:260: static const size_t length =
arraysize(kValues);
Here too.

http://codereview.chromium.org/9702094/diff/3015/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:261: double keys[length];
On 2012/03/16 18:28:12, satorux1 wrote:
> I may be wrong, but this is probably not allowed in C++. please use
std::vector.
> 
Yes, you could use arraysize(kValues) directly, but better just to use a vector.

[hashimoto, 2012-03-16 19:21:41]

http://codereview.chromium.org/9702094/diff/1002/dbus/values_util.cc
File dbus/values_util.cc (right):

http://codereview.chromium.org/9702094/diff/1002/dbus/values_util.cc#newcode49
dbus/values_util.cc:49: // If the type of keys is not STRING, convert it to
string.
On 2012/03/16 18:28:11, satorux1 wrote:
> On 2012/03/16 16:55:26, hashimoto wrote:
> > On 2012/03/16 16:23:59, satorux1 wrote:
> > > Why? I thought DictionaryValue can have non-strings as keys.
> > DictionaryValue's class comment says its keys are string, it there some
trick
> to
> > use non-strings as keys?
> >
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/base/values.h&exact_package...
> 
> Oh I see. Then it's likely JSON's spec. Please document about the behavior in
> the function comment.

Done.

http://codereview.chromium.org/9702094/diff/3015/dbus/values_util_unittest.cc
File dbus/values_util_unittest.cc (right):

http://codereview.chromium.org/9702094/diff/3015/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:255: }
On 2012/03/16 18:28:12, satorux1 wrote:
> we might want to test a dictionary with non-string keys

PopDoubleToIntDictionary is doing it.

http://codereview.chromium.org/9702094/diff/3015/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:259: static const int32 kValues[] = {0, 1, 1, 2, 3,
5, 8, 13, 21};
On 2012/03/16 18:37:41, stevenjb (chromium) wrote:
> Avoid static here, it's not necessary and could cause problems.

Done.

http://codereview.chromium.org/9702094/diff/3015/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:260: static const size_t length =
arraysize(kValues);
On 2012/03/16 18:37:41, stevenjb (chromium) wrote:
> Here too.

Done.

http://codereview.chromium.org/9702094/diff/3015/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:261: double keys[length];
On 2012/03/16 18:37:41, stevenjb (chromium) wrote:
> On 2012/03/16 18:28:12, satorux1 wrote:
> > I may be wrong, but this is probably not allowed in C++. please use
> std::vector.
> > 
> Yes, you could use arraysize(kValues) directly, but better just to use a
vector.

Done.

http://codereview.chromium.org/9702094/diff/3015/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:292: EXPECT_TRUE(value->Equals(&dictionary_value));
On 2012/03/16 18:28:12, satorux1 wrote:
> do we have a test for int64 that cannot be precisely represented in dobule?

Added PopExtremelyLargeIntegers.

[satorux1, 2012-03-16 19:47:20]

LGTM with nits.

http://codereview.chromium.org/9702094/diff/9003/dbus/values_util_unittest.cc
File dbus/values_util_unittest.cc (right):

http://codereview.chromium.org/9702094/diff/9003/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:137: const int64 kInt64Value = -123456789012345689;
looks dangerous. you may need to add LL suffix.

http://codereview.chromium.org/9702094/diff/9003/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:139: const uint64 kUint64Value =
9876543210987654321U;
ULL ?

http://codereview.chromium.org/9702094/diff/9003/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:293: values.push_back(8);
const int32 kValues[] = {0, 1, 1, 2, 3, 5, 8, 13, 21};
std::vector<int32> values(kValues, kValues + arraysize(kValues);

would work

[hashimoto, 2012-03-16 20:18:47]

http://codereview.chromium.org/9702094/diff/9003/dbus/values_util_unittest.cc
File dbus/values_util_unittest.cc (right):

http://codereview.chromium.org/9702094/diff/9003/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:137: const int64 kInt64Value = -123456789012345689;
On 2012/03/16 19:47:20, satorux1 wrote:
> looks dangerous. you may need to add LL suffix.

Thanks, done.

http://codereview.chromium.org/9702094/diff/9003/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:139: const uint64 kUint64Value =
9876543210987654321U;
On 2012/03/16 19:47:20, satorux1 wrote:
> ULL ?

Done.

http://codereview.chromium.org/9702094/diff/9003/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:293: values.push_back(8);
On 2012/03/16 19:47:20, satorux1 wrote:
> const int32 kValues[] = {0, 1, 1, 2, 3, 5, 8, 13, 21};
> std::vector<int32> values(kValues, kValues + arraysize(kValues);
> 
> would work
> 

Done.

[commit-bot: I haz the power, 2012-03-16 21:28:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/9702094/16001

[commit-bot: I haz the power, 2012-03-16 23:08:44]

Change committed as 127287