Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2925883003:
[Touch Adjustment] Pass primary_pointer_type to WebGestureEvent and disable adjustment for stylus (Closed)
|
Created:
3 years, 6 months ago by chongz Modified:
3 years, 5 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, dglazkov+blink, dtapuska+blinkwatch_chromium.org, dtapuska+chromiumwatch_chromium.org, kinuko+watch, Navid Zolghadr, platform-architecture-syd+reviews-web_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Touch Adjustment] Pass primary_pointer_type to WebGestureEvent and disable adjustment for stylus
Event Flow:
1. MotionEvent.GetToolType() =>
2. GestureEventDetails.primary_pointer_type_ =>
3. WebGestureEvent.primary_pointer_type
This CL is intended to fix stylus issue on ChromeOS devices. (e.g. Samsung
Chromebook Plus)
Background:
1. ChromeOS: Stylus generates touch events with large width&height.
2. Android: Stylus generates touch events with 0 width&height.
3. Windows: Stylus generates mouse events.
Manually tested on Samsung Galaxy Note 5 and |primary_pointer_type| was passed
successfully.
BUG=731856
Review-Url: https://codereview.chromium.org/2925883003
Cr-Commit-Position: refs/heads/master@{#482892}
Committed: https://chromium.googlesource.com/chromium/src/+/d65eacf7288645732c715f588fc3f5d4ae0e0d81
Patch Set 1 #
Total comments: 5
Patch Set 2 : Add layout test; Update comments; Remove ctor #
Total comments: 2
Patch Set 3 : dtapuska's comment: Add args check #
Total comments: 2
Patch Set 4 : dtapuska's comment: Fix layout test instead #
Total comments: 7
Patch Set 5 : Rebase #Patch Set 6 : jochen and dtapuska's comments: Add default value #Messages
Total messages: 60 (36 generated)
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 checked by chongz@chromium.org to run a CQ dry run
Patchset #1 (id:20001) 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...
Description was changed from ========== Add pointer_type to WebGestureEvent BUG= ========== to ========== [Touch Adjustment] Pass primary_pointer_type to WebGestureEvent and disable for stylus Property Flow: 1. MotionEvent.GetToolType() => 2. GestureEventDetails.primary_pointer_type_ => 3. WebGestureEvent.primary_pointer_type BUG=731856 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Touch Adjustment] Pass primary_pointer_type to WebGestureEvent and disable for stylus Property Flow: 1. MotionEvent.GetToolType() => 2. GestureEventDetails.primary_pointer_type_ => 3. WebGestureEvent.primary_pointer_type BUG=731856 ========== to ========== [Touch Adjustment] Pass primary_pointer_type to WebGestureEvent and disable adjustment for stylus Property Flow: 1. MotionEvent.GetToolType() => 2. GestureEventDetails.primary_pointer_type_ => 3. WebGestureEvent.primary_pointer_type BUG=731856 ==========
Description was changed from ========== [Touch Adjustment] Pass primary_pointer_type to WebGestureEvent and disable adjustment for stylus Property Flow: 1. MotionEvent.GetToolType() => 2. GestureEventDetails.primary_pointer_type_ => 3. WebGestureEvent.primary_pointer_type BUG=731856 ========== to ========== [Touch Adjustment] Pass primary_pointer_type to WebGestureEvent and disable adjustment for stylus Property Flow: 1. MotionEvent.GetToolType() => 2. GestureEventDetails.primary_pointer_type_ => 3. WebGestureEvent.primary_pointer_type Manually tested on Samsung Galaxy Note 5 and |primary_pointer_type| was passed successfully. BUG=731856 ==========
Description was changed from ========== [Touch Adjustment] Pass primary_pointer_type to WebGestureEvent and disable adjustment for stylus Property Flow: 1. MotionEvent.GetToolType() => 2. GestureEventDetails.primary_pointer_type_ => 3. WebGestureEvent.primary_pointer_type Manually tested on Samsung Galaxy Note 5 and |primary_pointer_type| was passed successfully. BUG=731856 ========== to ========== [Touch Adjustment] Pass primary_pointer_type to WebGestureEvent and disable adjustment for stylus Property Flow: 1. MotionEvent.GetToolType() => 2. GestureEventDetails.primary_pointer_type_ => 3. WebGestureEvent.primary_pointer_type This CL is intended to fix stylus issue on ChromeOS devices. Background: 1. ChromeOS: Stylus generates touch events with large width&height. 2. Android: Stylus generates touch events with 0 width&height. 3. Windows: Stylus generates mouse events. Manually tested on Samsung Galaxy Note 5 and |primary_pointer_type| was passed successfully. BUG=731856 ==========
Description was changed from ========== [Touch Adjustment] Pass primary_pointer_type to WebGestureEvent and disable adjustment for stylus Property Flow: 1. MotionEvent.GetToolType() => 2. GestureEventDetails.primary_pointer_type_ => 3. WebGestureEvent.primary_pointer_type This CL is intended to fix stylus issue on ChromeOS devices. Background: 1. ChromeOS: Stylus generates touch events with large width&height. 2. Android: Stylus generates touch events with 0 width&height. 3. Windows: Stylus generates mouse events. Manually tested on Samsung Galaxy Note 5 and |primary_pointer_type| was passed successfully. BUG=731856 ========== to ========== [Touch Adjustment] Pass primary_pointer_type to WebGestureEvent and disable adjustment for stylus Event Flow: 1. MotionEvent.GetToolType() => 2. GestureEventDetails.primary_pointer_type_ => 3. WebGestureEvent.primary_pointer_type This CL is intended to fix stylus issue on ChromeOS devices. Background: 1. ChromeOS: Stylus generates touch events with large width&height. 2. Android: Stylus generates touch events with 0 width&height. 3. Windows: Stylus generates mouse events. Manually tested on Samsung Galaxy Note 5 and |primary_pointer_type| was passed successfully. BUG=731856 ==========
Description was changed from ========== [Touch Adjustment] Pass primary_pointer_type to WebGestureEvent and disable adjustment for stylus Event Flow: 1. MotionEvent.GetToolType() => 2. GestureEventDetails.primary_pointer_type_ => 3. WebGestureEvent.primary_pointer_type This CL is intended to fix stylus issue on ChromeOS devices. Background: 1. ChromeOS: Stylus generates touch events with large width&height. 2. Android: Stylus generates touch events with 0 width&height. 3. Windows: Stylus generates mouse events. Manually tested on Samsung Galaxy Note 5 and |primary_pointer_type| was passed successfully. BUG=731856 ========== to ========== [Touch Adjustment] Pass primary_pointer_type to WebGestureEvent and disable adjustment for stylus Event Flow: 1. MotionEvent.GetToolType() => 2. GestureEventDetails.primary_pointer_type_ => 3. WebGestureEvent.primary_pointer_type This CL is intended to fix stylus issue on ChromeOS devices. (e.g. Samsung Chromebook Plus) Background: 1. ChromeOS: Stylus generates touch events with large width&height. 2. Android: Stylus generates touch events with 0 width&height. 3. Windows: Stylus generates mouse events. Manually tested on Samsung Galaxy Note 5 and |primary_pointer_type| was passed successfully. BUG=731856 ==========
chongz@chromium.org changed reviewers: + denniskempin@chromium.org, dtapuska@chromium.org
dtapuska@ PTAL, thanks! denniskempin@ Can you take a look if this will on ChromeOS? Or point me to where I can test as I don't have a Samsung Chromebook Plus to load my patch... Thanks! https://codereview.chromium.org/2925883003/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2925883003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:5283: root->SetTargetHandler(view_); For unknown reason |root->target_handler_| is NULL and I have to set it to |view_| to receive events. I'm not sure but is this the expected way of dispatching events?
mustaq@chromium.org changed reviewers: + mustaq@chromium.org
Do we need to cover conversions from non-MotionEvents? Like from ui::GestureEvent, as in RenderWidgetHostViewEventHandler::OnGestureEvent? Also spotted this unused function, please remove: WebViewImpl::HitTestResultForTap. https://codereview.chromium.org/2925883003/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2925883003/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebGestureEvent.h:43: WebPointerProperties::PointerType primary_pointer_type; Please drop "primary_", there is only one type for an event.
Do we need to cover conversions from non-MotionEvents? Like from ui::GestureEvent, as in RenderWidgetHostViewEventHandler::OnGestureEvent? Also spotted this unused function, please remove: WebViewImpl::HitTestResultForTap.
On 2017/06/14 20:37:49, mustaq wrote: > Do we need to cover conversions from non-MotionEvents? Like from > ui::GestureEvent, as in RenderWidgetHostViewEventHandler::OnGestureEvent? > > Also spotted this unused function, please remove: > WebViewImpl::HitTestResultForTap. Are there any layout tests that should be tested?
On 2017/06/14 20:51:35, dtapuska wrote: > On 2017/06/14 20:37:49, mustaq wrote: > > Do we need to cover conversions from non-MotionEvents? Like from > > ui::GestureEvent, as in RenderWidgetHostViewEventHandler::OnGestureEvent? Do you mean we could get |ui::GestureEvent| without going through the |GestureProvider::OnTouchEvent(const MotionEvent& event)| chain? Checking the callers of |ui::GestureEvent::ctor| I find 2 additional call sites: 1. "ash/.../drag_drop_controller.cc": |DispatchGestureEndToWindow()| 2. |EventFactoryEvdev::DispatchPinchEvent()| However these 2 are both simulated events and I think the default "Unknown" pointer type should be fine, no? Otherwise |ui::GestureEvent.details_| should already have |pointer_type| acquired from |GestureProvider|. > > > > Also spotted this unused function, please remove: > > WebViewImpl::HitTestResultForTap. Sure, but for the ease of review can I do it in a separate patch? > > Are there any layout tests that should be tested? It seems that we can only specify "touch", "mouse", "pen" in |pointerActionSequence()|, but not simulating "touch events from stylus". Or you just mean the "stylus-generated tap => no adjustment" part? https://codereview.chromium.org/2925883003/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2925883003/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebGestureEvent.h:43: WebPointerProperties::PointerType primary_pointer_type; On 2017/06/14 20:37:48, mustaq wrote: > Please drop "primary_", there is only one type for an event. dtapuska@ Are you ok with dropping "primary_"? The original reason is we have |primary_tool_type| in |GestureEventData| and Dave suggested adding "primary_".
> Do you mean we could get |ui::GestureEvent| without going through the > |GestureProvider::OnTouchEvent(const MotionEvent& event)| chain? > > Checking the callers of |ui::GestureEvent::ctor| I find 2 additional call sites: > 1. "ash/.../drag_drop_controller.cc": |DispatchGestureEndToWindow()| > 2. |EventFactoryEvdev::DispatchPinchEvent()| > However these 2 are both simulated events and I think the default "Unknown" > pointer type should be fine, no? > > Otherwise |ui::GestureEvent.details_| should already have |pointer_type| > acquired from |GestureProvider|. > Thanks for checking that these are simulated. I am fine as-is. > > > Also spotted this unused function, please remove: > > > WebViewImpl::HitTestResultForTap. > > Sure, but for the ease of review can I do it in a separate patch? > Sure. > > Are there any layout tests that should be tested? > > It seems that we can only specify "touch", "mouse", "pen" in > |pointerActionSequence()|, but not simulating "touch events from stylus". > > Or you just mean the "stylus-generated tap => no adjustment" part? > I agree with Dave we need tests. If |pointerActionSequence| seems hard to extend for TEs with type="pen", at least EventSender::AddTouchPoint could have the hardcoded pointer_type replaced with a |gin::Arguments*| param while preserving the old behavior as default. Please add a new test in touchadjustment/ (Note that the existing touchadjustment/ tests use Internals::touchNodeAdjustedToBestClickableNode to hittest directly w/o any event dispatch so we can't use them.) > https://codereview.chromium.org/2925883003/diff/40001/third_party/WebKit/publ... > File third_party/WebKit/public/platform/WebGestureEvent.h (right): > > https://codereview.chromium.org/2925883003/diff/40001/third_party/WebKit/publ... > third_party/WebKit/public/platform/WebGestureEvent.h:43: > WebPointerProperties::PointerType primary_pointer_type; > On 2017/06/14 20:37:48, mustaq wrote: > > Please drop "primary_", there is only one type for an event. > > dtapuska@ Are you ok with dropping "primary_"? > > The original reason is we have |primary_tool_type| in |GestureEventData| and > Dave suggested adding "primary_". Hmm, |GestureEventData.primary_tool_type| is definitely misleading here, this ultimately gets populated with MotionEvent::getToolType(), so I see no reason to have "primary" there. There is not such thing as "primary tool type" in the MotionEvent spec: https://developer.android.com/reference/android/view/MotionEvent.html Let's not repeat the mistake. I would suggest even adding a TODO to fix |primary_tool_type| later on. Dave, wdyt?
https://codereview.chromium.org/2925883003/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2925883003/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebGestureEvent.h:43: WebPointerProperties::PointerType primary_pointer_type; On 2017/06/14 23:53:23, chongz wrote: > On 2017/06/14 20:37:48, mustaq wrote: > > Please drop "primary_", there is only one type for an event. > > dtapuska@ Are you ok with dropping "primary_"? > > The original reason is we have |primary_tool_type| in |GestureEventData| and > Dave suggested adding "primary_". Probably we should indicate that for a multi-contact gesture this is the pointer type of the *first* point no?
On 2017/06/15 14:47:24, dtapuska wrote: > https://codereview.chromium.org/2925883003/diff/40001/third_party/WebKit/publ... > File third_party/WebKit/public/platform/WebGestureEvent.h (right): > > https://codereview.chromium.org/2925883003/diff/40001/third_party/WebKit/publ... > third_party/WebKit/public/platform/WebGestureEvent.h:43: > WebPointerProperties::PointerType primary_pointer_type; > On 2017/06/14 23:53:23, chongz wrote: > > On 2017/06/14 20:37:48, mustaq wrote: > > > Please drop "primary_", there is only one type for an event. > > > > dtapuska@ Are you ok with dropping "primary_"? > > > > The original reason is we have |primary_tool_type| in |GestureEventData| and > > Dave suggested adding "primary_". > > Probably we should indicate that for a multi-contact gesture this is the pointer > type of the *first* point no? I see, it's a different kind of "primary". Let's keep the "primary" prefix here, and add comments in both gesture_event_data.h and WebGestureEvent.h to explains: "The tool/pointer type for first touch point in the gesture".
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...
On 2017/06/15 14:42:41, mustaq wrote: > > Do you mean we could get |ui::GestureEvent| without going through the > > |GestureProvider::OnTouchEvent(const MotionEvent& event)| chain? > > > > Checking the callers of |ui::GestureEvent::ctor| I find 2 additional call > sites: > > 1. "ash/.../drag_drop_controller.cc": |DispatchGestureEndToWindow()| > > 2. |EventFactoryEvdev::DispatchPinchEvent()| > > However these 2 are both simulated events and I think the default "Unknown" > > pointer type should be fine, no? > > > > Otherwise |ui::GestureEvent.details_| should already have |pointer_type| > > acquired from |GestureProvider|. > > > > Thanks for checking that these are simulated. I am fine as-is. > > > > > Also spotted this unused function, please remove: > > > > WebViewImpl::HitTestResultForTap. > > > > Sure, but for the ease of review can I do it in a separate patch? > > > > Sure. > Filed https://crbug.com/733820. |WebViewImpl::HitTestResultForTap| is still used but we could probably remove |WebView::HitTestResultAt()|. > > > Are there any layout tests that should be tested? > > > > It seems that we can only specify "touch", "mouse", "pen" in > > |pointerActionSequence()|, but not simulating "touch events from stylus". > > > > Or you just mean the "stylus-generated tap => no adjustment" part? > > > > I agree with Dave we need tests. If |pointerActionSequence| seems hard to > extend for TEs with type="pen", at least EventSender::AddTouchPoint could have > the hardcoded pointer_type replaced with a |gin::Arguments*| param while > preserving the old behavior as default. Please add a new test in > touchadjustment/ > > (Note that the existing touchadjustment/ tests use > Internals::touchNodeAdjustedToBestClickableNode to hittest directly w/o any > event dispatch so we can't use them.) > Added layout test for the "stylus-generated tap => no adjustment" part, and the unit test will cover the "TouchEvent => MotionEvent => GestureTap" part. We can't use |pointerActionSequence()| as it doesn't distinguish between mouse-like pen and touch-like pen. https://codereview.chromium.org/2925883003/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2925883003/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebGestureEvent.h:43: WebPointerProperties::PointerType primary_pointer_type; On 2017/06/15 14:53:28, mustaq wrote: > On 2017/06/15 14:47:24, dtapuska wrote: > > > https://codereview.chromium.org/2925883003/diff/40001/third_party/WebKit/publ... > > File third_party/WebKit/public/platform/WebGestureEvent.h (right): > > > > > https://codereview.chromium.org/2925883003/diff/40001/third_party/WebKit/publ... > > third_party/WebKit/public/platform/WebGestureEvent.h:43: > > WebPointerProperties::PointerType primary_pointer_type; > > On 2017/06/14 23:53:23, chongz wrote: > > > On 2017/06/14 20:37:48, mustaq wrote: > > > > Please drop "primary_", there is only one type for an event. > > > > > > dtapuska@ Are you ok with dropping "primary_"? > > > > > > The original reason is we have |primary_tool_type| in |GestureEventData| and > > > Dave suggested adding "primary_". > > > > Probably we should indicate that for a multi-contact gesture this is the > pointer > > type of the *first* point no? > > I see, it's a different kind of "primary". Let's keep the "primary" prefix > here, and add comments in both gesture_event_data.h and WebGestureEvent.h to > explains: "The tool/pointer type for first touch point in the gesture". Updated comments.
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_...)
https://codereview.chromium.org/2925883003/diff/60001/content/shell/test_runn... File content/shell/test_runner/event_sender.cc (right): https://codereview.chromium.org/2925883003/diff/60001/content/shell/test_runn... content/shell/test_runner/event_sender.cc:2528: getPointerType(args, false, event.primary_pointer_type); You should probably check if there args and whether the return result is false as well here.
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...
dtapuska@ PTAL again, thanks! https://codereview.chromium.org/2925883003/diff/60001/content/shell/test_runn... File content/shell/test_runner/event_sender.cc (right): https://codereview.chromium.org/2925883003/diff/60001/content/shell/test_runn... content/shell/test_runner/event_sender.cc:2528: getPointerType(args, false, event.primary_pointer_type); On 2017/06/16 14:49:35, dtapuska wrote: > You should probably check if there args and whether the return result is false > as well here. Done.
https://codereview.chromium.org/2925883003/diff/80001/content/shell/test_runn... File content/shell/test_runner/event_sender.cc (right): https://codereview.chromium.org/2925883003/diff/80001/content/shell/test_runn... content/shell/test_runner/event_sender.cc:2529: if (!args->PeekNext().IsEmpty() && args->PeekNext()->IsString()) { Do you really need the IsString() ? If there are layout tests passing params that they should those should be fixed.
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/2925883003/diff/80001/content/shell/test_runn... File content/shell/test_runner/event_sender.cc (right): https://codereview.chromium.org/2925883003/diff/80001/content/shell/test_runn... content/shell/test_runner/event_sender.cc:2529: if (!args->PeekNext().IsEmpty() && args->PeekNext()->IsString()) { On 2017/06/16 15:46:23, dtapuska wrote: > Do you really need the IsString() ? > > If there are layout tests passing params that they should those should be fixed. Fixed layout tests. In this case I think we don't need |args->PeekNext().IsEmpty()| as |getPointerType()| will do the check and return true if empty. This will match the usage in other places.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dtapuska@ PTAL again, thanks! https://codereview.chromium.org/2925883003/diff/100001/content/shell/test_run... File content/shell/test_runner/event_sender.cc (right): https://codereview.chromium.org/2925883003/diff/100001/content/shell/test_run... content/shell/test_runner/event_sender.cc:2528: if (!getPointerType(args, false, event.primary_pointer_type)) dtapuska@ Are you ok with this style or do you want me to move |args->PeekNext().IsEmpty()| check out of |getPointerType()| for all occurrences?
On 2017/06/19 19:46:04, chongz wrote: > dtapuska@ PTAL again, thanks! > > https://codereview.chromium.org/2925883003/diff/100001/content/shell/test_run... > File content/shell/test_runner/event_sender.cc (right): > > https://codereview.chromium.org/2925883003/diff/100001/content/shell/test_run... > content/shell/test_runner/event_sender.cc:2528: if (!getPointerType(args, false, > event.primary_pointer_type)) > dtapuska@ Are you ok with this style or do you want me to move > |args->PeekNext().IsEmpty()| check out of |getPointerType()| for all > occurrences? lgtm
https://codereview.chromium.org/2925883003/diff/100001/content/shell/test_run... File content/shell/test_runner/event_sender.cc (right): https://codereview.chromium.org/2925883003/diff/100001/content/shell/test_run... content/shell/test_runner/event_sender.cc:2528: if (!getPointerType(args, false, event.primary_pointer_type)) On 2017/06/19 19:46:04, chongz wrote: > dtapuska@ Are you ok with this style or do you want me to move > |args->PeekNext().IsEmpty()| check out of |getPointerType()| for all > occurrences? this style is fine.
https://codereview.chromium.org/2925883003/diff/100001/content/shell/test_run... File content/shell/test_runner/event_sender.cc (right): https://codereview.chromium.org/2925883003/diff/100001/content/shell/test_run... content/shell/test_runner/event_sender.cc:2528: if (!getPointerType(args, false, event.primary_pointer_type)) On 2017/06/23 17:11:11, dtapuska wrote: > On 2017/06/19 19:46:04, chongz wrote: > > dtapuska@ Are you ok with this style or do you want me to move > > |args->PeekNext().IsEmpty()| check out of |getPointerType()| for all > > occurrences? > > this style is fine. Well really whomever added getPointerType should have added it as GetPointerType... to match the rest of the style.
chongz@chromium.org changed reviewers: + sadrul@chromium.org
sadrul@ PTAL at "ui/events/", thanks! https://codereview.chromium.org/2925883003/diff/100001/content/shell/test_run... File content/shell/test_runner/event_sender.cc (right): https://codereview.chromium.org/2925883003/diff/100001/content/shell/test_run... content/shell/test_runner/event_sender.cc:2528: if (!getPointerType(args, false, event.primary_pointer_type)) On 2017/06/23 17:14:37, dtapuska wrote: > On 2017/06/23 17:11:11, dtapuska wrote: > > On 2017/06/19 19:46:04, chongz wrote: > > > dtapuska@ Are you ok with this style or do you want me to move > > > |args->PeekNext().IsEmpty()| check out of |getPointerType()| for all > > > occurrences? > > > > this style is fine. > > Well really whomever added getPointerType should have added it as > GetPointerType... to match the rest of the style. Sure I will change it to |GetPointerType| in next iteration.
lgtm
chongz@chromium.org changed reviewers: + jochen@chromium.org
jochen@ PTAL at "content/" and "third_party/WebKit/", thanks!
lgtm with comment addressed https://codereview.chromium.org/2925883003/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2925883003/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGestureEvent.h:42: WebPointerProperties::PointerType primary_pointer_type; can you assign a default value here?
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/2925883003/diff/100001/content/shell/test_run... File content/shell/test_runner/event_sender.cc (right): https://codereview.chromium.org/2925883003/diff/100001/content/shell/test_run... content/shell/test_runner/event_sender.cc:2528: if (!getPointerType(args, false, event.primary_pointer_type)) On 2017/06/23 17:39:13, chongz wrote: > On 2017/06/23 17:14:37, dtapuska wrote: > > On 2017/06/23 17:11:11, dtapuska wrote: > > > On 2017/06/19 19:46:04, chongz wrote: > > > > dtapuska@ Are you ok with this style or do you want me to move > > > > |args->PeekNext().IsEmpty()| check out of |getPointerType()| for all > > > > occurrences? > > > > > > this style is fine. > > > > Well really whomever added getPointerType should have added it as > > GetPointerType... to match the rest of the style. > > Sure I will change it to |GetPointerType| in next iteration. Done. Changed to |GetPointerType|. https://codereview.chromium.org/2925883003/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2925883003/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGestureEvent.h:42: WebPointerProperties::PointerType primary_pointer_type; On 2017/06/27 09:29:24, jochen wrote: > can you assign a default value here? Done. Added default value for readability, but we shouldn't need it as we have done |memset()| in |WebInputEvent|.
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 jochen@chromium.org, dtapuska@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2925883003/#ps140001 (title: "jochen and dtapuska's comments: Add default value")
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": 140001, "attempt_start_ts": 1498627074118600,
"parent_rev": "41765c0da435e28b914a271fc64dfe2c761fdb5d", "commit_rev":
"d65eacf7288645732c715f588fc3f5d4ae0e0d81"}
Message was sent while issue was closed.
Description was changed from ========== [Touch Adjustment] Pass primary_pointer_type to WebGestureEvent and disable adjustment for stylus Event Flow: 1. MotionEvent.GetToolType() => 2. GestureEventDetails.primary_pointer_type_ => 3. WebGestureEvent.primary_pointer_type This CL is intended to fix stylus issue on ChromeOS devices. (e.g. Samsung Chromebook Plus) Background: 1. ChromeOS: Stylus generates touch events with large width&height. 2. Android: Stylus generates touch events with 0 width&height. 3. Windows: Stylus generates mouse events. Manually tested on Samsung Galaxy Note 5 and |primary_pointer_type| was passed successfully. BUG=731856 ========== to ========== [Touch Adjustment] Pass primary_pointer_type to WebGestureEvent and disable adjustment for stylus Event Flow: 1. MotionEvent.GetToolType() => 2. GestureEventDetails.primary_pointer_type_ => 3. WebGestureEvent.primary_pointer_type This CL is intended to fix stylus issue on ChromeOS devices. (e.g. Samsung Chromebook Plus) Background: 1. ChromeOS: Stylus generates touch events with large width&height. 2. Android: Stylus generates touch events with 0 width&height. 3. Windows: Stylus generates mouse events. Manually tested on Samsung Galaxy Note 5 and |primary_pointer_type| was passed successfully. BUG=731856 Review-Url: https://codereview.chromium.org/2925883003 Cr-Commit-Position: refs/heads/master@{#482892} Committed: https://chromium.googlesource.com/chromium/src/+/d65eacf7288645732c715f588fc3... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/d65eacf7288645732c715f588fc3... |
