Bug 118504 – Introduce isSVGFontElement() and isSVGImageElement(), use them

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Gyuyoung Kim]

Let's use isFoo() to enhance readibility in svg element classes along with html's.

[Gyuyoung Kim]

Created attachment 206317 [details]
Patch

[Benjamin Poulain]

Comment on attachment 206317 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206317&action=review

> Source/WebCore/svg/SVGFontElement.h:95
> +    ASSERT_WITH_SECURITY_IMPLICATION(!node || isSVGFontElement(node));

How did that compile with isSVGFontElement() declared underneath?

> Source/WebCore/svg/SVGImageElement.h:94
> +inline bool isSVGImageElement(const Element* node)
> +{
> +    return node->hasTagName(SVGNames::imageTag);
> +}

Why? Element is a node. This should not be needed.

[Gyuyoung Kim]

Created attachment 206365 [details]
 Patch for landing

[Gyuyoung Kim]

(In reply to comment #2)
> > Source/WebCore/svg/SVGImageElement.h:94
> > +inline bool isSVGImageElement(const Element* node)
> > +{
> > +    return node->hasTagName(SVGNames::imageTag);
> > +}
> 
> Why? Element is a node. This should not be needed.

I confused that because some html classes added it. It looks we don't need to keep it at there as well.

http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLAnchorElement.h#L162

[WebKit Commit Bot]

Comment on attachment 206365 [details]
 Patch for landing

Clearing flags on attachment: 206365

Committed r152524: <http://trac.webkit.org/changeset/152524>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Comment on attachment 206365 [details]
 Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=206365&action=review

> Source/WebCore/svg/SVGImageElement.cpp:67
> -    ASSERT(hasTagName(SVGNames::imageTag));
> +    ASSERT(isSVGImageElement(SVGNames::imageTag));

It broke the debug builds:

/home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/Source/WebCore/svg/SVGImageElement.cpp: In constructor 'WebCore::SVGImageElement::SVGImageElement(const WebCore::QualifiedName&, WebCore::Document*)':
[1;31m/home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/Source/WebCore/svg/SVGImageElement.cpp:67:44: error: cannot convert 'const WebCore::QualifiedName' to 'const WebCore::Node*' for argument '1' to 'bool WebCore::isSVGImageElement(const WebCore::Node*)'[0m


Could you fix it, please?

[Csaba Osztrogonác]

Fix already landed in http://trac.webkit.org/changeset/152526 ... but without any reference to this bug/commit and without any comment in this bug report :-/

[Gyuyoung Kim]

(In reply to comment #8)
> Fix already landed in http://trac.webkit.org/changeset/152526 ... but without any reference to this bug/commit and without any comment in this bug report :-/

Sorry for inconvenience about it. I will let him know how to write ChangeLog for unreviewed patch.

[Csaba Osztrogonác]

(In reply to comment #9)
> (In reply to comment #8)
> > Fix already landed in http://trac.webkit.org/changeset/152526 ... but without any reference to this bug/commit and without any comment in this bug report :-/
> 
> Sorry for inconvenience about it. I will let him know how to write ChangeLog for unreviewed patch.

Thanks. Not a big problem, but a good changelog and a comment 
in the original bug helps a lot to avoid unnecessary requests :)