Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2785533003:
Animated scroll shouldn't consume unhandled scrolls for OOPIFs. (Closed)
DescriptionAnimated scroll shouldn't require main-frame for OOPIFs.
At present the animated scroll code for mouse wheels will mark as
SCROLL_ON_IMPL any attempt to scroll if there is no scrollable
layer, and in an OOPIF this causes the GestureScrollUpdate to be
marked as consumed when in fact it isn't. In turn, this prevents
the GestureScrollUpdate from bubbling to the embedder.
This CL fixes LayerTreeHostImpl::ScrollAnimated() to corrently report
such GestureScrollUpdate events as ignored, so they can bubble to the
embedder.
This CL has no behavioral changes for Mac, as it does not support
smooth scrolling.
BUG=701178
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2785533003
Cr-Commit-Position: refs/heads/master@{#465828}
Committed: https://chromium.googlesource.com/chromium/src/+/a755e30003e89da8ed1ee6f9e4e039acb395e3af
Patch Set 1 #Patch Set 2 : Remove check for OOPIF in LTHI, fix DCHECK issue. #Patch Set 3 : Fix test compilation. #Patch Set 4 : Fix handling of scroll state in test mocks. #Patch Set 5 : Fix test. #Patch Set 6 : Rebase to master@{#463250}. #Patch Set 7 : Ooops, uncomment test fix! #Patch Set 8 : More rebase fixes. #Patch Set 9 : Plumb subframe flag to compositor. #Patch Set 10 : Fix Android compile. #
Total comments: 10
Patch Set 11 : Address suggestions, adjust timeout. #Patch Set 12 : Disable scroll bubbling test on Android. #
Total comments: 2
Patch Set 13 : Remove browser-side DCHECK mods. #Patch Set 14 : Rebase to master@{#465268}. #
Total comments: 12
Patch Set 15 : Address nits, add comments. #Depends on Patchset: Messages
Total messages: 104 (72 generated)
Description was changed from ========== Animated scroll shouldn't require main-frame for OOPIFs. At present the animated scroll code for mouse wheels will terminate any attempt to scroll if there is no scrollable layer, and in an OOPIF this causes the GestureScrollUpdate to be marked as consumed when in fact it isn't. In turn, this prevents the GestureScrollUpdate from bubbling to the embedder. This CL fixes LayerTreeHostImpl::ScrollAnimated() to corrently report such GestureScrollUpdate events as ignored, so they can bubble to the embedder. BUG=701178 ========== to ========== Animated scroll shouldn't require main-frame for OOPIFs. At present the animated scroll code for mouse wheels will terminate any attempt to scroll if there is no scrollable layer, and in an OOPIF this causes the GestureScrollUpdate to be marked as consumed when in fact it isn't. In turn, this prevents the GestureScrollUpdate from bubbling to the embedder. This CL fixes LayerTreeHostImpl::ScrollAnimated() to corrently report such GestureScrollUpdate events as ignored, so they can bubble to the embedder. BUG=701178 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by wjmaclean@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: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by wjmaclean@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 ========== Animated scroll shouldn't require main-frame for OOPIFs. At present the animated scroll code for mouse wheels will terminate any attempt to scroll if there is no scrollable layer, and in an OOPIF this causes the GestureScrollUpdate to be marked as consumed when in fact it isn't. In turn, this prevents the GestureScrollUpdate from bubbling to the embedder. This CL fixes LayerTreeHostImpl::ScrollAnimated() to corrently report such GestureScrollUpdate events as ignored, so they can bubble to the embedder. BUG=701178 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Animated scroll shouldn't require main-frame for OOPIFs. At present the animated scroll code for mouse wheels will terminate any attempt to scroll if there is no scrollable layer, and in an OOPIF this causes the GestureScrollUpdate to be marked as consumed when in fact it isn't. In turn, this prevents the GestureScrollUpdate from bubbling to the embedder. This CL fixes LayerTreeHostImpl::ScrollAnimated() to corrently report such GestureScrollUpdate events as ignored, so they can bubble to the embedder. BUG=701178 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
wjmaclean@chromium.org changed reviewers: + pdr@chromium.org
pdr@ - Does this seem sane to you?
On 2017/03/29 at 14:06:17, wjmaclean wrote: > pdr@ - Does this seem sane to you? There are some comments about OOPIF being broken for the root scroller (e.g., RootScrollerController.cpp) but I don't think we can assume that no scroll layer implies OOPIF. Is there a more explicit approach for detecting OOPIF? Is this testable?
On 2017/03/29 16:14:51, pdr. wrote: > On 2017/03/29 at 14:06:17, wjmaclean wrote: > > pdr@ - Does this seem sane to you? > > There are some comments about OOPIF being broken for the root scroller (e.g., > RootScrollerController.cpp) but I don't think we can assume that no scroll layer > implies OOPIF. Is there a more explicit approach for detecting OOPIF? You're right that a more-explicit means of testing for OOPIFs would be better ... I'll set something up. But at present, it is guaranteed that OOPIFs don't have viewport->MainScrollLayer()s. > Is this testable? Not sure how yet, but since it may depend on what we eventually end up doing, I'll hold off on devising a test until I get agreement on the approach.
The CQ bit was checked by wjmaclean@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...
Description was changed from ========== Animated scroll shouldn't require main-frame for OOPIFs. At present the animated scroll code for mouse wheels will terminate any attempt to scroll if there is no scrollable layer, and in an OOPIF this causes the GestureScrollUpdate to be marked as consumed when in fact it isn't. In turn, this prevents the GestureScrollUpdate from bubbling to the embedder. This CL fixes LayerTreeHostImpl::ScrollAnimated() to corrently report such GestureScrollUpdate events as ignored, so they can bubble to the embedder. BUG=701178 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Animated scroll shouldn't require main-frame for OOPIFs. At present the animated scroll code for mouse wheels will mark as SCROLL_ON_IMPL any attempt to scroll if there is no scrollable layer, and in an OOPIF this causes the GestureScrollUpdate to be marked as consumed when in fact it isn't. In turn, this prevents the GestureScrollUpdate from bubbling to the embedder. This CL fixes LayerTreeHostImpl::ScrollAnimated() to corrently report such GestureScrollUpdate events as ignored, so they can bubble to the embedder. It also modifies MouseWheelEventQueue to refer to it's client to know if a (non-synthetic) GestureScrollBegin/End are required, instead of hard-coding that state in the Queue itself. This is important since OOPIF scroll bubbling inserts its own Begin/Ends as needed, without the Queue's involvement. BUG=701178 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by wjmaclean@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by wjmaclean@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.
The CQ bit was checked by wjmaclean@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...
wjmaclean@chromium.org changed reviewers: + sahel@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
wjmaclean@chromium.org changed reviewers: + creis@chromium.org
creis@chromium.org: Please review changes in content/ sahel@ please verify the changes in MouseWheelEventQueue are OK. pdr@ - I've re-enabled a disabled scroll bubbling test that will work for this CL. It has a potential flakeyness in that there appears to be a separate issue where scroll animations that get updated in flight may not terminate properly, but I'm working with ajuma@ to track that down.
The CQ bit was checked by wjmaclean@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 wjmaclean@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
On 2017/04/10 13:56:15, wjmaclean wrote: > mailto:creis@chromium.org: Please review changes in > > content/ > > sahel@ please verify the changes in MouseWheelEventQueue are OK. > > pdr@ - I've re-enabled a disabled scroll bubbling test that will work for this > CL. It has a potential flakeyness in that there appears to be a separate issue > where scroll animations that get updated in flight may not terminate properly, > but I'm working with ajuma@ to track that down. The changes in MousWheelEventQueue are ok.
The CQ bit was checked by wjmaclean@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...
On 2017/04/10 13:56:15, wjmaclean wrote: > mailto:creis@chromium.org: Please review changes in > > content/ I'm at best an owner reviewer here-- I don't know this area well enough to review the change itself. Maybe I should do the owner's review after someone else (e.g., pdr@, kenrb@, etc) approves the approach?
[CC kenrb, site-isolation-reviews]
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
pdr@chromium.org changed reviewers: + bokan@chromium.org
@Bokan, could you review this?
On 2017/04/10 16:32:13, pdr. wrote: > @Bokan, could you review this? After a discussion with sahel@ I think we'll need to modify the code in ScrollAnimated() to only set SCROLL_IGNORED for OOPIF subframes after all, as apparently unhandled scrolls go back to the main thread (e.g. to be considered for touchpad pinch, etc.) and ScrollAnimated() scrolls apparently shouldn't do that.
The CQ bit was checked by wjmaclean@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...
wjmaclean@chromium.org changed reviewers: + aelias@chromium.org
aelias@chromium.org: Please review changes in cc/ ?
wjmaclean@chromium.org changed reviewers: + vollick@chromium.org
vollick@chromium.org: Please review changes in ui/compositor ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by wjmaclean@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2785533003/diff/180001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2785533003/diff/180001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3036: if (settings_.is_layer_tree_for_subframe) { Should this be `is_layer_tree_for_subframe && scroll_status.thread == SCROLL_ON_IMPL_THREAD`? If ScrollBegin says we should scroll on main because of MainThreadScrollingReasons, we shouldn't then switch it to ignore, right? https://codereview.chromium.org/2785533003/diff/180001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3039: scroll_status.thread = SCROLL_IGNORED; How does this work for non-animated scrolls? In the bug, creis@ says it's also broken when touch scrolling - that doesn't use ScrollAnimated. Should we be updating the non-animated path too? https://codereview.chromium.org/2785533003/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2785533003/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1108: base::debug::StackTrace().Print(); Left over debugging? https://codereview.chromium.org/2785533003/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/2785533003/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.h:357: bool is_in_gesture_scroll(blink::WebGestureDevice device) override; Touchscreen and Touchpad gesture event streams can differ for Flings but I can never remember how. Sahel should confirm that this change is ok.
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Addressed comments - ptal bokan@ ? https://codereview.chromium.org/2785533003/diff/180001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2785533003/diff/180001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3036: if (settings_.is_layer_tree_for_subframe) { On 2017/04/11 16:23:13, bokan wrote: > Should this be `is_layer_tree_for_subframe && scroll_status.thread == > SCROLL_ON_IMPL_THREAD`? If ScrollBegin says we should scroll on main because of > MainThreadScrollingReasons, we shouldn't then switch it to ignore, right? Good catch, thanks. https://codereview.chromium.org/2785533003/diff/180001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3039: scroll_status.thread = SCROLL_IGNORED; On 2017/04/11 16:23:14, bokan wrote: > How does this work for non-animated scrolls? In the bug, creis@ says it's also > broken when touch scrolling - that doesn't use ScrollAnimated. Should we be > updating the non-animated path too? I haven't been able to reproduce the touch scrolling issue. I think there *is* an issue with the current OOPIF scroll bubbling code if you follow a mousewheel scroll that bubbles with a mainframe touch scroll ... I plan on looking into that as a separate issue. https://codereview.chromium.org/2785533003/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2785533003/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1108: base::debug::StackTrace().Print(); On 2017/04/11 16:23:14, bokan wrote: > Left over debugging? Done. With the DCHECK not firing anymore, I missed removing that! :-)
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
I'm fine with the change to bubbling from ScrollAnimated, given how it currently works. I would just request an explicit test. In any case, I'm not an OWNER on any files here. I don't know enough about the other change to the queue to determine if Begin/End gestures are needed -- IMO it would be helpful to split that out from this CL. https://codereview.chromium.org/2785533003/diff/220001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2785533003/diff/220001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:1093: // Flaky: https://crbug.com/627238 If this used to be flaky, it means the functionality worked, right? So does that mean that this test isn't testing bubbling from animated wheel scrolls? If so, we need a test for that explicitly. If not, I'm confused.
https://codereview.chromium.org/2785533003/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/2785533003/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.h:357: bool is_in_gesture_scroll(blink::WebGestureDevice device) override; On 2017/04/11 16:23:14, bokan wrote: > Touchscreen and Touchpad gesture event streams can differ for Flings but I can > never remember how. Sahel should confirm that this change is ok. This function is used only for wheel/touchpad scrolling. For 4 combinations of existence of OOPIFs and scroll propagation vs latching, this logic works for all except the case that an OOPIF exists and scroll latching is enabled. This case is broken anyway and regardless of this change we need to fix it. Touchpad GFS comes after a GSE when wheel scroll latching is disabled (is_in_gesture_scroll is false), but when latching is enabled, similar to touch, GFS comes when is_in_gesture_scroll is true. In all cases GFS sets is_in_gesture_scroll to false.
https://codereview.chromium.org/2785533003/diff/220001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2785533003/diff/220001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:1093: // Flaky: https://crbug.com/627238 On 2017/04/11 22:18:29, bokan wrote: > If this used to be flaky, it means the functionality worked, right? Yes and no. :-) > So does that mean that this test isn't testing bubbling from animated wheel scrolls? No. > If so, we need a test for that explicitly. If not, I'm confused. It's a confusing story, but here goes: First, the existing test (without this CL) works for --disable-smooth-scrolling. Now, I've discovered that sometimes when the test runs, the scrolls sent to the child *aren't* animated, and they bubble as expected. But on other runs, the child scrolls are animated, and then the test fails without the code added by this CL at the end of ScrollAnimated(), which sets the status to SCROLL_IGNORED for unconsumed scrolls in children. *** I do not know why the scrolls sent to the child have unpredictable behaviour vis-á-vis animating or not. A third failure mode exists with this CL enabled, namely that the animation bubbled from the child hits a current animation in the parent. Normally this should just combine and work, but it seems the new scroll delta gets ignored, and the test timeouts waiting for the delta to be applied. I've filed a separate bug for that case. The delay I've added to the test is meant to minimise that case, but though it works well on my development machine, it still fails far too often on the bots, so the test may have to be delayed until the combining-animations bug is resolved.
On 2017/04/12 15:54:47, wjmaclean wrote: > https://codereview.chromium.org/2785533003/diff/220001/content/browser/site_p... > File content/browser/site_per_process_browsertest.cc (right): > > https://codereview.chromium.org/2785533003/diff/220001/content/browser/site_p... > content/browser/site_per_process_browsertest.cc:1093: // Flaky: > https://crbug.com/627238 > On 2017/04/11 22:18:29, bokan wrote: > > If this used to be flaky, it means the functionality worked, right? > > Yes and no. :-) > > > So does that mean that this test isn't testing bubbling from animated wheel > scrolls? > > No. > > > If so, we need a test for that explicitly. If not, I'm confused. > > It's a confusing story, but here goes: > > First, the existing test (without this CL) works for --disable-smooth-scrolling. > Now, I've discovered that sometimes when the test runs, the scrolls sent to the > child *aren't* animated, and they bubble as expected. But on other runs, the > child scrolls are animated, and then the test fails without the code added by > this CL at the end of ScrollAnimated(), which sets the status to SCROLL_IGNORED > for unconsumed scrolls in children. > > *** I do not know why the scrolls sent to the child have unpredictable behaviour > vis-á-vis animating or not. > > A third failure mode exists with this CL enabled, namely that the animation > bubbled from the child hits a current animation in the parent. Normally this > should just combine and work, but it seems the new scroll delta gets ignored, > and the test timeouts waiting for the delta to be applied. I've filed a separate > bug for that case. The delay I've added to the test is meant to minimise that > case, but though it works well on my development machine, it still fails far too > often on the bots, so the test may have to be delayed until the > combining-animations bug is resolved. ui/compositor lgtm.
https://codereview.chromium.org/2785533003/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/2785533003/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.h:357: bool is_in_gesture_scroll(blink::WebGestureDevice device) override; On 2017/04/12 15:40:22, sahel wrote: > On 2017/04/11 16:23:14, bokan wrote: > > Touchscreen and Touchpad gesture event streams can differ for Flings but I can > > never remember how. Sahel should confirm that this change is ok. > > This function is used only for wheel/touchpad scrolling. I don't think that's true. Taking a quick look, it was and still is called from RenderWidgetHostInputEventRouter::RouteTouchscreenGestureEvent.
On 2017/04/12 19:15:10, bokan wrote: > https://codereview.chromium.org/2785533003/diff/180001/content/browser/render... > File content/browser/renderer_host/render_widget_host_impl.h (right): > > https://codereview.chromium.org/2785533003/diff/180001/content/browser/render... > content/browser/renderer_host/render_widget_host_impl.h:357: bool > is_in_gesture_scroll(blink::WebGestureDevice device) override; > On 2017/04/12 15:40:22, sahel wrote: > > On 2017/04/11 16:23:14, bokan wrote: > > > Touchscreen and Touchpad gesture event streams can differ for Flings but I > can > > > never remember how. Sahel should confirm that this change is ok. > > > > This function is used only for wheel/touchpad scrolling. > > I don't think that's true. Taking a quick look, it was and still is called from > RenderWidgetHostInputEventRouter::RouteTouchscreenGestureEvent. Correct, but RWHI keeps separate state for each type of event.
https://codereview.chromium.org/2785533003/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/2785533003/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.h:357: bool is_in_gesture_scroll(blink::WebGestureDevice device) override; On 2017/04/12 19:15:10, bokan wrote: > On 2017/04/12 15:40:22, sahel wrote: > > On 2017/04/11 16:23:14, bokan wrote: > > > Touchscreen and Touchpad gesture event streams can differ for Flings but I > can > > > never remember how. Sahel should confirm that this change is ok. > > > > This function is used only for wheel/touchpad scrolling. > > I don't think that's true. Taking a quick look, it was and still is called from > RenderWidgetHostInputEventRouter::RouteTouchscreenGestureEvent. > I think is_in_touchscreen_gesture_scroll() is called in RenderWidgetHostInputEventRouter::RouteTouchscreenGestureEvent rather than is_in_gesture_scroll(). Maybe it should be changed to this new function, though.
The CQ bit was checked by wjmaclean@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by wjmaclean@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...
wjmaclean@chromium.org changed reviewers: - pdr@chromium.org
Description was changed from ========== Animated scroll shouldn't require main-frame for OOPIFs. At present the animated scroll code for mouse wheels will mark as SCROLL_ON_IMPL any attempt to scroll if there is no scrollable layer, and in an OOPIF this causes the GestureScrollUpdate to be marked as consumed when in fact it isn't. In turn, this prevents the GestureScrollUpdate from bubbling to the embedder. This CL fixes LayerTreeHostImpl::ScrollAnimated() to corrently report such GestureScrollUpdate events as ignored, so they can bubble to the embedder. It also modifies MouseWheelEventQueue to refer to it's client to know if a (non-synthetic) GestureScrollBegin/End are required, instead of hard-coding that state in the Queue itself. This is important since OOPIF scroll bubbling inserts its own Begin/Ends as needed, without the Queue's involvement. BUG=701178 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Animated scroll shouldn't require main-frame for OOPIFs. At present the animated scroll code for mouse wheels will mark as SCROLL_ON_IMPL any attempt to scroll if there is no scrollable layer, and in an OOPIF this causes the GestureScrollUpdate to be marked as consumed when in fact it isn't. In turn, this prevents the GestureScrollUpdate from bubbling to the embedder. This CL fixes LayerTreeHostImpl::ScrollAnimated() to corrently report such GestureScrollUpdate events as ignored, so they can bubble to the embedder. This CL has no behavioral changes for Mac, as it does not support smooth scrolling. BUG=701178 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
wjmaclean@chromium.org changed reviewers: - aelias@chromium.org
wjmaclean@chromium.org changed reviewers: - sahel@chromium.org
On 2017/04/18 15:43:43, sahel wrote: > https://codereview.chromium.org/2785533003/diff/180001/content/browser/render... > File content/browser/renderer_host/render_widget_host_impl.h (right): > > https://codereview.chromium.org/2785533003/diff/180001/content/browser/render... > content/browser/renderer_host/render_widget_host_impl.h:357: bool > is_in_gesture_scroll(blink::WebGestureDevice device) override; > On 2017/04/12 19:15:10, bokan wrote: > > On 2017/04/12 15:40:22, sahel wrote: > > > On 2017/04/11 16:23:14, bokan wrote: > > > > Touchscreen and Touchpad gesture event streams can differ for Flings but I > > can > > > > never remember how. Sahel should confirm that this change is ok. > > > > > > This function is used only for wheel/touchpad scrolling. > > > > I don't think that's true. Taking a quick look, it was and still is called > from > > RenderWidgetHostInputEventRouter::RouteTouchscreenGestureEvent. > > > > I think is_in_touchscreen_gesture_scroll() is called in > RenderWidgetHostInputEventRouter::RouteTouchscreenGestureEvent rather than > is_in_gesture_scroll(). Maybe it should be changed to this new function, though. This is all moot I think. I don't think the MWEQ changes were compatible with Mac after all, but the main part of the CL doesn't require them it seems. This CL defers the test, as it is held up on https://bugs.chromium.org/p/chromium/issues/detail?id=710513. bokan@ ... are you ok with this CL as it stands? I think the remaining parts you were concerned about are gone.
All except I'm still worried we're missing test coverage. Can we at least reenable the current test but switch it to the non-animated path? That should be non-flaky, right? At the least, we should file a bug to add support for testing animations and adding back this test.
On 2017/04/18 19:40:20, bokan wrote: > All except I'm still worried we're missing test coverage. Can we at least > reenable the current test but switch it to the non-animated path? That should be > non-flaky, right? At the least, we should file a bug to add support for testing > animations and adding back this test. I tried to re-land the existing test, but it seems to hit the DCHECK issue, so something needs to be done separately there. But it doesn't test the animation pathway ... that test will be blocked on fixing the bug I listed above. We can certainly file a new bug, blocked on that one, to get a test in place.
On 2017/04/18 19:46:29, wjmaclean wrote: > On 2017/04/18 19:40:20, bokan wrote: > > All except I'm still worried we're missing test coverage. Can we at least > > reenable the current test but switch it to the non-animated path? That should > be > > non-flaky, right? At the least, we should file a bug to add support for > testing > > animations and adding back this test. > > I tried to re-land the existing test, but it seems to hit the DCHECK issue, so > something needs to be done separately there. > > But it doesn't test the animation pathway ... that test will be blocked on > fixing the bug I listed above. We can certainly file a new bug, blocked on that > one, to get a test in place. Ok, chatted a bit offline. To summarize: The existing test, when converted to use non-animated scrolls trips DCHECK(!(*is_in_gesture_scroll)) in RenderWidgetHostImpl::ForwardGestureEventWithLatencyInfo (https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...) https://crbug.com/710513 is blocking having a reliable test for bubbling under animations. So we're blocked for a test to this CL's behavior and the DCHECK issue is unrelated to changes in this CL. Since this CL doesn't make any of the issues worse and not fixing it will affect scrolling in shipping features I'm ok landing this so lgtm. That said, I think we should prioritise cleaning up the gesture streams and getting the tests up and running as we're shipping untested code at this point.
wjmaclean@chromium.org changed reviewers: + aelias@chromium.org
aelias@ ... could you please look at cc/ ?
cc/ lgtm
cc/ lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
One real question about OOPIFs on Android, but otherwise looks good with just a few nits. https://codereview.chromium.org/2785533003/diff/260001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_client.h (right): https://codereview.chromium.org/2785533003/diff/260001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_client.h:62: virtual bool IsForSubframe() = 0; nit: No blank line before? (Doesn't seem substantially different enough from the ones above to need its own block.) Same throughout the CL. There's some ambiguity in the name-- is this only true for out-of-process iframes, or could it ever be true on same-process subframes? That seems worth clarifying in a comment. https://codereview.chromium.org/2785533003/diff/260001/cc/trees/layer_tree_se... File cc/trees/layer_tree_settings.h (right): https://codereview.chromium.org/2785533003/diff/260001/cc/trees/layer_tree_se... cc/trees/layer_tree_settings.h:114: bool is_layer_tree_for_subframe = false; Similarly, worth a comment if this actually means OOPIFs and not all subframes. https://codereview.chromium.org/2785533003/diff/260001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2785533003/diff/260001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:455: return false; I'm not familiar with this class, but would this cause a problem for OOPIFs on Android? Or do those not have one of these? https://codereview.chromium.org/2785533003/diff/260001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.h (right): https://codereview.chromium.org/2785533003/diff/260001/content/browser/render... content/browser/renderer_host/compositor_impl_android.h:105: bool IsForSubframe() override; nit: No blank line before, since this is part of the LayerTreeHostClient block. https://codereview.chromium.org/2785533003/diff/260001/content/renderer/gpu/r... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/2785533003/diff/260001/content/renderer/gpu/r... content/renderer/gpu/render_widget_compositor.cc:1127: return is_for_oopif_; Yeah, this is where my confusion comes from, since OOPIF != subframe. We don't have to call it IsForCrossProcessSubframe, but we should at least document what it means. https://codereview.chromium.org/2785533003/diff/260001/content/renderer/gpu/r... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/2785533003/diff/260001/content/renderer/gpu/r... content/renderer/gpu/render_widget_compositor.h:202: bool IsForSubframe() override; nit: No blank line before.
creis@ - Comments addressed, ptal? https://codereview.chromium.org/2785533003/diff/260001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_client.h (right): https://codereview.chromium.org/2785533003/diff/260001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_client.h:62: virtual bool IsForSubframe() = 0; On 2017/04/18 21:59:03, Charlie Reis wrote: > nit: No blank line before? (Doesn't seem substantially different enough from > the ones above to need its own block.) Same throughout the CL. > > There's some ambiguity in the name-- is this only true for out-of-process > iframes, or could it ever be true on same-process subframes? That seems worth > clarifying in a comment. Done. https://codereview.chromium.org/2785533003/diff/260001/cc/trees/layer_tree_se... File cc/trees/layer_tree_settings.h (right): https://codereview.chromium.org/2785533003/diff/260001/cc/trees/layer_tree_se... cc/trees/layer_tree_settings.h:114: bool is_layer_tree_for_subframe = false; On 2017/04/18 21:59:03, Charlie Reis wrote: > Similarly, worth a comment if this actually means OOPIFs and not all subframes. Done. https://codereview.chromium.org/2785533003/diff/260001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2785533003/diff/260001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:455: return false; On 2017/04/18 21:59:03, Charlie Reis wrote: > I'm not familiar with this class, but would this cause a problem for OOPIFs on > Android? Or do those not have one of these? I think, at worst, it leaves the status quo unchanged for Android. I don't think Android uses smooth scrolling for mouse wheels, or other 'bubbling' events ... I could be wrong though, in which case this might be the starting point to deal with that. https://codereview.chromium.org/2785533003/diff/260001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.h (right): https://codereview.chromium.org/2785533003/diff/260001/content/browser/render... content/browser/renderer_host/compositor_impl_android.h:105: bool IsForSubframe() override; On 2017/04/18 21:59:03, Charlie Reis wrote: > nit: No blank line before, since this is part of the LayerTreeHostClient block. Done. https://codereview.chromium.org/2785533003/diff/260001/content/renderer/gpu/r... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/2785533003/diff/260001/content/renderer/gpu/r... content/renderer/gpu/render_widget_compositor.cc:1127: return is_for_oopif_; On 2017/04/18 21:59:03, Charlie Reis wrote: > Yeah, this is where my confusion comes from, since OOPIF != subframe. We don't > have to call it IsForCrossProcessSubframe, but we should at least document what > it means. Acknowledged. https://codereview.chromium.org/2785533003/diff/260001/content/renderer/gpu/r... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/2785533003/diff/260001/content/renderer/gpu/r... content/renderer/gpu/render_widget_compositor.h:202: bool IsForSubframe() override; On 2017/04/18 21:59:03, Charlie Reis wrote: > nit: No blank line before. Done.
Thanks, LGTM. (And thanks for the discussion about tests-- hopefully we can find a way to resolve the DCHECK issue after this lands.)
The CQ bit was checked by wjmaclean@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vollick@chromium.org, bokan@chromium.org, aelias@chromium.org Link to the patchset: https://codereview.chromium.org/2785533003/#ps280001 (title: "Address nits, add comments.")
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": 280001, "attempt_start_ts": 1492642754425250,
"parent_rev": "8879a4bb7e30c99c71fe7a68936a168f7204462a", "commit_rev":
"a755e30003e89da8ed1ee6f9e4e039acb395e3af"}
Message was sent while issue was closed.
Description was changed from ========== Animated scroll shouldn't require main-frame for OOPIFs. At present the animated scroll code for mouse wheels will mark as SCROLL_ON_IMPL any attempt to scroll if there is no scrollable layer, and in an OOPIF this causes the GestureScrollUpdate to be marked as consumed when in fact it isn't. In turn, this prevents the GestureScrollUpdate from bubbling to the embedder. This CL fixes LayerTreeHostImpl::ScrollAnimated() to corrently report such GestureScrollUpdate events as ignored, so they can bubble to the embedder. This CL has no behavioral changes for Mac, as it does not support smooth scrolling. BUG=701178 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Animated scroll shouldn't require main-frame for OOPIFs. At present the animated scroll code for mouse wheels will mark as SCROLL_ON_IMPL any attempt to scroll if there is no scrollable layer, and in an OOPIF this causes the GestureScrollUpdate to be marked as consumed when in fact it isn't. In turn, this prevents the GestureScrollUpdate from bubbling to the embedder. This CL fixes LayerTreeHostImpl::ScrollAnimated() to corrently report such GestureScrollUpdate events as ignored, so they can bubble to the embedder. This CL has no behavioral changes for Mac, as it does not support smooth scrolling. BUG=701178 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2785533003 Cr-Commit-Position: refs/heads/master@{#465828} Committed: https://chromium.googlesource.com/chromium/src/+/a755e30003e89da8ed1ee6f9e4e0... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/a755e30003e89da8ed1ee6f9e4e0... |
