Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1440923002:
Convert the coordinates in WebInputEvent To Viewport in InputRouter (Closed)
DescriptionConvert the coordinates in WebInputEvent To Viewport in InputRouter before sending to renderer.
BUG=485650
TEST=InputRouterImlpScaleEventTest*, WebInputEventUtilTest.NoScalingWith1DSF
Committed: https://crrev.com/5692e28fe64601e69927d7bc4e70e4f9e3295d69
Cr-Commit-Position: refs/heads/master@{#362729}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : #
Total comments: 11
Patch Set 3 : #Messages
Total messages: 63 (36 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440923002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440923002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440923002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440923002/60001
The CQ bit was unchecked by oshima@chromium.org
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440923002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440923002/100001
Description was changed from ========== Convert Input To Viewport ========== to ========== Convert Input To Viewport in InputRouter ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rbyers@chromium.org changed reviewers: + rbyers@chromium.org
https://codereview.chromium.org/1440923002/diff/100001/content/browser/render... File content/browser/renderer_host/input/web_input_event_util.cc (right): https://codereview.chromium.org/1440923002/diff/100001/content/browser/render... content/browser/renderer_host/input/web_input_event_util.cc:80: return scaled_event; this is uninitialized, did you mean 'event'? https://codereview.chromium.org/1440923002/diff/100001/content/browser/render... content/browser/renderer_host/input/web_input_event_util.cc:126: // why isn't pinch scale scaled?
Thanks for early comments. This needs cleanup/comments and test. I'll update the code once we agree on how/where to do this conversion. https://codereview.chromium.org/1440923002/diff/100001/content/browser/render... File content/browser/renderer_host/input/web_input_event_util.cc (right): https://codereview.chromium.org/1440923002/diff/100001/content/browser/render... content/browser/renderer_host/input/web_input_event_util.cc:80: return scaled_event; On 2015/11/12 19:45:36, Rick Byers (slow until Nov 23) wrote: > this is uninitialized, did you mean 'event'? nullptr is used as a way to indicate that conversion didn't happen. I can't use event as it requires ownership transfer. https://codereview.chromium.org/1440923002/diff/100001/content/browser/render... content/browser/renderer_host/input/web_input_event_util.cc:126: // On 2015/11/12 19:45:36, Rick Byers (slow until Nov 23) wrote: > why isn't pinch scale scaled? pinch only has scale, which is agnostic to dsf. I tested on pixel and the pinch zoom behavior was same.
Patchset #3 (id:140001) has been deleted
Patchset #2 (id:120001) has been deleted
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440923002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440923002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #4 (id:200001) has been deleted
Patchset #2 (id:160001) has been deleted
Patchset #3 (id:220001) has been deleted
Patchset #2 (id:180001) has been deleted
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440923002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440923002/240001
Description was changed from ========== Convert Input To Viewport in InputRouter ========== to ========== Convert Input To Viewport in InputRouter BUG=485650 TEST=InputRouterImlpScaleEventTest*, WebInputEventUtilTest.NoScalingWith1DSF ==========
oshima@chromium.org changed reviewers: + aelias@chromium.org
Description was changed from ========== Convert Input To Viewport in InputRouter BUG=485650 TEST=InputRouterImlpScaleEventTest*, WebInputEventUtilTest.NoScalingWith1DSF ========== to ========== Convert the coordinates in WebInputEvent To Viewport in InputRouter before sending to renderer. BUG=485650 TEST=InputRouterImlpScaleEventTest*, WebInputEventUtilTest.NoScalingWith1DSF ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) ios_rel_device_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
This is ready for review. PTAL.
rbyers@chromium.org changed reviewers: + tdresser@chromium.org
The basic approach seems OK to me (given our discussion of the long term direction we want to head). Tim is an OWNER in most of these directories so I'll leave the detailed review to him. https://codereview.chromium.org/1440923002/diff/240001/content/browser/render... File content/browser/renderer_host/input/web_input_event_util.cc (right): https://codereview.chromium.org/1440923002/diff/240001/content/browser/render... content/browser/renderer_host/input/web_input_event_util.cc:74: scoped_ptr<blink::WebInputEvent> ConvertWebInputEventToViewport( nit: please add a TODO comment saying this mixing co-ordinate spaces in WebInputEvent is temporary and a link to your doc for the long-term plan.
Other than the concern with wheelTicks, LGTM with nits. https://codereview.chromium.org/1440923002/diff/240001/content/browser/render... File content/browser/renderer_host/input/input_router_impl_unittest.cc (right): https://codereview.chromium.org/1440923002/diff/240001/content/browser/render... content/browser/renderer_host/input/input_router_impl_unittest.cc:1911: void RunTouchEventTest(const std::string& name, WebTouchPoint::State state) { Add a comment indicating the setup required to use RunTouchEventTest. https://codereview.chromium.org/1440923002/diff/240001/content/browser/render... content/browser/renderer_host/input/input_router_impl_unittest.cc:2024: auto event = SyntheticWebGestureEventBuilder::Build( Some of the use of auto here seems a bit suspect to me. There are several cases where it isn't completely obvious what the inferred event type is. https://codereview.chromium.org/1440923002/diff/240001/content/browser/render... content/browser/renderer_host/input/input_router_impl_unittest.cc:2076: void TestLocationInSentEvent(const WebGestureEvent* sent_event) { Could we take the co-ordinates here as parameters? I think it would make the tests a bit easier to read. Currently if I want to view the expectations, I have to find the definition of this function. https://codereview.chromium.org/1440923002/diff/240001/content/browser/render... File content/browser/renderer_host/input/web_input_event_util.cc (right): https://codereview.chromium.org/1440923002/diff/240001/content/browser/render... content/browser/renderer_host/input/web_input_event_util.cc:83: *wheel_event = static_cast<const blink::WebMouseWheelEvent&>(event); Isn't there some way to avoid allocating an extra event? Something like: blink::WebMouseWheelEvent* wheel_event = static_cast<const blink::WebMouseWheelEvent*>(&event); scaled_event.reset(wheel_event); https://codereview.chromium.org/1440923002/diff/240001/content/browser/render... content/browser/renderer_host/input/web_input_event_util.cc:87: wheel_event->wheelTicksY *= scale; Is this correct, or should we recompute wheelTicks here? https://code.google.com/p/chromium/codesearch#chromium/src/components/html_vi...
https://codereview.chromium.org/1440923002/diff/240001/content/browser/render... File content/browser/renderer_host/input/input_router_impl_unittest.cc (right): https://codereview.chromium.org/1440923002/diff/240001/content/browser/render... content/browser/renderer_host/input/input_router_impl_unittest.cc:1911: void RunTouchEventTest(const std::string& name, WebTouchPoint::State state) { On 2015/11/27 21:35:20, tdresser wrote: > Add a comment indicating the setup required to use RunTouchEventTest. Done. https://codereview.chromium.org/1440923002/diff/240001/content/browser/render... content/browser/renderer_host/input/input_router_impl_unittest.cc:2024: auto event = SyntheticWebGestureEventBuilder::Build( On 2015/11/27 21:35:20, tdresser wrote: > Some of the use of auto here seems a bit suspect to me. There are several cases > where it isn't completely obvious what the inferred event type is. Done. https://codereview.chromium.org/1440923002/diff/240001/content/browser/render... content/browser/renderer_host/input/input_router_impl_unittest.cc:2076: void TestLocationInSentEvent(const WebGestureEvent* sent_event) { On 2015/11/27 21:35:20, tdresser wrote: > Could we take the co-ordinates here as parameters? I think it would make the > tests a bit easier to read. > > Currently if I want to view the expectations, I have to find the definition of > this function. Done. https://codereview.chromium.org/1440923002/diff/240001/content/browser/render... File content/browser/renderer_host/input/web_input_event_util.cc (right): https://codereview.chromium.org/1440923002/diff/240001/content/browser/render... content/browser/renderer_host/input/web_input_event_util.cc:83: *wheel_event = static_cast<const blink::WebMouseWheelEvent&>(event); On 2015/11/27 21:35:20, tdresser wrote: > Isn't there some way to avoid allocating an extra event? > Something like: > > blink::WebMouseWheelEvent* wheel_event = static_cast<const I thought it's risky to modify const object, which is a member of another (message) object, which may be used in different context. I'm happy to change if this is safe, so please let me know. > blink::WebMouseWheelEvent*>(&event); > scaled_event.reset(wheel_event); If it's ok to modify the const object, I can (and should) just return the pointer, as the ownership cannot be transfered. https://codereview.chromium.org/1440923002/diff/240001/content/browser/render... content/browser/renderer_host/input/web_input_event_util.cc:87: wheel_event->wheelTicksY *= scale; On 2015/11/27 21:35:20, tdresser wrote: > Is this correct, or should we recompute wheelTicks here? > > https://code.google.com/p/chromium/codesearch#chromium/src/components/html_vi... If delta X needs to be scaled, so this needs to be scaled too, and it looks to me that delta X needs to be scaled because the content has 2x size.
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440923002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440923002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
aelias@, do you have any comments on this? Otherwise, I'll go ahead and land this.
On 2015/12/01 20:31:58, oshima wrote: > aelias@, do you have any comments on this? Otherwise, I'll go ahead and land > this. LGTM, thanks for the clarification.
lgtm
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440923002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440923002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440923002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440923002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440923002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440923002/260001
Message was sent while issue was closed.
Description was changed from ========== Convert the coordinates in WebInputEvent To Viewport in InputRouter before sending to renderer. BUG=485650 TEST=InputRouterImlpScaleEventTest*, WebInputEventUtilTest.NoScalingWith1DSF ========== to ========== Convert the coordinates in WebInputEvent To Viewport in InputRouter before sending to renderer. BUG=485650 TEST=InputRouterImlpScaleEventTest*, WebInputEventUtilTest.NoScalingWith1DSF ==========
Message was sent while issue was closed.
Committed patchset #3 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Convert the coordinates in WebInputEvent To Viewport in InputRouter before sending to renderer. BUG=485650 TEST=InputRouterImlpScaleEventTest*, WebInputEventUtilTest.NoScalingWith1DSF ========== to ========== Convert the coordinates in WebInputEvent To Viewport in InputRouter before sending to renderer. BUG=485650 TEST=InputRouterImlpScaleEventTest*, WebInputEventUtilTest.NoScalingWith1DSF Committed: https://crrev.com/5692e28fe64601e69927d7bc4e70e4f9e3295d69 Cr-Commit-Position: refs/heads/master@{#362729} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5692e28fe64601e69927d7bc4e70e4f9e3295d69 Cr-Commit-Position: refs/heads/master@{#362729} |
