[CSS Regions][Mac] fast/regions/full-screen-video-from-region.html hits an assertion...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Feb 2013 12:28:19 +0000 (12:28 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Feb 2013 12:28:19 +0000 (12:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=106075

Patch by Andrei Bucur <abucur@adobe.com> on 2013-02-15
Reviewed by Tony Chang.

Source/WebCore:

The crash is caused by two issues.

The first problem is how a block inside a flow thread determines if the children needs relayout or not.
When the region chain is invalidated, the information is lost so we need to return true, even for the
enclosing RenderFlowThread. Because the video renderer is the first child of the flow thread this doesn't
happen.

The patch implements this behaviour by inspecting both if the region chain has changed and
if the block has no range computed yet.

The second problem is RenderMedia not inheriting from RenderBlock. The logic of child relayout doesn't apply
to it. In the test case, when the full screen button is pressed, the region changes width to fill the viewport,
the chain is invalidated and the box info hash map is cleared. When the video is laid out again (after fixing
the first issue) it has the same size so the controls don't do a layout. They remain without box info inside
the flow thread, thus causing the assertion.

The patch forces the controls to relayout if the region chain was invalidated. We can't use the
logicalWidthChangedInRegions method because it is block specific. This will be fixed in a later patch.

Tests: No new tests. fast/regions/full-screen-video-from-region.html no longer crashes.

* rendering/RenderBlock.cpp:
(WebCore::RenderBlock::checkForPaginationLogicalHeightChange):
* rendering/RenderFlowThread.cpp:
(WebCore::RenderFlowThread::RenderFlowThread):
(WebCore::RenderFlowThread::layout):
(WebCore::RenderFlowThread::logicalWidthChangedInRegions):
* rendering/RenderFlowThread.h: Renamed pageLogicalHeightChanged to pageLogicalSizeChanged.
* rendering/RenderMedia.cpp:
(WebCore::RenderMedia::layout):

LayoutTests:

Removed the crash/fail expectation for fast/regions/full-screen-video-from-region.html.

* platform/mac/TestExpectations:

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

LayoutTests/ChangeLog
LayoutTests/platform/mac/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderBlock.cpp
Source/WebCore/rendering/RenderFlowThread.cpp
Source/WebCore/rendering/RenderFlowThread.h
Source/WebCore/rendering/RenderMedia.cpp

index 1a35e5cd8685d8478140b9648a592b3d64403a10..292aeb85927cd024bd2b2fec19694c6040d6364e 100644 (file)
@@ -1,3 +1,14 @@
+2013-02-15  Andrei Bucur  <abucur@adobe.com>
+
+        [CSS Regions][Mac] fast/regions/full-screen-video-from-region.html hits an assertion in RenderFlowThread::removeRenderBoxRegionInfo
+        https://bugs.webkit.org/show_bug.cgi?id=106075
+
+        Reviewed by Tony Chang.
+
+        Removed the crash/fail expectation for fast/regions/full-screen-video-from-region.html.
+
+        * platform/mac/TestExpectations:
+
 2013-02-15  Andrew Wilson  <atwilson@chromium.org>
 
         Unreviewed chromium expectation changes after r142931.
index 1f47c21078c918b7b20e58c4fd180c37324b3688..1f7bacf0ba3058e834a8b9279e01a9608b00a8c3 100644 (file)
@@ -1350,8 +1350,6 @@ webkit.org/b/105986 [ Debug ] transitions/interrupt-transform-transition.html [
 webkit.org/b/105999 [ Lion ] fast/canvas/canvas-composite-canvas.html [ Failure ]
 webkit.org/b/105999 [ Lion ] fast/canvas/canvas-composite-image.html [ Failure ]
 
-webkit.org/b/106075 [ Debug ] fast/regions/full-screen-video-from-region.html [ Crash ]
-
 webkit.org/b/106151 [ Lion ] http/tests/misc/link-rel-icon-beforeload.html [ Pass Failure ]
 
 webkit.org/b/106185 fast/frames/flattening/iframe-flattening-fixed-height.html [ Pass Failure ]
index 419f106aa1ce28f613a0c5be7b2e0b5b94571daf..65a2140e18e911b3985927858cfddf60d03d528c 100644 (file)
@@ -1,3 +1,41 @@
+2013-02-15  Andrei Bucur  <abucur@adobe.com>
+
+        [CSS Regions][Mac] fast/regions/full-screen-video-from-region.html hits an assertion in RenderFlowThread::removeRenderBoxRegionInfo
+        https://bugs.webkit.org/show_bug.cgi?id=106075
+
+        Reviewed by Tony Chang.
+
+        The crash is caused by two issues.
+
+        The first problem is how a block inside a flow thread determines if the children needs relayout or not.
+        When the region chain is invalidated, the information is lost so we need to return true, even for the
+        enclosing RenderFlowThread. Because the video renderer is the first child of the flow thread this doesn't
+        happen.
+
+        The patch implements this behaviour by inspecting both if the region chain has changed and
+        if the block has no range computed yet.
+
+        The second problem is RenderMedia not inheriting from RenderBlock. The logic of child relayout doesn't apply
+        to it. In the test case, when the full screen button is pressed, the region changes width to fill the viewport,
+        the chain is invalidated and the box info hash map is cleared. When the video is laid out again (after fixing
+        the first issue) it has the same size so the controls don't do a layout. They remain without box info inside
+        the flow thread, thus causing the assertion.
+
+        The patch forces the controls to relayout if the region chain was invalidated. We can't use the
+        logicalWidthChangedInRegions method because it is block specific. This will be fixed in a later patch.
+
+        Tests: No new tests. fast/regions/full-screen-video-from-region.html no longer crashes.
+
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::checkForPaginationLogicalHeightChange):
+        * rendering/RenderFlowThread.cpp:
+        (WebCore::RenderFlowThread::RenderFlowThread):
+        (WebCore::RenderFlowThread::layout):
+        (WebCore::RenderFlowThread::logicalWidthChangedInRegions):
+        * rendering/RenderFlowThread.h: Renamed pageLogicalHeightChanged to pageLogicalSizeChanged.
+        * rendering/RenderMedia.cpp:
+        (WebCore::RenderMedia::layout):
+
 2013-02-13  Allan Sandfeld Jensen  <allan.jensen@digia.com>
 
         [CoordGfx] Regression from r135212: big layers with transform animations sometime fail to render tiles
index 02926738beb56f6807d200ad54061062902ab9ab..56e6af975dd566adfe60d8e23f70a09304ac821f 100644 (file)
@@ -1473,7 +1473,7 @@ void RenderBlock::checkForPaginationLogicalHeightChange(LayoutUnit& pageLogicalH
         colInfo->setPaginationUnit(paginationUnit());
     } else if (isRenderFlowThread()) {
         pageLogicalHeight = 1; // This is just a hack to always make sure we have a page logical height.
-        pageLogicalHeightChanged = toRenderFlowThread(this)->pageLogicalHeightChanged();
+        pageLogicalHeightChanged = toRenderFlowThread(this)->pageLogicalSizeChanged();
     }
 }
 
index 305b2acff5d9e5780933797afb6351b23d8cf921..e9a16796634258c241acdb364b6d07a19f480a95 100644 (file)
@@ -56,7 +56,7 @@ RenderFlowThread::RenderFlowThread(Document* document)
     , m_overset(true)
     , m_hasRegionsWithStyling(false)
     , m_dispatchRegionLayoutUpdateEvent(false)
-    , m_pageLogicalHeightChanged(false)
+    , m_pageLogicalSizeChanged(false)
 {
     ASSERT(document->cssRegionsEnabled());
     setInRenderFlowThread();
@@ -136,7 +136,7 @@ void RenderFlowThread::layout()
 {
     StackStats::LayoutCheckPoint layoutCheckPoint;
 
-    m_pageLogicalHeightChanged = m_regionsInvalidated && everHadLayout();
+    m_pageLogicalSizeChanged = m_regionsInvalidated && everHadLayout();
     if (m_regionsInvalidated) {
         m_regionsInvalidated = false;
         m_regionsHaveUniformLogicalWidth = true;
@@ -187,7 +187,7 @@ void RenderFlowThread::layout()
     CurrentRenderFlowThreadMaintainer currentFlowThreadSetter(this);
     RenderBlock::layout();
 
-    m_pageLogicalHeightChanged = false;
+    m_pageLogicalSizeChanged = false;
 
     if (lastRegion())
         lastRegion()->expandToEncompassFlowThreadContentsIfNeeded();
@@ -460,17 +460,21 @@ void RenderFlowThread::removeRenderBoxRegionInfo(RenderBox* box)
 
 bool RenderFlowThread::logicalWidthChangedInRegions(const RenderBlock* block, LayoutUnit offsetFromLogicalTopOfFirstPage)
 {
-    if (!hasRegions() || block == this) // Not necessary, since if any region changes, we do a full pagination relayout anyway.
+    if (!hasRegions())
         return false;
 
     RenderRegion* startRegion;
     RenderRegion* endRegion;
     getRegionRangeForBox(block, startRegion, endRegion);
 
-    // If the block doesn't have a startRegion (and implicitly a region range) it's safe to assume the width in regions has changed (e.g. the region chain was invalidated).
-    if (!startRegion)
+    // When the region chain is invalidated the box information is discarded so we must assume the width has changed.
+    if (m_pageLogicalSizeChanged && !startRegion)
         return true;
 
+    // Not necessary for the flow thread, since we already computed the correct info for it.
+    if (block == this)
+        return false;
+
     for (RenderRegionList::iterator iter = m_regionList.find(startRegion); iter != m_regionList.end(); ++iter) {
         RenderRegion* region = *iter;
         ASSERT(!region->needsLayout());
index 31d7697a11a4d920e12682972810fa0afe2e1915..2f5f76e29ef79bad89bf64e59c992bd00ca5cb18 100644 (file)
@@ -133,7 +133,7 @@ public:
 
     bool addForcedRegionBreak(LayoutUnit, RenderObject* breakChild, bool isBefore, LayoutUnit* offsetBreakAdjustment = 0);
 
-    bool pageLogicalHeightChanged() const { return m_pageLogicalHeightChanged; }
+    bool pageLogicalSizeChanged() const { return m_pageLogicalSizeChanged; }
 
     bool hasAutoLogicalHeightRegions() const { ASSERT(isAutoLogicalHeightRegionsCountConsistent()); return m_autoLogicalHeightRegionsCount; }
     void incrementAutoLogicalHeightRegions();
@@ -204,7 +204,7 @@ protected:
     bool m_overset : 1;
     bool m_hasRegionsWithStyling : 1;
     bool m_dispatchRegionLayoutUpdateEvent : 1;
-    bool m_pageLogicalHeightChanged : 1;
+    bool m_pageLogicalSizeChanged : 1;
 };
 
 inline RenderFlowThread* toRenderFlowThread(RenderObject* object)
index 385fb5068f75d7ebc89b428df2b8e1678c648167..416965af44c53ef54b6645e56e7819dbb6606104 100644 (file)
@@ -29,6 +29,7 @@
 #include "RenderMedia.h"
 
 #include "HTMLMediaElement.h"
+#include "RenderFlowThread.h"
 #include "RenderView.h"
 
 namespace WebCore {
@@ -66,8 +67,18 @@ void RenderMedia::layout()
     if (!controlsRenderer)
         return;
 
+    bool controlsNeedLayout = controlsRenderer->needsLayout();
+    // If the region chain has changed we also need to relayout the controls to update the region box info.
+    // FIXME: We can do better once we compute region box info for RenderReplaced, not only for RenderBlock.
+    if (inRenderFlowThread() && !controlsNeedLayout) {
+        const RenderFlowThread* flowThread = enclosingRenderFlowThread();
+        ASSERT(flowThread);
+        if (flowThread->pageLogicalSizeChanged())
+            controlsNeedLayout = true;
+    }
+
     LayoutSize newSize = contentBoxRect().size();
-    if (newSize == oldSize && !controlsRenderer->needsLayout())
+    if (newSize == oldSize && !controlsNeedLayout)
         return;
 
     // When calling layout() on a child node, a parent must either push a LayoutStateMaintainter, or