395667 - New style nsSVGInteger

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Comment 1

[Robert Longson [:longsonr]]

Attachment: new style nsSVGInteger

Switch Integers over to the style of nsSVGLength2 and nsSVGNumber2.

Comment 2

[Robert Longson [:longsonr]]

Attachment: new style nsSVGInteger

Use default values properly with invalid dual value attributes.

Comment 3

[tor]

Patch is missing the new nsSVGInteger.{h,cpp} files.

Comment 4

[Robert Longson [:longsonr]]

Attachment: complete patch

Oops

Comment 5

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

Comment on attachment 280576 [details] [diff] [review]
complete patch

We should keep an eye on the overhead of checking for each type of attribute in nsSVGElement. If it becomes noticeable, we could add a virtual GetInfoTypes() method that returns a set of flags indicating which Get*Info methods return non-empty lists.

+        if (aName == *intInfo.mIntegerInfo[i].mName) {
+          intInfo.mIntegers[i].Init(i, intInfo.mIntegerInfo[i].mDefaultValue);
+          DidChangeInteger(i, PR_FALSE);
+          break;
+        }

filterRes is being used twice in an IntegerAttributesInfo, so this 'break' is no good, right?

+    int       mDefaultValue;

PRInt32?

+  { &nsGkAtoms::filterRes, 0 },
+  { &nsGkAtoms::filterRes, 0 }

This is a little freaky, but I guess it will work OK with the fix above. But please document somewhere which kinds of AttributeInfos are allowed to have duplicate attributes and which aren't.

   // Create mapped properties:
 
This comment can go

+      mIntegerAttributes[FILTERRES_Y].SetBaseValue(resX, this, PR_FALSE);

resY

Comment 6

[Robert Longson [:longsonr]]

Attachment: version for check in

(In reply to comment #5)
> 
> +    int       mDefaultValue;
> 
> PRInt32?

Done.

> 
> +  { &nsGkAtoms::filterRes, 0 },
> +  { &nsGkAtoms::filterRes, 0 }
> 
> This is a little freaky, but I guess it will work OK with the fix above. But
> please document somewhere which kinds of AttributeInfos are allowed to have
> duplicate attributes and which aren't.

Well spotted. Basically *any* types may and actually already do have duplicate attributes. I've just removed the breaks for all types as per your suggestion. Since this applies to everything I've not put in any specific comment.

> 
>    // Create mapped properties:
> 
> This comment can go

It still applies to the stuff below, although there's only one left now.

    rv = AddMappedSVGValue(nsGkAtoms::href, mHref, kNameSpaceID_XLink);

> 
> +      mIntegerAttributes[FILTERRES_Y].SetBaseValue(resX, this, PR_FALSE);
> 
> resY
> 

I really did mean resX here. This the case that you have filterRes="300" which means set both the X and Y filter resolution to 300. We scan one value (resX) and then use it for both FILTERRES_X and FILTERRES_Y. At this point the value of the resY variable is undefined.

Comment 7

[Robert Longson [:longsonr]]

checked in.