Introduce new PasswordForm attributes for Credential Management API.

Introduce new PasswordForm attributes for Credential Management API.

Credential Management API is Javascript API that can be used by sites to sign user in or correctly store the credentials in the password manager.

BUG=400674
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291398

[vasilii, 2014-08-12 16:38:04]

Hi guys,

please review the CL.

[Ilya Sherman, 2014-08-12 17:51:51]

I don't think "YOLO" means anything to most people.  Could you clarify the CL
description?  (Unless this is meant to be vague, in which case I'd probably just
drop the acronym altogether.)

[Evan Stade, 2014-08-12 18:11:13]

On 2014/08/12 17:51:51, Ilya Sherman wrote:
> I don't think "YOLO" means anything to most people.

beg to differ :)

[vasilii, 2014-08-13 08:30:03]

On 2014/08/12 18:11:13, Evan Stade wrote:
> On 2014/08/12 17:51:51, Ilya Sherman wrote:
> > I don't think "YOLO" means anything to most people.
> 
> beg to differ :)

I've heard your cries :-)
Guys, please review.

[Mike West, 2014-08-18 18:52:12]

LGTM, but you'll need OWNERS reviews for the various bits. Thanks for this!

https://codereview.chromium.org/459103005/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_mac_unittest.cc (right):

https://codereview.chromium.org/459103005/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_mac_unittest.cc:214:
form->federation_url = GURL("http://accounts.google.com/login");
Nit: HTTPS throughout. :)

https://codereview.chromium.org/459103005/diff/20001/components/password_mana...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/459103005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:111:
s->BindString(COLUMN_FEDERATION_URL, form.federation_url.spec());
How do we deal with invalid URLs here? Perhaps a DCHECK somewhere?

https://codereview.chromium.org/459103005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:112:
s->BindInt(COLUMN_ZERO_CLICK, form.is_zero_click);
Nit: I'd suggest standardizing on either "zero click" or "is zero click".

https://codereview.chromium.org/459103005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:447: s.BindString(16,
form.origin.spec());
Nit: It's probably worth adding a "// WHERE starts here." comment, as this
confused me for a moment.

https://codereview.chromium.org/459103005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:561:
form->federation_url = GURL(s.ColumnString(COLUMN_FEDERATION_URL));
Nit: Do we do error handling anywhere else for things like malformed GURLs? If
not, perhaps we should DCHECK that the URLs are valid.

https://codereview.chromium.org/459103005/diff/20001/components/password_mana...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/459103005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:139:
form.avatar_url = GURL("http://accounts.google.com/Avatar");
Nit: HTTPS, please. We're not going to accept insecure URLs for either of these
values.

https://codereview.chromium.org/459103005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:598:
form.federation_url = GURL("http://accounts.google.com/federation");
Nit: Here too. :)

https://codereview.chromium.org/459103005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:944:
form.federation_url = GURL("http://accounts.google.com/federation");
Nit: And here.

[vasilii, 2014-08-20 11:45:35]

https://codereview.chromium.org/459103005/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_mac_unittest.cc (right):

https://codereview.chromium.org/459103005/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_mac_unittest.cc:214:
form->federation_url = GURL("http://accounts.google.com/login");
On 2014/08/18 18:52:11, Mike West wrote:
> Nit: HTTPS throughout. :)

Done.

https://codereview.chromium.org/459103005/diff/20001/components/password_mana...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/459103005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:111:
s->BindString(COLUMN_FEDERATION_URL, form.federation_url.spec());
On 2014/08/18 18:52:11, Mike West wrote:
> How do we deal with invalid URLs here? Perhaps a DCHECK somewhere?

GURL::spec() will assert and return the empty string for invalid URL. I guess
that PasswordManager should handle this.

https://codereview.chromium.org/459103005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:112:
s->BindInt(COLUMN_ZERO_CLICK, form.is_zero_click);
On 2014/08/18 18:52:11, Mike West wrote:
> Nit: I'd suggest standardizing on either "zero click" or "is zero click".

Done.

https://codereview.chromium.org/459103005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:447: s.BindString(16,
form.origin.spec());
On 2014/08/18 18:52:11, Mike West wrote:
> Nit: It's probably worth adding a "// WHERE starts here." comment, as this
> confused me for a moment.

Done.

https://codereview.chromium.org/459103005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:561:
form->federation_url = GURL(s.ColumnString(COLUMN_FEDERATION_URL));
On 2014/08/18 18:52:11, Mike West wrote:
> Nit: Do we do error handling anywhere else for things like malformed GURLs? If
> not, perhaps we should DCHECK that the URLs are valid.

But empty URLs are invalid too.

https://codereview.chromium.org/459103005/diff/20001/components/password_mana...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/459103005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:139:
form.avatar_url = GURL("http://accounts.google.com/Avatar");
On 2014/08/18 18:52:12, Mike West wrote:
> Nit: HTTPS, please. We're not going to accept insecure URLs for either of
these
> values.

Done.

https://codereview.chromium.org/459103005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:598:
form.federation_url = GURL("http://accounts.google.com/federation");
On 2014/08/18 18:52:12, Mike West wrote:
> Nit: Here too. :)

Done.

https://codereview.chromium.org/459103005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:944:
form.federation_url = GURL("http://accounts.google.com/federation");
On 2014/08/18 18:52:12, Mike West wrote:
> Nit: And here.

Done.

[Garrett Casto, 2014-08-21 22:13:41]

lgtm

https://codereview.chromium.org/459103005/diff/40001/components/autofill/core...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/459103005/diff/40001/components/autofill/core...
components/autofill/core/common/password_form.h:209: // User friendly name to
show in the UI.
Can you put a comment here talking about how these fields are going to be used
for a new authentication flow, will not be set for HTTP/HTML auth forms, etc.?
Without any context it's not clear why these fields should be here.

[vasilii, 2014-08-22 09:03:33]

Mike, last pass?

https://codereview.chromium.org/459103005/diff/40001/components/autofill/core...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/459103005/diff/40001/components/autofill/core...
components/autofill/core/common/password_form.h:209: // User friendly name to
show in the UI.
On 2014/08/21 22:13:41, Garrett Casto wrote:
> Can you put a comment here talking about how these fields are going to be used
> for a new authentication flow, will not be set for HTTP/HTML auth forms, etc.?
> Without any context it's not clear why these fields should be here.

Done.

[Mike West, 2014-08-22 09:53:14]

SLGTM.

https://codereview.chromium.org/459103005/diff/60001/components/autofill/core...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/459103005/diff/60001/components/autofill/core...
components/autofill/core/common/password_form.h:210: // fields are set by site.
When parsing an HTML form, this is not used.
Nit: I'd suggest a slight rewording for clarity. How about "These following
fields are set by a website using the Credential Manager API. They will be empty
and remain unused for sites which do not use that API."?

[vasilii, 2014-08-22 12:52:33]

https://codereview.chromium.org/459103005/diff/60001/components/autofill/core...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/459103005/diff/60001/components/autofill/core...
components/autofill/core/common/password_form.h:210: // fields are set by site.
When parsing an HTML form, this is not used.
On 2014/08/22 09:53:13, Mike West wrote:
> Nit: I'd suggest a slight rewording for clarity. How about "These following
> fields are set by a website using the Credential Manager API. They will be
empty
> and remain unused for sites which do not use that API."?

Done.

[vasilii, 2014-08-22 12:52:37]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-08-22 12:53:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vasilii@chromium.org/459103005/80001

[commit-bot: I haz the power, 2014-08-22 13:57:35]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[commit-bot: I haz the power, 2014-08-22 14:05:42]

Committed patchset #5 (80001) as 291398