mus: Forward user activity from window server to detector.

mus: Forward user activity from window server to detector.

Introduce an aura::UserActivityForwarder class that
receives reports of user activity from the window server via
mojo and forwards them to ui::UserActivityDetector so that
existing classes implementing ui::UserActivityObserver will
be notified.

This results in user activity being reported to the Chrome
OS power manager to prevent the display from being dimmed
and turned off.

BUG=626899
Review-Url: https://codereview.chromium.org/2639703004
Cr-Commit-Position: refs/heads/master@{#445095}
Committed: https://chromium.googlesource.com/chromium/src/+/94887a2c9af708195be7560415090dd70eabd049

[Daniel Erat, 2017-01-18 01:12:36]

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

[Daniel Erat, 2017-01-18 01:12:58]

derat@chromium.org changed reviewers:
+ jamescook@chromium.org, sadrul@chromium.org

[Daniel Erat, 2017-01-18 01:12:59]

does this approach seem reasonable? it seems to work, at least. :-P

https://codereview.chromium.org/2639703004/diff/1/ash/mus/user_activity_forwa...
File ash/mus/user_activity_forwarder.h (right):

https://codereview.chromium.org/2639703004/diff/1/ash/mus/user_activity_forwa...
ash/mus/user_activity_forwarder.h:28: class UserActivityForwarder : public
ui::mojom::UserActivityObserver {
a forwarder like this seems necessary since //services/ui depends on //ui,
preventing the existing UserActivityDetector class from registering from
notifications from UserActivityMonitor.

[commit-bot: I haz the power, 2017-01-18 01:13:25]

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, 2017-01-18 01:36:24]

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

[commit-bot: I haz the power, 2017-01-18 01:36:25]

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

[Daniel Erat, 2017-01-18 04:21:58]

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

[commit-bot: I haz the power, 2017-01-18 04:22:10]

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, 2017-01-18 05:08:51]

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

[commit-bot: I haz the power, 2017-01-18 05:08:52]

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

[Daniel Erat, 2017-01-18 16:36:52]

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

[commit-bot: I haz the power, 2017-01-18 16:37:07]

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

[James Cook, 2017-01-18 17:01:38]

https://codereview.chromium.org/2639703004/diff/40001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.h (right):

https://codereview.chromium.org/2639703004/diff/40001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.h:146:
std::unique_ptr<ui::UserActivityPowerManagerNotifier> user_activity_notifier_;
Is this the thing that actually sends the dbus signals? Does it register itself
as an observer on construction? Might be worth a comment somewhere.

https://codereview.chromium.org/2639703004/diff/40001/ash/mus/user_activity_f...
File ash/mus/user_activity_forwarder.cc (right):

https://codereview.chromium.org/2639703004/diff/40001/ash/mus/user_activity_f...
ash/mus/user_activity_forwarder.cc:40: }
If this class sticks around, could it get a trivial unit test? This could be
more-or-less a copy of the one you added, but instead call into OnUserActivity.
This would be testing at the mojo interface, which is something I'm trying to
encourage.

https://codereview.chromium.org/2639703004/diff/40001/ash/mus/user_activity_f...
File ash/mus/user_activity_forwarder.h (right):

https://codereview.chromium.org/2639703004/diff/40001/ash/mus/user_activity_f...
ash/mus/user_activity_forwarder.h:28: class UserActivityForwarder : public
ui::mojom::UserActivityObserver {
I wonder if this class could live under //ui. While //services/ui has DEPS +ui,
the public interfaces should not actually depend on much of //ui. I wonder if it
would work to make the //ui/base/user_activity code into its own module (not
part of //ui/base), perhaps under //ui/chromeos/user_activity. This class could
still exist, but be optionally constructed by the ash process and the chrome
browser process when running in mash.

This class could also continue to live here, and chrome browser could depend on
this directory, but that would be somewhat confusing, as we're trying to
eliminate chrome -> ash dependencies in general.

WDYT?

https://codereview.chromium.org/2639703004/diff/40001/ui/base/user_activity/u...
File ui/base/user_activity/user_activity_detector_unittest.cc (right):

https://codereview.chromium.org/2639703004/diff/40001/ui/base/user_activity/u...
ui/base/user_activity/user_activity_detector_unittest.cc:233: }
Nice test, easy to read.

[commit-bot: I haz the power, 2017-01-18 17:09:42]

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

[commit-bot: I haz the power, 2017-01-18 17:09:42]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Daniel Erat, 2017-01-18 18:54:20]

https://codereview.chromium.org/2639703004/diff/40001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.h (right):

https://codereview.chromium.org/2639703004/diff/40001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.h:146:
std::unique_ptr<ui::UserActivityPowerManagerNotifier> user_activity_notifier_;
On 2017/01/18 17:01:38, James Cook wrote:
> Is this the thing that actually sends the dbus signals? Does it register
itself
> as an observer on construction? Might be worth a comment somewhere.

done (added comments to ui/chromeos/user_activity_power_manager_notifier.h)

https://codereview.chromium.org/2639703004/diff/40001/ash/mus/user_activity_f...
File ash/mus/user_activity_forwarder.cc (right):

https://codereview.chromium.org/2639703004/diff/40001/ash/mus/user_activity_f...
ash/mus/user_activity_forwarder.cc:40: }
On 2017/01/18 17:01:38, James Cook wrote:
> If this class sticks around, could it get a trivial unit test? This could be
> more-or-less a copy of the one you added, but instead call into
OnUserActivity.
> This would be testing at the mojo interface, which is something I'm trying to
> encourage.

hmm. is there any way to exercise the calls to BindInterface and
AddUserActivityObserver from within a unit test, and then tell a stub
implementation of UserActivityMonitor to notify all of its observers? just
checking that OnUserActivity calls HandleExternalUserActivity doesn't feel
likely to catch regressions in the future.

https://codereview.chromium.org/2639703004/diff/40001/ash/mus/user_activity_f...
File ash/mus/user_activity_forwarder.h (right):

https://codereview.chromium.org/2639703004/diff/40001/ash/mus/user_activity_f...
ash/mus/user_activity_forwarder.h:28: class UserActivityForwarder : public
ui::mojom::UserActivityObserver {
On 2017/01/18 17:01:38, James Cook wrote:
> I wonder if this class could live under //ui. While //services/ui has DEPS
+ui,
> the public interfaces should not actually depend on much of //ui. I wonder if
it
> would work to make the //ui/base/user_activity code into its own module (not
> part of //ui/base), perhaps under //ui/chromeos/user_activity. This class
could
> still exist, but be optionally constructed by the ash process and the chrome
> browser process when running in mash.
> 
> This class could also continue to live here, and chrome browser could depend
on
> this directory, but that would be somewhat confusing, as we're trying to
> eliminate chrome -> ash dependencies in general.
> 
> WDYT?

are you proposing something like the following?

- leave services/ui/DEPS alone
- add -ui/chromeos to a new services/ui/public/DEPS file
- move this class to ui/chromeos/user_activity
- add +services/ui/public to ui/chromeos/user_activity/DEPS

(perhaps both of those .../public paths should be .../public/interfaces.)

that might work, but it makes the relationship between //services/ui and //ui
feel really tricky to me.

[Daniel Erat, 2017-01-18 19:04:32]

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

[commit-bot: I haz the power, 2017-01-18 19:04:54]

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, 2017-01-18 19:07:04]

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

[commit-bot: I haz the power, 2017-01-18 19:07:05]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[James Cook, 2017-01-18 20:17:33]

https://codereview.chromium.org/2639703004/diff/40001/ash/mus/user_activity_f...
File ash/mus/user_activity_forwarder.cc (right):

https://codereview.chromium.org/2639703004/diff/40001/ash/mus/user_activity_f...
ash/mus/user_activity_forwarder.cc:40: }
On 2017/01/18 18:54:20, Daniel Erat wrote:
> On 2017/01/18 17:01:38, James Cook wrote:
> > If this class sticks around, could it get a trivial unit test? This could be
> > more-or-less a copy of the one you added, but instead call into
> OnUserActivity.
> > This would be testing at the mojo interface, which is something I'm trying
to
> > encourage.
> 
> hmm. is there any way to exercise the calls to BindInterface and
> AddUserActivityObserver from within a unit test, and then tell a stub
> implementation of UserActivityMonitor to notify all of its observers? just
> checking that OnUserActivity calls HandleExternalUserActivity doesn't feel
> likely to catch regressions in the future.

I don't know how to exercise the BindInterface bits. I was thinking of ash
SessionController's unit tests. However, in that case the class under test is
processing mojo messages and doing interesting things with them. So I agree
testing at this level probably doesn't add much.

https://codereview.chromium.org/2639703004/diff/40001/ash/mus/user_activity_f...
File ash/mus/user_activity_forwarder.h (right):

https://codereview.chromium.org/2639703004/diff/40001/ash/mus/user_activity_f...
ash/mus/user_activity_forwarder.h:28: class UserActivityForwarder : public
ui::mojom::UserActivityObserver {
On 2017/01/18 18:54:20, Daniel Erat wrote:
> On 2017/01/18 17:01:38, James Cook wrote:
> > I wonder if this class could live under //ui. While //services/ui has DEPS
> +ui,
> > the public interfaces should not actually depend on much of //ui. I wonder
if
> it
> > would work to make the //ui/base/user_activity code into its own module (not
> > part of //ui/base), perhaps under //ui/chromeos/user_activity. This class
> could
> > still exist, but be optionally constructed by the ash process and the chrome
> > browser process when running in mash.
> > 
> > This class could also continue to live here, and chrome browser could depend
> on
> > this directory, but that would be somewhat confusing, as we're trying to
> > eliminate chrome -> ash dependencies in general.
> > 
> > WDYT?
> 
> are you proposing something like the following?
> 
> - leave services/ui/DEPS alone
> - add -ui/chromeos to a new services/ui/public/DEPS file
> - move this class to ui/chromeos/user_activity
> - add +services/ui/public to ui/chromeos/user_activity/DEPS
> 
> (perhaps both of those .../public paths should be .../public/interfaces.)
> 
> that might work, but it makes the relationship between //services/ui and //ui
> feel really tricky to me.

If you look under //ui you'll see several cases where DEPS allows
+services/service_manager and +services/ui/public

I was thinking:
- change services/ui/DEPS to remove +ui and add +ui/foo for all foo it actually
uses.
- add +services/ui/public to ui/chromeos/user_activity

What you're doing now is OK with me for ash, but how are you going to deal with
the user activity observers in chrome browser?

[sadrul, 2017-01-18 22:10:58]

https://codereview.chromium.org/2639703004/diff/60001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2639703004/diff/60001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:124: user_activity_detector_.get())) {
I guess we will now use the UserActivityDetector that ash::Shell creates?

https://codereview.chromium.org/2639703004/diff/60001/ash/mus/user_activity_f...
File ash/mus/user_activity_forwarder.cc (right):

https://codereview.chromium.org/2639703004/diff/60001/ash/mus/user_activity_f...
ash/mus/user_activity_forwarder.cc:26: const uint32_t kNotifyIntervalSec =
static_cast<uint32_t>(
constexpr?

https://codereview.chromium.org/2639703004/diff/60001/ash/mus/user_activity_f...
File ash/mus/user_activity_forwarder.h (right):

https://codereview.chromium.org/2639703004/diff/60001/ash/mus/user_activity_f...
ash/mus/user_activity_forwarder.h:6: #define ASH_MUS_USER_ACTIVITY_FORWARDER_H_
Would it make sense for this to live in the client-lib (ui/aura/mus), so that
the code in chrome/browser/chromeos can also use this?

https://codereview.chromium.org/2639703004/diff/60001/ash/mus/user_activity_f...
ash/mus/user_activity_forwarder.h:30:
UserActivityForwarder(service_manager::Connector* connector,
Instead of sending the connector here, can you send the Ptr? That would make the
testing easier.

https://codereview.chromium.org/2639703004/diff/60001/ui/base/user_activity/u...
File ui/base/user_activity/user_activity_detector.cc (right):

https://codereview.chromium.org/2639703004/diff/60001/ui/base/user_activity/u...
ui/base/user_activity/user_activity_detector.cc:123: last_activity_name_ = event
? event->name() : "";
s/""/std::string()/

[Daniel Erat, 2017-01-19 01:56:24]

https://codereview.chromium.org/2639703004/diff/60001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2639703004/diff/60001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:124: user_activity_detector_.get())) {
On 2017/01/18 22:10:58, sadrul wrote:
> I guess we will now use the UserActivityDetector that ash::Shell creates?

yep, i just saw sky's message about ash::Shell. :-P

ash::Shell creates UserActivityPowerManagerNotifier as well. it currently does
it after creating WmShellMus, so i'll need to think about initialization order
some more.

do you think that UserActivityForwarder should live in WmShellMus, or is there a
better place for it? should i just move it to ash::Shell for now to ensure that
it's created at the correct time? (that may make it harder to pass
UserActivityMonitor to it instead of the connector, though.)

https://codereview.chromium.org/2639703004/diff/60001/ash/mus/user_activity_f...
File ash/mus/user_activity_forwarder.cc (right):

https://codereview.chromium.org/2639703004/diff/60001/ash/mus/user_activity_f...
ash/mus/user_activity_forwarder.cc:26: const uint32_t kNotifyIntervalSec =
static_cast<uint32_t>(
On 2017/01/18 22:10:58, sadrul wrote:
> constexpr?

i tried this initially, but the UserActivityDetector static member appears to
make it fail.

https://codereview.chromium.org/2639703004/diff/60001/ash/mus/user_activity_f...
File ash/mus/user_activity_forwarder.h (right):

https://codereview.chromium.org/2639703004/diff/60001/ash/mus/user_activity_f...
ash/mus/user_activity_forwarder.h:6: #define ASH_MUS_USER_ACTIVITY_FORWARDER_H_
On 2017/01/18 22:10:58, sadrul wrote:
> Would it make sense for this to live in the client-lib (ui/aura/mus), so that
> the code in chrome/browser/chromeos can also use this?

sure! i didn't know about that directory, but it looks like it can depend on
both ui/base and services/ui, so that sounds perfect.

https://codereview.chromium.org/2639703004/diff/60001/ash/mus/user_activity_f...
ash/mus/user_activity_forwarder.h:30:
UserActivityForwarder(service_manager::Connector* connector,
On 2017/01/18 22:10:58, sadrul wrote:
> Instead of sending the connector here, can you send the Ptr? That would make
the
> testing easier.

sounds fine to me. do we have an existing pattern that we're following for
fake/stub implementations of mojo interfaces? i think i've seen arc code that
does this, but i don't see any obvious examples under services/ui/.

https://codereview.chromium.org/2639703004/diff/60001/ui/base/user_activity/u...
File ui/base/user_activity/user_activity_detector.cc (right):

https://codereview.chromium.org/2639703004/diff/60001/ui/base/user_activity/u...
ui/base/user_activity/user_activity_detector.cc:123: last_activity_name_ = event
? event->name() : "";
On 2017/01/18 22:10:58, sadrul wrote:
> s/""/std::string()/

Done.

[sadrul, 2017-01-19 04:27:45]

https://codereview.chromium.org/2639703004/diff/60001/ash/mus/user_activity_f...
File ash/mus/user_activity_forwarder.h (right):

https://codereview.chromium.org/2639703004/diff/60001/ash/mus/user_activity_f...
ash/mus/user_activity_forwarder.h:30:
UserActivityForwarder(service_manager::Connector* connector,
On 2017/01/19 01:56:24, Daniel Erat wrote:
> On 2017/01/18 22:10:58, sadrul wrote:
> > Instead of sending the connector here, can you send the Ptr? That would make
> the
> > testing easier.
> 
> sounds fine to me. do we have an existing pattern that we're following for
> fake/stub implementations of mojo interfaces? i think i've seen arc code that
> does this, but i don't see any obvious examples under services/ui/.

I have used stub implementations before (e.g. [1]). I am not sure if there's a
pattern we consistently follow across the codebase for this though.

1:
https://cs.chromium.org/chromium/src/services/ui/ws/user_activity_monitor_uni...

[Daniel Erat, 2017-01-20 00:08:05]

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

[commit-bot: I haz the power, 2017-01-20 00:08:48]

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

[Daniel Erat, 2017-01-20 00:09:29]

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

[Daniel Erat, 2017-01-20 00:09:29]

thanks for the pointer re adding tests that use mojo!

i've moved UserActivityForwarder from WmShellMus to Shell after discussion with
sky@ (who should also take a look now). :-)

[Daniel Erat, 2017-01-20 00:10:10]

Description was changed from

==========
mus: Forward user activity from window server to detector.

Introduce an ash::mus::UserActivityForwarder class that
receives reports of user activity from the window server via
mojo and forwards them to ui::UserActivityDetector so that
existing classes implementing ui::UserActivityObserver will
be notified.

This results in user activity being reported to the Chrome
OS power manager to prevent the display from being dimmed
and turned off.

BUG=626899
==========

to

==========
mus: Forward user activity from window server to detector.

Introduce an aura::UserActivityForwarder class that
receives reports of user activity from the window server via
mojo and forwards them to ui::UserActivityDetector so that
existing classes implementing ui::UserActivityObserver will
be notified.

This results in user activity being reported to the Chrome
OS power manager to prevent the display from being dimmed
and turned off.

BUG=626899
==========

[commit-bot: I haz the power, 2017-01-20 00:51:30]

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

[commit-bot: I haz the power, 2017-01-20 00:51:31]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[sky, 2017-01-20 00:53:02]

LGTM

[Daniel Erat, 2017-01-20 01:01:54]

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

[commit-bot: I haz the power, 2017-01-20 01:04:04]

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, 2017-01-20 02:10:40]

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

[commit-bot: I haz the power, 2017-01-20 02:10:41]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-01-20 03:38:49]

LGTM with nits.

https://codereview.chromium.org/2639703004/diff/100001/ui/aura/mus/user_activ...
File ui/aura/mus/user_activity_forwarder.cc (right):

https://codereview.chromium.org/2639703004/diff/100001/ui/aura/mus/user_activ...
ui/aura/mus/user_activity_forwarder.cc:21: const uint32_t kNotifyIntervalSec =
static_cast<uint32_t>(
nit: #include <stdint.h> for uint32_t ?

https://codereview.chromium.org/2639703004/diff/100001/ui/aura/mus/user_activ...
File ui/aura/mus/user_activity_forwarder.h (right):

https://codereview.chromium.org/2639703004/diff/100001/ui/aura/mus/user_activ...
ui/aura/mus/user_activity_forwarder.h:20: // Helper class that receives reports
of user activity from UserActivityMonitor
super nit: I find it helpful for classes like this to document which processes
provide these interfaces. For example, this could say something like "from mojo
UserActivityMonitor interface (in the window server) to ui::UserActivityMonitor
(usually in ash)".

While this might be a "layering violation" in comments, it's super helpful for
me to figure out what is connected to what.

https://codereview.chromium.org/2639703004/diff/100001/ui/aura/mus/user_activ...
File ui/aura/mus/user_activity_forwarder_unittest.cc (right):

https://codereview.chromium.org/2639703004/diff/100001/ui/aura/mus/user_activ...
ui/aura/mus/user_activity_forwarder_unittest.cc:82: RunUntilIdle();
nit: The test might be easier to read if all this initialization was in the test
body. In particular, it might make it more obvious in the test itself that the
forwarder is being exercised (since the test body never references the
forwarder).

https://codereview.chromium.org/2639703004/diff/100001/ui/aura/mus/user_activ...
ui/aura/mus/user_activity_forwarder_unittest.cc:102: }  // namespace aura
Thanks for adding test at the mojo level!

[sadrul, 2017-01-20 06:24:47]

lgtm

[Daniel Erat, 2017-01-20 16:49:45]

https://codereview.chromium.org/2639703004/diff/100001/ui/aura/mus/user_activ...
File ui/aura/mus/user_activity_forwarder.cc (right):

https://codereview.chromium.org/2639703004/diff/100001/ui/aura/mus/user_activ...
ui/aura/mus/user_activity_forwarder.cc:21: const uint32_t kNotifyIntervalSec =
static_cast<uint32_t>(
On 2017/01/20 03:38:49, James Cook (OOO Jan 20) wrote:
> nit: #include <stdint.h> for uint32_t ?

Done.

https://codereview.chromium.org/2639703004/diff/100001/ui/aura/mus/user_activ...
File ui/aura/mus/user_activity_forwarder.h (right):

https://codereview.chromium.org/2639703004/diff/100001/ui/aura/mus/user_activ...
ui/aura/mus/user_activity_forwarder.h:20: // Helper class that receives reports
of user activity from UserActivityMonitor
On 2017/01/20 03:38:49, James Cook (OOO Jan 20) wrote:
> super nit: I find it helpful for classes like this to document which processes
> provide these interfaces. For example, this could say something like "from
mojo
> UserActivityMonitor interface (in the window server) to
ui::UserActivityMonitor
> (usually in ash)".
> 
> While this might be a "layering violation" in comments, it's super helpful for
> me to figure out what is connected to what.

sure, done! i like these comments too.

https://codereview.chromium.org/2639703004/diff/100001/ui/aura/mus/user_activ...
File ui/aura/mus/user_activity_forwarder_unittest.cc (right):

https://codereview.chromium.org/2639703004/diff/100001/ui/aura/mus/user_activ...
ui/aura/mus/user_activity_forwarder_unittest.cc:82: RunUntilIdle();
On 2017/01/20 03:38:49, James Cook (OOO Jan 20) wrote:
> nit: The test might be easier to read if all this initialization was in the
test
> body. In particular, it might make it more obvious in the test itself that the
> forwarder is being exercised (since the test body never references the
> forwarder).

sure, sounds fine to me -- i don't anticipate adding more tests to this file. i
switched the test class to just be a typedef.

https://codereview.chromium.org/2639703004/diff/100001/ui/aura/mus/user_activ...
ui/aura/mus/user_activity_forwarder_unittest.cc:102: }  // namespace aura
On 2017/01/20 03:38:49, James Cook (OOO Jan 20) wrote:
> Thanks for adding test at the mojo level!

no problem!

[Daniel Erat, 2017-01-20 16:57:19]

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

[commit-bot: I haz the power, 2017-01-20 16:57:37]

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

[Daniel Erat, 2017-01-20 17:10:34]

The CQ bit was unchecked by derat@chromium.org

[Daniel Erat, 2017-01-20 17:10:38]

The CQ bit was checked by derat@chromium.org

[Daniel Erat, 2017-01-20 17:10:38]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
jamescook@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2639703004/#ps120001
(title: "address jamescook@'s comments")

[commit-bot: I haz the power, 2017-01-20 17:10:51]

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, 2017-01-20 18:17:28]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484932238215260,
"parent_rev": "9917dd563899eff0020ca4cad673832b620f86ec", "commit_rev":
"94887a2c9af708195be7560415090dd70eabd049"}

[commit-bot: I haz the power, 2017-01-20 18:18:00]

Description was changed from

==========
mus: Forward user activity from window server to detector.

Introduce an aura::UserActivityForwarder class that
receives reports of user activity from the window server via
mojo and forwards them to ui::UserActivityDetector so that
existing classes implementing ui::UserActivityObserver will
be notified.

This results in user activity being reported to the Chrome
OS power manager to prevent the display from being dimmed
and turned off.

BUG=626899
==========

to

==========
mus: Forward user activity from window server to detector.

Introduce an aura::UserActivityForwarder class that
receives reports of user activity from the window server via
mojo and forwards them to ui::UserActivityDetector so that
existing classes implementing ui::UserActivityObserver will
be notified.

This results in user activity being reported to the Chrome
OS power manager to prevent the display from being dimmed
and turned off.

BUG=626899

Review-Url: https://codereview.chromium.org/2639703004
Cr-Commit-Position: refs/heads/master@{#445095}
Committed:
https://chromium.googlesource.com/chromium/src/+/94887a2c9af708195be756041509...
==========

[commit-bot: I haz the power, 2017-01-20 18:18:04]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/94887a2c9af708195be756041509...