Closed
Bug 991545
Opened 10 years ago
Closed 10 years ago
SVGTransformableElement::GetAttributeChangeHint should use nsChangeHint_UpdatePostTransformOverflow
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: kip, Assigned: kip)
References
Details
Attachments
(1 file, 6 obsolete files)
9.49 KB,
patch
|
longsonr
:
review+
|
Details | Diff | Splinter Review |
Followup from Bug 984226 (Comment 27). SVGTransformableElement::GetAttributeChangeHint should use nsChangeHint_UpdatePostTransformOverflow when only the transform is changed to avoid extraneous calls to UpdateOverflow in OverflowChangedTracker::Flush.
Updated•10 years ago
|
Component: Layout → SVG
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → kgilbert
Assignee | ||
Comment 1•10 years ago
|
||
V1 fix for Bug 991545
Comment 2•10 years ago
|
||
It's not appropriate to use HasAttr with SVG code in general as attributes may exist via SMIL animation without having an underlying base attribute value set. nsSVGAnimateTransformList->IsExplicitlySet() will tell you whether we're transitioning from a value to no value i.e. it goes false when the element is no longer transformed. You may have to make DidAnimateTransformList take int32_t modType as its callers can tell whether there was a value before by calling IsExplicitlySet before they do any parsing. Be careful as there are two ways you can have animated transforms either animate motion or animating a transform so you'd need to check whether either was set.
Assignee | ||
Comment 3•10 years ago
|
||
Updated with feedback from Comment 2
Attachment #8401151 -
Attachment is obsolete: true
Attachment #8403007 -
Flags: review?(longsonr)
Comment 4•10 years ago
|
||
Comment on attachment 8403007 [details] [diff] [review] V2 fix for Bug 991545 >+ bool prevSet = mAnimateMotionTransform || (mTransforms && mTransforms->IsExplicitlySet()); mTransforms && mTransforms->IsExplicitlySet() doesn't change in this method but you call it twice. Only mAnimateMotionTransform changes. Cache the result and reuse it. > mAnimateMotionTransform = aMatrix ? new gfx::Matrix(*aMatrix) : nullptr; >- DidAnimateTransformList(); >+ bool nowSet = mAnimateMotionTransform || (mTransforms && mTransforms->IsExplicitlySet()); See above. > nsresult > nsSVGAnimatedTransformList::SetAnimValue(const SVGTransformList& aValue, > nsSVGElement *aElement) > { ... > } >- aElement->DidAnimateTransformList(); >+ bool nowSet = IsExplicitlySet() || aElement->GetAnimateMotionTransform(); IsExplicitlySet() must be true at this point. Optimise accordingly. > void > nsSVGAnimatedTransformList::ClearAnimValue(nsSVGElement *aElement) > { >+ bool prevSet = IsExplicitlySet() || aElement->GetAnimateMotionTransform(); aElement->GetAnimateMotionTransform() does not change. Don't call it twice, cache the result. > SVGAnimatedTransformList *domWrapper = > SVGAnimatedTransformList::GetDOMWrapperIfExists(this); > if (domWrapper) { > // When all animation ends, animVal simply mirrors baseVal, which may have > // a different number of items to the last active animated value. We must > // keep the length of our animVal's DOM wrapper list in sync, and again we > // must do that before touching mAnimVal. See comments above. > // > domWrapper->InternalAnimValListWillChangeLengthTo(mBaseVal.Length()); > } > mAnimVal = nullptr; >- aElement->DidAnimateTransformList(); >+ bool nowSet = IsExplicitlySet() || aElement->GetAnimateMotionTransform(); nsSVGAnimatedTransformList::ClearAnimValue cannot possibly result in a modType of nsIDOMMutationEvent::ADDITION. Optimise accordingly. >+ int32_t modType; >+ if (prevSet && !nowSet) { >+ modType = nsIDOMMutationEvent::REMOVAL; >+ } else if(!prevSet && nowSet) { >+ modType = nsIDOMMutationEvent::ADDITION; You'll never get here. See above. > void >-nsSVGElement::DidAnimateTransformList() >+nsSVGElement::DidAnimateTransformList(int32_t modType) Make this aModType > void DidAnimatePathSegList(); >- void DidAnimateTransformList(); >+ void DidAnimateTransformList(int32_t modType); aModType here too. r=longsonr with review comments addressed.
Attachment #8403007 -
Flags: review?(longsonr) → review+
Comment 5•10 years ago
|
||
Comment on attachment 8403007 [details] [diff] [review] V2 fix for Bug 991545 > nsresult rv = mAnimVal->CopyFrom(aValue); > if (NS_FAILED(rv)) { > // OOM. We clear the animation, and, importantly, ClearAnimValue() ensures > // that mAnimVal and its DOM wrapper (if any) will have the same length! > ClearAnimValue(aElement); One other thing is that here you're calling ClearAnimValue which will do a notification but you've already modified mAnimVal by this point so ClearAnimValue might not calculate the correct value for the before state. I'm not sure that actually matters though as I don't think you need the before state for ClearAnimValue per my previous comment.
Assignee | ||
Comment 6•10 years ago
|
||
Updated based on review in Comment 4 and Comment 5
Attachment #8403007 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
Corrected parameter variable name from modType to aModType
Attachment #8403470 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
Submitted patch to try: https://tbpl.mozilla.org/?tree=Try&rev=487f5e922c77
Assignee | ||
Comment 9•10 years ago
|
||
Would it be necessary to create reftests for this patch? If SVGTransformableElement::GetAttributeChangeHint is altered to simply return nsChangeHint_UpdatePostTransformOverflow without differentiating between nsIDOMMutationEvent::ADDITION and nsIDOMMutationEvent::MODIFICATION, it fails reftests in layout/svg/smil. It appears that these reftests will verify the functionality of this patch; however, they will not fail prior to applying this patch.
Flags: needinfo?(longsonr)
Comment 10•10 years ago
|
||
The thing I would worry about/test is the scenario where you have an attribute that exists and is also smil animated and then you remove the underlying attribute using a DOM removeAttribute. That might well give you a REMOVAL AttributeChanged (because generic content code doesn't account for attributes that only exist because of SMIL). That's the other code path through to AttributeChanged. Similarly with an attribute that only exists because of an active SMIL animation and then we call DOM setAttribute we'd get an ADDITION where we should really get a MODIFICATION.
Flags: needinfo?(longsonr)
Comment 11•10 years ago
|
||
On second thoughts the graphic doesn't need to be repainted after the comment 10 updates so we don't need any tests here. So I think this can land as is provided it passes try.
Assignee | ||
Comment 12•10 years ago
|
||
Re-submitted to try, stacked with the updated patch from Bug 984226: https://tbpl.mozilla.org/?tree=Try&rev=da4ca62a5f23 Once this try passes and the Bug 984226 patch lands, I will set checkin-needed on this bug.
Assignee | ||
Comment 13•10 years ago
|
||
Removed unused variable that caused a compiler warning. Re-submitted to try: https://tbpl.mozilla.org/?tree=Try&rev=a1b61d296e56
Attachment #8403497 -
Attachment is obsolete: true
Assignee | ||
Comment 14•10 years ago
|
||
Patch updated with a new include in nsSVGAnimatedTransformList.cpp, to fix the Linux Debug break reported in the try run.
Attachment #8406495 -
Attachment is obsolete: true
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8407022 [details] [diff] [review] V6 fix for Bug 991545,r=longsonr Submitted to try together with an updated patch for Bug 984226: https://tbpl.mozilla.org/?tree=Try&rev=1303db67e22e Once this passes try and Bug 984226 is landed, I will set checkin-needed on this bug.
Assignee | ||
Comment 16•10 years ago
|
||
> V6 fix for Bug 991545,r=longsonr
>
> Submitted to try together with an updated patch for Bug 984226:
>
> https://tbpl.mozilla.org/?tree=Try&rev=1303db67e22e
>
> Once this passes try and Bug 984226 is landed, I will set checkin-needed on
> this bug.
This try run failed some reftests:
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/content/svg/content/src/crashtests/539167-1.svg | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/dom/smil/crashtests/483584-2.svg | assertion count 4 is more than expected 0 assertions
It appears that in these cases, nsSVGAnimatedTransformList::IsExplicitlySet() was returning true due to nsSVGAnimatedTransformList::SetAnimValue() being passed an empty list. In this case, mIsAttrSet was causing nsSVGAnimatedTransformList::IsExplicitlySet() to return true, resulting in nsIDOMMutationEvent::MODIFICATION being returned by nsSVGAnimatedTransformList::SetAnimValue() when set to a non-empty list.
Replacing the IsExplicitlySet() calls with HasTransform() within nsSVGAnimatedTransformList::SetAnimValue() and nsSVGAnimatedTransformList::ClearAnimValue() appears to give the desired result and passes the reftests / crashtests that failed in the try run.
For the purpose of SVGTransformableElement::GetAttributeChangeHint(), we are interested in when a transform is actually set, rather than when the attribute is present. Would there by any negative side effects of using HasTransform() in this context?
Flags: needinfo?(longsonr)
Assignee | ||
Comment 17•10 years ago
|
||
Replaced the IsExplicitlySet() calls with HasTransform() within nsSVGAnimatedTransformList::SetAnimValue() and nsSVGAnimatedTransformList::ClearAnimValue() to fix the reftests / crashtests that failed in the try run. See Comment 16 for details.
Attachment #8407022 -
Attachment is obsolete: true
Attachment #8408394 -
Flags: review?(longsonr)
Assignee | ||
Comment 18•10 years ago
|
||
Comment on attachment 8408394 [details] [diff] [review] V7 fix for Bug 991545,r=longsonr Submitted to try: https://tbpl.mozilla.org/?tree=Try&rev=c78759cfc296
Updated•10 years ago
|
Attachment #8408394 -
Flags: review?(longsonr) → review+
Comment 19•10 years ago
|
||
using HasTransform seems OK to me.
Updated•10 years ago
|
Flags: needinfo?(longsonr)
Assignee | ||
Updated•10 years ago
|
Attachment #8408394 -
Attachment description: V7 fix for Bug 991545 → V7 fix for Bug 991545,r=longsonr
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 20•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/393f6dac510a
Keywords: checkin-needed
Comment 21•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/393f6dac510a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•