WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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+
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.
Top of Page
Format For Printing
XML
Clone This Bug