Refactoring gamepad polling to support dynamically added sources

Refactoring gamepad polling to support dynamically added sources

Also reduces duplicate code between platform data fetchers by having things like
sanitation happen in the provider, and makes the data fetchers themselves more
modular.

BUG=577414
Committed: https://crrev.com/618039679323b6fc09e2146768f121661b0cdd41
Cr-Commit-Position: refs/heads/master@{#371083}

[bajones, 2016-01-14 00:09:58]

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

[bajones, 2016-01-14 00:09:59]

Apologies for the large patch, but it's not clear to me what an effective way to
break it up would be.

Despite the noisy diff I think this simplifies gamepad handling by moving the
common bits all up to the provider level and eliminating the need to have just
one DataFetcher per platform (which meant that things like XInput and Raw Input
were awkwardly being handled in the same class.)

Ultimate goal is to enable dynamic gamepad sources for things like VR, where the
gamepads won't report at all until some other separate system has been
initialized, so it doesn't make sense to poll them till said system is active.
That'll require a bit more work to support DataFetcher removal, though, and this
CL was big enough as-is.

[scottmg, 2016-01-15 20:45:50]

On 2016/01/14 00:09:59, bajones wrote:
> Apologies for the large patch, but it's not clear to me what an effective way
to
> break it up would be.
> 
> Despite the noisy diff I think this simplifies gamepad handling by moving the
> common bits all up to the provider level and eliminating the need to have just
> one DataFetcher per platform (which meant that things like XInput and Raw
Input
> were awkwardly being handled in the same class.)
> 
> Ultimate goal is to enable dynamic gamepad sources for things like VR, where
the
> gamepads won't report at all until some other separate system has been
> initialized, so it doesn't make sense to poll them till said system is active.
> That'll require a bit more work to support DataFetcher removal, though, and
this
> CL was big enough as-is.

Hi, I've started looking at this, but I'm a little overwhelmed, it might take me
a while still. Sorry for the delay.

[bajones, 2016-01-15 21:15:09]

On 2016/01/15 20:45:50, scottmg wrote:
> On 2016/01/14 00:09:59, bajones wrote:
> > Apologies for the large patch, but it's not clear to me what an effective
way
> to
> > break it up would be.
> > 
> > Despite the noisy diff I think this simplifies gamepad handling by moving
the
> > common bits all up to the provider level and eliminating the need to have
just
> > one DataFetcher per platform (which meant that things like XInput and Raw
> Input
> > were awkwardly being handled in the same class.)
> > 
> > Ultimate goal is to enable dynamic gamepad sources for things like VR, where
> the
> > gamepads won't report at all until some other separate system has been
> > initialized, so it doesn't make sense to poll them till said system is
active.
> > That'll require a bit more work to support DataFetcher removal, though, and
> this
> > CL was big enough as-is.
> 
> Hi, I've started looking at this, but I'm a little overwhelmed, it might take
me
> a while still. Sorry for the delay.

No worries! I know it's a large patch that came out of the blue. Whenever you
have time.

[scottmg, 2016-01-18 22:54:52]

Mostly just nits. I'll try and do another pass, but I think it generally looks
like a nice refactoring.

Is there test coverage for the dynamic adding/removing that this is supporting?

This also moves around a lot of code that's only really tested manually, it'd
probably be a good idea to talk to QA to make sure they do a manual verification
of gamepad for M50 (I think we just branched?) insofar as possible.

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
File content/browser/gamepad/gamepad_data_fetcher.h (right):

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_data_fetcher.h:34: virtual void
OnAddedToProvider() {};
Stray ;

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc (right):

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:59: :
xinput_available_(false) {
Initialize xinput_connected_ array here.

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:126: 
Extra blank line.

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
File content/browser/gamepad/gamepad_platform_data_fetcher_win.h (right):

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_platform_data_fetcher_win.h:66: bool
xinuput_connected_[XUSER_MAX_COUNT];
xinuput -> xinput

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
File content/browser/gamepad/gamepad_provider.cc (right):

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_provider.cc:209: // Loop through each registered
data fetcher and poll it's gamepad data.
nit; its

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
File content/browser/gamepad/gamepad_provider.h (right):

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_provider.h:53: // Data fetcher-specific
identifier for this gamepad.
Blank before // line.

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_provider.h:159: // Keep track of when a polling
task is schedlued, so as to prevent us from
scheduled

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_provider.h:205: //static GamepadProvider*
instance_;
Delete.

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
File content/browser/gamepad/gamepad_provider_unittest.cc (right):

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_provider_unittest.cc:210: // gamepad reported
input
'.'s to end comments in this file.

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
File content/browser/gamepad/gamepad_service.h (right):

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_service.h:93:
GamepadService(scoped_ptr<GamepadDataFetcher> fetcher);
explicit

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_service.h:97: static void
SetInstance(GamepadService*);
GamepadService* instance

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_service.h:102: ConsumerInfo(GamepadConsumer*
consumer)
explicit

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
File content/browser/gamepad/gamepad_service_unittest.cc (right):

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_service_unittest.cc:83: // Must be set to zero
initially to pass sanitiation
Zero? But .value = 1.f?

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
File content/browser/gamepad/gamepad_test_helpers.cc (right):

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_test_helpers.cc:25: if
(test_data_.items[i].connected) {
Does this need to reset anything in the non-connected state? It looks like it
would have before.

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/raw...
File content/browser/gamepad/raw_input_data_fetcher_win.cc (right):

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/raw...
content/browser/gamepad/raw_input_data_fetcher_win.cc:289:
gamepad_info->source_id = ++last_source_id_;
Initialize enumeration_id here maybe? I can't tell if it could ever be
mistakenly referenced.

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/xbo...
File content/browser/gamepad/xbox_data_fetcher_mac.h (right):

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/xbo...
content/browser/gamepad/xbox_data_fetcher_mac.h:174: //Delegate* delegate_;
Delete.

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/xbo...
content/browser/gamepad/xbox_data_fetcher_mac.h:192: } // namespace content
2 space before //

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/xbo...
File content/browser/gamepad/xbox_data_fetcher_mac.mm (right):

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/xbo...
content/browser/gamepad/xbox_data_fetcher_mac.mm:800:
//delegate_->XboxDeviceAdd(controller);
Delete.

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/xbo...
content/browser/gamepad/xbox_data_fetcher_mac.mm:841:
//delegate_->XboxDeviceRemove(controller);
Delete.

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/xbo...
content/browser/gamepad/xbox_data_fetcher_mac.mm:862:
//delegate_->XboxValueChanged(controller, data);
Delete.

[bajones, 2016-01-20 22:49:32]

I've added a test for dynamic data fetcher addition (removal isn't supported
just yet). I'll make sure that QA knows about the changes, but I don't think it
should change too much for them.

The rest of your comments have been addressed. PTAL! (And thanks for calling out
some stuff that wasn't changed in the patch but needed cleanup anyway!)

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
File content/browser/gamepad/gamepad_test_helpers.cc (right):

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_test_helpers.cc:25: if
(test_data_.items[i].connected) {
On 2016/01/18 22:54:52, scottmg wrote:
> Does this need to reset anything in the non-connected state? It looks like it
> would have before.

That resetting now happens in the GamepadProvider. It's one of the benefits of
this refactor that we don't rely on the data fetchers for that kind of
management/cleanup any more.

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/raw...
File content/browser/gamepad/raw_input_data_fetcher_win.cc (right):

https://codereview.chromium.org/1586663006/diff/1/content/browser/gamepad/raw...
content/browser/gamepad/raw_input_data_fetcher_win.cc:289:
gamepad_info->source_id = ++last_source_id_;
On 2016/01/18 22:54:52, scottmg wrote:
> Initialize enumeration_id here maybe? I can't tell if it could ever be
> mistakenly referenced.

I don't think it can be, but it also doesn't hurt to initialize it here.

[scottmg, 2016-01-21 19:31:48]

I was a little confused by the INACTIVE/ACTIVE/NEWLY_ACTIVE state, but I think
it looks fine.

I wish we had some automated testing with robots or something to try
refactorings like this on real gamepads. :)

LGTM

[bajones, 2016-01-21 21:11:21]

On 2016/01/21 19:31:48, scottmg wrote:
> I wish we had some automated testing with robots or something to try
> refactorings like this on real gamepads. :)

I'm all for filing a request for a gamepad mashing robots from facilities. Maybe
we can rent some from Valve? https://youtu.be/uCgnWqoP4MM?t=1m10s

[bajones, 2016-01-21 21:11:27]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2016-01-21 21:11:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1586663006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1586663006/40001

[commit-bot: I haz the power, 2016-01-21 21:28:38]

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

[commit-bot: I haz the power, 2016-01-21 21:28:39]

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

[bajones, 2016-01-21 23:48:39]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-01-21 23:48:39]

The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1586663006/#ps60001
(title: "Trying to fix build error")

[commit-bot: I haz the power, 2016-01-21 23:49:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1586663006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1586663006/60001

[commit-bot: I haz the power, 2016-01-22 02:28:20]

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

[commit-bot: I haz the power, 2016-01-22 02:28:20]

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

[bajones, 2016-01-22 22:21:02]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-01-22 22:21:02]

The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1586663006/#ps80001
(title: "Avoid crash on Android content_unittests")

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1586663006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1586663006/80001

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

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-22 23:41:57]

Description was changed from

==========
Refactoring gamepad polling to support dynamically added sources

Also reduces duplicate code between platform data fetchers by having things like
sanitation happen in the provider, and makes the data fetchers themselves more
modular.

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

to

==========
Refactoring gamepad polling to support dynamically added sources

Also reduces duplicate code between platform data fetchers by having things like
sanitation happen in the provider, and makes the data fetchers themselves more
modular.

BUG=577414

Committed: https://crrev.com/618039679323b6fc09e2146768f121661b0cdd41
Cr-Commit-Position: refs/heads/master@{#371083}
==========

[commit-bot: I haz the power, 2016-01-22 23:41:58]

Patchset 5 (id:??) landed as
https://crrev.com/618039679323b6fc09e2146768f121661b0cdd41
Cr-Commit-Position: refs/heads/master@{#371083}

[bajones, 2016-01-23 06:37:47]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1627643002/ by bajones@chromium.org.

The reason for reverting is: Some MSan bots started failing
PepperGamepadHostTest.WaitForReply after landing this, I'm going to have to
assume that it's related and look at it later next week.

https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20ChromeOS%20...
https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Test....