Oops, I've missed nsSVGAngle2.cpp and nsSVGAngle2.h

(In reply to comment #4) > Created an attachment (id=282674) [details] > with all the files This seems to have unrelated boolean and length changes included.

I made the same changes to nsSVGLength2 as I did to nsSVGAngle. I can break out the length changes if you wish. The boolean changes are basically bug 397620 which I plan to land once the tree stops being red.

Comment on attachment 283170 [details] [diff] [review] address additional review comments > /* void setOrientToAuto (); */ > NS_IMETHODIMP nsSVGMarkerElement::SetOrientToAuto() > { >- mOrientType.SetBaseValue(SVG_MARKER_ORIENT_AUTO, this, PR_TRUE); >+ nsSVGMarkerElementBase::SetAttr(kNameSpaceID_None, >+ nsGkAtoms::orient, >+ NS_LITERAL_STRING("auto"), >+ PR_TRUE); > return NS_OK; > } Since you're calling the base class SetAttr, this will result in mOrientType not being set appropriately, won't it?

(In reply to comment #10) > Since you're calling the base class SetAttr, this will result in mOrientType > not being set appropriately, won't it? > Actually it does work :-) The SetAttr above matches this in nsGenericElement.h nsresult SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName, const nsAString& aValue, PRBool aNotify) { return SetAttr(aNameSpaceID, aName, nsnull, aValue, aNotify); } Which then calls the nsSVGMarkerElement SetAttr which sets the mOrientType. I could have called the nsSVGMarkerElement SetAttr directly at the expense of an extra null argument I guess. Would you like me to do that?

(In reply to comment #11) > Actually it does work :-) > > The SetAttr above matches this in nsGenericElement.h > > nsresult SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName, > const nsAString& aValue, PRBool aNotify) > { > return SetAttr(aNameSpaceID, aName, nsnull, aValue, aNotify); > } > > Which then calls the nsSVGMarkerElement SetAttr which sets the mOrientType. Ah-ha. > I could have called the nsSVGMarkerElement SetAttr directly at the expense of > an extra null argument I guess. Would you like me to do that? I think that make things easier to understand for future maintainers of the code. One other thing I noticed: static nsresult ToDOMSVGAngle(nsIDOMSVGAngle **aResult); This seems a bit odd since the other ToDOM* methods are conversions, while this is a new object. Maybe go back to the NS_NewDOMSVGAngle style function? r=tor with those two things addressed.

I also moved the DOMSVGAngle class to the nsSVGAngle.cpp file as it is only accessed via its base class.

Looks like I can check it in tomorrow as I have a=mconnor for red check-in via irc.

checked in.

Backed out per IRC request of end drivers due to suspicion of talos pageload regression.

Summary of changes: nsSVGElement.h - new function to store and retrieve whether an element has any attributes of a given type All Elements - register what type of attributes they have in their constructor nsSVGElement.cpp - check for whether an element has any attributes of a given type before calling the GetxxxInfo virtual functions Should be more performant due to decrease in number of virtual method calls. Thanks to Roc for the idea.

I reran what I hope are the talos svg pages. I can't see a significant difference in timings between the current trunk and the patch that was backed out but the new patch does seem to be slightly faster although the standard deviation in timings is so large it may just be wishful thinking.

I wasn't aware we had SVG usage in Talos Tp...

i.e., what Talos measurement was affected and why do we think this affected it?

Why store mAttributeTypes in a variable? Why not just make GetAttributeTypes virtual? One virtual call is still a lot better than 6. But I seriously doubt this could have caused any Tp regression. The Talos Tp pageset simply does not use SVG.

I think we should just reland the original patch and look closer.

I assumed it was these pages in some way http://bonsai.mozilla.org/rview.cgi?dir=mozilla/testing/performance/talos/page_load_test/svg&cvsroot=/cvsroot but only because they were the only pages I could find. None of them have <marker> elements in so they could only be affected by the SetAttr change going from 5 virtual function calls to 6.

(In reply to comment #23) > I think we should just reland the original patch and look closer. > I can do that tomorrow if that's OK.

The Talos page set is all copyright encumbered copies of real Web pages so we don't have it in CVS, unfortunately. I just checked and verified that there is no SVG usage in it.

Comment on attachment 283894 [details] [diff] [review] speed up attempt I'll reland the original patch tomorrow then.

checked in (again).

I don't think at a glance this patch fixed any visible bugs, but someone more knowledgeable should set the in-testsuite to the appropriate value.

This patch did fix some bugs. The crashes which are its dependencies and also refreshing the display properly when the DOM function SetAngleToAuto is called.