[webkit-reviews] review denied: [Bug 103652] CSP 1.1: Make the CSP_NEXT flag runtime enabled. : [Attachment 176793] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Mike West <mkwst at chromium.org>'s
request for review:
Bug 103652: CSP 1.1: Make the CSP_NEXT flag runtime enabled.
https://bugs.webkit.org/show_bug.cgi?id=103652

Attachment 176793: Patch
https://bugs.webkit.org/attachment.cgi?id=176793&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=176793&action=review


> Source/WebCore/page/ContentSecurityPolicy.cpp:1721
> +#if ENABLE(CSP_NEXT)
> +    if (m_scriptExecutionContext->isDocument()) {
> +	   Document* document =
static_cast<Document*>(m_scriptExecutionContext);
> +	   return document->settings() &&
document->settings()->securityPolicyEnabled();
> +    }
> +#endif

Rather than using Settings, we should just call securityPolicyEnabled() from
RuntimeEnabledFeatures.h.

I know it's kind of confusing to have two different mechanisms for enabling
features.  We use Settings if we want to vary the feature on a per-Page basis. 
We us RuntimeEnabledFeatures.h when we want to control the feature globally. 
For features that have DOM APIs, we need to control them globally, which is why
we use RuntimeEnabledFeatures.

> Source/WebCore/page/Settings.h:249
> +#if ENABLE(CSP_NEXT)
> +	   void setSecurityPolicyEnabled(bool enabled) {
m_securityPolicyEnabled = enabled; }
> +	   bool securityPolicyEnabled() const { return m_securityPolicyEnabled;
}
> +#else
> +	   void setSecurityPolicyEnabled(bool) { }
> +	   bool securityPolicyEnabled() const { return false; }
> +#endif

This isn't needed.  (If it were needed, you could have put it in Settings.in to
get this code autogenerated.)

> Source/WebKit/chromium/src/WebSettingsImpl.h:138
> +    virtual void setSecurityPolicyEnabled(bool);

I would pick a more specific name.  This API makes it sound like we're
disabling WebKit's security policy.  Perhaps
setExperimentalContentSecurityPolicyFeaturesEnabled() ?  Maybe that's too
verbose...