[webkit-reviews] review granted: [Bug 98683] SVGResources should use HashSet<AtomicString> instead of HashSet<AtomicStringImpl*> : [Attachment 167759] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Florin Malita
<fmalita at chromium.org>'s request for review:
Bug 98683: SVGResources should use HashSet<AtomicString> instead of
HashSet<AtomicStringImpl*>
https://bugs.webkit.org/show_bug.cgi?id=98683

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

------- Additional Comments from Darin Adler <darin at apple.com>
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?