Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2802993003:
Event unittests cover both wheel scroll latching and propagating cases. (Closed)
DescriptionEvent unittests cover both wheel scroll latching and propagating cases.
BUG=526463
Review-Url: https://codereview.chromium.org/2802993003
Cr-Commit-Position: refs/heads/master@{#462972}
Committed: https://chromium.googlesource.com/chromium/src/+/8a5b4060c1b81028deefeaddadee2115c84281f2
Patch Set 1 #
Total comments: 6
Patch Set 2 : InputHandlerProxyEventQueueTest parameterized. #Messages
Total messages: 21 (15 generated)
Patchset #2 (id:20001) 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: This issue passed the CQ dry run.
Description was changed from ========== Event unittests cover both wheel scroll latching and propagating cases. BUG=526463 ========== to ========== Event unittests cover both wheel scroll latching and propagating cases. BUG=526463 ==========
sahel@chromium.org changed reviewers: + dtapuska@chromium.org
https://codereview.chromium.org/2802993003/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy_unittest.cc (right): https://codereview.chromium.org/2802993003/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy_unittest.cc:525: &mock_input_handler_, &mock_client_, true); This test class is independent of wheel scroll latching vs propagation. The tests pass without any changes in both cases. Should I get the value from commandline flags or always set it to true or parameterize the test anyway? https://codereview.chromium.org/2802993003/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy_unittest.cc:1185: EXPECT_TRUE(input_handler_->gesture_scroll_on_impl_thread_for_testing()); in latching implementation for handling fling on impl thread we rely on gesture_scroll_on_impl_thread_ which gets set while handling GSB (similar to touch) It works in chrome because we don't sent GSE if a fling is going to happen; However, some of the unittests create a fling directly before sending any GSB. I don't know what the correct approach is, for now I called HandleGestureScrollBegin to set the flag to true.
https://codereview.chromium.org/2802993003/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy_unittest.cc (right): https://codereview.chromium.org/2802993003/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy_unittest.cc:525: &mock_input_handler_, &mock_client_, true); On 2017/04/06 19:22:29, sahel wrote: > This test class is independent of wheel scroll latching vs propagation. > The tests pass without any changes in both cases. > Should I get the value from commandline flags or always set it to true or > parameterize the test anyway? Probably would be best to parameterize it anyway. Looks like it can just be a test with param so it should be easy. That way we ensure that it doesn't *become* dependent on scroll latching. https://codereview.chromium.org/2802993003/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy_unittest.cc:1185: EXPECT_TRUE(input_handler_->gesture_scroll_on_impl_thread_for_testing()); On 2017/04/06 19:22:29, sahel wrote: > in latching implementation for handling fling on impl thread we rely on > gesture_scroll_on_impl_thread_ which gets set while handling GSB (similar to > touch) > > It works in chrome because we don't sent GSE if a fling is going to happen; > However, some of the unittests create a fling directly before sending any GSB. I > don't know what the correct approach is, for now I called > HandleGestureScrollBegin to set the flag to true. Seems reasonable to me.
Patchset #2 (id:40001) 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2802993003/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy_unittest.cc (right): https://codereview.chromium.org/2802993003/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy_unittest.cc:525: &mock_input_handler_, &mock_client_, true); On 2017/04/06 20:11:25, dtapuska wrote: > On 2017/04/06 19:22:29, sahel wrote: > > This test class is independent of wheel scroll latching vs propagation. > > The tests pass without any changes in both cases. > > Should I get the value from commandline flags or always set it to true or > > parameterize the test anyway? > > Probably would be best to parameterize it anyway. > > Looks like it can just be a test with param so it should be easy. That way we > ensure that it doesn't *become* dependent on scroll latching. Done. https://codereview.chromium.org/2802993003/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy_unittest.cc:1185: EXPECT_TRUE(input_handler_->gesture_scroll_on_impl_thread_for_testing()); On 2017/04/06 20:11:25, dtapuska wrote: > On 2017/04/06 19:22:29, sahel wrote: > > in latching implementation for handling fling on impl thread we rely on > > gesture_scroll_on_impl_thread_ which gets set while handling GSB (similar to > > touch) > > > > It works in chrome because we don't sent GSE if a fling is going to happen; > > However, some of the unittests create a fling directly before sending any GSB. > I > > don't know what the correct approach is, for now I called > > HandleGestureScrollBegin to set the flag to true. > > Seems reasonable to me. Acknowledged.
On 2017/04/07 18:10:24, sahel wrote: > https://codereview.chromium.org/2802993003/diff/1/ui/events/blink/input_handl... > File ui/events/blink/input_handler_proxy_unittest.cc (right): > > https://codereview.chromium.org/2802993003/diff/1/ui/events/blink/input_handl... > ui/events/blink/input_handler_proxy_unittest.cc:525: &mock_input_handler_, > &mock_client_, true); > On 2017/04/06 20:11:25, dtapuska wrote: > > On 2017/04/06 19:22:29, sahel wrote: > > > This test class is independent of wheel scroll latching vs propagation. > > > The tests pass without any changes in both cases. > > > Should I get the value from commandline flags or always set it to true or > > > parameterize the test anyway? > > > > Probably would be best to parameterize it anyway. > > > > Looks like it can just be a test with param so it should be easy. That way we > > ensure that it doesn't *become* dependent on scroll latching. > > Done. > > https://codereview.chromium.org/2802993003/diff/1/ui/events/blink/input_handl... > ui/events/blink/input_handler_proxy_unittest.cc:1185: > EXPECT_TRUE(input_handler_->gesture_scroll_on_impl_thread_for_testing()); > On 2017/04/06 20:11:25, dtapuska wrote: > > On 2017/04/06 19:22:29, sahel wrote: > > > in latching implementation for handling fling on impl thread we rely on > > > gesture_scroll_on_impl_thread_ which gets set while handling GSB (similar to > > > touch) > > > > > > It works in chrome because we don't sent GSE if a fling is going to happen; > > > However, some of the unittests create a fling directly before sending any > GSB. > > I > > > don't know what the correct approach is, for now I called > > > HandleGestureScrollBegin to set the flag to true. > > > > Seems reasonable to me. > > Acknowledged. lgtm
The CQ bit was checked by sahel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491593725666650,
"parent_rev": "365c1380f2e6f346ec267b48f9e238fce309ac38", "commit_rev":
"8a5b4060c1b81028deefeaddadee2115c84281f2"}
Message was sent while issue was closed.
Description was changed from ========== Event unittests cover both wheel scroll latching and propagating cases. BUG=526463 ========== to ========== Event unittests cover both wheel scroll latching and propagating cases. BUG=526463 Review-Url: https://codereview.chromium.org/2802993003 Cr-Commit-Position: refs/heads/master@{#462972} Committed: https://chromium.googlesource.com/chromium/src/+/8a5b4060c1b81028deefeaddadee... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/8a5b4060c1b81028deefeaddadee... |
