Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1993853002:
Set PointerType of pointer event on Windows (Closed)
|
Created:
4 years, 7 months ago by lanwei Modified:
4 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, dtapuska+chromiumwatch_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet PointerType of pointer event on Windows.
Pointer event's PointerType did not set on Windows for mouse type when converting from
ui::MouseEvent to blink::WebMouseEvent.
In WebMouseEventBuilder::Build for Windows, I set the pointer type of WebMouseEvent.
BUG=605639
Committed: https://crrev.com/8a78e7138f7fce8ce8bfe75803bbf8c33877da9a
Cr-Commit-Position: refs/heads/master@{#395999}
Patch Set 1 : Add pointer type to webmouseevent #Patch Set 2 : #
Total comments: 3
Patch Set 3 : Add pointer to webmousewheelevent #
Messages
Total messages: 29 (10 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== mouse win mouse win BUG=605639 ========== to ========== Set PointerType of pointer event on Windows. Pointer event's PointerType did not set on Windows for mouse type when converting from ui::MouseEvent to blink::WebMouseEvent. In WebMouseEventBuilder::Build for windows, I set the pointer type of WebMouseEvent. BUG=605639 ==========
Description was changed from ========== Set PointerType of pointer event on Windows. Pointer event's PointerType did not set on Windows for mouse type when converting from ui::MouseEvent to blink::WebMouseEvent. In WebMouseEventBuilder::Build for windows, I set the pointer type of WebMouseEvent. BUG=605639 ========== to ========== Set PointerType of pointer event on Windows. Pointer event's PointerType did not set on Windows for mouse type when converting from ui::MouseEvent to blink::WebMouseEvent. In WebMouseEventBuilder::Build for Windows, I set the pointer type of WebMouseEvent from MouseEvent. BUG=605639 ==========
lanwei@chromium.org changed reviewers: + mustaq@chromium.org, nzolghadr@chromium.org, tdresser@chromium.org
Description was changed from ========== Set PointerType of pointer event on Windows. Pointer event's PointerType did not set on Windows for mouse type when converting from ui::MouseEvent to blink::WebMouseEvent. In WebMouseEventBuilder::Build for Windows, I set the pointer type of WebMouseEvent from MouseEvent. BUG=605639 ========== to ========== Set PointerType of pointer event on Windows. Pointer event's PointerType did not set on Windows for mouse type when converting from ui::MouseEvent to blink::WebMouseEvent. In WebMouseEventBuilder::Build for Windows, I set the pointer type of WebMouseEvent. BUG=605639 ==========
https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/web_input_event_unittest.cc (right): https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... content/browser/renderer_host/input/web_input_event_unittest.cc:38: blink::WebPointerProperties::PointerType::Mouse); Is there anywhere on Win that we pass Pen pointer type to this API?
On 2016/05/18 20:51:42, Navid Zolghadr wrote: > https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... > File content/browser/renderer_host/input/web_input_event_unittest.cc (right): > > https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... > content/browser/renderer_host/input/web_input_event_unittest.cc:38: > blink::WebPointerProperties::PointerType::Mouse); > Is there anywhere on Win that we pass Pen pointer type to this API? No, I asked Ananata, he told me Windows currently does not support Pen pointer type. I checked that Windows and Linux set Mouse as the pointer type for stylus pen input.
dtapuska@chromium.org changed reviewers: + dtapuska@chromium.org
https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura.cc:206: ? MakeUntranslatedWebMouseEventFromNativeEvent( What about mouse wheel? I think you aren't updating the pointer in that case as well.
On 2016/05/18 21:05:26, dtapuska wrote: > https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... > File content/browser/renderer_host/web_input_event_aura.cc (right): > > https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... > content/browser/renderer_host/web_input_event_aura.cc:206: ? > MakeUntranslatedWebMouseEventFromNativeEvent( > What about mouse wheel? I think you aren't updating the pointer in that case as > well. Mustaq told me that mouse wheel is not a type of pointer event, we do not need to set the pointer type for it.
On 2016/05/19 02:52:11, lanwei wrote: > On 2016/05/18 21:05:26, dtapuska wrote: > > > https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... > > File content/browser/renderer_host/web_input_event_aura.cc (right): > > > > > https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... > > content/browser/renderer_host/web_input_event_aura.cc:206: ? > > MakeUntranslatedWebMouseEventFromNativeEvent( > > What about mouse wheel? I think you aren't updating the pointer in that case > as > > well. > > Mustaq told me that mouse wheel is not a type of pointer event, we do not need > to set the pointer type for it. But it is odd that it is set via the non windows case correctly.
On 2016/05/19 02:57:49, dtapuska wrote: > On 2016/05/19 02:52:11, lanwei wrote: > > On 2016/05/18 21:05:26, dtapuska wrote: > > > > > > https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... > > > File content/browser/renderer_host/web_input_event_aura.cc (right): > > > > > > > > > https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... > > > content/browser/renderer_host/web_input_event_aura.cc:206: ? > > > MakeUntranslatedWebMouseEventFromNativeEvent( > > > What about mouse wheel? I think you aren't updating the pointer in that case > > as > > > well. > > > > Mustaq told me that mouse wheel is not a type of pointer event, we do not need > > to set the pointer type for it. > > But it is odd that it is set via the non windows case correctly. We are just not sending any pointer events for wheel events in EventHandler: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... So I guess event setting it in other cases doesn't have any effect at the moment.
On 2016/05/19 03:00:36, Navid Zolghadr wrote: > On 2016/05/19 02:57:49, dtapuska wrote: > > On 2016/05/19 02:52:11, lanwei wrote: > > > On 2016/05/18 21:05:26, dtapuska wrote: > > > > > > > > > > https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... > > > > File content/browser/renderer_host/web_input_event_aura.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... > > > > content/browser/renderer_host/web_input_event_aura.cc:206: ? > > > > MakeUntranslatedWebMouseEventFromNativeEvent( > > > > What about mouse wheel? I think you aren't updating the pointer in that > case > > > as > > > > well. > > > > > > Mustaq told me that mouse wheel is not a type of pointer event, we do not > need > > > to set the pointer type for it. > > > > But it is odd that it is set via the non windows case correctly. > > We are just not sending any pointer events for wheel events in EventHandler: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > So I guess event setting it in other cases doesn't have any effect at the > moment. You are right. Both WheelEvent & PointerEvent are direct "subclasses" of MouseEvent, so pointerType is irrelevant for WheelEvent even though our internal data types for WheelEvent carry pointerType info. https://www.w3.org/TR/DOM-Level-3-Events/#interface-WheelEvent
https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura.cc:209: event.pointer_details().pointer_type)) To make sure that we have covered the event path from events_win.cc to web_input_event_builders_win.cc, could you please do a quick test to confirm that any hardcoded pointerType in ui::GetMousePointerDetailsFromNative flows all the way to JS? https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/win/even...
On 2016/05/19 14:21:16, mustaq wrote: > https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... > File content/browser/renderer_host/web_input_event_aura.cc (right): > > https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... > content/browser/renderer_host/web_input_event_aura.cc:209: > event.pointer_details().pointer_type)) > To make sure that we have covered the event path from events_win.cc to > web_input_event_builders_win.cc, could you please do a quick test to confirm > that any hardcoded pointerType in ui::GetMousePointerDetailsFromNative flows all > the way to JS? > https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/win/even... For completeness I think we should populate the wheel events very similarly just to avoid confusion in the future.
Patchset #3 (id:60001) has been deleted
On 2016/05/19 14:21:16, mustaq wrote: > https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... > File content/browser/renderer_host/web_input_event_aura.cc (right): > > https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... > content/browser/renderer_host/web_input_event_aura.cc:209: > event.pointer_details().pointer_type)) > To make sure that we have covered the event path from events_win.cc to > web_input_event_builders_win.cc, could you please do a quick test to confirm > that any hardcoded pointerType in ui::GetMousePointerDetailsFromNative flows all > the way to JS? > https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/win/even... I checked the pointer type for the mouse event used in JS are only set in ui::GetMousePointerDetailsFromNative in ui/events/win/events_win.cc for Windows.
On 2016/05/20 20:46:31, lanwei wrote: > On 2016/05/19 14:21:16, mustaq wrote: > > > https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... > > File content/browser/renderer_host/web_input_event_aura.cc (right): > > > > > https://codereview.chromium.org/1993853002/diff/40001/content/browser/rendere... > > content/browser/renderer_host/web_input_event_aura.cc:209: > > event.pointer_details().pointer_type)) > > To make sure that we have covered the event path from events_win.cc to > > web_input_event_builders_win.cc, could you please do a quick test to confirm > > that any hardcoded pointerType in ui::GetMousePointerDetailsFromNative flows > all > > the way to JS? > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/win/even... > > I checked the pointer type for the mouse event used in JS are only set in > ui::GetMousePointerDetailsFromNative in ui/events/win/events_win.cc for Windows. Thanks for confirming that chromium-side plumbing is complete after ui/events layer. Please add a TODO in ui::GetMousePointerDetailsFromNative in events_win.cc pointing to a new bug, and cc sadrul@ to the bug, to find the right person to fix it. LGTM otherwise.
LGTM
lgtm
LGTM
The CQ bit was checked by lanwei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993853002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993853002/80001
Message was sent while issue was closed.
Description was changed from ========== Set PointerType of pointer event on Windows. Pointer event's PointerType did not set on Windows for mouse type when converting from ui::MouseEvent to blink::WebMouseEvent. In WebMouseEventBuilder::Build for Windows, I set the pointer type of WebMouseEvent. BUG=605639 ========== to ========== Set PointerType of pointer event on Windows. Pointer event's PointerType did not set on Windows for mouse type when converting from ui::MouseEvent to blink::WebMouseEvent. In WebMouseEventBuilder::Build for Windows, I set the pointer type of WebMouseEvent. BUG=605639 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Set PointerType of pointer event on Windows. Pointer event's PointerType did not set on Windows for mouse type when converting from ui::MouseEvent to blink::WebMouseEvent. In WebMouseEventBuilder::Build for Windows, I set the pointer type of WebMouseEvent. BUG=605639 ========== to ========== Set PointerType of pointer event on Windows. Pointer event's PointerType did not set on Windows for mouse type when converting from ui::MouseEvent to blink::WebMouseEvent. In WebMouseEventBuilder::Build for Windows, I set the pointer type of WebMouseEvent. BUG=605639 Committed: https://crrev.com/8a78e7138f7fce8ce8bfe75803bbf8c33877da9a Cr-Commit-Position: refs/heads/master@{#395999} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8a78e7138f7fce8ce8bfe75803bbf8c33877da9a Cr-Commit-Position: refs/heads/master@{#395999} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
