Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2631853002:
Give OOPIF FrameViews their own scroll animation timelines and hosts (Closed)
|
Created:
3 years, 11 months ago by kenrb Modified:
3 years, 11 months ago CC:
chromium-reviews, kenneth.christiansen, mlamouri+watch-blink_chromium.org, dcheng, blink-layers+watch_chromium.org, blink-reviews, kinuko+watch, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGive 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}
Committed: https://chromium.googlesource.com/chromium/src/+/65c7df6e695eb4435ccf5485498b34fd1629f898
Patch Set 1 #Patch Set 2 : Still buggy #Patch Set 3 : Seems to work right now #Patch Set 4 : Attempt to fix tests #
Total comments: 18
Patch Set 5 : Test added #
Total comments: 10
Patch Set 6 : Fixes from bokan comments #Messages
Total messages: 46 (35 generated)
The CQ bit was checked by kenrb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== [WIP] Give OOPIF FrameViews their own scroll animation timelines and hosts BUG=675695 ========== to ========== [WIP] Give OOPIF FrameViews their own scroll animation timelines and hosts BUG=675695 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by kenrb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [WIP] Give OOPIF FrameViews their own scroll animation timelines and hosts BUG=675695 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== 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 ==========
kenrb@chromium.org changed reviewers: + bokan@chromium.org
bokan@: PTAL? Is this a reasonable short term fix for the --site-per-process scrolling bug?
The CQ bit was checked by kenrb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kenrb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bokan@chromium.org changed reviewers: + ajuma@chromium.org
Please wrap the description at 80 cols. Also, this needs a test. I'm fine landing the test after since there's time pressure here, but please start that in a follow up. +ajuma@ since I'm not well versed with animations so a familiar pair of eyes might help. https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:494: CompositorAnimationTimeline* timeline; For my own clarification, each process has its own ScrollingCoordinator, but the issue is that there's just one, where we might have multiple LayerTrees in a single process. Is that right? https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:501: timeline = m_programmaticScrollAnimatorTimeline.get(); In this case, the scrollableArea is a PLSA but those seem to return the timeline that's associated with their FrameView so shouldn't we be returning scrollableArea->layoutBox()->frameView()->compositorAnimationTimeline? https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:853: layerTreeView.compositorAnimationHost()); This seems suspect (and did before your patch)...layerTreeView.compositorAnimationHost() returns and stores into a unique_ptr a raw pointer to RenderWidgetCompositor::animation_host_ which is itself a unique_ptr. Unless I'm missing something, we have two unique_ptrs to one object, no? https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1885: return m_layer.layoutObject()->frameView()->compositorAnimationHost(); VisualViewport also calls this via ScrollingCoordinator but I forget how VisualViewport works with OOPIF, does just the top level renderer have one? Or do we just rely on OOP renderers never calling into their own instances of it? https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:1002: LOG(ERROR) << "WFWI::initializeLayerTreeView(), this = " << (void*)this Is this logging leftover from debugging?
ajuma@chromium.org changed reviewers: + loyso@chromium.org
+loyso to have a look too Looks good to me overall, modulo bokan's comments.
Description was changed from ========== 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 ========== to ========== 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 ==========
The CQ bit was checked by kenrb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:494: CompositorAnimationTimeline* timeline; On 2017/01/17 20:40:15, bokan wrote: > For my own clarification, each process has its own ScrollingCoordinator, but the > issue is that there's just one, where we might have multiple LayerTrees in a > single process. Is that right? Correct. The problem reproduces when we navigate two iframes to the same origin as each other (but different from the parent), so they become OOPIFs in the same process. Then there are two compositors and LayerTrees, but only one ScrollingCoordinator. https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:501: timeline = m_programmaticScrollAnimatorTimeline.get(); On 2017/01/17 20:40:15, bokan wrote: > In this case, the scrollableArea is a PLSA but those seem to return the timeline > that's associated with their FrameView so shouldn't we be returning > scrollableArea->layoutBox()->frameView()->compositorAnimationTimeline? That sounds reasonable, and simpler, but it breaks tests. It looks like this method is called during attachment of the root graphics layer in the main frame, and at that time scrollableArea->layoutBox() returns nullptr, but the work done here appears to be important. https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:853: layerTreeView.compositorAnimationHost()); On 2017/01/17 20:40:15, bokan wrote: > This seems suspect (and did before your > patch)...layerTreeView.compositorAnimationHost() returns and stores into a > unique_ptr a raw pointer to RenderWidgetCompositor::animation_host_ which is > itself a unique_ptr. Unless I'm missing something, we have two unique_ptrs to > one object, no? This does look strange, and I don't know for sure if current behavior is intended, but that's not quite what happens. makeUnique copies the object, so we end up owning a distinct CompositorAnimationHost. https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1885: return m_layer.layoutObject()->frameView()->compositorAnimationHost(); On 2017/01/17 20:40:16, bokan wrote: > VisualViewport also calls this via ScrollingCoordinator but I forget how > VisualViewport works with OOPIF, does just the top level renderer have one? Or > do we just rely on OOP renderers never calling into their own instances of it? There are VisualViewports in OOPIF processes, but they mostly do nothing. I think we store the root viewport size in it because of cases where a remote main frame gets swapped to a local main frame, but that's it. https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:1002: LOG(ERROR) << "WFWI::initializeLayerTreeView(), this = " << (void*)this On 2017/01/17 20:40:16, bokan wrote: > Is this logging leftover from debugging? Yes, sorry, removed.
Ok, assuming my understanding is correct, the fix lgtm. Some requested changes to the tests but if you want to land the code separately to make the branch cut that's fine by me. https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:501: timeline = m_programmaticScrollAnimatorTimeline.get(); On 2017/01/19 19:07:40, kenrb wrote: > On 2017/01/17 20:40:15, bokan wrote: > > In this case, the scrollableArea is a PLSA but those seem to return the > timeline > > that's associated with their FrameView so shouldn't we be returning > > scrollableArea->layoutBox()->frameView()->compositorAnimationTimeline? > > That sounds reasonable, and simpler, but it breaks tests. It looks like this > method is called during attachment of the root graphics layer in the main frame, > and at that time scrollableArea->layoutBox() returns nullptr, but the work done > here appears to be important. Ok, so to see if I understand - for the following configuration: A1 A1 = main frame in process A / \ B1 B2 B1,B2 = OOPIF frames in process B The current behavior in this patch is: Scrolling A1 uses A::ScrollingCoordinator::m_programmaticScrollAnimatorTimeline Scrolling a <div> in A1 uses A::ScrollingCoordinator::m_programmaticScrollAnimatorTimeline Scrolling B1 uses B1::m_animationTimeline Scrolling B2 uses B2::m_animationTimeline Scrolling a <div> inside either B1 or B2 uses B::ScrollingCoordinator::m_programmaticScrollAnimatorTimeline? https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:853: layerTreeView.compositorAnimationHost()); On 2017/01/19 19:07:40, kenrb wrote: > On 2017/01/17 20:40:15, bokan wrote: > > This seems suspect (and did before your > > patch)...layerTreeView.compositorAnimationHost() returns and stores into a > > unique_ptr a raw pointer to RenderWidgetCompositor::animation_host_ which is > > itself a unique_ptr. Unless I'm missing something, we have two unique_ptrs to > > one object, no? > > This does look strange, and I don't know for sure if current behavior is > intended, but that's not quite what happens. makeUnique copies the object, so we > end up owning a distinct CompositorAnimationHost. Oh, nevermind, I confused makeUnique with wrapUnique. https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1885: return m_layer.layoutObject()->frameView()->compositorAnimationHost(); On 2017/01/19 19:07:40, kenrb wrote: > On 2017/01/17 20:40:16, bokan wrote: > > VisualViewport also calls this via ScrollingCoordinator but I forget how > > VisualViewport works with OOPIF, does just the top level renderer have one? Or > > do we just rely on OOP renderers never calling into their own instances of it? > > There are VisualViewports in OOPIF processes, but they mostly do nothing. I > think we store the root viewport size in it because of cases where a remote main > frame gets swapped to a local main frame, but that's it. Ok, in that case, if it works please add DCHECKs inside VisualViewport::compositorAnimationHost and compositorAnimationTimeline that ensure we never call those from an OOPIF https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:1002: LOG(ERROR) << "WFWI::initializeLayerTreeView(), this = " << (void*)this On 2017/01/19 19:07:40, kenrb wrote: > On 2017/01/17 20:40:16, bokan wrote: > > Is this logging leftover from debugging? > > Yes, sorry, removed. Acknowledged. https://codereview.chromium.org/2631853002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/misc/resources/cross-origin-subframe-for-scrolling.html (right): https://codereview.chromium.org/2631853002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/misc/resources/cross-origin-subframe-for-scrolling.html:2: <script src="/js-test-resources/js-test.js"></script> Why do you need js-test here? IIRC, window.testRunner had a flag you could use to turn on async tests. Also, testharness.js is now encouraged for writing layout tests. https://codereview.chromium.org/2631853002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/misc/resources/cross-origin-subframe-for-scrolling.html:19: eventSender.mouseMoveTo(event.data.left + 5, event.data.top + 5); Does event sender perform the scroll event in the renderer form which it's called (and that's why you can't perform these mouse events from the top frame?). https://codereview.chromium.org/2631853002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/misc/resources/cross-origin-subframe-for-scrolling.html:21: requestAnimationFrame(() => {setTimeout(() => {event.source.postMessage("", "*")}, 500)}); I'm weary of timeouts, they're a frequent source of flakiness. Could we put the postMessage in an onScroll handler and have the test simply check that scrollTop > 0? https://codereview.chromium.org/2631853002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html (right): https://codereview.chromium.org/2631853002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html:2: <script src="/js-test-resources/js-test.js"></script> testharness.js is now the preferred choice for LayoutTests (http://darobin.github.io/test-harness-tutorial/docs/using-testharness.html) https://codereview.chromium.org/2631853002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html: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> Question: what makes these iframes OOPIFs? What's the origin on the main frame in LayoutTests that run from an http server?
Thanks, I'll see if I can land as is and post a follow-up tomorrow with DCHECKs and an improved layout test. https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:501: timeline = m_programmaticScrollAnimatorTimeline.get(); On 2017/01/19 20:05:39, bokan wrote: > On 2017/01/19 19:07:40, kenrb wrote: > > On 2017/01/17 20:40:15, bokan wrote: > > > In this case, the scrollableArea is a PLSA but those seem to return the > > timeline > > > that's associated with their FrameView so shouldn't we be returning > > > scrollableArea->layoutBox()->frameView()->compositorAnimationTimeline? > > > > That sounds reasonable, and simpler, but it breaks tests. It looks like this > > method is called during attachment of the root graphics layer in the main > frame, > > and at that time scrollableArea->layoutBox() returns nullptr, but the work > done > > here appears to be important. > > Ok, so to see if I understand - for the following configuration: > > A1 A1 = main frame in process A > / \ > B1 B2 B1,B2 = OOPIF frames in process B > > > The current behavior in this patch is: > > Scrolling A1 uses A::ScrollingCoordinator::m_programmaticScrollAnimatorTimeline > Scrolling a <div> in A1 uses > A::ScrollingCoordinator::m_programmaticScrollAnimatorTimeline > > Scrolling B1 uses B1::m_animationTimeline > Scrolling B2 uses B2::m_animationTimeline > Scrolling a <div> inside either B1 or B2 uses > B::ScrollingCoordinator::m_programmaticScrollAnimatorTimeline? > > Scrolling a div in B1 or B2 should use B{1,2}::m_animationTimeline, because PaintLayerScrollableArea gets the timeline and animation host from its local frame root. https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1885: return m_layer.layoutObject()->frameView()->compositorAnimationHost(); On 2017/01/19 20:05:39, bokan wrote: > On 2017/01/19 19:07:40, kenrb wrote: > > On 2017/01/17 20:40:16, bokan wrote: > > > VisualViewport also calls this via ScrollingCoordinator but I forget how > > > VisualViewport works with OOPIF, does just the top level renderer have one? > Or > > > do we just rely on OOP renderers never calling into their own instances of > it? > > > > There are VisualViewports in OOPIF processes, but they mostly do nothing. I > > think we store the root viewport size in it because of cases where a remote > main > > frame gets swapped to a local main frame, but that's it. > > Ok, in that case, if it works please add DCHECKs inside > VisualViewport::compositorAnimationHost and compositorAnimationTimeline that > ensure we never call those from an OOPIF I will do this in a follow up CL. https://codereview.chromium.org/2631853002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/misc/resources/cross-origin-subframe-for-scrolling.html (right): https://codereview.chromium.org/2631853002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/misc/resources/cross-origin-subframe-for-scrolling.html:2: <script src="/js-test-resources/js-test.js"></script> On 2017/01/19 20:05:39, bokan wrote: > Why do you need js-test here? IIRC, window.testRunner had a flag you could use > to turn on async tests. > > Also, testharness.js is now encouraged for writing layout tests. I realized that after I had already written it this way. Will change. https://codereview.chromium.org/2631853002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/misc/resources/cross-origin-subframe-for-scrolling.html:19: eventSender.mouseMoveTo(event.data.left + 5, event.data.top + 5); On 2017/01/19 20:05:39, bokan wrote: > Does event sender perform the scroll event in the renderer form which it's > called (and that's why you can't perform these mouse events from the top > frame?). Yes, we don't have a way to send events across processes in layout tests at this point. https://codereview.chromium.org/2631853002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/misc/resources/cross-origin-subframe-for-scrolling.html:21: requestAnimationFrame(() => {setTimeout(() => {event.source.postMessage("", "*")}, 500)}); On 2017/01/19 20:05:39, bokan wrote: > I'm weary of timeouts, they're a frequent source of flakiness. Could we put the > postMessage in an onScroll handler and have the test simply check that scrollTop > > 0? Will update in follow up. https://codereview.chromium.org/2631853002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html (right): https://codereview.chromium.org/2631853002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html:2: <script src="/js-test-resources/js-test.js"></script> On 2017/01/19 20:05:39, bokan wrote: > testharness.js is now the preferred choice for LayoutTests > (http://darobin.github.io/test-harness-tutorial/docs/using-testharness.html) Acknowledged, will update. https://codereview.chromium.org/2631853002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html: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> On 2017/01/19 20:05:39, bokan wrote: > Question: what makes these iframes OOPIFs? What's the origin on the main frame > in LayoutTests that run from an http server? We get a different origin by changing the port number (80 vs 8080). The different origin causes these to be cross-process when --site-per-process flag is specified, and we have a bot that runs layout tests with that flag.
https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:501: timeline = m_programmaticScrollAnimatorTimeline.get(); On 2017/01/19 21:36:19, kenrb wrote: > On 2017/01/19 20:05:39, bokan wrote: > > On 2017/01/19 19:07:40, kenrb wrote: > > > On 2017/01/17 20:40:15, bokan wrote: > > > > In this case, the scrollableArea is a PLSA but those seem to return the > > > timeline > > > > that's associated with their FrameView so shouldn't we be returning > > > > scrollableArea->layoutBox()->frameView()->compositorAnimationTimeline? > > > > > > That sounds reasonable, and simpler, but it breaks tests. It looks like this > > > method is called during attachment of the root graphics layer in the main > > frame, > > > and at that time scrollableArea->layoutBox() returns nullptr, but the work > > done > > > here appears to be important. > > > > Ok, so to see if I understand - for the following configuration: > > > > A1 A1 = main frame in process A > > / \ > > B1 B2 B1,B2 = OOPIF frames in process B > > > > > > The current behavior in this patch is: > > > > Scrolling A1 uses > A::ScrollingCoordinator::m_programmaticScrollAnimatorTimeline > > Scrolling a <div> in A1 uses > > A::ScrollingCoordinator::m_programmaticScrollAnimatorTimeline > > > > Scrolling B1 uses B1::m_animationTimeline > > Scrolling B2 uses B2::m_animationTimeline > > Scrolling a <div> inside either B1 or B2 uses > > B::ScrollingCoordinator::m_programmaticScrollAnimatorTimeline? > > > > > > Scrolling a div in B1 or B2 should use B{1,2}::m_animationTimeline, because > PaintLayerScrollableArea gets the timeline and animation host from its local > frame root. In that case, we're passing the wrong timeline to it here then, no?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kenrb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kenrb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2631853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:501: timeline = m_programmaticScrollAnimatorTimeline.get(); On 2017/01/19 21:37:50, bokan wrote: > On 2017/01/19 21:36:19, kenrb wrote: > > On 2017/01/19 20:05:39, bokan wrote: > > > On 2017/01/19 19:07:40, kenrb wrote: > > > > On 2017/01/17 20:40:15, bokan wrote: > > > > > In this case, the scrollableArea is a PLSA but those seem to return the > > > > timeline > > > > > that's associated with their FrameView so shouldn't we be returning > > > > > scrollableArea->layoutBox()->frameView()->compositorAnimationTimeline? > > > > > > > > That sounds reasonable, and simpler, but it breaks tests. It looks like > this > > > > method is called during attachment of the root graphics layer in the main > > > frame, > > > > and at that time scrollableArea->layoutBox() returns nullptr, but the work > > > done > > > > here appears to be important. > > > > > > Ok, so to see if I understand - for the following configuration: > > > > > > A1 A1 = main frame in process A > > > / \ > > > B1 B2 B1,B2 = OOPIF frames in process B > > > > > > > > > The current behavior in this patch is: > > > > > > Scrolling A1 uses > > A::ScrollingCoordinator::m_programmaticScrollAnimatorTimeline > > > Scrolling a <div> in A1 uses > > > A::ScrollingCoordinator::m_programmaticScrollAnimatorTimeline > > > > > > Scrolling B1 uses B1::m_animationTimeline > > > Scrolling B2 uses B2::m_animationTimeline > > > Scrolling a <div> inside either B1 or B2 uses > > > B::ScrollingCoordinator::m_programmaticScrollAnimatorTimeline? > > > > > > > > > > Scrolling a div in B1 or B2 should use B{1,2}::m_animationTimeline, because > > PaintLayerScrollableArea gets the timeline and animation host from its local > > frame root. > > In that case, we're passing the wrong timeline to it here then, no? Yes, that's a bug. I just uploaded a CL that fixes it, and also adds the DCHECKs in VisualViewport.
thanks, still lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kenrb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484876603844630,
"parent_rev": "590011e3f3b68faf7f68592b3b931181e0cb4672", "commit_rev":
"65c7df6e695eb4435ccf5485498b34fd1629f898"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/65c7df6e695eb4435ccf5485498b... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/65c7df6e695eb4435ccf5485498b... |
