[webkit-reviews] review granted: [Bug 91919] Implement the plugin-types Content Security Policy directive. : [Attachment 156932] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Mike West <mkwst at chromium.org>'s
request for review:
Bug 91919: Implement the plugin-types Content Security Policy directive.
https://bugs.webkit.org/show_bug.cgi?id=91919

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

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


> Source/WebCore/loader/SubframeLoader.cpp:133
> +	   if (!document()->contentSecurityPolicy()->allowObjectFromSource(url)

> +	       ||
!m_frame->document()->contentSecurityPolicy()->allowPluginType(mimeType,
declaredMimeType, url)) {

Why do these two lines use a different way to find the document?

> Source/WebCore/loader/SubframeLoader.cpp:301
> +	       ||
!element->document()->contentSecurityPolicy()->allowPluginType("application/x-j
ava-applet", "application/x-java-applet", codeBaseURL))

Can you make this string into a named constant?

> Source/WebCore/page/ContentSecurityPolicy.cpp:600
> +    bool checkPluginType(const String& type, const String& typeAttr) const;

typeAttr -> typeAttribute (please use complete words in variable names)

> Source/WebCore/page/ContentSecurityPolicy.cpp:628
> +    HashSet<String> m_pluginTypes;
> +    String m_pluginTypesDirective;

Should we create a PluginTypesDirective object to hold these pieces of state?

> Source/WebCore/page/ContentSecurityPolicy.cpp:729
> +	   message = makeString("`plugin-types` Content Security Policy
directive is empty; all plugins will be blocked.\n");

You don't need to call makeString if you've just got a single string.  You can
just use the assignment operator.

Also, please don't use ` in console messages.  We prefer '.

> Source/WebCore/page/ContentSecurityPolicy.cpp:1088
> +    // `plugin-types ____;` OR `plugin-types;`

No ` please.

> Source/WebCore/page/ContentSecurityPolicy.cpp:1089
> +    if (value.isNull()) {

isNull -> isEmpty ?

> Source/WebCore/page/ContentSecurityPolicy.cpp:1297
> +template<bool (CSPDirectiveList::*allowed)(const String&, const String&,
const KURL&, ContentSecurityPolicy::ReportingStatus) const>

I'm not sure its worth having this whole template given that we're probably
only going to have one function with this signature.