Attachment 362420 Details for Bug 194827 – Patch

[patch] Patch

bug-194827-20190219133609.patch (text/plain), 17.67 KB, created by Simon Fraser (smfr) on 2019-02-19 13:36:10 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Fraser (smfr)

Created: 2019-02-19 13:36:10 PST

Size: 17.67 KB

patch

obsolete

>Subversion Revision: 241729
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 7e86ea94c805f67c58c412f73b3330926e22f7d6..2ba78c6c01ff8678c7e82cc1e08bb0fef0c0b65c 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,50 @@
>+2019-02-19  Simon Fraser  <simon.fraser@apple.com>
>+
>+        REGRESSION (r238090): Toggling visibility on the <html> element can result in a blank web view
>+        https://bugs.webkit.org/show_bug.cgi?id=194827
>+        rdar://problem/47620594
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Incremental compositing updates, added in rr238090, use repaints as a trigger for re-evaluating
>+        layer configurations, since a repaint implies that a layer gains painted content. This is done
>+        via the call to setNeedsCompositingConfigurationUpdate() in RenderLayerBacking::setContentsNeedDisplay{InRect}.
>+        The RenderView's layer is opted out of this to avoid doing lots of redundant layer config recomputation
>+        for the root. The configuration state that matters here is whether the layer contains painted content,
>+        and therefore needs backing store; this is computed by RenderLayerBacking::isSimpleContainerCompositingLayer(),
>+        and feeds into GraphicsLayer::drawsContent().
>+
>+        However, if <html> starts as "visibility:hidden" or "opacity:0", as some sites do to hide incremental loading,
>+        then we'll fail to recompute 'drawsContent' for the root and leave the root with drawsContent=false, which
>+        causes RenderLayerBacking::setContentsNeedDisplay{InRect} to short-circuit, and then we paint nothing.
>+
>+        Ironically, 'drawsContent' doesn't actually save any backing store for the root, since it has no affect on
>+        the root tile caches; we always make tiles. So the simple fix here is to change RenderLayerBacking::isSimpleContainerCompositingLayer()
>+        to always return false for the RenderView's layer (the root).
>+        
>+        Testing this was tricky; ref testing doesn't work because we force repaint, and we normally skip
>+        properties of the root in layer tree dumps to hide WK1/WK2 differences. Therefore I had to add
>+        LAYER_TREE_INCLUDES_ROOT_LAYER_PROPERTIES and fix RenderLayerBacking::shouldDumpPropertyForLayer to
>+        respect it.
>+
>+        Test: compositing/visibility/root-visibility-toggle.html
>+
>+        * page/Frame.h:
>+        * platform/graphics/GraphicsLayer.cpp:
>+        (WebCore::GraphicsLayer::dumpProperties const):
>+        * platform/graphics/GraphicsLayerClient.h:
>+        (WebCore::GraphicsLayerClient::shouldDumpPropertyForLayer const):
>+        * rendering/RenderLayerBacking.cpp:
>+        (WebCore::RenderLayerBacking::isSimpleContainerCompositingLayer const):
>+        (WebCore::RenderLayerBacking::shouldDumpPropertyForLayer const):
>+        * rendering/RenderLayerBacking.h:
>+        * rendering/RenderLayerCompositor.cpp:
>+        (WebCore::RenderLayerCompositor::layerTreeAsText):
>+        * testing/Internals.cpp:
>+        (WebCore::toLayerTreeFlags):
>+        * testing/Internals.h:
>+        * testing/Internals.idl:
>+
> 2019-02-18  Eric Carlson  <eric.carlson@apple.com>
> 
>         Add MSE logging configuration
>diff --git a/Source/WebCore/page/Frame.h b/Source/WebCore/page/Frame.h
>index 74f6f4f03638c6b2a4736119153e5f34dbfa46a0..83dc4836ccc3253ae1885ca0c69182a3c4ef0646 100644
>--- a/Source/WebCore/page/Frame.h
>+++ b/Source/WebCore/page/Frame.h
>@@ -114,6 +114,7 @@ enum {
>     LayerTreeFlagsIncludeContentLayers = 1 << 5,
>     LayerTreeFlagsIncludeAcceleratesDrawing = 1 << 6,
>     LayerTreeFlagsIncludeBackingStoreAttached = 1 << 7,
>+    LayerTreeFlagsIncludeRootLayerProperties = 1 << 8,
> };
> typedef unsigned LayerTreeFlags;
> 
>diff --git a/Source/WebCore/platform/graphics/GraphicsLayer.cpp b/Source/WebCore/platform/graphics/GraphicsLayer.cpp
>index 08f013d0147a94352452ea2dd03e2c2d5e32711b..6b5cf2d3d9511f9f526d9bb93c2c7494f3aa17cf 100644
>--- a/Source/WebCore/platform/graphics/GraphicsLayer.cpp
>+++ b/Source/WebCore/platform/graphics/GraphicsLayer.cpp
>@@ -836,7 +836,7 @@ void GraphicsLayer::dumpProperties(TextStream& ts, LayerTreeAsTextBehavior behav
>     if (m_preserves3D)
>         ts << indent << "(preserves3D " << m_preserves3D << ")\n";
> 
>-    if (m_drawsContent && client().shouldDumpPropertyForLayer(this, "drawsContent"))
>+    if (m_drawsContent && client().shouldDumpPropertyForLayer(this, "drawsContent", behavior))
>         ts << indent << "(drawsContent " << m_drawsContent << ")\n";
> 
>     if (!m_contentsVisible)
>@@ -850,7 +850,7 @@ void GraphicsLayer::dumpProperties(TextStream& ts, LayerTreeAsTextBehavior behav
>         ts << indent << "(client " << static_cast<void*>(m_client) << ")\n";
>     }
> 
>-    if (m_backgroundColor.isValid() && client().shouldDumpPropertyForLayer(this, "backgroundColor"))
>+    if (m_backgroundColor.isValid() && client().shouldDumpPropertyForLayer(this, "backgroundColor", behavior))
>         ts << indent << "(backgroundColor " << m_backgroundColor.nameForRenderTreeAsText() << ")\n";
> 
>     if (behavior & LayerTreeAsTextIncludeAcceleratesDrawing && m_acceleratesDrawing)
>@@ -904,7 +904,7 @@ void GraphicsLayer::dumpProperties(TextStream& ts, LayerTreeAsTextBehavior behav
>         ts << ")\n";
>     }
> 
>-    if (behavior & LayerTreeAsTextIncludeRepaintRects && repaintRectMap().contains(this) && !repaintRectMap().get(this).isEmpty() && client().shouldDumpPropertyForLayer(this, "repaintRects")) {
>+    if (behavior & LayerTreeAsTextIncludeRepaintRects && repaintRectMap().contains(this) && !repaintRectMap().get(this).isEmpty() && client().shouldDumpPropertyForLayer(this, "repaintRects", behavior)) {
>         ts << indent << "(repaint rects\n";
>         for (size_t i = 0; i < repaintRectMap().get(this).size(); ++i) {
>             if (repaintRectMap().get(this)[i].isEmpty())
>diff --git a/Source/WebCore/platform/graphics/GraphicsLayerClient.h b/Source/WebCore/platform/graphics/GraphicsLayerClient.h
>index 1d0ceb1a158c977515fc8e837cf5c577a03b938f..63fd9edccfb55ed03c7d15a8078dea072456d6d0 100644
>--- a/Source/WebCore/platform/graphics/GraphicsLayerClient.h
>+++ b/Source/WebCore/platform/graphics/GraphicsLayerClient.h
>@@ -72,6 +72,7 @@ enum LayerTreeAsTextBehaviorFlags {
>     LayerTreeAsTextIncludePageOverlayLayers     = 1 << 6,
>     LayerTreeAsTextIncludeAcceleratesDrawing    = 1 << 7,
>     LayerTreeAsTextIncludeBackingStoreAttached  = 1 << 8,
>+    LayerTreeAsTextIncludeRootLayerProperties   = 1 << 9,
>     LayerTreeAsTextShowAll                      = 0xFFFF
> };
> typedef unsigned LayerTreeAsTextBehavior;
>@@ -123,7 +124,7 @@ public:
>     virtual bool isTrackingRepaints() const { return false; }
> 
>     virtual bool shouldSkipLayerInDump(const GraphicsLayer*, LayerTreeAsTextBehavior) const { return false; }
>-    virtual bool shouldDumpPropertyForLayer(const GraphicsLayer*, const char*) const { return true; }
>+    virtual bool shouldDumpPropertyForLayer(const GraphicsLayer*, const char*, LayerTreeAsTextBehavior) const { return true; }
> 
>     virtual bool shouldAggressivelyRetainTiles(const GraphicsLayer*) const { return false; }
>     virtual bool shouldTemporarilyRetainTileCohorts(const GraphicsLayer*) const { return true; }
>diff --git a/Source/WebCore/rendering/RenderLayerBacking.cpp b/Source/WebCore/rendering/RenderLayerBacking.cpp
>index 7d129b6c0a92397a3abf4be93cdfdc9217e46ed5..387a9d45ef6f88aa578bef06173469d12ae2d36d 100644
>--- a/Source/WebCore/rendering/RenderLayerBacking.cpp
>+++ b/Source/WebCore/rendering/RenderLayerBacking.cpp
>@@ -2099,6 +2099,9 @@ static bool isCompositedPlugin(RenderObject& renderer)
> // This is a useful optimization, because it allows us to avoid allocating backing store.
> bool RenderLayerBacking::isSimpleContainerCompositingLayer(PaintedContentsInfo& contentsInfo) const
> {
>+    if (m_owningLayer.isRenderViewLayer())
>+        return false;
>+
>     if (renderer().isRenderReplaced() && (!isCompositedPlugin(renderer()) || isRestartedPlugin(renderer())))
>         return false;
> 
>@@ -2114,29 +2117,6 @@ bool RenderLayerBacking::isSimpleContainerCompositingLayer(PaintedContentsInfo&
>     if (renderer().isDocumentElementRenderer() && m_owningLayer.isolatesCompositedBlending())
>         return false;
> 
>-    if (renderer().isRenderView()) {
>-        // Look to see if the root object has a non-simple background
>-        auto* rootObject = renderer().document().documentElement() ? renderer().document().documentElement()->renderer() : nullptr;
>-        if (!rootObject)
>-            return false;
>-        
>-        // Reject anything that has a border, a border-radius or outline,
>-        // or is not a simple background (no background, or solid color).
>-        if (hasPaintedBoxDecorationsOrBackgroundImage(rootObject->style()))
>-            return false;
>-        
>-        // Now look at the body's renderer.
>-        auto* body = renderer().document().body();
>-        if (!body)
>-            return false;
>-        auto* bodyRenderer = body->renderer();
>-        if (!bodyRenderer)
>-            return false;
>-        
>-        if (hasPaintedBoxDecorationsOrBackgroundImage(bodyRenderer->style()))
>-            return false;
>-    }
>-
>     return true;
> }
> 
>@@ -2724,11 +2704,11 @@ bool RenderLayerBacking::shouldSkipLayerInDump(const GraphicsLayer* layer, Layer
>     return m_isMainFrameRenderViewLayer && layer && layer == m_childContainmentLayer.get();
> }
> 
>-bool RenderLayerBacking::shouldDumpPropertyForLayer(const GraphicsLayer* layer, const char* propertyName) const
>+bool RenderLayerBacking::shouldDumpPropertyForLayer(const GraphicsLayer* layer, const char* propertyName, LayerTreeAsTextBehavior flags) const
> {
>     // For backwards compatibility with WebKit1 and other platforms,
>     // skip some properties on the root tile cache.
>-    if (m_isMainFrameRenderViewLayer && layer == m_graphicsLayer.get()) {
>+    if (m_isMainFrameRenderViewLayer && layer == m_graphicsLayer.get() && !(flags & LayerTreeAsTextIncludeRootLayerProperties)) {
>         if (!strcmp(propertyName, "drawsContent"))
>             return false;
> 
>diff --git a/Source/WebCore/rendering/RenderLayerBacking.h b/Source/WebCore/rendering/RenderLayerBacking.h
>index 10d1d772575e5ae39a7857af7aabbc23a6d35ee4..71ba37f2b83c4381d0b2f876578314329b904b29 100644
>--- a/Source/WebCore/rendering/RenderLayerBacking.h
>+++ b/Source/WebCore/rendering/RenderLayerBacking.h
>@@ -227,7 +227,7 @@ public:
> 
>     bool isTrackingRepaints() const override;
>     bool shouldSkipLayerInDump(const GraphicsLayer*, LayerTreeAsTextBehavior) const override;
>-    bool shouldDumpPropertyForLayer(const GraphicsLayer*, const char* propertyName) const override;
>+    bool shouldDumpPropertyForLayer(const GraphicsLayer*, const char* propertyName, LayerTreeAsTextBehavior) const override;
> 
>     bool shouldAggressivelyRetainTiles(const GraphicsLayer*) const override;
>     bool shouldTemporarilyRetainTileCohorts(const GraphicsLayer*) const override;
>diff --git a/Source/WebCore/rendering/RenderLayerCompositor.cpp b/Source/WebCore/rendering/RenderLayerCompositor.cpp
>index 19dab8a341386ccb840165664ff5f85570db983e..703457c70f754da71bdcfba161f35fbc2abcccc8 100644
>--- a/Source/WebCore/rendering/RenderLayerCompositor.cpp
>+++ b/Source/WebCore/rendering/RenderLayerCompositor.cpp
>@@ -1918,6 +1918,8 @@ String RenderLayerCompositor::layerTreeAsText(LayerTreeFlags flags)
>         layerTreeBehavior |= LayerTreeAsTextIncludeAcceleratesDrawing;
>     if (flags & LayerTreeFlagsIncludeBackingStoreAttached)
>         layerTreeBehavior |= LayerTreeAsTextIncludeBackingStoreAttached;
>+    if (flags & LayerTreeFlagsIncludeRootLayerProperties)
>+        layerTreeBehavior |= LayerTreeAsTextIncludeRootLayerProperties;
> 
>     // We skip dumping the scroll and clip layers to keep layerTreeAsText output
>     // similar between platforms.
>@@ -1925,7 +1927,7 @@ String RenderLayerCompositor::layerTreeAsText(LayerTreeFlags flags)
> 
>     // Dump an empty layer tree only if the only composited layer is the main frame's tiled backing,
>     // so that tests expecting us to drop out of accelerated compositing when there are no layers succeed.
>-    if (!hasContentCompositingLayers() && documentUsesTiledBacking() && !(layerTreeBehavior & LayerTreeAsTextIncludeTileCaches))
>+    if (!hasContentCompositingLayers() && documentUsesTiledBacking() && !(layerTreeBehavior & LayerTreeAsTextIncludeTileCaches) && !(layerTreeBehavior & LayerTreeAsTextIncludeRootLayerProperties))
>         layerTreeText = emptyString();
> 
>     // The true root layer is not included in the dump, so if we want to report
>diff --git a/Source/WebCore/testing/Internals.cpp b/Source/WebCore/testing/Internals.cpp
>index 8d57b7d6e0492cfcf0fb4062a7e9eb4aac52ffa4..edb3f6bf74a08aba136522f1c9ad3ca4b590c9dd 100644
>--- a/Source/WebCore/testing/Internals.cpp
>+++ b/Source/WebCore/testing/Internals.cpp
>@@ -2522,6 +2522,8 @@ static LayerTreeFlags toLayerTreeFlags(unsigned short flags)
>         layerTreeFlags |= LayerTreeFlagsIncludeAcceleratesDrawing;
>     if (flags & Internals::LAYER_TREE_INCLUDES_BACKING_STORE_ATTACHED)
>         layerTreeFlags |= LayerTreeFlagsIncludeBackingStoreAttached;
>+    if (flags & Internals::LAYER_TREE_INCLUDES_ROOT_LAYER_PROPERTIES)
>+        layerTreeFlags |= LayerTreeFlagsIncludeRootLayerProperties;
> 
>     return layerTreeFlags;
> }
>diff --git a/Source/WebCore/testing/Internals.h b/Source/WebCore/testing/Internals.h
>index 4e4d9d74a774658c8d086c6ec6c8341a07b86bd2..df84c6784fa346fe223dfcb951699d0bf7b8d362 100644
>--- a/Source/WebCore/testing/Internals.h
>+++ b/Source/WebCore/testing/Internals.h
>@@ -347,6 +347,7 @@ public:
>         LAYER_TREE_INCLUDES_CONTENT_LAYERS = 16,
>         LAYER_TREE_INCLUDES_ACCELERATES_DRAWING = 32,
>         LAYER_TREE_INCLUDES_BACKING_STORE_ATTACHED = 64,
>+        LAYER_TREE_INCLUDES_ROOT_LAYER_PROPERTIES = 128,
>     };
>     ExceptionOr<String> layerTreeAsText(Document&, unsigned short flags) const;
>     ExceptionOr<uint64_t> layerIDForElement(Element&);
>diff --git a/Source/WebCore/testing/Internals.idl b/Source/WebCore/testing/Internals.idl
>index 01399ec80ade258a1a19dfd3c6ff6db7697ef8ed..7ccdc77ac7b9f25e8df4a7ccf1211856b2de5d92 100644
>--- a/Source/WebCore/testing/Internals.idl
>+++ b/Source/WebCore/testing/Internals.idl
>@@ -372,6 +372,7 @@ enum CompositingPolicy {
>     const unsigned short LAYER_TREE_INCLUDES_CONTENT_LAYERS = 16;
>     const unsigned short LAYER_TREE_INCLUDES_ACCELERATES_DRAWING = 32;
>     const unsigned short LAYER_TREE_INCLUDES_BACKING_STORE_ATTACHED = 64;
>+    const unsigned short LAYER_TREE_INCLUDES_ROOT_LAYER_PROPERTIES = 128;
>     [MayThrowException] DOMString layerTreeAsText(Document document, optional unsigned short flags = 0);
> 
>     [MayThrowException] unsigned long long layerIDForElement(Element element);
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 6f970798a600efea9c763e0b9fdb901175f8ea86..fa978244a5954827eaaea0446fa38d324e07c227 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2019-02-19  Simon Fraser  <simon.fraser@apple.com>
>+
>+        REGRESSION (r238090): Toggling visibility on the <html> element can result in a blank web view
>+        https://bugs.webkit.org/show_bug.cgi?id=194827
>+        rdar://problem/47620594
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Test dumps layer tree with RenderLayerBacking::shouldDumpPropertyForLayer to show that the root has (drawsContent 1)
>+
>+        * compositing/visibility/root-visibility-toggle-expected.txt: Added.
>+        * compositing/visibility/root-visibility-toggle.html: Added.
>+        * platform/mac-wk1/compositing/visibility/root-visibility-toggle-expected.txt: Added.
>+
> 2019-02-18  Eric Carlson  <eric.carlson@apple.com>
> 
>         Add MSE logging configuration
>diff --git a/LayoutTests/compositing/visibility/root-visibility-toggle-expected.txt b/LayoutTests/compositing/visibility/root-visibility-toggle-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..9420fd15ce4fc09f7ea212d534a20e1b389b31c4
>--- /dev/null
>+++ b/LayoutTests/compositing/visibility/root-visibility-toggle-expected.txt
>@@ -0,0 +1,15 @@
>+This text should be visible.
>+
>+(GraphicsLayer
>+  (anchor 0.00 0.00)
>+  (bounds 800.00 600.00)
>+  (children 1
>+    (GraphicsLayer
>+      (bounds 800.00 600.00)
>+      (contentsOpaque 1)
>+      (drawsContent 1)
>+      (backgroundColor #FFFFFF)
>+    )
>+  )
>+)
>+
>diff --git a/LayoutTests/compositing/visibility/root-visibility-toggle.html b/LayoutTests/compositing/visibility/root-visibility-toggle.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..b57fe66c94f60ea4c50a18f5e6938a383e8097d1
>--- /dev/null
>+++ b/LayoutTests/compositing/visibility/root-visibility-toggle.html
>@@ -0,0 +1,34 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+    <style>
>+        html {
>+            visibility: hidden;
>+        }
>+    </style>
>+    <script>
>+        if (window.testRunner) {
>+            testRunner.dumpAsText();
>+            testRunner.waitUntilDone();
>+        }
>+
>+        function doTest()
>+        {
>+            requestAnimationFrame(() => {
>+                document.documentElement.style.visibility = 'visible';
>+
>+                if (window.internals)
>+                    document.getElementById('layers').textContent = window.internals.layerTreeAsText(document, internals.LAYER_TREE_INCLUDES_ROOT_LAYER_PROPERTIES);
>+
>+                if (window.testRunner)
>+                    testRunner.notifyDone();
>+            });
>+        }
>+        window.addEventListener('load', doTest, false);
>+    </script>
>+</head>
>+<body>
>+<p>This text should be visible.</p>
>+<pre id="layers"></pre>
>+</body>
>+</html>
>diff --git a/LayoutTests/platform/mac-wk1/compositing/visibility/root-visibility-toggle-expected.txt b/LayoutTests/platform/mac-wk1/compositing/visibility/root-visibility-toggle-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..db472d9bcdf3ea893168d808ecbb4cbba0c5e9f2
>--- /dev/null
>+++ b/LayoutTests/platform/mac-wk1/compositing/visibility/root-visibility-toggle-expected.txt
>@@ -0,0 +1,3 @@
>+This text should be visible.
>+
>+

Actions: View | Formatted Diff | Diff

Attachments on bug 194827: 362420