[webkit-reviews] review denied: [Bug 119492] Implement SVGImage::hasSingleSecurityOrigin() : [Attachment 208358] 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 208358: Patch
https://bugs.webkit.org/attachment.cgi?id=208358&action=review

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


review- because I still would prefer we not touch Element.h

> Source/WebCore/dom/Element.h:802
> +bool isElementContainedInSVGImage(const Element*);

This is the wrong header file. I suggest putting it into SVGImage.h or
HTMLAnchorElement.h since it’s about SVG images and links.

One of the points of having something be a free function instead of a member
function is that we can encapsulate ideas better and not pollute the core
classes with all the more complex concepts.

It would be better to have this function name talk about link policy, rather
than the SVG Image check. That would make the reason for the check at all the
call sites clearer. This function would then be the one place to add comments
explaining the policy. Otherwise, at each call site we’d raise the question,
“Why is an SVG image special here?” And then have to add comments everywhere.

If you don’t agree, and want to keep something more like the current name, I
would use isInSVGImage; I don’t think the words “element contained” are needed.


> Source/WebCore/svg/graphics/SVGImage.cpp:38
> +#include "NodeTraversal.h"

Why?