Drop SiteBoundCredential in favor of CredentialUserData

Drop SiteBoundCredential in favor of CredentialUserData

This change implements dropping the SiteBoundCredential interface in
favor of the CredentialUserData mixin. This change was introduced in the
Credential Manager API specification with commit
a43865bd8aa9842dbc8d8e688d5668c087677eac
(https://github.com/w3c/webappsec-credential-management/commit/a43865bd8aa9842dbc8d8e688d5668c087677eac).

Intent to Deprecate and Remove: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/X-5zjSOzLC4

R=vasilii@chromium.org,mkwst@chromium.org,jochen@chromium.org
BUG=714615
Review-Url: https://codereview.chromium.org/2832813002
Cr-Commit-Position: refs/heads/master@{#467305}
Committed: https://chromium.googlesource.com/chromium/src/+/c95e21cadda8ce43f68dc87d1c65d3ae39ab7850

[jdoerrie, 2017-04-20 12:49:40]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 12:49:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdoerrie, 2017-04-20 12:58:02]

Dear all, please review. This is my first adventure in this part of the
code-base, so please be gentle with me :)

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

https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialUserData.idl:9:
SecureContext,
I'm unsure about this, I basically just copied it from the spec:
https://w3c.github.io/webappsec-credential-management/#credentialuserdata

Given that |RuntimeEnabled=CredentialManager| is present in the other IDLs, this
should probably be kept, no?

[vasilii, 2017-04-20 14:21:31]

https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
(right):

https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt:4685:
getter name
Doesn't it have setters for the same.

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

https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialUserData.idl:9:
SecureContext,
On 2017/04/20 12:58:02, jdoerrie wrote:
> I'm unsure about this, I basically just copied it from the spec:
> https://w3c.github.io/webappsec-credential-management/#credentialuserdata
> 
> Given that |RuntimeEnabled=CredentialManager| is present in the other IDLs,
this
> should probably be kept, no?

For now yes. But I think we can drop |RuntimeEnabled=CredentialManager|
everywhere and in RuntimeEnabledFeatures.json5 too

[vasilii, 2017-04-20 14:24:46]

Also you can email blink-dev with an "Intent to deprecate and remove:
`SiteBoundCredential`". See https://www.chromium.org/blink/removing-features

[commit-bot: I haz the power, 2017-04-20 14:32:51]

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

[commit-bot: I haz the power, 2017-04-20 14:32:52]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-04-21 08:14:57]

Thanks for the comments so far, I will follow up with an email to blink-team
shortly.

https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
(right):

https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt:4685:
getter name
On 2017/04/20 14:21:31, vasilii wrote:
> Doesn't it have setters for the same.

Given that the iconURL and name are readonly attributes I don't think setters
are available.

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

https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialUserData.idl:9:
SecureContext,
On 2017/04/20 14:21:31, vasilii wrote:
> On 2017/04/20 12:58:02, jdoerrie wrote:
> > I'm unsure about this, I basically just copied it from the spec:
> > https://w3c.github.io/webappsec-credential-management/#credentialuserdata
> > 
> > Given that |RuntimeEnabled=CredentialManager| is present in the other IDLs,
> this
> > should probably be kept, no?
> 
> For now yes. But I think we can drop |RuntimeEnabled=CredentialManager|
> everywhere and in RuntimeEnabledFeatures.json5 too

Alright. Does |RuntimeEnabled=CredentialManager| actually imply |SecureContext|,
and if yes, should I drop |SecureContext|? According to the spec |SecureContext|
should be present on FederatedCredential and PasswordCredential as well, but
there I also only see |RuntimeEnabled=CredentialManager|.

[vasilii, 2017-04-21 08:42:59]

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

https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialUserData.idl:9:
SecureContext,
On 2017/04/21 08:14:57, jdoerrie wrote:
> On 2017/04/20 14:21:31, vasilii wrote:
> > On 2017/04/20 12:58:02, jdoerrie wrote:
> > > I'm unsure about this, I basically just copied it from the spec:
> > > https://w3c.github.io/webappsec-credential-management/#credentialuserdata
> > > 
> > > Given that |RuntimeEnabled=CredentialManager| is present in the other
IDLs,
> > this
> > > should probably be kept, no?
> > 
> > For now yes. But I think we can drop |RuntimeEnabled=CredentialManager|
> > everywhere and in RuntimeEnabledFeatures.json5 too
> 
> Alright. Does |RuntimeEnabled=CredentialManager| actually imply
|SecureContext|,
> and if yes, should I drop |SecureContext|? According to the spec
|SecureContext|
> should be present on FederatedCredential and PasswordCredential as well, but
> there I also only see |RuntimeEnabled=CredentialManager|.

RuntimeEnabled=CredentialManager is just a name of the feature which can be on
or off. It doesn't imply anything.
SecureContext should appear on the interfaces. Currently I can create a
PasswordCredential on a HTTP page. I think it was added to the spec recently.

[jochen (gone - plz use gerrit), 2017-04-21 08:58:13]

this requires an entry on chromestatus.com and an intent to deprecate email
(https://www.chromium.org/blink#TOC-Web-Platform-Changes:-Process)

I'd also recommend filing a new bug to track those changes.

[Mike West, 2017-04-24 07:34:08]

https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl.html
(right):

https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl.html:18:
USVString iconURL;
Either in this patch or a subsequent patch, we should upstream this test to the
shared test repository. You can do that by moving it to a new
`credential-management` directory under
`third_party/WebKit/LayoutTests/external/wpt/`.

https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl.html:21:
interface CredentialUserData {
Nit: Needs `NoInterfaceObject` + `SecureContext` attributes.

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

https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialUserData.idl:9:
SecureContext,
On 2017/04/20 at 14:21:31, vasilii wrote:
> On 2017/04/20 12:58:02, jdoerrie wrote:
> > I'm unsure about this, I basically just copied it from the spec:
> > https://w3c.github.io/webappsec-credential-management/#credentialuserdata
> > 
> > Given that |RuntimeEnabled=CredentialManager| is present in the other IDLs,
this
> > should probably be kept, no?
> 
> For now yes. But I think we can drop |RuntimeEnabled=CredentialManager|
everywhere and in RuntimeEnabledFeatures.json5 too

I agree. If y'all would like to remove that in a subsequent patch, I'm happy to
stamp it. :)

[jdoerrie, 2017-04-24 15:04:52]

Description was changed from

==========
Drop SiteBoundCredential in favor of CredentialUserData

This change implements dropping the SiteBoundCredential interface in
favor of the CredentialUserData mixin. This change was introduced in the
Credential Manager API specification with commit
a43865bd8aa9842dbc8d8e688d5668c087677eac
(https://github.com/w3c/webappsec-credential-management/commit/a43865bd8aa9842...).

R=vasilii@chromium.org,mkwst@chromium.org,jochen@chromium.org
BUG=400674
==========

to

==========
Drop SiteBoundCredential in favor of CredentialUserData

This change implements dropping the SiteBoundCredential interface in
favor of the CredentialUserData mixin. This change was introduced in the
Credential Manager API specification with commit
a43865bd8aa9842dbc8d8e688d5668c087677eac
(https://github.com/w3c/webappsec-credential-management/commit/a43865bd8aa9842...).

R=vasilii@chromium.org,mkwst@chromium.org,jochen@chromium.org
BUG=714615
==========

[jdoerrie, 2017-04-24 15:05:45]

Dear all, please take another look. I updated the referenced bug to be the OWP
launch bug, but maybe we should create another meta bug to track all these
related changes?

https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl.html
(right):

https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl.html:18:
USVString iconURL;
On 2017/04/24 07:34:08, Mike West wrote:
> Either in this patch or a subsequent patch, we should upstream this test to
the
> shared test repository. You can do that by moving it to a new
> `credential-management` directory under
> `third_party/WebKit/LayoutTests/external/wpt/`.

Done.

https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl.html:21:
interface CredentialUserData {
On 2017/04/24 07:34:08, Mike West wrote:
> Nit: Needs `NoInterfaceObject` + `SecureContext` attributes.

Done.

https://codereview.chromium.org/2832813002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/credential-management/idl-expected.txt
(right):

https://codereview.chromium.org/2832813002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/credential-management/idl-expected.txt:45:
PASS FederatedCredential interface: new FederatedCredential({ id: "id",
provider: "https://example.com", iconURL: "https://example.com/", name: "name"
}) must inherit property "name" with the proper type (2)
This test fails now after moving, it expects the following output:

	FAIL FederatedCredential interface: new FederatedCredential({ id: "id",
provider: "https://example.com", iconURL: "https://example.com/", name: "name"
}) must inherit property "name" with the proper type (2) assert_inherits:
property "name" not found in prototype chain

What am I missing?

[vasilii, 2017-04-24 16:46:26]

https://codereview.chromium.org/2832813002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/credential-management/idl-expected.txt
(right):

https://codereview.chromium.org/2832813002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/credential-management/idl-expected.txt:45:
PASS FederatedCredential interface: new FederatedCredential({ id: "id",
provider: "https://example.com", iconURL: "https://example.com/", name: "name"
}) must inherit property "name" with the proper type (2)
On 2017/04/24 15:05:45, jdoerrie wrote:
> This test fails now after moving, it expects the following output:
> 
> 	FAIL FederatedCredential interface: new FederatedCredential({ id: "id",
> provider: "https://example.com", iconURL: "https://example.com/", name: "name"
> }) must inherit property "name" with the proper type (2) assert_inherits:
> property "name" not found in prototype chain
> 
> What am I missing?

You applied "SecureContext" to CredentialUserData but didn't apply to anything
else. That can be a reason.

[Mike West, 2017-04-25 07:39:20]

https://codereview.chromium.org/2832813002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.html
(right):

https://codereview.chromium.org/2832813002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.html:1:
<!DOCTYPE html>
Rename this file to `idl.https.html`; otherwise it's loading
`http://web-platform.test:xxx`, which is not a secure context.

[Mike West, 2017-04-25 07:39:55]

Also, nit: Please link to the intent thread in the CL description. :)

[jdoerrie, 2017-04-25 09:28:49]

Description was changed from

==========
Drop SiteBoundCredential in favor of CredentialUserData

This change implements dropping the SiteBoundCredential interface in
favor of the CredentialUserData mixin. This change was introduced in the
Credential Manager API specification with commit
a43865bd8aa9842dbc8d8e688d5668c087677eac
(https://github.com/w3c/webappsec-credential-management/commit/a43865bd8aa9842...).

R=vasilii@chromium.org,mkwst@chromium.org,jochen@chromium.org
BUG=714615
==========

to

==========
Drop SiteBoundCredential in favor of CredentialUserData

This change implements dropping the SiteBoundCredential interface in
favor of the CredentialUserData mixin. This change was introduced in the
Credential Manager API specification with commit
a43865bd8aa9842dbc8d8e688d5668c087677eac
(https://github.com/w3c/webappsec-credential-management/commit/a43865bd8aa9842...).

Intent to Deprecate and Remove:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/X-5zjSOzLC4

R=vasilii@chromium.org,mkwst@chromium.org,jochen@chromium.org
BUG=714615
==========

[jdoerrie, 2017-04-25 09:41:20]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 09:41:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdoerrie, 2017-04-25 09:43:38]

Next Round :) I also added the intent thread to the description.

https://codereview.chromium.org/2832813002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/credential-management/idl-expected.txt
(right):

https://codereview.chromium.org/2832813002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/credential-management/idl-expected.txt:45:
PASS FederatedCredential interface: new FederatedCredential({ id: "id",
provider: "https://example.com", iconURL: "https://example.com/", name: "name"
}) must inherit property "name" with the proper type (2)
On 2017/04/24 16:46:26, vasilii wrote:
> On 2017/04/24 15:05:45, jdoerrie wrote:
> > This test fails now after moving, it expects the following output:
> > 
> > 	FAIL FederatedCredential interface: new FederatedCredential({ id: "id",
> > provider: "https://example.com", iconURL: "https://example.com/", name:
"name"
> > }) must inherit property "name" with the proper type (2) assert_inherits:
> > property "name" not found in prototype chain
> > 
> > What am I missing?
> 
> You applied "SecureContext" to CredentialUserData but didn't apply to anything
> else. That can be a reason.

I suspected the same, but adding "SecureContext" to the other interfaces did not
help. However, now the test passes after renaming the files.

https://codereview.chromium.org/2832813002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.html
(right):

https://codereview.chromium.org/2832813002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.html:1:
<!DOCTYPE html>
On 2017/04/25 07:39:20, Mike West wrote:
> Rename this file to `idl.https.html`; otherwise it's loading
> `http://web-platform.test:xxx`, which is not a secure context.

Done.

[Mike West, 2017-04-25 11:10:28]

Code change LGTM, but note that you'll still need jochen@, et al. to stamp the
changes to the //virtual/stable/webexposed test changes, and to likewise stamp
the intent thread.

Thanks!

[commit-bot: I haz the power, 2017-04-25 11:25:19]

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

[commit-bot: I haz the power, 2017-04-25 11:25:20]

Dry run: This issue passed the CQ dry run.

[vasilii, 2017-04-25 11:31:13]

lgtm

[jochen (gone - plz use gerrit), 2017-04-25 15:15:59]

(waiting for the intent thread to be resolved)

[jochen (gone - plz use gerrit), 2017-04-26 10:07:23]

lgtm

[jdoerrie, 2017-04-26 10:58:23]

The CQ bit was checked by jdoerrie@chromium.org

[commit-bot: I haz the power, 2017-04-26 10:58:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 12:35:03]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493204303176320,
"parent_rev": "bd27d5fb3a918c21d8231b32fa0ec929ec66afae", "commit_rev":
"c95e21cadda8ce43f68dc87d1c65d3ae39ab7850"}

[commit-bot: I haz the power, 2017-04-26 12:35:13]

Description was changed from

==========
Drop SiteBoundCredential in favor of CredentialUserData

This change implements dropping the SiteBoundCredential interface in
favor of the CredentialUserData mixin. This change was introduced in the
Credential Manager API specification with commit
a43865bd8aa9842dbc8d8e688d5668c087677eac
(https://github.com/w3c/webappsec-credential-management/commit/a43865bd8aa9842...).

Intent to Deprecate and Remove:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/X-5zjSOzLC4

R=vasilii@chromium.org,mkwst@chromium.org,jochen@chromium.org
BUG=714615
==========

to

==========
Drop SiteBoundCredential in favor of CredentialUserData

This change implements dropping the SiteBoundCredential interface in
favor of the CredentialUserData mixin. This change was introduced in the
Credential Manager API specification with commit
a43865bd8aa9842dbc8d8e688d5668c087677eac
(https://github.com/w3c/webappsec-credential-management/commit/a43865bd8aa9842...).

Intent to Deprecate and Remove:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/X-5zjSOzLC4

R=vasilii@chromium.org,mkwst@chromium.org,jochen@chromium.org
BUG=714615

Review-Url: https://codereview.chromium.org/2832813002
Cr-Commit-Position: refs/heads/master@{#467305}
Committed:
https://chromium.googlesource.com/chromium/src/+/c95e21cadda8ce43f68dc87d1c65...
==========

[commit-bot: I haz the power, 2017-04-26 12:35:14]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/c95e21cadda8ce43f68dc87d1c65...