Bug 168616

Summary:

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

Product:

WebKit

Reporter:

Jiewen Tan <jiewen_tan>

Component:

WebCore Misc.

Assignee:

Jiewen Tan <jiewen_tan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, cdumez, commit-queue, dbates, jiewen_tan, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

167375, 169364

Attachments:

Description	Flags
Patch	dbates: review+
Patch for landing	none
Patch for landing	none
Archive of layout-test-results from ews113 for mac-elcapitan	none

Jiewen Tan

Reported 2017-02-20 15:37:05 PST

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.

Attachments
Patch (259.88 KB, patch) 2017-02-20 16:02 PST, Jiewen Tan	dbates: review+	Details Formatted Diff Diff
Patch for landing (260.71 KB, patch) 2017-02-23 14:42 PST, Jiewen Tan	no flags	Details Formatted Diff Diff
Patch for landing (260.71 KB, patch) 2017-02-23 15:01 PST, Jiewen Tan	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews113 for mac-elcapitan (1.63 MB, application/zip) 2017-02-23 16:06 PST, Build Bot	no flags	Details
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Jiewen Tan

Comment 1 2017-02-20 15:37:37 PST

<rdar://problem/30167149>

Jiewen Tan

Comment 2 2017-02-20 16:02:16 PST

Created attachment 302189 [details] Patch

Sam Weinig

Comment 3 2017-02-20 17:18:38 PST

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 4 2017-02-21 07:56:34 PST

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 5 2017-02-23 14:27:27 PST

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

Comment 6 2017-02-23 14:42:01 PST

Created attachment 302580 [details] Patch for landing

Jiewen Tan

Comment 7 2017-02-23 15:01:09 PST

Created attachment 302585 [details] Patch for landing

Build Bot

Comment 8 2017-02-23 16:06:27 PST

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

Comment 9 2017-02-23 16:06:32 PST

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 10 2017-02-27 11:03:06 PST

Comment on attachment 302585 [details] Patch for landing Clearing flags on attachment: 302585 Committed r213081: <http://trac.webkit.org/changeset/213081>

Note You need to log in before you can comment on or make changes to this bug.