CREDENTIAL: Teach the 'PasswordCredential(HTMLFormElement)' constructor about URLSearchParams

CREDENTIAL: Teach the 'PasswordCredential(HTMLFormElement)' constructor about URLSearchParams

If the form's 'enctype' attribute is not explicitly 'multipart/form-data',
then generate a 'PasswordCredential' object that will be submitted with a
urlencoded body, just like the form itself would have been.

BUG=597569
Committed: https://crrev.com/6ba9227c85e36614c2d925f7e4ded01d0487cfd1
Cr-Commit-Position: refs/heads/master@{#384514}

[Mike West, 2016-03-24 12:02:08]

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

[Mike West, 2016-03-24 12:02:09]

And this one, maybe?

Sorry if you're overloaded... Just let me know. :)

-mike

[philipj_slow, 2016-03-24 13:58:09]

Started looking but have to call it a day now. I have Friday and Monday off
(public holidays) so I can't promise review until Tuesday.

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

https://codereview.chromium.org/1828923003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html:144:
f.setAttribute('enctype', 'multipart/form-data');
enctype is a reflected attribute (per spec) if you'd like to use that.

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

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

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

Also +Jochen, also if you're bored. :)

Thanks, Philip!

[philipj_slow, 2016-03-29 06:07:34]

lgtm

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

https://codereview.chromium.org/1828923003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html:144:
f.setAttribute('enctype', 'multipart/form-data');
On 2016/03/24 13:58:09, philipj_OOO_til_March29 wrote:
> enctype is a reflected attribute (per spec) if you'd like to use that.

However you change this, maybe use a non-canonical case to verify that the
implementation doesn't match case-sensitively.

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

https://codereview.chromium.org/1828923003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp:90:
if (equalIgnoringCase(form->fastGetAttribute(HTMLNames::enctypeAttr),
"multipart/form-data")) {
The spec doesn't say "ASCII case-insensitive" here. The IDL attribute is
reflected and normalizes case in the getter, but your spec links to the content
attribute.

Maybe you should use
https://html.spec.whatwg.org/multipage/forms.html#concept-fs-enctype in the
spec, and FormSubmission::encodingType() here? If that's not possible
implementation-wise, then just this with a comment that it's really supposed to
be checking the state would work I think.

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

lgtm

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

Thanks!

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

https://codereview.chromium.org/1828923003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html:144:
f.setAttribute('enctype', 'multipart/form-data');
On 2016/03/29 at 06:07:34, philipj_sick_March31 wrote:
> On 2016/03/24 13:58:09, philipj_OOO_til_March29 wrote:
> > enctype is a reflected attribute (per spec) if you'd like to use that.
> 
> However you change this, maybe use a non-canonical case to verify that the
implementation doesn't match case-sensitively.

Done and done. :)

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

https://codereview.chromium.org/1828923003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp:90:
if (equalIgnoringCase(form->fastGetAttribute(HTMLNames::enctypeAttr),
"multipart/form-data")) {
On 2016/03/29 at 06:07:34, philipj_sick_March31 wrote:
> The spec doesn't say "ASCII case-insensitive" here. The IDL attribute is
reflected and normalizes case in the getter, but your spec links to the content
attribute.

Well, the spec says that the content attribute is an enumerated attribute, which
already does case-folding and etc. `FormSubmission::encodingType()` is certainly
what we want, and I just missed that it's exposed as
`HTMLFormElement::enctype()`.

> Maybe you should use
https://html.spec.whatwg.org/multipage/forms.html#concept-fs-enctype in the
spec, and FormSubmission::encodingType() here? If that's not possible
implementation-wise, then just this with a comment that it's really supposed to
be checking the state would work I think.

This seems reasonable. I've updated the spec in
https://github.com/w3c/webappsec-credential-management/commit/736f64a1a09dbbb....

[Mike West, 2016-04-01 06:47:23]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-04-01 06:47:23]

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/1828923003/#ps20001
(title: "Rebase+Feedback")

[commit-bot: I haz the power, 2016-04-01 06:47:43]

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

[commit-bot: I haz the power, 2016-04-01 07:59:26]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-01 08:01:42]

Description was changed from

==========
CREDENTIAL: Teach the 'PasswordCredential(HTMLFormElement)' constructor about
URLSearchParams

If the form's 'enctype' attribute is not explicitly 'multipart/form-data',
then generate a 'PasswordCredential' object that will be submitted with a
urlencoded body, just like the form itself would have been.

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

to

==========
CREDENTIAL: Teach the 'PasswordCredential(HTMLFormElement)' constructor about
URLSearchParams

If the form's 'enctype' attribute is not explicitly 'multipart/form-data',
then generate a 'PasswordCredential' object that will be submitted with a
urlencoded body, just like the form itself would have been.

BUG=597569

Committed: https://crrev.com/6ba9227c85e36614c2d925f7e4ded01d0487cfd1
Cr-Commit-Position: refs/heads/master@{#384514}
==========

[commit-bot: I haz the power, 2016-04-01 08:01:43]

Patchset 2 (id:??) landed as
https://crrev.com/6ba9227c85e36614c2d925f7e4ded01d0487cfd1
Cr-Commit-Position: refs/heads/master@{#384514}