CREDENTIAL: Implement 'SiteBoundCredential'.

CREDENTIAL: Implement 'SiteBoundCredential'.

This patch shims 'SiteBoundCredential' between 'Credential' and
'{Password,Federated}Credential'. I don't see any need to push this noop
up the stack, however, so it does not introduce a similar concept at
the platform layer, nor does //content need to learn about this
indirection.

As a drive-by, this patch adds some IDL tests for 'CredentialsContainer'
since it was adding IDL tests for 'PasswordCredential' and
'FederatedCredential' anyway.

BUG=602980
Committed: https://crrev.com/be6cc81fc74f63c8e134525bbf15ea966c8403d5
Cr-Commit-Position: refs/heads/master@{#389475}

[Mike West, 2016-04-13 10:43:56]

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

[Mike West, 2016-04-13 10:44:59]

Philip, mind taking a look at this patch?

There are a few test failures introduced. The prototype bits look like they
might be legit, but the throwing bits are due to our version of `idlharness.js`
not understanding promises. What's the procedure for updating that?

[Mike West, 2016-04-13 10:46:20]

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl-expected.txt
(right):

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl-expected.txt:6:
FAIL CredentialsContainer interface: existence and properties of interface
prototype object assert_equals: class string of CredentialsContainer.prototype
expected "[object CredentialsContainerPrototype]" but got "[object
CredentialsContainer]"
Legit, I guess.

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl-expected.txt:8:
FAIL CredentialsContainer interface: operation get(CredentialRequestOptions)
assert_equals: property has wrong .length expected 1 but got 0
Ditto.

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl-expected.txt:14:
FAIL CredentialsContainer interface: calling get(CredentialRequestOptions) on
navigator.credentials with too few arguments must throw TypeError assert_throws:
Called with 0 arguments function "function () { [native code] }" did not throw
Our version of `idlharness.js` doesn't understand rejection.

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl-expected.txt:16:
FAIL CredentialsContainer interface: calling store(Credential) on
navigator.credentials with too few arguments must throw TypeError assert_throws:
Called with 0 arguments function "function () { [native code] }" did not throw
Our version of `idlharness.js` doesn't understand rejection.

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl-expected.txt:21:
FAIL PasswordCredential interface: existence and properties of interface
prototype object assert_equals: class string of PasswordCredential.prototype
expected "[object PasswordCredentialPrototype]" but got "[object
PasswordCredential]"
Legit, I guess.

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl-expected.txt:38:
FAIL FederatedCredential interface: existence and properties of interface
prototype object assert_equals: class string of FederatedCredential.prototype
expected "[object FederatedCredentialPrototype]" but got "[object
FederatedCredential]"
Legit, I guess.

[Mike West, 2016-04-18 20:31:40]

Ping? Other than the exposure tests that I need to correct, WDYT?

[philipj_slow, 2016-04-19 09:20:37]

lgtm % nits, sorry for breaking the slowness record.

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl-expected.txt
(right):

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl-expected.txt:6:
FAIL CredentialsContainer interface: existence and properties of interface
prototype object assert_equals: class string of CredentialsContainer.prototype
expected "[object CredentialsContainerPrototype]" but got "[object
CredentialsContainer]"
On 2016/04/13 10:46:20, Mike West (slow until 25th) wrote:
> Legit, I guess.

Do you know which bug it is? I try things like HTMLElement.prototype.toString()
but only get "[object Object]".

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl-expected.txt:8:
FAIL CredentialsContainer interface: operation get(CredentialRequestOptions)
assert_equals: property has wrong .length expected 1 but got 0
On 2016/04/13 10:46:20, Mike West (slow until 25th) wrote:
> Ditto.

Yeah, get() has an optional argument in CredentialsContainer.idl but not in the
spec. Spec changed?

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

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:8:
#include "bindings/core/v8/ExceptionMessages.h"
Added header but no other changes?

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/credentialmanager/FederatedCredential.idl
(right):

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/FederatedCredential.idl:5:
//
https://w3c.github.io/webappsec/specs/credentialmanagement/#federatedcredential
Update this URL too?

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

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.idl:5:
typedef (FormData or URLSearchParams) CredentialBodyType;
Spec link here?

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/credentialmanager/SiteBoundCredential.idl
(right):

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/SiteBoundCredential.idl:11:
readonly attribute DOMString name;
Spec says USVString for both of these. I shouldn't change the generated code I
think.

At first I wondered why name isn't just DOMString in the spec, but then I felt
like we've talked about this before and all of these things might end up
serialized in a URL or some context where it's nice to not have to deal with
bare surrogates. Right?

[Mike West, 2016-04-25 10:20:13]

Thanks!

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

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:8:
#include "bindings/core/v8/ExceptionMessages.h"
On 2016/04/19 at 09:20:36, philipj_slow wrote:
> Added header but no other changes?

Yeah. Dropped, thanks!

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/credentialmanager/SiteBoundCredential.idl
(right):

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/SiteBoundCredential.idl:11:
readonly attribute DOMString name;
On 2016/04/19 at 09:20:37, philipj_slow wrote:
> Spec says USVString for both of these. I shouldn't change the generated code I
think.
> 
> At first I wondered why name isn't just DOMString in the spec, but then I felt
like we've talked about this before and all of these things might end up
serialized in a URL or some context where it's nice to not have to deal with
bare surrogates. Right?

Hrm. No. `name` should be a DOMString. iconURL is a URL, and should be a
USVString. I'll update this file and the spec:
https://github.com/w3c/webappsec-credential-management/issues/14 (and Anne just
told me that I'm wrong. So, changing all of it to USVString. :) )

[philipj_slow, 2016-04-25 11:54:21]

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/credentialmanager/SiteBoundCredential.idl
(right):

https://codereview.chromium.org/1880393002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/SiteBoundCredential.idl:11:
readonly attribute DOMString name;
On 2016/04/25 10:20:13, Mike West (slow until 25th) wrote:
> On 2016/04/19 at 09:20:37, philipj_slow wrote:
> > Spec says USVString for both of these. I shouldn't change the generated code
I
> think.
> > 
> > At first I wondered why name isn't just DOMString in the spec, but then I
felt
> like we've talked about this before and all of these things might end up
> serialized in a URL or some context where it's nice to not have to deal with
> bare surrogates. Right?
> 
> Hrm. No. `name` should be a DOMString. iconURL is a URL, and should be a
> USVString. I'll update this file and the spec:
> https://github.com/w3c/webappsec-credential-management/issues/14 (and Anne
just
> told me that I'm wrong. So, changing all of it to USVString. :) )

OK, so USVString "for things that get utf-8 encoded" makes sense, not
specifically serialization for URLs.

https://codereview.chromium.org/1880393002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-basics.html
(right):

https://codereview.chromium.org/1880393002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-basics.html:21:
t.step_func(t.done.bind(t)));
Looks a bit odd, would t.step_func_done() suffice?

[Mike West, 2016-04-25 13:03:31]

https://codereview.chromium.org/1880393002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-basics.html
(right):

https://codereview.chromium.org/1880393002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-basics.html:21:
t.step_func(t.done.bind(t)));
On 2016/04/25 at 11:54:21, philipj_slow wrote:
> Looks a bit odd, would t.step_func_done() suffice?

Yes, it would. Thanks!

[Mike West, 2016-04-25 13:03:50]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-04-25 13:03:51]

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

[commit-bot: I haz the power, 2016-04-25 13:04:04]

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

[commit-bot: I haz the power, 2016-04-25 14:51:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-25 14:53:20]

Description was changed from

==========
CREDENTIAL: Implement 'SiteBoundCredential'.

This patch shims 'SiteBoundCredential' between 'Credential' and
'{Password,Federated}Credential'. I don't see any need to push this noop
up the stack, however, so it does not introduce a similar concept at
the platform layer, nor does //content need to learn about this
indirection.

As a drive-by, this patch adds some IDL tests for 'CredentialsContainer'
since it was adding IDL tests for 'PasswordCredential' and
'FederatedCredential' anyway.

BUG=602980
==========

to

==========
CREDENTIAL: Implement 'SiteBoundCredential'.

This patch shims 'SiteBoundCredential' between 'Credential' and
'{Password,Federated}Credential'. I don't see any need to push this noop
up the stack, however, so it does not introduce a similar concept at
the platform layer, nor does //content need to learn about this
indirection.

As a drive-by, this patch adds some IDL tests for 'CredentialsContainer'
since it was adding IDL tests for 'PasswordCredential' and
'FederatedCredential' anyway.

BUG=602980

Committed: https://crrev.com/be6cc81fc74f63c8e134525bbf15ea966c8403d5
Cr-Commit-Position: refs/heads/master@{#389475}
==========

[commit-bot: I haz the power, 2016-04-25 14:53:21]

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