Refactored gamepad polling to support dynamic sources

Refactored gamepad polling to support dynamic sources

Second attempt at landing. (See https://codereview.chromium.org/1586663006/)
Most gamepad code has been relocated to device/ since the original CL.

Reduces duplicate code between platform data fetchers by having things like
sanitation happen in the provider, and makes the data fetchers themselves more
modular. Data fetcher types can now be added and removed via the
GamepadDataFetcherManager.

BUG=577414
Committed: https://crrev.com/667f05771b5c1275f87599076d222c0ed21ba22b
Cr-Commit-Position: refs/heads/master@{#411375}

[bajones, 2016-07-07 22:29:20]

bajones@chromium.org changed reviewers:
+ scottmg@chromium.org

[bajones, 2016-07-07 22:29:20]

scottmg@: I know you're OOO at the moment, but I'm not sure who else would be
appropriate to review something like this. Happy to include another reviewer if
you have a suggestion!

This is largely the same as the previously landed and reverted
https://codereview.chromium.org/1586663006/, with the biggest changes being
accounting for the move of most gamepad code to device/ and the addition of the
GamepadDataFetcherManager. I've been meaning to get back to it for a while but
only recently have had a more pressing need to do so.

[bajones, 2016-07-07 22:29:28]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-07 22:30:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 22:33:02]

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

[commit-bot: I haz the power, 2016-07-07 22:33:03]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[bajones, 2016-07-07 22:45:49]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-07 22:46:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 23:02:40]

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

[commit-bot: I haz the power, 2016-07-07 23:02:41]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[bajones, 2016-07-08 18:13:39]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 18:14:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 18:33:30]

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

[commit-bot: I haz the power, 2016-07-08 18:33:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bajones, 2016-07-08 19:51:54]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 19:52:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 20:08:35]

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

[commit-bot: I haz the power, 2016-07-08 20:08:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bajones, 2016-07-08 21:10:48]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 21:11:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 22:33:36]

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

[commit-bot: I haz the power, 2016-07-08 22:33:37]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[bajones, 2016-07-08 22:39:00]

Okay, several dry-run passes later and the patch appears to work correctly cross
platform. :)

[bajones, 2016-07-11 17:51:06]

bajones@chromium.org changed reviewers:
+ denniskempin@chromium.org

[bajones, 2016-07-11 17:51:06]

denniskempin@: Since scottmg is going to be out for a while and you've been
digging into related code quite a bit recently, would you be willing to review
this change? It's been OKed in the past before the directory migration but was
reverted due to a pepper quirk at the time and I never got around to fixing it.

Any yes, this almost certainly conflicts at least a little bit with your patch.
:) I'm happy to rebase it against your CL when it lands.

[denniskempin, 2016-07-11 22:44:49]

denniskempin@google.com changed reviewers:
+ denniskempin@google.com

[denniskempin, 2016-07-11 22:44:50]

lgtm. However I don't know much about the osx/windows side. And I also don't
know if my lgtm counts for anything here ;)

https://codereview.chromium.org/2129003002/diff/80001/device/gamepad/gamepad_...
File device/gamepad/gamepad_data_fetcher.h (right):

https://codereview.chromium.org/2129003002/diff/80001/device/gamepad/gamepad_...
device/gamepad/gamepad_data_fetcher.h:18: GamepadDataFetcher();
Since the GamepadDataFetcher is now constructed on a different thread than where
it's being used.. would it make sense to add a thread checker?

https://codereview.chromium.org/2129003002/diff/80001/device/gamepad/gamepad_...
File device/gamepad/gamepad_provider_unittest.cc (right):

https://codereview.chromium.org/2129003002/diff/80001/device/gamepad/gamepad_...
device/gamepad/gamepad_provider_unittest.cc:148: // Tests that waiting for a
user gesture works properly.
update comment

[denniskempin, 2016-07-11 22:47:37]

On 2016/07/11 22:44:50, denniskempin wrote:
> lgtm. However I don't know much about the osx/windows side. And I also don't
> know if my lgtm counts for anything here ;)
> 
>
https://codereview.chromium.org/2129003002/diff/80001/device/gamepad/gamepad_...
> File device/gamepad/gamepad_data_fetcher.h (right):
> 
>
https://codereview.chromium.org/2129003002/diff/80001/device/gamepad/gamepad_...
> device/gamepad/gamepad_data_fetcher.h:18: GamepadDataFetcher();
> Since the GamepadDataFetcher is now constructed on a different thread than
where
> it's being used.. would it make sense to add a thread checker?
> 
>
https://codereview.chromium.org/2129003002/diff/80001/device/gamepad/gamepad_...
> File device/gamepad/gamepad_provider_unittest.cc (right):
> 
>
https://codereview.chromium.org/2129003002/diff/80001/device/gamepad/gamepad_...
> device/gamepad/gamepad_provider_unittest.cc:148: // Tests that waiting for a
> user gesture works properly.
> update comment

I am not sure how strict the merging rules are for M53, but keep in mind that we
will have to merge this to M53 for gamepad support on ARC++ as well. If you
think this might become a problem, maybe we could submit my exo gamepad code
first and then adjust my code in this CL here as well.

[bajones, 2016-07-11 23:02:52]

On 2016/07/11 22:44:50, denniskempin wrote:
> lgtm. However I don't know much about the osx/windows side. And I also don't
> know if my lgtm counts for anything here ;)

Since I'm an owner your LGTM is enough to satisfy the prechecks. And like I
said, you've been digging in this code more recently than anyone other than
myself.

> Since the GamepadDataFetcher is now constructed on a different thread than
where
> it's being used.. would it make sense to add a thread checker?

Good call. I'll see what makes sense here.

> update comment

Thanks, will do.

> I am not sure how strict the merging rules are for M53, but keep in mind that
we
> will have to merge this to M53 for gamepad support on ARC++ as well. If you
> think this might become a problem, maybe we could submit my exo gamepad code
> first and then adjust my code in this CL here as well.

I don't intend to merge this CL to 53. I'd rather have your code land and merge
that and it's dependent CL and THEN land this one. It's too big of a change for
a cherrypick IMO. I'm assuming your change isn't too far out from landing?
(within the next week?)

[denniskempin, 2016-07-11 23:49:23]

On 2016/07/11 23:02:52, bajones wrote:
> On 2016/07/11 22:44:50, denniskempin wrote:
> > lgtm. However I don't know much about the osx/windows side. And I also don't
> > know if my lgtm counts for anything here ;)
> 
> Since I'm an owner your LGTM is enough to satisfy the prechecks. And like I
> said, you've been digging in this code more recently than anyone other than
> myself.
> 
> > Since the GamepadDataFetcher is now constructed on a different thread than
> where
> > it's being used.. would it make sense to add a thread checker?
> 
> Good call. I'll see what makes sense here.
> 
> > update comment
> 
> Thanks, will do.
> 
> > I am not sure how strict the merging rules are for M53, but keep in mind
that
> we
> > will have to merge this to M53 for gamepad support on ARC++ as well. If you
> > think this might become a problem, maybe we could submit my exo gamepad code
> > first and then adjust my code in this CL here as well.
> 
> I don't intend to merge this CL to 53. I'd rather have your code land and
merge
> that and it's dependent CL and THEN land this one. It's too big of a change
for
> a cherrypick IMO. I'm assuming your change isn't too far out from landing?
> (within the next week?)

Just a few nits left. Hopefully it'll merge tomorrow :)

[bajones, 2016-07-14 18:53:22]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-07-14 18:53:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
denniskempin@google.com
Link to the patchset: https://codereview.chromium.org/2129003002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-07-14 18:54:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 19:08:24]

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

[commit-bot: I haz the power, 2016-07-14 19:08:25]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bajones, 2016-08-03 23:49:59]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 23:50:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 00:05:28]

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

[commit-bot: I haz the power, 2016-08-04 00:05:29]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bajones, 2016-08-04 00:36:14]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 00:36:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 00:48:32]

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

[commit-bot: I haz the power, 2016-08-04 00:48:33]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[bajones, 2016-08-07 03:09:23]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-07 03:09:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-07 04:17:07]

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

[commit-bot: I haz the power, 2016-08-07 04:17:09]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[bajones, 2016-08-07 05:21:49]

bajones@chromium.org changed reviewers:
+ reveman@chromium.org

[bajones, 2016-08-07 05:21:51]

reveman@: Please review changes in component/exo/

Well, that was a fun bit of blind debugging to get exo_unittests passing again!
I really ought to figure out the build environment for ChromeOS one of these
days. Feeling pretty good

[bajones, 2016-08-07 05:24:13]

This was a fun bit of blind debugging to get exo_unittests working again. :P I
really ought to figure out a ChromeOS build environment one of these days.

reveman@: Please review changes components/exo/

denniskempin@: You probably want to take a look at exo/gamepad.cc again too,
just to double check my logic after the rebase.

[reveman, 2016-08-07 16:45:44]

components/exo lgtm with nits

https://codereview.chromium.org/2129003002/diff/360001/components/exo/gamepad.cc
File components/exo/gamepad.cc (right):

https://codereview.chromium.org/2129003002/diff/360001/components/exo/gamepad...
components/exo/gamepad.cc:125: fetcher_->GetGamepadData(false);
nit: not new to this patch but can you add (false /* name */) here where 'name'
explains what this magic boolean is for?

https://codereview.chromium.org/2129003002/diff/360001/components/exo/gamepad...
components/exo/gamepad.cc:130: if (!pad_state.active_state &&
nit: I don't really understand this part. Can you add a comment explaining why
this 'if' statement is needed and why needs to be like this?

https://codereview.chromium.org/2129003002/diff/360001/components/exo/gamepad...
components/exo/gamepad.cc:135: MapAndSanitizeGamepadData(&pad_state,
&new_state.items[0], false);
nit: false /* name */ here too

https://codereview.chromium.org/2129003002/diff/360001/components/exo/gamepad...
components/exo/gamepad.cc:137: if (pad_state.active_state) {
nit: also don't understand this. if active_state is true then we force it to
INACTIVE and increment length? can you add a comment?

[bajones, 2016-08-07 17:30:15]

On 2016/08/07 16:45:44, reveman wrote:
> components/exo lgtm with nits
> 
>
https://codereview.chromium.org/2129003002/diff/360001/components/exo/gamepad.cc
> File components/exo/gamepad.cc (right):
> 
>
https://codereview.chromium.org/2129003002/diff/360001/components/exo/gamepad...
> components/exo/gamepad.cc:125: fetcher_->GetGamepadData(false);
> nit: not new to this patch but can you add (false /* name */) here where
'name'
> explains what this magic boolean is for?
> 
>
https://codereview.chromium.org/2129003002/diff/360001/components/exo/gamepad...
> components/exo/gamepad.cc:130: if (!pad_state.active_state &&
> nit: I don't really understand this part. Can you add a comment explaining why
> this 'if' statement is needed and why needs to be like this?
> 
>
https://codereview.chromium.org/2129003002/diff/360001/components/exo/gamepad...
> components/exo/gamepad.cc:135: MapAndSanitizeGamepadData(&pad_state,
> &new_state.items[0], false);
> nit: false /* name */ here too
> 
>
https://codereview.chromium.org/2129003002/diff/360001/components/exo/gamepad...
> components/exo/gamepad.cc:137: if (pad_state.active_state) {
> nit: also don't understand this. if active_state is true then we force it to
> INACTIVE and increment length? can you add a comment?

Thanks for the feedback! Comments added. Much of the weirdness is pulled from
code that makes more sense in it's original context in GamepadProvider, so I
apologize for the obscurity caused by porting it over.

[reveman, 2016-08-07 19:45:08]

On 2016/08/07 at 17:30:15, bajones wrote:
> On 2016/08/07 16:45:44, reveman wrote:
> > components/exo lgtm with nits
> > 
> >
https://codereview.chromium.org/2129003002/diff/360001/components/exo/gamepad.cc
> > File components/exo/gamepad.cc (right):
> > 
> >
https://codereview.chromium.org/2129003002/diff/360001/components/exo/gamepad...
> > components/exo/gamepad.cc:125: fetcher_->GetGamepadData(false);
> > nit: not new to this patch but can you add (false /* name */) here where
'name'
> > explains what this magic boolean is for?
> > 
> >
https://codereview.chromium.org/2129003002/diff/360001/components/exo/gamepad...
> > components/exo/gamepad.cc:130: if (!pad_state.active_state &&
> > nit: I don't really understand this part. Can you add a comment explaining
why
> > this 'if' statement is needed and why needs to be like this?
> > 
> >
https://codereview.chromium.org/2129003002/diff/360001/components/exo/gamepad...
> > components/exo/gamepad.cc:135: MapAndSanitizeGamepadData(&pad_state,
> > &new_state.items[0], false);
> > nit: false /* name */ here too
> > 
> >
https://codereview.chromium.org/2129003002/diff/360001/components/exo/gamepad...
> > components/exo/gamepad.cc:137: if (pad_state.active_state) {
> > nit: also don't understand this. if active_state is true then we force it to
> > INACTIVE and increment length? can you add a comment?
> 
> Thanks for the feedback! Comments added. Much of the weirdness is pulled from
code that makes more sense in it's original context in GamepadProvider, so I
apologize for the obscurity caused by porting it over.

Thanks for the comments. lgtm.

[bajones, 2016-08-08 18:06:35]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-08-08 18:06:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
denniskempin@google.com
Link to the patchset: https://codereview.chromium.org/2129003002/#ps380001
(title: "Added requested comments")

[commit-bot: I haz the power, 2016-08-08 18:07:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 19:23:20]

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

[commit-bot: I haz the power, 2016-08-08 19:23:24]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[bajones, 2016-08-11 16:13:23]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-08-11 16:13:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
denniskempin@google.com, reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2129003002/#ps400001
(title: "Disabled sanitization test on Android. Suffers from same bug as
PollingAccess test")

[commit-bot: I haz the power, 2016-08-11 16:13:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 18:11:54]

Committed patchset #21 (id:400001)

[commit-bot: I haz the power, 2016-08-11 18:14:16]

Description was changed from

==========
Refactored gamepad polling to support dynamic sources

Second attempt at landing. (See https://codereview.chromium.org/1586663006/)
Most gamepad code has been relocated to device/ since the original CL.

Reduces duplicate code between platform data fetchers by having things like
sanitation happen in the provider, and makes the data fetchers themselves more
modular. Data fetcher types can now be added and removed via the
GamepadDataFetcherManager.

BUG=577414
==========

to

==========
Refactored gamepad polling to support dynamic sources

Second attempt at landing. (See https://codereview.chromium.org/1586663006/)
Most gamepad code has been relocated to device/ since the original CL.

Reduces duplicate code between platform data fetchers by having things like
sanitation happen in the provider, and makes the data fetchers themselves more
modular. Data fetcher types can now be added and removed via the
GamepadDataFetcherManager.

BUG=577414

Committed: https://crrev.com/667f05771b5c1275f87599076d222c0ed21ba22b
Cr-Commit-Position: refs/heads/master@{#411375}
==========

[commit-bot: I haz the power, 2016-08-11 18:14:18]

Patchset 21 (id:??) landed as
https://crrev.com/667f05771b5c1275f87599076d222c0ed21ba22b
Cr-Commit-Position: refs/heads/master@{#411375}