Provide IPConfigs in networkingPrivate.GetProperties

Provide IPConfigs in networkingPrivate.GetProperties


BUG=365848
For components/wifi fixes for tests:

R=pneubeck@chromium.org
TBR=mef@chromium.org

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

[pneubeck (no reviews), 2014-05-02 09:57:42]

no full review yet.

https://codereview.chromium.org/267433005/diff/80001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/267433005/diff/80001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:323: if (shill_ip_method ==
shill::kTypeIPv4)
Ok, I think we have a mismatch of the properties' semantics here:

onc::ipconfig::Type means "What kind of ip address is this: IPv4 or IPv6".

shill::kMethodProperty means "The method by which Layer 3 state is managed."

If you really need the latter information in the UI, then we have to find
another way to communicate it than the onc::ipconfig::Type field.

[stevenjb, 2014-05-02 17:22:34]

https://codereview.chromium.org/267433005/diff/80001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/267433005/diff/80001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:323: if (shill_ip_method ==
shill::kTypeIPv4)
On 2014/05/02 09:57:43, pneubeck wrote:
> Ok, I think we have a mismatch of the properties' semantics here:
> 
> onc::ipconfig::Type means "What kind of ip address is this: IPv4 or IPv6".
> 
> shill::kMethodProperty means "The method by which Layer 3 state is managed."
> 
> If you really need the latter information in the UI, then we have to find
> another way to communicate it than the onc::ipconfig::Type field.

Hm, I see, OK. We have to support multiple "IPv6" entries anyway, so translating
dhcp -> kIPv4 seems like it would be fine.

[stevenjb, 2014-05-05 17:08:25]

PTAL

[pneubeck (no reviews), 2014-05-05 21:09:27]

please also add/update the networking API test. Since it's based on the
DefaultEnvironment, IPConfigs should appear in GetProperties, shouldn't they?

overall looks already good to me.
I'm just a little bit concerned that StateHandler is growing again (the next
change being your proposed GUID generation)

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/device...
File chromeos/network/device_state.cc (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/device...
chromeos/network/device_state.cc:136: // IP Configs will be added when they are
received.
the comment doesn't explain why you have to clear it here.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/device...
chromeos/network/device_state.cc:144: base::DictionaryValue* ip_config;
nit: initialize with NULL

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/device...
File chromeos/network/device_state.h (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/device...
chromeos/network/device_state.h:96: // Dictionary of IPConfig properties, keyed
by ip config path.
maybe 'ip config path' -> 'IPConfig path'

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/networ...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:377:
network_state_handler_->GetDeviceState(device);
Just some thought, not saying that it's really a problem:

If you get the ipconfigs from StateHandler's cache, might this lead to timing
issues?
Namely, if the properties received here are more up-to-date than the device
state, e.g. before and after connecting. This might lead to
ConnectionState=connected but no available ipconfigs.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:386:
shill::kIPConfigsProperty, ip_configs);
do we want to document that Shill doesn't use this property for Services but
only for Devices?

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:390:
callback.Run(service_path, *properties_copy.get());
.get() shouldn't be necessary

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/networ...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/networ...
chromeos/network/network_state.cc:194: const base::ListValue* dns_servers;
nit: initialize with NULL

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/networ...
chromeos/network/network_state.cc:208: if (!gurl.is_valid()) {
wrong negation?

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/onc/on...
File chromeos/network/onc/onc_translation_tables.cc (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/onc/on...
chromeos/network/onc/onc_translation_tables.cc:182: // TranslateIPConfig It is
only converted from Shill->ONC.
nit:  'IPConfig It' -> 'IPConfig. It' (missing dot)

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/onc/on...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/onc/on...
chromeos/network/onc/onc_translator_shill_to_onc.cc:330: return;  // Ignore
unhandled IPConfig types, e.g. bootp, zeroconf, ppp
optional nit: LOG(ERROR) maybe

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:493: if (type ==
ManagedState::MANAGED_TYPE_NETWORK) {
you could drop the type conditions (type == ManagedState....) and directly look
for kIPConfig*Property.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:495: const base::Value* value;
initialize with NULL

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:496: if
(properties.Get(shill::kIPConfigProperty, &value))
here and below, Get -> GetWithoutPathExpansion

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:500: const base::Value* value;
initialize with NULL

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:522: if (type ==
ManagedState::MANAGED_TYPE_NETWORK &&
again, checking for |type| not really necessary

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:556: const base::ListValue*
ip_configs;
initialize with NULL

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:559: for (size_t i = 0; i <
ip_configs->GetSize(); i++) {
better use base::ListValue::const_iterator

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:561: if (!ip_configs->GetString(i,
&ip_config_path) || ip_config_path.empty()) {
should call RequestIPConfig

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
File chromeos/network/shill_property_handler.h (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.h:75: virtual void
UpdateIPConfigProperites(
typo: Properites -> Properties (also in all implementations)

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.h:199: void
RequestIPConfig(ManagedState::ManagedType type,
please document the arguments

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.h:203: // Request a list IPConfig
properties from Shill.IPConfigClient.
'list of'

again, please document arguemnts

[stevenjb, 2014-05-06 01:15:11]

PTAL

Tests updated. I realized that:
a) The GetProperties test was testing "wifi2" and the stub wasn't providing
IPConfigs for wifi.
b) We should only be providing IPConfigs for connected networks, so I added a
second GetPropertiesForConnected test.
c) We should really be doing the IPConfigs insertion in ManagedNCH

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/device...
File chromeos/network/device_state.cc (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/device...
chromeos/network/device_state.cc:136: // IP Configs will be added when they are
received.
On 2014/05/05 21:09:28, pneubeck wrote:
> the comment doesn't explain why you have to clear it here.
Moved this logic and added a better comment.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/device...
chromeos/network/device_state.cc:144: base::DictionaryValue* ip_config;
On 2014/05/05 21:09:28, pneubeck wrote:
> nit: initialize with NULL

Done.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/device...
File chromeos/network/device_state.h (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/device...
chromeos/network/device_state.h:96: // Dictionary of IPConfig properties, keyed
by ip config path.
On 2014/05/05 21:09:28, pneubeck wrote:
> maybe 'ip config path' -> 'IPConfig path'

Done.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/networ...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:377:
network_state_handler_->GetDeviceState(device);
On 2014/05/05 21:09:28, pneubeck wrote:
> Just some thought, not saying that it's really a problem:
> 
> If you get the ipconfigs from StateHandler's cache, might this lead to timing
> issues?
> Namely, if the properties received here are more up-to-date than the device
> state, e.g. before and after connecting. This might lead to
> ConnectionState=connected but no available ipconfigs.

So, I thought about this some, and realized a few things:
1) We shouldn't do this here, we should do it in ManagedNCH.
2) We should only do this when State == Connected
3) Because Shill handles all requests in-order, the only edge case would occur
if the service changed state to Connected in the same frame that GetProperties
was requested. Otherwise Chrome would have already received the IPConfigs
changed signal and requested the IPConfigs, which would have been processed by
the time we got to this code. However, there will always be an edge case because
State may have changed to !Connected by the time we get here, in which case a
fresh request would produce the wrong result. Thus, I think we should stick with
the edge case that is less work :)

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:386:
shill::kIPConfigsProperty, ip_configs);
On 2014/05/05 21:09:28, pneubeck wrote:
> do we want to document that Shill doesn't use this property for Services but
> only for Devices?

Done. (elaborated comment above)

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:390:
callback.Run(service_path, *properties_copy.get());
On 2014/05/05 21:09:28, pneubeck wrote:
> .get() shouldn't be necessary

Done.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/networ...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/networ...
chromeos/network/network_state.cc:194: const base::ListValue* dns_servers;
On 2014/05/05 21:09:28, pneubeck wrote:
> nit: initialize with NULL
Not necessary here, Value is guaranteed to set dns_servers if it returns true.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/networ...
chromeos/network/network_state.cc:208: if (!gurl.is_valid()) {
On 2014/05/05 21:09:28, pneubeck wrote:
> wrong negation?
Huh. Wonder why this hasn't caused us problems? (I just moved it). Fixed.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/onc/on...
File chromeos/network/onc/onc_translation_tables.cc (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/onc/on...
chromeos/network/onc/onc_translation_tables.cc:182: // TranslateIPConfig It is
only converted from Shill->ONC.
On 2014/05/05 21:09:28, pneubeck wrote:
> nit:  'IPConfig It' -> 'IPConfig. It' (missing dot)

Done.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/onc/on...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/onc/on...
chromeos/network/onc/onc_translator_shill_to_onc.cc:330: return;  // Ignore
unhandled IPConfig types, e.g. bootp, zeroconf, ppp
On 2014/05/05 21:09:28, pneubeck wrote:
> optional nit: LOG(ERROR) maybe

Not an ERROR. Shill may provide other IPConfigs that we intentionally ignore.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:493: if (type ==
ManagedState::MANAGED_TYPE_NETWORK) {
On 2014/05/05 21:09:28, pneubeck wrote:
> you could drop the type conditions (type == ManagedState....) and directly
look
> for kIPConfig*Property.
I would rather have the explicit logic.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:495: const base::Value* value;
On 2014/05/05 21:09:28, pneubeck wrote:
> initialize with NULL
Not necessary.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:496: if
(properties.Get(shill::kIPConfigProperty, &value))
On 2014/05/05 21:09:28, pneubeck wrote:
> here and below, Get -> GetWithoutPathExpansion

Done.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:500: const base::Value* value;
On 2014/05/05 21:09:28, pneubeck wrote:
> initialize with NULL
Not necessary.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:522: if (type ==
ManagedState::MANAGED_TYPE_NETWORK &&
On 2014/05/05 21:09:28, pneubeck wrote:
> again, checking for |type| not really necessary
I'd rather be explicit.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:556: const base::ListValue*
ip_configs;
On 2014/05/05 21:09:28, pneubeck wrote:
> initialize with NULL
Unneeded

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:559: for (size_t i = 0; i <
ip_configs->GetSize(); i++) {
On 2014/05/05 21:09:28, pneubeck wrote:
> better use base::ListValue::const_iterator

Done.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:561: if (!ip_configs->GetString(i,
&ip_config_path) || ip_config_path.empty()) {
On 2014/05/05 21:09:28, pneubeck wrote:
> should call RequestIPConfig
I don't follow.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
File chromeos/network/shill_property_handler.h (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.h:75: virtual void
UpdateIPConfigProperites(
On 2014/05/05 21:09:28, pneubeck wrote:
> typo: Properites -> Properties (also in all implementations)

Done.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.h:199: void
RequestIPConfig(ManagedState::ManagedType type,
On 2014/05/05 21:09:28, pneubeck wrote:
> please document the arguments

Done.

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.h:203: // Request a list IPConfig
properties from Shill.IPConfigClient.
On 2014/05/05 21:09:28, pneubeck wrote:
> 'list of'
> 
> again, please document arguemnts

Done.

[pneubeck (no reviews), 2014-05-06 07:31:17]

lgtm

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/networ...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:377:
network_state_handler_->GetDeviceState(device);
On 2014/05/06 01:15:11, stevenjb wrote:
> On 2014/05/05 21:09:28, pneubeck wrote:
> > Just some thought, not saying that it's really a problem:
> > 
> > If you get the ipconfigs from StateHandler's cache, might this lead to
timing
> > issues?
> > Namely, if the properties received here are more up-to-date than the device
> > state, e.g. before and after connecting. This might lead to
> > ConnectionState=connected but no available ipconfigs.
> 
> So, I thought about this some, and realized a few things:
> 1) We shouldn't do this here, we should do it in ManagedNCH.
> 2) We should only do this when State == Connected
> 3) Because Shill handles all requests in-order, the only edge case would occur
> if the service changed state to Connected in the same frame that GetProperties
> was requested. Otherwise Chrome would have already received the IPConfigs
> changed signal and requested the IPConfigs, which would have been processed by
> the time we got to this code. However, there will always be an edge case
because
> State may have changed to !Connected by the time we get here, in which case a
> fresh request would produce the wrong result. Thus, I think we should stick
with
> the edge case that is less work :)

sg

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:561: if (!ip_configs->GetString(i,
&ip_config_path) || ip_config_path.empty()) {
On 2014/05/06 01:15:11, stevenjb wrote:
> On 2014/05/05 21:09:28, pneubeck wrote:
> > should call RequestIPConfig
> I don't follow.

for (base::ListValue::const_iterator iter...)
  RequestIPConfig(type, path, **iter);

to keep the NET_LOG_ERROR, you could move it to RequestIPConfig

[stevenjb, 2014-05-06 16:56:19]

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/267433005/diff/110001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:561: if (!ip_configs->GetString(i,
&ip_config_path) || ip_config_path.empty()) {
On 2014/05/06 07:31:17, pneubeck wrote:
> On 2014/05/06 01:15:11, stevenjb wrote:
> > On 2014/05/05 21:09:28, pneubeck wrote:
> > > should call RequestIPConfig
> > I don't follow.
> 
> for (base::ListValue::const_iterator iter...)
>   RequestIPConfig(type, path, **iter);
> 
> to keep the NET_LOG_ERROR, you could move it to RequestIPConfig
Oh, I see. I guess that's a little more clear. Done.

[stevenjb, 2014-05-06 16:56:26]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-05-06 16:57:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stevenjb@chromium.org/267433005/150001

[stevenjb, 2014-05-08 02:25:32]

+mef@chromium.org for components/wifi

[stevenjb, 2014-05-09 02:34:41]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-05-09 02:40:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stevenjb@chromium.org/267433005/360001

[commit-bot: I haz the power, 2014-05-09 05:00:46]

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:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-05-09 05:14:45]

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

[commit-bot: I haz the power, 2014-05-09 05:14:45]

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

[stevenjb, 2014-05-09 16:39:53]

mef@ - Apologies for TBRing the components/wifi change,s I would still like them
reviewed and we can discuss the best maintainable approach for the future
(crbug.com/371442)

[stevenjb, 2014-05-09 16:45:55]

Committed patchset #19 manually as r269334 (presubmit successful).