New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 703023 link

Starred by 1 user
Status: Fixed
Owner:
jinsuk...@chromium.org
Closed: Apr 2017
Cc:
█ ti...@chromium.org
boliu@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Feature



Sign in to add a comment

Move Joystick controller classes out of content/

Project Member Reported by jinsuk...@chromium.org, Mar 20 2017 
ContentViewCore initializes and manages Joystick-related classes (JoystickScrollProvider/JoystickZoomProvider and the native files). They belong to an input device category (though internally translated to mouse wheel), so I wonder if it makes sense to move them under, say, /device (maybe in org.chromium.device.joystick?).

Background: While working on refactoring a few other events forwarding flow (GenericMotionEvent, HoverEvent, etc. by EventForwarder), I found that some of them may be consumed by Joystick input device. Having the related classes in component/device rather than in content/ will allow org.chromium.ui.base.EventForwarder to make use of them without a  dependency on content/ (which is wrong).

Let me know what you think. If that sounds ok, I'd like to make a few changes (such as eliminating the dependency on WebContents, CVC, etc.) in the classes to migrate them to a new package path.

Comment 1 by jinsuk...@chromium.org, Mar 21 2017

Owner: jinsuk...@chromium.org
Status: Assigned (was: Available)

Comment 2 by boliu@chromium.org, Mar 21 2017 

why does ui need to know about joystick? shouldn't it be the other way around?

Comment 3 by jinsuk...@chromium.org, Mar 21 2017 

Well ui knows about input devices like mouse and keyboard, and I think joystick can be one of them too.

Comment 4 by boliu@chromium.org, Mar 21 2017 

I mean layering/dependency. You can have a method on EventForwarder that says onJoystickEvent, but you can't have ui code depend on device I think

Comment 5 by jinsuk...@chromium.org, Mar 21 2017 

I see. Then another option would be move them under ui/ for EventForwarder to access them. Does that sounds fine?
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 29 2017 

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

commit b40b402c7c6bfef5538ca4b28f8acc36764b5f70
Author: jinsukkim <jinsukkim@chromium.org>
Date: Wed Mar 29 09:39:08 2017

Forward GenericMotionEvent to EventForwarder

This CL sends down generic motion event (scroll, button clicks)
through ViewAndroid tree.

- The native class for JoystickScrollProvider was removed in order
to make use of ViewAndroid tree path.
- MotionEventAndroid was augmented to hold data for wheel scroll
event so that we can recycle ViewAndroid::HitTest for the event.
- Joystick-related classes are created lazily upon handling the first joystick event.

BUG= 703023 

Review-Url: https://codereview.chromium.org/2770613002
Cr-Commit-Position: refs/heads/master@{#460335}

[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/content/browser/BUILD.gn
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/content/browser/android/browser_jni_registrar.cc
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/content/browser/android/content_view_core_impl.cc
[delete] https://crrev.com/a9b00b963270f09b359251568257001734442383/content/browser/android/joystick_scroll_provider.cc
[delete] https://crrev.com/a9b00b963270f09b359251568257001734442383/content/browser/android/joystick_scroll_provider.h
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/content/public/android/BUILD.gn
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
[delete] https://crrev.com/a9b00b963270f09b359251568257001734442383/content/public/android/java/src/org/chromium/content/browser/input/AnimationIntervalProvider.java
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/ui/android/DEPS
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/ui/android/event_forwarder.cc
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/ui/android/event_forwarder.h
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/ui/android/java/src/org/chromium/ui/base/EventForwarder.java
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/ui/android/view_android.cc
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/ui/android/view_android.h
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/ui/android/view_android_unittests.cc
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/ui/android/view_client.cc
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/ui/android/view_client.h
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/ui/events/android/motion_event_android.cc
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/ui/events/android/motion_event_android.h
[modify] https://crrev.com/b40b402c7c6bfef5538ca4b28f8acc36764b5f70/ui/events/android/motion_event_android_unittest.cc

Comment 7 by jinsuk...@chromium.org, Apr 4 2017

Status: Fixed (was: Assigned)
The refactoring was done with joystick classes still remaining in content/. Closing
►

Sign in to add a comment