Attachment 342577 Details for Bug 186541 – Patch

[patch] Patch

bug-186541-20180612122438.patch (text/plain), 19.56 KB, created by Tim Horton on 2018-06-12 12:24:38 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Tim Horton

Created: 2018-06-12 12:24:38 PDT

Size: 19.56 KB

patch

obsolete

>Subversion Revision: 232735
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 317bf0d58d1a295774d1e4a7385eab2aa07f02ee..01bbc774b02dc570950ad6ef8c171c0c5d33da59 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,64 @@
>+2018-06-12  Tim Horton  <timothy_horton@apple.com>
>+
>+        Unpainted area while scrolling in Reader is white
>+        https://bugs.webkit.org/show_bug.cgi?id=186541
>+        <rdar://problem/40471363>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        New test: tiled-drawing/simple-document-with-dynamic-background-color.html
>+
>+        For platforms that do not use the overhang layer, we depend on
>+        RenderView's background color to fill unpainted space.
>+
>+        RenderView's background color is only updated inside updateRootLayerConfiguration,
>+        and it is possible with a simple enough page to change the document's
>+        background color without running that code.
>+
>+        * page/FrameView.cpp:
>+        (WebCore::FrameView::setTransparent):
>+        (WebCore::FrameView::setBaseBackgroundColor):
>+        Make use of the newly added rootBackgroundColorOrTransparencyChanged.
>+
>+        (WebCore::FrameView::calculateExtendedBackgroundMode const):
>+        Update a comment, since the function it mentioned is no longer.
>+
>+        (WebCore::FrameView::updateTilesForExtendedBackgroundMode):
>+        Remove this code that clears the root extended background color
>+        if using tiles to extend in both directions. Two reasons:
>+        1) it seems harmless to also have a root extended background color
>+        2) this just gets clobbered by the call in RenderView::paintBoxDecorations
>+
>+        * rendering/RenderLayerCompositor.cpp:
>+        (WebCore::RenderLayerCompositor::updateCompositingLayers):
>+        Add a bit that will do a updateConfiguration() on the root layer if no
>+        other work needs to be done, so that we can update the root layer's
>+        transparency or background color without doing a full layer rebuild.
>+
>+        (WebCore::RenderLayerCompositor::rootOrBodyStyleChanged):
>+        Make use of the newly added rootBackgroundColorOrTransparencyChanged.
>+
>+        (WebCore::RenderLayerCompositor::rootBackgroundColorOrTransparencyChanged):
>+        Change rootBackgroundTransparencyChanged to also cover color changes.
>+        Fold setRootExtendedBackgroundColor in here, and make use of
>+        setRootLayerConfigurationNeedsUpdate() instead of doing a full rebuild.
>+        Previously, we would bail if the transparency state hadn't changed;
>+        now, we'll also update the root layer's background color and the
>+        exposed-to-WebKit extended background color if they change too.
>+
>+        (WebCore::RenderLayerCompositor::rootBackgroundTransparencyChanged): Deleted.
>+        (WebCore::RenderLayerCompositor::setRootExtendedBackgroundColor): Deleted.
>+
>+        * rendering/RenderLayerCompositor.h:
>+        Add setRootLayerConfigurationNeedsUpdate, remove setRootExtendedBackgroundColor,
>+        and add both a bit indicating that the root layer configuration needs updating
>+        and the cached view background color to make the early return in
>+        rootBackgroundColorOrTransparencyChanged possible.
>+
>+        * rendering/RenderView.cpp:
>+        (WebCore::RenderView::paintBoxDecorations):
>+        Make use of the newly added rootBackgroundColorOrTransparencyChanged.
>+
> 2018-06-11  Tim Horton  <timothy_horton@apple.com>
> 
>         Link drag image is inconsistently unreadable in dark mode
>diff --git a/Source/WebCore/page/FrameView.cpp b/Source/WebCore/page/FrameView.cpp
>index 482747958e5b76fc90e10f88447f10f21cbd5c73..93af2d81593324593cb560a4b8bee18d37f1691d 100644
>--- a/Source/WebCore/page/FrameView.cpp
>+++ b/Source/WebCore/page/FrameView.cpp
>@@ -2895,7 +2895,7 @@ void FrameView::setTransparent(bool isTransparent)
>     if (!isViewForDocumentInFrame())
>         return;
> 
>-    renderView()->compositor().rootBackgroundTransparencyChanged();
>+    renderView()->compositor().rootBackgroundColorOrTransparencyChanged();
>     setNeedsLayout();
> }
> 
>@@ -2911,12 +2911,7 @@ Color FrameView::baseBackgroundColor() const
> 
> void FrameView::setBaseBackgroundColor(const Color& backgroundColor)
> {
>-    bool wasOpaque = m_baseBackgroundColor.isOpaque();
>-
>-    if (!backgroundColor.isValid())
>-        m_baseBackgroundColor = Color::white;
>-    else
>-        m_baseBackgroundColor = backgroundColor;
>+    m_baseBackgroundColor = backgroundColor.isValid() ? backgroundColor : Color::white;
> 
>     if (!isViewForDocumentInFrame())
>         return;
>@@ -2924,8 +2919,7 @@ void FrameView::setBaseBackgroundColor(const Color& backgroundColor)
>     recalculateScrollbarOverlayStyle();
>     setNeedsLayout();
> 
>-    if (m_baseBackgroundColor.isOpaque() != wasOpaque)
>-        renderView()->compositor().rootBackgroundTransparencyChanged();
>+    renderView()->compositor().rootBackgroundColorOrTransparencyChanged();
> }
> 
> void FrameView::updateBackgroundRecursively(const Color& backgroundColor, bool transparent)
>@@ -2969,7 +2963,7 @@ FrameView::ExtendedBackgroundMode FrameView::calculateExtendedBackgroundMode() c
> 
>     // Just because Settings::backgroundShouldExtendBeyondPage() is true does not necessarily mean
>     // that the background rect needs to be extended for painting. Simple backgrounds can be extended
>-    // just with RenderLayerCompositor::setRootExtendedBackgroundColor(). More complicated backgrounds,
>+    // just with RenderLayerCompositor's rootExtendedBackgroundColor. More complicated backgrounds,
>     // such as images, require extending the background rect to continue painting into the extended
>     // region. This function finds out if it is necessary to extend the background rect for painting.
> 
>@@ -3023,7 +3017,6 @@ void FrameView::updateTilesForExtendedBackgroundMode(ExtendedBackgroundMode mode
>     if (existingMode == mode)
>         return;
> 
>-    renderView->compositor().setRootExtendedBackgroundColor(mode == ExtendedBackgroundModeAll ? Color() : documentBackgroundColor());
>     backing->setTiledBackingHasMargins(mode & ExtendedBackgroundModeHorizontal, mode & ExtendedBackgroundModeVertical);
> }
> 
>diff --git a/Source/WebCore/rendering/RenderLayerCompositor.cpp b/Source/WebCore/rendering/RenderLayerCompositor.cpp
>index b5511103884849940f290b03a5d37587cfd81a90..be80cda855bfe4a309d2553c767bae1d6df658af 100644
>--- a/Source/WebCore/rendering/RenderLayerCompositor.cpp
>+++ b/Source/WebCore/rendering/RenderLayerCompositor.cpp
>@@ -669,6 +669,7 @@ bool RenderLayerCompositor::updateCompositingLayers(CompositingUpdateType update
>     
>     bool checkForHierarchyUpdate = m_reevaluateCompositingAfterLayout;
>     bool needGeometryUpdate = false;
>+    bool needRootLayerConfigurationUpdate = m_rootLayerConfigurationNeedsUpdate;
> 
>     switch (updateType) {
>     case CompositingUpdateType::AfterStyleChange:
>@@ -683,7 +684,7 @@ bool RenderLayerCompositor::updateCompositingLayers(CompositingUpdateType update
>         break;
>     }
> 
>-    if (!checkForHierarchyUpdate && !needGeometryUpdate)
>+    if (!checkForHierarchyUpdate && !needGeometryUpdate && !needRootLayerConfigurationUpdate)
>         return false;
> 
>     bool needHierarchyUpdate = m_compositingLayersNeedRebuild;
>@@ -691,6 +692,7 @@ bool RenderLayerCompositor::updateCompositingLayers(CompositingUpdateType update
> 
>     // Only clear the flag if we're updating the entire hierarchy.
>     m_compositingLayersNeedRebuild = false;
>+    m_rootLayerConfigurationNeedsUpdate = false;
>     updateRoot = &rootRenderLayer();
> 
>     if (isFullUpdate && updateType == CompositingUpdateType::AfterLayout)
>@@ -752,7 +754,8 @@ bool RenderLayerCompositor::updateCompositingLayers(CompositingUpdateType update
>         // most of the time, geometry is updated via RenderLayer::styleChanged().
>         updateLayerTreeGeometry(*updateRoot, 0);
>         ASSERT(!isFullUpdate || !m_subframeScrollLayersNeedReattach);
>-    }
>+    } else if (needRootLayerConfigurationUpdate)
>+        m_renderView.layer()->backing()->updateConfiguration();
>     
> #if !LOG_DISABLED
>     if (compositingLogEnabled() && isFullUpdate && (needHierarchyUpdate || needGeometryUpdate)) {
>@@ -3106,7 +3109,7 @@ void RenderLayerCompositor::rootOrBodyStyleChanged(RenderElement& renderer, cons
>         oldBackgroundColor = oldStyle->visitedDependentColorWithColorFilter(CSSPropertyBackgroundColor);
> 
>     if (oldBackgroundColor != renderer.style().visitedDependentColorWithColorFilter(CSSPropertyBackgroundColor))
>-        rootBackgroundTransparencyChanged();
>+        rootBackgroundColorOrTransparencyChanged();
> 
>     bool hadFixedBackground = oldStyle && oldStyle->hasEntirelyFixedBackground();
>     if (hadFixedBackground != renderer.style().hasEntirelyFixedBackground()) {
>@@ -3115,42 +3118,44 @@ void RenderLayerCompositor::rootOrBodyStyleChanged(RenderElement& renderer, cons
>     }
> }
> 
>-void RenderLayerCompositor::rootBackgroundTransparencyChanged()
>+void RenderLayerCompositor::rootBackgroundColorOrTransparencyChanged()
> {
>     if (!inCompositingMode())
>         return;
> 
>-    bool isTransparent = viewHasTransparentBackground();
>+    Color backgroundColor;
>+    bool isTransparent = viewHasTransparentBackground(&backgroundColor);
>+    
>+    Color extendedBackgroundColor = m_renderView.settings().backgroundShouldExtendBeyondPage() ? backgroundColor : Color();
>+    
>+    bool transparencyChanged = m_viewBackgroundIsTransparent != isTransparent;
>+    bool backgroundColorChanged = m_viewBackgroundColor != backgroundColor;
>+    bool extendedBackgroundColorChanged = m_rootExtendedBackgroundColor != extendedBackgroundColor;
> 
>-    LOG(Compositing, "RenderLayerCompositor %p rootBackgroundTransparencyChanged. isTransparent=%d, changed=%d", this, isTransparent, m_viewBackgroundIsTransparent != isTransparent);
>-    if (m_viewBackgroundIsTransparent == isTransparent)
>+    LOG(Compositing, "RenderLayerCompositor %p rootBackgroundColorOrTransparencyChanged. isTransparent=%d, transparencyChanged=%d, backgroundColorChanged=%d, extendedBackgroundColorChanged=%d", this, isTransparent, transparencyChanged, backgroundColorChanged, extendedBackgroundColorChanged);
>+    if (!transparencyChanged && !backgroundColorChanged && !extendedBackgroundColorChanged)
>         return;
> 
>     m_viewBackgroundIsTransparent = isTransparent;
>-
>-    // FIXME: We should do something less expensive than a full layer rebuild.
>-    setCompositingLayersNeedRebuild();
>-    scheduleCompositingLayerUpdate();
>-}
>-
>-void RenderLayerCompositor::setRootExtendedBackgroundColor(const Color& color)
>-{
>-    if (color == m_rootExtendedBackgroundColor)
>-        return;
>-
>-    m_rootExtendedBackgroundColor = color;
>-
>-    page().chrome().client().pageExtendedBackgroundColorDidChange(color);
>-
>+    m_viewBackgroundColor = backgroundColor;
>+    m_rootExtendedBackgroundColor = extendedBackgroundColor;
>+    
>+    if (extendedBackgroundColorChanged) {
>+        page().chrome().client().pageExtendedBackgroundColorDidChange(m_rootExtendedBackgroundColor);
>+        
> #if ENABLE(RUBBER_BANDING)
>-    if (!m_layerForOverhangAreas)
>-        return;
>-
>-    m_layerForOverhangAreas->setBackgroundColor(m_rootExtendedBackgroundColor);
>-
>-    if (!m_rootExtendedBackgroundColor.isValid())
>-        m_layerForOverhangAreas->setCustomAppearance(GraphicsLayer::ScrollingOverhang);
>+        if (!m_layerForOverhangAreas)
>+            return;
>+        
>+        m_layerForOverhangAreas->setBackgroundColor(m_rootExtendedBackgroundColor);
>+        
>+        if (!m_rootExtendedBackgroundColor.isValid())
>+            m_layerForOverhangAreas->setCustomAppearance(GraphicsLayer::ScrollingOverhang);
> #endif
>+    }
>+    
>+    setRootLayerConfigurationNeedsUpdate();
>+    scheduleCompositingLayerUpdate();
> }
> 
> void RenderLayerCompositor::updateOverflowControlsLayers()
>diff --git a/Source/WebCore/rendering/RenderLayerCompositor.h b/Source/WebCore/rendering/RenderLayerCompositor.h
>index 5f554b62555112a0d391d0dbc934390e166f68e4..35c3174fe784edc98aa78063f8b387a06b86f7ad 100644
>--- a/Source/WebCore/rendering/RenderLayerCompositor.h
>+++ b/Source/WebCore/rendering/RenderLayerCompositor.h
>@@ -165,7 +165,7 @@ public:
>     void rootOrBodyStyleChanged(RenderElement&, const RenderStyle* oldStyle);
> 
>     // Called after the view transparency, or the document or base background color change.
>-    void rootBackgroundTransparencyChanged();
>+    void rootBackgroundColorOrTransparencyChanged();
>     
>     // Repaint the appropriate layers when the given RenderLayer starts or stops being composited.
>     void repaintOnCompositingChange(RenderLayer&);
>@@ -313,7 +313,6 @@ public:
>     
>     void didPaintBacking(RenderLayerBacking*);
> 
>-    void setRootExtendedBackgroundColor(const Color&);
>     const Color& rootExtendedBackgroundColor() const { return m_rootExtendedBackgroundColor; }
> 
>     void updateRootContentLayerClipping();
>@@ -481,6 +480,8 @@ private:
> 
>     bool documentUsesTiledBacking() const;
>     bool isMainFrameCompositor() const;
>+    
>+    void setRootLayerConfigurationNeedsUpdate() { m_rootLayerConfigurationNeedsUpdate = true; }
> 
> private:
>     RenderView& m_renderView;
>@@ -501,6 +502,7 @@ private:
> 
>     bool m_compositing { false };
>     bool m_compositingLayersNeedRebuild { false };
>+    bool m_rootLayerConfigurationNeedsUpdate { false };
>     bool m_flushingLayers { false };
>     bool m_shouldFlushOnReattach { false };
>     bool m_forceCompositingMode { false };
>@@ -561,6 +563,7 @@ private:
>     double m_secondaryBackingStoreBytes { 0 };
> #endif
> 
>+    Color m_viewBackgroundColor;
>     Color m_rootExtendedBackgroundColor;
> 
>     HashMap<ScrollingNodeID, RenderLayer*> m_scrollingNodeToLayerMap;
>diff --git a/Source/WebCore/rendering/RenderView.cpp b/Source/WebCore/rendering/RenderView.cpp
>index c58bced8f54a17da7676d59072b10a0cc2cd82ae..b6ecdcf1e2770bc62837e08c8326ac403d51907c 100644
>--- a/Source/WebCore/rendering/RenderView.cpp
>+++ b/Source/WebCore/rendering/RenderView.cpp
>@@ -454,8 +454,7 @@ void RenderView::paintBoxDecorations(PaintInfo& paintInfo, const LayoutPoint&)
>         rootObscuresBackground = rendererObscuresBackground(*rootRenderer);
>     }
> 
>-    bool backgroundShouldExtendBeyondPage = settings().backgroundShouldExtendBeyondPage();
>-    compositor().setRootExtendedBackgroundColor(backgroundShouldExtendBeyondPage ? frameView().documentBackgroundColor() : Color());
>+    compositor().rootBackgroundColorOrTransparencyChanged();
> 
>     Page* page = document().page();
>     float pageScaleFactor = page ? page->pageScaleFactor() : 1;
>@@ -474,7 +473,7 @@ void RenderView::paintBoxDecorations(PaintInfo& paintInfo, const LayoutPoint&)
>         frameView().setCannotBlitToWindow(); // The parent must show behind the child.
>     else {
>         const Color& documentBackgroundColor = frameView().documentBackgroundColor();
>-        const Color& backgroundColor = (backgroundShouldExtendBeyondPage && documentBackgroundColor.isValid()) ? documentBackgroundColor : frameView().baseBackgroundColor();
>+        const Color& backgroundColor = (settings().backgroundShouldExtendBeyondPage() && documentBackgroundColor.isValid()) ? documentBackgroundColor : frameView().baseBackgroundColor();
>         if (backgroundColor.isVisible()) {
>             CompositeOperator previousOperator = paintInfo.context().compositeOperation();
>             paintInfo.context().setCompositeOperation(CompositeCopy);
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 2427f68c404ecfecc4e93ae730881c7e502ebfc0..a29e2ecbd01c3406cfa630fcc329cf164dd610de 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,22 @@
>+2018-06-12  Tim Horton  <timothy_horton@apple.com>
>+
>+        Unpainted area while scrolling in Reader is white
>+        https://bugs.webkit.org/show_bug.cgi?id=186541
>+        <rdar://problem/40471363>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * tiled-drawing/background-transparency-toggle-expected.txt:
>+        This is a progression; the extended background color now matches the color
>+        of the page at this point (#CCCCCC is the specified body background, black
>+        with 0.2 alpha, blended with the root's white background).
>+
>+        * tiled-drawing/simple-document-with-dynamic-background-color-expected.txt: Added.
>+        * tiled-drawing/simple-document-with-dynamic-background-color.html: Added.
>+        Added a test that ensures that dynamically changing the background color
>+        actually applies to the RenderView background. Previously, the second layer
>+        tree dump would have a black background where it should be red.
>+
> 2018-06-11  Jer Noble  <jer.noble@apple.com>
> 
>         Unreviewed gardening; add a late endTest(), in case none of the other events fire in time.
>diff --git a/LayoutTests/tiled-drawing/background-transparency-toggle-expected.txt b/LayoutTests/tiled-drawing/background-transparency-toggle-expected.txt
>index d58edb6538a79984e97a298ab88e02931086dbb9..d7e5e61e53c9e62b473c731d37bcb8579f7b9758 100644
>--- a/LayoutTests/tiled-drawing/background-transparency-toggle-expected.txt
>+++ b/LayoutTests/tiled-drawing/background-transparency-toggle-expected.txt
>@@ -49,6 +49,7 @@ Page tiles should be transparent if the body's background has alpha.
>     (GraphicsLayer
>       (bounds 785.00 1024.00)
>       (contentsOpaque 1)
>+      (backgroundColor #CCCCCC)
>       (tile cache coverage 0, 0 785 x 1024)
>       (tile size 785 x 512)
>       (top left tile 0, 0 tiles grid 1 x 2)
>diff --git a/LayoutTests/tiled-drawing/simple-document-with-dynamic-background-color-expected.txt b/LayoutTests/tiled-drawing/simple-document-with-dynamic-background-color-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..bf9aeb88d46544ded980a41b10abf7d2a0c51485
>--- /dev/null
>+++ b/LayoutTests/tiled-drawing/simple-document-with-dynamic-background-color-expected.txt
>@@ -0,0 +1,33 @@
>+(GraphicsLayer
>+  (anchor 0.00 0.00)
>+  (bounds 800.00 600.00)
>+  (children 1
>+    (GraphicsLayer
>+      (bounds 800.00 600.00)
>+      (contentsOpaque 1)
>+      (backgroundColor #000000)
>+      (tile cache coverage 0, 0 800 x 600)
>+      (tile size 800 x 600)
>+      (top left tile 0, 0 tiles grid 1 x 1)
>+      (in window 1)
>+    )
>+  )
>+)
>+
>+
>+(GraphicsLayer
>+  (anchor 0.00 0.00)
>+  (bounds 800.00 600.00)
>+  (children 1
>+    (GraphicsLayer
>+      (bounds 800.00 600.00)
>+      (contentsOpaque 1)
>+      (backgroundColor #FF0000)
>+      (tile cache coverage 0, 0 800 x 600)
>+      (tile size 800 x 600)
>+      (top left tile 0, 0 tiles grid 1 x 1)
>+      (in window 1)
>+    )
>+  )
>+)
>+
>diff --git a/LayoutTests/tiled-drawing/simple-document-with-dynamic-background-color.html b/LayoutTests/tiled-drawing/simple-document-with-dynamic-background-color.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..013da611e7199e4ec30a5932ebac7ec726bd849f
>--- /dev/null
>+++ b/LayoutTests/tiled-drawing/simple-document-with-dynamic-background-color.html
>@@ -0,0 +1,35 @@
>+<!DOCTYPE html>
>+
>+<html>
>+<head>
>+    <script>
>+        if (window.testRunner) {
>+            testRunner.dumpAsText();
>+            testRunner.waitUntilDone();
>+        }
>+
>+        function doTest()
>+        {
>+            if (!window.internals)
>+                return;
>+
>+            document.body.style.backgroundColor = "black";
>+
>+            // The RenderView's background color should be black.
>+            document.getElementById('layers').innerText = internals.layerTreeAsText(document, internals.LAYER_TREE_INCLUDES_TILE_CACHES) + "\n\n";
>+
>+            document.body.style.backgroundColor = "red";
>+
>+            // The RenderView's background color should be red.
>+            document.getElementById('layers').innerText += internals.layerTreeAsText(document, internals.LAYER_TREE_INCLUDES_TILE_CACHES);
>+
>+            testRunner.notifyDone();
>+        }
>+        window.addEventListener('load', doTest, false);
>+    </script>
>+</head>
>+
>+<body>
>+<pre id="layers">Layer tree goes here</p>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 186541: 342466 | 342473 | 342518 | 342530 | 342577