Expose Gamepad API instance for Cardboard button

Expose Gamepad API instance for Cardboard button

This change exposes a gamepad with a single button, which is driven by the touch screen.

BUG=718665
Review-Url: https://codereview.chromium.org/2879973002
Cr-Commit-Position: refs/heads/master@{#473986}
Committed: https://chromium.googlesource.com/chromium/src/+/6ca5bedb3683ccf3db53fbad5bb93c60233c368b

[billorr1, 2017-05-12 18:25:39]

billorr@google.com changed reviewers:
+ billorr@google.com, mthiesse@chromium.org

[billorr1, 2017-05-12 18:25:42]

Michael, please take a look.  I know I'll have to rebase on your controller
refactor, but curious what you think of this approach.  I wanted to reuse
existing plumbing/infrastructure rather than create a new gamepad type, but I'm
a bit on the fence for whether this is better than having a separate
implementation.

https://codereview.chromium.org/2879973002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2879973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:238:
&& (event.getActionMasked() == MotionEvent.ACTION_UP
nit: must have tabs instead of spaces... fix this

https://codereview.chromium.org/2879973002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2879973002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:519: 
nit: remove this extra blank line

https://codereview.chromium.org/2879973002/diff/1/device/vr/android/gvr/gvr_g...
File device/vr/android/gvr/gvr_gamepad_data_provider.h (right):

https://codereview.chromium.org/2879973002/diff/1/device/vr/android/gvr/gvr_g...
device/vr/android/gvr/gvr_gamepad_data_provider.h:18: gfx::Vector2dF touch_pos; 
// controller touch position
nit: comments should be complete sentences

[billorr1, 2017-05-15 23:04:17]

https://codereview.chromium.org/2879973002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2879973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:238:
&& (event.getActionMasked() == MotionEvent.ACTION_UP
On 2017/05/12 18:25:41, billorr1 wrote:
> nit: must have tabs instead of spaces... fix this

Done.

https://codereview.chromium.org/2879973002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2879973002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:519: 
On 2017/05/12 18:25:41, billorr1 wrote:
> nit: remove this extra blank line

Done.

https://codereview.chromium.org/2879973002/diff/1/device/vr/android/gvr/gvr_g...
File device/vr/android/gvr/gvr_gamepad_data_provider.h (right):

https://codereview.chromium.org/2879973002/diff/1/device/vr/android/gvr/gvr_g...
device/vr/android/gvr/gvr_gamepad_data_provider.h:18: gfx::Vector2dF touch_pos; 
// controller touch position
On 2017/05/12 18:25:42, billorr1 wrote:
> nit: comments should be complete sentences

Done.

[mthiesse, 2017-05-16 00:44:38]

We really shouldn't be re-using the gvrGamepadDataFetcher for this. We should
add a much simpler cardboardGamepadDataFetcher that doesn't go through
vr_shell_gl, because there's no reason to.

https://codereview.chromium.org/2879973002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2879973002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:231:
if (!CommandLine.getInstance().hasSwitch(ChromeSwitches.ENABLE_VR_SHELL_DEV)
I think you can remove the switch checks. onTouch should only be called when
paired with a cardboard, even if the view is touched, so we should
unconditionally call OnTrigger.

As mentioned below we should just remove this old workaround.

https://codereview.chromium.org/2879973002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:407:
public boolean dispatchTouchEvent(MotionEvent event) {
Sorry, you touching this code has reminded me that we probably don't need this
old cardboard-using-daydream-view workaround.

I think you can delete this whole function, we shouldn't need to listen for
dispatchTouchEvent anymore.

https://codereview.chromium.org/2879973002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2879973002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:603:
device::GamepadDataFetcherManager::GetInstance()->AddFactory(
sidenote, gamepad API is scary :P Add twice, remove once, and magically
everything is fine.

https://codereview.chromium.org/2879973002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.h (right):

https://codereview.chromium.org/2879973002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.h:233: // Registered fetcher, must
remain alive for UpdateGamepadData calls.
nit: fetchers

[billorr1, 2017-05-17 01:21:28]

On 2017/05/16 00:44:38, mthiesse wrote:
> We really shouldn't be re-using the gvrGamepadDataFetcher for this. We should
> add a much simpler cardboardGamepadDataFetcher that doesn't go through
> vr_shell_gl, because there's no reason to.
> 
>
https://codereview.chromium.org/2879973002/diff/20001/chrome/android/java/src...
> File
> chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
> (right):
> 
>
https://codereview.chromium.org/2879973002/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:231:
> if (!CommandLine.getInstance().hasSwitch(ChromeSwitches.ENABLE_VR_SHELL_DEV)
> I think you can remove the switch checks. onTouch should only be called when
> paired with a cardboard, even if the view is touched, so we should
> unconditionally call OnTrigger.
> 
> As mentioned below we should just remove this old workaround.
> 
>
https://codereview.chromium.org/2879973002/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:407:
> public boolean dispatchTouchEvent(MotionEvent event) {
> Sorry, you touching this code has reminded me that we probably don't need this
> old cardboard-using-daydream-view workaround.
> 
> I think you can delete this whole function, we shouldn't need to listen for
> dispatchTouchEvent anymore.
> 
>
https://codereview.chromium.org/2879973002/diff/20001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/vr_shell.cc (right):
> 
>
https://codereview.chromium.org/2879973002/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell.cc:603:
> device::GamepadDataFetcherManager::GetInstance()->AddFactory(
> sidenote, gamepad API is scary :P Add twice, remove once, and magically
> everything is fine.
> 
>
https://codereview.chromium.org/2879973002/diff/20001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/vr_shell.h (right):
> 
>
https://codereview.chromium.org/2879973002/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell.h:233: // Registered fetcher, must
> remain alive for UpdateGamepadData calls.
> nit: fetchers

Updated to use a new cardboard data fetcher.  I think this is "better", but is
more complex/more code.

[mthiesse, 2017-05-17 03:24:58]

On 2017/05/17 01:21:28, billorr1 wrote:
> On 2017/05/16 00:44:38, mthiesse wrote:
> > We really shouldn't be re-using the gvrGamepadDataFetcher for this. We
should
> > add a much simpler cardboardGamepadDataFetcher that doesn't go through
> > vr_shell_gl, because there's no reason to.
> > 
> >
>
https://codereview.chromium.org/2879973002/diff/20001/chrome/android/java/src...
> > File
> >
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2879973002/diff/20001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:231:
> > if (!CommandLine.getInstance().hasSwitch(ChromeSwitches.ENABLE_VR_SHELL_DEV)
> > I think you can remove the switch checks. onTouch should only be called when
> > paired with a cardboard, even if the view is touched, so we should
> > unconditionally call OnTrigger.
> > 
> > As mentioned below we should just remove this old workaround.
> > 
> >
>
https://codereview.chromium.org/2879973002/diff/20001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:407:
> > public boolean dispatchTouchEvent(MotionEvent event) {
> > Sorry, you touching this code has reminded me that we probably don't need
this
> > old cardboard-using-daydream-view workaround.
> > 
> > I think you can delete this whole function, we shouldn't need to listen for
> > dispatchTouchEvent anymore.
> > 
> >
>
https://codereview.chromium.org/2879973002/diff/20001/chrome/browser/android/...
> > File chrome/browser/android/vr_shell/vr_shell.cc (right):
> > 
> >
>
https://codereview.chromium.org/2879973002/diff/20001/chrome/browser/android/...
> > chrome/browser/android/vr_shell/vr_shell.cc:603:
> > device::GamepadDataFetcherManager::GetInstance()->AddFactory(
> > sidenote, gamepad API is scary :P Add twice, remove once, and magically
> > everything is fine.
> > 
> >
>
https://codereview.chromium.org/2879973002/diff/20001/chrome/browser/android/...
> > File chrome/browser/android/vr_shell/vr_shell.h (right):
> > 
> >
>
https://codereview.chromium.org/2879973002/diff/20001/chrome/browser/android/...
> > chrome/browser/android/vr_shell/vr_shell.h:233: // Registered fetcher, must
> > remain alive for UpdateGamepadData calls.
> > nit: fetchers
> 
> Updated to use a new cardboard data fetcher.  I think this is "better", but is
> more complex/more code.

Did you forget to post changes?

[mthiesse, 2017-05-18 18:31:34]

mthiesse@chromium.org changed reviewers:
+ bajones@chromium.org

[mthiesse, 2017-05-18 18:31:35]

lgtm, but please have brandon review the new gamepad device, as he knows this
code much better than me.

https://codereview.chromium.org/2879973002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2879973002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:233:
if (event.getActionMasked() == MotionEvent.ACTION_UP
nit: else if?

https://codereview.chromium.org/2879973002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2879973002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:264: // enable the gamepad
DCHECK that the gvr fetcher isn't active at the same time? Providing two
fetchers with the same device id might be confusing?

https://codereview.chromium.org/2879973002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:290: pad.timestamp =
cardboard_gamepad_timer_++;
Why are we using a counter for the timestamp? Why not provide the current time?

[bajones, 2017-05-19 22:38:40]

LGTM

https://codereview.chromium.org/2879973002/diff/40001/device/vr/android/gvr/c...
File device/vr/android/gvr/cardboard_gamepad_data_fetcher.cc (right):

https://codereview.chromium.org/2879973002/diff/40001/device/vr/android/gvr/c...
device/vr/android/gvr/cardboard_gamepad_data_fetcher.cc:89:
base::UTF8ToUTF16("Cardboard Controller"));
Nit: This sounds weird in this context. Maybe "Cardboard Button" or "Cardboard
Input" instead?

[billorr, 2017-05-22 20:10:44]

The CQ bit was checked by billorr@chromium.org to run a CQ dry run

[billorr, 2017-05-22 20:11:09]

https://codereview.chromium.org/2879973002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2879973002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:233:
if (event.getActionMasked() == MotionEvent.ACTION_UP
On 2017/05/18 18:31:35, mthiesse wrote:
> nit: else if?

Done.

https://codereview.chromium.org/2879973002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2879973002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:264: // enable the gamepad
On 2017/05/18 18:31:35, mthiesse wrote:
> DCHECK that the gvr fetcher isn't active at the same time? Providing two
> fetchers with the same device id might be confusing?

We do want them both active.  For Oculus or Vive you will have multiple gamepads
active for the same display.

https://codereview.chromium.org/2879973002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:290: pad.timestamp =
cardboard_gamepad_timer_++;
On 2017/05/18 18:31:35, mthiesse wrote:
> Why are we using a counter for the timestamp? Why not provide the current
time?

The timestamp is just a monotonically increasing value, and seems to be
implementation specific.

https://codereview.chromium.org/2879973002/diff/40001/device/vr/android/gvr/c...
File device/vr/android/gvr/cardboard_gamepad_data_fetcher.cc (right):

https://codereview.chromium.org/2879973002/diff/40001/device/vr/android/gvr/c...
device/vr/android/gvr/cardboard_gamepad_data_fetcher.cc:89:
base::UTF8ToUTF16("Cardboard Controller"));
On 2017/05/19 22:38:40, bajones wrote:
> Nit: This sounds weird in this context. Maybe "Cardboard Button" or "Cardboard
> Input" instead?

Done.

[commit-bot: I haz the power, 2017-05-22 20:11:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 20:14:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 20:14:20]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[billorr, 2017-05-22 21:05:14]

The CQ bit was checked by billorr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 21:05:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[billorr, 2017-05-22 21:26:55]

The CQ bit was unchecked by billorr@chromium.org

[billorr, 2017-05-22 21:26:56]

The CQ bit was checked by billorr@chromium.org

[billorr, 2017-05-22 21:26:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2879973002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-22 21:27:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 00:14:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 00:14:18]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[billorr, 2017-05-23 16:58:52]

The CQ bit was checked by billorr@chromium.org

[billorr, 2017-05-23 16:58:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2879973002/#ps100001
(title: "fix crash in tests")

[commit-bot: I haz the power, 2017-05-23 16:59:43]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 18:21:23]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495558732140190,
"parent_rev": "217d81bc49377c2235b8325a2f03dfce9ef45f95", "commit_rev":
"6ca5bedb3683ccf3db53fbad5bb93c60233c368b"}

[commit-bot: I haz the power, 2017-05-23 18:21:40]

Description was changed from

==========
Expose Gamepad API instance for Cardboard button

This change exposes a gamepad with a single button, which is driven by the touch
screen.

BUG=718665
==========

to

==========
Expose Gamepad API instance for Cardboard button

This change exposes a gamepad with a single button, which is driven by the touch
screen.

BUG=718665

Review-Url: https://codereview.chromium.org/2879973002
Cr-Commit-Position: refs/heads/master@{#473986}
Committed:
https://chromium.googlesource.com/chromium/src/+/6ca5bedb3683ccf3db53fbad5bb9...
==========

[commit-bot: I haz the power, 2017-05-23 18:21:43]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/6ca5bedb3683ccf3db53fbad5bb9...