Closed Bug 649568 Opened 11 years ago Closed 11 years ago

Clean up logic for addition & interpolation in SVGPathSegListSMILType


(Core :: SVG, defect)

Not set





(Reporter: dholbert, Assigned: dholbert)



(1 file, 1 obsolete file)

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.
Attached patch fix (obsolete) — Splinter Review
Assignee: nobody → dholbert
Attachment #525613 - Flags: review?(jwatt)
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 on attachment 525613 [details] [diff] [review]

(canceling review -- making a few minor tweaks & reposting in a minute)
Attachment #525613 - Flags: review?(jwatt)
Attached patch fix v1.1Splinter Review
(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)
Attachment #525613 - Attachment is obsolete: true
Attachment #525617 - Flags: review?(jwatt)
Attachment #525617 - Flags: review?(jwatt) → review+
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.