Improved activation logic to cover few more edge cases. Removed button that w...

Improved activation with following changes: 
- removed button that was used to fake payment process. 
- fixed race condition on connect/disconnect. 
- added UMA histograms to trace results of activation process.

BUG=chromium-os:9202, chromium-os:9220, chromium-os:9169
TEST=none



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66346

[oshima, 2010-11-16 05:44:37]

lgtm

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/resources/mob...
File chrome/browser/resources/mobile_setup.js (right):

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/resources/mob...
chrome/browser/resources/mobile_setup.js:144:
$('systemStatus').classList.remove('hidden');
just a suggestion for future. no need to consider now.

May be we should have show/hide function which takes an id, or may be add
show/hide method to DIV element?

[zel, 2010-11-16 05:56:06]

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/resources/mob...
File chrome/browser/resources/mobile_setup.js (right):

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/resources/mob...
chrome/browser/resources/mobile_setup.js:144:
$('systemStatus').classList.remove('hidden');
On 2010/11/16 05:44:37, oshima wrote:
> just a suggestion for future. no need to consider now.
> 
> May be we should have show/hide function which takes an id, or may be add
> show/hide method to DIV element?

It's a mixed content that is shown. Yes, it would be better to have a separate
div for each state. Originally, there was is a nicer way to handle visibility
with dom properties and CSS selectors that magically stopped working for me on
this page. I had no time to investigate that one...

[jglasgow, 2010-11-16 13:37:35]

The fixes in the CL LGTM, but I have suggestions on two other places where the
logic may be wrong.

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
File chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc (right):

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:523: new_state =
PLAN_ACTIVATION_SHOWING_PAYMENT;
Does this also need a check for restricted_pool()?  Maybe this is the path with
the DNS error.

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:546: new_state =
PLAN_ACTIVATION_INITIATING_ACTIVATION;
Looks like this could be called while connected -- which means that we could get
an EIO error from flimflam.

[Eric Shienbrood, 2010-11-16 15:57:19]

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
File chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc (right):

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:412:
browser->CloseTabContents(tab_contents_);
Shouldn't wifi and ethernet be re-enabled here?

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:498: } else if
(network->connection_state() == chromeos::STATE_READY) {
This can be "... if (network->connected()) {". There are several more
occurrences of this below.

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:523: new_state =
PLAN_ACTIVATION_SHOWING_PAYMENT;
On 2010/11/16 13:37:35, jglasgow wrote:
> Does this also need a check for restricted_pool()?  Maybe this is the path
with
> the DNS error.

Or change the check to network->is_active(), which is what I thought you were
going to do. This would be quicker than waiting for restricted_pool to be set.

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:546: new_state =
PLAN_ACTIVATION_INITIATING_ACTIVATION;
On 2010/11/16 13:37:35, jglasgow wrote:
> Looks like this could be called while connected -- which means that we could
get
> an EIO error from flimflam.

I'm not sure I see the problem here. The only other activation state is
ACTIVATION_STATE_ACTIVATING. I don't see how we'd get here when the UI state is
PLAN_ACTIVATION_START.

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:622: // Check for this special
case when we try to do activate partially
"...when we try to activate partially..."

[Jason Glasgow, 2010-11-16 16:13:50]

A few more comments

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
File chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc (right):

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:713: new_state =
PLAN_ACTIVATION_ERROR;
This seems to be dead code.

I'm not sure... should this be 

state_ = PLAN_ACTIVATION_ERROR? 

But I'm not sure that this will be handled correctly in EvaluateCellularNetwork
because the call failed more or less at the level of checking arguments, and
network->activate_state() will remain unchanged (i.e. it will not be state
activation error - we could argue this point).

[Eric Shienbrood, 2010-11-16 16:22:04]

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
File chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc (right):

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:412:
browser->CloseTabContents(tab_contents_);
On 2010/11/16 15:57:19, Eric Shienbrood wrote:
> Shouldn't wifi and ethernet be re-enabled here?

Actually, shouldn't it be calling CompleteActivation?

[zel, 2010-11-16 18:59:28]

PTAL

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
File chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc (right):

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:412:
browser->CloseTabContents(tab_contents_);
On 2010/11/16 16:22:04, Eric Shienbrood wrote:
> On 2010/11/16 15:57:19, Eric Shienbrood wrote:
> > Shouldn't wifi and ethernet be re-enabled here?
> 
> Actually, shouldn't it be calling CompleteActivation?

This is dead code, we don't use it at all. Removed.

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:498: } else if
(network->connection_state() == chromeos::STATE_READY) {
On 2010/11/16 15:57:19, Eric Shienbrood wrote:
> This can be "... if (network->connected()) {". There are several more
> occurrences of this below.

Done.

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:523: new_state =
PLAN_ACTIVATION_SHOWING_PAYMENT;
On 2010/11/16 13:37:35, jglasgow wrote:
> Does this also need a check for restricted_pool()?  Maybe this is the path
with
> the DNS error.

Done.

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:546: new_state =
PLAN_ACTIVATION_INITIATING_ACTIVATION;
On 2010/11/16 13:37:35, jglasgow wrote:
> Looks like this could be called while connected -- which means that we could
get
> an EIO error from flimflam.

This wasn't what was happening, but I have still removed this since we should
never end up here.

http://codereview.chromium.org/5072001/diff/3001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:713: new_state =
PLAN_ACTIVATION_ERROR;
On 2010/11/16 16:13:51, Jason Glasgow wrote:
> This seems to be dead code.
> 
> I'm not sure... should this be 
> 
> state_ = PLAN_ACTIVATION_ERROR? 
> 
> But I'm not sure that this will be handled correctly in
EvaluateCellularNetwork
> because the call failed more or less at the level of checking arguments, and
> network->activate_state() will remain unchanged (i.e. it will not be state
> activation error - we could argue this point).

Done.

[Eric Shienbrood, 2010-11-16 19:34:53]

LGTM
But it's hard for me to say with 100% confidence that the state machine is
completely correct. Let's hope we've gotten all the edge cases. We definitely
need unit tests.