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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 25 06:37:52 PDT 2013


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.


More information about the webkit-reviews mailing list