Make ScrollingCoordinator be per-compositor rather than per-page in a process |
|||||||
Issue descriptionFollow up bug from issue 675695 . ScrollingCoordinator is currently owned by Page, of which there is only one for a given page loaded in a process. However, internally, ScrollingCoordinator assumes that it is associated with a single compositor, which is not true in a page with multiple same-process OOPIFs. To address this inconsistency we need to associate a ScrollingCoordinator with each local frame root. Possibly it could be owned by FrameView, although I haven't really investigated this yet.
,
Jan 20 2017
,
Mar 7 2017
,
May 5 2017
Is someone actively working on this? Is it really a P!?
,
May 5 2017
No, and no. I don't recall, but that priority was probably not deliberate.
,
Feb 7 2018
Chipping away at this ...
,
Feb 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0e7b308819ce2a660b1b86393f5fa56ff817c369 commit 0e7b308819ce2a660b1b86393f5fa56ff817c369 Author: W. James MacLean <wjmaclean@chromium.org> Date: Fri Feb 09 20:11:32 2018 Use LocalFrames in ScrollingCoordinator compositing update. This CL converts the remaining methods called from UpdateAfterCompositingChangeIfNeeded() to use a provided LocalFrame instead of relying on the page's main frame, which will often not be local when OOPIFs are in use. This removes the TODO associated with bug 680606. Bug: 803227, 680606 Change-Id: I42e054e666c0701207a4eafc60cf00a1e6be12e2 Reviewed-on: https://chromium-review.googlesource.com/905273 Reviewed-by: David Bokan <bokan@chromium.org> Commit-Queue: James MacLean <wjmaclean@chromium.org> Cr-Commit-Position: refs/heads/master@{#535797} [modify] https://crrev.com/0e7b308819ce2a660b1b86393f5fa56ff817c369/third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp [modify] https://crrev.com/0e7b308819ce2a660b1b86393f5fa56ff817c369/third_party/WebKit/Source/core/frame/EventHandlerRegistry.h [modify] https://crrev.com/0e7b308819ce2a660b1b86393f5fa56ff817c369/third_party/WebKit/Source/core/frame/LocalFrameView.cpp [modify] https://crrev.com/0e7b308819ce2a660b1b86393f5fa56ff817c369/third_party/WebKit/Source/core/frame/LocalFrameView.h [modify] https://crrev.com/0e7b308819ce2a660b1b86393f5fa56ff817c369/third_party/WebKit/Source/core/layout/LayoutBox.cpp [modify] https://crrev.com/0e7b308819ce2a660b1b86393f5fa56ff817c369/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp [modify] https://crrev.com/0e7b308819ce2a660b1b86393f5fa56ff817c369/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h [modify] https://crrev.com/0e7b308819ce2a660b1b86393f5fa56ff817c369/third_party/WebKit/Source/core/testing/Internals.cpp
,
Feb 9 2018
,
Feb 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fcedfca07b3c7d02d8912b93d14546aace495840 commit fcedfca07b3c7d02d8912b93d14546aace495840 Author: W. James MacLean <wjmaclean@chromium.org> Date: Tue Feb 20 18:28:09 2018 Remove TouchEventRects hack from RenderWidgetCompositor Prior to https://chromium-review.googlesource.com/905273, ScrollingCoordinator did not update touch event rects for OOPIF subframes. To make sure that touch handlers in OOPIFs remained functional, the code had a hack to set the entire root layer layer of an OOPIF to be a touch event rect if the OOPIF contained any touch handlers at all. Now ScrollingCoordinator should be handling these rects for OOPIFs, so this CL removes the hack. Bug: 680606 Change-Id: I6a1b7badca7c9752c298a97ab17cbe28775e0eb8 Reviewed-on: https://chromium-review.googlesource.com/923583 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: James MacLean <wjmaclean@chromium.org> Cr-Commit-Position: refs/heads/master@{#537824} [modify] https://crrev.com/fcedfca07b3c7d02d8912b93d14546aace495840/content/renderer/gpu/render_widget_compositor.cc [modify] https://crrev.com/fcedfca07b3c7d02d8912b93d14546aace495840/content/renderer/gpu/render_widget_compositor.h [modify] https://crrev.com/fcedfca07b3c7d02d8912b93d14546aace495840/third_party/WebKit/Source/core/frame/LocalFrameView.cpp [modify] https://crrev.com/fcedfca07b3c7d02d8912b93d14546aace495840/third_party/WebKit/Source/core/loader/EmptyClients.h [modify] https://crrev.com/fcedfca07b3c7d02d8912b93d14546aace495840/third_party/WebKit/Source/core/page/ChromeClient.h [modify] https://crrev.com/fcedfca07b3c7d02d8912b93d14546aace495840/third_party/WebKit/Source/core/page/ChromeClientImpl.cpp [modify] https://crrev.com/fcedfca07b3c7d02d8912b93d14546aace495840/third_party/WebKit/Source/core/page/ChromeClientImpl.h
,
Feb 22 2018
Able to reproduce the issue on Windows 10, mac 10.13.3 and Ubuntu 14.04 using chrome reported version #57.0.2956.1 as per the follow up issue: 675695 . Verified the fix on Mac 10.13.3, Win-10 and Ubuntu 14.04 using latest chrome version #66.0.3352.0 as per the follow up issue: 675695 . Attaching screen cast for reference. Observed that both iframes scroll with the mouse wheel. Hence, the fix is working as expected. Adding the verified labels. Thanks...!!
,
Feb 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/446abe5ce915a0ff48a9a3860c21ca10bb57cd77 commit 446abe5ce915a0ff48a9a3860c21ca10bb57cd77 Author: Ken Buchanan <kenrb@chromium.org> Date: Fri Feb 23 18:05:50 2018 Refactor LocalFrameView state for ScrollingCoordinator This CL adds a new class ScrollingCoordinatorContext which contains state that used to belong to ScrollingCoordinator but needs to be duplicated for each local frame root when OOPIFs are instantiated. It is lazily created on LocalFrameViews that are belong to local roots (i.e. OOPIFs or main frames). This also moves layers_with_touch_rects_ out of ScrollingCoordinator, because reuse of that HashSet between frame roots was causing the crash in bug 812364 . Bug: 680606, 812364 Change-Id: I09feb2add534b460dd9425fb918a6c856e325efb Reviewed-on: https://chromium-review.googlesource.com/922702 Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Commit-Queue: Ken Buchanan <kenrb@chromium.org> Cr-Commit-Position: refs/heads/master@{#538819} [modify] https://crrev.com/446abe5ce915a0ff48a9a3860c21ca10bb57cd77/third_party/WebKit/Source/core/animation/CompositorAnimationsTest.cpp [modify] https://crrev.com/446abe5ce915a0ff48a9a3860c21ca10bb57cd77/third_party/WebKit/Source/core/animation/DocumentAnimations.cpp [modify] https://crrev.com/446abe5ce915a0ff48a9a3860c21ca10bb57cd77/third_party/WebKit/Source/core/frame/LocalFrameView.cpp [modify] https://crrev.com/446abe5ce915a0ff48a9a3860c21ca10bb57cd77/third_party/WebKit/Source/core/frame/LocalFrameView.h [modify] https://crrev.com/446abe5ce915a0ff48a9a3860c21ca10bb57cd77/third_party/WebKit/Source/core/page/BUILD.gn [modify] https://crrev.com/446abe5ce915a0ff48a9a3860c21ca10bb57cd77/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp [modify] https://crrev.com/446abe5ce915a0ff48a9a3860c21ca10bb57cd77/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h [add] https://crrev.com/446abe5ce915a0ff48a9a3860c21ca10bb57cd77/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinatorContext.cpp [add] https://crrev.com/446abe5ce915a0ff48a9a3860c21ca10bb57cd77/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinatorContext.h
,
Mar 1 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5bed7ef3c3cfd01e4070792fdce590acbafab482 commit 5bed7ef3c3cfd01e4070792fdce590acbafab482 Author: W. James MacLean <wjmaclean@chromium.org> Date: Thu Mar 01 00:43:06 2018 ScrollableArea rects need to be in coords of LocalRoot's Document. This CL corrects a bug where, in the case of an OOPIF, the ScrollableArea rects are computed in the coordinate space of the main frame, when they need to be in the coordinates of the LocalRoot's Document in order that the compositor, which is owned by the LocalRoot, can deal with them properly. The manifestation of the bug required a non-fast-scrollable rect to be recomputed after its localroot container was scrolled. In that case the coords change due to the scroll, when they shouldn't. This CL also updates Internals::nonFastScrollableRects() to work for OOPIF frames, and it adds code to force recalculation of the rects, otherwise it might be unable to detect if this change gets regressed (since scrolling is not sufficient to make the non-fast-scrollable rects change, something must also trigger their recalculation after the scroll). Bug: 807683, 680606 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: Iccb7a694f43e98265e1bfde3d334efe51d64a59a Reviewed-on: https://chromium-review.googlesource.com/922421 Reviewed-by: David Bokan <bokan@chromium.org> Reviewed-by: Ken Buchanan <kenrb@chromium.org> Commit-Queue: James MacLean <wjmaclean@chromium.org> Cr-Commit-Position: refs/heads/master@{#539959} [modify] https://crrev.com/5bed7ef3c3cfd01e4070792fdce590acbafab482/content/browser/site_per_process_hit_test_browsertest.cc [modify] https://crrev.com/5bed7ef3c3cfd01e4070792fdce590acbafab482/content/test/data/tall_page_with_local_iframe.html [modify] https://crrev.com/5bed7ef3c3cfd01e4070792fdce590acbafab482/third_party/WebKit/Source/core/frame/LocalFrameView.cpp [modify] https://crrev.com/5bed7ef3c3cfd01e4070792fdce590acbafab482/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp [modify] https://crrev.com/5bed7ef3c3cfd01e4070792fdce590acbafab482/third_party/WebKit/Source/core/testing/Internals.cpp [modify] https://crrev.com/5bed7ef3c3cfd01e4070792fdce590acbafab482/third_party/WebKit/Source/core/testing/Internals.h [modify] https://crrev.com/5bed7ef3c3cfd01e4070792fdce590acbafab482/third_party/WebKit/Source/core/testing/Internals.idl
,
Mar 1 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0396f9a5e71f868378f5a92651ad37c69aba5891 commit 0396f9a5e71f868378f5a92651ad37c69aba5891 Author: W. James MacLean <wjmaclean@chromium.org> Date: Thu Mar 01 16:15:47 2018 Make SitePerProcessInternalsHitTestBrowserTest multi-scale. Modify this text fixture to parameterize it over multiple device scale factors. Bug: 680606 Change-Id: I247afdf39ab9e640c711372ca6d07c66897d6bbd Reviewed-on: https://chromium-review.googlesource.com/943341 Reviewed-by: Ken Buchanan <kenrb@chromium.org> Commit-Queue: James MacLean <wjmaclean@chromium.org> Cr-Commit-Position: refs/heads/master@{#540173} [modify] https://crrev.com/0396f9a5e71f868378f5a92651ad37c69aba5891/content/browser/site_per_process_hit_test_browsertest.cc |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by majidvp@chromium.org
, Jan 20 2017