Bug 103652

Created attachment 176793 [details] Patch

Cobbled together from reading other folks' patches, but I think this looks right. Let's see what the bots make of it. :) WDYT, Adam?

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Comment on attachment 176793 [details] Patch 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...

Here's a recent patch that makes a feature runtime named: https://bugs.webkit.org/show_bug.cgi?id=103669

s/named/enabled/

Created attachment 176873 [details] Patch

Ok, thanks for taking the time to explain. The RuntimeEnabledFeatures/Settings distinction makes sense, and in this case it ends up being less code! Hooray! (In reply to comment #4) > > 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... Eh. We've got more than 80 columns here, we might as well use them. :)

Looks great.

Comment on attachment 176873 [details] Patch Clearing flags on attachment: 176873 Committed r136305: <http://trac.webkit.org/changeset/136305>

All reviewed patches have been landed. Closing bug.