Last Comment Bug 468996 - SVG SMIL: Implement "animateTransform"
: SVG SMIL: Implement "animateTransform"
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Brian Birtles (:birtles)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 216462 483584 539236 548899 556841 595608
Blocks: 469040 561726
  Show dependency treegraph
 
Reported: 2008-12-10 14:23 PST by Brian Birtles (:birtles)
Modified: 2010-09-23 14:14 PDT (History)
12 users (show)
roc: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch (103.08 KB, patch)
2008-12-10 16:54 PST, Brian Birtles (:birtles)
no flags Details | Diff | Splinter Review
Updated patch, rebased off smil5a.patch (108.33 KB, patch)
2008-12-18 18:10 PST, Brian Birtles (:birtles)
no flags Details | Diff | Splinter Review
patch v1c (104.92 KB, patch)
2008-12-23 14:11 PST, Brian Birtles (:birtles)
no flags Details | Diff | Splinter Review
patch v1d (94.30 KB, patch)
2008-12-29 15:16 PST, Brian Birtles (:birtles)
no flags Details | Diff | Splinter Review
patch v1e (99.65 KB, patch)
2008-12-29 20:42 PST, Brian Birtles (:birtles)
no flags Details | Diff | Splinter Review
patch v1g (96.57 KB, patch)
2009-01-06 19:26 PST, Brian Birtles (:birtles)
no flags Details | Diff | Splinter Review
patch v1k (96.56 KB, patch)
2009-01-14 18:52 PST, Brian Birtles (:birtles)
no flags Details | Diff | Splinter Review
patch v1l (107.62 KB, patch)
2009-01-15 20:03 PST, Brian Birtles (:birtles)
no flags Details | Diff | Splinter Review
patch v1m (112.28 KB, patch)
2009-01-15 20:45 PST, Brian Birtles (:birtles)
no flags Details | Diff | Splinter Review
patch v1n (111.35 KB, patch)
2009-01-18 13:24 PST, Brian Birtles (:birtles)
no flags Details | Diff | Splinter Review
patch v1o (115.90 KB, patch)
2009-01-18 17:09 PST, Brian Birtles (:birtles)
roc: review+
roc: superreview+
Details | Diff | Splinter Review

Description Brian Birtles (:birtles) 2008-12-10 14:23:59 PST
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) 2008-12-10 16:54:12 PST
Created attachment 352444 [details] [diff] [review]
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) 2008-12-10 16:56:20 PST
I forgot to mention, this patch applies on top of patch v4b from #216462
Comment 3 Brian Birtles (:birtles) 2008-12-18 18:10:29 PST
Created attachment 353767 [details] [diff] [review]
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) 2008-12-23 14:11:19 PST
Created attachment 354333 [details] [diff] [review]
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) 2008-12-29 15:16:28 PST
Created attachment 354749 [details] [diff] [review]
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) 2008-12-29 20:42:47 PST
Created attachment 354778 [details] [diff] [review]
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) 2009-01-06 19:26:13 PST
Created attachment 355714 [details] [diff] [review]
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) 2009-01-06 19:27:30 PST
That last comment should refer to attachment 355713 [details] [diff] [review] in bug 216462
Comment 9 Brian Birtles (:birtles) 2009-01-14 18:52:48 PST
Created attachment 357087 [details] [diff] [review]
patch v1k

Rebased off smil patch v5k -- see bug 216462 attachment
357078 [review]
Comment 10 Robert Longson 2009-01-15 14:55:10 PST
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) 2009-01-15 20:03:53 PST
Created attachment 357298 [details] [diff] [review]
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) 2009-01-15 20:11:22 PST
(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) 2009-01-15 20:19:21 PST
(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) 2009-01-15 20:45:16 PST
Created attachment 357305 [details] [diff] [review]
patch v1m

Addressed an issue with correctly getting the rotation centre of an underlying value.
Comment 15 Brian Birtles (:birtles) 2009-01-18 13:24:27 PST
Created attachment 357576 [details] [diff] [review]
patch v1n

Minor fixes. Prepare for review.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-01-18 16:20:33 PST
+    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) 2009-01-18 17:09:38 PST
Created attachment 357597 [details] [diff] [review]
patch v1o

Address comments from review.
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-01-19 01:45:05 PST
Pushed c8da73bbcb1b

Note You need to log in before you can comment on or make changes to this bug.