mash: Add SystemTrayClient interface, use to show date settings

mash: Add SystemTrayClient interface, use to show date settings

* Introduce SystemTrayClient in chrome browser. It is similar to
SystemTrayDelegateChromeos in that it both observes chrome for state changes
and responds to queries from ash.
* Chrome browser SystemTrayClient connects to ash SystemTray on startup so it
can queue any state changes it sees.
* Ash SystemTrayDelegateMus connects to chrome browser SystemTrayClient
interface on demand because it has no state to enqueue.
* Implement connection error handlers on both sides.
* Implement showing the date settings as a demonstration.
* Factor code shared between SystemTrayDelegateChromeos and SystemTrayClient
into SystemTrayCommon.

BUG=644361
TEST=clicking on date in system tray opens date settings, even after closing all
chrome browser windows

Committed: https://crrev.com/b9ac16b3800c89223de560b769a2db7fc8c912c0
Cr-Commit-Position: refs/heads/master@{#420990}

[James Cook, 2016-09-22 23:13:14]

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

[commit-bot: I haz the power, 2016-09-22 23:13:56]

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-22 23:19:17]

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

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

sky, please take a look.

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

https://codereview.chromium.org/2360143004/diff/20001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:40: system_tray_client_->ShowDateSettings();
Is there a naming convention for these interfaceptr member variables so that the
reader knows this is an IPC and not just a regular function call?

I previously just called it client_, but that seemed too vague.

https://codereview.chromium.org/2360143004/diff/20001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:48: // No special client connection error
handling.
Not sure if I need set_connection_error_handler() here.

https://codereview.chromium.org/2360143004/diff/20001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_client.h (right):

https://codereview.chromium.org/2360143004/diff/20001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_client.h:15: class SystemTrayClient : public
ash::mojom::SystemTrayClient,
I went with this class being SystemTrayClient because that seemed like the
convention from WindowTree. However, I could also go with SystemTrayClientMus to
emphasize this only exists for mustash.

https://codereview.chromium.org/2360143004/diff/20001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_client.h:32:
mojo::Binding<ash::mojom::SystemTrayClient> binding_;
Is this the right binding type?

https://codereview.chromium.org/2360143004/diff/20001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_common.h (right):

https://codereview.chromium.org/2360143004/diff/20001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_common.h:12: class SystemTrayCommon {
This is a class and not a namespace because I anticipate that it will have
state/members very soon. Then SystemTrayDelegateChromeos and SystemTrayClient
will each own an instance.

[commit-bot: I haz the power, 2016-09-23 00:24:45]

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

[commit-bot: I haz the power, 2016-09-23 00:24:46]

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

[sky, 2016-09-23 16:42:26]

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

https://codereview.chromium.org/2360143004/diff/20001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:40: system_tray_client_->ShowDateSettings();
How do you know SetClient() has been called by the time you get here?

https://codereview.chromium.org/2360143004/diff/20001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:40: system_tray_client_->ShowDateSettings();
On 2016/09/22 23:19:18, James Cook wrote:
> Is there a naming convention for these interfaceptr member variables so that
the
> reader knows this is an IPC and not just a regular function call?
> 
> I previously just called it client_, but that seemed too vague.

'client_' is actually used a fair amount. And no, there isn't a naming
convention that conveys the IPC call.

https://codereview.chromium.org/2360143004/diff/20001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:48: // No special client connection error
handling.
On 2016/09/22 23:19:18, James Cook wrote:
> Not sure if I need set_connection_error_handler() here.

It's only necessary if you want to take action if the client is destroyed. I
think the action you want to take is resetting special_tray_client_. That way if
SetClient is called again you don't DCHECK. Say for example chrome crashes then
comes up again.

https://codereview.chromium.org/2360143004/diff/20001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_client.h (right):

https://codereview.chromium.org/2360143004/diff/20001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_client.h:15: class SystemTrayClient : public
ash::mojom::SystemTrayClient,
On 2016/09/22 23:19:18, James Cook wrote:
> I went with this class being SystemTrayClient because that seemed like the
> convention from WindowTree. However, I could also go with SystemTrayClientMus
to
> emphasize this only exists for mustash.

What you have SGTM.

https://codereview.chromium.org/2360143004/diff/20001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_client.h:32:
mojo::Binding<ash::mojom::SystemTrayClient> binding_;
On 2016/09/22 23:19:18, James Cook wrote:
> Is this the right binding type?

The other one you could use is StrongBinding. StrongBinding owns the
implementation though (SystemTrayClient here). As you have
ChromeBrowserMainExtraPartsAsh owning SystemTrayClient you don't want
StrongBinding. In other words what you have is right for this case.

[James Cook, 2016-09-23 21:05:53]

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

[commit-bot: I haz the power, 2016-09-23 21:06:24]

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-23 21:43:47]

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

[commit-bot: I haz the power, 2016-09-23 21:43:48]

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

[James Cook, 2016-09-24 00:24:15]

Description was changed from

==========
mash: Add SystemTrayClient interface, use to show date settings

SystemTrayClient provides browser process support for SystemTrayDelegateMus
in the ash process.

* Bind the SystemTrayClient interface at startup after the browser connects
to the ash SystemTray interface
* Implement showing the date settings as a demonstration
* Factor code shared between SystemTrayDelegateChromeos and SystemTrayClient
into SystemTrayCommon

BUG=644361
TEST=clicking on date in system tray opens date settings
==========

to

==========
mash: Add SystemTrayClient interface, use to show date settings

* Introduce SystemTrayClient in chrome browser. It is similar to
SystemTrayDelegateChromeos in that it both observes chrome for state changes
and responds to queries from ash.
* Chrome browser SystemTrayClient connects to ash SystemTray on startup so it
can queue any state changes it sees.
* Ash SystemTrayDelegateMus connects to chrome browser SystemTrayClient
interface on demand because it has no state to enqueue.
* Implement connection error handlers on both sides.
* Implement showing the date settings as a demonstration.
* Factor code shared between SystemTrayDelegateChromeos and SystemTrayClient
into SystemTrayCommon.

BUG=644361
TEST=clicking on date in system tray opens date settings, even after closing all
chrome browser windows
==========

[James Cook, 2016-09-24 00:25:30]

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

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

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-24 00:26:41]

sky, please take another look.

This patchset is pretty different. I got rid of the SetClient method. See the
revised CL description and code comments for what I'm doing and why.

[James Cook, 2016-09-24 00:34:37]

One more thing...

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

https://codereview.chromium.org/2360143004/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_client.h:15: class SystemTrayClient : public
ash::mojom::SystemTrayClient,
I combined the chrome -> ash and ash -> chrome communication into one class
because it's like SystemTrayDelegateChromeos. However, I could also keep the
chrome -> ash pieces in SystemTrayControllerMus (created at startup, connects to
ash on startup) and add a new SystemTrayClient (created on demand when ash
requests it). The latter approach might make the ChromeInterfaceFactory stuff a
little more like the other interfaces, but would introduce even more classes.

Do you have a preference?

[commit-bot: I haz the power, 2016-09-24 01:18:47]

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

[commit-bot: I haz the power, 2016-09-24 01:18:48]

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

[sky, 2016-09-26 15:23:14]

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

https://codereview.chromium.org/2360143004/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:42: // Check if already connected.
optional: IMO this comment and 43 document exactly what the code does and don't
provide much value.

https://codereview.chromium.org/2360143004/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:47:
connector_->ConnectToInterface("exe:chrome", &system_tray_client_);
What you have here certainly works. Consider though that one nice thing about
the explicit SetClient call you had before is that it means ash doesn't care who
is providing the client interface and doesn't need to establish an out bound
connection like this.

https://codereview.chromium.org/2360143004/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:52: weak_factory_.GetWeakPtr()));
As this owns system_tray_client_ you shouldn't need a weakptr here.

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

https://codereview.chromium.org/2360143004/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_client.h:15: class SystemTrayClient : public
ash::mojom::SystemTrayClient,
On 2016/09/24 00:34:37, James Cook wrote:
> I combined the chrome -> ash and ash -> chrome communication into one class
> because it's like SystemTrayDelegateChromeos. However, I could also keep the
> chrome -> ash pieces in SystemTrayControllerMus (created at startup, connects
to
> ash on startup) and add a new SystemTrayClient (created on demand when ash
> requests it). The latter approach might make the ChromeInterfaceFactory stuff
a
> little more like the other interfaces, but would introduce even more classes.
> 
> Do you have a preference?

I actually prefer an explicit SetClient as it means less outgoing connections
from ash.

[sky, 2016-09-26 16:52:31]

James explained that if the code waited for chrome to connect then it would have
to queue up messages, which is undesirable. So, LGTM

[James Cook, 2016-09-26 17:58:38]

Patchset #3 (id:40001) has been deleted

[James Cook, 2016-09-26 17:59:40]

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

[commit-bot: I haz the power, 2016-09-26 18:00:08]

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-26 18:01:36]

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

[James Cook, 2016-09-26 18:01:38]

tsepez, please take a look at //ash/public/interfaces/system_tray.mojom.

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

https://codereview.chromium.org/2360143004/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:42: // Check if already connected.
On 2016/09/26 15:23:14, sky wrote:
> optional: IMO this comment and 43 document exactly what the code does and
don't
> provide much value.

Removed this one and the one on line 46.

https://codereview.chromium.org/2360143004/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:47:
connector_->ConnectToInterface("exe:chrome", &system_tray_client_);
On 2016/09/26 15:23:14, sky wrote:
> What you have here certainly works. Consider though that one nice thing about
> the explicit SetClient call you had before is that it means ash doesn't care
who
> is providing the client interface and doesn't need to establish an out bound
> connection like this.

As discussed offline, keeping this so commands on the system_tray_client_
interface get queued if the user takes an action before chrome is up.

https://codereview.chromium.org/2360143004/diff/60001/ash/mus/system_tray_del...
ash/mus/system_tray_delegate_mus.cc:52: weak_factory_.GetWeakPtr()));
On 2016/09/26 15:23:14, sky wrote:
> As this owns system_tray_client_ you shouldn't need a weakptr here.

Done.

[Tom Sepez, 2016-09-26 19:10:04]

mojom LGTM

[James Cook, 2016-09-26 19:12:12]

The CQ bit was unchecked by jamescook@chromium.org

[James Cook, 2016-09-26 19:12:19]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-09-26 19:12:20]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2360143004/#ps120001
(title: "reparent")

[commit-bot: I haz the power, 2016-09-26 19:13:06]

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-26 21:04:44]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-09-26 21:07:22]

Description was changed from

==========
mash: Add SystemTrayClient interface, use to show date settings

* Introduce SystemTrayClient in chrome browser. It is similar to
SystemTrayDelegateChromeos in that it both observes chrome for state changes
and responds to queries from ash.
* Chrome browser SystemTrayClient connects to ash SystemTray on startup so it
can queue any state changes it sees.
* Ash SystemTrayDelegateMus connects to chrome browser SystemTrayClient
interface on demand because it has no state to enqueue.
* Implement connection error handlers on both sides.
* Implement showing the date settings as a demonstration.
* Factor code shared between SystemTrayDelegateChromeos and SystemTrayClient
into SystemTrayCommon.

BUG=644361
TEST=clicking on date in system tray opens date settings, even after closing all
chrome browser windows
==========

to

==========
mash: Add SystemTrayClient interface, use to show date settings

* Introduce SystemTrayClient in chrome browser. It is similar to
SystemTrayDelegateChromeos in that it both observes chrome for state changes
and responds to queries from ash.
* Chrome browser SystemTrayClient connects to ash SystemTray on startup so it
can queue any state changes it sees.
* Ash SystemTrayDelegateMus connects to chrome browser SystemTrayClient
interface on demand because it has no state to enqueue.
* Implement connection error handlers on both sides.
* Implement showing the date settings as a demonstration.
* Factor code shared between SystemTrayDelegateChromeos and SystemTrayClient
into SystemTrayCommon.

BUG=644361
TEST=clicking on date in system tray opens date settings, even after closing all
chrome browser windows

Committed: https://crrev.com/b9ac16b3800c89223de560b769a2db7fc8c912c0
Cr-Commit-Position: refs/heads/master@{#420990}
==========

[commit-bot: I haz the power, 2016-09-26 21:07:24]

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