[webkit-reviews] review denied: [Bug 119492] Implement SVGImage::hasSingleSecurityOrigin() : [Attachment 208340] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Timothy Hatcher <timothy at apple.com>'s
request for review:
Bug 119492: Implement SVGImage::hasSingleSecurityOrigin()
https://bugs.webkit.org/show_bug.cgi?id=119492

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=208340&action=review


Looks good. review- because of change log, but I also suggest doing this
without touching Element.h/cpp

> Source/WebCore/ChangeLog:15
> +	   (WebCore::SVGImage::hasSingleSecurityOrigin): Added. Look for links
and return
> +	   false if any link is found.

This no longer matches the patch. Need a revised change log.

> Source/WebCore/dom/Element.h:592
> +    bool isEmbeddedThroughSVGImage() const;

It’s great to have this be a function, but I don’t think it’s good as a member
function. Can we make it a free function in HTMLAnchorElement.h/cpp and have
the other files include HTMLAnchorElement.h? Or something similar in some SVG
source file? Also, maybe we could name it for its effect rather than for what
it checks, shouldDisableLinks or shouldProhibitLinks?

> Source/WebCore/svg/graphics/SVGImage.h:57
> +    // SVG images disallow external resources (including same origin).
> +    virtual bool hasSingleSecurityOrigin() const OVERRIDE { return true; }

The comment here is too oblique, I think. Maybe something more like this?

    // Because SVG image rendering disallows external resources and links,
these images effectively are restricted to a single security origin.