Changeset 168791 in webkit


Ignore:
Timestamp:
May 13, 2014 11:33:48 PM (10 years ago)
Author:
abucur@adobe.com
Message:

[CSS Regions] Assertion failure in some cases with inline blocks
https://bugs.webkit.org/show_bug.cgi?id=132859

Reviewed by Mihnea Ovidenie.

Source/WebCore:
The patch hardens the conditions when the region range caches are
populated to avoid desynchronizations when objects move during layout.
This is true especially in the case of the boxes found inside
inline blocks, that get their range from the containing line.

There is a new function |computedRegionRangeForBox| that will always
return a region range for a box using a best effort algorithm. This should
be used only when there's no need to cache region information.

This change also allows better control over the lifecycle of the
|RenderBoxRegionInfo| objects stored on the regions. We can now iterate
over the full range of the box when cleaning up the region box info. The
same applies for the width change detection function.

Test: fast/regions/inline-block-shifted-region.html

  • rendering/RenderBlockLineLayout.cpp:

(WebCore::RenderBlockFlow::updateRegionForLine): Don't set the containing
region if the block doesn't have a range. The returned value would not
be correctly clamped.

  • rendering/RenderBox.cpp:

(WebCore::RenderBlock::hasRegionRangeInFlowThread):

  • rendering/RenderBox.h:
  • rendering/RenderFlowThread.cpp:

(WebCore::RenderFlowThread::removeRenderBoxRegionInfo): Iterate only over
the range of the box, not from the start of the region chain.
(WebCore::RenderFlowThread::logicalWidthChangedInRegionsForBlock): Same as
above.
(WebCore::RenderFlowThread::hasCachedRegionRangeForBox):
(WebCore::RenderFlowThread::getRegionRangeForBoxFromCachedInfo):
(WebCore::RenderFlowThread::getRegionRangeForBox):
(WebCore::RenderFlowThread::computedRegionRangeForBox): Best effort function
to determine the range of a box. It will always return something as long
as the flow thread has regions.
(WebCore::RenderFlowThread::objectShouldFragmentInFlowRegion): Use the new function
to determine the range.

  • rendering/RenderFlowThread.h:
  • rendering/RenderNamedFlowThread.cpp:

(WebCore::RenderNamedFlowThread::absoluteQuadsForBox): Use the new function to determine
the range.

LayoutTests:
Test that moving lines with inline blocks doesn't cause an assertion.

  • fast/regions/inline-block-shifted-region-expected.txt: Added.
  • fast/regions/inline-block-shifted-region.html: Added.
Location:
trunk
Files:
2 added
8 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r168778 r168791  
     12014-05-13  Andrei Bucur  <abucur@adobe.com>
     2
     3        [CSS Regions] Assertion failure in some cases with inline blocks
     4        https://bugs.webkit.org/show_bug.cgi?id=132859
     5
     6        Reviewed by Mihnea Ovidenie.
     7
     8        Test that moving lines with inline blocks doesn't cause an assertion.
     9
     10        * fast/regions/inline-block-shifted-region-expected.txt: Added.
     11        * fast/regions/inline-block-shifted-region.html: Added.
     12
    1132014-05-13  Hans Muller  <hmuller@adobe.com>
    214
  • trunk/Source/WebCore/ChangeLog

    r168779 r168791  
     12014-05-13  Andrei Bucur  <abucur@adobe.com>
     2
     3        [CSS Regions] Assertion failure in some cases with inline blocks
     4        https://bugs.webkit.org/show_bug.cgi?id=132859
     5
     6        Reviewed by Mihnea Ovidenie.
     7
     8        The patch hardens the conditions when the region range caches are
     9        populated to avoid desynchronizations when objects move during layout.
     10        This is true especially in the case of the boxes found inside
     11        inline blocks, that get their range from the containing line.
     12
     13        There is a new function |computedRegionRangeForBox| that will always
     14        return a region range for a box using a best effort algorithm. This should
     15        be used only when there's no need to cache region information.
     16
     17        This change also allows better control over the lifecycle of the
     18        |RenderBoxRegionInfo| objects stored on the regions. We can now iterate
     19        over the full range of the box when cleaning up the region box info. The
     20        same applies for the width change detection function.
     21
     22        Test: fast/regions/inline-block-shifted-region.html
     23
     24        * rendering/RenderBlockLineLayout.cpp:
     25        (WebCore::RenderBlockFlow::updateRegionForLine): Don't set the containing
     26        region if the block doesn't have a range. The returned value would not
     27        be correctly clamped.
     28        * rendering/RenderBox.cpp:
     29        (WebCore::RenderBlock::hasRegionRangeInFlowThread):
     30        * rendering/RenderBox.h:
     31        * rendering/RenderFlowThread.cpp:
     32        (WebCore::RenderFlowThread::removeRenderBoxRegionInfo): Iterate only over
     33        the range of the box, not from the start of the region chain.
     34        (WebCore::RenderFlowThread::logicalWidthChangedInRegionsForBlock): Same as
     35        above.
     36        (WebCore::RenderFlowThread::hasCachedRegionRangeForBox):
     37        (WebCore::RenderFlowThread::getRegionRangeForBoxFromCachedInfo):
     38        (WebCore::RenderFlowThread::getRegionRangeForBox):
     39        (WebCore::RenderFlowThread::computedRegionRangeForBox): Best effort function
     40        to determine the range of a box. It will always return something as long
     41        as the flow thread has regions.
     42        (WebCore::RenderFlowThread::objectShouldFragmentInFlowRegion): Use the new function
     43        to determine the range.
     44        * rendering/RenderFlowThread.h:
     45        * rendering/RenderNamedFlowThread.cpp:
     46        (WebCore::RenderNamedFlowThread::absoluteQuadsForBox): Use the new function to determine
     47        the range.
     48
    1492014-05-13  Simon Fraser  <simon.fraser@apple.com>
    250
  • trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp

    r168750 r168791  
    19291929    ASSERT(lineBox);
    19301930
    1931     if (auto containingRegion = regionAtBlockOffset(lineBox->lineTopWithLeading()))
    1932         lineBox->setContainingRegion(*containingRegion);
    1933     else
     1931    if (!hasRegionRangeInFlowThread())
    19341932        lineBox->clearContainingRegion();
     1933    else {
     1934        if (auto containingRegion = regionAtBlockOffset(lineBox->lineTopWithLeading()))
     1935            lineBox->setContainingRegion(*containingRegion);
     1936        else
     1937            lineBox->clearContainingRegion();
     1938    }
    19351939
    19361940    RootInlineBox* prevLineBox = lineBox->prevRootBox();
  • trunk/Source/WebCore/rendering/RenderBox.cpp

    r168380 r168791  
    149149
    150150    return region;
     151}
     152
     153bool RenderBox::hasRegionRangeInFlowThread() const
     154{
     155    RenderFlowThread* flowThread = flowThreadContainingBlock();
     156    if (!flowThread || !flowThread->hasValidRegionInfo())
     157        return false;
     158
     159    return flowThread->hasCachedRegionRangeForBox(this);
    151160}
    152161
  • trunk/Source/WebCore/rendering/RenderBox.h

    r168380 r168791  
    384384    LayoutRect clientBoxRectInRegion(RenderRegion*) const;
    385385    RenderRegion* clampToStartAndEndRegions(RenderRegion*) const;
     386    bool hasRegionRangeInFlowThread() const;
    386387    virtual LayoutUnit offsetFromLogicalTopOfFirstPage() const;
    387388   
  • trunk/Source/WebCore/rendering/RenderFlowThread.cpp

    r168621 r168791  
    579579    RenderRegion* startRegion = nullptr;
    580580    RenderRegion* endRegion = nullptr;
    581     getRegionRangeForBox(box, startRegion, endRegion);
    582 
    583     for (auto& region : m_regionList) {
    584         region->removeRenderBoxRegionInfo(box);
    585         if (region == endRegion)
    586             break;
     581    if (getRegionRangeForBox(box, startRegion, endRegion)) {
     582        for (auto it = m_regionList.find(startRegion), end = m_regionList.end(); it != end; ++it) {
     583            RenderRegion* region = *it;
     584            region->removeRenderBoxRegionInfo(box);
     585            if (region == endRegion)
     586                break;
     587        }
    587588    }
    588589
     
    641642    RenderRegion* startRegion = nullptr;
    642643    RenderRegion* endRegion = nullptr;
    643     getRegionRangeForBox(block, startRegion, endRegion);
    644 
    645     for (auto& region : m_regionList) {
     644    if (!getRegionRangeForBox(block, startRegion, endRegion))
     645        return;
     646
     647    for (auto it = m_regionList.find(startRegion), end = m_regionList.end(); it != end; ++it) {
     648        RenderRegion* region = *it;
    646649        ASSERT(!region->needsLayout() || region->isRenderRegionSet());
    647650
     
    755758    ASSERT(box);
    756759
    757     if (m_regionRangeMap.contains(box))
    758         return true;
    759 
    760     InlineElementBox* boxWrapper = box->inlineBoxWrapper();
    761     if (boxWrapper && boxWrapper->root().containingRegion())
    762         return true;
    763 
    764     return false;
     760    return m_regionRangeMap.contains(box);
    765761}
    766762
     
    780776    }
    781777
    782     InlineElementBox* boxWrapper = box->inlineBoxWrapper();
    783     if (boxWrapper && boxWrapper->root().containingRegion()) {
    784         startRegion = endRegion = boxWrapper->root().containingRegion();
    785         ASSERT(m_regionList.contains(startRegion) && m_regionList.contains(endRegion));
    786         return true;
    787     }
    788 
    789778    return false;
    790779}
     
    806795        return true;
    807796
    808     // Check if the box is contained in an unsplittable box.
    809     // If the unsplittable box has region range, then the start and end region for the box
    810     // should be equal with the region for the unsplittable box if any.
     797    return false;
     798}
     799
     800bool RenderFlowThread::computedRegionRangeForBox(const RenderBox* box, RenderRegion*& startRegion, RenderRegion*& endRegion) const
     801{
     802    ASSERT(box);
     803
     804    startRegion = endRegion = nullptr;
     805    if (!hasValidRegionInfo()) // We clear the ranges when we invalidate the regions.
     806        return false;
     807
     808    if (getRegionRangeForBox(box, startRegion, endRegion))
     809        return true;
     810
     811    // Search the region range using the information provided by the
     812    // containing block chain.
    811813    RenderBox* cb = const_cast<RenderBox*>(box);
    812     RenderBox* cbToUse = nullptr;
    813     while (!cb->isRenderFlowThread() && !cbToUse) {
     814    while (!cb->isRenderFlowThread()) {
     815        InlineElementBox* boxWrapper = cb->inlineBoxWrapper();
     816        if (boxWrapper && boxWrapper->root().containingRegion()) {
     817            startRegion = endRegion = boxWrapper->root().containingRegion();
     818            ASSERT(m_regionList.contains(startRegion));
     819            return true;
     820        }
     821
    814822        // FIXME: Use the containingBlock() value once we patch all the layout systems to be region range aware
    815823        // (e.g. if we use containingBlock() the shadow controls of a video element won't get the range from the
     
    819827        ASSERT(cb);
    820828
    821         if (hasCachedRegionRangeForBox(cb))
    822             cbToUse = cb;
    823     }
    824 
    825     // If a box doesn't have a cached region range it usually means the box belongs to a line so startRegion should be equal with endRegion.
    826     // FIXME: Find the cases when this startRegion should not be equal with endRegion and make sure these boxes have cached region ranges.
    827     if (cbToUse) {
    828         startRegion = endRegion = const_cast<RenderFlowThread*>(this)->regionAtBlockOffset(cbToUse, box->offsetFromLogicalTopOfFirstPage(), true, DisallowRegionAutoGeneration);
    829         return true;
    830     }
    831 
     829        // If a box doesn't have a cached region range it usually means the box belongs to a line so startRegion should be equal with endRegion.
     830        // FIXME: Find the cases when this startRegion should not be equal with endRegion and make sure these boxes have cached region ranges.
     831        if (hasCachedRegionRangeForBox(cb)) {
     832            startRegion = endRegion = const_cast<RenderFlowThread*>(this)->regionAtBlockOffset(cb, box->offsetFromLogicalTopOfFirstPage(), true, DisallowRegionAutoGeneration);
     833            return true;
     834        }
     835    }
     836
     837    ASSERT_NOT_REACHED();
    832838    return false;
    833839}
     
    865871    // If the box has no range, do not check regionInRange. Boxes inside inlines do not get ranges.
    866872    // Instead, the containing RootInlineBox will abort when trying to paint inside the wrong region.
    867     if (getRegionRangeForBox(enclosingBox, enclosingBoxStartRegion, enclosingBoxEndRegion)
     873    if (computedRegionRangeForBox(enclosingBox, enclosingBoxStartRegion, enclosingBoxEndRegion)
    868874        && !regionInRange(region, enclosingBoxStartRegion, enclosingBoxEndRegion))
    869875        return false;
  • trunk/Source/WebCore/rendering/RenderFlowThread.h

    r168621 r168791  
    144144    virtual void setRegionRangeForBox(const RenderBox*, RenderRegion*, RenderRegion*);
    145145    bool getRegionRangeForBox(const RenderBox*, RenderRegion*& startRegion, RenderRegion*& endRegion) const;
     146    bool computedRegionRangeForBox(const RenderBox*, RenderRegion*& startRegion, RenderRegion*& endRegion) const;
    146147    bool hasCachedRegionRangeForBox(const RenderBox*) const;
    147148
  • trunk/Source/WebCore/rendering/RenderNamedFlowThread.cpp

    r168306 r168791  
    821821    RenderRegion* endRegion = nullptr;
    822822    // If the box doesn't have a range, we don't know how it is fragmented so fallback to the default behaviour.
    823     if (!getRegionRangeForBox(renderer, startRegion, endRegion))
     823    if (!computedRegionRangeForBox(renderer, startRegion, endRegion))
    824824        return false;
    825825
Note: See TracChangeset for help on using the changeset viewer.