mus: Add UserActivityMonitor.

mus: Add UserActivityMonitor.

Introduce UserActivityMonitor in mus. Clients can install UserActivityObserver
to get notified of repeated user-activity, and UserIdlenessObserver to get
notified of when the user becomes idle/active.

BUG=623296
Committed: https://crrev.com/f2bca7dc696137ca9ae1101b6479ffb69e328577
Cr-Commit-Position: refs/heads/master@{#402398}

[sadrul, 2016-06-25 07:03:24]

Description was changed from

==========
mus: Add UserActivityMonitor.

BUG=none
==========

to

==========
mus: Add UserActivityMonitor.

Introduce UserActivityMonitor in mus. Clients can install UserActivityObserver
to get notified of repeated user-activity, and UserIdlenessObserver to get
notified of when the user becomes idle/active.

BUG=623296
==========

[sadrul, 2016-06-25 07:03:45]

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

[sky, 2016-06-27 15:54:26]

https://codereview.chromium.org/2094933003/diff/60001/components/mus/public/i...
File components/mus/public/interfaces/user_activity_monitor.mojom (right):

https://codereview.chromium.org/2094933003/diff/60001/components/mus/public/i...
components/mus/public/interfaces/user_activity_monitor.mojom:11: interface
UserIdlenessObserver {
nit: UserIdleObserver?

https://codereview.chromium.org/2094933003/diff/60001/components/mus/ws/user_...
File components/mus/ws/user_activity_monitor.cc (right):

https://codereview.chromium.org/2094933003/diff/60001/components/mus/ws/user_...
components/mus/ws/user_activity_monitor.cc:26: for (auto& pair :
activity_observers_) {
Should you remove from both of these lists any interfaceptrs that are no longer
bound?

https://codereview.chromium.org/2094933003/diff/60001/components/mus/ws/user_...
File components/mus/ws/user_activity_monitor.h (right):

https://codereview.chromium.org/2094933003/diff/60001/components/mus/ws/user_...
components/mus/ws/user_activity_monitor.h:29: void OnUserActivity();
nit: add descriptions for functions (including OnMinuteTimer below).

https://codereview.chromium.org/2094933003/diff/60001/components/mus/ws/user_...
components/mus/ws/user_activity_monitor.h:62: base::RepeatingTimer idle_timer_;
You should avoid running this timer if not necessary. By that I mean if there
are no observers or all observers have been notified of idle then then you don't
need to keep running the timer.

https://codereview.chromium.org/2094933003/diff/60001/components/mus/ws/windo...
File components/mus/ws/window_manager_state.h (right):

https://codereview.chromium.org/2094933003/diff/60001/components/mus/ws/windo...
components/mus/ws/window_manager_state.h:34: class WindowManagerState : public
EventDispatcherDelegate {
This class is intended to own state from the windowmanager. The
UserActivityManager maintains state from connections with the same user and not
necessarily from the windowmanager. The two should have different lifetimes. The
way you have things now there is no guarantee the windowmanagerstate has been
created at the time you try to use it
(WindowServer::GetUserActivityMonitorForUser).

UserIdTracker tracks the set of known users. One option is to make UserId a real
class that has members and owns a UserActivityMonitor.

[sadrul, 2016-06-27 15:57:01]

https://codereview.chromium.org/2094933003/diff/60001/components/mus/ws/windo...
File components/mus/ws/window_manager_state.h (right):

https://codereview.chromium.org/2094933003/diff/60001/components/mus/ws/windo...
components/mus/ws/window_manager_state.h:34: class WindowManagerState : public
EventDispatcherDelegate {
On 2016/06/27 15:54:26, sky wrote:
> This class is intended to own state from the windowmanager. The
> UserActivityManager maintains state from connections with the same user and
not
> necessarily from the windowmanager. The two should have different lifetimes.
The
> way you have things now there is no guarantee the windowmanagerstate has been
> created at the time you try to use it
> (WindowServer::GetUserActivityMonitorForUser).
> 
> UserIdTracker tracks the set of known users. One option is to make UserId a
real
> class that has members and owns a UserActivityMonitor.

I had considered having MusApp::UserState own the UserActivityMonitor
(https://cs.chromium.org/chromium/src/components/mus/mus_app.cc?sq=package:chr...).
Would that work?

[sky, 2016-06-27 16:16:39]

Indeed. That works as well.

  -Scott

On Mon, Jun 27, 2016 at 8:57 AM,  <sadrul@chromium.org> wrote:
>
>
https://codereview.chromium.org/2094933003/diff/60001/components/mus/ws/windo...
> File components/mus/ws/window_manager_state.h (right):
>
>
https://codereview.chromium.org/2094933003/diff/60001/components/mus/ws/windo...
> components/mus/ws/window_manager_state.h:34: class WindowManagerState :
> public EventDispatcherDelegate {
> On 2016/06/27 15:54:26, sky wrote:
>> This class is intended to own state from the windowmanager. The
>> UserActivityManager maintains state from connections with the same
> user and not
>> necessarily from the windowmanager. The two should have different
> lifetimes. The
>> way you have things now there is no guarantee the windowmanagerstate
> has been
>> created at the time you try to use it
>> (WindowServer::GetUserActivityMonitorForUser).
>>
>> UserIdTracker tracks the set of known users. One option is to make
> UserId a real
>> class that has members and owns a UserActivityMonitor.
>
> I had considered having MusApp::UserState own the UserActivityMonitor
>
(https://cs.chromium.org/chromium/src/components/mus/mus_app.cc?sq=package:chr...).
> Would that work?
>
> https://codereview.chromium.org/2094933003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2016-06-27 20:46:13]

https://codereview.chromium.org/2094933003/diff/60001/components/mus/public/i...
File components/mus/public/interfaces/user_activity_monitor.mojom (right):

https://codereview.chromium.org/2094933003/diff/60001/components/mus/public/i...
components/mus/public/interfaces/user_activity_monitor.mojom:11: interface
UserIdlenessObserver {
On 2016/06/27 15:54:25, sky wrote:
> nit: UserIdleObserver?

Done.

https://codereview.chromium.org/2094933003/diff/60001/components/mus/ws/user_...
File components/mus/ws/user_activity_monitor.cc (right):

https://codereview.chromium.org/2094933003/diff/60001/components/mus/ws/user_...
components/mus/ws/user_activity_monitor.cc:26: for (auto& pair :
activity_observers_) {
On 2016/06/27 15:54:26, sky wrote:
> Should you remove from both of these lists any interfaceptrs that are no
longer
> bound?

Yep (meant to do that before sending out for review, sorry). Done.

https://codereview.chromium.org/2094933003/diff/60001/components/mus/ws/user_...
File components/mus/ws/user_activity_monitor.h (right):

https://codereview.chromium.org/2094933003/diff/60001/components/mus/ws/user_...
components/mus/ws/user_activity_monitor.h:29: void OnUserActivity();
On 2016/06/27 15:54:26, sky wrote:
> nit: add descriptions for functions (including OnMinuteTimer below).

Done.

https://codereview.chromium.org/2094933003/diff/60001/components/mus/ws/user_...
components/mus/ws/user_activity_monitor.h:62: base::RepeatingTimer idle_timer_;
On 2016/06/27 15:54:26, sky wrote:
> You should avoid running this timer if not necessary. By that I mean if there
> are no observers or all observers have been notified of idle then then you
don't
> need to keep running the timer.

Good point! Done.

https://codereview.chromium.org/2094933003/diff/60001/components/mus/ws/windo...
File components/mus/ws/window_manager_state.h (right):

https://codereview.chromium.org/2094933003/diff/60001/components/mus/ws/windo...
components/mus/ws/window_manager_state.h:34: class WindowManagerState : public
EventDispatcherDelegate {
On 2016/06/27 15:57:01, sadrul wrote:
> On 2016/06/27 15:54:26, sky wrote:
> > This class is intended to own state from the windowmanager. The
> > UserActivityManager maintains state from connections with the same user and
> not
> > necessarily from the windowmanager. The two should have different lifetimes.
> The
> > way you have things now there is no guarantee the windowmanagerstate has
been
> > created at the time you try to use it
> > (WindowServer::GetUserActivityMonitorForUser).
> > 
> > UserIdTracker tracks the set of known users. One option is to make UserId a
> real
> > class that has members and owns a UserActivityMonitor.
> 
> I had considered having MusApp::UserState own the UserActivityMonitor
>
(https://cs.chromium.org/chromium/src/components/mus/mus_app.cc?sq=package:chr...).
> Would that work?

I ended up moving the ownership into WindowServer instead. (Keeping it in
MusApp::UserState would be possible, but we would need to add extra interfaces
somewhere to notify the UserActivityMonitor when some activity happens).

https://codereview.chromium.org/2094933003/diff/80001/components/mus/ws/windo...
File components/mus/ws/window_server.cc (right):

https://codereview.chromium.org/2094933003/diff/80001/components/mus/ws/windo...
components/mus/ws/window_server.cc:46:
OnUserIdAdded(user_id_tracker_.active_id());
This is necessary since the 'root user id' is added upon construction of
UserIdTracker, and the observers are not notified of it.

I didn't RemoveObserver() from user_id_tracker_ from the dtor since at that
point, UserIdTracker is also being destroyed.

[sky, 2016-06-27 21:47:24]

LGTM

[sadrul, 2016-06-27 23:39:49]

sadrul@chromium.org changed reviewers:
+ meacer@chromium.org

[sadrul, 2016-06-27 23:39:50]

+meacer@ for the change in user_activity_monitor.mojom

[meacer, 2016-06-28 00:30:12]

Lgtm

https://codereview.chromium.org/2094933003/diff/80001/components/mus/public/i...
File components/mus/public/interfaces/user_activity_monitor.mojom (right):

https://codereview.chromium.org/2094933003/diff/80001/components/mus/public/i...
components/mus/public/interfaces/user_activity_monitor.mojom:31:
UserIdleObserver observer);
indentation

https://codereview.chromium.org/2094933003/diff/80001/components/mus/ws/user_...
File components/mus/ws/user_activity_monitor.cc (right):

https://codereview.chromium.org/2094933003/diff/80001/components/mus/ws/user_...
components/mus/ws/user_activity_monitor.cc:76: info.idle_state =
mojom::UserIdleObserver::IdleState::IDLE;
I slightly prefer |info.idle_state = user_is_active ? ACTIVE: IDLE;| here.

https://codereview.chromium.org/2094933003/diff/80001/components/mus/ws/user_...
File components/mus/ws/user_activity_monitor.h (right):

https://codereview.chromium.org/2094933003/diff/80001/components/mus/ws/user_...
components/mus/ws/user_activity_monitor.h:32: void
Add(mojom::UserActivityMonitorRequest request);
nit: Missing documentation

[sadrul, 2016-06-28 03:04:13]

https://codereview.chromium.org/2094933003/diff/80001/components/mus/public/i...
File components/mus/public/interfaces/user_activity_monitor.mojom (right):

https://codereview.chromium.org/2094933003/diff/80001/components/mus/public/i...
components/mus/public/interfaces/user_activity_monitor.mojom:31:
UserIdleObserver observer);
On 2016/06/28 00:30:12, Mustafa Emre Acer wrote:
> indentation

Done.

https://codereview.chromium.org/2094933003/diff/80001/components/mus/ws/user_...
File components/mus/ws/user_activity_monitor.cc (right):

https://codereview.chromium.org/2094933003/diff/80001/components/mus/ws/user_...
components/mus/ws/user_activity_monitor.cc:76: info.idle_state =
mojom::UserIdleObserver::IdleState::IDLE;
On 2016/06/28 00:30:12, Mustafa Emre Acer wrote:
> I slightly prefer |info.idle_state = user_is_active ? ACTIVE: IDLE;| here.

Done.

https://codereview.chromium.org/2094933003/diff/80001/components/mus/ws/user_...
File components/mus/ws/user_activity_monitor.h (right):

https://codereview.chromium.org/2094933003/diff/80001/components/mus/ws/user_...
components/mus/ws/user_activity_monitor.h:32: void
Add(mojom::UserActivityMonitorRequest request);
On 2016/06/28 00:30:12, Mustafa Emre Acer wrote:
> nit: Missing documentation

Done.

[sadrul, 2016-06-28 03:04:23]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2016-06-28 03:04:24]

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

[commit-bot: I haz the power, 2016-06-28 03:04:36]

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-06-28 03:47:29]

Description was changed from

==========
mus: Add UserActivityMonitor.

Introduce UserActivityMonitor in mus. Clients can install UserActivityObserver
to get notified of repeated user-activity, and UserIdlenessObserver to get
notified of when the user becomes idle/active.

BUG=623296
==========

to

==========
mus: Add UserActivityMonitor.

Introduce UserActivityMonitor in mus. Clients can install UserActivityObserver
to get notified of repeated user-activity, and UserIdlenessObserver to get
notified of when the user becomes idle/active.

BUG=623296
==========

[commit-bot: I haz the power, 2016-06-28 03:47:30]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-28 03:50:01]

Description was changed from

==========
mus: Add UserActivityMonitor.

Introduce UserActivityMonitor in mus. Clients can install UserActivityObserver
to get notified of repeated user-activity, and UserIdlenessObserver to get
notified of when the user becomes idle/active.

BUG=623296
==========

to

==========
mus: Add UserActivityMonitor.

Introduce UserActivityMonitor in mus. Clients can install UserActivityObserver
to get notified of repeated user-activity, and UserIdlenessObserver to get
notified of when the user becomes idle/active.

BUG=623296

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

[commit-bot: I haz the power, 2016-06-28 03:50:03]

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