Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2860663006:
Remove WebTouchEvent from TouchEventManager APIs (Closed)
|
Created:
3 years, 7 months ago by Navid Zolghadr Modified:
3 years, 5 months ago CC:
chromium-reviews, blink-reviews, dglazkov+blink, dtapuska+blinkwatch_chromium.org, blink-reviews-api_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove WebTouchEvent from TouchEventManager APIs
This is the first CL to migrate the touch event
paths to pointerevents.
BUG=625841
Review-Url: https://codereview.chromium.org/2860663006
Cr-Commit-Position: refs/heads/master@{#481655}
Committed: https://chromium.googlesource.com/chromium/src/+/8af61ebffa92e8d012cb0a9046bcf91aa7762ece
Patch Set 1 #
Total comments: 8
Patch Set 2 : Redo the whole change #Patch Set 3 : Address comments and fix a test #Patch Set 4 : Fix more tests #
Total comments: 26
Patch Set 5 : Apply the comments #
Total comments: 32
Patch Set 6 : Rebase #Patch Set 7 : Apply more comments #Patch Set 8 : Fix some DCHECKs #
Total comments: 5
Patch Set 9 : Handle the cases for inconsitent inputs #
Total comments: 13
Patch Set 10 : Rename a few functions/vars #
Total comments: 2
Patch Set 11 : Add TODO for clarification of the fields #
Total comments: 2
Messages
Total messages: 80 (55 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
Do you think this is a good enough temporary approach for unblocking the ppapi?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
mustaq@google.com changed reviewers: + mustaq@google.com
Here are my comments for now: https://codereview.chromium.org/2860663006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:51: WebPointerEvent CreateWebPointerEventFromTouchPoint( We will ultimately need conversion methods from/to Web{Mouse,Touch}*, so may be we can move this to a WebPointerEventUtils class. I am fine with a TODO now. https://codereview.chromium.org/2860663006/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebPointerEvent.h (right): https://codereview.chromium.org/2860663006/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebPointerEvent.h:15: // WebPointerEvent Please add a detailed note here to clarify that this is a work in progress, to prevent other teams picking it up in the incomplete state. In particular, mention that: - this is used only in Blink event handling - and for a limited case (to touch-only). https://codereview.chromium.org/2860663006/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebPointerEvent.h:34: WebFloatPoint screen_position; Let's use the coordinate names as in WebMouseEvent, and quote crbug.com/508283. https://codereview.chromium.org/2860663006/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebPointerProperties.h (right): https://codereview.chromium.org/2860663006/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebPointerProperties.h:50: kPointerUndefined, kPointerUndefined is unused in Blink, and WebTouchPoint::State has a "stationary" state that we need here. Other than these two, all states are 1:1 with event types, right? From that perspective, we only need to store a Boolean here for the stationary state.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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 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...
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/2860663006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:51: WebPointerEvent CreateWebPointerEventFromTouchPoint( On 2017/05/08 17:07:30, use mustaq_at_chromium.org wrote: > We will ultimately need conversion methods from/to Web{Mouse,Touch}*, so may be > we can move this to a WebPointerEventUtils class. I am fine with a TODO now. I moved it to the WebPointerEvent constructor. https://codereview.chromium.org/2860663006/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebPointerEvent.h (right): https://codereview.chromium.org/2860663006/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebPointerEvent.h:15: // WebPointerEvent On 2017/05/08 17:07:30, use mustaq_at_chromium.org wrote: > Please add a detailed note here to clarify that this is a work in progress, to > prevent other teams picking it up in the incomplete state. In particular, > mention that: > - this is used only in Blink event handling > - and for a limited case (to touch-only). Done. https://codereview.chromium.org/2860663006/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebPointerEvent.h:34: WebFloatPoint screen_position; On 2017/05/08 17:07:30, use mustaq_at_chromium.org wrote: > Let's use the coordinate names as in WebMouseEvent, and quote crbug.com/508283. Removed completely as your CL makes this unnecessary. https://codereview.chromium.org/2860663006/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebPointerProperties.h (right): https://codereview.chromium.org/2860663006/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebPointerProperties.h:50: kPointerUndefined, On 2017/05/08 17:07:31, use mustaq_at_chromium.org wrote: > kPointerUndefined is unused in Blink, and WebTouchPoint::State has a > "stationary" state that we need here. Other than these two, all states are 1:1 > with event types, right? > > From that perspective, we only need to store a Boolean here for the stationary > state. I did the same idea in the new CL. But I don't add a new enum anymore. The boolean is called stale_ and is stored in TouchEventManager.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Here are my comments so far. I am not done with checking TEM yet. https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:427: if (pointer_event_target.target_node && pointer_event_target.target_frame && Why are you not skipping the stationary points here? https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:17: #include "public/platform/WebTouchEvent.h" This header file doesn't need this #include. Move it to .cpp. https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:168: adjusted_radius = FloatSize(transformed_event.width, transformed_event.height) Radius = (width/2, height/2); https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:292: // There is exactly one touch point here otherwise Please add a dcheck. https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/WebPointerEvent.cpp (right): https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebPointerEvent.cpp:13: WebInputEvent::Type PointerEventActionForTouchPointState( Again, why "Action" instead of "Type"? https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebPointerEvent.cpp:36: width(touch_point.radius_x), Width/height should be radius*2. https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebPluginContainerTest.cpp (right): https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebPluginContainerTest.cpp:648: WebTouchEvent event(WebInputEvent::kTouchStart, WebInputEvent::kNoModifiers, Clarification question: why do we need this? https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebInputEvent.h (right): https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebInputEvent.h:183: kPointerDown, Add a "section header": // WebPointerEvent: work in progress https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebInputEvent.h:188: kPointerActionLast = kPointerCancel, s/kPointerAction.../kPointerType.../? Existing blink::PointerType is a concern but its too different in meaning to be confusing IMO. https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebInputEvent.h:319: return type_ == other.type_; Let's update this after adding a method IsPointerEventType(). https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebInputEvent.h:365: CASE_TYPE(TouchScrollStarted); Please add the new event types here. Getting rid of the default case below is better? So that the compiler could catch any missing entry? https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebPointerEvent.h (right): https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebPointerEvent.h:31: // TODO(nzolghadr): We should unify the following fields and not have This comment applies to the width & height fields too. May be make it a class level comment.
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
ptal https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:427: if (pointer_event_target.target_node && pointer_event_target.target_frame && On 2017/06/08 20:02:52, mustaq wrote: > Why are you not skipping the stationary points here? I have moved the check outside of the function and basically ignore the stationary points for all the actions including passing it to TouchEventManager and firing the pointerevents. Later I'm going to move this if more and more outside and basically this class will never get a stationary point. https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:17: #include "public/platform/WebTouchEvent.h" On 2017/06/08 20:02:52, mustaq wrote: > This header file doesn't need this #include. Move it to .cpp. Removed https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:168: adjusted_radius = FloatSize(transformed_event.width, transformed_event.height) On 2017/06/08 20:02:52, mustaq wrote: > Radius = (width/2, height/2); I can see what you mean. But this is not what we did when we create the DOM pointerevent from touch points. We use the radius_x/y fields as is to set it the width/height of the DOM pointerevent. So I used the same definition of width height in WebPointerEvent. Do you think I should change the definition of width height in WebPointerEvent and not having it matched with DOM pointerevent? https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:292: // There is exactly one touch point here otherwise On 2017/06/08 20:02:52, mustaq wrote: > Please add a dcheck. Done. https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/WebPointerEvent.cpp (right): https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebPointerEvent.cpp:13: WebInputEvent::Type PointerEventActionForTouchPointState( On 2017/06/08 20:02:52, mustaq wrote: > Again, why "Action" instead of "Type"? I wanted to avoid the confusion with PointerType (i.e. "mouse", "touch", "pen". I renamed it to "Type" anyway. https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebPointerEvent.cpp:36: width(touch_point.radius_x), On 2017/06/08 20:02:52, mustaq wrote: > Width/height should be radius*2. I commented about this in another reply. Let me know what you think. https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebPluginContainerTest.cpp (right): https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebPluginContainerTest.cpp:648: WebTouchEvent event(WebInputEvent::kTouchStart, WebInputEvent::kNoModifiers, On 2017/06/08 20:02:52, mustaq wrote: > Clarification question: why do we need this? Out code never expects to receive a move coalesced with a press. So they should come in different rounds. We also check this condition visa CanCoalesce functions and it only returns true if both of the events are move and otherwise nothing can get coalesced. https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebInputEvent.h (right): https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebInputEvent.h:183: kPointerDown, On 2017/06/08 20:02:53, mustaq wrote: > Add a "section header": // WebPointerEvent: work in progress Done. https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebInputEvent.h:188: kPointerActionLast = kPointerCancel, On 2017/06/08 20:02:53, mustaq wrote: > s/kPointerAction.../kPointerType.../? Existing blink::PointerType is a concern > but its too different in meaning to be confusing IMO. Done. https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebInputEvent.h:319: return type_ == other.type_; On 2017/06/08 20:02:53, mustaq wrote: > Let's update this after adding a method IsPointerEventType(). Done. https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebInputEvent.h:365: CASE_TYPE(TouchScrollStarted); On 2017/06/08 20:02:53, mustaq wrote: > Please add the new event types here. > > Getting rid of the default case below is better? So that the compiler could > catch any missing entry? Done. https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebPointerEvent.h (right): https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebPointerEvent.h:31: // TODO(nzolghadr): We should unify the following fields and not have On 2017/06/08 20:02:53, mustaq wrote: > This comment applies to the width & height fields too. > > May be make it a class level comment. Done.
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_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
This looks great. I have a few more cleanup/documentation requests. Any thought how we are (ultimately) going to test event sequences spanning across multiple vsync windows? https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:427: if (pointer_event_target.target_node && pointer_event_target.target_frame && On 2017/06/08 21:18:40, Navid Zolghadr wrote: > On 2017/06/08 20:02:52, mustaq wrote: > > Why are you not skipping the stationary points here? > > I have moved the check outside of the function and basically ignore the > stationary points for all the actions including passing it to TouchEventManager > and firing the pointerevents. Later I'm going to move this if more and more > outside and basically this class will never get a stationary point. Sg, thanks. Please check the comments about stationary points in TEM & PEM, some are stale now. https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:168: adjusted_radius = FloatSize(transformed_event.width, transformed_event.height) On 2017/06/08 21:18:41, Navid Zolghadr wrote: > On 2017/06/08 20:02:52, mustaq wrote: > > Radius = (width/2, height/2); > > I can see what you mean. But this is not what we did when we create the DOM > pointerevent from touch points. We use the radius_x/y fields as is to set it the > width/height of the DOM pointerevent. So I used the same definition of width > height in WebPointerEvent. Do you think I should change the definition of width > height in WebPointerEvent and not having it matched with DOM pointerevent? Yikes, perhaps the blame goes to me! Filed crbug.com/731725, please add a TODO (or fix that bug first). Makes sense to keep the fix separate. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:210: web_touch_point.radius_x = touch_pointer_event.width; Repeat the TODO here for radius/width problem. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:218: if (touch_event_type == WebInputEvent::kTouchMove) { You already hardcoded the event type in Line 197 above. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:243: // This assumes that we only get move events as coalesced events. This comment also applies to the above hardcoded kTouchMove type. I suggest adding a DCHECK to enforce this early on in this function. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:256: for (unsigned i = 0; i < event.touches_length; ++i) { After adding to the coalesced events, |event| is not used anywhere. Why do you need this 2nd loop? https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:272: bool new_touch_point_since_last_raf = false; Let's not use "raf" here since this method will eventually be called at both VSync signals and discrete events. It seems s/_last_raf/_last_dispatch/ covers both the cases. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:273: bool any_touch_canceled_ended = false; ..._canceled_or_ended? https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:324: WebInputEvent::Type pointer_action = s/pointer_action/pointer_event_type/ https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:326: size_t pointer_action_idx = event_type_idx? https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:367: // First we construct the webcoalescedinputevent contains all the coalesced /containing/ https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:391: touch_event_target->DispatchEvent(touch_event); Consider these event sequece, all within the same vsync window: touchstart (id=11) => dispatched immediately touchstart (id=22) => dispatched immediately touchmove (id=22) => queued. touchmove (id=22) => queued touchend (id=x) => dispatch touchmoves then touchend, regardless of x So ultimately we will need to include timestamp order somehow. Since the current logic works now (because we are dispatching at every event for now), you may add a TODO and move on. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:393: // Only report for top level documents with a single touch on The UMA block is too long and a bit distracting here. Can we please move this off to a separate method? https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:634: touch_points_attributes_.erase(id); Can use .erase(releasedCanceledPoints) instead? https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.h (right): https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:60: // The list of coalesced events if there is any. Note that at the end of Please clarify if this list include all TEs, or only the ones corresponding to the TouchPoint. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:80: WebInputEventResult DispatchTouchEvents(); Rename to something like DispatchAccumulatedTouchEvents() to clarify the purpose. May be even add a comment that this is triggered by either VSync signal or discrete events. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:87: using TouchAttributeMap = Please keep the past comment re "indexed by touch ids". https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:92: TouchAttributeMap touch_points_attributes_; Nit: I would avoid double plural in the name. s/touch_points_attributes_/touch_attribute_map_/?
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/2860663006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:210: web_touch_point.radius_x = touch_pointer_event.width; On 2017/06/09 16:12:27, mustaq wrote: > Repeat the TODO here for radius/width problem. I didn't have TODO anywhere else. I added a TODO here and one in WebPointerEvent which I do the other side of translation. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:218: if (touch_event_type == WebInputEvent::kTouchMove) { On 2017/06/09 16:12:27, mustaq wrote: > You already hardcoded the event type in Line 197 above. This is in a for loop. So the value of the touch_event_type is not necessary kTouchMove. Although I just realized I can do the event.SetType out of this loop. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:243: // This assumes that we only get move events as coalesced events. On 2017/06/09 16:12:27, mustaq wrote: > This comment also applies to the above hardcoded kTouchMove type. I suggest > adding a DCHECK to enforce this early on in this function. I added a DCHECK. However, I also changed the code to handle more cases and updated the test in WebPluginContainerTest. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:256: for (unsigned i = 0; i < event.touches_length; ++i) { On 2017/06/09 16:12:27, mustaq wrote: > After adding to the coalesced events, |event| is not used anywhere. Why do you > need this 2nd loop? It's used for the next coalesced event (i.e. next iteration of the outer loop). Basically each coalesced event should be compared to its previous coalesced event to set the states and movement_x/y. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:272: bool new_touch_point_since_last_raf = false; On 2017/06/09 16:12:27, mustaq wrote: > Let's not use "raf" here since this method will eventually be called at both > VSync signals and discrete events. It seems s/_last_raf/_last_dispatch/ covers > both the cases. Done. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:273: bool any_touch_canceled_ended = false; On 2017/06/09 16:12:27, mustaq wrote: > ..._canceled_or_ended? Done. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:324: WebInputEvent::Type pointer_action = On 2017/06/09 16:12:27, mustaq wrote: > s/pointer_action/pointer_event_type/ Done. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:326: size_t pointer_action_idx = On 2017/06/09 16:12:27, mustaq wrote: > event_type_idx? Done. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:367: // First we construct the webcoalescedinputevent contains all the coalesced On 2017/06/09 16:12:27, mustaq wrote: > /containing/ Done. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:391: touch_event_target->DispatchEvent(touch_event); On 2017/06/09 16:12:27, mustaq wrote: > Consider these event sequece, all within the same vsync window: > touchstart (id=11) => dispatched immediately > touchstart (id=22) => dispatched immediately > touchmove (id=22) => queued. > touchmove (id=22) => queued > touchend (id=x) => dispatch touchmoves then touchend, regardless of x > > So ultimately we will need to include timestamp order somehow. > > Since the current logic works now (because we are dispatching at every event for > now), you may add a TODO and move on. This code does not handle that. My understanding at the moment is that the main_thread_event_queue will take care of calling HandleVSyncSignal whenever it desires. Note that Dave also added another API that let the extension to choose the events being sent as the come without any coalescing. So in that case the main_thread_event_queue class will send an event and immediately call the HandleVsyncSignal. Now that I'm thinking since I have those usecases maybe the name HandleVsyncSignal it not very fitting here. I changed it to FlushEvents instead. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:393: // Only report for top level documents with a single touch on On 2017/06/09 16:12:27, mustaq wrote: > The UMA block is too long and a bit distracting here. Can we please move this > off to a separate method? Done. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:634: touch_points_attributes_.erase(id); On 2017/06/09 16:12:27, mustaq wrote: > Can use .erase(releasedCanceledPoints) instead? Not quite. But there seems to be a RemoveAll function. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.h (right): https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:60: // The list of coalesced events if there is any. Note that at the end of On 2017/06/09 16:12:27, mustaq wrote: > Please clarify if this list include all TEs, or only the ones corresponding to > the TouchPoint. Done. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:80: WebInputEventResult DispatchTouchEvents(); On 2017/06/09 16:12:29, mustaq wrote: > Rename to something like DispatchAccumulatedTouchEvents() to clarify the > purpose. May be even add a comment that this is triggered by either VSync > signal or discrete events. Done. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:87: using TouchAttributeMap = On 2017/06/09 16:12:27, mustaq wrote: > Please keep the past comment re "indexed by touch ids". Done. https://codereview.chromium.org/2860663006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:92: TouchAttributeMap touch_points_attributes_; On 2017/06/09 16:12:29, mustaq wrote: > Nit: I would avoid double plural in the name. > s/touch_points_attributes_/touch_attribute_map_/? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
mustaq@chromium.org changed reviewers: - mustaq@google.com
Looks good, thanks. I will do a final pass after the tests are okay. https://codereview.chromium.org/2860663006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/PointerEventManager.cpp:368: return touch_event_manager_->FlushEvents(); Please add a TODO re main_thread_event_queue calls, possibly pointing to Dave's bug. https://codereview.chromium.org/2860663006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:318: DCHECK_EQ(event_type, web_pointer_event.GetType()); Any clue why multi-touch-partial-sequence.html causes a check-failure here?
https://codereview.chromium.org/2860663006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:318: DCHECK_EQ(event_type, web_pointer_event.GetType()); On 2017/06/13 15:15:24, mustaq wrote: > Any clue why multi-touch-partial-sequence.html causes a check-failure here? Apparently this assumption is incorrect. The current code handles that anyway. But in that test we can receive a move and down in one shot. Because apparently the down of the first touch point was out of the renderer. I guess in general our event_sender can send any combination as well and we shouldn't crash in any case. So I need to remove this DCHECK.
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...
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_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-...) 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_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 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/2860663006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/PointerEventManager.cpp:368: return touch_event_manager_->FlushEvents(); On 2017/06/13 15:15:24, mustaq wrote: > Please add a TODO re main_thread_event_queue calls, possibly pointing to Dave's > bug. Done. https://codereview.chromium.org/2860663006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:318: DCHECK_EQ(event_type, web_pointer_event.GetType()); On 2017/06/13 15:19:08, Navid Zolghadr wrote: > On 2017/06/13 15:15:24, mustaq wrote: > > Any clue why multi-touch-partial-sequence.html causes a check-failure here? > > Apparently this assumption is incorrect. The current code handles that anyway. > But in that test we can receive a move and down in one shot. Because apparently > the down of the first touch point was out of the renderer. I guess in general > our event_sender can send any combination as well and we shouldn't crash in any > case. So I need to remove this DCHECK. There were also other cases that event_sender allowed more stuff. I also handled them in the last patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/13 19:31:17, Navid Zolghadr wrote: > ptal. > > https://codereview.chromium.org/2860663006/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): > > https://codereview.chromium.org/2860663006/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/input/PointerEventManager.cpp:368: return > touch_event_manager_->FlushEvents(); > On 2017/06/13 15:15:24, mustaq wrote: > > Please add a TODO re main_thread_event_queue calls, possibly pointing to > Dave's > > bug. > > Done. > > https://codereview.chromium.org/2860663006/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): > > https://codereview.chromium.org/2860663006/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/input/TouchEventManager.cpp:318: > DCHECK_EQ(event_type, web_pointer_event.GetType()); > On 2017/06/13 15:19:08, Navid Zolghadr wrote: > > On 2017/06/13 15:15:24, mustaq wrote: > > > Any clue why multi-touch-partial-sequence.html causes a check-failure here? > > > > Apparently this assumption is incorrect. The current code handles that anyway. > > But in that test we can receive a move and down in one shot. Because > apparently > > the down of the first touch point was out of the renderer. I guess in general > > our event_sender can send any combination as well and we shouldn't crash in > any > > case. So I need to remove this DCHECK. > > There were also other cases that event_sender allowed more stuff. I also handled > them in the last patch. Does this look okay now?
lgtm
On 2017/06/15 16:20:12, mustaq wrote: > lgtm Dave, do you think this is good enough to land for the starter?
https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:90: web_touch_point.radius_x = web_pointer_event.width; I'm not sure we can break this today can we? What are the implications of doing that? ie. Broken touch disambiguation in web apps? Perhaps we should remove radiusX/radiusY from WebTouchPoint first before this change? https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:310: touch_point_attribute->coalesced_events_) Need brackets... https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:375: i++) { This loop uses the same variable name as the outside loop one. https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:698: Vector<int> releasedCanceledPoints; s/releasedCanceledPoints/released_canceled_points https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:719: void TouchEventManager::allTouchesReleasedCleanup() { s/all/All
https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:90: web_touch_point.radius_x = web_pointer_event.width; On 2017/06/20 16:17:21, dtapuska wrote: > I'm not sure we can break this today can we? > What are the implications of doing that? ie. Broken touch disambiguation in web > apps? Perhaps we should remove radiusX/radiusY from WebTouchPoint first before > this change? It is already broken. I just kept the logic as before. So nothing new is going to break here. Having that, do you still think we should move the width/height from WebPointerEvent to PointerProperties before this change? https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:310: touch_point_attribute->coalesced_events_) On 2017/06/20 16:17:21, dtapuska wrote: > Need brackets... I don't think it does. The line inside the for loop is still a one liner. Particularly our bracket pre submit checker didn't require this either. Do you still want me to put brackets?
https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:90: web_touch_point.radius_x = web_pointer_event.width; On 2017/06/20 16:33:00, Navid Zolghadr wrote: > On 2017/06/20 16:17:21, dtapuska wrote: > > I'm not sure we can break this today can we? > > What are the implications of doing that? ie. Broken touch disambiguation in > web > > apps? Perhaps we should remove radiusX/radiusY from WebTouchPoint first before > > this change? > > It is already broken. I just kept the logic as before. So nothing new is going > to break here. Having that, do you still think we should move the width/height > from WebPointerEvent to PointerProperties before this change? No as explained offline to me. WebPointerEvent is populated with the radiusX/radiusY from the original touch event and then populated back here. So it should be safe. Of course all those code is just intermediate until the whole pipeline is cleaned up. https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:310: touch_point_attribute->coalesced_events_) On 2017/06/20 16:33:00, Navid Zolghadr wrote: > On 2017/06/20 16:17:21, dtapuska wrote: > > Need brackets... > > I don't think it does. The line inside the for loop is still a one liner. > Particularly our bracket pre submit checker didn't require this either. Do you > still want me to put brackets? Generally if the condition/loop wraps onto more than one line we add braces because it has moved from simple to complex.
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/2860663006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:310: touch_point_attribute->coalesced_events_) On 2017/06/20 16:44:03, dtapuska wrote: > On 2017/06/20 16:33:00, Navid Zolghadr wrote: > > On 2017/06/20 16:17:21, dtapuska wrote: > > > Need brackets... > > > > I don't think it does. The line inside the for loop is still a one liner. > > Particularly our bracket pre submit checker didn't require this either. Do you > > still want me to put brackets? > > Generally if the condition/loop wraps onto more than one line we add braces > because it has moved from simple to complex. Done. https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:375: i++) { On 2017/06/20 16:17:21, dtapuska wrote: > This loop uses the same variable name as the outside loop one. Done. https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:698: Vector<int> releasedCanceledPoints; On 2017/06/20 16:17:21, dtapuska wrote: > s/releasedCanceledPoints/released_canceled_points Done. https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:719: void TouchEventManager::allTouchesReleasedCleanup() { On 2017/06/20 16:17:21, dtapuska wrote: > s/all/All Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/20 17:59:15, Navid Zolghadr wrote: > ptal > > https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): > > https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/input/TouchEventManager.cpp:310: > touch_point_attribute->coalesced_events_) > On 2017/06/20 16:44:03, dtapuska wrote: > > On 2017/06/20 16:33:00, Navid Zolghadr wrote: > > > On 2017/06/20 16:17:21, dtapuska wrote: > > > > Need brackets... > > > > > > I don't think it does. The line inside the for loop is still a one liner. > > > Particularly our bracket pre submit checker didn't require this either. Do > you > > > still want me to put brackets? > > > > Generally if the condition/loop wraps onto more than one line we add braces > > because it has moved from simple to complex. > > Done. > > https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/input/TouchEventManager.cpp:375: i++) { > On 2017/06/20 16:17:21, dtapuska wrote: > > This loop uses the same variable name as the outside loop one. > > Done. > > https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/input/TouchEventManager.cpp:698: Vector<int> > releasedCanceledPoints; > On 2017/06/20 16:17:21, dtapuska wrote: > > s/releasedCanceledPoints/released_canceled_points > > Done. > > https://codereview.chromium.org/2860663006/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/input/TouchEventManager.cpp:719: void > TouchEventManager::allTouchesReleasedCleanup() { > On 2017/06/20 16:17:21, dtapuska wrote: > > s/all/All > > Done. lgtm
nzolghadr@chromium.org changed reviewers: + rbyers@chromium.org
rbyers@chromium.org: Please review changes in third_party/WebKit/public/*
LGTM with nit https://codereview.chromium.org/2860663006/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebPointerEvent.h (right): https://codereview.chromium.org/2860663006/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebPointerEvent.h:30: float width; nit: please add comments describing the semantics / units for these three fields (or if you/bokan@ can't easily figure it out - a TODO with the open questions). In general we want the public API to be pretty well documented.
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...
https://codereview.chromium.org/2860663006/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebPointerEvent.h (right): https://codereview.chromium.org/2860663006/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebPointerEvent.h:30: float width; On 2017/06/21 15:03:27, Rick Byers wrote: > nit: please add comments describing the semantics / units for these three fields > (or if you/bokan@ can't easily figure it out - a TODO with the open questions). > In general we want the public API to be pretty well documented. done.
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 nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org, dtapuska@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2860663006/#ps200001 (title: "Add TODO for clarification of the fields")
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": 200001, "attempt_start_ts": 1498163854962760,
"parent_rev": "db57d232f21b4f2b09c362d6fabeb2a2286cd863", "commit_rev":
"8af61ebffa92e8d012cb0a9046bcf91aa7762ece"}
Message was sent while issue was closed.
Description was changed from ========== Remove WebTouchEvent from TouchEventManager APIs This is the first CL to migrate the touch event paths to pointerevents. BUG=625841 ========== to ========== Remove WebTouchEvent from TouchEventManager APIs This is the first CL to migrate the touch event paths to pointerevents. BUG=625841 Review-Url: https://codereview.chromium.org/2860663006 Cr-Commit-Position: refs/heads/master@{#481655} Committed: https://chromium.googlesource.com/chromium/src/+/8af61ebffa92e8d012cb0a9046bc... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/8af61ebffa92e8d012cb0a9046bc...
Message was sent while issue was closed.
jkwang@google.com changed reviewers: + jkwang@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/2860663006/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:362: last_coalesced_touch_event_.touches[i].movement_x = Why not call "CreateWebTouchPointFromWebPointerEvent(web_pointer_event, false);" here? Why not update "PositionInWidget" and "PositionInScreen" here?
Message was sent while issue was closed.
https://codereview.chromium.org/2860663006/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2860663006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:362: last_coalesced_touch_event_.touches[i].movement_x = On 2017/07/10 23:49:39, jkwang wrote: > Why not call "CreateWebTouchPointFromWebPointerEvent(web_pointer_event, false);" > here? > Why not update "PositionInWidget" and "PositionInScreen" here? I cannot think of any reason not to. Calling "CreateWebTouchPointFromWebPointerEvent(web_pointer_event, false)" seems to be taking care of it all. |
