Overhaul or remove game controller autoscrolling |
||||||||||||
Issue descriptionAs part of http://crbug.com/454355, two special gamepad behaviors were shipped in the Chromium for Android Java layer: left-stick scrolling and shoulder buttons zoom in/out. This code is problematic for the following reasons: 1. The code is written in Java and so cannot be shared with other Chromium platforms. 2. It's impossible to customize directly using Javascript (except via disabling scrolling/zooming entirely). 3. The unit tests for it are difficult to write and have sometimes been flaky. 4. The left-stick self-animating behavior is quite similar to Windows middle-click autoscroll (see http://crbug.com/522011 ) and selection-drag autoscroll, but the design, implementation, and tests are completely disjoint. It's possible to reimplement this in a way that addresses all these concerns. It would look something like a C++ browser/CC-impl thread autoscroll manager object that received various types of user input signals (gamepad, middle-click, mouse moves, selection handle drags) and generated a stream of synthetic wheel events from it (and also pinch events -- exclusively for the gamepad shoulder buttons). We're currently working on a design doc for this. This plan leaves the current gamepad autoscroll implementation in a limbo, as all the code is slated for eventual deletion. Lately, according to our internal dashboard, the tests have stopped being flaky, but we may disable them if they flake again.
,
Sep 7 2016
Please hold off until we have a design doc before writing code, to save time (e.g. we haven't made the final decision on whether this should go into CC-impl or browser).
,
Sep 7 2016
I will start working on it in Q4.
,
Sep 27 2016
,
Mar 27 2017
+cc folks currently refactoring this in https://codereview.chromium.org/2770613002/. The long-term plan for this is still "a C++ browser/CC-impl thread autoscroll manager object that received various types of user input signals (gamepad, middle-click, mouse moves, selection handle drags) and generated a stream of synthetic wheel events from it (and also pinch events -- exclusively for the gamepad shoulder buttons)."
,
Mar 27 2017
cc also sunyunjia@ who is working on middle-click autoscroll in http://crbug.com/522011
,
Mar 28 2017
I am afraid I won't be able to pick it up soon. Marking it as available if anyone is willing to start soon.
,
Apr 27 2017
,
May 9 2017
I looked into what this would involve today. The biggest use of AutoscrollController.cpp is selection-drag autoscroll, so we'll want to preserve the nuances of that. That latches to a single scroller without chaining. That can't be done purely on the browser process. But it's exactly how touchscreen flings behave, and also the preexisting concept of "fling boosting" is similar to how we need to change the velocity on the existing animation when autoscroll parameters change. So I think the way to do this is to generalize the fling code in input_handler_proxy.cc to support non-decelerating fling curves.
,
May 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8 commit c560f19cffa304227d7e1e9b3cf00acb4cd92cb8 Author: Alexandre Elias <aelias@chromium.org> Date: Thu May 18 08:15:19 2017 Switch gamepad autoscroll to fling In order to avoid special-purpose animation callbacks on the UI thread, this changes gamepad left-stick autoscroll to use the existing CC-impl-thread fling system instead. The browser sends a FlingStart gesture whenever the stick changes position, and the impl thread scrolls at a fixed velocity while the stick remains in place. This is plumbed through via a new "SyntheticAutoscroll" WebGestureDevice. I called it that instead of "Gamepad" because I plan to use this device type to drive middle-click autoscroll, drag-and-drop autoscroll, and selection out-of-bounds autoscroll, which are all similar indefinite fixed-velocity scrolls currently animating on the Blink main thread. This also leaves for now gamepad trigger autozoom using the old approach, I'll address it separately (still deciding whether to delete or support this subfeature). BUG= 644488 TBR=avi@chromium.org,dtrainor@chromium.org (for mechanical changes in content/child/, chrome/android/javatests/) TEST=ContentViewScrollingTest.testJoystickScroll Change-Id: I41329e23899ffd90ab27440cfbf52a537f82a9a5 Reviewed-on: https://chromium-review.googlesource.com/502728 Reviewed-by: Alexandre Elias <aelias@chromium.org> Reviewed-by: Theresa Wellington <twellington@chromium.org> Reviewed-by: Selim Gurun <sgurun@chromium.org> Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Reviewed-by: David Tapuska <dtapuska@chromium.org> Reviewed-by: Jinsuk Kim <jinsukkim@chromium.org> Reviewed-by: Kent Tamura <tkent@chromium.org> Commit-Queue: Alexandre Elias <aelias@chromium.org> Cr-Commit-Position: refs/heads/master@{#472737} [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/android_webview/java/src/org/chromium/android_webview/AwContents.java [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/content/browser/android/content_view_core_impl.cc [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/content/browser/android/content_view_core_impl.h [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/content/browser/renderer_host/render_widget_host_impl.cc [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/content/browser/renderer_host/render_widget_host_impl.h [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/content/browser/renderer_host/render_widget_host_input_event_router.cc [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/content/child/blink_platform_impl.cc [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/content/public/android/BUILD.gn [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java [delete] https://crrev.com/2cc0e2bbd1160f43d3400c6cc7c67c5a92a62aff/content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/third_party/WebKit/Source/platform/scroll/Scrollbar.cpp [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/third_party/WebKit/Source/web/WebViewImpl.cpp [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/third_party/WebKit/public/platform/WebGestureDevice.h [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/third_party/WebKit/public/platform/WebGestureEvent.h [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/ui/events/BUILD.gn [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/ui/events/blink/input_handler_proxy.cc [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/ui/events/gestures/blink/DEPS [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/ui/events/gestures/blink/web_gesture_curve_impl.cc [modify] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/ui/events/gestures/blink/web_gesture_curve_impl.h [add] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/ui/events/gestures/fixed_velocity_curve.cc [add] https://crrev.com/c560f19cffa304227d7e1e9b3cf00acb4cd92cb8/ui/events/gestures/fixed_velocity_curve.h
,
May 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5c2987aea858fe28a76f6d41ca86ed2771b39e4b commit 5c2987aea858fe28a76f6d41ca86ed2771b39e4b Author: aelias <aelias@chromium.org> Date: Fri May 19 03:50:00 2017 Delete Android joystick zoom code. Reasons: - This zooming code cannot naturally be refactored to use a similar approach as http://crrev.com/472737; that would require introducing a "zoom velocity" to fling events used only for this, which would be a maintenance burden. - I tried connecting a PS4 controller and the trigger axes weren't mapped at all. In general, button mapping on Android is highly random and the left-stick is one of the only things mapped consistently. And this wasn't hooked up to the gamepad mappings, which is wildly incomplete anyway. So it's unlikely that any users are benefiting from this feature except on the specific controller-style NVidia Shield (which is a discontinued product) that http://crrev.com/380572 was tested on. - Zooming is a relatively obscure use case now that most sites are mobile-optimized. BUG= 644488 Review-Url: https://codereview.chromium.org/2869313003 Cr-Commit-Position: refs/heads/master@{#473069} [modify] https://crrev.com/5c2987aea858fe28a76f6d41ca86ed2771b39e4b/content/public/android/BUILD.gn [modify] https://crrev.com/5c2987aea858fe28a76f6d41ca86ed2771b39e4b/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java [delete] https://crrev.com/44116dd8f6d76d2b9c85d51e5916fe08604407a6/content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java [delete] https://crrev.com/44116dd8f6d76d2b9c85d51e5916fe08604407a6/content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java
,
May 19 2017
,
Aug 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1794739316000394fb7e0d4ae4245f2166590218 commit 1794739316000394fb7e0d4ae4245f2166590218 Author: Bo Liu <boliu@chromium.org> Date: Thu Aug 24 13:56:22 2017 android: Return zero velocity joystick event as unhandled This is a follow up after refactor in refs/heads/master@{#472737}. Bring back checks for zero velocity joystick events and leave them as unhandled. These checks are important because they allow client to then convert unhandled joystick events to dpad events. Bug: 758463 Bug: 644488 Change-Id: I855103dc6878dada3701e7be3b268186ccf4bc71 Reviewed-on: https://chromium-review.googlesource.com/631476 Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Commit-Queue: Bo <boliu@chromium.org> Cr-Commit-Position: refs/heads/master@{#497030} [modify] https://crrev.com/1794739316000394fb7e0d4ae4245f2166590218/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
,
Aug 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/10bef95abc02eea604380458c6f30346d5ae6782 commit 10bef95abc02eea604380458c6f30346d5ae6782 Author: Bo Liu <boliu@chromium.org> Date: Thu Aug 24 16:30:41 2017 android: Return zero velocity joystick event as unhandled This is a follow up after refactor in refs/heads/master@{#472737}. Bring back checks for zero velocity joystick events and leave them as unhandled. These checks are important because they allow client to then convert unhandled joystick events to dpad events. Bug: 758463 Bug: 644488 Change-Id: I855103dc6878dada3701e7be3b268186ccf4bc71 Reviewed-on: https://chromium-review.googlesource.com/631476 Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Commit-Queue: Bo <boliu@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#497030}(cherry picked from commit 1794739316000394fb7e0d4ae4245f2166590218) Reviewed-on: https://chromium-review.googlesource.com/633483 Reviewed-by: Bo <boliu@chromium.org> Cr-Commit-Position: refs/branch-heads/3163@{#855} Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528} [modify] https://crrev.com/10bef95abc02eea604380458c6f30346d5ae6782/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
,
Aug 28 2017
Issue 650351 has been merged into this issue.
,
Aug 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2a599b1b932054e3181686c7110ed629795e8c29 commit 2a599b1b932054e3181686c7110ed629795e8c29 Author: Bo Liu <boliu@chromium.org> Date: Tue Aug 29 18:41:29 2017 [Merge M60] android: Return zero velocity joystick event as unhandled This is a follow up after refactor in refs/heads/master@{#472737}. Bring back checks for zero velocity joystick events and leave them as unhandled. These checks are important because they allow client to then convert unhandled joystick events to dpad events. Bug: 758463 Bug: 644488 Change-Id: I855103dc6878dada3701e7be3b268186ccf4bc71 Reviewed-on: https://chromium-review.googlesource.com/631476 Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Commit-Queue: Bo <boliu@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#497030}(cherry picked from commit 1794739316000394fb7e0d4ae4245f2166590218) Reviewed-on: https://chromium-review.googlesource.com/642012 Reviewed-by: Bo <boliu@chromium.org> Cr-Commit-Position: refs/branch-heads/3112@{#745} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/2a599b1b932054e3181686c7110ed629795e8c29/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by sshe...@nvidia.com
, Sep 7 2016