Bug 168616 – [CredentialManagement] Add IDL definitions for Credential, SiteBoundCredential, and PasswordCredential

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jiewen Tan]

Add IDL definitions and verify PasswordCredential object can be instantiated.
A. Add IDL definition for Credential: <https://w3c.github.io/webappsec-credential-management/#interfaces-credential-types-credential>.
B. Add IDL definition for SiteBoundCredential: <https://w3c.github.io/webappsec-credential-management/#interfaces-credential-types-siteboundcredential>.
C. Add IDL definition for PasswordCredential: <https://w3c.github.io/webappsec-credential-management/#interfaces-credential-types-passwordcredential>.
D. Write tests for the interfaces Credential, SiteBoundCredential, and PasswordCredential.

[Jiewen Tan]

<rdar://problem/30167149>

[Jiewen Tan]

Created attachment 302189 [details]
Patch

[Sam Weinig]

Comment on attachment 302189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302189&action=review

> Source/WebCore/Modules/credentials/PasswordCredential.cpp:34
> +    // This is a dummy constructor for now.

This comment doesn't add much.

> Source/WebCore/Modules/credentials/PasswordCredential.h:28
> +#include "DomFormData.h"

This should bee DOMFormData.h

> Source/WebCore/Modules/credentials/PasswordCredential.h:30
> +#include "URLSearchParams.h"

If you added an out of line destructor, you could probably #include less.

> Source/WebCore/Modules/credentials/PasswordCredential.h:31
> +#include <wtf/RefCounted.h>

I don't think you need this.

> Source/WebCore/Modules/credentials/PasswordCredential.idl:29
> +    Constructor(PasswordCredentialData data),

The spec has an additional constructor that takes an HTMLFormElement. Why leave that out?

> Source/WebCore/Modules/credentials/SiteBoundCredential.cpp:35
> +    , m_origin(URL())

This is unnecessary.

> Source/WebCore/Modules/credentials/SiteBoundCredential.h:50
> +    URL m_origin;

Hard to tell, since it is unused, but should this be a SecurityOrigin/SecurityOriginData instead of a URL if it just needed for the origin?

[Daniel Bates]

Comment on attachment 302189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302189&action=review

Please take care to fix the Windows and GTK builds before landing. We will need to limit these interfaces to a secure context. Towards this we will have to introduce such a concept. We can do this is in a subsequent patch. Please address Sam Weinig's remarks. In particular, we should look to store a SecurityOrigin in the [[origin]] slot in a SiteBoundCredential object instead of using a URL.

> Source/WebCore/Modules/credentials/BasicCredential.cpp:45
> +        return "password";

We should use ASCIILiteral() here to avoid making a copying of the string literal.

> Source/WebCore/Modules/credentials/BasicCredential.cpp:47
> +        return "federated";

Ditto.

[Jiewen Tan]

Comment on attachment 302189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302189&action=review

Thanks Sam and Dan for reviewing my patch.

>> Source/WebCore/Modules/credentials/BasicCredential.cpp:45
>> +        return "password";
> 
> We should use ASCIILiteral() here to avoid making a copying of the string literal.

Fixed.

>> Source/WebCore/Modules/credentials/BasicCredential.cpp:47
>> +        return "federated";
> 
> Ditto.

Fixed.

>> Source/WebCore/Modules/credentials/PasswordCredential.cpp:34
>> +    // This is a dummy constructor for now.
> 
> This comment doesn't add much.

Removed.

>> Source/WebCore/Modules/credentials/PasswordCredential.h:28
>> +#include "DomFormData.h"
> 
> This should bee DOMFormData.h

Fixed.

>> Source/WebCore/Modules/credentials/PasswordCredential.h:30
>> +#include "URLSearchParams.h"
> 
> If you added an out of line destructor, you could probably #include less.

Doesn't seems working.

>> Source/WebCore/Modules/credentials/PasswordCredential.h:31
>> +#include <wtf/RefCounted.h>
> 
> I don't think you need this.

Removed.

>> Source/WebCore/Modules/credentials/PasswordCredential.idl:29
>> +    Constructor(PasswordCredentialData data),
> 
> The spec has an additional constructor that takes an HTMLFormElement. Why leave that out?

Added a dummy one as well.

>> Source/WebCore/Modules/credentials/SiteBoundCredential.cpp:35
>> +    , m_origin(URL())
> 
> This is unnecessary.

Removed.

>> Source/WebCore/Modules/credentials/SiteBoundCredential.h:50
>> +    URL m_origin;
> 
> Hard to tell, since it is unused, but should this be a SecurityOrigin/SecurityOriginData instead of a URL if it just needed for the origin?

Changed to SecurityOrigin.

[Jiewen Tan]

Created attachment 302580 [details]
Patch for landing

[Jiewen Tan]

Created attachment 302585 [details]
Patch for landing

[Build Bot]

Comment on attachment 302585 [details]
Patch for landing

Attachment 302585 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3181564

New failing tests:
media/modern-media-controls/volume-down-support/volume-down-support.html

[Build Bot]

Created attachment 302600 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[WebKit Commit Bot]

Comment on attachment 302585 [details]
Patch for landing

Clearing flags on attachment: 302585

Committed r213081: <http://trac.webkit.org/changeset/213081>