RESOLVED FIXED 168616
[CredentialManagement] Add IDL definitions for Credential, SiteBoundCredential, and PasswordCredential
https://bugs.webkit.org/show_bug.cgi?id=168616
Summary [CredentialManagement] Add IDL definitions for Credential, SiteBoundCredentia...
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+
Patch for landing (260.71 KB, patch)
2017-02-23 14:42 PST, Jiewen Tan
no flags
Patch for landing (260.71 KB, patch)
2017-02-23 15:01 PST, Jiewen Tan
no flags
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
Jiewen Tan
Comment 1 2017-02-20 15:37:37 PST
Jiewen Tan
Comment 2 2017-02-20 16:02:16 PST
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.