Closed Bug 395667 Opened 13 years ago Closed 13 years ago

New style nsSVGInteger

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: longsonr, Assigned: longsonr)

Details

Attachments

(1 file, 3 obsolete files)

 
Assignee: nobody → longsonr
Attached patch new style nsSVGInteger (obsolete) — Splinter Review
Switch Integers over to the style of nsSVGLength2 and nsSVGNumber2.
Attachment #280357 - Flags: review?(tor)
Attachment #280357 - Flags: review?(tor)
Attached patch new style nsSVGInteger (obsolete) — Splinter Review
Use default values properly with invalid dual value attributes.
Attachment #280357 - Attachment is obsolete: true
Attachment #280363 - Flags: review?(tor)
Patch is missing the new nsSVGInteger.{h,cpp} files.
Attached patch complete patch (obsolete) — Splinter Review
Oops
Attachment #280363 - Attachment is obsolete: true
Attachment #280576 - Flags: review?(tor)
Attachment #280363 - Flags: review?(tor)
Attachment #280576 - Flags: review?(tor) → review+
Attachment #280576 - Flags: superreview?(roc)
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
Attachment #280576 - Flags: superreview?(roc)
Attachment #280576 - Flags: superreview+
Attachment #280576 - Flags: approval1.9+
(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.
Attachment #280576 - Attachment is obsolete: true
checked in.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.