Closed
Bug 649568
Opened 13 years ago
Closed 13 years ago
Clean up logic for addition & interpolation in SVGPathSegListSMILType
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla6
People
(Reporter: dholbert, Assigned: dholbert)
Details
Attachments
(1 file, 1 obsolete file)
17.07 KB,
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
Assignee | ||
Comment 2•13 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•13 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•13 years ago
|
||
(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•13 years ago
|
Attachment #525617 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 5•13 years ago
|
||
Landed: http://hg.mozilla.org/mozilla-central/rev/a80424c4b0a8
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Assignee | ||
Updated•13 years ago
|
Flags: in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•