Convert all connect code to use NetworkHandler instead of NetworkLibrary

Convert all connect code to use NetworkHandler instead of NetworkLibrary

* Eliminates kUseNewNetworkConnectionHandler
* Converts *_config_view to NetworkStateHandler + network_connect
* Migrates non Chrome/Browser dependent network_connect code to
  ash::network_connect.

BUG=263978
R=gauravsh@chromium.org, pneubeck@chromium.org

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

[stevenjb, 2013-07-30 01:07:49]

OK, I apologize for the size of this, but I've mostly already pulled out what I
could.

I'm going to test this as much as I can tomorrow in MTV, but I don't expect us
to catch everything the first round. That said, please try to take a careful
look at any code you are familiar with.

The good news is that once this is in, there won't be that much code left
relying on NetworkLibrary...

[stevenjb, 2013-08-01 23:35:42]

OK, this has been rebased with the dependent changes and tested. I also added
some additional debugging info when setting properties that should help both
with any new bugs, and with tracking down some old ones.
PTAL

[gauravsh, 2013-08-02 23:40:21]

Here's a first batch of minor comments for everything under src/chromeos/*. I
will look at the code under chrome/browser/ next.

https://codereview.chromium.org/21046008/diff/42001/chromeos/network/favorite...
File chromeos/network/favorite_state.cc (right):

https://codereview.chromium.org/21046008/diff/42001/chromeos/network/favorite...
chromeos/network/favorite_state.cc:25: const base::Value& value) {
NIT: fix indent here

https://codereview.chromium.org/21046008/diff/42001/chromeos/network/network_...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/21046008/diff/42001/chromeos/network/network_...
chromeos/network/network_configuration_handler.cc:52: return key ==
flimflam::kEapPrivateKeyPasswordProperty ||
Looking at service_constants.h, should the following also be filtered out?
- EapClientPrivateKey
- EapPinProperty
- ApnPasswordProperty

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

https://codereview.chromium.org/21046008/diff/42001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:388:
provider_properties->GetStringWithoutPathExpansion(
Ok if there's no vpn_provider_host?

https://codereview.chromium.org/21046008/diff/42001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:435: if (type ==
flimflam::kTypeVPN) {
Lets make all property settings for a given type-vpn provider be in a single
place (like in the previous code). Even though that means duplicating the if
(cert_loader_) check, it's more readable and scan that all required properties
are correctly being set.

// Configure tpm slots/pins if needed.
string tpm_slot = ...
string tpm_pin = ...

// Set properties
if (type == flimflam::kTypeVPN) {
   if (vpn_provider_type == flimflam::kProviderOpenVpn) {
    // set all required config_properties for OpenVPN type.
    // set properties needing tpm if cert_loader_
   } else {
    // set all required config_properties for L2IPSec type.
    // set properties needing tpm if cert_loader_
   }
} else if (type == flimflam::kTypeWifi) {
  // set all required config_properties for 8021x
  // set properties needing tpm if cert_loader_
}

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

https://codereview.chromium.org/21046008/diff/42001/chromeos/network/network_...
chromeos/network/network_connection_handler.h:187: // Handle success or failure
from Shill.Service.ActivateCellularModem.
NIT: s/or failure//

[gauravsh, 2013-08-03 01:15:54]

For the most part I have tried to compare the old code (using NetworkLibrary)
and the new one (using NetworkHandler) to check that they are functionally
similar.

While nothing out of the ordinary jumped out to me, it's hard to be sure that
there are no problems because of the size of the change.

https://codereview.chromium.org/21046008/diff/42001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/network_config_view.cc (right):

https://codereview.chromium.org/21046008/diff/42001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_config_view.cc:104: new
WifiConfigView(this, "" /* service_path */, false /* show_8021x */);
NIT: This is more readable if the args on separate lines.

https://codereview.chromium.org/21046008/diff/42001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_config_view.cc:109: child_config_view_ =
new VPNConfigView(this, "" /* service_path */);
NIT: args on separate lines.

https://codereview.chromium.org/21046008/diff/42001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_config_view.cc:212: new
WifiConfigView(this, "" /* service_path */, true /* show_8021x */);
NIT: separate lines if you please

https://codereview.chromium.org/21046008/diff/42001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/vpn_config_view.cc (right):

https://codereview.chromium.org/21046008/diff/42001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:570: layout_->StartRow(0, 0);
The enable_psk_passphrase_ isn't needed anymore?

https://codereview.chromium.org/21046008/diff/42001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:582: layout_->StartRow(0, 0);
Why no check for the service_path_.empty() here?

https://codereview.chromium.org/21046008/diff/42001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:614:
layout_->AddView(username_textfield_);
No longer a call to username_textfield_->SetText(). Intentional?

https://codereview.chromium.org/21046008/diff/42001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:649: layout_->AddView(new
ControlledSettingIndicatorView(group_name_ui_data_));
Here as well.

https://codereview.chromium.org/21046008/diff/42001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:681: if (!vpn) {
DCHECK the type to make sure it's indeed a kTypeVpn?

https://codereview.chromium.org/21046008/diff/42001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:717:
flimflam::kPassphraseRequiredProperty, &user_passphrase_required);
Where is user_passphrase_required used?

https://codereview.chromium.org/21046008/diff/42001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:726: if (provider_type_index
== PROVIDER_TYPE_INDEX_L2TP_IPSEC_PSK) {
To keep this method smaller, this ONC dictionary handling can be moved into a
separate method.

https://codereview.chromium.org/21046008/diff/42001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:1015: void
VPNConfigView::ParseVPNUIProperty(NetworkPropertyUIData* property_ui_data,
Since property_ui_data is an output parameter, lets make it the last argument.

https://codereview.chromium.org/21046008/diff/42001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/wimax_config_view.cc (right):

https://codereview.chromium.org/21046008/diff/42001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wimax_config_view.cc:101:
GetNetworkState(service_path_);
Add DCHECK to verity this is a kTypeWimax network.

https://codereview.chromium.org/21046008/diff/42001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wimax_config_view.cc:156: bool wimax_shared =
!wimax->IsPrivate();
What are lines l156 - l175 doing? (add a comment too please)

[stevenjb, 2013-08-06 00:32:48]

https://chromiumcodereview.appspot.com/21046008/diff/42001/chrome/browser/chr...
File chrome/browser/chromeos/options/network_config_view.cc (right):

https://chromiumcodereview.appspot.com/21046008/diff/42001/chrome/browser/chr...
chrome/browser/chromeos/options/network_config_view.cc:104: new
WifiConfigView(this, "" /* service_path */, false /* show_8021x */);
On 2013/08/03 01:15:54, gauravsh wrote:
> NIT: This is more readable if the args on separate lines.

Done.

https://chromiumcodereview.appspot.com/21046008/diff/42001/chrome/browser/chr...
chrome/browser/chromeos/options/network_config_view.cc:109: child_config_view_ =
new VPNConfigView(this, "" /* service_path */);
On 2013/08/03 01:15:54, gauravsh wrote:
> NIT: args on separate lines.

Done.

https://chromiumcodereview.appspot.com/21046008/diff/42001/chrome/browser/chr...
chrome/browser/chromeos/options/network_config_view.cc:212: new
WifiConfigView(this, "" /* service_path */, true /* show_8021x */);
On 2013/08/03 01:15:54, gauravsh wrote:
> NIT: separate lines if you please

Done.

https://chromiumcodereview.appspot.com/21046008/diff/42001/chrome/browser/chr...
File chrome/browser/chromeos/options/vpn_config_view.cc (right):

https://chromiumcodereview.appspot.com/21046008/diff/42001/chrome/browser/chr...
chrome/browser/chromeos/options/vpn_config_view.cc:570: layout_->StartRow(0, 0);
On 2013/08/03 01:15:54, gauravsh wrote:
> The enable_psk_passphrase_ isn't needed anymore?

Because we are asynchronously requesting the properties required to set
enable_psk_passphrase_, we initially assume it will be true when we layout the
View, then disable (rather than omit) the elements that do not apply. This will
typically add 1-2 lines for configured VPNs, but vastly simplifies the layout.
(Dynamic layout in Views is a bit of a pain).

When we re-factor the UI, we plan to greatly simplify this for configured VPNs.

https://chromiumcodereview.appspot.com/21046008/diff/42001/chrome/browser/chr...
chrome/browser/chromeos/options/vpn_config_view.cc:582: layout_->StartRow(0, 0);
On 2013/08/03 01:15:54, gauravsh wrote:
> Why no check for the service_path_.empty() here?

Hm, I'm not sure actually, good catch, I'll fix that.
Done.

https://chromiumcodereview.appspot.com/21046008/diff/42001/chrome/browser/chr...
chrome/browser/chromeos/options/vpn_config_view.cc:614:
layout_->AddView(username_textfield_);
On 2013/08/03 01:15:54, gauravsh wrote:
> No longer a call to username_textfield_->SetText(). Intentional?
Yes, that got moved to the bottom of InitFromProperties when the username
property is available.

https://chromiumcodereview.appspot.com/21046008/diff/42001/chrome/browser/chr...
chrome/browser/chromeos/options/vpn_config_view.cc:649: layout_->AddView(new
ControlledSettingIndicatorView(group_name_ui_data_));
On 2013/08/03 01:15:54, gauravsh wrote:
> Here as well.
Ditto

https://chromiumcodereview.appspot.com/21046008/diff/42001/chrome/browser/chr...
chrome/browser/chromeos/options/vpn_config_view.cc:681: if (!vpn) {
On 2013/08/03 01:15:54, gauravsh wrote:
> DCHECK the type to make sure it's indeed a kTypeVpn?
Added a DCHECK in Init()

https://chromiumcodereview.appspot.com/21046008/diff/42001/chrome/browser/chr...
chrome/browser/chromeos/options/vpn_config_view.cc:717:
flimflam::kPassphraseRequiredProperty, &user_passphrase_required);
On 2013/08/03 01:15:54, gauravsh wrote:
> Where is user_passphrase_required used?
It's not any more. I looked at how we were using this and I'm pretty certain it
isn't useful for VPNs. 
Removed.

https://chromiumcodereview.appspot.com/21046008/diff/42001/chrome/browser/chr...
chrome/browser/chromeos/options/vpn_config_view.cc:726: if (provider_type_index
== PROVIDER_TYPE_INDEX_L2TP_IPSEC_PSK) {
On 2013/08/03 01:15:54, gauravsh wrote:
> To keep this method smaller, this ONC dictionary handling can be moved into a
> separate method.
Added ParseUIProperties()

https://chromiumcodereview.appspot.com/21046008/diff/42001/chrome/browser/chr...
chrome/browser/chromeos/options/vpn_config_view.cc:1015: void
VPNConfigView::ParseVPNUIProperty(NetworkPropertyUIData* property_ui_data,
On 2013/08/03 01:15:54, gauravsh wrote:
> Since property_ui_data is an output parameter, lets make it the last argument.

Done.

https://chromiumcodereview.appspot.com/21046008/diff/42001/chrome/browser/chr...
File chrome/browser/chromeos/options/wimax_config_view.cc (right):

https://chromiumcodereview.appspot.com/21046008/diff/42001/chrome/browser/chr...
chrome/browser/chromeos/options/wimax_config_view.cc:101:
GetNetworkState(service_path_);
On 2013/08/03 01:15:54, gauravsh wrote:
> Add DCHECK to verity this is a kTypeWimax network.
Added a DCHECK in Init()

https://chromiumcodereview.appspot.com/21046008/diff/42001/chrome/browser/chr...
chrome/browser/chromeos/options/wimax_config_view.cc:156: bool wimax_shared =
!wimax->IsPrivate();
On 2013/08/03 01:15:54, gauravsh wrote:
> What are lines l156 - l175 doing? (add a comment too please)
Comment Added.

https://chromiumcodereview.appspot.com/21046008/diff/42001/chromeos/network/f...
File chromeos/network/favorite_state.cc (right):

https://chromiumcodereview.appspot.com/21046008/diff/42001/chromeos/network/f...
chromeos/network/favorite_state.cc:25: const base::Value& value) {
On 2013/08/02 23:40:22, gauravsh wrote:
> NIT: fix indent here

Done.

https://chromiumcodereview.appspot.com/21046008/diff/42001/chromeos/network/n...
File chromeos/network/network_configuration_handler.cc (right):

https://chromiumcodereview.appspot.com/21046008/diff/42001/chromeos/network/n...
chromeos/network/network_configuration_handler.cc:52: return key ==
flimflam::kEapPrivateKeyPasswordProperty ||
On 2013/08/02 23:40:22, gauravsh wrote:
> Looking at service_constants.h, should the following also be filtered out?
> - EapClientPrivateKey
> - EapPinProperty
> - ApnPasswordProperty
Added.

https://chromiumcodereview.appspot.com/21046008/diff/42001/chromeos/network/n...
File chromeos/network/network_connection_handler.cc (right):

https://chromiumcodereview.appspot.com/21046008/diff/42001/chromeos/network/n...
chromeos/network/network_connection_handler.cc:388:
provider_properties->GetStringWithoutPathExpansion(
On 2013/08/02 23:40:22, gauravsh wrote:
> Ok if there's no vpn_provider_host?
Probably not. Fixed.

https://chromiumcodereview.appspot.com/21046008/diff/42001/chromeos/network/n...
chromeos/network/network_connection_handler.cc:435: if (type ==
flimflam::kTypeVPN) {
Yeah, getting this all right was a bit tricky, bit I should be able to do this
by setting strings first then checking if they are empty.

https://chromiumcodereview.appspot.com/21046008/diff/42001/chromeos/network/n...
File chromeos/network/network_connection_handler.h (right):

https://chromiumcodereview.appspot.com/21046008/diff/42001/chromeos/network/n...
chromeos/network/network_connection_handler.h:187: // Handle success or failure
from Shill.Service.ActivateCellularModem.
On 2013/08/02 23:40:22, gauravsh wrote:
> NIT: s/or failure//

Done.

[pneubeck (no reviews), 2013-08-06 15:45:19]

I got approximately to the middle of wifi_config_view.cc... in one day.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
File ash/system/chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:39: bool
IsDirectActivatedCarrier(const std::string& carrier) {
nit: maybe it's worth explaining what "DirectActivated" means.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:57:
ash::Shell::GetInstance()->system_tray_delegate()->EnrollOrConfigureNetwork(
make this conditional on owning_window!=NULL and log an error otherwise.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:61: if (error_name ==
NetworkConnectionHandler::kErrorActivationRequired ||
kErrorActivationRequired should lead to
network_connect::ActivateCellular(service_path);
and replace lines 104-109.

(lines 104-109 duplicate NetworkRequiresActivation in
network_connection_handler.cc)

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:68: if (error_name ==
NetworkConnectionHandler::kErrorConnected ||
merge all IFs with the same statement.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:98: const NetworkState* network =
NetworkHandler::Get()->network_state_handler()->
if you did the change proposed above, this could be replaced by handling
NetworkConnectionHandler::kErrorNotFound in OnConnectFailed.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:154: if (iter.value().GetType()
== base::Value::TYPE_STRING) {
how about:

std::string value_str;
if (iter.value().GetAsString(&value_str) && value_str.empty())
  properties_to_clear->push_back(iter.key());

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:175:
CallConnectToNetwork(service_path, check_error_state, NULL);
either document that parent=NULL is a valid argument at the definition of
CallConnectToNetwork/OnConnectFailed and handle it in OnConnectFailed.
or provide a valid parent window here.

Similarly it should be mentioned if parent=NULL is a valid arg to
ConnectToNetwork.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:177:
NetworkHandler::Get()->network_configuration_handler()->ClearProperties(
NetworkConfigurationHandler::ClearProperties could early return if the list is
empty.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:216: if (!cellular) {
what if cellular->path() != service_path ?
why not instead GetNetworkState(service_path) and check that the type is
kTypeCellular.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:242:
NetworkHandler::Get()->network_configuration_handler()->SetProperties(
NetworkConfigurationHandler::SetProperties could early return if the dict is
empty.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:253: bool shared) {
|shared| is not used.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:255: const NetworkProfile*
profile =
enclose this in a 'if (!shared)'

log error if GetDefaultUserProfile() returns NULL.
but ensure that shared profile is not used if shared=false.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:260: flimflam::kProfileProperty,
profile->path);
set a 'std::string profile_path' in the if clauses and call
 properties->SetStringWithoutPathExpansion(
   flimflam::kProfileProperty, profile_path);
afterwards.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
File ash/system/chromeos/network/network_connect.h (right):

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.h:20: 
nit: maybe add a comment that, here, 'properties' means Shill properties.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.h:22: // |owning_window| is used to
parent any UI on failure (e.g. for enrollment).
nite: not sure if 'enrollment' is self explanatory. maybe 'certificate
enrollment'?

https://codereview.chromium.org/21046008/diff/55001/ash/system/tray/system_tr...
File ash/system/tray/system_tray_delegate.h (right):

https://codereview.chromium.org/21046008/diff/55001/ash/system/tray/system_tr...
ash/system/tray/system_tray_delegate.h:254: // Shows UI to enroll the network
specified by |network_id| if appropriate,
here, at ConfigureNetwork and ShowMobileSetup: what is network_id?

https://codereview.chromium.org/21046008/diff/55001/ash/system/tray/system_tr...
ash/system/tray/system_tray_delegate.h:255: // otherwise behaves the same as
ConfigureNetwork.
what about parent_window == NULL ?

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/enrollment_dialog_view.cc (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/e...
chrome/browser/chromeos/enrollment_dialog_view.cc:240: // for the most part they
won't know what it means, since it's probably coming
TODO comment obsolete now with the return value?

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/enrollment_dialog_view.h (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/e...
chrome/browser/chromeos/enrollment_dialog_view.h:18: // A virtual class that can
be used to handle certificate enrollment URIs when
'virtual class' -> Interface.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/m...
File chrome/browser/chromeos/mobile/mobile_activator.cc (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:519:
ash::network_connect::ConnectToNetwork(network->service_path(), NULL);
nit: NULL /* no parent window */

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/network_config_view.cc (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_config_view.cc:90: DCHECK(network);
if you DCHECK anyways, why not pass a NetworkState* to this function instead of
a service_path?
If you do that, NetworkConfigView::Show can avoid showing the dialog and
log/show an error instead.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/network_config_view.h (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_config_view.h:48: static void
ShowForType(const std::string& type, gfx::NativeWindow parent);
mention what format |type| is of. referring to NetworkState::type() should be
enough.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_config_view.h:115: const std::string&
service_path);
mention that 'service_path' can be NULL, which means that a new network will be
configured.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/network_connect.cc (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_connect.cc:53: if (cellular &&
again what about cellular->path() != service_path ?

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_connect.cc:104:
NET_LOG_ERROR("Configuring Unknown network", service_path);
nit: Unknown -> unknown

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/network_connect.h (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_connect.h:15: class DictionaryValue;
unused.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_connect.h:34: // * Configure and connect
to the network with a matching cert but without
you changed the behavior of this function:
it doesn't try to connect or trigger the enrollment dialog.

it seems to be intentional. please update the comment and rename the function.
e.g. ShowNetworkConfiguration and describe the difference to settings.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/passphrase_textfield.cc (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/passphrase_textfield.cc:13: show_fake_(false),
I'm not happy about this change.
If this should use a default value at all, then it should be the secure one
namely show_fake_=true.

Also would it require a comment at the constructor, that it starts with this
default value.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/vpn_config_view.cc (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:44: enum
ProviderTypeComboboxIndex {
You are using this even if the combobox doesn't exist.
In that case it's not really an index but only a type. That's confusing.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:281: int provider_type_index
= GetProviderTypeIndex();
move it to it's usage.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:450: if
(!provider_type_.empty())
IMO, it would be less confusing if you would use service_path_.empty() instead.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:475: DCHECK(vpn->type() ==
flimflam::kTypeVPN);
DCHECK(vpn)

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:507:
base::Bind(&VPNConfigView::InitFromProperties, AsWeakPtr()),
The dialog will be visible before the callback is called.
How will the dialog look like in between? Is it flickering?
And if it ever fails the dialog ends up in a weird state.

Alternatively, fetching the properties before showing the dialog might be less
distracting. In that case, you could also de-duplicate this code:
InitFromProperties could be a virtual method in ChildNetworkConfigView.
and GetProperties could be called by Network

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:629:
user_passphrase_textfield_ = new PassphraseTextfield();
as feared, SetShowFake is never called the password would be always visible.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:680: Refresh();
Shouldn't this be called from InitFromProperties?

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:700:
service_properties.GetStringWithoutPathExpansion(
provider_properties->

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:704:
service_properties.GetStringWithoutPathExpansion(
provider_properties->

most vpn properties (both l2tp and openvpn) are nested in the provider
properties when reading from Shill.

i didn't mark them below.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:707:
shill::kL2tpIpsecCaCertPemProperty, &ca_cert_pem_);
CaCertPemProperty is a list, not a string.
You can only show the first entry of that list.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:718:
shill::kOpenVPNCaCertPemProperty, &ca_cert_pem_);
CaCertPemProperty is a list, not a string.
You can only show the first entry of that list.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:750: std::string
type_dict_name = ProviderTypeToONCDictKey(provider_type_);
nit: indentation

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:750: std::string
type_dict_name = ProviderTypeToONCDictKey(provider_type_);
that's again confusing: there is a provider_type_ and provider_type_index
can they ever describe different provider types?

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:786: std::string group_name =
GetGroupName();
nit:
this is not a credential.
similarly CACerts are not credentials.
better call the function SetModifiableProperties

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/vpn_config_view.h (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.h:47: public
base::SupportsWeakPtr<VPNConfigView> {
please use the WeakPtrFactory, since it's not used from the outside.

similarly for the other *ConfigViews.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.h:77: // Initializes data
members and create UI controls.
separate the comment.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.h:79: void InitControls();
comment that this sets up all controls but without any values.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.h:142: std::string
provider_type_;
comment that it's only set if modifying an existing network.
(maybe move to "Cached VPN properties" too).

also, looking at the implementation, it would be much easier to understand, if
you would immediately translate the provider type into ProviderTypeComboboxIndex
in InitFromProperties and store that instead of the original Shill string.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.h:185: // Cached VPN properties
mention, that it's only set if modifying an existing network.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/wifi_config_view.cc (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:690: bool share_network =
GetShareNetwork(wifi_shared);
The 'shared' setting cannot and shouldn't be changed in this dialog.
In the past it wasn't possible either.
See the condition
  if (!share_network_checkbox_)
in GetShareNetwork.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:691: if (share_network !=
wifi_shared) {
Very strong "No!".

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:1114: if (!wifi ||
!wifi->IsPrivate()) {
this is not the same as before.

!IsPrivate can also mean that it's a configured&shared network.
You have to use profile_path.empty().

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:1121: if (wifi)
why this condition?

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:1208:
shill::kEapCaCertPemProperty, &eap_ca_cert_pem);
CaCertPem is a list not a string.
you have to read the first entry of that list.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/wifi_config_view.h (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.h:94: void Init(const
std::string& service_path, bool show_8021x);
service_path arg not required, use ChildNetworkConfigView::service_path_.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.h:98: const std::string&
service_path,
service_path arg not required, use ChildNetworkConfigView::service_path_.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/wimax_config_view.cc (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wimax_config_view.cc:325: if
(!wimax->IsPrivate()) {
is comment for WifiConfigView

must be profile_path.empty()

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wimax_config_view.cc:331:
share_network_checkbox_->SetChecked(true);
why?

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/wimax_config_view.h (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wimax_config_view.h:58: // Initializes UI.
nit: indentation

[stevenjb, 2013-08-06 20:23:55]

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
File ash/system/chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:39: bool
IsDirectActivatedCarrier(const std::string& carrier) {
On 2013/08/06 15:45:19, pneubeck wrote:
> nit: maybe it's worth explaining what "DirectActivated" means.

Done.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:57:
ash::Shell::GetInstance()->system_tray_delegate()->EnrollOrConfigureNetwork(
On 2013/08/06 15:45:19, pneubeck wrote:
> make this conditional on owning_window!=NULL and log an error otherwise.

It's not conditional on owning_window != NULL. The delegate should handle that
case by attaching the window to the default browser.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:61: if (error_name ==
NetworkConnectionHandler::kErrorActivationRequired ||
On 2013/08/06 15:45:19, pneubeck wrote:
> kErrorActivationRequired should lead to
> network_connect::ActivateCellular(service_path);
> and replace lines 104-109.
> 
> (lines 104-109 duplicate NetworkRequiresActivation in
> network_connection_handler.cc)

Yes... that looks sound. Done.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:68: if (error_name ==
NetworkConnectionHandler::kErrorConnected ||
On 2013/08/06 15:45:19, pneubeck wrote:
> merge all IFs with the same statement.

I merged PassphraseRequired with Config/AuthReq and removed the TODO - the jury
is still out as to whether or not we will ever inline simple passphrase or other
configuration.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:98: const NetworkState* network =
NetworkHandler::Get()->network_state_handler()->
On 2013/08/06 15:45:19, pneubeck wrote:
> if you did the change proposed above, this could be replaced by handling
> NetworkConnectionHandler::kErrorNotFound in OnConnectFailed.

Actually, removing this will trigger a similar log entry and show a notification
which is really what we want, so removed.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:154: if (iter.value().GetType()
== base::Value::TYPE_STRING) {
On 2013/08/06 15:45:19, pneubeck wrote:
> how about:
> 
> std::string value_str;
> if (iter.value().GetAsString(&value_str) && value_str.empty())
>   properties_to_clear->push_back(iter.key());

Done.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:175:
CallConnectToNetwork(service_path, check_error_state, NULL);
On 2013/08/06 15:45:19, pneubeck wrote:
> either document that parent=NULL is a valid argument at the definition of
> CallConnectToNetwork/OnConnectFailed and handle it in OnConnectFailed.
> or provide a valid parent window here.
> 
> Similarly it should be mentioned if parent=NULL is a valid arg to
> ConnectToNetwork.

Done.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:177:
NetworkHandler::Get()->network_configuration_handler()->ClearProperties(
On 2013/08/06 15:45:19, pneubeck wrote:
> NetworkConfigurationHandler::ClearProperties could early return if the list is
> empty.

Done.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:216: if (!cellular) {
On 2013/08/06 15:45:19, pneubeck wrote:
> what if cellular->path() != service_path ?
> why not instead GetNetworkState(service_path) and check that the type is
> kTypeCellular.
Yeah, this was converted a little too literally. Done.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:242:
NetworkHandler::Get()->network_configuration_handler()->SetProperties(
On 2013/08/06 15:45:19, pneubeck wrote:
> NetworkConfigurationHandler::SetProperties could early return if the dict is
> empty.

Done.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:253: bool shared) {
On 2013/08/06 15:45:19, pneubeck wrote:
> |shared| is not used.
Oops. Fixed this on the calling side, removed 'shared'.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:255: const NetworkProfile*
profile =
On 2013/08/06 15:45:19, pneubeck wrote:
> enclose this in a 'if (!shared)'
> 
> log error if GetDefaultUserProfile() returns NULL.
> but ensure that shared profile is not used if shared=false.

Done.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:260: flimflam::kProfileProperty,
profile->path);
On 2013/08/06 15:45:19, pneubeck wrote:
> set a 'std::string profile_path' in the if clauses and call
>  properties->SetStringWithoutPathExpansion(
>    flimflam::kProfileProperty, profile_path);
> afterwards.

Done.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
File ash/system/chromeos/network/network_connect.h (right):

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.h:20: 
On 2013/08/06 15:45:19, pneubeck wrote:
> nit: maybe add a comment that, here, 'properties' means Shill properties.
Modified comments of specific methods.

https://codereview.chromium.org/21046008/diff/55001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.h:22: // |owning_window| is used to
parent any UI on failure (e.g. for enrollment).
On 2013/08/06 15:45:19, pneubeck wrote:
> nite: not sure if 'enrollment' is self explanatory. maybe 'certificate
> enrollment'?

Done.

https://codereview.chromium.org/21046008/diff/55001/ash/system/tray/system_tr...
File ash/system/tray/system_tray_delegate.h (right):

https://codereview.chromium.org/21046008/diff/55001/ash/system/tray/system_tr...
ash/system/tray/system_tray_delegate.h:254: // Shows UI to enroll the network
specified by |network_id| if appropriate,
On 2013/08/06 15:45:19, pneubeck wrote:
> here, at ConfigureNetwork and ShowMobileSetup: what is network_id?
Talk to sadrul; I didn't want to rename this everywhere in this CL. It's a
service path.

https://codereview.chromium.org/21046008/diff/55001/ash/system/tray/system_tr...
ash/system/tray/system_tray_delegate.h:255: // otherwise behaves the same as
ConfigureNetwork.
On 2013/08/06 15:45:19, pneubeck wrote:
> what about parent_window == NULL ?
Added comment.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/enrollment_dialog_view.cc (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/e...
chrome/browser/chromeos/enrollment_dialog_view.cc:240: // for the most part they
won't know what it means, since it's probably coming
On 2013/08/06 15:45:19, pneubeck wrote:
> TODO comment obsolete now with the return value?

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/enrollment_dialog_view.h (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/e...
chrome/browser/chromeos/enrollment_dialog_view.h:18: // A virtual class that can
be used to handle certificate enrollment URIs when
On 2013/08/06 15:45:19, pneubeck wrote:
> 'virtual class' -> Interface.

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/m...
File chrome/browser/chromeos/mobile/mobile_activator.cc (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:519:
ash::network_connect::ConnectToNetwork(network->service_path(), NULL);
On 2013/08/06 15:45:19, pneubeck wrote:
> nit: NULL /* no parent window */

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/network_config_view.cc (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_config_view.cc:90: DCHECK(network);
On 2013/08/06 15:45:19, pneubeck wrote:
> if you DCHECK anyways, why not pass a NetworkState* to this function instead
of
> a service_path?
> If you do that, NetworkConfigView::Show can avoid showing the dialog and
> log/show an error instead.

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/network_config_view.h (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_config_view.h:48: static void
ShowForType(const std::string& type, gfx::NativeWindow parent);
On 2013/08/06 15:45:19, pneubeck wrote:
> mention what format |type| is of. referring to NetworkState::type() should be
> enough.

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_config_view.h:115: const std::string&
service_path);
On 2013/08/06 15:45:19, pneubeck wrote:
> mention that 'service_path' can be NULL, which means that a new network will
be
> configured.

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/network_connect.cc (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_connect.cc:53: if (cellular &&
On 2013/08/06 15:45:19, pneubeck wrote:
> again what about cellular->path() != service_path ?

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_connect.cc:104:
NET_LOG_ERROR("Configuring Unknown network", service_path);
On 2013/08/06 15:45:19, pneubeck wrote:
> nit: Unknown -> unknown

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/network_connect.h (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_connect.h:15: class DictionaryValue;
On 2013/08/06 15:45:19, pneubeck wrote:
> unused.
Used by FindPolicyForActiveUser

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_connect.h:34: // * Configure and connect
to the network with a matching cert but without
On 2013/08/06 15:45:19, pneubeck wrote:
> you changed the behavior of this function:
> it doesn't try to connect or trigger the enrollment dialog.
> 
> it seems to be intentional. please update the comment and rename the function.
> e.g. ShowNetworkConfiguration and describe the difference to settings.

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/passphrase_textfield.cc (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/passphrase_textfield.cc:13: show_fake_(false),
On 2013/08/06 15:45:19, pneubeck wrote:
> I'm not happy about this change.
> If this should use a default value at all, then it should be the secure one
> namely show_fake_=true.
> 
> Also would it require a comment at the constructor, that it starts with this
> default value.
We should really deprecate this entirely (it's only used by VpnConfigView, we
should really just use a Textfield with STYLE_OBSCURED), but I didn't want to do
that here. I can push a follow-up though. show_fake doesn't obscure anything, it
just shows ***** instead of blank.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/vpn_config_view.cc (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:44: enum
ProviderTypeComboboxIndex {
On 2013/08/06 15:45:19, pneubeck wrote:
> You are using this even if the combobox doesn't exist.
> In that case it's not really an index but only a type. That's confusing.

Removed 'Combobox'

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:281: int provider_type_index
= GetProviderTypeIndex();
On 2013/08/06 15:45:19, pneubeck wrote:
> move it to it's usage.

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:450: if
(!provider_type_.empty())
On 2013/08/06 15:45:19, pneubeck wrote:
> IMO, it would be less confusing if you would use service_path_.empty()
instead.
Refactored

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:475: DCHECK(vpn->type() ==
flimflam::kTypeVPN);
On 2013/08/06 15:45:19, pneubeck wrote:
> DCHECK(vpn)

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:507:
base::Bind(&VPNConfigView::InitFromProperties, AsWeakPtr()),
On 2013/08/06 15:45:19, pneubeck wrote:
> The dialog will be visible before the callback is called.
> How will the dialog look like in between? Is it flickering?
> And if it ever fails the dialog ends up in a weird state.
> 
> Alternatively, fetching the properties before showing the dialog might be less
> distracting. In that case, you could also de-duplicate this code:
> InitFromProperties could be a virtual method in ChildNetworkConfigView.
> and GetProperties could be called by Network

Views Layout is... challenging. This creates all of the controls first, only
filling in properties when received. I combined the Init methods to make that
more clear (the separation was essentially an intermediate step).

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:629:
user_passphrase_textfield_ = new PassphraseTextfield();
On 2013/08/06 15:45:19, pneubeck wrote:
> as feared, SetShowFake is never called the password would be always visible.
We never set the value of this to the passphrase. "Fake" isn't obscuring
anything, it's supposed to be an indicator that Shill knows the passphrase
so the user doesn't need to provide it. We don't know that until
InitFromProperties. If that never gets called somehow, this would just be blank.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:680: Refresh();
On 2013/08/06 15:45:19, pneubeck wrote:
> Shouldn't this be called from InitFromProperties?

It should really be called by both, done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:700:
service_properties.GetStringWithoutPathExpansion(
On 2013/08/06 15:45:19, pneubeck wrote:
> provider_properties->
Bah, good catch, Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:704:
service_properties.GetStringWithoutPathExpansion(
On 2013/08/06 15:45:19, pneubeck wrote:
> provider_properties->
> 
> most vpn properties (both l2tp and openvpn) are nested in the provider
> properties when reading from Shill.
> 
> i didn't mark them below.

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:707:
shill::kL2tpIpsecCaCertPemProperty, &ca_cert_pem_);
On 2013/08/06 15:45:19, pneubeck wrote:
> CaCertPemProperty is a list, not a string.
> You can only show the first entry of that list.
Gah. Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:718:
shill::kOpenVPNCaCertPemProperty, &ca_cert_pem_);
On 2013/08/06 15:45:19, pneubeck wrote:
> CaCertPemProperty is a list, not a string.
> You can only show the first entry of that list.

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:750: std::string
type_dict_name = ProviderTypeToONCDictKey(provider_type_);
On 2013/08/06 15:45:19, pneubeck wrote:
> nit: indentation

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:750: std::string
type_dict_name = ProviderTypeToONCDictKey(provider_type_);
On 2013/08/06 15:45:19, pneubeck wrote:
> that's again confusing: there is a provider_type_ and provider_type_index
> can they ever describe different provider types?

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:786: std::string group_name =
GetGroupName();
On 2013/08/06 15:45:19, pneubeck wrote:
> nit:
> this is not a credential.
> similarly CACerts are not credentials.
> better call the function SetModifiableProperties
Named SetConfigProperties

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/vpn_config_view.h (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.h:47: public
base::SupportsWeakPtr<VPNConfigView> {
On 2013/08/06 15:45:19, pneubeck wrote:
> please use the WeakPtrFactory, since it's not used from the outside.
> 
> similarly for the other *ConfigViews.
Meh. There are no member dependencies and it's more typing, but fine.
Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.h:77: // Initializes data
members and create UI controls.
On 2013/08/06 15:45:19, pneubeck wrote:
> separate the comment.
Combined the functions, the separation ended up being kind of arbitrary.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.h:79: void InitControls();
On 2013/08/06 15:45:19, pneubeck wrote:
> comment that this sets up all controls but without any values.
Ditto

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.h:142: std::string
provider_type_;
On 2013/08/06 15:45:19, pneubeck wrote:
> comment that it's only set if modifying an existing network.
> (maybe move to "Cached VPN properties" too).
> 
> also, looking at the implementation, it would be much easier to understand, if
> you would immediately translate the provider type into
ProviderTypeComboboxIndex
> in InitFromProperties and store that instead of the original Shill string.

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.h:185: // Cached VPN properties
On 2013/08/06 15:45:19, pneubeck wrote:
> mention, that it's only set if modifying an existing network.

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/wifi_config_view.cc (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:690: bool share_network =
GetShareNetwork(wifi_shared);
On 2013/08/06 15:45:19, pneubeck wrote:
> The 'shared' setting cannot and shouldn't be changed in this dialog.
> In the past it wasn't possible either.
> See the condition
>   if (!share_network_checkbox_)
> in GetShareNetwork.

The logic for this has gotten twisted over time. At one point is was desirable
and useful to use this UI to change the sharing of a network. However, at this
point I am fine with just deprecating that behavior, at least until we re-factor
this UI.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:691: if (share_network !=
wifi_shared) {
On 2013/08/06 15:45:19, pneubeck wrote:
> Very strong "No!".
oO

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:1114: if (!wifi ||
!wifi->IsPrivate()) {
On 2013/08/06 15:45:19, pneubeck wrote:
> this is not the same as before.
> 
> !IsPrivate can also mean that it's a configured&shared network.
> You have to use profile_path.empty().

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:1121: if (wifi)
On 2013/08/06 15:45:19, pneubeck wrote:
> why this condition?
Removed. This gets set appropriately in RefreshShareCheckbox.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:1208:
shill::kEapCaCertPemProperty, &eap_ca_cert_pem);
On 2013/08/06 15:45:19, pneubeck wrote:
> CaCertPem is a list not a string.
> you have to read the first entry of that list.

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/wifi_config_view.h (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.h:94: void Init(const
std::string& service_path, bool show_8021x);
On 2013/08/06 15:45:19, pneubeck wrote:
> service_path arg not required, use ChildNetworkConfigView::service_path_.

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.h:98: const std::string&
service_path,
On 2013/08/06 15:45:19, pneubeck wrote:
> service_path arg not required, use ChildNetworkConfigView::service_path_.

Here, service_path is part of the callback signature

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/wimax_config_view.cc (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wimax_config_view.cc:325: if
(!wimax->IsPrivate()) {
On 2013/08/06 15:45:19, pneubeck wrote:
> is comment for WifiConfigView
> 
> must be profile_path.empty()

Done.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wimax_config_view.cc:331:
share_network_checkbox_->SetChecked(true);
On 2013/08/06 15:45:19, pneubeck wrote:
> why?
Updated.

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/wimax_config_view.h (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wimax_config_view.h:58: // Initializes UI.
On 2013/08/06 15:45:19, pneubeck wrote:
> nit: indentation

Done.

[pneubeck (no reviews), 2013-08-07 16:20:59]

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/wifi_config_view.cc (right):

https://codereview.chromium.org/21046008/diff/55001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:691: if (share_network !=
wifi_shared) {
On 2013/08/06 20:23:55, stevenjb (chromium) wrote:
> On 2013/08/06 15:45:19, pneubeck wrote:
> > Very strong "No!".
> oO
This was related to changing the shared state of previously configured networks.
This isn't the case anymore in the last patch, so it's ok.

https://codereview.chromium.org/21046008/diff/75001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/wifi_config_view.cc (right):

https://codereview.chromium.org/21046008/diff/75001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:710:
ash::network_connect::ConfigureNetworkAndConnect(service_path_, properties);
This is calling Service::SetProperties, which ignores profile path, according to
the documentation src/platform/shill/doc/service-api.txt

Actually, I'm not sure what SetProperties/SetProperty does for an
unconfigured-network.

https://codereview.chromium.org/21046008/diff/75001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:1165:
identity_textfield_->SetText(UTF8ToUTF16(identity));
set below again from kEapIdentityProperty

https://codereview.chromium.org/21046008/diff/75001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:1179: RefreshEapFields();
RefreshEapFields calls phase_2_auth_combobox_->SetSelectedIndex(0);
maybe Refresh* should be called before settings values.

[stevenjb, 2013-08-07 20:22:04]

OK, this cleans up the profile setting. I tested it and confirmed that it works
as before:
* We don't show the 'Share' checkbox for networks with a profile set (this is an
edge case anyway, we only show the dialog for existing networks when credentials
change, or for a VPN requiring username/passphrase/OTP, but those can't be
shared anyway)
* We do show the 'Share' checkbox for visible networks without a profile set if
logged in, and set the Profile as indicated using
NetworkConfigurationHandler::SetNetworkProfile (new)
* We do show the 'Share' checkbox for hidden networks ('Join other') and set the
Profile property in the properties passed to
NetworkConfigurationHandler::CreateConfiguration (which already handles the
property correctly)

https://codereview.chromium.org/21046008/diff/75001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/wifi_config_view.cc (right):

https://codereview.chromium.org/21046008/diff/75001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:710:
ash::network_connect::ConfigureNetworkAndConnect(service_path_, properties);
On 2013/08/07 16:20:59, pneubeck wrote:
> This is calling Service::SetProperties, which ignores profile path, according
to
> the documentation src/platform/shill/doc/service-api.txt
> 
> Actually, I'm not sure what SetProperties/SetProperty does for an
> unconfigured-network.

Hmm, interesting question. The existing code is using 'SetProperty', I had
assumed that 'SetProperties' would behave similarly. Looking at Shill it's not
clear whether or not 'SetProperty' does the right thing either. I'll talk to
Paul about that.

https://codereview.chromium.org/21046008/diff/75001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:1165:
identity_textfield_->SetText(UTF8ToUTF16(identity));
On 2013/08/07 16:20:59, pneubeck wrote:
> set below again from kEapIdentityProperty
Huh, look at that. We were doing the same before. Looks like 'Identity' isn't
actually used. Removed.

https://codereview.chromium.org/21046008/diff/75001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:1179: RefreshEapFields();
On 2013/08/07 16:20:59, pneubeck wrote:
> RefreshEapFields calls phase_2_auth_combobox_->SetSelectedIndex(0);
> maybe Refresh* should be called before settings values.

RefreshEapFields() is called whenever eap_method_combobox_ changes because
values checked in that function (e.g. PassphraseActive()) are dependent on its
setting.

When the setting changes we call phase_2_auth_combobox_->SetSelectedIndex(0) to
reset the index, which is why (I think) this gets called here, before we set the
initial value.

It's not clear to me that this logic is great, but I believe it is consistent
with what we currently have.

[gauravsh, 2013-08-07 23:33:37]

Review Feedback Part 1/2 (for chromeos/network/*)

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

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:679: void
NetworkConnectionHandler::CallShillActivate(
Talking to Arman, it looks like this would be a better fit for a separate
NetworkActivationHandler which lets us make activation related shill calls and
manage activation state. (The WIP MobileActivator change I have needs this and
CompleteCellularActivation as well).

Since connection and activation are very different things, I am inclined to
agree with that as well.

[stevenjb, 2013-08-08 01:02:51]

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

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:679: void
NetworkConnectionHandler::CallShillActivate(
On 2013/08/07 23:33:37, gauravsh wrote:
> Talking to Arman, it looks like this would be a better fit for a separate
> NetworkActivationHandler which lets us make activation related shill calls and
> manage activation state. (The WIP MobileActivator change I have needs this and
> CompleteCellularActivation as well).
> 
> Since connection and activation are very different things, I am inclined to
> agree with that as well.
> 

That sounds reasonable to me, although I am hesitant to add that to this CL
also. Would you be OK with adding a TODO to move this later?

[gauravsh, 2013-08-08 01:27:23]

LGTM

Given our overhaul plans for a lot of the settings code, I concentrated on the
translation from NetworkLibrary -> Network*Handlers as opposed to
fixing/refactoring the existing structure. Nothing obviously wrong jumped out to
me. 

In any case, please do wait for comments on the last PS from Phillip as well. 

On 2013/08/08 01:02:51, stevenjb (chromium) wrote:
>
https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
> File chromeos/network/network_connection_handler.cc (right):
> 
>
https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
> chromeos/network/network_connection_handler.cc:679: void
> NetworkConnectionHandler::CallShillActivate(
> On 2013/08/07 23:33:37, gauravsh wrote:
> > Talking to Arman, it looks like this would be a better fit for a separate
> > NetworkActivationHandler which lets us make activation related shill calls
and
> > manage activation state. (The WIP MobileActivator change I have needs this
and
> > CompleteCellularActivation as well).
> > 
> > Since connection and activation are very different things, I am inclined to
> > agree with that as well.
> > 
> 
> That sounds reasonable to me, although I am hesitant to add that to this CL
> also. Would you be OK with adding a TODO to move this later?

That's fine. I am already doing this as part of my MobileActivator change (which
wouldn't be landing before the M30 branch at this point).

[pneubeck (no reviews), 2013-08-08 11:10:00]

lgtm modulo the remaining comments.
up to you to wait for one more review of the next patchset.

https://codereview.chromium.org/21046008/diff/98001/ash/system/chromeos/netwo...
File ash/system/chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/21046008/diff/98001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:181: if
(!chromeos::LoginState::Get()->IsUserAuthenticated())
I'm surprised by this special return value "".
Why would we ever want that Shill implicitly selects a profile.
Such special cases lead to a more complicated Shill API.

IMO, you should instead force shared=true in this case and return
kSharedProfilePath.
Or drop this condition completely: It's a bug if shared=true and there's no user
logged in (-> LOG an error).

https://codereview.chromium.org/21046008/diff/98001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:252:
base::Bind(&SetPropertiesFailed,
If this fails, you might end up silently writing the config to the wrong
profile, because SetProperties below is called unconditionally.

https://codereview.chromium.org/21046008/diff/98001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/vpn_config_view.cc (right):

https://codereview.chromium.org/21046008/diff/98001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:372: &properties, false /*
shared */);
misleading, compare with line 385.

shared -> not shared

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
chromeos/network/network_configuration_handler.cc:223: for
(base::DictionaryValue::Iterator iter(properties);
uff.. isn't this a bit too much logging?

Shill can already log every dbus detail.

e.g. you could at least drop the values or, which I would prefer, make this
logging dependent on the debugging flag (either defined(NDEBUG) or ENABLE_DLOG.

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
chromeos/network/network_configuration_handler.cc:226: if
(!IsPassphrase(iter.key()))
I don't like this blacklisting at all.
Yet another place that's very likely to become outdated and introduce a security
hole.

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
chromeos/network/network_configuration_handler.cc:228:
NET_LOG_DEBUG("SetProperty",  service_path + "." + iter.key() + "=" + v);
you might even consider making NET_LOG_DEBUG (not here, but in
network_event_log.h) a no op if !ENABLE_DLOG

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
chromeos/network/network_configuration_handler.cc:273: for
(base::DictionaryValue::Iterator iter(properties);
if _really_ necessary (as said above, I think it's to much and risky logging),
share the code with SetProperties

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
File chromeos/network/network_configuration_handler.h (right):

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
chromeos/network/network_configuration_handler.h:108: 
nit: remove one empty line

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
File chromeos/network/network_profile_handler.h (right):

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
chromeos/network/network_profile_handler.h:58: const NetworkProfile*
GetDefaultUserProfile() const;
Instead of deciding what the "DefaultUserProfile" is in chromeos/network,
you could instead solve this as I did with
network_connect::FindPolicyForActiveUser.

Of course, for M30 it shouldn't make a difference.

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

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
chromeos/network/network_state.h:105: // Sets |out| to the UIData sepcified by
|value|. Returns true successfully
nit: sepcified -> specified
... true _if_ ...

[stevenjb, 2013-08-08 19:00:56]

https://codereview.chromium.org/21046008/diff/98001/ash/system/chromeos/netwo...
File ash/system/chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/21046008/diff/98001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:181: if
(!chromeos::LoginState::Get()->IsUserAuthenticated())
On 2013/08/08 11:10:00, pneubeck wrote:
> I'm surprised by this special return value "".
> Why would we ever want that Shill implicitly selects a profile.
> Such special cases lead to a more complicated Shill API.
> 
> IMO, you should instead force shared=true in this case and return
> kSharedProfilePath.
> Or drop this condition completely: It's a bug if shared=true and there's no
user
> logged in (-> LOG an error).

Fair enough. Done.

https://codereview.chromium.org/21046008/diff/98001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:252:
base::Bind(&SetPropertiesFailed,
On 2013/08/08 11:10:00, pneubeck wrote:
> If this fails, you might end up silently writing the config to the wrong
> profile, because SetProperties below is called unconditionally.

If it fails... there are probably bigger problems... but yeah, the delay isn't
going to be noticeable, I'll add a step to the callback chain.

https://codereview.chromium.org/21046008/diff/98001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/vpn_config_view.cc (right):

https://codereview.chromium.org/21046008/diff/98001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:372: &properties, false /*
shared */);
On 2013/08/08 11:10:00, pneubeck wrote:
> misleading, compare with line 385.
> 
> shared -> not shared

Done.

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
chromeos/network/network_configuration_handler.cc:223: for
(base::DictionaryValue::Iterator iter(properties);
On 2013/08/08 11:10:00, pneubeck wrote:
> uff.. isn't this a bit too much logging?
> 
> Shill can already log every dbus detail.
> 
> e.g. you could at least drop the values or, which I would prefer, make this
> logging dependent on the debugging flag (either defined(NDEBUG) or
ENABLE_DLOG.

Nobody runs Debug builds in practice.

For M30 there are going to be cases we missed where somebody's network
configuration no longer works. This way we will know exactly what properties the
UI thinks it is setting, in one convenient location. It's why I created
NetworkEventLog in the first place.

There is other logging that I have started backing off on, e.g. wifi signal
strength updates, but I'd rather start off logging too much than too little,
especially around network configuration.

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
chromeos/network/network_configuration_handler.cc:226: if
(!IsPassphrase(iter.key()))
On 2013/08/08 11:10:00, pneubeck wrote:
> I don't like this blacklisting at all.
> Yet another place that's very likely to become outdated and introduce a
security
> hole.
I'm not sure I agree. As you mentioned, we already log these in Shill. If we are
really concerned we can remove this logging in 31 or 32, but it was essential
for me to get this all working correctly in the first place, and I don't expect
to have gotten it 100% correct on the first try.

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
chromeos/network/network_configuration_handler.cc:228:
NET_LOG_DEBUG("SetProperty",  service_path + "." + iter.key() + "=" + v);
On 2013/08/08 11:10:00, pneubeck wrote:
> you might even consider making NET_LOG_DEBUG (not here, but in
> network_event_log.h) a no op if !ENABLE_DLOG

Again, I don't think that is useful. The point of this is to debug real world
failures.

I would consider making the logging Dev channel only. I'd prefer to wait at
least until 31 to make that change however.

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
chromeos/network/network_configuration_handler.cc:273: for
(base::DictionaryValue::Iterator iter(properties);
On 2013/08/08 11:10:00, pneubeck wrote:
> if _really_ necessary (as said above, I think it's to much and risky logging),
> share the code with SetProperties

Done.

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
File chromeos/network/network_configuration_handler.h (right):

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
chromeos/network/network_configuration_handler.h:108: 
On 2013/08/08 11:10:00, pneubeck wrote:
> nit: remove one empty line

Done.

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
File chromeos/network/network_profile_handler.h (right):

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
chromeos/network/network_profile_handler.h:58: const NetworkProfile*
GetDefaultUserProfile() const;
On 2013/08/08 11:10:00, pneubeck wrote:
> Instead of deciding what the "DefaultUserProfile" is in chromeos/network,
> you could instead solve this as I did with
> network_connect::FindPolicyForActiveUser.
> 
> Of course, for M30 it shouldn't make a difference.

Neither Ash nor ChromeOS have access to UserManager. I was planning to wait for
any multi-profile re-factoring before figuring out the right way to do this.

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

https://codereview.chromium.org/21046008/diff/98001/chromeos/network/network_...
chromeos/network/network_state.h:105: // Sets |out| to the UIData sepcified by
|value|. Returns true successfully
On 2013/08/08 11:10:00, pneubeck wrote:
> nit: sepcified -> specified
> ... true _if_ ...

Done.

[pneubeck (no reviews), 2013-08-08 19:20:35]

last changes seem to be fine.

https://codereview.chromium.org/21046008/diff/144002/ash/system/chromeos/netw...
File ash/system/chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/21046008/diff/144002/ash/system/chromeos/netw...
ash/system/chromeos/network/network_connect.cc:181: if
(!chromeos::LoginState::Get()->IsUserAuthenticated() && !shared) {
Ok. my concentration seem to falter. Sorry.

As I mentioned somewhere else, implicit switching to the shared profile might be
problematic.
Thus, the right thing might be to return empty string _as an error_ (and log an
error).

Callers of GetNetworkProfilePath would then have to check the return-value for
empty() and if so abort.

... but I think this can be done after this CL.
Sorry again, for this see-saw.

https://codereview.chromium.org/21046008/diff/144002/chromeos/network/network...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/21046008/diff/144002/chromeos/network/network...
chromeos/network/network_state.h:94: // Converts the prefix length to a network
string.
netmask ?

[pneubeck (no reviews), 2013-08-08 19:20:35]

last changes seem to be fine.

https://codereview.chromium.org/21046008/diff/144002/ash/system/chromeos/netw...
File ash/system/chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/21046008/diff/144002/ash/system/chromeos/netw...
ash/system/chromeos/network/network_connect.cc:181: if
(!chromeos::LoginState::Get()->IsUserAuthenticated() && !shared) {
Ok. my concentration seem to falter. Sorry.

As I mentioned somewhere else, implicit switching to the shared profile might be
problematic.
Thus, the right thing might be to return empty string _as an error_ (and log an
error).

Callers of GetNetworkProfilePath would then have to check the return-value for
empty() and if so abort.

... but I think this can be done after this CL.
Sorry again, for this see-saw.

https://codereview.chromium.org/21046008/diff/144002/chromeos/network/network...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/21046008/diff/144002/chromeos/network/network...
chromeos/network/network_state.h:94: // Converts the prefix length to a network
string.
netmask ?

[commit-bot: I haz the power, 2013-08-08 20:22:42]

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

[stevenjb, 2013-08-09 04:10:25]

Committed patchset #13 manually as r216568 (presubmit successful).