Gamepad API: add support for connection events

Gamepad API: add support for connection events

Co-authored with Brandon Jones.

This CL continues the work of updating the Gamepad API to latest spec.
IPC's added for connected and disconnected events. The connection state
is observed by GamepadProvider together with polling. The other option
would be to add logic to each platform fetcher. Doing it in GamepadProvider
avoids duplicated logic and I think it is a bit simpler and more consistent
with the polling base nature of the gamepad implementation.

Extra care has been taken to make it consistent with the policy of not
exposing gamepad data to the page before a user gesture is observed.
When a new page starts listening it will not get any events until
we see a user gesture. When we see it we notify it about all the connected
pads.

Now we stop polling as soon as blink is no more interested in gamepad data.

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

[kbalazs, 2014-03-14 21:07:31]

This depends on the Blink part: https://codereview.chromium.org/200783002/. PTAL

[jam, 2014-03-17 19:01:00]

https://codereview.chromium.org/195873019/diff/1/content/browser/gamepad/game...
File content/browser/gamepad/gamepad_consumer.h (right):

https://codereview.chromium.org/195873019/diff/1/content/browser/gamepad/game...
content/browser/gamepad/gamepad_consumer.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
license header doesn't use "(c)" anymore, see dev.chromium.org page on this

https://codereview.chromium.org/195873019/diff/1/content/browser/gamepad/game...
content/browser/gamepad/gamepad_consumer.h:21: protected:
nit: here and for public, one space indent per style guide

https://codereview.chromium.org/195873019/diff/1/content/common/gamepad_conne...
File content/common/gamepad_connection_event_message_params.cc (right):

https://codereview.chromium.org/195873019/diff/1/content/common/gamepad_conne...
content/common/gamepad_connection_event_message_params.cc:1: #include
"content/common/gamepad_connection_event_message_params.h"
nit: need license header

https://codereview.chromium.org/195873019/diff/1/content/common/gamepad_conne...
content/common/gamepad_connection_event_message_params.cc:9: size_t
StringLengthInBytes(const blink::WebUChar web_string[N]) {
nit: no indentation in namespace

https://codereview.chromium.org/195873019/diff/1/content/common/gamepad_conne...
File content/common/gamepad_connection_event_message_params.h (right):

https://codereview.chromium.org/195873019/diff/1/content/common/gamepad_conne...
content/common/gamepad_connection_event_message_params.h:2: #define
CONTENT_COMMON_GAMEPAD_CONNECTION_EVENT_MESSAGE_PARAMS
need license header

https://codereview.chromium.org/195873019/diff/1/content/content_common.gypi
File content/content_common.gypi (right):

https://codereview.chromium.org/195873019/diff/1/content/content_common.gypi#...
content/content_common.gypi:717: 
why?

[kbalazs, 2014-03-20 20:42:49]

Fixes nits and some other changes.

https://codereview.chromium.org/195873019/diff/1/content/browser/gamepad/game...
File content/browser/gamepad/gamepad_consumer.h (right):

https://codereview.chromium.org/195873019/diff/1/content/browser/gamepad/game...
content/browser/gamepad/gamepad_consumer.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/03/17 19:01:00, jam wrote:
> license header doesn't use "(c)" anymore, see http://dev.chromium.org page on
this

Done.

https://codereview.chromium.org/195873019/diff/1/content/browser/gamepad/game...
content/browser/gamepad/gamepad_consumer.h:21: protected:
On 2014/03/17 19:01:00, jam wrote:
> nit: here and for public, one space indent per style guide

Done.

https://codereview.chromium.org/195873019/diff/1/content/common/gamepad_conne...
File content/common/gamepad_connection_event_message_params.cc (right):

https://codereview.chromium.org/195873019/diff/1/content/common/gamepad_conne...
content/common/gamepad_connection_event_message_params.cc:1: #include
"content/common/gamepad_connection_event_message_params.h"
On 2014/03/17 19:01:00, jam wrote:
> nit: need license header

Done.

https://codereview.chromium.org/195873019/diff/1/content/common/gamepad_conne...
content/common/gamepad_connection_event_message_params.cc:9: size_t
StringLengthInBytes(const blink::WebUChar web_string[N]) {
On 2014/03/17 19:01:00, jam wrote:
> nit: no indentation in namespace

Done.

https://codereview.chromium.org/195873019/diff/1/content/common/gamepad_conne...
File content/common/gamepad_connection_event_message_params.h (right):

https://codereview.chromium.org/195873019/diff/1/content/common/gamepad_conne...
content/common/gamepad_connection_event_message_params.h:2: #define
CONTENT_COMMON_GAMEPAD_CONNECTION_EVENT_MESSAGE_PARAMS
On 2014/03/17 19:01:00, jam wrote:
> need license header

Done.

[kbalazs, 2014-03-20 20:44:07]

More exactly the additional change is that we stop polling when blink set the
listener to 0. This is in accordance with the blink patch (and abarth's review
comments about it).

[jam, 2014-03-24 21:40:41]

On 2014/03/20 20:44:07, kbalazs wrote:
> More exactly the additional change is that we stop polling when blink set the
> listener to 0. This is in accordance with the blink patch (and abarth's review
> comments about it).

owner lgtm, i defer to the other folks for the details

[bajones, 2014-03-27 18:56:48]

LGTM, but that should be taken with a grain of salt since I wrote a small part
of the code here. All of the edge cases I would be concerned about seem to be
well accounted for though. I'd appreciate a second look from scottmg@.

cdn@ or cevans@: Could you review content/common?

https://codereview.chromium.org/195873019/diff/60001/content/common/gamepad_c...
File content/common/gamepad_connection_event_message_params.cc (right):

https://codereview.chromium.org/195873019/diff/60001/content/common/gamepad_c...
content/common/gamepad_connection_event_message_params.cc:14: size_t
StringLength(const blink::WebUChar web_string[N]) {
Suprised that we don't have a common way to do this already.

[kbalazs, 2014-03-27 23:25:32]

On 2014/03/27 18:56:48, bajones wrote:
> LGTM, but that should be taken with a grain of salt since I wrote a small part
> of the code here. All of the edge cases I would be concerned about seem to be
> well accounted for though. I'd appreciate a second look from scottmg@.

Unfortunately not (all of the edge cases are well accounted). I tested it on
Android and it doesn't work well.

When the app is paused (goes background) we remove the listener and readd it
when it is resumed. With this cl GamepadService will consider it as a new
GamepadConsumer that needs to wait for gesture. If in the meantime a device was
disconnected it will not be sent to the renderer. Also we shouldn't dispatch
'gamepadconnected' for all connected pads after another gesture again because
the page already tracks the state of gamepads. So I had to add some more extra
state to handle this. I am going to upload it in a sec.

It's tricky to handle all this cases, especially considering visibility state.
What should we do for a page that polled before becoming non-visible and now
becomes visible again? I think we should not try to inform it about changes that
happened in the meantime but that means the script cannot only depend on the
events to track the state of connected gamepads.

> 
> cdn@ or cevans@: Could you review content/common?
> 
>
https://codereview.chromium.org/195873019/diff/60001/content/common/gamepad_c...
> File content/common/gamepad_connection_event_message_params.cc (right):
> 
>
https://codereview.chromium.org/195873019/diff/60001/content/common/gamepad_c...
> content/common/gamepad_connection_event_message_params.cc:14: size_t
> StringLength(const blink::WebUChar web_string[N]) {
> Suprised that we don't have a common way to do this already.

Yes it's a bit lame but WebUChar[] is not a common string representation we use
when crossing boundaries of layers.

[kbalazs, 2014-03-31 15:36:59]

PTAL?

[kbalazs, 2014-03-31 22:55:43]

On 2014/03/31 15:36:59, kbalazs wrote:
> PTAL?

I mean the patch changed quite a lot, so I think we need another round :)

[bajones, 2014-04-01 05:29:58]

On 2014/03/31 22:55:43, kbalazs wrote:
> On 2014/03/31 15:36:59, kbalazs wrote:
> > PTAL?
> 
> I mean the patch changed quite a lot, so I think we need another round :)

Thanks for pointing out that the code has been significantly updated. I wouldn't
have thought to re-check it otherwise.

Updated code still LGTM. Still needs a reviewer for content/common.

[kbalazs, 2014-04-01 21:46:25]

On 2014/04/01 05:29:58, bajones wrote:
> On 2014/03/31 22:55:43, kbalazs wrote:
> > On 2014/03/31 15:36:59, kbalazs wrote:
> > > PTAL?
> > 
> > I mean the patch changed quite a lot, so I think we need another round :)
> 
> Thanks for pointing out that the code has been significantly updated. I
wouldn't
> have thought to re-check it otherwise.
> 
> Updated code still LGTM. Still needs a reviewer for content/common.

Jam has the power over that so it's good to go.

[kbalazs, 2014-04-02 17:03:58]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-04-02 17:04:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/195873019/160001

[commit-bot: I haz the power, 2014-04-02 17:32:23]

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

[commit-bot: I haz the power, 2014-04-02 17:32:24]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[kbalazs, 2014-04-02 19:12:59]

@cdn, @cevans: could you rubber stamp the messages in content/common please?

[Cris Neckar, 2014-04-02 19:54:19]

On 2014/04/02 19:12:59, kbalazs wrote:
> @cdn, @cevans: could you rubber stamp the messages in content/common please?

IPC messages LGTM

[kbalazs, 2014-04-02 22:46:30]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-04-02 22:46:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/195873019/160001

[commit-bot: I haz the power, 2014-04-02 22:55:40]

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

[commit-bot: I haz the power, 2014-04-02 22:55:40]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[kbalazs, 2014-04-03 00:41:02]

On 2014/04/02 22:55:40, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.chromium on win_chromium_compile_dbg

Oops, no ParamTraits for whar_t on windows. Probably I should rework the message
sending and specialize ParamTraits for WebGamepad.

[kbalazs, 2014-04-03 22:40:44]

On 2014/04/02 19:54:19, Cris Neckar wrote:
> On 2014/04/02 19:12:59, kbalazs wrote:
> > @cdn, @cevans: could you rubber stamp the messages in content/common please?
> 
> IPC messages LGTM

Please take another look, I reworked it by adding ParamTraits specialization for
blink::WebGamepad instead of using a helper class.

[kbalazs, 2014-04-08 16:37:46]

On 2014/04/03 22:40:44, kbalazs wrote:
> On 2014/04/02 19:54:19, Cris Neckar wrote:
> > On 2014/04/02 19:12:59, kbalazs wrote:
> > > @cdn, @cevans: could you rubber stamp the messages in content/common
please?
> > 
> > IPC messages LGTM
> 
> Please take another look, I reworked it by adding ParamTraits specialization
for
> blink::WebGamepad instead of using a helper class.

@cevans: ping :)

[kbalazs, 2014-04-23 19:14:45]

Could you please review the messages in content/common? I'm asking it for 2
weeks. I totally rewrite that part so I guess it's not ok to land without asking
for review again. The errors on try bots are definitely not related to the
patch.

[Cris Neckar, 2014-04-25 17:15:26]

On 2014/04/23 19:14:45, kbalazs wrote:
> Could you please review the messages in content/common? I'm asking it for 2
> weeks. I totally rewrite that part so I guess it's not ok to land without
asking
> for review again. The errors on try bots are definitely not related to the
> patch.

Sorry about that. Feel free to ping us on chat if we aren't responding.

I guess I am curious why you are writing your own ParamTraits here.
Couldn't you do this using IPC_STRUCT_TRAITS_ macros?

[kbalazs, 2014-04-25 17:29:44]

On 2014/04/25 17:15:26, Cris Neckar wrote:
> On 2014/04/23 19:14:45, kbalazs wrote:
> > Could you please review the messages in content/common? I'm asking it for 2
> > weeks. I totally rewrite that part so I guess it's not ok to land without
> asking
> > for review again. The errors on try bots are definitely not related to the
> > patch.
> 
> Sorry about that. Feel free to ping us on chat if we aren't responding.
> 
> I guess I am curious why you are writing your own ParamTraits here.
> Couldn't you do this using IPC_STRUCT_TRAITS_ macros?

No it doesn't work, there is no ParamTraits specialization for raw fixed size
arrays. Probably a better long term solution would be to add a that but I guess
this is ok for now?

[Cris Neckar, 2014-04-25 17:36:14]

On 2014/04/25 17:29:44, kbalazs wrote:
> On 2014/04/25 17:15:26, Cris Neckar wrote:
> > On 2014/04/23 19:14:45, kbalazs wrote:
> > > Could you please review the messages in content/common? I'm asking it for
2
> > > weeks. I totally rewrite that part so I guess it's not ok to land without
> > asking
> > > for review again. The errors on try bots are definitely not related to the
> > > patch.
> > 
> > Sorry about that. Feel free to ping us on chat if we aren't responding.
> > 
> > I guess I am curious why you are writing your own ParamTraits here.
> > Couldn't you do this using IPC_STRUCT_TRAITS_ macros?
> 
> No it doesn't work, there is no ParamTraits specialization for raw fixed size
> arrays. Probably a better long term solution would be to add a that but I
guess
> this is ok for now?

Alright then, lgtm for now. I wish we could avoid adding more of these though.

[kbalazs, 2014-04-25 17:48:39]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-04-25 17:53:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/195873019/200001

[commit-bot: I haz the power, 2014-04-25 22:33:58]

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

[commit-bot: I haz the power, 2014-04-25 22:33:59]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_compile_dbg

[kbalazs, 2014-04-28 06:28:00]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-04-28 06:28:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/195873019/200001

[commit-bot: I haz the power, 2014-04-28 06:37:51]

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

[commit-bot: I haz the power, 2014-04-28 06:37:51]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[kbalazs, 2014-04-28 16:03:32]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-04-28 16:03:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/195873019/220001

[commit-bot: I haz the power, 2014-04-28 16:17:13]

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

[commit-bot: I haz the power, 2014-04-28 16:17:14]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[kbalazs, 2014-04-28 22:07:10]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-04-28 22:08:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/195873019/240001

[commit-bot: I haz the power, 2014-04-28 23:04:35]

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

[commit-bot: I haz the power, 2014-04-28 23:04:36]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[kbalazs, 2014-04-28 23:42:06]

On 2014/04/28 23:04:36, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.chromium on chromium_presubmit

What a mess, now I need lgtm for the DEPS change. Isn't everyone a DEPS owner?

[abarth-chromium, 2014-04-29 16:58:47]

https://codereview.chromium.org/195873019/diff/240001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/195873019/diff/240001/content/browser/DEPS#ne...
content/browser/DEPS:36: "+third_party/WebKit/public/platform/WebGamepad.h",
LGTM

[kbalazs, 2014-04-29 16:59:42]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-04-29 17:00:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/195873019/240001

[commit-bot: I haz the power, 2014-04-30 02:45:53]

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

[commit-bot: I haz the power, 2014-04-30 02:45:54]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium

[kbalazs, 2014-04-30 18:21:49]

On 2014/04/30 02:45:54, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   win_chromium_x64_rel on tryserver.chromium

Hm, actually the windows bot have a point:

e:\b\build\slave\win\build\src\content\common\gamepad_param_traits.cc(62) :error
C2220: warning treated as error - no 'object' file generated
e:\b\build\slave\win\build\src\content\common\gamepad_messages.h(18) : warning
C4366: The result of the unary '&' operator may be unaligned

These unaligned accesses might bite us, especially on ARM. Maybe this could be
workarounded some other way but I think the right fix is to make sure WebGamepad
has a better packing which requires a blink change.

[kbalazs, 2014-05-02 18:20:10]

On 2014/04/30 18:21:49, kbalazs wrote:
> On 2014/04/30 02:45:54, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   win_chromium_x64_rel on tryserver.chromium
> 
> Hm, actually the windows bot have a point:
> 
> e:\b\build\slave\win\build\src\content\common\gamepad_param_traits.cc(62)
:error
> C2220: warning treated as error - no 'object' file generated
> e:\b\build\slave\win\build\src\content\common\gamepad_messages.h(18) : warning
> C4366: The result of the unary '&' operator may be unaligned
> 
> These unaligned accesses might bite us, especially on ARM. Maybe this could be
> workarounded some other way but I think the right fix is to make sure
WebGamepad
> has a better packing which requires a blink change.

Nevermind, memcpy is my good old friend here. I can just encode with one
WriteData and decode with one ReadData + memcpy. Still it's odd that we use
#pragma pack without a good reason but let's fix that when the repositories
merged.

[kbalazs, 2014-05-02 18:23:15]

On 2014/05/02 18:20:10, kbalazs wrote:
> On 2014/04/30 18:21:49, kbalazs wrote:
> > On 2014/04/30 02:45:54, I haz the power (commit-bot) wrote:
> > > Try jobs failed on following builders:
> > >   win_chromium_x64_rel on tryserver.chromium
> > 
> > Hm, actually the windows bot have a point:
> > 
> > e:\b\build\slave\win\build\src\content\common\gamepad_param_traits.cc(62)
> :error
> > C2220: warning treated as error - no 'object' file generated
> > e:\b\build\slave\win\build\src\content\common\gamepad_messages.h(18) :
warning
> > C4366: The result of the unary '&' operator may be unaligned
> > 
> > These unaligned accesses might bite us, especially on ARM. Maybe this could
be
> > workarounded some other way but I think the right fix is to make sure
> WebGamepad
> > has a better packing which requires a blink change.
> 
> Nevermind, memcpy is my good old friend here. I can just encode with one
> WriteData and decode with one ReadData + memcpy. Still it's odd that we use
> #pragma pack without a good reason but let's fix that when the repositories
> merged.

Looks like I didn't actually made the comment I supposed to answer here. So the
problem was that WebGamepad is declared with #pragma pack so it doesn't have a
sane alignment. And it's used in ppapi code as well (the exact same struct
defined and then a WebGamepad is bit_cast-ed to it) so there is no easy way to
fix it in blink without breaking chromium.

[kbalazs, 2014-05-02 18:23:46]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-05-02 18:24:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/195873019/260001

[commit-bot: I haz the power, 2014-05-02 22:18:09]

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

[commit-bot: I haz the power, 2014-05-02 22:18:10]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium

[bajones, 2014-05-02 22:27:49]

On 2014/05/02 22:18:10, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   win_chromium_rel on tryserver.chromium

The GPU bots were known bad for a while, but it should be cleared up now. The
failure on win_chromium_rel looks unrelated to your patch. Might want to try the
CQ once more to see if those bots cycle green, otherwise it may be best to land
the patch manually. If you're not a committer then I'd be happy to do it for
you.

I never would have dreamed this would be a month and a half of work, but thanks
for sticking with it! If you're ever in the Bay Area I owe you a
lunch/beer/something. :)

[bajones, 2014-05-02 23:06:53]

On 2014/05/02 22:27:49, bajones wrote:
> On 2014/05/02 22:18:10, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   win_chromium_rel on tryserver.chromium
> 
> The GPU bots were known bad for a while, but it should be cleared up now. The
> failure on win_chromium_rel looks unrelated to your patch. Might want to try
the
> CQ once more to see if those bots cycle green, otherwise it may be best to
land
> the patch manually. If you're not a committer then I'd be happy to do it for
> you.

Ah, I take it back. The win_x64 issue is real.

> I never would have dreamed this would be a month and a half of work, but
thanks
> for sticking with it! If you're ever in the Bay Area I owe you a
> lunch/beer/something. :)

This I do not take back. You're still awesome!

[kbalazs, 2014-05-02 23:25:54]

On 2014/05/02 23:06:53, bajones wrote:
> On 2014/05/02 22:27:49, bajones wrote:
> > On 2014/05/02 22:18:10, I haz the power (commit-bot) wrote:
> > > Try jobs failed on following builders:
> > >   win_chromium_rel on tryserver.chromium
> > 
> > The GPU bots were known bad for a while, but it should be cleared up now.
The
> > failure on win_chromium_rel looks unrelated to your patch. Might want to try
> the
> > CQ once more to see if those bots cycle green, otherwise it may be best to
> land
> > the patch manually. If you're not a committer then I'd be happy to do it for
> > you.
> 
> Ah, I take it back. The win_x64 issue is real.

Yep.

I forgot about this error with the macro. That's why I was trying to fix the
packing on WebGamepad at the first place, but than I had the idea that memcpy
can save me and I missed that this error has nothing to do with my code, it just
needs a properly packed type.

Anyways, I have a plan but that will involve some hack.

> 
> > I never would have dreamed this would be a month and a half of work, but
> thanks
> > for sticking with it! If you're ever in the Bay Area I owe you a
> > lunch/beer/something. :)
> 
> This I do not take back. You're still awesome!

Yay, if not sooner at the next BlinkOn we could have those beers. :)

[jam, 2014-05-05 17:20:31]

https://codereview.chromium.org/195873019/diff/260001/content/renderer/render...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/195873019/diff/260001/content/renderer/render...
content/renderer/render_thread_impl.h:372: void
SetGamepadListener(blink::WebGamepadListener* listener);
nit: how about we remove this method, and instead expose a getter for
gamepad_shared_memory_reader_? RenderThreadImpl shouldn't have to know about
every feature, other than being a central place that owns process-wide objects
in the renderer

[jam, 2014-05-05 17:42:59]

https://codereview.chromium.org/195873019/diff/260001/content/browser/rendere...
File content/browser/renderer_host/gamepad_browser_message_filter.h (right):

https://codereview.chromium.org/195873019/diff/260001/content/browser/rendere...
content/browser/renderer_host/gamepad_browser_message_filter.h:22: explicit
GamepadBrowserMessageFilter(IPC::Sender* sender);
this isn't safe, since the filter could outlive RenderProcessHost on its
shutdown. why pass this in anyways? BrowserMessageFilter derives from
IPC::Sender and you can use Send() already

[kbalazs, 2014-05-05 21:04:13]

> 
> I forgot about this error with the macro. That's why I was trying to fix the
> packing on WebGamepad at the first place, but than I had the idea that memcpy
> can save me and I missed that this error has nothing to do with my code, it
just
> needs a properly packed type.
> 
> Anyways, I have a plan but that will involve some hack.

LOL, I don't even need to use this macro for WebGamepadButtons now that I have
ParamTraits for WebGamepad. Alignment errors should gone now (crossing fingers).

[kbalazs, 2014-05-05 21:04:56]

On 2014/05/05 17:20:31, jam wrote:
>
https://codereview.chromium.org/195873019/diff/260001/content/renderer/render...
> File content/renderer/render_thread_impl.h (right):
> 
>
https://codereview.chromium.org/195873019/diff/260001/content/renderer/render...
> content/renderer/render_thread_impl.h:372: void
> SetGamepadListener(blink::WebGamepadListener* listener);
> nit: how about we remove this method, and instead expose a getter for
> gamepad_shared_memory_reader_? RenderThreadImpl shouldn't have to know about
> every feature, other than being a central place that owns process-wide objects
> in the renderer

Done.

[kbalazs, 2014-05-05 21:05:13]

On 2014/05/05 17:42:59, jam wrote:
>
https://codereview.chromium.org/195873019/diff/260001/content/browser/rendere...
> File content/browser/renderer_host/gamepad_browser_message_filter.h (right):
> 
>
https://codereview.chromium.org/195873019/diff/260001/content/browser/rendere...
> content/browser/renderer_host/gamepad_browser_message_filter.h:22: explicit
> GamepadBrowserMessageFilter(IPC::Sender* sender);
> this isn't safe, since the filter could outlive RenderProcessHost on its
> shutdown. why pass this in anyways? BrowserMessageFilter derives from
> IPC::Sender and you can use Send() already

Very true, thanks. Done.

[jam, 2014-05-05 21:19:08]

I took a closer look at this cl and here are my (last, promise) round of
comments.

https://codereview.chromium.org/195873019/diff/270001/content/browser/gamepad...
File content/browser/gamepad/gamepad_consumer.h (right):

https://codereview.chromium.org/195873019/diff/270001/content/browser/gamepad...
content/browser/gamepad/gamepad_consumer.h:14: class CONTENT_EXPORT
GamepadConsumer {
nit: I'm guessing you don't need the export

https://codereview.chromium.org/195873019/diff/270001/content/browser/gamepad...
content/browser/gamepad/gamepad_consumer.h:16: virtual void
OnGamepadConnected(unsigned index,
nit: per style guide, the first parameter in each line has to begin at the same
column. so change this to 

  virtual void OnGamepadConnected(
      unsigned index,
      const blink::WebGamepad& webGamepad) {}

also for below

https://codereview.chromium.org/195873019/diff/270001/content/browser/gamepad...
content/browser/gamepad/gamepad_consumer.h:17: const blink::WebGamepad&
webGamepad) {}
nit: use google style guide, i.e. web_gamepad or just gamepad

https://codereview.chromium.org/195873019/diff/270001/content/browser/gamepad...
File content/browser/gamepad/gamepad_provider.h (right):

https://codereview.chromium.org/195873019/diff/270001/content/browser/gamepad...
content/browser/gamepad/gamepad_provider.h:127: struct PadState {
per style guide, this should be a class and not struct. this means moving the
members to the private section and having getters.

https://codereview.chromium.org/195873019/diff/270001/content/browser/rendere...
File content/browser/renderer_host/gamepad_browser_message_filter.h (right):

https://codereview.chromium.org/195873019/diff/270001/content/browser/rendere...
content/browser/renderer_host/gamepad_browser_message_filter.h:22: explicit
GamepadBrowserMessageFilter();
nit: undo this

https://codereview.chromium.org/195873019/diff/270001/content/browser/rendere...
content/browser/renderer_host/gamepad_browser_message_filter.h:28: virtual void
OnGamepadConnected(
nit: convention is to start this section with "// GamepadConsumer
implementation"

https://codereview.chromium.org/195873019/diff/270001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_gamepad_host.h (right):

https://codereview.chromium.org/195873019/diff/270001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_gamepad_host.h:27: public
GamepadConsumer {
so you derived from this but don't need to implement any of its methods?

https://codereview.chromium.org/195873019/diff/270001/content/renderer/gamepa...
File content/renderer/gamepad_shared_memory_reader.h (right):

https://codereview.chromium.org/195873019/diff/270001/content/renderer/gamepa...
content/renderer/gamepad_shared_memory_reader.h:20: class
GamepadSharedMemoryReader : public IPC::MessageFilter {
generally, you would implement an IPC::MessageFilter in a child process if you
want to dispatch IPCs on a thread other than the main thread. but here, you're
dispatching them on the IO thread and then posttask'ing to the main thread. so
they should just be dispatched there directly.

the way to do this for control IPCs is to derive from RenderProcessObserver and
override OnControlMessageReceived.

[kbalazs, 2014-05-06 22:20:33]

Nits fixed, suggestions incorporated.

https://codereview.chromium.org/195873019/diff/270001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_gamepad_host.h (right):

https://codereview.chromium.org/195873019/diff/270001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_gamepad_host.h:27: public
GamepadConsumer {
On 2014/05/05 21:19:09, jam wrote:
> so you derived from this but don't need to implement any of its methods?

It needs to derive because GamepadService now holds GamepadConsumer pointers. I
have no clue whether the pepper thing needs the connection events but this is
not in the scope of the change. Now I made GamepadConsumer's methods pure
virtual and override them here with emtpy implementation, so it is clearer.

https://codereview.chromium.org/195873019/diff/270001/content/renderer/gamepa...
File content/renderer/gamepad_shared_memory_reader.h (right):

https://codereview.chromium.org/195873019/diff/270001/content/renderer/gamepa...
content/renderer/gamepad_shared_memory_reader.h:20: class
GamepadSharedMemoryReader : public IPC::MessageFilter {
On 2014/05/05 21:19:09, jam wrote:
> generally, you would implement an IPC::MessageFilter in a child process if you
> want to dispatch IPCs on a thread other than the main thread. but here, you're
> dispatching them on the IO thread and then posttask'ing to the main thread. so
> they should just be dispatched there directly.
> 
> the way to do this for control IPCs is to derive from RenderProcessObserver
and
> override OnControlMessageReceived.

Done, indeed much better this way.

[kbalazs, 2014-05-06 23:01:12]

@jam: I guess you want to take a final rubber-stamp, so PTAL.

[jam, 2014-05-06 23:39:13]

lgtm

[kbalazs, 2014-05-07 01:24:19]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-05-07 01:28:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/195873019/310001

[commit-bot: I haz the power, 2014-05-07 12:59:03]

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:
  android_clang_dbg on tryserver.chromium
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_clang_dbg on tryserver.chromium
  mac_chromium_compile_dbg on tryserver.chromium
  mac_chromium_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 14:35:18]

FYI, CQ is re-trying this CL (attempt #2).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_clang_dbg on tryserver.chromium
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_clang_dbg on tryserver.chromium
  mac_chromium_compile_dbg on tryserver.chromium
  mac_chromium_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 14:46:12]

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

[commit-bot: I haz the power, 2014-05-07 14:46:14]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium

[kbalazs, 2014-05-07 16:23:28]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-05-07 16:24:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/195873019/330001

[kbalazs, 2014-05-07 18:03:01]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-05-07 18:08:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/195873019/350001

[commit-bot: I haz the power, 2014-05-08 17:50:16]

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:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-08 22:54:09]

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

[commit-bot: I haz the power, 2014-05-08 22:54:10]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[kbalazs, 2014-05-08 23:42:40]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-05-08 23:53:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/195873019/350001

[commit-bot: I haz the power, 2014-05-09 06:21:12]

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:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-09 09:09:49]

Change committed as 269189