Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2401643005:
Create user gesture indicator for pointerup (Closed)
|
Created:
4 years, 2 months ago by Navid Zolghadr Modified:
4 years, 2 months ago CC:
chromium-reviews, blink-reviews, nzolghadr+blinkwatch_chromium.org, dtapuska+blinkwatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate user gesture indicator for pointerup
Creating a user gesture token before dispatching
pointerup event
BUG=609363
Committed: https://crrev.com/53723b1ce879dcd1e5500fe8a3ad070f9dd3543a
Cr-Commit-Position: refs/heads/master@{#425022}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Add usergesture for touch pointerup #Patch Set 3 : Move tests back to where they were before #
Messages
Total messages: 30 (15 generated)
The CQ bit was checked by nzolghadr@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...
nzolghadr@chromium.org changed reviewers: + dtapuska@chromium.org, mustaq@chromium.org
nzolghadr@chromium.org changed reviewers: + rbyers@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/popup-allowed-from-pointerup-exactly-once.html (right): https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/popup-allowed-from-pointerup-exactly-once.html:1: <script src="../../resources/testharness.js"></script> Since LayoutTests/fast/events contains hundreds of files, I suggest moving all UGI tests into a new subfolder. https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:174: wrapUnique(new UserGestureIndicator(DefinitelyProcessingUserGesture)); Can't we make this a one-liner like this? if(pointerup) UserGestureIndicator gestureIndicator(Definitely...);
https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/popup-allowed-from-pointerup-exactly-once.html (right): https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/popup-allowed-from-pointerup-exactly-once.html:1: <script src="../../resources/testharness.js"></script> On 2016/10/07 19:06:21, mustaq wrote: > Since LayoutTests/fast/events contains hundreds of files, I suggest moving all > UGI tests into a new subfolder. Sure. https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:174: wrapUnique(new UserGestureIndicator(DefinitelyProcessingUserGesture)); On 2016/10/07 19:06:21, mustaq wrote: > Can't we make this a one-liner like this? > if(pointerup) > UserGestureIndicator gestureIndicator(Definitely...); That wouldn't work. Because the variable will go out of scope as soon as if ends and the gesture indicator will be destroyed. I need a variable here that is created conditionally and is alive until the end of the function. Beside that Dave wanted me to use the token in TouchEventManager which I'm trying to understand what the logic is there. So this code might change a little. I'll try to finish up the updates and send a new patch today.
https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/popup-allowed-from-pointerup-exactly-once.html (right): https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/popup-allowed-from-pointerup-exactly-once.html:27: } nit: assert that the popup function was actually run (just so that some other bug that prevents the pointerup dispatch would lead to a false-pass).
https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:174: wrapUnique(new UserGestureIndicator(DefinitelyProcessingUserGesture)); On 2016/10/11 13:19:26, Navid Zolghadr wrote: > On 2016/10/07 19:06:21, mustaq wrote: > > Can't we make this a one-liner like this? > > if(pointerup) > > UserGestureIndicator gestureIndicator(Definitely...); > > That wouldn't work. Because the variable will go out of scope as soon as if ends > and the gesture indicator will be destroyed. I need a variable here that is > created conditionally and is alive until the end of the function. > > Beside that Dave wanted me to use the token in TouchEventManager which I'm > trying to understand what the logic is there. So this code might change a > little. I'll try to finish up the updates and send a new patch today. Note that you can also avoid indirection via a unique_ptr by just making the constructor argument conditional. See this related pending overhaul: https://codereview.chromium.org/2401123002/
On 2016/10/11 15:11:52, Rick Byers wrote: > https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): > > https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/input/PointerEventManager.cpp:174: wrapUnique(new > UserGestureIndicator(DefinitelyProcessingUserGesture)); > On 2016/10/11 13:19:26, Navid Zolghadr wrote: > > On 2016/10/07 19:06:21, mustaq wrote: > > > Can't we make this a one-liner like this? > > > if(pointerup) > > > UserGestureIndicator gestureIndicator(Definitely...); > > > > That wouldn't work. Because the variable will go out of scope as soon as if > ends > > and the gesture indicator will be destroyed. I need a variable here that is > > created conditionally and is alive until the end of the function. > > > > Beside that Dave wanted me to use the token in TouchEventManager which I'm > > trying to understand what the logic is there. So this code might change a > > little. I'll try to finish up the updates and send a new patch today. If crbug.com/611981 is successful then I'll be able to simplify this logic dramatically, and then a shared token looks like it'll just fall out. I wouldn't spend any more time now trying to share the token (especially since you want to merge this fix back to M55). > > Note that you can also avoid indirection via a unique_ptr by just making the > constructor argument conditional. See this related pending overhaul: > https://codereview.chromium.org/2401123002/
The CQ bit was checked by nzolghadr@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...
ptal. https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/popup-allowed-from-pointerup-exactly-once.html (right): https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/popup-allowed-from-pointerup-exactly-once.html:27: } On 2016/10/11 15:08:50, Rick Byers wrote: > nit: assert that the popup function was actually run (just so that some other > bug that prevents the pointerup dispatch would lead to a false-pass). It times out if that doesn't run. At least this is the result as far as I saw on my local machine when I commented that addEventListener line. https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:174: wrapUnique(new UserGestureIndicator(DefinitelyProcessingUserGesture)); On 2016/10/11 15:11:52, Rick Byers wrote: > On 2016/10/11 13:19:26, Navid Zolghadr wrote: > > On 2016/10/07 19:06:21, mustaq wrote: > > > Can't we make this a one-liner like this? > > > if(pointerup) > > > UserGestureIndicator gestureIndicator(Definitely...); > > > > That wouldn't work. Because the variable will go out of scope as soon as if > ends > > and the gesture indicator will be destroyed. I need a variable here that is > > created conditionally and is alive until the end of the function. > > > > Beside that Dave wanted me to use the token in TouchEventManager which I'm > > trying to understand what the logic is there. So this code might change a > > little. I'll try to finish up the updates and send a new patch today. > > Note that you can also avoid indirection via a unique_ptr by just making the > constructor argument conditional. See this related pending overhaul: > https://codereview.chromium.org/2401123002/ Since the other change landed before this if I add more changes to this CL I will get more conflicts when merging to 55. Following our offline discussion I'll keep this part as is.
On 2016/10/12 15:18:01, Navid Zolghadr wrote: > ptal. > > https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/LayoutTe... > File > third_party/WebKit/LayoutTests/fast/events/popup-allowed-from-pointerup-exactly-once.html > (right): > > https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/events/popup-allowed-from-pointerup-exactly-once.html:27: > } > On 2016/10/11 15:08:50, Rick Byers wrote: > > nit: assert that the popup function was actually run (just so that some other > > bug that prevents the pointerup dispatch would lead to a false-pass). > > It times out if that doesn't run. At least this is the result as far as I saw on > my local machine when I commented that addEventListener line. > > https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): > > https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/input/PointerEventManager.cpp:174: wrapUnique(new > UserGestureIndicator(DefinitelyProcessingUserGesture)); > On 2016/10/11 15:11:52, Rick Byers wrote: > > On 2016/10/11 13:19:26, Navid Zolghadr wrote: > > > On 2016/10/07 19:06:21, mustaq wrote: > > > > Can't we make this a one-liner like this? > > > > if(pointerup) > > > > UserGestureIndicator gestureIndicator(Definitely...); > > > > > > That wouldn't work. Because the variable will go out of scope as soon as if > > ends > > > and the gesture indicator will be destroyed. I need a variable here that is > > > created conditionally and is alive until the end of the function. > > > > > > Beside that Dave wanted me to use the token in TouchEventManager which I'm > > > trying to understand what the logic is there. So this code might change a > > > little. I'll try to finish up the updates and send a new patch today. > > > > Note that you can also avoid indirection via a unique_ptr by just making the > > constructor argument conditional. See this related pending overhaul: > > https://codereview.chromium.org/2401123002/ > > Since the other change landed before this if I add more changes to this CL I > will get more conflicts when merging to 55. Following our offline discussion > I'll keep this part as is. Since you're going to want to merge this CL, maybe land the movement of tests as a follow-up CL to keep the diff here much smaller?
On 2016/10/12 15:18:01, Navid Zolghadr wrote: > ptal. > > https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/LayoutTe... > File > third_party/WebKit/LayoutTests/fast/events/popup-allowed-from-pointerup-exactly-once.html > (right): > > https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/events/popup-allowed-from-pointerup-exactly-once.html:27: > } > On 2016/10/11 15:08:50, Rick Byers wrote: > > nit: assert that the popup function was actually run (just so that some other > > bug that prevents the pointerup dispatch would lead to a false-pass). > > It times out if that doesn't run. At least this is the result as far as I saw on > my local machine when I commented that addEventListener line. > > https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): > > https://codereview.chromium.org/2401643005/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/input/PointerEventManager.cpp:174: wrapUnique(new > UserGestureIndicator(DefinitelyProcessingUserGesture)); > On 2016/10/11 15:11:52, Rick Byers wrote: > > On 2016/10/11 13:19:26, Navid Zolghadr wrote: > > > On 2016/10/07 19:06:21, mustaq wrote: > > > > Can't we make this a one-liner like this? > > > > if(pointerup) > > > > UserGestureIndicator gestureIndicator(Definitely...); > > > > > > That wouldn't work. Because the variable will go out of scope as soon as if > > ends > > > and the gesture indicator will be destroyed. I need a variable here that is > > > created conditionally and is alive until the end of the function. > > > > > > Beside that Dave wanted me to use the token in TouchEventManager which I'm > > > trying to understand what the logic is there. So this code might change a > > > little. I'll try to finish up the updates and send a new patch today. > > > > Note that you can also avoid indirection via a unique_ptr by just making the > > constructor argument conditional. See this related pending overhaul: > > https://codereview.chromium.org/2401123002/ > > Since the other change landed before this if I add more changes to this CL I > will get more conflicts when merging to 55. Following our offline discussion > I'll keep this part as is. Since you're going to want to merge this CL, maybe land the movement of tests as a follow-up CL to keep the diff here much smaller?
The CQ bit was checked by nzolghadr@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...
done. ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org Link to the patchset: https://codereview.chromium.org/2401643005/#ps40001 (title: "Move tests back to where they were before")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Create user gesture indicator for pointerup Creating a user gesture token before dispatching pointerup event BUG=609363 ========== to ========== Create user gesture indicator for pointerup Creating a user gesture token before dispatching pointerup event BUG=609363 Committed: https://crrev.com/53723b1ce879dcd1e5500fe8a3ad070f9dd3543a Cr-Commit-Position: refs/heads/master@{#425022} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/53723b1ce879dcd1e5500fe8a3ad070f9dd3543a Cr-Commit-Position: refs/heads/master@{#425022} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
