397916 - Simplify nsSVGLength2

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: patch

create some helper functions. Make the code more compatible with future SMIL work and similar to the new nsSVGAngle code.

Comment 1

[tor]

I'm not sure I see how this is a simplification.  You've moved some methods around in the file which makes the diff a bit hard to read, but the code seems pretty much the same before and after.

Comment 2

[Robert Longson [:longsonr]]

I tried to make it look more like the SMIL version and also abstract out the parts which make nsSVGAngle different to nsSVGLength2 so that its easier to fix them in the future. 

Some of the methods that were public are not used outside the class so I made them private and moved the implementation so the private methods are together. Would you rather I left the order as it was?

Comment 3

[tor]

(In reply to comment #2)
> I tried to make it look more like the SMIL version and also abstract out the
> parts which make nsSVGAngle different to nsSVGLength2 so that its easier to fix
> them in the future. 

What do you mean by "look more like the SMIL version"?  This is an animatable length value.

Comment 4

[Robert Longson [:longsonr]]

I mean look more like the version you would get if bug 216462 was applied.

Comment 5

[Robert Longson [:longsonr]]

bug 216462 adds a SetAnimValue to nsSVGLength2 and so abstracts out the implementation of SetBaseValue into a helper that can be called for that function too. This patch implements that small part of bug 216462 and also gives you the ConvertToUserUnits method that bug 216462 wants.

Comment 6

[tor]

Comment on attachment 282712 [details] [diff] [review]
patch

>+static nsresult
>+GetValueFromString(const nsAString &aValueAsString,
>+                   float *aValue,
>+                   PRUint16 *aUnitType)
>+{
>+  char *str = ToNewCString(aValueAsString);
>+  if (!str)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  char* number = str;
>+  while (*number && isspace(*number))
>+    ++number;

strtod can handle leading whitespace by itself.

>+  nsresult rv = NS_ERROR_FAILURE;
>+  
>+  if (*number) {
>+    char *rest;
>+    *aValue = static_cast<float>(PR_strtod(number, &rest));
>+    if (rest != number) {
>+      *aUnitType = GetUnitTypeForString(nsCRT::strtok(rest,
>+                                        "\x20\x9\xD\xA",
>+                                        &rest));

Shouldn't you check for an invalid unit return and report an error?

Comment 7

[Robert Longson [:longsonr]]

(In reply to comment #6)
> Shouldn't you check for an invalid unit return and report an error?
> 

Yes.

Comment 8

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 9

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

+GetUnitString(nsAString& unit, PRUint16 unitType)
+{

Why not just have an array of atoms indexed by unitType?

+static PRUint16
+GetUnitTypeForString(const char* unitStr)

You could then implement this function by iterating through the same array.

+nsSVGLength2::ConvertToUserUnits(float aValue, nsSVGElement *aSVGElement)
+nsSVGLength2::ConvertToUserUnits(float aValue, nsSVGSVGElement *aCtx)

How about having a single helper function that returns a scale factor, that the caller can then multiply by or divide by?

Comment 10

[Robert Longson [:longsonr]]

Attachment: address sr comments

Comment 11

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

Comment on attachment 283169 [details] [diff] [review]
address sr comments

+  mSpecifiedUnitType = static_cast<PRUint8>(unitType);

constructor-style cast (3 occurrences)

Comment 12

[Robert Longson [:longsonr]]

Attachment: version for check in

Comment 13

[Robert Longson [:longsonr]]

checked in.