Unlock Chrome when a phone with Smart Lock service is found.

Unlock Chrome when a phone with Smart Lock service is found.

This CL starts finding BLE phone devices that advertise smart
lock services when the screen is locked. It unlocks the
Chrome when such a device is found.

BUG=478073
Committed: https://crrev.com/3f45fd5444081f44205896e6c6a211fbfaa98ed5
Cr-Commit-Position: refs/heads/master@{#327416}

[msarda, 2015-04-24 13:17:30]

msarda@chromium.org changed reviewers:
+ sacomoto@chromium.org

[msarda, 2015-04-24 13:17:31]

Please take a look.

[msarda, 2015-04-27 16:13:49]

msarda@chromium.org changed reviewers:
+ isherman@chromium.org, jochen@chromium.org

[msarda, 2015-04-27 16:13:50]

Ilya: Please do an overall review of this CL.
Jochen: Please do a review of components/proximity_auth/DEPS.

Thank you.

[jochen (gone - plz use gerrit), 2015-04-27 19:53:09]

deps lgtm

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
File components/proximity_auth/DEPS (right):

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
components/proximity_auth/DEPS:2: "+content/public/browser/browser_context.h",
why not just allow all of c/p/browser?

[Ilya Sherman, 2015-04-28 01:02:33]

Thanks, Mihai.

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
File components/proximity_auth/ble/proximity_auth_ble_system.cc (right):

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
components/proximity_auth/ble/proximity_auth_ble_system.cc:11: #include
"content/public/browser/browser_context.h"
I don't think this include is actually used -- the BrowserContext pointer is
never dereferenced.

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
components/proximity_auth/ble/proximity_auth_ble_system.cc:17: const char
kSmartLockServiceUUID[] = "b3b7e28e-a000-3e17-bd86-6e97b9e28c11";
nit: Please leave a blank line after this one.

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
components/proximity_auth/ble/proximity_auth_ble_system.cc:18: }
nit: "  // namespace"

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
components/proximity_auth/ble/proximity_auth_ble_system.cc:20: namespace
proximity_auth {
nit: Please move this to be above the line declaring the anonymous namespace.

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
File components/proximity_auth/ble/proximity_auth_ble_system.h (right):

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
components/proximity_auth/ble/proximity_auth_ble_system.h:31:
content::BrowserContext* browser_context);
nit: Reminder: If you remove the |browser_context| as I recommend below, this
will need to be marked as "explicit".  (I wish we had an automated check for
this -- it's very easy to forget.)

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
components/proximity_auth/ble/proximity_auth_ble_system.h:48: browser_context_; 
// Not owned. Must outlive this object.
Hmm, before, we'd discussed keeping the BLE code's dependencies minimal, so that
we could build an app outside of Chrome, without pulling in a ton of
dependencies.  Maybe the ProximityAuthClient methods shouldn't take a
|BrowserContext| parameter?  The concrete implementation in Chrome can easily
find the appropriate BrowserContext anyway.

[msarda, 2015-04-28 12:44:59]

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
File components/proximity_auth/DEPS (right):

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
components/proximity_auth/DEPS:2: "+content/public/browser/browser_context.h",
On 2015/04/27 19:53:09, jochen wrote:
> why not just allow all of c/p/browser?

I finally removed this DEPS check.

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
File components/proximity_auth/ble/proximity_auth_ble_system.cc (right):

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
components/proximity_auth/ble/proximity_auth_ble_system.cc:11: #include
"content/public/browser/browser_context.h"
On 2015/04/28 01:02:33, Ilya Sherman wrote:
> I don't think this include is actually used -- the BrowserContext pointer is
> never dereferenced.

Good catch. Thank you.

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
components/proximity_auth/ble/proximity_auth_ble_system.cc:17: const char
kSmartLockServiceUUID[] = "b3b7e28e-a000-3e17-bd86-6e97b9e28c11";
On 2015/04/28 01:02:33, Ilya Sherman wrote:
> nit: Please leave a blank line after this one.

Done.

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
components/proximity_auth/ble/proximity_auth_ble_system.cc:18: }
On 2015/04/28 01:02:33, Ilya Sherman wrote:
> nit: "  // namespace"

Done.

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
components/proximity_auth/ble/proximity_auth_ble_system.cc:20: namespace
proximity_auth {
On 2015/04/28 01:02:33, Ilya Sherman wrote:
> nit: Please move this to be above the line declaring the anonymous namespace.

Done.

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
File components/proximity_auth/ble/proximity_auth_ble_system.h (right):

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
components/proximity_auth/ble/proximity_auth_ble_system.h:31:
content::BrowserContext* browser_context);
On 2015/04/28 01:02:33, Ilya Sherman wrote:
> nit: Reminder: If you remove the |browser_context| as I recommend below, this
> will need to be marked as "explicit".  (I wish we had an automated check for
> this -- it's very easy to forget.)

I kept the browser_context but removed the include and the dependency. I think
that is fine. I need it to actually unlock the screenlock_bridge. What do you
think?

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
components/proximity_auth/ble/proximity_auth_ble_system.h:48: browser_context_; 
// Not owned. Must outlive this object.
On 2015/04/28 01:02:33, Ilya Sherman wrote:
> Hmm, before, we'd discussed keeping the BLE code's dependencies minimal, so
that
> we could build an app outside of Chrome, without pulling in a ton of
> dependencies.  Maybe the ProximityAuthClient methods shouldn't take a
> |BrowserContext| parameter?  The concrete implementation in Chrome can easily
> find the appropriate BrowserContext anyway.

I removed the dependency and the DEPS rule as they are not needed.

[Ilya Sherman, 2015-04-28 23:21:25]

LGTM

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
File components/proximity_auth/ble/proximity_auth_ble_system.h (right):

https://codereview.chromium.org/1102473003/diff/80002/components/proximity_au...
components/proximity_auth/ble/proximity_auth_ble_system.h:31:
content::BrowserContext* browser_context);
On 2015/04/28 12:44:59, msarda wrote:
> On 2015/04/28 01:02:33, Ilya Sherman wrote:
> > nit: Reminder: If you remove the |browser_context| as I recommend below,
this
> > will need to be marked as "explicit".  (I wish we had an automated check for
> > this -- it's very easy to forget.)
> 
> I kept the browser_context but removed the include and the dependency. I think
> that is fine. I need it to actually unlock the screenlock_bridge. What do you
> think?

I think it would be better to eliminate the hint of a content dependency
entirely, but we can do that in a separate CL if you prefer.

[Ilya Sherman, 2015-04-28 23:21:48]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-04-28 23:21:49]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1102473003/#ps110001
(title: "Nits")

[commit-bot: I haz the power, 2015-04-28 23:23:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1102473003/110001

[commit-bot: I haz the power, 2015-04-29 02:04:55]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2015-04-29 02:05:42]

Patchset 7 (id:??) landed as
https://crrev.com/3f45fd5444081f44205896e6c6a211fbfaa98ed5
Cr-Commit-Position: refs/heads/master@{#327416}