Closed
Bug 540478
Opened 11 years ago
Closed 11 years ago
Add support for SMIL animation of enumeration attributes in SVG
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
People
(Reporter: jwatt, Assigned: jwatt)
References
Details
Attachments
(2 files)
15.99 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
27.15 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
We should add support for SMIL animation of enumeration attributes in SVG.
![]() |
Assignee | |
Comment 1•11 years ago
|
||
![]() |
Assignee | |
Comment 2•11 years ago
|
||
Attachment #422246 -
Flags: review?(dholbert)
Updated•11 years ago
|
Attachment #422246 -
Attachment is patch: true
Attachment #422246 -
Attachment mime type: application/octet-stream → text/plain
Comment 3•11 years ago
|
||
Comment on attachment 422246 [details] [diff] [review] patch to add support for animating enum attributes >diff --git a/content/smil/Makefile.in b/content/smil/Makefile.in > nsSMILValue.cpp \ >+ SMILEnumType.cpp \ Why no "ns" prefix here? (nsSMILEnumType) Same in bug 540479. Are we moving away from that?
![]() |
Assignee | |
Comment 4•11 years ago
|
||
Yes, there's a move across the whole moz codebase to using the 'mozilla' namespace to replace 'ns' now that namespaces are officially allowed in moz code.
Comment 5•11 years ago
|
||
Comment on attachment 422246 [details] [diff] [review] patch to add support for animating enum attributes >+ mBaseVal = mAnimVal = mapping->mVal; >+ #ifdef MOZ_SMIL >+ if (mIsAnimated) { >+ aSVGElement->AnimationNeedsResample(); >+ } >+ #endif >+ } I think the #ifdef and #endif should be flush left. I think this occurs in a couple of places. More importantly are you planning to do auto angled markers as part of an angle animation patch?
Updated•11 years ago
|
Attachment #422245 -
Flags: review?(dholbert) → review+
![]() |
Assignee | |
Comment 6•11 years ago
|
||
(In reply to comment #5) > I think the #ifdef and #endif should be flush left. I think this occurs in a > couple of places. I can do that on checkin. (Personally I find it more readable the way I have it, but I don't care all that much.) > More importantly are you planning to do auto angled markers as part of an angle > animation patch? Yes, I have a separate patch for that. I'm still fiddling with tests for the other patches that I have. I'll upload the others soon.
Comment 7•11 years ago
|
||
Comment on attachment 422246 [details] [diff] [review] patch to add support for animating enum attributes Similar to bug 540479 comment 2: - SMILEnumType::ComputeDistance probably shouldn't bother modifying its outparam - SMILEnumType.h possibly shouldn't have the "NS_" prefix on its #define (and should use mozilla_ instead)? - Nix whitespace-on-blank-line after GetAnimValue definition (in nsSVGEnum.h) And, +1 to Robert's note about #ifdef indentation in nsSVGEnum::SetBaseValueString & SetBaseValue. >+nsSVGEnum::SMILEnum::SetAnimValue(const nsSMILValue& aValue) >+{ >+ NS_ASSERTION(aValue.mType == &SMILEnumType::sSingleton, >+ "Unexpected type to assign animated value"); >+ if (aValue.mType == &SMILEnumType::sSingleton) { >+ mVal->SetAnimValue(PRUint16(aValue.mU.mUint), mSVGElement); So we're converting a 64-bit unsigned integer (aValue.mU.mUint) into a 16-bit unsigned integer. We should assert that we're not overflowing the 16-bit value there, because we should never be dealing with enumerated values that large (and if we end up with one, something's broken). So anyway, just before the conversion, we should add something like: NS_ABORT_IF_FALSE(aValue.mU.mUint <= USHRT_MAX, "out of bounds enumerated value"); >+void >+nsSVGFE::DidAnimateEnum(PRUint8 aAttrEnum) >+{ >+ // nsSVGLeafFrame doesn't implement AttributeChanged. >+ nsIFrame* frame = GetPrimaryFrame(); >+ if (frame) { >+ nsSVGEffects::InvalidateRenderingObservers(frame); >+ } >+} >+ So this gives us three functions with the exact same body. Bug 540479 adds a fourth, "DidAnimateBool". Can we avoid this duplication by refactoring this method body into a helper function that each of these methods can call? The same applies to nsSVGFilterElement::DidAnimateEnum, though that one's not as bad since it only has two of these duplicate-functions after this patch. Still, the same principle applies -- we should probably be using a helper method there, too. (It can be inline, if you want to avoid the overhead of making a function call.) >diff --git a/layout/reftests/svg/smil/anim-filter-filterUnits-01.svg b/layout/reftests/svg/smil/anim-filter-filterUnits-01.svg Both reftests have some very long lines, making them a bit hard to read. (longest line is 112 chars in both cases) Could you prettify the line-wrapping, ideally to < 80 chars? Also -- nit :) -- should these reftests be named "XXX-1.svg", rather than "XXX-01.svg"? I think the former is more common in mozilla tests, and in this case, it looks like there's only one version of each test, which makes the two-digit numbering seem excessive. (Maybe we'll branch off a few subtests later, but probably not 10 more...?) r=dholbert with the above addressed (or rebutted :))
Attachment #422246 -
Flags: review?(dholbert) → review+
![]() |
Assignee | |
Comment 8•11 years ago
|
||
Pushed: http://hg.mozilla.org/mozilla-central/rev/df15d308c851 http://hg.mozilla.org/mozilla-central/rev/03641989306e (In reply to comment #7) > Also -- nit :) -- should these reftests be named "XXX-1.svg", rather than > "XXX-01.svg"? I think the former is more common in mozilla tests, and in this > case, it looks like there's only one version of each test, which makes the > two-digit numbering seem excessive. (Maybe we'll branch off a few subtests > later, but probably not 10 more...?) The -xx convention is standard for the SVG reftests. For many there will be more than one tests in future (or that's my intention), and it seems better to me to start out with consistency rather than changing convention for some of them as we roll over the limit. It's only one extra character after all. :-)
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
![]() |
Assignee | |
Comment 9•11 years ago
|
||
s/more than one tests/more tests than can be encoded in one decimal digit/
You need to log in
before you can comment on or make changes to this bug.
Description
•