New issue
Advanced search Search tips

Issue 811168 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Move Joystick-related parts in CVC to //device

Project Member Reported by jinsuk...@chromium.org, Feb 12 2018

Issue description

A tiny amount of logic/variable reside in ContentViewCore for joystick event handling/state management. Tiny as it is, it straddles on a few other flows (Ime/Focus/MotionEvent), having CVC keep them inside where all these flows go through, hence making it hard to decouple those flows. 

Since it is a kind of aux device (like gamepad), it would make sense to move what's in CVC to //device/joystick so that its state and flow can be referenced by other parts not only through/from CVC but directly from it if necessary.

The new device dir will have following contents:
 - status indicating joystick scroll is enabled 
 - logic that converts generic motion event to fling action that emulates joystick scroll


One possible refactoring around it would be to let joystick use ui/ (EventForwarder) to send down scroll/fling events to native like other events already being handled there. This direction of changes helps break up CVC further (i.e. deleting more interface methods defined for ContentView - ContentViewCore).

This raises a follow-up refactoring possibility of moving scroll/fling-related parts out of content/, probably to ui. All these scroll-related actions look like good fit for being routed to RenderWidgetHostViewAndroid through ViewAndroid hierarchy without going through native CVC.



 
I'm debating if it's worth creating a new component under //device/ with small logic. It may well just stay in content/, and making Joystick(Handler) backed by WebContentsUserData. Initial CL is here https://crrev.com/c/915747.
Project Member

Comment 2 by bugdroid1@chromium.org, Feb 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e6c2eafb638b0a69a5c13fdf04636bdc92327305

commit e6c2eafb638b0a69a5c13fdf04636bdc92327305
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Fri Feb 23 01:10:36 2018

Define JoystickHandler for Android

JoystickHandler in content/ is WebContentsUserData-backed
class that handles generic motion events by converting them
to fling event. This sets ContentViewCore free from having to
manage joystick-related states and keep several view event flows
together inside.

Fling events that joystick input is converted to are now
routed to EventForwarder and going down ViewAndroid tree.
Updated the flow to handle more gesture events relevant to
fling scroll.

Bug:  811168 

Change-Id: I6904e92e708fdea33dba8fc0df2ace48134331fa
Reviewed-on: https://chromium-review.googlesource.com/915747
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538650}
[modify] https://crrev.com/e6c2eafb638b0a69a5c13fdf04636bdc92327305/android_webview/java/src/org/chromium/android_webview/AwContents.java
[modify] https://crrev.com/e6c2eafb638b0a69a5c13fdf04636bdc92327305/chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java
[modify] https://crrev.com/e6c2eafb638b0a69a5c13fdf04636bdc92327305/content/browser/android/content_view_core.cc
[modify] https://crrev.com/e6c2eafb638b0a69a5c13fdf04636bdc92327305/content/browser/android/content_view_core.h
[modify] https://crrev.com/e6c2eafb638b0a69a5c13fdf04636bdc92327305/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/e6c2eafb638b0a69a5c13fdf04636bdc92327305/content/public/android/BUILD.gn
[modify] https://crrev.com/e6c2eafb638b0a69a5c13fdf04636bdc92327305/content/public/android/java/src/org/chromium/content/browser/ContentViewCoreImpl.java
[add] https://crrev.com/e6c2eafb638b0a69a5c13fdf04636bdc92327305/content/public/android/java/src/org/chromium/content/browser/JoystickHandler.java
[modify] https://crrev.com/e6c2eafb638b0a69a5c13fdf04636bdc92327305/content/public/android/java/src/org/chromium/content_public/browser/ContentViewCore.java
[modify] https://crrev.com/e6c2eafb638b0a69a5c13fdf04636bdc92327305/content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java
[modify] https://crrev.com/e6c2eafb638b0a69a5c13fdf04636bdc92327305/content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestContentViewCore.java
[modify] https://crrev.com/e6c2eafb638b0a69a5c13fdf04636bdc92327305/ui/android/event_forwarder.cc
[modify] https://crrev.com/e6c2eafb638b0a69a5c13fdf04636bdc92327305/ui/android/event_forwarder.h
[modify] https://crrev.com/e6c2eafb638b0a69a5c13fdf04636bdc92327305/ui/android/java/src/org/chromium/ui/base/EventForwarder.java
[modify] https://crrev.com/e6c2eafb638b0a69a5c13fdf04636bdc92327305/ui/events/android/gesture_event_android.cc
[modify] https://crrev.com/e6c2eafb638b0a69a5c13fdf04636bdc92327305/ui/events/android/gesture_event_android.h
[modify] https://crrev.com/e6c2eafb638b0a69a5c13fdf04636bdc92327305/ui/events/blink/blink_event_util.cc

Status: Fixed (was: Available)

Sign in to add a comment