CREDENTIAL: Persist PasswordForm's primary key through CredentialInfo.

CREDENTIAL: Persist PasswordForm's primary key through CredentialInfo.

When we create a PasswordForm from a CredentialInfo, we're stripping out
data that PasswordStore requires in order to find the form in the
database. This breaks the auto-sign-in bit toggle for the credential if
it was saved via our heuristics, and not via the API.

This patch extends CredentialInfo to ensure that the data we need is
available.

BUG=594110
R=vasilii@chromium.org

[Mike West, 2016-03-11 12:12:06]

WDYT?

[vasilii, 2016-03-11 13:20:05]

I don't think that the new fields belong to CredentialInfo. It is the structure
for transfer data to the renderer.
What about returning the entire PasswordForm to  CredentialManagerDispatcher?

[Mike West, 2016-03-11 13:54:53]

On 2016/03/11 at 13:20:05, vasilii wrote:
> I don't think that the new fields belong to CredentialInfo. It is the
structure for transfer data to the renderer.
> What about returning the entire PasswordForm to  CredentialManagerDispatcher?

I think this is a lot simpler. :)

I think passing the PasswordForm back directly is probably a good thing to do,
but it requires a larger set of changes, and would be riskier to merge back to
M50 if we decide we want to try doing that. This seems fairly minimal.

What do you think about combining a refactoring of the SendCredential call with
a larger set of changes to remove the "federated_credentials" vector from all
the various UI call sites we pipe it through? We can remove basically all the
logic from
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
and make it pretty straightforward to pass just the form back.

[vasilii, 2016-03-11 14:29:48]

I'm OK with the quick fix for M50. But for ToT we can do better.

https://codereview.chromium.org/1785273002/diff/20001/components/password_man...
File
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/1785273002/diff/20001/components/password_man...
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc:423:
form_.signon_realm = "this is a realm";
I want another origin_url too. It's a part of the unique key.

https://codereview.chromium.org/1785273002/diff/20001/components/password_man...
File components/password_manager/core/common/credential_manager_types.cc
(right):

https://codereview.chromium.org/1785273002/diff/20001/components/password_man...
components/password_manager/core/common/credential_manager_types.cc:61: if
(info.signon_realm.empty()) {
How can it be empty?

https://codereview.chromium.org/1785273002/diff/20001/components/password_man...
File components/password_manager/core/common/credential_manager_types.h (right):

https://codereview.chromium.org/1785273002/diff/20001/components/password_man...
components/password_manager/core/common/credential_manager_types.h:67:
std::string signon_realm;
What if you just have autofill::PasswordForm or scoped_ptr<PasswordForm> here.
And copy it or some of its fields?
I have concerns that PasswordForm::origin may be different from what you pass
into CreatePasswordFormFromCredentialInfo.

[vasilii, 2016-03-24 10:19:35]

What's with this? I don't think that merging to M50 now is justified. How about
implementing the proper fix? If you don't have time, please let me know and I'll
take this on.

[Mike West, 2016-03-24 11:24:42]

On 2016/03/24 at 10:19:35, vasilii wrote:
> What's with this? I don't think that merging to M50 now is justified. How
about implementing the proper fix? If you don't have time, please let me know
and I'll take this on.

If you have time to turn this into something shiny and good, please do. I've
been buried in perf the last week, and now I'm trying to catch up on last minute
API changes that Mozilla has requested (like
https://codereview.chromium.org/1828213002).

If you don't have time, I'll get to it next week (I'm out through Wednesday). No
worries either way.

[vasilii, 2016-03-24 17:58:56]

On 2016/03/24 11:24:42, Mike West wrote:
> On 2016/03/24 at 10:19:35, vasilii wrote:
> > What's with this? I don't think that merging to M50 now is justified. How
> about implementing the proper fix? If you don't have time, please let me know
> and I'll take this on.
> 
> If you have time to turn this into something shiny and good, please do. I've
> been buried in perf the last week, and now I'm trying to catch up on last
minute
> API changes that Mozilla has requested (like
> https://codereview.chromium.org/1828213002).
> 
> If you don't have time, I'll get to it next week (I'm out through Wednesday).
No
> worries either way.

I copied the CL to https://codereview.chromium.org/1832933002/