Credential Management: Support zeroclick credential in 'request()'.

Credential Management: Support zeroclick credential in 'request()'.

If only a single credential is available, and zero-click behavior is
enabled, then we don't need to ask the user for permission to present
the credential in response to a website's call to `request()`.

This patch implements that logic, walking through the local credentials
for an origin in the password store, and returning zeroclick credentials
without prompting.

If no such credential is available, and the caller set 'zero_click_only',
then we also won't prompt the user, and will simply return an empty
credential.

BUG=400674
Committed: https://crrev.com/5fdae3e42e19f605f65d9608eda189a31642dee4
Cr-Commit-Position: refs/heads/master@{#313885}

[Mike West, 2015-01-28 12:30:35]

mkwst@chromium.org changed reviewers:
+ vabr@chromium.org, vasilii@chromium.org

[Mike West, 2015-01-28 12:30:36]

Vaclav, mind taking a look, since Vasilii isn't at his desk? :)

[vasilii, 2015-01-28 13:25:30]

https://codereview.chromium.org/879913004/diff/20001/components/password_mana...
File
components/password_manager/content/browser/credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/879913004/diff/20001/components/password_mana...
components/password_manager/content/browser/credential_manager_dispatcher.cc:170:
if (form->is_zero_click && !multiple_zero_click) {
AS we discussed, I'll rename form->is_zero_click to
form->one_time_disable_zero_click.
Therefore I'd suggest that you don't use it for now.
On the other hand, you can use CredentialManagerDispatcher::IsZeroClickAllowed()
(to be extended with the global flag) assuming that "allow zero click" per form
is true.

[vabr (Chromium), 2015-01-28 13:37:52]

This LGTM, although I expect you want to clarify Vasilii's comment with him
first.

https://codereview.chromium.org/879913004/diff/20001/components/password_mana...
File
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/879913004/diff/20001/components/password_mana...
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc:312:
dispatcher()->OnRequestCredential(kRequestId, true, federations);
nit: I guess that "true" is the "require 0-click" bit, right? Could you please
comment it?

https://codereview.chromium.org/879913004/diff/20001/components/password_mana...
File
components/password_manager/content/browser/credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/879913004/diff/20001/components/password_mana...
components/password_manager/content/browser/credential_manager_dispatcher.cc:178:
discarded_results.push_back(form);
Why not just
delete form;
?

https://codereview.chromium.org/879913004/diff/20001/components/password_mana...
components/password_manager/content/browser/credential_manager_dispatcher.cc:190:
discarded_results.assign(local_results.begin(), local_results.end());
Why not use STLDeleteElements? This is a bit cryptic. Also below.

(FYI, I should be soon changing the type of results to ScopedVector, so
ultimately explicit deletion will not be needed.)

[Mike West, 2015-01-28 14:42:14]

WDYT of this approach, Vasilii?

https://codereview.chromium.org/879913004/diff/20001/components/password_mana...
File
components/password_manager/content/browser/credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/879913004/diff/20001/components/password_mana...
components/password_manager/content/browser/credential_manager_dispatcher.cc:170:
if (form->is_zero_click && !multiple_zero_click) {
On 2015/01/28 13:25:30, vasilii wrote:
> AS we discussed, I'll rename form->is_zero_click to
> form->one_time_disable_zero_click.
> Therefore I'd suggest that you don't use it for now.
> On the other hand, you can use
CredentialManagerDispatcher::IsZeroClickAllowed()
> (to be extended with the global flag) assuming that "allow zero click" per
form
> is true.

Perfect, thanks.

https://codereview.chromium.org/879913004/diff/20001/components/password_mana...
components/password_manager/content/browser/credential_manager_dispatcher.cc:178:
discarded_results.push_back(form);
On 2015/01/28 13:37:52, vabr (Chromium) wrote:
> Why not just
> delete form;
> ?

That would be too easy!

https://codereview.chromium.org/879913004/diff/20001/components/password_mana...
components/password_manager/content/browser/credential_manager_dispatcher.cc:190:
discarded_results.assign(local_results.begin(), local_results.end());
On 2015/01/28 13:37:52, vabr (Chromium) wrote:
> Why not use STLDeleteElements? This is a bit cryptic. Also below.
> 
> (FYI, I should be soon changing the type of results to ScopedVector, so
> ultimately explicit deletion will not be needed.)

Sure. I can do that.

https://codereview.chromium.org/879913004/diff/40001/components/password_mana...
File
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/879913004/diff/40001/components/password_mana...
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc:298:
client_->set_zero_click_disabled(true);
Since we don't check the 'is_zero_click' attribute anymore, this failed to pop a
prompt.

https://codereview.chromium.org/879913004/diff/40001/components/password_mana...
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc:374:
client_->set_zero_click_disabled(true);
Ditto.

https://codereview.chromium.org/879913004/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/879913004/diff/40001/components/password_mana...
components/password_manager/core/browser/password_manager_client.h:167: virtual
bool IsZeroClickDisabled();
Added this so I can override it in the client; since this patch no longer relies
on the 'is_zero_click' attribute, previously passing tests were no longer
triggering the prompt. See the other comments for details.

[vasilii, 2015-01-28 16:14:59]

lgtm

https://codereview.chromium.org/879913004/diff/60001/components/password_mana...
File
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/879913004/diff/60001/components/password_mana...
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc:323:
CredentialManagerOnRequestCredentialWithZeroClickOnlyEmptyPasswordStore) {
optional: the test names became too long. I'd omit "CredentialManager"
substring. It doesn't seem that a reader would miss the context looking at
CredentialManagerDispatcherTest.* in
content_credential_manager_dispatcher_unittest.cc.

https://codereview.chromium.org/879913004/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/879913004/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager_client.h:167: virtual
bool IsZeroClickDisabled();
I'd prefer to be on the positive side: IsZeroClickEnabled(). It'll be more
convenient.

[vasilii, 2015-01-28 16:16:17]

It seems the description is obsolete.

[Mike West, 2015-01-29 11:36:34]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2015-01-29 11:37:06]

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

[commit-bot: I haz the power, 2015-01-29 12:51:49]

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

[commit-bot: I haz the power, 2015-01-29 12:51:50]

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

[Mike West, 2015-01-30 08:19:37]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2015-01-30 08:20:33]

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

[commit-bot: I haz the power, 2015-01-30 09:09:09]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-30 09:10:08]

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