Eliminate c/b/chromeos/options/network_connect.cc

Eliminate c/b/chromeos/options/network_connect.cc


This is just some code cleanup designed to faciliate moving the Network
Notifications to the message center.
It also improves the logic for showing a notification after a connect
error by eliminating the NetworkState dependency (by explicitly fetching
the properties before showing the notification).

Having two 'network_connect.cc' files was confusing, also the functions 
remaining don't directly have anything to do with connect, they are 
support functions with Chrome dependencies and belong in 
AshSystemTrayDelegate. Now all of the connect flow is in one place 
(ash::network_connect).

BUG=271546
R=gauravsh@chromium.org, jennyz@chromium.org, pneubeck@chromium.org

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

[stevenjb, 2013-08-19 19:50:57]

ptal

[gauravsh, 2013-08-20 00:59:48]

lgtm

[pneubeck (no reviews), 2013-08-20 08:32:26]

could you explain why you want to eliminate
c/b/chromeos/options/network_connect.cc in the commit message?

I think you do it to make responsibilities more clear -- every call (can this be
made more precise?) goes through ash now.

https://codereview.chromium.org/22796014/diff/4001/ash/ash_chromeos_strings.grdp
File ash/ash_chromeos_strings.grdp (right):

https://codereview.chromium.org/22796014/diff/4001/ash/ash_chromeos_strings.g...
ash/ash_chromeos_strings.grdp:150: <message
name="IDS_NETWORK_ACTIVATION_ERROR_TITLE" desc="Title for network connection
error notification">
the 'desc' is inconsistent with the text content:
'network connection error' versus 'network activation error'

https://codereview.chromium.org/22796014/diff/4001/ash/ash_chromeos_strings.g...
ash/ash_chromeos_strings.grdp:153: <message
name="IDS_NETWORK_ACTIVATION_NEEDS_CONNECTION" desc="Message when attempting to
activate a mobile network that requries a connection">
nit: requries -> requires

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_connect.cc:296: cellular->activation_state()
!= flimflam::kActivationStateActivated &&
the first three conditions

  cellular && cellular->type() == flimflam::kTypeCellular &&
  cellular->activation_state() != flimflam::kActivationStateActivated

could, but don't have to, be moved in front and lead to a LOG_ERROR/return (and
prevent showing the MobileSetupDialog as well).

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_connect.cc:426: if
(!ash::Shell::HasInstance())
should the other functions check this as well?

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_connect.h (right):

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_connect.h:31: // setup and either shows a
notifications or opens the mobile setup dialog.
nit: notifications -> notification

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_state_notifier.cc (right):

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:46: // Note: this will
trigger the correct UI for out of credit notifications.
nit:
if you depend on that behavior, it should be mentioned in the method comment of
SystemTrayDelegate::ConfigureNetwork

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:93: network->path() !=
last_active_network_ ||
if you return in this case (network->path() != last_active_network_)
Why then even listen for/implement NetworkPropertiesUpdated? Isn't
DefaultNetworkChanged then sufficient (which also notifies about state changes)?

If there's a good reason, please document it.

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:175:
NetworkState::GetNameFromProperties(service_path, shill_properties);
GetNameFromProperties should explicitly handle shill_properties.empty() if you
use it that way here.

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:180: string16 error_msg;
network_name will be empty here if shill_properties is empty.

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:188:
IDS_NETWORK_CONNECTION_ERROR_MESSAGE_WITH_DETAILS,
nit:
the names of the IDS_* constants are confusing:
here it's called ..._WITH_DETAILS, but details are only provided in the first
if-case.

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:196:
NetworkObserver::NetworkType type = NetworkObserver::NETWORK_UNKNOWN;
this duplicates the type mapping.
could you instead change GetNetworkTypeForNetworkState from a NetworkState
argument to  (std::string& shill_network_type, std::string&
shill_network_technology) arguments?

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_state_notifier.h (right):

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.h:55: void
ShowNetworkConnectError(const std::string& error_name,
what strings are allowed for |error_name| ?

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.h:57: void
ConnectErrorPropertiesSucceeded(
these three should be private?

https://codereview.chromium.org/22796014/diff/4001/chrome/browser/chromeos/sy...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/22796014/diff/4001/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:271:
ash::network_connect::ActivateCellular(service_path);
It would be awesome if we could reach a state where the control flow doesn't
jump several times between ash/ and browser/. If possible, the delegate
shouldn't depend on ash at all.

only solution could be to expose more specific functions through the delegate
like NetworkConfigView::Show.
Then you could move this function as well as ConfigureNetwork completely to
ash::network_connect
and change EnrollOrConfigureNetwork to EnrollNetwork only (and implement
EnrollOrConfigureNetwork in ash::network_connect)

[jennyz, 2013-08-20 17:06:30]

https://codereview.chromium.org/22796014/diff/4001/chrome/browser/chromeos/sy...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/22796014/diff/4001/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:279:
ShowNetworkSettingsPage(service_path);
Do you need to return after ShowNetworkSettingsPage? Otherwise, it goes to
NOTREACHED().

[stevenjb, 2013-08-20 20:47:43]

ptal

https://codereview.chromium.org/22796014/diff/4001/ash/ash_chromeos_strings.grdp
File ash/ash_chromeos_strings.grdp (right):

https://codereview.chromium.org/22796014/diff/4001/ash/ash_chromeos_strings.g...
ash/ash_chromeos_strings.grdp:150: <message
name="IDS_NETWORK_ACTIVATION_ERROR_TITLE" desc="Title for network connection
error notification">
On 2013/08/20 08:32:26, pneubeck wrote:
> the 'desc' is inconsistent with the text content:
> 'network connection error' versus 'network activation error'

Done.

https://codereview.chromium.org/22796014/diff/4001/ash/ash_chromeos_strings.g...
ash/ash_chromeos_strings.grdp:153: <message
name="IDS_NETWORK_ACTIVATION_NEEDS_CONNECTION" desc="Message when attempting to
activate a mobile network that requries a connection">
On 2013/08/20 08:32:26, pneubeck wrote:
> nit: requries -> requires

Done.

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_connect.cc:296: cellular->activation_state()
!= flimflam::kActivationStateActivated &&
On 2013/08/20 08:32:26, pneubeck wrote:
> the first three conditions
> 
>   cellular && cellular->type() == flimflam::kTypeCellular &&
>   cellular->activation_state() != flimflam::kActivationStateActivated
> 
> could, but don't have to, be moved in front and lead to a LOG_ERROR/return
(and
> prevent showing the MobileSetupDialog as well).

I think we may want show MobileSetupDialog even if ActivationState != Activated,
but I will early exit / error on the first two.

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_connect.cc:426: if
(!ash::Shell::HasInstance())
On 2013/08/20 08:32:26, pneubeck wrote:
> should the other functions check this as well?

Yes, at least anything that gets called by a callback; good catch.

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_connect.h (right):

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_connect.h:31: // setup and either shows a
notifications or opens the mobile setup dialog.
On 2013/08/20 08:32:26, pneubeck wrote:
> nit: notifications -> notification

Done.

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_state_notifier.cc (right):

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:46: // Note: this will
trigger the correct UI for out of credit notifications.
On 2013/08/20 08:32:26, pneubeck wrote:
> nit:
> if you depend on that behavior, it should be mentioned in the method comment
of
> SystemTrayDelegate::ConfigureNetwork
Removed this (unused) and updated SystemTrayDelegate::ConfigureNetwork comment

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:93: network->path() !=
last_active_network_ ||
On 2013/08/20 08:32:26, pneubeck wrote:
> if you return in this case (network->path() != last_active_network_)
> Why then even listen for/implement NetworkPropertiesUpdated? Isn't
> DefaultNetworkChanged then sufficient (which also notifies about state
changes)?
> 
> If there's a good reason, please document it.

I greatly simplify this logic in the next CL, I didn't want to change too much
here, although it looks like I inadvertently changed the existing logic slightly
so I will revert that in this CL.

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:175:
NetworkState::GetNameFromProperties(service_path, shill_properties);
On 2013/08/20 08:32:26, pneubeck wrote:
> GetNameFromProperties should explicitly handle shill_properties.empty() if you
> use it that way here.

Done.

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:180: string16 error_msg;
On 2013/08/20 08:32:26, pneubeck wrote:
> network_name will be empty here if shill_properties is empty.

Yes, that logic hasn't changed; I will address that in the next CL when we
switch to using message_center, otherwise the change may get lost.

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:188:
IDS_NETWORK_CONNECTION_ERROR_MESSAGE_WITH_DETAILS,
On 2013/08/20 08:32:26, pneubeck wrote:
> nit:
> the names of the IDS_* constants are confusing:
> here it's called ..._WITH_DETAILS, but details are only provided in the first
> if-case.
I will also address that in the next CL.

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:196:
NetworkObserver::NetworkType type = NetworkObserver::NETWORK_UNKNOWN;
On 2013/08/20 08:32:26, pneubeck wrote:
> this duplicates the type mapping.
> could you instead change GetNetworkTypeForNetworkState from a NetworkState
> argument to  (std::string& shill_network_type, std::string&
> shill_network_technology) arguments?

This is temporary and going away soon.

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_state_notifier.h (right):

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.h:55: void
ShowNetworkConnectError(const std::string& error_name,
On 2013/08/20 08:32:26, pneubeck wrote:
> what strings are allowed for |error_name| ?
Documented.

https://codereview.chromium.org/22796014/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.h:57: void
ConnectErrorPropertiesSucceeded(
On 2013/08/20 08:32:26, pneubeck wrote:
> these three should be private?
> 

Done.

https://codereview.chromium.org/22796014/diff/4001/chrome/browser/chromeos/sy...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/22796014/diff/4001/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:271:
ash::network_connect::ActivateCellular(service_path);
On 2013/08/20 08:32:26, pneubeck wrote:
> It would be awesome if we could reach a state where the control flow doesn't
> jump several times between ash/ and browser/. If possible, the delegate
> shouldn't depend on ash at all.
> 
> only solution could be to expose more specific functions through the delegate
> like NetworkConfigView::Show.
> Then you could move this function as well as ConfigureNetwork completely to
> ash::network_connect
> and change EnrollOrConfigureNetwork to EnrollNetwork only (and implement
> EnrollOrConfigureNetwork in ash::network_connect)

Eventually this should get replaced with "ShowNetworkSettingsPage". I'm not sure
it's worth adding extra delegate methods in the meanwhile.

https://codereview.chromium.org/22796014/diff/4001/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:279:
ShowNetworkSettingsPage(service_path);
On 2013/08/20 17:06:30, jennyz wrote:
> Do you need to return after ShowNetworkSettingsPage? Otherwise, it goes to
> NOTREACHED().

Good catch. Done.

[jennyz, 2013-08-21 00:18:49]

lgtm

[pneubeck (no reviews), 2013-08-21 07:33:36]

lgtm

[stevenjb, 2013-08-21 16:33:07]

Committed patchset #4 manually as r218752 (presubmit successful).