Introduce lock_screen_apps to chrome/browser/chromeos

Introduce lock_screen_apps to chrome/browser/chromeos

Adds a flag to guard the feature.

Adds folder to chrome/browser/chromeos to be used for implementing
support for (note taking) lock screen apps.

Initially, only an action state controller is added, which currently
doesn't do much, but this is intended to change - the class in question
will be used to communicate the action handling state to different parts
of the system.

BUG=715781
Review-Url: https://codereview.chromium.org/2839383002
Cr-Commit-Position: refs/heads/master@{#468779}
Committed: https://chromium.googlesource.com/chromium/src/+/86ccfebb8d4cd066b640d742e6dcd6043cf04cf4

[tbarzic, 2017-04-27 00:57:36]

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

[commit-bot: I haz the power, 2017-04-27 00:58:15]

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-04-27 01:33:40]

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

[commit-bot: I haz the power, 2017-04-27 01:33:41]

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

[tbarzic, 2017-04-27 21:08:26]

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

[commit-bot: I haz the power, 2017-04-27 21:09:11]

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-04-27 21:42:46]

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

[commit-bot: I haz the power, 2017-04-27 21:42:47]

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

[tbarzic, 2017-04-28 03:57:09]

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

[commit-bot: I haz the power, 2017-04-28 03:57:50]

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-04-28 04:59:46]

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

[commit-bot: I haz the power, 2017-04-28 04:59:47]

Dry run: This issue passed the CQ dry run.

[tbarzic, 2017-04-28 22:01:11]

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

[commit-bot: I haz the power, 2017-04-28 22:02:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbarzic, 2017-04-28 22:50:18]

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

[commit-bot: I haz the power, 2017-04-28 22:50:58]

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-04-28 23:09:03]

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

[commit-bot: I haz the power, 2017-04-28 23:09:04]

Dry run: Try jobs failed on following builders:
  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-...)

[tbarzic, 2017-05-01 18:12:57]

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

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

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-05-01 19:22:42]

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

[commit-bot: I haz the power, 2017-05-01 19:22:44]

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

[tbarzic, 2017-05-02 00:15:52]

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

[commit-bot: I haz the power, 2017-05-02 00:16:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbarzic, 2017-05-02 00:21:44]

tbarzic@chromium.org changed reviewers:
+ jdufault@chromium.org, xiyuan@chromium.org

[tbarzic, 2017-05-02 00:21:45]

This doesn't do much yet, but the plan is for this to be the communication point
between extension APIs, ash (for launching new note action), lock screen UI,
etc.

[commit-bot: I haz the power, 2017-05-02 02:31:46]

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

[commit-bot: I haz the power, 2017-05-02 02:31:47]

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

[xiyuan, 2017-05-02 18:12:23]

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/lock_screen_apps/state_controller.cc (right):

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/state_controller.cc:66: ActionState
action_state) {
nit: DCHECK_EQ(Action::kNewNote, action);

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/state_controller.cc:67: ActionState
old_state = GetActionState(action);
nit: const

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/lock_screen_apps/state_controller.h (right):

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/state_controller.h:14: class
StateObserver;
nit: move this down after line 17. Prevailing style leverage's existing
namespace for forward decls.

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/state_controller.h:34: void
HandleAction(Action action);
Do we care about whether the action is handled? e.g. return a bool to indicate
that.

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/state_controller.h:48: bool
UpdateActionState(Action action, ActionState state);
How do we plan to change state with this method being private? By HandleAction()
?

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc
(right):

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc:28:
state_change_count_++;
nit: pre-inc, i.e. ++state_change_count_;

[tbarzic, 2017-05-02 18:43:37]

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/lock_screen_apps/state_controller.cc (right):

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/state_controller.cc:66: ActionState
action_state) {
On 2017/05/02 18:12:22, xiyuan wrote:
> nit: DCHECK_EQ(Action::kNewNote, action);

Done.

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/state_controller.cc:67: ActionState
old_state = GetActionState(action);
On 2017/05/02 18:12:22, xiyuan wrote:
> nit: const

Done.

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/lock_screen_apps/state_controller.h (right):

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/state_controller.h:14: class
StateObserver;
On 2017/05/02 18:12:23, xiyuan wrote:
> nit: move this down after line 17. Prevailing style leverage's existing
> namespace for forward decls.

Done.

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/state_controller.h:34: void
HandleAction(Action action);
On 2017/05/02 18:12:23, xiyuan wrote:
> Do we care about whether the action is handled? e.g. return a bool to indicate
> that.

Not necessarily, but it could be useful, e.g. for tests.

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/state_controller.h:48: bool
UpdateActionState(Action action, ActionState state);
On 2017/05/02 18:12:23, xiyuan wrote:
> How do we plan to change state with this method being private? By
HandleAction()
> ?

Yes, I plan not to have this expose explicit update state method. Instead it
will expose specific actions (like HandleAction, or MoveToBackground, observers
for session state (to disable actions on user unlock, to track state of
registered apps)) that will internally update the state.

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc
(right):

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc:28:
state_change_count_++;
On 2017/05/02 18:12:23, xiyuan wrote:
> nit: pre-inc, i.e. ++state_change_count_;

Done.

[xiyuan, 2017-05-02 19:23:03]

lgtm

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/lock_screen_apps/state_controller.h (right):

https://codereview.chromium.org/2839383002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/state_controller.h:48: bool
UpdateActionState(Action action, ActionState state);
On 2017/05/02 18:43:37, tbarzic wrote:
> On 2017/05/02 18:12:23, xiyuan wrote:
> > How do we plan to change state with this method being private? By
> HandleAction()
> > ?
> 
> Yes, I plan not to have this expose explicit update state method. Instead it
> will expose specific actions (like HandleAction, or MoveToBackground,
observers
> for session state (to disable actions on user unlock, to track state of
> registered apps)) that will internally update the state.

Acknowledged.

[jdufault, 2017-05-02 20:16:03]

lgtm

https://codereview.chromium.org/2839383002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/lock_screen_apps/OWNERS (right):

https://codereview.chromium.org/2839383002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/OWNERS:1: jdufault@chromium.org
I'm probably not going to be writing much code for this feature so feel free to
remove me from OWNERs.

https://codereview.chromium.org/2839383002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/lock_screen_apps/state_controller.cc (right):

https://codereview.chromium.org/2839383002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/state_controller.cc:70: const
ActionState kOldState = GetActionState(action);
kOldState => old_state? I don't think constant style naming applies because
kOldState varies throughout program execution.

https://codereview.chromium.org/2839383002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/lock_screen_apps/state_controller.h (right):

https://codereview.chromium.org/2839383002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/state_controller.h:17: // Manages state
of lock scree action handler apps, and notifies
nit: scree -> screen.

[jdufault, 2017-05-02 20:16:04]

lgtm

[tbarzic, 2017-05-02 20:20:12]

https://codereview.chromium.org/2839383002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/lock_screen_apps/OWNERS (right):

https://codereview.chromium.org/2839383002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/OWNERS:1: jdufault@chromium.org
On 2017/05/02 20:16:02, jdufault wrote:
> I'm probably not going to be writing much code for this feature so feel free
to
> remove me from OWNERs.

Done.

https://codereview.chromium.org/2839383002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/lock_screen_apps/state_controller.cc (right):

https://codereview.chromium.org/2839383002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/state_controller.cc:70: const
ActionState kOldState = GetActionState(action);
On 2017/05/02 20:16:02, jdufault wrote:
> kOldState => old_state? I don't think constant style naming applies because
> kOldState varies throughout program execution.

Done.

https://codereview.chromium.org/2839383002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/lock_screen_apps/state_controller.h (right):

https://codereview.chromium.org/2839383002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/lock_screen_apps/state_controller.h:17: // Manages state
of lock scree action handler apps, and notifies
On 2017/05/02 20:16:02, jdufault wrote:
> nit: scree -> screen.

Done.

[tbarzic, 2017-05-02 20:24:01]

The CQ bit was checked by tbarzic@chromium.org

[tbarzic, 2017-05-02 20:24:02]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
jdufault@chromium.org
Link to the patchset: https://codereview.chromium.org/2839383002/#ps160001
(title: ".")

[commit-bot: I haz the power, 2017-05-02 20:24:48]

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-05-02 21:27:05]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493756641773100,
"parent_rev": "97188b254443246d4ff70df35fc0078c49d69096", "commit_rev":
"86ccfebb8d4cd066b640d742e6dcd6043cf04cf4"}

[commit-bot: I haz the power, 2017-05-02 21:27:21]

Description was changed from

==========
Introduce lock_screen_apps to chrome/browser/chromeos

Adds a flag to guard the feature.

Adds folder to chrome/browser/chromeos to be used for implementing
support for (note taking) lock screen apps.

Initially, only an action state controller is added, which currently
doesn't do much, but this is intended to change - the class in question
will be used to communicate the action handling state to different parts
of the system.

BUG=715781
==========

to

==========
Introduce lock_screen_apps to chrome/browser/chromeos

Adds a flag to guard the feature.

Adds folder to chrome/browser/chromeos to be used for implementing
support for (note taking) lock screen apps.

Initially, only an action state controller is added, which currently
doesn't do much, but this is intended to change - the class in question
will be used to communicate the action handling state to different parts
of the system.

BUG=715781

Review-Url: https://codereview.chromium.org/2839383002
Cr-Commit-Position: refs/heads/master@{#468779}
Committed:
https://chromium.googlesource.com/chromium/src/+/86ccfebb8d4cd066b640d742e6dc...
==========

[commit-bot: I haz the power, 2017-05-02 21:27:22]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/86ccfebb8d4cd066b640d742e6dc...

[James Cook, 2017-05-03 23:30:38]

On 2017/05/02 21:27:22, commit-bot: I haz the power wrote:
> Committed patchset #9 (id:160001) as
>
https://chromium.googlesource.com/chromium/src/+/86ccfebb8d4cd066b640d742e6dc...

Drive-by: Can the StateController be owned by some other object instead of being
a singleton? Like maybe ChromeBrowserMainPartsChromeOS? I'm OK with there still
being a static Get() method, but then at least the lifetime will be easier to
understand.