Update Gamepad.axes / GamepadButton.value to match the latest specification

Update Gamepad.axes / GamepadButton.value to match the latest specification

Update Gamepad.axes / GamepadButton.value 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

Those attributes should be of type double as per the specification but Blink
was still using float type as in older version of the specification. This can
lead to precision problem on JavaScript side.

This CL introduces temporary ENABLE_NEW_GAMEPAD_API #ifdefs to help with the
transition. Those will be removed once the Chromium side has been updated.

R=eseidel@chromium.org, bajones@chromium.org
BUG=344556
TEST=gamepad/gamepad-events-basic.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174428

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174544

[Inactive, 2014-05-13 01:55:35]

Chromium-side CL: https://codereview.chromium.org/280713004/

[Inactive, 2014-05-19 13:33:49]

ping review?

[Inactive, 2014-05-19 13:43:51]

+eseidel instead of abarth as I heard Adam is away.

[eseidel, 2014-05-20 03:04:05]

bajones is your man.

[eseidel, 2014-05-20 03:05:37]

https://codereview.chromium.org/280393004/diff/1/Source/modules/gamepad/Gamep...
File Source/modules/gamepad/GamepadCommon.h (right):

https://codereview.chromium.org/280393004/diff/1/Source/modules/gamepad/Gamep...
Source/modules/gamepad/GamepadCommon.h:57: #if defined(ENABLE_NEW_GAMEPAD_API)
We don't normally have compile time flags in Blink.  I'm confused by this.

https://codereview.chromium.org/280393004/diff/1/public/platform/WebGamepads.h
File public/platform/WebGamepads.h (right):

https://codereview.chromium.org/280393004/diff/1/public/platform/WebGamepads....
public/platform/WebGamepads.h:56: COMPILE_ASSERT(sizeof(WebGamepads) == 2888,
WebGamepads_has_wrong_size);
I doubt we need to assert the size of such a large object.

[eseidel, 2014-05-20 03:05:53]

I see now that the #defines are temporary.  rslgtm.

[Inactive, 2014-05-20 18:22:49]

bajones, is this change OK with you? Can I land this?

[bajones, 2014-05-20 18:24:55]

On 2014/05/20 18:22:49, Chris Dumez wrote:
> bajones, is this change OK with you? Can I land this?

LGTM

[Inactive, 2014-05-20 18:28:44]

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

[Inactive, 2014-05-20 18:28:52]

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

[Inactive, 2014-05-20 18:29:14]

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

[commit-bot: I haz the power, 2014-05-20 18:29:40]

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

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

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

[Inactive, 2014-05-20 18:37:57]

I will wait for the Chromium-side change to land first. This way I will be able
to remove the #ifdefs from this CL directly, and add a "#define
ENABLE_NEW_GAMEPAD_API 1" instead.

[Inactive, 2014-05-20 22:18:37]

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

[commit-bot: I haz the power, 2014-05-20 22:19:12]

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

[commit-bot: I haz the power, 2014-05-20 23:43:47]

Change committed as 174428

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

This patch was reverted for causing a build failure on the win dbg bot. It seems
the "win" try bot is confirming the issue on this CL:
FAILED: ninja -t msvc -e environment.x86 -- E:\b\build\goma/gomacc
"E:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo
/showIncludes /FC
@obj\content\renderer\pepper\content.pepper_plugin_instance_impl.obj.rsp /c
..\..\content\renderer\pepper\pepper_plugin_instance_impl.cc
/Foobj\content\renderer\pepper\content.pepper_plugin_instance_impl.obj
/Fdobj\content\content.cc.pdb 
e:\b\build\slave\win\build\src\base\macros.h(270) : error C2118: negative
subscript
       
e:\b\build\slave\win\build\src\content\renderer\pepper\pepper_plugin_instance_impl.cc(483)
: see reference to function template instantiation 'Dest
bit_cast<ppapi::WebKitGamepads,blink::WebGamepads>(const Source &)' being
compiled
        with
        [
            Dest=ppapi::WebKitGamepads
,            Source=blink::WebGamepads
        ]
ninja: build stopped: subcommand failed.

However, I don't understand what the problem is. It looks like the "#define
ENABLE_NEW_GAMEPAD_API 1" in public/platform/WebGamepad.h does not get picked up
somehow.

[Inactive, 2014-05-21 13:33:47]

On 2014/05/21 13:18:54, Chris Dumez wrote:
> This patch was reverted for causing a build failure on the win dbg bot. It
seems
> the "win" try bot is confirming the issue on this CL:
> FAILED: ninja -t msvc -e environment.x86 -- E:\b\build\goma/gomacc
> "E:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo
> /showIncludes /FC
> @obj\content\renderer\pepper\content.pepper_plugin_instance_impl.obj.rsp /c
> ..\..\content\renderer\pepper\pepper_plugin_instance_impl.cc
> /Foobj\content\renderer\pepper\content.pepper_plugin_instance_impl.obj
> /Fdobj\content\content.cc.pdb 
> e:\b\build\slave\win\build\src\base\macros.h(270) : error C2118: negative
> subscript
>        
>
e:\b\build\slave\win\build\src\content\renderer\pepper\pepper_plugin_instance_impl.cc(483)
> : see reference to function template instantiation 'Dest
> bit_cast<ppapi::WebKitGamepads,blink::WebGamepads>(const Source &)' being
> compiled
>         with
>         [
>             Dest=ppapi::WebKitGamepads
> ,            Source=blink::WebGamepads
>         ]
> ninja: build stopped: subcommand failed.
> 
> However, I don't understand what the problem is. It looks like the "#define
> ENABLE_NEW_GAMEPAD_API 1" in public/platform/WebGamepad.h does not get picked
up
> somehow.

Or this could be related to the #pragma pack(push, 1) that is used for msvc? At
least the pragma is the same in WebGamepad.h and ppb_gamepad_shared.h ...

[dmichael (off chromium), 2014-05-21 16:49:31]

https://codereview.chromium.org/280393004/diff/60001/public/platform/WebGamep...
File public/platform/WebGamepad.h (right):

https://codereview.chromium.org/280393004/diff/60001/public/platform/WebGamep...
public/platform/WebGamepad.h:34: #define ENABLE_NEW_GAMEPAD_API 1
I don't think this is safe. You have Chromium code that conditionally compiles
based on this:
https://codereview.chromium.org/280713004/diff/100001/ppapi/shared_impl/ppb_g...
When compiling a translation unit, you might or might not get this #define,
depending on if this file is included at all, and when it is included. I.e., If
WebGamepad.h is included *Before* ppb_gamepad_shared.h, then WebKitGamepad in
ppb_gamepad_shared.h will use double in *that* translation unit. In that case,
it won't match the definition of WebKitGamepad that was compiled for the
ppb_gamepad_shared.cc, so you'll get a linker error, I think.

If WebGamepad.h is included *After* ppb_gamepad_shared.h, then WebKitGamepad in
ppb_gamepad_shared.h uses floats instead of doubles, so the sizes won't match.
This is probably why you get an error building pepper_plugin_instance_impl.cc
with this change.

I suspect the easiest way to fix this is to just define ENABLE_NEW_GAMEPAD_API=1
in GYP for all targets that are dependent on blink. That should flip the switch
all over the relevant parts of the Chromium build to make sure it starts
building the "double" version in all translation units.

If that doesn't work, you might need a more complicated DEPS dance where you
make WebGamepad depend on the same #define to conditionally compile the same
way. Then flip on ENABLE_NEW_GAMEPAD_API in chromium temporarily. *Then* you'd
be able to remove the conditional compilation stuff in blink and chromium.

[Inactive, 2014-05-21 17:08:11]

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

[Inactive, 2014-05-21 17:09:28]

On 2014/05/21 16:49:31, dmichael wrote:
>
https://codereview.chromium.org/280393004/diff/60001/public/platform/WebGamep...
> File public/platform/WebGamepad.h (right):
> 
>
https://codereview.chromium.org/280393004/diff/60001/public/platform/WebGamep...
> public/platform/WebGamepad.h:34: #define ENABLE_NEW_GAMEPAD_API 1
> I don't think this is safe. You have Chromium code that conditionally compiles
> based on this:
>
https://codereview.chromium.org/280713004/diff/100001/ppapi/shared_impl/ppb_g...
> When compiling a translation unit, you might or might not get this #define,
> depending on if this file is included at all, and when it is included. I.e.,
If
> WebGamepad.h is included *Before* ppb_gamepad_shared.h, then WebKitGamepad in
> ppb_gamepad_shared.h will use double in *that* translation unit. In that case,
> it won't match the definition of WebKitGamepad that was compiled for the
> ppb_gamepad_shared.cc, so you'll get a linker error, I think.
> 
> If WebGamepad.h is included *After* ppb_gamepad_shared.h, then WebKitGamepad
in
> ppb_gamepad_shared.h uses floats instead of doubles, so the sizes won't match.
> This is probably why you get an error building pepper_plugin_instance_impl.cc
> with this change.
> 
> I suspect the easiest way to fix this is to just define
ENABLE_NEW_GAMEPAD_API=1
> in GYP for all targets that are dependent on blink. That should flip the
switch
> all over the relevant parts of the Chromium build to make sure it starts
> building the "double" version in all translation units.
> 
> If that doesn't work, you might need a more complicated DEPS dance where you
> make WebGamepad depend on the same #define to conditionally compile the same
> way. Then flip on ENABLE_NEW_GAMEPAD_API in chromium temporarily. *Then* you'd
> be able to remove the conditional compilation stuff in blink and chromium.

Thanks dmichael@. I will likely go the GYP way. This is what was done before for
the previous NEW_GAMEPAD_API changes.
I am thus adding the #ifdefs back to this CL for landing.

[commit-bot: I haz the power, 2014-05-21 17:49:37]

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

[commit-bot: I haz the power, 2014-05-22 06:42:13]

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_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/9163)

[commit-bot: I haz the power, 2014-05-22 11:20:33]

Change committed as 174544