Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(6)

Issue 2815823003: Notify OverscrollController of gesture events in plugins. (Closed)

Created:
7 months, 1 week ago by Kevin McNee
Modified:
5 months, 3 weeks ago
CC:
chromium-reviews, creis+watch_chromium.org, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nasko+codewatch_chromium.org, jam, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, danakj+watch_chromium.org, James Su
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Notify OverscrollController of gesture events in plugins. RenderWidgetHostViewGuest resends unconsumed GestureScrollUpdates to its embedder (via BrowserPluginGuest::ResendEventToEmbedder) which are then individually wrapped by RenderWidgetHostImpl with GestureScrollBegins and GestureScrollEnds. Hence OverscrollController (a) sees a GestureScrollEnd immediately following every GestureScrollUpdate and ends the overscroll gesture, and (b) does not see GestureScrollUpdates consumed by the plugin, so OverscrollController starts an overscroll gesture even though the content of the plugin was scrolled. Also, OverscrollController does not have a chance to filter the plugin's gesture events, so even if an overscroll gesture has started, the plugin can start consuming scroll updates again. We now notify OverscrollController (as well as the Mac history swiper) of the ACKed gesture events from the plugin, so that it can update its state correctly and allow it to filter the gesture events of the plugin. They also ignore the wrapper GestureScrollBegins and GestureScrollEnds as they do not indicate the beginning/end of the user's gesture. Note that OOPIF-based guests do not use this resending logic. They instead rely on RenderWidgetHostViewChildFrame's scroll bubbling logic. Ensuring this is correct w.r.t. overscroll is being tracked in crbug.com/713368 . BUG=694393 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2815823003 Cr-Commit-Position: refs/heads/master@{#476282} Committed: https://chromium.googlesource.com/chromium/src/+/19fd249f2bd7f2a7ebf6f26a365119ac71e694cb

Patch Set 1 #

Patch Set 2 : Fix for OOPIFs. #

Patch Set 3 : Just deal with BrowserPlugin for now. #

Patch Set 4 : Rebase only. #

Patch Set 5 : Rebase only. #

Patch Set 6 : Fix for Mac history swiper. #

Total comments: 6

Patch Set 7 : Rebase. #

Patch Set 8 : Address comments. #

Patch Set 9 : Rebase. #

Patch Set 10 : Rebase. #

Patch Set 11 : Add test to verify OverscrollController sees consumed scrolls. #

Total comments: 6

Patch Set 12 : Address comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+237 lines, -4 lines) Patch
M chrome/browser/apps/guest_view/web_view_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +49 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm View 1 2 3 4 5 1 chunk +8 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_widget_host_view_child_frame.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_widget_host_view_child_frame.cc View 1 2 3 4 5 6 7 8 9 1 chunk +17 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_widget_host_view_guest.h View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_widget_host_view_guest.cc View 1 2 3 4 5 6 7 8 1 chunk +30 lines, -1 line 0 comments Download
M content/browser/renderer_host/overscroll_controller.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +12 lines, -3 lines 0 comments Download
M content/browser/renderer_host/overscroll_controller.cc View 1 11 1 chunk +10 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +7 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +13 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_base.h View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_base.cc View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -0 lines 0 comments Download
M content/public/test/browser_test_utils.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +19 lines, -0 lines 0 comments Download
M content/public/test/browser_test_utils.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +57 lines, -0 lines 0 comments Download

Messages

Total messages: 82 (57 generated)
Kevin McNee
Adding kenrb for any comments on the notifying of owners about BrowserPlugin's consumed gesture events. ...
7 months, 1 week ago (2017-04-13 15:35:14 UTC) #9
Kevin McNee
On 2017/04/13 15:35:14, Kevin McNee wrote: > Adding kenrb for any comments on the notifying ...
7 months, 1 week ago (2017-04-13 20:25:41 UTC) #10
kenrb
On 2017/04/13 20:25:41, Kevin McNee wrote: > On 2017/04/13 15:35:14, Kevin McNee wrote: > > ...
7 months, 1 week ago (2017-04-13 21:08:57 UTC) #11
Kevin McNee
On 2017/04/13 21:08:57, kenrb wrote: > On 2017/04/13 20:25:41, Kevin McNee wrote: > > On ...
7 months ago (2017-04-19 21:43:32 UTC) #14
Kevin McNee
On 2017/04/19 21:43:32, Kevin McNee wrote: > On 2017/04/13 21:08:57, kenrb wrote: > > On ...
6 months, 2 weeks ago (2017-05-04 21:17:47 UTC) #27
kenrb
lgtm
6 months, 2 weeks ago (2017-05-05 15:26:23 UTC) #28
Kevin McNee
creis, sadrul, erikchen: Now ready for owner reviews. PTAL. creis for content/browser/frame_host sadrul for content/browser/renderer_host ...
6 months, 2 weeks ago (2017-05-05 18:39:25 UTC) #30
erikchen
https://codereview.chromium.org/2815823003/diff/100001/chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm File chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm (right): https://codereview.chromium.org/2815823003/diff/100001/chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm#newcode139 chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm:139: if (event.resending_plugin_id != -1) { this class is a ...
6 months, 2 weeks ago (2017-05-05 19:15:37 UTC) #31
Charlie Reis
Similar to https://codereview.chromium.org/2866523002/, is there a test you can add for this? Otherwise looks good ...
6 months, 2 weeks ago (2017-05-05 23:45:44 UTC) #32
Kevin McNee
https://codereview.chromium.org/2815823003/diff/100001/chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm File chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm (right): https://codereview.chromium.org/2815823003/diff/100001/chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm#newcode139 chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm:139: if (event.resending_plugin_id != -1) { On 2017/05/05 19:15:36, erikchen ...
6 months, 1 week ago (2017-05-16 16:24:23 UTC) #33
erikchen
thanks, chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm lgtm
6 months, 1 week ago (2017-05-17 04:42:40 UTC) #34
Kevin McNee
Once I've covered the OOPIF case, it'll make sense to add a SitePerProcessBrowserTest for overscrolling ...
6 months ago (2017-05-17 22:39:34 UTC) #35
Charlie Reis
On 2017/05/17 22:39:34, Kevin McNee wrote: > Once I've covered the OOPIF case, it'll make ...
6 months ago (2017-05-19 17:07:04 UTC) #52
Kevin McNee
On 2017/05/19 17:07:04, Charlie Reis wrote: > On 2017/05/17 22:39:34, Kevin McNee wrote: > > ...
6 months ago (2017-05-19 19:46:11 UTC) #54
Charlie Reis
On 2017/05/19 19:46:11, Kevin McNee wrote: > On 2017/05/19 17:07:04, Charlie Reis wrote: > > ...
6 months ago (2017-05-19 20:46:19 UTC) #55
Kevin McNee
ping sadrul I have all the LGTMs needed for this, but did you want to ...
6 months ago (2017-05-23 15:21:03 UTC) #56
sadrul
lgtm Is it at all possible to write some tests? This all seems fairly subtle. ...
5 months, 4 weeks ago (2017-05-24 22:22:27 UTC) #57
Kevin McNee
On 2017/05/24 22:22:27, sadrul wrote: > lgtm > > Is it at all possible to ...
5 months, 3 weeks ago (2017-05-29 15:09:51 UTC) #63
wjmaclean
lgtm
5 months, 3 weeks ago (2017-05-29 15:28:03 UTC) #64
sadrul
Thank you for adding the test!
5 months, 3 weeks ago (2017-05-29 16:53:44 UTC) #65
Charlie Reis
Hmm, it seems like there's a lot of unfortunate changes that the test required. If ...
5 months, 3 weeks ago (2017-05-30 22:35:17 UTC) #66
Kevin McNee
https://codereview.chromium.org/2815823003/diff/200001/content/public/test/browser_test_utils.h File content/public/test/browser_test_utils.h (right): https://codereview.chromium.org/2815823003/diff/200001/content/public/test/browser_test_utils.h#newcode857 content/public/test/browser_test_utils.h:857: // (crbug.com/533069). On 2017/05/30 22:35:17, Charlie Reis wrote: > ...
5 months, 3 weeks ago (2017-05-31 18:07:36 UTC) #73
Charlie Reis
Thanks! LGTM.
5 months, 3 weeks ago (2017-05-31 21:21:38 UTC) #76
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2815823003/220001
5 months, 3 weeks ago (2017-06-01 14:37:47 UTC) #79
commit-bot: I haz the power
5 months, 3 weeks ago (2017-06-01 14:42:58 UTC) #82
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/19fd249f2bd7f2a7ebf6f26a3651...

Powered by Google App Engine
This is Rietveld efc10ee0f