Updating Gamepad API to match latest spec

Updating Gamepad API to match latest spec

BUG=344556
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253623

[bajones, 2014-02-18 23:38:03]

Working towards implementing the API as detailed in
https://dvcs.w3.org/hg/gamepad/raw-file/default/gamepad.html

This is the Chrome-side half of the code. Blink-side code is
https://codereview.chromium.org/170993002/ (Still needs a minor update to add
the appropriate #define)

A couple of things to note here: While the Blink code will expose buttons as a
structure ({float value, bool pressed}) at this level we're still just tracking
an array of float values and populating "pressed" in Blink with an arbitrary
threshold. We can pull that concept all the way down to the gamepad_platform_...
level but the only benefit I can see is if some hardware specified a custom
"pressed" threshold (I'm not aware of any that do.) Otherwise it seems like just
more memory to IPC.

Also, this code doesn't include the connected/disconnected events. Not sure if
it's better to attach them to this CL or do those separately to keep the
complexity of each commit down.

Finally, I have very little concept of how the pepper side of this all works nor
do I have a clear idea of how to test it. The current code was the result of
hacking away at the PPAPI code till it compiled again. :P Happy to address any
issues that may crop up in that area.

[scottmg, 2014-02-18 23:45:26]

https://codereview.chromium.org/165983005/diff/100001/content/browser/gamepad...
File content/browser/gamepad/gamepad_platform_data_fetcher_linux.cc (right):

https://codereview.chromium.org/165983005/diff/100001/content/browser/gamepad...
content/browser/gamepad/gamepad_platform_data_fetcher_linux.cc:184: #ifdef
ENABLE_NEW_GAMEPAD_API
I'm not sure I like the #ifs. How are we going to decide when to switch? Do you
know if FF de-prefixed when they updated to this spec or are they still using
mozBlah?

https://codereview.chromium.org/165983005/diff/100001/ppapi/shared_impl/ppb_g...
File ppapi/shared_impl/ppb_gamepad_shared.cc (right):

https://codereview.chromium.org/165983005/diff/100001/ppapi/shared_impl/ppb_g...
ppapi/shared_impl/ppb_gamepad_shared.cc:31: output_pad.buttons[j].pressed =
webkit_pad.buttons[j] >= 0.1f ?
I think the deadzoning/threshold should happen in the data fetcher. For example,
http://msdn.microsoft.com/en-us/library/windows/desktop/ee417001(v=vs.85).asp...
specifies how it ought to be done for XInput.

[bajones, 2014-02-18 23:54:04]

https://codereview.chromium.org/165983005/diff/100001/content/browser/gamepad...
File content/browser/gamepad/gamepad_platform_data_fetcher_linux.cc (right):

https://codereview.chromium.org/165983005/diff/100001/content/browser/gamepad...
content/browser/gamepad/gamepad_platform_data_fetcher_linux.cc:184: #ifdef
ENABLE_NEW_GAMEPAD_API
On 2014/02/18 23:45:26, scottmg wrote:
> I'm not sure I like the #ifs. How are we going to decide when to switch? Do
you
> know if FF de-prefixed when they updated to this spec or are they still using
> mozBlah?

Sorry, I should have clarified this. These #ifdefs are only in place to allow
the blink and chrome commits to land separately and cleanly without compiler
errors. Once this is landed the Blink commit will include the #define to turn
this code on, and a final commit will remove the now unecessary #ifdef

https://codereview.chromium.org/165983005/diff/100001/ppapi/shared_impl/ppb_g...
File ppapi/shared_impl/ppb_gamepad_shared.cc (right):

https://codereview.chromium.org/165983005/diff/100001/ppapi/shared_impl/ppb_g...
ppapi/shared_impl/ppb_gamepad_shared.cc:31: output_pad.buttons[j].pressed =
webkit_pad.buttons[j] >= 0.1f ?
On 2014/02/18 23:45:26, scottmg wrote:
> I think the deadzoning/threshold should happen in the data fetcher. For
example,
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ee417001%28v=vs.85%29...
> specifies how it ought to be done for XInput.

Fair enough. That will require a few more #ifdef guards to make sure everything
meshes nicely but it shouldn't be too hard.

[scottmg, 2014-02-18 23:57:29]

https://codereview.chromium.org/165983005/diff/100001/content/browser/gamepad...
File content/browser/gamepad/gamepad_platform_data_fetcher_linux.cc (right):

https://codereview.chromium.org/165983005/diff/100001/content/browser/gamepad...
content/browser/gamepad/gamepad_platform_data_fetcher_linux.cc:184: #ifdef
ENABLE_NEW_GAMEPAD_API
On 2014/02/18 23:54:04, bajones wrote:
> On 2014/02/18 23:45:26, scottmg wrote:
> > I'm not sure I like the #ifs. How are we going to decide when to switch? Do
> you
> > know if FF de-prefixed when they updated to this spec or are they still
using
> > mozBlah?
> 
> Sorry, I should have clarified this. These #ifdefs are only in place to allow
> the blink and chrome commits to land separately and cleanly without compiler
> errors. Once this is landed the Blink commit will include the #define to turn
> this code on, and a final commit will remove the now unecessary #ifdef

Gotcha, that sounds good.

My concern then is that we're breaking existing code, in both js and ppapi
interfaces. I know it's prefixed still, but I think that's still frowned upon.

(Hopefully in PPAPI it's enough to just rev the api version? But presumably
there's some work to do to service both versions in that case.)

[bajones, 2014-02-19 00:04:49]

On 2014/02/18 23:57:29, scottmg wrote:
> My concern then is that we're breaking existing code, in both js and ppapi
> interfaces. I know it's prefixed still, but I think that's still frowned upon.

I'm somewhat concerned about this as well, but I assume that this scenario has
already been given some consideration when the spec was updated. This does bring
us inline with Mozilla's implementation, so at least it's not completely foreign
to developers.

If we don't want to break anybody I guess we do have the option of leaving the
prefixed version of the code as is forevermore and exposing this new API as a
non-prefixed version behind a flag. (I think I just threw up a bit while typing
that. Please don't agree to that...) I haven't seen too many gamepad-enabled
sites in the wild aside from "tester" apps, though, so if we're going to make a
breaking change now is probably the time.

[scottmg, 2014-02-19 00:20:39]

On 2014/02/19 00:04:49, bajones wrote:
> On 2014/02/18 23:57:29, scottmg wrote:
> > My concern then is that we're breaking existing code, in both js and ppapi
> > interfaces. I know it's prefixed still, but I think that's still frowned
upon.
> 
> I'm somewhat concerned about this as well, but I assume that this scenario has
> already been given some consideration when the spec was updated. This does
bring
> us inline with Mozilla's implementation, so at least it's not completely
foreign
> to developers.
> 
> If we don't want to break anybody I guess we do have the option of leaving the
> prefixed version of the code as is forevermore and exposing this new API as a
> non-prefixed version behind a flag. (I think I just threw up a bit while
typing
> that. Please don't agree to that...) I haven't seen too many gamepad-enabled
> sites in the wild aside from "tester" apps, though, so if we're going to make
a
> breaking change now is probably the time.

Yeah, I dunno, it's hard to call. I was hoping that any breaking changes could
be done at de-prefix time. Mostly, I'm concerned about |buttons| as the rest
looks backwards compatible (?). I don't think it'd be _that_ horrible to pass
the new and improved version most of the way, and then in the blink exposing
code in the prefixed version make |buttons| a set of floats, but in the
non-prefixed make it the pressed/value struct.

I don't think it was particularly given any consideration when updating the
spec, Ted just kind of changed it , I think mostly because they hadn't shipped
anything yet. :)

We wouldn't want to keep both versions forevermore, but if we just change it,
the drawback is that it's kind of a mess for people to deal with as it rolls
through Canary->Stable. At least with some overlap of both versions, there's a
chance they can keep their app working.

There's limited users, but e.g. https://www.google.com/doodles/hurdles-2012 that
will just magically break, which sucks. Maybe the blink-side reviewers will have
feedback on how this should be approached.

[bajones, 2014-02-19 00:32:56]

On 2014/02/19 00:20:39, scottmg wrote:
> Yeah, I dunno, it's hard to call. I was hoping that any breaking changes could
> be done at de-prefix time. Mostly, I'm concerned about |buttons| as the rest
> looks backwards compatible (?).

Generally, yes. The one other pain point is that we go from indicating standard
gamepads in the gamepad id (eg: "Logitech F310 (STANDARD GAMEPAD Vendor:...)")
to an explicit mapping string. If anybody is searching the id string for that
they'll break. We could, of course, keep the "STANDARD GAMEPAD" string as part
of the id and also populate the mapping, which is kinda gross due to data
duplication but not terrible. I'd want to drop it at some point.

> I don't think it'd be _that_ horrible to pass
> the new and improved version most of the way, and then in the blink exposing
> code in the prefixed version make |buttons| a set of floats, but in the
> non-prefixed make it the pressed/value struct.

That's pretty reasonable, actually. My question then becomes what are the
roadblocks to exposing the non-prefixed version? I'd prefer to get people using
it soon-ish so that new gamepad-enabled pages don't suffer from the backwards
compatibility issue we're talking about.

> There's limited users, but e.g. https://www.google.com/doodles/hurdles-2012
that
> will just magically break, which sucks. Maybe the blink-side reviewers will
have
> feedback on how this should be approached.

Yeah, that would be a good point to hash out the details of this. In the
meantime, however, I like the plan of updating the Chrome side and handling
compatibility in Blink. It allows this CL to get through anyway (after some
updates.)

[scottmg, 2014-02-19 01:35:44]

On 2014/02/19 00:32:56, bajones wrote:
> On 2014/02/19 00:20:39, scottmg wrote:
> > Yeah, I dunno, it's hard to call. I was hoping that any breaking changes
could
> > be done at de-prefix time. Mostly, I'm concerned about |buttons| as the rest
> > looks backwards compatible (?).
> 
> Generally, yes. The one other pain point is that we go from indicating
standard
> gamepads in the gamepad id (eg: "Logitech F310 (STANDARD GAMEPAD Vendor:...)")
> to an explicit mapping string. If anybody is searching the id string for that
> they'll break. We could, of course, keep the "STANDARD GAMEPAD" string as part
> of the id and also populate the mapping, which is kinda gross due to data
> duplication but not terrible. I'd want to drop it at some point.

Yeah, I guess we'd want to keep it in .id until the the prefixed version went
away then.

> 
> > I don't think it'd be _that_ horrible to pass
> > the new and improved version most of the way, and then in the blink exposing
> > code in the prefixed version make |buttons| a set of floats, but in the
> > non-prefixed make it the pressed/value struct.
> 
> That's pretty reasonable, actually. My question then becomes what are the
> roadblocks to exposing the non-prefixed version? I'd prefer to get people
using
> it soon-ish so that new gamepad-enabled pages don't suffer from the backwards
> compatibility issue we're talking about.

Agreed. I think there's a template that's supposed to go to blink-dev for
comment on de-prefix. I'm not familiar with the specific requirements that need
to be met though.

> 
> > There's limited users, but e.g. https://www.google.com/doodles/hurdles-2012
> that
> > will just magically break, which sucks. Maybe the blink-side reviewers will
> have
> > feedback on how this should be approached.
> 
> Yeah, that would be a good point to hash out the details of this. In the
> meantime, however, I like the plan of updating the Chrome side and handling
> compatibility in Blink. It allows this CL to get through anyway (after some
> updates.)

sgtm

[bajones, 2014-02-20 00:57:02]

CL updated. This will have to land after
https://codereview.chromium.org/170993002/ now, and this CL should be updated
with the necessary DEPS roll before landing.

Still would like feedback on the PPAPI side of things. I see that there's a
versioning mechanism built in but I'm not sure how to utilize it in this
situation to ensure we don't break existing apps.

[scottmg, 2014-02-20 01:15:12]

content/ parts lgtm with those changes, not sure about ppapi parts.

https://codereview.chromium.org/165983005/diff/300001/content/browser/gamepad...
File content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm (right):

https://codereview.chromium.org/165983005/diff/300001/content/browser/gamepad...
content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm:24: void
copyNSStringAsUTF16LittleEndian(
rename to Copy...

https://codereview.chromium.org/165983005/diff/300001/content/browser/gamepad...
content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm:431:
pad.buttons[6].pressed = data.triggers[0] > 0.1f;
Did the .1 come from anywhere in particular? it'd be 30/255 to match
XINPUT_GAMEPAD_TRIGGER_THRESHOLD, which is close. Maybe pull it out into a
kSomethingSomething anyway.

https://codereview.chromium.org/165983005/diff/300001/content/browser/gamepad...
File content/browser/gamepad/gamepad_standard_mappings.h (right):

https://codereview.chromium.org/165983005/diff/300001/content/browser/gamepad...
content/browser/gamepad/gamepad_standard_mappings.h:56: const float
default_button_pressed_threshold = 0.1f;
kDefaultButtonPressedThreshold

(and possibly share this one with _mac)

[bajones, 2014-02-21 00:20:31]

Refactored to eliminate the PPAPI interface change. I realized we don't need to
do it yet, and it keeps the interface in sync between Javascript and PNaCl.
Still had to touch the ppb_gamepad_shared files to keep the internal interfaces
in sync (although now I question if we even need that copy of the struct. We're
casting into it but then immediately copying the results into another structure.
Why?)

[scottmg, 2014-02-21 04:54:48]

On 2014/02/21 00:20:31, bajones wrote:
> Refactored to eliminate the PPAPI interface change. I realized we don't need
to
> do it yet, and it keeps the interface in sync between Javascript and PNaCl.
> Still had to touch the ppb_gamepad_shared files to keep the internal
interfaces
> in sync (although now I question if we even need that copy of the struct.
We're
> casting into it but then immediately copying the results into another
structure.
> Why?)

Sorry, I'm not sure which one you mean. |WebKitGamepad|? Maybe it's because
ppapi can't include webkit's defintion? Brett might know better.

[bajones, 2014-02-24 23:00:46]

brettw@ or bbudge@? Can I get some feedback on the PPAPI portions of this CL? Or
if there's someone more suited to this review I'd be happy to ping them instead.

[bbudge, 2014-02-24 23:11:05]

ppapi LGTM w/nits

https://codereview.chromium.org/165983005/diff/500001/ppapi/shared_impl/ppb_g...
File ppapi/shared_impl/ppb_gamepad_shared.cc (right):

https://codereview.chromium.org/165983005/diff/500001/ppapi/shared_impl/ppb_g...
ppapi/shared_impl/ppb_gamepad_shared.cc:29: // Can't memcpy because buttons are
different types
nit: this comment doesn't seem necessary to me when looking at the loop.

https://codereview.chromium.org/165983005/diff/500001/ppapi/shared_impl/ppb_g...
File ppapi/shared_impl/ppb_gamepad_shared.h (right):

https://codereview.chromium.org/165983005/diff/500001/ppapi/shared_impl/ppb_g...
ppapi/shared_impl/ppb_gamepad_shared.h:22: float value;
nit: indent should be 2 spaces

[bbudge, 2014-02-24 23:14:51]

Is the 'mapping' data filled in somewhere else?

https://codereview.chromium.org/165983005/diff/500001/ppapi/shared_impl/ppb_g...
File ppapi/shared_impl/ppb_gamepad_shared.cc (right):

https://codereview.chromium.org/165983005/diff/500001/ppapi/shared_impl/ppb_g...
ppapi/shared_impl/ppb_gamepad_shared.cc:32: }
I don't understand how mapping data is converted.

[bajones, 2014-02-24 23:25:55]

Thank you!

https://codereview.chromium.org/165983005/diff/500001/ppapi/shared_impl/ppb_g...
File ppapi/shared_impl/ppb_gamepad_shared.cc (right):

https://codereview.chromium.org/165983005/diff/500001/ppapi/shared_impl/ppb_g...
ppapi/shared_impl/ppb_gamepad_shared.cc:32: }
On 2014/02/24 23:14:51, bbudge wrote:
> I don't understand how mapping data is converted.

I assume you're referring to the string "mapping" member of WebKitGamepad? The
pepper idl doesn't define it (yet), and so we don't copy it here. When we go to
update the user-facing definitions we'll need to add it here.

[bajones, 2014-02-24 23:31:01]

Looks like I still need a LG on /content/shell and /content/common

[bajones, 2014-02-25 21:41:38]

brettw@, jam@, or piman@, can I get a quick review on /content/shell and
/content/common? Both are very minor changes.

[jam, 2014-02-26 00:15:28]

lgtm

[bajones, 2014-02-26 00:20:04]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-02-26 00:35:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/165983005/520001

[Paweł Hajdan Jr., 2014-02-26 05:31:53]

The CQ bit was unchecked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2014-02-26 05:41:23]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-26 06:03:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/165983005/520001

[commit-bot: I haz the power, 2014-02-26 10:17:44]

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

[commit-bot: I haz the power, 2014-02-26 10:17:45]

Try jobs failed on following builders: linux_chromium_rel

[bajones, 2014-02-26 17:51:50]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-02-26 17:53:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/165983005/540001

[commit-bot: I haz the power, 2014-02-26 23:28:37]

Change committed as 253623