[webkit-reviews] review denied: [Bug 117610] [CSS Shapes] limit shape image values to same origin : [Attachment 205200] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Hans Muller
<giles_joplin at yahoo.com>'s request for review:
Bug 117610: [CSS Shapes] limit shape image values to same origin
https://bugs.webkit.org/show_bug.cgi?id=117610

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=205200&action=review


> Source/WebCore/css/StyleResolver.cpp:4106
> +    DEFINE_STATIC_LOCAL(ResourceLoaderOptions, options, (SendCallbacks,
SniffContent, BufferData, AllowStoredCredentials, AskClientForAllCredentials,
DoSecurityCheck,  RestrictToSameOrigin));

Two spaces before RestrictToSameOrigin.

I don't think that it's great to duplicate most default options, with one
change. Can you just start with a default, and update only the security policy?


This code doesn't look hot enough to warrant reusing the options. I'd just
avoid DEFINE_STATIC_LOCAL here if that makes us make the code more complicated
than it needs to be.

> Source/WebCore/css/StyleResolver.cpp:4112
> +    StyleImage* image = shapeValue->image();
> +    if (!image)
> +	   return;
> +
> +    StylePendingImage* pendingImage =
static_cast<StylePendingImage*>(image);

Before being split out in to a separate function, this code didn't attempt
casting without a isPendingImage() check.

I don't know much about this code, but since this change is not explained in
ChangeLog, I'm going to assume that this is a bug.

> Source/WebCore/css/StyleResolver.cpp:-4180
> -	       if (m_state.style()->shapeInside() &&
m_state.style()->shapeInside()->image() &&
m_state.style()->shapeInside()->image()->isPendingImage())

Also, we used to null check m_state.style()->shapeInside(), but do not any
more.

> Source/WebCore/loader/ResourceLoaderOptions.h:82
> +    ResourceLoaderOptions(
> +	   SendCallbackPolicy sendLoadCallbacks,
> +	   ContentSniffingPolicy sniffContent,
> +	   DataBufferingPolicy dataBufferingPolicy,
> +	   StoredCredentials allowCredentials,
> +	   ClientCredentialPolicy credentialPolicy,
> +	   SecurityCheckPolicy securityCheck,
> +	   RequestOriginPolicy requestOriginPolicy)

This is not a usual WebKit style. Normally, we just let the line run longer.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:340
> -	   // These types of resources can be loaded from any origin.
> +	   // These types of resources can be loaded from any origin by
default.
>	   // FIXME: Are we sure about CachedResource::FontResource?
> +	   if (options.requestOriginPolicy == RestrictToSameOrigin &&
!m_document->securityOrigin()->canRequest(url)) {

You updated the comment, but it's still misplaced and confusing.

There is no logic about resource types here that needs to be explained, we are
just adhering to options.requestOriginPolicy.