[CSSRegions] Use RenderRegion::isValid() before using a region
authormihnea@adobe.com <mihnea@adobe.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 7 Apr 2014 06:38:03 +0000 (06:38 +0000)
committermihnea@adobe.com <mihnea@adobe.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 7 Apr 2014 06:38:03 +0000 (06:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=131232

Reviewed by Andreas Kling.

Source/WebCore:

RenderRegion method isValid() should be used to test whether a region
is good to use instead of a mix between isValid() and flowThread().
When the region is designed to fragment content from a parent flow thread,
the m_flowThread is not nullified anymore, thus ensuring the same treatment for all invalid
regions.
Covered by existing regions tests.

* inspector/InspectorOverlay.cpp:
(WebCore::buildObjectForElementInfo):
* rendering/RenderBox.cpp:
(WebCore::RenderBox::layoutOverflowRectForPropagation):
* rendering/RenderBoxModelObject.cpp:
(WebCore::RenderBoxModelObject::paintMaskForTextFillBox):
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::updateLayerPositions):
(WebCore::RenderLayer::paintLayer):
(WebCore::RenderLayer::hitTestLayer):
(WebCore::RenderLayer::calculateClipRects):
* rendering/RenderNamedFlowFragment.cpp:
(WebCore::RenderNamedFlowFragment::pageLogicalHeight):
(WebCore::RenderNamedFlowFragment::maxPageLogicalHeight):
* rendering/RenderNamedFlowThread.cpp:
(WebCore::RenderNamedFlowThread::getRanges):
(WebCore::RenderNamedFlowThread::clearRenderObjectCustomStyle):
(WebCore::RenderNamedFlowThread::checkRegionsWithStyling):
* rendering/RenderRegion.cpp:
(WebCore::RenderRegion::RenderRegion):
(WebCore::RenderRegion::positionForPoint):
(WebCore::RenderRegion::pageLogicalWidth):
(WebCore::RenderRegion::pageLogicalHeight):
(WebCore::RenderRegion::styleDidChange):
(WebCore::RenderRegion::installFlowThread):
(WebCore::RenderRegion::attachRegion):
(WebCore::RenderRegion::detachRegion):
(WebCore::RenderRegion::ensureOverflowForBox):
(WebCore::RenderRegion::renderBoxRegionInfo):

LayoutTests:

Adjust test expectation now that an invalid region is not unnecessary repainted.

* fast/regions/repaint/invalid-region-repaint-crash-expected.txt:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@166867 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/regions/repaint/invalid-region-repaint-crash-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/inspector/InspectorOverlay.cpp
Source/WebCore/rendering/RenderBox.cpp
Source/WebCore/rendering/RenderBoxModelObject.cpp
Source/WebCore/rendering/RenderLayer.cpp
Source/WebCore/rendering/RenderNamedFlowFragment.cpp
Source/WebCore/rendering/RenderNamedFlowThread.cpp
Source/WebCore/rendering/RenderRegion.cpp

index 916d74d..a4805f0 100644 (file)
@@ -1,3 +1,14 @@
+2014-04-06  Mihnea Ovidenie  <mihnea@adobe.com>
+
+        [CSSRegions] Use RenderRegion::isValid() before using a region
+        https://bugs.webkit.org/show_bug.cgi?id=131232
+
+        Reviewed by Andreas Kling.
+
+        Adjust test expectation now that an invalid region is not unnecessary repainted.
+
+        * fast/regions/repaint/invalid-region-repaint-crash-expected.txt:
+
 2014-04-06  Darin Adler  <darin@apple.com>
 
         Refactor post-attach and HTMLObjectElement-related code
index 03f6380..0d8581e 100644 (file)
@@ -13,9 +13,5 @@ On success it should not crash and you should see 3 rectangles painted in the fo
   (rect 100 100 50 50)
   (rect 100 100 50 100)
   (rect 100 100 50 50)
-  (rect 100 100 50 150)
-  (rect 100 100 50 100)
-  (rect 100 100 50 100)
-  (rect 100 100 50 50)
 )
 
index a34f8f4..59aacdd 100644 (file)
@@ -1,3 +1,47 @@
+2014-04-06  Mihnea Ovidenie  <mihnea@adobe.com>
+
+        [CSSRegions] Use RenderRegion::isValid() before using a region
+        https://bugs.webkit.org/show_bug.cgi?id=131232
+
+        Reviewed by Andreas Kling.
+
+        RenderRegion method isValid() should be used to test whether a region
+        is good to use instead of a mix between isValid() and flowThread().
+        When the region is designed to fragment content from a parent flow thread,
+        the m_flowThread is not nullified anymore, thus ensuring the same treatment for all invalid
+        regions.
+        Covered by existing regions tests.
+
+        * inspector/InspectorOverlay.cpp:
+        (WebCore::buildObjectForElementInfo):
+        * rendering/RenderBox.cpp:
+        (WebCore::RenderBox::layoutOverflowRectForPropagation):
+        * rendering/RenderBoxModelObject.cpp:
+        (WebCore::RenderBoxModelObject::paintMaskForTextFillBox):
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::updateLayerPositions):
+        (WebCore::RenderLayer::paintLayer):
+        (WebCore::RenderLayer::hitTestLayer):
+        (WebCore::RenderLayer::calculateClipRects):
+        * rendering/RenderNamedFlowFragment.cpp:
+        (WebCore::RenderNamedFlowFragment::pageLogicalHeight):
+        (WebCore::RenderNamedFlowFragment::maxPageLogicalHeight):
+        * rendering/RenderNamedFlowThread.cpp:
+        (WebCore::RenderNamedFlowThread::getRanges):
+        (WebCore::RenderNamedFlowThread::clearRenderObjectCustomStyle):
+        (WebCore::RenderNamedFlowThread::checkRegionsWithStyling):
+        * rendering/RenderRegion.cpp:
+        (WebCore::RenderRegion::RenderRegion):
+        (WebCore::RenderRegion::positionForPoint):
+        (WebCore::RenderRegion::pageLogicalWidth):
+        (WebCore::RenderRegion::pageLogicalHeight):
+        (WebCore::RenderRegion::styleDidChange):
+        (WebCore::RenderRegion::installFlowThread):
+        (WebCore::RenderRegion::attachRegion):
+        (WebCore::RenderRegion::detachRegion):
+        (WebCore::RenderRegion::ensureOverflowForBox):
+        (WebCore::RenderRegion::renderBoxRegionInfo):
+
 2014-04-06  Benjamin Poulain  <benjamin@webkit.org>
 
         Fix the debug bots after r166863
index 0c290ac..8b790da 100644 (file)
@@ -29,7 +29,6 @@
 #include "config.h"
 
 #if ENABLE(INSPECTOR)
-
 #include "InspectorOverlay.h"
 
 #include "DocumentLoader.h"
@@ -678,8 +677,9 @@ static PassRefPtr<InspectorObject> buildObjectForElementInfo(Node* node)
     
     if (renderer->isRenderNamedFlowFragmentContainer()) {
         RenderNamedFlowFragment* region = toRenderBlockFlow(renderer)->renderNamedFlowFragment();
-        RenderFlowThread* flowThread = region->flowThread();
-        if (flowThread && flowThread->isRenderNamedFlowThread()) {
+        if (region->isValid()) {
+            RenderFlowThread* flowThread = region->flowThread();
+            ASSERT(flowThread && flowThread->isRenderNamedFlowThread());
             RefPtr<InspectorObject> regionFlowInfo = InspectorObject::create();
             regionFlowInfo->setString("name", toRenderNamedFlowThread(flowThread)->flowThreadName());
             regionFlowInfo->setArray("regions", buildObjectForCSSRegionsHighlight(region, flowThread));
index 1178353..15489d5 100644 (file)
@@ -4553,13 +4553,12 @@ LayoutRect RenderBox::overflowRectForPaintRejection(RenderNamedFlowFragment* nam
     // cause the paint rejection algorithm to prevent them from painting when using different width regions.
     // e.g. an absolutely positioned box with bottom:0px and right:0px would have it's frameRect.x relative
     // to the flow thread, not the last region (in which it will end up because of bottom:0px)
-    if (namedFlowFragment) {
-        if (RenderFlowThread* flowThread = namedFlowFragment->flowThread()) {
-            RenderRegion* startRegion = nullptr;
-            RenderRegion* endRegion = nullptr;
-            if (flowThread->getRegionRangeForBox(this, startRegion, endRegion))
-                overflowRect.unite(namedFlowFragment->visualOverflowRectForBox(this));
-        }
+    if (namedFlowFragment && namedFlowFragment->isValid()) {
+        RenderFlowThread* flowThread = namedFlowFragment->flowThread();
+        RenderRegion* startRegion = nullptr;
+        RenderRegion* endRegion = nullptr;
+        if (flowThread->getRegionRangeForBox(this, startRegion, endRegion))
+            overflowRect.unite(namedFlowFragment->visualOverflowRectForBox(this));
     }
     
     if (!m_overflow || !usesCompositedScrolling())
index b65103b..3e6a2b4 100644 (file)
@@ -589,9 +589,8 @@ void RenderBoxModelObject::paintMaskForTextFillBox(ImageBuffer* maskImage, const
         box->paint(info, LayoutPoint(scrolledPaintRect.x() - box->x(), scrolledPaintRect.y() - box->y()), rootBox.lineTop(), rootBox.lineBottom());
     } else if (isRenderNamedFlowFragmentContainer()) {
         RenderNamedFlowFragment* region = toRenderBlockFlow(this)->renderNamedFlowFragment();
-        if (!region->flowThread())
-            return;
-        region->flowThread()->layer()->paintNamedFlowThreadInsideRegion(maskImageContext, region, maskRect, maskRect.location(), PaintBehaviorForceBlackText, RenderLayer::PaintLayerTemporaryClipRects);
+        if (region->isValid())
+            region->flowThread()->layer()->paintNamedFlowThreadInsideRegion(maskImageContext, region, maskRect, maskRect.location(), PaintBehaviorForceBlackText, RenderLayer::PaintLayerTemporaryClipRects);
     } else {
         LayoutSize localOffset = isBox() ? toRenderBox(this)->locationOffset() : LayoutSize();
         paint(info, scrolledPaintRect.location() - localOffset);
index 621bd68..7b12f4a 100644 (file)
@@ -439,7 +439,7 @@ void RenderLayer::updateLayerPositions(RenderGeometryMap* geometryMap, UpdateLay
                     // If we just repainted a region, we must also repaint the flow thread since it is the one
                     // doing the actual painting of the flowed content.
                     RenderNamedFlowFragment* region = toRenderBlockFlow(&renderer())->renderNamedFlowFragment();
-                    if (region->flowThread())
+                    if (region->isValid())
                         region->flowThread()->layer()->repaintIncludingDescendants();
                 }
             }
@@ -3743,10 +3743,8 @@ void RenderLayer::paintLayer(GraphicsContext* context, const LayerPaintingInfo&
         if (enclosingPaginationLayer())
             info.renderNamedFlowFragment = nullptr;
         else {
-            RenderFlowThread* flowThread = namedFlowFragment->flowThread();
-            ASSERT(flowThread);
-
-            if (!flowThread->objectShouldPaintInFlowRegion(&renderer(), namedFlowFragment))
+            ASSERT(namedFlowFragment->isValid());
+            if (!namedFlowFragment->flowThread()->objectShouldPaintInFlowRegion(&renderer(), namedFlowFragment))
                 return;
         }
     }
@@ -4820,9 +4818,8 @@ RenderLayer* RenderLayer::hitTestLayer(RenderLayer* rootLayer, RenderLayer* cont
     // This is true as long as we clamp the range of a box to its containing block range.
     // FIXME: Fix hit testing with in-flow threads included in out-of-flow threads.
     if (hitTestLocation.region()) {
+        ASSERT(hitTestLocation.region()->isValid());
         RenderFlowThread* flowThread = hitTestLocation.region()->flowThread();
-        ASSERT(flowThread);
-
         if (!flowThread->objectShouldPaintInFlowRegion(&renderer(), hitTestLocation.region()))
             return 0;
     }
@@ -5492,6 +5489,7 @@ void RenderLayer::calculateRects(const ClipRectsContext& clipRectsContext, const
 
     RenderFlowThread* flowThread = clipRectsContext.region ? clipRectsContext.region->flowThread() : 0;
     if (isSelfPaintingLayer() && flowThread && !renderer().isInFlowRenderFlowThread()) {
+        ASSERT(clipRectsContext.region->isValid());
         const RenderBoxModelObject& boxModelObject = toRenderBoxModelObject(renderer());
         LayoutRect layerBoundsWithVisualOverflow = clipRectsContext.region->visualOverflowRectForBox(&boxModelObject);
 
index c72a2f1..cd8dbb1 100644 (file)
@@ -166,7 +166,7 @@ void RenderNamedFlowFragment::updateLogicalHeight()
 
 LayoutUnit RenderNamedFlowFragment::pageLogicalHeight() const
 {
-    ASSERT(m_flowThread);
+    ASSERT(isValid());
     if (hasComputedAutoHeight() && m_flowThread->inMeasureContentLayoutPhase()) {
         ASSERT(hasAutoLogicalHeight());
         return computedAutoHeight();
@@ -178,7 +178,7 @@ LayoutUnit RenderNamedFlowFragment::pageLogicalHeight() const
 // height value for auto-height regions in the first layout phase of the parent named flow.
 LayoutUnit RenderNamedFlowFragment::maxPageLogicalHeight() const
 {
-    ASSERT(m_flowThread);
+    ASSERT(isValid());
     ASSERT(hasAutoLogicalHeight() && m_flowThread->inMeasureContentLayoutPhase());
     ASSERT(isAnonymous());
     ASSERT(parent());
index 84a1182..74b8d47 100644 (file)
@@ -725,7 +725,8 @@ void RenderNamedFlowThread::getRanges(Vector<RefPtr<Range>>& rangeObjects, const
 
             // start position
             if (logicalTopForRenderer < logicalTopForRegion && startsAboveRegion) {
-                if (renderer->isText()) { // Text crosses region top
+                if (renderer->isText()) {
+                    // Text crosses region top
                     // for Text elements, just find the last textbox that is contained inside the region and use its start() offset as start position
                     RenderText* textRenderer = toRenderText(renderer);
                     for (InlineTextBox* box = textRenderer->firstTextBox(); box; box = box->nextTextBox()) {
@@ -735,12 +736,14 @@ void RenderNamedFlowThread::getRanges(Vector<RefPtr<Range>>& rangeObjects, const
                         startsAboveRegion = false;
                         break;
                     }
-                } else { // node crosses region top
+                } else {
+                    // node crosses region top
                     // for all elements, except Text, just set the start position to be before their children
                     startsAboveRegion = true;
                     range->setStart(Position(node, Position::PositionIsBeforeChildren));
                 }
-            } else { // node starts inside region
+            } else {
+                // node starts inside region
                 // for elements that start inside the region, set the start position to be before them. If we found one, we will just skip the others until
                 // the range is closed.
                 if (startsAboveRegion) {
@@ -754,7 +757,8 @@ void RenderNamedFlowThread::getRanges(Vector<RefPtr<Range>>& rangeObjects, const
             // end position
             if (logicalBottomForRegion < logicalBottomForRenderer && (endsBelowRegion || (!endsBelowRegion && !node->isDescendantOf(lastEndNode)))) {
                 // for Text elements, just find just find the last textbox that is contained inside the region and use its start()+len() offset as end position
-                if (renderer->isText()) { // Text crosses region bottom
+                if (renderer->isText()) {
+                    // Text crosses region bottom
                     RenderText* textRenderer = toRenderText(renderer);
                     InlineTextBox* lastBox = 0;
                     for (InlineTextBox* box = textRenderer->firstTextBox(); box; box = box->nextTextBox()) {
@@ -769,13 +773,15 @@ void RenderNamedFlowThread::getRanges(Vector<RefPtr<Range>>& rangeObjects, const
                     }
                     endsBelowRegion = false;
                     lastEndNode = node;
-                } else { // node crosses region bottom
+                } else {
+                    // node crosses region bottom
                     // for all elements, except Text, just set the start position to be after their children
                     range->setEnd(Position(node, Position::PositionIsAfterChildren));
                     endsBelowRegion = true;
                     lastEndNode = node;
                 }
-            } else { // node ends inside region
+            } else {
+                // node ends inside region
                 // for elements that ends inside the region, set the end position to be after them
                 // allow this end position to be changed only by other elements that are not descendants of the current end node
                 if (endsBelowRegion || (!endsBelowRegion && !node->isDescendantOf(lastEndNode))) {
@@ -816,9 +822,8 @@ void RenderNamedFlowThread::clearRenderObjectCustomStyle(const RenderObject* obj
 {
     // Clear the styles for the object in the regions.
     // FIXME: Region styling is not computed only for the region range of the object so this is why we need to walk the whole chain.
-    for (auto& region : m_regionList) {
+    for (auto& region : m_regionList)
         toRenderNamedFlowFragment(region)->clearObjectStyleInRegion(object);
-    }
 }
 
 void RenderNamedFlowThread::removeFlowChildInfo(RenderObject* child)
index e0e7c8a..ee4f167 100644 (file)
@@ -51,7 +51,7 @@ namespace WebCore {
 RenderRegion::RenderRegion(Element& element, PassRef<RenderStyle> style, RenderFlowThread* flowThread)
     : RenderBlockFlow(element, std::move(style))
     , m_flowThread(flowThread)
-    , m_parentNamedFlowThread(0)
+    , m_parentNamedFlowThread(nullptr)
     , m_isValid(false)
 {
 }
@@ -59,7 +59,7 @@ RenderRegion::RenderRegion(Element& element, PassRef<RenderStyle> style, RenderF
 RenderRegion::RenderRegion(Document& document, PassRef<RenderStyle> style, RenderFlowThread* flowThread)
     : RenderBlockFlow(document, std::move(style))
     , m_flowThread(flowThread)
-    , m_parentNamedFlowThread(0)
+    , m_parentNamedFlowThread(nullptr)
     , m_isValid(false)
 {
 }
@@ -106,7 +106,6 @@ LayoutPoint RenderRegion::mapRegionPointIntoFlowThreadCoordinates(const LayoutPo
 
 VisiblePosition RenderRegion::positionForPoint(const LayoutPoint& point)
 {
-    ASSERT(m_flowThread);
     if (!isValid() || !m_flowThread->firstChild()) // checking for empty region blocks.
         return RenderBlock::positionForPoint(point);
 
@@ -115,13 +114,13 @@ VisiblePosition RenderRegion::positionForPoint(const LayoutPoint& point)
 
 LayoutUnit RenderRegion::pageLogicalWidth() const
 {
-    ASSERT(m_flowThread);
+    ASSERT(isValid());
     return m_flowThread->isHorizontalWritingMode() ? contentWidth() : contentHeight();
 }
 
 LayoutUnit RenderRegion::pageLogicalHeight() const
 {
-    ASSERT(m_flowThread);
+    ASSERT(isValid());
     return m_flowThread->isHorizontalWritingMode() ? contentHeight() : contentWidth();
 }
 
@@ -209,7 +208,7 @@ void RenderRegion::styleDidChange(StyleDifference diff, const RenderStyle* oldSt
 {
     RenderBlockFlow::styleDidChange(diff, oldStyle);
 
-    if (!m_flowThread)
+    if (!isValid())
         return;
 
     if (oldStyle && oldStyle->writingMode() != style().writingMode())
@@ -289,11 +288,6 @@ void RenderRegion::installFlowThread()
     }
 
     m_parentNamedFlowThread = &*closestFlowThreadAncestor;
-
-    // Do not take into account a region that links a flow with itself. The dependency
-    // cannot change, so it is not worth adding it to the list.
-    if (m_flowThread == m_parentNamedFlowThread)
-        m_flowThread = nullptr;
 }
 
 void RenderRegion::attachRegion()
@@ -309,7 +303,7 @@ void RenderRegion::attachRegion()
     // and we are attaching the region to the flow thread.
     installFlowThread();
     
-    if (!m_flowThread)
+    if (m_flowThread == m_parentNamedFlowThread)
         return;
 
     // Only after adding the region to the thread, the region is marked to be valid.
@@ -320,7 +314,7 @@ void RenderRegion::detachRegion()
 {
     if (m_flowThread)
         m_flowThread->removeRegionFromThread(this);
-    m_flowThread = 0;
+    m_flowThread = nullptr;
 }
 
 RenderBoxRegionInfo* RenderRegion::renderBoxRegionInfo(const RenderBox* box) const
@@ -436,9 +430,8 @@ void RenderRegion::adjustRegionBoundsFromFlowThreadPortionRect(const LayoutPoint
 
 void RenderRegion::ensureOverflowForBox(const RenderBox* box, RefPtr<RenderOverflow>& overflow, bool forceCreation)
 {
-    RenderFlowThread* flowThread = this->flowThread();
-    ASSERT(flowThread);
-    
+    ASSERT(isValid());
+
     RenderBoxRegionInfo* boxInfo = renderBoxRegionInfo(box);
     if (!boxInfo && !forceCreation)
         return;
@@ -450,7 +443,7 @@ void RenderRegion::ensureOverflowForBox(const RenderBox* box, RefPtr<RenderOverf
     
     LayoutRect borderBox = box->borderBoxRectInRegion(this);
     LayoutRect clientBox;
-    ASSERT(flowThread->objectShouldPaintInFlowRegion(box, this));
+    ASSERT(m_flowThread->objectShouldPaintInFlowRegion(box, this));
 
     if (!borderBox.isEmpty()) {
         borderBox = rectFlowPortionForBox(box, borderBox);
@@ -458,8 +451,8 @@ void RenderRegion::ensureOverflowForBox(const RenderBox* box, RefPtr<RenderOverf
         clientBox = box->clientBoxRectInRegion(this);
         clientBox = rectFlowPortionForBox(box, clientBox);
         
-        flowThread->flipForWritingModeLocalCoordinates(borderBox);
-        flowThread->flipForWritingModeLocalCoordinates(clientBox);
+        m_flowThread->flipForWritingModeLocalCoordinates(borderBox);
+        m_flowThread->flipForWritingModeLocalCoordinates(clientBox);
     }
 
     if (boxInfo) {