Closed Bug 397749 Opened 13 years ago Closed 13 years ago

New style nsSVGAngle

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: longsonr, Assigned: longsonr)

References

Details

Attachments

(1 file, 8 obsolete files)

 
OS: Windows 95 → All
Attached patch patch (obsolete) — Splinter Review
Oops, I've missed nsSVGAngle2.cpp and nsSVGAngle2.h
Attached patch completed patch (obsolete) — Splinter Review
Attachment #282562 - Attachment is obsolete: true
Attachment #282673 - Flags: review?(tor)
Attached patch with all the files (obsolete) — Splinter Review
Attachment #282673 - Attachment is obsolete: true
Attachment #282674 - Flags: review?(tor)
Attachment #282673 - Flags: review?(tor)
Flags: blocking1.9?
(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.
Attachment #282674 - Attachment is obsolete: true
Attachment #282711 - Flags: review?(tor)
Attachment #282674 - Flags: review?(tor)
Blocks: 397704
Attached patch address review comments (obsolete) — Splinter Review
Attachment #282711 - Attachment is obsolete: true
Attachment #283031 - Flags: review?(tor)
Attachment #282711 - Flags: review?(tor)
Attachment #283031 - Flags: review?(tor)
Attachment #283031 - Attachment is obsolete: true
Attachment #283170 - Flags: review?(tor)
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.
Attachment #283170 - Attachment is obsolete: true
Attachment #283324 - Flags: review?(tor)
Attachment #283170 - Flags: review?(tor)
Attachment #283324 - Attachment is obsolete: true
Attachment #283341 - Flags: review?(tor)
Attachment #283324 - Flags: review?(tor)
Attachment #283341 - Flags: review?(tor) → review+
Attachment #283341 - Flags: superreview?(roc)
Attachment #283341 - Flags: superreview?(roc)
Attachment #283341 - Flags: superreview+
Attachment #283341 - Flags: approval1.9+
Looks like I can check it in tomorrow as I have a=mconnor for red check-in via irc.
checked in.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Backed out per IRC request of end drivers due to suspicion of talos pageload regression.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch speed up attempt (obsolete) — Splinter Review
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.
Attachment #283341 - Attachment is obsolete: true
Attachment #283894 - Flags: review?(tor)
Blocks: 398903
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.
Attachment #283894 - Flags: review?(tor) → review+
Attachment #283894 - Flags: superreview?(roc)
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.
Attachment #283894 - Flags: superreview?(roc)
Attachment #283341 - Attachment is obsolete: false
Attachment #283894 - Attachment is obsolete: true
checked in (again).
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
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.
Flags: in-testsuite?
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.
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.