[webkit-reviews] review denied: [Bug 123114] [CSS Shapes] CORS-enabled fetch for shape image values : [Attachment 214977] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <akling at apple.com> has denied Hans Muller
<giles_joplin at yahoo.com>'s request for review:
Bug 123114: [CSS Shapes] CORS-enabled fetch for shape image values
https://bugs.webkit.org/show_bug.cgi?id=123114

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

------- Additional Comments from Andreas Kling <akling at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=214977&action=review


I think this approach makes sense. I would say r=me, but there are some things
I think we can do better:

> Source/WebCore/rendering/shapes/ShapeInfo.cpp:44
> +bool ShapeInfo<RenderType>::checkImageOrigin(const RenderType* renderer,
const StyleImage* styleImage)

This function assumes that 'renderer' and 'styleImage' are both non-null, so we
should pass them by reference instead.
It only uses the renderer to dig out the Document, so we could just pass the
Document from the call sites and avoid templatizing this function.

> Source/WebCore/rendering/shapes/ShapeInfo.cpp:46
> +    ASSERT(styleImage && styleImage->isCachedImage() &&
styleImage->cachedImage() && styleImage->cachedImage()->image());

Hm. Instead of all this asserting, could we just make the function take a
CachedImage& directly?
You're using the StyleImage to grab at the CachedImage inside.

> Source/WebCore/rendering/shapes/ShapeInfo.cpp:49
> +    bool originClean =
styleImage->cachedImage()->isOriginClean(renderer->document().securityOrigin())
;
> +    if (!originClean) {

Why not just return right away?
if
(styleImage->cachedImage()->isOriginClean(renderer->document().securityOrigin()
))
    return true;
...
return false;

> Source/WebCore/loader/cache/CachedImage.cpp:577
> +bool CachedImage::isOriginClean(SecurityOrigin* securityOrigin)

It's ugly that this takes the SecurityOrigin by pointer, since it could crash
if nullptr is passed.
Although this is a larger problem with SecurityContexts in WebCore, and not
something we need to fix in this patch.