mustash: Show networking items in ash system tray

mustash: Show networking items in ash system tray

* Initialize chromeos::NetworkHandler during startup
* Include //ui/chromeos strings and assets in the mash target
* Add VPNDelegateMus to SystemTrayDelegateMus because the system tray expects
a non-null VPN delegate during construction
* Add a StubNetworkingConfigDelegate to SystemTrayDelegateMus. This is just
a stub impl because I think the functionality will be rolled into an ash
network information cache in the future.

TODO: Sort out which pieces of NetworkHandler state are controlled by ash vs
browser

BUG=647412, 644355
TEST=manual, chrome --mash shows networking items

Committed: https://crrev.com/40496aad11135e1b863823467e62c4bd956128ae
Cr-Commit-Position: refs/heads/master@{#421881}

[James Cook, 2016-09-28 18:51:01]

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

[commit-bot: I haz the power, 2016-09-28 18:51:17]

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

[James Cook, 2016-09-28 18:52:53]

jamescook@chromium.org changed reviewers:
+ msw@chromium.org

[James Cook, 2016-09-28 18:52:54]

msw, please take a look.

https://codereview.chromium.org/2378193002/diff/1/ash/mus/vpn_delegate_mus.cc
File ash/mus/vpn_delegate_mus.cc (right):

https://codereview.chromium.org/2378193002/diff/1/ash/mus/vpn_delegate_mus.cc...
ash/mus/vpn_delegate_mus.cc:14: // TODO(mash): http://crbug.com/651148
I didn't put NOTIMPLEMENTEDs here because we're pretty spammy at startup
already.

[commit-bot: I haz the power, 2016-09-28 19:19:44]

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

[commit-bot: I haz the power, 2016-09-28 19:19:45]

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

[James Cook, 2016-09-28 20:58:53]

Hold off if you haven't started already. I need to fix non-chromeos builds, and
I'm going to do a bit of shuffling to make that happen.

[James Cook, 2016-09-28 21:27:28]

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

[James Cook, 2016-09-28 21:28:02]

msw: OK, PTAL now

[commit-bot: I haz the power, 2016-09-28 21:28:03]

Dry run: 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-09-28 21:51:27]

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

[commit-bot: I haz the power, 2016-09-28 21:51:27]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[James Cook, 2016-09-28 21:53:42]

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

[commit-bot: I haz the power, 2016-09-28 21:54:00]

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

[msw, 2016-09-28 22:19:54]

lgtm with minor nits

https://codereview.chromium.org/2378193002/diff/1/ash/mus/vpn_delegate_mus.cc
File ash/mus/vpn_delegate_mus.cc (right):

https://codereview.chromium.org/2378193002/diff/1/ash/mus/vpn_delegate_mus.cc...
ash/mus/vpn_delegate_mus.cc:14: // TODO(mash): http://crbug.com/651148
On 2016/09/28 18:52:53, James Cook wrote:
> I didn't put NOTIMPLEMENTEDs here because we're pretty spammy at startup
> already.

You could use #define NOTIMPLEMENTED_POLICY 5 if you just one one message per
run, but it's fine if even that's too much.

https://codereview.chromium.org/2378193002/diff/40001/ash/common/system/netwo...
File ash/common/system/networking_config_delegate.h (right):

https://codereview.chromium.org/2378193002/diff/40001/ash/common/system/netwo...
ash/common/system/networking_config_delegate.h:34: // of the network
|service_path|. If no extension is registered, the
very optional nit: "Returns null if no extension is registered."

https://codereview.chromium.org/2378193002/diff/40001/ash/mus/system_tray_del...
File ash/mus/system_tray_delegate_mus.h (right):

https://codereview.chromium.org/2378193002/diff/40001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.h:22: class VPNDelegate;
optional nit: if the fwd decl suffices, make vpn_delegate_ a member even if
!OS_CHROMEOS, and return its default null value in GetVPNDelegate (just to
eliminate a coulple preprocessor blocks). Otherwise maybe put this in an
OS_CHROMEOS block too.

https://codereview.chromium.org/2378193002/diff/40001/ash/mus/vpn_delegate_mus.h
File ash/mus/vpn_delegate_mus.h (right):

https://codereview.chromium.org/2378193002/diff/40001/ash/mus/vpn_delegate_mu...
ash/mus/vpn_delegate_mus.h:13: // VPN delegate for mustash. Fetches VPN
information from chrome browser over
optional nit: nix 'browser' for a one-liner

[commit-bot: I haz the power, 2016-09-28 22:24:26]

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

[commit-bot: I haz the power, 2016-09-28 22:24:27]

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

[James Cook, 2016-09-29 16:45:39]

https://codereview.chromium.org/2378193002/diff/40001/ash/common/system/netwo...
File ash/common/system/networking_config_delegate.h (right):

https://codereview.chromium.org/2378193002/diff/40001/ash/common/system/netwo...
ash/common/system/networking_config_delegate.h:34: // of the network
|service_path|. If no extension is registered, the
On 2016/09/28 22:19:53, msw wrote:
> very optional nit: "Returns null if no extension is registered."

Done.

https://codereview.chromium.org/2378193002/diff/40001/ash/mus/system_tray_del...
File ash/mus/system_tray_delegate_mus.h (right):

https://codereview.chromium.org/2378193002/diff/40001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.h:22: class VPNDelegate;
On 2016/09/28 22:19:54, msw wrote:
> optional nit: if the fwd decl suffices, make vpn_delegate_ a member even if
> !OS_CHROMEOS, and return its default null value in GetVPNDelegate (just to
> eliminate a coulple preprocessor blocks). Otherwise maybe put this in an
> OS_CHROMEOS block too.

Actually, I went the other way and made this whole file chromeos-only. That's
more like what classic ash does -- the only interesting implementation of
SystemTrayDelegate is SystemTrayDelegateChromeos. Other platforms get
DefaultSystemTrayDelegate.

Also, SystemTrayDelegateMus expects to find a mojo:SystemTrayClient interface
from chrome, but that interface is only provided by chrome on chromeos.

https://codereview.chromium.org/2378193002/diff/40001/ash/mus/vpn_delegate_mus.h
File ash/mus/vpn_delegate_mus.h (right):

https://codereview.chromium.org/2378193002/diff/40001/ash/mus/vpn_delegate_mu...
ash/mus/vpn_delegate_mus.h:13: // VPN delegate for mustash. Fetches VPN
information from chrome browser over
On 2016/09/28 22:19:54, msw wrote:
> optional nit: nix 'browser' for a one-liner

Rewrote to one line. I'm trying to use the word "browser" a lot, or more
specifically the phrase "browser process" to emphasize which process ash is
communicating with.

[James Cook, 2016-09-29 16:45:46]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-09-29 16:45:46]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2378193002/#ps80001
(title: "typo")

[commit-bot: I haz the power, 2016-09-29 16:46:12]

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

[msw, 2016-09-29 16:54:52]

https://codereview.chromium.org/2378193002/diff/80001/ash/mus/window_manager_...
File ash/mus/window_manager_application.cc (right):

https://codereview.chromium.org/2378193002/diff/80001/ash/mus/window_manager_...
ash/mus/window_manager_application.cc:183: // Chrome-with-ash only runs on
Chrome OS, so don't provide the SystemTray
q: What about windows debugging?

[James Cook, 2016-09-29 17:01:52]

The CQ bit was unchecked by jamescook@chromium.org

[James Cook, 2016-09-29 17:05:10]

https://codereview.chromium.org/2378193002/diff/80001/ash/mus/window_manager_...
File ash/mus/window_manager_application.cc (right):

https://codereview.chromium.org/2378193002/diff/80001/ash/mus/window_manager_...
ash/mus/window_manager_application.cc:183: // Chrome-with-ash only runs on
Chrome OS, so don't provide the SystemTray
On 2016/09/29 16:54:52, msw wrote:
> q: What about windows debugging?

You can't debug system tray behavior on Windows today, other than what
DefaultSystemTrayDelegate provides.

If you would prefer, I could go back to lots of ifdefs in SystemTrayDelegateMus.
(I can't do the always-have-a-vpndelegate-member thing, as
~SystemTrayDelegateMus will attempt to destroy it, and I can't ifdef that part
out. So it's fairly ifdef-y. And I'm still pretty sure things will misbehave on
Windows, since SystemTrayClient doesn't exist there.)

Do you want me to hold off on landing this? I took it out of the CQ.

[msw, 2016-09-29 17:26:47]

lgtm

https://codereview.chromium.org/2378193002/diff/80001/ash/mus/window_manager_...
File ash/mus/window_manager_application.cc (right):

https://codereview.chromium.org/2378193002/diff/80001/ash/mus/window_manager_...
ash/mus/window_manager_application.cc:183: // Chrome-with-ash only runs on
Chrome OS, so don't provide the SystemTray
On 2016/09/29 17:05:10, James Cook wrote:
> On 2016/09/29 16:54:52, msw wrote:
> > q: What about windows debugging?
> 
> You can't debug system tray behavior on Windows today, other than what
> DefaultSystemTrayDelegate provides.
> 
> If you would prefer, I could go back to lots of ifdefs in
SystemTrayDelegateMus.
> (I can't do the always-have-a-vpndelegate-member thing, as
> ~SystemTrayDelegateMus will attempt to destroy it, and I can't ifdef that part
> out. So it's fairly ifdef-y. And I'm still pretty sure things will misbehave
on
> Windows, since SystemTrayClient doesn't exist there.)
> 
> Do you want me to hold off on landing this? I took it out of the CQ.

Ah, using DefaultSystemTrayDelegate is fine; I thought this meant that we
wouldn't create any SystemTrayDelegate instance, but I was just wrong.

[James Cook, 2016-09-29 18:31:37]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2016-09-29 18:32:39]

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-09-29 18:39:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-29 18:41:20]

Description was changed from

==========
mustash: Show networking items in ash system tray

* Initialize chromeos::NetworkHandler during startup
* Include //ui/chromeos strings and assets in the mash target
* Add VPNDelegateMus to SystemTrayDelegateMus because the system tray expects
a non-null VPN delegate during construction
* Add a StubNetworkingConfigDelegate to SystemTrayDelegateMus. This is just
a stub impl because I think the functionality will be rolled into an ash
network information cache in the future.

TODO: Sort out which pieces of NetworkHandler state are controlled by ash vs
browser

BUG=647412,644355
TEST=manual, chrome --mash shows networking items
==========

to

==========
mustash: Show networking items in ash system tray

* Initialize chromeos::NetworkHandler during startup
* Include //ui/chromeos strings and assets in the mash target
* Add VPNDelegateMus to SystemTrayDelegateMus because the system tray expects
a non-null VPN delegate during construction
* Add a StubNetworkingConfigDelegate to SystemTrayDelegateMus. This is just
a stub impl because I think the functionality will be rolled into an ash
network information cache in the future.

TODO: Sort out which pieces of NetworkHandler state are controlled by ash vs
browser

BUG=647412,644355
TEST=manual, chrome --mash shows networking items

Committed: https://crrev.com/40496aad11135e1b863823467e62c4bd956128ae
Cr-Commit-Position: refs/heads/master@{#421881}
==========

[commit-bot: I haz the power, 2016-09-29 18:41:21]

Patchset 5 (id:??) landed as
https://crrev.com/40496aad11135e1b863823467e62c4bd956128ae
Cr-Commit-Position: refs/heads/master@{#421881}