Switch over MobileActivator to use Network*Handler

Switch over MobileActivator to use Network*Handler

Add a NetworkActivationHandler for handling cellular activation calls to Shill.

BUG=188753
TEST=tested Verizon LTE (on link) and Verizon 3g (on Lumpy) activation

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

[gauravsh, 2013-08-13 23:45:28]

I'll be testing this over the next few days but getting this out now to get the
review started.

[armansito, 2013-08-14 00:49:51]

Just a few nits. Let me know if I can help test this in any way.

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

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:231: network->path() !=
service_path_) {
I guess the original code expected this to never happen. Perhaps add a log
message?

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc (right):

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc:460:
UpdatePortalReachability(network, false /*force notification*/);
The inline comment here makes me think that "false" means "notification should
be forced" while the method signature implies otherwise. Can you change (or
remove) the inline comment?

https://codereview.chromium.org/22611005/diff/12001/chromeos/network/network_...
File chromeos/network/network_connection_handler.cc (left):

https://codereview.chromium.org/22611005/diff/12001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:322: 
A small suggestion: now that there is a NetworkActivationHandler, it might make
sense to move the "NetworkRequiresActivation" function, which is defined in this
file in an anonymous namespace, over there.

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

https://codereview.chromium.org/22611005/diff/12001/chromeos/network/network_...
chromeos/network/network_state.h:120: friend class MobileActivatorTest;
I see the convenience of making this a friend but is this absolutely necessary
and would it be hard to implement this test without it? (The same applies for
NetworkChangeNotifierChromeosUpdateTest below, though it's not related to this
CL).

[stevenjb, 2013-08-14 02:28:30]

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

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:29: #include
"chrome/browser/chromeos/cros/network_library.h"
Remove

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:31: #include
"chromeos/network/cros_network_functions.h"
We shouldn't be using this...?

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:179: return;
Remove

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:198: void
MobileActivator::NetworkManagerChanged() {
We should rename this, something like RefreshCellularNetwork.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:278: const NetworkState*
network = FindMatchingCellularNetwork(false);
We should really just use service_path_ here I think.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:308: const NetworkState*
network = FindMatchingCellularNetwork(true);
Seems like we should also use service_path_ here.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:330: const NetworkState*
network = FindMatchingCellularNetwork(true);
Use service_path_?

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:366: bool reattach_observer)
{
I don't think we need this argument.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:400: if
(nsh->GetTechnologyState(flimflam::kTypeCellular) ==
nit: local for technology_state

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:448: const NetworkState*
network = FindMatchingCellularNetwork(true);
Use service_path_?

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:455: const NetworkState*
network = FindMatchingCellularNetwork(true);
Use service_path_?

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:523:
ChangeState(FindMatchingCellularNetwork(true),
Use service_path_?

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:529: const NetworkState*
network = FindMatchingCellularNetwork(true);
Use service_path_?

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:809: service_path);
Need to confirm network != NULL here or in ChangeState()

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:811: LOG(ERROR) << "Failed to
call Activate() on service in shill.";
NET_LOG_ERROR

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:865: LOG(WARNING) <<
"Activating service " << network->path();
NET_LOG_EVENT

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:869: "",  // carrier.
nit: no '.'

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/mobile_setup_dialog.cc (right):

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/mobile_setup_dialog.cc:184: const std::string&
error_description) {
Since this does nothing is there any point in implementing it, or even observing
MobileActivator?

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc (right):

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc:22: #include
"chrome/browser/chromeos/cros/network_library.h"
Remove

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc:411: const NetworkState*
network = nsh->GetNetworkState(path.substr(1));
the path.substr(1) should probably be assigned to a local and documented.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc:496: return;
Remove

https://codereview.chromium.org/22611005/diff/12001/chromeos/network/network_...
File chromeos/network/network_activation_handler.cc (right):

https://codereview.chromium.org/22611005/diff/12001/chromeos/network/network_...
chromeos/network/network_activation_handler.cc:51: service_path);
We need to pass NetworkStateHandler to NetworkActivationHandler::Init(). See
other examples and note in network_handler.h above network_state_handler().

That said... I'm not sure we should check NetworkState here or below, we don't
need it; the caller needs to handle errors if a network gets dropped anyway. So
we probably don't need or want the NetworkState dependency here. (I realize I
added this check, but in hindsight it really shouldn't be necessary).

https://codereview.chromium.org/22611005/diff/12001/chromeos/network/network_...
File chromeos/network/network_activation_handler.h (right):

https://codereview.chromium.org/22611005/diff/12001/chromeos/network/network_...
chromeos/network/network_activation_handler.h:60: const
network_handler::ErrorCallback& error_callback);
nit: WS

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

https://codereview.chromium.org/22611005/diff/12001/chromeos/network/network_...
chromeos/network/network_state.h:120: friend class MobileActivatorTest;
On 2013/08/14 00:49:51, armansito wrote:
> I see the convenience of making this a friend but is this absolutely necessary
> and would it be hard to implement this test without it? (The same applies for
> NetworkChangeNotifierChromeosUpdateTest below, though it's not related to this
> CL).

I personally prefer using 'friend' for tests and keeping the members/accessors
private, assuming that the test actually needs access to them. (As opposed to
writing special ForTest accessors for trivial getters).

[gauravsh, 2013-08-14 21:42:29]

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

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:29: #include
"chrome/browser/chromeos/cros/network_library.h"
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> Remove

Done.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:31: #include
"chromeos/network/cros_network_functions.h"
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> We shouldn't be using this...?

Yeah, things got whacky when I rebased removed.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:179: return;
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> Remove

Done.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:198: void
MobileActivator::NetworkManagerChanged() {
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> We should rename this, something like RefreshCellularNetwork.

Not sure I understand. This is an observer override for
NetworkStateHanderObserver.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:231: network->path() !=
service_path_) {
On 2013/08/14 00:49:51, armansito wrote:
> I guess the original code expected this to never happen. Perhaps add a log
> message?

Done.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:278: const NetworkState*
network = FindMatchingCellularNetwork(false);
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> We should really just use service_path_ here I think.

Yeah, I don't think we need FindMatchingCellularNetwork. Not sure about why the
|reattach_observer| bit was added to it (it's only set to false in
ContinueActivation).

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:308: const NetworkState*
network = FindMatchingCellularNetwork(true);
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> Seems like we should also use service_path_ here.

Done.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:330: const NetworkState*
network = FindMatchingCellularNetwork(true);
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> Use service_path_?

Done.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:366: bool reattach_observer)
{
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> I don't think we need this argument.

Done.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:400: if
(nsh->GetTechnologyState(flimflam::kTypeCellular) ==
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> nit: local for technology_state

Done.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:448: const NetworkState*
network = FindMatchingCellularNetwork(true);
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> Use service_path_?

Done.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:455: const NetworkState*
network = FindMatchingCellularNetwork(true);
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> Use service_path_?

Done.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:523:
ChangeState(FindMatchingCellularNetwork(true),
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> Use service_path_?

Done.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:529: const NetworkState*
network = FindMatchingCellularNetwork(true);
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> Use service_path_?

Done.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:809: service_path);
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> Need to confirm network != NULL here or in ChangeState()

Done.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:811: LOG(ERROR) << "Failed to
call Activate() on service in shill.";
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> NET_LOG_ERROR

Done.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:865: LOG(WARNING) <<
"Activating service " << network->path();
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> NET_LOG_EVENT

Done.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:869: "",  // carrier.
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> nit: no '.'

Done.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/mobile_setup_dialog.cc (right):

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/mobile_setup_dialog.cc:184: const std::string&
error_description) {
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> Since this does nothing is there any point in implementing it, or even
observing
> MobileActivator?

Yeah, I am not sure why this was observing MobileActivator in the first place.
Removed.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc (right):

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc:22: #include
"chrome/browser/chromeos/cros/network_library.h"
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> Remove

Done.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc:411: const NetworkState*
network = nsh->GetNetworkState(path.substr(1));
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> the path.substr(1) should probably be assigned to a local and documented.

The path is of the form //service/5, so the path.substr() gets rid of the extra
'/'.

I don't really understand how the URL of the WebContents frame gets set to this.
Do any of you know?

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc:460:
UpdatePortalReachability(network, false /*force notification*/);
On 2013/08/14 00:49:51, armansito wrote:
> The inline comment here makes me think that "false" means "notification should
> be forced" while the method signature implies otherwise. Can you change (or
> remove) the inline comment?

Done.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc:496: return;
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> Remove

Done.

https://codereview.chromium.org/22611005/diff/12001/chromeos/network/network_...
File chromeos/network/network_activation_handler.cc (right):

https://codereview.chromium.org/22611005/diff/12001/chromeos/network/network_...
chromeos/network/network_activation_handler.cc:51: service_path);
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> We need to pass NetworkStateHandler to NetworkActivationHandler::Init(). See
> other examples and note in network_handler.h above network_state_handler().
> 
> That said... I'm not sure we should check NetworkState here or below, we don't
> need it; the caller needs to handle errors if a network gets dropped anyway.
So
> we probably don't need or want the NetworkState dependency here. (I realize I
> added this check, but in hindsight it really shouldn't be necessary).

Done.

https://codereview.chromium.org/22611005/diff/12001/chromeos/network/network_...
File chromeos/network/network_activation_handler.h (right):

https://codereview.chromium.org/22611005/diff/12001/chromeos/network/network_...
chromeos/network/network_activation_handler.h:60: const
network_handler::ErrorCallback& error_callback);
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> nit: WS

Done.

https://codereview.chromium.org/22611005/diff/12001/chromeos/network/network_...
File chromeos/network/network_connection_handler.cc (left):

https://codereview.chromium.org/22611005/diff/12001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:322: 
On 2013/08/14 00:49:51, armansito wrote:
> A small suggestion: now that there is a NetworkActivationHandler, it might
make
> sense to move the "NetworkRequiresActivation" function, which is defined in
this
> file in an anonymous namespace, over there.

NetworkRequiresActivation() is used by ConnectToNetwork in
NetworkConnectionHandler. I guess we could make it static in
NetworkActivationHandler. What do you think?

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

https://codereview.chromium.org/22611005/diff/12001/chromeos/network/network_...
chromeos/network/network_state.h:120: friend class MobileActivatorTest;
On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> On 2013/08/14 00:49:51, armansito wrote:
> > I see the convenience of making this a friend but is this absolutely
necessary
> > and would it be hard to implement this test without it? (The same applies
for
> > NetworkChangeNotifierChromeosUpdateTest below, though it's not related to
this
> > CL).
> 
> I personally prefer using 'friend' for tests and keeping the members/accessors
> private, assuming that the test actually needs access to them. (As opposed to
> writing special ForTest accessors for trivial getters).

In this particular case, it's needed to manually set the state and
activation_state of a NetworkState.

Since there's a lot of precedent for the "friend class <TestFixture>" in the
chromium code base, I also find this preferable to adding a Set*ForTest to
NetworkState.

[stevenjb, 2013-08-14 21:58:35]

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

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:198: void
MobileActivator::NetworkManagerChanged() {
On 2013/08/14 21:42:29, gauravsh wrote:
> On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> > We should rename this, something like RefreshCellularNetwork.
> 
> Not sure I understand. This is an observer override for
> NetworkStateHanderObserver.

Oh, right. I don't think we need to observe that here with NetworkStateHandler.
In fact, I think we could probably remove that entirely... it really only
happens if a list changes (we already notify for that) or a technology changes
(which should maybe have a specific notification). I'll make a note to audit
that.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc (right):

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc:411: const NetworkState*
network = nsh->GetNetworkState(path.substr(1));
On 2013/08/14 21:42:29, gauravsh wrote:
> On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> > the path.substr(1) should probably be assigned to a local and documented.
> 
> The path is of the form //service/5, so the path.substr() gets rid of the
extra
> '/'.
> 
> I don't really understand how the URL of the WebContents frame gets set to
this.
> Do any of you know?
I do not. We should at least add some sort of comment, even if it's a 'TODO:
investigate why there is an extra '/' at the beginning of path'

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

https://codereview.chromium.org/22611005/diff/26001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:799: NET_LOG_ERROR("Cellular
service no longer exists", service_path);
Need to return here; ChangeState does not safely handle NULL network.

[armansito, 2013-08-14 22:46:34]

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc (right):

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc:411: const NetworkState*
network = nsh->GetNetworkState(path.substr(1));
On 2013/08/14 21:58:35, stevenjb (chromium) wrote:
> On 2013/08/14 21:42:29, gauravsh wrote:
> > On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> > > the path.substr(1) should probably be assigned to a local and documented.
> > 
> > The path is of the form //service/5, so the path.substr() gets rid of the
> extra
> > '/'.
> > 
> > I don't really understand how the URL of the WebContents frame gets set to
> this.
> > Do any of you know?
> I do not. We should at least add some sort of comment, even if it's a 'TODO:
> investigate why there is an extra '/' at the beginning of path'
> 

The URL should be obtained from the service in question (via Service.Olp). This
code launches an extension that lives in chromeos-assets in the ChromeOS source.

https://codereview.chromium.org/22611005/diff/12001/chromeos/network/network_...
File chromeos/network/network_connection_handler.cc (left):

https://codereview.chromium.org/22611005/diff/12001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:322: 
On 2013/08/14 21:42:29, gauravsh wrote:
> On 2013/08/14 00:49:51, armansito wrote:
> > A small suggestion: now that there is a NetworkActivationHandler, it might
> make
> > sense to move the "NetworkRequiresActivation" function, which is defined in
> this
> > file in an anonymous namespace, over there.
> 
> NetworkRequiresActivation() is used by ConnectToNetwork in
> NetworkConnectionHandler. I guess we could make it static in
> NetworkActivationHandler. What do you think?

sgtm.

[gauravsh, 2013-08-15 01:46:35]

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

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:198: void
MobileActivator::NetworkManagerChanged() {
On 2013/08/14 21:58:35, stevenjb (chromium) wrote:
> On 2013/08/14 21:42:29, gauravsh wrote:
> > On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> > > We should rename this, something like RefreshCellularNetwork.
> > 
> > Not sure I understand. This is an observer override for
> > NetworkStateHanderObserver.
> 
> Oh, right. I don't think we need to observe that here with
NetworkStateHandler.
> In fact, I think we could probably remove that entirely... it really only
> happens if a list changes (we already notify for that) or a technology changes
> (which should maybe have a specific notification). I'll make a note to audit
> that.
> 

Ok, so are you saying we should get rid of the NetworkManagerChanged event, and
instead replace rely on the appropriate NetworkListChanged/TechnologyChanged
events?

In any case, I'd prefer to do that in a separate change. There's a lot of places
where we use NetworkManagerChanged events. They all rely on casting a wider net
on getting notified of major Shill manager changes, so we may need to figure out
the appropriate level of granularity first.

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc (right):

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc:411: const NetworkState*
network = nsh->GetNetworkState(path.substr(1));
On 2013/08/14 21:58:35, stevenjb (chromium) wrote:
> On 2013/08/14 21:42:29, gauravsh wrote:
> > On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> > > the path.substr(1) should probably be assigned to a local and documented.
> > 
> > The path is of the form //service/5, so the path.substr() gets rid of the
> extra
> > '/'.
> > 
> > I don't really understand how the URL of the WebContents frame gets set to
> this.
> > Do any of you know?
> I do not. We should at least add some sort of comment, even if it's a 'TODO:
> investigate why there is an extra '/' at the beginning of path'
> 

Done.

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

https://codereview.chromium.org/22611005/diff/26001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:799: NET_LOG_ERROR("Cellular
service no longer exists", service_path);
On 2013/08/14 21:58:35, stevenjb (chromium) wrote:
> Need to return here; ChangeState does not safely handle NULL network.

Done.

[stevenjb, 2013-08-15 02:00:10]

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

https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:198: void
MobileActivator::NetworkManagerChanged() {
On 2013/08/15 01:46:35, gauravsh wrote:
> On 2013/08/14 21:58:35, stevenjb (chromium) wrote:
> > On 2013/08/14 21:42:29, gauravsh wrote:
> > > On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> > > > We should rename this, something like RefreshCellularNetwork.
> > > 
> > > Not sure I understand. This is an observer override for
> > > NetworkStateHanderObserver.
> > 
> > Oh, right. I don't think we need to observe that here with
> NetworkStateHandler.
> > In fact, I think we could probably remove that entirely... it really only
> > happens if a list changes (we already notify for that) or a technology
changes
> > (which should maybe have a specific notification). I'll make a note to audit
> > that.
> > 
> 
> Ok, so are you saying we should get rid of the NetworkManagerChanged event,
and
> instead replace rely on the appropriate NetworkListChanged/TechnologyChanged
> events?
> 
> In any case, I'd prefer to do that in a separate change. There's a lot of
places
> where we use NetworkManagerChanged events. They all rely on casting a wider
net
> on getting notified of major Shill manager changes, so we may need to figure
out
> the appropriate level of granularity first.

I certainly wasn't suggesting changing all of them here :)

However, I can't think of a Manager change that would affect this class? While
named the same, this is fairly different from how it behaved for NetworkLibrary.
We're observing DefaultNetworkChanged and NetworkPropertiesUpdated here; that
should be sufficient.

[gauravsh, 2013-08-15 22:19:39]

On 2013/08/15 02:00:10, stevenjb (chromium) wrote:
>
https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
> File chrome/browser/chromeos/mobile/mobile_activator.cc (right):
> 
>
https://codereview.chromium.org/22611005/diff/12001/chrome/browser/chromeos/m...
> chrome/browser/chromeos/mobile/mobile_activator.cc:198: void
> MobileActivator::NetworkManagerChanged() {
> On 2013/08/15 01:46:35, gauravsh wrote:
> > On 2013/08/14 21:58:35, stevenjb (chromium) wrote:
> > > On 2013/08/14 21:42:29, gauravsh wrote:
> > > > On 2013/08/14 02:28:31, stevenjb (chromium) wrote:
> > > > > We should rename this, something like RefreshCellularNetwork.
> > > > 
> > > > Not sure I understand. This is an observer override for
> > > > NetworkStateHanderObserver.
> > > 
> > > Oh, right. I don't think we need to observe that here with
> > NetworkStateHandler.
> > > In fact, I think we could probably remove that entirely... it really only
> > > happens if a list changes (we already notify for that) or a technology
> changes
> > > (which should maybe have a specific notification). I'll make a note to
audit
> > > that.
> > > 
> > 
> > Ok, so are you saying we should get rid of the NetworkManagerChanged event,
> and
> > instead replace rely on the appropriate NetworkListChanged/TechnologyChanged
> > events?
> > 
> > In any case, I'd prefer to do that in a separate change. There's a lot of
> places
> > where we use NetworkManagerChanged events. They all rely on casting a wider
> net
> > on getting notified of major Shill manager changes, so we may need to figure
> out
> > the appropriate level of granularity first.
> 
> I certainly wasn't suggesting changing all of them here :)
> 
> However, I can't think of a Manager change that would affect this class? While
> named the same, this is fairly different from how it behaved for
NetworkLibrary.
> We're observing DefaultNetworkChanged and NetworkPropertiesUpdated here; that
> should be sufficient.

Got it. Done.

[stevenjb, 2013-08-16 18:19:05]

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

https://codereview.chromium.org/22611005/diff/42001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:479: this,
&MobileActivator::ReconnectTimedOut);
Note: This logic is probably fine for now, but when we re-factor this code we
should use the DefaultNetworkChanged() and NetworkPropertiesUpdated() observers
to detect state changes instead of a timer.

[stevenjb, 2013-08-16 18:19:25]

LGTM

[gauravsh, 2013-08-22 22:36:43]

PTAL

I had to make a few changes in response the results of testing:

- Since StartActivation() is now called asynchronously, we need extra state
keeping to defer state machine transition until success/failure for the request
is known
- Some factoring of NetworkHandler calls in MobileActivator for easy mocking in
unit tests.
- The post method comparison should be case insensitive.

[stevenjb, 2013-08-23 00:10:55]

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

https://codereview.chromium.org/22611005/diff/95001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:285: service_path_);
GetNetworkState()?

https://codereview.chromium.org/22611005/diff/95001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:537: const std::string&
service_path) {
I think this is always called with service_path_, in which case we should omit
the argument.

https://codereview.chromium.org/22611005/diff/95001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:701: else if (activation ==
flimflam::kActivationStateNotActivated ||
} else

https://codereview.chromium.org/22611005/diff/95001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:798: const NetworkState*
network = GetNetworkState(service_path);
This can probably use NSH->GetNetworkState()?

[gauravsh, 2013-08-27 22:58:12]

Phew! I think things finally all work. I tested both 4G LTE and 3G activation
and it works.

In addition to your feedback, the new patch addresses:
- the service path changes as the modem goes through resets. So now we update
the service path appropriately by tracking cellular services on the same device
path
- fixed a potential null dereference crasher in creation of
network_handler::ErrorCallback

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

https://codereview.chromium.org/22611005/diff/95001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:285: service_path_);
On 2013/08/23 00:10:55, stevenjb (chromium) wrote:
> GetNetworkState()?

Done.

https://codereview.chromium.org/22611005/diff/95001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:537: const std::string&
service_path) {
On 2013/08/23 00:10:55, stevenjb (chromium) wrote:
> I think this is always called with service_path_, in which case we should omit
> the argument.

Not in 2 cases which matter for the unit tests - InitateActivation and
HandleActivationFailure (former is easy to deal with, the latter makes us lose
test coverage for activation failure state transitions).

https://codereview.chromium.org/22611005/diff/95001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:701: else if (activation ==
flimflam::kActivationStateNotActivated ||
On 2013/08/23 00:10:55, stevenjb (chromium) wrote:
> } else

Done.

https://codereview.chromium.org/22611005/diff/95001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:798: const NetworkState*
network = GetNetworkState(service_path);
On 2013/08/23 00:10:55, stevenjb (chromium) wrote:
> This can probably use NSH->GetNetworkState()?

This gets used in the test.

[stevenjb, 2013-08-28 21:22:46]

https://codereview.chromium.org/22611005/diff/106001/chrome/browser/chromeos/...
File chrome/browser/chromeos/mobile/mobile_activator.cc (right):

https://codereview.chromium.org/22611005/diff/106001/chrome/browser/chromeos/...
chrome/browser/chromeos/mobile/mobile_activator.cc:377: } else if
(technology_state !=NetworkStateHandler::TECHNOLOGY_ENABLED) {
' ' after !=

https://codereview.chromium.org/22611005/diff/106001/chrome/browser/chromeos/...
chrome/browser/chromeos/mobile/mobile_activator.cc:552: const
NetworkState*MobileActivator::GetNetworkState(
' ' after *

https://codereview.chromium.org/22611005/diff/106001/chrome/browser/chromeos/...
chrome/browser/chromeos/mobile/mobile_activator.cc:603: activation ==
flimflam::kActivationStateActivating;
nit: not sure that local is_partially_activated makes things any clearer

https://codereview.chromium.org/22611005/diff/106001/chrome/browser/chromeos/...
chrome/browser/chromeos/mobile/mobile_activator.cc:608: LOG(WARNING) <<
"Activation failure detected " << network->path();
NET_LOG_EVENT()

https://codereview.chromium.org/22611005/diff/106001/chrome/browser/chromeos/...
chrome/browser/chromeos/mobile/mobile_activator.cc:1030: }
I know it was like this, but all of these strings should be constants defined
somewhere

[gauravsh, 2013-08-28 23:45:07]

https://codereview.chromium.org/22611005/diff/106001/chrome/browser/chromeos/...
File chrome/browser/chromeos/mobile/mobile_activator.cc (right):

https://codereview.chromium.org/22611005/diff/106001/chrome/browser/chromeos/...
chrome/browser/chromeos/mobile/mobile_activator.cc:377: } else if
(technology_state !=NetworkStateHandler::TECHNOLOGY_ENABLED) {
On 2013/08/28 21:22:47, stevenjb (chromium) wrote:
> ' ' after !=

Done.

https://codereview.chromium.org/22611005/diff/106001/chrome/browser/chromeos/...
chrome/browser/chromeos/mobile/mobile_activator.cc:552: const
NetworkState*MobileActivator::GetNetworkState(
On 2013/08/28 21:22:47, stevenjb (chromium) wrote:
> ' ' after *

Done.

https://codereview.chromium.org/22611005/diff/106001/chrome/browser/chromeos/...
chrome/browser/chromeos/mobile/mobile_activator.cc:603: activation ==
flimflam::kActivationStateActivating;
On 2013/08/28 21:22:47, stevenjb (chromium) wrote:
> nit: not sure that local is_partially_activated makes things any clearer

Done.

https://codereview.chromium.org/22611005/diff/106001/chrome/browser/chromeos/...
chrome/browser/chromeos/mobile/mobile_activator.cc:608: LOG(WARNING) <<
"Activation failure detected " << network->path();
On 2013/08/28 21:22:47, stevenjb (chromium) wrote:
> NET_LOG_EVENT()

Done.

https://codereview.chromium.org/22611005/diff/106001/chrome/browser/chromeos/...
chrome/browser/chromeos/mobile/mobile_activator.cc:1030: }
On 2013/08/28 21:22:47, stevenjb (chromium) wrote:
> I know it was like this, but all of these strings should be constants defined
> somewhere

Actually, this method isn't even used anywhere. So, removed.

[stevenjb, 2013-08-29 17:04:30]

lgtm

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gauravsh@chromium.org/22611005/126001

[commit-bot: I haz the power, 2013-08-29 17:40:01]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[gauravsh, 2013-08-29 20:09:22]

+xiyuan for c/b/ui/webui/chromeos/OWNERS

[xiyuan, 2013-08-29 20:28:26]

c/b/ui/webui/chromeos LGTM

[commit-bot: I haz the power, 2013-08-29 22:03:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gauravsh@chromium.org/22611005/126001

[commit-bot: I haz the power, 2013-08-30 01:11:37]

Change committed as 220463