Changeset 241788 in webkit


Ignore:
Timestamp:
Feb 19, 2019 5:39:34 PM (5 years ago)
Author:
Simon Fraser
Message:

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 Antti Koivisto.

Source/WebCore:

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:

LayoutTests:

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.
Location:
trunk
Files:
4 added
11 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r241787 r241788  
     12019-02-19  Simon Fraser  <simon.fraser@apple.com>
     2
     3        REGRESSION (r238090): Toggling visibility on the <html> element can result in a blank web view
     4        https://bugs.webkit.org/show_bug.cgi?id=194827
     5        rdar://problem/47620594
     6
     7        Reviewed by Antti Koivisto.
     8
     9        Test dumps layer tree with RenderLayerBacking::shouldDumpPropertyForLayer to show that the root has (drawsContent 1)
     10
     11        * compositing/visibility/root-visibility-toggle-expected.txt: Added.
     12        * compositing/visibility/root-visibility-toggle.html: Added.
     13        * platform/mac-wk1/compositing/visibility/root-visibility-toggle-expected.txt: Added.
     14
    1152019-02-19  Joseph Pecoraro  <pecoraro@apple.com>
    216

  • trunk/Source/WebCore/ChangeLog

    r241780 r241788  
     12019-02-19  Simon Fraser  <simon.fraser@apple.com>
     2
     3        REGRESSION (r238090): Toggling visibility on the <html> element can result in a blank web view
     4        https://bugs.webkit.org/show_bug.cgi?id=194827
     5        rdar://problem/47620594
     6
     7        Reviewed by Antti Koivisto.
     8
     9        Incremental compositing updates, added in rr238090, use repaints as a trigger for re-evaluating
     10        layer configurations, since a repaint implies that a layer gains painted content. This is done
     11        via the call to setNeedsCompositingConfigurationUpdate() in RenderLayerBacking::setContentsNeedDisplay{InRect}.
     12        The RenderView's layer is opted out of this to avoid doing lots of redundant layer config recomputation
     13        for the root. The configuration state that matters here is whether the layer contains painted content,
     14        and therefore needs backing store; this is computed by RenderLayerBacking::isSimpleContainerCompositingLayer(),
     15        and feeds into GraphicsLayer::drawsContent().
     16
     17        However, if <html> starts as "visibility:hidden" or "opacity:0", as some sites do to hide incremental loading,
     18        then we'll fail to recompute 'drawsContent' for the root and leave the root with drawsContent=false, which
     19        causes RenderLayerBacking::setContentsNeedDisplay{InRect} to short-circuit, and then we paint nothing.
     20
     21        Ironically, 'drawsContent' doesn't actually save any backing store for the root, since it has no affect on
     22        the root tile caches; we always make tiles. So the simple fix here is to change RenderLayerBacking::isSimpleContainerCompositingLayer()
     23        to always return false for the RenderView's layer (the root).
     24       
     25        Testing this was tricky; ref testing doesn't work because we force repaint, and we normally skip
     26        properties of the root in layer tree dumps to hide WK1/WK2 differences. Therefore I had to add
     27        LAYER_TREE_INCLUDES_ROOT_LAYER_PROPERTIES and fix RenderLayerBacking::shouldDumpPropertyForLayer to
     28        respect it.
     29
     30        Test: compositing/visibility/root-visibility-toggle.html
     31
     32        * page/Frame.h:
     33        * platform/graphics/GraphicsLayer.cpp:
     34        (WebCore::GraphicsLayer::dumpProperties const):
     35        * platform/graphics/GraphicsLayerClient.h:
     36        (WebCore::GraphicsLayerClient::shouldDumpPropertyForLayer const):
     37        * rendering/RenderLayerBacking.cpp:
     38        (WebCore::RenderLayerBacking::isSimpleContainerCompositingLayer const):
     39        (WebCore::RenderLayerBacking::shouldDumpPropertyForLayer const):
     40        * rendering/RenderLayerBacking.h:
     41        * rendering/RenderLayerCompositor.cpp:
     42        (WebCore::RenderLayerCompositor::layerTreeAsText):
     43        * testing/Internals.cpp:
     44        (WebCore::toLayerTreeFlags):
     45        * testing/Internals.h:
     46        * testing/Internals.idl:
     47
    1482019-02-19  Ryosuke Niwa  <rniwa@webkit.org>
    249

  • trunk/Source/WebCore/page/Frame.h

    r241320 r241788  
    115115    LayerTreeFlagsIncludeAcceleratesDrawing = 1 << 6,
    116116    LayerTreeFlagsIncludeBackingStoreAttached = 1 << 7,
     117    LayerTreeFlagsIncludeRootLayerProperties = 1 << 8,
    117118};
    118119typedef unsigned LayerTreeFlags;

  • trunk/Source/WebCore/platform/graphics/GraphicsLayer.cpp

    r241183 r241788  
    837837        ts << indent << "(preserves3D " << m_preserves3D << ")\n";
    838838
    839     if (m_drawsContent && client().shouldDumpPropertyForLayer(this, "drawsContent"))
     839    if (m_drawsContent && client().shouldDumpPropertyForLayer(this, "drawsContent", behavior))
    840840        ts << indent << "(drawsContent " << m_drawsContent << ")\n";
    841841
    … …  
    851851    }
    852852
    853     if (m_backgroundColor.isValid() && client().shouldDumpPropertyForLayer(this, "backgroundColor"))
     853    if (m_backgroundColor.isValid() && client().shouldDumpPropertyForLayer(this, "backgroundColor", behavior))
    854854        ts << indent << "(backgroundColor " << m_backgroundColor.nameForRenderTreeAsText() << ")\n";
    855855
    … …  
    905905    }
    906906
    907     if (behavior & LayerTreeAsTextIncludeRepaintRects && repaintRectMap().contains(this) && !repaintRectMap().get(this).isEmpty() && client().shouldDumpPropertyForLayer(this, "repaintRects")) {
     907    if (behavior & LayerTreeAsTextIncludeRepaintRects && repaintRectMap().contains(this) && !repaintRectMap().get(this).isEmpty() && client().shouldDumpPropertyForLayer(this, "repaintRects", behavior)) {
    908908        ts << indent << "(repaint rects\n";
    909909        for (size_t i = 0; i < repaintRectMap().get(this).size(); ++i) {

  • trunk/Source/WebCore/platform/graphics/GraphicsLayerClient.h

    r240047 r241788  
    7373    LayerTreeAsTextIncludeAcceleratesDrawing    = 1 << 7,
    7474    LayerTreeAsTextIncludeBackingStoreAttached  = 1 << 8,
     75    LayerTreeAsTextIncludeRootLayerProperties   = 1 << 9,
    7576    LayerTreeAsTextShowAll                      = 0xFFFF
    7677};
    … …  
    124125
    125126    virtual bool shouldSkipLayerInDump(const GraphicsLayer*, LayerTreeAsTextBehavior) const { return false; }
    126     virtual bool shouldDumpPropertyForLayer(const GraphicsLayer*, const char*) const { return true; }
     127    virtual bool shouldDumpPropertyForLayer(const GraphicsLayer*, const char*, LayerTreeAsTextBehavior) const { return true; }
    127128
    128129    virtual bool shouldAggressivelyRetainTiles(const GraphicsLayer*) const { return false; }

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

    r240941 r241788  
    21002100bool RenderLayerBacking::isSimpleContainerCompositingLayer(PaintedContentsInfo& contentsInfo) const
    21012101{
     2102    if (m_owningLayer.isRenderViewLayer())
     2103        return false;
     2104
    21022105    if (renderer().isRenderReplaced() && (!isCompositedPlugin(renderer()) || isRestartedPlugin(renderer())))
    21032106        return false;
    … …  
    21142117    if (renderer().isDocumentElementRenderer() && m_owningLayer.isolatesCompositedBlending())
    21152118        return false;
    2116 
    2117     if (renderer().isRenderView()) {
    2118         // Look to see if the root object has a non-simple background
    2119         auto* rootObject = renderer().document().documentElement() ? renderer().document().documentElement()->renderer() : nullptr;
    2120         if (!rootObject)
    2121             return false;
    2122        
    2123         // Reject anything that has a border, a border-radius or outline,
    2124         // or is not a simple background (no background, or solid color).
    2125         if (hasPaintedBoxDecorationsOrBackgroundImage(rootObject->style()))
    2126             return false;
    2127        
    2128         // Now look at the body's renderer.
    2129         auto* body = renderer().document().body();
    2130         if (!body)
    2131             return false;
    2132         auto* bodyRenderer = body->renderer();
    2133         if (!bodyRenderer)
    2134             return false;
    2135        
    2136         if (hasPaintedBoxDecorationsOrBackgroundImage(bodyRenderer->style()))
    2137             return false;
    2138     }
    21392119
    21402120    return true;
    … …  
    27252705}
    27262706
    2727 bool RenderLayerBacking::shouldDumpPropertyForLayer(const GraphicsLayer* layer, const char* propertyName) const
     2707bool RenderLayerBacking::shouldDumpPropertyForLayer(const GraphicsLayer* layer, const char* propertyName, LayerTreeAsTextBehavior flags) const
    27282708{
    27292709    // For backwards compatibility with WebKit1 and other platforms,
    27302710    // skip some properties on the root tile cache.
    2731     if (m_isMainFrameRenderViewLayer && layer == m_graphicsLayer.get()) {
     2711    if (m_isMainFrameRenderViewLayer && layer == m_graphicsLayer.get() && !(flags & LayerTreeAsTextIncludeRootLayerProperties)) {
    27322712        if (!strcmp(propertyName, "drawsContent"))
    27332713            return false;

  • trunk/Source/WebCore/rendering/RenderLayerBacking.h

    r240899 r241788  
    228228    bool isTrackingRepaints() const override;
    229229    bool shouldSkipLayerInDump(const GraphicsLayer*, LayerTreeAsTextBehavior) const override;
    230     bool shouldDumpPropertyForLayer(const GraphicsLayer*, const char* propertyName) const override;
     230    bool shouldDumpPropertyForLayer(const GraphicsLayer*, const char* propertyName, LayerTreeAsTextBehavior) const override;
    231231
    232232    bool shouldAggressivelyRetainTiles(const GraphicsLayer*) const override;

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

    r240940 r241788  
    19191919    if (flags & LayerTreeFlagsIncludeBackingStoreAttached)
    19201920        layerTreeBehavior |= LayerTreeAsTextIncludeBackingStoreAttached;
     1921    if (flags & LayerTreeFlagsIncludeRootLayerProperties)
     1922        layerTreeBehavior |= LayerTreeAsTextIncludeRootLayerProperties;
    19211923
    19221924    // We skip dumping the scroll and clip layers to keep layerTreeAsText output
    … …  
    19261928    // Dump an empty layer tree only if the only composited layer is the main frame's tiled backing,
    19271929    // so that tests expecting us to drop out of accelerated compositing when there are no layers succeed.
    1928     if (!hasContentCompositingLayers() && documentUsesTiledBacking() && !(layerTreeBehavior & LayerTreeAsTextIncludeTileCaches))
     1930    if (!hasContentCompositingLayers() && documentUsesTiledBacking() && !(layerTreeBehavior & LayerTreeAsTextIncludeTileCaches) && !(layerTreeBehavior & LayerTreeAsTextIncludeRootLayerProperties))
    19291931        layerTreeText = emptyString();
    19301932

  • trunk/Source/WebCore/testing/Internals.cpp

    r241761 r241788  
    25162516    if (flags & Internals::LAYER_TREE_INCLUDES_BACKING_STORE_ATTACHED)
    25172517        layerTreeFlags |= LayerTreeFlagsIncludeBackingStoreAttached;
     2518    if (flags & Internals::LAYER_TREE_INCLUDES_ROOT_LAYER_PROPERTIES)
     2519        layerTreeFlags |= LayerTreeFlagsIncludeRootLayerProperties;
    25182520
    25192521    return layerTreeFlags;

  • trunk/Source/WebCore/testing/Internals.h

    r241761 r241788  
    348348        LAYER_TREE_INCLUDES_ACCELERATES_DRAWING = 32,
    349349        LAYER_TREE_INCLUDES_BACKING_STORE_ATTACHED = 64,
     350        LAYER_TREE_INCLUDES_ROOT_LAYER_PROPERTIES = 128,
    350351    };
    351352    ExceptionOr<String> layerTreeAsText(Document&, unsigned short flags) const;

  • trunk/Source/WebCore/testing/Internals.idl

    r241761 r241788  
    373373    const unsigned short LAYER_TREE_INCLUDES_ACCELERATES_DRAWING = 32;
    374374    const unsigned short LAYER_TREE_INCLUDES_BACKING_STORE_ATTACHED = 64;
     375    const unsigned short LAYER_TREE_INCLUDES_ROOT_LAYER_PROPERTIES = 128;
    375376    [MayThrowException] DOMString layerTreeAsText(Document document, optional unsigned short flags = 0);
    376377
Note: See TracChangeset for help on using the changeset viewer.