Bug 13592

Summary: parseMappedAttribute inconsistency
Product: WebKit Reporter: Rob Buis <rwlbuis>
Component: SVGAssignee: Nobody <webkit-unassigned>
Severity: Normal    
Priority: P2    
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
Description Flags
First attempt oliver: review+

Description Rob Buis 2007-05-05 03:01:14 PDT
parseMappedAttribute a lot of the time sets a local var to get the attribute value. In a lot of cases, as shown by the coverage results, this value is not used at all. This is very clear here:


Also half the time here the value is not used:


Even though a compiler may be able to optimize the local var away, I think it is better to make it clear and be consistent with html code here too. Patch coming up.
Comment 1 Rob Buis 2007-05-05 03:40:38 PDT
Created attachment 14350 [details]
First  attempt

Should be straightforward patch. Would be nice if somebody could benchmark what the change brings us :)

Comment 2 Oliver Hunt 2007-05-05 03:58:34 PDT
Comment on attachment 14350 [details]
First  attempt


I'd lift add const String& value = attr-value() after the first name check.  It seem icky to query multiple times.

I don't think this gains anything.

in those cases where you access attr->value() multiple times i think you should just declare a var.  eg. the attributeTypeAttr case.

I'd consider a local const& to hold the name, but otherwise looks good

looks good

local for the attribute name, and maybe use locals in those branches that query the value multiple times.

local for the name again

local for name, local for value on those branches that access the value multiple times

local for the name

local for name, local for value those places it's used multiple times

ooh, good *and* fixed indenting

local for name if it's used multiple imes


local for value when it's used multiple times
Comment 3 Oliver Hunt 2007-05-05 04:04:54 PDT
Comment on attachment 14350 [details]
First  attempt

Ignore last comment, seems like this patch matches the style elsewhere
Comment 4 Rob Buis 2007-05-05 04:15:24 PDT
Landed in r21272.