dbus/values_util.h: Add functions to append collection type values to message.

dbus/values_util.h: Add functions to append collection type values to message.

Added functions AppendValueData and AppendValueDataAsVariant to values_util.h.
These functions are useful for writing both basic types and variant lists and
dictionaries to a message writer. Lists and dictionaries are written with type
"av" and "a{sv}", respectively.

BUG=359413
TEST=dbus_unittests,chromeos_unittests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272167

[armansito, 2014-04-03 08:15:32]

avakulenko@, wiley@: FYI

These functions cover the most generic use case of variant dictionaries and
lists. The one call-site in Chromium that I know, that explicitly uses a typed
dictionary is ShillClientHelper::AppendValueDataAsVariant, which writes the
given dictionary or list as "a{ss}" or "as". I'm not sure if changing this code
to use the new functions would break shill, as I don't know if shill would
automatically handle a dictionary of variant in that case (this is something to
test). Otherwise, we can add functions to values_util.h to append non-variant
collections to a writer if this is required by more users.

[keybuk, 2014-04-03 16:32:22]

https://codereview.chromium.org/221393004/diff/1/dbus/values_util_unittest.cc
File dbus/values_util_unittest.cc (right):

https://codereview.chromium.org/221393004/diff/1/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:452: TEST(ValuesUtilTest, AppendDictionary) {
You need test cases for the basic values too

[armansito, 2014-04-03 17:20:05]

https://codereview.chromium.org/221393004/diff/1/dbus/values_util_unittest.cc
File dbus/values_util_unittest.cc (right):

https://codereview.chromium.org/221393004/diff/1/dbus/values_util_unittest.cc...
dbus/values_util_unittest.cc:452: TEST(ValuesUtilTest, AppendDictionary) {
On 2014/04/03 16:32:23, keybuk wrote:
> You need test cases for the basic values too

Done.

[Alex Vakulenko, 2014-04-03 17:31:36]

On 2014/04/03 17:20:05, armansito wrote:
> https://codereview.chromium.org/221393004/diff/1/dbus/values_util_unittest.cc
> File dbus/values_util_unittest.cc (right):
> 
>
https://codereview.chromium.org/221393004/diff/1/dbus/values_util_unittest.cc...
> dbus/values_util_unittest.cc:452: TEST(ValuesUtilTest, AppendDictionary) {
> On 2014/04/03 16:32:23, keybuk wrote:
> > You need test cases for the basic values too
> 
> Done.

The code looks good. The only design concern I still have is the asymmetrical
nature of Pop... and Append... methods. PopDataAsValue can handle arrays and
dicts of pretty much any DBus type (like "ay", or "a{ss}") while Append.... only
support variants ("av" and "a{sv}"). What if I need to create a message call for
a method that takes "a{ss}"? I'll have to roll out my own implementation then.

Not a major concern, but still I would expect that base library functionality
like that should support common scenarios...

Just my $0.02...

[armansito, 2014-04-03 18:25:10]

On 2014/04/03 17:31:36, avakulenko wrote:
> On 2014/04/03 17:20:05, armansito wrote:
> >
https://codereview.chromium.org/221393004/diff/1/dbus/values_util_unittest.cc
> > File dbus/values_util_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/221393004/diff/1/dbus/values_util_unittest.cc...
> > dbus/values_util_unittest.cc:452: TEST(ValuesUtilTest, AppendDictionary) {
> > On 2014/04/03 16:32:23, keybuk wrote:
> > > You need test cases for the basic values too
> > 
> > Done.
> 
> The code looks good. The only design concern I still have is the asymmetrical
> nature of Pop... and Append... methods. PopDataAsValue can handle arrays and
> dicts of pretty much any DBus type (like "ay", or "a{ss}") while Append....
only
> support variants ("av" and "a{sv}"). What if I need to create a message call
for
> a method that takes "a{ss}"? I'll have to roll out my own implementation then.
> 
> Not a major concern, but still I would expect that base library functionality
> like that should support common scenarios...
> 
> Just my $0.02...

I've been thinking about this same issue. We could have a function that handles,
say, a single top level dictionary with a basic type as its value type. But
then, things start getting out of hand if you maintain dictionaries of
dictionaries, or dictionaries of lists of arbitrary depth (e.g. "a{sa{sas}}"),
in which case I think it's better to let the maintainer write the function
manually, or really just use variants.

The asymmetry you mention is a side-effect of the variant nature of base::Value,
namely, D-Bus arrays are typed, whereas base/values.h containers
(base::DictionaryValue and base::ListValue) are variant collections by default.
Without explicitly knowing the type of the output collection, it's unclear how
to infer it given a base:: collection. Should it be based on the first element?
Should nested collections be automatically handled? If a collection is malformed
(i.e. contains elements of various types), then how does the library report
errors?

I'm against bloating the library too much and the variant functions added in
this CL cover the most common use case. That said, there might be value in
adding an AppendListValueOfType and AppendDictionaryValueOfType. keybuk@, any
suggestions?

[Alex Vakulenko, 2014-04-03 18:33:39]

> I've been thinking about this same issue. We could have a function that
handles,
> say, a single top level dictionary with a basic type as its value type. But
> then, things start getting out of hand if you maintain dictionaries of
> dictionaries, or dictionaries of lists of arbitrary depth (e.g. "a{sa{sas}}"),
> in which case I think it's better to let the maintainer write the function
> manually, or really just use variants.
> 
> The asymmetry you mention is a side-effect of the variant nature of
base::Value,
> namely, D-Bus arrays are typed, whereas base/values.h containers
> (base::DictionaryValue and base::ListValue) are variant collections by
default.
> Without explicitly knowing the type of the output collection, it's unclear how
> to infer it given a base:: collection. Should it be based on the first
element?
> Should nested collections be automatically handled? If a collection is
malformed
> (i.e. contains elements of various types), then how does the library report
> errors?
> 
> I'm against bloating the library too much and the variant functions added in
> this CL cover the most common use case. That said, there might be value in
> adding an AppendListValueOfType and AppendDictionaryValueOfType. keybuk@, any
> suggestions?

Append...OfType is exactly the thing I was thinking about. I understand about
nesting and complicated types, but at least single level collection is of use.
But I'm not insisting. value_utils.h could be looked at from another angle -
being able to send base::Value* over DBus and in that regard it is almost
symmetrical. Given base::Value on a caller side, you'll get the same (almost, in
most cases) base::Value on callee's side (it will be using variants on the
wire). So it could be "good enough" in that regard.

It's up to you.

[keybuk, 2014-05-20 18:18:24]

dbus/ lgtm

There is, of course, always room to improve API (e.g. nested containers) - but
there's no reason to hold up a CL that's immediately useful just because it
doesn't do everything

[armansito, 2014-05-21 22:52:45]

The CQ bit was checked by armansito@chromium.org

[commit-bot: I haz the power, 2014-05-21 22:56:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/armansito@chromium.org/221393004/20001

[commit-bot: I haz the power, 2014-05-22 07:29:24]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-22 08:25:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-22 08:25:24]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[armansito, 2014-05-22 10:30:48]

The CQ bit was checked by armansito@chromium.org

[commit-bot: I haz the power, 2014-05-22 10:32:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/armansito@chromium.org/221393004/20001

[commit-bot: I haz the power, 2014-05-22 11:52:56]

Change committed as 272167

[Ben Chan, 2014-05-22 13:32:55]

On 2014/05/22 11:52:56, I haz the power (commit-bot) wrote:
> Change committed as 272167

armansito / avakulenko: This CL just missed the libchrome uprev train. Is the
added code needed on the Chrome OS side?  I could probably include this one to
libchrome-dbus if this patch applies and builds cleanly. WDYT?