Refactored mobile activation engine outside of WebUI handler in order to expose its state to other …

Refactored mobile activation engine outside of WebUI handler in order to expose its state to other layer of UI (i.e. dialog, background activation process...).

BUG=130420
TEST=manual regression test for mobile device activation

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

[rkc, 2012-06-05 21:48:03]

https://chromiumcodereview.appspot.com/10456045/diff/18004/chrome/browser/chr...
File chrome/browser/chromeos/mobile/mobile_activator.cc (right):

https://chromiumcodereview.appspot.com/10456045/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/mobile/mobile_activator.cc:81: const char
kErrorAlreadyRunning[] = "already_running";
Since the 'already_running' logic has replaced with the 'terminated' logic, I
believe this is unused now.

https://chromiumcodereview.appspot.com/10456045/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/mobile/mobile_activator.cc:87: CellularNetwork*
FindCellularNetwork(
This function is uncessary. It can be replaced in code with
chromeos::CrosLibrary::Get()->GetNetworkLibrary()->FindCellularNetworkByPath(service_path)

https://chromiumcodereview.appspot.com/10456045/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/mobile/mobile_activator.cc:203: void
MobileActivator::TerminateActivation() {
This class is a singleton, so the destructor will only be called at process
exit. Is all this cleanup relevant at that point?

https://chromiumcodereview.appspot.com/10456045/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/mobile/mobile_activator.cc:230: // because the service
path remains the same.
Fix outdated/now incorrect comment.

https://chromiumcodereview.appspot.com/10456045/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/mobile/mobile_activator.cc:284: if (!network ||
!network->SupportsActivation())
A NULL check for network is already being done above in
MobileActivator::InitiateActivation; move the check for SupportsActivation also
there?

https://chromiumcodereview.appspot.com/10456045/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/mobile/mobile_activator.cc:494: base::Unretained(this),
delay),
Though this class is Singleton, it will be destructed at process exit. In the
rare case the class gets destructed before the delay occurs, we can crash at
exit. Continuing to use Weak Pointers would mitigate the condition.

https://chromiumcodereview.appspot.com/10456045/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/mobile/mobile_activator.cc:516: base::Unretained(this),
Same as above.

https://chromiumcodereview.appspot.com/10456045/diff/18004/chrome/browser/chr...
File chrome/browser/chromeos/network_message_observer.h (right):

https://chromiumcodereview.appspot.com/10456045/diff/18004/chrome/browser/chr...
chrome/browser/chromeos/network_message_observer.h:43: virtual void
ShowNoDataNotification(const CellularNetwork* cellular,
Nit: Should this be const CellularNetwork* const cellular?

https://chromiumcodereview.appspot.com/10456045/diff/18004/chrome/browser/ui/...
File chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc (right):

https://chromiumcodereview.appspot.com/10456045/diff/18004/chrome/browser/ui/...
chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc:119: explicit
MobileSetupUIHTMLSource();
no explicit.

https://chromiumcodereview.appspot.com/10456045/diff/18004/chrome/browser/ui/...
chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc:142: explicit
MobileSetupHandler();
no explicit.

[zel, 2012-06-05 23:02:35]

http://codereview.chromium.org/10456045/diff/18004/chrome/browser/chromeos/mo...
File chrome/browser/chromeos/mobile/mobile_activator.cc (right):

http://codereview.chromium.org/10456045/diff/18004/chrome/browser/chromeos/mo...
chrome/browser/chromeos/mobile/mobile_activator.cc:81: const char
kErrorAlreadyRunning[] = "already_running";
On 2012/06/05 21:48:03, Rahul Chaturvedi wrote:
> Since the 'already_running' logic has replaced with the 'terminated' logic, I
> believe this is unused now.

Done.

http://codereview.chromium.org/10456045/diff/18004/chrome/browser/chromeos/mo...
chrome/browser/chromeos/mobile/mobile_activator.cc:87: CellularNetwork*
FindCellularNetwork(
On 2012/06/05 21:48:03, Rahul Chaturvedi wrote:
> This function is uncessary. It can be replaced in code with
>
chromeos::CrosLibrary::Get()->GetNetworkLibrary()->FindCellularNetworkByPath(service_path)

Removed.

http://codereview.chromium.org/10456045/diff/18004/chrome/browser/chromeos/mo...
chrome/browser/chromeos/mobile/mobile_activator.cc:203: void
MobileActivator::TerminateActivation() {
On 2012/06/05 21:48:03, Rahul Chaturvedi wrote:
> This class is a singleton, so the destructor will only be called at process
> exit. Is all this cleanup relevant at that point?

TerminateActivation() is called when user closes the activation dialog as well -
see MobileSetupHandler::~MobileSetupHandler()

http://codereview.chromium.org/10456045/diff/18004/chrome/browser/chromeos/mo...
chrome/browser/chromeos/mobile/mobile_activator.cc:230: // because the service
path remains the same.
On 2012/06/05 21:48:03, Rahul Chaturvedi wrote:
> Fix outdated/now incorrect comment.

Done.

http://codereview.chromium.org/10456045/diff/18004/chrome/browser/chromeos/mo...
chrome/browser/chromeos/mobile/mobile_activator.cc:284: if (!network ||
!network->SupportsActivation())
On 2012/06/05 21:48:03, Rahul Chaturvedi wrote:
> A NULL check for network is already being done above in
> MobileActivator::InitiateActivation; move the check for SupportsActivation
also
> there?

In theory FindCellularNetworkByMeid() can return NULL if the service had
disappeared in the mean time.

http://codereview.chromium.org/10456045/diff/18004/chrome/browser/chromeos/mo...
chrome/browser/chromeos/mobile/mobile_activator.cc:494: base::Unretained(this),
delay),
On 2012/06/05 21:48:03, Rahul Chaturvedi wrote:
> Though this class is Singleton, it will be destructed at process exit. In the
> rare case the class gets destructed before the delay occurs, we can crash at
> exit. Continuing to use Weak Pointers would mitigate the condition.

Done.

http://codereview.chromium.org/10456045/diff/18004/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc (right):

http://codereview.chromium.org/10456045/diff/18004/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc:119: explicit
MobileSetupUIHTMLSource();
On 2012/06/05 21:48:03, Rahul Chaturvedi wrote:
> no explicit.

Done.

http://codereview.chromium.org/10456045/diff/18004/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc:142: explicit
MobileSetupHandler();
On 2012/06/05 21:48:03, Rahul Chaturvedi wrote:
> no explicit.

Done.

[csilv, 2012-06-05 23:20:20]

internet_options_handler2.cc lgtm

[stevenjb (google-dont-use), 2012-06-05 23:20:21]

LGTM for chrome/browser/chromeos/status/network_menu.cc

[rkc, 2012-06-05 23:26:58]

On 2012/06/05 23:20:21, stevenjb1 wrote:
> LGTM for chrome/browser/chromeos/status/network_menu.cc

LGTM

[sky, 2012-06-05 23:28:18]

LGTM