[Proximity Auth] Port the UnlockManager class.

[Proximity Auth] Port the UnlockManager class.

This ports most of the code, with a few TODOs remaining for future CLs.

BUG=501626
R=tengs@chromium.org

Committed: https://crrev.com/6e9846255dbb5b9bcf572536b757e998d6471117
Cr-Commit-Position: refs/heads/master@{#343537}

[Tim Song, 2015-07-23 21:32:16]

Thanks, Ilya! Looks good, just a few small suggestions.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/c...
File components/proximity_auth/controller.h (right):

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/c...
components/proximity_auth/controller.h:14: class Controller {
nit: add a comment describing the purpose of the controller.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
File components/proximity_auth/unlock_manager.cc (right):

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager.cc:175:
proximity_auth_client_->FinalizeSignin(*sign_in_secret_);
Shouldn't you move these two lines inside AcceptAuthAttempt()?
AcceptAuthAttempt() is always calling FinalizeUnlock right now.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager.cc:245: void
UnlockManager::OnScreenLockStateChanged(bool is_locked) {
There is already a |ScreenlockState| enum that has a different meaning than this
"ScreenLockState". Perhaps a better name would simply be OnScreenLocked().

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
File components/proximity_auth/unlock_manager.h (right):

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager.h:51: // the remote devices is
authenticated.
nit: and in range.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager.h:58: // Called when the controller's
state changes to |state|.
nit: there is no |state| argument.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager.h:154: // Used to call into the
embedder. Stored as a weak reference, and expected to
nit: this is not stored as a weak reference.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager.h:180:
base::WeakPtrFactory<UnlockManager> clear_waking_up_state_weak_ptr_factory_;
Wouldn't it be clearer to use timers instead to better express intent?

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
File components/proximity_auth/unlock_manager_unittest.cc (right):

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager_unittest.cc:251: ON_CALL(controller_,
GetState())
Wouldn't it be easier to make the controller a fake instead? You don't seem to
be setting any expectations.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager_unittest.cc:262: ON_CALL(controller_,
GetState())
Can't you replace these lines SimulateUserPresentState()? Same goes for the
other test cases below.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager_unittest.cc:298:
.WillByDefault(Return(Controller::State::SECURE_CHANNEL_ESTABLISHED));
Don't you need to make controller_->GetClient() return null here as well?

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager_unittest.cc:408:
unlock_manager_->SetController(nullptr);
nit: add an expectation for for the INACTIVE state.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager_unittest.cc:419:
unlock_manager_->SetController(nullptr);
Why simulate the user present state at all for this test case (and below) if
afterwards, all that work is negated by clearing the controller?

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager_unittest.cc:751:
OnAuthAttempted_Unlock_SupportsSignIn_UnlockRequestSucceeds_UnlockEventSendFails)
{
This formatting is kind of weird. Perhaps there is someway to shorten the name?

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager_unittest.cc:768:
OnAuthAttempted_Unlock_SupportsSignIn_UnlockRequestSucceeds_UnlockEventSendSucceeds)
{
same here.

[Ilya Sherman, 2015-08-11 23:37:15]

Apologies for the long delay -- I was on vacation for two weeks, and then
working remotely for a week.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/c...
File components/proximity_auth/controller.h (right):

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/c...
components/proximity_auth/controller.h:14: class Controller {
On 2015/07/23 21:32:15, Tim Song wrote:
> nit: add a comment describing the purpose of the controller.

Done.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
File components/proximity_auth/unlock_manager.cc (right):

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager.cc:175:
proximity_auth_client_->FinalizeSignin(*sign_in_secret_);
On 2015/07/23 21:32:15, Tim Song wrote:
> Shouldn't you move these two lines inside AcceptAuthAttempt()?
> AcceptAuthAttempt() is always calling FinalizeUnlock right now.

The current flow matches what the app is doing (see line 641 of
unlock_manager.js).  For sign-in, the app calls FinalizeSignin and then
FinalizeUnlock.  (I do agree that it's a little confusing that this is the
flow.)

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager.cc:245: void
UnlockManager::OnScreenLockStateChanged(bool is_locked) {
On 2015/07/23 21:32:15, Tim Song wrote:
> There is already a |ScreenlockState| enum that has a different meaning than
this
> "ScreenLockState". Perhaps a better name would simply be OnScreenLocked().

Done.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
File components/proximity_auth/unlock_manager.h (right):

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager.h:51: // the remote devices is
authenticated.
On 2015/07/23 21:32:15, Tim Song wrote:
> nit: and in range.

Done.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager.h:58: // Called when the controller's
state changes to |state|.
On 2015/07/23 21:32:15, Tim Song wrote:
> nit: there is no |state| argument.

Done.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager.h:154: // Used to call into the
embedder. Stored as a weak reference, and expected to
On 2015/07/23 21:32:15, Tim Song wrote:
> nit: this is not stored as a weak reference.

Done.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager.h:180:
base::WeakPtrFactory<UnlockManager> clear_waking_up_state_weak_ptr_factory_;
On 2015/07/23 21:32:15, Tim Song wrote:
> Wouldn't it be clearer to use timers instead to better express intent?

I tend to prefer directly posting tasks to message loops, for ease of testing. 
I can switch to a timer if it's important to you, though.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
File components/proximity_auth/unlock_manager_unittest.cc (right):

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager_unittest.cc:251: ON_CALL(controller_,
GetState())
On 2015/07/23 21:32:15, Tim Song wrote:
> Wouldn't it be easier to make the controller a fake instead? You don't seem to
> be setting any expectations.

How would you structure the fake?  Provide a set_state() method?  IMO a mock is
slightly easier to use, since there's no need to define a custom interface for
setting the expected state.  But, happy to change this if it's important to you.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager_unittest.cc:262: ON_CALL(controller_,
GetState())
On 2015/07/23 21:32:16, Tim Song wrote:
> Can't you replace these lines SimulateUserPresentState()? Same goes for the
> other test cases below.

My goal was to have each test case be pretty explicit about what actions it
takes, so that it's clear what's being tested.  So, while you're probably right
that there's unnecessary code duplication, I think it helps make each of the
tests cases clearer on its own.  That said, I can change this if it's important
to you.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager_unittest.cc:298:
.WillByDefault(Return(Controller::State::SECURE_CHANNEL_ESTABLISHED));
On 2015/07/23 21:32:15, Tim Song wrote:
> Don't you need to make controller_->GetClient() return null here as well?

Done.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager_unittest.cc:408:
unlock_manager_->SetController(nullptr);
On 2015/07/23 21:32:16, Tim Song wrote:
> nit: add an expectation for for the INACTIVE state.

Done.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager_unittest.cc:419:
unlock_manager_->SetController(nullptr);
On 2015/07/23 21:32:15, Tim Song wrote:
> Why simulate the user present state at all for this test case (and below) if
> afterwards, all that work is negated by clearing the controller?

The idea is to test going from a good state to a bad one.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager_unittest.cc:751:
OnAuthAttempted_Unlock_SupportsSignIn_UnlockRequestSucceeds_UnlockEventSendFails)
{
On 2015/07/23 21:32:15, Tim Song wrote:
> This formatting is kind of weird. Perhaps there is someway to shorten the
name?

Done.

https://codereview.chromium.org/1239193005/diff/1/components/proximity_auth/u...
components/proximity_auth/unlock_manager_unittest.cc:768:
OnAuthAttempted_Unlock_SupportsSignIn_UnlockRequestSucceeds_UnlockEventSendSucceeds)
{
On 2015/07/23 21:32:15, Tim Song wrote:
> same here.

Done.

[Tim Song, 2015-08-13 02:18:29]

Thanks, Ilya. LGTM.

[Ilya Sherman, 2015-08-13 21:30:51]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2015-08-13 21:31:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1239193005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1239193005/20001

[commit-bot: I haz the power, 2015-08-13 21:33:49]

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

[commit-bot: I haz the power, 2015-08-13 21:33:50]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Ilya Sherman, 2015-08-13 23:35:31]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-08-13 23:35:31]

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

[commit-bot: I haz the power, 2015-08-13 23:36:25]

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

[Ilya Sherman, 2015-08-13 23:46:26]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-08-13 23:46:26]

The patchset sent to the CQ was uploaded after l-g-t-m from tengs@chromium.org
Link to the patchset: https://codereview.chromium.org/1239193005/#ps60001
(title: "Alphabetize + update comments")

[commit-bot: I haz the power, 2015-08-13 23:47:09]

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

[commit-bot: I haz the power, 2015-08-13 23:58:54]

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

[commit-bot: I haz the power, 2015-08-13 23:58:55]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ilya Sherman, 2015-08-14 01:05:43]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-08-14 01:05:43]

The patchset sent to the CQ was uploaded after l-g-t-m from tengs@chromium.org
Link to the patchset: https://codereview.chromium.org/1239193005/#ps80001
(title: "Update mocks for tests to fix a couple of test compile errors")

[commit-bot: I haz the power, 2015-08-14 01:06:02]

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

[commit-bot: I haz the power, 2015-08-14 01:41:47]

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

[commit-bot: I haz the power, 2015-08-14 01:41:48]

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

[Ilya Sherman, 2015-08-14 04:32:32]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-08-14 04:32:32]

The patchset sent to the CQ was uploaded after l-g-t-m from tengs@chromium.org
Link to the patchset: https://codereview.chromium.org/1239193005/#ps100001
(title: "Fix ChromeOS compilation errors")

[commit-bot: I haz the power, 2015-08-14 04:32:37]

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

[commit-bot: I haz the power, 2015-08-14 05:29:13]

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

[commit-bot: I haz the power, 2015-08-14 05:29:14]

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

[Ilya Sherman, 2015-08-14 22:27:44]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-08-14 22:27:44]

The patchset sent to the CQ was uploaded after l-g-t-m from tengs@chromium.org
Link to the patchset: https://codereview.chromium.org/1239193005/#ps120001
(title: "Fix destruction order in tests on ChromeOS")

[commit-bot: I haz the power, 2015-08-14 22:28:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1239193005/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1239193005/120001

[commit-bot: I haz the power, 2015-08-15 02:48:13]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-08-15 02:48:56]

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