Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1276703003:
Handle UI operations through gamepad (Closed)
DescriptionHandle UI operations through gamepad
This change allows user to handle UI operations such as
tab navigation, close tab, focus URL bar, open menu, move
to previous/next link through gamepad buttons.
BUG=454355
Committed: https://crrev.com/0da8c6edf6af93a5ab7d347341a677c83d5e6282
Cr-Commit-Position: refs/heads/master@{#344434}
Patch Set 1 #
Total comments: 4
Patch Set 2 : fixed previous comments #
Total comments: 2
Patch Set 3 : fixed previous comments #
Total comments: 2
Patch Set 4 : Renaming mIsGamepadAccessed variable and notifyForGamepadsAccess method #
Total comments: 2
Patch Set 5 : reversed gamepad activation logic #Patch Set 6 : rebase #
Messages
Total messages: 27 (6 generated)
sshelke@nvidia.com changed reviewers: + jdduke@chromium.org, newt@chromium.org
https://codereview.chromium.org/1276703003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java (right): https://codereview.chromium.org/1276703003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java:37: private static boolean isGamepadAPIActive(KeyEvent event, ChromeActivity activity) { I don't think we need the KeyEvent arg here, do we? https://codereview.chromium.org/1276703003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java:119: && !(KeyEvent.isGamepadButton(keyCode) && !isGamepadAPIActive(event, activity))) { This big if statement is getting pretty meaty, hmmm. One idea might be: if (KeyEvent.isGamepadButton(keyCode)) { if (isGamepadAPIActive(activity)) return false; } else if (!event.isCtrlPressed()...) { ... https://codereview.chromium.org/1276703003/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1276703003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1177: public boolean isGamepadAccessed() { Nit: Javadoc :) Actually, I'm wondering if there's a better term than "isGamepadAccessed". That doesn't really tell the caller what's going on. Maybe "isGamepadAPIActive" or "isGamepadAPIEngaged" or something like that? https://codereview.chromium.org/1276703003/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java (right): https://codereview.chromium.org/1276703003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:301: return getInstance().mIsGamepadAccessed; Can you move this method up below "isGamepadEvent", and also add a Javadoc? Ideally it would describe what it means to be "accessed", i.e., "@return whether the HTML5 gamepad API is active" or something like that.
Fixed previous comments.
https://codereview.chromium.org/1276703003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java (right): https://codereview.chromium.org/1276703003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java:38: if (activity.getCurrentContentViewCore() != null) { nit: store the reference to the ContentViewCore (since activity.getCurrentContentViewCore() is not a trivial getter): ContentViewCore viewCore = activity.getCurrentContentViewCore(); return viewCore == null ? false : viewCore.isGamepadAPIActive();
Fixed comments. https://codereview.chromium.org/1276703003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java (right): https://codereview.chromium.org/1276703003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java:38: if (activity.getCurrentContentViewCore() != null) { On 2015/08/10 18:52:07, newt wrote: > nit: store the reference to the ContentViewCore (since > activity.getCurrentContentViewCore() is not a trivial getter): > > ContentViewCore viewCore = activity.getCurrentContentViewCore(); > return viewCore == null ? false : viewCore.isGamepadAPIActive(); Done.
OK, thanks. This looks fine, my last question is whether these keys ever normally reach web content as standard key events (http://www.w3schools.com/jsref/event_onkeydown.asp). Basically, can a web app currently listen to and handle these events as key events (outside of the HTML5 API)? Or do we not send them at all to web content?
https://codereview.chromium.org/1276703003/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java (right): https://codereview.chromium.org/1276703003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:252: public static boolean isGamepadAccessed() { When does this method return true? Whenever a gamepad is connected? Or only when the webpage is using the HTML5 gamepad APIs?
https://codereview.chromium.org/1276703003/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java (right): https://codereview.chromium.org/1276703003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:252: public static boolean isGamepadAccessed() { On 2015/08/11 18:51:15, newt wrote: > When does this method return true? Whenever a gamepad is connected? Or only when > the webpage is using the HTML5 gamepad APIs? Only when the HTML5 gamepad API is in use. We should probably rename the member variable and method to reference "API active" instead of "accessed" as in ContentViewCore, so mIsGamepadAccessed -> mIsGamepadAPIActive and notifyForGamepadsAccess -> setGamepadAPIActive.
On 2015/08/11 20:15:00, jdduke wrote: > https://codereview.chromium.org/1276703003/diff/40001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java > (right): > > https://codereview.chromium.org/1276703003/diff/40001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:252: > public static boolean isGamepadAccessed() { > On 2015/08/11 18:51:15, newt wrote: > > When does this method return true? Whenever a gamepad is connected? Or only > when > > the webpage is using the HTML5 gamepad APIs? > > Only when the HTML5 gamepad API is in use. We should probably rename the member > variable and method to reference "API active" instead of "accessed" as in > ContentViewCore, so mIsGamepadAccessed -> mIsGamepadAPIActive and > notifyForGamepadsAccess -> setGamepadAPIActive. Renamed mIsGamepadAccessed to mIsGamepadAPIActive and notifyForGamepadsAccess to setGamepadAPIActive for clear understanding of functionality .
On 2015/08/11 15:25:43, jdduke wrote: > OK, thanks. This looks fine, my last question is whether these keys ever > normally reach web content as standard key events > (http://www.w3schools.com/jsref/event_onkeydown.asp). Basically, can a web app > currently listen to and handle these events as key events (outside of the HTML5 > API)? Or do we not send them at all to web content? If gamepad events are handled in KeyboardShtortcuts then those events wont be passed to webcontents. KeyboardShortcut consumes those events. The behavior is similar to F3 and F5 keys. Remaining buttons which are not handled by KeyboardShortcuts such as BUTTON_A is forwarded to webcontents for further handling.
lgtm, but please wait for newt@'s approval. Let's land this and see how it goes, it's possible there may have been sites that relied on these key events to allow non-HTML5 gamepad interaction, but I imagine the new shortcuts will be more useful.
https://codereview.chromium.org/1276703003/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java (right): https://codereview.chromium.org/1276703003/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:303: static void setGamepadAPIActive(boolean isAccessPaused) { This method parameter should be reversed, i.e. isAccessPaused should be renamed to isActive, the "!" below should be removed, and whoever calls this API should pass the opposite of whatever they're currently passing. https://codereview.chromium.org/1276703003/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/GamepadList.java:307: private void setIsGamepadAccessed(boolean isGamepadAccessed) { replace "accessed" with "active" here too
Reversed the logic which notifies for gamepad activation in gamepad_data_fetcher. Also renamed variable names mentioned in previous comments.
lgtm. Thanks!
The CQ bit was checked by sshelke@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1276703003/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1276703003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1276703003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
sshelke@nvidia.com changed reviewers: + scottmg@chromium.org
Thanks newt. Adding scottmg for review.
content/browser/gamepad/gamepad_platform_data_fetcher_android.cc lgtm (didn't look at the others)
On 2015/08/19 16:08:41, scottmg wrote: > content/browser/gamepad/gamepad_platform_data_fetcher_android.cc lgtm (didn't > look at the others) Thanks Scottmg.
The CQ bit was checked by sshelke@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1276703003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1276703003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/0da8c6edf6af93a5ab7d347341a677c83d5e6282 Cr-Commit-Position: refs/heads/master@{#344434} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
