[Proximity Auth] Create one ProximityAuthClient per profile, rather than one global one.

[Proximity Auth] Create one ProximityAuthClient per profile, rather than one global one.

BUG=501626
TEST=none
R=tengs@chromium.org,mlerman@chromium.org

Committed: https://crrev.com/3ead4bc4ebb743b3771afb47dbfbd4eb2131ef1b
Cr-Commit-Position: refs/heads/master@{#336983}

[Ilya Sherman, 2015-06-26 07:09:07]

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

[commit-bot: I haz the power, 2015-06-26 07:09:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1209193003/1

[Ilya Sherman, 2015-06-26 07:09:39]

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

[Ilya Sherman, 2015-06-26 07:10:30]

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

[Ilya Sherman, 2015-06-26 07:10:30]

+Mike for chrome/browser/signin OWNERS
+Marijn for chrome/browser/extensions OWNERS

[commit-bot: I haz the power, 2015-06-26 08:43:26]

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

[commit-bot: I haz the power, 2015-06-26 08:43:27]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Mike Lerman, 2015-06-26 13:09:51]

A few small comments. Apologies for typos, I'm on my phone on a train.

https://chromiumcodereview.appspot.com/1209193003/diff/1/chrome/browser/signi...
File chrome/browser/signin/chrome_proximity_auth_client.cc (right):

https://chromiumcodereview.appspot.com/1209193003/diff/1/chrome/browser/signi...
chrome/browser/signin/chrome_proximity_auth_client.cc:25: return
signin_manager->GetAuthenticatedUsername();
Please use GetAuthenticatedAccountId() to uniquely identify the account. If you
need the account's email address, consider using the AccountTrackerService, as
emails can change.

https://chromiumcodereview.appspot.com/1209193003/diff/1/chrome/browser/signi...
chrome/browser/signin/chrome_proximity_auth_client.cc:29:
profiles::LockProfile(profile_);
Before calling Lock(), perhaps DCHECK IsProfileLockAvailable()?

https://chromiumcodereview.appspot.com/1209193003/diff/1/chrome/browser/signi...
File chrome/browser/signin/chrome_proximity_auth_client_factory.cc (right):

https://chromiumcodereview.appspot.com/1209193003/diff/1/chrome/browser/signi...
chrome/browser/signin/chrome_proximity_auth_client_factory.cc:28: }
DependsOn(SigninManagerFactory)?

https://chromiumcodereview.appspot.com/1209193003/diff/1/chrome/browser/signi...
File chrome/browser/signin/chrome_proximity_auth_client_factory.h (right):

https://chromiumcodereview.appspot.com/1209193003/diff/1/chrome/browser/signi...
chrome/browser/signin/chrome_proximity_auth_client_factory.h:20: // them with
profiles. Listens for each profile's destruction notification and
I don't see where you react to the Profile destruction, and the KeyedService
doesn't override Shutdown().

[Tim Song, 2015-06-26 23:46:43]

https://codereview.chromium.org/1209193003/diff/1/chrome/browser/signin/chrom...
File chrome/browser/signin/chrome_proximity_auth_client.h (right):

https://codereview.chromium.org/1209193003/diff/1/chrome/browser/signin/chrom...
chrome/browser/signin/chrome_proximity_auth_client.h:17: public KeyedService {
Is there a benefit to making this ProximityAuthClient implementation a
KeyedService? What about initializing and storing it in the EasyUnlockService?

https://codereview.chromium.org/1209193003/diff/1/components/proximity_auth/s...
File components/proximity_auth/screenlock_bridge.cc (right):

https://codereview.chromium.org/1209193003/diff/1/components/proximity_auth/s...
components/proximity_auth/screenlock_bridge.cc:151: void
ScreenlockBridge::Lock(ProximityAuthClient* client) {
Hmm... requiring the client here couples the ScreenlockBridge really tightly
with proximity_auth. I wonder if its worth having the Lock() and Unlock()
methods at all, since they only seem to be used in a small number of places.

https://codereview.chromium.org/1209193003/diff/1/components/proximity_auth/s...
components/proximity_auth/screenlock_bridge.cc:161: void
ScreenlockBridge::Unlock(ProximityAuthClient* client) {
I think it would be better to take the username directly as an argument.

[Ilya Sherman, 2015-06-27 08:18:00]

https://codereview.chromium.org/1209193003/diff/1/chrome/browser/signin/chrom...
File chrome/browser/signin/chrome_proximity_auth_client.cc (right):

https://codereview.chromium.org/1209193003/diff/1/chrome/browser/signin/chrom...
chrome/browser/signin/chrome_proximity_auth_client.cc:25: return
signin_manager->GetAuthenticatedUsername();
On 2015/06/26 13:09:51, Mike Lerman wrote:
> Please use GetAuthenticatedAccountId() to uniquely identify the account. If
you
> need the account's email address, consider using the AccountTrackerService, as
> emails can change. 

Hmm, I'm hesitant to change this, as there seems to be a lot subtlety involved. 
We need a value that will eventually be used to identify the profile within the
ProfileInfoCache class.  It looks like GetAuthenticatedUsername uses the
AccountTrackerService internally.

https://codereview.chromium.org/1209193003/diff/1/chrome/browser/signin/chrom...
chrome/browser/signin/chrome_proximity_auth_client.cc:29:
profiles::LockProfile(profile_);
On 2015/06/26 13:09:51, Mike Lerman wrote:
> Before calling Lock(), perhaps DCHECK IsProfileLockAvailable()? 

Acknowledged.

https://codereview.chromium.org/1209193003/diff/1/chrome/browser/signin/chrom...
File chrome/browser/signin/chrome_proximity_auth_client.h (right):

https://codereview.chromium.org/1209193003/diff/1/chrome/browser/signin/chrom...
chrome/browser/signin/chrome_proximity_auth_client.h:17: public KeyedService {
On 2015/06/26 23:46:43, Tim Song wrote:
> Is there a benefit to making this ProximityAuthClient implementation a
> KeyedService? What about initializing and storing it in the EasyUnlockService?

Good idea!  Done.

https://codereview.chromium.org/1209193003/diff/1/chrome/browser/signin/chrom...
File chrome/browser/signin/chrome_proximity_auth_client_factory.cc (right):

https://codereview.chromium.org/1209193003/diff/1/chrome/browser/signin/chrom...
chrome/browser/signin/chrome_proximity_auth_client_factory.cc:28: }
On 2015/06/26 13:09:51, Mike Lerman wrote:
> DependsOn(SigninManagerFactory)? 

Acknowledged.  I think it actually wouldn't have been necessary, since there's
no lifetime dependency between the two classes... but this file is gone now.

https://codereview.chromium.org/1209193003/diff/1/chrome/browser/signin/chrom...
File chrome/browser/signin/chrome_proximity_auth_client_factory.h (right):

https://codereview.chromium.org/1209193003/diff/1/chrome/browser/signin/chrom...
chrome/browser/signin/chrome_proximity_auth_client_factory.h:20: // them with
profiles. Listens for each profile's destruction notification and
On 2015/06/26 13:09:51, Mike Lerman wrote:
> I don't see where you react to the Profile destruction, and the KeyedService
> doesn't override Shutdown(). 

Whoops, you can see which class I copy/pasta'd :P

Removed this whole file, so no longer relevant.  Good catch though =)

https://codereview.chromium.org/1209193003/diff/1/components/proximity_auth/s...
File components/proximity_auth/screenlock_bridge.cc (right):

https://codereview.chromium.org/1209193003/diff/1/components/proximity_auth/s...
components/proximity_auth/screenlock_bridge.cc:151: void
ScreenlockBridge::Lock(ProximityAuthClient* client) {
On 2015/06/26 23:46:43, Tim Song wrote:
> Hmm... requiring the client here couples the ScreenlockBridge really tightly
> with proximity_auth. I wonder if its worth having the Lock() and Unlock()
> methods at all, since they only seem to be used in a small number of places.

I removed the Lock() implementation for now, since we aren't actually using this
class on non-ChromeOS platforms for now... and we'd probably prefer to lock the
screen -- rather than Chrome -- on the other platforms anyway.

https://codereview.chromium.org/1209193003/diff/1/components/proximity_auth/s...
components/proximity_auth/screenlock_bridge.cc:161: void
ScreenlockBridge::Unlock(ProximityAuthClient* client) {
On 2015/06/26 23:46:43, Tim Song wrote:
> I think it would be better to take the username directly as an argument.

Done.

[Mike Lerman, 2015-06-29 13:41:28]

https://codereview.chromium.org/1209193003/diff/40001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_proximity_auth_client.h (right):

https://codereview.chromium.org/1209193003/diff/40001/chrome/browser/signin/c...
chrome/browser/signin/chrome_proximity_auth_client.h:15: class
ChromeProximityAuthClient : public proximity_auth::ProximityAuthClient {
This class really just provides one small function now: Get the
AuthenticatedUserName. Maybe this should just be an inner class / anonymous
namespace class within the EasyUnlockService? The EasyUnlockService directly
owns the reference anyways, and has access to the Profile*.

[Ilya Sherman, 2015-06-29 18:03:38]

https://codereview.chromium.org/1209193003/diff/40001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_proximity_auth_client.h (right):

https://codereview.chromium.org/1209193003/diff/40001/chrome/browser/signin/c...
chrome/browser/signin/chrome_proximity_auth_client.h:15: class
ChromeProximityAuthClient : public proximity_auth::ProximityAuthClient {
On 2015/06/29 13:41:28, Mike Lerman wrote:
> This class really just provides one small function now: Get the
> AuthenticatedUserName. Maybe this should just be an inner class / anonymous
> namespace class within the EasyUnlockService? The EasyUnlockService directly
> owns the reference anyways, and has access to the Profile*.

I'm about to add more code to this class in a follow-up CL =)  This is the
embedder interface for the in-progress proximity_auth component, so it basically
does everything that the component wants to do, but can't implement without
reaching into //chrome

[Mike Lerman, 2015-06-29 18:04:02]

Okay. lgtm.

[Tim Song, 2015-06-29 18:07:07]

LGTM :)

[Ilya Sherman, 2015-06-29 18:14:00]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2015-06-29 18:15:00]

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

[Marijn Kruisselbrink, 2015-06-29 18:15:04]

https://codereview.chromium.org/1209193003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/screenlock_private/screenlock_private_api.cc
(right):

https://codereview.chromium.org/1209193003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/screenlock_private/screenlock_private_api.cc:83:
service->proximity_auth_client()->GetAuthenticatedUsername());
Can you add a comment to that Unlock method to describe what the user_email
parameter is/what it is used for? Reading this code and the interface it calls I
have no idea why it is passing some seemingly arbitrary "username" to a method
expecting a "user_email". Or would it make sense for Unlock to take the
proximity_auth_client() and get whatever it needs to get from there itself?

[commit-bot: I haz the power, 2015-06-29 18:19:07]

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

[commit-bot: I haz the power, 2015-06-29 18:19:08]

Exceeded global retry quota

[Ilya Sherman, 2015-06-30 00:22:00]

https://codereview.chromium.org/1209193003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/screenlock_private/screenlock_private_api.cc
(right):

https://codereview.chromium.org/1209193003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/screenlock_private/screenlock_private_api.cc:83:
service->proximity_auth_client()->GetAuthenticatedUsername());
On 2015/06/29 18:15:04, Marijn Kruisselbrink wrote:
> Can you add a comment to that Unlock method to describe what the user_email
> parameter is/what it is used for? Reading this code and the interface it calls
I
> have no idea why it is passing some seemingly arbitrary "username" to a method
> expecting a "user_email". Or would it make sense for Unlock to take the
> proximity_auth_client() and get whatever it needs to get from there itself?

Done, though I'm not sure how much it helps.  I think the confusion stems from
the fact that the identifier is called a username in some places in the code,
and an email address in others.  So, this method implementation is a bit
surprising:
https://code.google.com/p/chromium/codesearch#chromium/src/components/signin/...

[Ilya Sherman, 2015-06-30 00:23:54]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-06-30 00:23:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlerman@chromium.org, tengs@chromium.org
Link to the patchset: https://codereview.chromium.org/1209193003/#ps60001
(title: "Add a doc string")

[commit-bot: I haz the power, 2015-06-30 00:24:54]

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

[commit-bot: I haz the power, 2015-06-30 00:28:06]

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

[commit-bot: I haz the power, 2015-06-30 00:28:07]

Exceeded global retry quota

[Ilya Sherman, 2015-06-30 00:40:49]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-06-30 00:40:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlerman@chromium.org, tengs@chromium.org
Link to the patchset: https://codereview.chromium.org/1209193003/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-30 00:41:41]

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

[commit-bot: I haz the power, 2015-06-30 00:51:32]

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

[commit-bot: I haz the power, 2015-06-30 00:51:33]

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

[Ilya Sherman, 2015-06-30 01:36:58]

Whoops, sorry, those should have been CQ dry runs.  I realized also that I did
not address part of your comment:

https://codereview.chromium.org/1209193003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/screenlock_private/screenlock_private_api.cc
(right):

https://codereview.chromium.org/1209193003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/screenlock_private/screenlock_private_api.cc:83:
service->proximity_auth_client()->GetAuthenticatedUsername());
On 2015/06/30 00:22:00, Ilya Sherman wrote:
> On 2015/06/29 18:15:04, Marijn Kruisselbrink wrote:
> > Can you add a comment to that Unlock method to describe what the user_email
> > parameter is/what it is used for? Reading this code and the interface it
calls
> I
> > have no idea why it is passing some seemingly arbitrary "username" to a
method
> > expecting a "user_email". Or would it make sense for Unlock to take the
> > proximity_auth_client() and get whatever it needs to get from there itself?
> 
> Done, though I'm not sure how much it helps.  I think the confusion stems from
> the fact that the identifier is called a username in some places in the code,
> and an email address in others.  So, this method implementation is a bit
> surprising:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/signin/...

To address the other part of your reply: Tim specifically asked me *not* to pass
the proximity_auth_client to Unlock, so as to reduce coupling.  There's some
chance we'll want to extract the ScreenlockBridge class outside of the
proximity_auth component later on.

[Marijn Kruisselbrink, 2015-06-30 19:57:42]

On 2015/06/30 at 01:36:58, isherman wrote:
> Whoops, sorry, those should have been CQ dry runs.  I realized also that I did
not address part of your comment:
> 
>
https://codereview.chromium.org/1209193003/diff/40001/chrome/browser/extensio...
> File
chrome/browser/extensions/api/screenlock_private/screenlock_private_api.cc
(right):
> 
>
https://codereview.chromium.org/1209193003/diff/40001/chrome/browser/extensio...
> chrome/browser/extensions/api/screenlock_private/screenlock_private_api.cc:83:
service->proximity_auth_client()->GetAuthenticatedUsername());
> On 2015/06/30 00:22:00, Ilya Sherman wrote:
> > On 2015/06/29 18:15:04, Marijn Kruisselbrink wrote:
> > > Can you add a comment to that Unlock method to describe what the
user_email
> > > parameter is/what it is used for? Reading this code and the interface it
calls
> > I
> > > have no idea why it is passing some seemingly arbitrary "username" to a
method
> > > expecting a "user_email". Or would it make sense for Unlock to take the
> > > proximity_auth_client() and get whatever it needs to get from there
itself?
> > 
> > Done, though I'm not sure how much it helps.  I think the confusion stems
from
> > the fact that the identifier is called a username in some places in the
code,
> > and an email address in others.  So, this method implementation is a bit
> > surprising:
> >
https://code.google.com/p/chromium/codesearch#chromium/src/components/signin/...
> 
> To address the other part of your reply: Tim specifically asked me *not* to
pass the proximity_auth_client to Unlock, so as to reduce coupling.  There's
some chance we'll want to extract the ScreenlockBridge class outside of the
proximity_auth component later on.

Okay, extensions lgtm

[Ilya Sherman, 2015-06-30 21:03:24]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-06-30 21:03:36]

The CQ bit was unchecked by isherman@chromium.org

[Ilya Sherman, 2015-07-01 05:10:46]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-07-01 05:10:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlerman@chromium.org, mek@chromium.org, tengs@chromium.org
Link to the patchset: https://codereview.chromium.org/1209193003/#ps100001
(title: "Disable tests on non-ChromeOS, since there's nothing left to test on
the other platforms")

[commit-bot: I haz the power, 2015-07-01 05:11:01]

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

[commit-bot: I haz the power, 2015-07-01 06:04:51]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-01 06:05:48]

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