Move screenlock_bridge to components/proximity_auth

Move ScreenlockBridge to components/proximity_auth.

This CL moves ScreenlockBridge to components/proximity_auth.

It also moves the usage of profile out of ScreenLockBridge as this
dependency is forbidden in //components.

TBR=rogerta,antrim,rdevlin.cronin,tengs

BUG=479670
Committed: https://crrev.com/f80a09109fae223241b5462e20a3556edfbd6740
Cr-Commit-Position: refs/heads/master@{#327049}

[msarda, 2015-04-21 16:02:56]

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

[msarda, 2015-04-21 16:02:57]

Ilya: This CL is still missing a BUILD.gn file. I just want to have a validation
for my approach before I make that nit.

https://codereview.chromium.org/1096293003/diff/60001/components/signin.gypi
File components/signin.gypi (right):

https://codereview.chromium.org/1096293003/diff/60001/components/signin.gypi#...
components/signin.gypi:155: # GN version: //components/signin/content:content
This file was not yet added. I plan to add it in a next patch.

https://codereview.chromium.org/1096293003/diff/60001/components/signin/conte...
File components/signin/content/screenlock_bridge.h (right):

https://codereview.chromium.org/1096293003/diff/60001/components/signin/conte...
components/signin/content/screenlock_bridge.h:134: virtual void
Unlock(content::BrowserContext* browser_context) = 0;
These 2 methods are both required here as we do not have access to the
SigninManagerFactory from //components.

[Ilya Sherman, 2015-04-21 23:02:39]

Thanks, Mihai.

https://codereview.chromium.org/1096293003/diff/10040/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/1096293003/diff/10040/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:502:
SigninManagerFactory::GetForProfile(profile);
This is not exactly equivalent to the previous implementation.  Could you please
preserve the previous implementation?

https://codereview.chromium.org/1096293003/diff/10040/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:504: }
Rather than duplicating the implementations of these methods here and in
user_manager_screen_handler.cc, I'd prefer to share the implementation.  One
reasonable approach is to use a client interface, as we do in other components.

https://codereview.chromium.org/1096293003/diff/10040/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/1096293003/diff/10040/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:96:
->GetAuthenticatedUsername();
Please keep the comment from screenlock_bridge.cc along with this code.  Better
yet, take me up on my suggestion to use a client interface, and call into that
interface here.

https://codereview.chromium.org/1096293003/diff/10040/components/signin/conte...
File components/signin/content/screenlock_bridge.cc (right):

https://codereview.chromium.org/1096293003/diff/10040/components/signin/conte...
components/signin/content/screenlock_bridge.cc:5: #include
"components/signin/content/screenlock_bridge.h"
I think the proximity_auth component is more appropriate.  In terms of whether
to nest in a content directory or not, it depends: Do we foresee ever building
the proximity auth component on iOS?  If so, then a layered component makes
sense -- and you should take my comments referring to a "client" interface as
really referring to a "driver" interface.  If not, then there's probably no need
for the extra layer of nesting.

[msarda, 2015-04-22 16:42:42]

Please take another look.

https://codereview.chromium.org/1096293003/diff/10040/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/1096293003/diff/10040/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:502:
SigninManagerFactory::GetForProfile(profile);
On 2015/04/21 23:02:39, Ilya Sherman wrote:
> This is not exactly equivalent to the previous implementation.  Could you
please
> preserve the previous implementation?

Done.

https://codereview.chromium.org/1096293003/diff/10040/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:504: }
On 2015/04/21 23:02:39, Ilya Sherman wrote:
> Rather than duplicating the implementations of these methods here and in
> user_manager_screen_handler.cc, I'd prefer to share the implementation.  One
> reasonable approach is to use a client interface, as we do in other
components.

I considered using a client interface, however IMHO the API would be extremely
weird as the ScreenlockBridge is a singleton and it only created and accessed
via ScreenlockBridge::Get. I do not see how / when I could set the client of the
SceeenLockBridge.

I think the natural place for this is the LockHandler (this is actually the
client interface for the ScreenlockBridge).

https://codereview.chromium.org/1096293003/diff/10040/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/1096293003/diff/10040/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:96:
->GetAuthenticatedUsername();
On 2015/04/21 23:02:39, Ilya Sherman wrote:
> Please keep the comment from screenlock_bridge.cc along with this code. 
Better
> yet, take me up on my suggestion to use a client interface, and call into that
> interface here.

Done.

[Ilya Sherman, 2015-04-23 07:09:48]

https://codereview.chromium.org/1096293003/diff/10040/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/1096293003/diff/10040/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:504: }
On 2015/04/22 16:42:42, msarda wrote:
> On 2015/04/21 23:02:39, Ilya Sherman wrote:
> > Rather than duplicating the implementations of these methods here and in
> > user_manager_screen_handler.cc, I'd prefer to share the implementation.  One
> > reasonable approach is to use a client interface, as we do in other
> components.
> 
> I considered using a client interface, however IMHO the API would be extremely
> weird as the ScreenlockBridge is a singleton and it only created and accessed
> via ScreenlockBridge::Get. I do not see how / when I could set the client of
the
> SceeenLockBridge.

The component setup code could call a method to set the client for the
ScreenlockBridge very early on, before any other calls to Get() are made.  The
ScreenlockBridge could guarantee this by not null-checking the client, which
would mean that if the setup code did not execute early enough, we'd see a null
pointer dereference, which is easy to track down.

> I think the natural place for this is the LockHandler (this is actually the
> client interface for the ScreenlockBridge).

Actually, the LockHandler is only set when the screen is locked.  So, it really
doesn't make very much sense for the LockHandler to have a Lock() method -- the
ScreenlockBridge won't have a lock handler set if we're calling Lock().



All that said, if you really don't like the idea of using a client interface,
and can demonstrate that I'm wrong about the Lock() method, then please at least
create a single ChromeLockHandler class that these two classes can both derive
from, rather than duplicating the code.

[msarda, 2015-04-23 08:17:38]

On 2015/04/23 07:09:48, Ilya Sherman wrote:
>
https://codereview.chromium.org/1096293003/diff/10040/chrome/browser/chromeos...
> File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):
> 
>
https://codereview.chromium.org/1096293003/diff/10040/chrome/browser/chromeos...
> chrome/browser/chromeos/login/screens/user_selection_screen.cc:504: }
> On 2015/04/22 16:42:42, msarda wrote:
> > On 2015/04/21 23:02:39, Ilya Sherman wrote:
> > > Rather than duplicating the implementations of these methods here and in
> > > user_manager_screen_handler.cc, I'd prefer to share the implementation. 
One
> > > reasonable approach is to use a client interface, as we do in other
> > components.
> > 
> > I considered using a client interface, however IMHO the API would be
extremely
> > weird as the ScreenlockBridge is a singleton and it only created and
accessed
> > via ScreenlockBridge::Get. I do not see how / when I could set the client of
> the
> > SceeenLockBridge.
> 
> The component setup code could call a method to set the client for the
> ScreenlockBridge very early on, before any other calls to Get() are made.  The
> ScreenlockBridge could guarantee this by not null-checking the client, which
> would mean that if the setup code did not execute early enough, we'd see a
null
> pointer dereference, which is easy to track down.
> 
> > I think the natural place for this is the LockHandler (this is actually the
> > client interface for the ScreenlockBridge).
> 
> Actually, the LockHandler is only set when the screen is locked.  So, it
really
> doesn't make very much sense for the LockHandler to have a Lock() method --
the
> ScreenlockBridge won't have a lock handler set if we're calling Lock().
> 
> 
> 
> All that said, if you really don't like the idea of using a client interface,
> and can demonstrate that I'm wrong about the Lock() method, then please at
least
> create a single ChromeLockHandler class that these two classes can both derive
> from, rather than duplicating the code.

I'll add the client API then. I'll remove the ScreenlockBridge::Get and add a
C-function like GetScreenlockBridgeInstance() somewhere in
//chrome/browser/signin/ that will return a shared instance. This function will
be responsible to create the ScreenlockBridge and set its client if the global
does not exists or return the shared instace if it exists. Is this OK for you?

[msarda, 2015-04-23 11:05:59]

Patchset #11 (id:190001) has been deleted

[msarda, 2015-04-23 12:06:39]

Patchset #12 (id:230001) has been deleted

[msarda, 2015-04-23 12:08:41]

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

[msarda, 2015-04-23 12:08:42]

R+ Sylvain to have a local reviewer and avoid long review cycles.

I've added the ProximityAuthClient and a util function to get the global
instance of the ScreenlockBridge.
Please take another look.

[Ilya Sherman, 2015-04-23 20:52:54]

Thanks, Mihai, this looks much better.  At some point, we'll presumably need
code within the //proximity_auth component to be able to access the
ScreenlockBridge -- I assume that's why you're moving it -- and hence having the
GetScreenlockBridgeInstance() method live purely within //chrome won't be viable
in the long run.  But, it's an ok intermediate point for now.

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_util.cc (right):

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:10: #include "base/strings/string16.h"
nit: Not needed?

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:20: // A Chrome specific
implementation of the ProximityAuthClient.
nit: "Chrome specific" -> "Chrome-specific"

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:24: virtual
~ChromeProximityAuthClient();
nit: Please leave a blank line after this one.

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:25: std::string
GetAuthenticatedUsername(
nit: Above this line, please add "// ProximityAuthClient:"

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:37: }
Optional: Eventually, we'll probably a more involved client interface, and it
will make sense to have a separate header and .cc file for this class.  My
personal preference would be to split those off now, but I don't feel strongly
about it.

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:46: CHECK(signin_manager);
nit: No need for the explicit CHECK -- a null pointer dereference will crash
regardless.

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:54: // A utilitary class that serves
as the glue code required to initialize and
nit: s/utilitary/utility

https://codereview.chromium.org/1096293003/diff/250001/components/proximity_a...
File components/proximity_auth/proximity_auth_client.h (right):

https://codereview.chromium.org/1096293003/diff/250001/components/proximity_a...
components/proximity_auth/proximity_auth_client.h:16: class ProximityAuthClient
{
Please ensconce this in the proximity_auth namespace.

https://codereview.chromium.org/1096293003/diff/250001/components/proximity_a...
File components/proximity_auth/screenlock_bridge.cc (right):

https://codereview.chromium.org/1096293003/diff/250001/components/proximity_a...
components/proximity_auth/screenlock_bridge.cc:52: : client_(client),
lock_handler_(NULL) {
nit: s/NULL/nullptr

https://codereview.chromium.org/1096293003/diff/250001/components/proximity_a...
File components/proximity_auth/screenlock_bridge.h (right):

https://codereview.chromium.org/1096293003/diff/250001/components/proximity_a...
components/proximity_auth/screenlock_bridge.h:28: class ScreenlockBridge {
Please ensconce this in the proximity_auth namespace.

[sdefresne, 2015-04-24 14:22:03]

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (right):

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:22: 
#include "components/proximity_auth/screenlock_bridge.h"

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_util.cc (right):

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:24: virtual
~ChromeProximityAuthClient();
~ChromeProximityAuthClient() override;

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:46: CHECK(signin_manager);
On 2015/04/23 20:52:53, Ilya Sherman wrote:
> nit: No need for the explicit CHECK -- a null pointer dereference will crash
> regardless.

It will only crash reliably if the called method is a virtual method, otherwise
the method address is resolved at compile time and depending on the method
implementation it may not crash.

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:64:
scoped_ptr<ChromeProximityAuthClient> proximity_auth_client_;
Why not directly use instance value instead of scoped_ptr?

ChromeProximityAuthClient proximity_auth_client_;
ScreenlockBridge screenlock_bridge_;

EasyUnlockUtil::EasyUnlockUtil()
: proximity_auth_client_(), screenlock_bride_(&proximity_auth_client_) {
}

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:66: friend struct
base::DefaultLazyInstanceTraits<EasyUnlockUtil>;
style: friend comes before everything else

https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

[msarda, 2015-04-24 15:47:06]

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (right):

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:22: 
On 2015/04/24 14:22:02, sdefresne wrote:
> #include "components/proximity_auth/screenlock_bridge.h"

Not needed - already included by its header 
chrome/browser/signin/easy_unlock_service_signin_chromeos.h

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_util.cc (right):

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:10: #include "base/strings/string16.h"
On 2015/04/23 20:52:53, Ilya Sherman wrote:
> nit: Not needed?

Done.

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:20: // A Chrome specific
implementation of the ProximityAuthClient.
On 2015/04/23 20:52:53, Ilya Sherman wrote:
> nit: "Chrome specific" -> "Chrome-specific"

Done.

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:25: std::string
GetAuthenticatedUsername(
On 2015/04/23 20:52:53, Ilya Sherman wrote:
> nit: Above this line, please add "// ProximityAuthClient:"

Done.

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:37: }
On 2015/04/23 20:52:53, Ilya Sherman wrote:
> Optional: Eventually, we'll probably a more involved client interface, and it
> will make sense to have a separate header and .cc file for this class.  My
> personal preference would be to split those off now, but I don't feel strongly
> about it.

I prefer to do that later as I would argue we could leave it here and just make
it public via a call like GetProximityAuthClientInstance().

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:46: CHECK(signin_manager);
On 2015/04/24 14:22:02, sdefresne wrote:
> On 2015/04/23 20:52:53, Ilya Sherman wrote:
> > nit: No need for the explicit CHECK -- a null pointer dereference will crash
> > regardless.
> 
> It will only crash reliably if the called method is a virtual method,
otherwise
> the method address is resolved at compile time and depending on the method
> implementation it may not crash.

+1 for what Sylvain said (this was also the reason why I added it). We've seen 
crashes in Bling related to Profile that has a stack that was pretty hard to
read.

Note that GetAuthenticatedUsername is not virtual.

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:54: // A utilitary class that serves
as the glue code required to initialize and
On 2015/04/23 20:52:53, Ilya Sherman wrote:
> nit: s/utilitary/utility

Done.

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:54: // A utilitary class that serves
as the glue code required to initialize and
On 2015/04/23 20:52:53, Ilya Sherman wrote:
> nit: s/utilitary/utility

Done.

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:64:
scoped_ptr<ChromeProximityAuthClient> proximity_auth_client_;
On 2015/04/24 14:22:03, sdefresne wrote:
> Why not directly use instance value instead of scoped_ptr?
> 
> ChromeProximityAuthClient proximity_auth_client_;
> ScreenlockBridge screenlock_bridge_;
> 
> EasyUnlockUtil::EasyUnlockUtil()
> : proximity_auth_client_(), screenlock_bride_(&proximity_auth_client_) {
> }

Done.

https://codereview.chromium.org/1096293003/diff/250001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:66: friend struct
base::DefaultLazyInstanceTraits<EasyUnlockUtil>;
On 2015/04/24 14:22:02, sdefresne wrote:
> style: friend comes before everything else
> 
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

Done.

https://codereview.chromium.org/1096293003/diff/250001/components/proximity_a...
File components/proximity_auth/proximity_auth_client.h (right):

https://codereview.chromium.org/1096293003/diff/250001/components/proximity_a...
components/proximity_auth/proximity_auth_client.h:16: class ProximityAuthClient
{
On 2015/04/23 20:52:53, Ilya Sherman wrote:
> Please ensconce this in the proximity_auth namespace.

Done.

https://codereview.chromium.org/1096293003/diff/250001/components/proximity_a...
File components/proximity_auth/screenlock_bridge.cc (right):

https://codereview.chromium.org/1096293003/diff/250001/components/proximity_a...
components/proximity_auth/screenlock_bridge.cc:52: : client_(client),
lock_handler_(NULL) {
On 2015/04/23 20:52:53, Ilya Sherman wrote:
> nit: s/NULL/nullptr

Done.

https://codereview.chromium.org/1096293003/diff/250001/components/proximity_a...
File components/proximity_auth/screenlock_bridge.h (right):

https://codereview.chromium.org/1096293003/diff/250001/components/proximity_a...
components/proximity_auth/screenlock_bridge.h:28: class ScreenlockBridge {
On 2015/04/23 20:52:53, Ilya Sherman wrote:
> Please ensconce this in the proximity_auth namespace.

Done.

[msarda, 2015-04-24 15:49:31]

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

[msarda, 2015-04-24 15:49:32]

R+ derat@

Dan: Please take a look at the DEPS change in components/proximity_auth/DEPS.

Thank you.

[msarda, 2015-04-24 15:52:40]

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

[msarda, 2015-04-24 15:52:41]

TBR+ Roger as sign-in OWNER.

[msarda, 2015-04-24 15:56:42]

msarda@chromium.org changed reviewers:
+ antrim@chromium.org, rdevlin.cronin@chromium.org, tengs@chromium.org

[Daniel Erat, 2015-04-24 16:00:03]

lgtm for chromeos/ dependency in components/proximity_auth/DEPS

[Ilya Sherman, 2015-04-24 21:22:02]

Thanks, Mihai.  LGTM % the remaining nits.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc (right):

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/chromeo...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:10: #include
"chrome/browser/signin/easy_unlock_util.h"
Oh, I meant to call this out in my last round of review: Let's not introduce a
generic "util" file, as they tend to become dumping grounds.  Let's give this a
more focused name -- perhaps screenlock_bridge_accessor.h, or something like
that.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:25: #include
"components/signin/core/browser/signin_manager_base.h"
nit: Not needed?

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_auth_attempt_unittest.cc (right):

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_auth_attempt_unittest.cc:8: #include
"chrome/browser/profiles/profile.h"
nit: Not needed?

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_auth_attempt_unittest.cc:11: #include
"chrome/browser/signin/signin_manager_factory.h"
nit: Not needed?

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_auth_attempt_unittest.cc:13: #include
"components/signin/core/browser/signin_manager_base.h"
nit: Not needed?

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_service_regular.cc:100: CHECK(signin_manager);
This should be at most a DCHECK.  CHECK is used very sparingly in Chromium code.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (right):

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:279:
proximity_auth::ScreenlockBridge::LockHandler::SIGNIN_SCREEN)
nit: Please indent this line four more spaces.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_util.cc (right):

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:22: ~ChromeProximityAuthClient()
override;
nit: Please inline these trivial constructor and destructor methods, if you're
going to keep the class definition in the anonymous namespace.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:26: content::BrowserContext*
browser_context) override;
nit: Can this be const?

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:54: // A /utility class that serves as
the glue code required to initialize and
nit: Please remove the "/" before "utility"

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:58: proximity_auth::ScreenlockBridge*
GetScreenlockBridge();
nit: This is a trivial accessor, so please name it "screenlock_bridge()".

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:77: : proximity_auth_client_(),
screenlock_bridge_(&proximity_auth_client_) {
nit: Please omit the default constructor call for "proximity_auth_client_()"

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:85: }
nit: Please inline all of the EasyUnlockUtil methods -- they are all trivial.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_util.h (right):

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/1096293003/diff/330001/components/proximity_a...
File components/proximity_auth/screenlock_bridge.cc (right):

https://codereview.chromium.org/1096293003/diff/330001/components/proximity_a...
components/proximity_auth/screenlock_bridge.cc:17: 
Optional nit: I'd omit this newline.

https://codereview.chromium.org/1096293003/diff/330001/components/proximity_a...
components/proximity_auth/screenlock_bridge.cc:167: }
nit: No need for curlies.

https://codereview.chromium.org/1096293003/diff/330001/components/proximity_a...
File components/proximity_auth/screenlock_bridge.h (right):

https://codereview.chromium.org/1096293003/diff/330001/components/proximity_a...
components/proximity_auth/screenlock_bridge.h:32: explicit
ScreenlockBridge(ProximityAuthClient* client);
Please document the lifetime expectations for the |client|.

[msarda, 2015-04-27 11:29:25]

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc (right):

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/chromeo...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:10: #include
"chrome/browser/signin/easy_unlock_util.h"
On 2015/04/24 21:22:01, Ilya Sherman wrote:
> Oh, I meant to call this out in my last round of review: Let's not introduce a
> generic "util" file, as they tend to become dumping grounds.  Let's give this
a
> more focused name -- perhaps screenlock_bridge_accessor.h, or something like
> that.

I've renamed this class to proximit_auth_facade as I plan this to be the facade
for all the singleton instances of the proximity auth component. I think we need
something to manage the lifecycle of all the singleton instances (for example
ScreenlockBridge needs to outlive the ChromeProximityAuthClient and they are
both singleton objects). I'm fine to rename this class to whatever else if you
think this is needed.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:25: #include
"components/signin/core/browser/signin_manager_base.h"
On 2015/04/24 21:22:01, Ilya Sherman wrote:
> nit: Not needed?

Done.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_auth_attempt_unittest.cc (right):

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_auth_attempt_unittest.cc:8: #include
"chrome/browser/profiles/profile.h"
On 2015/04/24 21:22:01, Ilya Sherman wrote:
> nit: Not needed?

Done.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_auth_attempt_unittest.cc:11: #include
"chrome/browser/signin/signin_manager_factory.h"
On 2015/04/24 21:22:01, Ilya Sherman wrote:
> nit: Not needed?

Done.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_auth_attempt_unittest.cc:13: #include
"components/signin/core/browser/signin_manager_base.h"
On 2015/04/24 21:22:01, Ilya Sherman wrote:
> nit: Not needed?

Done.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_service_regular.cc:100: CHECK(signin_manager);
On 2015/04/24 21:22:02, Ilya Sherman wrote:
> This should be at most a DCHECK.  CHECK is used very sparingly in Chromium
code.

Done.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (right):

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:279:
proximity_auth::ScreenlockBridge::LockHandler::SIGNIN_SCREEN)
On 2015/04/24 21:22:02, Ilya Sherman wrote:
> nit: Please indent this line four more spaces.

Done.

FYI, the format above was achieved with git cl format. I think you should file a
bug for it if you think it does not match the styleguide.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_util.cc (right):

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/04/24 21:22:02, Ilya Sherman wrote:
> nit: 2015

Done.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:22: ~ChromeProximityAuthClient()
override;
On 2015/04/24 21:22:02, Ilya Sherman wrote:
> nit: Please inline these trivial constructor and destructor methods, if you're
> going to keep the class definition in the anonymous namespace.

Done.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:26: content::BrowserContext*
browser_context) override;
On 2015/04/24 21:22:02, Ilya Sherman wrote:
> nit: Can this be const?

Done.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:54: // A /utility class that serves as
the glue code required to initialize and
On 2015/04/24 21:22:02, Ilya Sherman wrote:
> nit: Please remove the "/" before "utility"

Done.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:58: proximity_auth::ScreenlockBridge*
GetScreenlockBridge();
On 2015/04/24 21:22:02, Ilya Sherman wrote:
> nit: This is a trivial accessor, so please name it "screenlock_bridge()".

I think this is not really a trivial accessor - it returns a pointer not just a
reference. I kept it as is for now.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:77: : proximity_auth_client_(),
screenlock_bridge_(&proximity_auth_client_) {
On 2015/04/24 21:22:02, Ilya Sherman wrote:
> nit: Please omit the default constructor call for "proximity_auth_client_()"

Done.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.cc:85: }
On 2015/04/24 21:22:02, Ilya Sherman wrote:
> nit: Please inline all of the EasyUnlockUtil methods -- they are all trivial.

Done.

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_util.h (right):

https://codereview.chromium.org/1096293003/diff/330001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_util.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/04/24 21:22:02, Ilya Sherman wrote:
> nit: 2015

Done.

https://codereview.chromium.org/1096293003/diff/330001/components/proximity_a...
File components/proximity_auth/screenlock_bridge.cc (right):

https://codereview.chromium.org/1096293003/diff/330001/components/proximity_a...
components/proximity_auth/screenlock_bridge.cc:17: 
On 2015/04/24 21:22:02, Ilya Sherman wrote:
> Optional nit: I'd omit this newline.

Done.

https://codereview.chromium.org/1096293003/diff/330001/components/proximity_a...
components/proximity_auth/screenlock_bridge.cc:167: }
On 2015/04/24 21:22:02, Ilya Sherman wrote:
> nit: No need for curlies.

Done.

https://codereview.chromium.org/1096293003/diff/330001/components/proximity_a...
File components/proximity_auth/screenlock_bridge.h (right):

https://codereview.chromium.org/1096293003/diff/330001/components/proximity_a...
components/proximity_auth/screenlock_bridge.h:32: explicit
ScreenlockBridge(ProximityAuthClient* client);
On 2015/04/24 21:22:02, Ilya Sherman wrote:
> Please document the lifetime expectations for the |client|.

Done.

[msarda, 2015-04-27 11:33:31]

The CQ bit was checked by msarda@chromium.org

[msarda, 2015-04-27 11:33:32]

The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1096293003/#ps370001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-27 11:33:38]

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

[commit-bot: I haz the power, 2015-04-27 11:38:27]

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

[commit-bot: I haz the power, 2015-04-27 11:38:28]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msarda, 2015-04-27 12:11:08]

The CQ bit was checked by msarda@chromium.org

[commit-bot: I haz the power, 2015-04-27 12:11:14]

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

[commit-bot: I haz the power, 2015-04-27 13:42:51]

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

[commit-bot: I haz the power, 2015-04-27 13:42:52]

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

[msarda, 2015-04-27 13:57:19]

The CQ bit was checked by msarda@chromium.org

[commit-bot: I haz the power, 2015-04-27 13:57:28]

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

[commit-bot: I haz the power, 2015-04-27 15:30:30]

Committed patchset #18 (id:370001)

[commit-bot: I haz the power, 2015-04-27 15:31:27]

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

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

On 2015/04/27 15:31:27, I haz the power (commit-bot) wrote:
> Patchset 18 (id:??) landed as
> https://crrev.com/f80a09109fae223241b5462e20a3556edfbd6740
> Cr-Commit-Position: refs/heads/master@{#327049}

Thank you for the review.