Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2186983002:
Re-route Touchscreen GesturePinch events to root view. (Closed)
|
Created:
4 years, 4 months ago by wjmaclean Modified:
4 years, 4 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. |
DescriptionRe-route Touchscreen GesturePinch events to root view.
This CL fixes the routing of GesturePinch* events so that they always go
to the root view. Also, any other gesture events between
GesturePinchBegin and GesturePinchEnd (namely GestureScrollUpdate) are
also routed to the root view, so that the visual viewport pans
correctly.
BUG=627926
Committed: https://crrev.com/237a3cdd338750a6a35d336912c5830868380272
Cr-Commit-Position: refs/heads/master@{#410198}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address kenrb@'s comments. #
Total comments: 14
Patch Set 3 : Address creis@ comments. #
Total comments: 1
Patch Set 4 : Modify to consult RWHI's GestureScroll status. #Patch Set 5 : Fix typo. #
Messages
Total messages: 36 (25 generated)
Description was changed from ========== Re-route Touchscreen GesturePinch events to root view. This CL fixes the routing of GesturePinch* events so that they always go to the root view. Also, any other gesture events between GesturePinchBegin and GesturePinchEnd (namely GestureScrollUpdate) are also routed to the root view, so that the visual viewport pans correctly. BUG=627926 ========== to ========== Re-route Touchscreen GesturePinch events to root view. This CL fixes the routing of GesturePinch* events so that they always go to the root view. Also, any other gesture events between GesturePinchBegin and GesturePinchEnd (namely GestureScrollUpdate) are also routed to the root view, so that the visual viewport pans correctly. BUG=627926 ==========
wjmaclean@chromium.org changed reviewers: + kenrb@chromium.org
kenrb@ - would you please give this a first look? It won't land until after https://bugs.chromium.org/p/chromium/issues/detail?id=631135 is resolved.
The CQ bit was checked by wjmaclean@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.
lgtm https://codereview.chromium.org/2186983002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2186983002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:396: // revisit how this code should work. nit: Add the bug number that this depends on, 526463. https://codereview.chromium.org/2186983002/diff/1/content/browser/site_per_pr... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2186983002/diff/1/content/browser/site_per_pr... content/browser/site_per_process_browsertest.cc:7575: void SetupRootAdnChild() { nit: Typo.
wjmaclean@chromium.org changed reviewers: + creis@chromium.org
creis@ - a not-too-large CL, please take a look? https://codereview.chromium.org/2186983002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2186983002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:396: // revisit how this code should work. On 2016/07/27 21:49:49, kenrb wrote: > nit: Add the bug number that this depends on, 526463. Done. https://codereview.chromium.org/2186983002/diff/1/content/browser/site_per_pr... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2186983002/diff/1/content/browser/site_per_pr... content/browser/site_per_process_browsertest.cc:7575: void SetupRootAdnChild() { On 2016/07/27 21:49:49, kenrb wrote: > nit: Typo. Done.
The CQ bit was checked by wjmaclean@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.
[+site-isolation-reviews] Mostly deferring to kenrb on the behavior. Otherwise LGTM with nits. Thanks! https://codereview.chromium.org/2186983002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.h (right): https://codereview.chromium.org/2186983002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.h:140: bool in_touchscreen_gesture_pinch_; Can you add comments for these? It's a bit unfortunate that none of the members of this class are documented. https://codereview.chromium.org/2186983002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.h:141: bool root_view_gesture_scrolling_; Is this stale? It's never initialized, and looks unused. https://codereview.chromium.org/2186983002/diff/20001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2186983002/diff/20001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:189: bool EventWasReceived() const { return !events_received_.empty(); } Let's be consistent with naming convention (vs events_received). I think it makes sense for all of these to be hacker_case, right? e.g., event_received, event_type, reset_events_received https://codereview.chromium.org/2186983002/diff/20001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:190: void ResetEventReceived() { events_received_.clear(); } nit: Events should be plural. https://codereview.chromium.org/2186983002/diff/20001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:7531: #if defined(USE_AURA) There's a lot of Aura-specific blocks in this file, and the file is enormous. Can we split them out into a separate site_per_process_aura_browsertest.cc? I'm ok doing that in a followup CL, to avoid making unrelated changes here. https://codereview.chromium.org/2186983002/diff/20001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:7536: void SendPinchBeginEndSequence(RenderWidgetHostViewAura* rwhva, Please add an explanatory comment here, since there's a lot going on in this method. https://codereview.chromium.org/2186983002/diff/20001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:7540: // will nit: Fix wrapping.
https://codereview.chromium.org/2186983002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.h (right): https://codereview.chromium.org/2186983002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.h:140: bool in_touchscreen_gesture_pinch_; On 2016/07/28 17:23:02, Charlie Reis wrote: > Can you add comments for these? It's a bit unfortunate that none of the members > of this class are documented. Done. https://codereview.chromium.org/2186983002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.h:141: bool root_view_gesture_scrolling_; On 2016/07/28 17:23:02, Charlie Reis wrote: > Is this stale? It's never initialized, and looks unused. Yes, thanks for catching it! Done. https://codereview.chromium.org/2186983002/diff/20001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2186983002/diff/20001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:189: bool EventWasReceived() const { return !events_received_.empty(); } On 2016/07/28 17:23:02, Charlie Reis wrote: > Let's be consistent with naming convention (vs events_received). I think it > makes sense for all of these to be hacker_case, right? e.g., event_received, > event_type, reset_events_received Perhaps this can also be done in a separate CL, as it too will touch a lot of code unrelated to this CL? https://codereview.chromium.org/2186983002/diff/20001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:190: void ResetEventReceived() { events_received_.clear(); } On 2016/07/28 17:23:02, Charlie Reis wrote: > nit: Events should be plural. Done. https://codereview.chromium.org/2186983002/diff/20001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:7531: #if defined(USE_AURA) On 2016/07/28 17:23:02, Charlie Reis wrote: > There's a lot of Aura-specific blocks in this file, and the file is enormous. > Can we split them out into a separate site_per_process_aura_browsertest.cc? I'm > ok doing that in a followup CL, to avoid making unrelated changes here. Acknowledged. https://codereview.chromium.org/2186983002/diff/20001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:7540: // will On 2016/07/28 17:23:02, Charlie Reis wrote: > nit: Fix wrapping. Done.
Thanks. I assume you're not blocked on me, but still LGTM. https://codereview.chromium.org/2186983002/diff/20001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2186983002/diff/20001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:189: bool EventWasReceived() const { return !events_received_.empty(); } On 2016/07/29 14:28:31, wjmaclean wrote: > On 2016/07/28 17:23:02, Charlie Reis wrote: > > Let's be consistent with naming convention (vs events_received). I think it > > makes sense for all of these to be hacker_case, right? e.g., event_received, > > event_type, reset_events_received > > Perhaps this can also be done in a separate CL, as it too will touch a lot of > code unrelated to this CL? Sure, that's fine. https://codereview.chromium.org/2186983002/diff/40001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2186983002/diff/40001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:7538: // codepath will require GesturePinch* to be encolsed in nit: enclosed
The CQ bit was checked by wjmaclean@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...
kenrb@ - I've modified to use the state in RenderWidgetHostImpl as we discussed ... can you please take another look?
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
The CQ bit was checked by wjmaclean@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 wjmaclean@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2186983002/#ps80001 (title: "Fix typo.")
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.
Description was changed from ========== Re-route Touchscreen GesturePinch events to root view. This CL fixes the routing of GesturePinch* events so that they always go to the root view. Also, any other gesture events between GesturePinchBegin and GesturePinchEnd (namely GestureScrollUpdate) are also routed to the root view, so that the visual viewport pans correctly. BUG=627926 ========== to ========== Re-route Touchscreen GesturePinch events to root view. This CL fixes the routing of GesturePinch* events so that they always go to the root view. Also, any other gesture events between GesturePinchBegin and GesturePinchEnd (namely GestureScrollUpdate) are also routed to the root view, so that the visual viewport pans correctly. BUG=627926 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Re-route Touchscreen GesturePinch events to root view. This CL fixes the routing of GesturePinch* events so that they always go to the root view. Also, any other gesture events between GesturePinchBegin and GesturePinchEnd (namely GestureScrollUpdate) are also routed to the root view, so that the visual viewport pans correctly. BUG=627926 ========== to ========== Re-route Touchscreen GesturePinch events to root view. This CL fixes the routing of GesturePinch* events so that they always go to the root view. Also, any other gesture events between GesturePinchBegin and GesturePinchEnd (namely GestureScrollUpdate) are also routed to the root view, so that the visual viewport pans correctly. BUG=627926 Committed: https://crrev.com/237a3cdd338750a6a35d336912c5830868380272 Cr-Commit-Position: refs/heads/master@{#410198} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/237a3cdd338750a6a35d336912c5830868380272 Cr-Commit-Position: refs/heads/master@{#410198} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
