Changeset 166867 in webkit


Ignore:
Timestamp:
Apr 6, 2014 11:38:03 PM (10 years ago)
Author:
mihnea@adobe.com
Message:

[CSSRegions] Use RenderRegion::isValid() before using a region
​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:
Location:
trunk
Files:
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r166854 r166867  
     12014-04-06  Mihnea Ovidenie  <mihnea@adobe.com>
     2
     3        [CSSRegions] Use RenderRegion::isValid() before using a region
     4        https://bugs.webkit.org/show_bug.cgi?id=131232
     5
     6        Reviewed by Andreas Kling.
     7
     8        Adjust test expectation now that an invalid region is not unnecessary repainted.
     9
     10        * fast/regions/repaint/invalid-region-repaint-crash-expected.txt:
     11
    1122014-04-06  Darin Adler  <darin@apple.com>
    213

  • trunk/LayoutTests/fast/regions/repaint/invalid-region-repaint-crash-expected.txt

    r163021 r166867  
    1414  (rect 100 100 50 100)
    1515  (rect 100 100 50 50)
    16   (rect 100 100 50 150)
    17   (rect 100 100 50 100)
    18   (rect 100 100 50 100)
    19   (rect 100 100 50 50)
    2016)
    2117

  • trunk/Source/WebCore/ChangeLog

    r166865 r166867  
     12014-04-06  Mihnea Ovidenie  <mihnea@adobe.com>
     2
     3        [CSSRegions] Use RenderRegion::isValid() before using a region
     4        https://bugs.webkit.org/show_bug.cgi?id=131232
     5
     6        Reviewed by Andreas Kling.
     7
     8        RenderRegion method isValid() should be used to test whether a region
     9        is good to use instead of a mix between isValid() and flowThread().
     10        When the region is designed to fragment content from a parent flow thread,
     11        the m_flowThread is not nullified anymore, thus ensuring the same treatment for all invalid
     12        regions.
     13        Covered by existing regions tests.
     14
     15        * inspector/InspectorOverlay.cpp:
     16        (WebCore::buildObjectForElementInfo):
     17        * rendering/RenderBox.cpp:
     18        (WebCore::RenderBox::layoutOverflowRectForPropagation):
     19        * rendering/RenderBoxModelObject.cpp:
     20        (WebCore::RenderBoxModelObject::paintMaskForTextFillBox):
     21        * rendering/RenderLayer.cpp:
     22        (WebCore::RenderLayer::updateLayerPositions):
     23        (WebCore::RenderLayer::paintLayer):
     24        (WebCore::RenderLayer::hitTestLayer):
     25        (WebCore::RenderLayer::calculateClipRects):
     26        * rendering/RenderNamedFlowFragment.cpp:
     27        (WebCore::RenderNamedFlowFragment::pageLogicalHeight):
     28        (WebCore::RenderNamedFlowFragment::maxPageLogicalHeight):
     29        * rendering/RenderNamedFlowThread.cpp:
     30        (WebCore::RenderNamedFlowThread::getRanges):
     31        (WebCore::RenderNamedFlowThread::clearRenderObjectCustomStyle):
     32        (WebCore::RenderNamedFlowThread::checkRegionsWithStyling):
     33        * rendering/RenderRegion.cpp:
     34        (WebCore::RenderRegion::RenderRegion):
     35        (WebCore::RenderRegion::positionForPoint):
     36        (WebCore::RenderRegion::pageLogicalWidth):
     37        (WebCore::RenderRegion::pageLogicalHeight):
     38        (WebCore::RenderRegion::styleDidChange):
     39        (WebCore::RenderRegion::installFlowThread):
     40        (WebCore::RenderRegion::attachRegion):
     41        (WebCore::RenderRegion::detachRegion):
     42        (WebCore::RenderRegion::ensureOverflowForBox):
     43        (WebCore::RenderRegion::renderBoxRegionInfo):
     44
    1452014-04-06  Benjamin Poulain  <benjamin@webkit.org>
    246

  • trunk/Source/WebCore/inspector/InspectorOverlay.cpp

    r165890 r166867  
    3030
    3131#if ENABLE(INSPECTOR)
    32 
    3332#include "InspectorOverlay.h"
    3433
    … …  
    679678    if (renderer->isRenderNamedFlowFragmentContainer()) {
    680679        RenderNamedFlowFragment* region = toRenderBlockFlow(renderer)->renderNamedFlowFragment();
    681         RenderFlowThread* flowThread = region->flowThread();
    682         if (flowThread && flowThread->isRenderNamedFlowThread()) {
     680        if (region->isValid()) {
     681            RenderFlowThread* flowThread = region->flowThread();
     682            ASSERT(flowThread && flowThread->isRenderNamedFlowThread());
    683683            RefPtr<InspectorObject> regionFlowInfo = InspectorObject::create();
    684684            regionFlowInfo->setString("name", toRenderNamedFlowThread(flowThread)->flowThreadName());

  • trunk/Source/WebCore/rendering/RenderBox.cpp

    r166784 r166867  
    45544554    // e.g. an absolutely positioned box with bottom:0px and right:0px would have it's frameRect.x relative
    45554555    // to the flow thread, not the last region (in which it will end up because of bottom:0px)
    4556     if (namedFlowFragment) {
    4557         if (RenderFlowThread* flowThread = namedFlowFragment->flowThread()) {
    4558             RenderRegion* startRegion = nullptr;
    4559             RenderRegion* endRegion = nullptr;
    4560             if (flowThread->getRegionRangeForBox(this, startRegion, endRegion))
    4561                 overflowRect.unite(namedFlowFragment->visualOverflowRectForBox(this));
    4562         }
     4556    if (namedFlowFragment && namedFlowFragment->isValid()) {
     4557        RenderFlowThread* flowThread = namedFlowFragment->flowThread();
     4558        RenderRegion* startRegion = nullptr;
     4559        RenderRegion* endRegion = nullptr;
     4560        if (flowThread->getRegionRangeForBox(this, startRegion, endRegion))
     4561            overflowRect.unite(namedFlowFragment->visualOverflowRectForBox(this));
    45634562    }
    45644563   

  • trunk/Source/WebCore/rendering/RenderBoxModelObject.cpp

    r166784 r166867  
    590590    } else if (isRenderNamedFlowFragmentContainer()) {
    591591        RenderNamedFlowFragment* region = toRenderBlockFlow(this)->renderNamedFlowFragment();
    592         if (!region->flowThread())
    593             return;
    594         region->flowThread()->layer()->paintNamedFlowThreadInsideRegion(maskImageContext, region, maskRect, maskRect.location(), PaintBehaviorForceBlackText, RenderLayer::PaintLayerTemporaryClipRects);
     592        if (region->isValid())
     593            region->flowThread()->layer()->paintNamedFlowThreadInsideRegion(maskImageContext, region, maskRect, maskRect.location(), PaintBehaviorForceBlackText, RenderLayer::PaintLayerTemporaryClipRects);
    595594    } else {
    596595        LayoutSize localOffset = isBox() ? toRenderBox(this)->locationOffset() : LayoutSize();

  • trunk/Source/WebCore/rendering/RenderLayer.cpp

    r166641 r166867  
    440440                    // doing the actual painting of the flowed content.
    441441                    RenderNamedFlowFragment* region = toRenderBlockFlow(&renderer())->renderNamedFlowFragment();
    442                     if (region->flowThread())
     442                    if (region->isValid())
    443443                        region->flowThread()->layer()->repaintIncludingDescendants();
    444444                }
    … …  
    37443744            info.renderNamedFlowFragment = nullptr;
    37453745        else {
    3746             RenderFlowThread* flowThread = namedFlowFragment->flowThread();
    3747             ASSERT(flowThread);
    3748 
    3749             if (!flowThread->objectShouldPaintInFlowRegion(&renderer(), namedFlowFragment))
     3746            ASSERT(namedFlowFragment->isValid());
     3747            if (!namedFlowFragment->flowThread()->objectShouldPaintInFlowRegion(&renderer(), namedFlowFragment))
    37503748                return;
    37513749        }
    … …  
    48214819    // FIXME: Fix hit testing with in-flow threads included in out-of-flow threads.
    48224820    if (hitTestLocation.region()) {
     4821        ASSERT(hitTestLocation.region()->isValid());
    48234822        RenderFlowThread* flowThread = hitTestLocation.region()->flowThread();
    4824         ASSERT(flowThread);
    4825 
    48264823        if (!flowThread->objectShouldPaintInFlowRegion(&renderer(), hitTestLocation.region()))
    48274824            return 0;
    … …  
    54935490    RenderFlowThread* flowThread = clipRectsContext.region ? clipRectsContext.region->flowThread() : 0;
    54945491    if (isSelfPaintingLayer() && flowThread && !renderer().isInFlowRenderFlowThread()) {
     5492        ASSERT(clipRectsContext.region->isValid());
    54955493        const RenderBoxModelObject& boxModelObject = toRenderBoxModelObject(renderer());
    54965494        LayoutRect layerBoundsWithVisualOverflow = clipRectsContext.region->visualOverflowRectForBox(&boxModelObject);

  • trunk/Source/WebCore/rendering/RenderNamedFlowFragment.cpp

    r166781 r166867  
    167167LayoutUnit RenderNamedFlowFragment::pageLogicalHeight() const
    168168{
    169     ASSERT(m_flowThread);
     169    ASSERT(isValid());
    170170    if (hasComputedAutoHeight() && m_flowThread->inMeasureContentLayoutPhase()) {
    171171        ASSERT(hasAutoLogicalHeight());
    … …  
    179179LayoutUnit RenderNamedFlowFragment::maxPageLogicalHeight() const
    180180{
    181     ASSERT(m_flowThread);
     181    ASSERT(isValid());
    182182    ASSERT(hasAutoLogicalHeight() && m_flowThread->inMeasureContentLayoutPhase());
    183183    ASSERT(isAnonymous());

  • trunk/Source/WebCore/rendering/RenderNamedFlowThread.cpp

    r166781 r166867  
    726726            // start position
    727727            if (logicalTopForRenderer < logicalTopForRegion && startsAboveRegion) {
    728                 if (renderer->isText()) { // Text crosses region top
     728                if (renderer->isText()) {
     729                    // Text crosses region top
    729730                    // for Text elements, just find the last textbox that is contained inside the region and use its start() offset as start position
    730731                    RenderText* textRenderer = toRenderText(renderer);
    … …  
    736737                        break;
    737738                    }
    738                 } else { // node crosses region top
     739                } else {
     740                    // node crosses region top
    739741                    // for all elements, except Text, just set the start position to be before their children
    740742                    startsAboveRegion = true;
    741743                    range->setStart(Position(node, Position::PositionIsBeforeChildren));
    742744                }
    743             } else { // node starts inside region
     745            } else {
     746                // node starts inside region
    744747                // 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
    745748                // the range is closed.
    … …  
    755758            if (logicalBottomForRegion < logicalBottomForRenderer && (endsBelowRegion || (!endsBelowRegion && !node->isDescendantOf(lastEndNode)))) {
    756759                // 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
    757                 if (renderer->isText()) { // Text crosses region bottom
     760                if (renderer->isText()) {
     761                    // Text crosses region bottom
    758762                    RenderText* textRenderer = toRenderText(renderer);
    759763                    InlineTextBox* lastBox = 0;
    … …  
    770774                    endsBelowRegion = false;
    771775                    lastEndNode = node;
    772                 } else { // node crosses region bottom
     776                } else {
     777                    // node crosses region bottom
    773778                    // for all elements, except Text, just set the start position to be after their children
    774779                    range->setEnd(Position(node, Position::PositionIsAfterChildren));
    … …  
    776781                    lastEndNode = node;
    777782                }
    778             } else { // node ends inside region
     783            } else {
     784                // node ends inside region
    779785                // for elements that ends inside the region, set the end position to be after them
    780786                // allow this end position to be changed only by other elements that are not descendants of the current end node
    … …  
    817823    // Clear the styles for the object in the regions.
    818824    // 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.
    819     for (auto& region : m_regionList) {
     825    for (auto& region : m_regionList)
    820826        toRenderNamedFlowFragment(region)->clearObjectStyleInRegion(object);
    821     }
    822827}
    823828

  • trunk/Source/WebCore/rendering/RenderRegion.cpp

    r166489 r166867  
    5252    : RenderBlockFlow(element, std::move(style))
    5353    , m_flowThread(flowThread)
    54     , m_parentNamedFlowThread(0)
     54    , m_parentNamedFlowThread(nullptr)
    5555    , m_isValid(false)
    5656{
    … …  
    6060    : RenderBlockFlow(document, std::move(style))
    6161    , m_flowThread(flowThread)
    62     , m_parentNamedFlowThread(0)
     62    , m_parentNamedFlowThread(nullptr)
    6363    , m_isValid(false)
    6464{
    … …  
    107107VisiblePosition RenderRegion::positionForPoint(const LayoutPoint& point)
    108108{
    109     ASSERT(m_flowThread);
    110109    if (!isValid() || !m_flowThread->firstChild()) // checking for empty region blocks.
    111110        return RenderBlock::positionForPoint(point);
    … …  
    116115LayoutUnit RenderRegion::pageLogicalWidth() const
    117116{
    118     ASSERT(m_flowThread);
     117    ASSERT(isValid());
    119118    return m_flowThread->isHorizontalWritingMode() ? contentWidth() : contentHeight();
    120119}
    … …  
    122121LayoutUnit RenderRegion::pageLogicalHeight() const
    123122{
    124     ASSERT(m_flowThread);
     123    ASSERT(isValid());
    125124    return m_flowThread->isHorizontalWritingMode() ? contentHeight() : contentWidth();
    126125}
    … …  
    210209    RenderBlockFlow::styleDidChange(diff, oldStyle);
    211210
    212     if (!m_flowThread)
     211    if (!isValid())
    213212        return;
    214213
    … …  
    290289
    291290    m_parentNamedFlowThread = &*closestFlowThreadAncestor;
    292 
    293     // Do not take into account a region that links a flow with itself. The dependency
    294     // cannot change, so it is not worth adding it to the list.
    295     if (m_flowThread == m_parentNamedFlowThread)
    296         m_flowThread = nullptr;
    297291}
    298292
    … …  
    310304    installFlowThread();
    311305   
    312     if (!m_flowThread)
     306    if (m_flowThread == m_parentNamedFlowThread)
    313307        return;
    314308
    … …  
    321315    if (m_flowThread)
    322316        m_flowThread->removeRegionFromThread(this);
    323     m_flowThread = 0;
     317    m_flowThread = nullptr;
    324318}
    325319
    … …  
    437431void RenderRegion::ensureOverflowForBox(const RenderBox* box, RefPtr<RenderOverflow>& overflow, bool forceCreation)
    438432{
    439     RenderFlowThread* flowThread = this->flowThread();
    440     ASSERT(flowThread);
    441    
     433    ASSERT(isValid());
     434
    442435    RenderBoxRegionInfo* boxInfo = renderBoxRegionInfo(box);
    443436    if (!boxInfo && !forceCreation)
    … …  
    451444    LayoutRect borderBox = box->borderBoxRectInRegion(this);
    452445    LayoutRect clientBox;
    453     ASSERT(flowThread->objectShouldPaintInFlowRegion(box, this));
     446    ASSERT(m_flowThread->objectShouldPaintInFlowRegion(box, this));
    454447
    455448    if (!borderBox.isEmpty()) {
    … …  
    459452        clientBox = rectFlowPortionForBox(box, clientBox);
    460453       
    461         flowThread->flipForWritingModeLocalCoordinates(borderBox);
    462         flowThread->flipForWritingModeLocalCoordinates(clientBox);
     454        m_flowThread->flipForWritingModeLocalCoordinates(borderBox);
     455        m_flowThread->flipForWritingModeLocalCoordinates(clientBox);
    463456    }
    464457
Note: See TracChangeset for help on using the changeset viewer.