392928 - New style nsSVGEnum

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Attachment: new style nsSVGEnum

Switch enums over to the style of nsSVGLength2 and nsSVGNumber2.  Saves us about 5K in code/data and makes the properties more specification compliant, as they will revert to their default values if the attribute is removed.

Comment 1

[Robert Longson [:longsonr]]

Comment on attachment 277426 [details] [diff] [review]
new style nsSVGEnum

>+  struct EnumInfo {
>+    nsIAtom** mName;
>+    PRUint16  mDefaultValue;
>+    nsSVGEnumMapping *mMapping;
>+  };

put the PRUint16 last.

>-nsSVGEnum::SetValueString(const nsAString& aValue)
>+nsresult
>+nsSVGEnum::SetBaseValueString(const nsAString& aValue,
>+                               nsSVGElement *aSVGElement,
>+                               PRBool aDoSetAttr)
> {

Nit: Indent arguments 1 space less.

> nsIAtom*
> nsSVGLinearGradientFrame::GetType() const
>@@ -581,22 +577,22 @@ nsSVGLinearGradientFrame::GradientLookup
>   nsSVGLinearGradientElement *element =
>     static_cast<nsSVGLinearGradientElement*>(gradient);
> 
>   // Object bounding box units are handled by setting the appropriate
>   // transform in GetGradientTransfrom, but we need to handle user
>   // space units as part of the individual Get* routines.  Fixes 323669.

Could you s/GetGradientTransfrom/GetGradientTransform/ here?

> already_AddRefed<gfxPattern>
> nsSVGLinearGradientFrame::CreateGradient()
>@@ -663,22 +659,22 @@ nsSVGRadialGradientFrame::GradientLookup
>   nsSVGRadialGradientElement *element =
>     static_cast<nsSVGRadialGradientElement*>(gradient);
> 
>   // Object bounding box units are handled by setting the appropriate
>   // transform in GetGradientTransfrom, but we need to handle user
>   // space units as part of the individual Get* routines.  Fixes 323669.

same correction as above here

>@@ -568,34 +567,31 @@ nsSVGOuterSVGFrame::NotifyViewportChange

>-      // of it's currentScale and currentTranslate properties
>-      PRUint16 val;
>-      mZoomAndPan->GetIntegerValue(val);

>-      }
>+    // our content is the document element so we must premultiply the values
>+    // of it's currentScale and currentTranslate properties

s/it's/its/

I can see you just copied it but you might as well correct it ;-)

r=longsonr with those addressed.

Comment 2

[tor]

Attachment: update per longsonr's comments

Comment 3

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

+        if (NS_FAILED(rv)) {
+          ReportAttributeParseFailure(GetOwnerDoc(), aAttribute, aValue);
+          return PR_FALSE;
+        }
+        aResult.SetTo(aValue);
+        return PR_TRUE;
+   

Is there an easy way to share this exit path with the same path for numbers and lengths?

+private:
+  nsSVGEnumMapping *mMapping;

Why is this needed? Can't we get this via the element's GetEnumInfo?

+  PRUint16 mAnimVal;
+  PRUint16 mBaseVal;

Wouldn't PRUint8 be more than adequate here? I can't imagine even the SVG WG defining an enum with more than 256 values.

In any case you should make a new typedef for this, nsSVGEnumValue or something and use it appropriately.

 interface nsIDOMSVGElement : nsIDOMElement 
 { 
+  // These constants are taken from SVGUnitTypes
+  const unsigned short SVG_UNIT_TYPE_UNKNOWN           = 0;
+  const unsigned short SVG_UNIT_TYPE_USERSPACEONUSE    = 1;
+  const unsigned short SVG_UNIT_TYPE_OBJECTBOUNDINGBOX = 2;

Is there any problem spec-wise having these in this interface?

Shouldn't we be removing mZoomAndPan from nsSVGOuterSVGFrame.h?

Comment 4

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

BTW I love this patch! But I hope that from now until 1.9 release I don't get too many non-blocker-material patches to review.

Comment 5

[tor]

(In reply to comment #4)
> BTW I love this patch! But I hope that from now until 1.9 release I don't get
> too many non-blocker-material patches to review.

This is something we'd like to get into 1.9, as it's one of the steps of fixing the problem that removing an attribute doesn't affect the DOM.

In general, though, your request presents a problem.  Since there are only two superreviewers familiar with SVG code, you and myself, patches I write need to go through you.  I would like to continue working on SVG rather than get frozen out completely during the 1.9 timeframe.

Comment 6

[tor]

(In reply to comment #3)
> +  PRUint16 mAnimVal;
> +  PRUint16 mBaseVal;
> 
> Wouldn't PRUint8 be more than adequate here? I can't imagine even the SVG WG
> defining an enum with more than 256 values.

Don't give them ideas...

I think I might have made it a PRUint16 to avoid compiler warning about moving between sized integral types, as the alternative of casting looks like a hack.  I'll take another look at changing the type.

>  interface nsIDOMSVGElement : nsIDOMElement 
>  { 
> +  // These constants are taken from SVGUnitTypes
> +  const unsigned short SVG_UNIT_TYPE_UNKNOWN           = 0;
> +  const unsigned short SVG_UNIT_TYPE_USERSPACEONUSE    = 1;
> +  const unsigned short SVG_UNIT_TYPE_OBJECTBOUNDINGBOX = 2;
> 
> Is there any problem spec-wise having these in this interface?

Wasn't sure what to do about this, to tell the truth.  The SVG IDL has a SVGUnitTypes interface which doesn't inherit from anything and just specifies those constants.  I could do the same, but if I go that route do I need to add it to DOMClassInfo?

Comment 7

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

(In reply to comment #5)
> (In reply to comment #4)
> > BTW I love this patch! But I hope that from now until 1.9 release I don't get
> > too many non-blocker-material patches to review.
> 
> This is something we'd like to get into 1.9, as it's one of the steps of fixing
> the problem that removing an attribute doesn't affect the DOM.

OK.

> In general, though, your request presents a problem.  Since there are only two
> superreviewers familiar with SVG code, you and myself, patches I write need to
> go through you.  I would like to continue working on SVG rather than get frozen
> out completely during the 1.9 timeframe.

I guess I'm saying I'd prefer you to work on blocker-ish issues than cleanup. (I understand now that this is perhaps a blocker-ish issue.) If you run out of blocker-ish issues to work on, then let's discuss further...

Comment 8

[Robert Longson [:longsonr]]

(In reply to comment #6)
> (In reply to comment #3)
> > +  PRUint16 mAnimVal;
> > +  PRUint16 mBaseVal;
> > 
> > Wouldn't PRUint8 be more than adequate here? I can't imagine even the SVG WG
> > defining an enum with more than 256 values.
> 
> Don't give them ideas...
> 
> I think I might have made it a PRUint16 to avoid compiler warning about moving
> between sized integral types, as the alternative of casting looks like a hack. 
> I'll take another look at changing the type.

I considered making this comment too but nsIDOMSVGAnimatedEnum has shorts for base and animated values which put me off. I don't think you can get large values in there though as all nsIVGAnimatedEnumerations in IDL are readonly and the setting DOM methods take strings.

Comment 9

[tor]

Attachment: address roc's comments

Comment 10

[Robert Longson [:longsonr]]

Comment on attachment 277954 [details] [diff] [review]
address roc's comments

>+
>+typedef PRUint8 nsSVGEnumValue;
> 
> struct nsSVGEnumMapping {
>-    nsIAtom **key;
>-    PRUint16 val;
>+  nsIAtom **mKey;
>+  nsSVGEnumValue mVal;
> };
> 

>+class nsSVGEnum
>+{
>+public:
>+  void Init(PRUint8 aAttrEnum, PRUint16 aValue) {
>+    mAnimVal = mBaseVal = aValue;
>+    mAttrEnum = aAttrEnum;
>+  }

You are going to get warnings about the aValue assignments. Up to you if you fix them with static_cast.  Having the argument as PRUint16 is good IMHO.

>+
>+  nsresult SetBaseValueString(const nsAString& aValue,
>+                              nsSVGElement *aSVGElement,
>+                              PRBool aDoSetAttr);
>+  void GetBaseValueString(nsAString& aValue,
>+                          nsSVGElement *aSVGElement);
>+
>+  void SetBaseValue(nsSVGEnumValue aValue,
>+                    nsSVGElement *aSVGElement,
>+                    PRBool aDoSetAttr);

Seems odd that Init takes a PRUint16 aValue but SetBaseValue takes a nsSVGEnumValue. Perhaps this should take a PRUint16 too and optionally have a static_cast in the implementation.

>+
>+  PRUint16 GetBaseValue()
>+    { return mBaseVal; }
>+  PRUint16 GetAnimValue()
>+    { return mAnimVal; }
>+
>+  nsresult ToDOMAnimatedEnum(nsIDOMSVGAnimatedEnumeration **aResult,
>+                             nsSVGElement* aSVGElement);
>+
>+private:
>+  nsSVGEnumValue mAnimVal;
>+  nsSVGEnumValue mBaseVal;
>+  PRUint8 mAttrEnum; // element specified tracking for attribute
>+
>+  nsSVGEnumMapping *GetMapping(nsSVGElement *aSVGElement);
>+
>+  struct DOMAnimatedEnum : public nsIDOMSVGAnimatedEnumeration
>+  {
>+    NS_DECL_ISUPPORTS
>+
>+    DOMAnimatedEnum(nsSVGEnum* aVal, nsSVGElement *aSVGElement)
>+      : mVal(aVal), mSVGElement(aSVGElement) {}
>+
>+    nsSVGEnum *mVal; // kept alive because it belongs to content
>+    nsRefPtr<nsSVGElement> mSVGElement;
>+
>+    NS_IMETHOD GetBaseVal(PRUint16* aResult)
>+      { *aResult = mVal->GetBaseValue(); return NS_OK; }
>+    NS_IMETHOD SetBaseVal(PRUint16 aValue)
>+      { mVal->SetBaseValue(aValue, mSVGElement, PR_TRUE); return NS_OK; }

You will get a warning here unless you either change mVal->SetBaseValue to take a PRUint16, or cast the assignment. I'd go for the former.

>+    NS_IMETHOD GetAnimVal(PRUint16* aResult)
>+      { *aResult = mVal->GetAnimValue(); return NS_OK; }
>+  };
>+};
> 

I have to admit that I haven't checked whether you implemented the rest of roc's comments but I'm sure you have ;-).

r=longsonr

Comment 11

[tor]

Attachment: with casting

gcc seems slacker than MSVC about type conversion warnings, even when -Wconversion is set by configure.in.

Comment 12

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

+  if (!info.mEnumCount || mAttrEnum > info.mEnumCount)
+    return nsnull;

Shouldn't this be an assertion? It can't ever happen normally, right?

Comment 13

[tor]

(In reply to comment #12)
> +  if (!info.mEnumCount || mAttrEnum > info.mEnumCount)
> +    return nsnull;
> 
> Shouldn't this be an assertion? It can't ever happen normally, right?

This won't happen for SVGEnums that are used as mapped attributes, but unfortunately the SVG DOM has exceptions for this.  The marker element has an orientType enum that is derived from the 'orient' attribute along with orientAngle.  Since this enum never has to map to strings, I pass in a invalid attrEnum (0xFF) when initializing.

Comment 14

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

Comment on attachment 278083 [details] [diff] [review]
with casting

OK, please add a comment to that effect.

Comment 15

[tor]

Attachment: checkin version

Added comment and switched to NS_ASSERTION as non-attrib enums should not ever get into a case of calling GetMapping.

Comment 16

[tor]

Checked in.