chromeos: Convert system tray session length limit to mojo

chromeos: Convert system tray session length limit to mojo

Chrome and ash run in different processes under mustash so we need to
eliminate ash-to-chrome delegate methods. This eliminates two methods on
ash::SystemTrayDelegate.

Use the SessionController mojom interface to send the session length limit
to ash from Chrome's local state preferences. (The preferences mojo service
does not yet support local state prefs.)

BUG=712759
TEST=added to ash_unittests, chrome unit_tests, manually set up a managed
device using go/dmstug, set the pref via policy and verify that the menu
item and notification appear as expected

Review-Url: https://codereview.chromium.org/2923083002
Cr-Commit-Position: refs/heads/master@{#477646}
Committed: https://chromium.googlesource.com/chromium/src/+/c934896452fd0d3e518d1ffe77f68e0a25e66b34

[James Cook, 2017-06-05 23:19:03]

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

[commit-bot: I haz the power, 2017-06-05 23:19:38]

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-06-05 23:23:47]

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

[James Cook, 2017-06-05 23:23:48]

msw, please take a look.

(A moral victory - this CL adds tests, yet the line count goes down!)

https://codereview.chromium.org/2923083002/diff/1/ash/system/session/tray_ses...
File ash/system/session/tray_session_length_limit.cc (left):

https://codereview.chromium.org/2923083002/diff/1/ash/system/session/tray_ses...
ash/system/session/tray_session_length_limit.cc:93:
delegate->GetSessionLengthLimit(&time_limit_)) {
As far as I can tell there was never a need for the boolean return values of
these Get() methods. The UI is only relevant when there is a non-zero length
limit and a non-null start time, and the implementation code in chrome always
fills in both.

https://codereview.chromium.org/2923083002/diff/1/chrome/browser/ui/ash/syste...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (left):

https://codereview.chromium.org/2923083002/diff/1/chrome/browser/ui/ash/syste...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:439: session_started_ =
true;
This was unused.

[commit-bot: I haz the power, 2017-06-06 03:37:08]

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

[commit-bot: I haz the power, 2017-06-06 03:37:12]

Dry run: This issue passed the CQ dry run.

[msw, 2017-06-06 19:01:05]

nice! lgtm with minor nits

https://codereview.chromium.org/2923083002/diff/1/ash/public/interfaces/sessi...
File ash/public/interfaces/session_controller.mojom (right):

https://codereview.chromium.org/2923083002/diff/1/ash/public/interfaces/sessi...
ash/public/interfaces/session_controller.mojom:146: // Creates or updates a
notification saying the session length is limited
optional nit: rephrase as "Adds a countdown timer to the system tray menu and
creates or updates a notification saying the session length is limited (e.g...."

https://codereview.chromium.org/2923083002/diff/1/ash/session/session_control...
File ash/session/session_controller.h (right):

https://codereview.chromium.org/2923083002/diff/1/ash/session/session_control...
ash/session/session_controller.h:189: // Zero if there is no session length
limit.
nit: "The session length limit; set to zero if there is no limit."

https://codereview.chromium.org/2923083002/diff/1/ash/session/session_control...
ash/session/session_controller.h:192: // Stored in a pref in case of a crash in
the middle of the session. Can
nit: "The session start time, set at login or on the first user activity; set to
null if there is no session length limit. This value is also stored in a pref in
case of a crash during the session."

https://codereview.chromium.org/2923083002/diff/1/ash/system/session/tray_ses...
File ash/system/session/tray_session_length_limit.cc (left):

https://codereview.chromium.org/2923083002/diff/1/ash/system/session/tray_ses...
ash/system/session/tray_session_length_limit.cc:93:
delegate->GetSessionLengthLimit(&time_limit_)) {
On 2017/06/05 23:23:48, James Cook (OOO Tue) wrote:
> As far as I can tell there was never a need for the boolean return values of
> these Get() methods. The UI is only relevant when there is a non-zero length
> limit and a non-null start time, and the implementation code in chrome always
> fills in both.

Acknowledged.

https://codereview.chromium.org/2923083002/diff/1/ash/system/session/tray_ses...
File ash/system/session/tray_session_length_limit_unittest.cc (right):

https://codereview.chromium.org/2923083002/diff/1/ash/system/session/tray_ses...
ash/system/session/tray_session_length_limit_unittest.cc:27: LabelTrayView*
GetTrayBubbleView() {
nit: maybe add some session length limit context to this helper's name?

https://codereview.chromium.org/2923083002/diff/1/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2923083002/diff/1/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:604: TraySessionLengthLimit*
SystemTray::GetTraySessionLengthLimitForTesting()
aside: these could all be simple accessors inlined in the header.

https://codereview.chromium.org/2923083002/diff/1/ash/test/test_system_tray_d...
File ash/test/test_system_tray_delegate.cc (right):

https://codereview.chromium.org/2923083002/diff/1/ash/test/test_system_tray_d...
ash/test/test_system_tray_delegate.cc:7: #include <string>
nit: remove

https://codereview.chromium.org/2923083002/diff/1/ash/test/test_system_tray_d...
ash/test/test_system_tray_delegate.cc:9: #include "ash/shell.h"
nit: remove

https://codereview.chromium.org/2923083002/diff/1/ash/test/test_system_tray_d...
ash/test/test_system_tray_delegate.cc:10: #include "base/time/time.h"
nit: remove

https://codereview.chromium.org/2923083002/diff/1/chrome/browser/ui/ash/sessi...
File chrome/browser/ui/ash/session_controller_client.cc (right):

https://codereview.chromium.org/2923083002/diff/1/chrome/browser/ui/ash/sessi...
chrome/browser/ui/ash/session_controller_client.cc:142:
local_state_registrar_.reset(new PrefChangeRegistrar);
nit: MakeUnique

https://codereview.chromium.org/2923083002/diff/1/chrome/browser/ui/ash/sessi...
chrome/browser/ui/ash/session_controller_client.cc:177:
SendSessionLengthLimit();
nit: put above comment, so this seems less associated with the comment.

[James Cook, 2017-06-07 00:31:49]

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

[commit-bot: I haz the power, 2017-06-07 00:32:10]

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-06-07 00:48:12]

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

[James Cook, 2017-06-07 00:48:14]

dcheng, can I get owners approval for
ash/public/interfaces/session_controller.mojom? tsepez@ is OOO. This is
refactoring for go/mustash, for communication between the ash system ui process
and chrome browser.

msw, thanks for the review!

https://codereview.chromium.org/2923083002/diff/1/ash/public/interfaces/sessi...
File ash/public/interfaces/session_controller.mojom (right):

https://codereview.chromium.org/2923083002/diff/1/ash/public/interfaces/sessi...
ash/public/interfaces/session_controller.mojom:146: // Creates or updates a
notification saying the session length is limited
On 2017/06/06 19:01:04, msw wrote:
> optional nit: rephrase as "Adds a countdown timer to the system tray menu and
> creates or updates a notification saying the session length is limited
(e.g...."

Done.

https://codereview.chromium.org/2923083002/diff/1/ash/session/session_control...
File ash/session/session_controller.h (right):

https://codereview.chromium.org/2923083002/diff/1/ash/session/session_control...
ash/session/session_controller.h:189: // Zero if there is no session length
limit.
On 2017/06/06 19:01:04, msw wrote:
> nit: "The session length limit; set to zero if there is no limit."

Done.

https://codereview.chromium.org/2923083002/diff/1/ash/session/session_control...
ash/session/session_controller.h:192: // Stored in a pref in case of a crash in
the middle of the session. Can
On 2017/06/06 19:01:04, msw wrote:
> nit: "The session start time, set at login or on the first user activity; set
to
> null if there is no session length limit. This value is also stored in a pref
in
> case of a crash during the session."

Done.

https://codereview.chromium.org/2923083002/diff/1/ash/system/session/tray_ses...
File ash/system/session/tray_session_length_limit_unittest.cc (right):

https://codereview.chromium.org/2923083002/diff/1/ash/system/session/tray_ses...
ash/system/session/tray_session_length_limit_unittest.cc:27: LabelTrayView*
GetTrayBubbleView() {
On 2017/06/06 19:01:04, msw wrote:
> nit: maybe add some session length limit context to this helper's name?

Done.

https://codereview.chromium.org/2923083002/diff/1/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2923083002/diff/1/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:604: TraySessionLengthLimit*
SystemTray::GetTraySessionLengthLimitForTesting()
On 2017/06/06 19:01:04, msw wrote:
> aside: these could all be simple accessors inlined in the header.

Acknowledged. I plan to introduce a testapi class, so I just stayed consistent
with the existing ones for now.

https://codereview.chromium.org/2923083002/diff/1/ash/test/test_system_tray_d...
File ash/test/test_system_tray_delegate.cc (right):

https://codereview.chromium.org/2923083002/diff/1/ash/test/test_system_tray_d...
ash/test/test_system_tray_delegate.cc:7: #include <string>
On 2017/06/06 19:01:04, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2923083002/diff/1/ash/test/test_system_tray_d...
ash/test/test_system_tray_delegate.cc:9: #include "ash/shell.h"
On 2017/06/06 19:01:04, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2923083002/diff/1/ash/test/test_system_tray_d...
ash/test/test_system_tray_delegate.cc:10: #include "base/time/time.h"
On 2017/06/06 19:01:04, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2923083002/diff/1/chrome/browser/ui/ash/sessi...
File chrome/browser/ui/ash/session_controller_client.cc (right):

https://codereview.chromium.org/2923083002/diff/1/chrome/browser/ui/ash/sessi...
chrome/browser/ui/ash/session_controller_client.cc:142:
local_state_registrar_.reset(new PrefChangeRegistrar);
On 2017/06/06 19:01:05, msw wrote:
> nit: MakeUnique

Done.

https://codereview.chromium.org/2923083002/diff/1/chrome/browser/ui/ash/sessi...
chrome/browser/ui/ash/session_controller_client.cc:177:
SendSessionLengthLimit();
On 2017/06/06 19:01:04, msw wrote:
> nit: put above comment, so this seems less associated with the comment.

Done.

[commit-bot: I haz the power, 2017-06-07 02:06:32]

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

[commit-bot: I haz the power, 2017-06-07 02:06:33]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-06-07 06:49:20]

ipc lgtm

[James Cook, 2017-06-07 15:04:04]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2017-06-07 15:04:04]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2923083002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-06-07 15:04:24]

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-06-07 15:10:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496847844067510,
"parent_rev": "4808ce31616eb64d5526fb8fa9ef8b8f6d992df3", "commit_rev":
"c934896452fd0d3e518d1ffe77f68e0a25e66b34"}

[commit-bot: I haz the power, 2017-06-07 15:10:47]

Description was changed from

==========
chromeos: Convert system tray session length limit to mojo

Chrome and ash run in different processes under mustash so we need to
eliminate ash-to-chrome delegate methods. This eliminates two methods on
ash::SystemTrayDelegate.

Use the SessionController mojom interface to send the session length limit
to ash from Chrome's local state preferences. (The preferences mojo service
does not yet support local state prefs.)

BUG=712759
TEST=added to ash_unittests, chrome unit_tests, manually set up a managed
device using go/dmstug, set the pref via policy and verify that the menu
item and notification appear as expected
==========

to

==========
chromeos: Convert system tray session length limit to mojo

Chrome and ash run in different processes under mustash so we need to
eliminate ash-to-chrome delegate methods. This eliminates two methods on
ash::SystemTrayDelegate.

Use the SessionController mojom interface to send the session length limit
to ash from Chrome's local state preferences. (The preferences mojo service
does not yet support local state prefs.)

BUG=712759
TEST=added to ash_unittests, chrome unit_tests, manually set up a managed
device using go/dmstug, set the pref via policy and verify that the menu
item and notification appear as expected

Review-Url: https://codereview.chromium.org/2923083002
Cr-Commit-Position: refs/heads/master@{#477646}
Committed:
https://chromium.googlesource.com/chromium/src/+/c934896452fd0d3e518d1ffe77f6...
==========

[commit-bot: I haz the power, 2017-06-07 15:10:49]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/c934896452fd0d3e518d1ffe77f6...