NetworkState cleanup, pass properties to InitialPropertiesReceived

NetworkState cleanup, pass properties to InitialPropertiesReceived

Some NetworkState cleanup.

This passes the complete dictionary of properties to NetworkState::
InitialPropertesReceived() so that they can be processed without having
to parse and store each individual property.

Also renames technology_ to network_technology_ to match the Shill terminology (and reduce confusion with type_ which is really technology type).

BUG=263978
R=pneubeck@chromium.org

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

[pneubeck (no reviews), 2013-07-29 19:13:24]

I don't quite understand what this user_profile_required property will be used
for.
Will it be obsolete if NetworkConnectionHandler would more rely on GetProperties
instead of NetworkState?

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/favorite_...
File chromeos/network/favorite_state.h (right):

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/favorite_...
chromeos/network/favorite_state.h:38: // Returns true if the network properties
are stored in the user profile.
nit: the user profile -> a user profile

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/managed_s...
File chromeos/network/managed_state.h (right):

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/managed_s...
chromeos/network/managed_state.h:60: // parsed properties, e.g. name from
hex_ssid in NetworkState.
nit: remove 'parsed'

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

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
chromeos/network/network_state.cc:55: // Otherwise returns true and sets
|ui_data| if |ui_data_value| is a valid
You can simplify this by setting ui_data to new NetworkUIData(), if
|ui_data_value| is empty.
And setting ui_data to NULL (or not setting it) can replace the result false.

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
chromeos/network/network_state.cc:216: void
NetworkState::InitialPropertiesReceived(
I'm missing a return value that indicates if a property was changed.
Only if something changed, a notification should be triggered.

Currently, if properties are changed _only_ in this function (and not in
PropertyChanged), then no notification is emitted.

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
chromeos/network/network_state.cc:455: if (ui_data->certificate_type() ==
CLIENT_CERT_TYPE_PATTERN)
ClientCertPatterns can currently only be set by user policy.
If a user policy is already set, then IsPrivate is already true. Why would you
have to check certificate_type here additionally?

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
chromeos/network/network_state.h:91: // Returns true if the network properties
are stored in the user profile.
ditto: the -> a

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
File chromeos/network/network_state_unittest.cc (right):

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
chromeos/network/network_state_unittest.cc:18: class TestStringValue : public
base::Value {
Because I stumbled over this before and needs a lot of investigation to
understand, please add a comment why this is TestStringValue is required. My
guess: for utf8 error testing. But that would actually mean, that the Shill/Dbus
interface would create invalid base::Values, wouldn't it?

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
chromeos/network/network_state_unittest.cc:62: void
SignalInitialPropertiesReceived() {
If InitialPropertiesReceived returns a bool, then it should be check in the test
too.

[stevenjb, 2013-07-29 20:25:47]

'user_profile_required' will be used to determine whether or not a network can
be shared. This implements the existing logic; it is likely that the logic is
not entirely complete.

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/favorite_...
File chromeos/network/favorite_state.h (right):

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/favorite_...
chromeos/network/favorite_state.h:38: // Returns true if the network properties
are stored in the user profile.
On 2013/07/29 19:13:25, pneubeck wrote:
> nit: the user profile -> a user profile

Done.

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/managed_s...
File chromeos/network/managed_state.h (right):

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/managed_s...
chromeos/network/managed_state.h:60: // parsed properties, e.g. name from
hex_ssid in NetworkState.
On 2013/07/29 19:13:25, pneubeck wrote:
> nit: remove 'parsed'

Done.

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

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
chromeos/network/network_state.cc:55: // Otherwise returns true and sets
|ui_data| if |ui_data_value| is a valid
On 2013/07/29 19:13:25, pneubeck wrote:
> You can simplify this by setting ui_data to new NetworkUIData(), if
> |ui_data_value| is empty.
> And setting ui_data to NULL (or not setting it) can replace the result false.
Done

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
chromeos/network/network_state.cc:216: void
NetworkState::InitialPropertiesReceived(
On 2013/07/29 19:13:25, pneubeck wrote:
> I'm missing a return value that indicates if a property was changed.
> Only if something changed, a notification should be triggered.
> 
> Currently, if properties are changed _only_ in this function (and not in
> PropertyChanged), then no notification is emitted.

Hmm, I guess that's theoretically possible. Fixed.

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
chromeos/network/network_state.cc:455: if (ui_data->certificate_type() ==
CLIENT_CERT_TYPE_PATTERN)
On 2013/07/29 19:13:25, pneubeck wrote:
> ClientCertPatterns can currently only be set by user policy.
> If a user policy is already set, then IsPrivate is already true. Why would you
> have to check certificate_type here additionally?

A PSK network that is Private can be shared.
A network requiring a certificate may be Private but can not be shared.
This property is used to make that distinction.

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
chromeos/network/network_state.h:91: // Returns true if the network properties
are stored in the user profile.
On 2013/07/29 19:13:25, pneubeck wrote:
> ditto: the -> a

Done.

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
File chromeos/network/network_state_unittest.cc (right):

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
chromeos/network/network_state_unittest.cc:18: class TestStringValue : public
base::Value {
On 2013/07/29 19:13:25, pneubeck wrote:
> Because I stumbled over this before and needs a lot of investigation to
> understand, please add a comment why this is TestStringValue is required. My
> guess: for utf8 error testing. But that would actually mean, that the
Shill/Dbus
> interface would create invalid base::Values, wouldn't it?

This code predates https://chromiumcodereview.appspot.com/11418048 and can now
be removed.
Done.

https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
chromeos/network/network_state_unittest.cc:62: void
SignalInitialPropertiesReceived() {
On 2013/07/29 19:13:25, pneubeck wrote:
> If InitialPropertiesReceived returns a bool, then it should be check in the
test
> too.

Done.

[pneubeck (no reviews), 2013-07-29 21:52:20]

On 2013/07/29 20:25:47, stevenjb (chromium) wrote:
> 'user_profile_required' will be used to determine whether or not a network can
> be shared. This implements the existing logic; it is likely that the logic is
> not entirely complete.

I see.
Wifi::RequiresUserProfile is only called in wifi_config_view.cc under certain
conditions
 
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
namely, if a not visible network is configured or if the network is unconfigured
(wifi->profile_type() == PROFILE_NONE).
Configured networks cannot be shared currently.

If the network is visible but not configured yet, you might use NetworkState,
but then UIData will never be set.
I think the original code was already not that correct. 'user_profile_required'
can only depend on properties of unconfigured networks.

In case of a new network (not visible), the NetworkState cannot be used, if the
'share this network' checkbox should depend on the settings that the user
selects in the dialog.

> 
>
https://codereview.chromium.org/21030006/diff/3001/chromeos/network/favorite_...
> File chromeos/network/favorite_state.h (right):
> 
>
https://codereview.chromium.org/21030006/diff/3001/chromeos/network/favorite_...
> chromeos/network/favorite_state.h:38: // Returns true if the network
properties
> are stored in the user profile.
> On 2013/07/29 19:13:25, pneubeck wrote:
> > nit: the user profile -> a user profile
> 
> Done.
> 
>
https://codereview.chromium.org/21030006/diff/3001/chromeos/network/managed_s...
> File chromeos/network/managed_state.h (right):
> 
>
https://codereview.chromium.org/21030006/diff/3001/chromeos/network/managed_s...
> chromeos/network/managed_state.h:60: // parsed properties, e.g. name from
> hex_ssid in NetworkState.
> On 2013/07/29 19:13:25, pneubeck wrote:
> > nit: remove 'parsed'
> 
> Done.
> 
>
https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
> File chromeos/network/network_state.cc (right):
> 
>
https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
> chromeos/network/network_state.cc:55: // Otherwise returns true and sets
> |ui_data| if |ui_data_value| is a valid
> On 2013/07/29 19:13:25, pneubeck wrote:
> > You can simplify this by setting ui_data to new NetworkUIData(), if
> > |ui_data_value| is empty.
> > And setting ui_data to NULL (or not setting it) can replace the result
false.
> Done
> 
>
https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
> chromeos/network/network_state.cc:216: void
> NetworkState::InitialPropertiesReceived(
> On 2013/07/29 19:13:25, pneubeck wrote:
> > I'm missing a return value that indicates if a property was changed.
> > Only if something changed, a notification should be triggered.
> > 
> > Currently, if properties are changed _only_ in this function (and not in
> > PropertyChanged), then no notification is emitted.
> 
> Hmm, I guess that's theoretically possible. Fixed.
> 
>
https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
> chromeos/network/network_state.cc:455: if (ui_data->certificate_type() ==
> CLIENT_CERT_TYPE_PATTERN)
> On 2013/07/29 19:13:25, pneubeck wrote:
> > ClientCertPatterns can currently only be set by user policy.
> > If a user policy is already set, then IsPrivate is already true. Why would
you
> > have to check certificate_type here additionally?
> 
> A PSK network that is Private can be shared.
> A network requiring a certificate may be Private but can not be shared.
> This property is used to make that distinction.
> 
>
https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
> File chromeos/network/network_state.h (right):
> 
>
https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
> chromeos/network/network_state.h:91: // Returns true if the network properties
> are stored in the user profile.
> On 2013/07/29 19:13:25, pneubeck wrote:
> > ditto: the -> a
> 
> Done.
> 
>
https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
> File chromeos/network/network_state_unittest.cc (right):
> 
>
https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
> chromeos/network/network_state_unittest.cc:18: class TestStringValue : public
> base::Value {
> On 2013/07/29 19:13:25, pneubeck wrote:
> > Because I stumbled over this before and needs a lot of investigation to
> > understand, please add a comment why this is TestStringValue is required. My
> > guess: for utf8 error testing. But that would actually mean, that the
> Shill/Dbus
> > interface would create invalid base::Values, wouldn't it?
> 
> This code predates https://chromiumcodereview.appspot.com/11418048 and can now
> be removed.
> Done.
> 
>
https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_s...
> chromeos/network/network_state_unittest.cc:62: void
> SignalInitialPropertiesReceived() {
> On 2013/07/29 19:13:25, pneubeck wrote:
> > If InitialPropertiesReceived returns a bool, then it should be check in the
> test
> > too.
> 
> Done.

[stevenjb, 2013-07-29 22:58:53]

I see now. You're right, we never show the 'Share' UI (because of the
PROFILE_NONE check) for remembered networks, so we don't really need this,
I'll rip it out.



On Mon, Jul 29, 2013 at 2:52 PM, <pneubeck@chromium.org> wrote:

> On 2013/07/29 20:25:47, stevenjb (chromium) wrote:
>
>> 'user_profile_required' will be used to determine whether or not a
>> network can
>> be shared. This implements the existing logic; it is likely that the
>> logic is
>> not entirely complete.
>>
>
> I see.
> Wifi::RequiresUserProfile is only called in wifi_config_view.cc under
> certain
> conditions
>
> https://code.google.com/p/**chromium/codesearch#chromium/**
> src/chrome/browser/chromeos/**options/wifi_config_view.cc&**
>
cl=GROK&rcl=1375096781&l=1106<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chromeos/options/wifi_config_view.cc&cl=GROK&rcl=1375096781&l=1106>
> namely, if a not visible network is configured or if the network is
> unconfigured
> (wifi->profile_type() == PROFILE_NONE).
> Configured networks cannot be shared currently.
>
> If the network is visible but not configured yet, you might use
> NetworkState,
> but then UIData will never be set.
> I think the original code was already not that correct.
> 'user_profile_required'
> can only depend on properties of unconfigured networks.
>
> In case of a new network (not visible), the NetworkState cannot be used,
> if the
> 'share this network' checkbox should depend on the settings that the user
> selects in the dialog.
>
>
>
>
> https://codereview.chromium.**org/21030006/diff/3001/**
>
chromeos/network/favorite_**state.h<https://codereview.chromium.org/21030006/diff/3001/chromeos/network/favorite_state.h>
>
>> File chromeos/network/favorite_**state.h (right):
>>
>
>
> https://codereview.chromium.**org/21030006/diff/3001/**
>
chromeos/network/favorite_**state.h#newcode38<https://codereview.chromium.org/21030006/diff/3001/chromeos/network/favorite_state.h#newcode38>
>
>> chromeos/network/favorite_**state.h:38: // Returns true if the network
>>
> properties
>
>> are stored in the user profile.
>> On 2013/07/29 19:13:25, pneubeck wrote:
>> > nit: the user profile -> a user profile
>>
>
>  Done.
>>
>
>
> https://codereview.chromium.**org/21030006/diff/3001/**
>
chromeos/network/managed_**state.h<https://codereview.chromium.org/21030006/diff/3001/chromeos/network/managed_state.h>
>
>> File chromeos/network/managed_**state.h (right):
>>
>
>
> https://codereview.chromium.**org/21030006/diff/3001/**
>
chromeos/network/managed_**state.h#newcode60<https://codereview.chromium.org/21030006/diff/3001/chromeos/network/managed_state.h#newcode60>
>
>> chromeos/network/managed_**state.h:60: // parsed properties, e.g. name
>> from
>> hex_ssid in NetworkState.
>> On 2013/07/29 19:13:25, pneubeck wrote:
>> > nit: remove 'parsed'
>>
>
>  Done.
>>
>
>
> https://codereview.chromium.**org/21030006/diff/3001/**
>
chromeos/network/network_**state.cc<https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_state.cc>
>
>> File chromeos/network/network_**state.cc (right):
>>
>
>
> https://codereview.chromium.**org/21030006/diff/3001/**
>
chromeos/network/network_**state.cc#newcode55<https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_state.cc#newcode55>
>
>> chromeos/network/network_**state.cc:55: // Otherwise returns true and
>> sets
>> |ui_data| if |ui_data_value| is a valid
>> On 2013/07/29 19:13:25, pneubeck wrote:
>> > You can simplify this by setting ui_data to new NetworkUIData(), if
>> > |ui_data_value| is empty.
>> > And setting ui_data to NULL (or not setting it) can replace the result
>>
> false.
>
>> Done
>>
>
>
> https://codereview.chromium.**org/21030006/diff/3001/**
>
chromeos/network/network_**state.cc#newcode216<https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_state.cc#newcode216>
>
>> chromeos/network/network_**state.cc:216: void
>> NetworkState::**InitialPropertiesReceived(
>> On 2013/07/29 19:13:25, pneubeck wrote:
>> > I'm missing a return value that indicates if a property was changed.
>> > Only if something changed, a notification should be triggered.
>> >
>> > Currently, if properties are changed _only_ in this function (and not in
>> > PropertyChanged), then no notification is emitted.
>>
>
>  Hmm, I guess that's theoretically possible. Fixed.
>>
>
>
> https://codereview.chromium.**org/21030006/diff/3001/**
>
chromeos/network/network_**state.cc#newcode455<https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_state.cc#newcode455>
>
>> chromeos/network/network_**state.cc:455: if (ui_data->certificate_type()
>> ==
>> CLIENT_CERT_TYPE_PATTERN)
>> On 2013/07/29 19:13:25, pneubeck wrote:
>> > ClientCertPatterns can currently only be set by user policy.
>> > If a user policy is already set, then IsPrivate is already true. Why
>> would
>>
> you
>
>> > have to check certificate_type here additionally?
>>
>
>  A PSK network that is Private can be shared.
>> A network requiring a certificate may be Private but can not be shared.
>> This property is used to make that distinction.
>>
>
>
> https://codereview.chromium.**org/21030006/diff/3001/**
>
chromeos/network/network_**state.h<https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_state.h>
>
>> File chromeos/network/network_**state.h (right):
>>
>
>
> https://codereview.chromium.**org/21030006/diff/3001/**
>
chromeos/network/network_**state.h#newcode91<https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_state.h#newcode91>
>
>> chromeos/network/network_**state.h:91: // Returns true if the network
>> properties
>> are stored in the user profile.
>> On 2013/07/29 19:13:25, pneubeck wrote:
>> > ditto: the -> a
>>
>
>  Done.
>>
>
>
> https://codereview.chromium.**org/21030006/diff/3001/**
>
chromeos/network/network_**state_unittest.cc<https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_state_unittest.cc>
>
>> File chromeos/network/network_**state_unittest.cc (right):
>>
>
>
> https://codereview.chromium.**org/21030006/diff/3001/**
>
chromeos/network/network_**state_unittest.cc#newcode18<https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_state_unittest.cc#newcode18>
>
>> chromeos/network/network_**state_unittest.cc:18: class TestStringValue :
>> public
>> base::Value {
>> On 2013/07/29 19:13:25, pneubeck wrote:
>> > Because I stumbled over this before and needs a lot of investigation to
>> > understand, please add a comment why this is TestStringValue is
>> required. My
>> > guess: for utf8 error testing. But that would actually mean, that the
>> Shill/Dbus
>> > interface would create invalid base::Values, wouldn't it?
>>
>
>  This code predates
https://chromiumcodereview.**appspot.com/11418048<https://chromiumcodereview....
can now
>> be removed.
>> Done.
>>
>
>
> https://codereview.chromium.**org/21030006/diff/3001/**
>
chromeos/network/network_**state_unittest.cc#newcode62<https://codereview.chromium.org/21030006/diff/3001/chromeos/network/network_state_unittest.cc#newcode62>
>
>> chromeos/network/network_**state_unittest.cc:62: void
>> SignalInitialPropertiesReceive**d() {
>> On 2013/07/29 19:13:25, pneubeck wrote:
>> > If InitialPropertiesReceived returns a bool, then it should be check in
>> the
>> test
>> > too.
>>
>
>  Done.
>>
>
>
>
https://codereview.chromium.**org/21030006/<https://codereview.chromium.org/2...
>

[stevenjb, 2013-07-29 23:36:12]

PTAL

[gauravsh, 2013-07-30 00:19:23]

On 2013/07/29 23:36:12, stevenjb (chromium) wrote:
> PTAL

The commit title and message needs updating.

Also, what's the motivation for changing technology -> network_technology in
NetworkState? Doesn't seem to convey any additional information.

[gauravsh, 2013-07-30 00:24:13]

On 2013/07/30 00:19:23, gauravsh wrote:
> On 2013/07/29 23:36:12, stevenjb (chromium) wrote:
> > PTAL
> 
> The commit title and message needs updating.
> 
> Also, what's the motivation for changing technology -> network_technology in
> NetworkState? Doesn't seem to convey any additional information.

Regardless of the merits of the name change, can you make it a separate CL to
keep the diff here smaller and simpler?

[stevenjb, 2013-07-30 00:24:17]

Description updated.

technology_ was renamed network_technology_ to match the Shill property name and
distinguish it from the technology type property (kTypeWifi, etc). I got
confused at one point and figured being more explicity was better.

[gauravsh, 2013-07-30 00:31:50]

On 2013/07/30 00:24:17, stevenjb (chromium) wrote:
> Description updated.

The patch description still has the wrong title.

> 
> technology_ was renamed network_technology_ to match the Shill property name
and
> distinguish it from the technology type property (kTypeWifi, etc). I got
> confused at one point and figured being more explicity was better.

Ah ok. It's fine to have it in this CL but please update the commit description.
All I want is that the rationale/reason be documented, so future readers have
context for the change.

[stevenjb, 2013-07-30 00:34:16]

On 2013/07/30 00:31:50, gauravsh wrote:
> On 2013/07/30 00:24:17, stevenjb (chromium) wrote:
> > Description updated.
> 
> The patch description still has the wrong title.
> 
> > 
> > technology_ was renamed network_technology_ to match the Shill property name
> and
> > distinguish it from the technology type property (kTypeWifi, etc). I got
> > confused at one point and figured being more explicity was better.
> 
> Ah ok. It's fine to have it in this CL but please update the commit
description.
> All I want is that the rationale/reason be documented, so future readers have
> context for the change.

Done.

[pneubeck (no reviews), 2013-07-30 09:42:52]

lgtm

https://codereview.chromium.org/21030006/diff/36001/chromeos/network/network_...
File chromeos/network/network_state_unittest.cc (left):

https://codereview.chromium.org/21030006/diff/36001/chromeos/network/network_...
chromeos/network/network_state_unittest.cc:18: class TestStringValue : public
base::Value {
As I guessed, this was for utf8 errors testing. Now
NetworkStateTest.SsidTruncateInvalid in debug is broken. Although I don't see
the sense in this test as the DBus interface would have to create invalid
base::Values and that shouldn't be handled in NetworkState.

[pneubeck (no reviews), 2013-07-30 15:03:46]

https://codereview.chromium.org/21030006/diff/36001/chromeos/network/managed_...
File chromeos/network/managed_state.h (right):

https://codereview.chromium.org/21030006/diff/36001/chromeos/network/managed_...
chromeos/network/managed_state.h:16: class DictionaryValue;
order

https://codereview.chromium.org/21030006/diff/36001/chromeos/network/managed_...
chromeos/network/managed_state.h:63: virtual bool InitialPropertiesReceived(
'Initial' is misleading. It's called everytime a full set of properties is
received.

[stevenjb, 2013-07-30 17:40:27]

On 2013/07/30 09:42:52, pneubeck wrote:
> lgtm
> 
>
https://codereview.chromium.org/21030006/diff/36001/chromeos/network/network_...
> File chromeos/network/network_state_unittest.cc (left):
> 
>
https://codereview.chromium.org/21030006/diff/36001/chromeos/network/network_...
> chromeos/network/network_state_unittest.cc:18: class TestStringValue : public
> base::Value {
> As I guessed, this was for utf8 errors testing. Now
> NetworkStateTest.SsidTruncateInvalid in debug is broken. Although I don't see
> the sense in this test as the DBus interface would have to create invalid
> base::Values and that shouldn't be handled in NetworkState.

Ah. Right. Agreed. I'll get rid of the test, not worth the confusion /
maintenance cost.

[stevenjb, 2013-07-30 17:42:46]

On 2013/07/30 15:03:46, pneubeck wrote:
>
https://codereview.chromium.org/21030006/diff/36001/chromeos/network/managed_...
> File chromeos/network/managed_state.h (right):
> 
>
https://codereview.chromium.org/21030006/diff/36001/chromeos/network/managed_...
> chromeos/network/managed_state.h:16: class DictionaryValue;
> order
> 
>
https://codereview.chromium.org/21030006/diff/36001/chromeos/network/managed_...
> chromeos/network/managed_state.h:63: virtual bool InitialPropertiesReceived(
> 'Initial' is misleading. It's called everytime a full set of properties is
> received.

Fair point. We only re-request the complete set of properties after a
configuration change (which we need to make more robust, as we discussed). I
will think about the right naming and change it then.

[stevenjb, 2013-07-30 17:46:41]

On 2013/07/30 17:40:27, stevenjb (chromium) wrote:
> On 2013/07/30 09:42:52, pneubeck wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/21030006/diff/36001/chromeos/network/network_...
> > File chromeos/network/network_state_unittest.cc (left):
> > 
> >
>
https://codereview.chromium.org/21030006/diff/36001/chromeos/network/network_...
> > chromeos/network/network_state_unittest.cc:18: class TestStringValue :
public
> > base::Value {
> > As I guessed, this was for utf8 errors testing. Now
> > NetworkStateTest.SsidTruncateInvalid in debug is broken. Although I don't
see
> > the sense in this test as the DBus interface would have to create invalid
> > base::Values and that shouldn't be handled in NetworkState.
> 
> Ah. Right. Agreed. I'll get rid of the test, not worth the confusion /
> maintenance cost.

Actually... I think it's possible that a bug in Shill could produce a non utf8
string which got passed to StringValue(). Since it's a DCHECK, it could happen
and should be handled. I'll restore TestStringValue and comment it
appropriately.

[pneubeck (no reviews), 2013-07-30 17:47:57]

On 2013/07/30 17:46:41, stevenjb (chromium) wrote:
> On 2013/07/30 17:40:27, stevenjb (chromium) wrote:
> > On 2013/07/30 09:42:52, pneubeck wrote:
> > > lgtm
> > > 
> > >
> >
>
https://codereview.chromium.org/21030006/diff/36001/chromeos/network/network_...
> > > File chromeos/network/network_state_unittest.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/21030006/diff/36001/chromeos/network/network_...
> > > chromeos/network/network_state_unittest.cc:18: class TestStringValue :
> public
> > > base::Value {
> > > As I guessed, this was for utf8 errors testing. Now
> > > NetworkStateTest.SsidTruncateInvalid in debug is broken. Although I don't
> see
> > > the sense in this test as the DBus interface would have to create invalid
> > > base::Values and that shouldn't be handled in NetworkState.
> > 
> > Ah. Right. Agreed. I'll get rid of the test, not worth the confusion /
> > maintenance cost.
> 
> Actually... I think it's possible that a bug in Shill could produce a non utf8
> string which got passed to StringValue(). Since it's a DCHECK, it could happen
> and should be handled. I'll restore TestStringValue and comment it
> appropriately.

That's fine for now, but the correct solution is to handle that _before_ the
base::StringValue is even created.

[stevenjb, 2013-07-31 01:33:56]

Committed patchset #8 manually as r214503 (presubmit successful).