Bug 13592 - parseMappedAttribute inconsistency
Summary: parseMappedAttribute inconsistency
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-05-05 03:01 PDT by Rob Buis
Modified: 2007-05-05 04:15 PDT (History)
0 users

See Also:


Attachments
First attempt (48.89 KB, patch)
2007-05-05 03:40 PDT, Rob Buis
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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:

http://www.openembedded.org/~zecke/coverage/webkit/mac/__WebCore__ksvg2__svg__SVGZoomAndPan.cpp.html

Also half the time here the value is not used:

http://www.openembedded.org/~zecke/coverage/webkit/mac/__WebCore__ksvg2__svg__SVGLinearGradientElement.cpp.html

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 :)
Cheers,

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

*SVGAnimateMotionElement::parseMappedAttribute

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

*SVGAnimateTransformElement::parseMappedAttribute
I don't think this gains anything.

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

*SVGCircleElement::parseMappedAttribute
*SVGClipPathElement::parseMappedAttribute
*SVGCursorElement::parseMappedAttribute
*SVGEllipseElement::parseMappedAttribute
I'd consider a local const& to hold the name, but otherwise looks good

*SVGExternalResourcesRequired::parseMappedAttribute
looks good

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

*SVGImageElement::parseMappedAttribute
*SVGLineElement::parseMappedAttribute
*SVGLinearGradientElement::parseMappedAttribute
*SVGMarkerElement::parseMappedAttribute
*SVGMaskElement::parseMappedAttribute
*SVGPathElement::parseMappedAttribute
local for the name again

*SVGPatternElement::parseMappedAttribute
local for name, local for value on those branches that access the value multiple times

*SVGRadialGradientElement::parseMappedAttribute
*SVGRectElement::parseMappedAttribute
local for the name

*SVGSVGElement::parseMappedAttribute
local for name, local for value those places it's used multiple times

*SVGScriptElement::parseMappedAttribute
ooh, good *and* fixed indenting

*SVGStopElement::parseMappedAttribute
*SVGTests::parseMappedAttribute
*SVGTextContentElement::parseMappedAttribute
*SVGTextPositioningElement::parseMappedAttribute
*SVGUseElement::parseMappedAttribute
local for name if it's used multiple imes

*SVGViewElement::parseMappedAttribute
good

*SVGZoomAndPan::parseMappedAttribute
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.