Last Comment Bug 649568 - Clean up logic for addition & interpolation in SVGPathSegListSMILType
: Clean up logic for addition & interpolation in SVGPathSegListSMILType
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Daniel Holbert [:dholbert]
Depends on:
  Show dependency treegraph
Reported: 2011-04-12 21:48 PDT by Daniel Holbert [:dholbert]
Modified: 2011-04-28 12:10 PDT (History)
0 users
dholbert: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

fix (16.89 KB, patch)
2011-04-12 21:49 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Review
fix v1.1 (17.07 KB, patch)
2011-04-12 22:06 PDT, Daniel Holbert [:dholbert]
jwatt: review+
Details | Diff | Review

Description Daniel Holbert [:dholbert] 2011-04-12 21:48:09 PDT
In working on bug 641393, I noticed a few issues in SVGPathSegListSMILType, and I decided to spin them off into a separate bug.  Issues below:

(A) The following chunk of SVGPathSegListSMILType::Add() doesn't take
    aCount into consideration:
> 339   // Allow addition to empty dest.
> 340   if (dest.IsEmpty()) {
> 341     return dest.CopyFrom(valueToAdd);
> 342   }
    (Note that fixing this isn't as simple as inserting " * aCount" on
     valueToAdd's contents, since we explicitly _don't_ want to multiply
     the encoded segType & the boolean arc flags by anything)

(B) Sort of an offshoot of A: the straightforward fix for (A) would result
    in a bunch of near-duplicated code -- we'd effectively copy the existing
    block of code in Interpolate() that does special-casing for boolean flags.

(C) Unlike other list types, SVGPathSegListSMILType uses IsEmpty() instead
    of Element() to check for 'identity' values.  It should change to use

(D) SVGPathSegListSMILType doesn't actually propagate Element() values to
    its outparams right now, so a trivial fix for (C) doesn't work, because
    Element() is currently null a lot of the time (when it shouldn't be).

This patch fixes all of the above, using...
 * ...a new method "AddWeightedPathSegLists" (and its helper AddWeightedPathSegs) to share code between Add() and Interpolate(). This method is analogous to nsStyleAnimation::AddWeighted.  This addresses points A & B.
 * ...a new method IsIdentity(), wrapping Element(), to address point C
 * ...some targeted SetElement() calls to address point D. (Specifically, I call SetElement() in every spot where we use SetLength() to grow a formerly-identity list into a non-identity-list.)

NOTE: I assert that our nsTArray::SetLength() calls all succeed, because nsTArray is infallible-by-default now.
Comment 1 Daniel Holbert [:dholbert] 2011-04-12 21:49:14 PDT
Created attachment 525613 [details] [diff] [review]
Comment 2 Daniel Holbert [:dholbert] 2011-04-12 21:51:13 PDT
This patch applies on top of bug 641393's patches, btw. ("Patch 2" on that bug makes some non-functional style-consistency-tweaks in the contextual regions of this bug's patch)
Comment 3 Daniel Holbert [:dholbert] 2011-04-12 22:00:05 PDT
Comment on attachment 525613 [details] [diff] [review]

(canceling review -- making a few minor tweaks & reposting in a minute)
Comment 4 Daniel Holbert [:dholbert] 2011-04-12 22:06:25 PDT
Created attachment 525617 [details] [diff] [review]
fix v1.1

(This just adds some minor comment/whitespace tweaks, as well as one more substitution of the new #defines LARGE_ARC_FLAG_IDX & SWEEP_FLAG_IDX that I'd missed before)
Comment 5 Daniel Holbert [:dholbert] 2011-04-28 12:08:26 PDT

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