Attachment 359288 Details for Bug 193290 – Patch

[patch] Patch

bug-193290-20190116120205.patch (text/plain), 17.14 KB, created by Simon Fraser (smfr) on 2019-01-16 12:02:05 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Fraser (smfr)

Created: 2019-01-16 12:02:05 PST

Size: 17.14 KB

patch

obsolete

>Subversion Revision: 240012
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 20d23be40beeed4dd869af05cdc652a2ac3fdc03..c88e19cfdcef9444e13a3dad681e395d77cb74f3 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,41 @@
>+2019-01-15  Simon Fraser  <simon.fraser@apple.com>
>+
>+        Make didCommitChangesForLayer() explicitly about the platform layer changing because of tile/non-tile swapping
>+        https://bugs.webkit.org/show_bug.cgi?id=193290
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        RenderLayerCompositor::didFlushChangesForLayer() triggers updates scrolling tree nodes for
>+        the flushed layer, but it's not clear what has changed at this point.
>+
>+        didCommitChangesForLayer()/didFlushChangesForLayer() were added to explicitly handle the
>+        case where the underlying platform layer for a GraphicsLayer changes because the layer swaps
>+        between tiled and non-tiled, and structural layer changes; we need to push the new layer to
>+        the scrolling tree because it operates on platform layers. So the only work that
>+        didFlushChangesForLayer() should do is to update layers on scrolling tree nodes; it doesn't
>+        need to do any geometry updating. Move towards that goal by renaming this callback to
>+        didChangePlatformLayerForLayer() to make its function more explicit.
>+
>+        * platform/graphics/GraphicsLayerClient.h:
>+        (WebCore::GraphicsLayerClient::didChangePlatformLayerForLayer):
>+        (WebCore::GraphicsLayerClient::didCommitChangesForLayer const): Deleted.
>+        * platform/graphics/ca/GraphicsLayerCA.cpp:
>+        (WebCore::GraphicsLayerCA::flushCompositingStateForThisLayerOnly):
>+        (WebCore::GraphicsLayerCA::recursiveCommitChanges):
>+        (WebCore::GraphicsLayerCA::commitLayerChangesBeforeSublayers):
>+        * platform/graphics/ca/GraphicsLayerCA.h:
>+        * rendering/RenderLayerBacking.cpp:
>+        (WebCore::RenderLayerBacking::didChangePlatformLayerForLayer):
>+        (WebCore::RenderLayerBacking::didCommitChangesForLayer const): Deleted.
>+        * rendering/RenderLayerBacking.h:
>+        * rendering/RenderLayerCompositor.cpp:
>+        (WebCore::RenderLayerCompositor::didChangePlatformLayerForLayer):
>+        (WebCore::LegacyWebKitScrollingLayerCoordinator::didChangePlatformLayerForLayer):
>+        (WebCore::RenderLayerCompositor::didFlushChangesForLayer): Deleted.
>+        (WebCore::RenderLayerCompositor::didCommitChangesForLayer const): Deleted.
>+        (WebCore::LegacyWebKitScrollingLayerCoordinator::didFlushChangesForLayer): Deleted.
>+        * rendering/RenderLayerCompositor.h:
>+
> 2019-01-15  Simon Fraser  <simon.fraser@apple.com>
> 
>         Animations should only trigger layer recomposite when necessary
>diff --git a/Source/WebCore/platform/graphics/GraphicsLayerClient.h b/Source/WebCore/platform/graphics/GraphicsLayerClient.h
>index 25068eb43f38db4e7811acf0d0b2d9e09572b53b..1d0ceb1a158c977515fc8e837cf5c577a03b938f 100644
>--- a/Source/WebCore/platform/graphics/GraphicsLayerClient.h
>+++ b/Source/WebCore/platform/graphics/GraphicsLayerClient.h
>@@ -101,7 +101,7 @@ public:
>     virtual void notifyFlushBeforeDisplayRefresh(const GraphicsLayer*) { }
> 
>     virtual void paintContents(const GraphicsLayer*, GraphicsContext&, GraphicsLayerPaintingPhase, const FloatRect& /* inClip */, GraphicsLayerPaintBehavior) { }
>-    virtual void didCommitChangesForLayer(const GraphicsLayer*) const { }
>+    virtual void didChangePlatformLayerForLayer(const GraphicsLayer*) { }
> 
>     // Provides current transform (taking transform-origin and animations into account). Input matrix has been
>     // initialized to identity already. Returns false if the layer has no transform.
>diff --git a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
>index e574f978509560ca03a67f827e9c3a18229d1eb4..e4f67cd1ab39d8d96dbbd60e6b32d0022650f3f1 100644
>--- a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
>+++ b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
>@@ -1271,16 +1271,16 @@ void GraphicsLayerCA::flushCompositingState(const FloatRect& visibleRect)
> void GraphicsLayerCA::flushCompositingStateForThisLayerOnly()
> {
>     float pageScaleFactor;
>-    bool hadChanges = m_uncommittedChanges;
>+    bool layerTypeChanged = false;
>     
>     CommitState commitState;
> 
>     FloatPoint offset = computePositionRelativeToBase(pageScaleFactor);
>-    commitLayerChangesBeforeSublayers(commitState, pageScaleFactor, offset);
>+    commitLayerChangesBeforeSublayers(commitState, pageScaleFactor, offset, layerTypeChanged);
>     commitLayerChangesAfterSublayers(commitState);
> 
>-    if (hadChanges)
>-        client().didCommitChangesForLayer(this);
>+    if (layerTypeChanged)
>+        client().didChangePlatformLayerForLayer(this);
> }
> 
> static inline bool accumulatesTransform(const GraphicsLayerCA& layer)
>@@ -1565,8 +1565,9 @@ void GraphicsLayerCA::recursiveCommitChanges(CommitState& commitState, const Tra
>         baseRelativePosition += m_position;
> 
>     bool wasRunningTransformAnimation = isRunningTransformAnimation();
>-
>-    commitLayerChangesBeforeSublayers(childCommitState, pageScaleFactor, baseRelativePosition);
>+    bool layerTypeChanged = false;
>+    
>+    commitLayerChangesBeforeSublayers(childCommitState, pageScaleFactor, baseRelativePosition, layerTypeChanged);
> 
>     bool nowRunningTransformAnimation = wasRunningTransformAnimation;
>     if (m_uncommittedChanges & AnimationChanged)
>@@ -1586,7 +1587,7 @@ void GraphicsLayerCA::recursiveCommitChanges(CommitState& commitState, const Tra
> 
>     if (GraphicsLayerCA* maskLayer = downcast<GraphicsLayerCA>(m_maskLayer.get())) {
>         maskLayer->setVisibleAndCoverageRects(rects, m_isViewportConstrained || commitState.ancestorIsViewportConstrained);
>-        maskLayer->commitLayerChangesBeforeSublayers(childCommitState, pageScaleFactor, baseRelativePosition);
>+        maskLayer->commitLayerChangesBeforeSublayers(childCommitState, pageScaleFactor, baseRelativePosition, layerTypeChanged);
>     }
> 
>     bool hasDescendantsWithRunningTransformAnimations = false;
>@@ -1616,8 +1617,8 @@ void GraphicsLayerCA::recursiveCommitChanges(CommitState& commitState, const Tra
>     if (affectedByTransformAnimation && m_layer->layerType() == PlatformCALayer::LayerTypeTiledBackingLayer)
>         client().notifyFlushBeforeDisplayRefresh(this);
> 
>-    if (hadChanges)
>-        client().didCommitChangesForLayer(this);
>+    if (layerTypeChanged)
>+        client().didChangePlatformLayerForLayer(this);
> 
>     if (usesDisplayListDrawing() && m_drawsContent && (!m_hasEverPainted || hadDirtyRects)) {
>         TraceScope tracingScope(DisplayListRecordStart, DisplayListRecordEnd);
>@@ -1712,7 +1713,7 @@ static bool isCustomBackdropLayerType(PlatformCALayer::LayerType layerType)
>     return layerType == PlatformCALayer::LayerTypeLightSystemBackdropLayer || layerType == PlatformCALayer::LayerTypeDarkSystemBackdropLayer;
> }
> 
>-void GraphicsLayerCA::commitLayerChangesBeforeSublayers(CommitState& commitState, float pageScaleFactor, const FloatPoint& positionRelativeToBase)
>+void GraphicsLayerCA::commitLayerChangesBeforeSublayers(CommitState& commitState, float pageScaleFactor, const FloatPoint& positionRelativeToBase, bool& layerChanged)
> {
>     SetForScope<bool> committingChangesChange(m_isCommittingChanges, true);
> 
>@@ -1739,12 +1740,16 @@ void GraphicsLayerCA::commitLayerChangesBeforeSublayers(CommitState& commitState
>     else if (currentLayerType == PlatformCALayer::LayerTypeTiledBackingLayer || isCustomBackdropLayerType(m_layer->layerType()))
>         neededLayerType = PlatformCALayer::LayerTypeWebLayer;
> 
>-    if (neededLayerType != m_layer->layerType())
>+    if (neededLayerType != m_layer->layerType()) {
>         changeLayerTypeTo(neededLayerType);
>+        layerChanged = true;
>+    }
> 
>     // Need to handle Preserves3DChanged first, because it affects which layers subsequent properties are applied to
>-    if (m_uncommittedChanges & (Preserves3DChanged | ReplicatedLayerChanged | BackdropFiltersChanged))
>-        updateStructuralLayer();
>+    if (m_uncommittedChanges & (Preserves3DChanged | ReplicatedLayerChanged | BackdropFiltersChanged)) {
>+        if (updateStructuralLayer())
>+            layerChanged = true;
>+    }
> 
>     if (m_uncommittedChanges & GeometryChanged)
>         updateGeometry(pageScaleFactor, positionRelativeToBase);
>@@ -2265,12 +2270,12 @@ void GraphicsLayerCA::updateWindRule()
>     m_layer->setShapeWindRule(m_shapeLayerWindRule);
> }
> 
>-void GraphicsLayerCA::updateStructuralLayer()
>+bool GraphicsLayerCA::updateStructuralLayer()
> {
>-    ensureStructuralLayer(structuralLayerPurpose());
>+    return ensureStructuralLayer(structuralLayerPurpose());
> }
> 
>-void GraphicsLayerCA::ensureStructuralLayer(StructuralLayerPurpose purpose)
>+bool GraphicsLayerCA::ensureStructuralLayer(StructuralLayerPurpose purpose)
> {
>     const LayerChangeFlags structuralLayerChangeFlags = NameChanged
>         | GeometryChanged
>@@ -2283,6 +2288,8 @@ void GraphicsLayerCA::ensureStructuralLayer(StructuralLayerPurpose purpose)
>         | MaskLayerChanged
>         | OpacityChanged;
> 
>+    bool structuralLayerChanged = false;
>+
>     if (purpose == NoStructuralLayer) {
>         if (m_structuralLayer) {
>             // Replace the transformLayer in the parent with this layer.
>@@ -2297,14 +2304,13 @@ void GraphicsLayerCA::ensureStructuralLayer(StructuralLayerPurpose purpose)
> 
>             // Release the structural layer.
>             m_structuralLayer = nullptr;
>+            structuralLayerChanged = true;
> 
>             addUncommittedChanges(structuralLayerChangeFlags);
>         }
>-        return;
>+        return structuralLayerChanged;
>     }
> 
>-    bool structuralLayerChanged = false;
>-    
>     if (purpose == StructuralLayerForPreserves3D) {
>         if (m_structuralLayer && m_structuralLayer->layerType() != PlatformCALayer::LayerTypeTransformLayer)
>             m_structuralLayer = nullptr;
>@@ -2324,7 +2330,7 @@ void GraphicsLayerCA::ensureStructuralLayer(StructuralLayerPurpose purpose)
>     }
>     
>     if (!structuralLayerChanged)
>-        return;
>+        return false;
>     
>     addUncommittedChanges(structuralLayerChangeFlags);
> 
>@@ -2349,6 +2355,7 @@ void GraphicsLayerCA::ensureStructuralLayer(StructuralLayerPurpose purpose)
>     }
> 
>     moveAnimations(m_layer.get(), m_structuralLayer.get());
>+    return true;
> }
> 
> GraphicsLayerCA::StructuralLayerPurpose GraphicsLayerCA::structuralLayerPurpose() const
>diff --git a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h
>index c17573ec18b312e26d4a12b057554bb3ff2f84d8..cc92f580e57549aa6f51e84556f34ebd4d052ecf 100644
>--- a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h
>+++ b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h
>@@ -277,7 +277,7 @@ private:
>         return m_animations && m_animations->runningAnimations.contains(animationName);
>     }
> 
>-    void commitLayerChangesBeforeSublayers(CommitState&, float pageScaleFactor, const FloatPoint& positionRelativeToBase);
>+    void commitLayerChangesBeforeSublayers(CommitState&, float pageScaleFactor, const FloatPoint& positionRelativeToBase, bool& layerTypeChanged);
>     void commitLayerChangesAfterSublayers(CommitState&);
> 
>     FloatPoint computePositionRelativeToBase(float& pageScale) const;
>@@ -402,7 +402,7 @@ private:
>     void updateContentsVisibility();
>     void updateContentsOpaque(float pageScaleFactor);
>     void updateBackfaceVisibility();
>-    void updateStructuralLayer();
>+    bool updateStructuralLayer();
>     void updateDrawsContent();
>     void updateCoverage(const CommitState&);
>     void updateBackgroundColor();
>@@ -443,7 +443,7 @@ private:
>         StructuralLayerForReplicaFlattening,
>         StructuralLayerForBackdrop
>     };
>-    void ensureStructuralLayer(StructuralLayerPurpose);
>+    bool ensureStructuralLayer(StructuralLayerPurpose);
>     StructuralLayerPurpose structuralLayerPurpose() const;
>     
>     void ensureLayerAnimations();
>diff --git a/Source/WebCore/rendering/RenderLayerBacking.cpp b/Source/WebCore/rendering/RenderLayerBacking.cpp
>index d573aa03673175aa49fc1f07e1a2cc78912195fc..b64f8f769012d716896d6cba4bb5c693648a1850 100644
>--- a/Source/WebCore/rendering/RenderLayerBacking.cpp
>+++ b/Source/WebCore/rendering/RenderLayerBacking.cpp
>@@ -2660,9 +2660,9 @@ bool RenderLayerBacking::paintsOpaquelyAtNonIntegralScales(const GraphicsLayer*)
>     return m_isMainFrameRenderViewLayer;
> }
> 
>-void RenderLayerBacking::didCommitChangesForLayer(const GraphicsLayer* layer) const
>+void RenderLayerBacking::didChangePlatformLayerForLayer(const GraphicsLayer* layer)
> {
>-    compositor().didFlushChangesForLayer(m_owningLayer, layer);
>+    compositor().didChangePlatformLayerForLayer(m_owningLayer, layer);
> }
> 
> bool RenderLayerBacking::getCurrentTransform(const GraphicsLayer* graphicsLayer, TransformationMatrix& transform) const
>diff --git a/Source/WebCore/rendering/RenderLayerBacking.h b/Source/WebCore/rendering/RenderLayerBacking.h
>index 8d5f43c4815d9d97ada4ed7c719b49297af86bcc..4b0301d38683fd69e5ce50495a34b01a51df0882 100644
>--- a/Source/WebCore/rendering/RenderLayerBacking.h
>+++ b/Source/WebCore/rendering/RenderLayerBacking.h
>@@ -208,7 +208,8 @@ public:
> 
>     float pageScaleFactor() const override;
>     float zoomedOutPageScaleFactor() const override;
>-    void didCommitChangesForLayer(const GraphicsLayer*) const override;
>+
>+    void didChangePlatformLayerForLayer(const GraphicsLayer*) override;
>     bool getCurrentTransform(const GraphicsLayer*, TransformationMatrix&) const override;
> 
>     bool isTrackingRepaints() const override;
>diff --git a/Source/WebCore/rendering/RenderLayerCompositor.cpp b/Source/WebCore/rendering/RenderLayerCompositor.cpp
>index 28de97774a38387877e8fda6f3c96d1bdb15e2bd..d0e87d6ddd714cd5410d9255d48962095e3296bb 100644
>--- a/Source/WebCore/rendering/RenderLayerCompositor.cpp
>+++ b/Source/WebCore/rendering/RenderLayerCompositor.cpp
>@@ -536,16 +536,17 @@ void RenderLayerCompositor::updateCustomLayersAfterFlush()
> #endif
> }
> 
>-void RenderLayerCompositor::didFlushChangesForLayer(RenderLayer& layer, const GraphicsLayer* graphicsLayer)
>+void RenderLayerCompositor::didChangePlatformLayerForLayer(RenderLayer& layer, const GraphicsLayer* graphicsLayer)
> {
>     if (m_scrollCoordinatedLayers.contains(&layer))
>         m_scrollCoordinatedLayersNeedingUpdate.add(&layer);
> 
> #if PLATFORM(IOS_FAMILY)
>     if (m_legacyScrollingLayerCoordinator)
>-        m_legacyScrollingLayerCoordinator->didFlushChangesForLayer(layer);
>+        m_legacyScrollingLayerCoordinator->didChangePlatformLayerForLayer(layer);
> #endif
> 
>+    // FIXME: right?
>     auto* backing = layer.backing();
>     if (backing->backgroundLayerPaintsFixedRootBackground() && graphicsLayer == backing->backgroundLayer())
>         fixedRootBackgroundLayerChanged();
>@@ -2985,11 +2986,6 @@ float RenderLayerCompositor::contentsScaleMultiplierForNewTiles(const GraphicsLa
> #endif
> }
> 
>-void RenderLayerCompositor::didCommitChangesForLayer(const GraphicsLayer*) const
>-{
>-    // Nothing to do here yet.
>-}
>-
> bool RenderLayerCompositor::documentUsesTiledBacking() const
> {
>     auto* layer = m_renderView.layer();
>@@ -4230,7 +4226,7 @@ void LegacyWebKitScrollingLayerCoordinator::removeScrollingLayer(RenderLayer& la
>     m_chromeClient.removeScrollingLayer(layer.renderer().element(), scrollingLayer, contentsLayer);
> }
> 
>-void LegacyWebKitScrollingLayerCoordinator::didFlushChangesForLayer(RenderLayer& layer)
>+void LegacyWebKitScrollingLayerCoordinator::didChangePlatformLayerForLayer(RenderLayer& layer)
> {
>     if (m_scrollingLayers.contains(&layer))
>         m_scrollingLayersNeedingUpdate.add(&layer);
>diff --git a/Source/WebCore/rendering/RenderLayerCompositor.h b/Source/WebCore/rendering/RenderLayerCompositor.h
>index 0f7a0878843db28f08f43da9707428cc4b7e05ae..9c9074dd2e6ea085ece350898469ff5c2f4a0acb 100644
>--- a/Source/WebCore/rendering/RenderLayerCompositor.h
>+++ b/Source/WebCore/rendering/RenderLayerCompositor.h
>@@ -107,7 +107,7 @@ public:
>     void addScrollingLayer(RenderLayer&);
>     void removeScrollingLayer(RenderLayer&, RenderLayerBacking&);
> 
>-    void didFlushChangesForLayer(RenderLayer&);
>+    void didChangePlatformLayerForLayer(RenderLayer&);
> 
> private:
>     void updateScrollingLayer(RenderLayer&);
>@@ -159,9 +159,9 @@ public:
>     // at specific times.
>     void scheduleLayerFlush(bool canThrottle);
>     void flushPendingLayerChanges(bool isFlushRoot = true);
>-    
>+
>     // Called when the GraphicsLayer for the given RenderLayer has flushed changes inside of flushPendingLayerChanges().
>-    void didFlushChangesForLayer(RenderLayer&, const GraphicsLayer*);
>+    void didChangePlatformLayerForLayer(RenderLayer&, const GraphicsLayer*);
> 
>     // Called when something outside WebKit affects the visible rect (e.g. delegated scrolling). Might schedule a layer flush.
>     void didChangeVisibleRect();
>@@ -289,8 +289,7 @@ public:
>     float contentsScaleMultiplierForNewTiles(const GraphicsLayer*) const override;
>     float pageScaleFactor() const override;
>     float zoomedOutPageScaleFactor() const override;
>-
>-    void didCommitChangesForLayer(const GraphicsLayer*) const override;
>+    void didChangePlatformLayerForLayer(const GraphicsLayer*) override { }
>     void notifyFlushBeforeDisplayRefresh(const GraphicsLayer*) override;
> 
>     void layerTiledBackingUsageChanged(const GraphicsLayer*, bool /*usingTiledBacking*/);

Flags:

thorton: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193290: 358718 | 358735 | 359242 | 359253 | 359288