Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2034213002:
Reland: Fix touchpad gesture routing to renderers (Closed)
|
Created:
4 years, 6 months ago by mohsen Modified:
4 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReland: Fix touchpad gesture routing to renderers
With the addition for touchpad pinch zoom, we will have gesture events
with touchpad as the source device. RenderWidgetHostInputEventRouter
should handle these types of gestures properly.
Also, handling touchpad fling events that are recently switched to use
RenderWidgetHostInputEventRouter path.
BUG=410580
Committed: https://crrev.com/c6cf0184697c9b473b78cdc0e44490a73da95486
Committed: https://crrev.com/298aa38b91c86e1540aeb2a1c119c9040715037c
Cr-Original-Commit-Position: refs/heads/master@{#401723}
Cr-Commit-Position: refs/heads/master@{#403016}
Patch Set 1 #Patch Set 2 : Cleaned up comments #
Total comments: 6
Patch Set 3 : Fixed DCHECK's #
Total comments: 11
Patch Set 4 : Addressed review comments #
Total comments: 1
Patch Set 5 : Got rid of delta checks in test #
Total comments: 4
Patch Set 6 : Rebased #Patch Set 7 : Handled touchpad flings #Patch Set 8 : Updated test to include fling #Patch Set 9 : Fixed Windows failure #
Total comments: 3
Messages
Total messages: 82 (30 generated)
mohsen@chromium.org changed reviewers: + sadrul@chromium.org
sadrul@: Can you please take a look...
sadrul@chromium.org changed reviewers: + tdresser@chromium.org
+tdresser@ It doesn't look like the target for touchpad-wheel events are ever 'locked'. Is there a reason to do that for touchpad-pinch events, then? https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:287: DCHECK(event->sourceDevice == blink::WebGestureDeviceTouchscreen); DCHECK_EQ https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:324: DCHECK(event->sourceDevice == blink::WebGestureDeviceTouchpad); _EQ https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.h (right): https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.h:130: gfx::Vector2d touchpad_gesture_delta_; You probably won't have both touchpad and touchscreen gestures at the same time, so maybe just have the old fileds, with a separate 'gesture source' field that can be either touchscreen or touchpad?
On 2016/06/06 15:21:47, sadrul wrote: > +tdresser@ > > It doesn't look like the target for touchpad-wheel events are ever 'locked'. Is > there a reason to do that for touchpad-pinch events, then? > > https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_input_event_router.cc > (right): > > https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_input_event_router.cc:287: > DCHECK(event->sourceDevice == blink::WebGestureDeviceTouchscreen); > DCHECK_EQ > > https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_input_event_router.cc:324: > DCHECK(event->sourceDevice == blink::WebGestureDeviceTouchpad); > _EQ > > https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_input_event_router.h > (right): > > https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_input_event_router.h:130: > gfx::Vector2d touchpad_gesture_delta_; > You probably won't have both touchpad and touchscreen gestures at the same time, > so maybe just have the old fileds, with a separate 'gesture source' field that > can be either touchscreen or touchpad? See http://crbug.com/526463. We do want touchpad scrolling to latch.
On 2016/06/06 15:23:20, tdresser wrote: > On 2016/06/06 15:21:47, sadrul wrote: > > +tdresser@ > > > > It doesn't look like the target for touchpad-wheel events are ever 'locked'. > Is > > there a reason to do that for touchpad-pinch events, then? > > > > > https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... > > File content/browser/renderer_host/render_widget_host_input_event_router.cc > > (right): > > > > > https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... > > content/browser/renderer_host/render_widget_host_input_event_router.cc:287: > > DCHECK(event->sourceDevice == blink::WebGestureDeviceTouchscreen); > > DCHECK_EQ > > > > > https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... > > content/browser/renderer_host/render_widget_host_input_event_router.cc:324: > > DCHECK(event->sourceDevice == blink::WebGestureDeviceTouchpad); > > _EQ > > > > > https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... > > File content/browser/renderer_host/render_widget_host_input_event_router.h > > (right): > > > > > https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... > > content/browser/renderer_host/render_widget_host_input_event_router.h:130: > > gfx::Vector2d touchpad_gesture_delta_; > > You probably won't have both touchpad and touchscreen gestures at the same > time, > > so maybe just have the old fileds, with a separate 'gesture source' field that > > can be either touchscreen or touchpad? > > See http://crbug.com/526463. > > We do want touchpad scrolling to latch. Oh, I see. Interesting! (mus would ideally have the same behaviour)
On 2016/06/06 15:24:53, sadrul wrote: > On 2016/06/06 15:23:20, tdresser wrote: > > On 2016/06/06 15:21:47, sadrul wrote: > > > +tdresser@ > > > > > > It doesn't look like the target for touchpad-wheel events are ever 'locked'. > > Is > > > there a reason to do that for touchpad-pinch events, then? > > > > > > > > > https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... > > > File content/browser/renderer_host/render_widget_host_input_event_router.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... > > > content/browser/renderer_host/render_widget_host_input_event_router.cc:287: > > > DCHECK(event->sourceDevice == blink::WebGestureDeviceTouchscreen); > > > DCHECK_EQ > > > > > > > > > https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... > > > content/browser/renderer_host/render_widget_host_input_event_router.cc:324: > > > DCHECK(event->sourceDevice == blink::WebGestureDeviceTouchpad); > > > _EQ > > > > > > > > > https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... > > > File content/browser/renderer_host/render_widget_host_input_event_router.h > > > (right): > > > > > > > > > https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... > > > content/browser/renderer_host/render_widget_host_input_event_router.h:130: > > > gfx::Vector2d touchpad_gesture_delta_; > > > You probably won't have both touchpad and touchscreen gestures at the same > > time, > > > so maybe just have the old fileds, with a separate 'gesture source' field > that > > > can be either touchscreen or touchpad? > > > > See http://crbug.com/526463. > > > > We do want touchpad scrolling to latch. > > Oh, I see. Interesting! (mus would ideally have the same behaviour) Yup, absolutely, sorry I'd forgotten there would be work required here.
https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:287: DCHECK(event->sourceDevice == blink::WebGestureDeviceTouchscreen); On 2016/06/06 at 15:21:47, sadrul wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:324: DCHECK(event->sourceDevice == blink::WebGestureDeviceTouchpad); On 2016/06/06 at 15:21:47, sadrul wrote: > _EQ Done. https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.h (right): https://codereview.chromium.org/2034213002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.h:130: gfx::Vector2d touchpad_gesture_delta_; On 2016/06/06 at 15:21:47, sadrul wrote: > You probably won't have both touchpad and touchscreen gestures at the same time, so maybe just have the old fileds, with a separate 'gesture source' field that can be either touchscreen or touchpad? I don't think it is impossible to have both gesture types at the same time (e.g. user starting a touchscreen scroll while doing a touchpad pinch zoom). In this case should the new gesture sequence override the existing one? With my CL these two can happen simultaneously.
A few comments and nits. Otherwise lgtm. Please wait for a review from tdresser@ too https://codereview.chromium.org/2034213002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2034213002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:334: touchpad_gesture_delta_ = transformed_point - original_point; Add a TODO here like above for TouchStart that computing just the delta isn't going to be sufficient in some cases. https://codereview.chromium.org/2034213002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.h (right): https://codereview.chromium.org/2034213002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.h:131: int active_touches_; Would it make sense to group these variables? e.g. struct { RWHVBase* target; gfx::Vector2d delta; } touch_, touchscreen_gesture_, touchpad_gesture_; https://codereview.chromium.org/2034213002/diff/40001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2034213002/diff/40001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:5044: gfx::Point gesture_point, const& https://codereview.chromium.org/2034213002/diff/40001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:5068: EXPECT_EQ(expected_target, router_touchpad_gesture_target); Is it possible to also test the location of the event?
LGTM with nits. https://codereview.chromium.org/2034213002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2034213002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:56: if (view == touchscreen_gesture_target_) { We should be able to clean this up once we switch to storing this data in a struct. https://codereview.chromium.org/2034213002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.h (right): https://codereview.chromium.org/2034213002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.h:131: int active_touches_; On 2016/06/08 15:56:30, sadrul wrote: > Would it make sense to group these variables? e.g. > > struct { > RWHVBase* target; > gfx::Vector2d delta; > } touch_, touchscreen_gesture_, touchpad_gesture_; +1 We should perhaps rename the TouchscreenGestureTargetData struct to TargetData, and instantiate it 3 times here?
https://codereview.chromium.org/2034213002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2034213002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:56: if (view == touchscreen_gesture_target_) { On 2016/06/08 at 17:41:12, tdresser wrote: > We should be able to clean this up once we switch to storing this data in a struct. What sort of cleanup do you have in mind? I opted for just setting |target| to nullptr and leaving |delta| as is since |delta| is always initialized whenever |target| is initialized and its usages are guarded by |target| not being nullptr. Or, do you want a full-blown class implementation for TargetData with getter/setters and a reset() function? https://codereview.chromium.org/2034213002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:334: touchpad_gesture_delta_ = transformed_point - original_point; On 2016/06/08 at 15:56:30, sadrul wrote: > Add a TODO here like above for TouchStart that computing just the delta isn't going to be sufficient in some cases. Done. https://codereview.chromium.org/2034213002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.h (right): https://codereview.chromium.org/2034213002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.h:131: int active_touches_; On 2016/06/08 at 17:41:12, tdresser wrote: > On 2016/06/08 15:56:30, sadrul wrote: > > Would it make sense to group these variables? e.g. > > > > struct { > > RWHVBase* target; > > gfx::Vector2d delta; > > } touch_, touchscreen_gesture_, touchpad_gesture_; > > +1 > > We should perhaps rename the TouchscreenGestureTargetData struct to TargetData, and instantiate it 3 times here? Cool. That makes it much cleaner. Done. https://codereview.chromium.org/2034213002/diff/40001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2034213002/diff/40001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:5044: gfx::Point gesture_point, On 2016/06/08 at 15:56:30, sadrul wrote: > const& Done. (here and another place that I copied this code form) https://codereview.chromium.org/2034213002/diff/40001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:5068: EXPECT_EQ(expected_target, router_touchpad_gesture_target); On 2016/06/08 at 15:56:30, sadrul wrote: > Is it possible to also test the location of the event? Do you mean the delta as in |RWHIER::TargetData::delta|? If yes, I've added checks for it here. Does it look good? If yes, I can add them to SendGestureTapSequenceWithExpectedTarget() and SetTouchTapWithExpectedTarget(), too.
https://codereview.chromium.org/2034213002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2034213002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:42: if (view == touch_target_.target) { I was envisioning getting a pointer to the appropriate TargetData, and then resetting all of it's fields. This is probably fine though. What I was thinking: TargetData* target_data; switch(view) { case touch_target_.target: target_data = &touch_target; break; ... } target_data.target = nullptr; target_data.delta = gfx::Vector2d();
sadrul@: I got rid of delta checks I'd added in the previous patch set and added TODOs to add tests for event location. Does it look good?
On 2016/06/13 18:11:19, mohsen wrote: > sadrul@: I got rid of delta checks I'd added in the previous patch set and added > TODOs to add tests for event location. Does it look good? Apart from the weirdness in the test code (comment below), which seems to already exist in current code, yeah, looks good. https://codereview.chromium.org/2034213002/diff/80001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2034213002/diff/80001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:5045: RenderWidgetHostViewBase*& router_touchpad_gesture_target, This is weird.
The CQ bit was checked by mohsen@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/2034213002/80001
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 mohsen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2034213002/#ps80001 (title: "Got rid of delta checks in test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2034213002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mohsen@chromium.org changed reviewers: + jam@chromium.org
jam@: Can you please take an OWNERS look at content/browser/site_per_process_browsertest.cc?
On 2016/06/14 03:06:19, mohsen wrote: > jam@: Can you please take an OWNERS look at > content/browser/site_per_process_browsertest.cc? please reroute to creis or nasko
mohsen@chromium.org changed reviewers: + nasko@chromium.org - jam@chromium.org
jam@: Sure, I had added you because nasko@ and creis@ were marked as very slow / OOO. nasko@: Please take a look...
mohsen@chromium.org changed reviewers: + creis@chromium.org - nasko@chromium.org
creis@: nasko@ is still OOO. Can you please take a look?
[+site-isolation-reviews] Thanks for adding the OOPIF test! Mostly deferring to sadrul and tdresser, but LGTM. https://codereview.chromium.org/2034213002/diff/80001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2034213002/diff/80001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:4838: #if defined(USE_AURA) Wow, this whole Aura-only section of the tests is confusing in a file this large. It should really move to a separate test file. Not necessary for this CL, I suppose.
The CQ bit was checked by mohsen@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/2034213002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2034213002/diff/80001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2034213002/diff/80001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:4838: #if defined(USE_AURA) On 2016/06/23 at 00:07:40, Charlie Reis wrote: > Wow, this whole Aura-only section of the tests is confusing in a file this large. It should really move to a separate test file. Not necessary for this CL, I suppose. Acknowledged. https://codereview.chromium.org/2034213002/diff/80001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:5045: RenderWidgetHostViewBase*& router_touchpad_gesture_target, On 2016/06/13 at 18:14:59, sadrul wrote: > This is weird. Acknowledged.
The CQ bit was checked by mohsen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2034213002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix touchpad gesture routing to renderers With the addition for touchpad pinch zoom, we will have gesture events with touchpad as the source device. RenderWidgetHostInputEventRouter should handle these types of gestures properly. BUG=410580 ========== to ========== Fix touchpad gesture routing to renderers With the addition for touchpad pinch zoom, we will have gesture events with touchpad as the source device. RenderWidgetHostInputEventRouter should handle these types of gestures properly. BUG=410580 Committed: https://crrev.com/c6cf0184697c9b473b78cdc0e44490a73da95486 Cr-Commit-Position: refs/heads/master@{#401723} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c6cf0184697c9b473b78cdc0e44490a73da95486 Cr-Commit-Position: refs/heads/master@{#401723}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2096923002/ by dgozman@chromium.org. The reason for reverting is: Looks like this broke SitePerProcessBrowserTest.ScrollEventToOOPIF: https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2....
Message was sent while issue was closed.
kenrb@chromium.org changed reviewers: + kenrb@chromium.org
Message was sent while issue was closed.
The test that was broken by this CL had been landed only a few hours earlier, yesterday. RenderWidgetHostViewAura::OnScrollEvent() sends a fling cancel when it gets a trackpad scroll, before sending the wheel event, which appears to break some gesture event assumptions built into this CL.
The CQ bit was checked by mohsen@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...
Description was changed from ========== Fix touchpad gesture routing to renderers With the addition for touchpad pinch zoom, we will have gesture events with touchpad as the source device. RenderWidgetHostInputEventRouter should handle these types of gestures properly. BUG=410580 Committed: https://crrev.com/c6cf0184697c9b473b78cdc0e44490a73da95486 Cr-Commit-Position: refs/heads/master@{#401723} ========== to ========== Fix touchpad gesture routing to renderers With the addition for touchpad pinch zoom, we will have gesture events with touchpad as the source device. RenderWidgetHostInputEventRouter should handle these types of gestures properly. Also, handling touchpad fling events that are recently switched to use RenderWidgetHostInputEventRouter path. BUG=410580 Committed: https://crrev.com/c6cf0184697c9b473b78cdc0e44490a73da95486 Cr-Commit-Position: refs/heads/master@{#401723} ==========
sadrul@: I talked with kenrb@ and we decided to update my patch to handle touchpad flings, too. Please take a look and see if the changes look good. Otherwise, we probably need to revert kenrb@'s patch (https://crrev.com/2052353002) which broke touchpad fling and then try to fix the issue properly.
The CQ bit was checked by mohsen@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_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2016/06/24 20:08:51, mohsen wrote: > sadrul@: I talked with kenrb@ and we decided to update my patch to handle > touchpad flings, too. Please take a look and see if the changes look good. lgtm > Otherwise, we probably need to revert kenrb@'s patch > (https://crrev.com/2052353002) which broke touchpad fling and then try to fix > the issue properly.
Patchset #9 (id:160001) has been deleted
The CQ bit was checked by mohsen@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: This issue passed the CQ dry run.
The CQ bit was checked by mohsen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, tdresser@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2034213002/#ps180001 (title: "Fixed Windows failure")
The CQ bit was unchecked by mohsen@chromium.org
Description was changed from ========== Fix touchpad gesture routing to renderers With the addition for touchpad pinch zoom, we will have gesture events with touchpad as the source device. RenderWidgetHostInputEventRouter should handle these types of gestures properly. Also, handling touchpad fling events that are recently switched to use RenderWidgetHostInputEventRouter path. BUG=410580 Committed: https://crrev.com/c6cf0184697c9b473b78cdc0e44490a73da95486 Cr-Commit-Position: refs/heads/master@{#401723} ========== to ========== Reland: Fix touchpad gesture routing to renderers With the addition for touchpad pinch zoom, we will have gesture events with touchpad as the source device. RenderWidgetHostInputEventRouter should handle these types of gestures properly. Also, handling touchpad fling events that are recently switched to use RenderWidgetHostInputEventRouter path. BUG=410580 Committed: https://crrev.com/c6cf0184697c9b473b78cdc0e44490a73da95486 Cr-Commit-Position: refs/heads/master@{#401723} ==========
The CQ bit was checked by mohsen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Reland: Fix touchpad gesture routing to renderers With the addition for touchpad pinch zoom, we will have gesture events with touchpad as the source device. RenderWidgetHostInputEventRouter should handle these types of gestures properly. Also, handling touchpad fling events that are recently switched to use RenderWidgetHostInputEventRouter path. BUG=410580 Committed: https://crrev.com/c6cf0184697c9b473b78cdc0e44490a73da95486 Cr-Commit-Position: refs/heads/master@{#401723} ========== to ========== Reland: Fix touchpad gesture routing to renderers With the addition for touchpad pinch zoom, we will have gesture events with touchpad as the source device. RenderWidgetHostInputEventRouter should handle these types of gestures properly. Also, handling touchpad fling events that are recently switched to use RenderWidgetHostInputEventRouter path. BUG=410580 Committed: https://crrev.com/c6cf0184697c9b473b78cdc0e44490a73da95486 Committed: https://crrev.com/298aa38b91c86e1540aeb2a1c119c9040715037c Cr-Original-Commit-Position: refs/heads/master@{#401723} Cr-Commit-Position: refs/heads/master@{#403016} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/298aa38b91c86e1540aeb2a1c119c9040715037c Cr-Commit-Position: refs/heads/master@{#403016}
Message was sent while issue was closed.
Description was changed from ========== Reland: Fix touchpad gesture routing to renderers With the addition for touchpad pinch zoom, we will have gesture events with touchpad as the source device. RenderWidgetHostInputEventRouter should handle these types of gestures properly. Also, handling touchpad fling events that are recently switched to use RenderWidgetHostInputEventRouter path. BUG=410580 Committed: https://crrev.com/c6cf0184697c9b473b78cdc0e44490a73da95486 Committed: https://crrev.com/298aa38b91c86e1540aeb2a1c119c9040715037c Cr-Original-Commit-Position: refs/heads/master@{#401723} Cr-Commit-Position: refs/heads/master@{#403016} ========== to ========== Reland: Fix touchpad gesture routing to renderers With the addition for touchpad pinch zoom, we will have gesture events with touchpad as the source device. RenderWidgetHostInputEventRouter should handle these types of gestures properly. Also, handling touchpad fling events that are recently switched to use RenderWidgetHostInputEventRouter path. BUG=410580 Committed: https://crrev.com/c6cf0184697c9b473b78cdc0e44490a73da95486 Committed: https://crrev.com/298aa38b91c86e1540aeb2a1c119c9040715037c Cr-Original-Commit-Position: refs/heads/master@{#401723} Cr-Commit-Position: refs/heads/master@{#403016} ==========
Message was sent while issue was closed.
wjmaclean@chromium.org changed reviewers: + wjmaclean@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2034213002/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2034213002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_input_event_router.cc:317: if (event->type == blink::WebInputEvent::GesturePinchBegin || As best I recall, we should *never* be sending pinch events anywhere other than the main frame. BrowserPlugin is certainly not going to be expecting them, and it will break expected behaviour for anything with a WebView.
Message was sent while issue was closed.
https://codereview.chromium.org/2034213002/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2034213002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_input_event_router.cc:317: if (event->type == blink::WebInputEvent::GesturePinchBegin || On 2016/07/14 at 12:15:11, wjmaclean wrote: > As best I recall, we should *never* be sending pinch events anywhere other than the main frame. BrowserPlugin is certainly not going to be expecting them, and it will break expected behaviour for anything with a WebView. I think *touchpad* pinch events need to be sent to whatever frame is under the cursor to give it the opportunity to handle the ctrl+wheel event generated from the pinch event for custom pinch handling (For example in Google Maps). WDYT?
Message was sent while issue was closed.
https://codereview.chromium.org/2034213002/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2034213002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_input_event_router.cc:317: if (event->type == blink::WebInputEvent::GesturePinchBegin || On 2016/07/15 at 14:40:05, mohsen wrote: > On 2016/07/14 at 12:15:11, wjmaclean wrote: > > As best I recall, we should *never* be sending pinch events anywhere other than the main frame. BrowserPlugin is certainly not going to be expecting them, and it will break expected behaviour for anything with a WebView. > > I think *touchpad* pinch events need to be sent to whatever frame is under the cursor to give it the opportunity to handle the ctrl+wheel event generated from the pinch event for custom pinch handling (For example in Google Maps). WDYT? Also, don't we already send touchscreen pinch events to the frame at the pinch location, rather than the main frame as you pointed out? Am I missing something here?
Message was sent while issue was closed.
Sending the touchscreen pinch gestures is a mistake I will fix at the same time as I fix the touch pad pinch gestures. On 15 Jul 2016 10:46, <mohsen@chromium.org> wrote: > > > https://codereview.chromium.org/2034213002/diff/180001/content/browser/render... > File > content/browser/renderer_host/render_widget_host_input_event_router.cc > (right): > > > https://codereview.chromium.org/2034213002/diff/180001/content/browser/render... > content/browser/renderer_host/render_widget_host_input_event_router.cc:317: > if (event->type == blink::WebInputEvent::GesturePinchBegin || > On 2016/07/15 at 14:40:05, mohsen wrote: > > On 2016/07/14 at 12:15:11, wjmaclean wrote: > > > As best I recall, we should *never* be sending pinch events anywhere > other than the main frame. BrowserPlugin is certainly not going to be > expecting them, and it will break expected behaviour for anything with a > WebView. > > > > I think *touchpad* pinch events need to be sent to whatever frame is > under the cursor to give it the opportunity to handle the ctrl+wheel > event generated from the pinch event for custom pinch handling (For > example in Google Maps). WDYT? > > Also, don't we already send touchscreen pinch events to the frame at the > pinch location, rather than the main frame as you pointed out? Am I > missing something here? > > https://codereview.chromium.org/2034213002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2016/07/15 14:54:37, wjmaclean wrote: > Sending the touchscreen pinch gestures is a mistake I will fix at the same > time as I fix the touch pad pinch gestures. > > On 15 Jul 2016 10:46, <mailto:mohsen@chromium.org> wrote: > > > > > > > > https://codereview.chromium.org/2034213002/diff/180001/content/browser/render... > > File > > content/browser/renderer_host/render_widget_host_input_event_router.cc > > (right): > > > > > > > https://codereview.chromium.org/2034213002/diff/180001/content/browser/render... > > content/browser/renderer_host/render_widget_host_input_event_router.cc:317: > > if (event->type == blink::WebInputEvent::GesturePinchBegin || > > On 2016/07/15 at 14:40:05, mohsen wrote: > > > On 2016/07/14 at 12:15:11, wjmaclean wrote: > > > > As best I recall, we should *never* be sending pinch events anywhere > > other than the main frame. BrowserPlugin is certainly not going to be > > expecting them, and it will break expected behaviour for anything with a > > WebView. > > > > > > I think *touchpad* pinch events need to be sent to whatever frame is > > under the cursor to give it the opportunity to handle the ctrl+wheel > > event generated from the pinch event for custom pinch handling (For > > example in Google Maps). WDYT? > > > > Also, don't we already send touchscreen pinch events to the frame at the > > pinch location, rather than the main frame as you pointed out? Am I > > missing something here? > > > > https://codereview.chromium.org/2034213002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. It's a good point though that we need to let apps preventDefault the ctrl-wheel gesture...
Message was sent while issue was closed.
On 2016/07/15 16:47:36, tdresser wrote: > It's a good point though that we need to let apps preventDefault the ctrl-wheel > gesture... Sure, but don't the wheels go to the target frame first, come back, then create the gesture? That's how touchscreen works.
Message was sent while issue was closed.
On 2016/07/15 16:52:23, wjmaclean wrote: > On 2016/07/15 16:47:36, tdresser wrote: > > > It's a good point though that we need to let apps preventDefault the > ctrl-wheel > > gesture... > > Sure, but don't the wheels go to the target frame first, come back, then create > the gesture? That's how touchscreen works. Oh, yeah, of course. SGTM!
Message was sent while issue was closed.
On 2016/07/15 at 16:52:23, wjmaclean wrote: > On 2016/07/15 16:47:36, tdresser wrote: > > > It's a good point though that we need to let apps preventDefault the ctrl-wheel > > gesture... > > Sure, but don't the wheels go to the target frame first, come back, then create the gesture? That's how touchscreen works. Not for the touchpad. Touchpad driver sends us pinch gesture which we send to the renderer. The renderer synthesizes a ctrl+wheel which is sent to the page which can be preventDefault'ed.
Message was sent while issue was closed.
On 2016/07/15 16:58:30, mohsen wrote: > On 2016/07/15 at 16:52:23, wjmaclean wrote: > > On 2016/07/15 16:47:36, tdresser wrote: > > > > > It's a good point though that we need to let apps preventDefault the > ctrl-wheel > > > gesture... > > > > Sure, but don't the wheels go to the target frame first, come back, then > create the gesture? That's how touchscreen works. > > Not for the touchpad. Touchpad driver sends us pinch gesture which we send to > the renderer. The renderer synthesizes a ctrl+wheel which is sent to the page > which can be preventDefault'ed. Hmm, but pinch zoom is only applied on the main frame, so subframes shouldn't be allowed to zoom on their own I don't think. We should get bokan@ in on this discussion.
Message was sent while issue was closed.
On 2016/07/15 17:02:16, wjmaclean wrote: > Hmm, but pinch zoom is only applied on the main frame, so subframes shouldn't be > allowed to zoom on their own I don't think. We should get bokan@ in on this > discussion. +bokan@ - pls look at recent comments re pinch-zoom on subframes, from touchpad.
Message was sent while issue was closed.
On 2016/07/15 17:03:32, wjmaclean wrote: > On 2016/07/15 17:02:16, wjmaclean wrote: > > > Hmm, but pinch zoom is only applied on the main frame, so subframes shouldn't > be > > allowed to zoom on their own I don't think. We should get bokan@ in on this > > discussion. > > +bokan@ - pls look at recent comments re pinch-zoom on subframes, from touchpad. Mohsen's right that today we send the Pinch gesture to Blink, it synthesizes a wheel event, and if that's unhandled we pinch-zoom. This breaks for OOPIF (and is probably already broken today in Mac with --enable-site-isolation). It works outside site-isolation because the WebViewImpl handles the pinch-zoom so it can apply it globally. If this is to work with OOPIF we need to move this logic up into the browser. So the browser should synthesize a wheel event, send it to the renderer associated with the frame, if the ACK comes back unhandled, send the pinch-zoom event to the root view.
Message was sent while issue was closed.
The bug for pinch gestures in OOPIF is https://bugs.chromium.org/p/chromium/issues/detail?id=627926. I have copied bokan's last comment to that thread and welcome any further comment. It sounds to me like that summarizes well the remaining work related to pinch and OOPIFs. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
