In ShillClientHelper use a{sv} for property dictionaries

In ShillClientHelper use a{sv} for property dictionaries

This is dependent on Shill change:
https://chromium-review.googlesource.com/242491
(This change won't work without the Shill change, but it also won't make
anything any more broken than it already is).

BUG=449273
Committed: https://crrev.com/328b02a2657a393da0f532a4fba3872c225a9e95
Cr-Commit-Position: refs/heads/master@{#313101}

[stevenjb, 2015-01-22 23:14:30]

stevenjb@chromium.org changed reviewers:
+ benchan@chromium.org, hashimoto@chromium.org, pneubeck@chromium.org,
pstew@chromium.org

[stevenjb, 2015-01-22 23:14:31]

I tested this with both StaticIPConfig and with Cellular.APN (using the changes
in https://codereview.chromium.org/700383008/) and both now work with
SetProperties, hooray!

https://codereview.chromium.org/867043002/diff/1/chromeos/dbus/shill_client_h...
File chromeos/dbus/shill_client_helper.cc (right):

https://codereview.chromium.org/867043002/diff/1/chromeos/dbus/shill_client_h...
chromeos/dbus/shill_client_helper.cc:226: }
The above is copy/pasted from the original code (lines 405-421)

https://codereview.chromium.org/867043002/diff/1/chromeos/dbus/shill_client_h...
chromeos/dbus/shill_client_helper.cc:424: false /* use_string_dictionaries */);
Note: this changes the default behavior, but this is only used for Shill, and
the only writable Shill dictionaries are Cellular.APN (which this is not used
for directly) and StaticIPConfig (which this is also not used for directly but
which uses the new 'default' behavior).

[hashimoto, 2015-01-23 02:53:48]

https://codereview.chromium.org/867043002/diff/1/chromeos/dbus/shill_client_h...
File chromeos/dbus/shill_client_helper.cc (right):

https://codereview.chromium.org/867043002/diff/1/chromeos/dbus/shill_client_h...
chromeos/dbus/shill_client_helper.cc:424: false /* use_string_dictionaries */);
On 2015/01/22 23:14:31, stevenjb wrote:
> Note: this changes the default behavior, but this is only used for Shill, and
> the only writable Shill dictionaries are Cellular.APN (which this is not used
> for directly) and StaticIPConfig (which this is also not used for directly but
> which uses the new 'default' behavior).

Isn't it possible to change Shill to accept a{sv} as Cellular.APN to use this
format exclusively?
Automatically switching the message format depending on the key name looks
smart, but might be confusing to understand for newcomers.

https://codereview.chromium.org/867043002/diff/1/chromeos/dbus/shill_client_h...
chromeos/dbus/shill_client_helper.cc:428: void
ShillClientHelper::AppendValueDataAsVariantEx(
"Ex" suffix here doesn't look pretty.
How about adding the use_string_dictionaries argument directly to
AppendValueDataAsVariant(), instead of having 2 methods?
The argument can be replaced with an enum to make the user code readable.

[stevenjb, 2015-01-23 18:49:21]

https://codereview.chromium.org/867043002/diff/1/chromeos/dbus/shill_client_h...
File chromeos/dbus/shill_client_helper.cc (right):

https://codereview.chromium.org/867043002/diff/1/chromeos/dbus/shill_client_h...
chromeos/dbus/shill_client_helper.cc:424: false /* use_string_dictionaries */);
On 2015/01/23 02:53:48, hashimoto wrote:
> On 2015/01/22 23:14:31, stevenjb wrote:
> > Note: this changes the default behavior, but this is only used for Shill,
and
> > the only writable Shill dictionaries are Cellular.APN (which this is not
used
> > for directly) and StaticIPConfig (which this is also not used for directly
but
> > which uses the new 'default' behavior).
> 
> Isn't it possible to change Shill to accept a{sv} as Cellular.APN to use this
> format exclusively?
> Automatically switching the message format depending on the key name looks
> smart, but might be confusing to understand for newcomers.

We're looking into that but it's going to be a pretty messy change on the Shill
side and we need an intermediate solution. (Also, Shill would have to support
both versions in the interrum which may be a hughe pain). I'll add a TODO to the
comment.

https://codereview.chromium.org/867043002/diff/1/chromeos/dbus/shill_client_h...
chromeos/dbus/shill_client_helper.cc:428: void
ShillClientHelper::AppendValueDataAsVariantEx(
On 2015/01/23 02:53:48, hashimoto wrote:
> "Ex" suffix here doesn't look pretty.
> How about adding the use_string_dictionaries argument directly to
> AppendValueDataAsVariant(), instead of having 2 methods?
> The argument can be replaced with an enum to make the user code readable.
I don't think it makes sense to change the signature just to support an
implementation change. I removed the new call from the header and renamed the
internal one instead.

[hashimoto, 2015-01-26 05:30:59]

lgtm

https://codereview.chromium.org/867043002/diff/20001/chromeos/dbus/shill_clie...
File chromeos/dbus/shill_client_helper.cc (right):

https://codereview.chromium.org/867043002/diff/20001/chromeos/dbus/shill_clie...
chromeos/dbus/shill_client_helper.cc:404: void WriteStringDictionary(const
base::DictionaryValue& dictionary,
nit: How about "Append" instead of "Write" to be consistent with other
functions? (It's up to you as "Write" seems also OK to me, though.)

[stevenjb, 2015-01-26 18:49:57]

https://codereview.chromium.org/867043002/diff/20001/chromeos/dbus/shill_clie...
File chromeos/dbus/shill_client_helper.cc (right):

https://codereview.chromium.org/867043002/diff/20001/chromeos/dbus/shill_clie...
chromeos/dbus/shill_client_helper.cc:404: void WriteStringDictionary(const
base::DictionaryValue& dictionary,
On 2015/01/26 05:30:59, hashimoto wrote:
> nit: How about "Append" instead of "Write" to be consistent with other
> functions? (It's up to you as "Write" seems also OK to me, though.)

Done.

[stevenjb, 2015-01-26 18:50:00]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-01-26 18:51:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/867043002/40001

[commit-bot: I haz the power, 2015-01-26 19:39:42]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-26 19:40:34]

Patchset 3 (id:??) landed as
https://crrev.com/328b02a2657a393da0f532a4fba3872c225a9e95
Cr-Commit-Position: refs/heads/master@{#313101}