Update Gamepad API to match the latest specification

Update Gamepad API to match the latest specification

Update Gamepad API to match the latest specification:
https://dvcs.w3.org/hg/gamepad/raw-file/default/gamepad.html#widl-Gamepad-axes
https://dvcs.w3.org/hg/gamepad/raw-file/default/gamepad.html#widl-GamepadButton-value

Namely, Gamepad.axes / GamepadButton.value should be of type double, not float.

R=bbudge@chromium.org, jam@chromium.org, scottmg@chromium.org
BUG=344556
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271775

[scottmg, 2014-05-13 18:16:49]

+bajones, but lgtm

[scottmg, 2014-05-13 18:18:17]

https://codereview.chromium.org/280713004/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/280713004/diff/1/build/common.gypi#newcode2424
build/common.gypi:2424: 'defines': ['ENABLE_NEW_GAMEPAD_API=1'],
Er, were these intended to be guarded on this?

[Inactive, 2014-05-13 18:21:45]

https://codereview.chromium.org/280713004/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/280713004/diff/1/build/common.gypi#newcode2424
build/common.gypi:2424: 'defines': ['ENABLE_NEW_GAMEPAD_API=1'],
On 2014/05/13 18:18:18, scottmg wrote:
> Er, were these intended to be guarded on this?

This is only to assist with the transition. The CL for the blink side is at
https://codereview.chromium.org/280393004/ and needs to land first.

[bbudge, 2014-05-13 18:35:43]

Since you're changing a stable Pepper interface, and this changes the size and
layout of exposed structures, you'll have to version the API.

For an example, see ppapi/api/ppb_file_io.idl.

[jam, 2014-05-13 21:05:47]

Not sure why I got added as a reviewer, since I'm not an owner in ppapi and
everyone is an owner in build/common.gypi. but since you added me :)

I don't see why you need to change common.gypi for 3-sided webkit changes. we do
that all the time without having to edit it.

[Inactive, 2014-05-13 21:21:52]

On 2014/05/13 21:05:47, jam wrote:
> Not sure why I got added as a reviewer, since I'm not an owner in ppapi and
> everyone is an owner in build/common.gypi. but since you added me :)
> 
> I don't see why you need to change common.gypi for 3-sided webkit changes. we
do
> that all the time without having to edit it.

I used the exact same reviewers as the original NEW_GAMEPAD_API CL:
https://codereview.chromium.org/165983005

I could have double check which ones were strictly needed, sorry about that.

Regarding the use of a build time flag for the 3-sided blink change, I am doing
what was done in the original NEW_GAMEPAD_API CL mentioned above. I agree that
we usually don't need such flag if there is a way to have the old and the new
API co-exist during the transition. However, for this specific case, since the
ppapi code is relying of the size of a struct that is part of the Blink public
API, it does not seem easy to achieve without a build time flag.

The plan is:
1. Land the Blink-side change (https://codereview.chromium.org/280393004/) with
the ENABLE_NEW_GAMEPAD_API #ifdefs so that the old code is still the one being
built and all the compile time assertions are passing.
2. Land this change on Chromium side that aligns the ppapi code and defines the
new ENABLE_NEW_GAMEPAD_API flag so that the new Blink code starts being used
3. Drop the ENABLE_NEW_GAMEPAD_API flag on Blink side
4. Drop the ENABLE_NEW_GAMEPAD_API flag on Chromium side

If someone has a better proposal, I am happy to update my CLs. As I said, I ran
into those build time assertions and wasn't sure how to fix it so I looked up
the original CL that updated the Gamepad API and used the same approach for my
CL.

[jam, 2014-05-13 23:32:44]

On 2014/05/13 21:21:52, Chris Dumez wrote:
> On 2014/05/13 21:05:47, jam wrote:
> > Not sure why I got added as a reviewer, since I'm not an owner in ppapi and
> > everyone is an owner in build/common.gypi. but since you added me :)
> > 
> > I don't see why you need to change common.gypi for 3-sided webkit changes.
we
> do
> > that all the time without having to edit it.
> 
> I used the exact same reviewers as the original NEW_GAMEPAD_API CL:
> https://codereview.chromium.org/165983005
> 
> I could have double check which ones were strictly needed, sorry about that.
> 
> Regarding the use of a build time flag for the 3-sided blink change, I am
doing
> what was done in the original NEW_GAMEPAD_API CL mentioned above. I agree that
> we usually don't need such flag if there is a way to have the old and the new
> API co-exist during the transition. However, for this specific case, since the
> ppapi code is relying of the size of a struct that is part of the Blink public
> API, it does not seem easy to achieve without a build time flag.
> 
> The plan is:
> 1. Land the Blink-side change (https://codereview.chromium.org/280393004/)
with
> the ENABLE_NEW_GAMEPAD_API #ifdefs so that the old code is still the one being
> built and all the compile time assertions are passing.
> 2. Land this change on Chromium side that aligns the ppapi code and defines
the
> new ENABLE_NEW_GAMEPAD_API flag so that the new Blink code starts being used
> 3. Drop the ENABLE_NEW_GAMEPAD_API flag on Blink side
> 4. Drop the ENABLE_NEW_GAMEPAD_API flag on Chromium side
> 
> If someone has a better proposal, I am happy to update my CLs. As I said, I
ran
> into those build time assertions and wasn't sure how to fix it so I looked up
> the original CL that updated the Gamepad API and used the same approach for my
> CL.

you can accomplish the same thing by having a "#define NEW_FOO" in a webkit
header that the chrome side looks for. basically it's similar to what you're
doing, without havingg to edit build/common.gypi

[Inactive, 2014-05-14 13:50:47]

On 2014/05/13 23:32:44, jam wrote:
> On 2014/05/13 21:21:52, Chris Dumez wrote:
> > On 2014/05/13 21:05:47, jam wrote:
> > > Not sure why I got added as a reviewer, since I'm not an owner in ppapi
and
> > > everyone is an owner in build/common.gypi. but since you added me :)
> > > 
> > > I don't see why you need to change common.gypi for 3-sided webkit changes.
> we
> > do
> > > that all the time without having to edit it.
> > 
> > I used the exact same reviewers as the original NEW_GAMEPAD_API CL:
> > https://codereview.chromium.org/165983005
> > 
> > I could have double check which ones were strictly needed, sorry about that.
> > 
> > Regarding the use of a build time flag for the 3-sided blink change, I am
> doing
> > what was done in the original NEW_GAMEPAD_API CL mentioned above. I agree
that
> > we usually don't need such flag if there is a way to have the old and the
new
> > API co-exist during the transition. However, for this specific case, since
the
> > ppapi code is relying of the size of a struct that is part of the Blink
public
> > API, it does not seem easy to achieve without a build time flag.
> > 
> > The plan is:
> > 1. Land the Blink-side change (https://codereview.chromium.org/280393004/)
> with
> > the ENABLE_NEW_GAMEPAD_API #ifdefs so that the old code is still the one
being
> > built and all the compile time assertions are passing.
> > 2. Land this change on Chromium side that aligns the ppapi code and defines
> the
> > new ENABLE_NEW_GAMEPAD_API flag so that the new Blink code starts being used
> > 3. Drop the ENABLE_NEW_GAMEPAD_API flag on Blink side
> > 4. Drop the ENABLE_NEW_GAMEPAD_API flag on Chromium side
> > 
> > If someone has a better proposal, I am happy to update my CLs. As I said, I
> ran
> > into those build time assertions and wasn't sure how to fix it so I looked
up
> > the original CL that updated the Gamepad API and used the same approach for
my
> > CL.
> 
> you can accomplish the same thing by having a "#define NEW_FOO" in a webkit
> header that the chrome side looks for. basically it's similar to what you're
> doing, without havingg to edit build/common.gypi

How does this work for the Chromium side IDLs (e.g. ppapi/api/ppb_gamepad.idl) ?
If I understand correctly, your proposal is to add a DEFINE in
public/platform/WebGamepad.h and have #ifdefs for it on Chromium side, right? I
understand how this would work for cpp code on Chromium side. However, I am
unclear about IDL files since those don't include headers and I don't know if we
can use #ifdefs in there.

[Inactive, 2014-05-14 14:33:10]

On 2014/05/13 18:35:43, bbudge wrote:
> Since you're changing a stable Pepper interface, and this changes the size and
> layout of exposed structures, you'll have to version the API.
> 
> For an example, see ppapi/api/ppb_file_io.idl.

Done but I am not familiar with this.

[dmichael (off chromium), 2014-05-14 15:14:03]

I'm fine with the idea of updating the API to match the spec, but I expect
there's a lot more that needs to be done to make sure that:
1) The backing implementation works with this new version.
2) The old version is still supported as well, for existing NaCl apps that use
Gamepad.

https://codereview.chromium.org/280713004/diff/20001/ppapi/api/ppb_gamepad.idl
File ppapi/api/ppb_gamepad.idl (right):

https://codereview.chromium.org/280713004/diff/20001/ppapi/api/ppb_gamepad.id...
ppapi/api/ppb_gamepad.idl:105: [version=1.0, macro="PPB_GAMEPAD_INTERFACE",
singleton]
This needs a new version (1.1). I'm not certain how the IDL generator will
handle this if you just omit "version=1.0" or change it to "version=1.1"... I
don't know that we've done a change like this where the function signature has
not changed, but a struct parameter *has* changed.

What I think we *want* is to have two versions of PP_GamepadsSampleData:
PP_GamepadsSampleData_1_0 and PP_GamepadsSampleData_1_1, and maybe a
PP_GamepadsSampleData typedef to the 1.1 version.

In our backing implementation, we'll need to know which one the plugin is using,
so the browser can write the correct one in to shared memory
(content/browser/renderer_host/pepper_gamepad_host.*). The proxy also might need
to change to accommodate the new size. Maybe it would be good enough to always
deal with the larger size, and the smaller one will "just work". That code's in
ppapi/proxy/gamepad_resource.*

[dmichael (off chromium), 2014-05-14 15:28:45]

On 2014/05/14 15:14:03, dmichael wrote:
> I'm fine with the idea of updating the API to match the spec, but I expect
> there's a lot more that needs to be done to make sure that:
> 1) The backing implementation works with this new version.
> 2) The old version is still supported as well, for existing NaCl apps that use
> Gamepad.
> 
> https://codereview.chromium.org/280713004/diff/20001/ppapi/api/ppb_gamepad.idl
> File ppapi/api/ppb_gamepad.idl (right):
> 
>
https://codereview.chromium.org/280713004/diff/20001/ppapi/api/ppb_gamepad.id...
> ppapi/api/ppb_gamepad.idl:105: [version=1.0, macro="PPB_GAMEPAD_INTERFACE",
> singleton]
> This needs a new version (1.1). I'm not certain how the IDL generator will
> handle this if you just omit "version=1.0" or change it to "version=1.1"... I
> don't know that we've done a change like this where the function signature has
> not changed, but a struct parameter *has* changed.
> 
> What I think we *want* is to have two versions of PP_GamepadsSampleData:
> PP_GamepadsSampleData_1_0 and PP_GamepadsSampleData_1_1, and maybe a
> PP_GamepadsSampleData typedef to the 1.1 version.
> 
> In our backing implementation, we'll need to know which one the plugin is
using,
> so the browser can write the correct one in to shared memory
> (content/browser/renderer_host/pepper_gamepad_host.*). The proxy also might
need
> to change to accommodate the new size. Maybe it would be good enough to always
> deal with the larger size, and the smaller one will "just work". That code's
in
> ppapi/proxy/gamepad_resource.*
Looking at it harder, you probably just want to leave this API alone for now.
Instead, you'll want to tinker with the method that converts the WebKitGamepads
in to the PP_GamepadSampleData:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/...
All the plumbing looks like it will "just work" (I think) with the change in
size, except that part. And there, you really need to make it support the old
format. We can add the new version of the API later on for plugins that want
double precision, but we can't remove support for the old format, so you should
just target that format for now.

[Inactive, 2014-05-14 17:26:23]

On 2014/05/14 15:28:45, dmichael wrote:
> On 2014/05/14 15:14:03, dmichael wrote:
> > I'm fine with the idea of updating the API to match the spec, but I expect
> > there's a lot more that needs to be done to make sure that:
> > 1) The backing implementation works with this new version.
> > 2) The old version is still supported as well, for existing NaCl apps that
use
> > Gamepad.
> > 
> >
https://codereview.chromium.org/280713004/diff/20001/ppapi/api/ppb_gamepad.idl
> > File ppapi/api/ppb_gamepad.idl (right):
> > 
> >
>
https://codereview.chromium.org/280713004/diff/20001/ppapi/api/ppb_gamepad.id...
> > ppapi/api/ppb_gamepad.idl:105: [version=1.0, macro="PPB_GAMEPAD_INTERFACE",
> > singleton]
> > This needs a new version (1.1). I'm not certain how the IDL generator will
> > handle this if you just omit "version=1.0" or change it to "version=1.1"...
I
> > don't know that we've done a change like this where the function signature
has
> > not changed, but a struct parameter *has* changed.
> > 
> > What I think we *want* is to have two versions of PP_GamepadsSampleData:
> > PP_GamepadsSampleData_1_0 and PP_GamepadsSampleData_1_1, and maybe a
> > PP_GamepadsSampleData typedef to the 1.1 version.
> > 
> > In our backing implementation, we'll need to know which one the plugin is
> using,
> > so the browser can write the correct one in to shared memory
> > (content/browser/renderer_host/pepper_gamepad_host.*). The proxy also might
> need
> > to change to accommodate the new size. Maybe it would be good enough to
always
> > deal with the larger size, and the smaller one will "just work". That code's
> in
> > ppapi/proxy/gamepad_resource.*
> Looking at it harder, you probably just want to leave this API alone for now.
> Instead, you'll want to tinker with the method that converts the
WebKitGamepads
> in to the PP_GamepadSampleData:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/...
> All the plumbing looks like it will "just work" (I think) with the change in
> size, except that part. And there, you really need to make it support the old
> format. We can add the new version of the API later on for plugins that want
> double precision, but we can't remove support for the old format, so you
should
> just target that format for now.

Sounds like a lot less trouble indeed, thanks. I will prepare a new patch.

[jam, 2014-05-14 17:31:55]

On 2014/05/14 13:50:47, Chris Dumez wrote:
> On 2014/05/13 23:32:44, jam wrote:
> > On 2014/05/13 21:21:52, Chris Dumez wrote:
> > > On 2014/05/13 21:05:47, jam wrote:
> > > > Not sure why I got added as a reviewer, since I'm not an owner in ppapi
> and
> > > > everyone is an owner in build/common.gypi. but since you added me :)
> > > > 
> > > > I don't see why you need to change common.gypi for 3-sided webkit
changes.
> > we
> > > do
> > > > that all the time without having to edit it.
> > > 
> > > I used the exact same reviewers as the original NEW_GAMEPAD_API CL:
> > > https://codereview.chromium.org/165983005
> > > 
> > > I could have double check which ones were strictly needed, sorry about
that.
> > > 
> > > Regarding the use of a build time flag for the 3-sided blink change, I am
> > doing
> > > what was done in the original NEW_GAMEPAD_API CL mentioned above. I agree
> that
> > > we usually don't need such flag if there is a way to have the old and the
> new
> > > API co-exist during the transition. However, for this specific case, since
> the
> > > ppapi code is relying of the size of a struct that is part of the Blink
> public
> > > API, it does not seem easy to achieve without a build time flag.
> > > 
> > > The plan is:
> > > 1. Land the Blink-side change (https://codereview.chromium.org/280393004/)
> > with
> > > the ENABLE_NEW_GAMEPAD_API #ifdefs so that the old code is still the one
> being
> > > built and all the compile time assertions are passing.
> > > 2. Land this change on Chromium side that aligns the ppapi code and
defines
> > the
> > > new ENABLE_NEW_GAMEPAD_API flag so that the new Blink code starts being
used
> > > 3. Drop the ENABLE_NEW_GAMEPAD_API flag on Blink side
> > > 4. Drop the ENABLE_NEW_GAMEPAD_API flag on Chromium side
> > > 
> > > If someone has a better proposal, I am happy to update my CLs. As I said,
I
> > ran
> > > into those build time assertions and wasn't sure how to fix it so I looked
> up
> > > the original CL that updated the Gamepad API and used the same approach
for
> my
> > > CL.
> > 
> > you can accomplish the same thing by having a "#define NEW_FOO" in a webkit
> > header that the chrome side looks for. basically it's similar to what you're
> > doing, without havingg to edit build/common.gypi
> 
> How does this work for the Chromium side IDLs (e.g. ppapi/api/ppb_gamepad.idl)
?
> If I understand correctly, your proposal is to add a DEFINE in
> public/platform/WebGamepad.h and have #ifdefs for it on Chromium side, right?
I
> understand how this would work for cpp code on Chromium side. However, I am
> unclear about IDL files since those don't include headers and I don't know if
we
> can use #ifdefs in there.

I may be missing something, but where is the idl using this flag now?

[Inactive, 2014-05-14 17:34:48]

The latest version of the patch addresses the comments:
- I am no longer changing the plugin API as suggested by dmichael@
- I am no longer editing common.gypi as suggested by jam@

[dmichael (off chromium), 2014-05-14 17:39:01]

Are you able to do a manual test of this? E.g., you can plug in a gamepad and
try something like AirMech from the Chrome Web Store or the NaCl SDK gamepad
example just to verify they still work with this change?

I don't think we have an end-to-end integration test for Pepper gamepad :-/

https://codereview.chromium.org/280713004/diff/40001/ppapi/shared_impl/ppb_ga...
File ppapi/shared_impl/ppb_gamepad_shared.cc (right):

https://codereview.chromium.org/280713004/diff/40001/ppapi/shared_impl/ppb_ga...
ppapi/shared_impl/ppb_gamepad_shared.cc:25: output_pad.axes[j] =
webkit_pad.axes[j];
I would expect you to get a warning for loss of precision if the left is a float
and the right is a double. You probably should static_cast<float>?

[Inactive, 2014-05-14 17:49:50]

https://codereview.chromium.org/280713004/diff/40001/ppapi/shared_impl/ppb_ga...
File ppapi/shared_impl/ppb_gamepad_shared.cc (right):

https://codereview.chromium.org/280713004/diff/40001/ppapi/shared_impl/ppb_ga...
ppapi/shared_impl/ppb_gamepad_shared.cc:25: output_pad.axes[j] =
webkit_pad.axes[j];
On 2014/05/14 17:39:02, dmichael wrote:
> I would expect you to get a warning for loss of precision if the left is a
float
> and the right is a double. You probably should static_cast<float>?

My compiler did not complain but this is a good point. I have added the cast.

[Inactive, 2014-05-14 17:50:57]

On 2014/05/14 17:39:01, dmichael wrote:
> Are you able to do a manual test of this? E.g., you can plug in a gamepad and
> try something like AirMech from the Chrome Web Store or the NaCl SDK gamepad
> example just to verify they still work with this change?

This shouldn't be a problem, I have the hardware. I will give it a try.

[Inactive, 2014-05-14 17:54:37]

scottmg@, could you please review the corresponding Blink side patch
(https://codereview.chromium.org/280393004/)?

All this is to update Chromium to match the proposal on Blink side but no one
commented about the Blink-side change at all :)

[dmichael (off chromium), 2014-05-14 18:00:10]

lgtm

[Inactive, 2014-05-14 18:30:31]

I tested AirMech and my patch seems to break Gamepad support with the Blink-side
patch applied and the ENABLE_NEW_GAMEPAD_API flag on. Gamepad support works with
the ENABLE_NEW_GAMEPAD_API off though. I am investigating to find the reason.

[Inactive, 2014-05-14 19:04:35]

On 2014/05/14 18:30:31, Chris Dumez wrote:
> I tested AirMech and my patch seems to break Gamepad support with the
Blink-side
> patch applied and the ENABLE_NEW_GAMEPAD_API flag on. Gamepad support works
with
> the ENABLE_NEW_GAMEPAD_API off though. I am investigating to find the reason.

Never mind my last comment, I cannot seem to reproduce the problem. The Gamepad
works fine in AirMech.

[bbudge, 2014-05-14 20:21:33]

LGTM w/comment

https://codereview.chromium.org/280713004/diff/80001/ppapi/shared_impl/ppb_ga...
File ppapi/shared_impl/ppb_gamepad_shared.h (right):

https://codereview.chromium.org/280713004/diff/80001/ppapi/shared_impl/ppb_ga...
ppapi/shared_impl/ppb_gamepad_shared.h:23: #if defined(ENABLE_NEW_GAMEPAD_API)
You could reduce the # of #if's if you just had a single one that typedef'd the
numeric type:

#if defined(ENABLE_NEW_GAMEPAD_API)
 typedef double axisValueType;
#else
 typedef float axisValueType;
#endif

Then use it anywhere you need to access these values.

[Inactive, 2014-05-20 18:34:46]

https://codereview.chromium.org/280713004/diff/80001/ppapi/shared_impl/ppb_ga...
File ppapi/shared_impl/ppb_gamepad_shared.h (right):

https://codereview.chromium.org/280713004/diff/80001/ppapi/shared_impl/ppb_ga...
ppapi/shared_impl/ppb_gamepad_shared.h:23: #if defined(ENABLE_NEW_GAMEPAD_API)
On 2014/05/14 20:21:34, bbudge wrote:
> You could reduce the # of #if's if you just had a single one that typedef'd
the
> numeric type:
> 
> #if defined(ENABLE_NEW_GAMEPAD_API)
>  typedef double axisValueType;
> #else
>  typedef float axisValueType;
> #endif
> 
> Then use it anywhere you need to access these values.

Done.

[Inactive, 2014-05-20 18:35:07]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-05-20 18:35:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/280713004/100001

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

Change committed as 271775

[Inactive, 2014-05-21 15:53:22]

I am having trouble landing the blink-side change
(https://codereview.chromium.org/280393004/) because the win  bot is hitting a
compile assertion and supposedly blink::Gamepads and ppapi::WebKitGamepads have
different sizes:
http://build.chromium.org/p/tryserver.chromium/builders/win/builds/160697/ste...

I currently have no idea why this is happening with msvc (only). If anyone has
any insight on this, please let me know.

[chromium-reviews, 2014-05-21 16:49:48]

I added a comment here explaining what I think is happening:
https://codereview.chromium.org/280393004/diff/60001/public/platform/WebGamep...


On Wed, May 21, 2014 at 9:53 AM, <ch.dumez@samsung.com> wrote:

> I am having trouble landing the blink-side change
> (https://codereview.chromium.org/280393004/) because the win  bot is
> hitting a
> compile assertion and supposedly blink::Gamepads and ppapi::WebKitGamepads
> have
> different sizes:
> http://build.chromium.org/p/tryserver.chromium/builders/
> win/builds/160697/steps/compile/logs/stdio
>
> I currently have no idea why this is happening with msvc (only). If anyone
> has
> any insight on this, please let me know.
>
>
> https://codereview.chromium.org/280713004/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.