Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
165483003:
Gamepad API for Android (Closed)
|
Created:
6 years, 10 months ago by kbalazs Modified:
6 years, 9 months ago Reviewers:
jdduke (slow), bajones, bulach, boliu, xwang, Ted C, Tom Sepez, darin (slow to review), scottmg CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionGamepad API for Android
This patch implements the platform part of gamepad API for Android and enables
it. This is different from other platforms where low level polling based API's
are used. On Android gamepad/joystick input is available via the generic event
based input system and therefore this implementation is hooked to the input
handling of ContentView. Most of the actual logic is implemented in java.
Although a poller thread is not necessary for this implementation I kept using
it for the native part merely to be able to share more code with desktop.
The runtime cost of this code is between 0.1-0.3 milliseconds per frame.
Support for more devices is remaining work.
TEST=http://www.html5rocks.com/en/tutorials/doodles/gamepad/gamepad-tester/tester.html
BUG=330094
Patch Set 1 #
Total comments: 3
Patch Set 2 : updated #
Total comments: 10
Patch Set 3 : incorporated comments #
Total comments: 85
Patch Set 4 : comments addressed #Patch Set 5 : fix forgotten things #
Total comments: 10
Patch Set 6 : address comments #Messages
Total messages: 30 (0 generated)
Thanks for doing this, and sorry for the slow review. I've left a few minor comments, but overall I think this is on the right track. (With the caveat that I'm not at all familiar with Android's input systems) The biggest change that needs to be made is to rebase against the latest code to account for the new buttons structure. https://codereview.chromium.org/165483003/diff/1/content/browser/gamepad/game... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/165483003/diff/1/content/browser/gamepad/game... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:76: memcpy(pad.buttons, raw_buttons, pad.buttonsLength * sizeof(float)); You'll need to rebase this code, the buttons are no longer an array of floats. https://codereview.chromium.org/165483003/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java (right): https://codereview.chromium.org/165483003/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:30: * GamepadPlatformDataFetcherAndroid is merely a wrepper around this. Nit: wrepper->wrapper https://codereview.chromium.org/165483003/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java (right): https://codereview.chromium.org/165483003/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java:235: class SamsungEIGP20GamepadDataMapper extends GamepadDataMapper { I would prefer to not add mappings that haven't been tested against the actual hardware.
Well, I uploaded it mostly for reference as https://codereview.chromium.org/133943002/ was already under review at the time. Previously we agreed to go with the other patch as it was the first. However, I am happy to update this if you want.
Updated according to api changes.
Great, thanks! This update LGTM. I think I prefer the structure of this patch slightly over https://codereview.chromium.org/133943002/, primarily because of the cleaner mapping mechanism. Let's get the appropriate approvals from the rest of the owners and land it. Once this has landed I'd like to get the mappings for the Shield and Xbox360 controllers defined in the other patch added as well.
@bulach, @darin: I'd like you to get owner approval for the appropriate parts
thanks! +ted for the content view core changes.. I have a few comments below: https://codereview.chromium.org/165483003/diff/60001/content/browser/gamepad/... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/165483003/diff/60001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:16: void copyJavaStringToWebUCharArray( nit: s/copy/Copy/ https://codereview.chromium.org/165483003/diff/60001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:17: JNIEnv* env, jstring src, blink::WebUChar* array, int array_lenght) { nit: s/lenght/length/ also, it should probably be size_t https://codereview.chromium.org/165483003/diff/60001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:19: const jchar* utf16 = env->GetStringChars(src, 0); hmm... we try to avoid calling into "env->" directly.. also, the length * sizeof can overflow, should probably do everything in "size_t" space. something like: COMPILE_ASSERT(sizeof(string16::char_type) == sizeof(blink::WebUChar), string16_and_WebUChar_are_same_size); base::string16 data; base::ConvertJavaStringToUTF16(env, src, &data); CHECK(array_length > static_cast<size_t>(0)); const size_t bytes_to_copy = std::min(data.size(), array_length - 1); memcpy(array, data.data(), bytes_to_copy); array[bytes_to_copy] = 0; https://codereview.chromium.org/165483003/diff/60001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:82: jfloat* raw_axes = env->GetFloatArrayElements(axes, 0); ditto, use base::android::JavaFloatArrayToFloatVector(...) and work with a std::vector https://codereview.chromium.org/165483003/diff/60001/content/browser/gamepad/... File content/browser/gamepad/gamepad_platform_data_fetcher_android.h (right): https://codereview.chromium.org/165483003/diff/60001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:46: base::android::ScopedJavaLocalRef<jobject> j_gamepad_adapter_; this should be ScopedJavaGlobalRef instead of Local. the "Local" are just for, well, local variables, and they'll be potentially GCd after the callstack is yield back to java.. "Global" is a stronger ref to the java object, and will kept it alive until it's explicitly gone. https://codereview.chromium.org/165483003/diff/60001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:51: } nit: // namespace content https://codereview.chromium.org/165483003/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/165483003/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1697: nit: spurious? https://codereview.chromium.org/165483003/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java (right): https://codereview.chromium.org/165483003/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java:34: protected enum CanonicalButtonIndex { enums are a no-go in android :( http://developer.android.com/training/articles/memory.html also, if this is shared with c++, please see the example from "net_error_list.h": https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_error... in a nutshell: - extract the enum values in a separate place (like net_error_list.h) - expand it in both c++, like: https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_error... - and also in java, using a template to generate "static final int", like: https://code.google.com/p/chromium/codesearch#chromium/src/net/android/java/N...
https://codereview.chromium.org/165483003/diff/60001/content/browser/gamepad/... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/165483003/diff/60001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:82: jfloat* raw_axes = env->GetFloatArrayElements(axes, 0); On 2014/03/05 12:34:39, bulach wrote: > ditto, use base::android::JavaFloatArrayToFloatVector(...) and work with a > std::vector Ok. For the record I used the jni api's directly to avoid an extra copy. It shouldn't be a real problem though. https://codereview.chromium.org/165483003/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java (right): https://codereview.chromium.org/165483003/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java:34: protected enum CanonicalButtonIndex { Should I keep the naming scheme kButtonWhatever? It is not very java friendly but if I change I have to update all the C++ code that use these. I think it's better to follow the naming scheme as in net_error_list.h but I would rather postpone updating the C++ code in a follow-up. Does that makes sense?
Incorporated comments. For now I did not update C++ code to use generated enums. This will be a rename churn so I would rather to in a follow-up.
"The runtime cost of this code is between 0.1-0.3 milliseconds per frame." Can you talk about this some more? Is this 0.1-0.3ms/frame, even when a gamepad is not attached? Or if it's simply attached? Or if it's actively being used by the web content?
On 2014/03/05 23:05:12, jdduke wrote: > "The runtime cost of this code is between 0.1-0.3 milliseconds per frame." > > Can you talk about this some more? Is this 0.1-0.3ms/frame, even when a gamepad > is not attached? Or if it's simply attached? Or if it's actively being used by > the web content? kbalazs@ can speak for the Android code, but at a high level Chrome does not instantiate a GamepadPlatformDataFetcher until you have visited a page that calls getGamepads at least once. Once you have left all pages which were querying for Gamepad data the GamepadPlatformDataFetcher::PauseHint function will be called to indicate that polling is no longer needed. That does not preclude the Java portion of this code from always running (it's not immediately obvious to me what the object lifetimes are.) but there are mechanisms in place to allow the platform to make intelligent decisions.
TL;DR: I got this "cost" from the trace event that I added in GetGamepadData. As @bajones wrote we only pay this cost when a page needs the data. Also there is the other cost of processing the incoming events by the java code but it is not even in the magnitude of a millisecond.
+jdduke as he's a better owner in general for inputy things, but here are my thoughts on the java side of things. I'll try to dig around in native as well. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java (right): https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:22: class WebGamepadData { I'd prefer this to be a separate class as it is referred to by other files in the package. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:39: private static final int NUM_WEB_GAMEPADS = 4; where did this number come from? https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:55: public static GamepadAdapter getInstance() { Any reason this has to be an instance? We are trying to remove the expectations that only a single content view/web contents will be visible at any given time. I'm just wondering if this can be tied 1-1 to a content view. It also makes sure that we don't leak events from one content view to another. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:65: enumerateDevices(); I would probably call this initializeDevices https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:69: public void pause() { For all non-private methods, you should have javadoc. Here too, I'd really like to see pause and resume tied to the ContentView instead of singleton. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:70: if (mIsPaused) in java, braces are always required if the statement and conditional don't all fit on the same line. For this (and many places), you can just do: if (mIsPaused) return; https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:74: for (int i = 0; i < NUM_WEB_GAMEPADS; ++i) { in java land, we use i++ (applies many places) https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:90: InputDeviceHandler[] handlers = new InputDeviceHandler[NUM_WEB_GAMEPADS]; I would add the assert runningOnUiThread to here to make sure all mutators of the mDeviceHandlers happens on the UI thread. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:91: int[] ids = mInputManager.getInputDeviceIds(); the api doesn't clarify if null is a valid return for this. might want to check null just to be cautious https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:105: public void onInputDeviceAdded(int deviceID) { I think you're supposed to use a lower d in ID. https://source.android.com/source/code-style.html#treat-acronyms-as-words So, "deviceId" and elsewhere https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:162: event.getAction() != KeyEvent.ACTION_UP) { +4 indent https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:177: public static GamepadAdapter attach(long nativeDataFetcher) { why public? https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:215: int indexForDeviceID(int deviceID) { indexForDeviceId https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:218: mDeviceHandlers[i].getInputDevice().getId() == deviceID) +4 indent and needs braces https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:254: private final SparseArray<Boolean> mButtons = new SparseArray<Boolean>(); Should be able to use SparseBooleanArray here. I would also probably call this mButtonPressedStates or something like that as mButtons sounds like a collection of buttons. And along those lines, maybe mAxisValues above. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:281: mAxes.setValueAt(i, value); Hmm...this will result in a new capital Float object for each motion event. That can result in more GC churn than you want for something that happens potentially for every frame. Although it's more manual, I think you'll want to use just a float[] to avoid these objects. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:297: return mMapper.map(mAxes, mButtons); Is this also called every often? It looks like there are a bunch of object creations here as well. You probably want to look at getting the number of allocations to 0 if at all possible. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java (right): https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java:23: public abstract WebGamepadData map(SparseArray<Float> axes, SparseArray<Boolean> buttons); per my comment about object creation in the other file, you might want to have this take a data object and write over it or have this class own one that will get reused. Either way a a comment on this would be good. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java:44: Float x = (Float) axes.get(MotionEvent.AXIS_X); get on a typed SparseArray should mean that you don't need to have these casts anywhere. Same for all the following functions. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java:138: } This doesn't verify axes size is large enough. Might be good to have some assertion in this file that the value you expect doesn't change in native and not get updated here. And looking at it, this looks like it's only initialized to size 4, so this would out of bounds fail if all these values were present. I think some sanity checking on bounds is definitely something we should do here. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java:163: data.id = PS3_SIXAXIS_DEVICE_NAME + " (STANDARD_GAMEPAD)"; I would make " (STANDARD_GAMEPAD)" and "standard" constants as well.
Thanks for the review. The most important thing that we need to agree on is the singleton/per view design. I made some comments why I think singleton is better. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java (right): https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:39: private static final int NUM_WEB_GAMEPADS = 4; On 2014/03/07 20:18:09, Ted C wrote: > where did this number come from? From the spec :) Also from blink::WebGamepads. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:55: public static GamepadAdapter getInstance() { On 2014/03/07 20:18:09, Ted C wrote: > Any reason this has to be an instance? > > We are trying to remove the expectations that only a single content view/web > contents will be visible at any given time. I'm just wondering if this can be > tied 1-1 to a content view. It also makes sure that we don't leak events from > one content view to another. I'm not sure a 1-1 relation is a really good idea. Afaik there is certainly one view that can be focused and generate input events at a time. The native side of the implementation is already organized around singletons, i.e. there is one GamepadService, one GamepadProvider and one GamepadPlatformDataFetcher. The desktop implementations read the device directly and provide the data to every renderer that started reading it. I think leaking events between views is not something the current design tries to avoid and it would require a complete redesign (not just in the platform specific part). https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:69: public void pause() { On 2014/03/07 20:18:09, Ted C wrote: > For all non-private methods, you should have javadoc. > > Here too, I'd really like to see pause and resume tied to the ContentView > instead of singleton. You are right in that if there is more than one active ContentView they can conflict here. Considering to keep the singleton design, I think it can be addressed by adding a counter for pause/resume or calling them by ContentView when it gains/loses focus. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:162: event.getAction() != KeyEvent.ACTION_UP) { On 2014/03/07 20:18:09, Ted C wrote: > +4 indent This is apparently 4, could you elaborate on how this should be written? https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:177: public static GamepadAdapter attach(long nativeDataFetcher) { On 2014/03/07 20:18:09, Ted C wrote: > why public? If I remember correctly the jni generator only allows it to be called by native if it's public. I can double check. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:281: mAxes.setValueAt(i, value); On 2014/03/07 20:18:09, Ted C wrote: > Hmm...this will result in a new capital Float object for each motion event. > That can result in more GC churn than you want for something that happens > potentially for every frame. > > Although it's more manual, I think you'll want to use just a float[] to avoid > these objects. IMHO an associative array is quite convenient here. I want to be able to represent the data in a device agnostic way. I'm not saying that this representation is the best but that's what I found the best so far. This will create a few object (<10) per frame, do you think it's really a notable cost? Anyways, I can think about how to avoid that. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java (right): https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java:44: Float x = (Float) axes.get(MotionEvent.AXIS_X); On 2014/03/07 20:18:09, Ted C wrote: > get on a typed SparseArray should mean that you don't need to have these casts > anywhere. > > Same for all the following functions. Right, but unfortunately there is no such thing as SparseFloatArray (and SparseArray is not generic). I can use a HashMap<int, float> although I'm not sure that would be more efficient. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java:138: } On 2014/03/07 20:18:09, Ted C wrote: > This doesn't verify axes size is large enough. Might be good to have some > assertion in this file that the value you expect doesn't change in native and > not get updated here. > > And looking at it, this looks like it's only initialized to size 4, so this > would out of bounds fail if all these values were present. I think some sanity > checking on bounds is definitely something we should do here. Right, I overlooked it.
Thanks for the nice addition! A few comments. https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:34: using blink::WebGamepads; Please move the using statements above the initial nameespace declaration. https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:37: j_gamepad_adapter_.Reset( If the GamePadAdapter is a singleton in practice, we don't need to hold a reference to it. Just make getGamepadData and setFetched static methods on the GamepadAdapter class that redirect to the proper instance. https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:50: data_->length = WebGamepads::itemsLengthCap; Rather than storing |data_| and assuming we'll get a callback, pass |pads| to Java, and have Java call a static |RefreshGamepadData()| function, similar to the one below but with the |pads| object as an argument. Then populate the |pads| object there, and GamePadAdapter no longer needs a GamepadPlatformDataFetcherAndroid reference (no need to Attach()). https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:57: Java_GamepadAdapter_setFetched( Same with the other function, make this a static call on the Java side that redirects to the appropriate instance. https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:61: void GamepadPlatformDataFetcherAndroid::RefreshDevice( This function can be made static (and local to this .cc file), see above comments. https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:83: memcpy(pad.axes, axes_data.begin(), pad.axesLength * sizeof(float)); Could you add a line space in between each logical vector copy grouping here? https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... File content/browser/gamepad/gamepad_platform_data_fetcher_android.h (right): https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:26: virtual void PauseHint(bool paused); OVERRIDE? https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:29: void RefreshDevice(JNIEnv* env, See .cc comments about how this function can be made static (and local to the .cc file). https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:43: blink::WebGamepads* data_; I'm not sure we need to hold on to this reference. See comments in the .cc file. https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:46: base::android::ScopedJavaGlobalRef<jobject> j_gamepad_adapter_; I'm not sure we need this either. The call into the GamePadAdapter might as well be static if we're going with a singleton. https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... File content/browser/gamepad/gamepad_standard_mappings.h (right): https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_standard_mappings.h:29: Please file a bug. Until we adopt the mentioned header templates, we should use a function to translate between the two enum types.
https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:50: data_->length = WebGamepads::itemsLengthCap; On 2014/03/07 23:16:39, jdduke wrote: > Rather than storing |data_| and assuming we'll get a callback, pass |pads| to > Java, and have Java call a static |RefreshGamepadData()| function, similar to > the one below but with the |pads| object as an argument. Then populate the > |pads| object there, and GamePadAdapter no longer needs a > GamepadPlatformDataFetcherAndroid reference (no need to Attach()). I see now, you definitely still need attach in some fashion, but you could probably just re-use the |setFetched()| function for that. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java (right): https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:181: return instance; Could we rename mIsFetched to something like |mHasListener| or |mDataRequested|. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:281: mAxes.setValueAt(i, value); On 2014/03/07 22:40:22, kbalazs wrote: > On 2014/03/07 20:18:09, Ted C wrote: > > Hmm...this will result in a new capital Float object for each motion event. > > That can result in more GC churn than you want for something that happens > > potentially for every frame. > > > > Although it's more manual, I think you'll want to use just a float[] to avoid > > these objects. > > IMHO an associative array is quite convenient here. I want to be able to > represent the data in a device agnostic way. I'm not saying that this > representation is the best but that's what I found the best so far. This will > create a few object (<10) per frame, do you think it's really a notable cost? > Anyways, I can think about how to avoid that. Any amount of garbage created per frame is undesirable, unfortunately. It's not the memory cost, but the garbage collection trigger that hurts.
+boliu to keep me honest w/ my comments about webview. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java (right): https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:55: public static GamepadAdapter getInstance() { On 2014/03/07 22:40:22, kbalazs wrote: > On 2014/03/07 20:18:09, Ted C wrote: > > Any reason this has to be an instance? > > > > We are trying to remove the expectations that only a single content view/web > > contents will be visible at any given time. I'm just wondering if this can be > > tied 1-1 to a content view. It also makes sure that we don't leak events from > > one content view to another. > > I'm not sure a 1-1 relation is a really good idea. Afaik there is certainly one > view that can be focused and generate input events at a time. The native side of > the implementation is already organized around singletons, i.e. there is one > GamepadService, one GamepadProvider and one GamepadPlatformDataFetcher. The > desktop implementations read the device directly and provide the data to every > renderer that started reading it. I think leaking events between views is not > something the current design tries to avoid and it would require a complete > redesign (not just in the platform specific part). You are definitely correct that only a single view can have focus and generate events. And that is why I find it quite odd (and in my opinion very much the wrong and incorrect behavior) that you could have two desktop browser windows open that were registered for gamepad input and that we would be sending it to both even though only one had window focus (or maybe the renderer only polls for active windows). Personally, that feels incorrect, but I agree that it is better to match desktop behavior then gate this on a larger refactor. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:64: mIsPaused = false; I just noticed that this is the same calls as resume(), so you can probably just call that here. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:69: public void pause() { On 2014/03/07 22:40:22, kbalazs wrote: > On 2014/03/07 20:18:09, Ted C wrote: > > For all non-private methods, you should have javadoc. > > > > Here too, I'd really like to see pause and resume tied to the ContentView > > instead of singleton. > > You are right in that if there is more than one active ContentView they can > conflict here. Considering to keep the singleton design, I think it can be > addressed by adding a counter for pause/resume or calling them by ContentView > when it gains/loses focus. Relying on pause and resume is somewhat dangerous as onHide might never be called on a ContentView if it is being destroyed. And at the same time, onDestroy is also not reliably called in cases like WebView (where applications can just just rely on garbage collection). I believe you can rely on onAttachedToWindow and onDetachedFromWindow in WebView as well as Chrome, so maybe that is ok for now. Granted, having to keep a counter seems very fragile and the same logic has been the source of many of bugs for us in the past. I guess if you did: private boolean mAttachedToWindow; ContentViewCore#onAttachedToWindow() { if (mAttachedToWindow) return; .... GameAdapter.getInstance().registerListener(); mAttachedToWindow = true; } ContentViewCore#onDetachedFromWindow() { if (!mAttachedToWindow) return; GameAdapter.getInstance().unregisterListener(); mAttachedToWindow = false; } That is probably the safest way to to ensure the counters are never out of sync. Conversely, if you want to be move overzealous of unregistering, you can track a separate boolean for mGamepadRegistered and allow unregistering in onHide(), onDetachedFromWindow() and destroy(). Granted, I think if you only register in onAttachedToWindow(), you should remove the registration in the constructor and only do it in these methods, which I think is cleaner anyway. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:162: event.getAction() != KeyEvent.ACTION_UP) { On 2014/03/07 22:40:22, kbalazs wrote: > On 2014/03/07 20:18:09, Ted C wrote: > > +4 indent > > This is apparently 4, could you elaborate on how this should be written? +4 indent was meant to say it needs to be indented 4 more. Second lines of conditionals require two levels of indent (4 spaces per level): if (event.getAction() != KeyEvent.ACTION_DOWN && event.getAction() != KeyEvent.ACTION_UP) { ... https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:177: public static GamepadAdapter attach(long nativeDataFetcher) { On 2014/03/07 22:40:22, kbalazs wrote: > On 2014/03/07 20:18:09, Ted C wrote: > > why public? > > If I remember correctly the jni generator only allows it to be called by native > if it's public. I can double check. Ah, yeah the jni generator definitely allows you to call private methods. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java (right): https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java:44: Float x = (Float) axes.get(MotionEvent.AXIS_X); On 2014/03/07 22:40:22, kbalazs wrote: > On 2014/03/07 20:18:09, Ted C wrote: > > get on a typed SparseArray should mean that you don't need to have these casts > > anywhere. > > > > Same for all the following functions. > > Right, but unfortunately there is no such thing as SparseFloatArray (and > SparseArray is not generic). I can use a HashMap<int, float> although I'm not > sure that would be more efficient. My previous comment was that calling .get() on a SparseArray<Float> is a templated function that doesn't require you to cast the return value. In the above cases, it can just be written: Float x = axes.get(MotionEvent.AXIS_X); Float y = axes.get(MotionEvent.AXIS_Y); With that being said, I think you have to forgo the niceness of SparseArray for the floats due to the GC issue. I totally agree it's annoying, but we've done as much as we can to no allocate anything per frame where ever possible.
On 2014/03/08 01:06:00, Ted C wrote: > +boliu to keep me honest w/ my comments about webview. > > https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java > (right): > > https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:55: > public static GamepadAdapter getInstance() { > On 2014/03/07 22:40:22, kbalazs wrote: > > On 2014/03/07 20:18:09, Ted C wrote: > > > Any reason this has to be an instance? > > > > > > We are trying to remove the expectations that only a single content view/web > > > contents will be visible at any given time. I'm just wondering if this can > be > > > tied 1-1 to a content view. It also makes sure that we don't leak events > from > > > one content view to another. > > > > I'm not sure a 1-1 relation is a really good idea. Afaik there is certainly > one > > view that can be focused and generate input events at a time. The native side > of > > the implementation is already organized around singletons, i.e. there is one > > GamepadService, one GamepadProvider and one GamepadPlatformDataFetcher. The > > desktop implementations read the device directly and provide the data to every > > renderer that started reading it. I think leaking events between views is not > > something the current design tries to avoid and it would require a complete > > redesign (not just in the platform specific part). > > You are definitely correct that only a single view can have focus and > generate events. > > And that is why I find it quite odd (and in my opinion very much the wrong > and incorrect behavior) that you could have two desktop browser windows > open that were registered for gamepad input and that we would be sending > it to both even though only one had window focus (or maybe the renderer > only polls for active windows). > > Personally, that feels incorrect, but I agree that it is better to match > desktop behavior then gate this on a larger refactor. > > https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:64: > mIsPaused = false; > I just noticed that this is the same calls as resume(), so you can probably > just call that here. > > https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:69: > public void pause() { > On 2014/03/07 22:40:22, kbalazs wrote: > > On 2014/03/07 20:18:09, Ted C wrote: > > > For all non-private methods, you should have javadoc. > > > > > > Here too, I'd really like to see pause and resume tied to the ContentView > > > instead of singleton. > > > > You are right in that if there is more than one active ContentView they can > > conflict here. Considering to keep the singleton design, I think it can be > > addressed by adding a counter for pause/resume or calling them by ContentView > > when it gains/loses focus. > > Relying on pause and resume is somewhat dangerous as onHide might never be > called on a ContentView if it is being destroyed. And at the same time, > onDestroy is also not reliably called in cases like WebView (where > applications can just just rely on garbage collection). > > I believe you can rely on onAttachedToWindow and onDetachedFromWindow in > WebView as well as Chrome, so maybe that is ok for now. Granted, having > to keep a counter seems very fragile and the same logic has been the > source of many of bugs for us in the past. > > I guess if you did: > > private boolean mAttachedToWindow; > > ContentViewCore#onAttachedToWindow() { > if (mAttachedToWindow) return; > .... > GameAdapter.getInstance().registerListener(); > mAttachedToWindow = true; > } > > ContentViewCore#onDetachedFromWindow() { > if (!mAttachedToWindow) return; > GameAdapter.getInstance().unregisterListener(); > mAttachedToWindow = false; > } > > That is probably the safest way to to ensure the counters are never > out of sync. Conversely, if you want to be move overzealous of > unregistering, you can track a separate boolean for mGamepadRegistered > and allow unregistering in onHide(), onDetachedFromWindow() and > destroy(). > > Granted, I think if you only register in onAttachedToWindow(), you should > remove the registration in the constructor and only do it in these methods, > which I think is cleaner anyway. > > https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:162: > event.getAction() != KeyEvent.ACTION_UP) { > On 2014/03/07 22:40:22, kbalazs wrote: > > On 2014/03/07 20:18:09, Ted C wrote: > > > +4 indent > > > > This is apparently 4, could you elaborate on how this should be written? > > +4 indent was meant to say it needs to be indented 4 more. > > Second lines of conditionals require two levels of indent (4 spaces per level): > > if (event.getAction() != KeyEvent.ACTION_DOWN > && event.getAction() != KeyEvent.ACTION_UP) { > ... > > https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:177: > public static GamepadAdapter attach(long nativeDataFetcher) { > On 2014/03/07 22:40:22, kbalazs wrote: > > On 2014/03/07 20:18:09, Ted C wrote: > > > why public? > > > > If I remember correctly the jni generator only allows it to be called by > native > > if it's public. I can double check. > > Ah, yeah the jni generator definitely allows you to call private methods. > > https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java > (right): > > https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java:44: > Float x = (Float) axes.get(MotionEvent.AXIS_X); > On 2014/03/07 22:40:22, kbalazs wrote: > > On 2014/03/07 20:18:09, Ted C wrote: > > > get on a typed SparseArray should mean that you don't need to have these > casts > > > anywhere. > > > > > > Same for all the following functions. > > > > Right, but unfortunately there is no such thing as SparseFloatArray (and > > SparseArray is not generic). I can use a HashMap<int, float> although I'm not > > sure that would be more efficient. > > My previous comment was that calling .get() on a SparseArray<Float> is a > templated function that doesn't require you to cast the return value. > > In the above cases, it can just be written: > > Float x = axes.get(MotionEvent.AXIS_X); > Float y = axes.get(MotionEvent.AXIS_Y); > > With that being said, I think you have to forgo the niceness of SparseArray > for the floats due to the GC issue. I totally agree it's annoying, but > we've done as much as we can to no allocate anything per frame where ever > possible. Any chance the GamepadAdapter and Mapper can be unit testable? It'd be nice to make sure we don't regress anywhere here in the future. The finality of the InputDevice on Android makes it probably harder to do because I don't see an easy way to mock an InputDevice and test these classes in isolation (with a MockContext you would have been able to give a dummy input manager, but I don't know if that will help much).
https://codereview.chromium.org/165483003/diff/80001/content/content.gyp File content/content.gyp (right): https://codereview.chromium.org/165483003/diff/80001/content/content.gyp#newc... content/content.gyp:504: 'target_name': 'content_gamepad_mapping', This will need changes in android_webview/all_webview.gyp and LOCAL_GENERATED_SOURCES in android_webview/Android.mk https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java (right): https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:62: mInputManager = (InputManager) context.getSystemService(Context.INPUT_SERVICE); Can this ever be null (say if webview is in a service)? What if there is no keyboard active (I got my device into that situation before...) Probably safer to not assume getSystemService a real object. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:69: public void pause() { What Ted said. Webview is a view, so Activity lifetime events does not apply (webview can be used in an android service where there is no activity). onHide is never called in webview either because there is no such thing as a hidden webview in chrome's sense. The only pair of calls webview can guarantee to match is onAttachedToWindow and onDetachedFromWindow. It's not perfect either since webview can be used without ever being attached, but there is no better alternative to support refcounting a global resource. But I guess gamepad support makes no sense for an unattached webview anyway, so might be ok.
On 2014/03/07 23:33:26, jdduke wrote: > https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... > File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): > > https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... > content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:50: > data_->length = WebGamepads::itemsLengthCap; > On 2014/03/07 23:16:39, jdduke wrote: > > Rather than storing |data_| and assuming we'll get a callback, pass |pads| to > > Java, and have Java call a static |RefreshGamepadData()| function, similar to > > the one below but with the |pads| object as an argument. Then populate the > > |pads| object there, and GamePadAdapter no longer needs a > > GamepadPlatformDataFetcherAndroid reference (no need to Attach()). > > I see now, you definitely still need attach in some fashion, but you could > probably just re-use the |setFetched()| function for that. As far as i understand the jni generator use the name of the first argument of the native* function to know what class the native function belongs to. So it seems to me that only methods can be called from java (as far as I don't want to hand-roll jni).
https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java (right): https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:162: event.getAction() != KeyEvent.ACTION_UP) { On 2014/03/08 01:06:00, Ted C wrote: > On 2014/03/07 22:40:22, kbalazs wrote: > > On 2014/03/07 20:18:09, Ted C wrote: > > > +4 indent > > > > This is apparently 4, could you elaborate on how this should be written? > > +4 indent was meant to say it needs to be indented 4 more. > > Second lines of conditionals require two levels of indent (4 spaces per level): > > if (event.getAction() != KeyEvent.ACTION_DOWN > && event.getAction() != KeyEvent.ACTION_UP) { > ... I see. For the record I don't find this rule neither in the Android style guide nor at http://www.chromium.org/developers/coding-style/java so probably this should be written somewhere.
Sadly, the chrome android style guide is a multi step affair. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java (right): https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:162: event.getAction() != KeyEvent.ACTION_UP) { On 2014/03/11 20:38:34, kbalazs wrote: > On 2014/03/08 01:06:00, Ted C wrote: > > On 2014/03/07 22:40:22, kbalazs wrote: > > > On 2014/03/07 20:18:09, Ted C wrote: > > > > +4 indent > > > > > > This is apparently 4, could you elaborate on how this should be written? > > > > +4 indent was meant to say it needs to be indented 4 more. > > > > Second lines of conditionals require two levels of indent (4 spaces per > level): > > > > if (event.getAction() != KeyEvent.ACTION_DOWN > > && event.getAction() != KeyEvent.ACTION_UP) { > > ... > > I see. For the record I don't find this rule neither in the Android style guide > nor at http://www.chromium.org/developers/coding-style/java so probably this > should be written somewhere. It's in the referenced android style guide: http://source.android.com/source/code-style.html#use-spaces-for-indentation """ We use 8 space indents for line wraps, including function calls and assignments. """
Updated, PTAL https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:34: using blink::WebGamepads; On 2014/03/07 23:16:39, jdduke wrote: > Please move the using statements above the initial nameespace declaration. Done. https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:37: j_gamepad_adapter_.Reset( On 2014/03/07 23:16:39, jdduke wrote: > If the GamePadAdapter is a singleton in practice, we don't need to hold a > reference to it. Just make getGamepadData and setFetched static methods on the > GamepadAdapter class that redirect to the proper instance. Done. https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:50: data_->length = WebGamepads::itemsLengthCap; On 2014/03/07 23:33:27, jdduke wrote: > On 2014/03/07 23:16:39, jdduke wrote: > > Rather than storing |data_| and assuming we'll get a callback, pass |pads| to > > Java, and have Java call a static |RefreshGamepadData()| function, similar to > > the one below but with the |pads| object as an argument. Then populate the > > |pads| object there, and GamePadAdapter no longer needs a > > GamepadPlatformDataFetcherAndroid reference (no need to Attach()). > > I see now, you definitely still need attach in some fashion, but you could > probably just re-use the |setFetched()| function for that. Done. https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:57: Java_GamepadAdapter_setFetched( On 2014/03/07 23:16:39, jdduke wrote: > Same with the other function, make this a static call on the Java side that > redirects to the appropriate instance. Done. https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:83: memcpy(pad.axes, axes_data.begin(), pad.axesLength * sizeof(float)); On 2014/03/07 23:16:39, jdduke wrote: > Could you add a line space in between each logical vector copy grouping here? Done. https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... File content/browser/gamepad/gamepad_platform_data_fetcher_android.h (right): https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:26: virtual void PauseHint(bool paused); On 2014/03/07 23:16:39, jdduke wrote: > OVERRIDE? Done. https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:29: void RefreshDevice(JNIEnv* env, On 2014/03/07 23:16:39, jdduke wrote: > See .cc comments about how this function can be made static (and local to the > .cc file). I think the jni generator only supports methods so I didn't change this. https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:43: blink::WebGamepads* data_; On 2014/03/07 23:16:39, jdduke wrote: > I'm not sure we need to hold on to this reference. See comments in the .cc file. I kept it as I think it makes more sense than passing back and forth. https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:46: base::android::ScopedJavaGlobalRef<jobject> j_gamepad_adapter_; On 2014/03/07 23:16:39, jdduke wrote: > I'm not sure we need this either. The call into the GamePadAdapter might as > well be static if we're going with a singleton. Done. https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... File content/browser/gamepad/gamepad_standard_mappings.h (right): https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_standard_mappings.h:29: On 2014/03/07 23:16:39, jdduke wrote: > Please file a bug. Until we adopt the mentioned header templates, we should use > a function to translate between the two enum types. Bug filed: http://crbug.com/351558 https://codereview.chromium.org/165483003/diff/80001/content/content.gyp File content/content.gyp (right): https://codereview.chromium.org/165483003/diff/80001/content/content.gyp#newc... content/content.gyp:504: 'target_name': 'content_gamepad_mapping', On 2014/03/10 18:27:04, boliu wrote: > This will need changes in android_webview/all_webview.gyp and > LOCAL_GENERATED_SOURCES in android_webview/Android.mk I moved it to be a dependency for content_java. Do you think it is enough for the webview? Also edited Android.mk https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java (right): https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:22: class WebGamepadData { On 2014/03/07 20:18:09, Ted C wrote: > I'd prefer this to be a separate class as it is referred to by other files in > the package. You meant a separate file, right? :) Done. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:62: mInputManager = (InputManager) context.getSystemService(Context.INPUT_SERVICE); On 2014/03/10 18:27:04, boliu wrote: > Can this ever be null (say if webview is in a service)? What if there is no > keyboard active (I got my device into that situation before...) > > Probably safer to not assume getSystemService a real object. Done, added checks. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:65: enumerateDevices(); On 2014/03/07 20:18:09, Ted C wrote: > I would probably call this initializeDevices Done. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:69: public void pause() { On 2014/03/07 20:18:09, Ted C wrote: > For all non-private methods, you should have javadoc. > > Here too, I'd really like to see pause and resume tied to the ContentView > instead of singleton. javadoc added. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:69: public void pause() { On 2014/03/10 18:27:04, boliu wrote: > What Ted said. > > Webview is a view, so Activity lifetime events does not apply (webview can be > used in an android service where there is no activity). onHide is never called > in webview either because there is no such thing as a hidden webview in chrome's > sense. > > The only pair of calls webview can guarantee to match is onAttachedToWindow and > onDetachedFromWindow. It's not perfect either since webview can be used without > ever being attached, but there is no better alternative to support refcounting a > global resource. But I guess gamepad support makes no sense for an unattached > webview anyway, so might be ok. Relying on onAttachedToWindow and onDetachedFromWindow now. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:70: if (mIsPaused) On 2014/03/07 20:18:09, Ted C wrote: > in java, braces are always required if the statement and conditional don't all > fit on the same line. > > For this (and many places), you can just do: > > if (mIsPaused) return; Done. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:74: for (int i = 0; i < NUM_WEB_GAMEPADS; ++i) { On 2014/03/07 20:18:09, Ted C wrote: > in java land, we use i++ (applies many places) Done. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:90: InputDeviceHandler[] handlers = new InputDeviceHandler[NUM_WEB_GAMEPADS]; On 2014/03/07 20:18:09, Ted C wrote: > I would add the assert runningOnUiThread to here to make sure all mutators of > the mDeviceHandlers happens on the UI thread. Done. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:91: int[] ids = mInputManager.getInputDeviceIds(); On 2014/03/07 20:18:09, Ted C wrote: > the api doesn't clarify if null is a valid return for this. might want to check > null just to be cautious Done. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:105: public void onInputDeviceAdded(int deviceID) { On 2014/03/07 20:18:09, Ted C wrote: > I think you're supposed to use a lower d in ID. > > https://source.android.com/source/code-style.html#treat-acronyms-as-words > > So, "deviceId" and elsewhere Done. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:162: event.getAction() != KeyEvent.ACTION_UP) { On 2014/03/11 20:42:47, Ted C wrote: > On 2014/03/11 20:38:34, kbalazs wrote: > > On 2014/03/08 01:06:00, Ted C wrote: > > > On 2014/03/07 22:40:22, kbalazs wrote: > > > > On 2014/03/07 20:18:09, Ted C wrote: > > > > > +4 indent > > > > > > > > This is apparently 4, could you elaborate on how this should be written? > > > > > > +4 indent was meant to say it needs to be indented 4 more. > > > > > > Second lines of conditionals require two levels of indent (4 spaces per > > level): > > > > > > if (event.getAction() != KeyEvent.ACTION_DOWN > > > && event.getAction() != KeyEvent.ACTION_UP) { > > > ... > > > > I see. For the record I don't find this rule neither in the Android style > guide > > nor at http://www.chromium.org/developers/coding-style/java so probably this > > should be written somewhere. > > It's in the referenced android style guide: > http://source.android.com/source/code-style.html#use-spaces-for-indentation > > """ > We use 8 space indents for line wraps, including function calls and assignments. > """ Ah, ok, I missed it. Done. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:177: public static GamepadAdapter attach(long nativeDataFetcher) { On 2014/03/08 01:06:00, Ted C wrote: > On 2014/03/07 22:40:22, kbalazs wrote: > > On 2014/03/07 20:18:09, Ted C wrote: > > > why public? > > > > If I remember correctly the jni generator only allows it to be called by > native > > if it's public. I can double check. > > Ah, yeah the jni generator definitely allows you to call private methods. Now I remember, the presubmit complained about private + @CalledByNative. Now I used package private. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:181: return instance; On 2014/03/07 23:33:27, jdduke wrote: > Could we rename mIsFetched to something like |mHasListener| or |mDataRequested|. Done. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:254: private final SparseArray<Boolean> mButtons = new SparseArray<Boolean>(); On 2014/03/07 20:18:09, Ted C wrote: > Should be able to use SparseBooleanArray here. > > I would also probably call this mButtonPressedStates or something like that as > mButtons sounds like a collection of buttons. > > And along those lines, maybe mAxisValues above. I still wanted something like an associative array that can be indexed with the keycode and axis id constants. But I realized that all the axis id's and the key codes are small constants, less than 256. So now I just use two 256 size primitive arrays. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java (right): https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java:23: public abstract WebGamepadData map(SparseArray<Float> axes, SparseArray<Boolean> buttons); On 2014/03/07 20:18:09, Ted C wrote: > per my comment about object creation in the other file, you might want to have > this take a data object and write over it or have this class own one that will > get reused. > > Either way a a comment on this would be good. Done, I hold the object in the mapper now. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java:138: } On 2014/03/07 20:18:09, Ted C wrote: > This doesn't verify axes size is large enough. Might be good to have some > assertion in this file that the value you expect doesn't change in native and > not get updated here. > > And looking at it, this looks like it's only initialized to size 4, so this > would out of bounds fail if all these values were present. I think some sanity > checking on bounds is definitely something we should do here. Done. https://codereview.chromium.org/165483003/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/GamepadDataMapper.java:163: data.id = PS3_SIXAXIS_DEVICE_NAME + " (STANDARD_GAMEPAD)"; On 2014/03/07 20:18:09, Ted C wrote: > I would make " (STANDARD_GAMEPAD)" and "standard" constants as well. Done.
https://codereview.chromium.org/165483003/diff/80001/content/content.gyp File content/content.gyp (right): https://codereview.chromium.org/165483003/diff/80001/content/content.gyp#newc... content/content.gyp:504: 'target_name': 'content_gamepad_mapping', On 2014/03/12 00:17:36, kbalazs wrote: > I moved it to be a dependency for content_java. Do you think it is enough for > the webview? Also edited Android.mk No (but you should add it to content_java regardless) So android webview, when built inside the android tree, doesn't use gyp to build any java *source* files. It's built in one step by android_webview/Android.mk; look at the LOCAL_SRC_FILES variable. The issue is this is not a java source file, but that it's generating a source file. So we have to keep a list of these in android_webview/all_webview.gyp. The android_aosp bot will catch this. I'm only giving you a heads up. https://codereview.chromium.org/165483003/diff/120001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java (right): https://codereview.chromium.org/165483003/diff/120001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:61: if (mInputManager != null) { You'd need to null check everywhere mInputManager is used... Actually getSystemService is called all over the place without null checks, so if there really are issues, this won't be the only place. So ignore my comment about null checks and revert this. Sorry about that.
Thanks, definitely making progress. https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... File content/browser/gamepad/gamepad_platform_data_fetcher_android.h (right): https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:29: void RefreshDevice(JNIEnv* env, On 2014/03/12 00:17:36, kbalazs wrote: > On 2014/03/07 23:16:39, jdduke wrote: > > See .cc comments about how this function can be made static (and local to the > > .cc file). > > I think the jni generator only supports methods so I didn't change this. ViewConfiguration https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/android/vie... provides an example of how to use a static method local to a .cc file from Java. https://codereview.chromium.org/165483003/diff/80001/content/browser/gamepad/... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:43: blink::WebGamepads* data_; On 2014/03/12 00:17:36, kbalazs wrote: > On 2014/03/07 23:16:39, jdduke wrote: > > I'm not sure we need to hold on to this reference. See comments in the .cc > file. > > I kept it as I think it makes more sense than passing back and forth. Except, we're not passing "back and forth" as much as we're issuing a direct request to populate a given entity. This is a lot more clear than caching a pointer to this object and issuing a request at some future point in time to populate a field that is temporary. https://codereview.chromium.org/165483003/diff/120001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java (right): https://codereview.chromium.org/165483003/diff/120001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:167: if (!mDataRequested) return false; I'm a little concerned about the event flow when |mDataRequested| is false. In that case, we ignore all of the additional input events, but at the same time we haven't actually reset the data in |InputDeviceHandler|. That may contain some potentially stale state, which will linger until processing is resumed, e.g., if we pause capture while buttons are down, we might have miss the "button up" event, and when capture resumes we'll continue reporting that the button is down. https://codereview.chromium.org/165483003/diff/120001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:205: instance.mNativeDataFetcher = nativeDataFetcher; Hmm, we should not be caching the |nativeDataFetcher|. |getGamepadData| should take as an argument the native pointer to the |blink::WebGamepads* pads|, calling back into a static function to populate the |pads| object directly. In short, we shouldn't need to store *any* native pointers in this class. https://codereview.chromium.org/165483003/diff/120001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:218: mDataRequested = true; This variable is accessed on the UI thread, it should be protected. Are you setting it to true here to signal the start of collection, potentially missing the initial set of data on the first call? That's not a huge deal, I just want to be clear about the motivation. https://codereview.chromium.org/165483003/diff/120001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:221: if (mDeviceHandlers[i] == null) { Just so I understand this correctly, if |collectGamepadData| is called multiples times in succession, without additional input, the same set of data should be returned? In that case, |reportGamepadData| might be a little more clear to indicate that that the "collecting" is non-destructive and read-only.
Folks, I am quite surprised to see the reviewers moving over to this implementation. I had pushed the patch much earlier and have been addressing all your review comments without fail. kbalazs had earlier said that both the implementations were mostly similar and had agreed to go ahead with my implementation. He also gave some feedback on ( https://codereview.chromium.org/133943002/ ) , which I’ve addressed. I have incorporated the changes related with the gamepad mapping in my latest patch-set as requested by the reviewers. Although it got delayed because of the crash that comes on chromium Top-of-the-Tree(even without my patch), so I have opened a bug ( https://code.google.com/p/chromium/issues/detail?id=349834 ) to track that. My latest patch-set fixes this crash as well. Now I see that reviewers have moved over to this patch. Can someone explain the context/reasoning behind the shift? AFAIK, this doesn’t conform to accepted open sourcing practices. How can we work towards a common ground?
Hi Saurab, Let us take a few steps back please, there's no reason to assume we want conflict and/or we're not following "accepted open sourcing practices". I got asked to review kbalazs patch (see #5 and #6 above) I then added ted who brought jdduke and boliu. None of these reviewers, including myself, were listed as reviewers on your patch. This duplication is really unfortunate, but I think rather than questioning the practices we're following, it'd be more helpful to understand the reasons, which looks like: 1) your patch didn't have anyone from the android team, i.e., content/public/android/OWNERS 2) the android team was not aware of any other effort in this area. Given that this duplicated effort has already happened, here's my suggestion to best fix it: 1) You and kbalazs should decide which patch is more complete by now.. Remember: this is not a competition, but rather a collaboration to get the best code possible to our users. 2) Whatever your decision is, please ensure all android/ OWNERS, and all other reviewers, are fully aware. It is really difficult to manage such a large project like chrome crossing many organizations, but there's no reason at all to assume / imply malice on any of our members. I'd welcome anybody's thoughts on any other ideas on how to improve and move forward. Thanks, Marcus On 2014/03/12 12:01:43, SaurabhK wrote: > Folks, I am quite surprised to see the reviewers moving over to this > implementation. I had pushed the patch much earlier and have been addressing all > your review comments without fail. > > kbalazs had earlier said that both the implementations were mostly similar and > had agreed to go ahead with my implementation. He also gave some feedback on ( > https://codereview.chromium.org/133943002/ ) , which I’ve addressed. > I have incorporated the changes related with the gamepad mapping in my latest > patch-set as requested by the reviewers. Although it got delayed because of the > crash that comes on chromium Top-of-the-Tree(even without my patch), so I have > opened a bug ( https://code.google.com/p/chromium/issues/detail?id=349834 ) to > track that. My latest patch-set fixes this crash as well. > > Now I see that reviewers have moved over to this patch. Can someone explain the > context/reasoning behind the shift? AFAIK, this doesn’t conform to accepted open > sourcing practices. How can we work towards a common ground?
On 2014/03/12 14:21:29, bulach wrote: > Hi Saurab, > > Let us take a few steps back please, there's no reason to assume we want > conflict and/or we're not following "accepted open sourcing practices". > > I got asked to review kbalazs patch (see #5 and #6 above) > I then added ted who brought jdduke and boliu. > > None of these reviewers, including myself, were listed as reviewers on your > patch. > > This duplication is really unfortunate, but I think rather than questioning the > practices we're following, it'd be more helpful to understand the reasons, which > looks like: > 1) your patch didn't have anyone from the android team, i.e., > content/public/android/OWNERS > 2) the android team was not aware of any other effort in this area. > > Given that this duplicated effort has already happened, here's my suggestion to > best fix it: > 1) You and kbalazs should decide which patch is more complete by now.. > Remember: this is not a competition, but rather a collaboration to get the best > code possible to our users. > 2) Whatever your decision is, please ensure all android/ OWNERS, and all other > reviewers, are fully aware. > > It is really difficult to manage such a large project like chrome crossing many > organizations, but there's no reason at all to assume / imply malice on any of > our members. > > I'd welcome anybody's thoughts on any other ideas on how to improve and move > forward. > > Thanks, > Marcus > I don't want to add to much to this. This feature is important for us and we wanted to proceed asap. The other patch was not updated for about 2 weeks and @bajones reviewed both so I thought it's ok if I update mine. I will continue updating it.
On 2014/03/12 12:01:43, SaurabhK wrote: > Folks, I am quite surprised to see the reviewers moving over to this > implementation. I had pushed the patch much earlier and have been addressing all > your review comments without fail. > > kbalazs had earlier said that both the implementations were mostly similar and > had agreed to go ahead with my implementation. He also gave some feedback on ( > https://codereview.chromium.org/133943002/ ) , which I’ve addressed. > I have incorporated the changes related with the gamepad mapping in my latest > patch-set as requested by the reviewers. Although it got delayed because of the > crash that comes on chromium Top-of-the-Tree(even without my patch), so I have > opened a bug ( https://code.google.com/p/chromium/issues/detail?id=349834 ) to > track that. My latest patch-set fixes this crash as well. > > Now I see that reviewers have moved over to this patch. Can someone explain the > context/reasoning behind the shift? AFAIK, this doesn’t conform to accepted open > sourcing practices. How can we work towards a common ground? If there's been any confusion here it's my fault, and I apologize. I was told that there were currently two patches trying to get Gamepad support on Android, and so I looked at them both, left comments on both, and kbalaz@ replied within a day. I felt that there were some benefits to this version of the code, and so I suggested that we move forward with it. I apologize that I have not had the time to reply to your CL, since shortly after reviewing these patches my attention was diverted to address issues in other parts of Chrome. For my part this is a feature that I'm very interested in seeing land, regardless of who authored the patch. No slight was intended, kbalaz@ has simply been a little quicker to respond. I think that bulach@'s recommendations for moving forward are sound, but whichever patch is pushed forward at this point I want to make sure it includes the fix for the crash you identified (https://code.google.com/p/chromium/issues/detail?id=349834), which is probably also my fault. :) Very sorry again for the confusion!
Updated. https://codereview.chromium.org/165483003/diff/120001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java (right): https://codereview.chromium.org/165483003/diff/120001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:61: if (mInputManager != null) { On 2014/03/12 00:58:46, boliu wrote: > You'd need to null check everywhere mInputManager is used... > > Actually getSystemService is called all over the place without null checks, so > if there really are issues, this won't be the only place. So ignore my comment > about null checks and revert this. Sorry about that. Done. https://codereview.chromium.org/165483003/diff/120001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:167: if (!mDataRequested) return false; On 2014/03/12 01:06:19, jdduke wrote: > I'm a little concerned about the event flow when |mDataRequested| is false. In > that case, we ignore all of the additional input events, but at the same time we > haven't actually reset the data in |InputDeviceHandler|. That may contain some > potentially stale state, which will linger until processing is resumed, e.g., if > we pause capture while buttons are down, we might have miss the "button up" > event, and when capture resumes we'll continue reporting that the button is > down. Good point, trying to handle this now. https://codereview.chromium.org/165483003/diff/120001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:205: instance.mNativeDataFetcher = nativeDataFetcher; On 2014/03/12 01:06:19, jdduke wrote: > Hmm, we should not be caching the |nativeDataFetcher|. |getGamepadData| should > take as an argument the native pointer to the |blink::WebGamepads* pads|, > calling back into a static function to populate the |pads| object directly. In > short, we shouldn't need to store *any* native pointers in this class. Done. https://codereview.chromium.org/165483003/diff/120001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:218: mDataRequested = true; On 2014/03/12 01:06:19, jdduke wrote: > This variable is accessed on the UI thread, it should be protected. Are you > setting it to true here to signal the start of collection, potentially missing > the initial set of data on the first call? That's not a huge deal, I just want > to be clear about the motivation. There are in fact a race with the UI thread about this variable but I cannot think about a way that it could cause any problems We just delay the data on more frame in worst case. Now that I added clearInput() to avoid the potentional anomalies you pointed out it got trickier but I think it's still ok without locking. I added a long comment about that. The motivation of setDataRequested is just to add an implementation for PlatformDataFetcher::PauseHint. It is supposed to be an optimization. It is probably not that useful. We can early return instead of processing the events but the processing is fast anyway. However I think there is another reason it might be necessary: it's probably not ok to consume the events and stop further dispatching when the web page doesn't observe gamepad data. https://codereview.chromium.org/165483003/diff/120001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java:221: if (mDeviceHandlers[i] == null) { On 2014/03/12 01:06:19, jdduke wrote: > Just so I understand this correctly, if |collectGamepadData| is called multiples > times in succession, without additional input, the same set of data should be > returned? In that case, |reportGamepadData| might be a little more clear to > indicate that that the "collecting" is non-destructive and read-only. Done.
>> I don't want to add to much to this. This feature is important for us and we >> wanted to proceed asap. The other patch was not updated for about 2 weeks and >> @bajones reviewed both so I thought it's ok if I update mine. I will continue >> updating it. This patch is also important for NVIDIA and we also want this patch to land asap that’s why we have open-sourced this much earlier. There has never been a delay of 2 weeks and I have constantly been updating the Issue. The crash was a showstopper for me to address reviewer comments and the bug that I had filed for the crash never got addressed which further delayed the upload of new patch-set. I also don’t want to add too much here except that kbalazs has agreed earlier that we will go ahead with my implementation. And considering the fact that I have open-sourced and I have interacted and with reviewers much earlier, I believe reviewers should review my patch. |
