CREDENTIAL: Implement the 'PasswordCredential(HTMLFormElement)' constructor.

CREDENTIAL: Implement the 'PasswordCredential(HTMLFormElement)' constructor.

After some discussion in https://github.com/w3c/webappsec-credential-management/issues/13,
we've added a new constructor to 'PasswordCredential' with the goal of
encouraging the usage of backwards-compatible autocomplete attributes.

Spec: https://w3c.github.io/webappsec-credential-management/#passwordcredential-form-constructor

BUG=597569
Committed: https://crrev.com/506f83825ac36bbf9d95ca4fc5e7edb65dc54a51
Cr-Commit-Position: refs/heads/master@{#384494}

[Mike West, 2016-03-24 11:21:02]

mkwst@chromium.org changed reviewers:
+ philipj@opera.com

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

Hey Philip, do you have time to review this patch?

-mike

[Mike West, 2016-03-24 15:16:26]

mkwst@chromium.org changed reviewers:
+ jochen@chromium.org

[Mike West, 2016-03-24 15:16:27]

+Jochen, just in case you're bored over the long weekend.

[philipj_slow, 2016-03-29 05:45:54]

lgtm

https://codereview.chromium.org/1828213002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html
(right):

https://codereview.chromium.org/1828213002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html:137:
assert_throws(new TypeError(), _ => { new PasswordCredential(f); });
{} seem to not be needed here.

https://codereview.chromium.org/1828213002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html:143:
password.value = "sekrit";
assert that it doesn't throw after this to make sure password was restored
correctly

https://codereview.chromium.org/1828213002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp
(right):

https://codereview.chromium.org/1828213002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp:56:
for (FormAssociatedElement* element : form->associatedElements()) {
The spec wants the "submittable elements" which is a subset of the
"form-associated elements". Is it the early return below that squares the
difference? If so, documenting this would be nice.

https://codereview.chromium.org/1828213002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp:65:
if (equalIgnoringCase(autocomplete, "name") || equalIgnoringCase(autocomplete,
"nickname"))
Maybe more else all the way down? Unless there's a lot of inlining the compiler
probably can't tell that the cases are mutually exclusive.

And same order as in the spec?

[jochen (gone - plz use gerrit), 2016-03-30 16:29:26]

lgtm

[Mike West, 2016-03-31 08:17:20]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-03-31 08:17:21]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1828213002/#ps20001
(title: "Rebase+Feedback")

[commit-bot: I haz the power, 2016-03-31 08:17:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828213002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828213002/20001

[Mike West, 2016-03-31 08:31:41]

Thanks!

https://codereview.chromium.org/1828213002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html
(right):

https://codereview.chromium.org/1828213002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html:137:
assert_throws(new TypeError(), _ => { new PasswordCredential(f); });
On 2016/03/29 at 05:45:54, philipj_sick_March31 wrote:
> {} seem to not be needed here.

Removed.

https://codereview.chromium.org/1828213002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html:143:
password.value = "sekrit";
On 2016/03/29 at 05:45:54, philipj_sick_March31 wrote:
> assert that it doesn't throw after this to make sure password was restored
correctly

Sure.

https://codereview.chromium.org/1828213002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp
(right):

https://codereview.chromium.org/1828213002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp:56:
for (FormAssociatedElement* element : form->associatedElements()) {
On 2016/03/29 at 05:45:54, philipj_sick_March31 wrote:
> The spec wants the "submittable elements" which is a subset of the
"form-associated elements". Is it the early return below that squares the
difference? If so, documenting this would be nice.

Done.

https://codereview.chromium.org/1828213002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp:65:
if (equalIgnoringCase(autocomplete, "name") || equalIgnoringCase(autocomplete,
"nickname"))
On 2016/03/29 at 05:45:54, philipj_sick_March31 wrote:
> Maybe more else all the way down? Unless there's a lot of inlining the
compiler probably can't tell that the cases are mutually exclusive.
> 
> And same order as in the spec?

Done and done.

[commit-bot: I haz the power, 2016-03-31 09:18:24]

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

[commit-bot: I haz the power, 2016-03-31 09:18:25]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Mike West, 2016-03-31 17:06:11]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2016-03-31 17:06:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828213002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828213002/20001

[commit-bot: I haz the power, 2016-03-31 18:00:56]

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

[commit-bot: I haz the power, 2016-03-31 18:00:57]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Mike West, 2016-03-31 18:54:47]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2016-03-31 18:55:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828213002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828213002/20001

[Mike West, 2016-03-31 19:40:39]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-03-31 19:40:39]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1828213002/#ps40001
(title: "EXPORT")

[commit-bot: I haz the power, 2016-03-31 19:41:27]

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

[commit-bot: I haz the power, 2016-03-31 20:05:33]

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

[commit-bot: I haz the power, 2016-03-31 20:05:34]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Mike West, 2016-03-31 21:21:03]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-03-31 21:21:04]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1828213002/#ps60001
(title: "EXPORT2")

[commit-bot: I haz the power, 2016-03-31 21:21:26]

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

[commit-bot: I haz the power, 2016-03-31 21:29:58]

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

[commit-bot: I haz the power, 2016-03-31 21:29:59]

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

[Mike West, 2016-04-01 05:09:32]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2016-04-01 05:10:06]

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

[commit-bot: I haz the power, 2016-04-01 05:39:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-01 05:41:13]

Description was changed from

==========
CREDENTIAL: Implement the 'PasswordCredential(HTMLFormElement)' constructor.

After some discussion in
https://github.com/w3c/webappsec-credential-management/issues/13,
we've added a new constructor to 'PasswordCredential' with the goal of
encouraging the usage of backwards-compatible autocomplete attributes.

Spec:
https://w3c.github.io/webappsec-credential-management/#passwordcredential-for...

BUG=597569
==========

to

==========
CREDENTIAL: Implement the 'PasswordCredential(HTMLFormElement)' constructor.

After some discussion in
https://github.com/w3c/webappsec-credential-management/issues/13,
we've added a new constructor to 'PasswordCredential' with the goal of
encouraging the usage of backwards-compatible autocomplete attributes.

Spec:
https://w3c.github.io/webappsec-credential-management/#passwordcredential-for...

BUG=597569

Committed: https://crrev.com/506f83825ac36bbf9d95ca4fc5e7edb65dc54a51
Cr-Commit-Position: refs/heads/master@{#384494}
==========

[commit-bot: I haz the power, 2016-04-01 05:41:14]

Patchset 4 (id:??) landed as
https://crrev.com/506f83825ac36bbf9d95ca4fc5e7edb65dc54a51
Cr-Commit-Position: refs/heads/master@{#384494}