Pepper gamepad support

Pepper gamepad support

BUG=79098
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117077

[scottmg, 2012-01-05 20:33:54]

First attempt at plumbing a gamepad interface. Brett, do you have time to take a
look before you're gone?

[brettw, 2012-01-05 22:50:03]

Looks good overall.

http://codereview.chromium.org/9085027/diff/4020/ppapi/api/dev/ppb_gamepad_de...
File ppapi/api/dev/ppb_gamepad_dev.idl (right):

http://codereview.chromium.org/9085027/diff/4020/ppapi/api/dev/ppb_gamepad_de...
ppapi/api/dev/ppb_gamepad_dev.idl:29: uint16_t[128] id;
This is the same thing as uint16_t id[128], right? That's the normal way I've
seen this written in our code, so I'd do that here as well.

http://codereview.chromium.org/9085027/diff/4020/ppapi/api/dev/ppb_gamepad_de...
ppapi/api/dev/ppb_gamepad_dev.idl:40: uint32_t axesLength;
Google style would be axes_length (same for buttons below).

http://codereview.chromium.org/9085027/diff/4020/ppapi/api/dev/ppb_gamepad_de...
ppapi/api/dev/ppb_gamepad_dev.idl:57: float_t[32] buttons;
In other places, we've just done "float" rather than "float_t", so I'd do that
in this file as well.

http://codereview.chromium.org/9085027/diff/4020/ppapi/api/dev/ppb_gamepad_de...
ppapi/api/dev/ppb_gamepad_dev.idl:77: * The <code>PPB_Gamepad_Dev</code>
interface contains pointers to several
This is wrong because you only provide a pointer to one function :). And the
pointer thing is useless anyway. I'd write something about what the gamepad
interface generally allows you to do.

http://codereview.chromium.org/9085027/diff/4020/ppapi/cpp/dev/gamepad_dev.h
File ppapi/cpp/dev/gamepad_dev.h (right):

http://codereview.chromium.org/9085027/diff/4020/ppapi/cpp/dev/gamepad_dev.h#...
ppapi/cpp/dev/gamepad_dev.h:16: Gamepad(Instance* instance);
This class doesn't actually do anything. Normally these types of classes are
resources. Since you have no resource and there's only one function, I'm
thinking that maybe we don't even need the C++ wrapper.

http://codereview.chromium.org/9085027/diff/4020/webkit/plugins/ppapi/ppapi_p...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/9085027/diff/4020/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1307:
delegate()->SampleGamepads(reinterpret_cast<WebKit::WebGamepads*>(data));
We should have some compiler asserts about the size of the structs and the
offsets of the elements inside so we know if this is no longer a valid
conversion. You can see at the top of this file there are already a bunch of
compile asserts.

[scottmg, 2012-01-06 00:54:21]

Thanks!

http://codereview.chromium.org/9085027/diff/4020/ppapi/api/dev/ppb_gamepad_de...
File ppapi/api/dev/ppb_gamepad_dev.idl (right):

http://codereview.chromium.org/9085027/diff/4020/ppapi/api/dev/ppb_gamepad_de...
ppapi/api/dev/ppb_gamepad_dev.idl:29: uint16_t[128] id;
On 2012/01/05 22:50:04, brettw wrote:
> This is the same thing as uint16_t id[128], right? That's the normal way I've
> seen this written in our code, so I'd do that here as well.

Yes, it's just an array of ushorts. C-style doesn't seem to work though?

../api/dev/ppb_gamepad_dev.idl(29) : Error : Unexpected "[" after symbol id.

http://codereview.chromium.org/9085027/diff/4020/ppapi/api/dev/ppb_gamepad_de...
ppapi/api/dev/ppb_gamepad_dev.idl:40: uint32_t axesLength;
On 2012/01/05 22:50:04, brettw wrote:
> Google style would be axes_length (same for buttons below).

Done.

http://codereview.chromium.org/9085027/diff/4020/ppapi/api/dev/ppb_gamepad_de...
ppapi/api/dev/ppb_gamepad_dev.idl:57: float_t[32] buttons;
On 2012/01/05 22:50:04, brettw wrote:
> In other places, we've just done "float" rather than "float_t", so I'd do that
> in this file as well.

Yeah, that's what I did at first, but doesn't seem to work. Doing a grep for
"float" in ppapi/api, it looks like they're all float_t, perhaps something
changed recently?

http://codereview.chromium.org/9085027/diff/4020/ppapi/api/dev/ppb_gamepad_de...
ppapi/api/dev/ppb_gamepad_dev.idl:77: * The <code>PPB_Gamepad_Dev</code>
interface contains pointers to several
On 2012/01/05 22:50:04, brettw wrote:
> This is wrong because you only provide a pointer to one function :). And the
> pointer thing is useless anyway. I'd write something about what the gamepad
> interface generally allows you to do.

Done. (I plead guilty to some copy-paste-o-rama)

http://codereview.chromium.org/9085027/diff/4020/ppapi/cpp/dev/gamepad_dev.h
File ppapi/cpp/dev/gamepad_dev.h (right):

http://codereview.chromium.org/9085027/diff/4020/ppapi/cpp/dev/gamepad_dev.h#...
ppapi/cpp/dev/gamepad_dev.h:16: Gamepad(Instance* instance);
On 2012/01/05 22:50:04, brettw wrote:
> This class doesn't actually do anything. Normally these types of classes are
> resources. Since you have no resource and there's only one function, I'm
> thinking that maybe we don't even need the C++ wrapper.

Nuked.

http://codereview.chromium.org/9085027/diff/4020/webkit/plugins/ppapi/ppapi_p...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/9085027/diff/4020/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1307:
delegate()->SampleGamepads(reinterpret_cast<WebKit::WebGamepads*>(data));
On 2012/01/05 22:50:04, brettw wrote:
> We should have some compiler asserts about the size of the structs and the
> offsets of the elements inside so we know if this is no longer a valid
> conversion. You can see at the top of this file there are already a bunch of
> compile asserts.

Done. Had to be DCHECK per comments & irc.

[brettw, 2012-01-06 23:36:43]

LGTM, just some example suggestions.

http://codereview.chromium.org/9085027/diff/8026/ppapi/examples/gamepad/gamep...
File ppapi/examples/gamepad/gamepad.cc (right):

http://codereview.chromium.org/9085027/diff/8026/ppapi/examples/gamepad/gamep...
ppapi/examples/gamepad/gamepad.cc:61:
RequestInputEvents(PP_INPUTEVENT_CLASS_MOUSE |
It looks like you don't ever actually use the input events, so I'd remove this
line.

http://codereview.chromium.org/9085027/diff/8026/ppapi/examples/gamepad/gamep...
ppapi/examples/gamepad/gamepad.cc:66: virtual void DidChangeView(const pp::Rect&
position, const pp::Rect& clip) {
Would you mind using the new version of DidChangeView (the one that takes a
const View& as an arg)?

http://codereview.chromium.org/9085027/diff/8026/ppapi/examples/gamepad/gamep...
ppapi/examples/gamepad/gamepad.cc:90:
device_context_.Flush(pp::CompletionCallback(&FlushCallback, this));
Here you should say
callback_factory_.NewRequiredCallback(&MyInstance::OnFlush) instead (you'll have
to add an int32_t result param to OnFlush. Then you can delete your
FlushCallback function.

[commit-bot: I haz the power, 2012-01-10 18:04:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/9085027/22006

[commit-bot: I haz the power, 2012-01-10 19:24:30]

Change committed as 117077