Add NetworkConnectionHandler class

Add NetworkConnectionHandler class

This moves NetworkConfigurationHandler::Connect -> 
NetworkConnectionHandler::ConnectToNetwork and adds error handling
and certificate checking.

Depends on https://codereview.chromium.org/14522013/

Test with --use-new-network-configuration-handlers

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

[stevenjb, 2013-05-03 20:42:42]

OK, here it is, NetworkConnectionHandler.

I'm not trying to get this into M28 (on the contrary I plan to wait for the
branch), so no urgency on the review.

[pneubeck (no reviews), 2013-05-07 08:46:39]

As you can see from my comments, I'm not overly happy with the structure yet.

Highlevel comments:
Any certificate handling
  ->  CertificateLoader/-Handler
Any configuration validation for completeness (connectable?)
  -> based on ONC and at least out of the ConnectionHandler, so that it can be
used more widely.

Please let me land the merging of Managed-/NetworkConfigurationHandler first
(doing that today).

https://codereview.chromium.org/14566009/diff/2001/chromeos/dbus/shill_manage...
File chromeos/dbus/shill_manager_client.h (right):

https://codereview.chromium.org/14566009/diff/2001/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client.h:45: // Add/Remove/Clear Service should only
be called from ShillServiceClient.
nit: Group the functions more clearly (newline before/after) and update the
comment to Add/Remove/Clear ManagerService (?). What about the
MoveServiceToIndex in between?

https://codereview.chromium.org/14566009/diff/2001/chromeos/dbus/shill_manage...
File chromeos/dbus/shill_manager_client_stub.cc (right):

https://codereview.chromium.org/14566009/diff/2001/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client_stub.cc:190: LOG(ERROR) << "ConfugureService
requies GUID and Type to be defined";
ConfugureService -> ConfigureService

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:162: void
NetworkConnectionHandler::InvokeErrorCallback(
cc-local

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:166: std::string error_message =
"Connect Error: " + error_name;
These error_messages are so useless. Not only here but in general. Maybe we can
do a cleanup soon and remove them all from the handler layer and above.

I wonder if Shill adds more useful messages.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:171: bool
NetworkConnectionHandler::NetworkMayNeedCredentials(
cc-local

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:172: const NetworkState* network)
const {
should be
const NetworkState&

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:182: bool
NetworkConnectionHandler::NetworkRequiresActivation(
cc-local

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:183: const NetworkState* network)
const {
should be
const NetworkState&

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:189: void
NetworkConnectionHandler::GetPropertiesCallback(
this should use ONC as the NetworkConfigurationHandler in this form will be
replaced by ManagedNetworkConfigurationHandler.

compared to the SetProperties call below, this should remain a
ConfigurationHandler call, as it depends indeed on configuration (e.g. the
certificate pattern).

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:208:
properties.GetString(flimflam::kProviderHostProperty, &hostname);
I'm strongly against adding general configuration checks here (hostname,
username). Why should username and client_cert_id even be required?

For that we need a more complete solution that is based on ONC. E.g. the ONC
validator already checks for config completeness.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:235:
NetworkUIData::CreateUIDataFromShill(properties);
UIData is and should be read by the config handler.
If we use ONC here (or better in the CertificateHandler), then we don't need
this dependency.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:239: service_path,
error_callback, kConfigurationRequired, error_message);
is that really "ConfigurationRequired"

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:242: if
(ui_data->certificate_type() != CLIENT_CERT_TYPE_PATTERN ||
Why is that an error? Certificate references should be fine too.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:257:
scoped_refptr<net::X509Certificate> matching_cert =
I don't like this code to be here: investigating cert properties, cert matching
and writing the cert related Shill properties (at least lines 233-240 and
256-307).

The CertificateLoader/Handler will have to apply and update certificates as
well. I would prefer to request the CertificateLoader to apply the pattern ASAP
with a return callback. Things are also more complicated because of certificate
references which are local per ONC file. That complexity should be very local in
one class.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:264: // Set the appropriate
propeties, then call connect on success.
nit: propeties -> properties

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:268:
network->GetConfigProperties(&new_properties);
why is this necessary?

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:300:
NetworkConfigurationHandler::Get()->SetProperties(
CertId, KeyId, Pin, ... are not configuration properties and have to be written
directly using the ShillServiceClient.

Password is a configuration however and needs different treatment.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.h (right):

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:23: // The
NetworkConnectionHandler class is used to connect to networks. It
The main responsibility (not contradicting to the listed steps) of this class
should be:
 Keeping track of the state transitions during the process of connecting. (I.e.
a state machine)

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:27: // 2. Request additional
information (e.g. user data which contains certificate
I'm not happy about that checking of sufficient information (see also later
comments).
It's fine with respect to additional information from Shill (like Connectable,
PassphraseRequired) but not wrt. user configuration.
For configuration, we need such tests/verification also in other places (mostly
UI and import of config).

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:29: // 3. Set certificate and
related properties to allow a successful connection
See my later comments: CertificateLoader/Handler is responsible for that.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:61: // |error_message| will
contain and additional error string for debugging.
and -> an

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:88: private:
there are several functions that could be .cc-local or at least static (I marked
them in the .cc).
If they are not tested then I'd put them into the cc.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:104: // Invokes |callback| with
|error_name| for 'result'.
'result' ?

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
File chromeos/network/network_connection_handler_unittest.cc (right):

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler_unittest.cc:55: base::Value* json =
base::JSONReader::Read(json_string);
network/onc/onc_utils.h: ReadDictionaryFromJson
which we could move to network/utils.h or similar.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler_unittest.cc:102: std::string result
= result_;
nit: swap maybe

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler_unittest.cc:131:
TEST_F(NetworkConnectionHandlerTest, NetworkConnectionHandlerConnectSuccess) {
We should also test that at least a connect request to Shill was done.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_u...
File chromeos/network/network_ui_data.cc (right):

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_u...
chromeos/network/network_ui_data.cc:237: scoped_ptr<NetworkUIData>
NetworkUIData::CreateUIDataFromShill(
This exists already in the configuration handler (GetUIData):
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/m...

and shouldn't be duplicated.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_u...
File chromeos/network/network_ui_data.h (right):

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_u...
chromeos/network/network_ui_data.h:93: static scoped_ptr<NetworkUIData>
CreateUIDataFromShill(
I don't think that this should be placed here.
I would really like to keep the knowledge about "what is stored in which Shill
property and in which format" as local as possible.

I.e. UIData itself shouldn't know about where and how it is stored.

[stevenjb, 2013-05-08 01:57:22]

I am fine with switching to calling MangedNetworkConfigurationHandler.

I am also OK with eliminating configuration code from this class.

For the other changes I prefer to take an incremental approach as mentioned.

https://codereview.chromium.org/14566009/diff/2001/chromeos/dbus/shill_manage...
File chromeos/dbus/shill_manager_client.h (right):

https://codereview.chromium.org/14566009/diff/2001/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client.h:45: // Add/Remove/Clear Service should only
be called from ShillServiceClient.
On 2013/05/07 08:46:39, pneubeck wrote:
> nit: Group the functions more clearly (newline before/after) and update the
> comment to Add/Remove/Clear ManagerService (?). What about the
> MoveServiceToIndex in between?
Reordered and moved.

https://codereview.chromium.org/14566009/diff/2001/chromeos/dbus/shill_manage...
File chromeos/dbus/shill_manager_client_stub.cc (right):

https://codereview.chromium.org/14566009/diff/2001/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client_stub.cc:190: LOG(ERROR) << "ConfugureService
requies GUID and Type to be defined";
On 2013/05/07 08:46:39, pneubeck wrote:
> ConfugureService -> ConfigureService

Done.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:162: void
NetworkConnectionHandler::InvokeErrorCallback(
On 2013/05/07 08:46:39, pneubeck wrote:
> cc-local

Done.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:166: std::string error_message =
"Connect Error: " + error_name;
On 2013/05/07 08:46:39, pneubeck wrote:
> These error_messages are so useless. Not only here but in general. Maybe we
can
> do a cleanup soon and remove them all from the handler layer and above.
> 
> I wonder if Shill adds more useful messages.

I'm not quite sure what your complaint is, I have found the entries in the Event
Log very helpful for debugging, but having error_name in error_message was
redundant so I fixed that.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:171: bool
NetworkConnectionHandler::NetworkMayNeedCredentials(
On 2013/05/07 08:46:39, pneubeck wrote:
> cc-local

Done.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:172: const NetworkState* network)
const {
On 2013/05/07 08:46:39, pneubeck wrote:
> should be
> const NetworkState&
Even though cost& is generally preferred, const* is allowed, and I prefer to
keep the code internally consistent, i.e. use only network->foo() instead of
mixing with network.foo().

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:182: bool
NetworkConnectionHandler::NetworkRequiresActivation(
On 2013/05/07 08:46:39, pneubeck wrote:
> cc-local

Done.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:183: const NetworkState* network)
const {
On 2013/05/07 08:46:39, pneubeck wrote:
> should be
> const NetworkState&
ditto.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:189: void
NetworkConnectionHandler::GetPropertiesCallback(
On 2013/05/07 08:46:39, pneubeck wrote:
> this should use ONC as the NetworkConfigurationHandler in this form will be
> replaced by ManagedNetworkConfigurationHandler.
> 
> compared to the SetProperties call below, this should remain a
> ConfigurationHandler call, as it depends indeed on configuration (e.g. the
> certificate pattern).

I don't fully understand your comment. 

I am fine with using ManagedNetworkConfigurationHandler instead.

More details below.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:208:
properties.GetString(flimflam::kProviderHostProperty, &hostname);
I am duplicating the logic from NetworkLibrary. I would like to minimze changes
to the logic in this pass.

In a later pass I would be fine with moving or removing this logic (after
careful consideration), but until/unless it exists elsewhere I would like to
preserve it.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:235:
NetworkUIData::CreateUIDataFromShill(properties);
On 2013/05/07 08:46:39, pneubeck wrote:
> UIData is and should be read by the config handler.
> If we use ONC here (or better in the CertificateHandler), then we don't need
> this dependency.
Same comment as above - that sounds like a good follow-up improvement.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:239: service_path,
error_callback, kConfigurationRequired, error_message);
On 2013/05/07 08:46:39, pneubeck wrote:
> is that really "ConfigurationRequired"
I screwed up this logic; what were intended to be early exits became failures. I
combined the logic to simplify this.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:257:
scoped_refptr<net::X509Certificate> matching_cert =
This also seems like something that can be improved in a follow-up CL; I'd like
to maintain the existing logic for now.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:300:
NetworkConfigurationHandler::Get()->SetProperties(
On 2013/05/07 08:46:39, pneubeck wrote:
> CertId, KeyId, Pin, ... are not configuration properties and have to be
written
> directly using the ShillServiceClient.
> 
> Password is a configuration however and needs different treatment.

I removed the configuration from this flow, as I mentioned I think we can
migrate it to NetworkConfigurationHandler when we update NetworkConfigView.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.h (right):

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:23: // The
NetworkConnectionHandler class is used to connect to networks. It
On 2013/05/07 08:46:39, pneubeck wrote:
> The main responsibility (not contradicting to the listed steps) of this class
> should be:
>  Keeping track of the state transitions during the process of connecting.
(I.e.
> a state machine)
This is also the only class that should make Connect calls to Shill. I'll expand
the comment.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:27: // 2. Request additional
information (e.g. user data which contains certificate
On 2013/05/07 08:46:39, pneubeck wrote:
> I'm not happy about that checking of sufficient information (see also later
> comments).
> It's fine with respect to additional information from Shill (like Connectable,
> PassphraseRequired) but not wrt. user configuration.
> For configuration, we need such tests/verification also in other places
(mostly
> UI and import of config).

I agree with centralizing any shared logic, however this class should still make
those calls, at least to the extent of establishing whether or not sufficient
information is available.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:29: // 3. Set certificate and
related properties to allow a successful connection
On 2013/05/07 08:46:39, pneubeck wrote:
> See my later comments: CertificateLoader/Handler is responsible for that.

I have thought about this and related comments, and I think I agree that we can
remove any certificate configuration from this class and handle it exclusively
elsewhere.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:61: // |error_message| will
contain and additional error string for debugging.
On 2013/05/07 08:46:39, pneubeck wrote:
> and -> an

Done.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:104: // Invokes |callback| with
|error_name| for 'result'.
On 2013/05/07 08:46:39, pneubeck wrote:
> 'result' ?

Done.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
File chromeos/network/network_connection_handler_unittest.cc (right):

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler_unittest.cc:55: base::Value* json =
base::JSONReader::Read(json_string);
On 2013/05/07 08:46:39, pneubeck wrote:
> network/onc/onc_utils.h: ReadDictionaryFromJson
> which we could move to network/utils.h or similar.

Done.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler_unittest.cc:102: std::string result
= result_;
On 2013/05/07 08:46:39, pneubeck wrote:
> nit: swap maybe

Done.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler_unittest.cc:131:
TEST_F(NetworkConnectionHandlerTest, NetworkConnectionHandlerConnectSuccess) {
On 2013/05/07 08:46:39, pneubeck wrote:
> We should also test that at least a connect request to Shill was done.
Done

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_u...
File chromeos/network/network_ui_data.cc (right):

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_u...
chromeos/network/network_ui_data.cc:237: scoped_ptr<NetworkUIData>
NetworkUIData::CreateUIDataFromShill(
On 2013/05/07 08:46:39, pneubeck wrote:
> This exists already in the configuration handler (GetUIData):
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/m...
> 
> and shouldn't be duplicated.

Done.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_u...
File chromeos/network/network_ui_data.h (right):

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_u...
chromeos/network/network_ui_data.h:93: static scoped_ptr<NetworkUIData>
CreateUIDataFromShill(
On 2013/05/07 08:46:39, pneubeck wrote:
> I don't think that this should be placed here.
> I would really like to keep the knowledge about "what is stored in which Shill
> property and in which format" as local as possible.
> 
> I.e. UIData itself shouldn't know about where and how it is stored.

Hmm, I'm not entirely convinced that UIData should be independent of Shill, but
made GetUIData part of ManagedConfigurationHandler and used that instead.

[pneubeck (no reviews), 2013-05-08 08:07:46]

I'm fine with cleanups in follow-up patches.
Please mention in the commit message that you cleaned up the ShillStubs as well.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:166: std::string error_message =
"Connect Error: " + error_name;
On 2013/05/08 01:57:22, stevenjb (chromium) wrote:
> On 2013/05/07 08:46:39, pneubeck wrote:
> > These error_messages are so useless. Not only here but in general. Maybe we
> can
> > do a cleanup soon and remove them all from the handler layer and above.
> > 
> > I wonder if Shill adds more useful messages.
> 
> I'm not quite sure what your complaint is, I have found the entries in the
Event
> Log very helpful for debugging, but having error_name in error_message was
> redundant so I fixed that.

In many places of chrome where we create (not forward) error_name and
error_message pairs, their content is redundant.
If Shill really puts useful information in both fields, then it's fine.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:189: void
NetworkConnectionHandler::GetPropertiesCallback(
On 2013/05/08 01:57:22, stevenjb (chromium) wrote:
> On 2013/05/07 08:46:39, pneubeck wrote:
> > this should use ONC as the NetworkConfigurationHandler in this form will be
> > replaced by ManagedNetworkConfigurationHandler.
> > 
> > compared to the SetProperties call below, this should remain a
> > ConfigurationHandler call, as it depends indeed on configuration (e.g. the
> > certificate pattern).
> 
> I don't fully understand your comment. 
> 
> I am fine with using ManagedNetworkConfigurationHandler instead.
> 
> More details below.
> 

NetworkConfigurationHandler::GetProperties returns a Shill dictionary.

ManagedNetworkConfigurationHandler::GetProperties returns a ONC dictionary.

The code in this function shouldn't work anymore, e.g.
properties.GetString(flimflam::kProviderHostProperty, &hostname);

must be

properties -> network_config::kVPN -> vpn::kHost

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.h (right):

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:27: // 2. Request additional
information (e.g. user data which contains certificate
On 2013/05/08 01:57:22, stevenjb (chromium) wrote:
> On 2013/05/07 08:46:39, pneubeck wrote:
> > I'm not happy about that checking of sufficient information (see also later
> > comments).
> > It's fine with respect to additional information from Shill (like
Connectable,
> > PassphraseRequired) but not wrt. user configuration.
> > For configuration, we need such tests/verification also in other places
> (mostly
> > UI and import of config).
> 
> I agree with centralizing any shared logic, however this class should still
make
> those calls, at least to the extent of establishing whether or not sufficient
> information is available.
> 

sg

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:88: private:
On 2013/05/07 08:46:39, pneubeck wrote:
> there are several functions that could be .cc-local or at least static (I
marked
> them in the .cc).
> If they are not tested then I'd put them into the cc.

Done.

https://codereview.chromium.org/14566009/diff/32001/chromeos/network/network_...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/14566009/diff/32001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:56: NetworkConnectionHandler*
g_connection_handler_instance = NULL;
static

https://codereview.chromium.org/14566009/diff/32001/chromeos/network/network_...
File chromeos/network/network_connection_handler_unittest.cc (right):

https://codereview.chromium.org/14566009/diff/32001/chromeos/network/network_...
chromeos/network/network_connection_handler_unittest.cc:65: *json_dict.get(),
.get() not needed.

[stevenjb, 2013-05-08 18:48:00]

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:166: std::string error_message =
"Connect Error: " + error_name;
On 2013/05/08 08:07:46, pneubeck wrote:
> On 2013/05/08 01:57:22, stevenjb (chromium) wrote:
> > On 2013/05/07 08:46:39, pneubeck wrote:
> > > These error_messages are so useless. Not only here but in general. Maybe
we
> > can
> > > do a cleanup soon and remove them all from the handler layer and above.
> > > 
> > > I wonder if Shill adds more useful messages.
> > 
> > I'm not quite sure what your complaint is, I have found the entries in the
> Event
> > Log very helpful for debugging, but having error_name in error_message was
> > redundant so I fixed that.
> 
> In many places of chrome where we create (not forward) error_name and
> error_message pairs, their content is redundant.
> If Shill really puts useful information in both fields, then it's fine.

I see. In this case, it is helpful to know that the shill error originated from
a connect attempt (but appending the error name again was in fact redundant)

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:189: void
NetworkConnectionHandler::GetPropertiesCallback(
On 2013/05/08 08:07:46, pneubeck wrote:
> On 2013/05/08 01:57:22, stevenjb (chromium) wrote:
> > On 2013/05/07 08:46:39, pneubeck wrote:
> > > this should use ONC as the NetworkConfigurationHandler in this form will
be
> > > replaced by ManagedNetworkConfigurationHandler.
> > > 
> > > compared to the SetProperties call below, this should remain a
> > > ConfigurationHandler call, as it depends indeed on configuration (e.g. the
> > > certificate pattern).
> > 
> > I don't fully understand your comment. 
> > 
> > I am fine with using ManagedNetworkConfigurationHandler instead.
> > 
> > More details below.
> > 
> 
> NetworkConfigurationHandler::GetProperties returns a Shill dictionary.
> 
> ManagedNetworkConfigurationHandler::GetProperties returns a ONC dictionary.
> 
> The code in this function shouldn't work anymore, e.g.
> properties.GetString(flimflam::kProviderHostProperty, &hostname);
> 
> must be
> 
> properties -> network_config::kVPN -> vpn::kHost

I see. Somehow I didn't realize that. OK, I'll switch this back to
NetworkConfigurationHandler and we can migrate later, is that reasonable?

https://codereview.chromium.org/14566009/diff/32001/chromeos/network/network_...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/14566009/diff/32001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:56: NetworkConnectionHandler*
g_connection_handler_instance = NULL;
On 2013/05/08 08:07:46, pneubeck wrote:
> static

Done.

https://codereview.chromium.org/14566009/diff/32001/chromeos/network/network_...
File chromeos/network/network_connection_handler_unittest.cc (right):

https://codereview.chromium.org/14566009/diff/32001/chromeos/network/network_...
chromeos/network/network_connection_handler_unittest.cc:65: *json_dict.get(),
On 2013/05/08 08:07:46, pneubeck wrote:
> .get() not needed.

Done.

[gauravsh, 2013-05-08 19:22:04]

https://codereview.chromium.org/14566009/diff/47001/chromeos/network/network_...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/14566009/diff/47001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:38: bool
NetworkMayNeedCredentials(const NetworkState* network) {
Please add a comment or use a better name. A passphrase is a credential as well,
and this is only checking for a subset of the type of networks that require some
form of credentials.

https://codereview.chromium.org/14566009/diff/47001/chromeos/network/network_...
File chromeos/network/network_connection_handler.h (right):

https://codereview.chromium.org/14566009/diff/47001/chromeos/network/network_...
chromeos/network/network_connection_handler.h:77: static const char
kNetworkNotFound[];
as per style, static const members should be declared before any methods.

https://codereview.chromium.org/14566009/diff/47001/chromeos/network/network_...
chromeos/network/network_connection_handler.h:79: static const char
kConnecting[];
Renaming these constants as kConnectedError, kConnectingError, etc. would make
it much clearer what they are meant to be used for.

https://codereview.chromium.org/14566009/diff/47001/chromeos/network/network_...
chromeos/network/network_connection_handler.h:104: // whether to invoke
|error_callback|, or attemt a connection.
attempt

This also needs a more detailed comment since it's only parsing properties for
the 8021x and VPN case.

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

https://codereview.chromium.org/14566009/diff/47001/chromeos/network/network_...
chromeos/network/network_state.h:59: // Wifi property accessors
Seems redundant. There's already a wireless accessor comment above.

https://codereview.chromium.org/14566009/diff/50001/chromeos/network/network_...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/14566009/diff/50001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:207: if (network->type() ==
flimflam::kTypeVPN) {
There are 2 separate checks here:
1) whether we have sufficient properties for connecting to a VPN
2) whether we have sufficient properties for connecting to a certificate based
network

Moving these into own separate methods would make it more readable and also
allow you to write unit tests for these checks.

[stevenjb, 2013-05-09 00:13:00]

https://codereview.chromium.org/14566009/diff/47001/chromeos/network/network_...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/14566009/diff/47001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:38: bool
NetworkMayNeedCredentials(const NetworkState* network) {
On 2013/05/08 19:22:04, gauravsh wrote:
> Please add a comment or use a better name. A passphrase is a credential as
well,
> and this is only checking for a subset of the type of networks that require
some
> form of credentials.

Suggestions? This seems pretty descriptive; I don't think the name implies that
it checks every possible credential.

https://codereview.chromium.org/14566009/diff/47001/chromeos/network/network_...
File chromeos/network/network_connection_handler.h (right):

https://codereview.chromium.org/14566009/diff/47001/chromeos/network/network_...
chromeos/network/network_connection_handler.h:77: static const char
kNetworkNotFound[];
On 2013/05/08 19:22:04, gauravsh wrote:
> as per style, static const members should be declared before any methods.

Done.

https://codereview.chromium.org/14566009/diff/47001/chromeos/network/network_...
chromeos/network/network_connection_handler.h:79: static const char
kConnecting[];
On 2013/05/08 19:22:04, gauravsh wrote:
> Renaming these constants as kConnectedError, kConnectingError, etc. would make
> it much clearer what they are meant to be used for.

Done.

https://codereview.chromium.org/14566009/diff/47001/chromeos/network/network_...
chromeos/network/network_connection_handler.h:104: // whether to invoke
|error_callback|, or attemt a connection.
On 2013/05/08 19:22:04, gauravsh wrote:
> attempt
> 
> This also needs a more detailed comment since it's only parsing properties for
> the 8021x and VPN case.

Renamed, improved comment.

https://codereview.chromium.org/14566009/diff/50001/chromeos/network/network_...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/14566009/diff/50001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:207: if (network->type() ==
flimflam::kTypeVPN) {
On 2013/05/08 19:22:04, gauravsh wrote:
> There are 2 separate checks here:
> 1) whether we have sufficient properties for connecting to a VPN
> 2) whether we have sufficient properties for connecting to a certificate based
> network
> 
> Moving these into own separate methods would make it more readable and also
> allow you to write unit tests for these checks.

I'm not sure that unit tests for either section would be especially useful,
however this function should definitely be broken up.
Done.

[gauravsh, 2013-05-09 18:22:55]

https://codereview.chromium.org/14566009/diff/47001/chromeos/network/network_...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/14566009/diff/47001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:38: bool
NetworkMayNeedCredentials(const NetworkState* network) {
On 2013/05/09 00:13:00, stevenjb (chromium) wrote:
> On 2013/05/08 19:22:04, gauravsh wrote:
> > Please add a comment or use a better name. A passphrase is a credential as
> well,
> > and this is only checking for a subset of the type of networks that require
> some
> > form of credentials.
> 
> Suggestions? This seems pretty descriptive; I don't think the name implies
that
> it checks every possible credential.

Since this method is limited to checking for the VPN and certificate network
case, a comment reflecting that is useful information for readers of the code in
the future. They (and that includes us) may not have the full context or
understand the intention of what exactly this is meant to be checking for.

[pneubeck (no reviews), 2013-05-13 09:29:35]

lgtm with some minor issues.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:189: void
NetworkConnectionHandler::GetPropertiesCallback(
On 2013/05/08 18:48:00, stevenjb (chromium) wrote:
> On 2013/05/08 08:07:46, pneubeck wrote:
> > On 2013/05/08 01:57:22, stevenjb (chromium) wrote:
> > > On 2013/05/07 08:46:39, pneubeck wrote:
> > > > this should use ONC as the NetworkConfigurationHandler in this form will
> be
> > > > replaced by ManagedNetworkConfigurationHandler.
> > > > 
> > > > compared to the SetProperties call below, this should remain a
> > > > ConfigurationHandler call, as it depends indeed on configuration (e.g.
the
> > > > certificate pattern).
> > > 
> > > I don't fully understand your comment. 
> > > 
> > > I am fine with using ManagedNetworkConfigurationHandler instead.
> > > 
> > > More details below.
> > > 
> > 
> > NetworkConfigurationHandler::GetProperties returns a Shill dictionary.
> > 
> > ManagedNetworkConfigurationHandler::GetProperties returns a ONC dictionary.
> > 
> > The code in this function shouldn't work anymore, e.g.
> > properties.GetString(flimflam::kProviderHostProperty, &hostname);
> > 
> > must be
> > 
> > properties -> network_config::kVPN -> vpn::kHost
> 
> I see. Somehow I didn't realize that. OK, I'll switch this back to
> NetworkConfigurationHandler and we can migrate later, is that reasonable?

yes, fine with me.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:208:
properties.GetString(flimflam::kProviderHostProperty, &hostname);
On 2013/05/08 01:57:22, stevenjb (chromium) wrote:
> I am duplicating the logic from NetworkLibrary. I would like to minimze
changes
> to the logic in this pass.
> 
> In a later pass I would be fine with moving or removing this logic (after
> careful consideration), but until/unless it exists elsewhere I would like to
> preserve it.

then better add a comment that this code is preliminary and needs
removal/revision. Otherwise, we may wonder in the future about, which parts were
on purpose and which not.

https://codereview.chromium.org/14566009/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:235:
NetworkUIData::CreateUIDataFromShill(properties);
On 2013/05/08 01:57:22, stevenjb (chromium) wrote:
> On 2013/05/07 08:46:39, pneubeck wrote:
> > UIData is and should be read by the config handler.
> > If we use ONC here (or better in the CertificateHandler), then we don't need
> > this dependency.
> Same comment as above - that sounds like a good follow-up improvement.

then also add a comment here, please.

https://codereview.chromium.org/14566009/diff/65001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/14566009/diff/65001/chrome/browser/chromeos/c...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:378:
NetworkConnectionHandler::Shutdown();
nit because changed in the other CL: Order of initialization.

https://codereview.chromium.org/14566009/diff/65001/chromeos/dbus/shill_manag...
File chromeos/dbus/shill_manager_client_stub.cc (right):

https://codereview.chromium.org/14566009/diff/65001/chromeos/dbus/shill_manag...
chromeos/dbus/shill_manager_client_stub.cc:448:
GetListProperty(flimflam::kServiceWatchListProperty)->Remove(
please drop a comment in the header, that this adds/moves to the front of the
watch list.

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

https://codereview.chromium.org/14566009/diff/65001/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client_stub.cc:193:
chromeos::switches::kEnableStubInteractive)) {
for tests: shouldn't we still emit the associating state, even if we immediately
switch to online?

https://codereview.chromium.org/14566009/diff/65001/chromeos/network/cert_loa...
File chromeos/network/cert_loader.cc (right):

https://codereview.chromium.org/14566009/diff/65001/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:31: if (base::chromeos::IsRunningOnChromeOS())
did or how did certificates work in chromeos-on-linux with CertLibrary?

https://codereview.chromium.org/14566009/diff/65001/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:124: // For background see this discussion on
dev-tech-crypto.lists.mozilla.org:
Nit: maybe drop a comment where you copied this from.

https://codereview.chromium.org/14566009/diff/65001/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:249: LOG(WARNING) << "Re-Requesting
Certificates.";
nit: if this is happens frequently on normal starts, then maybe vlog

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

https://codereview.chromium.org/14566009/diff/65001/chromeos/network/network_...
chromeos/network/network_state.h:42: void
GetConfigProperties(base::DictionaryValue* dictionary) const;
not used anymore. maybe needed in the future, but if not we'll easily forget to
remove it.

https://codereview.chromium.org/14566009/diff/65001/chromeos/network/network_...
File chromeos/network/network_ui_data.h (right):

https://codereview.chromium.org/14566009/diff/65001/chromeos/network/network_...
chromeos/network/network_ui_data.h:85: static scoped_ptr<NetworkUIData>
CreateUIDataFromONC(
nit: rename to CreateFromONC

[stevenjb, 2013-05-13 20:25:51]

https://codereview.chromium.org/14566009/diff/65001/chromeos/dbus/shill_manag...
File chromeos/dbus/shill_manager_client_stub.cc (right):

https://codereview.chromium.org/14566009/diff/65001/chromeos/dbus/shill_manag...
chromeos/dbus/shill_manager_client_stub.cc:448:
GetListProperty(flimflam::kServiceWatchListProperty)->Remove(
On 2013/05/13 09:29:36, pneubeck wrote:
> please drop a comment in the header, that this adds/moves to the front of the
> watch list.
Commented here as an implementation comment.

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

https://codereview.chromium.org/14566009/diff/65001/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client_stub.cc:193:
chromeos::switches::kEnableStubInteractive)) {
On 2013/05/13 09:29:36, pneubeck wrote:
> for tests: shouldn't we still emit the associating state, even if we
immediately
> switch to online?
Fair. Done.

https://codereview.chromium.org/14566009/diff/65001/chromeos/network/cert_loa...
File chromeos/network/cert_loader.cc (right):

https://codereview.chromium.org/14566009/diff/65001/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:31: if (base::chromeos::IsRunningOnChromeOS())
On 2013/05/13 09:29:36, pneubeck wrote:
> did or how did certificates work in chromeos-on-linux with CertLibrary?
I do not believe that they ever have worked.

https://codereview.chromium.org/14566009/diff/65001/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:249: LOG(WARNING) << "Re-Requesting
Certificates.";
On 2013/05/13 09:29:36, pneubeck wrote:
> nit: if this is happens frequently on normal starts, then maybe vlog
It doesn't, or shouldn't.

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

https://codereview.chromium.org/14566009/diff/65001/chromeos/network/network_...
chromeos/network/network_state.h:42: void
GetConfigProperties(base::DictionaryValue* dictionary) const;
On 2013/05/13 09:29:36, pneubeck wrote:
> not used anymore. maybe needed in the future, but if not we'll easily forget
to
> remove it.

Done.

https://codereview.chromium.org/14566009/diff/65001/chromeos/network/network_...
File chromeos/network/network_ui_data.h (right):

https://codereview.chromium.org/14566009/diff/65001/chromeos/network/network_...
chromeos/network/network_ui_data.h:85: static scoped_ptr<NetworkUIData>
CreateUIDataFromONC(
On 2013/05/13 09:29:36, pneubeck wrote:
> nit: rename to CreateFromONC

Done.

[Greg Spencer (Chromium), 2013-05-13 20:28:52]

LGTM

https://codereview.chromium.org/14566009/diff/80001/chromeos/network/cert_loa...
File chromeos/network/cert_loader.cc (right):

https://codereview.chromium.org/14566009/diff/80001/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:249: LOG(WARNING) << "Re-Requesting
Certificates.";
Nit: Does this really need to be a warning?  Could it be a VLOG(1)?

[stevenjb, 2013-05-14 00:07:08]

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

https://codereview.chromium.org/14566009/diff/65001/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client_stub.cc:193:
chromeos::switches::kEnableStubInteractive)) {
On 2013/05/13 20:25:52, stevenjb (chromium) wrote:
> On 2013/05/13 09:29:36, pneubeck wrote:
> > for tests: shouldn't we still emit the associating state, even if we
> immediately
> > switch to online?
> Fair. Done.
> 
Oops, I recall why I didn't do this now; it affects the predictability of JS
tests (onNetworksChangedEventConnect).

[commit-bot: I haz the power, 2013-05-14 00:07:40]

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

[commit-bot: I haz the power, 2013-05-14 04:37:58]

Change committed as 199893

[pneubeck (no reviews), 2013-05-14 14:27:33]

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

https://codereview.chromium.org/14566009/diff/65001/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client_stub.cc:193:
chromeos::switches::kEnableStubInteractive)) {
On 2013/05/14 00:07:08, stevenjb (chromium) wrote:
> On 2013/05/13 20:25:52, stevenjb (chromium) wrote:
> > On 2013/05/13 09:29:36, pneubeck wrote:
> > > for tests: shouldn't we still emit the associating state, even if we
> > immediately
> > > switch to online?
> > Fair. Done.
> > 
> Oops, I recall why I didn't do this now; it affects the predictability of JS
> tests (onNetworksChangedEventConnect).

The right solution is to behave like Shill (whatever it does which is likely
depending on the network type).

At least drop a comment, why you omit it here.