Implement CredentialsContainer::create

Implement CredentialsContainer::create

This change adds `CredentialsContainer::create` to the Credential Management API
allowing for asynchronous creation of Credential objects.

Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/uEGfd5NmjbY/IUWw7lC6EAAJ

BUG=722242
Review-Url: https://codereview.chromium.org/2884703002
Cr-Commit-Position: refs/heads/master@{#473197}
Committed: https://chromium.googlesource.com/chromium/src/+/f8a3ddf0546af55ee59924af78370c2b8dd57fd4

[jdoerrie, 2017-05-15 17:01:21]

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

[commit-bot: I haz the power, 2017-05-15 17:01:48]

Dry run: 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-05-15 17:34:03]

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

[commit-bot: I haz the power, 2017-05-15 17:34:04]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jdoerrie, 2017-05-16 06:27:33]

Description was changed from

==========
Implement CredentialsContainer::create

BUG=722242
==========

to

==========
Implement CredentialsContainer::create

This change adds `CredentialsContainer::create` to the Credential Management API
allowing for asynchronous creation of Credential objects.

Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/uEGfd5NmjbY/IUWw7lC6...

BUG=722242
==========

[jdoerrie, 2017-05-16 06:28:19]

Description was changed from

==========
Implement CredentialsContainer::create

This change adds `CredentialsContainer::create` to the Credential Management API
allowing for asynchronous creation of Credential objects.

Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/uEGfd5NmjbY/IUWw7lC6...

BUG=722242
==========

to

==========
Implement CredentialsContainer::create

This change adds `CredentialsContainer::create` to the Credential Management API
allowing for asynchronous creation of Credential objects.

Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/uEGfd5NmjbY/IUWw7lC6...

BUG=722242
==========

[jdoerrie, 2017-05-16 06:34:26]

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

[jdoerrie, 2017-05-16 06:34:27]

Hi Mike and Jochen, please review. I went with Jochen's idea to guard this
feature with RuntimeEnabled while waiting for the 3 LGTMs. I think a few tests
are still missing, but I wanted to hear your initial feedback on this change.

https://codereview.chromium.org/2884703002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/virtual/service-worker-navigation-preload-disabled/webexposed/global-interface-listing-expected.txt
(right):

https://codereview.chromium.org/2884703002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/virtual/service-worker-navigation-preload-disabled/webexposed/global-interface-listing-expected.txt:1016:
method create
I'm not sure why the LayoutTests currently fail, if I understood
https://www.chromium.org/blink/runtime-enabled-features correctly, an
"experimental" feature should always be enabled in tests.

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

https://codereview.chromium.org/2884703002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:224:
"navigator.credentials", "create");
Here I was the most unsure about, I just looked for other places where
exception_states where created from script_states and adapted that. Please let
me know if this is appropriate.

[Mike West, 2017-05-16 09:21:00]

A few comments, looking mostly good.

The `//platform/*/virtual/stable` tests are the ones that look like they're
failing on the bots: that seems reasonable, since you've only enabled the
feature for experimental cases, not "stable" use. :)

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

https://codereview.chromium.org/2884703002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialCreationOptions.idl:5:
//
https://w3c.github.io/webappsec-credential-management/#dictdef-credentialcrea...
Can you link to the two partial definitions?

https://w3c.github.io/webappsec-credential-management/#dictdef-credentialcrea...
and
https://w3c.github.io/webappsec-credential-management/#dictdef-credentialcrea...

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

https://codereview.chromium.org/2884703002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:215:
if (!(options.hasPassword() ^ options.hasFederated())) {
XOR works today, as we only have two entities. When we add 'publicKey', we'll
need to make this more complicated. Perhaps add a TODO to that effect?

https://codereview.chromium.org/2884703002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:218:
"Exactly one of password or federated must be provided."));
I'd rephrase this to something like, "Only 'password' and 'federated' credential
types are currently supported.", as we'll be adding 'publicKey' at some point in
the nearish future.

https://codereview.chromium.org/2884703002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:223:
ExceptionState::kConstructionContext,
Nit: This isn't actually a constructor, it's just a method invocation that
happens to create an object. Please use `kExecutionContext` instead. :)

https://codereview.chromium.org/2884703002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:224:
"navigator.credentials", "create");
On 2017/05/16 at 06:34:27, jdoerrie wrote:
> Here I was the most unsure about, I just looked for other places where
exception_states where created from script_states and adapted that. Please let
me know if this is appropriate.

Nit: Please replace "navigator.credentials" with "CredentialsContainer".

Other than those two nits, this looks fine to me. :)

https://codereview.chromium.org/2884703002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:230:
resolver->Resolve(PasswordCredential::Create(
When you write tests, can you verify that passing in something other than a
`PasswordCredentialData` or `HTMLFormElement` returns a reasonable exception
(`TypeError`, for instance)?

https://codereview.chromium.org/2884703002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:235:
FederatedCredential::Create(options.federated(), exception_state));
When you write tests, can you verify that passing in something other than a
`FederatedCredentialData` does the right thing?

[jdoerrie, 2017-05-16 13:23:15]

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

[commit-bot: I haz the power, 2017-05-16 13:23:37]

Dry run: CQ is trying da patch.

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

[jdoerrie, 2017-05-16 13:29:56]

Thanks, Mike! Please have another look :) Thank you for explaining the "stable"
tests, that makes a lot of sense. I should have read the path more carefully. I
decided to drop the RuntimeEnabled, because there are already two LGTMs on the
I2S. If Jochen is fine with it too we can directly ship this. Since I depend on
Jochen to review this anyway I am not adding any blockers this way.

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

https://codereview.chromium.org/2884703002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialCreationOptions.idl:5:
//
https://w3c.github.io/webappsec-credential-management/#dictdef-credentialcrea...
On 2017/05/16 09:20:59, Mike West wrote:
> Can you link to the two partial definitions?
> 
>
https://w3c.github.io/webappsec-credential-management/#dictdef-credentialcrea...
> and
>
https://w3c.github.io/webappsec-credential-management/#dictdef-credentialcrea...

Done.

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

https://codereview.chromium.org/2884703002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:215:
if (!(options.hasPassword() ^ options.hasFederated())) {
On 2017/05/16 09:21:00, Mike West wrote:
> XOR works today, as we only have two entities. When we add 'publicKey', we'll
> need to make this more complicated. Perhaps add a TODO to that effect?

Done.

https://codereview.chromium.org/2884703002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:218:
"Exactly one of password or federated must be provided."));
On 2017/05/16 09:21:00, Mike West wrote:
> I'd rephrase this to something like, "Only 'password' and 'federated'
credential
> types are currently supported.", as we'll be adding 'publicKey' at some point
in
> the nearish future.

Done.

https://codereview.chromium.org/2884703002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:223:
ExceptionState::kConstructionContext,
On 2017/05/16 09:21:00, Mike West wrote:
> Nit: This isn't actually a constructor, it's just a method invocation that
> happens to create an object. Please use `kExecutionContext` instead. :)

Done.

https://codereview.chromium.org/2884703002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:224:
"navigator.credentials", "create");
On 2017/05/16 09:21:00, Mike West wrote:
> On 2017/05/16 at 06:34:27, jdoerrie wrote:
> > Here I was the most unsure about, I just looked for other places where
> exception_states where created from script_states and adapted that. Please let
> me know if this is appropriate.
> 
> Nit: Please replace "navigator.credentials" with "CredentialsContainer".
> 
> Other than those two nits, this looks fine to me. :)

Done.

https://codereview.chromium.org/2884703002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:230:
resolver->Resolve(PasswordCredential::Create(
On 2017/05/16 09:21:00, Mike West wrote:
> When you write tests, can you verify that passing in something other than a
> `PasswordCredentialData` or `HTMLFormElement` returns a reasonable exception
> (`TypeError`, for instance)?

Done.

https://codereview.chromium.org/2884703002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:235:
FederatedCredential::Create(options.federated(), exception_state));
On 2017/05/16 09:20:59, Mike West wrote:
> When you write tests, can you verify that passing in something other than a
> `FederatedCredentialData` does the right thing?

Done.

[commit-bot: I haz the power, 2017-05-16 15:36:59]

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

[commit-bot: I haz the power, 2017-05-16 15:37:00]

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

[Mike West, 2017-05-17 06:57:57]

Looking good. Just a few comments on the tests.

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

https://codereview.chromium.org/2884703002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-basics.html:7:
(function() {
You don't need this wrapper, here or elsewhere.

https://codereview.chromium.org/2884703002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-basics.html:17:
navigator.credentials.create({}));
Please add a test for `{password: { /* invalid password stuff */ }, federated: {
/* invalid federated stuff */ }}`, and for `{ invalid: whatever }`.

https://codereview.chromium.org/2884703002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-basics.html:35:
assert_equals(credential.passwordName, 'password');
Since this test doesn't rely on the internal mocks that we added for `get()`, I
think it's a good idea to split this test into one which verifies the specced
properties in `//external/wpt/credential-management/`, and one in
`//http/tests/credentialmanager/` which verifies the properties we're keeping
around just for legacy compatibility. The former can be safely upstreamed, but
we wouldn't want to put those requirements on other vendors.

[jdoerrie, 2017-05-17 08:53:23]

Thanks Mike! Just a quick follow-up comment.

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

https://codereview.chromium.org/2884703002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-basics.html:35:
assert_equals(credential.passwordName, 'password');
On 2017/05/17 06:57:57, Mike West wrote:
> Since this test doesn't rely on the internal mocks that we added for `get()`,
I
> think it's a good idea to split this test into one which verifies the specced
> properties in `//external/wpt/credential-management/`, and one in
> `//http/tests/credentialmanager/` which verifies the properties we're keeping
> around just for legacy compatibility. The former can be safely upstreamed, but
> we wouldn't want to put those requirements on other vendors.

Sound good :) Just to make sure I understand you correctly:
You basically want me to move the vast majority of this file to
//external/wpt/credential-management/, while only keeping

promise_test(function(t) {
  var credential_data = {
    id: 'id',
    password: 'pencil',
  };

  return navigator.credentials.create({password: credential_data})
      .then(function(credential) {
        assert_equals(credential.idName, 'username');
        assert_equals(credential.passwordName, 'password');
        assert_equals(credential.additionalData, null);
      });
}, "navigator.credentials.create() with valid PasswordCredentialData");

and the corresponding formData test in this file, right?

[jdoerrie, 2017-05-17 10:58:05]

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

[commit-bot: I haz the power, 2017-05-17 10:58:28]

Dry run: CQ is trying da patch.

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

[jdoerrie, 2017-05-17 11:29:09]

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

https://codereview.chromium.org/2884703002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-basics.html:7:
(function() {
On 2017/05/17 06:57:57, Mike West wrote:
> You don't need this wrapper, here or elsewhere.

Done.

https://codereview.chromium.org/2884703002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-basics.html:17:
navigator.credentials.create({}));
On 2017/05/17 06:57:57, Mike West wrote:
> Please add a test for `{password: { /* invalid password stuff */ }, federated:
{
> /* invalid federated stuff */ }}`, and for `{ invalid: whatever }`.

Done.

[Mike West, 2017-05-17 11:39:41]

LGTM with a tiny nit. Thanks!

https://codereview.chromium.org/2884703002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/credential-management/credentialscontainer-create-basics.https.html
(right):

https://codereview.chromium.org/2884703002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/credential-management/credentialscontainer-create-basics.https.html:27:
assert_equals(credential.type, 'password');
Can you add something like `// TODO(jdoerrie): Add checks for 'password'.`?

[jdoerrie, 2017-05-17 11:46:44]

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

[commit-bot: I haz the power, 2017-05-17 11:46:47]

Dry run: CQ is trying da patch.

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

[jdoerrie, 2017-05-17 11:48:26]

Thanks, Mike!

A gentle ping for jochen@ to review the I2S and to rubber-stamp this CL.

[commit-bot: I haz the power, 2017-05-17 14:21:55]

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

[commit-bot: I haz the power, 2017-05-17 14:21:56]

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

[haraken, 2017-05-17 14:51:49]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2017-05-17 14:51:50]

https://codereview.chromium.org/2884703002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp
(right):

https://codereview.chromium.org/2884703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:227:
"CredentialsContainer", "create");

Can we add [RaisesException=Constructor] to the IDL file and remove this?

[jdoerrie, 2017-05-19 09:47:13]

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

[commit-bot: I haz the power, 2017-05-19 09:47:53]

Dry run: CQ is trying da patch.

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

[jdoerrie, 2017-05-19 10:04:50]

https://codereview.chromium.org/2884703002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp
(right):

https://codereview.chromium.org/2884703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:227:
"CredentialsContainer", "create");
On 2017/05/17 14:51:50, haraken wrote:
> 
> Can we add [RaisesException=Constructor] to the IDL file and remove this?

Done, given that this is not really a constructor I only added [RaisesException]
to the method. Please let me know if this is inappropriate here.

[commit-bot: I haz the power, 2017-05-19 11:56:21]

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

[commit-bot: I haz the power, 2017-05-19 11:56:22]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jochen (gone - plz use gerrit), 2017-05-19 13:55:17]

API lgtm, please wait for Kentaro to approve

[haraken, 2017-05-19 14:03:10]

LGTM

[jdoerrie, 2017-05-19 14:26:55]

The CQ bit was checked by jdoerrie@chromium.org

[jdoerrie, 2017-05-19 14:26:56]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2884703002/#ps80001
(title: "RaisesException")

[commit-bot: I haz the power, 2017-05-19 14:27:26]

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-05-19 15:30:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495204015977780,
"parent_rev": "96aad180385b7e538b525cfda971289498617877", "commit_rev":
"f8a3ddf0546af55ee59924af78370c2b8dd57fd4"}

[commit-bot: I haz the power, 2017-05-19 15:30:36]

Description was changed from

==========
Implement CredentialsContainer::create

This change adds `CredentialsContainer::create` to the Credential Management API
allowing for asynchronous creation of Credential objects.

Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/uEGfd5NmjbY/IUWw7lC6...

BUG=722242
==========

to

==========
Implement CredentialsContainer::create

This change adds `CredentialsContainer::create` to the Credential Management API
allowing for asynchronous creation of Credential objects.

Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/uEGfd5NmjbY/IUWw7lC6...

BUG=722242

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

[commit-bot: I haz the power, 2017-05-19 15:30:37]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/f8a3ddf0546af55ee59924af7837...