Move NetworkStateNotifier and NetworkConnect from src/ui

Move NetworkStateNotifier and NetworkConnect from src/ui

This moves:
NetworkConnect -> src/chromeos
NetworkStateNotifier -> chrome/browser/chromeos/net

It also moves the following from chrome/browser/ui/ash to keep all
network UI code in the same place:
NetwrokConnectDelegateChromeos -> chrome/browser/chromeos/net

All UI code in NetworkConnect has been moved to NetworkConnect::Delegate.

BUG=657127
Committed: https://crrev.com/7f42fb3550ed2e094542944712c7350736bc1b1a
Cr-Commit-Position: refs/heads/master@{#426908}

[stevenjb, 2016-10-19 20:25:30]

stevenjb@chromium.org changed reviewers:
+ estade@chromium.org, jamescook@chromium.org

[James Cook, 2016-10-20 00:11:43]

I like it. Mostly little things.

https://codereview.chromium.org/2434683003/diff/1/ash/common/system/system_no...
File ash/common/system/system_notifier.cc (right):

https://codereview.chromium.org/2434683003/diff/1/ash/common/system/system_no...
ash/common/system/system_notifier.cc:74: const char kNotifierNetwork[] =
"ui.chromeos.network";
Where are these declared?

https://codereview.chromium.org/2434683003/diff/1/ash/common/system/system_no...
ash/common/system/system_notifier.cc:75: const char kNotifierNetworkError[] =
"ui.chromeos.network.error";
Also, should they still start with "ui.chromeos" or "ash."?  I'm not sure if the
message center keeps track of historical suppressions by name, so it might need
to be the former, but if not then the latter would be nice for consistency.

https://codereview.chromium.org/2434683003/diff/1/ash/mus/network_connect_del...
File ash/mus/network_connect_delegate_mus.cc (right):

https://codereview.chromium.org/2434683003/diff/1/ash/mus/network_connect_del...
ash/mus/network_connect_delegate_mus.cc:49: LOG(ERROR) << "Network Connect
Error: " << error_name
Can you copy either the TODO or NOTIMPLEMENTED from above (or both) here?

https://codereview.chromium.org/2434683003/diff/1/chrome/app/chromeos_strings...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/2434683003/diff/1/chrome/app/chromeos_strings...
chrome/app/chromeos_strings.grdp:2338: <!-- Options strings -->
Little notes like this go a long way. Thanks for adding it.

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:256:
chromeos::NetworkConnect::Initialize(network_connect_delegate_.get());
nit: chromeos:: maybe not needed here? (We're in namespace chromeos)

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/mob...
File chrome/browser/chromeos/mobile/mobile_activator.cc (right):

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/mob...
chrome/browser/chromeos/mobile/mobile_activator.cc:628:
chromeos::NetworkConnect::Get()->ConnectToNetwork(network->path());
nit: chromeos not needed

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/network_connect_delegate_chromeos.h (right):

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_connect_delegate_chromeos.h:19: : public
chromeos::NetworkConnect::Delegate {
nit: chromeos:: not needed

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/network_state_notifier.cc (right):

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_state_notifier.cc:83:
ash::system_notifier::kNotifierNetworkError, callback));
Just FYI - in the long run this isn't going to work. We can't have chrome
depending directly on ash internals like this. Either the constants are
important enough that they should be part of ash's public interface (e.g. in
//ash/public/cpp), or they should be chrome-only constants (with a way for
chrome to tell the notification center that this one should appear over the lock
screen), or this class should live in //ash.

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_state_notifier.cc:444: if
(!SystemTrayClient::Get())
I don't think this is needed. SystemTrayClient is created during pre-profile
init, so should be available for the lifetime of the browser.

If it has to be checked just for tests, please add a comment.

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/shill_error.cc (right):

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/shill_error.cc:46: return l10n_util::GetStringUTF16(
optional nit: "using l10n_util::GetStringFUTF16;" at the top might make this
file easier to read.

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/opt...
File chrome/browser/chromeos/options/vpn_config_view.cc (right):

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/opt...
chrome/browser/chromeos/options/vpn_config_view.cc:383:
chromeos::NetworkConnect::Get()->CreateConfigurationAndConnect(&properties,
chromeos:: not needed (and probably in many other files, I didn't check them
all)

https://codereview.chromium.org/2434683003/diff/1/chromeos/network/network_co...
File chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/2434683003/diff/1/chromeos/network/network_co...
chromeos/network/network_connect.cc:150: if
(LoginState::Get()->IsUserLoggedIn())
Not for this CL: It would be very nice to eliminate the dependency here on
LoginState.

https://codereview.chromium.org/2434683003/diff/1/chromeos/network/network_co...
chromeos/network/network_connect.cc:356: if (!network)
Just for my knowledge: If there's no known network here, would it be better to
show a generic network connect error (like don't early return and fall through
with empty guid)?

https://codereview.chromium.org/2434683003/diff/1/chromeos/network/network_co...
File chromeos/network/network_connect.h (right):

https://codereview.chromium.org/2434683003/diff/1/chromeos/network/network_co...
chromeos/network/network_connect.h:24: // of UI is handled by the
NetworkConnect::Delegate implementation.
Thanks for the added docs.

[stevenjb, 2016-10-20 00:57:21]

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

[stevenjb, 2016-10-20 00:57:23]

+flackr@chromium.org for tools/gritsettings/resource_ids

PTAL

https://codereview.chromium.org/2434683003/diff/1/ash/common/system/system_no...
File ash/common/system/system_notifier.cc (right):

https://codereview.chromium.org/2434683003/diff/1/ash/common/system/system_no...
ash/common/system/system_notifier.cc:74: const char kNotifierNetwork[] =
"ui.chromeos.network";
On 2016/10/20 00:11:43, James Cook wrote:
> Where are these declared?

They are already in the header even though they weren't in the .cc file. Not
sure how/why.

https://codereview.chromium.org/2434683003/diff/1/ash/common/system/system_no...
ash/common/system/system_notifier.cc:75: const char kNotifierNetworkError[] =
"ui.chromeos.network.error";
On 2016/10/20 00:11:43, James Cook wrote:
> Also, should they still start with "ui.chromeos" or "ash."?  I'm not sure if
the
> message center keeps track of historical suppressions by name, so it might
need
> to be the former, but if not then the latter would be nice for consistency.

Renamed to ash.

https://codereview.chromium.org/2434683003/diff/1/ash/mus/network_connect_del...
File ash/mus/network_connect_delegate_mus.cc (right):

https://codereview.chromium.org/2434683003/diff/1/ash/mus/network_connect_del...
ash/mus/network_connect_delegate_mus.cc:49: LOG(ERROR) << "Network Connect
Error: " << error_name
On 2016/10/20 00:11:43, James Cook wrote:
> Can you copy either the TODO or NOTIMPLEMENTED from above (or both) here?

It's kind of implemented :) I'm not sure what the precise plan is, but I'll copy
the TODO.

https://codereview.chromium.org/2434683003/diff/1/chrome/app/chromeos_strings...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/2434683003/diff/1/chrome/app/chromeos_strings...
chrome/app/chromeos_strings.grdp:2338: <!-- Options strings -->
On 2016/10/20 00:11:43, James Cook wrote:
> Little notes like this go a long way. Thanks for adding it.

Acknowledged.

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:256:
chromeos::NetworkConnect::Initialize(network_connect_delegate_.get());
On 2016/10/20 00:11:43, James Cook wrote:
> nit: chromeos:: maybe not needed here? (We're in namespace chromeos)

Done.

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/mob...
File chrome/browser/chromeos/mobile/mobile_activator.cc (right):

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/mob...
chrome/browser/chromeos/mobile/mobile_activator.cc:628:
chromeos::NetworkConnect::Get()->ConnectToNetwork(network->path());
On 2016/10/20 00:11:43, James Cook wrote:
> nit: chromeos not needed

Done.

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/network_connect_delegate_chromeos.h (right):

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_connect_delegate_chromeos.h:19: : public
chromeos::NetworkConnect::Delegate {
On 2016/10/20 00:11:43, James Cook wrote:
> nit: chromeos:: not needed

Done.

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/network_state_notifier.cc (right):

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_state_notifier.cc:83:
ash::system_notifier::kNotifierNetworkError, callback));
On 2016/10/20 00:11:43, James Cook wrote:
> Just FYI - in the long run this isn't going to work. We can't have chrome
> depending directly on ash internals like this. Either the constants are
> important enough that they should be part of ash's public interface (e.g. in
> //ash/public/cpp), or they should be chrome-only constants (with a way for
> chrome to tell the notification center that this one should appear over the
lock
> screen), or this class should live in //ash.

Hmm. I seem to recall there being a *lot* of c/b/chromeos code that depends on
ash internals. The rule was always that we can't have the inverse relationship.

Regardless, this should be cleaned up when we figure out ownership, etc, of the
message center.

FWIW, ash::system_notifier::ShouldAlwaysShowPopups() is called from
c/b/notifications/.

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_state_notifier.cc:444: if
(!SystemTrayClient::Get())
On 2016/10/20 00:11:43, James Cook wrote:
> I don't think this is needed. SystemTrayClient is created during pre-profile
> init, so should be available for the lifetime of the browser.
> 
> If it has to be checked just for tests, please add a comment.

This is an async callback triggered by the message center. I would prefer to
have this code be safe, independent of the shutdown rules for the message center
vs. SystemTrayClient. I've seen too many shutdown crashes...

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/shill_error.cc (right):

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/shill_error.cc:46: return l10n_util::GetStringUTF16(
On 2016/10/20 00:11:43, James Cook wrote:
> optional nit: "using l10n_util::GetStringFUTF16;" at the top might make this
> file easier to read.

Acknowledged.

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/opt...
File chrome/browser/chromeos/options/vpn_config_view.cc (right):

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/opt...
chrome/browser/chromeos/options/vpn_config_view.cc:383:
chromeos::NetworkConnect::Get()->CreateConfigurationAndConnect(&properties,
On 2016/10/20 00:11:43, James Cook wrote:
> chromeos:: not needed (and probably in many other files, I didn't check them
> all)

Fixed throughout.

https://codereview.chromium.org/2434683003/diff/1/chromeos/network/network_co...
File chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/2434683003/diff/1/chromeos/network/network_co...
chromeos/network/network_connect.cc:150: if
(LoginState::Get()->IsUserLoggedIn())
On 2016/10/20 00:11:43, James Cook wrote:
> Not for this CL: It would be very nice to eliminate the dependency here on
> LoginState.

Acknowledged. Long term plan is to include network settings in login. (That
said, LoginState was created to allow for this dependency and we have lots of
LoginState dependencies in chromeos/network).

https://codereview.chromium.org/2434683003/diff/1/chromeos/network/network_co...
chromeos/network/network_connect.cc:356: if (!network)
On 2016/10/20 00:11:43, James Cook wrote:
> Just for my knowledge: If there's no known network here, would it be better to
> show a generic network connect error (like don't early return and fall through
> with empty guid)?

I guess we can go ahead and fall through, we already handle the case of an empty
guid. In practice this shouldn't happen, it's just not logically impossible in
an asynchronous world.

https://codereview.chromium.org/2434683003/diff/1/chromeos/network/network_co...
File chromeos/network/network_connect.h (right):

https://codereview.chromium.org/2434683003/diff/1/chromeos/network/network_co...
chromeos/network/network_connect.h:24: // of UI is handled by the
NetworkConnect::Delegate implementation.
On 2016/10/20 00:11:43, James Cook wrote:
> Thanks for the added docs.

Acknowledged.

[flackr, 2016-10-20 01:04:28]

tools/gritsettings/resource_ids lgtm

[James Cook, 2016-10-20 15:48:42]

LGTM. Thanks for cleaning this up.

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/shill_error.cc (right):

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/shill_error.cc:46: return l10n_util::GetStringUTF16(
On 2016/10/20 00:57:23, stevenjb wrote:
> On 2016/10/20 00:11:43, James Cook wrote:
> > optional nit: "using l10n_util::GetStringFUTF16;" at the top might make this
> > file easier to read.
> 
> Acknowledged.

Did you mean to do it, or just acknowledge that I said optional? :-)

https://codereview.chromium.org/2434683003/diff/1/chromeos/network/network_co...
File chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/2434683003/diff/1/chromeos/network/network_co...
chromeos/network/network_connect.cc:150: if
(LoginState::Get()->IsUserLoggedIn())
On 2016/10/20 00:57:23, stevenjb wrote:
> On 2016/10/20 00:11:43, James Cook wrote:
> > Not for this CL: It would be very nice to eliminate the dependency here on
> > LoginState.
> 
> Acknowledged. Long term plan is to include network settings in login. (That
> said, LoginState was created to allow for this dependency and we have lots of
> LoginState dependencies in chromeos/network).

Just for my knowledge: "include network settings in login" means allow it to be
changed during login, i.e. this if() would go away?

[stevenjb, 2016-10-20 17:16:16]

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/shill_error.cc (right):

https://codereview.chromium.org/2434683003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/shill_error.cc:46: return l10n_util::GetStringUTF16(
On 2016/10/20 15:48:41, James Cook wrote:
> On 2016/10/20 00:57:23, stevenjb wrote:
> > On 2016/10/20 00:11:43, James Cook wrote:
> > > optional nit: "using l10n_util::GetStringFUTF16;" at the top might make
this
> > > file easier to read.
> > 
> > Acknowledged.
> 
> Did you mean to do it, or just acknowledge that I said optional? :-)

Just acknowledge the optional part :) This code was just moved, I just as soon
minimize the changes here. But I like to double check that I responded to every
review comment :)

https://codereview.chromium.org/2434683003/diff/1/chromeos/network/network_co...
File chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/2434683003/diff/1/chromeos/network/network_co...
chromeos/network/network_connect.cc:150: if
(LoginState::Get()->IsUserLoggedIn())
On 2016/10/20 15:48:41, James Cook wrote:
> On 2016/10/20 00:57:23, stevenjb wrote:
> > On 2016/10/20 00:11:43, James Cook wrote:
> > > Not for this CL: It would be very nice to eliminate the dependency here on
> > > LoginState.
> > 
> > Acknowledged. Long term plan is to include network settings in login. (That
> > said, LoginState was created to allow for this dependency and we have lots
of
> > LoginState dependencies in chromeos/network).
> 
> Just for my knowledge: "include network settings in login" means allow it to
be
> changed during login, i.e. this if() would go away?

Correct. ShowNetworkSettings would behave differently in login (we will show a
different settings UI with just networking and maybe bluetooth, display, etc).

[stevenjb, 2016-10-20 21:28:32]

The CQ bit was checked by stevenjb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 21:29:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[stevenjb, 2016-10-20 22:17:40]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-10-20 22:17:40]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2434683003/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-20 22:18:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 00:04:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 00:04:26]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[stevenjb, 2016-10-21 01:15:45]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-10-21 01:15:45]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2434683003/#ps80001
(title: "export NetworkConnect::Delegate")

[commit-bot: I haz the power, 2016-10-21 01:16:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 02:46:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 02:47:00]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[stevenjb, 2016-10-21 16:57:46]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-10-21 16:57:46]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2434683003/#ps100001
(title: "Fix unit test")

[commit-bot: I haz the power, 2016-10-21 16:58:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[stevenjb, 2016-10-21 18:28:29]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-10-21 18:28:29]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2434683003/#ps120001
(title: "Fix virtuals")

[commit-bot: I haz the power, 2016-10-21 18:29:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 22:00:41]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-21 22:02:53]

Description was changed from

==========
Move NetworkStateNotifier and NetworkConnect from src/ui

This moves:
NetworkConnect -> src/chromeos
NetworkStateNotifier -> chrome/browser/chromeos/net

It also moves the following from chrome/browser/ui/ash to keep all
network UI code in the same place:
NetwrokConnectDelegateChromeos -> chrome/browser/chromeos/net

All UI code in NetworkConnect has been moved to NetworkConnect::Delegate.

BUG=657127
==========

to

==========
Move NetworkStateNotifier and NetworkConnect from src/ui

This moves:
NetworkConnect -> src/chromeos
NetworkStateNotifier -> chrome/browser/chromeos/net

It also moves the following from chrome/browser/ui/ash to keep all
network UI code in the same place:
NetwrokConnectDelegateChromeos -> chrome/browser/chromeos/net

All UI code in NetworkConnect has been moved to NetworkConnect::Delegate.

BUG=657127

Committed: https://crrev.com/7f42fb3550ed2e094542944712c7350736bc1b1a
Cr-Commit-Position: refs/heads/master@{#426908}
==========

[commit-bot: I haz the power, 2016-10-21 22:02:54]

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