Bug 221067

Created attachment 418598 [details] WIP patch

GTK port disables Threaded Scrolling in the default mode WEBKIT_HARDWARE_ACCELERATION_POLICY_ON_DEMAND. https://github.com/WebKit/WebKit/blob/409d2a1d7c100ee0b7996b26a3b12ec2207609fc/Source/WebKit/UIProcess/gtk/WebPreferencesGtk.cpp#L49 So, a scrolling tree isn't generated for the page. https://github.com/WebKit/WebKit/blob/409d2a1d7c100ee0b7996b26a3b12ec2207609fc/Source/WebKit/WebProcess/WebPage/WebPage.cpp#L742

See also: Bug 202449 – [GTK][WPE] Enable async scrolling

setForceCompositingMode | setThreadedScrollingEnabled | | v v true true : Good (Mac, GTK, WPE) false false : Good (GTK) false true : Bad (WinCairo) It seems that WinCairo should align with other ports. There are WKPreferencesSetAcceleratedCompositingEnabled and WKPreferencesSetThreadedScrollingEnabled. However, no API corresponding to setForceCompositingMode.

Created attachment 418609 [details] WIP patch

Comment on attachment 418609 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=418609&action=review > Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.cpp:256 > + settings.setForceCompositingMode(store.getBoolValueForKey(WebPreferencesKey::acceleratedCompositingEnabledKey())); This patch doesn't work well in the case of disabling AC in runtime by using WKPreferencesSetAcceleratedCompositingEnabled(pref, false) because the WebKit::WebPage already has a scrolling tree. This problem can be reproduced with GTK MiniBrowser by switching compositing policy from 'always' to 'never' in runtime.

(In reply to Fujii Hironori from comment #6) > This problem can be reproduced with GTK MiniBrowser by switching > compositing policy from 'always' to 'never' in runtime. I'm going to fix the GTK problem in this bug ticket. 1. Start GTK MiniBrowser 2. Set Hardware Acceleration Policy to "always" in Preferences 3. Go to a non-AC mode page. For example, https://trac.webkit.org/wiki 4. Set Hardware Acceleration Policy to "never" in Preferences 5. Reload 6. Roll the mouse wheel 7. The page scrolling doesn't work as expected.

Created attachment 419553 [details] Patch

hmm, does this mean that it would be possible to always use the async scrolling for AC mode? We currently enable it only when forced, assuming that it was needed to be enabled from the very beginning and it couldn't be enabled/disabled on demand.

It would be much more reliable if AC were always enabled in GTK.

(In reply to Carlos Garcia Campos from comment #9) > hmm, does this mean that it would be possible to always use the async > scrolling for AC mode? We currently enable it only when forced, assuming > that it was needed to be enabled from the very beginning and it couldn't be > enabled/disabled on demand. Good point. But, I don't know the answer. I will test and check more the case. Do you think that dynamically changing Hardware Acceleration Policy isn't supported in GTK port?

(In reply to Simon Fraser (smfr) from comment #10) > It would be much more reliable if AC were always enabled in GTK. Yeah, I know Appple devs want to remove non-AC mode support from WebKit2. However, GTK, WPE, PlayStation and WinCairo is still using non-AC mode in WebKit2. WPE port is disabling accelerated compositing if it failed to create EGL context. https://github.com/WebKit/WebKit/blob/f0f02bbbc09b40caf023955b32ca5c37639346f2/Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.cpp#L251,L252

(In reply to Fujii Hironori from comment #11) > (In reply to Carlos Garcia Campos from comment #9) > > hmm, does this mean that it would be possible to always use the async > > scrolling for AC mode? We currently enable it only when forced, assuming > > that it was needed to be enabled from the very beginning and it couldn't be > > enabled/disabled on demand. > > Good point. But, I don't know the answer. I will test and check more the > case. I tested dynamically enabling/disabling force compositing mode by changing Hardware Acceleration Policy in GTK MiniBrowser. It works nicely. Also tested setThreadedScrollingEnabled(true) in "ondemand" Hardware Acceleration Policy. It also works nicely.

(In reply to Simon Fraser (smfr) from comment #10) > It would be much more reliable if AC were always enabled in GTK. We tried once and it didn't work for several reasons. First problem was memory consumption, the GL context created in every web process increases the memory usage. That's not a problem when you have just a few web views, but it became a problem when the browser had lots of tabs opened. Second problem was dealing with graphics drivers in linux. Our plan is to switch to AC mode only when building with GTK4, and try to improve the memory consumption (with tab suspension, for example). Or maybe the memory consumption is no longer a problem when using the GPU process. But for now, we have to keep the ondemand mode (note also that now ondemand never goes back to non-AC for a page after entering AC mode).

(In reply to Fujii Hironori from comment #13) > (In reply to Fujii Hironori from comment #11) > > (In reply to Carlos Garcia Campos from comment #9) > > > hmm, does this mean that it would be possible to always use the async > > > scrolling for AC mode? We currently enable it only when forced, assuming > > > that it was needed to be enabled from the very beginning and it couldn't be > > > enabled/disabled on demand. > > > > Good point. But, I don't know the answer. I will test and check more the > > case. > > I tested dynamically enabling/disabling force compositing mode by > changing Hardware Acceleration Policy in GTK MiniBrowser. It > works nicely. > > Also tested setThreadedScrollingEnabled(true) in "ondemand" > Hardware Acceleration Policy. It also works nicely. That's great, we should always enable async scrolling then, after this patch lands. Thanks!

Darin asked me no to add a platform prefix "[GTK]" in the bug title for cross-platform code change.

Created attachment 420247 [details] Patch

Comment on attachment 420247 [details] Patch Clearing flags on attachment: 420247 Committed r272867 (234101@main): <https://commits.webkit.org/234101@main>

All reviewed patches have been landed. Closing bug.

<rdar://problem/74357679>

It looks like the changes in https://trac.webkit.org/changeset/272867/webkit broke 50+ tests on Mac Debug wk2 https://build.webkit.org/results/Apple-BigSur-Debug-WK2-Tests/r272879%20(158)/results.html I am able to reproduce this on r272867 but not r272866

Reverted r272867 for reason: Broke 50+ fast/layoutformattingcontext/ tests on Mac Debug WK2 Committed r272891 (234124@main): <https://commits.webkit.org/234124@main>

Created attachment 420581 [details] absolute-positioned-box-with-inline-sibling-crash-log.txt Thank you for reverting my patch. fast/layoutformattingcontext tests were failing because WebPage::enterAcceleratedCompositingMode was called twice. The first time by RenderLayerCompositor::attachRootLayer, the second time by LayerController::attachRootLayer. These tests are setting LayoutFormattingContextEnabled=true in the test header.

I'm still investigating the assertion failure. I need more time. I'm going to split the patch and land the part of it. Bug 222529 – EventDispatcher::wheelEvent is accessing m_scrollingTrees without locking m_scrollingTreesMutex since r271235

*** This bug has been marked as a duplicate of bug 225512 ***