Skip to content

Conversation

@whsieh
Copy link
Member

@whsieh whsieh commented Apr 20, 2023

337e6be

REGRESSION (262243@main): LFC enters a broken, semi-enabled state after rendering scrollbars
https://bugs.webkit.org/show_bug.cgi?id=255741
rdar://108112443

Reviewed by Simon Fraser and Alan Baradlay.

When GPU process and UI-side compositing on macOS are enabled, 262243@main had inadvertently added a
call to `LocalFrameView::displayView()` in the process of asking for the current page scale; this
instantiates a `m_displayView` in the process, the existence of which causes us to reattach or
detach the root layer in `LocalFrameView::setIsInWindow()`.

`Display::View` was not intended to be used outside of LFC, so we should avoid instantiating it
unless necessary; to fix this bug, we instead add a `deviceScaleFactor()` override hook to
`ScrollableArea`, implement it in all relevant subclasses by asking the `Page`, and then plumbing
this information at paint time to `Scrollbar`, through `ScrollableArea`. This is done (as opposed to
adding a method that reads off of the `FrameView` or `Page` directly in scrollbar) to avoid layering
violations due to accessing non-platform WebCore logic from within `platform/`.

Additionally, we enforce this moving forward by asserting that LFC is enabled when asking for
`Display::View`, which would have fired in the previous test I added in 262243@main.

* Source/WebCore/page/LocalFrameView.cpp:
(WebCore::LocalFrameView::displayView):
(WebCore::LocalFrameView::deviceScaleFactor const):
* Source/WebCore/page/LocalFrameView.h:
* Source/WebCore/platform/ScrollableArea.h:
(WebCore::ScrollableArea::deviceScaleFactor const):
* Source/WebCore/platform/Scrollbar.cpp:
(WebCore::Scrollbar::deviceScaleFactor const):
* Source/WebCore/rendering/RenderLayerCompositor.cpp:
(WebCore::RenderLayerCompositor::deviceScaleFactor const):
* Source/WebCore/rendering/RenderLayerScrollableArea.cpp:
(WebCore::RenderLayerScrollableArea::deviceScaleFactor const):
* Source/WebCore/rendering/RenderLayerScrollableArea.h:
* Source/WebCore/rendering/RenderListBox.cpp:
(WebCore::RenderListBox::deviceScaleFactor const):
* Source/WebCore/rendering/RenderListBox.h:

Canonical link: https://commits.webkit.org/263241@main

4b4e531

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@whsieh whsieh requested a review from cdumez as a code owner April 20, 2023 19:37
@whsieh whsieh self-assigned this Apr 20, 2023
@whsieh whsieh added the Layout and Rendering For bugs with layout and rendering of Web pages. label Apr 20, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 20, 2023
@whsieh whsieh removed the merging-blocked Applied to prevent a change from being merged label Apr 21, 2023
@whsieh whsieh requested review from a team and rniwa as code owners April 21, 2023 01:52
@whsieh whsieh requested a review from alanbaradlay April 21, 2023 01:55
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 21, 2023
@whsieh whsieh removed the merging-blocked Applied to prevent a change from being merged label Apr 21, 2023
@whsieh whsieh added the merge-queue Applied to send a pull request to merge-queue label Apr 21, 2023
…er rendering scrollbars

https://bugs.webkit.org/show_bug.cgi?id=255741
rdar://108112443

Reviewed by Simon Fraser and Alan Baradlay.

When GPU process and UI-side compositing on macOS are enabled, 262243@main had inadvertently added a
call to `LocalFrameView::displayView()` in the process of asking for the current page scale; this
instantiates a `m_displayView` in the process, the existence of which causes us to reattach or
detach the root layer in `LocalFrameView::setIsInWindow()`.

`Display::View` was not intended to be used outside of LFC, so we should avoid instantiating it
unless necessary; to fix this bug, we instead add a `deviceScaleFactor()` override hook to
`ScrollableArea`, implement it in all relevant subclasses by asking the `Page`, and then plumbing
this information at paint time to `Scrollbar`, through `ScrollableArea`. This is done (as opposed to
adding a method that reads off of the `FrameView` or `Page` directly in scrollbar) to avoid layering
violations due to accessing non-platform WebCore logic from within `platform/`.

Additionally, we enforce this moving forward by asserting that LFC is enabled when asking for
`Display::View`, which would have fired in the previous test I added in 262243@main.

* Source/WebCore/page/LocalFrameView.cpp:
(WebCore::LocalFrameView::displayView):
(WebCore::LocalFrameView::deviceScaleFactor const):
* Source/WebCore/page/LocalFrameView.h:
* Source/WebCore/platform/ScrollableArea.h:
(WebCore::ScrollableArea::deviceScaleFactor const):
* Source/WebCore/platform/Scrollbar.cpp:
(WebCore::Scrollbar::deviceScaleFactor const):
* Source/WebCore/rendering/RenderLayerCompositor.cpp:
(WebCore::RenderLayerCompositor::deviceScaleFactor const):
* Source/WebCore/rendering/RenderLayerScrollableArea.cpp:
(WebCore::RenderLayerScrollableArea::deviceScaleFactor const):
* Source/WebCore/rendering/RenderLayerScrollableArea.h:
* Source/WebCore/rendering/RenderListBox.cpp:
(WebCore::RenderListBox::deviceScaleFactor const):
* Source/WebCore/rendering/RenderListBox.h:

Canonical link: https://commits.webkit.org/263241@main
@webkit-commit-queue
Copy link
Collaborator

Committed 263241@main (337e6be): https://commits.webkit.org/263241@main

Reviewed commits have been landed. Closing PR #12987 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 337e6be into WebKit:main Apr 21, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Apr 21, 2023
@whsieh whsieh deleted the eng/255741 branch April 21, 2023 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Layout and Rendering For bugs with layout and rendering of Web pages.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants