468996 - SVG SMIL: Implement "animateTransform"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20081202 Minefield/3.1b3pre
Build Identifier: 

The current implementation of animateTransform in #216462 is for demonstration purposes only. It lacks support for:
* rotation angles > 180
* rotation around a center
* by-animation (especially for scaling)
* to-animation
* paced animation

As well as that it's very inefficient (using nsIDOMSVGMatrix). A proper implementation is required that addresses these issues.

Reproducible: Always

Comment 1

[Brian Birtles (:birtles)]

Attachment: Proposed patch

This patch addresses this issues outlined in this bug with the exception of paced animation. I'll open a new bug for paced animation as this requires architectural changes.

Comment 2

[Brian Birtles (:birtles)]

I forgot to mention, this patch applies on top of patch v4b from #216462

Comment 3

[Brian Birtles (:birtles)]

Attachment: Updated patch, rebased off smil5a.patch

This is an updated patch that is rebased off smil5a.patch from bug 216462.

It also includes the optimisations discussed under brb-22 of that bug. (See bug 216462, comment 142).

Comment 4

[Brian Birtles (:birtles)]

Attachment: patch v1c

Updated patch:
* Re-based off smil5b
* Brought implementation into line with the interpretation of SMIL used by the SVGT1.2 test suite.

Outstanding issues for a subsequent patch:
* Implement paced animation
* Fix naming of reftest files
* Fix naming of some local variables
* Check and update documentation in nsISMILType.h
* Remove aData parameter from nsISMILAttr::GetSMILType() and subclasses

Comment 5

[Brian Birtles (:birtles)]

Attachment: patch v1d

Changes in this version:
* Rebased on patch v5c from bug 216462.
* animateTransform functionality moved entirely to this patch
* changes to nsSMILValue migrated to patch for bug 216462
* implemented paced animation for animateTransform according to the definitions used in SVGT1.2
* other outstanding issues from previous patch addressed (extra param for nsISMILAttr::GetSMILType, naming of reftests and local variables)

Comment 6

[Brian Birtles (:birtles)]

Attachment: patch v1e

* Migrated changes to nsSVGAnimatedTransformList from patch for bug 216462 to this patch
* Rebased off changes to patch for bug 216462 -- now applies on top of patch v5d (attachment 354777 [details] [diff] [review])

Comment 7

[Brian Birtles (:birtles)]

Attachment: patch v1g

Rebasing off updated underlying smil patch v5g -- see bug 216462 attachment 355712 [details] [diff] [review]
(Skipping version 1f so the letters line up with the underlying smil patches)

Comment 8

[Brian Birtles (:birtles)]

That last comment should refer to attachment 355713 [details] [diff] [review] in bug 216462

Comment 9

[Brian Birtles (:birtles)]

Attachment: patch v1k

Rebased off smil patch v5k -- see bug 216462 attachment
357078 [review]

Comment 10

[Robert Longson [:longsonr]]

Comment on attachment 357087 [details] [diff] [review]
patch v1k

I've not really looked at all of this but I do have a couple of things to say while I remember...

>+PRBool
>+nsSVGAnimateTransformElement::ParseAttribute(PRInt32 aNamespaceID,
>+                                             nsIAtom* aAttribute,
>+                                             const nsAString& aValue,
>+                                             nsAttrValue& aResult)
>+{
>+  // 'type' is an <animateTransform>-specific attribute, and we'll handle it
>+  // specially.
>+  if (aNamespaceID == kNameSpaceID_None && aAttribute == nsGkAtoms::type) {
>+    aResult.ParseAtom(aValue);
>+    nsIAtom* atom = aResult.GetAtomValue();
>+    if (atom != nsGkAtoms::translate ||
>+        atom != nsGkAtoms::scale ||
>+        atom != nsGkAtoms::rotate ||
>+        atom != nsGkAtoms::skewX ||
>+        atom != nsGkAtoms::skewY) {
>+      ReportAttributeParseFailure(GetOwnerDoc(), aAttribute, aValue);
>+    }
>+    return PR_TRUE;
>+  } else {

} else { is unnecessary

>+    return nsSVGAnimateTransformElementBase::ParseAttribute(aNamespaceID, 
>+                                                            aAttribute, aValue,
>+                                                            aResult);
>+  }
>+}
>+

>+inline void
>+nsSVGTransformSMILAttr::SkipCommaWsp(nsACString::const_iterator& aIter,
>+                               const nsACString::const_iterator& aIterEnd) const
>+{
>+  while (aIter != aIterEnd && (IsSpace(*aIter) || *aIter == ','))
>+    ++aIter;
>+}

This will just skip over ,,,,,, It probably shouldn't should it? Instead that should likely be a parse error.

Perhaps you could consider redoing the parsing using nsCommaSeparatedTokenizer and/or nsWhitespaceTokenizer as appropriate.

Comment 11

[Brian Birtles (:birtles)]

Attachment: patch v1l

I've reworked most of the patch to provide more sensible return values to getAnimVal. There's a new reftest to check this behaviour. You can also see it in action at: http://brian.sol1.net/svg/tests/transformAnimVal.svg

I've also fixed a number of other bugs and addressed some of the Robert Longson's comments (comment 10).

Comment 12

[Brian Birtles (:birtles)]

(In reply to comment #10)
> This will just skip over ,,,,,, It probably shouldn't should it? Instead that
> should likely be a parse error.

Thanks for that, it should be fixed by the latest patch. However, a trailing comma is still accepted.

> Perhaps you could consider redoing the parsing using nsCommaSeparatedTokenizer
> and/or nsWhitespaceTokenizer as appropriate.

I had a look at those and I can't see how to use them here as items can either be separated by whitespace or a comma and nsCommaSeparatedTokenizer seems to require a comma. (Actually, SVG's a little contradictory here, the spec for animateTransform shows commas for translate and scale but not for rotate. I'm following the BNF in http://www.w3.org/TR/SVG/coords.html#TransformAttribute instead which allows either whitespace or a comma or both.)

Comment 13

[Cameron McCormack (:heycam)]

(In reply to comment #12)
> (Actually, SVG's a little contradictory here, the spec for
> animateTransform shows commas for translate and scale but not for rotate. I'm
> following the BNF in http://www.w3.org/TR/SVG/coords.html#TransformAttribute
> instead which allows either whitespace or a comma or both.)

I have an action to investigate lists in attribute values in SVG, to check what implementations allow and write an erratum if necessary.  I've noticed at least that the grammar for transform lists includes:

transforms:
    transform
    | transform comma-wsp+ transforms
  
which doesn't seem correct.  I think it should be 'comma-wsp?' in that second production, i.e., nothing or whitespace with an optional comma in it.  (Gecko seems to use 'comma-wsp*' when parsing the @transform attribute.)

Comment 14

[Brian Birtles (:birtles)]

Attachment: patch v1m

Addressed an issue with correctly getting the rotation centre of an underlying value.

Comment 15

[Brian Birtles (:birtles)]

Attachment: patch v1n

Minor fixes. Prepare for review.

Comment 16

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

+    NS_WARNING("Trying to add incompatible types.");

This (and the equivalent in Add above) should be an NS_ERROR, assuming there's no markup which can get us into this situation.

+  if (aName == nsGkAtoms::transform) {
+    nsCOMPtr<nsIDOMSVGTransformable> transformable(
+            do_QueryInterface(static_cast<nsIContent*>(this)));
+    NS_ENSURE_TRUE(transformable, nsnull);

I think this should just be 
  if (!transformable)
    return nsnull;
We don't want to produce any warnings or errors in this case (unless they go to the error console), it's quite possible for people to write markup that triggers this.

+class nsSVGSMILTransform
+{

Add a comment explaining what this class is for, and especially what the parameters mean for each transform type.

+    for (int i = 0; i < 6; ++i)
+      mParams[i] = 0;

{} around the one-line statements in this file.

+class nsSVGTransformSMILType : public nsISMILType
+{

Here we need some comments explaining how many transforms are allowed in the transform array under different conditions, and what the various operations require in terms of the number of transforms, and why those requirements actually hold.

Otherwise looks great!

Comment 17

[Brian Birtles (:birtles)]

Attachment: patch v1o

Address comments from review.

Comment 18

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

Pushed c8da73bbcb1b