Clean up logic for addition & interpolation in SVGPathSegListSMILType

RESOLVED FIXED in mozilla6

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla6
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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
    Element().

(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.
(Assignee)

Comment 1

6 years ago
Created attachment 525613 [details] [diff] [review]
fix
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #525613 - Flags: review?(jwatt)
(Assignee)

Comment 2

6 years ago
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)
(Assignee)

Comment 3

6 years ago
Comment on attachment 525613 [details] [diff] [review]
fix

(canceling review -- making a few minor tweaks & reposting in a minute)
Attachment #525613 - Flags: review?(jwatt)
(Assignee)

Comment 4

6 years ago
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)
Attachment #525613 - Attachment is obsolete: true
Attachment #525617 - Flags: review?(jwatt)

Updated

6 years ago
Attachment #525617 - Flags: review?(jwatt) → review+
(Assignee)

Comment 5

6 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/a80424c4b0a8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
(Assignee)

Updated

6 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.