386416 - SVG: order attribute of feConvolveMatrix does not accept comma separated integers

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Florian]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a6pre) Gecko/20070629 Minefield/3.0a6pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a6pre) Gecko/20070629 Minefield/3.0a6pre

The feConvolveMatrix will not be displayed if the order attribute has a value like "3,3" or "1,10".

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Robert Longson [:longsonr]]

It does accept 2 integers, it just doesn't like the comma between them at the moment.

Comment 2

[Robert Longson [:longsonr]]

Attachment: patch

Comment 3

[Florian]

Possible ways how to write a List in SVG documents is explained here:
http://www.w3.org/TR/SVG11/types.html#DataTypeList

Comment 4

[Florian]

Attachment: A SVG file for testing the patch.

Comment 5

[Florian]

Attachment: A SVG file with a incorrrect list. (two commas instead of one)

Comment 6

[Florian]

Attachment: A second SVG file with a incorrrect list. (two whitespaces before the comma)

Comment 7

[Robert Longson [:longsonr]]

(In reply to comment #4)
> Created an attachment (id=270487) [details]
> A SVG file for testing the patch.
> 

url(#filter3) refers to something that does not exist.

(In reply to comment #5)
> Created an attachment (id=270488) [details]
> A SVG file with a incorrrect list. (two commas instead of one)
> 

This would parse OK with the patch which is wrong but then pretty much all of the svg number list processing suffers from this fault.

(In reply to comment #6)
> Created an attachment (id=270489) [details]
> A second SVG file with a incorrrect list. (two whitespaces before the comma)
> 

There is nothing wrong with this.

Comment 8

[Florian]

(In reply to comment #7)
> (In reply to comment #4)
> > Created an attachment (id=270487) [details] [details]
> > A SVG file for testing the patch.
> > 
> 
> url(#filter3) refers to something that does not exist.
One of the filters has a wrong id, I will submit a corrected version.

> 
> (In reply to comment #5)
> > Created an attachment (id=270488) [details] [details]
> > A SVG file with a incorrrect list. (two commas instead of one)
> > 
> 
> This would parse OK with the patch which is wrong but then pretty much all of
> the svg number list processing suffers from this fault.
Well you the Mozilla SVG developers have do decide this by your own. It would be nice if you at least start from now on to use/write some kind of pharseSVGIntList function.


> 
> (In reply to comment #6)
> > Created an attachment (id=270489) [details] [details]
> > A second SVG file with a incorrrect list. (two whitespaces before the comma)
> > 
> 
> There is nothing wrong with this.
> 
Year, my mistake, I overlooked the +. It is wrong that the example is wrong.

Comment 9

[Florian]

Attachment: A SVG file for testing the patch - revised edition

Comment 10

[Robert Longson [:longsonr]]

(In reply to comment #8)
> > This would parse OK with the patch which is wrong but then pretty much all of
> > the svg number list processing suffers from this fault.
> Well you the Mozilla SVG developers have do decide this by your own. It would
> be nice if you at least start from now on to use/write some kind of
> pharseSVGIntList function.
> 

Do feel free to raise another bug about this so we remember to tackle it as a follow up.

Comment 11

[tor]

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

It seems that this new version would allow invalid strings like "4,,5".  Also appears that case of only one value won't be handled correctly (setting output for both to that value).

Can nsAutoPtr be used to manage the string?

Comment 12

[Robert Longson [:longsonr]]

Attachment: updated patch

No string management required with this version.

Comment 13

[tor]

Comment on attachment 284472 [details] [diff] [review]
updated patch

>+PRBool
>+nsSVGElement::ParseNumberOptionalNumber(nsIAtom* aAttribute, const nsAString& aValue,
>+                                        nsSVGNumber2* aNum1, nsSVGNumber2* aNum2, 
>+                                        float defaultValue1, float defaultValue2)
>+{
>+  char *rest;
>+  PRBool parseError = PR_FALSE;
>+
>+  NS_ConvertUTF16toUTF8 value(aValue);
>+  value.CompressWhitespace(PR_FALSE, PR_TRUE);
>+  const char *str = value.get();
>+
>+  float x = static_cast<float>(PR_strtod(str, &rest));
>+  float y = defaultValue2;
>+
>+  if (str == rest) {
>+    //first value was illformed
>+    parseError = PR_TRUE;
>+  } else {
>+    if (*rest == '\0') {
>+      //second value was not supplied
>+      y = x;
>+    } else {

>+      for (PRBool commaFound = PR_FALSE;
>+           *rest == ',' || NS_IsAsciiWhitespace(*rest); ++rest) {
>+        if (*rest == ',') {
>+          if (commaFound)
>+            break;
>+          commaFound = PR_TRUE;
>+          continue;
>+        }
>+      }

Would this be slightly simpler?

while (NS_IsAsciiWhitespace(*rest))
  rest++;
if (*rest == ',')
  rest++;

>+      y = static_cast<float>(PR_strtod(rest, &rest));
>+      if (*rest != '\0') {
>+        //second value was illformed or there was trailing content
>+        parseError = PR_TRUE;
>+      }
>+    }
>+  }
>+
>+  if (parseError) {
>+    ReportAttributeParseFailure(GetOwnerDoc(), aAttribute, aValue);
>+    x = defaultValue1;
>+    y = defaultValue2;
>+  }
>+
>+  aNum1->SetBaseValue(x, this, PR_FALSE);
>+  aNum2->SetBaseValue(y, this, PR_FALSE);
>+
>+  return (!parseError);
>+}

Comment 14

[tor]

Comment on attachment 284472 [details] [diff] [review]
updated patch

> PRBool
> nsSVGFEGaussianBlurElement::ParseAttribute(PRInt32 aNameSpaceID, nsIAtom* aName,
>                                            const nsAString& aValue,
>                                            nsAttrValue& aResult)
> {
>   if (aName == nsGkAtoms::stdDeviation && aNameSpaceID == kNameSpaceID_None) {
>-    return ScanDualValueAttribute(aValue, nsGkAtoms::stdDeviation,
>+    if (ParseNumberOptionalNumber(aName, aValue,
>                                   &mNumberAttributes[STD_DEV_X],
>                                   &mNumberAttributes[STD_DEV_Y],
>-                                  &sNumberInfo[STD_DEV_X],
>-                                  &sNumberInfo[STD_DEV_Y],
>-                                  aResult);
>+                                  sNumberInfo[STD_DEV_X].mDefaultValue,
>+                                  sNumberInfo[STD_DEV_Y].mDefaultValue)) {
>+      aResult.SetTo(aValue);
>+      return PR_TRUE;
>+    }
>+    return PR_TRUE;
>   }
>   return nsSVGFEGaussianBlurElementBase::ParseAttribute(aNameSpaceID, aName,
>                                                         aValue, aResult);
> }

Used for example, though all the ParseAttribute implementations in the patch share the same basic structure.  This is only setting aResult for a successful parse of the attribute, which means that if someone tries to set a garbage value and then read it back with getAttribute, the value returned will be the previous value.  If you change the second PR_TRUE to PR_FALSE, nsGenericElement::SetAttr will do the SetTo for you.

Comment 15

[tor]

Strictly speaking, the SVG specification forbids leading whitespace for lists (of which "xxx optional xxx" is a special case).  Your patch will accept a string of that type.

Comment 16

[tor]

(In reply to comment #15)
> Strictly speaking, the SVG specification forbids leading whitespace for lists
> (of which "xxx optional xxx" is a special case).  Your patch will accept a
> string of that type.

Likewise for trailing whitespace.

Valid:

"3 4"
"3,4"
"3 ,4"
"3 , 4"

Invalid:

" 3,4"
"3,4 "

http://www.w3.org/TR/SVG11/types.html#DataTypeList

Comment 17

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 18

[tor]

Comment on attachment 284617 [details] [diff] [review]
address review comments

>+PRBool
>+nsSVGElement::ParseNumberOptionalNumber(nsIAtom* aAttribute, const nsAString& aValue,
>+                                        nsSVGNumber2* aNum1, nsSVGNumber2* aNum2, 
>+                                        float defaultValue1, float defaultValue2)
>+{
>+  NS_ConvertUTF16toUTF8 value(aValue);
>+  const char *str = value.get();
>+
>+  PRBool parseError = NS_IsAsciiWhitespace(*str);

Shouldn't everything below up to the parseError test be in a "if (!parseError)"?

>+  char *rest;
>+  float x = static_cast<float>(PR_strtod(str, &rest));
>+  float y = x;
>+
>+  if (str == rest) {
>+    //first value was illformed
>+    parseError = PR_TRUE;
>+  } else {
>+    if (*rest != '\0') {
>+      while (NS_IsAsciiWhitespace(*rest)) {
>+        ++rest;
>+      }
>+      if (*rest == ',') {
>+        ++rest;
>+      }
>+
>+      y = static_cast<float>(PR_strtod(rest, &rest));
>+      if (*rest != '\0') {
>+        //second value was illformed or there was trailing content
>+        parseError = PR_TRUE;
>+      }
>+    }
>+  }

>+  if (parseError) {
>+    ReportAttributeParseFailure(GetOwnerDoc(), aAttribute, aValue);
>+    x = defaultValue1;
>+    y = defaultValue2;
>+  }

If you add nsAttrValue to the parameters and a "else aResult.SetTo(aValue);" you can avoid that code at all the call sites and instead just do "return Parse...".

>+  aNum1->SetBaseValue(x, this, PR_FALSE);
>+  aNum2->SetBaseValue(y, this, PR_FALSE);
>+
>+  return (!parseError);
>+}

Comment 19

[Robert Longson [:longsonr]]

Attachment: address additional review comments

(In reply to comment #18)
> >+  PRBool parseError = NS_IsAsciiWhitespace(*str);
> 
> Shouldn't everything below up to the parseError test be in a "if
> (!parseError)"?

Sure, if you want. The common valid case is fractionally slower the uncommon invalid case is faster. Functionally the same outcome.

> If you add nsAttrValue to the parameters and a "else aResult.SetTo(aValue);"
> you can avoid that code at all the call sites and instead just do "return
> Parse...".
> 

Done.

Comment 20

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

+    x = y = static_cast<float>(PR_strtod(str, &rest));

Use constructor casts to cast to float.

What if the Parse* functions just took the indexes of the number-attributes and integer-attributes instead of pointers to nsSVGNumber and nsSVGInteger? Then they could look up the correct default value, saving two parameters and reducing code in callers.

Comment 21

[Robert Longson [:longsonr]]

Attachment: address sr comments

Comment 22

[Robert Longson [:longsonr]]

checked in.