Enable lock even when no password hash is present.

Enable lock even when no password hash is present.

After discussions, lock will be always available if the flag dictates,
and online auth will be required for the first unlock.

This is subequent to https://codereview.chromium.org/497783002/.

BUG=335086
Committed: https://crrev.com/56f82b67ad5705722d1e5983e00f5dce401c1bd3
Cr-Commit-Position: refs/heads/master@{#294026}

[Mike Lerman, 2014-09-08 15:30:09]

Patchset #1 (id:1) has been deleted

[Mike Lerman, 2014-09-08 16:25:46]

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

[Mike Lerman, 2014-09-08 16:25:49]

Hi Mike,

After discussions with my team and our PM, we want lock to be enabled at all
times. I was thinking of leaving in the methods I created in the CL linked in
the description, and just removing the Disabled-Widget code here. Otherwise,
I'll revert the CL linked above.

Thanks for the review!

Mike

[msw, 2014-09-08 19:20:43]

While I like your index refactoring of c/b/signin/local_auth.cc from the earlier
CL, I think you should remove the LocalAuthCredentialsExist helpers if they're
no longer used and won't be used in an imminently forthcoming CL.

https://codereview.chromium.org/548393002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm
(left):

https://codereview.chromium.org/548393002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:463:
TEST_F(ProfileChooserControllerTest, SignedInProfileLockDisabled) {
Does it make sense to leave these tests intact, and adjust their expectations to
the button being enabled? Or is that behavior sufficiently covered in other
tests, where the lack of local auth credentials is a trivial difference not
worth explicitly testing?

[Mike Lerman, 2014-09-09 16:47:08]

On 2014/09/08 19:20:43, msw wrote:
> While I like your index refactoring of c/b/signin/local_auth.cc from the
earlier
> CL, I think you should remove the LocalAuthCredentialsExist helpers if they're
> no longer used and won't be used in an imminently forthcoming CL.
> 

The answer here is maybe. We've decided that the average signed-in user will
always have lock available to them, regardless if we have a password hash or
not. We're still determining what to do with users who have 3rd party
authentication (SAML Identity Providers). We may need the new function call, but
I don't know that. So... I'll remove it for now. I can always dig this CL back
out and re-add it if necessary.

[Mike Lerman, 2014-09-09 16:47:53]

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

[Mike Lerman, 2014-09-09 16:47:54]

Hi Drew,

Adding you as a reviewer for c/b/signin since Roger's on vacation.

Thanks!

https://codereview.chromium.org/548393002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm
(left):

https://codereview.chromium.org/548393002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:463:
TEST_F(ProfileChooserControllerTest, SignedInProfileLockDisabled) {
On 2014/09/08 19:20:43, msw wrote:
> Does it make sense to leave these tests intact, and adjust their expectations
to
> the button being enabled? Or is that behavior sufficiently covered in other
> tests, where the lack of local auth credentials is a trivial difference not
> worth explicitly testing?

It's covered above. Line 381-5 within AccountManagementLayout verifies that the
Lock button is present and where it's supposed to be in the menu. These tests
were present only to explicitly test the Enabled/Disabled state.

[msw, 2014-09-09 16:52:56]

lgtm

https://codereview.chromium.org/548393002/diff/40001/chrome/browser/signin/lo...
File chrome/browser/signin/local_auth_unittest.cc (left):

https://codereview.chromium.org/548393002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth_unittest.cc:40:
EXPECT_TRUE(LocalAuthCredentialsExist(prof));
You can keep/move the functions if they're truly valuable here.

[Andrew T Wilson (Slow), 2014-09-09 20:26:07]

lgtm

[Mike Lerman, 2014-09-09 20:37:43]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-09-09 20:58:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlerman@chromium.org/548393002/40001

[commit-bot: I haz the power, 2014-09-09 23:01:05]

Committed patchset #2 (id:40001) as 6ae1bc5fe2bc726331666ed1bdfd14c73c111cfa

[commit-bot: I haz the power, 2014-09-10 03:56:13]

Patchset 2 (id:??) landed as
https://crrev.com/56f82b67ad5705722d1e5983e00f5dce401c1bd3
Cr-Commit-Position: refs/heads/master@{#294026}