Bug 98683 – SVGResources should use HashSet<AtomicString> instead of HashSet<AtomicStringImpl*>

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

SVGResources should use HashSet<AtomicString> instead of HashSet<AtomicStringImpl*>

They do basically the same thing, and the former is much more common (and less code).  It's also safe, on the off-chance that we're using AtomicStrings which might otherwise go away.

[Florin Malita]

Created attachment 167759 [details]
Patch

[Darin Adler]

Comment on attachment 167759 [details]
Patch

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

> Source/WebCore/rendering/svg/SVGResources.cpp:205
> +    AtomicString tagName = element->tagQName().localName();

This adds a bit of reference count churn. If we know that the element won’t be deleted while we’re working on it, we could use const AtomicString& as the type instead.

> Source/WebCore/rendering/svg/SVGResources.cpp:206
> +    if (tagName.isEmpty())

I think we only need an isNull check here. An isEmpty check is both more expensive than a null check, and also may not be correct since the old code did a null check. I wonder if we have test coverage for the empty string case?

[Eric Seidel (no email)]

Comment on attachment 167759 [details]
Patch

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

>> Source/WebCore/rendering/svg/SVGResources.cpp:205
>> +    AtomicString tagName = element->tagQName().localName();
> 
> This adds a bit of reference count churn. If we know that the element won’t be deleted while we’re working on it, we could use const AtomicString& as the type instead.

Also, I think you can just call element->localName() directly and don't have to go through the qname?

[Florin Malita]

Thanks for reviewing, updated.

(In reply to comment #2)
> > Source/WebCore/rendering/svg/SVGResources.cpp:205
> > +    AtomicString tagName = element->tagQName().localName();
> 
> This adds a bit of reference count churn. If we know that the element won’t be deleted while we’re working on it, we could use const AtomicString& as the type instead.

Done.


> > Source/WebCore/rendering/svg/SVGResources.cpp:206
> > +    if (tagName.isEmpty())
> 
> I think we only need an isNull check here. An isEmpty check is both more expensive than a null check, and also may not be correct since the old code did a null check. I wonder if we have test coverage for the empty string case?

Good point, should avoid changing behavior here. Even if it's possible to get an empty tag name here, it shouldn't have any impact as it will just skip through the remaining conditionals and return false. Updated.

(In reply to comment #3)
> Also, I think you can just call element->localName() directly and don't have to go through the qname?

Done.

[Florin Malita]

Created attachment 167786 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 167786 [details]
Patch for landing

Clearing flags on attachment: 167786

Committed r130780: <http://trac.webkit.org/changeset/130780>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.