Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2907053004:
GSB uses delta_hints to calculate scrolling chain. (Closed)
|
Created:
3 years, 6 months ago by sahel Modified:
3 years, 5 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, cc-bugs_chromium.org, chromium-reviews, dglazkov+blink, dtapuska+blinkwatch_chromium.org, dtapuska+chromiumwatch_chromium.org, kenneth.christiansen, kinuko+watch, majidvp, Navid Zolghadr, sunyunjia Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionGSB uses delta_hints to calculate scrolling chain.
This cl uses delta_hints while calculating the scroll chain. If the
scroller cannot consume any delta hints, we skip it and don't add it to
the scrolling chain (viewports are exceptions) and the chain is only
calculated while handling GSB events and used for the rest of the GSU
events in current scrolling sequence. This is an optimization for
handling GSUs since if the scrolling chain is empty we do an early return
rather than recomputing the chain and then returning not handled.
This cl also includes changes to unittests to make sure that GSB events
have delta hint information; However, the event sender sets ignore delta
hint to true to avoid layouttests failures. This will be changed in a
separate cl.
TBR=rbyers@chromium.org
BUG=526463
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2907053004
Cr-Commit-Position: refs/heads/master@{#482416}
Committed: https://chromium.googlesource.com/chromium/src/+/60b41cdfe22237cb667050017349375ca9272ccf
Patch Set 1 #
Total comments: 2
Patch Set 2 : debug version #
Total comments: 10
Patch Set 3 : failing tests fixed. #Patch Set 4 : viewport is always added to the scorll chain, failing unittests fixed. #Patch Set 5 : Event sender sets ignore_delta_hints to true. #Patch Set 6 : delta hints must be identical to the first GSU deltas #Patch Set 7 : gpusmoothscrollBy forwards wheel events with phase info. #Patch Set 8 : fix scroll-customization failing test, CanScroll returns true when delta is zero. #Patch Set 9 : Merged with master. #
Total comments: 24
Patch Set 10 : rebased. #Patch Set 11 : review comments addressed. #
Total comments: 14
Patch Set 12 : review comments addressed. #
Total comments: 2
Patch Set 13 : Merge branch 'master' into GSB_checks_delta_hints #
Total comments: 9
Patch Set 14 : ignore_delta_hints_ deleted. #
Total comments: 2
Patch Set 15 : dcheck for scroll_node not null #Patch Set 16 : Merge branch 'master' into GSB_checks_delta_hints #Messages
Total messages: 102 (74 generated)
Description was changed from ========== GSB uses delta_hints to calculate scrolling chain. This WIP uses delta_hints while calculating the scroll chain, and the chain is only calculated while handling GSB events and used for the rest of the GSU events in current scrolling sequence. BUG=526463 ========== to ========== GSB uses delta_hints to calculate scrolling chain. This WIP uses delta_hints while calculating the scroll chain, and the chain is only calculated while handling GSB events and used for the rest of the GSU events in current scrolling sequence. BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== GSB uses delta_hints to calculate scrolling chain. This WIP uses delta_hints while calculating the scroll chain, and the chain is only calculated while handling GSB events and used for the rest of the GSU events in current scrolling sequence. BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== GSB uses delta_hints to calculate scrolling chain. This WIP uses delta_hints while calculating the scroll chain, and the chain is only calculated while handling GSB events and used for the rest of the GSU events in current scrolling sequence. BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
sahel@chromium.org changed reviewers: + sunyunjia@chromium.org
The CQ bit was checked by sahel@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...)
sahel@chromium.org changed reviewers: + bokan@chromium.org
https://codereview.chromium.org/2907053004/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2907053004/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:3296: ScrollNode* viewport_scroll_node = OuterViewportScrollNode(); While you're here, please change this from OuterViewportScrollNode to viewport()->MainScrollLayer().
dtapuska@chromium.org changed reviewers: + dtapuska@chromium.org
https://codereview.chromium.org/2907053004/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2907053004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2779: return scroll_status; Why is this an early return? It avoids the uma metric collection. Is that intended? https://codereview.chromium.org/2907053004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:3296: LOG(ERROR) << "in distribute scroll delta before for loop"; Why log errors? https://codereview.chromium.org/2907053004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollStateInit.idl (right): https://codereview.chromium.org/2907053004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollStateInit.idl:10: double deltaXHint = 0; This seems odd to add. best to talk to tdresser@ if you haven't.
https://codereview.chromium.org/2907053004/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2907053004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2753: if (!scrolling_node) { Early return happens here too when scrolling is ignored. https://codereview.chromium.org/2907053004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2779: return scroll_status; On 2017/05/30 15:20:38, dtapuska wrote: > Why is this an early return? It avoids the uma metric collection. Is that > intended? I want GSB handling to return ignored when there is no node that can consume delta hints, this way, I can do scroll bubbling only based on GSB acks. It is an early return because I wanted it to be similar to the above case when scrolling is ignored and early return skips collecting the metrics. Should I change it to collect them? https://codereview.chromium.org/2907053004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:3296: LOG(ERROR) << "in distribute scroll delta before for loop"; On 2017/05/30 15:20:38, dtapuska wrote: > Why log errors? This is a debug version I was trying to figure out why the unittests are failing and bokan helped me by looking at the code. I will clean it up once I fixed all the failing tests. https://codereview.chromium.org/2907053004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollStateInit.idl (right): https://codereview.chromium.org/2907053004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollStateInit.idl:10: double deltaXHint = 0; On 2017/05/30 15:20:38, dtapuska wrote: > This seems odd to add. best to talk to tdresser@ if you haven't. Don't we have all fields here? I will talk to tdresser@.
sahel@chromium.org changed reviewers: + tdresser@chromium.org
On 2017/05/30 15:36:15, sahel wrote: > https://codereview.chromium.org/2907053004/diff/20001/cc/trees/layer_tree_hos... > File cc/trees/layer_tree_host_impl.cc (right): > > https://codereview.chromium.org/2907053004/diff/20001/cc/trees/layer_tree_hos... > cc/trees/layer_tree_host_impl.cc:2753: if (!scrolling_node) { > Early return happens here too when scrolling is ignored. > > https://codereview.chromium.org/2907053004/diff/20001/cc/trees/layer_tree_hos... > cc/trees/layer_tree_host_impl.cc:2779: return scroll_status; > On 2017/05/30 15:20:38, dtapuska wrote: > > Why is this an early return? It avoids the uma metric collection. Is that > > intended? > > I want GSB handling to return ignored when there is no node that can consume > delta hints, this way, I can do scroll bubbling only based on GSB acks. > > It is an early return because I wanted it to be similar to the above case when > scrolling is ignored and early return skips collecting the metrics. Should I > change it to collect them? > > https://codereview.chromium.org/2907053004/diff/20001/cc/trees/layer_tree_hos... > cc/trees/layer_tree_host_impl.cc:3296: LOG(ERROR) << "in distribute scroll delta > before for loop"; > On 2017/05/30 15:20:38, dtapuska wrote: > > Why log errors? > > This is a debug version I was trying to figure out why the unittests are failing > and bokan helped me by looking at the code. > I will clean it up once I fixed all the failing tests. > > https://codereview.chromium.org/2907053004/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/page/scrolling/ScrollStateInit.idl (right): > > https://codereview.chromium.org/2907053004/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/page/scrolling/ScrollStateInit.idl:10: double > deltaXHint = 0; > On 2017/05/30 15:20:38, dtapuska wrote: > > This seems odd to add. best to talk to tdresser@ if you haven't. > > Don't we have all fields here? > I will talk to tdresser@. tdresser@, could you please take a look at ScrollStateInit.idl? Dave is concerned that I shouldn't add the new fields there.
https://codereview.chromium.org/2907053004/diff/20001/ui/events/blink/input_h... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2907053004/diff/20001/ui/events/blink/input_h... ui/events/blink/input_handler_proxy.cc:144: scroll_state_data.delta_x_hint = -event.data.scroll_begin.delta_x_hint; The delta on a scroll_begin is called a hint, but for scroll_state I think we could just put it in delta_x and delta_y without issue (and use ignore_delta_hints here to decide whether to add it or not). WDYT?
https://codereview.chromium.org/2907053004/diff/20001/ui/events/blink/input_h... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2907053004/diff/20001/ui/events/blink/input_h... ui/events/blink/input_handler_proxy.cc:144: scroll_state_data.delta_x_hint = -event.data.scroll_begin.delta_x_hint; On 2017/05/30 15:57:44, bokan wrote: > The delta on a scroll_begin is called a hint, but for scroll_state I think we > could just put it in delta_x and delta_y without issue (and use > ignore_delta_hints here to decide whether to add it or not). WDYT? I had this implementation initially, but ended up actually distributing these delta and changing scroll offsets on GSB. I can say that these are only hints but I thought this flag might get confused by ignore_delta_hints flag. I can change it if you prefer to avoid separate variables for delta hints.
https://codereview.chromium.org/2907053004/diff/20001/ui/events/blink/input_h... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2907053004/diff/20001/ui/events/blink/input_h... ui/events/blink/input_handler_proxy.cc:144: scroll_state_data.delta_x_hint = -event.data.scroll_begin.delta_x_hint; On 2017/05/30 16:02:06, sahel wrote: > On 2017/05/30 15:57:44, bokan wrote: > > The delta on a scroll_begin is called a hint, but for scroll_state I think we > > could just put it in delta_x and delta_y without issue (and use > > ignore_delta_hints here to decide whether to add it or not). WDYT? > > I had this implementation initially, but ended up actually distributing these > delta and changing scroll offsets on GSB. > > I can say that these are only hints but I thought this flag might get confused > by ignore_delta_hints flag. I can change it if you prefer to avoid separate > variables for delta hints. Tim is the expert here so I'll defer to him but personally I think the "hint" aspect is implied by the context of the scroll_state having is_beginning = true (i.e. we know that a GSB doesn't *scroll* the delta it provides) so having a separate delta_x/y is overly verbose.
On 2017/05/30 16:05:29, bokan wrote: > https://codereview.chromium.org/2907053004/diff/20001/ui/events/blink/input_h... > File ui/events/blink/input_handler_proxy.cc (right): > > https://codereview.chromium.org/2907053004/diff/20001/ui/events/blink/input_h... > ui/events/blink/input_handler_proxy.cc:144: scroll_state_data.delta_x_hint = > -event.data.scroll_begin.delta_x_hint; > On 2017/05/30 16:02:06, sahel wrote: > > On 2017/05/30 15:57:44, bokan wrote: > > > The delta on a scroll_begin is called a hint, but for scroll_state I think > we > > > could just put it in delta_x and delta_y without issue (and use > > > ignore_delta_hints here to decide whether to add it or not). WDYT? > > > > I had this implementation initially, but ended up actually distributing these > > delta and changing scroll offsets on GSB. > > > > I can say that these are only hints but I thought this flag might get confused > > by ignore_delta_hints flag. I can change it if you prefer to avoid separate > > variables for delta hints. > > Tim is the expert here so I'll defer to him but personally I think the "hint" > aspect is implied by the context of the scroll_state having is_beginning = true > (i.e. we know that a GSB doesn't *scroll* the delta it provides) so having a > separate delta_x/y is overly verbose. I think avoiding overloading delta_{x,y} to mean different things depending on whether or not we're dealing with a scroll start is probably correct. This seems like it's just extending what we do in WebGestureEvent to ScrollState - if we're going to use overload delta to mean different things in ScrollState, we should probably do it in WebGestureEvent too. My preference would be to do what Sahel has done here.
On 2017/05/30 17:17:40, tdresser wrote: > On 2017/05/30 16:05:29, bokan wrote: > > > https://codereview.chromium.org/2907053004/diff/20001/ui/events/blink/input_h... > > File ui/events/blink/input_handler_proxy.cc (right): > > > > > https://codereview.chromium.org/2907053004/diff/20001/ui/events/blink/input_h... > > ui/events/blink/input_handler_proxy.cc:144: scroll_state_data.delta_x_hint = > > -event.data.scroll_begin.delta_x_hint; > > On 2017/05/30 16:02:06, sahel wrote: > > > On 2017/05/30 15:57:44, bokan wrote: > > > > The delta on a scroll_begin is called a hint, but for scroll_state I think > > we > > > > could just put it in delta_x and delta_y without issue (and use > > > > ignore_delta_hints here to decide whether to add it or not). WDYT? > > > > > > I had this implementation initially, but ended up actually distributing > these > > > delta and changing scroll offsets on GSB. > > > > > > I can say that these are only hints but I thought this flag might get > confused > > > by ignore_delta_hints flag. I can change it if you prefer to avoid separate > > > variables for delta hints. > > > > Tim is the expert here so I'll defer to him but personally I think the "hint" > > aspect is implied by the context of the scroll_state having is_beginning = > true > > (i.e. we know that a GSB doesn't *scroll* the delta it provides) so having a > > separate delta_x/y is overly verbose. > > I think avoiding overloading delta_{x,y} to mean different things depending on > whether or not we're dealing with a scroll start is probably correct. > This seems like it's just extending what we do in WebGestureEvent to ScrollState > - if we're going to use overload delta to mean different things in ScrollState, > we should probably do it in WebGestureEvent too. > > My preference would be to do what Sahel has done here. Ok, sgtm!
The CQ bit was checked by sahel@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_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sahel@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 sahel@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...
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by sahel@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_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 sahel@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/2907053004/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2907053004/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:3296: ScrollNode* viewport_scroll_node = OuterViewportScrollNode(); On 2017/05/30 15:07:06, bokan wrote: > While you're here, please change this from OuterViewportScrollNode to > viewport()->MainScrollLayer(). Done.
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_...)
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by sahel@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 ========== GSB uses delta_hints to calculate scrolling chain. This WIP uses delta_hints while calculating the scroll chain, and the chain is only calculated while handling GSB events and used for the rest of the GSU events in current scrolling sequence. BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== GSB uses delta_hints to calculate scrolling chain. This cl uses delta_hints while calculating the scroll chain. If the scroller cannot consume any delta hints, we skip it and don't add it to the scrolling chain. (viewports are exceptions) and the chain is only calculated while handling GSB events and used for the rest of the GSU events in current scrolling sequence. This is an optimization for handling GSUs since if the scrolling chain is empty we do an early return rather than recomputing the chain and then returning not handled. This cl also includes changes to unittests to make sure that GSB events have delta hint information; However, the event sender sets ignore delta hint to true to avoid layouttests failures. This will be changed in a separate cl. BUG=526463 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: 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 sahel@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Patchset #9 (id:200001) has been deleted
sunyunjia@chromium.org changed reviewers: + majidvp@chromium.org
The CQ bit was checked by sahel@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.
After landing https://codereview.chromium.org/2924953002/ , this cl now passes all the trybots.
Nit in Commit Message:
"This cl uses delta_hints while calculating the scroll chain. If the
scroller cannot consume any delta hints, we skip it and don't add it to
the scrolling chain. (viewports are exceptions) and the chain is only"
^
|
No Period
https://codereview.chromium.org/2907053004/diff/220001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):
https://codereview.chromium.org/2907053004/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3372: if (scroll_node->scrolls_inner_viewport)
{
I don't think you need this special case, at least not here. I think
ComputeScrollDelta does though. It should check if the given node is either
Inner or Outer and compute the amount consumable between the both of them.
https://codereview.chromium.org/2907053004/diff/220001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):
https://codereview.chromium.org/2907053004/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:6250: ExpectNone(*scroll_info.get(),
child_scroll_id);
What's this change about? The comment above says the layer shouldn't have
scrolled but it used to check that it does? Was the test before incorrect? Does
your patch fix the behavior?
https://codereview.chromium.org/2907053004/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):
https://codereview.chromium.org/2907053004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:110: if
(CanScroll(scroll_state, cur_element))
Do we have to do this after we've added our first element? i.e. Why do we have
to change the scroll chain computation apart from which element we latch to
initially?
https://codereview.chromium.org/2907053004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:135:
DCHECK(current_element);
Rather than DCHECK, just pass the Element by const ref.
https://codereview.chromium.org/2907053004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:371: if
(current_scroll_chain_.empty()) {
Nit: no {}
https://codereview.chromium.org/2907053004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:444: if
(current_scroll_chain_.empty())
We can early out before creating and initializing the scroll_state_data.
https://codereview.chromium.org/2907053004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:488: if
(!current_scroll_chain_.empty())
We should skip the scroll_state_data initialization above as well.
https://codereview.chromium.org/2907053004/diff/220001/ui/events/blink/input_...
File ui/events/blink/input_handler_proxy.cc (right):
https://codereview.chromium.org/2907053004/diff/220001/ui/events/blink/input_...
ui/events/blink/input_handler_proxy.cc:147:
event.data.scroll_begin.ignore_delta_hints;
Do you know what this flag is used for? Does it ever get used in production or
is it just for tests?
Description was changed from ========== GSB uses delta_hints to calculate scrolling chain. This cl uses delta_hints while calculating the scroll chain. If the scroller cannot consume any delta hints, we skip it and don't add it to the scrolling chain. (viewports are exceptions) and the chain is only calculated while handling GSB events and used for the rest of the GSU events in current scrolling sequence. This is an optimization for handling GSUs since if the scrolling chain is empty we do an early return rather than recomputing the chain and then returning not handled. This cl also includes changes to unittests to make sure that GSB events have delta hint information; However, the event sender sets ignore delta hint to true to avoid layouttests failures. This will be changed in a separate cl. BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== GSB uses delta_hints to calculate scrolling chain. This cl uses delta_hints while calculating the scroll chain. If the scroller cannot consume any delta hints, we skip it and don't add it to the scrolling chain (viewports are exceptions) and the chain is only calculated while handling GSB events and used for the rest of the GSU events in current scrolling sequence. This is an optimization for handling GSUs since if the scrolling chain is empty we do an early return rather than recomputing the chain and then returning not handled. This cl also includes changes to unittests to make sure that GSB events have delta hint information; However, the event sender sets ignore delta hint to true to avoid layouttests failures. This will be changed in a separate cl. BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Patchset #10 (id:240001) has been deleted
Patchset #11 (id:280001) has been deleted
The CQ bit was checked by sahel@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_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2907053004/diff/220001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2907053004/diff/220001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3372: if (scroll_node->scrolls_inner_viewport) { On 2017/06/19 21:43:17, bokan wrote: > I don't think you need this special case, at least not here. I think > ComputeScrollDelta does though. It should check if the given node is either > Inner or Outer and compute the amount consumable between the both of them. I don't think ComputeScrollDelta checks for viewport nodes. (https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?sq=pack...) The viewport::ScrollAnimated though handles the special cases and calls this for inner and outer viewport nodes. (https://cs.chromium.org/chromium/src/cc/layers/viewport.cc?sq=package:chromiu...). https://codereview.chromium.org/2907053004/diff/220001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2907053004/diff/220001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl_unittest.cc:6250: ExpectNone(*scroll_info.get(), child_scroll_id); On 2017/06/19 21:43:17, bokan wrote: > What's this change about? The comment above says the layer shouldn't have > scrolled but it used to check that it does? Was the test before incorrect? Does > your patch fix the behavior? As we discussed the comment is incorrect, I confirmed it with tdresser@ as well. The child layer should scroll, so I left the test expectation as it is and corrected the comment. I had to change layer_tree_host_impl.cc to take transformations into account in canConsumeDelta function. https://codereview.chromium.org/2907053004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2907053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:110: if (CanScroll(scroll_state, cur_element)) On 2017/06/19 21:43:17, bokan wrote: > Do we have to do this after we've added our first element? i.e. Why do we have > to change the scroll chain computation apart from which element we latch to > initially? Once scroll latching is enabled, we can get rid of the chain and only have one target element. But, for now since it is still behind a flag, we still compute the chain. https://codereview.chromium.org/2907053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:135: DCHECK(current_element); On 2017/06/19 21:43:17, bokan wrote: > Rather than DCHECK, just pass the Element by const ref. Done. https://codereview.chromium.org/2907053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:371: if (current_scroll_chain_.empty()) { On 2017/06/19 21:43:17, bokan wrote: > Nit: no {} Done. https://codereview.chromium.org/2907053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:444: if (current_scroll_chain_.empty()) On 2017/06/19 21:43:17, bokan wrote: > We can early out before creating and initializing the scroll_state_data. Done. https://codereview.chromium.org/2907053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:488: if (!current_scroll_chain_.empty()) On 2017/06/19 21:43:17, bokan wrote: > We should skip the scroll_state_data initialization above as well. Done. https://codereview.chromium.org/2907053004/diff/220001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2907053004/diff/220001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:147: event.data.scroll_begin.ignore_delta_hints; On 2017/06/19 21:43:17, bokan wrote: > Do you know what this flag is used for? Does it ever get used in production or > is it just for tests? I will get rid of this once I fix the event_sender to get delta_hints in layouttests.
https://codereview.chromium.org/2907053004/diff/220001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2907053004/diff/220001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3372: if (scroll_node->scrolls_inner_viewport) { On 2017/06/22 16:34:17, sahel wrote: > On 2017/06/19 21:43:17, bokan wrote: > > I don't think you need this special case, at least not here. I think > > ComputeScrollDelta does though. It should check if the given node is either > > Inner or Outer and compute the amount consumable between the both of them. > > I don't think ComputeScrollDelta checks for viewport nodes. > (https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?sq=pack...) > > The viewport::ScrollAnimated though handles the special cases and calls this for > inner and outer viewport nodes. > (https://cs.chromium.org/chromium/src/cc/layers/viewport.cc?sq=package:chromiu...). Sorry, I meant that we should add it to ComputeScrollDelta. But on second thought, looking at viewport::ScrollAnimated, I think I was wrong. I still think this block is wrong though. Currently, if we hit the inner viewport, we'll check to see if the outer viewport can scroll the delta and add the inner viewport if it can. I think you should be checking if scroll_node == viewport()->MainScrollLayer() and if so, return the result of a (new) viewport()->CanScrollDelta() which checks whether you can scroll either inner or outer. Alternatively, if you hit the viewport()->MainScrollLayer(), shouldn't we just return true? I.e. the viewport always "consumes" the scroll from latching's perspective. Aside: if you hit the inner viewport layer, it means you haven't hit the outer (since we break as soon as we hit it above). This can only happen when document.rootScroller ships so we don't really have to worry about it today. When it does though, I think this is still right because in that case we'd only want to scroll the inner viewport so adding it to the chain is fine there. https://codereview.chromium.org/2907053004/diff/220001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2907053004/diff/220001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl_unittest.cc:6250: ExpectNone(*scroll_info.get(), child_scroll_id); On 2017/06/22 16:34:17, sahel wrote: > On 2017/06/19 21:43:17, bokan wrote: > > What's this change about? The comment above says the layer shouldn't have > > scrolled but it used to check that it does? Was the test before incorrect? > Does > > your patch fix the behavior? > > As we discussed the comment is incorrect, I confirmed it with tdresser@ as well. > The child layer should scroll, so I left the test expectation as it is and > corrected the comment. I had to change layer_tree_host_impl.cc to take > transformations into account in canConsumeDelta function. Acknowledged. https://codereview.chromium.org/2907053004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2907053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:110: if (CanScroll(scroll_state, cur_element)) On 2017/06/22 16:34:20, sahel wrote: > On 2017/06/19 21:43:17, bokan wrote: > > Do we have to do this after we've added our first element? i.e. Why do we have > > to change the scroll chain computation apart from which element we latch to > > initially? > > Once scroll latching is enabled, we can get rid of the chain and only have one > target element. But, for now since it is still behind a flag, we still compute > the chain. Right, but I don't mean making the chain just one element. I mean, shouldn't this be `if (!scroll_chain.IsEmpty() || CanScroll())`? That is, once you've added an element to the chain, we could just add the rest of the Elements as usual I think. That said, I *think* it shouldn't make a difference. Also though, why is it ok that this isn't behind a RuntimeEnabledFeature check? Won't this change behavior for latching being off? https://codereview.chromium.org/2907053004/diff/220001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2907053004/diff/220001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:147: event.data.scroll_begin.ignore_delta_hints; On 2017/06/22 16:34:20, sahel wrote: > On 2017/06/19 21:43:17, bokan wrote: > > Do you know what this flag is used for? Does it ever get used in production or > > is it just for tests? > > I will get rid of this once I fix the event_sender to get delta_hints in > layouttests. Acknowledged. https://codereview.chromium.org/2907053004/diff/300001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2907053004/diff/300001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3135: ScrollTree* scroll_tree, While we're moving things around, please pass this and scroll_node by const-ref https://codereview.chromium.org/2907053004/diff/300001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3137: gfx::Vector2dF* local_scroll_delta) { call these out_local_start_point and out_local_scroll_delta https://codereview.chromium.org/2907053004/diff/300001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3171: DCHECK(!end_clipped); Do you know when this happens? DCHECK followed by handling the case is forbidden by chromium style. I'd say lets remove the DCHECKs https://codereview.chromium.org/2907053004/diff/300001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3209: DCHECK(!end_clipped); Ditto here, remove DCHECK. https://codereview.chromium.org/2907053004/diff/300001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3396: gfx::PointF local_start_point; If you sometimes don't need the output of an out param, make it so that the method can accept a nullptr and just avoids setting that param in that case. https://codereview.chromium.org/2907053004/diff/300001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3402: &local_scroll_delta)) { just pass in &delta_to_scroll https://codereview.chromium.org/2907053004/diff/300001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2907053004/diff/300001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.h:636: bool CalculateLocalScrollDeltaAndStartPoint( Please add a short description of what this method does and what it returns.
The CQ bit was checked by sahel@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_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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...) 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 sahel@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.
https://codereview.chromium.org/2907053004/diff/220001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2907053004/diff/220001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3372: if (scroll_node->scrolls_inner_viewport) { On 2017/06/22 21:44:37, bokan wrote: > On 2017/06/22 16:34:17, sahel wrote: > > On 2017/06/19 21:43:17, bokan wrote: > > > I don't think you need this special case, at least not here. I think > > > ComputeScrollDelta does though. It should check if the given node is either > > > Inner or Outer and compute the amount consumable between the both of them. > > > > I don't think ComputeScrollDelta checks for viewport nodes. > > > (https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?sq=pack...) > > > > The viewport::ScrollAnimated though handles the special cases and calls this > for > > inner and outer viewport nodes. > > > (https://cs.chromium.org/chromium/src/cc/layers/viewport.cc?sq=package:chromiu...). > > Sorry, I meant that we should add it to ComputeScrollDelta. But on second > thought, looking at viewport::ScrollAnimated, I think I was wrong. > > I still think this block is wrong though. Currently, if we hit the inner > viewport, we'll check to see if the outer viewport can scroll the delta and add > the inner viewport if it can. > > I think you should be checking if scroll_node == viewport()->MainScrollLayer() > and if so, return the result of a (new) viewport()->CanScrollDelta() which > checks whether you can scroll either inner or outer. Alternatively, if you hit > the viewport()->MainScrollLayer(), shouldn't we just return true? I.e. the > viewport always "consumes" the scroll from latching's perspective. > > Aside: if you hit the inner viewport layer, it means you haven't hit the outer > (since we break as soon as we hit it above). This can only happen when > document.rootScroller ships so we don't really have to worry about it today. > When it does though, I think this is still right because in that case we'd only > want to scroll the inner viewport so adding it to the chain is fine there. I deleted this special case, as we discussed always adding the outerviewport if it exists is enough and if not the inner viewport will be treated like other scroll nodes. https://codereview.chromium.org/2907053004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2907053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:110: if (CanScroll(scroll_state, cur_element)) On 2017/06/22 21:44:37, bokan wrote: > On 2017/06/22 16:34:20, sahel wrote: > > On 2017/06/19 21:43:17, bokan wrote: > > > Do we have to do this after we've added our first element? i.e. Why do we > have > > > to change the scroll chain computation apart from which element we latch to > > > initially? > > > > Once scroll latching is enabled, we can get rid of the chain and only have one > > target element. But, for now since it is still behind a flag, we still compute > > the chain. > > Right, but I don't mean making the chain just one element. I mean, shouldn't > this be `if (!scroll_chain.IsEmpty() || CanScroll())`? That is, once you've > added an element to the chain, we could just add the rest of the Elements as > usual I think. That said, I *think* it shouldn't make a difference. > > Also though, why is it ok that this isn't behind a RuntimeEnabledFeature check? > Won't this change behavior for latching being off? When scroll latching is off, there will be one GSB for each GSU, so for handling each GSU we will only add elements to the chain that can at least partially consume the delta hints on GSB which is identical to the GSU's actual deltas. In terms of behavior it will be exactly the same while in terms of performance, we are skipping the nodes that cannot consume any delta which means going up the chain while handling GSU will be more efficient. https://codereview.chromium.org/2907053004/diff/300001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2907053004/diff/300001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3135: ScrollTree* scroll_tree, On 2017/06/22 21:44:38, bokan wrote: > While we're moving things around, please pass this and scroll_node by const-ref Done. https://codereview.chromium.org/2907053004/diff/300001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3137: gfx::Vector2dF* local_scroll_delta) { On 2017/06/22 21:44:38, bokan wrote: > call these out_local_start_point and out_local_scroll_delta Done. https://codereview.chromium.org/2907053004/diff/300001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3171: DCHECK(!end_clipped); On 2017/06/22 21:44:38, bokan wrote: > Do you know when this happens? DCHECK followed by handling the case is forbidden > by chromium style. I'd say lets remove the DCHECKs Done. https://codereview.chromium.org/2907053004/diff/300001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3209: DCHECK(!end_clipped); On 2017/06/22 21:44:38, bokan wrote: > Ditto here, remove DCHECK. Done. https://codereview.chromium.org/2907053004/diff/300001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3396: gfx::PointF local_start_point; On 2017/06/22 21:44:38, bokan wrote: > If you sometimes don't need the output of an out param, make it so that the > method can accept a nullptr and just avoids setting that param in that case. Done. https://codereview.chromium.org/2907053004/diff/300001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3402: &local_scroll_delta)) { On 2017/06/22 21:44:38, bokan wrote: > just pass in &delta_to_scroll Done. https://codereview.chromium.org/2907053004/diff/300001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2907053004/diff/300001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.h:636: bool CalculateLocalScrollDeltaAndStartPoint( On 2017/06/22 21:44:38, bokan wrote: > Please add a short description of what this method does and what it returns. Done.
lgtm % small comment https://codereview.chromium.org/2907053004/diff/220001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2907053004/diff/220001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3372: if (scroll_node->scrolls_inner_viewport) { On 2017/06/23 18:22:03, sahel wrote: > On 2017/06/22 21:44:37, bokan wrote: > > On 2017/06/22 16:34:17, sahel wrote: > > > On 2017/06/19 21:43:17, bokan wrote: > > > > I don't think you need this special case, at least not here. I think > > > > ComputeScrollDelta does though. It should check if the given node is > either > > > > Inner or Outer and compute the amount consumable between the both of them. > > > > > > I don't think ComputeScrollDelta checks for viewport nodes. > > > > > > (https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?sq=pack...) > > > > > > The viewport::ScrollAnimated though handles the special cases and calls this > > for > > > inner and outer viewport nodes. > > > > > > (https://cs.chromium.org/chromium/src/cc/layers/viewport.cc?sq=package:chromiu...). > > > > Sorry, I meant that we should add it to ComputeScrollDelta. But on second > > thought, looking at viewport::ScrollAnimated, I think I was wrong. > > > > I still think this block is wrong though. Currently, if we hit the inner > > viewport, we'll check to see if the outer viewport can scroll the delta and > add > > the inner viewport if it can. > > > > I think you should be checking if scroll_node == viewport()->MainScrollLayer() > > and if so, return the result of a (new) viewport()->CanScrollDelta() which > > checks whether you can scroll either inner or outer. Alternatively, if you hit > > the viewport()->MainScrollLayer(), shouldn't we just return true? I.e. the > > viewport always "consumes" the scroll from latching's perspective. > > > > Aside: if you hit the inner viewport layer, it means you haven't hit the outer > > (since we break as soon as we hit it above). This can only happen when > > document.rootScroller ships so we don't really have to worry about it today. > > When it does though, I think this is still right because in that case we'd > only > > want to scroll the inner viewport so adding it to the chain is fine there. > > I deleted this special case, as we discussed always adding the outerviewport if > it exists is enough and if not the inner viewport will be treated like other > scroll nodes. Acknowledged. https://codereview.chromium.org/2907053004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2907053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:110: if (CanScroll(scroll_state, cur_element)) On 2017/06/23 18:22:03, sahel wrote: > On 2017/06/22 21:44:37, bokan wrote: > > On 2017/06/22 16:34:20, sahel wrote: > > > On 2017/06/19 21:43:17, bokan wrote: > > > > Do we have to do this after we've added our first element? i.e. Why do we > > have > > > > to change the scroll chain computation apart from which element we latch > to > > > > initially? > > > > > > Once scroll latching is enabled, we can get rid of the chain and only have > one > > > target element. But, for now since it is still behind a flag, we still > compute > > > the chain. > > > > Right, but I don't mean making the chain just one element. I mean, shouldn't > > this be `if (!scroll_chain.IsEmpty() || CanScroll())`? That is, once you've > > added an element to the chain, we could just add the rest of the Elements as > > usual I think. That said, I *think* it shouldn't make a difference. > > > > Also though, why is it ok that this isn't behind a RuntimeEnabledFeature > check? > > Won't this change behavior for latching being off? > > When scroll latching is off, there will be one GSB for each GSU, so for handling > each GSU we will only add elements to the chain that can at least partially > consume the delta hints on GSB which is identical to the GSU's actual deltas. In > terms of behavior it will be exactly the same while in terms of performance, we > are skipping the nodes that cannot consume any delta which means going up the > chain while handling GSU will be more efficient. Right, thanks for the explanation. https://codereview.chromium.org/2907053004/diff/310001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2907053004/diff/310001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.h:637: // local delta, respectively. Please also document what the return value means.
sahel@chromium.org changed reviewers: - dtapuska@chromium.org, majidvp@chromium.org, sunyunjia@chromium.org
sahel@chromium.org changed reviewers: + weiliangc@chromium.org
weiliangc@chromium.org: Please review changes in cc/*
https://codereview.chromium.org/2907053004/diff/330001/cc/input/scroll_state_... File cc/input/scroll_state_data.h (right): https://codereview.chromium.org/2907053004/diff/330001/cc/input/scroll_state_... cc/input/scroll_state_data.h:25: // Scroll delta hint in viewport coordinates (DIP). Maybe mention what we mean by "hint" in this context. https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:131: : scroll_state.deltaY(); I'm tempted to say we should take the sum of the delta and the hint. It's faster, less code, and only slightly more confusing. Either way is fine by me - if you do use the sum, maybe add a comment that only one will ever be populated. (That is true, right?) https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollState.h (right): https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollState.h:50: // Indicates whether delta hints should be ignored while handling GSB or not. When this work is done, is this needed, or not? Could we just use 0 values for the deltas if this isn't needed? https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGestureEvent.h:90: bool ignore_delta_hints; Why do we need this? Should it have a comment?
On 2017/06/23 19:14:12, tdresser wrote: > https://codereview.chromium.org/2907053004/diff/330001/cc/input/scroll_state_... > File cc/input/scroll_state_data.h (right): > > https://codereview.chromium.org/2907053004/diff/330001/cc/input/scroll_state_... > cc/input/scroll_state_data.h:25: // Scroll delta hint in viewport coordinates > (DIP). > Maybe mention what we mean by "hint" in this context. > > https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): > > https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/input/ScrollManager.cpp:131: : > scroll_state.deltaY(); > I'm tempted to say we should take the sum of the delta and the hint. > > It's faster, less code, and only slightly more confusing. > > Either way is fine by me - if you do use the sum, maybe add a comment that only > one will ever be populated. (That is true, right?) > > https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/page/scrolling/ScrollState.h (right): > > https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/page/scrolling/ScrollState.h:50: // Indicates > whether delta hints should be ignored while handling GSB or not. > When this work is done, is this needed, or not? Could we just use 0 values for > the deltas if this isn't needed? > > https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/pub... > File third_party/WebKit/public/platform/WebGestureEvent.h (right): > > https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/pub... > third_party/WebKit/public/platform/WebGestureEvent.h:90: bool > ignore_delta_hints; > Why do we need this? Should it have a comment? Er, LGTM % above nits.
https://codereview.chromium.org/2907053004/diff/310001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2907053004/diff/310001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.h:637: // local delta, respectively. On 2017/06/23 18:56:44, bokan wrote: > Please also document what the return value means. Done. https://codereview.chromium.org/2907053004/diff/330001/cc/input/scroll_state_... File cc/input/scroll_state_data.h (right): https://codereview.chromium.org/2907053004/diff/330001/cc/input/scroll_state_... cc/input/scroll_state_data.h:25: // Scroll delta hint in viewport coordinates (DIP). On 2017/06/23 19:14:11, tdresser wrote: > Maybe mention what we mean by "hint" in this context. Done. https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:131: : scroll_state.deltaY(); On 2017/06/23 19:14:11, tdresser wrote: > I'm tempted to say we should take the sum of the delta and the hint. > > It's faster, less code, and only slightly more confusing. > > Either way is fine by me - if you do use the sum, maybe add a comment that only > one will ever be populated. (That is true, right?) I slightly prefer to leave it as it is: Even though what you say is correct because only one of them is populated and the other one is zero, it's not logically correct to sum the two values. But if you prefer less lines of code, I can change it. https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollState.h (right): https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollState.h:50: // Indicates whether delta hints should be ignored while handling GSB or not. On 2017/06/23 19:14:12, tdresser wrote: > When this work is done, is this needed, or not? Could we just use 0 values for > the deltas if this isn't needed? You are right, I initially added this for doing two rounds of GSB hittesting once with scroll deltas and if the result cannot consume then with ignoring delta hints. After talking to bokan@, dtapuska@ we decided that it's not needed. For the sake of event_sender treating zero delta hints as true is enough. I deleted the parameter. https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGestureEvent.h:90: bool ignore_delta_hints; On 2017/06/23 19:14:12, tdresser wrote: > Why do we need this? Should it have a comment? As you mentioned in previous comments, relying on zero delta hints is enough. I deleted the redundant parameter.
https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2907053004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:131: : scroll_state.deltaY(); On 2017/06/23 20:01:00, sahel wrote: > On 2017/06/23 19:14:11, tdresser wrote: > > I'm tempted to say we should take the sum of the delta and the hint. > > > > It's faster, less code, and only slightly more confusing. > > > > Either way is fine by me - if you do use the sum, maybe add a comment that > only > > one will ever be populated. (That is true, right?) > > I slightly prefer to leave it as it is: Even though what you say is correct > because only one of them is populated and the other one is zero, it's not > logically correct to sum the two values. > > But if you prefer less lines of code, I can change it. Nope, leaving this as is SGTM.
The CQ bit was checked by sahel@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...
sahel@chromium.org changed reviewers: + dpranke@chromium.org
dpranke@chromium.org: Please review changes in third_party/WebKit/public/platform/WebGestureEvent.h
On 2017/06/23 20:45:55, sahel wrote: > mailto:dpranke@chromium.org: Please review changes in > > third_party/WebKit/public/platform/WebGestureEvent.h I'm not the right person to review this. I suggest someone like rbyers@ instead. It's a trivial change, so it's probably okay to TBR them.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== GSB uses delta_hints to calculate scrolling chain. This cl uses delta_hints while calculating the scroll chain. If the scroller cannot consume any delta hints, we skip it and don't add it to the scrolling chain (viewports are exceptions) and the chain is only calculated while handling GSB events and used for the rest of the GSU events in current scrolling sequence. This is an optimization for handling GSUs since if the scrolling chain is empty we do an early return rather than recomputing the chain and then returning not handled. This cl also includes changes to unittests to make sure that GSB events have delta hint information; However, the event sender sets ignore delta hint to true to avoid layouttests failures. This will be changed in a separate cl. BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== GSB uses delta_hints to calculate scrolling chain. This cl uses delta_hints while calculating the scroll chain. If the scroller cannot consume any delta hints, we skip it and don't add it to the scrolling chain (viewports are exceptions) and the chain is only calculated while handling GSB events and used for the rest of the GSU events in current scrolling sequence. This is an optimization for handling GSUs since if the scrolling chain is empty we do an early return rather than recomputing the chain and then returning not handled. This cl also includes changes to unittests to make sure that GSB events have delta hint information; However, the event sender sets ignore delta hint to true to avoid layouttests failures. This will be changed in a separate cl. TBR=rbyers@chromium.org BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
On 2017/06/23 20:54:36, Dirk Pranke wrote: > On 2017/06/23 20:45:55, sahel wrote: > > mailto:dpranke@chromium.org: Please review changes in > > > > third_party/WebKit/public/platform/WebGestureEvent.h > > I'm not the right person to review this. I suggest someone like rbyers@ instead. > It's a trivial change, so it's probably okay to TBR them. Done, thanks!
cc/ LGTM https://codereview.chromium.org/2907053004/diff/350001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2907053004/diff/350001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3388: gfx::Vector2dF delta_to_scroll; nit: DCHECK scroll_node not nullptr.
The CQ bit was checked by sahel@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: 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 sahel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, tdresser@chromium.org, weiliangc@chromium.org Link to the patchset: https://codereview.chromium.org/2907053004/#ps390001 (title: "Merge branch 'master' into GSB_checks_delta_hints")
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": 390001, "attempt_start_ts": 1498504785823110,
"parent_rev": "2daeff3cbf72a9d38a08df1a3093a88ef2151e67", "commit_rev":
"60b41cdfe22237cb667050017349375ca9272ccf"}
Message was sent while issue was closed.
Description was changed from ========== GSB uses delta_hints to calculate scrolling chain. This cl uses delta_hints while calculating the scroll chain. If the scroller cannot consume any delta hints, we skip it and don't add it to the scrolling chain (viewports are exceptions) and the chain is only calculated while handling GSB events and used for the rest of the GSU events in current scrolling sequence. This is an optimization for handling GSUs since if the scrolling chain is empty we do an early return rather than recomputing the chain and then returning not handled. This cl also includes changes to unittests to make sure that GSB events have delta hint information; However, the event sender sets ignore delta hint to true to avoid layouttests failures. This will be changed in a separate cl. TBR=rbyers@chromium.org BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== GSB uses delta_hints to calculate scrolling chain. This cl uses delta_hints while calculating the scroll chain. If the scroller cannot consume any delta hints, we skip it and don't add it to the scrolling chain (viewports are exceptions) and the chain is only calculated while handling GSB events and used for the rest of the GSU events in current scrolling sequence. This is an optimization for handling GSUs since if the scrolling chain is empty we do an early return rather than recomputing the chain and then returning not handled. This cl also includes changes to unittests to make sure that GSB events have delta hint information; However, the event sender sets ignore delta hint to true to avoid layouttests failures. This will be changed in a separate cl. TBR=rbyers@chromium.org BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2907053004 Cr-Commit-Position: refs/heads/master@{#482416} Committed: https://chromium.googlesource.com/chromium/src/+/60b41cdfe22237cb667050017349... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:390001) as https://chromium.googlesource.com/chromium/src/+/60b41cdfe22237cb667050017349...
Message was sent while issue was closed.
https://codereview.chromium.org/2907053004/diff/350001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2907053004/diff/350001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3388: gfx::Vector2dF delta_to_scroll; On 2017/06/26 18:55:09, weiliangc wrote: > nit: DCHECK scroll_node not nullptr. Done. |
