Credential Manager: Switch 'request()' to GetAutofillableLogins().

Credential Manager: Switch 'request()' to GetAutofillableLogins().

This patch changes the CredentialManagerDispatcher implementation of
'request()' to grab all the credentials from the PasswordStore, rather
than just those that match the origin of the currently committed
WebContents. This change is the first step towards supporting federations.

Rather than doing multiple requests to the PasswordStore for the current
origin, and each of the federation origins, we'll grab everything, and
filter the result list before handing it back to the UI for display.

BUG=400674
Committed: https://crrev.com/7d06c95de96bac42e426e11cb7119ee3341e3df0
Cr-Commit-Position: refs/heads/master@{#308362}

[Mike West, 2014-12-15 09:58:17]

mkwst@chromium.org changed reviewers:
+ vabr@google.com

[Mike West, 2014-12-15 09:58:18]

vabr@: Mind taking a look at this CL, following up from our conversation ~2 two
weeks ago (before I went to MTV, and then spent a week dying of terrible
illnesses).

gcasto@, vasilii@: FYI.

[vabr (Chromium), 2014-12-15 10:28:02]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org
- vabr@google.com

[vabr (Chromium), 2014-12-15 10:47:22]

LGTM!

Vaclav

https://codereview.chromium.org/802303002/diff/1/components/password_manager/...
File
components/password_manager/content/browser/content_credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/802303002/diff/1/components/password_manager/...
components/password_manager/content/browser/content_credential_manager_dispatcher.cc:143:
if (filtered_results.empty() ||
An optional "BTW" nit:
It is important that in this if-condition, PromptUserToChooseCredentials remains
always the last one (otherwise we might invoke the prompt for bad origins etc.).
If I were rewriting the code, I would probably separate the first two checks
into their own if-block with an early return, to make that clear. But that's
both just an optional subjective suggestion, and it does not be addressed in
this CL.

https://codereview.chromium.org/802303002/diff/1/components/password_manager/...
File
components/password_manager/content/browser/content_credential_manager_dispatcher.h
(right):

https://codereview.chromium.org/802303002/diff/1/components/password_manager/...
components/password_manager/content/browser/content_credential_manager_dispatcher.h:10:
#include "base/memory/weak_ptr.h"
Pleas add #include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/802303002/diff/1/components/password_manager/...
File
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/802303002/diff/1/components/password_manager/...
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc:241:
store_->AddLogin(cross_origin_form_);
I'm not sure what are you testing here. That the SendCredential message is sent
even though the store also contains credentials for other sites?

[Mike West, 2014-12-15 12:59:24]

Mind taking another look at the change to
content_credential_manager_dispatcher.cc:144 to dispose of unused PasswordForm
objects?

https://codereview.chromium.org/802303002/diff/1/components/password_manager/...
File
components/password_manager/content/browser/content_credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/802303002/diff/1/components/password_manager/...
components/password_manager/content/browser/content_credential_manager_dispatcher.cc:143:
if (filtered_results.empty() ||
On 2014/12/15 10:47:22, vabr (OOO soon) wrote:
> An optional "BTW" nit:
> It is important that in this if-condition, PromptUserToChooseCredentials
remains
> always the last one (otherwise we might invoke the prompt for bad origins
etc.).
> If I were rewriting the code, I would probably separate the first two checks
> into their own if-block with an early return, to make that clear. But that's
> both just an optional subjective suggestion, and it does not be addressed in
> this CL.

I'll take a look at that in the next CL when I poke at the filters again.

https://codereview.chromium.org/802303002/diff/1/components/password_manager/...
File
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/802303002/diff/1/components/password_manager/...
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc:241:
store_->AddLogin(cross_origin_form_);
On 2014/12/15 10:47:22, vabr (OOO soon) wrote:
> I'm not sure what are you testing here. That the SendCredential message is
sent
> even though the store also contains credentials for other sites?

We're now requesting all the data, not just all the data for a specific origin.
I want to ensure that extra data in the DB doesn't confuse things.

Splitting this out into a separate test for clarity.

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

https://codereview.chromium.org/802303002/diff/20001/components/password_mana...
components/password_manager/content/browser/content_credential_manager_dispatcher.cc:144:
delete form;
I've made this change from patchset #1 to make the ASAN bot happy. WDYT?

[vabr (Chromium), 2014-12-15 13:27:26]

lgtm

https://codereview.chromium.org/802303002/diff/1/components/password_manager/...
File
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/802303002/diff/1/components/password_manager/...
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc:241:
store_->AddLogin(cross_origin_form_);
On 2014/12/15 12:59:24, Mike West wrote:
> On 2014/12/15 10:47:22, vabr (OOO soon) wrote:
> > I'm not sure what are you testing here. That the SendCredential message is
> sent
> > even though the store also contains credentials for other sites?
> 
> We're now requesting all the data, not just all the data for a specific
origin.
> I want to ensure that extra data in the DB doesn't confuse things.
> 
> Splitting this out into a separate test for clarity.

Acknowledged, the new test makes good sense to me.

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

https://codereview.chromium.org/802303002/diff/20001/components/password_mana...
components/password_manager/content/browser/content_credential_manager_dispatcher.cc:144:
delete form;
On 2014/12/15 12:59:24, Mike West wrote:
> I've made this change from patchset #1 to make the ASAN bot happy. WDYT?

ASAN bot and you are two pretty smart guys. SGTM!

https://codereview.chromium.org/802303002/diff/20001/components/password_mana...
components/password_manager/content/browser/content_credential_manager_dispatcher.cc:151:
SendCredential(pending_request_->id, CredentialInfo());
I like the split, but it needs an early return, otherwise
PromptUserToChooseCredentials below gets called way too often.

[Mike West, 2014-12-15 14:56:54]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-12-15 14:57:11]

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

[commit-bot: I haz the power, 2014-12-15 16:33:14]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-15 16:33:56]

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