332162 - Rewrite nsSVGLength

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Comment 1

[tor]

Attachment: work in progress - single element converted (circle)

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks very exciting!

+  mCx.mCtxType = nsSVGUtils::X;
+  mCx.mKind = CX;

Make these constructor parameters.

+  *aCx = mCx.ToDOMAnimatedLength(this);
   NS_IF_ADDREF(*aCx);
   return NS_OK;

How about packaging this up into a method GetDOMAnimatedLength, so you can just write
  return mCx.GetDOMAnimatedLength(this, aCx);
?

We're going to want to cache some of these as properties of the content, but we'll be able to do that in nsSVGLength(2) in a later patch without changing this API. I do hope you replace your usage of nsISVGContent with direct use of nsSVGElement before checkin.

I think we could save a lot of code if we put mCx, mCy, mR into an array of length 3 right in the element. Then we could declare a static array of atoms* {nsGkAtoms::cx, nsGkAtoms::cy, nsGkAtoms::r} and have a helper nsSVGUtils::ParseAttributeLengths which takes an array of atoms and an array of lengths. DidChange can use the same array (perhaps via another nsSVGUtils helper, but perhaps not because it will become very simple).

* Actually declaring an array of atoms is a pain because they're not constant values, but you can cheat by declaraing the array as {&nsGkAtoms::cx, &nsGkAtoms::cy, &nsGkAtoms::r} and doing an extra dereference in the code.

+nsSVGCoordCtx *
+nsSVGElement::GetCtxByType(PRUint16 aCtxType)
+{

Calling this for every GetBaseValue/GetAnimValue doesn't seem great. How about offering versions that take an nsSVGCoordCtxProvider* as an additional parameter? Then callers that need to do multiple GetBaseValue/GetAnimValue calls on the same content object can just get the coordctxprovider once. 

We should also deCOMtaminate nsSVGCoordCtxProvider to return non-addrefed pointers. Perhaps that should happen before this lands.

+nsSVGCircleElement::DidChange(PRUint16 aKind)

Make it 8 bits.

+  float mAnimatedValue;
+  float mBaseValue;
+  PRUint16 mSpecifiedUnitType;
+  PRInt8 mKind; // element specified tracking for attribute
+  PRUint8 mCtxType:2; // X, Y or Unspecified
+  PRPackedBool mIsAnimated:1;

These should not be public.

I assume the logic in nsSVGLength2::SetBaseValueString is copied from elsewhere, and the original can go away after this is complete?

> isValidUnitType

These lowercase helper names aren't really kosher...

+  char *str = ToNewCString(aValueAsString);

Can we use nsAString string iterators to avoid ToNewCString and the dynamic allocation?

Comment 3

[tor]

(In reply to comment #2)
> +  *aCx = mCx.ToDOMAnimatedLength(this);
>    NS_IF_ADDREF(*aCx);
>    return NS_OK;
> 
> How about packaging this up into a method GetDOMAnimatedLength, so you can just
> write
>   return mCx.GetDOMAnimatedLength(this, aCx);
> ?

How about just calling AddRef() in the tearoff constructors?

> I think we could save a lot of code if we put mCx, mCy, mR into an array of
> length 3 right in the element. Then we could declare a static array of atoms*
> {nsGkAtoms::cx, nsGkAtoms::cy, nsGkAtoms::r} and have a helper
> nsSVGUtils::ParseAttributeLengths which takes an array of atoms and an array of
> lengths. DidChange can use the same array (perhaps via another nsSVGUtils
> helper, but perhaps not because it will become very simple).

Version using arrays attached - not sure it adds to the clarity of the code, and might make things messier when we do similar conversions for the other SVGAnimated* types.

Stopped applying comments at this point to get feedback on arrays.

Comment 4

[tor]

Attachment: version with arrays

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #3)
> How about just calling AddRef() in the tearoff constructors?

I think, since these only get called on behalf of XPCOM users, that they might as well have XPCOM signature. Saves "return NS_OK;" per use :-)

> Version using arrays attached - not sure it adds to the clarity of the code,
> and might make things messier when we do similar conversions for the other
> SVGAnimated* types.

mAttributeNames should be called gAttributeNames. And mAttributeNames and mAttributes should be called mLengthAttributeNames and mLengthAttributes.

nsSVGCircleElement::DidChange should assert that the kind is in range.

Probably better to just null-terminate gAttributeNames and remove the length parameter from nsSVGUtils::ParseAttributeLength.

I guess nsSVGCircleElement::DidChange should be DidChangeLength.

+  mAttributes[CX] = nsSVGLength2(nsSVGUtils::X, CX);
+  mAttributes[CY] = nsSVGLength2(nsSVGUtils::Y, CY);
+  mAttributes[R]  = nsSVGLength2(nsSVGUtils::XY, R);

These should be in the constructor, not Init. Ideally they would be variable initializers but I guess C++ can't do that. Might be more obviously efficient if you wrote mAttributes[CX].Init(nsSVGUtils::X, CX) etc.

When we convert other types, we might not want to use arrays for those, because I suspect we'll be mostly dealing with just one instance of each type per element. I don't see any need to smush all the different kinds of attributes into one single array.

Comment 6

[tor]

> We should also deCOMtaminate nsSVGCoordCtxProvider to return non-addrefed
> pointers. Perhaps that should happen before this lands.

nsSVGLength (old implementation) keeps a pointer to a nsSVGCoordCtx, so deCOMtaminating would need to wait until we tranfer everything over to the new implementation.

> I assume the logic in nsSVGLength2::SetBaseValueString is copied from
> elsewhere, and the original can go away after this is complete?

It's copied from the old nsSVGLength.  Are you referring to other code in the tree besides this?

> +  char *str = ToNewCString(aValueAsString);
> 
> Can we use nsAString string iterators to avoid ToNewCString and the dynamic
> allocation?

Calling ToFloat on a nsAString looks like it will do an allocation, so the savings seem small.

Let me know what you think of the attached version.  I'll get started transitioning over other uses of svg lengths.

Comment 7

[tor]

Attachment: work in progress

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  NS_ASSERTION(aKind >= 0 && aKind <= 3, "aKind out of range");

< 3

I still think DidChange should be DidChangeLength.

I like this :-)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #6)
> nsSVGLength (old implementation) keeps a pointer to a nsSVGCoordCtx, so
> deCOMtaminating would need to wait until we tranfer everything over to the new
> implementation.

OK

> > I assume the logic in nsSVGLength2::SetBaseValueString is copied from
> > elsewhere, and the original can go away after this is complete?
> 
> It's copied from the old nsSVGLength.  Are you referring to other code in the
> tree besides this?

No, that's fine

> > +  char *str = ToNewCString(aValueAsString);
> > 
> > Can we use nsAString string iterators to avoid ToNewCString and the dynamic
> > allocation?
> 
> Calling ToFloat on a nsAString looks like it will do an allocation, so the
> savings seem small.

OK, but we've got to fix this one way or another. This needs a followup bug.

Comment 10

[tor]

Attachment: add rect element

Switched over <rect> and added unset logic (test with rx/ry on rect).

Since the new length no longer caches the nsSVGCoordCtx and I think that things that would cause ParentChangeChanged() to be called on an element would cause a reframe and thus redraw of relevant portions, removed that method from rect/circle.

Comment 11

[tor]

Attachment: wip - basic graphics elements + <svg>

Comment 12

[Robert Longson [:longsonr]]

> 
> //----------------------------------------------------------------------
> // nsISVGContent methods
> 
> nsresult
> nsSVGSVGElement::UnsetAttr(PRInt32 aNamespaceID, nsIAtom* aName,
>                            PRBool aNotify)
> {
>   if (aNamespaceID == kNameSpaceID_None) {
>     if (aName == nsGkAtoms::x)    
>       mLengthAttributes[X].Init(nsSVGUtils::X, X);
>     else if (aName == nsGkAtoms::y)
>       mLengthAttributes[Y].Init(nsSVGUtils::Y, Y);
>     else if (aName == nsGkAtoms::width)
>       mLengthAttributes[WIDTH].Init(nsSVGUtils::X, WIDTH);
>     else if (aName == nsGkAtoms::height)
>       mLengthAttributes[HEIGHT].Init(nsSVGUtils::Y, HEIGHT);
>     else if (aName == nsGkAtoms::viewBox && mCoordCtx) {
>       nsCOMPtr<nsIDOMSVGRect> vb;
>       mViewBox->GetAnimVal(getter_AddRefs(vb));
>       vb->SetX(0);
>       vb->SetY(0);
>       vb->SetWidth(mLengthAttributes[WIDTH].GetAnimValue(mCoordCtx));
>       vb->SetHeight(mLengthAttributes[HEIGHT].GetAnimValue(mCoordCtx));
>     }
>   }

I think this should be 
mLengthAttributes[WIDTH].Init(nsSVGUtils::X, WIDTH, 100,
                              nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE);
Similarly for HEIGHT.

It looks like once you do nsSVGMarkerElement you should be able to get rid of the nsSVGViewBox class.

Comment 13

[tor]

(In reply to comment #12)
> I think this should be 
> mLengthAttributes[WIDTH].Init(nsSVGUtils::X, WIDTH, 100,
>                               nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE);
> Similarly for HEIGHT.

Thanks for catching that.

> It looks like once you do nsSVGMarkerElement you should be able to get rid of
> the nsSVGViewBox class.

That's the plan.

Comment 14

[tor]

Attachment: wip - add gradients, image, use

Comment 15

[tor]

Attachment: wip - all but marker

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It seems to me that there should be a way to save the duplicated code in the element constructors that gets repeated in UnsetAttr.

Perhaps the thing to do is to extend gLengthAttributeNames to include the initial setup for each nsSVGLength2. Then you can have one nsSVGUtils helper method to initialize the lengths and another nsSVGUtils method to handle UnsetAttr. Something like

class nsSVGLength2 {
  ...
  struct Info {
    nsIAtom** mName;
    PRUint8   mCtxType;
    PRUint16  mDefaultUnitType;
    float     mDefaultValue;
  };

...
void
nsSVGUtils::InitLengths(nsSVGLength2* aLengths, PRUint32 aLengthsCount,
                        nsSVGLength2::Info* aDefaults);
PRBool
nsSVGUtils::UnsetLengthAttr(nsSVGLength2* aLengths, PRUint32 aLengthsCount,
                            nsSVGLength2::Info* aDefaults, nsIAtom* aAttr);

Perhaps ParseAttributeLength and these methods should be static methods of nsSVGLength2.

Hmm ... another idea ... what if we had

struct nsSVGElement::LengthInfo {
  nsSVGLength2*       mLengths;
  nsSVGLength2::Info* mLengthInfo;
  PRUint32            mLengthCount;
};

and in nsSVGElement,

  virtual nsSVGElement::LengthInfo GetLengthInfo();

Then I think you could have a single implementation of DidChangeLength, UnsetAttr and ParseAttribute in nsSVGElement that most subclasses would not need to override. Also you wouldn't need any code in the subclass constructors to initialize the lengths. That would be pretty cool.

+  NS_IMETHOD GetX(nsSVGLength2 **aX);
+  NS_IMETHOD GetY(nsSVGLength2 **aY);
+  NS_IMETHOD GetHeight(nsSVGLength2 **aHeight);
+  NS_IMETHOD GetWidth(nsSVGLength2 **aWidth);

Can't these be nonvirtual and just return nsSVGLength2* directly?

+  nsSVGRectElement *rect = NS_STATIC_CAST(nsSVGRectElement*, mContent);
+  nsCOMPtr<nsSVGCoordCtxProvider> ctx =
+    nsSVGUtils::GetCoordContextProvider(mContent);
+
+  x = rect->mLengthAttributes[nsSVGRectElement::X].GetAnimValue(ctx);
+  y = rect->mLengthAttributes[nsSVGRectElement::Y].GetAnimValue(ctx);
+  width = rect->mLengthAttributes[nsSVGRectElement::WIDTH].GetAnimValue(ctx);
+  height = rect->mLengthAttributes[nsSVGRectElement::HEIGHT].GetAnimValue(ctx);
+  rx = rect->mLengthAttributes[nsSVGRectElement::RX].GetAnimValue(ctx);
+  ry = rect->mLengthAttributes[nsSVGRectElement::RY].GetAnimValue(ctx);

For the code blocks like this, it might be better (pardon me for being repetitive) to call a helper function in nsSVGElement:

  void GetAnimatedLengthValues(...);

Make it a real varargs function that takes a series of float* parameters
terminated by null, so you can call it like this:
  nsSVGRectElement *rect = NS_STATIC_CAST(nsSVGRectElement*, mContent);
  rect->GetAnimatedLengthValues(&x, &y, &width, &height, &rx, &ry, nsnull);

Mmmmm.

Comment 17

[Jonathan Watt [:jwatt]]

You've used the code from nsSVGLength as the basis for nsSVGLength2, so maybe you should carry the contributor(s) list across too.

Comment 18

[Jonathan Watt [:jwatt]]

In fact, once you've completely got rid of the current nsSVGLength, would it be possible to grep for "nsSVGLength2" and replace it with "nsSVGLength" before moving all the code from nsSVGLength2.cpp to nsSVGLength.cpp? That way we get to keep the CVS history of everything. Or was that already your plan?

Comment 19

[Jonathan Watt [:jwatt]]

I'm not really convinced about using an array for the attributes. Do we really save that much? And is it worth obfuscating the code and (less importantly) making it slightly slower?

Comment 20

[Jonathan Watt [:jwatt]]

Instead of "Kind" can you you use "AttrName"? That seems a lot more indicative of what the values are. I.e. aKind -> aAttrName and mKind -> mAttrName etc.

+  for (PRUint32 i = 0; i < 3; i++)
+    mLengthAttributes[i].SetContext(this);

Can you take the 3 from mLengthAttributes rather than hardcode it in?

+  NS_ASSERTION(aKind >= 0 && aKind <= 3, "aKind out of range");

Can you use the enum vals rather than literals so we can grep for them. Same in lots of other places.

+  float mAnimatedValue;
+  float mBaseValue;

Nit: can you make this just mAnimVal and mBaseVal for shortness and to match the DOM names?

+  PRUint16 mSpecifiedUnitType;
+  PRInt8 mKind; // element specified tracking for attribute
+  PRUint8 mCtxType:2; // X, Y or Unspecified
+  PRPackedBool mIsAnimated:1;

Seems like you could get rid of the overhead of (packing/unpacking) bitfields by making mSpecifiedUnitType a PRUint8. If you're converting enum (unsigned int) to PRInt8 (signed char) for mKind I don't see why not. (I'm wondering if there isn't some overhead to mixing 4 byte unsigned enum values with 1 byte unsigned PRInt8 values though?)

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #19)
> I'm not really convinced about using an array for the attributes. Do we
> really save that much? And is it worth obfuscating the code and (less
> importantly) making it slightly slower?

It shrinks the code a heck of a lot, especially if tor adopts my later suggestions. In the end, this makes it easier to understand.

(In reply to comment #20)
> +  for (PRUint32 i = 0; i < 3; i++)
> +    mLengthAttributes[i].SetContext(this);
> 
> Can you take the 3 from mLengthAttributes rather than hardcode it in?

This isn't in the latest version of the patch.

> +  NS_ASSERTION(aKind >= 0 && aKind <= 3, "aKind out of range");
> 
> Can you use the enum vals rather than literals so we can grep for them. Same
> in lots of other places.

With my suggestions above, these assertions get moved to a central method and the upper bound is named.

> +  PRUint16 mSpecifiedUnitType;
> +  PRInt8 mKind; // element specified tracking for attribute
> +  PRUint8 mCtxType:2; // X, Y or Unspecified
> +  PRPackedBool mIsAnimated:1;
> 
> Seems like you could get rid of the overhead of (packing/unpacking) bitfields
> by making mSpecifiedUnitType a PRUint8.

That's probably a good idea.

> If you're converting enum (unsigned
> int) to PRInt8 (signed char) for mKind I don't see why not. (I'm wondering if
> there isn't some overhead to mixing 4 byte unsigned enum values with 1 byte
> unsigned PRInt8 values though?)

Not really.

Comment 22

[Jonathan Watt [:jwatt]]

Sorry about the dud suggestions. Was looking at the smaller patch first, but thought I had checked the last. :-/

> Not really.

I thought sign extension was more expensive that zero extension. I know I'm picking at small nits here, but I meantion it as much as to learn as to improve the patch. I'll try and find something more substantial to comment on. ;-)

Comment 23

[tor]

Attachment: wip - all converted, before doing suggested modifications

Comment 24

[tor]

Attachment: wip - circle element/frame switched to alternate method

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #22)
> I thought sign extension was more expensive that zero extension.

It is, but usually only one extra instruction that doesn't touch memory, so that's about as cheap as something can be without being free. And of course only a conversion from PRInt8 to PRUint32 is actually sign-extending, the other way around is free. Anyway, many compilers (not sure about gcc) will actually look at the range of your enum values and make the enum 8-bit if that's wide enough.

Comment 26

[tor]

Attachment: wip - new method plus initialization

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  void GetLengthInfo(LengthAttributesInfo *aInfo);

Make it
LengthAttributesInfo GetLengthInfo();
and put a constructor on LengthAttributesInfo so you can write

nsSVGElement::LengthAttributesInfo
nsSVGCircleElement::GetLengthInfo()
{
  return LengthAttributesInfo(mLengthAttributes, gLengthInfo, 3);
}

+  mLengthAttributes[CX].Init(nsSVGUtils::X, CX);
+  mLengthAttributes[CY].Init(nsSVGUtils::Y, CY);
+  mLengthAttributes[R].Init(nsSVGUtils::XY, R);

These can go up to a loop in nsSVGElement.

Comment 28

[Robert Longson [:longsonr]]

How about setting aInfo->mLengthCount = sizeof gLengthInfo / sizeof gLengthInfo[0]. Is there any mozilla macro which does this as it's a common thing to do - get the number of items in an array?

In nsSVGElement::ParseAttribute you could put a single check for aNamespaceID == kNameSpaceID_None around ParseStyleAttribute and the length code.

Nit: I see you are tidying up the code with spaces you have missed one in your new code:
GetAnimValue(NS_STATIC_CAST(nsSVGCoordCtxProvider*,nsnull))
I think this line occurs twice.

Comment 29

[tor]

Attachment: swtich to new method, address review comments

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is glorious. Are you ready for real review now?

Comment 31

[tor]

Comment on attachment 218052 [details] [diff] [review]
swtich to new method, address review comments

Sure.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  LengthAttributesInfo GetLengthInfo();

Declare these virtual, because they are.

+    for (PRUint32 i = 0; i < info.mLengthCount; i++)
+      if (aAttribute == *info.mLengthInfo[i].mName) {

I prefer braces around multiline statements.

It seems to me that when someone changes the attribute, we're going to fire attribute change notifications twice, once via SetAttr->ParseAttribute->SetBaseValueString->DidChangeLength and then again from SetAttr. I think SetBaseValueString needs to take an aNotify parameter and only call DidChangeLength if that's true.

+    nsCOMPtr<nsIDOMSVGFilterElement> filter = do_QueryInterface(GetParent());
+    nsCOMPtr<nsISVGValue> value = do_QueryInterface(GetParent());
+    if (filter && value) {
+      value->BeginBatchUpdate();
+      value->EndBatchUpdate();
+    }

Why both testing filter? It just saves a bit of time in a case where the SVG is already malformed, right? I wouldn't bother testing "if (aNamespaceID == kNameSpaceID_None)" either, that's normally going to be true so not a very helpful optimization.

+float
+nsSVGLength2::GetValue(float aVal, nsSVGCoordCtx *aCtx)

I think this would be better named ConvertValue.

+void
+nsSVGLength2::SetValue(float aValue, nsSVGElement *aContent)

And this one should be 'SetBaseValue'.

+void
+nsSVGLength2::SetValueInSpecifiedUnits(float aValue, nsSVGElement *aContent)

SetBaseValueInSpecifiedUnits

+  nsresult ToDOMBaseVal(nsIDOMSVGLength **aResult, nsSVGElement* aContent)
+    { *aResult = new DOMBaseVal(this, aContent);
+      if (!*aResult) return NS_ERROR_OUT_OF_MEMORY;
+      return NS_OK; }
+  nsresult ToDOMAnimVal(nsIDOMSVGLength **aResult, nsSVGElement* aContent)
+    { *aResult = new DOMAnimVal(this, aContent);
+      if (!*aResult) return NS_ERROR_OUT_OF_MEMORY;
+      return NS_OK; }
+  nsresult ToDOMAnimatedLength(nsIDOMSVGAnimatedLength **aResult,
+                               nsSVGElement* aContent)
+    { *aResult =new DOMAnimatedLength(this, aContent);
+      if (!*aResult) return NS_ERROR_OUT_OF_MEMORY;
+      return NS_OK; }

I suggest making these non-inline.

+    nsSVGLength2* mVal; // kept alive because it belongs to content

"belongs to mContent"

I think you should move the AddRef from the DOM*Val constructors to the ToDOM*Val methods, to better match convention elsewhere.

Is it really intended that if the length is not animated, then doing ".animVal.value = 55" succeeds?

+    float viewportWidth, viewportHeight;
+    viewportWidth =
+      mLengthAttributes[MARKERWIDTH].GetAnimValue(mCoordCtx);
+    viewportHeight = 
+      mLengthAttributes[MARKERHEIGHT].GetAnimValue(mCoordCtx);

You could just declare "float ..." where you assign.

+    float refX, refY;
+    refX =
+      mLengthAttributes[REFX].GetAnimValue(mCoordCtx);
+    refY = 
+      mLengthAttributes[REFY].GetAnimValue(mCoordCtx);

ditto

Why doesn't nsSVGElement::UnsetAttr call DidChangeLength?

In some simple frame classes --- circle, ellipse, image, svg, line, rect --- I think you can just get by with #including nsSVGElement.h.

@@ -319,29 +300,22 @@ nsSVGInnerSVGFrame::PaintSVG(nsISVGRende
+    nsCOMPtr<nsSVGCoordCtxProvider> ctx =
+      nsSVGUtils::GetCoordContextProvider(mContent);

I don't think you need this anymore.

+    nsSVGSVGElement *svg = NS_STATIC_CAST(nsSVGSVGElement*, mContent);
+    nsCOMPtr<nsSVGCoordCtxProvider> ctx =
+      nsSVGUtils::GetCoordContextProvider(mContent);
+    x = svg->mLengthAttributes[nsSVGSVGElement::X].GetAnimValue(ctx);
+    y = svg->mLengthAttributes[nsSVGSVGElement::Y].GetAnimValue(ctx);

Can't you call GetAnimatedLengthValues here?

+  nsSVGUseElement *use = NS_STATIC_CAST(nsSVGUseElement*, mContent);
+  x = use->mLengthAttributes[nsSVGUseElement::X].GetAnimValue(use);
+  y = use->mLengthAttributes[nsSVGUseElement::Y].GetAnimValue(use);

Why can't you call GetAnimatedLengthValues here?

+nsSVGUtils::GetCoordContextProvider(nsIContent *aContent)

I think it would be slightly better if you make the parameter an nsSVGElement* so the callers have the burdern of casting and/or checking.

Comment 33

[tor]

Attachment: address review comments

Comment 34

[tor]

Attachment: forgot the include cleanup

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 218198 [details] [diff] [review]
forgot the include cleanup

This patch doesn't have the include cleanup.

+    info.mLengths[i].Init(info.mLengthInfo[i].mCtxType,
+                          i,
+                          info.mLengthInfo[i].mDefaultValue,
+                          info.mLengthInfo[i].mDefaultUnitType);

You missed braces around this:

+  char *str = ToNewCString(aValueAsString);

You need to return NS_ERROR_OUT_OF_MEMORY if this returns null.

You don't need an aNotify parameter in DidChangeLength. Just don't call it if aNotify is false. Which means that you don't need to call it in UnsetAttr after all.

+nsSVGUtils::UserSpace(nsIContent *aContent,
+                      nsSVGLength2 *aLength)

This should take an nsSVGElement*.

r+sr with those changes.

Something for future analysis/fixing: why do we need circle, ellipse etc frame types? Couldn't we move path construction to the content elements, and use nsSVGPathGeometryFrame for all of them? You could implement AttributeChanged generically by assuming that changes to any of the nsSVGLength2s in the content element require a path update.

Currently this patch makes access to a length property always return a new object (and ditto for access to .baseVal and .animVal). Our old behaviour was to always return the same object, and IIRC jwatt did tests to show that's true for other SVG DOM implementations. Consistency is needed here, so I think we're going to have to cache these wrappers in an object property. This can be done in a followup patch.

Comment 36

[tor]

Attachment: address next batch of comments, remove extra header files

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 218216 [details] [diff] [review]
address next batch of comments, remove extra header files

+  friend class nsSVGCircleFrame;
+

You don't need these anymore.

-  nsCOMPtr<nsIContent> mTarget;
+  nsSVGElement *mTarget;

Is it OK for this not to be a strong reference?

I think we're good to go.

Comment 38

[tor]

Attachment: remove friends, use strong references

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 218225 [details] [diff] [review]
remove friends, use strong references

Go go go

Comment 40

[Jonathan Watt [:jwatt]]

Getting the context *every* time we might need it is bad. Most of the time we'll probably encounter unitless or px lengths. We don't need the context for these types of lengths, so in the current code we don't fetch it. With this patch we will always fetch it, and it's not a cheap call.

+ nsSVGLength2::ConvertValue

Can we call this ConvertToUserUnits or something similar? The word "convert" by itself doesn't give you any idea what it does. It provides a nice match to ConvertToSpecifiedUnits.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's a good point. So where are we getting the provider?

nsSVGLength2::SetBaseValue: this one's easy to make lazy.

nsSVGLength2::GetBaseValue:
nsSVGLength2::GetAnimValue: fairly easy, just need a ConvertValueToUserUnits variant that takes an nsSVGElement*

nsSVGUtils::UserSpace: Easy, make it call GetAnimValue(nsSVGElement*)

nsSVGElement::GetAnimatedLengthValues: fairly easy. Start with ctx null, inside the loop, if info.mLengths[i].GetSpecifiedUnitType() is not px or number and ctx is null, set ctx.

Comment 42

[Jonathan Watt [:jwatt]]

Yeah, so the coordCtx issue can be fixed in a follow up patch, but I'd like it if the name of nsSVGLength2::ConvertValue could be changed and the following were addressed:

All the gLengthInfo static members should really be called sLengthInfo. Also it would be nice to use sizeof rather than hardcode in the size of sLengthInfo when calling LengthAttributesInfo IMHO. I.e.

  LengthAttributesInfo(mLengthAttributes, sLengthInfo, sizeof(sLengthInfo));


You're using the name "aContent" for variables that are of type nsSVGElement*, not of type nsIContent. The name aContent is used for objects that could be a comment, PI, entity, or whatever, not just for elements. Can you change the names to aSVGElement to make it clear throughout the code that it's not just an some type of content, or even an element we're dealing with, but rather an *SVG* element? E.g. I'm thinking of:

  nsSVGLength2::SetBaseValue(float aValue, nsSVGElement *aContent)


At the declarations and definitions of:

  nsSVGElement::DidChangeLength(PRUint8 aAttrEnum, PRBool aNotify)

and:

  nsSVGLength2::SetBaseValueString(const nsAString &aValueAsString,
                                   nsSVGElement *aContent,
                                   PRBool aNotify)

can you change aNotify to aSetAttr/aDoSetAttr please? The "notifications" that may occur are mutation events, and they only occur if we set the attribute. The mutation events are of little interest to us, but we are *very* interested in whether the attribute should be set or not.


Could nsSVGElement::GetLengthInfo return a static LengthAttributesInfo rather than construct a new object every times it's called?

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #42)
> All the gLengthInfo static members should really be called sLengthInfo. Also it
> would be nice to use sizeof rather than hardcode in the size of sLengthInfo
> when calling LengthAttributesInfo IMHO. I.e.
> 
>   LengthAttributesInfo(mLengthAttributes, sLengthInfo, sizeof(sLengthInfo));

That's not correct.

> You're using the name "aContent" for variables that are of type nsSVGElement*,
> not of type nsIContent. The name aContent is used for objects that could be a
> comment, PI, entity, or whatever, not just for elements. Can you change the
> names to aSVGElement to make it clear throughout the code that it's not just an
> some type of content, or even an element we're dealing with, but rather an
> *SVG* element? E.g. I'm thinking of:
> 
>   nsSVGLength2::SetBaseValue(float aValue, nsSVGElement *aContent)

I think this change is a bit too picky ... 

> can you change aNotify to aSetAttr/aDoSetAttr please? The "notifications" that
> may occur are mutation events, and they only occur if we set the attribute. The
> mutation events are of little interest to us, but we are *very* interested in
> whether the attribute should be set or not.

This controls not whether we set the attribute, but whether we fire AttributeChanged notifications.

> Could nsSVGElement::GetLengthInfo return a static LengthAttributesInfo rather
> than construct a new object every times it's called?

No, because it contains a pointer to element-specific data.

Comment 44

[Jonathan Watt [:jwatt]]

> That's not correct.

I thought 'g' was for 'global' (which these aren't) and 's' was for non-global 'static'.

> This controls not whether we set the attribute, but whether we fire
> AttributeChanged notifications.

My over-tired ramblings about mutation events being the *only* notifications that are sent out are of course wrong. SetAttrAndNotify is of also responsible for sending out the AttributeChanged notifications, and we rely on them. But I don't think I'm wrong to say that the flag controls whether we set the attribute or not. Right at the top of nsSVGElement::DidChangeLength there's this:

+nsSVGElement::DidChangeLength(PRUint8 aAttrEnum, PRBool aNotify)
+{
+  if (!aNotify)
+    return;

So since we don't call SetAttrAndNotify, the attribute isn't set. Setting the attribute and notifying of course go hand in hand, but I'd *much* prefer to call the argument aDoSetAttr. It would have helped me see straight away what was going on when looking at one of the DidChangeLength specializations, rather than having to find the base definition and examine it.

Comment 45

[Jonathan Watt [:jwatt]]

In nsSVGElement::UnsetAttr the nsSVGLength2 stuff that comes after the IsEventName() should be wrapped in an |else| statement.

I think you should have an assertion before the |if (info.mLengthCount == 0)| check in nsSVGElement::GetAnimatedLengthValues. That should never evaluate to false.

Will nsSVGMarkerElement still get notifications for all the objects it would with the current nsSVGMarkerElement::DidModifySVGObservable? It doesn't look like it will be notified about changes to the viewBox to me, but admitedly I'm somewhat sleep deprived ATM.

Comment 46

[Jonathan Watt [:jwatt]]

+  SetAttrAndNotify(kNameSpaceID_None, *info.mLengthInfo[aAttrEnum].mName,
+                   nsnull, oldStr, newValue,
+                   PR_FALSE, PR_TRUE, PR_TRUE);

Yikes! We're firing mutation events unconditionally? Why? Why don't we just call nsGenericElement::SetAttr instead of SetAttrAndNotify? That seems to be the right thing to do IMHO. It will return early if the new value is the same as the old for example. The only thing in there that we don't need is the XLink namespace check, but that's nothing compared to the cost of setting the attribute as a whole, so I don't think we should care about that.

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What's incorrect is that sizeof isn't giving you the length of the array. Use NS_ARRAY_LENGTH instead.

Changing the name from g to s and using NS_ARRAY_LENGTH are, I suppose, small but worthwhile improvements.

(In reply to comment #44)
> My over-tired ramblings about mutation events being the *only* notifications
> that are sent out are of course wrong. SetAttrAndNotify is of also responsible
> for sending out the AttributeChanged notifications, and we rely on them. But I
> don't think I'm wrong to say that the flag controls whether we set the
> attribute or not. Right at the top of nsSVGElement::DidChangeLength there's
> this:

I guess you're right.

Comment 48

[Jonathan Watt [:jwatt]]

> What's incorrect is that sizeof isn't giving you the length of the array. Use
> NS_ARRAY_LENGTH instead.

Oops. Yeah, that should have been sizeof(arr)/sizeof(*arr) or something.

Comment 49

[tor]

Attachment: latest round of comments + lazy context

Comment 50

[Jonathan Watt [:jwatt]]

Comment on attachment 218347 [details] [diff] [review]
latest round of comments + lazy context

> I think you should have an assertion before the |if (info.mLengthCount == 0)|
> check in nsSVGElement::GetAnimatedLengthValues. That should never evaluate to
> false.

Really, I think we should have that assertion. If we hit that return something has gone badly wrong, but we'll quietly return and be none the wiser. With that assertion, r=jwatt.


I'd also have prefered the aContent -> aSVGElement change, since little changes like this make source clearer and easier to read quickly IMHO. The reason I'm picky about these little things is that we'll never go back and make that change after this is checked in, but we would go back and do the coordCtx change for instance. Anyway, I suppose I'll reluctantly (kicking and screaming ;-) let this go since it isn't something to block review over.

BTW, this patch completely rocks! I am so loving these changes! :-)

Comment 51

[tor]

Attachment: final version (hopefully) for checkin

Comment 52

[Jonathan Watt [:jwatt]]

Nice! Thanks for changing aContent. :-)

Comment 53

[tor]

Checked in.  Needed a couple of curious changes to get vc6 happy:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/content/svg/content/src&command=DIFF_FRAMESET&file=nsSVGLength2.h&rev1=1.1&rev2=1.2&root=/cvsroot
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/content/svg/content/src&command=DIFF_FRAMESET&file=nsSVGElement.h&rev1=1.52&rev2=1.53&root=/cvsroot

Comment 54

[Boris Zbarsky [:bzbarsky]]

This caused bug 352295