NetworkChangeNotifierChromeos: Handle IPConfig property changes on the default network

NetworkChangeNotifierChromeos: Handle IPConfig property changes on the default network

BUG=164501
TEST=verify that dns refreshes while in connected state result in a dns change.


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

[stevenjb, 2013-03-27 22:18:51]

https://codereview.chromium.org/12634019/diff/5002/chromeos/dbus/shill_servic...
File chromeos/dbus/shill_service_client.h (right):

https://codereview.chromium.org/12634019/diff/5002/chromeos/dbus/shill_servic...
chromeos/dbus/shill_service_client.h:51: const std::string& ipconfig_path,
Do we really want to add this to AddService()? It doesn't seem like an essential
property. Why not just use SetServiceProperty() when we want to set it?

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
chromeos/network/network_state.cc:15: for (size_t i = 0; i !=
string_list.GetSize(); ++i) {
nit: < instead of !=

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
chromeos/network/network_state.cc:21: *result += name_server;
personal nit (feel free to ignore): result->append() seems more clear than
*result +=

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
chromeos/network/network_state.cc:49: } else if (key ==
flimflam::kAddressProperty) {
Please add a comment:
// Note: This does not correspond an actual Shill.Service property

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
chromeos/network/network_state.cc:51: } else if (key ==
flimflam::kNameServersProperty) {
Same comment here

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:401: if (key !=
flimflam::kSignalStrengthProperty)
nit: add to if clause above

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
File chromeos/network/network_state_handler_observer.h (right):

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
chromeos/network/network_state_handler_observer.h:38: // changes. If you are
interested in those events, use
nit: s/If you are/If (avoid "you" in comments)

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
File chromeos/network/network_state_handler_unittest.cc (right):

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
chromeos/network/network_state_handler_unittest.cc:179: "", add_to_watchlist);
I don't like adding all of these "", especially since I forsee adding more tests
that use AddService() (see my earlier comment)

[gauravsh, 2013-03-27 23:16:03]

https://codereview.chromium.org/12634019/diff/5002/chromeos/dbus/shill_servic...
File chromeos/dbus/shill_service_client.h (right):

https://codereview.chromium.org/12634019/diff/5002/chromeos/dbus/shill_servic...
chromeos/dbus/shill_service_client.h:51: const std::string& ipconfig_path,
On 2013/03/27 22:18:52, stevenjb (chromium) wrote:
> Do we really want to add this to AddService()? It doesn't seem like an
essential
> property. Why not just use SetServiceProperty() when we want to set it?

This is needed because there are 2 distinct scenarios that I'd like to test. We
have 2 separate code paths for IPConfig requests - one when we add the service
the first time with IPConfig set, and the second when a service's IPConfig
property changes (or gets added).

Open to alternative suggestions. How about a separate AddService version that
takes the IPConfig path?

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
chromeos/network/network_state.cc:13: bool ConvertListValueToString(const
base::ListValue& string_list,
BTW, this is lifted off of cros_network_functions.cc|h. At some point, we might
consider moving such helpers (like Get*Value() in managed_state.g|cc) into a
separate file. (Maybe even now?)

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
chromeos/network/network_state.cc:15: for (size_t i = 0; i !=
string_list.GetSize(); ++i) {
On 2013/03/27 22:18:52, stevenjb (chromium) wrote:
> nit: < instead of !=

Done.

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
chromeos/network/network_state.cc:21: *result += name_server;
On 2013/03/27 22:18:52, stevenjb (chromium) wrote:
> personal nit (feel free to ignore): result->append() seems more clear than
> *result +=

Done.

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
chromeos/network/network_state.cc:49: } else if (key ==
flimflam::kAddressProperty) {
On 2013/03/27 22:18:52, stevenjb (chromium) wrote:
> Please add a comment:
> // Note: This does not correspond an actual Shill.Service property

Added a comment to network_state.h.

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
chromeos/network/network_state.cc:51: } else if (key ==
flimflam::kNameServersProperty) {
On 2013/03/27 22:18:52, stevenjb (chromium) wrote:
> Same comment here

Done.

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:401: if (key !=
flimflam::kSignalStrengthProperty)
On 2013/03/27 22:18:52, stevenjb (chromium) wrote:
> nit: add to if clause above

Done.

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
File chromeos/network/network_state_handler_observer.h (right):

https://codereview.chromium.org/12634019/diff/5002/chromeos/network/network_s...
chromeos/network/network_state_handler_observer.h:38: // changes. If you are
interested in those events, use
On 2013/03/27 22:18:52, stevenjb (chromium) wrote:
> nit: s/If you are/If (avoid "you" in comments)

Done.

[stevenjb, 2013-03-27 23:55:10]

https://codereview.chromium.org/12634019/diff/5002/chromeos/dbus/shill_servic...
File chromeos/dbus/shill_service_client.h (right):

https://codereview.chromium.org/12634019/diff/5002/chromeos/dbus/shill_servic...
chromeos/dbus/shill_service_client.h:51: const std::string& ipconfig_path,
On 2013/03/27 23:16:04, gauravsh wrote:
> On 2013/03/27 22:18:52, stevenjb (chromium) wrote:
> > Do we really want to add this to AddService()? It doesn't seem like an
> essential
> > property. Why not just use SetServiceProperty() when we want to set it?
> 
> This is needed because there are 2 distinct scenarios that I'd like to test.
We
> have 2 separate code paths for IPConfig requests - one when we add the service
> the first time with IPConfig set, and the second when a service's IPConfig
> property changes (or gets added).
> 
> Open to alternative suggestions. How about a separate AddService version that
> takes the IPConfig path?

Yeah, I'd prefer a separate AddServiceWithIpconfig() then, thanks.

[pneubeck (no reviews), 2013-03-28 10:37:54]

https://codereview.chromium.org/12634019/diff/5002/chromeos/dbus/shill_servic...
File chromeos/dbus/shill_service_client.h (right):

https://codereview.chromium.org/12634019/diff/5002/chromeos/dbus/shill_servic...
chromeos/dbus/shill_service_client.h:51: const std::string& ipconfig_path,
On 2013/03/27 23:55:10, stevenjb (chromium) wrote:
> On 2013/03/27 23:16:04, gauravsh wrote:
> > On 2013/03/27 22:18:52, stevenjb (chromium) wrote:
> > > Do we really want to add this to AddService()? It doesn't seem like an
> > essential
> > > property. Why not just use SetServiceProperty() when we want to set it?
> > 
> > This is needed because there are 2 distinct scenarios that I'd like to test.
> We
> > have 2 separate code paths for IPConfig requests - one when we add the
service
> > the first time with IPConfig set, and the second when a service's IPConfig
> > property changes (or gets added).
> > 
> > Open to alternative suggestions. How about a separate AddService version
that
> > takes the IPConfig path?
> 
> Yeah, I'd prefer a separate AddServiceWithIpconfig() then, thanks.
> 

If you want to have initial properties set, then why not add a more scalable
properties argument:
AddServiceWithProperties(...., const DictionaryValue& initial_properties)
(instead of const&, scoped_ptr might be an alternative)

This would also help Manager::ConfigureService and maybe other places.

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_ipcon...
File chromeos/dbus/shill_ipconfig_client_stub.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_ipcon...
chromeos/dbus/shill_ipconfig_client_stub.cc:70: if
(!stub_ipconfig_.GetDictionaryWithoutPathExpansion(ipconfig_path.value(),
swap cases.

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_ipcon...
File chromeos/dbus/shill_ipconfig_client_stub.h (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_ipcon...
chromeos/dbus/shill_ipconfig_client_stub.h:49: // Dictionary of <ipconfig_path,
Property dictionaries>
nit: Property -> property

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_ipcon...
chromeos/dbus/shill_ipconfig_client_stub.h:50: base::DictionaryValue
stub_ipconfig_;
plural?
this is already part of a stub. 'ipconfigs_' should be enough.

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_manag...
File chromeos/dbus/shill_manager_client.h (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_manag...
chromeos/dbus/shill_manager_client.h:55: // Used to reset all properties; does
not notify observers.
keep the empty line (in front of comments)

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_servi...
File chromeos/dbus/shill_service_client.h (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client.h:52: virtual void
AddServiceWithIPConfig(const std::string& service_path,
If you want to have initial properties set, then why not add a more scalable
properties argument:
AddServiceWithProperties(...., const DictionaryValue& initial_properties)
(instead of const&, scoped_ptr might be an alternative)

This would also help Manager::ConfigureService and maybe other places.

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_servi...
File chromeos/dbus/shill_service_client_stub.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client_stub.cc:267: AddServiceInternal(service_path,
name, type, state, ipconfig_path,
Why not adding the implementation here?

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
File chromeos/network/network_change_notifier_chromeos.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos.cc:153: if
(default_network->dns_servers() != dns_servers_) {
Not for this CL, but maybe for future changes:
If NetworkState would report which properties have changed, we wouldn't need the
caching of ip_address and dns_servers in this class.

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
File chromeos/network/network_change_notifier_chromeos_unittest.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos_unittest.cc:16: namespace
chromeos {
Can we please take one step back and think about what we're actually testing
here?

The Notifier does mostly two things:
  1) Translate the connection state (ConnectionTypeFromShill, ~30 lines)
  2) Diff the old network state with the new state (UpdateState, ~40 lines)


This test, as it is, tests for 2) and for
  3) Caching of the last network state (~10 lines)

Why does that need 230 lines of code?
The test could be simplified by e.g. having one test with the parameters
(initial state, new state, expected changes)

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
chromeos/network/network_state.cc:13: bool ConvertListValueToString(const
base::ListValue& string_list,
You can use this one (move it to base/value_conversions.*):

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
chromeos/network/network_state.cc:86:
dictionary->SetStringWithoutPathExpansion(flimflam::kAddressProperty,
Create a nested IPConfig (shill::kIPConfigProperty) dictionary and store the
ipaddress and nameservers in there.

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
chromeos/network/network_state.h:84: std::string dns_servers_;
Why does dns_servers have to be a string?
If anybody will ever use the value itself (the notifier doesn't do that), we
just have to parse it again...

better use std::vector<std::string>

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/shill_pr...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/shill_pr...
chromeos/network/shill_property_handler.cc:441: flimflam::kAddressProperty,
Use the complete nested key, like:
 printf('%s.%s', shill::kIPConfigProperty, flimflam::kAddressProperty)

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/shill_pr...
chromeos/network/shill_property_handler.cc:450:
listener_->UpdateNetworkServiceProperty(service_path,
ditto

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/shill_pr...
File chromeos/network/shill_property_handler_unittest.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/shill_pr...
chromeos/network/shill_property_handler_unittest.cc:76:
std::vector<std::string>& entries(const std::string& type) {
change to the number of the entries:
  ... return entries_[type].size()

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/shill_pr...
chromeos/network/shill_property_handler_unittest.cc:79: std::map<std::string,
int>& property_updates(const std::string& type) {
add another argument for the nested key (path).

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/shill_pr...
chromeos/network/shill_property_handler_unittest.cc:113:
property_updates(type)[path] += 1;
should be
  property_updates(type, path) += 1

[gauravsh, 2013-03-28 18:48:54]

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_ipcon...
File chromeos/dbus/shill_ipconfig_client_stub.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_ipcon...
chromeos/dbus/shill_ipconfig_client_stub.cc:70: if
(!stub_ipconfig_.GetDictionaryWithoutPathExpansion(ipconfig_path.value(),
On 2013/03/28 10:37:54, pneubeck wrote:
> swap cases.

Curious, Why?

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_ipcon...
File chromeos/dbus/shill_ipconfig_client_stub.h (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_ipcon...
chromeos/dbus/shill_ipconfig_client_stub.h:49: // Dictionary of <ipconfig_path,
Property dictionaries>
On 2013/03/28 10:37:54, pneubeck wrote:
> nit: Property -> property

Done.

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_ipcon...
chromeos/dbus/shill_ipconfig_client_stub.h:50: base::DictionaryValue
stub_ipconfig_;
On 2013/03/28 10:37:54, pneubeck wrote:
> plural?
> this is already part of a stub. 'ipconfigs_' should be enough.

Done.

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_manag...
File chromeos/dbus/shill_manager_client.h (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_manag...
chromeos/dbus/shill_manager_client.h:55: // Used to reset all properties; does
not notify observers.
On 2013/03/28 10:37:54, pneubeck wrote:
> keep the empty line (in front of comments)

Done.

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_servi...
File chromeos/dbus/shill_service_client.h (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client.h:52: virtual void
AddServiceWithIPConfig(const std::string& service_path,
On 2013/03/28 10:37:54, pneubeck wrote:
> If you want to have initial properties set, then why not add a more scalable
> properties argument:
> AddServiceWithProperties(...., const DictionaryValue& initial_properties)
> (instead of const&, scoped_ptr might be an alternative)
> 
> This would also help Manager::ConfigureService and maybe other places.

I don't anticipate adding more right now. IPConfig was a special case because of
how it's handled in ShillPropertyHandler.

That said, if we end up needing a more general version in the future, we can add
a more general purpose AddServiceWithProperties then.

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_servi...
File chromeos/dbus/shill_service_client_stub.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client_stub.cc:267: AddServiceInternal(service_path,
name, type, state, ipconfig_path,
On 2013/03/28 10:37:54, pneubeck wrote:
> Why not adding the implementation here?

No particular reason. Done.

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
File chromeos/network/network_change_notifier_chromeos.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos.cc:153: if
(default_network->dns_servers() != dns_servers_) {
On 2013/03/28 10:37:54, pneubeck wrote:
> Not for this CL, but maybe for future changes:
> If NetworkState would report which properties have changed, we wouldn't need
the
> caching of ip_address and dns_servers in this class.

Perhaps. That'd be fine with the current implementation - but I anticipate that
refinements to the NCN will require us to know the actual values for certain
transitions.

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
File chromeos/network/network_change_notifier_chromeos_unittest.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos_unittest.cc:16: namespace
chromeos {
On 2013/03/28 10:37:54, pneubeck wrote:
> Can we please take one step back and think about what we're actually testing
> here?
> 
> The Notifier does mostly two things:
>   1) Translate the connection state (ConnectionTypeFromShill, ~30 lines)
>   2) Diff the old network state with the new state (UpdateState, ~40 lines)
> 
> 
> This test, as it is, tests for 2) and for
>   3) Caching of the last network state (~10 lines)
> 
> Why does that need 230 lines of code?
> The test could be simplified by e.g. having one test with the parameters
> (initial state, new state, expected changes)

The # of lines for the test compared to the lines of code being tested is
irrelevant. These are regression test cases which are meant to verify some
common state transitions we see in practice, and to ensure they are correctly
being handled. Not every one of these cases will be explicitly handled in the
code, which can make the tests easily larger than the code being tested.

If you feel there are specific test cases which are not useful, please point
them out.

I do agree with you that we should de-duplicate the testing code. Which is what
I did by making the test table driven in the next patchset. However, as you'll
see, we are now at 231 lines. :)

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
chromeos/network/network_state.cc:13: bool ConvertListValueToString(const
base::ListValue& string_list,
On 2013/03/28 10:37:54, pneubeck wrote:
> You can use this one (move it to base/value_conversions.*):
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...

I don't think this is general and universal enough to belong in base (and highly
doubt the base OWNERS will accept it either).

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
chromeos/network/network_state.cc:86:
dictionary->SetStringWithoutPathExpansion(flimflam::kAddressProperty,
On 2013/03/28 10:37:54, pneubeck wrote:
> Create a nested IPConfig (shill::kIPConfigProperty) dictionary and store the
> ipaddress and nameservers in there.

Wouldn't that be confusing - this isn't the complete set of IP Config
properties. What is the intended purpose of GetProperties?

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
chromeos/network/network_state.h:84: std::string dns_servers_;
On 2013/03/28 10:37:54, pneubeck wrote:
> Why does dns_servers have to be a string?
> If anybody will ever use the value itself (the notifier doesn't do that), we
> just have to parse it again...
> 
> better use std::vector<std::string>

I guess the reverse question is - why does it need to be a vector?

I made this a comma separated string since that's how the current NetworkLibrary
code stores it - see NetworkIPConfig. I assumed, perhaps incorrectly, that there
was a good reason to do so, say since it's only used to display at the network
config/info screen.

gspencer/stevenjb: Can you comment on the reason why NetworkIPConfig stores DNS
servers as a comma separated string?

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/shill_pr...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/shill_pr...
chromeos/network/shill_property_handler.cc:441: flimflam::kAddressProperty,
On 2013/03/28 10:37:54, pneubeck wrote:
> Use the complete nested key, like:
>  printf('%s.%s', shill::kIPConfigProperty, flimflam::kAddressProperty)

I don't see the gain. Is it because you are worried about name conflicts? Using
the complete nested key just means needing StringPrintfs here and in
network_state.cc.

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/shill_pr...
chromeos/network/shill_property_handler.cc:450:
listener_->UpdateNetworkServiceProperty(service_path,
On 2013/03/28 10:37:54, pneubeck wrote:
> ditto

See my question above.

[stevenjb, 2013-03-28 19:27:13]

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_ipcon...
File chromeos/dbus/shill_ipconfig_client_stub.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_ipcon...
chromeos/dbus/shill_ipconfig_client_stub.cc:70: if
(!stub_ipconfig_.GetDictionaryWithoutPathExpansion(ipconfig_path.value(),
On 2013/03/28 18:48:54, gauravsh wrote:
> On 2013/03/28 10:37:54, pneubeck wrote:
> > swap cases.
> 
> Curious, Why?
Usually the positive test should proceed the negative case, and I agree that
that would be slightly more clear here, but it really should be a nit.

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_servi...
File chromeos/dbus/shill_service_client.h (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client.h:52: virtual void
AddServiceWithIPConfig(const std::string& service_path,
On 2013/03/28 18:48:54, gauravsh wrote:
> On 2013/03/28 10:37:54, pneubeck wrote:
> > If you want to have initial properties set, then why not add a more scalable
> > properties argument:
> > AddServiceWithProperties(...., const DictionaryValue& initial_properties)
> > (instead of const&, scoped_ptr might be an alternative)
> > 
> > This would also help Manager::ConfigureService and maybe other places.
> 
> I don't anticipate adding more right now. IPConfig was a special case because
of
> how it's handled in ShillPropertyHandler.
> 
> That said, if we end up needing a more general version in the future, we can
add
> a more general purpose AddServiceWithProperties then.

I agree that AddServiceWithProperties is probably unnecessary.
SetServiceProperties() would be a more likely addition, since most properties
can reasonably be set after the fact for testing purposes; as mentioned IPConfig
is a special case.

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
File chromeos/network/network_change_notifier_chromeos.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos.cc:153: if
(default_network->dns_servers() != dns_servers_) {
On 2013/03/28 18:48:54, gauravsh wrote:
> On 2013/03/28 10:37:54, pneubeck wrote:
> > Not for this CL, but maybe for future changes:
> > If NetworkState would report which properties have changed, we wouldn't need
> the
> > caching of ip_address and dns_servers in this class.
> 
> Perhaps. That'd be fine with the current implementation - but I anticipate
that
> refinements to the NCN will require us to know the actual values for certain
> transitions.

I would prefer to avoid the bookkeeping overhead of tracking changed values in
NetworkStateHandler; doing that robustly is tricky at best and tends to have a
lot of edge cases. I think it is reasonable and simpler to do that sort of
bookkeeping in the UI code.

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
File chromeos/network/network_change_notifier_chromeos_unittest.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos_unittest.cc:16: namespace
chromeos {
On 2013/03/28 18:48:54, gauravsh wrote:
> On 2013/03/28 10:37:54, pneubeck wrote:
> > Can we please take one step back and think about what we're actually testing
> > here?
> > 
> > The Notifier does mostly two things:
> >   1) Translate the connection state (ConnectionTypeFromShill, ~30 lines)
> >   2) Diff the old network state with the new state (UpdateState, ~40 lines)
> > 
> > 
> > This test, as it is, tests for 2) and for
> >   3) Caching of the last network state (~10 lines)
> > 
> > Why does that need 230 lines of code?
> > The test could be simplified by e.g. having one test with the parameters
> > (initial state, new state, expected changes)
> 
> The # of lines for the test compared to the lines of code being tested is
> irrelevant. These are regression test cases which are meant to verify some
> common state transitions we see in practice, and to ensure they are correctly
> being handled. Not every one of these cases will be explicitly handled in the
> code, which can make the tests easily larger than the code being tested.
> 
> If you feel there are specific test cases which are not useful, please point
> them out.
> 
> I do agree with you that we should de-duplicate the testing code. Which is
what
> I did by making the test table driven in the next patchset. However, as you'll
> see, we are now at 231 lines. :) 

I tend to agree that lines of test code is not a good metric here.
De-duplication of common code is good, but I would much rather have too many
tests than too few.

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
chromeos/network/network_state.cc:13: bool ConvertListValueToString(const
base::ListValue& string_list,
On 2013/03/28 18:48:54, gauravsh wrote:
> On 2013/03/28 10:37:54, pneubeck wrote:
> > You can use this one (move it to base/value_conversions.*):
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...
> 
> I don't think this is general and universal enough to belong in base (and
highly
> doubt the base OWNERS will accept it either).

I tend to agree that this shouldn't be generalized. Getting back a std::vector<>
of strings (which is what the referenced function does) and converting that to a
comma-separated list wouldn't be any simpler than this.

Also, since cros_network_functions.cc is going away, I'm fine with duplicating
this. If we need it elsewhere in code we intend to maintain, we can add it to a
utility module at that time.

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
chromeos/network/network_state.cc:86:
dictionary->SetStringWithoutPathExpansion(flimflam::kAddressProperty,
On 2013/03/28 18:48:54, gauravsh wrote:
> On 2013/03/28 10:37:54, pneubeck wrote:
> > Create a nested IPConfig (shill::kIPConfigProperty) dictionary and store the
> > ipaddress and nameservers in there.
> 
> Wouldn't that be confusing - this isn't the complete set of IP Config
> properties. What is the intended purpose of GetProperties?

For now I think that keeping them separate is fine. If we add additional
IPConfig properties I agree with Philipp; it might be better to more closely
match the Shill model, and to limit the number of top level NetworkState
properties.

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/network_...
chromeos/network/network_state.h:84: std::string dns_servers_;
On 2013/03/28 18:48:54, gauravsh wrote:
> On 2013/03/28 10:37:54, pneubeck wrote:
> > Why does dns_servers have to be a string?
> > If anybody will ever use the value itself (the notifier doesn't do that), we
> > just have to parse it again...
> > 
> > better use std::vector<std::string>
> 
> I guess the reverse question is - why does it need to be a vector?
> 
> I made this a comma separated string since that's how the current
NetworkLibrary
> code stores it - see NetworkIPConfig. I assumed, perhaps incorrectly, that
there
> was a good reason to do so, say since it's only used to display at the network
> config/info screen.
> 
> gspencer/stevenjb: Can you comment on the reason why NetworkIPConfig stores
DNS
> servers as a comma separated string?

I don't know that I would say that there was a *good* reason to use a CSV for
dns_servers (name_servers in NetworkIPConfig), but searching through the code it
looks like we:

* Compare strings, which is a little simpler with a CSV than a vector<string>
* Use at least one const, kGoogleNameServers, which is currently a simple CSV

I don't think we should re-factor that code here, so lets keep it as a string
for now.

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/shill_pr...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/shill_pr...
chromeos/network/shill_property_handler.cc:441: flimflam::kAddressProperty,
On 2013/03/28 18:48:54, gauravsh wrote:
> On 2013/03/28 10:37:54, pneubeck wrote:
> > Use the complete nested key, like:
> >  printf('%s.%s', shill::kIPConfigProperty, flimflam::kAddressProperty)
> 
> I don't see the gain. Is it because you are worried about name conflicts?
Using
> the complete nested key just means needing StringPrintfs here and in
> network_state.cc.

Yes, please do not. NetworkState is a convenience class. We use flimflam/shill
strings so that we do not have to redefine a bunch of similar strings, but it is
not meant to be an exact 1:1 mirror of Shill.Service.

That said, if we do add any more IPConfig properties, we should use a dictionary
ip_config_, and set kAddressProperty and kNameServersProperty in that (and a
separate UpdateNetworkServiceIPConfigProperty() method)

[gauravsh, 2013-03-28 19:54:23]

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_ipcon...
File chromeos/dbus/shill_ipconfig_client_stub.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/dbus/shill_ipcon...
chromeos/dbus/shill_ipconfig_client_stub.cc:70: if
(!stub_ipconfig_.GetDictionaryWithoutPathExpansion(ipconfig_path.value(),
On 2013/03/28 19:27:13, stevenjb (chromium) wrote:
> On 2013/03/28 18:48:54, gauravsh wrote:
> > On 2013/03/28 10:37:54, pneubeck wrote:
> > > swap cases.
> > 
> > Curious, Why?
> Usually the positive test should proceed the negative case, and I agree that
> that would be slightly more clear here, but it really should be a nit.
> 
> 

Done.

[stevenjb, 2013-03-28 20:30:58]

https://codereview.chromium.org/12634019/diff/52002/chromeos/dbus/shill_ipcon...
File chromeos/dbus/shill_ipconfig_client_stub.cc (right):

https://codereview.chromium.org/12634019/diff/52002/chromeos/dbus/shill_ipcon...
chromeos/dbus/shill_ipconfig_client_stub.cc:48: &dict))
nit: align

https://codereview.chromium.org/12634019/diff/52002/chromeos/network/network_...
File chromeos/network/network_change_notifier_chromeos_unittest.cc (right):

https://codereview.chromium.org/12634019/diff/52002/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos_unittest.cc:159:
NotifierUpdateTestCase test_cases[] = {
To make this more readable, please:
* Start each struct on a new line (e.g. line 160, 170)
* Don't follow a struct with non-struct values, e.g. line 163, put 'true, true,
true' on its own line
* Put the trailing } for each top leave entry on its own line, this will help
separate each test case

https://codereview.chromium.org/12634019/diff/52002/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos_unittest.cc:211:
test_cases[i].initial_state.dns_servers);
This could be further simplified by having SetNotifierState take a
NotifierState&

https://codereview.chromium.org/12634019/diff/52002/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos_unittest.cc:218:
test_cases[i].default_network_state.dns_servers);
Same here for DefaultNotifierState

https://codereview.chromium.org/12634019/diff/52002/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos_unittest.cc:224:
test_cases[i].expected_state.dns_servers);
And here with NotifierState&

https://codereview.chromium.org/12634019/diff/52002/chromeos/network/network_...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/12634019/diff/52002/chromeos/network/network_...
chromeos/network/network_state.h:84: std::string dns_servers_;
After further thought, I think Philipp is correct; since none of the current
NetworkState consumers need this to be a CSV.

Let's use ListValue here (instead of vector<string>) so that:
a) If we move these into a DictionaryValue it will just work
b) We can conveniently print the value as JSON

[gauravsh, 2013-03-29 00:02:10]

https://codereview.chromium.org/12634019/diff/52002/chromeos/dbus/shill_ipcon...
File chromeos/dbus/shill_ipconfig_client_stub.cc (right):

https://codereview.chromium.org/12634019/diff/52002/chromeos/dbus/shill_ipcon...
chromeos/dbus/shill_ipconfig_client_stub.cc:48: &dict))
On 2013/03/28 20:30:58, stevenjb (chromium) wrote:
> nit: align

Done.

https://codereview.chromium.org/12634019/diff/52002/chromeos/network/network_...
File chromeos/network/network_change_notifier_chromeos_unittest.cc (right):

https://codereview.chromium.org/12634019/diff/52002/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos_unittest.cc:159:
NotifierUpdateTestCase test_cases[] = {
On 2013/03/28 20:30:58, stevenjb (chromium) wrote:
> To make this more readable, please:
> * Start each struct on a new line (e.g. line 160, 170)
> * Don't follow a struct with non-struct values, e.g. line 163, put 'true,
true,
> true' on its own line
> * Put the trailing } for each top leave entry on its own line, this will help
> separate each test case

Done. (I used the clang chromium style formatter to do this earlier.)

https://codereview.chromium.org/12634019/diff/52002/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos_unittest.cc:211:
test_cases[i].initial_state.dns_servers);
On 2013/03/28 20:30:58, stevenjb (chromium) wrote:
> This could be further simplified by having SetNotifierState take a
> NotifierState&

Good idea. Done.

https://codereview.chromium.org/12634019/diff/52002/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos_unittest.cc:218:
test_cases[i].default_network_state.dns_servers);
On 2013/03/28 20:30:58, stevenjb (chromium) wrote:
> Same here for DefaultNotifierState

Done.

https://codereview.chromium.org/12634019/diff/52002/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos_unittest.cc:224:
test_cases[i].expected_state.dns_servers);
On 2013/03/28 20:30:58, stevenjb (chromium) wrote:
> And here with NotifierState&

Done.

https://codereview.chromium.org/12634019/diff/52002/chromeos/network/network_...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/12634019/diff/52002/chromeos/network/network_...
chromeos/network/network_state.h:84: std::string dns_servers_;
On 2013/03/28 20:30:58, stevenjb (chromium) wrote:
> After further thought, I think Philipp is correct; since none of the current
> NetworkState consumers need this to be a CSV.
> 
> Let's use ListValue here (instead of vector<string>) so that:
> a) If we move these into a DictionaryValue it will just work
> b) We can conveniently print the value as JSON

As per offline discussion with Steven, I have changed this to a vector<string>
for now. If we end up adding more IPConfig properties, we can switch to using a
DictionaryValue for storing IPConfig properties instead.

[stevenjb, 2013-03-29 00:21:36]

Looking good thanks, just a few more comments.

https://codereview.chromium.org/12634019/diff/52002/chromeos/network/network_...
File chromeos/network/network_change_notifier_chromeos_unittest.cc (right):

https://codereview.chromium.org/12634019/diff/52002/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos_unittest.cc:159:
NotifierUpdateTestCase test_cases[] = {
On 2013/03/29 00:02:11, gauravsh wrote:
> On 2013/03/28 20:30:58, stevenjb (chromium) wrote:
> > To make this more readable, please:
> > * Start each struct on a new line (e.g. line 160, 170)
> > * Don't follow a struct with non-struct values, e.g. line 163, put 'true,
> true,
> > true' on its own line
> > * Put the trailing } for each top leave entry on its own line, this will
help
> > separate each test case
> 
> Done. (I used the clang chromium style formatter to do this earlier.)
Interesting. Well, this looks much nicer now, so thanks.

https://codereview.chromium.org/12634019/diff/71001/chromeos/network/network_...
File chromeos/network/network_change_notifier_chromeos.cc (right):

https://codereview.chromium.org/12634019/diff/71001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos.cc:156: VLOG(1) <<
default_network->dns_servers()[i] << ",";
Hey look:
base::JoinString(default_network->dns_servers(), ",")

https://codereview.chromium.org/12634019/diff/71001/chromeos/network/network_...
File chromeos/network/network_change_notifier_chromeos_unittest.cc (right):

https://codereview.chromium.org/12634019/diff/71001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos_unittest.cc:10: #include
"base/strings/string_split.h"
nice.

https://codereview.chromium.org/12634019/diff/71001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos_unittest.cc:91: };
These should go in the anonymous namespace

https://codereview.chromium.org/12634019/diff/71001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos_unittest.cc:155: };
This too

https://codereview.chromium.org/12634019/diff/71001/chromeos/network/network_...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/12634019/diff/71001/chromeos/network/network_...
chromeos/network/network_state.cc:88: value->Append(new
base::StringValue(dns_servers()[i]));
value->AppendStrings(dns_servers());

[gauravsh, 2013-03-29 00:41:50]

https://codereview.chromium.org/12634019/diff/71001/chromeos/network/network_...
File chromeos/network/network_change_notifier_chromeos.cc (right):

https://codereview.chromium.org/12634019/diff/71001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos.cc:156: VLOG(1) <<
default_network->dns_servers()[i] << ",";
On 2013/03/29 00:21:36, stevenjb (chromium) wrote:
> Hey look:
> base::JoinString(default_network->dns_servers(), ",")

Done.

https://codereview.chromium.org/12634019/diff/71001/chromeos/network/network_...
File chromeos/network/network_change_notifier_chromeos_unittest.cc (right):

https://codereview.chromium.org/12634019/diff/71001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos_unittest.cc:91: };
On 2013/03/29 00:21:36, stevenjb (chromium) wrote:
> These should go in the anonymous namespace

Done.

https://codereview.chromium.org/12634019/diff/71001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos_unittest.cc:155: };
On 2013/03/29 00:21:36, stevenjb (chromium) wrote:
> This too

Done.

https://codereview.chromium.org/12634019/diff/71001/chromeos/network/network_...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/12634019/diff/71001/chromeos/network/network_...
chromeos/network/network_state.cc:88: value->Append(new
base::StringValue(dns_servers()[i]));
On 2013/03/29 00:21:36, stevenjb (chromium) wrote:
> value->AppendStrings(dns_servers());

Thanks. Done.

[stevenjb, 2013-03-29 00:52:10]

LGTM

https://codereview.chromium.org/12634019/diff/88001/chromeos/network/network_...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/12634019/diff/88001/chromeos/network/network_...
chromeos/network/network_state.cc:89: value);
nit: one line?

[pneubeck (no reviews), 2013-03-29 19:16:52]

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/shill_pr...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/shill_pr...
chromeos/network/shill_property_handler.cc:441: flimflam::kAddressProperty,
This comment was based on the proposed changes to NetworkState::GetProperties
and that NetworkState::PropertyChanged should be the inverse function of
GetProperties.


Still, I don't see the need for this change of keys. Look at the above
NetworkServicePropertyChangedCallback. It simply forwards the key/value pairs.
Later the pairs are filtered in NetworkState. In NetworkState::PropertyChange,
we know that we receive Shill properties.

With this code you break that property. This implementation deviates from the
Shill format and to understand NetworkState::PropertyChange, I have to read
again more code.

On 2013/03/28 19:27:13, stevenjb (chromium) wrote:
> On 2013/03/28 18:48:54, gauravsh wrote:
> > On 2013/03/28 10:37:54, pneubeck wrote:
> > > Use the complete nested key, like:
> > >  printf('%s.%s', shill::kIPConfigProperty, flimflam::kAddressProperty)
> > 
> > I don't see the gain. Is it because you are worried about name conflicts?
> Using
> > the complete nested key just means needing StringPrintfs here and in
> > network_state.cc.
> 
> Yes, please do not. NetworkState is a convenience class. We use flimflam/shill
> strings so that we do not have to redefine a bunch of similar strings, but it
is
> not meant to be an exact 1:1 mirror of Shill.Service.
> 
> That said, if we do add any more IPConfig properties, we should use a
dictionary
> ip_config_, and set kAddressProperty and kNameServersProperty in that (and a
> separate UpdateNetworkServiceIPConfigProperty() method)
> 
>

https://codereview.chromium.org/12634019/diff/88001/chromeos/network/network_...
File chromeos/network/network_change_notifier_chromeos_unittest.cc (right):

https://codereview.chromium.org/12634019/diff/88001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos_unittest.cc:156:
NotifierUpdateTestCase test_cases[] = {
Thanks. That is so much more readable.

https://codereview.chromium.org/12634019/diff/88001/chromeos/network/network_...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/12634019/diff/88001/chromeos/network/network_...
chromeos/network/network_state.cc:84:
dictionary->SetStringWithoutPathExpansion(flimflam::kAddressProperty,
My veto here:

The only usage for this function is to obtain a correct subset of a Shill
dictionary which can be translated to "extended" ONC. The result is then
returned by GetState/GetVisibleNetworks of the extension API.
I will not accept to add yet another conditional translation for these new keys.

I see two alternatives which I'm fine with:

- Either remove these two dictionary->Set..  and adapt the comment (that this
function returns only a subset of the properties accepted by Property changed).
This is ok, because we don't have any user of these properties through the
extension API, yet.

- Or make it a correct Shill (nested) dictionary. That was my 1st proposal: A
shill service has a property IPConfig and that in turn has Address and
NameServers.

[stevenjb, 2013-03-29 19:41:25]

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/shill_pr...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/12634019/diff/36021/chromeos/network/shill_pr...
chromeos/network/shill_property_handler.cc:441: flimflam::kAddressProperty,
On 2013/03/29 19:16:52, pneubeck wrote:
> This comment was based on the proposed changes to NetworkState::GetProperties
> and that NetworkState::PropertyChanged should be the inverse function of
> GetProperties.
> 
> 
> Still, I don't see the need for this change of keys. Look at the above
> NetworkServicePropertyChangedCallback. It simply forwards the key/value pairs.
> Later the pairs are filtered in NetworkState. In NetworkState::PropertyChange,
> we know that we receive Shill properties.
> 
> With this code you break that property. This implementation deviates from the
> Shill format and to understand NetworkState::PropertyChange, I have to read
> again more code.
> 
> On 2013/03/28 19:27:13, stevenjb (chromium) wrote:
> > On 2013/03/28 18:48:54, gauravsh wrote:
> > > On 2013/03/28 10:37:54, pneubeck wrote:
> > > > Use the complete nested key, like:
> > > >  printf('%s.%s', shill::kIPConfigProperty, flimflam::kAddressProperty)
> > > 
> > > I don't see the gain. Is it because you are worried about name conflicts?
> > Using
> > > the complete nested key just means needing StringPrintfs here and in
> > > network_state.cc.
> > 
> > Yes, please do not. NetworkState is a convenience class. We use
flimflam/shill
> > strings so that we do not have to redefine a bunch of similar strings, but
it
> is
> > not meant to be an exact 1:1 mirror of Shill.Service.
> > 
> > That said, if we do add any more IPConfig properties, we should use a
> dictionary
> > ip_config_, and set kAddressProperty and kNameServersProperty in that (and a
> > separate UpdateNetworkServiceIPConfigProperty() method)
> > 
> > 
> 
OK, I thought about it some more, and now agree that the keys we use in
NetworkState::PropertyChanged() and NetworkState::GetProperties() should match
Shill, even though we do not need to store members in NetworkState that way. I
was needlessly conflating the two situations.

So, here lets go ahead and use:
  const std::string kIPConfigAddressProperty =
    std::string(shill::kIPConfigProperty) + '.' + flimflam::kAddressProperty;

for the key here and in NetworkState. You can define it as a const in
NetworkState to ensure that they match.

https://codereview.chromium.org/12634019/diff/88001/chromeos/network/network_...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/12634019/diff/88001/chromeos/network/network_...
chromeos/network/network_state.cc:84:
dictionary->SetStringWithoutPathExpansion(flimflam::kAddressProperty,
On 2013/03/29 19:16:53, pneubeck wrote:
> My veto here:
> 
> The only usage for this function is to obtain a correct subset of a Shill
> dictionary which can be translated to "extended" ONC. The result is then
> returned by GetState/GetVisibleNetworks of the extension API.
> I will not accept to add yet another conditional translation for these new
keys.
> 
> I see two alternatives which I'm fine with:
> 
> - Either remove these two dictionary->Set..  and adapt the comment (that this
> function returns only a subset of the properties accepted by Property
changed).
> This is ok, because we don't have any user of these properties through the
> extension API, yet.
> 
> - Or make it a correct Shill (nested) dictionary. That was my 1st proposal: A
> shill service has a property IPConfig and that in turn has Address and
> NameServers.

OK, now I understand the concern, and agree that this makes sense, lets go with
Philipp's second suggestion.

[gauravsh, 2013-04-01 20:02:06]

Thanks for the feedback Phillip. The new patch goes the nested dictionary route
as you recommended.

There's just one remaining thing - I can't make the '.' separated std::string a
class constant (style guide forbids static initializers for non-POD types) - so
end up duplicating the full property string in shill_property_handler and
network_state.

https://codereview.chromium.org/12634019/diff/88001/chromeos/network/network_...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/12634019/diff/88001/chromeos/network/network_...
chromeos/network/network_state.cc:84:
dictionary->SetStringWithoutPathExpansion(flimflam::kAddressProperty,
On 2013/03/29 19:41:25, stevenjb (chromium) wrote:
> On 2013/03/29 19:16:53, pneubeck wrote:
> > My veto here:
> > 
> > The only usage for this function is to obtain a correct subset of a Shill
> > dictionary which can be translated to "extended" ONC. The result is then
> > returned by GetState/GetVisibleNetworks of the extension API.
> > I will not accept to add yet another conditional translation for these new
> keys.
> > 
> > I see two alternatives which I'm fine with:
> > 
> > - Either remove these two dictionary->Set..  and adapt the comment (that
this
> > function returns only a subset of the properties accepted by Property
> changed).
> > This is ok, because we don't have any user of these properties through the
> > extension API, yet.
> > 
> > - Or make it a correct Shill (nested) dictionary. That was my 1st proposal:
A
> > shill service has a property IPConfig and that in turn has Address and
> > NameServers.
> 
> OK, now I understand the concern, and agree that this makes sense, lets go
with
> Philipp's second suggestion.

Done.

https://codereview.chromium.org/12634019/diff/88001/chromeos/network/network_...
chromeos/network/network_state.cc:89: value);
On 2013/03/29 00:52:10, stevenjb (chromium) wrote:
> nit: one line?

Done.

[stevenjb, 2013-04-01 20:28:37]

On 2013/04/01 20:02:06, gauravsh wrote:
> Thanks for the feedback Phillip. The new patch goes the nested dictionary
route
> as you recommended.
> 
> There's just one remaining thing - I can't make the '.' separated std::string
a
> class constant (style guide forbids static initializers for non-POD types) -
so
> end up duplicating the full property string in shill_property_handler and
> network_state.
> 
You're right of course, I would suggest a static method instead:

// static
std::string NetworkState::IPConfigProperty(const char* property) {
  return std::string(shill::kIPConfigProperty) + '.' + property;
}

Otherwise, LGTM!

[pneubeck (no reviews), 2013-04-02 08:20:13]

LGTM, thanks!

[commit-bot: I haz the power, 2013-04-02 19:52:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gauravsh@chromium.org/12634019/118001

[commit-bot: I haz the power, 2013-04-02 23:24:30]

Change committed as 191929