The chrome.screenlockPrivate API allows select apps to control the ChromeOS ScreenLocker.

The chrome.screenlockPrivate API allows select apps to control the ChromeOS ScreenLocker.

https://docs.google.com/document/d/1kSxngeyBPZIB7kK-QfDXgpzrDBvnVBcJfbK3HMFQIZg/edit

The live version of the whitelisted app uses the chrome.usb API to execute a challenge-response protocol with an experimental device that has received a provisional green light from Google Security Team for unlocking.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235529

[benjhayden, 2013-11-07 18:08:16]

First draft.

[xiyuan, 2013-11-07 19:08:10]

+mpcomplete for extension API

It is probably good to also send the proposal to extensions-dev group for
discussion.

I am not sure whether this should be a public API since we don't want a random
app to able to unlock the user's device. For private api, usually we have
Private suffix.

If the API is chromeos only, the code should be moved to
c/b/chromeos/extensions/ where chromeosPrivate api code lives.

[Matt Perry, 2013-11-07 19:41:35]

Please follow the instructions at http://go/newchromeapi. (It applies to private
APIs as well, but the review should be a bit easier.)

[benjhayden, 2013-11-11 23:44:30]

Done and done.

[Matt Perry, 2013-11-12 21:26:30]

https://codereview.chromium.org/60583003/diff/60001/chrome/common/extensions/...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/60583003/diff/60001/chrome/common/extensions/...
chrome/common/extensions/api/_api_features.json:314: "channel": "stable",
you don't need this line - thedependency on the permission is enough.

https://codereview.chromium.org/60583003/diff/60001/chrome/common/extensions/...
chrome/common/extensions/api/_api_features.json:318: "whitelist":
["begdogjjbmiahnbahpidlnbgpbjbpajl"],
ditto.

https://codereview.chromium.org/60583003/diff/60001/chrome/common/extensions/...
File chrome/common/extensions/api/screenlock_private.idl (right):

https://codereview.chromium.org/60583003/diff/60001/chrome/common/extensions/...
chrome/common/extensions/api/screenlock_private.idl:24: static void
onChange(boolean locked);
nit: onChanged to be consistent with other events of the same name.

https://codereview.chromium.org/60583003/diff/110001/chrome/common/extensions...
File
chrome/common/extensions/docs/examples/api/screenlock/weblocker/manifest.json
(right):

https://codereview.chromium.org/60583003/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/screenlock/weblocker/manifest.json:1:
{"name":"Lock/Unlock ChromeOS through the Web",
Could you turn this into an api test?

[benjhayden, 2013-11-13 00:46:22]

https://codereview.chromium.org/60583003/diff/60001/chrome/common/extensions/...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/60583003/diff/60001/chrome/common/extensions/...
chrome/common/extensions/api/_api_features.json:314: "channel": "stable",
On 2013/11/12 21:26:30, Matt Perry wrote:
> you don't need this line - thedependency on the permission is enough.

Done.

https://codereview.chromium.org/60583003/diff/60001/chrome/common/extensions/...
chrome/common/extensions/api/_api_features.json:318: "whitelist":
["begdogjjbmiahnbahpidlnbgpbjbpajl"],
On 2013/11/12 21:26:30, Matt Perry wrote:
> ditto.

Done.

https://codereview.chromium.org/60583003/diff/60001/chrome/common/extensions/...
File chrome/common/extensions/api/screenlock_private.idl (right):

https://codereview.chromium.org/60583003/diff/60001/chrome/common/extensions/...
chrome/common/extensions/api/screenlock_private.idl:24: static void
onChange(boolean locked);
On 2013/11/12 21:26:30, Matt Perry wrote:
> nit: onChanged to be consistent with other events of the same name.

Done.

https://codereview.chromium.org/60583003/diff/110001/chrome/common/extensions...
File
chrome/common/extensions/docs/examples/api/screenlock/weblocker/manifest.json
(right):

https://codereview.chromium.org/60583003/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/screenlock/weblocker/manifest.json:1:
{"name":"Lock/Unlock ChromeOS through the Web",
On 2013/11/12 21:26:30, Matt Perry wrote:
> Could you turn this into an api test?

Done.

[Matt Perry, 2013-11-13 01:01:14]

https://codereview.chromium.org/60583003/diff/230001/chrome/common/extensions...
File
chrome/common/extensions/docs/examples/api/screenlockPrivate/weblocker/bg.js
(right):

https://codereview.chromium.org/60583003/diff/230001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/screenlockPrivate/weblocker/bg.js:15:
xhr.open('get', 'http://scrap.mtv.corp.google.com:54329/value', true);
I wouldn't check this in with a reference to a corp URL.

https://codereview.chromium.org/60583003/diff/230001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/screenlockPrivate/weblocker/bg.js:15:
xhr.open('get', 'http://scrap.mtv.corp.google.com:54329/value', true);
Do you need this example now that there's an API test?

https://codereview.chromium.org/60583003/diff/230001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/screenlockPrivate/test.js (right):

https://codereview.chromium.org/60583003/diff/230001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/screenlockPrivate/test.js:16: function
watchdog(wrapped) {
Cool addition, but the test fixture has a built in timeout. I would just rely on
that and remove this to simplify the test code.

https://codereview.chromium.org/60583003/diff/230001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/screenlockPrivate/test.js:47:
console.log('step stage=' + stage + ' locked=' + locked);
remove the logs before checking in to reduce log spew

[benjhayden, 2013-11-13 17:17:50]

https://codereview.chromium.org/60583003/diff/230001/chrome/common/extensions...
File
chrome/common/extensions/docs/examples/api/screenlockPrivate/weblocker/bg.js
(right):

https://codereview.chromium.org/60583003/diff/230001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/screenlockPrivate/weblocker/bg.js:15:
xhr.open('get', 'http://scrap.mtv.corp.google.com:54329/value', true);
On 2013/11/13 01:01:14, Matt Perry wrote:
> Do you need this example now that there's an API test?

Done.

https://codereview.chromium.org/60583003/diff/230001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/screenlockPrivate/weblocker/bg.js:15:
xhr.open('get', 'http://scrap.mtv.corp.google.com:54329/value', true);
On 2013/11/13 01:01:14, Matt Perry wrote:
> I wouldn't check this in with a reference to a corp URL.

Done.

https://codereview.chromium.org/60583003/diff/230001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/screenlockPrivate/test.js (right):

https://codereview.chromium.org/60583003/diff/230001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/screenlockPrivate/test.js:16: function
watchdog(wrapped) {
On 2013/11/13 01:01:14, Matt Perry wrote:
> Cool addition, but the test fixture has a built in timeout. I would just rely
on
> that and remove this to simplify the test code.

Done.

https://codereview.chromium.org/60583003/diff/230001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/screenlockPrivate/test.js:47:
console.log('step stage=' + stage + ' locked=' + locked);
On 2013/11/13 01:01:14, Matt Perry wrote:
> remove the logs before checking in to reduce log spew

Done.

[xiyuan, 2013-11-13 20:47:29]

Mostly good from my side.

https://codereview.chromium.org/60583003/diff/370001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/screenlock_private_api.cc (right):

https://codereview.chromium.org/60583003/diff/370001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/screenlock_private_api.cc:44:
ScreenLocker::Show();
Let's do the locking via session manager.

BusThreadManager::Get()->GetSessionManagerClient()->RequestLockScreen();

This would make sure correct upstart signal is emitted and all interested
daemons are notified.

[Matt Perry, 2013-11-13 20:49:37]

lgtm

[benjhayden, 2013-11-13 21:42:21]

https://codereview.chromium.org/60583003/diff/370001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/screenlock_private_api.cc (right):

https://codereview.chromium.org/60583003/diff/370001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/screenlock_private_api.cc:44:
ScreenLocker::Show();
On 2013/11/13 20:47:30, xiyuan wrote:
> Let's do the locking via session manager.
> 
> BusThreadManager::Get()->GetSessionManagerClient()->RequestLockScreen();
> 
> This would make sure correct upstart signal is emitted and all interested
> daemons are notified.

Done.

[xiyuan, 2013-11-13 22:03:42]

lgtm

[benjhayden, 2013-11-13 22:44:21]

Should setLocked(false) go through the session manager or
ScreenLocker::UnlockOnLoginSuccess(), or does ScreenLocker::Hide() send all the
proper notifications?

[benjhayden, 2013-11-13 22:46:03]

CC Will FHI

[xiyuan, 2013-11-13 22:53:07]

On 2013/11/13 22:44:21, benjhayden_chromium wrote:
> Should setLocked(false) go through the session manager or
> ScreenLocker::UnlockOnLoginSuccess(), or does ScreenLocker::Hide() send all
the
> proper notifications?

ScreenLocker::Hide() should be sufficient to unlocking. ScreenLocker dtor will
send out notifications to session manager, which then emits signal.

[benjhayden, 2013-11-13 23:21:19]

erg@chromium.org: Please review changes in
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc

[Elliot Glaysher, 2013-11-13 23:27:18]

lgtm

[commit-bot: I haz the power, 2013-11-14 17:50:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/60583003/630001

[commit-bot: I haz the power, 2013-11-14 18:09:02]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[benjhayden, 2013-11-14 18:13:16]

Any opinions on whether I should make this review public or dcommit?

[xiyuan, 2013-11-14 18:17:30]

On 2013/11/14 18:13:16, benjhayden_chromium wrote:
> Any opinions on whether I should make this review public or dcommit?

I think it's okay to make this review public. The code goes to public repo
eventually.

[commit-bot: I haz the power, 2013-11-14 18:21:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/60583003/630001

[commit-bot: I haz the power, 2013-11-14 19:04:36]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-15 18:23:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/60583003/1120001

[commit-bot: I haz the power, 2013-11-15 20:32:10]

Retried try job too often on linux_chromeos_clang for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-11-16 02:37:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/60583003/1530001

[commit-bot: I haz the power, 2013-11-17 16:03:55]

Change committed as 235529

[korcikorci1984, 2015-01-28 18:03:39]

korcikorci1984@gmail.com changed reviewers:
+ korcikorci1984@gmail.com