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.
Created attachment 167759 [details] Patch
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?
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?
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.
Created attachment 167786 [details] Patch for landing
Comment on attachment 167786 [details] Patch for landing Clearing flags on attachment: 167786 Committed r130780: <http://trac.webkit.org/changeset/130780>
All reviewed patches have been landed. Closing bug.