Attachment 360988 Details for Bug 194198 – Patch

[patch] Patch

bug-194198-20190202173203.patch (text/plain), 25.52 KB, created by Simon Fraser (smfr) on 2019-02-02 17:32:04 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Fraser (smfr)

Created: 2019-02-02 17:32:04 PST

Size: 25.52 KB

patch

obsolete

>Subversion Revision: 240899
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 0583c72f630984342ef143ec8d5cf8a72769daff..4ad2ab8e3d7a647f593c2ed8dfc2aa78c4c30606 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,69 @@
>+2019-02-02  Simon Fraser  <simon.fraser@apple.com>
>+
>+        Make setNeedsLayout on the root more explicitly about triggering its side-effects
>+        https://bugs.webkit.org/show_bug.cgi?id=194198
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Calling setNeedsLayout() on the FrameView or RenderView is an odd concept; the render tree
>+        generally manages its own dirty state.
>+
>+        Most callers of setNeedsLayout() on the root are really trying to trigger the side-effects
>+        of layout, like compositing updates, which are required when view configuration state, like
>+        headers, footers and transparency, change. These dependencies are currently implicit and
>+        poorly defined.
>+
>+        Renaming "setNeedsLayout" on FrameView is a step towards being more explicit about pure
>+        rendering updates, vs updates of downstream data strutures like compositing. It's now called
>+        setNeedsLayoutAfterViewConfigurationChange(). In addition, expose
>+        setNeedsCompositingConfigurationUpdate() and setNeedsCompositingGeometryUpdate() so callers
>+        can trigger the appropriate types of compositing updates on the root layer.
>+
>+        In addition, FrameViewLayoutContext::setNeedsLayoutAfterViewConfigurationChange() schedules a
>+        layout. Withtout this, some callers would dirty the RenderView's layout but rely on some
>+        other trigger to make the layout happen.
>+
>+        This cleanup was prompted by noticing that FrameView::setHeaderHeight() dirtied layout
>+        but never scheduled it, making banner insertion in MiniBrowser unreliable.
>+
>+        This patch also removes the aliasing of headerHeight/footerHeight between Page and
>+        FrameView. Banners are a property of Page, so FrameView fetches the banner heights
>+        from Page.
>+
>+        * page/FrameView.cpp:
>+        (WebCore::FrameView::headerHeight const):
>+        (WebCore::FrameView::footerHeight const):
>+        (WebCore::FrameView::availableContentSizeChanged):
>+        (WebCore::FrameView::setNeedsLayoutAfterViewConfigurationChange):
>+        (WebCore::FrameView::setNeedsCompositingConfigurationUpdate):
>+        (WebCore::FrameView::setNeedsCompositingGeometryUpdate):
>+        (WebCore::FrameView::scheduleSelectionUpdate):
>+        (WebCore::FrameView::setTransparent):
>+        (WebCore::FrameView::setBaseBackgroundColor):
>+        (WebCore::FrameView::setAutoSizeFixedMinimumHeight):
>+        (WebCore::FrameView::enableAutoSizeMode):
>+        (WebCore::FrameView::setHeaderHeight): Deleted.
>+        (WebCore::FrameView::setFooterHeight): Deleted.
>+        (WebCore::FrameView::setNeedsLayout): Deleted.
>+        * page/FrameView.h:
>+        * page/FrameViewLayoutContext.cpp:
>+        (WebCore::FrameViewLayoutContext::setNeedsLayoutAfterViewConfigurationChange):
>+        (WebCore::FrameViewLayoutContext::setNeedsLayout): Deleted.
>+        * page/FrameViewLayoutContext.h:
>+        * page/Page.cpp:
>+        (WebCore::Page::setPageScaleFactor):
>+        (WebCore::Page::setHeaderHeight):
>+        (WebCore::Page::setFooterHeight):
>+        (WebCore::Page::addHeaderWithHeight): Deleted.
>+        (WebCore::Page::addFooterWithHeight): Deleted.
>+        * page/Page.h:
>+        * rendering/RenderLayerCompositor.cpp:
>+        (WebCore::RenderLayerCompositor::updateBacking):
>+        * testing/Internals.cpp:
>+        (WebCore::Internals::resetToConsistentState):
>+        (WebCore::Internals::setHeaderHeight):
>+        (WebCore::Internals::setFooterHeight):
>+
> 2019-02-02  Simon Fraser  <simon.fraser@apple.com>
> 
>         Rename "scrollingLayer" in RenderLayerBacking to "scrollContainerLayer" for clarity
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 121ee2dde5ca99e436bf9256d41294ab48adb8e6..db0e6f0bb438599415f4174446e23eea8140d205 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,24 @@
>+2019-02-02  Simon Fraser  <simon.fraser@apple.com>
>+
>+        Make setNeedsLayout on the root more explicitly about triggering its side-effects
>+        https://bugs.webkit.org/show_bug.cgi?id=194198
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Call the newly named functions.
>+
>+        * WebProcess/Plugins/PDF/PDFPlugin.mm:
>+        (WebKit::PDFPlugin::updateScrollbars):
>+        * WebProcess/Plugins/PluginView.cpp:
>+        (WebKit::PluginView::didInitializePlugin):
>+        * WebProcess/WebPage/WebPage.cpp:
>+        (WebKit::WebPage::setHeaderBannerHeightForTesting):
>+        (WebKit::WebPage::setFooterBannerHeightForTesting):
>+        * WebProcess/WebPage/mac/PageBannerMac.mm:
>+        (WebKit::PageBanner::addToPage):
>+        (WebKit::PageBanner::detachFromPage):
>+        (WebKit::PageBanner::hide):
>+
> 2018-12-16  Darin Adler  <darin@apple.com>
> 
>         Convert additional String::format clients to alternative approaches
>diff --git a/Source/WebKitLegacy/mac/ChangeLog b/Source/WebKitLegacy/mac/ChangeLog
>index a22164a9c7fcb2ab3fef0e3b20814a8db28590ce..3b8783988da98160a1b26f8b3dda34395f23009f 100644
>--- a/Source/WebKitLegacy/mac/ChangeLog
>+++ b/Source/WebKitLegacy/mac/ChangeLog
>@@ -1,3 +1,17 @@
>+2019-02-02  Simon Fraser  <simon.fraser@apple.com>
>+
>+        Make setNeedsLayout on the root more explicitly about triggering its side-effects
>+        https://bugs.webkit.org/show_bug.cgi?id=194198
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Call the newly named functions.
>+
>+        * WebView/WebFrame.mm:
>+        (-[WebFrame setNeedsLayout]):
>+        * WebView/WebHTMLView.mm:
>+        (-[WebHTMLView setNeedsLayout:]):
>+
> 2019-01-31  Alex Christensen  <achristensen@webkit.org>
> 
>         Revert r238819 which is unneeded and caused a performance regression.
>diff --git a/Source/WebCore/page/FrameView.cpp b/Source/WebCore/page/FrameView.cpp
>index 5e619d52a36fa63eb40ff81ddba31b45544d1ded..fbf2472f5ca0add030a67ba76fa35f246f7eff8f 100644
>--- a/Source/WebCore/page/FrameView.cpp
>+++ b/Source/WebCore/page/FrameView.cpp
>@@ -1071,24 +1071,16 @@ LayoutPoint FrameView::scrollPositionRespectingCustomFixedPosition() const
>     return scrollPositionForFixedPosition();
> }
> 
>-void FrameView::setHeaderHeight(int headerHeight)
>+int FrameView::headerHeight() const
> {
>-    if (frame().page())
>-        ASSERT(frame().isMainFrame());
>-    m_headerHeight = headerHeight;
>-
>-    if (RenderView* renderView = this->renderView())
>-        renderView->setNeedsLayout();
>+    Page* page = frame().page();
>+    return page ? page->headerHeight() : 0;
> }
> 
>-void FrameView::setFooterHeight(int footerHeight)
>+int FrameView::footerHeight() const
> {
>-    if (frame().page())
>-        ASSERT(frame().isMainFrame());
>-    m_footerHeight = footerHeight;
>-
>-    if (RenderView* renderView = this->renderView())
>-        renderView->setNeedsLayout();
>+    Page* page = frame().page();
>+    return page ? page->footerHeight() : 0;
> }
> 
> float FrameView::topContentInset(TopContentInsetType contentInsetTypeToReturn) const
>@@ -2697,7 +2689,7 @@ void FrameView::availableContentSizeChanged(AvailableSizeChangeReason reason)
>     }
> 
>     updateLayoutViewport();
>-    setNeedsLayout();
>+    setNeedsLayoutAfterViewConfigurationChange();
>     ScrollView::availableContentSizeChanged(reason);
> }
> 
>@@ -2942,9 +2934,27 @@ bool FrameView::needsLayout() const
>     return layoutContext().needsLayout();
> }
> 
>-void FrameView::setNeedsLayout()
>+void FrameView::setNeedsLayoutAfterViewConfigurationChange()
>+{
>+    layoutContext().setNeedsLayoutAfterViewConfigurationChange();
>+}
>+
>+void FrameView::setNeedsCompositingConfigurationUpdate()
> {
>-    layoutContext().setNeedsLayout();
>+    RenderView* renderView = this->renderView();
>+    if (renderView->usesCompositing()) {
>+        if (auto* rootLayer = renderView->layer())
>+            renderView->layer()->setNeedsCompositingConfigurationUpdate();
>+    }
>+}
>+
>+void FrameView::setNeedsCompositingGeometryUpdate()
>+{
>+    RenderView* renderView = this->renderView();
>+    if (renderView->usesCompositing()) {
>+        if (auto* rootLayer = renderView->layer())
>+            renderView->layer()->setNeedsCompositingGeometryUpdate();
>+    }
> }
> 
> void FrameView::scheduleSelectionUpdate()
>@@ -2953,8 +2963,7 @@ void FrameView::scheduleSelectionUpdate()
>         return;
>     // FIXME: We should not need to go through the layout process since selection update does not change dimension/geometry.
>     // However we can't tell at this point if the tree is stable yet, so let's just schedule a root only layout for now.
>-    setNeedsLayout();
>-    layoutContext().scheduleLayout();
>+    setNeedsLayoutAfterViewConfigurationChange();
> }
> 
> bool FrameView::isTransparent() const
>@@ -2976,8 +2985,8 @@ void FrameView::setTransparent(bool isTransparent)
>     if (!isViewForDocumentInFrame())
>         return;
> 
>-    renderView()->compositor().rootBackgroundColorOrTransparencyChanged();
>-    setNeedsLayout();
>+    setNeedsLayoutAfterViewConfigurationChange();
>+    setNeedsCompositingConfigurationUpdate();
> }
> 
> bool FrameView::hasOpaqueBackground() const
>@@ -2998,9 +3007,8 @@ void FrameView::setBaseBackgroundColor(const Color& backgroundColor)
>         return;
> 
>     recalculateScrollbarOverlayStyle();
>-    setNeedsLayout();
>-
>-    renderView()->compositor().rootBackgroundColorOrTransparencyChanged();
>+    setNeedsLayoutAfterViewConfigurationChange();
>+    setNeedsCompositingConfigurationUpdate();
> }
> 
> void FrameView::updateBackgroundRecursively(bool transparent)
>@@ -3540,7 +3548,7 @@ void FrameView::setAutoSizeFixedMinimumHeight(int fixedMinimumHeight)
> 
>     m_autoSizeFixedMinimumHeight = fixedMinimumHeight;
> 
>-    setNeedsLayout();
>+    setNeedsLayoutAfterViewConfigurationChange();
> }
> 
> RenderElement* FrameView::viewportRenderer() const
>@@ -4540,7 +4548,7 @@ void FrameView::enableAutoSizeMode(bool enable, const IntSize& minSize, const In
>     m_maxAutoSize = maxSize;
>     m_didRunAutosize = false;
> 
>-    setNeedsLayout();
>+    setNeedsLayoutAfterViewConfigurationChange();
>     layoutContext().scheduleLayout();
>     if (m_shouldAutoSize) {
>         overrideViewportSizeForCSSViewportUnits({ minSize.width(), m_overrideViewportSize ? m_overrideViewportSize->height : WTF::nullopt });
>diff --git a/Source/WebCore/page/FrameView.h b/Source/WebCore/page/FrameView.h
>index 636ecb92f92ba4346011aa678f8400eb76375584..890adb0e11a8db4b046f069b173962b25ba664bd 100644
>--- a/Source/WebCore/page/FrameView.h
>+++ b/Source/WebCore/page/FrameView.h
>@@ -118,7 +118,11 @@ public:
>     void queuePostLayoutCallback(WTF::Function<void ()>&&);
> 
>     WEBCORE_EXPORT bool needsLayout() const;
>-    WEBCORE_EXPORT void setNeedsLayout();
>+    WEBCORE_EXPORT void setNeedsLayoutAfterViewConfigurationChange();
>+
>+    void setNeedsCompositingConfigurationUpdate();
>+    void setNeedsCompositingGeometryUpdate();
>+
>     void setViewportConstrainedObjectsNeedLayout();
> 
>     WEBCORE_EXPORT bool renderedCharactersExceed(unsigned threshold);
>@@ -574,10 +578,8 @@ public:
> 
>     LayoutPoint scrollPositionRespectingCustomFixedPosition() const;
> 
>-    int headerHeight() const final { return m_headerHeight; }
>-    WEBCORE_EXPORT void setHeaderHeight(int);
>-    int footerHeight() const final { return m_footerHeight; }
>-    WEBCORE_EXPORT void setFooterHeight(int);
>+    WEBCORE_EXPORT int headerHeight() const final;
>+    WEBCORE_EXPORT int footerHeight() const final;
> 
>     WEBCORE_EXPORT float topContentInset(TopContentInsetType = TopContentInsetType::WebCoreContentInset) const final;
>     void topContentInsetDidChange(float newTopContentInset);
>diff --git a/Source/WebCore/page/FrameViewLayoutContext.cpp b/Source/WebCore/page/FrameViewLayoutContext.cpp
>index cefd38345cf5369f10ab0977bf144f8cbed40bf2..32427e1403250917f9b87860db0c2818c74b06aa 100644
>--- a/Source/WebCore/page/FrameViewLayoutContext.cpp
>+++ b/Source/WebCore/page/FrameViewLayoutContext.cpp
>@@ -313,7 +313,7 @@ bool FrameViewLayoutContext::needsLayout() const
>         || (m_disableSetNeedsLayoutCount && m_setNeedsLayoutWasDeferred);
> }
> 
>-void FrameViewLayoutContext::setNeedsLayout()
>+void FrameViewLayoutContext::setNeedsLayoutAfterViewConfigurationChange()
> {
>     if (m_disableSetNeedsLayoutCount) {
>         m_setNeedsLayoutWasDeferred = true;
>@@ -323,6 +323,7 @@ void FrameViewLayoutContext::setNeedsLayout()
>     if (auto* renderView = this->renderView()) {
>         ASSERT(!renderView->inHitTesting());
>         renderView->setNeedsLayout();
>+        scheduleLayout();
>     }
> }
> 
>diff --git a/Source/WebCore/page/FrameViewLayoutContext.h b/Source/WebCore/page/FrameViewLayoutContext.h
>index b07cf5a5d9b38ec152273ba84773d3b3d7b1f88a..09c1415727a12d9dc7d7c406a6007d7200223853 100644
>--- a/Source/WebCore/page/FrameViewLayoutContext.h
>+++ b/Source/WebCore/page/FrameViewLayoutContext.h
>@@ -50,10 +50,12 @@ public:
>     ~FrameViewLayoutContext();
> 
>     void layout();
>-
>-    void setNeedsLayout();
>     bool needsLayout() const;
> 
>+    // We rely on the side-effects of layout, like compositing updates, to update state in various subsystems
>+    // whose dependencies are poorly defined. This call triggers such updates.
>+    void setNeedsLayoutAfterViewConfigurationChange();
>+
>     void scheduleLayout();
>     void scheduleSubtreeLayout(RenderElement& layoutRoot);
>     void unscheduleLayout();
>diff --git a/Source/WebCore/page/Page.cpp b/Source/WebCore/page/Page.cpp
>index 0783004f107d03c3070aee52f4dffc5b2e96f2d7..c5800176be5449e962dec58646b8cc60bc553fae 100644
>--- a/Source/WebCore/page/Page.cpp
>+++ b/Source/WebCore/page/Page.cpp
>@@ -1005,11 +1005,8 @@ void Page::setPageScaleFactor(float scale, const IntPoint& origin, bool inStable
>     m_pageScaleFactor = scale;
> 
>     if (!m_settings->delegatesPageScaling()) {
>-        if (auto* renderView = document->renderView()) {
>-            renderView->setNeedsLayout();
>-            if (renderView->hasLayer() && renderView->layer()->isComposited())
>-                renderView->layer()->setNeedsCompositingGeometryUpdate();
>-        }
>+        view->setNeedsLayoutAfterViewConfigurationChange();
>+        view->setNeedsCompositingGeometryUpdate();
> 
>         document->resolveStyle(Document::ResolveStyleType::Rebuild);
> 
>@@ -1969,8 +1966,11 @@ VisibilityState Page::visibilityState() const
> }
> 
> #if ENABLE(RUBBER_BANDING)
>-void Page::addHeaderWithHeight(int headerHeight)
>+void Page::setHeaderHeight(int headerHeight)
> {
>+    if (headerHeight == m_headerHeight)
>+        return;
>+
>     m_headerHeight = headerHeight;
> 
>     FrameView* frameView = mainFrame().view();
>@@ -1981,12 +1981,15 @@ void Page::addHeaderWithHeight(int headerHeight)
>     if (!renderView)
>         return;
> 
>-    frameView->setHeaderHeight(m_headerHeight);
>-    renderView->compositor().updateLayerForHeader(m_headerHeight);
>+    frameView->setNeedsLayoutAfterViewConfigurationChange();
>+    frameView->setNeedsCompositingGeometryUpdate();
> }
> 
>-void Page::addFooterWithHeight(int footerHeight)
>+void Page::setFooterHeight(int footerHeight)
> {
>+    if (footerHeight == m_footerHeight)
>+        return;
>+
>     m_footerHeight = footerHeight;
> 
>     FrameView* frameView = mainFrame().view();
>@@ -1997,8 +2000,8 @@ void Page::addFooterWithHeight(int footerHeight)
>     if (!renderView)
>         return;
> 
>-    frameView->setFooterHeight(m_footerHeight);
>-    renderView->compositor().updateLayerForFooter(m_footerHeight);
>+    frameView->setNeedsLayoutAfterViewConfigurationChange();
>+    frameView->setNeedsCompositingGeometryUpdate();
> }
> #endif
> 
>diff --git a/Source/WebCore/page/Page.h b/Source/WebCore/page/Page.h
>index 335d6a05fcbc14dc26ad70ef6493bd4b77aa32e2..18dc397adbe11de73c08d3d622ac0ca9a606fca8 100644
>--- a/Source/WebCore/page/Page.h
>+++ b/Source/WebCore/page/Page.h
>@@ -520,8 +520,8 @@ public:
>     OptionSet<LayoutMilestone> requestedLayoutMilestones() const { return m_requestedLayoutMilestones; }
> 
> #if ENABLE(RUBBER_BANDING)
>-    WEBCORE_EXPORT void addHeaderWithHeight(int);
>-    WEBCORE_EXPORT void addFooterWithHeight(int);
>+    WEBCORE_EXPORT void setHeaderHeight(int);
>+    WEBCORE_EXPORT void setFooterHeight(int);
> #endif
> 
>     int headerHeight() const { return m_headerHeight; }
>diff --git a/Source/WebCore/rendering/RenderLayerCompositor.cpp b/Source/WebCore/rendering/RenderLayerCompositor.cpp
>index f2eac310afa239dc971878423db86c2e0192ea09..97c17864fa4f8628adc66406a62109d25e419506 100644
>--- a/Source/WebCore/rendering/RenderLayerCompositor.cpp
>+++ b/Source/WebCore/rendering/RenderLayerCompositor.cpp
>@@ -1543,16 +1543,17 @@ bool RenderLayerCompositor::updateBacking(RenderLayer& layer, RequiresCompositin
>             layer.ensureBacking();
> 
>             if (layer.isRenderViewLayer() && useCoordinatedScrollingForLayer(layer)) {
>+                auto& frameView = m_renderView.frameView();
>                 if (auto* scrollingCoordinator = this->scrollingCoordinator())
>-                    scrollingCoordinator->frameViewRootLayerDidChange(m_renderView.frameView());
>+                    scrollingCoordinator->frameViewRootLayerDidChange(frameView);
> #if ENABLE(RUBBER_BANDING)
>-                updateLayerForHeader(page().headerHeight());
>-                updateLayerForFooter(page().footerHeight());
>+                updateLayerForHeader(frameView.headerHeight());
>+                updateLayerForFooter(frameView.footerHeight());
> #endif
>                 updateRootContentLayerClipping();
> 
>                 if (auto* tiledBacking = layer.backing()->tiledBacking())
>-                    tiledBacking->setTopContentInset(m_renderView.frameView().topContentInset());
>+                    tiledBacking->setTopContentInset(frameView.topContentInset());
>             }
> 
>             // This layer and all of its descendants have cached repaints rects that are relative to
>diff --git a/Source/WebCore/testing/Internals.cpp b/Source/WebCore/testing/Internals.cpp
>index 61cff4d054ee4d7e5d1bfe13a52a87319abadef9..7efdb54e3db2e1aeb4697c8df533d7fc2e267742 100644
>--- a/Source/WebCore/testing/Internals.cpp
>+++ b/Source/WebCore/testing/Internals.cpp
>@@ -449,8 +449,8 @@ void Internals::resetToConsistentState(Page& page)
> 
>     FrameView* mainFrameView = page.mainFrame().view();
>     if (mainFrameView) {
>-        mainFrameView->setHeaderHeight(0);
>-        mainFrameView->setFooterHeight(0);
>+        page.setHeaderHeight(0);
>+        page.setFooterHeight(0);
>         page.setTopContentInset(0);
>         mainFrameView->setUseFixedLayout(false);
>         mainFrameView->setFixedLayoutSize(IntSize());
>@@ -2903,7 +2903,7 @@ void Internals::setHeaderHeight(float height)
>     if (!document || !document->view())
>         return;
> 
>-    document->view()->setHeaderHeight(height);
>+    document->page()->setHeaderHeight(height);
> }
> 
> void Internals::setFooterHeight(float height)
>@@ -2912,7 +2912,7 @@ void Internals::setFooterHeight(float height)
>     if (!document || !document->view())
>         return;
> 
>-    document->view()->setFooterHeight(height);
>+    document->page()->setFooterHeight(height);
> }
> 
> void Internals::setTopContentInset(float contentInset)
>diff --git a/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm b/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm
>index 3467b3fecf911c071655a2eccf6942b00397dcac..4af6660729a955be9b7a7150b2f6e8a8d52b9ca7 100644
>--- a/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm
>+++ b/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm
>@@ -735,7 +735,7 @@ void PDFPlugin::updateScrollbars()
>         else
>             frameView->removeScrollableArea(this);
> 
>-        frameView->setNeedsLayout();
>+        frameView->setNeedsLayoutAfterViewConfigurationChange();
>     }
>     
>     if (m_verticalScrollbarLayer) {
>diff --git a/Source/WebKit/WebProcess/Plugins/PluginView.cpp b/Source/WebKit/WebProcess/Plugins/PluginView.cpp
>index 0d7fbe134c60a740f9d9774fa915fe8853e0ccb9..a301afb880e49a7a30d307bb2cd5dfbf1543cd4a 100644
>--- a/Source/WebKit/WebProcess/Plugins/PluginView.cpp
>+++ b/Source/WebKit/WebProcess/Plugins/PluginView.cpp
>@@ -663,7 +663,7 @@ void PluginView::didInitializePlugin()
>     if (wantsWheelEvents()) {
>         if (Frame* frame = m_pluginElement->document().frame()) {
>             if (FrameView* frameView = frame->view())
>-                frameView->setNeedsLayout();
>+                frameView->setNeedsLayoutAfterViewConfigurationChange();
>         }
>     }
> 
>diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>index 8526740e438b09794a86e5312f23c24eed8be059..f746ab2118d37d55e1db8629830d751cee93fe58 100644
>--- a/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>+++ b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>@@ -2129,14 +2129,14 @@ void WebPage::showPageBanners()
> void WebPage::setHeaderBannerHeightForTesting(int height)
> {
> #if ENABLE(RUBBER_BANDING)
>-    corePage()->addHeaderWithHeight(height);
>+    corePage()->setHeaderHeight(height);
> #endif
> }
> 
> void WebPage::setFooterBannerHeightForTesting(int height)
> {
> #if ENABLE(RUBBER_BANDING)
>-    corePage()->addFooterWithHeight(height);
>+    corePage()->setFooterHeight(height);
> #endif
> }
> 
>diff --git a/Source/WebKit/WebProcess/WebPage/mac/PageBannerMac.mm b/Source/WebKit/WebProcess/WebPage/mac/PageBannerMac.mm
>index 14986fc2ac3fe0c53c2f8f895c3b3ef8fd95dcb1..236933df0b2cf04bca0db9fee3a2549739eabe95 100644
>--- a/Source/WebKit/WebProcess/WebPage/mac/PageBannerMac.mm
>+++ b/Source/WebKit/WebProcess/WebPage/mac/PageBannerMac.mm
>@@ -57,10 +57,10 @@ void PageBanner::addToPage(Type type, WebPage* webPage)
> 
>     switch (m_type) {
>     case Header:
>-        m_webPage->corePage()->addHeaderWithHeight(m_height);
>+        m_webPage->corePage()->setHeaderHeight(m_height);
>         break;
>     case Footer:
>-        m_webPage->corePage()->addFooterWithHeight(m_height);
>+        m_webPage->corePage()->setFooterHeight(m_height);
>         break;
>     case NotSet:
>         ASSERT_NOT_REACHED();
>@@ -86,9 +86,9 @@ void PageBanner::detachFromPage()
>     if (m_webPage->corePage()) {
>         // We can hide the banner by removing the parent layer that hosts it.
>         if (m_type == Header)
>-            m_webPage->corePage()->addHeaderWithHeight(0);
>+            m_webPage->corePage()->setHeaderHeight(0);
>         else if (m_type == Footer)
>-            m_webPage->corePage()->addFooterWithHeight(0);
>+            m_webPage->corePage()->setFooterHeight(0);
>     }
> 
>     m_type = NotSet;
>@@ -99,9 +99,9 @@ void PageBanner::hide()
> {
>     // We can hide the banner by removing the parent layer that hosts it.
>     if (m_type == Header)
>-        m_webPage->corePage()->addHeaderWithHeight(0);
>+        m_webPage->corePage()->setHeaderHeight(0);
>     else if (m_type == Footer)
>-        m_webPage->corePage()->addFooterWithHeight(0);
>+        m_webPage->corePage()->setFooterHeight(0);
> 
>     m_isHidden = true;
> }
>diff --git a/Source/WebKitLegacy/mac/WebView/WebFrame.mm b/Source/WebKitLegacy/mac/WebView/WebFrame.mm
>index a93def61399a5cc54ef61cae9c0379905890cd7c..e0459f7cfb4dc9d690695b23316a27d5d652e948 100644
>--- a/Source/WebKitLegacy/mac/WebView/WebFrame.mm
>+++ b/Source/WebKitLegacy/mac/WebView/WebFrame.mm
>@@ -1310,7 +1310,7 @@ - (void)setNeedsLayout
> {
>     WebCore::Frame *frame = core(self);
>     if (frame->view())
>-        frame->view()->setNeedsLayout();
>+        frame->view()->setNeedsLayoutAfterViewConfigurationChange();
> }
> 
> - (CGSize)renderedSizeOfNode:(DOMNode *)node constrainedToWidth:(float)width
>diff --git a/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm b/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm
>index 7b4394aef3d9577374d6e9ee259cbd1df80d0fa8..c9cf7f9e7fa1d1f640cf85ad29a3fae4bfed15f2 100644
>--- a/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm
>+++ b/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm
>@@ -3869,7 +3869,7 @@ - (void)setNeedsLayout: (BOOL)flag
>         if (frame->document() && frame->document()->pageCacheState() != Document::NotInPageCache)
>             return;
>         if (FrameView* view = frame->view())
>-            view->setNeedsLayout();
>+            view->setNeedsLayoutAfterViewConfigurationChange();
>     }
> }
> 
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 12ce4207d282f9a9c34182ba6c5023e0c1340081..059fc970d718f52111de0b0c3a6c73195e705e1c 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,15 @@
>+2019-02-02  Simon Fraser  <simon.fraser@apple.com>
>+
>+        Make setNeedsLayout on the root more explicitly about triggering its side-effects
>+        https://bugs.webkit.org/show_bug.cgi?id=194198
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        No need to set the banner heights on navigation now, since Page stores them.
>+
>+        * MiniBrowser/mac/WK2BrowserWindowController.m:
>+        (-[WK2BrowserWindowController webView:didFinishNavigation:]):
>+
> 2019-02-02  Zalan Bujtas  <zalan@apple.com>
> 
>         [LFC] Initialize ICB's style with fixed width/height.
>diff --git a/Tools/MiniBrowser/mac/WK2BrowserWindowController.m b/Tools/MiniBrowser/mac/WK2BrowserWindowController.m
>index e9d4152ef64c478b56aa983fdaa176f83cdf9f45..721e48d948fe04dc87716443545d75401fe4ab89 100644
>--- a/Tools/MiniBrowser/mac/WK2BrowserWindowController.m
>+++ b/Tools/MiniBrowser/mac/WK2BrowserWindowController.m
>@@ -674,12 +674,6 @@ - (void)webView:(WKWebView *)webView didCommitNavigation:(WKNavigation *)navigat
> - (void)webView:(WKWebView *)webView didFinishNavigation:(WKNavigation *)navigation
> {
>     LOG(@"didFinishNavigation: %@", navigation);
>-    
>-    // Banner heights don't persist across page loads (oddly, since Page stores them), so reset on every page load.
>-    if ([[SettingsController shared] isSpaceReservedForBanners]) {
>-        [_webView _setHeaderBannerHeight:testHeaderBannerHeight];
>-        [_webView _setFooterBannerHeight:testFooterBannerHeight];
>-    }
> }
> 
> - (void)webView:(WKWebView *)webView didReceiveAuthenticationChallenge:(NSURLAuthenticationChallenge *)challenge completionHandler:(void (^)(NSURLSessionAuthChallengeDisposition disposition, NSURLCredential *__nullable credential))completionHandler

Actions: View | Formatted Diff | Diff

Attachments on bug 194198: 360988 | 360989 | 360991 | 360995