Skip to content

Commit 66ec94e

Browse files
kenrbkenrb
committed
Give OOPIF FrameViews their own scroll animation timelines and hosts
OOPIFs need independent scroll animation timelines and hosts, because they have separate compositors. Ultimately we need to associate a ScrollingCoordinator which each local frame root, but until that work is done this CL resolves some known scrolling problems in --site-per-process mode. BUG=675695 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2631853002 Cr-Commit-Position: refs/heads/master@{#444947} (cherry picked from commit 65c7df6) Review-Url: https://codereview.chromium.org/2654883002 . Cr-Commit-Position: refs/branch-heads/2987@{#62} Cr-Branched-From: ad51088-refs/heads/master@{#444943}
1 parent 1b0254b commit 66ec94e

14 files changed

+205
-34
lines changed

‎third_party/WebKit/LayoutTests/http/tests/misc/resources/cross-origin-subframe-for-scrolling.html

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
<!DOCTYPE html>
2+
<script src="/js-test-resources/js-test.js"></script>
3+
<div style="height: 300px"></div>
4+
5+
<script>
6+
setPrintTestResultsLazily();
7+
self.jsTestIsAsync = true;
8+
9+
window.onload = (() => {
10+
if (!window.eventSender || !window.internals) {
11+
debug("This test requires window.eventSender.");
12+
return;
13+
}
14+
internals.settings.setScrollAnimatorEnabled(true);
15+
});
16+
17+
function handleMessage(event) {
18+
if (event.data.hasOwnProperty('scrollBy')) {
19+
eventSender.mouseMoveTo(event.data.left + 5, event.data.top + 5);
20+
eventSender.mouseScrollBy(0, event.data.scrollBy);
21+
requestAnimationFrame(() => {setTimeout(() => {event.source.postMessage("", "*")}, 500)});
22+
} else {
23+
event.source.postMessage({scrollTop: document.documentElement.scrollTop}, "*");
24+
}
25+
}
26+
27+
window.addEventListener("message", handleMessage);
28+
</script>

‎third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes-expected.txt

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
Verify that two sibling cross-origin iframes both correctly scroll on MouseWheel events, as per https://crbug.com/675695.
2+
3+
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
4+
5+
6+
PASS event.data.scrollTop is 40
7+
PASS event.data.scrollTop is 40
8+
PASS successfullyParsed is true
9+
10+
TEST COMPLETE
11+

‎third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
<!DOCTYPE html>
2+
<script src="/js-test-resources/js-test.js"></script>
3+
<iframe id="target-iframe1" src="http://localhost:8080/misc/resources/cross-origin-subframe-for-scrolling.html" style="height: 100px; width: 100px; overflow-y: scroll; position: absolute; left: 50px; top: 50px"></iframe>
4+
<iframe id="target-iframe2" src="http://localhost:8080/misc/resources/cross-origin-subframe-for-scrolling.html" style="height: 100px; width: 100px; overflow-y: scroll; position: absolute; left: 50px; top: 200px"></iframe>
5+
6+
<script>
7+
description("Verify that two sibling cross-origin iframes both correctly scroll on MouseWheel events, as per https://crbug.com/675695.");
8+
9+
// States:
10+
// 0 => Scroll sent to iframe1
11+
// 1 => Fetching scroll offset from iframe1
12+
// 2 => Scroll sent to iframe2
13+
// 3 => Fetching scroll offset from iframe2
14+
var state = 0;
15+
var iframe1 = document.getElementById("target-iframe1");
16+
var iframe2 = document.getElementById("target-iframe2");
17+
setPrintTestResultsLazily();
18+
self.jsTestIsAsync = true;
19+
20+
function handleMessage(event) {
21+
if (state == 0) {
22+
iframe1.contentWindow.postMessage("", "*");
23+
state = 1;
24+
} else if (state == 1) {
25+
shouldBeEqualToNumber("event.data.scrollTop", 40);
26+
state = 2;
27+
iframe2.contentWindow.postMessage({scrollBy: -1, left: iframe2.offsetLeft, top: iframe2.offsetTop}, "*");
28+
iframe2.contentWindow.postMessage("", "*");
29+
} else if (state == 2) {
30+
iframe1.contentWindow.postMessage("", "*");
31+
state = 3;
32+
} else if (state == 3) {
33+
shouldBeEqualToNumber("event.data.scrollTop", 40);
34+
finishJSTest();
35+
}
36+
}
37+
38+
window.addEventListener("message", handleMessage);
39+
40+
iframe1.onload = (() => {
41+
iframe1.contentWindow.postMessage({scrollBy: -1, left: iframe1.offsetLeft, top: iframe1.offsetTop}, "*");
42+
});
43+
44+
</script>

‎third_party/WebKit/Source/core/frame/FrameView.cpp

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -566,11 +566,30 @@ ScrollingCoordinator* FrameView::scrollingCoordinator() const {
566566
}
567567

568568
CompositorAnimationHost* FrameView::compositorAnimationHost() const {
569+
// When m_animationHost is not nullptr, this is the FrameView for an OOPIF.
570+
if (m_animationHost)
571+
return m_animationHost.get();
572+
573+
if (m_frame->localFrameRoot() != m_frame)
574+
return m_frame->localFrameRoot()->view()->compositorAnimationHost();
575+
576+
if (!m_frame->isMainFrame())
577+
return nullptr;
578+
569579
ScrollingCoordinator* c = scrollingCoordinator();
570580
return c ? c->compositorAnimationHost() : nullptr;
571581
}
572582

573583
CompositorAnimationTimeline* FrameView::compositorAnimationTimeline() const {
584+
if (m_animationTimeline)
585+
return m_animationTimeline.get();
586+
587+
if (m_frame->localFrameRoot() != m_frame)
588+
return m_frame->localFrameRoot()->view()->compositorAnimationTimeline();
589+
590+
if (!m_frame->isMainFrame())
591+
return nullptr;
592+
574593
ScrollingCoordinator* c = scrollingCoordinator();
575594
return c ? c->compositorAnimationTimeline() : nullptr;
576595
}
@@ -4999,4 +5018,14 @@ void FrameView::applyTransformForTopFrameSpace(TransformState& transformState) {
49995018
LayoutSize(-viewportIntersectionRect.x(), -viewportIntersectionRect.y()));
50005019
}
50015020

5021+
void FrameView::setAnimationTimeline(
5022+
std::unique_ptr<CompositorAnimationTimeline> timeline) {
5023+
m_animationTimeline = std::move(timeline);
5024+
}
5025+
5026+
void FrameView::setAnimationHost(
5027+
std::unique_ptr<CompositorAnimationHost> host) {
5028+
m_animationHost = std::move(host);
5029+
}
5030+
50025031
} // namespace blink

‎third_party/WebKit/Source/core/frame/FrameView.h

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@
4040
#include "core/paint/ScrollbarManager.h"
4141
#include "platform/RuntimeEnabledFeatures.h"
4242
#include "platform/Widget.h"
43+
#include "platform/animation/CompositorAnimationHost.h"
44+
#include "platform/animation/CompositorAnimationTimeline.h"
4345
#include "platform/geometry/IntRect.h"
4446
#include "platform/geometry/LayoutRect.h"
4547
#include "platform/graphics/Color.h"
@@ -819,6 +821,14 @@ class CORE_EXPORT FrameView final
819821

820822
void applyTransformForTopFrameSpace(TransformState&);
821823

824+
// TODO(kenrb): These are temporary methods pending resolution of
825+
// https://crbug.com/680606. Animation timelines and hosts for scrolling
826+
// are normally owned by ScrollingCoordinator, but there is only one
827+
// of those objects per page. To get around this, we temporarily stash a
828+
// unique timeline and host on each OOPIF FrameView.
829+
void setAnimationTimeline(std::unique_ptr<CompositorAnimationTimeline>);
830+
void setAnimationHost(std::unique_ptr<CompositorAnimationHost>);
831+
822832
protected:
823833
// Scroll the content via the compositor.
824834
bool scrollContentsFastPath(const IntSize& scrollDelta);
@@ -1190,6 +1200,10 @@ class CORE_EXPORT FrameView final
11901200
// The size of the vector depends on the number of
11911201
// main thread scrolling reasons.
11921202
Vector<int> m_mainThreadScrollingReasonsCounter;
1203+
1204+
// TODO(kenrb): Remove these when https://crbug.com/680606 is resolved.
1205+
std::unique_ptr<CompositorAnimationTimeline> m_animationTimeline;
1206+
std::unique_ptr<CompositorAnimationHost> m_animationHost;
11931207
};
11941208

11951209
inline void FrameView::incrementVisuallyNonEmptyCharacterCount(unsigned count) {

‎third_party/WebKit/Source/core/frame/VisualViewport.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -801,12 +801,14 @@ bool VisualViewport::shouldDisableDesktopWorkarounds() const {
801801
}
802802

803803
CompositorAnimationHost* VisualViewport::compositorAnimationHost() const {
804+
DCHECK(frameHost().page().mainFrame()->isLocalFrame());
804805
ScrollingCoordinator* c = frameHost().page().scrollingCoordinator();
805806
return c ? c->compositorAnimationHost() : nullptr;
806807
}
807808

808809
CompositorAnimationTimeline* VisualViewport::compositorAnimationTimeline()
809810
const {
811+
DCHECK(frameHost().page().mainFrame()->isLocalFrame());
810812
ScrollingCoordinator* c = frameHost().page().scrollingCoordinator();
811813
return c ? c->compositorAnimationTimeline() : nullptr;
812814
}

‎third_party/WebKit/Source/core/page/Page.cpp

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -507,14 +507,16 @@ DEFINE_TRACE(Page) {
507507
PageVisibilityNotifier::trace(visitor);
508508
}
509509

510-
void Page::layerTreeViewInitialized(WebLayerTreeView& layerTreeView) {
510+
void Page::layerTreeViewInitialized(WebLayerTreeView& layerTreeView,
511+
FrameView* view) {
511512
if (scrollingCoordinator())
512-
scrollingCoordinator()->layerTreeViewInitialized(layerTreeView);
513+
scrollingCoordinator()->layerTreeViewInitialized(layerTreeView, view);
513514
}
514515

515-
void Page::willCloseLayerTreeView(WebLayerTreeView& layerTreeView) {
516+
void Page::willCloseLayerTreeView(WebLayerTreeView& layerTreeView,
517+
FrameView* view) {
516518
if (m_scrollingCoordinator)
517-
m_scrollingCoordinator->willCloseLayerTreeView(layerTreeView);
519+
m_scrollingCoordinator->willCloseLayerTreeView(layerTreeView, view);
518520
}
519521

520522
void Page::willBeDestroyed() {

‎third_party/WebKit/Source/core/page/Page.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -226,8 +226,8 @@ class CORE_EXPORT Page final : public GarbageCollectedFinalized<Page>,
226226

227227
DECLARE_TRACE();
228228

229-
void layerTreeViewInitialized(WebLayerTreeView&);
230-
void willCloseLayerTreeView(WebLayerTreeView&);
229+
void layerTreeViewInitialized(WebLayerTreeView&, FrameView*);
230+
void willCloseLayerTreeView(WebLayerTreeView&, FrameView*);
231231

232232
void willBeDestroyed();
233233

‎third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp

Lines changed: 37 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -508,8 +508,19 @@ bool ScrollingCoordinator::scrollableAreaScrollLayerDidChange(
508508
isForRootLayer(scrollableArea))
509509
m_page->chromeClient().registerViewportLayers();
510510

511-
scrollableArea->layerForScrollingDidChange(
512-
m_programmaticScrollAnimatorTimeline.get());
511+
CompositorAnimationTimeline* timeline;
512+
// FrameView::compositorAnimationTimeline() can indirectly return
513+
// m_programmaticScrollAnimatorTimeline if it does not have its own
514+
// timeline.
515+
if (scrollableArea->isFrameView()) {
516+
timeline = toFrameView(scrollableArea)->compositorAnimationTimeline();
517+
} else if (scrollableArea->isPaintLayerScrollableArea()) {
518+
timeline = toPaintLayerScrollableArea(scrollableArea)
519+
->compositorAnimationTimeline();
520+
} else {
521+
timeline = m_programmaticScrollAnimatorTimeline.get();
522+
}
523+
scrollableArea->layerForScrollingDidChange(timeline);
513524

514525
return !!webLayer;
515526
}
@@ -852,20 +863,37 @@ void ScrollingCoordinator::setShouldUpdateScrollLayerPositionOnMainThread(
852863
}
853864

854865
void ScrollingCoordinator::layerTreeViewInitialized(
855-
WebLayerTreeView& layerTreeView) {
866+
WebLayerTreeView& layerTreeView,
867+
FrameView* view) {
856868
if (Platform::current()->isThreadedAnimationEnabled() &&
857869
layerTreeView.compositorAnimationHost()) {
858-
m_animationHost = WTF::makeUnique<CompositorAnimationHost>(
859-
layerTreeView.compositorAnimationHost());
860-
m_programmaticScrollAnimatorTimeline =
870+
std::unique_ptr<CompositorAnimationTimeline> timeline =
861871
CompositorAnimationTimeline::create();
862-
m_animationHost->addTimeline(*m_programmaticScrollAnimatorTimeline.get());
872+
std::unique_ptr<CompositorAnimationHost> host =
873+
WTF::makeUnique<CompositorAnimationHost>(
874+
layerTreeView.compositorAnimationHost());
875+
if (view && view->frame().localFrameRoot() != m_page->mainFrame()) {
876+
view->setAnimationHost(std::move(host));
877+
view->setAnimationTimeline(std::move(timeline));
878+
view->compositorAnimationHost()->addTimeline(
879+
*view->compositorAnimationTimeline());
880+
} else {
881+
m_animationHost = std::move(host);
882+
m_programmaticScrollAnimatorTimeline = std::move(timeline);
883+
m_animationHost->addTimeline(*m_programmaticScrollAnimatorTimeline.get());
884+
}
863885
}
864886
}
865887

866888
void ScrollingCoordinator::willCloseLayerTreeView(
867-
WebLayerTreeView& layerTreeView) {
868-
if (m_programmaticScrollAnimatorTimeline) {
889+
WebLayerTreeView& layerTreeView,
890+
FrameView* view) {
891+
if (view && view->frame().localFrameRoot() != m_page->mainFrame()) {
892+
view->compositorAnimationHost()->removeTimeline(
893+
*view->compositorAnimationTimeline());
894+
view->setAnimationTimeline(nullptr);
895+
view->setAnimationHost(nullptr);
896+
} else if (m_programmaticScrollAnimatorTimeline) {
869897
m_animationHost->removeTimeline(
870898
*m_programmaticScrollAnimatorTimeline.get());
871899
m_programmaticScrollAnimatorTimeline = nullptr;

‎third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,12 @@ class CORE_EXPORT ScrollingCoordinator final
6262
~ScrollingCoordinator();
6363
DECLARE_TRACE();
6464

65-
void layerTreeViewInitialized(WebLayerTreeView&);
66-
void willCloseLayerTreeView(WebLayerTreeView&);
65+
// The FrameView argument is optional, nullptr causes the the scrolling
66+
// animation host and timeline to be owned by the ScrollingCoordinator. When
67+
// not null, the host and timeline are attached to the specified FrameView.
68+
// A FrameView only needs to own them when it is the view for an OOPIF.
69+
void layerTreeViewInitialized(WebLayerTreeView&, FrameView*);
70+
void willCloseLayerTreeView(WebLayerTreeView&, FrameView*);
6771

6872
void willBeDestroyed();
6973

‎third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1882,18 +1882,12 @@ void PaintLayerScrollableArea::resetRebuildScrollbarLayerFlags() {
18821882

18831883
CompositorAnimationHost* PaintLayerScrollableArea::compositorAnimationHost()
18841884
const {
1885-
if (ScrollingCoordinator* coordinator = getScrollingCoordinator())
1886-
return coordinator->compositorAnimationHost();
1887-
1888-
return nullptr;
1885+
return m_layer.layoutObject()->frameView()->compositorAnimationHost();
18891886
}
18901887

18911888
CompositorAnimationTimeline*
18921889
PaintLayerScrollableArea::compositorAnimationTimeline() const {
1893-
if (ScrollingCoordinator* coordinator = getScrollingCoordinator())
1894-
return coordinator->compositorAnimationTimeline();
1895-
1896-
return nullptr;
1890+
return m_layer.layoutObject()->frameView()->compositorAnimationTimeline();
18971891
}
18981892

18991893
PaintLayerScrollableArea*

‎third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -651,8 +651,10 @@ bool WebFrameWidgetImpl::isAcceleratedCompositingActive() const {
651651
}
652652

653653
void WebFrameWidgetImpl::willCloseLayerTreeView() {
654-
if (m_layerTreeView)
655-
page()->willCloseLayerTreeView(*m_layerTreeView);
654+
if (m_layerTreeView) {
655+
page()->willCloseLayerTreeView(*m_layerTreeView,
656+
m_localRoot->frame()->view());
657+
}
656658

657659
setIsAcceleratedCompositingActive(false);
658660
m_mutator = nullptr;
@@ -996,8 +998,10 @@ void WebFrameWidgetImpl::initializeLayerTreeView() {
996998
devTools->layerTreeViewChanged(m_layerTreeView);
997999

9981000
page()->settings().setAcceleratedCompositingEnabled(m_layerTreeView);
999-
if (m_layerTreeView)
1000-
page()->layerTreeViewInitialized(*m_layerTreeView);
1001+
if (m_layerTreeView) {
1002+
page()->layerTreeViewInitialized(*m_layerTreeView,
1003+
m_localRoot->frame()->view());
1004+
}
10011005

10021006
// FIXME: only unittests, click to play, Android priting, and printing (for
10031007
// headers and footers) make this assert necessary. We should make them not

‎third_party/WebKit/Source/web/WebPagePopupImpl.cpp

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -395,7 +395,12 @@ void WebPagePopupImpl::setIsAcceleratedCompositingActive(bool enter) {
395395
m_isAcceleratedCompositingActive = true;
396396
m_animationHost = WTF::makeUnique<CompositorAnimationHost>(
397397
m_layerTreeView->compositorAnimationHost());
398-
m_page->layerTreeViewInitialized(*m_layerTreeView);
398+
m_page->layerTreeViewInitialized(*m_layerTreeView,
399+
m_popupClient->ownerElement()
400+
.document()
401+
.frame()
402+
->localFrameRoot()
403+
->view());
399404
} else {
400405
m_isAcceleratedCompositingActive = false;
401406
m_animationHost = nullptr;
@@ -412,8 +417,14 @@ void WebPagePopupImpl::beginFrame(double lastFrameTimeMonotonic) {
412417
}
413418

414419
void WebPagePopupImpl::willCloseLayerTreeView() {
415-
if (m_page && m_layerTreeView)
416-
m_page->willCloseLayerTreeView(*m_layerTreeView);
420+
if (m_page && m_layerTreeView) {
421+
m_page->willCloseLayerTreeView(*m_layerTreeView,
422+
m_popupClient->ownerElement()
423+
.document()
424+
.frame()
425+
->localFrameRoot()
426+
->view());
427+
}
417428

418429
setIsAcceleratedCompositingActive(false);
419430
m_layerTreeView = nullptr;

‎third_party/WebKit/Source/web/WebViewImpl.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2515,7 +2515,7 @@ void WebViewImpl::willCloseLayerTreeView() {
25152515
}
25162516

25172517
if (m_layerTreeView)
2518-
page()->willCloseLayerTreeView(*m_layerTreeView);
2518+
page()->willCloseLayerTreeView(*m_layerTreeView, nullptr);
25192519

25202520
setRootLayer(nullptr);
25212521
m_animationHost = nullptr;
@@ -3956,7 +3956,7 @@ void WebViewImpl::initializeLayerTreeView() {
39563956

39573957
m_page->settings().setAcceleratedCompositingEnabled(m_layerTreeView);
39583958
if (m_layerTreeView)
3959-
m_page->layerTreeViewInitialized(*m_layerTreeView);
3959+
m_page->layerTreeViewInitialized(*m_layerTreeView, nullptr);
39603960

39613961
// FIXME: only unittests, click to play, Android printing, and printing (for
39623962
// headers and footers) make this assert necessary. We should make them not

0 commit comments

Comments
 (0)