Implement the gamepad API in the IPC proxy

Implement the gamepad API in the IPC proxy

This does some reworking of the gamepad system to make it possible to hook in (previously it assumed that it was only talking to renderers) and I did some cleanup. Gamepad files were renamed to match the classes, and I did a bunch of test infrastructure work.

IMPORTANT BEHAVIOR CHANGE: This changes the Web gamepad API to report all gamepad data as soon as any of them are interacted with. This is what we need to do for Pepper anyway (since it gets all or none of the share memory) and I think makes more sense for most consumers anyway.

I separated out the user gesture detection code into a place where it can be used in the browser process as well, and exposed functionality in the gamepad provider to be notified when a user gesture happens.

The existing gamepad test was disabled and had bitrotted. This fixes it.

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

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

[brettw, 2012-09-04 22:23:09]

Don't panic, this is not as scary as it looks.

scottmg: Please review
- content/browser/gamepad
- content/common
I renamed the the platform_data_fetcher files so the file names match the class
names, but made no other changes to these files. This is partly why the change
looks scary, but you don't have to review all the adds and deletes for these
files.

raymes: The other stuff. There are a lot of files but most of the changes are
relatively small hookup thingys and test stuff.

[scottmg, 2012-09-05 16:42:05]

content/browser and content/common lgtm. Thanks for all the cleanup!

https://chromiumcodereview.appspot.com/10912062/diff/8001/content/browser/gam...
File content/browser/gamepad/gamepad_provider.h (right):

https://chromiumcodereview.appspot.com/10912062/diff/8001/content/browser/gam...
content/browser/gamepad/gamepad_provider.h:37: explicit
GamepadProvider(scoped_ptr<GamepadDataFetcher> fetcher);
We pass scoped_ptr by value? ... Ah, apparently they're really
scoped+unique_ptr!

https://chromiumcodereview.appspot.com/10912062/diff/8001/content/browser/gam...
content/browser/gamepad/gamepad_provider.h:97:
scoped_refptr<base::MessageLoopProxy> > >
nit: Maybe a struct instead of pair.

[raymes, 2012-09-05 21:05:57]

Just nits but I want to go over it one more time.

http://codereview.chromium.org/10912062/diff/8001/content/browser/renderer_ho...
File content/browser/renderer_host/gamepad_browser_message_filter.cc (right):

http://codereview.chromium.org/10912062/diff/8001/content/browser/renderer_ho...
content/browser/renderer_host/gamepad_browser_message_filter.cc:48:
*renderer_handle = service->GetSharedMemoryHandleForProcess(peer_handle());
Nit: Move this into the if statement for clarity? Or just have
if (is_started)
  NOT_REACHED();
at the top.

http://codereview.chromium.org/10912062/diff/8001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper/pepper_gamepad_host.cc (right):

http://codereview.chromium.org/10912062/diff/8001/content/browser/renderer_ho...
content/browser/renderer_host/pepper/pepper_gamepad_host.cc:74:
base::SharedMemoryHandle shm =
nit: variable naming. maybe just handle?

http://codereview.chromium.org/10912062/diff/8001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper/pepper_gamepad_host_unittest.cc
(right):

http://codereview.chromium.org/10912062/diff/8001/content/browser/renderer_ho...
content/browser/renderer_host/pepper/pepper_gamepad_host_unittest.cc:154: // It
should have send a callback.
*sent

http://codereview.chromium.org/10912062/diff/8001/content/browser/renderer_ho...
content/browser/renderer_host/pepper/pepper_gamepad_host_unittest.cc:163:
ASSERT_TRUE(PpapiPluginMsg_Gamepad_SendMemory::Read(&reply_msg,
Do you actually want to check values here?

http://codereview.chromium.org/10912062/diff/8001/content/content_browser.gypi
File content/content_browser.gypi (right):

http://codereview.chromium.org/10912062/diff/8001/content/content_browser.gyp...
content/content_browser.gypi:375:
'browser/gamepad/gamepad_platform_data_fetcher.h',
these are out of order now

http://codereview.chromium.org/10912062/diff/8001/ppapi/examples/gamepad/game...
File ppapi/examples/gamepad/gamepad.cc (right):

http://codereview.chromium.org/10912062/diff/8001/ppapi/examples/gamepad/game...
ppapi/examples/gamepad/gamepad.cc:88: pp::ImageData image(this,
PP_IMAGEDATAFORMAT_BGRA_PREMUL, size, true);
What's this for?

[brettw, 2012-09-05 21:25:51]

New snap up. PTAL.

http://codereview.chromium.org/10912062/diff/8001/content/browser/gamepad/gam...
File content/browser/gamepad/gamepad_provider.h (right):

http://codereview.chromium.org/10912062/diff/8001/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_provider.h:37: explicit
GamepadProvider(scoped_ptr<GamepadDataFetcher> fetcher);
Yes, this is teh new hotness.

http://codereview.chromium.org/10912062/diff/8001/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_provider.h:97:
scoped_refptr<base::MessageLoopProxy> > >
On 2012/09/05 16:42:05, scottmg wrote:
> nit: Maybe a struct instead of pair.

Done.

http://codereview.chromium.org/10912062/diff/8001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper/pepper_gamepad_host_unittest.cc
(right):

http://codereview.chromium.org/10912062/diff/8001/content/browser/renderer_ho...
content/browser/renderer_host/pepper/pepper_gamepad_host_unittest.cc:163:
ASSERT_TRUE(PpapiPluginMsg_Gamepad_SendMemory::Read(&reply_msg,
On 2012/09/05 21:05:57, raymes wrote:
> Do you actually want to check values here?

Done.

http://codereview.chromium.org/10912062/diff/8001/ppapi/examples/gamepad/game...
File ppapi/examples/gamepad/gamepad.cc (right):

http://codereview.chromium.org/10912062/diff/8001/ppapi/examples/gamepad/game...
ppapi/examples/gamepad/gamepad.cc:88: pp::ImageData image(this,
PP_IMAGEDATAFORMAT_BGRA_PREMUL, size, true);
This was a bug in the example where it doesn't request that the data be cleared,
but doesn't clear it itself. When we wrote it this was OK since we always
cleared. My new image data cache means it isn't always cleared and the example
has some painting artifacts.

[raymes, 2012-09-05 23:37:13]

https://chromiumcodereview.appspot.com/10912062/diff/10003/content/browser/ga...
File content/browser/gamepad/gamepad_provider.cc (right):

https://chromiumcodereview.appspot.com/10912062/diff/10003/content/browser/ga...
content/browser/gamepad/gamepad_provider.cc:83: void
GamepadProvider::RegisterForUserGesture(const base::Closure& closure) {
This must be called from the IO thread, right? Should we add a dcheck here?

[raymes, 2012-09-06 04:07:53]

non- content/browser/gamepad/* code looks ok.

Looks like the gamepad_provider_unittest is failing though and there are some
other minor style errors caught by the other bots.

The fact that we have to have 2 versions of the structs/code is kinda nasty
though. Do you have any ideas for a longer term solution? What was the argument
for not having a new directory which is shared between the plugin/renderer?

https://chromiumcodereview.appspot.com/10912062/diff/10003/content/browser/ga...
File content/browser/gamepad/gamepad_provider.cc (right):

https://chromiumcodereview.appspot.com/10912062/diff/10003/content/browser/ga...
content/browser/gamepad/gamepad_provider.cc:83: void
GamepadProvider::RegisterForUserGesture(const base::Closure& closure) {
On 2012/09/05 23:37:13, raymes wrote:
> This must be called from the IO thread, right? Should we add a dcheck here?

Nevermind I see this is protected and can be called from any thread.

[brettw, 2012-09-06 18:24:25]

This should fix the test (it was reading uninitialized data, the fix is in
gamepad_user_gesture to only read the number of buttons there are).

New snap up.

[raymes, 2012-09-06 18:29:20]

On 2012/09/06 18:24:25, brettw wrote:
> This should fix the test (it was reading uninitialized data, the fix is in
> gamepad_user_gesture to only read the number of buttons there are).
> 
> New snap up.

lgtm

[brettw, 2012-09-06 22:03:57]

New snap up with PPAPI resource message handle sharing. I added a handle vector
and functions for adding and querying the handles in the message. I used this in
the new resource. I also tweaked the test to make it link on Linux.

[brettw, 2012-09-06 22:06:18]

You should be able to diff patches 6 & 7 to see what changed.

[brettw, 2012-09-06 22:18:07]

I made the new general-purpose handle getter private after a suggestion from
David Michael and fixed the order of the functions in
resource_message_params.cc.

[raymes, 2012-09-06 22:27:41]

lgtm

[commit-bot: I haz the power, 2012-09-09 23:36:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brettw@chromium.org/10912062/16017

[commit-bot: I haz the power, 2012-09-09 23:36:43]

Failed to apply patch for ppapi/proxy/ppapi_messages.h:
While running patch -p1 --forward --force;
patching file ppapi/proxy/ppapi_messages.h
Hunk #1 FAILED at 1555.
1 out of 1 hunk FAILED -- saving rejects to file
ppapi/proxy/ppapi_messages.h.rej

[commit-bot: I haz the power, 2012-09-10 04:52:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brettw@chromium.org/10912062/12012

[commit-bot: I haz the power, 2012-09-10 07:06:41]

Change committed as 155676

[Isaac (away), 2012-09-10 08:56:58]

Hello!  This CL has broken a unit test on the android tester
http://build.chromium.org/p/chromium.linux/builders/Android%20Tester%20%28dbg%29

See here
[http://build.chromium.org/p/chromium.linux/builders/Android%20Tester%20%28dbg%29/builds/37/steps/Logcat%20dump/logs/stdio
] and search for GamepadProviderTest.PollingAccess

[Isaac (away), 2012-09-10 08:56:59]

Hello!  This CL has broken a unit test on the android tester
http://build.chromium.org/p/chromium.linux/builders/Android%20Tester%20%28dbg%29

See here
[http://build.chromium.org/p/chromium.linux/builders/Android%20Tester%20%28dbg%29/builds/37/steps/Logcat%20dump/logs/stdio
] and search for GamepadProviderTest.PollingAccess