Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
133943002:
Gamepad API support for chrome on android (Closed)
|
Created:
6 years, 11 months ago by SaurabhK Modified:
6 years, 2 months ago Reviewers:
jdduke (slow), Yaron, bajones, xwang, bulach, kbalazs, boliu, scottmg, Ted C, nyquist, Feng Qian, Tom Sepez, mnaganov (inactive), darin (slow to review), brettw, aelias_OOO_until_Jul13 Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionGamepad API support for chrome on android
This change adds code to get gamepad data from java objects and
provide these updates to the GamepadProvider which then writes to
GamepadHardwareBuffer which in turn is read by SharedMemoryReader and
later returned by JS to the web page.
This also adds new framework classes/methods required for Gamepad API
support. Frameworks changes are responsible for :
- Identifying gamepad devices and their capabilities.
- Managing connected gamepad devices
- Map the connected gamepad devices to standard Gamepad format.
- Keeping gamepads axes/buttons data up-to-date and returning it to
native whenever requested. In android we cannot get gamepad data
directly from sources, so framework is modified to capture
gamepad key and motion events and extract gamepad data from these
events.
* Class GamepadPlatformDataFetcherAndroid : Android specific
implementation of gamepad data fetcher.
- Responsible for communication with java class and accessing gamepad data.
- It adds methods for communication with singleton java GamepadList class
to get gamepads data.
* Class ContentViewCore : Manages gamepad list and notifies of new
key/motion event for gamepads.
* Class GamepadList : A new class to manage connected gamepad devices
* Class GamepadDevice : A new class to manage information related to
each gamepad device.
* Class GamepadMappings : This class is responsible for mapping of
known gamepads to the standard gamepad.
This change enables gamepad API by default.
Adds support for parsing float array return type in JNI generator.
NVIDIA Shield and XBox360 gamepads are mapped to the standard gamepad
BUG=330094
TEST=http://www.html5rocks.com/en/tutorials/doodles/gamepad/gamepad-tester/tester.html
R=tsepez@chromium.org
R=darin@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270620
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274212
Patch Set 1 #
Total comments: 26
Patch Set 2 : Gamepad API support for chrome on android (Work In Progress) #Patch Set 3 : Gamepad API support for chrome on android (Work In Progress) #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 32
Patch Set 7 : #Patch Set 8 : #
Total comments: 32
Patch Set 9 : #
Total comments: 12
Patch Set 10 : #Patch Set 11 : #
Total comments: 10
Patch Set 12 : #
Total comments: 8
Patch Set 13 : Address review comments #Patch Set 14 : #
Total comments: 43
Patch Set 15 : #
Total comments: 30
Patch Set 16 : #Patch Set 17 : #
Total comments: 22
Patch Set 18 : Uploading forgotten things in previous patch #
Total comments: 48
Patch Set 19 : #
Total comments: 47
Patch Set 20 : #
Total comments: 4
Patch Set 21 : Fixed final Nits #
Total comments: 4
Patch Set 22 : #Patch Set 23 : Rebase #Patch Set 24 : #Patch Set 25 : Fix android_clang_dbg issues #Patch Set 26 : Rebase #
Total comments: 1
Patch Set 27 : #Patch Set 28 : Rebase #
Total comments: 1
Patch Set 29 : Fix for GamepadList on ICS #Patch Set 30 : #
Total comments: 6
Patch Set 31 : #Patch Set 32 : #
Total comments: 1
Messages
Total messages: 152 (0 generated)
+scottmg, who was the original author of chromium's gamepad support. he should probably take a look at this CL as well.
messages LGTM. https://codereview.chromium.org/133943002/diff/1/content/common/gamepad_messa... File content/common/gamepad_messages.h (right): https://codereview.chromium.org/133943002/diff/1/content/common/gamepad_messa... content/common/gamepad_messages.h:27: // Asks the browser process to pause/resume polling .The number of resume Nit: space before period. Nit: the number of resume messages should match the number of pause messages. Nit: what happens if its already paused and a new pause comes in? do they nest? You might want to explain. https://codereview.chromium.org/133943002/diff/1/content/common/gamepad_messa... content/common/gamepad_messages.h:33: // Sets the last gamepad data access timestamp. It helps identify gamepad nit: sets it to the current time? I'm not sure I understand you second sentence.
https://codereview.chromium.org/133943002/diff/1/content/common/gamepad_messa... File content/common/gamepad_messages.h (right): https://codereview.chromium.org/133943002/diff/1/content/common/gamepad_messa... content/common/gamepad_messages.h:27: // Asks the browser process to pause/resume polling .The number of resume On 2014/01/10 22:57:39, Tom Sepez wrote: > Nit: space before period. > Nit: the number of resume messages should match the number of pause messages. > Nit: what happens if its already paused and a new pause comes in? do they nest? > You might want to explain. >Correcting space before period in next patch-set. >These messages don't nest, I am checking the state of gamepad service before these messages try to change its state. Hence removing line "The number of resume should match the number of pause." from the comment in next patch-set. https://codereview.chromium.org/133943002/diff/1/content/common/gamepad_messa... content/common/gamepad_messages.h:33: // Sets the last gamepad data access timestamp. It helps identify gamepad On 2014/01/10 22:57:39, Tom Sepez wrote: > nit: sets it to the current time? I'm not sure I understand you second sentence. Yes, it sets the current time. It helps to identify if gamepad data is being accessed or not. If gamepad data is not getting accessed then we can put poll thread to pause state. Cases like:- Tab switch and browser minimize. Updating description in next patch-set.
Thanks for taking this on! A few brief questions below before I look at the rest. In general I would prefer to either reuse start/stop (changing their semantics slightly if we need to), or include the pause/resume in all platforms rather than adding a bunch of OS_ANDROIDs. https://codereview.chromium.org/133943002/diff/1/android_webview/java/src/org... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/133943002/diff/1/android_webview/java/src/org... android_webview/java/src/org/chromium/android_webview/AwContents.java:2: // Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved. I think copyright is supposed to be assigned to Chromium. (For example, I'm currently employed by Google, but there's no Google copyrights in Chromium-authored code.) https://codereview.chromium.org/133943002/diff/1/content/browser/android/game... File content/browser/android/gamepad_reader_impl.h (right): https://codereview.chromium.org/133943002/diff/1/content/browser/android/game... content/browser/android/gamepad_reader_impl.h:61: class GamepadsReader{ There's a lot of code that doesn't follow the style guide in this patch (spacing, indent, method case, comment format, etc). Did you see presubmit warnings when uploading? https://codereview.chromium.org/133943002/diff/1/content/renderer/gamepad_sha... File content/renderer/gamepad_shared_memory_reader.h (right): https://codereview.chromium.org/133943002/diff/1/content/renderer/gamepad_sha... content/renderer/gamepad_shared_memory_reader.h:22: void ResumeGamepads(); Could you explain why the renderer is calling these? What if a renderer crashes and the Pause/Resumes become unbalanced?
Thanks for the patch! Too much code style issues in your code, the pylint.py tool in depot_tools may help you identify code style issues locally. https://codereview.chromium.org/133943002/diff/1/content/browser/android/game... File content/browser/android/gamepad_reader_impl.cc (right): https://codereview.chromium.org/133943002/diff/1/content/browser/android/game... content/browser/android/gamepad_reader_impl.cc:83: return 0; What is javaGamepadListObject for? Is that to check whether the gamepads have been interact with? https://codereview.chromium.org/133943002/diff/1/content/browser/gamepad/game... File content/browser/gamepad/gamepad_platform_data_fetcher_android.h (right): https://codereview.chromium.org/133943002/diff/1/content/browser/gamepad/game... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:53: // polling_thread_. Where the method is called? https://codereview.chromium.org/133943002/diff/1/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/133943002/diff/1/content/content_browser.gypi... content/content_browser.gypi:269: 'browser/android/gamepad_reader_impl.cc', + 'browser/android/gamepad_reader_impl.h' https://codereview.chromium.org/133943002/diff/1/content/content_browser.gypi... content/content_browser.gypi:557: 'browser/gamepad/gamepad_platform_data_fetcher_android.cc', + 'browser/gamepad/gamepad_platform_data_fetcher_android.h', https://codereview.chromium.org/133943002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:129: // Following values should be changed if the gamepad layout changes Will it better to separate the device specific stuff to another file/classes? Such as DeviceMappings.java. https://codereview.chromium.org/133943002/diff/1/content/renderer/renderer_we... File content/renderer/renderer_webkitplatformsupport_impl.h (right): https://codereview.chromium.org/133943002/diff/1/content/renderer/renderer_we... content/renderer/renderer_webkitplatformsupport_impl.h:129: virtual void resumeGamepads(); More code in blink to call this?
https://codereview.chromium.org/133943002/diff/1/android_webview/java/src/org... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/133943002/diff/1/android_webview/java/src/org... android_webview/java/src/org/chromium/android_webview/AwContents.java:2: // Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved. On 2014/01/13 19:42:43, scottmg wrote: > I think copyright is supposed to be assigned to Chromium. (For example, I'm > currently employed by Google, but there's no Google copyrights in > Chromium-authored code.) Will change it in new patch https://codereview.chromium.org/133943002/diff/1/content/browser/android/game... File content/browser/android/gamepad_reader_impl.cc (right): https://codereview.chromium.org/133943002/diff/1/content/browser/android/game... content/browser/android/gamepad_reader_impl.cc:83: return 0; On 2014/01/14 03:03:22, xwang wrote: > What is javaGamepadListObject for? Is that to check whether the gamepads have > been interact with? Yes, it is to check if gamepad device has been interacted with. https://codereview.chromium.org/133943002/diff/1/content/browser/android/game... File content/browser/android/gamepad_reader_impl.h (right): https://codereview.chromium.org/133943002/diff/1/content/browser/android/game... content/browser/android/gamepad_reader_impl.h:61: class GamepadsReader{ On 2014/01/13 19:42:43, scottmg wrote: > There's a lot of code that doesn't follow the style guide in this patch > (spacing, indent, method case, comment format, etc). Did you see presubmit > warnings when uploading? WIll update in next patchset. https://codereview.chromium.org/133943002/diff/1/content/browser/gamepad/game... File content/browser/gamepad/gamepad_platform_data_fetcher_android.h (right): https://codereview.chromium.org/133943002/diff/1/content/browser/gamepad/game... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:53: // polling_thread_. On 2014/01/14 03:03:22, xwang wrote: > Where the method is called? It is called from GamepadProvider::SendPauseHint() https://codereview.chromium.org/133943002/diff/1/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/133943002/diff/1/content/content_browser.gypi... content/content_browser.gypi:269: 'browser/android/gamepad_reader_impl.cc', On 2014/01/14 03:03:22, xwang wrote: > + 'browser/android/gamepad_reader_impl.h' Done https://codereview.chromium.org/133943002/diff/1/content/content_browser.gypi... content/content_browser.gypi:557: 'browser/gamepad/gamepad_platform_data_fetcher_android.cc', On 2014/01/14 03:03:22, xwang wrote: > + 'browser/gamepad/gamepad_platform_data_fetcher_android.h', Done https://codereview.chromium.org/133943002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:129: // Following values should be changed if the gamepad layout changes On 2014/01/14 03:03:22, xwang wrote: > Will it better to separate the device specific stuff to another file/classes? > Such as DeviceMappings.java. Will add another class in next patch set. https://codereview.chromium.org/133943002/diff/1/content/renderer/gamepad_sha... File content/renderer/gamepad_shared_memory_reader.h (right): https://codereview.chromium.org/133943002/diff/1/content/renderer/gamepad_sha... content/renderer/gamepad_shared_memory_reader.h:22: void ResumeGamepads(); On 2014/01/13 19:42:43, scottmg wrote: > Could you explain why the renderer is calling these? What if a renderer crashes > and the Pause/Resumes become unbalanced? According to my observation polling thread which gets started in the constructor of GamepadSharedMemoryReader never stops (even if i switch/close the tab that accesses gamepad data or minimize the browser application). In my implementation I want to resume and pause polling thread whenever gamepad data is accessed and released. And I make sure the state of polling thread in NavigatorGamepad's constructor and destructor. Hence these calls are from NavigatorGamepad. Also i am keeping a timeout in GamepadProvider which will put polling thread to pause state whenever gamepad access stops. SO coming back to the question of renderer crash, there can be two states (resume/pause) of renderer at time of crash. 1. Paused In this case whenever new webpage is loaded that accesses gamepad data, NavigatorGamepad will make sure that the thread has resumed. 2. Resumed In this case the timeout in GamepadProvider will put the polling thread to pause as the gamepad access has been stopped. https://codereview.chromium.org/133943002/diff/1/content/renderer/renderer_we... File content/renderer/renderer_webkitplatformsupport_impl.h (right): https://codereview.chromium.org/133943002/diff/1/content/renderer/renderer_we... content/renderer/renderer_webkitplatformsupport_impl.h:129: virtual void resumeGamepads(); On 2014/01/14 03:03:22, xwang wrote: > More code in blink to call this? I am working on blink code, I will upload that in couple of days.
https://codereview.chromium.org/133943002/diff/1/content/browser/android/game... File content/browser/android/gamepad_reader_impl.cc (right): https://codereview.chromium.org/133943002/diff/1/content/browser/android/game... content/browser/android/gamepad_reader_impl.cc:83: return 0; Why not directly call haveDevicesBeenInteractedWith() to do the check? It's confusing to use javaGamepadListObject for this. On 2014/01/15 16:57:08, SaurabhK wrote: > On 2014/01/14 03:03:22, xwang wrote: > > What is javaGamepadListObject for? Is that to check whether the gamepads have > > been interact with? > > Yes, it is to check if gamepad device has been interacted with. https://codereview.chromium.org/133943002/diff/1/content/browser/gamepad/game... File content/browser/gamepad/gamepad_platform_data_fetcher_android.h (right): https://codereview.chromium.org/133943002/diff/1/content/browser/gamepad/game... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:54: virtual void PauseHint(bool ispaused); Well, it should be "virtual void PauseHint(bool ispaused) OVERRIDE;"
Also added dependent patch in blink https://codereview.chromium.org/148353002/ https://codereview.chromium.org/133943002/diff/1/content/browser/android/game... File content/browser/android/gamepad_reader_impl.cc (right): https://codereview.chromium.org/133943002/diff/1/content/browser/android/game... content/browser/android/gamepad_reader_impl.cc:83: return 0; Now i am directly calling haveDevicesBeenInteractedWith() in latest patchset https://codereview.chromium.org/133943002/diff/1/content/browser/gamepad/game... File content/browser/gamepad/gamepad_platform_data_fetcher_android.h (right): https://codereview.chromium.org/133943002/diff/1/content/browser/gamepad/game... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:54: virtual void PauseHint(bool ispaused); Updated it in latest patchset.
>> A few brief questions below before I look at the rest. In general I would prefer to either reuse start/stop (changing their semantics slightly if we need to), or include the pause/resume in all platforms rather than adding a bunch of OS_ANDROIDs. I am working on including pause/resume for all the platforms
Uploaded final rebased patch. It needs to evaluated along with https://codereview.chromium.org/148353002/
(I haven't looked at the Java code, lacking much knowledge anyway.) I don't like the timer or the pause/resume still, I still don't understand why they need to be different than the existing Start/Stop. https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... File content/browser/gamepad/gamepad_platform_data_fetcher.cc (right): https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher.cc:10: #if !defined(OS_ANDROID) nit; merge with the others https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:11: #if defined(OS_ANDROID) Remove this, it shouldn't be compiled on other platforms. https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:19: blink::WebGamepads* pads, bool) { nit; this should be +4. git cl format or clang-format? https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:20: GamepadsReader* m_gamepadsReader = GamepadsReader::GetInstance(); No m_, and chromium C++ code should use hacker_style for locals (also ifDeviceConnected below) https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:37: // queried using class android.hardware.input.InputManager. Is it possible to correlate to a /dev/ or something? Would that generally be readable? https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:40: base::TruncateUTF8ToByteSize(string1, WebGamepad::idLengthCap - 1, Why truncate the UTF8 rather than the UTF16? Is this really UTF8 in the first place or ASCII? https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:47: pads->items[i].timestamp = m_gamepadsReader->GetDeviceTimestamp(i); Is this the only place this is used? If so, it could just be a monotonically increasing int. https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:50: // state of axeses. nit; "axes" https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:51: std::vector<float> axes = m_gamepadsReader->GetDeviceAxes(i); So many copies from kernel to content. :( https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:67: // JNI are larger than permitted value. Is this really what we want? It would probably be better just to return the first axesLengthCap and buttonsLengthCap of them. https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:69: pads->items[i].buttonsLength > WebGamepad::buttonsLengthCap) { nit; indent wrong https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:92: GamepadPlatformDataFetcherAndroid::~GamepadPlatformDataFetcherAndroid() { Move this up with the constructor to match the header order. https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... File content/browser/gamepad/gamepad_platform_data_fetcher_android.h (right): https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:17: #if defined(OS_ANDROID) Remove this, it shouldn't be included elsewhere. https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:33: bool devices_changed_hint) OVERRIDE; nit; align at (. https://codereview.chromium.org/133943002/diff/510001/content/browser/rendere... File content/browser/renderer_host/gamepad_browser_message_filter.cc (right): https://codereview.chromium.org/133943002/diff/510001/content/browser/rendere... content/browser/renderer_host/gamepad_browser_message_filter.cc:5: #include "base/time/time.h" This should go below the include of gamepad_browser_message_filter.h with the others after a blank line. https://codereview.chromium.org/133943002/diff/510001/content/browser/rendere... content/browser/renderer_host/gamepad_browser_message_filter.cc:34: OnGamepadUpdateTimestamp) nit; align at (. https://codereview.chromium.org/133943002/diff/510001/content/browser/rendere... content/browser/renderer_host/gamepad_browser_message_filter.cc:91: base::Time::NowFromSystemTime()); Ah, I see. Why can't we detect the situations above, and call OnGamepadStopPolling instead then of Pause and Resume?
(Sorry for the long turnaround by the way, it's a large amount of code to try to understand. If you can find smaller pieces that make sense separately it would probably speed things up, but I'll try to review quicker on the next patch.)
https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... File content/browser/gamepad/gamepad_platform_data_fetcher.cc (right): https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher.cc:10: #if !defined(OS_ANDROID) Done https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:11: #if defined(OS_ANDROID) On 2014/02/06 21:17:07, scottmg wrote: Done. https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:19: blink::WebGamepads* pads, bool) { On 2014/02/06 21:17:07, scottmg wrote: Done. https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:20: GamepadsReader* m_gamepadsReader = GamepadsReader::GetInstance(); On 2014/02/06 21:17:07, scottmg wrote: Done. https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:37: // queried using class android.hardware.input.InputManager. On 2014/02/06 21:17:07, scottmg wrote: /dev/ is not readable for android. We instead depend on framework classes MotionRanges and KeyCharacterMap and we extract gamepad data from dispatchKeyEvent and dispatchMotionEvent callbacks. 1. We extract the info of all the device connected and identify gamepad devices out of these. 2. Get the number of axes by counting axes manually. 3. Get the number of buttons by quering each keycode and then the connected device tells us if that keycode can be produced. https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:40: base::TruncateUTF8ToByteSize(string1, WebGamepad::idLengthCap - 1, On 2014/02/06 21:17:07, scottmg wrote: Yes, it is UTF8 that gets returned from GamepadReader. I have tried to keep it parallel to linux file. https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:47: pads->items[i].timestamp = m_gamepadsReader->GetDeviceTimestamp(i); On 2014/02/06 21:17:07, scottmg wrote: The timestamp is not monotonically increasing. It is the system time when the last update has come from the connected gamepad device. i.e. When a key press event of Motion update event has been recieved from the device. https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:50: // state of axeses. On 2014/02/06 21:17:07, scottmg wrote: Done. https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:51: std::vector<float> axes = m_gamepadsReader->GetDeviceAxes(i); On 2014/02/06 21:17:07, scottmg wrote: We are not reading it from the kernel, instead we are reading it from a java object (GamepadList). GamepadList has already maintained the state of connected GamepadDevices. GamepadList is updated only when if there is any change in the state of connected gamepad devices(KeyEvent or MotionEvent comes from one of the connected devices). It is the responsiblity of GamepadList to provide the gamepad data in Standard gamepad format for the known devices. https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:67: // JNI are larger than permitted value. On 2014/02/06 21:17:07, scottmg wrote: Done. https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:69: pads->items[i].buttonsLength > WebGamepad::buttonsLengthCap) { On 2014/02/06 21:17:07, scottmg wrote: Done. https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:92: GamepadPlatformDataFetcherAndroid::~GamepadPlatformDataFetcherAndroid() { On 2014/02/06 21:17:07, scottmg wrote: Done. https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... File content/browser/gamepad/gamepad_platform_data_fetcher_android.h (right): https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:17: #if defined(OS_ANDROID) On 2014/02/06 21:17:07, scottmg wrote: Removed https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:33: bool devices_changed_hint) OVERRIDE; On 2014/02/06 21:17:07, scottmg wrote: Done https://codereview.chromium.org/133943002/diff/510001/content/browser/rendere... File content/browser/renderer_host/gamepad_browser_message_filter.cc (right): https://codereview.chromium.org/133943002/diff/510001/content/browser/rendere... content/browser/renderer_host/gamepad_browser_message_filter.cc:91: base::Time::NowFromSystemTime()); On 2014/02/06 21:17:07, scottmg wrote: Regarding the Pause/Resume and the timer, we have used it on our android browser because we have two implementation that should work alternately. 1. Gamepad API 2. Browser controller support Whenever any webpage accesses gamepad data we have to make sure that KeyEvent and MotionEvent from the gamepad device should not get consumed and are transfered to JS of the webpage. And whenever webpage releases gamepad data, the device controller should be used for scroll and navigational purposes. Therefore we want to make sure that GamepadList java object is notified of gamepad data access/release. I can try to use StopPoll and StartPoll for the same purpose instead of Pause/Resume. As currently start and stop is related with the lifetime of SharedMemoryReader object, so for the usecase of tab switch i have implemented Timer. I think that i can start with its implementation as a seperate patch. And in this patch, it will be better to go ahead with just the android platform specific fetcher implementation.
Could you upload again please? Some chunks are missing (this is some retvield issue that happens sometimes).
On 2014/02/10 12:50:47, SaurabhK wrote: > https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... > File content/browser/gamepad/gamepad_platform_data_fetcher.cc (right): > > https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... > content/browser/gamepad/gamepad_platform_data_fetcher.cc:10: #if > !defined(OS_ANDROID) > Done > > https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... > File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): > > https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... > content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:11: #if > defined(OS_ANDROID) > On 2014/02/06 21:17:07, scottmg wrote: > > Done. > > https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... > content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:19: > blink::WebGamepads* pads, bool) { > On 2014/02/06 21:17:07, scottmg wrote: > > Done. > > https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... > content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:20: > GamepadsReader* m_gamepadsReader = GamepadsReader::GetInstance(); > On 2014/02/06 21:17:07, scottmg wrote: > > Done. > > https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... > content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:37: // queried > using class android.hardware.input.InputManager. > On 2014/02/06 21:17:07, scottmg wrote: > > /dev/ is not readable for android. We instead depend on framework classes > MotionRanges and KeyCharacterMap and we extract gamepad data from > dispatchKeyEvent and dispatchMotionEvent callbacks. > 1. We extract the info of all the device connected and identify gamepad devices > out of these. > 2. Get the number of axes by counting axes manually. > 3. Get the number of buttons by quering each keycode and then the connected > device tells us if that keycode can be produced. > > https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... > content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:40: > base::TruncateUTF8ToByteSize(string1, WebGamepad::idLengthCap - 1, > On 2014/02/06 21:17:07, scottmg wrote: > > Yes, it is UTF8 that gets returned from GamepadReader. > I have tried to keep it parallel to linux file. > > https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... > content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:47: > pads->items[i].timestamp = m_gamepadsReader->GetDeviceTimestamp(i); > On 2014/02/06 21:17:07, scottmg wrote: > > The timestamp is not monotonically increasing. It is the system time when the > last update has come from the connected gamepad device. > i.e. When a key press event of Motion update event has been recieved from the > device. > > https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... > content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:50: // state of > axeses. > On 2014/02/06 21:17:07, scottmg wrote: > > Done. > > https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... > content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:51: > std::vector<float> axes = m_gamepadsReader->GetDeviceAxes(i); > On 2014/02/06 21:17:07, scottmg wrote: > > We are not reading it from the kernel, instead we are reading it from a java > object (GamepadList). > GamepadList has already maintained the state of connected GamepadDevices. > GamepadList is updated only when if there is any change in the state of > connected gamepad devices(KeyEvent or MotionEvent comes from one of the > connected devices). > It is the responsiblity of GamepadList to provide the gamepad data in Standard > gamepad format for the known devices. > > https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... > content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:67: // JNI are > larger than permitted value. > On 2014/02/06 21:17:07, scottmg wrote: > > Done. > > https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... > content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:69: > pads->items[i].buttonsLength > WebGamepad::buttonsLengthCap) { > On 2014/02/06 21:17:07, scottmg wrote: > > Done. > > https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... > content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:92: > GamepadPlatformDataFetcherAndroid::~GamepadPlatformDataFetcherAndroid() { > On 2014/02/06 21:17:07, scottmg wrote: > > Done. > > https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... > File content/browser/gamepad/gamepad_platform_data_fetcher_android.h (right): > > https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... > content/browser/gamepad/gamepad_platform_data_fetcher_android.h:17: #if > defined(OS_ANDROID) > On 2014/02/06 21:17:07, scottmg wrote: > > Removed > > https://codereview.chromium.org/133943002/diff/510001/content/browser/gamepad... > content/browser/gamepad/gamepad_platform_data_fetcher_android.h:33: bool > devices_changed_hint) OVERRIDE; > On 2014/02/06 21:17:07, scottmg wrote: > > Done > > https://codereview.chromium.org/133943002/diff/510001/content/browser/rendere... > File content/browser/renderer_host/gamepad_browser_message_filter.cc (right): > > https://codereview.chromium.org/133943002/diff/510001/content/browser/rendere... > content/browser/renderer_host/gamepad_browser_message_filter.cc:91: > base::Time::NowFromSystemTime()); > On 2014/02/06 21:17:07, scottmg wrote: > > Regarding the Pause/Resume and the timer, we have used it on our android browser > because we have two implementation that should work alternately. > 1. Gamepad API > 2. Browser controller support > Whenever any webpage accesses gamepad data we have to make sure that KeyEvent > and MotionEvent from the gamepad device should not get consumed and are > transfered to JS of the webpage. > And whenever webpage releases gamepad data, the device controller should be used > for scroll and navigational purposes. > > Therefore we want to make sure that GamepadList java object is notified of > gamepad data access/release. > I can try to use StopPoll and StartPoll for the same purpose instead of > Pause/Resume. As currently start and stop is related with the lifetime of > SharedMemoryReader object, so for the usecase of tab switch i have implemented > Timer. > I think that i can start with its implementation as a seperate patch. > And in this patch, it will be better to go ahead with just the android platform > specific fetcher implementation. Agree with you to separate the patch into two parts. We could enable the feature on Android firstly, then leave the second part as an enhancement for further review.
On 2014/02/17 20:39:32, kbalazs wrote: > Could you upload again please? Some chunks are missing (this is some retvield > issue that happens sometimes). Glad to hear more inputs to move this feature on Android into a better shape. I just had a look at your patch, not deeply, I prefer the way to use less jni binding. We could disscuss more after @sausrbhK separate the patch.
Submitting the final patch without pause/resume implementation. I will take that up after this patch gets merged. This patch now make only single JNI call to read all the gamepad data instead of multiple JNI calls in previous patch.
Left some comments. Take it as informal since I'm not a reviewer :) https://codereview.chromium.org/133943002/diff/1290001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/133943002/diff/1290001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:1177: mContentViewCore.onPause(); I'm not sure we need these. But if we do we will need it for content shell and test shell as well. I'm not saying we need to fix it together with this patch though. Btw ContentView/ContentViewCore onPause/onResume was intentinally removed at some point (I don't have the rev number now but I was looking at it). The idea was that onShow and onHide should be enough. I'm not sure this was a good idea, currently onShow/onHide does not called correctly for content/test shell, but probably we don't want to reintroduce them (although for me that's fine). Maybe we should skip this part for now? https://codereview.chromium.org/133943002/diff/1290001/content/browser/androi... File content/browser/android/gamepad_reader_impl.cc (right): https://codereview.chromium.org/133943002/diff/1290001/content/browser/androi... content/browser/android/gamepad_reader_impl.cc:52: gamepads_data_->length = devicecount; I don't think we need this, other platforms just initialize it to 4 and set the connected flags appropriately. https://codereview.chromium.org/133943002/diff/1290001/content/browser/androi... content/browser/android/gamepad_reader_impl.cc:83: base::string16 tmp16 = base::UTF8ToUTF16(device_name); Can we get rid of converting back and forth (utf16->utf8->utf16)? Both WebUChar and jchar is 2 byte so I think it's better to just memcpy the java string to to pad.id. https://codereview.chromium.org/133943002/diff/1290001/content/browser/androi... content/browser/android/gamepad_reader_impl.cc:100: for (unsigned int j = 0; j < pad.axesLength; j++) This can also be done with only one copy instead of two. I would use the jni api's directly to grab the data and memcpy it. Of course one can argue that this is more error prone, but in my opinion it's worth it. https://codereview.chromium.org/133943002/diff/1290001/content/browser/androi... content/browser/android/gamepad_reader_impl.cc:108: base::android::JavaFloatArrayToFloatVector(env, jbuttons, &buttons); Ditto. https://codereview.chromium.org/133943002/diff/1290001/content/browser/androi... content/browser/android/gamepad_reader_impl.cc:118: void GamepadsReader::NotifyForGamepadsAccess(bool isaccesspaused) { is_access_paused? Or just access_paused? https://codereview.chromium.org/133943002/diff/1290001/content/child/runtime_... File content/child/runtime_features.cc (right): https://codereview.chromium.org/133943002/diff/1290001/content/child/runtime_... content/child/runtime_features.cc:35: WebRuntimeFeatures::enableGamepad(true); No need for turning it on, you can simply remove this, by default it is turned on. https://codereview.chromium.org/133943002/diff/1290001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1835: if (mGamepadList.isGamepadAccessed() && mGamepadList.isGamepadEvent(event)) { I would move both checks to the callee and remove these functions. It's better if only GamepadList has to know about it's state. https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1883: if (mGamepadList.isGamepadAccessed() && mGamepadList.isGamepadEvent(event)) { Ditto. https://codereview.chromium.org/133943002/diff/1290001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:114: mGamepadType = GamepadMappings.GamepadType.Shield; I think it would be an improvement to represent everything that's device specific in another class. This can be an abstract class with a factory method, implemented for each device type (and probably a generic or non-mapping one). https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:167: values[i++] = (Float)itr.next(); For me it looks a bit fragile that you depend on the ordering of the keys stored in the hashmap. For example what happens if the map doesn't have every value from standardGamepadKeyCodes? For me it looks like it would be better to use the keycodes directly for the mapping. https://codereview.chromium.org/133943002/diff/1290001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:26: private boolean IS_GAMEPAD_ACCESSED = false; Shouldn't be named with initials since it's not a constant :) https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:132: public int getCount() { This doesn't look very useful to me, I would just use the constant directly. https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:139: mHaveDevicesBeenInteractedWith = true; I think it's not strictly necessary to maintain this. The renderer side shared memory reader (GamepadSharedMemoryReader) checks this to prevent fingerprinting. https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:160: // Check if any registered gamepad is disconnected when webview was in pause state. Can you word it more generally? This feature is not just for the webview so we shouldn't refer to it exclusively. https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:161: for (int i = 0; i < getCount(); ++i) { Isn't it enough to null out the array and enumerate again? You could move it to a common method in this case and call it from the constructor. Or you want to keep the original index for devices we already saw? https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:224: if ((sources & InputDevice.SOURCE_JOYSTICK) == InputDevice.SOURCE_JOYSTICK ) { Shouln't it be '(sources & InputDevice.SOURCE_JOYSTICK) != 0'? I'm not sure but the documentation says an input device can have multiple inputs and probably we want it to be true for everything that provides joystick input. https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:242: if ((event.getSource() & InputDevice.SOURCE_JOYSTICK) == InputDevice.SOURCE_JOYSTICK) { Ditto about the condition. https://codereview.chromium.org/133943002/diff/1290001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java (right): https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:99: values[StandardAxes.AXIS_0] = rawAxes[0]; Like I said before I think it would be probably better to use the constants defined by KeyEvent and MotionEvent here. I did that by pushing the values to a map (SparseArray) that use keycodes and axis id's as keys and I pass the 2 maps to the mapper. With this we could also write a fallback mapper if we don't have the device specific code for the actual device. The Android API is generic enough that this could probably work in most cases.
Partial review, I will continue then. https://codereview.chromium.org/133943002/diff/1290001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/133943002/diff/1290001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:1177: mContentViewCore.onPause(); I also questioned whether we need to reproduce onPause/Resume for GamePad, any obvious benefit on this? Due to performance? Agree to skip this part as we have another CL to discuss the issue. On 2014/02/20 00:48:24, kbalazs wrote: > I'm not sure we need these. But if we do we will need it for content shell and > test shell as well. I'm not saying we need to fix it together with this patch > though. Btw ContentView/ContentViewCore onPause/onResume was intentinally > removed at some point (I don't have the rev number now but I was looking at it). > The idea was that onShow and onHide should be enough. I'm not sure this was a > good idea, currently onShow/onHide does not called correctly for content/test > shell, but probably we don't want to reintroduce them (although for me that's > fine). > > Maybe we should skip this part for now? https://codereview.chromium.org/133943002/diff/1290001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher.cc (right): https://codereview.chromium.org/133943002/diff/1290001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher.cc:11: How about exclude the gamepad_platform_data_fetcher.cc in content_browser.gypi for Android build, instead of involving the new MACROs? https://codereview.chromium.org/133943002/diff/1290001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:27: private InputManager inputManager; inputManager -> mInputManager? https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:49: private static GamepadList gamepadList = null; gamepadList -> sGamepadList https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:77: private boolean haveDevicesBeenInteractedWith() { return mHaveDevicesBeenInteractedWith; } Is it necessary to add a private function to access mHaveDevicesBeenInteractedWith? I feel it's straightforward to use it directly.
https://codereview.chromium.org/133943002/diff/1290001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/133943002/diff/1290001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:1177: mContentViewCore.onPause(); On 2014/02/25 10:26:58, xwang wrote: I agree to remove it as its getting discussed in another CL Removing. https://codereview.chromium.org/133943002/diff/1290001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher.cc (right): https://codereview.chromium.org/133943002/diff/1290001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher.cc:11: On 2014/02/25 10:26:58, xwang wrote: Done. https://codereview.chromium.org/133943002/diff/1290001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:26: private boolean IS_GAMEPAD_ACCESSED = false; On 2014/02/20 00:48:24, kbalazs wrote: Done. https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:27: private InputManager inputManager; On 2014/02/25 10:26:58, xwang wrote: Done. https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:49: private static GamepadList gamepadList = null; On 2014/02/25 10:26:58, xwang wrote: Done. https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:77: private boolean haveDevicesBeenInteractedWith() { return mHaveDevicesBeenInteractedWith; } On 2014/02/25 10:26:58, xwang wrote: Removed it. It was earlier getting used when i was passing it to Native. Now it is not needed. https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:132: public int getCount() { On 2014/02/20 00:48:24, kbalazs wrote: Done. https://codereview.chromium.org/133943002/diff/1290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:160: // Check if any registered gamepad is disconnected when webview was in pause state. On 2014/02/20 00:48:24, kbalazs wrote: Removing onPause and onResume as it is already getting discussed in another CL.
I think you will need to rebase it because of the work done in http://crbug.com/344556
Looks promising. I've left some comments, primarily about the way Gamepads are being mapped. https://codereview.chromium.org/133943002/diff/1410001/content/browser/androi... File content/browser/android/gamepad_reader_impl.cc (right): https://codereview.chromium.org/133943002/diff/1410001/content/browser/androi... content/browser/android/gamepad_reader_impl.cc:108: memcpy(pad.buttons, raw_buttons, pad.buttonsLength * sizeof(float)); This needs to be rebased. The current code no longer uses a float array for button state. https://codereview.chromium.org/133943002/diff/1410001/content/child/runtime_... File content/child/runtime_features.cc (right): https://codereview.chromium.org/133943002/diff/1410001/content/child/runtime_... content/child/runtime_features.cc:35: WebRuntimeFeatures::enableGamepad(true); Can't this line be removed now? https://codereview.chromium.org/133943002/diff/1410001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1410001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:46: private static final int[] standardGamepadKeyCodes = { This is more-or-less a duplicate of the StandardButtons class in GamepadMappings. Is there a reasonable way to combine them to reduce duplication? https://codereview.chromium.org/133943002/diff/1410001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:110: protected void setGamepadType() { I'd rather this logic occur in the GamepadMappings. That way as new gamepads are added and mapped only a single files needs to be touched. https://codereview.chromium.org/133943002/diff/1410001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:122: if (mDeviceName.contains("NVIDIA") && mDeviceName.contains("Controller")) { This feels imprecise. If NVIDIA ever makes another controller this may start registering incorrectly. Is there a way to detect vendor/product IDs like the desktop code does, or a similar device-specific value? If the device name is the most precise identifier we have I'd prefer to do an exact string match against the full name instead. https://codereview.chromium.org/133943002/diff/1410001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java (right): https://codereview.chromium.org/133943002/diff/1410001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:63: public static float[] mapToStandardGamepadAxes(float[] rawAxes, GamepadType gamepadType) { The way this is set up now feels like it will be awkward to maintain going forward. I'd prefer to see it work similarly to the desktop mapping code (see gamepad_standard_mappings_linux.cc as an example.) We should be able to retrieve a mapping interface on device connection by passing a device identifier (name or vendor/product id) to a factory method. Then the mapper interface can be called abstractly from GamepadDevice without bouncing through multiple switch statements.
On latest synced codebase, I get crash on content shell on enabling gamepad feature for Android. I have filed an issue for the same. Issue number : 349834 It looks like a regression from commit 66708e1b8c24cae7639e603356e32b6291533a29
This patch also fixes the crash on chromium Top-of-the-Tree which has been caused by commit 66708e1b8c24cae7639e603356e32b6291533a29 . I have already opened a bug ( https://code.google.com/p/chromium/issues/detail?id=349834 ) for the same crash but it has not been assigned till now. https://codereview.chromium.org/133943002/diff/1410001/content/browser/androi... File content/browser/android/gamepad_reader_impl.cc (right): https://codereview.chromium.org/133943002/diff/1410001/content/browser/androi... content/browser/android/gamepad_reader_impl.cc:108: memcpy(pad.buttons, raw_buttons, pad.buttonsLength * sizeof(float)); On 2014/02/28 23:56:11, bajones wrote: Done. https://codereview.chromium.org/133943002/diff/1410001/content/child/runtime_... File content/child/runtime_features.cc (right): https://codereview.chromium.org/133943002/diff/1410001/content/child/runtime_... content/child/runtime_features.cc:35: WebRuntimeFeatures::enableGamepad(true); On 2014/02/28 23:56:11, bajones wrote: Removed now. https://codereview.chromium.org/133943002/diff/1410001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1410001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:46: private static final int[] standardGamepadKeyCodes = { On 2014/02/28 23:56:11, bajones wrote: I have to maintain the list of keycode because I need to pass it to InputDevice to get the list of supported Keycodes that can be produced by a connected device. Whereas I am using class StandardButtons to map the buttons to standard gamepad button array format. These Keycodes in standardGamepadKeyCodes have values that do not follow any order. All i can do is explicitly add and subtract from these values at the time of mapping to standard format in the mapper function, but it does not look like a nice approach to me. Please advice if i should go ahead with this approach. https://codereview.chromium.org/133943002/diff/1410001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:110: protected void setGamepadType() { On 2014/02/28 23:56:11, bajones wrote: Done. https://codereview.chromium.org/133943002/diff/1410001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:122: if (mDeviceName.contains("NVIDIA") && mDeviceName.contains("Controller")) { On 2014/02/28 23:56:11, bajones wrote: Done. https://codereview.chromium.org/133943002/diff/1410001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java (right): https://codereview.chromium.org/133943002/diff/1410001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:63: public static float[] mapToStandardGamepadAxes(float[] rawAxes, GamepadType gamepadType) { On 2014/02/28 23:56:11, bajones wrote: The new patchset now retrieves by passing device name. For more accuracy there is additional comparision for raw buttons and raw axes on the gamepad device.
Adding reviewers to review the gamepad implementation.
content/browser/gamepad LGTM. Also, thank you for consolidating the mapping logic into a single file. I would still prefer to see a factory pattern used here (Like in kbalazs@ patch: https://codereview.chromium.org/165483003/diff/140001/content/public/android/...) since that would make it easier to extend in the future, but I don't think that should be a blocking issue at this point.
On 2014/03/17 17:47:20, bajones wrote: > content/browser/gamepad LGTM. > > Also, thank you for consolidating the mapping logic into a single file. I would > still prefer to see a factory pattern used here (Like in kbalazs@ patch: > https://codereview.chromium.org/165483003/diff/140001/content/public/android/...) > since that would make it easier to extend in the future, but I don't think that > should be a blocking issue at this point. bajones@: Why are we LGTM'ing two conflicting patches? Have the differences between this and crrev.com/165483003 been sufficiently resolved offline for this to move forward?
On 2014/03/17 17:52:07, jdduke wrote: > On 2014/03/17 17:47:20, bajones wrote: > > content/browser/gamepad LGTM. > > > > Also, thank you for consolidating the mapping logic into a single file. I > would > > still prefer to see a factory pattern used here (Like in kbalazs@ patch: > > > https://codereview.chromium.org/165483003/diff/140001/content/public/android/...) > > since that would make it easier to extend in the future, but I don't think > that > > should be a blocking issue at this point. > > bajones@: Why are we LGTM'ing two conflicting patches? Have the differences > between this and crrev.com/165483003 been sufficiently resolved offline for this > to move forward? Sorry, NOT lgtm.
> On 2014/03/17 17:52:07, jdduke wrote: > > bajones@: Why are we LGTM'ing two conflicting patches? Have the differences > > between this and crrev.com/165483003 been sufficiently resolved offline for > this > > to move forward? Hmm, I suppose this is what skhatri@nvidia.com was referring to with "I also don't want to add too much here except that kbalazs has agreed earlier that we will go ahead with my implementation."? b.kelemen@samsung.com, could you chime in here?
On 2014/03/17 21:42:48, jdduke wrote: > > On 2014/03/17 17:52:07, jdduke wrote: > > > bajones@: Why are we LGTM'ing two conflicting patches? Have the differences > > > between this and crrev.com/165483003 been sufficiently resolved offline for > > this > > > to move forward? > > Hmm, I suppose this is what mailto:skhatri@nvidia.com was referring to with "I also > don't want to add too much here except that kbalazs has agreed earlier that we > will go ahead with my implementation."? mailto:b.kelemen@samsung.com, could you chime > in here? Yes, when I saw this first I suggested to go forward with it as it was the first one. I uploaded my work before noticing this one. Later I uploaded my updated patch as I was not entirely satisfied with the progress of this patch and because @bajones reviewed mine as well. I made some comments on it that Skhatri didn't try to address (no offense, just for the notice). For example any kind of locking is still missing from here although the data is accessed concurrently on 2 threads. Also it hasn't been updated for a while. Yes we can say that I tried to push my version and it wasn't a very tidy thing from me. I confess that. Whatever the resolution is going to be, please not land it as it is. Besides the locking thing there was a bunch of review comments in https://codereview.chromium.org/165483003 that is not addressed here yet.
I must mention that I have never received any comment regarding the locking thing. I can look for other comments in https://codereview.chromium.org/165483003 and will address them here aswell if they apply. kbalazs had made various comments of which I addressed many and left some with which I was not satisfied. Either way, if the reviewers think that there are still some areas in my patch that needs to be changed, I am open for it.
https://codereview.chromium.org/133943002/diff/1470001/content/browser/androi... File content/browser/android/gamepad_reader_impl.cc (right): https://codereview.chromium.org/133943002/diff/1470001/content/browser/androi... content/browser/android/gamepad_reader_impl.cc:86: pad.id[name_to_copy] = 0; Looks like we have an extra assignment here, also below. with pad.mapping. https://codereview.chromium.org/133943002/diff/1470001/content/browser/androi... content/browser/android/gamepad_reader_impl.cc:104: pad.axesLength = (axes.size() > WebGamepad::axesLengthCap) Why not std::min? https://codereview.chromium.org/133943002/diff/1470001/content/browser/androi... content/browser/android/gamepad_reader_impl.cc:116: pad.buttonsLength = (buttons.size() > WebGamepad::buttonsLengthCap) Also std::min. https://codereview.chromium.org/133943002/diff/1470001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/133943002/diff/1470001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:18: GamepadsReader::GetInstance()->GetGamepadData(pads); Please have a look at my comments for gamepad_platform_data_fetcher_android.cc from the other patch. In short, because you're using a Java singleton, you don't need a C++ singleton. The native code should simply call into a static method on the Java class, and the Java code should never need to *store* a pointer to native code. Any calls into Java from native should pass a pointer to the |WebGamePads| object, which can be populated when calling back into a (static) native method. We should never store a reference to |pads|. In that case, we should implement the logic here, and remove |gamepad_read_impl.{cc,h}|. https://codereview.chromium.org/133943002/diff/1470001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java (right): https://codereview.chromium.org/133943002/diff/1470001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:15: public static final int BUTTON_A = 0; I believe we're getting these values from gamepad_standard_mappings.h? We may want to do what was done in the other patch, and have these constants auto-generated for Java and C++ via a _list.h/.template file. We'll still need to translate between the auto-generated enum values and the blink values, but at least we'll always remain in sync.
https://codereview.chromium.org/133943002/diff/1470001/content/browser/androi... File content/browser/android/gamepad_reader_impl.cc (right): https://codereview.chromium.org/133943002/diff/1470001/content/browser/androi... content/browser/android/gamepad_reader_impl.cc:86: pad.id[name_to_copy] = 0; On 2014/03/18 09:43:15, jdduke wrote: Oh yes, some how i missed it. Removing in next patchset. https://codereview.chromium.org/133943002/diff/1470001/content/browser/androi... content/browser/android/gamepad_reader_impl.cc:104: pad.axesLength = (axes.size() > WebGamepad::axesLengthCap) On 2014/03/18 09:43:15, jdduke wrote: Using it now in next patchset. https://codereview.chromium.org/133943002/diff/1470001/content/browser/androi... content/browser/android/gamepad_reader_impl.cc:116: pad.buttonsLength = (buttons.size() > WebGamepad::buttonsLengthCap) On 2014/03/18 09:43:15, jdduke wrote: Chainging in next patch-set. https://codereview.chromium.org/133943002/diff/1470001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/133943002/diff/1470001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:18: GamepadsReader::GetInstance()->GetGamepadData(pads); On 2014/03/18 09:43:15, jdduke wrote: I had created a seperate class GamepadReader just for the JNI interaction. I had seen other classes that make JNI call were placed at content/browser/android/ like content_view_core.cc, therefore i kept the structure like that. Now i see that such files are at various directory locations so i will update it. https://codereview.chromium.org/133943002/diff/1470001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java (right): https://codereview.chromium.org/133943002/diff/1470001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:15: public static final int BUTTON_A = 0; On 2014/03/18 09:43:15, jdduke wrote: Incorporating in next patchset.
jdduke@: GamepadMapping changes : Work in progress
https://codereview.chromium.org/133943002/diff/1490001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/133943002/diff/1490001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:47: pads->length = WebGamepads::itemsLengthCap; Hmm, are we guaranteed to always populate |itemLengthCap| number of elements? What you might do is set this to 0, then in |SetGamepadData| you can increase |pads->length| to include the index, something like: SetGamepadData(...) { DCHECK(jgamepads); blink::WebGamepads* gamepads = reinterpret_cast<WebGamepads*>(jgamepads); DCHECK_EQ(gamepads->length, index); DCHECK_LT(index, WebGamepads::itemsLengthCap); ++gamepads->length; .... } Then at least we have some validation that we're filling the required fields (in order). https://codereview.chromium.org/133943002/diff/1490001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1490001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:160: return true; Why is this true and not false? https://codereview.chromium.org/133943002/diff/1490001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1490001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:164: if (sGamepadList.isGamepadAccessed() && sGamepadList.isGamepadEvent(event)) As noted in the other patch, we need to ensure consistency when we transition between capture modes (mIsGamepadAccessed). We should probably be resetting the gamepad data when access is paused (or all windows have been detached), or perhaps we stop consuming new KeyEvent.ACTION_DOWN's, and only consume ACTION_UP's for keys that have already been registered as ACTION_DOWN. I don't think we forward SOURCE_JOYSTICK events to native, so we have a bit more flexibility there. https://codereview.chromium.org/133943002/diff/1490001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:298: assert sGamepadList != null; This is not thread safe.
https://codereview.chromium.org/133943002/diff/1490001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/133943002/diff/1490001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:47: pads->length = WebGamepads::itemsLengthCap; On 2014/03/19 09:44:58, jdduke wrote: Yes, incorporating this in next patchset, https://codereview.chromium.org/133943002/diff/1490001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1490001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:160: return true; On 2014/03/19 09:44:58, jdduke wrote: Oops, i think i missed it by mistake. Modifying it now. https://codereview.chromium.org/133943002/diff/1490001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1490001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:164: if (sGamepadList.isGamepadAccessed() && sGamepadList.isGamepadEvent(event)) On 2014/03/19 09:44:58, jdduke wrote: Even i agree that resetting the gamepad state after we receive a hint that the access is paused is a correct idea. Implementing it in next patchset. Apart from that I am already setting all the connected gamepad devices to null value after i get onDetachedFromWindow callback. https://codereview.chromium.org/133943002/diff/1490001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:298: assert sGamepadList != null; On 2014/03/19 09:44:58, jdduke wrote: Done.
A few of us were travelling, hence the lag time on review. Thanks for your patience. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:26: protected int mDeviceId; Do you plan on subclassing this? If not, these members really ought to be private. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:130: mMapping = GamepadMappings.mapToStandardGamepad(mAxisValues, mButtonsValues, mRawAxes, GamepadMappings.mapToStandardGamepad returns a Boolean, so I don't really understand this assignment. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:159: public void clearData() { I'm going to led tedchoc@ or another Java specialist chime in on these files, but in general, particularly for non-trivial getters, you'll want to (Javadoc) document these public methods. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:167: mTimestamp = SystemClock.uptimeMillis(); Do you want really want to update mTimestamp if it's not a gamepad key? https://codereview.chromium.org/133943002/diff/1530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:52: assert ThreadUtils.runningOnUiThread(); Do we have any guarantees that this method will be called before the native (C++) gamepad is accessed? https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:60: mInputManager = (InputManager) context.getSystemService(Context.INPUT_SERVICE); mInputManager is only accessed on the UI thread, so it can be pulled out of the lock guard. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:99: if (isGamepadDevice(inputDevice)) Please pull this check outside the lock, and early return if false. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:147: public GamepadDevice getDeviceById(int deviceId) { Why is this public? Any public member that accesses private data will likely need some kind of synchronization primitive. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:157: public GamepadDevice getDevice(int index) { Again, why public? https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:159: assert index >= 0 && index <= MAX_GAMEPADS - 1; index < MAX_GAMEPADS https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:164: if (sGamepadList.mIsGamepadAccessed && sGamepadList.isGamepadEvent(event)) These public static methods are accessing shared data (mIsGamepadAccessed), and need a synchronization primitive to be thread safe. Here's how you might structure this: if (!isGamepadEvent(event)) return false; return sGamepadList.updateForEvent(event); Then, include the check for |mIsGamepadAccessed| in the synchronized portion of |updateForEvent|. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:173: mHaveDevicesBeenInteractedWith = true; Why not store this variable in each GamepadDevice? https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:233: private boolean isGamepadDevice(InputDevice inputDevice) { This method can be static. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:248: private boolean isGamepadEvent(MotionEvent event) { This method can be static. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:282: nativeSetGamepadData(gamepads, i, getMapping(i), true, Why all these helper methods? Why not do something like: final GamepadDevice device = getDevice(i); if (device != null && device.hasBeenInteractedWith()) { nativeSetGamepadData(nativeGamepads, i, device.getMapping(), ...). } else { nativeSetGamepadData(...) } Then you can get rid of all the helper getters. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:307: private native void nativeSetGamepadData(long gamepads, Please rename all |gamepads| references to |nativeGamepads|. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/gamepad_mapping/CanonicalAxisIndex.template (right): https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/gamepad_mapping/CanonicalAxisIndex.template:8: #define CANONICAL_AXIS_INDEX(name, value) public static final int name = value; I don't think we need to create a new directory just for these files. They should go in the same folder as the (only) files that use them, so perhaps we should just move the other new gamepad files to the input/ directory.
https://codereview.chromium.org/133943002/diff/1530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:26: protected int mDeviceId; On 2014/03/24 17:38:15, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:130: mMapping = GamepadMappings.mapToStandardGamepad(mAxisValues, mButtonsValues, mRawAxes, On 2014/03/24 17:38:15, jdduke wrote: I am setting the mMapping to string "standard" if the device type is known otherwise i should set it to an empty string. Doing it using ternary operator. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:159: public void clearData() { On 2014/03/24 17:38:15, jdduke wrote: SO, shall i be appending "@SuppressWarnings("javadoc")" before the method |clearData| ? https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:167: mTimestamp = SystemClock.uptimeMillis(); On 2014/03/24 17:38:15, jdduke wrote: Changeing it now. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:52: assert ThreadUtils.runningOnUiThread(); On 2014/03/24 17:38:15, jdduke wrote: Whenever a view becomes visible, I assumed that onAttachedToWindow is the first callback that we get and everything happens after that. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:60: mInputManager = (InputManager) context.getSystemService(Context.INPUT_SERVICE); On 2014/03/24 17:38:15, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:99: if (isGamepadDevice(inputDevice)) On 2014/03/24 17:38:15, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:147: public GamepadDevice getDeviceById(int deviceId) { On 2014/03/24 17:38:15, jdduke wrote: Changing it to private now. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:157: public GamepadDevice getDevice(int index) { On 2014/03/24 17:38:15, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:159: assert index >= 0 && index <= MAX_GAMEPADS - 1; On 2014/03/24 17:38:15, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:164: if (sGamepadList.mIsGamepadAccessed && sGamepadList.isGamepadEvent(event)) On 2014/03/24 17:38:15, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:173: mHaveDevicesBeenInteractedWith = true; On 2014/03/24 17:38:15, jdduke wrote: Now storing in GamepadDevice. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:233: private boolean isGamepadDevice(InputDevice inputDevice) { On 2014/03/24 17:38:15, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:248: private boolean isGamepadEvent(MotionEvent event) { On 2014/03/24 17:38:15, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:282: nativeSetGamepadData(gamepads, i, getMapping(i), true, On 2014/03/24 17:38:15, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:307: private native void nativeSetGamepadData(long gamepads, On 2014/03/24 17:38:15, jdduke wrote: Is it needed? because i was under the impression that when we prefix with 'native' we need to provide the attached native class object. (?) And 'gamepads' is a object of type webgamepad passed from native(C++). https://codereview.chromium.org/133943002/diff/1530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/gamepad_mapping/CanonicalAxisIndex.template (right): https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/gamepad_mapping/CanonicalAxisIndex.template:8: #define CANONICAL_AXIS_INDEX(name, value) public static final int name = value; On 2014/03/24 17:38:15, jdduke wrote: Done.
Thanks, this is getting close. tedchoc@, would you mind taking a pass over the Java code? Also, it would be *really* nice if we could at least have a few instrumentation tests that verify this doesn't explode. I'm not sure how possible that is with the input manager, but I would feel a lot better knowing we've at least covered adding a few devices, injecting a few events, disconnecting the devices, detaching from the window, and verifying the data is collected. Let me know if you have questions about that and we can find the right person who might be able to help. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:159: public void clearData() { On 2014/03/26 16:10:07, SaurabhK wrote: > On 2014/03/24 17:38:15, jdduke wrote: > > SO, shall i be appending "@SuppressWarnings("javadoc")" before the method > |clearData| ? No, you should definitely comment this and all other public methods. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:258: public boolean isGamepadEvent(KeyEvent event) { Hmm, is this accessed anywhere else? If not, please make private. Looks like it can also be static. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:293: static void notifyForGamepadsAccess(boolean isaccesspaused) { Nit: isAccessPaused. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:296: sGamepadList.mIsGamepadAccessed = !isaccesspaused; It feels a little funky locking the static instance member in the static method. Please factor out this code into a (member) function, so you'll have: assert sGamepadList != null; sGamepadList.notifyForGamepadsAccess(isAccessPaused); much as you've done with |getGamepadData()|. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:307: private native void nativeSetGamepadData(long gamepads, On 2014/03/26 16:10:07, SaurabhK wrote: > On 2014/03/24 17:38:15, jdduke wrote: > > Is it needed? > because i was under the impression that when we prefix with 'native' we need to > provide the attached native class object. (?) > And 'gamepads' is a object of type webgamepad passed from native(C++). Hmm, I think that's a convention, but I don't feel strongly about this either way. What you have is probably fine, maybe tedchoc@ has an opinion on this. https://codereview.chromium.org/133943002/diff/1550001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/133943002/diff/1550001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:80: pad.timestamp = timestamp; Hmm, do you know if the time measurement matters here, i.e., milliseconds vs microseconds? Or just as long as they're monotonically increasing? https://codereview.chromium.org/133943002/diff/1550001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:130: for (unsigned int j = 0; j < pad.buttonsLength; j++) { Hmm, what guarantees do we have that the button indices in blink and the button indices from Java are in sync? Don't we need a translation method for canonical_{axis,button}_index_list.h to map between the two? https://codereview.chromium.org/133943002/diff/1550001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:55: // A boolean that is set to true when the user interacts with the gamepad device. Nit: Just say "Set to true when", no need to declare a boolean. https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:165: Arrays.fill(mAxisValues, 0); Can you also reset |mHasDeviceBeenInteractedWith|? May be a slight optimization. https://codereview.chromium.org/133943002/diff/1550001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:167: if (gamepad != null) { if (gamepad == null) return false; return gamepad.updateForEvent(event); https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:203: if (gamepadDevice == null) { Nit: No braces, return on same line. https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:226: if ((event.getSource() & InputDevice.SOURCE_JOYSTICK) == InputDevice.SOURCE_JOYSTICK) { Just return (event.getSource() & InputDevice.SOURCE_JOYSTICK) == InputDevice.SOU RCE_JOYSTICK; https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:279: gamepadDevice.clearData(); Nit: if (gamepadDevice == null) continue; gamepadDevice.clearData();
b.kelemen@samung.com, could you also take a brief glance at the mapping code and make sure this will work for your purposes? I believe you had bindings for a PS3 controller, which we can add those in a follow-up patch, just want to make sure we're not missing anything.
On 2014/03/26 16:39:56, jdduke wrote: > mailto:b.kelemen@samung.com, could you also take a brief glance at the mapping code and > make sure this will work for your purposes? I believe you had bindings for a > PS3 controller, which we can add those in a follow-up patch, just want to make > sure we're not missing anything. Sure, I will take a look later today.
IMHO there is still some room for improvement. https://codereview.chromium.org/133943002/diff/1550001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/133943002/diff/1550001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:130: for (unsigned int j = 0; j < pad.buttonsLength; j++) { On 2014/03/26 16:37:58, jdduke wrote: > Hmm, what guarantees do we have that the button indices in blink and the button > indices from Java are in sync? Don't we need a translation method for > canonical_{axis,button}_index_list.h to map between the two? This data should be already mapped if a mapping is possible. The indices for the so called standard gamepad are defined by the standard, and we implement the mapping according to that. Blink doesn't have it's own definition represented as an enum or something. I think this is ok. https://codereview.chromium.org/133943002/diff/1550001/content/browser/gamepa... File content/browser/gamepad/gamepad_provider.cc (right): https://codereview.chromium.org/133943002/diff/1550001/content/browser/gamepa... content/browser/gamepad/gamepad_provider.cc:116: #if defined(OS_LINUX) || defined(OS_ANDROID) I think we should use base::MessageLoop::TYPE_DEFAULT on Android because there is no need for an io loop and I guess a default loop is the cheapest (at least in terms of dependencies). https://codereview.chromium.org/133943002/diff/1550001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:165: Arrays.fill(mAxisValues, 0); On 2014/03/26 16:37:58, jdduke wrote: > Can you also reset |mHasDeviceBeenInteractedWith|? May be a slight optimization. I'm not sure we should do that because the fact that the app moved to background doesn't change that the device has already been interacted with. I think we want to continue providing gamepad data to the page immediately if there was interaction before. https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:179: mKeyCodeValueMap.put(keyCode, 1.0f); This creates 2 new objects possibly several times per frame. I have been told to avoid doing that to reduce gc churn and managed to do in the other patch, so I guess this is not ok. Hint: I simply created 256 size raw arrays and used the axis id and the keycode as index (both is always smaller) as a cheap kind of associative array. https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:184: mTimestamp = SystemClock.uptimeMillis(); Why not using the timestamp from the event? https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:195: mTimestamp = SystemClock.uptimeMillis(); Why not using the timestamp from the event? https://codereview.chromium.org/133943002/diff/1550001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:258: if (device != null && device.hasDeviceBeenInteractedWith()) { The gesture check (a la hasBeenInteractedWith()) is also checked lower in the stack with a different definition. The standard says we should not provide gamepad data before a user gesture is observed, and a gesture is by definition that one of the primer buttons have been pushed. I think it would be better to remove this logic from here, GamepadSharedMemoryReader takes care of not exposing the data before user gesture. https://codereview.chromium.org/133943002/diff/1550001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java (right): https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:14: class GamepadMappings { I would slightly prefer an abstract class implemented per device. https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:59: mappedButtons[CanonicalButtonIndex.BUTTON_PRIMARY] = rawButtons[0]; I would also prefer if we could use the symbolic constants from the api (like AXIS_FOO and KEYCODE_BUTTON_BAR). To me it looks clearer and easier to add new devices. Not a very big deal though, I'm ok if you keep it that way.
Thanks kbalazs for the review. I have incoporated almost all your review comments and left my explaination for others. The important thing is to not miss anything that might be needed by you, things like mapping PS3 controller https://codereview.chromium.org/133943002/diff/1530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:159: public void clearData() { On 2014/03/26 16:37:58, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:258: public boolean isGamepadEvent(KeyEvent event) { On 2014/03/26 16:37:58, jdduke wrote: Made private static in next patch. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:293: static void notifyForGamepadsAccess(boolean isaccesspaused) { On 2014/03/26 16:37:58, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:296: sGamepadList.mIsGamepadAccessed = !isaccesspaused; On 2014/03/26 16:37:58, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1550001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/133943002/diff/1550001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:80: pad.timestamp = timestamp; On 2014/03/26 16:37:58, jdduke wrote: timestamp is of type DOMHighResTimeStamp which is a double representing a number of milliseconds. I am assigning the timestamp to event time using |getEventTime| which also returns milliseconds. https://codereview.chromium.org/133943002/diff/1550001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:130: for (unsigned int j = 0; j < pad.buttonsLength; j++) { On 2014/03/26 19:24:53, kbalazs wrote: THe buttons are already mapped to statndard format if the device type is known, otherwise the buttons will be exposed in raw format. So for the known devices the buttons in blink and buttons in java will be in sync. https://codereview.chromium.org/133943002/diff/1550001/content/browser/gamepa... File content/browser/gamepad/gamepad_provider.cc (right): https://codereview.chromium.org/133943002/diff/1550001/content/browser/gamepa... content/browser/gamepad/gamepad_provider.cc:116: #if defined(OS_LINUX) || defined(OS_ANDROID) On 2014/03/26 19:24:53, kbalazs wrote: Thanks, changing it now. https://codereview.chromium.org/133943002/diff/1550001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:55: // A boolean that is set to true when the user interacts with the gamepad device. On 2014/03/26 16:37:58, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:165: Arrays.fill(mAxisValues, 0); On 2014/03/26 19:24:53, kbalazs wrote: Yes, even I feel that we should not be resetting |mHasDeviceBeenInteractedWith| for the same reason pointed out by kbalazs. https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:179: mKeyCodeValueMap.put(keyCode, 1.0f); On 2014/03/26 19:24:53, kbalazs wrote: In my opinion, I dont think that it is an issue as Keycode and key values(1.0f, 0.0f) are both primitive datatypes. And apart from that I find LinkedHashMap more useful than keeping fixed size array in this situation. Mostly because we have to maintain the order of the buttons and in case of unkown gamepad device we pass gamepad data in raw format. https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:184: mTimestamp = SystemClock.uptimeMillis(); On 2014/03/26 19:24:53, kbalazs wrote: Using it now. https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:195: mTimestamp = SystemClock.uptimeMillis(); On 2014/03/26 19:24:53, kbalazs wrote: Done. https://codereview.chromium.org/133943002/diff/1550001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:167: if (gamepad != null) { On 2014/03/26 16:37:58, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:203: if (gamepadDevice == null) { On 2014/03/26 16:37:58, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:226: if ((event.getSource() & InputDevice.SOURCE_JOYSTICK) == InputDevice.SOURCE_JOYSTICK) { On 2014/03/26 16:37:58, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:258: if (device != null && device.hasDeviceBeenInteractedWith()) { On 2014/03/26 19:24:53, kbalazs wrote: Correctly said. I am removing the |hasBeenInteractedWith()| check now. https://codereview.chromium.org/133943002/diff/1550001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:279: gamepadDevice.clearData(); On 2014/03/26 16:37:58, jdduke wrote: Done.
> In my opinion, I dont think that it is an issue as Keycode and key values(1.0f, > 0.0f) are both primitive datatypes. That's not correct, it will create objects. What's happening here is autoboxing. Unfortunately java doesn't have containers for primitive types. Android has SparseIntegerArray but not SparseFloatArray. > And apart from that I find LinkedHashMap more useful than keeping fixed size > array in this situation. > Mostly because we have to maintain the order of the buttons and in case of > unkown gamepad device we pass gamepad data in raw format. The order of buttons is not important if we can use the constants from KeyEvent and MotionEvent. Even if the gamepad is uknown the android input framework already mapped the input to KeyEvent's and MotionEvent's with well defined key codes and axis ids. We can try to remap it to the standard gamepad's indexes. Imho with that approach we have a better chance that we will produce something the script on the page can work with.
Now i get that there is autoboxing that is heppening but at the same time I am also not very convinced with using a raw array and it needs me to change quite a lot of code. I will look for a better alternative and resume the work when i will get back to office on Tuesday.
On 2014/03/28 14:08:55, SaurabhK wrote: > Now i get that there is autoboxing that is heppening but at the same time I am > also not very convinced with using a raw array and it needs me to change quite a > lot of code. > I will look for a better alternative and resume the work when i will get back to > office on Tuesday. Sounds good. I don't have a strong preference for the final solution, but we should do all we can to avoid creating garbage every frame.
Addressed all review comments. Please tell, if i am missing anything.
A few nits, and one key suggestion (see below, about adopting native mapping functions as other platforms have done). https://codereview.chromium.org/133943002/diff/1590001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/133943002/diff/1590001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:39: } PauseHint(true);? https://codereview.chromium.org/133943002/diff/1590001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:43: JNIEnv* env = AttachCurrentThread(); Let's add |TRACE_EVENT0("GAMEPAD", "GetGamepadData");| to the first line. https://codereview.chromium.org/133943002/diff/1590001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:47: pads->length = 0; Should we unconditionally set pads->length = 0, before the env check? https://codereview.chromium.org/133943002/diff/1590001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:63: jstring mapping, The mapping should only ever be "standard" or "", right? If so, let's just pass in |jboolean standard_mapping|, and set to the known string value. https://codereview.chromium.org/133943002/diff/1590001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/133943002/diff/1590001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1701: if (GamepadList.dispatchKeyEvent(event)) { Nit: |return true;| on the same line as the conditional, remove braces. Also below. https://codereview.chromium.org/133943002/diff/1590001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1590001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:133: mMapping = GamepadMappings.mapToStandardGamepad(mAxisValues, mButtonsValues, mRawAxes, I still don't understand why we're assigning |mMapping|, a String, to the result of mapToStandardGamepad, a bool. https://codereview.chromium.org/133943002/diff/1590001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1590001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:208: if (index == -1) { |return false;| on same line as conditional, no braces. https://codereview.chromium.org/133943002/diff/1590001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:283: private void isGamepadAccessed(boolean isGamepadAccessed) { |setIsGamepadAccessed|. https://codereview.chromium.org/133943002/diff/1590001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java (right): https://codereview.chromium.org/133943002/diff/1590001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:14: class GamepadMappings { Actually... I'm looking at the other platform code, and I'm wondering why we're doing the standard mappings in Java. Is there any reason we can't pass the raw values to native, populate the WebGamepad, and use the same mapping approach as every other platform? (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ga...) As far as I can tell, the logic should be more or less exactly the same, right? We'll just add a |gamepad_standard_mappings_android.cc|. Sorry to dump more work on your plate, but having all mappings done in a consistent way across platforms should make this a bit easier to maintain. Then we no longer need the Java template mappings for canonical axis/button values. https://codereview.chromium.org/133943002/diff/1590001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:18: protected static final int mRawShieldAxes = 10; protected -> private https://codereview.chromium.org/133943002/diff/1590001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:29: boolean isMapped = true; Let's remove the |isMapped| variable. Instead, have each successful condition return true after mapping. Then remove the final |else| branch, and just return false at the end of the function.
Just now i realised that i had missed a huge bunch of changes in my previous patch. I will take care of the reviewer comments on patch #17 on Monday.
Just wanted to give a quick first past mainly focused style https://codereview.chromium.org/133943002/diff/1610001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1701: if (GamepadList.dispatchKeyEvent(event)) { in javaland, you can put the statement and conditional all on one line if it fits. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:5: package org.chromium.content.browser; for all these gamepage files, can you move them into the input/ package? https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:18: /* for javadocs, the starting token is /** https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:21: remove blank line https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:58: mMapping = " "; can you make " " and "standard" constants. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:82: GamepadDevice() {} why do you need the empty constructor? https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:85: * Map the axes and buttons of a gamepad device to a standard gamepad format. for these javadoc comments and below, the seond and third lines need to be indented one more: /** * ... */ https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:88: mMapping = GamepadMappings.mapToStandardGamepad(mAxisValues, mButtonsValues, mRawAxes, style nit in java, the indenting is different than that of c++. The following lines should be indented 8 from the start of the previous line and not aligned with the ( above. Granted, if all params will fit on a line together, I try to use the following: mMapping = GamepadMappings.mapToStandardGamepad( mAxisValues, mButtonsValues, mRawAxes, mRawButtons, mDeviceName) ? "standard" : " "; I don't know if that will be under 100 chars in this case though, so you might need to do: mMapping = GamepadMappings.mapToStandardGamepad(mAxisValues, mButtonsValues, mRawAxes, mRawButtons, mDeviceName) ? "standard" : " "; https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:121: public float[] getAxes() { for consistency with the ones above, these two should probably be one liners as well. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:130: remove extra blank line https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:148: public boolean updateForEvent(KeyEvent event) { for consistency with android, I would call this handleKeyEvent and handleMotionEvent below. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:169: // Update axes values. does this need a corresponding isGamepadEvent like the key event handler above? https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:170: for (int i = 0; i < mAxes.length; ++i) { i++ in java https://codereview.chromium.org/133943002/diff/1610001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:25: private GamepadDevice[] mGamepadDevices = new GamepadDevice[MAX_GAMEPADS]; for readability, try to order the params like: static finals statics finals other members Blank lines can sometimes help separate them as well. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:26: private boolean mIsGamepadAccessed = false; false is the default, so it's not needed https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:46: private static GamepadList sGamepadList = null; move this up with the other params. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:123: for (int i = 0; i < MAX_GAMEPADS; ++i) { i++ https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:139: for (int i = 0; i < MAX_GAMEPADS; ++i) { i++ https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:163: private static boolean updateForEvent(KeyEvent event) { Based on the calling site, I don't think these should be marked as static. Also per my naming suggestion in the other file, I would call this handleKeyEvent and handleMotionEvent https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:226: InputDevice.SOURCE_JOYSTICK); should be indented 8 https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:239: remove blank line https://codereview.chromium.org/133943002/diff/1610001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java (right): https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:17: float[] rawAxes, float[] rawButtons, use java indenting https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:26: else { needs to go on the line above with the } https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:36: rawButtons[KeyEvent.KEYCODE_BUTTON_B]; should be indented 8 from the start of the previous line (same in the rest of the file)
There were many reasons to keep the mapping here in Java side. We started with the android stock browser where most of the implementaion was in Java and upon shifting to chrome it was observed that the native was used just to pass the gamepad data via gamepad_hardware_buffer and every other major things were still needed to be handled in java like:- 1. Identifying gamepad devices and understanding their capabilities 2. Managing gamepad devices 3. Keeping gamepad data up-to-date. So, I feel its more appropriate to keep mapping along with rest of the code. But, If you still have a strong preference for moving mapping to native I can go ahead with incorporating that as well.
If we move the mapping to native we will end up with code like this: mapped->buttons[kButtonPrimary] = input.buttons[63]; mapped->buttons[kButtonSecondary] = input.buttons[64]; where 63 is KeyEvent.KEYCODE_BUTTON_X and 64 is KeyEvent.KEYCODE_BUTTON_Y. So we either need to replicate those constants in native or add a bunch of comments. And we will need the same amount of code, there is no way to merge it with other platforms, just as every other platforms has it's own mapping code. I think it makes more sense to keep it in java.
https://codereview.chromium.org/133943002/diff/1590001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/133943002/diff/1590001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:39: } On 2014/04/04 15:10:17, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1590001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:43: JNIEnv* env = AttachCurrentThread(); On 2014/04/04 15:10:17, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1590001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:47: pads->length = 0; On 2014/04/04 15:10:17, jdduke wrote: Yes, that sounds better. https://codereview.chromium.org/133943002/diff/1590001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:63: jstring mapping, On 2014/04/04 15:10:17, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1590001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/133943002/diff/1590001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1701: if (GamepadList.dispatchKeyEvent(event)) { On 2014/04/04 15:10:17, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1590001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1590001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:133: mMapping = GamepadMappings.mapToStandardGamepad(mAxisValues, mButtonsValues, mRawAxes, On 2014/04/04 15:10:17, jdduke wrote: As earlier we were passing all the gamepad data after mapping to standard format, that why i decided to pass it as a string. Now changed to boollean. https://codereview.chromium.org/133943002/diff/1590001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1590001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:208: if (index == -1) { On 2014/04/04 15:10:17, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1590001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:283: private void isGamepadAccessed(boolean isGamepadAccessed) { On 2014/04/04 15:10:17, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1590001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java (right): https://codereview.chromium.org/133943002/diff/1590001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:14: class GamepadMappings { On 2014/04/04 15:10:17, jdduke wrote: In the current patch-set I have incorporated all other review comments expect moving mapping to native. Me and kbalazs have provided our reasons for keeping mapping in Java in our previous comments. If your opinion differs then please provide your feedback on it. https://codereview.chromium.org/133943002/diff/1590001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:18: protected static final int mRawShieldAxes = 10; On 2014/04/04 15:10:17, jdduke wrote: I have removed this check, it looks to me that checking the device name is suffiecient to determine a device. https://codereview.chromium.org/133943002/diff/1590001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:29: boolean isMapped = true; On 2014/04/04 15:10:17, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1701: if (GamepadList.dispatchKeyEvent(event)) { On 2014/04/04 18:52:18, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:5: package org.chromium.content.browser; On 2014/04/04 18:52:18, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:18: /* On 2014/04/04 18:52:18, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:21: On 2014/04/04 18:52:18, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:58: mMapping = " "; On 2014/04/04 18:52:18, Ted C wrote: Now i am keeping mMapping as boolean after review comment from Jdduke https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:82: GamepadDevice() {} On 2014/04/04 18:52:18, Ted C wrote: Removed it. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:85: * Map the axes and buttons of a gamepad device to a standard gamepad format. On 2014/04/04 18:52:18, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:88: mMapping = GamepadMappings.mapToStandardGamepad(mAxisValues, mButtonsValues, mRawAxes, On 2014/04/04 18:52:18, Ted C wrote: Keeping all paramaeters in one line after indenting 8 from start of above line. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:121: public float[] getAxes() { On 2014/04/04 18:52:18, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:130: On 2014/04/04 18:52:18, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:148: public boolean updateForEvent(KeyEvent event) { On 2014/04/04 18:52:18, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:169: // Update axes values. On 2014/04/04 18:52:18, Ted C wrote: Added a check https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadDevice.java:170: for (int i = 0; i < mAxes.length; ++i) { On 2014/04/04 18:52:18, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:25: private GamepadDevice[] mGamepadDevices = new GamepadDevice[MAX_GAMEPADS]; On 2014/04/04 18:52:18, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:26: private boolean mIsGamepadAccessed = false; On 2014/04/04 18:52:18, Ted C wrote: Removed https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:46: private static GamepadList sGamepadList = null; On 2014/04/04 18:52:18, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:123: for (int i = 0; i < MAX_GAMEPADS; ++i) { On 2014/04/04 18:52:18, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:139: for (int i = 0; i < MAX_GAMEPADS; ++i) { On 2014/04/04 18:52:18, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:163: private static boolean updateForEvent(KeyEvent event) { On 2014/04/04 18:52:18, Ted C wrote: Keeping non-static now. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:226: InputDevice.SOURCE_JOYSTICK); On 2014/04/04 18:52:18, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadList.java:239: On 2014/04/04 18:52:18, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java (right): https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:17: float[] rawAxes, float[] rawButtons, On 2014/04/04 18:52:18, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:26: else { On 2014/04/04 18:52:18, Ted C wrote: Removed else after review comments from Jdduke https://codereview.chromium.org/133943002/diff/1610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/GamepadMappings.java:36: rawButtons[KeyEvent.KEYCODE_BUTTON_B]; On 2014/04/04 18:52:18, Ted C wrote: Done.
Addressed review comments. Please tell, if i am missing anything.
https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:97: base::android::ConvertJavaStringToUTF16(env, devicename, &device_name); It's a shame to have to do this conversion every frame, but it's probably not a big deal for now. Could you write a "TODO: Store a cached WebGamePad object in GamepadPlatformDataFetcherAndroid and only update constant WebGamepad values when a device has changed." https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:136: pad.buttons[j].value = buttons[j]; Should this be |pad.buttons[j].value = buttons[j] ? 0.f : 1.f|? Looking at the implementations for all other platforms, they do something like that. https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher_android.h (right): https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:41: blink::WebGamepads* gamepads_data_; I think you can remove this variable now?
Mainly nits and styling. I do have a big concern about testing for gamepad
support. I'm really worried that if we have no test coverage for this then it
will get broken very quickly.
Have you given any thoughts on how we could test this feature? jdduke and I
discussed briefly and I think the best approach we could come up with is
allowing mocking out of the input devices (sadly they are all final in android,
so we'd need wrappers for all).
i.e. something like:
class InputManagerWrapper {
private mInputManager;
InputManagerWrapper(Context context) {
mInputManager = (InputManager)
context.getSystemService(Context.INPUT_SERVICE);
}
/** ... */
public InputDeviceWrapper getInputDevice(int id) {
InputDevice device = mInputManager.getInputDevice(id);
return device == null ? null : new InputDeviceWrapper(device);
}
/** ... */
public int[] getInputDeviceIds() {
return mInputManager.getInputDeviceIds();
}
....
}
class InputDeviceWrapper {
int getDeviceId();
List<MotionRangeWrapper> getMotionRanges();
...
}
class MotionRangeWrapper {
...
}
Then all your classes would deal with the Wrappers instead of the Android
versions. In GamepadList, we'd need to add the ability (either through a factory
or a static method) to override the InputManagerWrapper in tests that would
allow us to mock out all the input devices and ensure we can properly dispatch
gamepad events down to the page. We could simply test that the gamepad data is
being fetched as a simple plumbing test. We could even have a test html page
that prints out the gamepad values to some sort of javascript accessible dom
node and we could pull those values and verify they are what we expect (this is
trickier and has more chances of being flaky).
I don't know if this is something you feel like taking on or if jdduke or myself
could pick this up after this lands.
https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa...
File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right):
https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa...
content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:1: // Copyright
(c) 2014 The Chromium Authors. All rights reserved.
no (c) here either
https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa...
content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:44: bool) {
include the paramater name even if unused.
https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa...
content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:56: void
GamepadPlatformDataFetcherAndroid::PauseHint(bool isaccesspaused) {
variable name should match the header
https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa...
File content/browser/gamepad/gamepad_platform_data_fetcher_android.h (right):
https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa...
content/browser/gamepad/gamepad_platform_data_fetcher_android.h:1: // Copyright
(c) 2014 The Chromium Authors. All rights reserved.
no (c)
https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa...
content/browser/gamepad/gamepad_platform_data_fetcher_android.h:18: #include
"third_party/WebKit/public/platform/WebGamepads.h"
no blank line above this.
https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa...
content/browser/gamepad/gamepad_platform_data_fetcher_android.h:21: using
blink::WebGamepad;
you are fully qualifying blink::WebGamepads, so I don't think you need these
(and it doesn't look like you use blink::WebGamepad at all in this file).
https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa...
content/browser/gamepad/gamepad_platform_data_fetcher_android.h:30: //
polling_thread_.
polling_thread_ is a private class variable in gamepad_provider.cc, so I don't
think you should reference it here.
Maybe just say runs on the background polling thread.
And this comment doesn't really seem necessary to me. It seems more an
implementation description than when this should be used. I'd probably just
drop it.
https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa...
content/browser/gamepad/gamepad_platform_data_fetcher_android.h:31: virtual void
PauseHint(bool ispaused) OVERRIDE;
is_paused or just paused to match the overridden method signature
https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa...
content/browser/gamepad/gamepad_platform_data_fetcher_android.h:41:
blink::WebGamepads* gamepads_data_;
this doesn't look used in the implementation
https://codereview.chromium.org/133943002/diff/1630001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/GamepadDevice.java
(right):
https://codereview.chromium.org/133943002/diff/1630001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/GamepadDevice.java:16:
* Class to manage information related to each connected gamepad device.
No need for "Class to", this can just be "Manages"
https://codereview.chromium.org/133943002/diff/1630001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/GamepadDevice.java:26:
private boolean mMapping;
now that this is a boolean, I think mIsStandardGamepad is easier to understand.
although "standard" is somewhat confusing to me as well, but it looks like that
is
what native uses and expects.
update the getter as well.
https://codereview.chromium.org/133943002/diff/1630001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java
(right):
https://codereview.chromium.org/133943002/diff/1630001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:19:
@JNINamespace("content")
the annotations usually go right above the class definition (below the javadoc)
https://codereview.chromium.org/133943002/diff/1630001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:67:
mInputManager = (InputManager) context.getSystemService(Context.INPUT_SERVICE);
one too many spaces after the =
https://codereview.chromium.org/133943002/diff/1630001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:236:
return gamepad;
I think this is fine just as:
return getDeviceById(event.getDeviceId());
https://codereview.chromium.org/133943002/diff/1630001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:248:
* Specific handling for dpad keys is required because
these two lines are implementation specific. They should probably just be a
plain java comment near the code.
https://codereview.chromium.org/133943002/diff/1630001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:265:
static void getGamepadData(long gamepads) {
I would call this updateGamepadData because it doesn't return anything.
I would also change the gamepads variable to be either:
nativeWebGamepads or
webGamepadsPtr
Same below.
https://codereview.chromium.org/133943002/diff/1630001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:275:
device.mapButtonsAndAxes();
maybe updateButtonsAndAxesMapping
The verb "map" doesn't seem like something that needs to be done more than once
to me.
https://codereview.chromium.org/133943002/diff/1630001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:279:
else
the else needs to be on the line with the } from the if.
Since the if had braces, this also needs braces. And in java land, you always
braces unless the conditional and statement can all fit on the same line. ex:
if (blah = foo) something();
else if (blah == baz) somethingElse();
Often this isn't very legible, so we usually don't do that unless it is a single
"if"
https://codereview.chromium.org/133943002/diff/1630001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java
(right):
https://codereview.chromium.org/133943002/diff/1630001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:88:
mappedButtons[CanonicalButtonIndex.BUTTON_LEFT_TRIGGER] = rawAxes[2]; // or
rawAxes[3];
I'm confused of what the comment // or rawAxes[3] means. Is that actionable?
Reagarding testing of the feature i am thinking of creating a test HTML page that can show the connected devices and their capabilities in form of dom nodes. So this way we can have a visual and an easy way to check what the particular device is capable of. Apart from that i will be OOO for next one week and if anyone wants to go ahead and take this task I am also fine with it. I have addressed rest of the review comments in my latest patchset. https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/04/16 23:36:53, Ted C wrote: Again, i have seen similar copyright in other files like gamepad_platform_data_fetcher_linux.cc) https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:44: bool) { On 2014/04/16 23:36:53, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:56: void GamepadPlatformDataFetcherAndroid::PauseHint(bool isaccesspaused) { On 2014/04/16 23:36:53, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:97: base::android::ConvertJavaStringToUTF16(env, devicename, &device_name); On 2014/04/16 23:23:44, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:136: pad.buttons[j].value = buttons[j]; On 2014/04/16 23:23:44, jdduke wrote: We already store buttons data in either 1.0f or 0.0f format upon getting key event. Also for the conversion of axes to buttons value we have already put the check in form of |negativeAxisValueAsButton/positiveAxisValueAsButton| which maps to the expected values in GamepadMapping.java. So it is not needed here. https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher_android.h (right): https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/04/16 23:36:53, Ted C wrote: Is it? I notived that other files (gamepad_platform_data_fetcher_linux.h) are using this type of copyright "// Copyright (c) 2012" If you think it should be removed, i can do that. https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:18: #include "third_party/WebKit/public/platform/WebGamepads.h" On 2014/04/16 23:36:53, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:21: using blink::WebGamepad; On 2014/04/16 23:36:53, Ted C wrote: Removed it. https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:30: // polling_thread_. On 2014/04/16 23:36:53, Ted C wrote: Dropping the comment now. https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:31: virtual void PauseHint(bool ispaused) OVERRIDE; On 2014/04/16 23:36:53, Ted C wrote: Using paused https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:41: blink::WebGamepads* gamepads_data_; On 2014/04/16 23:36:53, Ted C wrote: Removed. https://codereview.chromium.org/133943002/diff/1630001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/GamepadDevice.java (right): https://codereview.chromium.org/133943002/diff/1630001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadDevice.java:16: * Class to manage information related to each connected gamepad device. On 2014/04/16 23:36:53, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1630001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadDevice.java:26: private boolean mMapping; On 2014/04/16 23:36:53, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1630001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1630001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:19: @JNINamespace("content") On 2014/04/16 23:36:53, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1630001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:67: mInputManager = (InputManager) context.getSystemService(Context.INPUT_SERVICE); On 2014/04/16 23:36:53, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1630001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:236: return gamepad; On 2014/04/16 23:36:53, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1630001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:248: * Specific handling for dpad keys is required because On 2014/04/16 23:36:53, Ted C wrote: Moved to switch case near dpad case check. https://codereview.chromium.org/133943002/diff/1630001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:265: static void getGamepadData(long gamepads) { On 2014/04/16 23:36:53, Ted C wrote: Using webGamepadsPtr now https://codereview.chromium.org/133943002/diff/1630001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:275: device.mapButtonsAndAxes(); On 2014/04/16 23:36:53, Ted C wrote: Using updateButtonsAndAxesMapping now. https://codereview.chromium.org/133943002/diff/1630001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:279: else On 2014/04/16 23:36:53, Ted C wrote: THanks for the explanation. Now formatting in the same way. https://codereview.chromium.org/133943002/diff/1630001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java (right): https://codereview.chromium.org/133943002/diff/1630001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:88: mappedButtons[CanonicalButtonIndex.BUTTON_LEFT_TRIGGER] = rawAxes[2]; // or rawAxes[3]; On 2014/04/16 23:36:53, Ted C wrote: No there is no action. Actually the axes value for left trigger button is duplicated at index 2,3 and for right trigger button it is duplicated at index 6,7. So I just added comment with intention to state that any of these 2 indices can be used to map to button value. I think I should remove it as it can create confusion.
On 2014/04/18 14:51:55, SaurabhK(OOO_27Apr) wrote: > Reagarding testing of the feature i am thinking of creating a test HTML page > that can show the connected devices and their capabilities in form of dom nodes. > So this way we can have a visual and an easy way to check what the particular > device is capable of. Isn't it the same as the "Gamepad Tester": http://www.html5rocks.com/en/tutorials/doodles/gamepad/gamepad-tester/tester.... ? I agree that autotesting would be useful for gamepad but I think this is way beyond the scope of this patch.
https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher_android.h (right): https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/04/18 14:51:57, SaurabhK(OOO_27Apr) wrote: > On 2014/04/16 23:36:53, Ted C wrote: > Is it? > I notived that other files (gamepad_platform_data_fetcher_linux.h) are using > this type of copyright "// Copyright (c) 2012" > If you think it should be removed, i can do that. (c) is deprecated. It's there in a lot of files but new files shouldn't use. https://codereview.chromium.org/133943002/diff/1630001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java (right): https://codereview.chromium.org/133943002/diff/1630001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:88: mappedButtons[CanonicalButtonIndex.BUTTON_LEFT_TRIGGER] = rawAxes[2]; // or rawAxes[3]; On 2014/04/18 14:51:57, SaurabhK(OOO_27Apr) wrote: > On 2014/04/16 23:36:53, Ted C wrote: > > No there is no action. > Actually the axes value for left trigger button is duplicated at index 2,3 and > for right trigger button it is duplicated at index 6,7. > So I just added comment with intention to state that any of these 2 indices can > be used to map to button value. > I think I should remove it as it can create confusion. I agree :)
content/browser/gamepad lgtm with a few nits. Please also run "git cl format" on your patch to catch any C++ styling issues. https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:97: base::android::ConvertJavaStringToUTF16(env, devicename, &device_name); On 2014/04/18 14:51:57, SaurabhK(OOO_27Apr) wrote: > On 2014/04/16 23:23:44, jdduke wrote: > > Done. Hmm, is the "Done." part in a patch that hasn't been uploaded? https://codereview.chromium.org/133943002/diff/1650001/content/browser/gamepa... File content/browser/gamepad/canonical_button_index_list.h (right): https://codereview.chromium.org/133943002/diff/1650001/content/browser/gamepa... content/browser/gamepad/canonical_button_index_list.h:8: Could you add a "// TODO: Consolidate with CanonicalButtonIndex enum in gamepad_standard_mappings.h, crbug.com/351558.", and a similar comment in canonical_axis_index_list.h? https://codereview.chromium.org/133943002/diff/1650001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher_android.h (right): https://codereview.chromium.org/133943002/diff/1650001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:20: using blink::WebGamepads; Nit: using Foo; is best left out of headers, and it looks like you fully qualify the reference here, so you should be OK just removing this.
https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher_android.cc (right): https://codereview.chromium.org/133943002/diff/1630001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.cc:97: base::android::ConvertJavaStringToUTF16(env, devicename, &device_name); On 2014/04/21 19:53:58, jdduke wrote: I meant that i have introduced a TODO in the next patch-set (patch-set 20). https://codereview.chromium.org/133943002/diff/1650001/content/browser/gamepa... File content/browser/gamepad/canonical_button_index_list.h (right): https://codereview.chromium.org/133943002/diff/1650001/content/browser/gamepa... content/browser/gamepad/canonical_button_index_list.h:8: On 2014/04/21 19:53:58, jdduke wrote: Added TODO here and canonical_axis_index_list.h https://codereview.chromium.org/133943002/diff/1650001/content/browser/gamepa... File content/browser/gamepad/gamepad_platform_data_fetcher_android.h (right): https://codereview.chromium.org/133943002/diff/1650001/content/browser/gamepa... content/browser/gamepad/gamepad_platform_data_fetcher_android.h:20: using blink::WebGamepads; On 2014/04/21 19:53:58, jdduke wrote: Removed it
lgtm -- we'll coordinate testing for this in a subsequent patch. I want to have tests in before the next stable release, but that is fairly far off so we have a decent runway. https://codereview.chromium.org/133943002/diff/1670001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1670001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:33: private GamepadDevice[] mGamepadDevices = new GamepadDevice[MAX_GAMEPADS]; this can be final
Adding aelias@ who probably has more gamepad experience than the rest of us...
I'm wondering what happens in practice when a user tries to plug in an unknown gamepad, like a PS3 controller or a Logitech Dual Action for example? Does it not work at all or does it end up with scrambled mappings? https://codereview.chromium.org/133943002/diff/1670001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java (right): https://codereview.chromium.org/133943002/diff/1670001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:114: private static void mapUnkownGamepad(float[] mappedButtons, float[] rawButtons, nit: should be "Unknown"
>> I'm wondering what happens in practice when a user tries to plug in an unknown gamepad, like a PS3 controller or a Logitech Dual Action for example? Does it not work at all or does it end up with scrambled mappings? When any unknown gamepad is connected then the buttons and axes will be exposed in a format provided by a mapper for unknown gamepads that should map most of the keys but there is a possibility that some buttons and axes dont get mapped properly and thus these will be shown as extra buttons or extra axes. https://codereview.chromium.org/133943002/diff/1670001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1670001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:33: private GamepadDevice[] mGamepadDevices = new GamepadDevice[MAX_GAMEPADS]; On 2014/04/22 19:33:24, Ted C wrote: Done. https://codereview.chromium.org/133943002/diff/1670001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java (right): https://codereview.chromium.org/133943002/diff/1670001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:114: private static void mapUnkownGamepad(float[] mappedButtons, float[] rawButtons, On 2014/04/22 23:46:26, aelias wrote: Done.
On 2014/04/24 14:00:02, SaurabhK(OOO_27Apr) wrote: > When any unknown gamepad is connected then the buttons and axes will be exposed > in a format provided by a mapper for unknown gamepads that should map most of > the keys but there is a possibility that some buttons and axes dont get mapped > properly and thus these will be shown as extra buttons or extra axes. This is, by the way, consistent with desktop Gamepad support. We expose as many buttons/axes as possible but don't guarantee the order unless the "mapping" property is set to a non-empty string. This allows the browser to access the long tail of devices that are out there through in-game controller configuration screens and similar. It wouldn't be practical to only expose gamepads that had been previously mapped.
The CQ bit was checked by skhatri@nvidia.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skhatri@nvidia.com/133943002/1690001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for android_webview/Android.mk: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file android_webview/Android.mk Hunk #1 FAILED at 59. 1 out of 1 hunk FAILED -- saving rejects to file android_webview/Android.mk.rej Patch: android_webview/Android.mk Index: android_webview/Android.mk diff --git a/android_webview/Android.mk b/android_webview/Android.mk index 62483521771e2195e39743fdd8a7357f78c94d3f..df679fb502d2378e70951ceafcfabaf3f4366af0 100644 --- a/android_webview/Android.mk +++ b/android_webview/Android.mk @@ -59,6 +59,8 @@ $(call intermediates-dir-for,GYP,shared)/templates/org/chromium/base/MemoryPress $(call intermediates-dir-for,GYP,shared)/templates/org/chromium/content/browser/GestureEventType.java \ $(call intermediates-dir-for,GYP,shared)/templates/org/chromium/content/browser/PageTransitionTypes.java \ $(call intermediates-dir-for,GYP,shared)/templates/org/chromium/content/browser/SpeechRecognitionError.java \ +$(call intermediates-dir-for,GYP,shared)/templates/org/chromium/content/browser/input/gamepad_mapping/CanonicalAxisIndex.java \ +$(call intermediates-dir-for,GYP,shared)/templates/org/chromium/content/browser/input/gamepad_mapping/CanonicalButtonIndex.java \ $(call intermediates-dir-for,GYP,shared)/templates/org/chromium/content/browser/input/PopupItemType.java \ $(call intermediates-dir-for,GYP,shared)/templates/org/chromium/content/common/ResultCodes.java \ $(call intermediates-dir-for,GYP,shared)/templates/org/chromium/media/ImageFormat.java \
The CQ bit was checked by skhatri@nvidia.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skhatri@nvidia.com/133943002/1710001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium
The CQ bit was checked by skhatri@nvidia.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skhatri@nvidia.com/133943002/1730001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium
The CQ bit was checked by skhatri@nvidia.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skhatri@nvidia.com/133943002/1750001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium
build failures are not flakes, re-checking cq is not going to do anything
I finally gained access to an XBox 360 device to do some basic testing. It seems to work on the provided HTML5 gamepad test link. However, it appears that once polling starts for the device, normal DPAD input for navigating pages will not work in any tab until you restart the browser.
On 2014/04/29 15:38:09, jdduke wrote: > I finally gained access to an XBox 360 device to do some basic testing. It > seems to work on the provided HTML5 gamepad test link. However, it appears that > once polling starts for the device, normal DPAD input for navigating pages will > not work in any tab until you restart the browser. Nevermind this comment, DPAD navigation works just fine after the final tab that's doing polling really closes (I wasn't waiting long enough for the tab to truly terminate with the delayed "Undo" close feature).
On 2014/04/29 15:40:11, jdduke wrote: > On 2014/04/29 15:38:09, jdduke wrote: > > I finally gained access to an XBox 360 device to do some basic testing. It > > seems to work on the provided HTML5 gamepad test link. However, it appears > that > > once polling starts for the device, normal DPAD input for navigating pages > will > > not work in any tab until you restart the browser. > > Nevermind this comment, DPAD navigation works just fine after the final tab > that's doing polling really closes (I wasn't waiting long enough for the tab to > truly terminate with the delayed "Undo" close feature). Uh, it still doesn't sound very correct. Why do we deliver the gamepad event to an onfocused tab? (Anyway, I guess we should continue this discussion in the bug.)
The CQ bit was checked by skhatri@nvidia.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skhatri@nvidia.com/133943002/1770001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
Adding mnaganov, brettw and yfriedman
as the patch misses review for :-
android_webview/Android.mk
android_webview/android_webview.gyp
base/android/jni_generator/jni_generator.py
content/child/runtime_features.cc
content/content.gyp
android_webview lgtm
base/android/jni_generator/ lgtm
content.gypi lgtm
The CQ bit was checked by skhatri@nvidia.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skhatri@nvidia.com/133943002/1770001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for content/browser/android/browser_jni_registrar.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file content/browser/android/browser_jni_registrar.cc
Hunk #2 succeeded at 60 (offset 3 lines).
Hunk #3 FAILED at 86.
1 out of 3 hunks FAILED -- saving rejects to file
content/browser/android/browser_jni_registrar.cc.rej
Patch: content/browser/android/browser_jni_registrar.cc
Index: content/browser/android/browser_jni_registrar.cc
diff --git a/content/browser/android/browser_jni_registrar.cc
b/content/browser/android/browser_jni_registrar.cc
index
fca55a936e2c7254754a8443a1a752133afdf85f..0bce2e598af559d064ed39b3736256b07dee082f
100644
--- a/content/browser/android/browser_jni_registrar.cc
+++ b/content/browser/android/browser_jni_registrar.cc
@@ -25,6 +25,7 @@
#include "content/browser/battery_status/battery_status_manager_android.h"
#include "content/browser/device_sensors/sensor_manager_android.h"
#include "content/browser/frame_host/navigation_controller_android.h"
+#include "content/browser/gamepad/gamepad_platform_data_fetcher_android.h"
#include "content/browser/geolocation/location_api_adapter_android.h"
#include "content/browser/media/android/media_drm_credential_manager.h"
#include "content/browser/media/android/media_resource_getter_impl.h"
@@ -56,6 +57,8 @@ base::android::RegistrationMethod kContentRegisteredMethods[]
= {
{"DateTimePickerAndroid", content::RegisterDateTimeChooserAndroid},
{"DownloadControllerAndroidImpl",
content::DownloadControllerAndroidImpl::RegisterDownloadController},
+ {"GamepadList", content::GamepadPlatformDataFetcherAndroid::
+ RegisterGamepadPlatformDataFetcherAndroid},
{"InterstitialPageDelegateAndroid",
content::InterstitialPageDelegateAndroid::
RegisterInterstitialPageDelegateAndroid},
@@ -83,7 +86,8 @@ base::android::RegistrationMethod kContentRegisteredMethods[]
= {
{"WebContentsObserverAndroid",
content::RegisterWebContentsObserverAndroid},
{"WebViewStatics", content::RegisterWebViewStatics},
{"BatterStatusManagerAndroid",
- content::BatteryStatusManagerAndroid::Register}, };
+ content::BatteryStatusManagerAndroid::Register},
+};
} // namespace
The CQ bit was checked by skhatri@nvidia.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skhatri@nvidia.com/133943002/1790001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was checked by skhatri@nvidia.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skhatri@nvidia.com/133943002/1790001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
https://codereview.chromium.org/133943002/diff/1790001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1790001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:29: private static GamepadList sGamepadList = null; It looks like we're initializing this variable on one thread and reading it on another, neither of which operations are guarded by a lock. Do we have any guarantees that the calls from native by the polling thread will always be made after the first call to |onAttachedToWindow|? Ted, would a volatile be appropriate here (at the very least) to at least make sure the assert in the native static calls gets a sensible value?
On 2014/05/12 15:25:32, jdduke wrote: According to the android developer spec ( http://developer.android.com/reference/android/view/View.html ) it is said that the |onAttachedToWindow| is guaranteed to be called before onDraw(android.graphics.Canvas). So i think that calls from polling thread should also be made after |onAttachedToWindow|. However i am not sure about any of the exception. Ted, please provide your opinion on the use of volatile here.
On 2014/05/13 09:19:40, SaurabhK wrote: > On 2014/05/12 15:25:32, jdduke wrote: > > According to the android developer spec ( > http://developer.android.com/reference/android/view/View.html ) it is said that > the |onAttachedToWindow| is guaranteed to be called before > onDraw(android.graphics.Canvas). So i think that calls from polling thread > should also be made after |onAttachedToWindow|. However i am not sure about any > of the exception. > Ted, please provide your opinion on the use of volatile here. Android webview dev here. You can't assume that onDraw is only called after onAttachedToWindow. That spec only outlines what android system library will do, not what apps are allowed to do, and apps are free to call onDraw at any time it likes. I don't think it's relevant to the your discussion though, just wanted to clarify.
On 2014/05/13 15:40:38, boliu wrote: > On 2014/05/13 09:19:40, SaurabhK wrote: > > On 2014/05/12 15:25:32, jdduke wrote: > > > > According to the android developer spec ( > > http://developer.android.com/reference/android/view/View.html ) it is said > that > > the |onAttachedToWindow| is guaranteed to be called before > > onDraw(android.graphics.Canvas). So i think that calls from polling thread > > should also be made after |onAttachedToWindow|. However i am not sure about > any > > of the exception. > > Ted, please provide your opinion on the use of volatile here. > > Android webview dev here. You can't assume that onDraw is only called after > onAttachedToWindow. That spec only outlines what android system library will do, > not what apps are allowed to do, and apps are free to call onDraw at any time it > likes. I don't think it's relevant to the your discussion though, just wanted to > clarify. Actually, this raises a good point. What is to say that a WebView developer couldn't create an instance that would start polling for gamepad information (without ever being attached to a window). Might be better to switch to something like: private static class LazyHolder { private static final GamepadList INSTANCE = new GamepadList(); } public static GamepadList getInstance() { return LazyHolder.INSTANCE; } Then use getInstance() everywhere for thread safe creation. The initialization of GamepadList looks quite trivial, so there shouldn't be any harm in doing it this way instead.
The CQ bit was checked by skhatri@nvidia.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skhatri@nvidia.com/133943002/1830001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by skhatri@nvidia.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skhatri@nvidia.com/133943002/1830001
On 2014/05/15 06:42:49, SaurabhK wrote: Re-checking cq as the same bots were passing in earlier patch-set. Side by side i am looking for ways to reproduce the issue at my end to see if there is any real issue.
Message was sent while issue was closed.
Change committed as 270620
Message was sent while issue was closed.
https://codereview.chromium.org/133943002/diff/1830001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1830001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:9: import android.hardware.input.InputManager.InputDeviceListener; This breaks Chrome on ICS devices. It requires API level 16, but ICS has level 14. developer.android.com/reference/android/hardware/input/InputManager.InputDeviceListener.html Can you fix it ASAP?
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/298483002/ by aelias@chromium.org. The reason for reverting is: Gamepad list lookup causes a startup crash on Ice Cream Sandwich devices. Please reland after adding the appropriate version check and testing locally (note: you don't need to get OWNERS approval a second time when relanding if you reupload to this codereview entry instead of creating a new one)..
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/284413005/ by kkimlabs@chromium.org. The reason for reverting is: This is failing on ICS because InputDeviceListener is API 16. Please fix it and resubmit :). (e.g., API guarding like used in ApiCompatibilityUtils.java).
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/292763004/ by kkimlabs@chromium.org. The reason for reverting is: This is failing on ICS because InputDeviceListener is API 16. Please fix it and resubmit :). (e.g., API guarding like in ApiCompatibilityUtils.java).
Message was sent while issue was closed.
Sorry for all the conflicting reverts. The one that landed is https://codereview.chromium.org/298483002/
On 2014/05/19 19:38:36, aelias wrote: I am looking for an API similar to InputDeviceListener for API level 14 but i am unable to find any, does anyone know of any similar API ? Boliu, do you have any idea about it? I can in that case go ahead with an API guarding.
On 2014/05/20 08:10:36, SaurabhK wrote: > On 2014/05/19 19:38:36, aelias wrote: > > I am looking for an API similar to InputDeviceListener for API level 14 but i am > unable to find any, does anyone know of any similar API ? > Boliu, do you have any idea about it? > I can in that case go ahead with an API guarding. I was playing with a potential fix yesterday, I'll upload as an example of how to (hopefully) fix the issue (although we'll want to double check with tedchoc@).
The way the android jvm works, it will only look up a method when it is used. So as long as you don't call a something that's "not there", eg a jellybean method on ics device, then everything is fine. This is how ApiCompatibilityUtils work: https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...
On 2014/05/20 15:35:16, boliu wrote: The revert commit of this change is not reflecting here on the latest synced tree. The log only shows commits till revision 2711107 while the revert change is committed as 271452. May be there is an issue with the server and it is not updated. Will upload the patch once the code is updated on top-of-tree.
On 2014/05/21 12:06:11, SaurabhK wrote: It happens because of known issue being discussed in https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/infras... Looks like the issue is sorted for me but the sync is extremely slow and taking a lot of time.
On 2014/05/26 14:45:24, SaurabhK wrote: I am unable to get acess to an ICS device, so i cant test it for ICS.
The CQ bit was checked by skhatri@nvidia.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skhatri@nvidia.com/133943002/1850001
The CQ bit was unchecked by jdduke@chromium.org
You're not actually using the |kDisableGamepad| flag, so this won't fix anything. It looks like you use the SDK version check in runtime_features.cc, that should be sufficient so you can get rid of the |kDisableGamepad| flag and just always use the version check.
On 2014/05/27 17:31:09, jdduke wrote: jdduke, does the latest patch-set looks perfect?
I'll apply this patch locally and test on an ICS device to be sure, please wait for my response before committing. https://codereview.chromium.org/133943002/diff/1870001/content/child/runtime_... File content/child/runtime_features.cc (right): https://codereview.chromium.org/133943002/diff/1870001/content/child/runtime_... content/child/runtime_features.cc:95: Nit: Remove this extra newline. https://codereview.chromium.org/133943002/diff/1870001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1870001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:77: assert ThreadUtils.runningOnUiThread(); if (!isGamepadSupported()) return; https://codereview.chromium.org/133943002/diff/1870001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:96: assert ThreadUtils.runningOnUiThread(); if (!isGamepadSupported()) return;
On 2014/05/29 17:46:34, jdduke wrote: > I'll apply this patch locally and test on an ICS device to be sure, please wait > for my response before committing. > > https://codereview.chromium.org/133943002/diff/1870001/content/child/runtime_... > File content/child/runtime_features.cc (right): > > https://codereview.chromium.org/133943002/diff/1870001/content/child/runtime_... > content/child/runtime_features.cc:95: > Nit: Remove this extra newline. > > https://codereview.chromium.org/133943002/diff/1870001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java > (right): > > https://codereview.chromium.org/133943002/diff/1870001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:77: > assert ThreadUtils.runningOnUiThread(); > if (!isGamepadSupported()) return; > > https://codereview.chromium.org/133943002/diff/1870001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:96: > assert ThreadUtils.runningOnUiThread(); > if (!isGamepadSupported()) return; OK, with the changes mentioned above, this runs fine on ICS (or rather, it doesn't run (support is disabled), but it doesn't crash either).
https://codereview.chromium.org/133943002/diff/1870001/content/child/runtime_... File content/child/runtime_features.cc (right): https://codereview.chromium.org/133943002/diff/1870001/content/child/runtime_... content/child/runtime_features.cc:95: On 2014/05/29 17:46:36, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1870001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1870001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:77: assert ThreadUtils.runningOnUiThread(); On 2014/05/29 17:46:36, jdduke wrote: Done. https://codereview.chromium.org/133943002/diff/1870001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:96: assert ThreadUtils.runningOnUiThread(); On 2014/05/29 17:46:36, jdduke wrote: Done.
The CQ bit was checked by skhatri@nvidia.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skhatri@nvidia.com/133943002/1890001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/10940) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/14008)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
On 2014/05/30 14:09:15, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_gpu on tryserver.chromium.gpu > (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) Looks like a legit failure, you'll want to move "#include "base/android/build_info.h"" in runtime_features.cc to the Android include guard (there's already one in that file, just group it with those).
On 2014/05/30 14:44:59, jdduke wrote: Patchset 32 : Fix for linux_gpu on tryserver.chromium.gpu and Rebase
The CQ bit was checked by skhatri@nvidia.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skhatri@nvidia.com/133943002/1910001
Message was sent while issue was closed.
Change committed as 274212
Message was sent while issue was closed.
https://codereview.chromium.org/133943002/diff/1910001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java (right): https://codereview.chromium.org/133943002/diff/1910001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:243: return ((inputDevice.getSources() & InputDevice.SOURCE_JOYSTICK) == inputDevice could be null, see http://developer.android.com/reference/android/view/InputDevice.html#getDevic... and crash: https://code.google.com/p/chromium/issues/detail?id=421168 |
