As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact
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]
: Jet Villegas (:jet)
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 | Splinter Review
fix v1.1 (17.07 KB, patch)
2011-04-12 22:06 PDT, Daniel Holbert [:dholbert]
jwatt: review+
Details | Diff | Splinter Review

Description User image 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 User image Daniel Holbert [:dholbert] 2011-04-12 21:49:14 PDT
Created attachment 525613 [details] [diff] [review]
Comment 2 User image 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 User image 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 User image 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 User image 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.