Create a NetworkConnect class and Delegate

Create a NetworkConnect class and Delegate

This is in preparation for moving NetworkConnect and NetworkStateNotifier
from ash to ui/chromeos.

BUG=425996
Committed: https://crrev.com/6d99f5fd5afbe29b979d8b153d89daa1ead8542e
Cr-Commit-Position: refs/heads/master@{#301387}

[stevenjb, 2014-10-23 01:38:20]

stevenjb@chromium.org changed reviewers:
+ armansito@chromium.org, tbarzic@chromium.org

[stevenjb, 2014-10-23 01:38:21]

There shouldn't be any functional changes here, just some re-factoring and
cleanup.

[stevenjb, 2014-10-23 01:40:52]

Also, note: one of the reasons I am making NetworkConnect a class (beyond
tracking the Delegate) is that I anticipate adding Observers also, so that other
pieces of code (e.g. the WiFi sync code) can be notified on a successful user
initiated connect, regardless of where it originated from. I will do that in a
follow-up CL however.

[armansito, 2014-10-24 20:02:22]

lgtm with nits

https://codereview.chromium.org/673713003/diff/40001/ash/system/chromeos/netw...
File ash/system/chromeos/network/network_connect.cc (left):

https://codereview.chromium.org/673713003/diff/40001/ash/system/chromeos/netw...
ash/system/chromeos/network/network_connect.cc:9: #include
"ash/system/chromeos/network/network_state_notifier.h"
So I guess you left this here since NetworkStateNotifier is also moving out of
ash? Would it make sense to define generic delegate methods for these
notifications and have the delegate handle calling the necessary methods on
NetworkStateNotifier?

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

https://codereview.chromium.org/673713003/diff/40001/ash/system/chromeos/netw...
ash/system/chromeos/network/network_connect.cc:625: CHECK(g_network_connect ==
NULL);
Use DCHECK?

https://codereview.chromium.org/673713003/diff/40001/ash/system/chromeos/netw...
File ash/system/chromeos/network/network_state_notifier.cc (right):

https://codereview.chromium.org/673713003/diff/40001/ash/system/chromeos/netw...
ash/system/chromeos/network/network_state_notifier.cc:222: void
NetworkStateNotifier::ShowMobileActivationkError(
s/ShowMobileActivationkError/ShowMobileActivationError/ ?

[armansito, 2014-10-24 20:02:23]

lgtm with nits

[stevenjb, 2014-10-24 23:01:49]

stevenjb@chromium.org changed reviewers:
+ oshima@chromium.org

[stevenjb, 2014-10-24 23:01:50]

+osima@ for c/b/ui/ash OWNER

https://codereview.chromium.org/673713003/diff/40001/ash/system/chromeos/netw...
File ash/system/chromeos/network/network_connect.cc (left):

https://codereview.chromium.org/673713003/diff/40001/ash/system/chromeos/netw...
ash/system/chromeos/network/network_connect.cc:9: #include
"ash/system/chromeos/network/network_state_notifier.h"
On 2014/10/24 20:02:21, armansito wrote:
> So I guess you left this here since NetworkStateNotifier is also moving out of
> ash? Would it make sense to define generic delegate methods for these
> notifications and have the delegate handle calling the necessary methods on
> NetworkStateNotifier?

Correct. They are both moving and NetworkConnect will own NetworkStateNotifier
in the next CL.

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

https://codereview.chromium.org/673713003/diff/40001/ash/system/chromeos/netw...
ash/system/chromeos/network/network_connect.cc:625: CHECK(g_network_connect ==
NULL);
On 2014/10/24 20:02:21, armansito wrote:
> Use DCHECK?

I prefer CHECK for methods like this that don't get called very often since so
many developers primarily use Release builds. (It's fairly easy to accidentally
end up with multiple calls to singleton initializers in tests).

https://codereview.chromium.org/673713003/diff/40001/ash/system/chromeos/netw...
File ash/system/chromeos/network/network_state_notifier.cc (right):

https://codereview.chromium.org/673713003/diff/40001/ash/system/chromeos/netw...
ash/system/chromeos/network/network_state_notifier.cc:222: void
NetworkStateNotifier::ShowMobileActivationkError(
On 2014/10/24 20:02:21, armansito wrote:
> s/ShowMobileActivationkError/ShowMobileActivationError/ ?

Oops, thanks! Done.

[oshima, 2014-10-24 23:14:20]

c/b/ui/ash lgtm with nits

https://codereview.chromium.org/673713003/diff/60001/chrome/browser/ui/ash/ne...
File chrome/browser/ui/ash/network_connect_delegate_chromeos.cc (right):

https://codereview.chromium.org/673713003/diff/60001/chrome/browser/ui/ash/ne...
chrome/browser/ui/ash/network_connect_delegate_chromeos.cc:24: gfx::NativeWindow
GetNativeWindow() {
aura::Window if this is cros only.

https://codereview.chromium.org/673713003/diff/60001/chrome/browser/ui/ash/ne...
chrome/browser/ui/ash/network_connect_delegate_chromeos.cc:55: if
(!IsUIAvailable())
if (IsUIAvailable())
   ... ?

same for other the rest. (Don't feel strongly so you may leave it if you want)

https://codereview.chromium.org/673713003/diff/60001/chrome/browser/ui/ash/ne...
File chrome/browser/ui/ash/network_connect_delegate_chromeos.h (right):

https://codereview.chromium.org/673713003/diff/60001/chrome/browser/ui/ash/ne...
chrome/browser/ui/ash/network_connect_delegate_chromeos.h:1: // Copyright 2013
The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/673713003/diff/60001/chrome/browser/ui/ash/ne...
chrome/browser/ui/ash/network_connect_delegate_chromeos.h:18: virtual
~NetworkConnectDelegateChromeOS();
let's use new c++11 style for new files.

~NetworkConnectDelegateChromeOS() override;

https://codereview.chromium.org/673713003/diff/60001/chrome/browser/ui/ash/ne...
chrome/browser/ui/ash/network_connect_delegate_chromeos.h:20: virtual void
ShowNetworkConfigure(const std::string& network_id) override;
nuke "virtual"

https://codereview.chromium.org/673713003/diff/60001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.h (right):

https://codereview.chromium.org/673713003/diff/60001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:70: virtual void
Initialize() override;
not sure why this wasn't convered in https://codereview.chromium.org/671653002. 
I'll update in a separate CL.

[tbarzic, 2014-10-25 00:35:58]

lgtm

[tbarzic, 2014-10-25 00:35:58]

lgtm

[stevenjb, 2014-10-27 16:24:46]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2014-10-27 16:25:01]

https://codereview.chromium.org/673713003/diff/60001/chrome/browser/ui/ash/ne...
File chrome/browser/ui/ash/network_connect_delegate_chromeos.cc (right):

https://codereview.chromium.org/673713003/diff/60001/chrome/browser/ui/ash/ne...
chrome/browser/ui/ash/network_connect_delegate_chromeos.cc:55: if
(!IsUIAvailable())
On 2014/10/24 23:14:20, oshima wrote:
> if (IsUIAvailable())
>    ... ?
> 
> same for other the rest. (Don't feel strongly so you may leave it if you want)

Not entirely sure what you're suggesting, but I prefer early exits to wrapping
code in an if block, and I'd rather explicitly use ! than call the helper
something like 'IsUIUnAvailable', so leaving this as-is.

https://codereview.chromium.org/673713003/diff/60001/chrome/browser/ui/ash/ne...
File chrome/browser/ui/ash/network_connect_delegate_chromeos.h (right):

https://codereview.chromium.org/673713003/diff/60001/chrome/browser/ui/ash/ne...
chrome/browser/ui/ash/network_connect_delegate_chromeos.h:1: // Copyright 2013
The Chromium Authors. All rights reserved.
On 2014/10/24 23:14:20, oshima wrote:
> 2014

Done.

https://codereview.chromium.org/673713003/diff/60001/chrome/browser/ui/ash/ne...
chrome/browser/ui/ash/network_connect_delegate_chromeos.h:18: virtual
~NetworkConnectDelegateChromeOS();
On 2014/10/24 23:14:20, oshima wrote:
> let's use new c++11 style for new files.
> 
> ~NetworkConnectDelegateChromeOS() override;

Done.

https://codereview.chromium.org/673713003/diff/60001/chrome/browser/ui/ash/ne...
chrome/browser/ui/ash/network_connect_delegate_chromeos.h:20: virtual void
ShowNetworkConfigure(const std::string& network_id) override;
On 2014/10/24 23:14:20, oshima wrote:
> nuke "virtual"

Done.

https://codereview.chromium.org/673713003/diff/60001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.h (right):

https://codereview.chromium.org/673713003/diff/60001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:70: virtual void
Initialize() override;
On 2014/10/24 23:14:20, oshima wrote:
> not sure why this wasn't convered in
https://codereview.chromium.org/671653002. 
> I'll update in a separate CL.

I'll go ahead and fix it here, I personally prefer fixing these with other
changes (where it makes sense) rather than introducing extra CLs.

[commit-bot: I haz the power, 2014-10-27 16:25:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/673713003/120001

[commit-bot: I haz the power, 2014-10-27 17:15:14]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-10-27 17:16:00]

Patchset 7 (id:??) landed as
https://crrev.com/6d99f5fd5afbe29b979d8b153d89daa1ead8542e
Cr-Commit-Position: refs/heads/master@{#301387}