mash: Fix system tray clock 12/24 hour time setting

mash: Fix system tray clock 12/24 hour time setting

The 12 vs. 24 hour time preference comes from chrome browser because it
depends on prefs, machine settings, login state, etc.

* Add ash.mojom.SystemTray interface
* Add SystemTrayControllerMus in //chrome which listens for 12/24 pref updates
and sends them to ash over mojo
* Add SystemTrayDelegateMus in //ash/mus which listens for time pref updates
over mojo and routes them to the system tray
* SystemTrayDelegateMus caches the 12/24 preference as it is queried
synchronously elsewhere in ash

BUG=644361
TEST=open settings > advanced settings, toggle 24 hour clock preference, system
tray display updates itself

Committed: https://crrev.com/6763ee8aaac990f173b64a17cf0e29b914fc94ce
Cr-Commit-Position: refs/heads/master@{#419789}

[James Cook, 2016-09-19 22:51:18]

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

[James Cook, 2016-09-19 22:51:37]

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

[James Cook, 2016-09-19 22:51:38]

sky, please take a look.

https://codereview.chromium.org/2351893002/diff/20001/ash/mus/window_manager_...
File ash/mus/window_manager_application.h (right):

https://codereview.chromium.org/2351893002/diff/20001/ash/mus/window_manager_...
ash/mus/window_manager_application.h:57: public
shell::InterfaceFactory<ash::mojom::ShelfController>,
Are all these things intentionally left in the ash:: namespace because of their
mojom? If not, should I remove ash:: throughout this file and the .cc?

https://codereview.chromium.org/2351893002/diff/20001/ash/public/interfaces/s...
File ash/public/interfaces/system_tray.mojom (right):

https://codereview.chromium.org/2351893002/diff/20001/ash/public/interfaces/s...
ash/public/interfaces/system_tray.mojom:8: interface SystemTray {
Do we have a naming convention for these things? msw was using ShelfController
and WallpaperController. I suspect that was because just "Shelf" is generic and
there are shelf-y controller classes in //chrome. But that feels odd to me,
since mojom.ShelfController is implemented by ShelfDelegateMus, not some kind of
ControllerImpl. Should I stick with this, or switch to SystemTrayController for
consistency?

[commit-bot: I haz the power, 2016-09-19 22:51:48]

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-19 22:56:16]

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

[commit-bot: I haz the power, 2016-09-19 22:56:17]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2016-09-19 23:47:52]

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

https://codereview.chromium.org/2351893002/diff/20001/ash/mus/window_manager_...
ash/mus/window_manager_application.cc:181: ash::mojom::SystemTray* system_tray =
static_cast<SystemTrayDelegateMus*>(
This cast makes me nervous. Could you cache the SystemTrayDelegateMus one some
where so you don't need the cast?

https://codereview.chromium.org/2351893002/diff/20001/ash/mus/window_manager_...
File ash/mus/window_manager_application.h (right):

https://codereview.chromium.org/2351893002/diff/20001/ash/mus/window_manager_...
ash/mus/window_manager_application.h:57: public
shell::InterfaceFactory<ash::mojom::ShelfController>,
On 2016/09/19 22:51:38, James Cook wrote:
> Are all these things intentionally left in the ash:: namespace because of
their
> mojom? If not, should I remove ash:: throughout this file and the .cc?

If you mean that ash:: is redundant with the namespace of this file and can be
removed, go for it. As long as it compiles SGTM. It may be a holdover from when
this code was in the mash namespace.

https://codereview.chromium.org/2351893002/diff/20001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2351893002/diff/20001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:1381: if (is_chromeos) {
Move to is_chromeos on 1188?

[James Cook, 2016-09-20 01:50:27]

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

[commit-bot: I haz the power, 2016-09-20 01:50:44]

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-20 01:52:11]

sky, please take another look.

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

https://codereview.chromium.org/2351893002/diff/20001/ash/mus/window_manager_...
ash/mus/window_manager_application.cc:181: ash::mojom::SystemTray* system_tray =
static_cast<SystemTrayDelegateMus*>(
On 2016/09/19 23:47:52, sky wrote:
> This cast makes me nervous. Could you cache the SystemTrayDelegateMus one some
> where so you don't need the cast?

How's this? I looked at caching the pointer, but it's pretty awkward.
ShellDelegateMus creates it. But ShellDelegateMus is stored as-a ShellDelegate
in WmShell. So I'd have to cache a ShellDelegateMus* in WmShellMus. WmShellMus's
constructor looks like this:

WmShellMus::WmShellMus(unique_ptr<ShellDelegate> delegate)
  : WmShell(std::move(delegate)) {}

I could change |delegate| to ShellDelegateMus* and cache it and take ownership,
but then we lose unique_ptr as a sign of ownership transfer. Or I could pass
both a unique_ptr and a raw pointer, which is just bizarre. Or I could cast
WmShell::delegate() to a ShellDelegateMus... which is a static_cast<> again.
Yuck.

https://codereview.chromium.org/2351893002/diff/20001/ash/public/interfaces/s...
File ash/public/interfaces/system_tray.mojom (right):

https://codereview.chromium.org/2351893002/diff/20001/ash/public/interfaces/s...
ash/public/interfaces/system_tray.mojom:8: interface SystemTray {
On 2016/09/19 22:51:38, James Cook wrote:
> Do we have a naming convention for these things? msw was using ShelfController
> and WallpaperController. I suspect that was because just "Shelf" is generic
and
> there are shelf-y controller classes in //chrome. But that feels odd to me,
> since mojom.ShelfController is implemented by ShelfDelegateMus, not some kind
of
> ControllerImpl. Should I stick with this, or switch to SystemTrayController
for
> consistency?

Ping?

https://codereview.chromium.org/2351893002/diff/20001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2351893002/diff/20001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:1381: if (is_chromeos) {
On 2016/09/19 23:47:52, sky wrote:
> Move to is_chromeos on 1188?

Done.

[sky, 2016-09-20 02:45:17]

LGTM

https://codereview.chromium.org/2351893002/diff/20001/ash/public/interfaces/s...
File ash/public/interfaces/system_tray.mojom (right):

https://codereview.chromium.org/2351893002/diff/20001/ash/public/interfaces/s...
ash/public/interfaces/system_tray.mojom:8: interface SystemTray {
On 2016/09/20 01:52:11, James Cook wrote:
> On 2016/09/19 22:51:38, James Cook wrote:
> > Do we have a naming convention for these things? msw was using
ShelfController
> > and WallpaperController. I suspect that was because just "Shelf" is generic
> and
> > there are shelf-y controller classes in //chrome. But that feels odd to me,
> > since mojom.ShelfController is implemented by ShelfDelegateMus, not some
kind
> of
> > ControllerImpl. Should I stick with this, or switch to SystemTrayController
> for
> > consistency?
> 
> Ping?

Sorry, somehow missed your question. One naming convention we've gone with is
Foo and FooClient. You can see that in WindowTree and WindowTreeClient. As you
pointed out we aren't particularly consistent in ash code yet. I don't feel
strongly here. I'm fine with SystemTray.

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

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

[commit-bot: I haz the power, 2016-09-20 02:45:20]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-09-20 16:07:14]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2016-09-20 16:07:34]

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-20 16:16:04]

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

[commit-bot: I haz the power, 2016-09-20 16:16:05]

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

[James Cook, 2016-09-20 16:29:09]

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

[James Cook, 2016-09-20 16:29:10]

tsepez, can I get mojom security OWNERS for
ash/public/interfaces/system_tray.mojom ?

[Tom Sepez, 2016-09-20 16:34:42]

mojom LGTM.

https://codereview.chromium.org/2351893002/diff/40001/ash/public/interfaces/s...
File ash/public/interfaces/system_tray.mojom (right):

https://codereview.chromium.org/2351893002/diff/40001/ash/public/interfaces/s...
ash/public/interfaces/system_tray.mojom:11: SetUse24HourClock(bool use_24_hour);
nit: do you want an enum in place of a bool?  Would there ever be some other
possibility?

[James Cook, 2016-09-20 16:42:57]

https://codereview.chromium.org/2351893002/diff/40001/ash/public/interfaces/s...
File ash/public/interfaces/system_tray.mojom (right):

https://codereview.chromium.org/2351893002/diff/40001/ash/public/interfaces/s...
ash/public/interfaces/system_tray.mojom:11: SetUse24HourClock(bool use_24_hour);
On 2016/09/20 16:34:42, Tom Sepez wrote:
> nit: do you want an enum in place of a bool?  Would there ever be some other
> possibility?

I looked at using an enum, but in this case the only setting is 12 vs. 24, and
that's what most of the existing C++ methods use, so I went with a bool.

I you feel strongly that I should use an enum ping me and I'll change it in a
followup CL.

[James Cook, 2016-09-20 16:43:03]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2016-09-20 16:43:25]

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-20 16:48:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-20 16:50:39]

Description was changed from

==========
mash: Fix system tray clock 12/24 hour time setting

The 12 vs. 24 hour time preference comes from chrome browser because it
depends on prefs, machine settings, login state, etc.

* Add ash.mojom.SystemTray interface
* Add SystemTrayControllerMus in //chrome which listens for 12/24 pref updates
and sends them to ash over mojo
* Add SystemTrayDelegateMus in //ash/mus which listens for time pref updates
over mojo and routes them to the system tray
* SystemTrayDelegateMus caches the 12/24 preference as it is queried
synchronously elsewhere in ash

BUG=644361
TEST=open settings > advanced settings, toggle 24 hour clock preference, system
tray display updates itself
==========

to

==========
mash: Fix system tray clock 12/24 hour time setting

The 12 vs. 24 hour time preference comes from chrome browser because it
depends on prefs, machine settings, login state, etc.

* Add ash.mojom.SystemTray interface
* Add SystemTrayControllerMus in //chrome which listens for 12/24 pref updates
and sends them to ash over mojo
* Add SystemTrayDelegateMus in //ash/mus which listens for time pref updates
over mojo and routes them to the system tray
* SystemTrayDelegateMus caches the 12/24 preference as it is queried
synchronously elsewhere in ash

BUG=644361
TEST=open settings > advanced settings, toggle 24 hour clock preference, system
tray display updates itself

Committed: https://crrev.com/6763ee8aaac990f173b64a17cf0e29b914fc94ce
Cr-Commit-Position: refs/heads/master@{#419789}
==========

[commit-bot: I haz the power, 2016-09-20 16:50:41]

Patchset 3 (id:??) landed as
https://crrev.com/6763ee8aaac990f173b64a17cf0e29b914fc94ce
Cr-Commit-Position: refs/heads/master@{#419789}