Bug 1340257 - Part 1. Remove Assertion failure: mightHaveNoneSVGMask. r=heycam
authorcku <cku@mozilla.com>
Wed, 22 Feb 2017 15:56:53 +0800
changeset 373272 771cb7e75d7c8bb242dd39b93570ff82bc637f5b
parent 373271 d84d57475e82244ae621e7a2a2c609a5cbb45fdf
child 373273 ba444f145ea8d43ad70fe2b80a3a2c6dc999ac5e
push id10863
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 23:02:23 +0000
treeherdermozilla-aurora@0931190cd725 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1340257
milestone54.0a1
Bug 1340257 - Part 1. Remove Assertion failure: mightHaveNoneSVGMask. r=heycam After fighting with this assertion several months, I decided to remove it for two reasons: This assertion allows PreEffectBBoxProperty not being cached only under specific condition. But the condition is wider then we expect. 1. PreEffectsBBoxProperty is cached by nsIFrame::FinishAndStoreOverflow(this function calls ComputeEffectsRect which cache this property actually) and it is called from nsXXXFrame::Reflow on demand. Yes, *on demand*, not always. And this is the fist reason that why I think we should just remove this assertion. For example, nsBlockFrame::Reflow calls FinishAndStoreOverflow to store this property. But like BRFrame, it does not call FinishAndStoreOverflow at all. In anohter word, if you apply any SVG effect to a BRFrame, you will always hit this assertion. Here is an example: <br style="filter: saturate(0%);"/> So, if we still want to keep this assertion, we may need to create a list which list all frame types that cache PreEffectsBBoxProperty, and do this check only if the type of aFrame is listed. This is error prone since we may introduce a new frame type at any time and forget to update this table. 2. So, I think it's better just removing this assertion. The assertion that we really need is the next one(2nd one): MOZ_ASSERT(!preTransformOverflows, "GetVisualOverflowRect() won't return the pre-effects rect!"); Since hitting that assertion, the 2nd one, means caller will retrieve wrong effect region. Hitting the first assertion only means we do not cache PreEffectsBBoxProperty, it's pretty normal and not hurt anything. This is the second reason that I think we should remvoe this assertion. MozReview-Commit-ID: JfiYTiP2laG
layout/svg/nsSVGIntegrationUtils.cpp
--- a/layout/svg/nsSVGIntegrationUtils.cpp
+++ b/layout/svg/nsSVGIntegrationUtils.cpp
@@ -80,76 +80,30 @@ public:
 private:
 
   static nsRect GetPreEffectsVisualOverflowRect(nsIFrame* aFrame,
                                                 bool aCheckPropCache) {
     nsRect* r = aFrame->Properties().Get(nsIFrame::PreEffectsBBoxProperty());
     if (r) {
       return *r;
     }
-    // Despite the fact that we're invoked for frames with SVG effects applied,
-    // we can actually get here. All continuations and IB split siblings of a
-    // frame with SVG effects applied will have the PreEffectsBBoxProperty
-    // property set on them. Therefore, the frames that are passed to us will
-    // always have that property set...well, with one exception. If the frames
-    // for an element with SVG effects applied have been subject to an "IB
-    // split", then the block frame(s) that caused the split will have been
-    // wrapped in anonymous, inline-block, nsBlockFrames of pseudo-type
-    // nsCSSAnonBoxes::mozAnonymousBlock. These "IB split sibling" anonymous
-    // blocks will have the PreEffectsBBoxProperty property set on them, but
-    // they will never be passed to us. Instead, we'll be passed the block
-    // children that they wrap, which don't have the PreEffectsBBoxProperty
-    // property set on them. This is actually okay. What we care about is
-    // collecting the _pre_ effects visual overflow rects of the frames to
-    // which the SVG effects have been applied. Since the IB split results in
-    // any overflow rect adjustments for transforms, effects, etc. taking
-    // place on the anonymous block wrappers, the wrapped children are left
-    // with their overflow rects unaffected. In other words, calling
-    // GetVisualOverflowRect() on the children will return their pre-effects
-    // visual overflow rects, just as we need.
-    //
-    // A couple of tests that demonstrate the IB split and cause us to get here
-    // are:
-    //
-    //  * reftests/svg/svg-integration/clipPath-html-06.xhtml
-    //  * reftests/svg/svg-integration/clipPath-html-06-extref.xhtml
-    //
-    // If we ever got passed a frame with the PreTransformOverflowAreasProperty
-    // property set, that would be bad, since then our GetVisualOverflowRect()
-    // call would give us the post-effects, and post-transform, overflow rect.
-    //
-    // There is one more exceptions, in
-    // nsStyleImageLayers::Layer::CalcDifference, we do not add
-    // nsChangeHint_UpdateOverflow hint when image mask(not SVG mask) property
-    // value changed, since replace image mask does not cause layout change.
-    // So even if we apply a new mask image to this frame,
-    // PreEffectsBBoxProperty might still left empty.
+
 #ifdef DEBUG
-    if (aCheckPropCache) {
-      nsIFrame* firstFrame =
-        nsLayoutUtils::FirstContinuationOrIBSplitSibling(aFrame);
-      bool mightHaveNoneSVGMask =
-        nsSVGEffects::GetEffectProperties(firstFrame).MightHaveNoneSVGMask();
+    // Having PreTransformOverflowAreasProperty cached means
+    // GetVisualOverflowRect() will return post-effect rect, which is not what
+    // we want. This function intentional reports pre-effect rect. But it does
+    // not matter if there is no SVG effect on this frame, since no effect
+    // means post-effect rect matches pre-effect rect.
+    if (nsSVGIntegrationUtils::UsingEffectsForFrame(aFrame)) {
+      nsOverflowAreas* preTransformOverflows =
+        aFrame->Properties().Get(aFrame->PreTransformOverflowAreasProperty());
 
-      NS_ASSERTION(mightHaveNoneSVGMask ||
-                   aFrame->GetParent()->StyleContext()->GetPseudo() ==
-                   nsCSSAnonBoxes::mozAnonymousBlock,
-                   "How did we getting here, then?");
+      MOZ_ASSERT(!preTransformOverflows,
+                 "GetVisualOverflowRect() won't return the pre-effects rect!");
     }
-
-    bool hasEffect = nsSVGIntegrationUtils::UsingEffectsForFrame(aFrame);
-    nsOverflowAreas* preTransformOverflows =
-      aFrame->Properties().Get(aFrame->PreTransformOverflowAreasProperty());
-    // Having PreTransformOverflowAreasProperty means GetVisualOverflowRect()
-    // will return post-effect rect, which is not what we want, this function
-    // intentional reports pre-effect rect. But it does not matter if there is
-    // no SVG effect on this frame, since no effect means post-effect rect
-    // matches pre-effect rect.
-    MOZ_ASSERT(!hasEffect || !preTransformOverflows,
-               "GetVisualOverflowRect() won't return the pre-effects rect!");
 #endif
     return aFrame->GetVisualOverflowRect();
   }
 
   nsIFrame*     mFirstContinuation;
   nsIFrame*     mCurrentFrame;
   const nsRect& mCurrentFrameOverflowArea;
   nsRect        mResult;