Credential Manager: Introduce PendingRequestTask.

Credential Manager: Introduce PendingRequestTask.

Currently, CredentialManagerDispatcher is a PasswordStoreObserver, and
handles responses from `request()` directly. We can't do that going
forward, as we also need to handle responses when dealing with (at
least) `notifySignedOut()`.

This patch splits the password store observer functionality out into
a new `PendingRequestTask` class, with the intent of introducing
`PendingSignedOutTask`, etc. classes in the future.

BUG=450581
R=vabr@chromium.org

Committed: https://crrev.com/a5b7b8897f016a6f4c12c726eb061e3953e9fbe6
Cr-Commit-Position: refs/heads/master@{#314154}

[Mike West, 2015-01-29 13:07:27]

WDYT, Vaclav? The new class doesn't seem substantial enough to split out into a
separate file, nor do I really want to move the request processing out somewhere
else.

I don't really like that the guts of the logic live in the new class, however.
Hrm. Not sure if this is the right structure.

Thoughts?

[Mike West, 2015-01-29 14:03:07]

(Sorry, shouldn't have thrown it to the bots. It relies on
https://codereview.chromium.org/879913004/, which hasn't landed yet. :( )

[vabr (Chromium), 2015-01-29 14:04:35]

Hi Mike,

I left a couple of comments. I don't see a problem with moving
OnGetPasswordStoreResults in the inner class.

Also, I assume that the only change to the body of OnGetPasswordStoreResults was
to adjust to the move, nothing functional. Please let me know, if there was
more.

Cheers,
Vaclav

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

https://codereview.chromium.org/886793002/diff/1/components/password_manager/...
components/password_manager/content/browser/credential_manager_dispatcher.cc:91:
CredentialManagerDispatcher* dispatcher_;
nit: Maybe comment that this is a weak (back-)link to the owning class.

https://codereview.chromium.org/886793002/diff/1/components/password_manager/...
components/password_manager/content/browser/credential_manager_dispatcher.cc:91:
CredentialManagerDispatcher* dispatcher_;
All data members here should be const, they are not changed anywhere. (Beware of
making dispatcher_ a constant pointer, not a pointer to a constant.)

https://codereview.chromium.org/886793002/diff/1/components/password_manager/...
components/password_manager/content/browser/credential_manager_dispatcher.cc:95:
std::vector<GURL> federations_;
Seems like a lot of stuff to copy. Is there any chance that keeping a const ref
would actually suffice? If yes, great!
If not (because the lifetime issues are not clear (like -- who owns federations
passed into CredentialManagerDispatcher::OnRequestCredential), could we transfer
ownership to the PendingRequestTask by using a ScopedVector instead? I'd be fine
with doing the latter in a follow-up, providing a corresponding TODO comment in
this CL.

This is in light of the performance focus -- copying lists of strings (which
GURLs ultimately are) has been known to eat a lot of time.

https://codereview.chromium.org/886793002/diff/1/components/password_manager/...
components/password_manager/content/browser/credential_manager_dispatcher.cc:96:
};
Does this class need to be copied or assigned? If not, please use
DISALLOW_COPY_AND_ASSIGN.

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

https://codereview.chromium.org/886793002/diff/1/components/password_manager/...
components/password_manager/content/browser/credential_manager_dispatcher.h:87:
// PasswordStore; we store request details here in order to properly
nit: Should the comment be updated to cover the new functionality beyond storing
data?

[Mike West, 2015-01-30 12:14:37]

Mind taking another look?

[vabr (Chromium), 2015-01-30 12:54:19]

LGTM with comments.

Thanks!
Vaclav

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

https://codereview.chromium.org/886793002/diff/20001/components/password_mana...
components/password_manager/content/browser/credential_manager_dispatcher.cc:37:
for (const GURL& origin : request_federations)
If this is possible in the IPC framework we have, I suggest the following
changes:
1. Changing the type of federations in
CredentialManagerDispatcher::OnRequestCredential and request_federations here to
something non-const.
2. Making federations_ a vector of GURL (given that we do not expect duplicated
URLs, and creating the set is actually O(N logN), the linear search in the
vector as opposed to the log search in the map should not make the performance
worse).
3. Swapping the request_federations with federations_ instead of copying the
contents.

If the above is not possible, let's live with the copy you have here.

https://codereview.chromium.org/886793002/diff/20001/components/password_mana...
components/password_manager/content/browser/credential_manager_dispatcher.cc:48:
std::vector<autofill::PasswordForm*> local_results;
Consider using ScopedVector for both these vectors. Then you can drop the
STLDeleteElements, and use .get() if you still need the vector with the raw
pointers.

[Mike West, 2015-02-02 08:56:20]

https://codereview.chromium.org/886793002/diff/20001/components/password_mana...
>
components/password_manager/content/browser/credential_manager_dispatcher.cc:37:
> for (const GURL& origin : request_federations)
> If this is possible in the IPC framework we have, I suggest the following
> changes:
>
> [SNIP]
>
> If the above is not possible, let's live with the copy you have here.

The IPC system is unhappy with an attempt to remove `const`. :(

>
https://codereview.chromium.org/886793002/diff/20001/components/password_mana...
>
components/password_manager/content/browser/credential_manager_dispatcher.cc:48:
> std::vector<autofill::PasswordForm*> local_results;
> Consider using ScopedVector for both these vectors. Then you can drop the
> STLDeleteElements, and use .get() if you still need the vector with the raw
> pointers.

Done.

[vabr (Chromium), 2015-02-02 09:15:45]

> The IPC system is unhappy with an attempt to remove `const`. :(

Fair enough. We can leave it as it is, but could you please insert a TODO(vabr)
to have a look how to avoid copying the federations out by accessing the
IPC-supplied argument in a non-const way?

Thanks & LGTM!

Vaclav

[Mike West, 2015-02-02 09:22:17]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2015-02-02 09:23:21]

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

[commit-bot: I haz the power, 2015-02-02 10:13:01]

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

[commit-bot: I haz the power, 2015-02-02 10:13:01]

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

[Mike West, 2015-02-02 15:37:34]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2015-02-02 15:37:50]

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

[Mike West, 2015-02-02 15:38:32]

Unscopedvectorizing the code; it's going to require changes to the UI code to
accept a ScopedVector, otherwise the PasswordForm items are destructed before
they're used in the bubble.

Added a TODO for a subsequent CL.

[commit-bot: I haz the power, 2015-02-02 16:17:58]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-02 16:19:09]

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

[vabr (Chromium), 2015-02-02 16:52:34]

On 2015/02/02 15:38:32, Mike West wrote:
> Unscopedvectorizing the code; it's going to require changes to the UI code to
> accept a ScopedVector, otherwise the PasswordForm items are destructed before
> they're used in the bubble.
> 
> Added a TODO for a subsequent CL.

Don't worry, this is being taken care of already in
https://codereview.chromium.org/866983003/.

Cheers,
Vaclav