Summary: | CSP 1.1: Stub out SecurityPolicyViolationEvent interface. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mike West <mkwst> | ||||||||
Component: | WebKit Misc. | Assignee: | Mike West <mkwst> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, esprehn+autocc, gyuyoung.kim, mkwst+watchlist, ojan.autocc, rakuco, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 85558 | ||||||||||
Attachments: |
|
Description
Mike West
2013-03-19 04:19:44 PDT
Created attachment 193819 [details]
Patch
Comment on attachment 193819 [details]
Patch
Clearing r? until I've done a few rounds with the EWS bots, because I hate adding files. I'll be shocked if this compiles everywhere. :)
Comment on attachment 193819 [details]
Patch
Hrm. I am pleasantly shocked. :)
r?ing. Would one of you fine folks on CC mind taking a look?
Comment on attachment 193819 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193819&action=review This looks fine, but I'd like to look at it one more time to make sure the web-visible changes are properly hidden when CSP.next is disabled at runtime. > Source/WebCore/dom/Document.idl:344 > + [NotEnumerable, Conditional=CSP_NEXT] attribute EventListener onsecuritypolicyviolation; This attribute needs to be enabled at runtime or else it will be visible to the web. > Source/WebCore/dom/SecurityPolicyViolationEvent.h:36 > + }; No need for a ; here. > Source/WebCore/dom/SecurityPolicyViolationEvent.h:55 > + static PassRefPtr<SecurityPolicyViolationEvent> create(const String& documentURI, const String& referrer, const String& blockedURI, const String& violatedDirective, const String& effectiveDirective, const String& originalPolicy, const String& sourceURL, int lineNumber) I'd skip this one too. Even C++ code is better off using SecurityPolicyViolationEventInit. > Source/WebCore/dom/SecurityPolicyViolationEvent.h:65 > + void initSecurityPolicyViolationEvent(const AtomicString& eventType, bool canBubble, bool cancelable, const String& documentURI, const String& referrer, const String& blockedURI, const String& violatedDirective, const String& effectiveDirective, const String& originalPolicy, const String& sourceURL, int lineNumber) We should skip this function. We only implement these sorts of functions for legacy events. > Source/WebCore/dom/SecurityPolicyViolationEvent.idl:29 > + void initSecurityPolicyViolationEvent(in [Optional=DefaultIsUndefined] DOMString type, We should remove this function from the API. We're implementing new events in the DOM4 Events style. > Source/WebCore/page/DOMWindow.idl:573 > + [Conditional=CSP_NEXT] attribute SecurityPolicyViolationEventConstructor SecurityPolicyViolationEvent; This attribute should also be enabled at runtime. Created attachment 193867 [details]
Patch
(In reply to comment #5) > Created an attachment (id=193867) [details] > Patch Thanks, Adam. The runtime flag is important, thanks for remembering it. I've taken another pass; would you mind taking another look? Comment on attachment 193867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193867&action=review > Source/WebCore/dom/SecurityPolicyViolationEvent.h:69 > + virtual const AtomicString& interfaceName() const { return eventNames().interfaceForSecurityPolicyViolationEvent; } Does this whole header need a CSP_NEXT ifdef? > Source/WebCore/dom/SecurityPolicyViolationEvent.h:76 > + SecurityPolicyViolationEvent(const String& documentURI, const String& referrer, const String& blockedURI, const String& violatedDirective, const String& effectiveDirective, const String& originalPolicy, const String& sourceURL, int lineNumber) This function now has no callers. Created attachment 193919 [details]
Patch
(In reply to comment #8) > Created an attachment (id=193919) [details] > Patch Addressed the bits of feedback from the last round, and I'll throw this to the EWS bots one more time, since there was a merge conflict in the xcode project. Assuming that the bots are happy, I'll CQ it in the morning. Thanks! Comment on attachment 193919 [details]
Patch
Bots are happy. CQing. :)
Comment on attachment 193919 [details] Patch Clearing flags on attachment: 193919 Committed r146305: <http://trac.webkit.org/changeset/146305> All reviewed patches have been landed. Closing bug. |