Use mojo SystemTray interfaces for both mash and classic ash

Use mojo SystemTray interfaces for both mash and classic ash

The goal is to stop having to maintain separate SystemTrayDelegateMus and
SystemTrayDelegateChromeos implementations and eliminate the indirection
introduced by SystemTrayCommon.

* Always create the SystemTrayClient object in chrome, whether running ash
in-process or not
* Always allow code in //ash to bind to mojom::SystemTrayClient, even when ash
runs in-process
* Introduce WmShell::IsRunningInMash so ash can tell which mode its in
* Handle connecting to "exe:chrome" vs. "exe:content_browser" based on the
process mode
* Introduces SystemTrayController in ash, which wraps mojom::SystemTrayClient
* Peel two methods off the SystemTrayDelegate interface and changes them
to call into SystemTrayController and route over mojo
* Remove mojo usage from SystemTrayDelegateMus. Eventually it should go away
completely.

To keep this CL small it breaks a few system tray features when running under
mash (e.g. opening the general settings). This will get fixed in the next CL.

TODO: Remove SystemTrayCommon and move its functionality into SystemTrayClient
TODO: For methods like SystemTrayDelegate::ShowSettings, migrate callers to
use ash::SystemTrayController and always route over mojo.

BUG=647412
TEST=manual, clicking on system tray date opens date settings, changing time
from 12 to 24 hour works, in both classic ash and mash

Committed: https://crrev.com/2a6b869d3921b816777679003d1df24f6c7b84fa
Cr-Commit-Position: refs/heads/master@{#422676}

[James Cook, 2016-09-29 00:53:31]

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

[James Cook, 2016-09-29 00:53:31]

sky/msw - this is not for formal review, but I'd appreciate it if you could take
a look before our chat tomorrow.

This is one possible approach to converting ash -> chrome delegates.

Old approach had two call paths:
1. Ash code -> SystemTrayDelegate / SystemTrayDelegateMus -> mojo ->
SystemTrayClient -> SystemTrayCommon
2. Ash code -> SystemTrayDelegate / SystemTrayDelegateChromeOS ->
SystemTrayCommon

New approach has only one path:
Ash code -> SystemTrayController -> mojo -> SystemTrayClient

Pros:
A. Less indirection
B. Only one call path
C. Forces engineers adding new functionality to consider mojo -- there's no easy
way to leave a mus-only stub/notimplemented.

Cons:
X. Fairly different than the existing delegates. More code change in //ash to
move from delegate to SystemTrayController.
Y. Not sure what to do about tests. Not easy to make a stub SystemTrayController
like it was for SystemTrayDelegate. Not sure what happens if an ash unittest
calls a method on the mojo SystemTray interface and there isn't one.
Z. C above is also a con. :-)

[sky, 2016-09-29 16:34:05]

I like it! Here are a couple of comments.

https://codereview.chromium.org/2381753002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_controller.cc (right):

https://codereview.chromium.org/2381753002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.cc:17: // TODO: In general, how do
we get initial values?
This is a similar problem with the keyboard, where you need to know state right
away. If you need state it implies you have to connect in the constructor.

https://codereview.chromium.org/2381753002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.cc:47:
connector_->ConnectToInterface("exe:chrome", &system_tray_client_);
The name of this may need to be exe:content_browser for the classic ash case.
Having to use two different names is rather confusing though. Make we should see
about renaming exe:chrome to exe:content_browser.

https://codereview.chromium.org/2381753002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.cc:65:
FOR_EACH_OBSERVER(ClockObserver, clock_observers_, OnDateFormatChanged());
If you're expecting the client to always call this, then it seems entirely
possible the current value matches that from the client. In other words should
this verify hour_clock_type_ actually changed before calling observers?

https://codereview.chromium.org/2381753002/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2381753002/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2992: if
(chrome::IsRunningInMash()) {
I don't think there is a reason this needs to be conditional. In other words if
you always add the filter it should just work (but see my other comment about
connection name being different).

[James Cook, 2016-09-29 16:43:33]

https://codereview.chromium.org/2381753002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_controller.cc (right):

https://codereview.chromium.org/2381753002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.cc:47:
connector_->ConnectToInterface("exe:chrome", &system_tray_client_);
On 2016/09/29 16:34:05, sky wrote:
> The name of this may need to be exe:content_browser for the classic ash case.
> Having to use two different names is rather confusing though. Make we should
see
> about renaming exe:chrome to exe:content_browser.

Note for msw: We also need to think about reconnections when running ash_shell
on Windows. In that case chrome may not be compiled/available, and if it comes
up it may not provide the SystemTrayClient  interface. This would result in a
connection error. Then this code would try to reconnect. Whoops. :-)

For both of you: I still want to be able to tolerate chrome crashing and coming
back up, though. Maybe we just don't retry on non-chromeos platforms? Or provide
a stub SystemTrayClient impl for Chrome on Windows?

[James Cook, 2016-09-30 20:50:31]

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

[commit-bot: I haz the power, 2016-09-30 20:51:12]

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-30 21:00:59]

Description was changed from

==========
WIP: Always use SystemTray / SystemTrayClient interfaces for mash

* Introduces SystemTrayController in ash to be the mojom::SystemTray
implementation and to cache state that ash needs to query synchronously
* Eliminates SystemTrayDelegateMus
* As methods are ported to mojo they are removed from SystemTrayDelegate
and added to SystemTrayController.
* In chrome, methods are moved from SystemTrayDelegateChromeos to
SystemTrayClient.
* There is no longer a need for SystemTrayCommon in chrome.

This would require always creating the SystemTrayClient object in chrome
(easy) and always allowing code in ash to bind to it (even if running in
process). I'm not sure how to do the latter.

BUG=
==========

to

==========
Use SystemTray / SystemTrayClient interfaces for mash and classic ash

The goal is to stop having to maintain separate SystemTrayDelegateMus and
SystemTrayDelegateChromeos implementations and eliminate the indirection
introduced by SystemTrayCommon.

* Always create the SystemTrayClient object in chrome, whether running ash
in-process or not
* Always allow code in //ash to bind to mojom::SystemTrayClient, even when ash
runs in-process
* Introduce WmShell::IsRunningInMash so ash can tell which mode its in
* Handle connecting to "exe:chrome" vs. "exe:content_browser" based on the
process mode
* Introduces SystemTrayController in ash, which wraps mojom::SystemTrayClient
* Peel two methods off the SystemTrayDelegate interface and changes them
to call into SystemTrayController and route over mojo
* Remove mojo usage from SystemTrayDelegateMus. Eventually it should go away
completely.

To keep this CL small it breaks a few system tray features when running under
mash (e.g. opening the general settings). This will get fixed in the next CL.

TODO: Remove SystemTrayCommon and move its functionality into SystemTrayClient
TODO: For methods like SystemTrayDelegate::ShowSettings, migrate callers to
use ash::SystemTrayController and always route over mojo.

BUG=647412
TEST=manual, clicking on system tray date opens date settings, changing time
from 12 to 24 hour works, in both classic ash and mash
==========

[James Cook, 2016-09-30 21:01:26]

Description was changed from

==========
Use SystemTray / SystemTrayClient interfaces for mash and classic ash

The goal is to stop having to maintain separate SystemTrayDelegateMus and
SystemTrayDelegateChromeos implementations and eliminate the indirection
introduced by SystemTrayCommon.

* Always create the SystemTrayClient object in chrome, whether running ash
in-process or not
* Always allow code in //ash to bind to mojom::SystemTrayClient, even when ash
runs in-process
* Introduce WmShell::IsRunningInMash so ash can tell which mode its in
* Handle connecting to "exe:chrome" vs. "exe:content_browser" based on the
process mode
* Introduces SystemTrayController in ash, which wraps mojom::SystemTrayClient
* Peel two methods off the SystemTrayDelegate interface and changes them
to call into SystemTrayController and route over mojo
* Remove mojo usage from SystemTrayDelegateMus. Eventually it should go away
completely.

To keep this CL small it breaks a few system tray features when running under
mash (e.g. opening the general settings). This will get fixed in the next CL.

TODO: Remove SystemTrayCommon and move its functionality into SystemTrayClient
TODO: For methods like SystemTrayDelegate::ShowSettings, migrate callers to
use ash::SystemTrayController and always route over mojo.

BUG=647412
TEST=manual, clicking on system tray date opens date settings, changing time
from 12 to 24 hour works, in both classic ash and mash
==========

to

==========
Use SystemTray / SystemTrayClient interfaces for mash and classic ash

The goal is to stop having to maintain separate SystemTrayDelegateMus and
SystemTrayDelegateChromeos implementations and eliminate the indirection
introduced by SystemTrayCommon.

* Always create the SystemTrayClient object in chrome, whether running ash
in-process or not
* Always allow code in //ash to bind to mojom::SystemTrayClient, even when ash
runs in-process
* Introduce WmShell::IsRunningInMash so ash can tell which mode its in
* Handle connecting to "exe:chrome" vs. "exe:content_browser" based on the
process mode
* Introduces SystemTrayController in ash, which wraps mojom::SystemTrayClient
* Peel two methods off the SystemTrayDelegate interface and changes them
to call into SystemTrayController and route over mojo
* Remove mojo usage from SystemTrayDelegateMus. Eventually it should go away
completely.

To keep this CL small it breaks a few system tray features when running under
mash (e.g. opening the general settings). This will get fixed in the next CL.

TODO: Remove SystemTrayCommon and move its functionality into SystemTrayClient
TODO: For methods like SystemTrayDelegate::ShowSettings, migrate callers to
use ash::SystemTrayController and always route over mojo.

BUG=647412
TEST=manual, clicking on system tray date opens date settings, changing time
from 12 to 24 hour works, in both classic ash and mash
==========

[James Cook, 2016-09-30 21:01:26]

jamescook@chromium.org changed reviewers:
- msw@chromium.org, sky@chromium.org

[James Cook, 2016-09-30 21:01:53]

Description was changed from

==========
Use SystemTray / SystemTrayClient interfaces for mash and classic ash

The goal is to stop having to maintain separate SystemTrayDelegateMus and
SystemTrayDelegateChromeos implementations and eliminate the indirection
introduced by SystemTrayCommon.

* Always create the SystemTrayClient object in chrome, whether running ash
in-process or not
* Always allow code in //ash to bind to mojom::SystemTrayClient, even when ash
runs in-process
* Introduce WmShell::IsRunningInMash so ash can tell which mode its in
* Handle connecting to "exe:chrome" vs. "exe:content_browser" based on the
process mode
* Introduces SystemTrayController in ash, which wraps mojom::SystemTrayClient
* Peel two methods off the SystemTrayDelegate interface and changes them
to call into SystemTrayController and route over mojo
* Remove mojo usage from SystemTrayDelegateMus. Eventually it should go away
completely.

To keep this CL small it breaks a few system tray features when running under
mash (e.g. opening the general settings). This will get fixed in the next CL.

TODO: Remove SystemTrayCommon and move its functionality into SystemTrayClient
TODO: For methods like SystemTrayDelegate::ShowSettings, migrate callers to
use ash::SystemTrayController and always route over mojo.

BUG=647412
TEST=manual, clicking on system tray date opens date settings, changing time
from 12 to 24 hour works, in both classic ash and mash
==========

to

==========
Use mojo SystemTray interfaces for both mash and classic ash

The goal is to stop having to maintain separate SystemTrayDelegateMus and
SystemTrayDelegateChromeos implementations and eliminate the indirection
introduced by SystemTrayCommon.

* Always create the SystemTrayClient object in chrome, whether running ash
in-process or not
* Always allow code in //ash to bind to mojom::SystemTrayClient, even when ash
runs in-process
* Introduce WmShell::IsRunningInMash so ash can tell which mode its in
* Handle connecting to "exe:chrome" vs. "exe:content_browser" based on the
process mode
* Introduces SystemTrayController in ash, which wraps mojom::SystemTrayClient
* Peel two methods off the SystemTrayDelegate interface and changes them
to call into SystemTrayController and route over mojo
* Remove mojo usage from SystemTrayDelegateMus. Eventually it should go away
completely.

To keep this CL small it breaks a few system tray features when running under
mash (e.g. opening the general settings). This will get fixed in the next CL.

TODO: Remove SystemTrayCommon and move its functionality into SystemTrayClient
TODO: For methods like SystemTrayDelegate::ShowSettings, migrate callers to
use ash::SystemTrayController and always route over mojo.

BUG=647412
TEST=manual, clicking on system tray date opens date settings, changing time
from 12 to 24 hour works, in both classic ash and mash
==========

[James Cook, 2016-09-30 21:18:25]

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

[James Cook, 2016-09-30 21:18:27]

msw, please take a look, especially at the SystemTrayDelegate /
SystemTrayController / SystemTrayClient pieces and the non-mojo bits in DateView
and SystemTray.

rockot, please take a look too, especially at the manifest files,
MojoInterfaceFactory, and the initialization bits in ChromeContentBrowserClient
and ChromeBrowserExtraMainPartsAsh.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/mojo_interfa...
File ash/common/mojo_interface_factory.h (right):

https://codereview.chromium.org/2381753002/diff/60001/ash/common/mojo_interfa...
ash/common/mojo_interface_factory.h:22: class ASH_EXPORT MojoInterfaceFactory {
I could also add namespace mojo_interface_factory and have a bare function
ash::mojo_interface_factory::RegisterInterfaces, but I went with this because
it's similar to ChromeInterfaceFactory.

I don't want to have a bare function in the ash namespace because I find that
hard to read at the call site.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/date/...
File ash/common/system/date/date_view.cc (right):

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/date/...
ash/common/system/date/date_view.cc:194:
WmShell::Get()->system_tray_delegate()->ShowSetTimeDialog();
msw, this and many other system_tray_delegate()->ShowFoo() calls will migrate in
a follow-up CL.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_controller.cc (right):

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.cc:27: if (system_tray_client_)
Explicitly checking for system_tray_client_ instead of trying to do
ConnectToSystemTrayClient()->DoFoo() allows me to do no-ops on Windows and in
tests.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.cc:37: // Chrome OS, so don't try
to connect on other platforms.
clang-format left-justifies this.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_controller.h (right):

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.h:41: 
The other ShowFoo() methods will end up here.

[commit-bot: I haz the power, 2016-09-30 21:20:38]

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

[commit-bot: I haz the power, 2016-09-30 21:20:39]

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_...)

[msw, 2016-09-30 23:26:50]

Brilliant! I really like this pattern. Mostly minor nits.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/mojo_interfa...
File ash/common/mojo_interface_factory.h (right):

https://codereview.chromium.org/2381753002/diff/60001/ash/common/mojo_interfa...
ash/common/mojo_interface_factory.h:22: class ASH_EXPORT MojoInterfaceFactory {
On 2016/09/30 21:18:26, James Cook wrote:
> I could also add namespace mojo_interface_factory and have a bare function
> ash::mojo_interface_factory::RegisterInterfaces, but I went with this because
> it's similar to ChromeInterfaceFactory.
> 
> I don't want to have a bare function in the ash namespace because I find that
> hard to read at the call site.

This seems fine to me.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/date/...
File ash/common/system/date/date_view.cc (right):

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/date/...
ash/common/system/date/date_view.cc:194:
WmShell::Get()->system_tray_delegate()->ShowSetTimeDialog();
On 2016/09/30 21:18:26, James Cook wrote:
> msw, this and many other system_tray_delegate()->ShowFoo() calls will migrate
in
> a follow-up CL.

Acknowledged.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_controller.cc (right):

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.cc:11: #include
"base/i18n/time_formatting.h"
nit: not needed (included in header)

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.cc:21: base::HourClockType
SystemTrayController::GetHourClockType() const {
optional nit: make this a simple accessor; inline in the header?

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.cc:27: if (system_tray_client_)
On 2016/09/30 21:18:26, James Cook wrote:
> Explicitly checking for system_tray_client_ instead of trying to do
> ConnectToSystemTrayClient()->DoFoo() allows me to do no-ops on Windows and in
> tests.

Acknowledged. Though ConnectToSystemTrayClient could return a bool:
  if (ConnectToSystemTrayClient())
    system_tray_client_->ShowDateSettings();

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.cc:37: // Chrome OS, so don't try
to connect on other platforms.
On 2016/09/30 21:18:27, James Cook wrote:
> clang-format left-justifies this.

Acknowledged.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_controller.h (right):

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.h:11: #include
"base/observer_list.h"
nit: remove

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.h:20: class ClockObserver;
nit: remove

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.h:22: // Wrapper around the
mojom::SystemTrayClient interface that makes the initial
nit: mention that it implements mojom::SystemTray?

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.h:31: // TODO: Consider renaming
this to SystemTrayClient or renaming the current
I definitely favor the latter option :)

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.h:41: 
On 2016/09/30 21:18:27, James Cook wrote:
> The other ShowFoo() methods will end up here.

Acknowledged.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.h:65: // 12 or 24 hour display.
optional nit: The type of hour display: 12 or 24 hours.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2381753002/diff/60001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:181: // Returns true when ash is running in its own
process.
nit: s/process/mojo application/? (might still be same process IIUC)

https://codereview.chromium.org/2381753002/diff/60001/ash/mus/system_tray_del...
File ash/mus/system_tray_delegate_mus.cc (right):

https://codereview.chromium.org/2381753002/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:8: #include
"ash/common/system/tray/system_tray_notifier.h"
nit: remove

https://codereview.chromium.org/2381753002/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:9: #include "ash/common/wm_shell.h"
nit: remove

https://codereview.chromium.org/2381753002/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:11: #include "base/bind.h"
nit: remove

https://codereview.chromium.org/2381753002/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:12: #include "base/bind_helpers.h"
nit: remove

https://codereview.chromium.org/2381753002/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:13: #include "base/i18n/time_formatting.h"
nit: remove

https://codereview.chromium.org/2381753002/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:14: #include "base/logging.h"
nit: remove

https://codereview.chromium.org/2381753002/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:15: #include
"services/shell/public/cpp/connector.h"
nit: remove

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

https://codereview.chromium.org/2381753002/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.h:11: #include
"ash/public/interfaces/system_tray.mojom.h"
nit: remove

https://codereview.chromium.org/2381753002/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.h:12: #include "base/i18n/time_formatting.h"
nit: remove

https://codereview.chromium.org/2381753002/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.h:16: class Connector;
nit: remove

https://codereview.chromium.org/2381753002/diff/60001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2381753002/diff/60001/ash/mus/window_manager....
ash/mus/window_manager.cc:79: shell_.reset(new
WmShellMus(base::MakeUnique<ShellDelegateMus>(connector_),
q: Would it make sense for the shell delegate to provide an accessor to its
connector, since it's the owner? Then shell init can just access it that way.
It's no big deal either way, especially if that complicates ash::Shell init.

https://codereview.chromium.org/2381753002/diff/60001/chrome/app/mojo/chrome_...
File chrome/app/mojo/chrome_manifest.json (right):

https://codereview.chromium.org/2381753002/diff/60001/chrome/app/mojo/chrome_...
chrome/app/mojo/chrome_manifest.json:23: "ash": [
It might be nice to avoid using classes; but maybe that's just my preference (I
prefer that clients call out the explicit interfaces needed, rather than an
alias to a set of interfaces)

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

https://codereview.chromium.org/2381753002/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:28: #include
"chrome/browser/chromeos/system/system_clock.h"
nit: remove

https://codereview.chromium.org/2381753002/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:29: #include
"chrome/browser/chromeos/system/system_clock_observer.h"
nit: remove

https://codereview.chromium.org/2381753002/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:162: bool
GetShouldUse24HourClockForTesting() const;
nit: can we remove this? (make tests call a new SystemTrayClient accessor?)

https://codereview.chromium.org/2381753002/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:173: bool
ShouldUse24HourClock() const;
nit: can we remove this? (is this even defined anywhere?)

https://codereview.chromium.org/2381753002/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:285: base::HourClockType
clock_type_;
nit: remove

https://codereview.chromium.org/2381753002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (left):

https://codereview.chromium.org/2381753002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:55: #if
defined(OS_CHROMEOS)
nice :)

[James Cook, 2016-10-03 18:07:42]

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

[James Cook, 2016-10-03 18:07:56]

rockot, ping?

msw, please take another look.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_controller.cc (right):

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.cc:11: #include
"base/i18n/time_formatting.h"
On 2016/09/30 23:26:48, msw wrote:
> nit: not needed (included in header)

Done.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.cc:21: base::HourClockType
SystemTrayController::GetHourClockType() const {
On 2016/09/30 23:26:48, msw wrote:
> optional nit: make this a simple accessor; inline in the header?

Done.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.cc:27: if (system_tray_client_)
On 2016/09/30 23:26:48, msw wrote:
> On 2016/09/30 21:18:26, James Cook wrote:
> > Explicitly checking for system_tray_client_ instead of trying to do
> > ConnectToSystemTrayClient()->DoFoo() allows me to do no-ops on Windows and
in
> > tests.
> 
> Acknowledged. Though ConnectToSystemTrayClient could return a bool:
>   if (ConnectToSystemTrayClient())
>     system_tray_client_->ShowDateSettings();

Done.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_controller.h (right):

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.h:11: #include
"base/observer_list.h"
On 2016/09/30 23:26:49, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.h:20: class ClockObserver;
On 2016/09/30 23:26:49, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.h:22: // Wrapper around the
mojom::SystemTrayClient interface that makes the initial
On 2016/09/30 23:26:48, msw wrote:
> nit: mention that it implements mojom::SystemTray?

Rewrote.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.h:65: // 12 or 24 hour display.
On 2016/09/30 23:26:49, msw wrote:
> optional nit: The type of hour display: 12 or 24 hours.

Done.

https://codereview.chromium.org/2381753002/diff/60001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2381753002/diff/60001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:181: // Returns true when ash is running in its own
process.
On 2016/09/30 23:26:49, msw wrote:
> nit: s/process/mojo application/? (might still be same process IIUC)

Done.

https://codereview.chromium.org/2381753002/diff/60001/ash/mus/system_tray_del...
File ash/mus/system_tray_delegate_mus.cc (right):

https://codereview.chromium.org/2381753002/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:8: #include
"ash/common/system/tray/system_tray_notifier.h"
On 2016/09/30 23:26:49, msw wrote:
> nit: remove

Done, here and below. Geez, I missed a lot of clean up. :-P  Sorry 'bout that.

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

https://codereview.chromium.org/2381753002/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.h:11: #include
"ash/public/interfaces/system_tray.mojom.h"
On 2016/09/30 23:26:49, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2381753002/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.h:12: #include "base/i18n/time_formatting.h"
On 2016/09/30 23:26:49, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2381753002/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.h:16: class Connector;
On 2016/09/30 23:26:49, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2381753002/diff/60001/chrome/app/mojo/chrome_...
File chrome/app/mojo/chrome_manifest.json (right):

https://codereview.chromium.org/2381753002/diff/60001/chrome/app/mojo/chrome_...
chrome/app/mojo/chrome_manifest.json:23: "ash": [
On 2016/09/30 23:26:49, msw wrote:
> It might be nice to avoid using classes; but maybe that's just my preference
(I
> prefer that clients call out the explicit interfaces needed, rather than an
> alias to a set of interfaces)

Me too, but rockot recommended I do it this way. (Don't you have to put provided
interfaces in classes?) rockot, thoughts?

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

https://codereview.chromium.org/2381753002/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:28: #include
"chrome/browser/chromeos/system/system_clock.h"
On 2016/09/30 23:26:49, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2381753002/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:29: #include
"chrome/browser/chromeos/system/system_clock_observer.h"
On 2016/09/30 23:26:49, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2381753002/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:162: bool
GetShouldUse24HourClockForTesting() const;
On 2016/09/30 23:26:49, msw wrote:
> nit: can we remove this? (make tests call a new SystemTrayClient accessor?)

I inlined the implementation in the one test that needed it.

https://codereview.chromium.org/2381753002/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:173: bool
ShouldUse24HourClock() const;
On 2016/09/30 23:26:49, msw wrote:
> nit: can we remove this? (is this even defined anywhere?)

Done.

https://codereview.chromium.org/2381753002/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:285: base::HourClockType
clock_type_;
On 2016/09/30 23:26:49, msw wrote:
> nit: remove

Done.

[commit-bot: I haz the power, 2016-10-03 18:08:18]

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

[Ken Rockot(use gerrit already), 2016-10-03 18:19:03]

Manifests, browser client code, MojoInterfaceFactory, and (rough review of
everything else) all LGTM

https://codereview.chromium.org/2381753002/diff/80001/ash/common/mojo_interfa...
File ash/common/mojo_interface_factory.h (right):

https://codereview.chromium.org/2381753002/diff/80001/ash/common/mojo_interfa...
ash/common/mojo_interface_factory.h:22: class ASH_EXPORT MojoInterfaceFactory {
optional nit: Style guide says don't introduce classes which only have static
methods, but to instead just use namespaced functions. I personally don't see
the value of that rule, but you could optionally move this to something like
ash::RegisterMojoInterfaces.

https://codereview.chromium.org/2381753002/diff/80001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_controller.h (right):

https://codereview.chromium.org/2381753002/diff/80001/ash/common/system/tray/...
ash/common/system/tray/system_tray_controller.h:42: void
BindRequest(mojom::SystemTrayRequest request);
Then again... if you keep MojoInterfaceFactory as a class, you could make it a
friend of SystemTrayController and hide this method.

[msw, 2016-10-03 19:04:43]

lgtm

https://codereview.chromium.org/2381753002/diff/60001/chrome/app/mojo/chrome_...
File chrome/app/mojo/chrome_manifest.json (right):

https://codereview.chromium.org/2381753002/diff/60001/chrome/app/mojo/chrome_...
chrome/app/mojo/chrome_manifest.json:23: "ash": [
On 2016/10/03 18:07:56, James Cook wrote:
> On 2016/09/30 23:26:49, msw wrote:
> > It might be nice to avoid using classes; but maybe that's just my preference
> (I
> > prefer that clients call out the explicit interfaces needed, rather than an
> > alias to a set of interfaces)
> 
> Me too, but rockot recommended I do it this way. (Don't you have to put
provided
> interfaces in classes?) rockot, thoughts?

No, you don't *need* to put provided interfaces in classes, afaik. It'd be good
to get one more look here from rockot@, but it's not a big deal.

[commit-bot: I haz the power, 2016-10-03 19:22:49]

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

[commit-bot: I haz the power, 2016-10-03 19:22:50]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[James Cook, 2016-10-03 20:14:59]

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

[James Cook, 2016-10-03 20:15:00]

tsepez, can I get security review for the *manifest.json files?  (And should
//chrome/app/mojo have an OWNERS file with security reviewers? It doesn't have
one.)

rockot, thoughts on msw's comment?

https://codereview.chromium.org/2381753002/diff/60001/chrome/app/mojo/chrome_...
File chrome/app/mojo/chrome_manifest.json (right):

https://codereview.chromium.org/2381753002/diff/60001/chrome/app/mojo/chrome_...
chrome/app/mojo/chrome_manifest.json:23: "ash": [
On 2016/10/03 19:04:43, msw wrote:
> On 2016/10/03 18:07:56, James Cook wrote:
> > On 2016/09/30 23:26:49, msw wrote:
> > > It might be nice to avoid using classes; but maybe that's just my
preference
> > (I
> > > prefer that clients call out the explicit interfaces needed, rather than
an
> > > alias to a set of interfaces)
> > 
> > Me too, but rockot recommended I do it this way. (Don't you have to put
> provided
> > interfaces in classes?) rockot, thoughts?
> 
> No, you don't *need* to put provided interfaces in classes, afaik. It'd be
good
> to get one more look here from rockot@, but it's not a big deal.

rockot, thoughts?

https://codereview.chromium.org/2381753002/diff/80001/ash/common/mojo_interfa...
File ash/common/mojo_interface_factory.h (right):

https://codereview.chromium.org/2381753002/diff/80001/ash/common/mojo_interfa...
ash/common/mojo_interface_factory.h:22: class ASH_EXPORT MojoInterfaceFactory {
On 2016/10/03 18:19:03, Ken Rockot wrote:
> optional nit: Style guide says don't introduce classes which only have static
> methods, but to instead just use namespaced functions. I personally don't see
> the value of that rule, but you could optionally move this to something like
> ash::RegisterMojoInterfaces.

Changed to use namespaced functions.

[James Cook, 2016-10-03 20:16:06]

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

[commit-bot: I haz the power, 2016-10-03 20:16:33]

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

[Tom Sepez, 2016-10-03 21:48:48]

JSON lgtm; would you be kind enough to add the OWNERS file?

[James Cook, 2016-10-03 23:08:24]

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

[commit-bot: I haz the power, 2016-10-03 23:09:06]

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-10-03 23:09:36]

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

[James Cook, 2016-10-03 23:09:37]

sky, can I get an OWNERS stamp for chrome/app/mojo/OWNERS and
c/b/chrome_content_browser_client.cc ?

[commit-bot: I haz the power, 2016-10-03 23:13:44]

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

[commit-bot: I haz the power, 2016-10-03 23:13:45]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  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_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  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_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_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sky, 2016-10-03 23:29:01]

LGTM

[James Cook, 2016-10-03 23:42:05]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-10-03 23:42:06]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
msw@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2381753002/#ps160001
(title: "rebase again")

[commit-bot: I haz the power, 2016-10-03 23:42:53]

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-04 02:41:51]

Description was changed from

==========
Use mojo SystemTray interfaces for both mash and classic ash

The goal is to stop having to maintain separate SystemTrayDelegateMus and
SystemTrayDelegateChromeos implementations and eliminate the indirection
introduced by SystemTrayCommon.

* Always create the SystemTrayClient object in chrome, whether running ash
in-process or not
* Always allow code in //ash to bind to mojom::SystemTrayClient, even when ash
runs in-process
* Introduce WmShell::IsRunningInMash so ash can tell which mode its in
* Handle connecting to "exe:chrome" vs. "exe:content_browser" based on the
process mode
* Introduces SystemTrayController in ash, which wraps mojom::SystemTrayClient
* Peel two methods off the SystemTrayDelegate interface and changes them
to call into SystemTrayController and route over mojo
* Remove mojo usage from SystemTrayDelegateMus. Eventually it should go away
completely.

To keep this CL small it breaks a few system tray features when running under
mash (e.g. opening the general settings). This will get fixed in the next CL.

TODO: Remove SystemTrayCommon and move its functionality into SystemTrayClient
TODO: For methods like SystemTrayDelegate::ShowSettings, migrate callers to
use ash::SystemTrayController and always route over mojo.

BUG=647412
TEST=manual, clicking on system tray date opens date settings, changing time
from 12 to 24 hour works, in both classic ash and mash
==========

to

==========
Use mojo SystemTray interfaces for both mash and classic ash

The goal is to stop having to maintain separate SystemTrayDelegateMus and
SystemTrayDelegateChromeos implementations and eliminate the indirection
introduced by SystemTrayCommon.

* Always create the SystemTrayClient object in chrome, whether running ash
in-process or not
* Always allow code in //ash to bind to mojom::SystemTrayClient, even when ash
runs in-process
* Introduce WmShell::IsRunningInMash so ash can tell which mode its in
* Handle connecting to "exe:chrome" vs. "exe:content_browser" based on the
process mode
* Introduces SystemTrayController in ash, which wraps mojom::SystemTrayClient
* Peel two methods off the SystemTrayDelegate interface and changes them
to call into SystemTrayController and route over mojo
* Remove mojo usage from SystemTrayDelegateMus. Eventually it should go away
completely.

To keep this CL small it breaks a few system tray features when running under
mash (e.g. opening the general settings). This will get fixed in the next CL.

TODO: Remove SystemTrayCommon and move its functionality into SystemTrayClient
TODO: For methods like SystemTrayDelegate::ShowSettings, migrate callers to
use ash::SystemTrayController and always route over mojo.

BUG=647412
TEST=manual, clicking on system tray date opens date settings, changing time
from 12 to 24 hour works, in both classic ash and mash
==========

[commit-bot: I haz the power, 2016-10-04 02:41:52]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-10-04 02:45:00]

Description was changed from

==========
Use mojo SystemTray interfaces for both mash and classic ash

The goal is to stop having to maintain separate SystemTrayDelegateMus and
SystemTrayDelegateChromeos implementations and eliminate the indirection
introduced by SystemTrayCommon.

* Always create the SystemTrayClient object in chrome, whether running ash
in-process or not
* Always allow code in //ash to bind to mojom::SystemTrayClient, even when ash
runs in-process
* Introduce WmShell::IsRunningInMash so ash can tell which mode its in
* Handle connecting to "exe:chrome" vs. "exe:content_browser" based on the
process mode
* Introduces SystemTrayController in ash, which wraps mojom::SystemTrayClient
* Peel two methods off the SystemTrayDelegate interface and changes them
to call into SystemTrayController and route over mojo
* Remove mojo usage from SystemTrayDelegateMus. Eventually it should go away
completely.

To keep this CL small it breaks a few system tray features when running under
mash (e.g. opening the general settings). This will get fixed in the next CL.

TODO: Remove SystemTrayCommon and move its functionality into SystemTrayClient
TODO: For methods like SystemTrayDelegate::ShowSettings, migrate callers to
use ash::SystemTrayController and always route over mojo.

BUG=647412
TEST=manual, clicking on system tray date opens date settings, changing time
from 12 to 24 hour works, in both classic ash and mash
==========

to

==========
Use mojo SystemTray interfaces for both mash and classic ash

The goal is to stop having to maintain separate SystemTrayDelegateMus and
SystemTrayDelegateChromeos implementations and eliminate the indirection
introduced by SystemTrayCommon.

* Always create the SystemTrayClient object in chrome, whether running ash
in-process or not
* Always allow code in //ash to bind to mojom::SystemTrayClient, even when ash
runs in-process
* Introduce WmShell::IsRunningInMash so ash can tell which mode its in
* Handle connecting to "exe:chrome" vs. "exe:content_browser" based on the
process mode
* Introduces SystemTrayController in ash, which wraps mojom::SystemTrayClient
* Peel two methods off the SystemTrayDelegate interface and changes them
to call into SystemTrayController and route over mojo
* Remove mojo usage from SystemTrayDelegateMus. Eventually it should go away
completely.

To keep this CL small it breaks a few system tray features when running under
mash (e.g. opening the general settings). This will get fixed in the next CL.

TODO: Remove SystemTrayCommon and move its functionality into SystemTrayClient
TODO: For methods like SystemTrayDelegate::ShowSettings, migrate callers to
use ash::SystemTrayController and always route over mojo.

BUG=647412
TEST=manual, clicking on system tray date opens date settings, changing time
from 12 to 24 hour works, in both classic ash and mash

Committed: https://crrev.com/2a6b869d3921b816777679003d1df24f6c7b84fa
Cr-Commit-Position: refs/heads/master@{#422676}
==========

[commit-bot: I haz the power, 2016-10-04 02:45:01]

Patchset 9 (id:??) landed as
https://crrev.com/2a6b869d3921b816777679003d1df24f6c7b84fa
Cr-Commit-Position: refs/heads/master@{#422676}