Closed Bug 991545 Opened 6 years ago Closed 6 years ago

SVGTransformableElement::GetAttributeChangeHint should use nsChangeHint_UpdatePostTransformOverflow

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: kip, Assigned: kip)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
Component: Layout → SVG
Assignee: nobody → kgilbert
Attached patch bug991545.patch (obsolete) — Splinter Review
V1 fix for Bug 991545
Depends on: 984226
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.
Attached patch V2 fix for Bug 991545 (obsolete) — Splinter Review
Updated with feedback from Comment 2
Attachment #8401151 - Attachment is obsolete: true
Attachment #8403007 - Flags: review?(longsonr)
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 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.
Attached patch V3 fix for Bug 991545,r=longsonr (obsolete) — Splinter Review
Updated based on review in Comment 4 and Comment 5
Attachment #8403007 - Attachment is obsolete: true
Attached patch V4 fix for Bug 991545,r=longsonr (obsolete) — Splinter Review
Corrected parameter variable name from modType to aModType
Attachment #8403470 - Attachment is obsolete: true
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)
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)
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.
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.
Attached patch V5 fix for Bug 991545,r=longsonr (obsolete) — Splinter Review
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
Attached patch V6 fix for Bug 991545,r=longsonr (obsolete) — Splinter Review
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
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.
> 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)
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)
Attachment #8408394 - Flags: review?(longsonr) → review+
using HasTransform seems OK to me.
Flags: needinfo?(longsonr)
Attachment #8408394 - Attachment description: V7 fix for Bug 991545 → V7 fix for Bug 991545,r=longsonr
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/393f6dac510a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Depends on: 1255276
Depends on: 1373136
You need to log in before you can comment on or make changes to this bug.