Bug 98683

Summary: SVGResources should use HashSet<AtomicString> instead of HashSet<AtomicStringImpl*>
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: SVGAssignee: Florin Malita <fmalita>
Status: RESOLVED FIXED    
Severity: Normal CC: fmalita, krit, pdr, schenney, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Eric Seidel (no email) 2012-10-08 13:37:36 PDT
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.
Comment 1 Florin Malita 2012-10-09 08:19:33 PDT
Created attachment 167759 [details]
Patch
Comment 2 Darin Adler 2012-10-09 10:05:20 PDT
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 3 Eric Seidel (no email) 2012-10-09 10:25:46 PDT
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?
Comment 4 Florin Malita 2012-10-09 10:58:25 PDT
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.
Comment 5 Florin Malita 2012-10-09 10:58:48 PDT
Created attachment 167786 [details]
Patch for landing
Comment 6 WebKit Review Bot 2012-10-09 11:36:13 PDT
Comment on attachment 167786 [details]
Patch for landing

Clearing flags on attachment: 167786

Committed r130780: <http://trac.webkit.org/changeset/130780>
Comment 7 WebKit Review Bot 2012-10-09 11:36:16 PDT
All reviewed patches have been landed.  Closing bug.