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

Issue 2815823003: Notify OverscrollController of gesture events in plugins.

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by Kevin McNee
Modified:
1 day, 2 hours 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

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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+90 lines, -1 line) Patch
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 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 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.cc View 1 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 1 chunk +2 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 1 chunk +8 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 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 1 chunk +6 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 56 (40 generated)
Kevin McNee
Adding kenrb for any comments on the notifying of owners about BrowserPlugin's consumed gesture events. ...
1 month, 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 ...
1 month, 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: > > ...
1 month, 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 ...
1 month 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 ...
2 weeks, 5 days ago (2017-05-04 21:17:47 UTC) #27
kenrb
lgtm
2 weeks, 5 days 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 ...
2 weeks, 4 days 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 ...
2 weeks, 4 days ago (2017-05-05 19:15:37 UTC) #31
Charlie Reis (overloaded)
Similar to https://codereview.chromium.org/2866523002/, is there a test you can add for this? Otherwise looks good ...
2 weeks, 4 days 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 ...
1 week, 1 day 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
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 days, 19 hours ago (2017-05-17 22:39:34 UTC) #35
Charlie Reis (overloaded)
On 2017/05/17 22:39:34, Kevin McNee wrote: > Once I've covered the OOPIF case, it'll make ...
5 days 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: > > ...
4 days, 21 hours ago (2017-05-19 19:46:11 UTC) #54
Charlie Reis (overloaded)
On 2017/05/19 19:46:11, Kevin McNee wrote: > On 2017/05/19 17:07:04, Charlie Reis wrote: > > ...
4 days, 20 hours ago (2017-05-19 20:46:19 UTC) #55
Kevin McNee
1 day, 2 hours ago (2017-05-23 15:21:03 UTC) #56
ping sadrul

I have all the LGTMs needed for this, but did you want to take a look at the
aura changes before I commit?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06