Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2746513002:
[Compositor event queue] Queue GestureFlingStart/Cancel together with scroll/pinch (Closed)
Description[Compositor event queue] Queue GestureFlingStart/Cancel together with scroll/pinch
Currently only |GestureScroll*| and |GesturePinch*| will be put into the queue. We
should cover |GestureFlingStart/Cancel| as well since it's order-sensitive.
BUG=693289
Review-Url: https://codereview.chromium.org/2746513002
Cr-Commit-Position: refs/heads/master@{#456772}
Committed: https://chromium.googlesource.com/chromium/src/+/ad977926ecd213a1063fb7d429d6fd1d5ee7dce7
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address tdresser@ and dtapuska@'s comments #
Total comments: 3
Patch Set 3 : dtapuska@'s review 2: Use IsGestureScrollOrFlingOrPinch #
Messages
Total messages: 35 (24 generated)
The CQ bit was checked by chongz@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 ========== Include GestureFlingStart/Cancel in compositor event queue BUG= ========== to ========== [Compositor event queue] Queue GestureFlingStart/Cancel Currently only |GestureScroll*| and |GesturePinch*| will be put into the queue. We should queue |GestureFlingStart/Cancel| as well since it's order-sensitive. BUG=693289 ==========
Description was changed from ========== [Compositor event queue] Queue GestureFlingStart/Cancel Currently only |GestureScroll*| and |GesturePinch*| will be put into the queue. We should queue |GestureFlingStart/Cancel| as well since it's order-sensitive. BUG=693289 ========== to ========== [Compositor event queue] Queue GestureFlingStart/Cancel together with scroll/pinch Currently only |GestureScroll*| and |GesturePinch*| will be put into the queue. We should queue |GestureFlingStart/Cancel| as well since it's order-sensitive. BUG=693289 ==========
Description was changed from ========== [Compositor event queue] Queue GestureFlingStart/Cancel together with scroll/pinch Currently only |GestureScroll*| and |GesturePinch*| will be put into the queue. We should queue |GestureFlingStart/Cancel| as well since it's order-sensitive. BUG=693289 ========== to ========== [Compositor event queue] Queue GestureFlingStart/Cancel together with scroll/pinch Currently only |GestureScroll*| and |GesturePinch*| will be put into the queue. We should cover |GestureFlingStart/Cancel| as well since it's order-sensitive. BUG=693289 ==========
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
chongz@chromium.org changed reviewers: + tdresser@chromium.org
tdresser@ PTAL, thanks!
LGTM with nit. https://codereview.chromium.org/2746513002/diff/20001/ui/events/blink/blink_e... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/2746513002/diff/20001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:965: bool IsGestureScrollFlingPinch(WebInputEvent::Type type) { I'd prefer we had an "Or" in there. IsGestureScrollFlingOrPinch? https://codereview.chromium.org/2746513002/diff/20001/ui/events/blink/blink_e... File ui/events/blink/blink_event_util.h (right): https://codereview.chromium.org/2746513002/diff/20001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.h:95: DCHECK(IsGestureScrollFlingPinch(event.type())); It isn't immediately obvious why this DCHECK is here. Should it have a comment? https://codereview.chromium.org/2746513002/diff/20001/ui/events/blink/input_h... File ui/events/blink/input_handler_proxy_unittest.cc (right): https://codereview.chromium.org/2746513002/diff/20001/ui/events/blink/input_h... ui/events/blink/input_handler_proxy_unittest.cc:138: } else if (type == WebInputEvent::GestureFlingStart) { Should we have a test involving a fling cancel?
dtapuska@chromium.org changed reviewers: + dtapuska@chromium.org
https://codereview.chromium.org/2746513002/diff/20001/ui/events/blink/blink_e... File ui/events/blink/blink_event_util.h (right): https://codereview.chromium.org/2746513002/diff/20001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.h:95: DCHECK(IsGestureScrollFlingPinch(event.type())); On 2017/03/10 15:31:06, tdresser wrote: > It isn't immediately obvious why this DCHECK is here. Should it have a comment? I'd really expect the dcheck to be calling this: https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebIn...
The CQ bit was checked by chongz@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/2746513002/diff/20001/ui/events/blink/blink_e... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/2746513002/diff/20001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:965: bool IsGestureScrollFlingPinch(WebInputEvent::Type type) { On 2017/03/10 15:31:06, tdresser wrote: > I'd prefer we had an "Or" in there. > > IsGestureScrollFlingOrPinch? Done. https://codereview.chromium.org/2746513002/diff/20001/ui/events/blink/blink_e... File ui/events/blink/blink_event_util.h (right): https://codereview.chromium.org/2746513002/diff/20001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.h:95: DCHECK(IsGestureScrollFlingPinch(event.type())); On 2017/03/10 15:49:14, dtapuska wrote: > On 2017/03/10 15:31:06, tdresser wrote: > > It isn't immediately obvious why this DCHECK is here. Should it have a > comment? > > I'd really expect the dcheck to be calling this: > https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebIn... Done. https://codereview.chromium.org/2746513002/diff/20001/ui/events/blink/input_h... File ui/events/blink/input_handler_proxy_unittest.cc (right): https://codereview.chromium.org/2746513002/diff/20001/ui/events/blink/input_h... ui/events/blink/input_handler_proxy_unittest.cc:138: } else if (type == WebInputEvent::GestureFlingStart) { On 2017/03/10 15:31:06, tdresser wrote: > Should we have a test involving a fling cancel? I've improved the test to cover FlingCancel better. But actually this "if" condition is only for setting extra parameters and won't prevent creating FlingCancel events.
https://codereview.chromium.org/2746513002/diff/40001/ui/events/blink/blink_e... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/2746513002/diff/40001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:965: bool IsGestureScrollFlingOrPinch(WebInputEvent::Type type) { Isn't it IsGestureScrollOrFlingOrPinch?
https://codereview.chromium.org/2746513002/diff/40001/ui/events/blink/blink_e... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/2746513002/diff/40001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:965: bool IsGestureScrollFlingOrPinch(WebInputEvent::Type type) { On 2017/03/14 15:06:51, dtapuska wrote: > Isn't it IsGestureScrollOrFlingOrPinch? I was reading it with an implicit "," between Scroll and Fling. Either way.
On 2017/03/14 15:06:51, dtapuska wrote: > https://codereview.chromium.org/2746513002/diff/40001/ui/events/blink/blink_e... > File ui/events/blink/blink_event_util.cc (right): > > https://codereview.chromium.org/2746513002/diff/40001/ui/events/blink/blink_e... > ui/events/blink/blink_event_util.cc:965: bool > IsGestureScrollFlingOrPinch(WebInputEvent::Type type) { > Isn't it IsGestureScrollOrFlingOrPinch? I'm fine with leaving it named that way but then I think it needs a comment so I don't need to look at the implementation. Because as I read it I believe it is only a Fling or a Pinch.
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 chongz@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/2746513002/diff/40001/ui/events/blink/blink_e... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/2746513002/diff/40001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:965: bool IsGestureScrollFlingOrPinch(WebInputEvent::Type type) { On 2017/03/14 15:27:15, tdresser wrote: > On 2017/03/14 15:06:51, dtapuska wrote: > > Isn't it IsGestureScrollOrFlingOrPinch? > > I was reading it with an implicit "," between Scroll and Fling. > Either way. I've renamed it to |IsGestureScrollOrFlingOrPinch()| so readers don't need to look for implementation or comments.
On 2017/03/14 17:51:24, chongz wrote: > https://codereview.chromium.org/2746513002/diff/40001/ui/events/blink/blink_e... > File ui/events/blink/blink_event_util.cc (right): > > https://codereview.chromium.org/2746513002/diff/40001/ui/events/blink/blink_e... > ui/events/blink/blink_event_util.cc:965: bool > IsGestureScrollFlingOrPinch(WebInputEvent::Type type) { > On 2017/03/14 15:27:15, tdresser wrote: > > On 2017/03/14 15:06:51, dtapuska wrote: > > > Isn't it IsGestureScrollOrFlingOrPinch? > > > > I was reading it with an implicit "," between Scroll and Fling. > > Either way. > > I've renamed it to |IsGestureScrollOrFlingOrPinch()| so readers don't need to > look for implementation or comments. lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chongz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2746513002/#ps60001 (title: "dtapuska@'s review 2: Use IsGestureScrollOrFlingOrPinch")
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": 1489516760335740,
"parent_rev": "93de86905a40d330fd5418a772b7a4ef83491215", "commit_rev":
"ad977926ecd213a1063fb7d429d6fd1d5ee7dce7"}
Message was sent while issue was closed.
Description was changed from ========== [Compositor event queue] Queue GestureFlingStart/Cancel together with scroll/pinch Currently only |GestureScroll*| and |GesturePinch*| will be put into the queue. We should cover |GestureFlingStart/Cancel| as well since it's order-sensitive. BUG=693289 ========== to ========== [Compositor event queue] Queue GestureFlingStart/Cancel together with scroll/pinch Currently only |GestureScroll*| and |GesturePinch*| will be put into the queue. We should cover |GestureFlingStart/Cancel| as well since it's order-sensitive. BUG=693289 Review-Url: https://codereview.chromium.org/2746513002 Cr-Commit-Position: refs/heads/master@{#456772} Committed: https://chromium.googlesource.com/chromium/src/+/ad977926ecd213a1063fb7d429d6... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/ad977926ecd213a1063fb7d429d6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
