Issue 8568029: Add gamepad hardware data fetcher, and higher level thread container

Issue 8568029: Add gamepad hardware data fetcher, and higher level thread container (Closed)

Created:
9 years, 1 month ago by scottmg

Modified:
9 years ago

Reviewers:
cpu_(ooo_6.6-7.5), jam, Paweł Hajdan Jr.

CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr., jam, dpranke-watch+content_chromium.org

Base URL:
http://git.chromium.org/chromium/src.git@master

Visibility:
Public.

More Reviews

Description

Add gamepad hardware data fetcher, and higher level thread container DataFetcher is the hardware and platform-specific data getter that talks to OS level interfaces for each platform and fill in a structure provided to it. Currently only Windows is here. Provider is a manager for the background thread that the DataFetcher runs on, and interacts with the rest of browser. This is part of a larger patch, the remainder of which is at http://codereview.chromium.org/8345027/ BUG=79050 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111189 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111456

Patch Set 1 #

Total comments: 46

Patch Set 2 : updates per review #

Patch Set 3 : fix merge #

Patch Set 4 : fix non-windows #

Patch Set 5 : update test per review #

Patch Set 6 : remove unneeded include #

Patch Set 7 : try to remove unrelated gypi changes #

Patch Set 8 : fix for older windows sdk #

Patch Set 9 : add virtual for OVERRIDE checker #

Patch Set 10 : add some silly osx init code #

Total comments: 2

Patch Set 11 : unittest improvements #

Total comments: 2

Patch Set 12 : review changes #

Patch Set 13 : delayload xinput for XP #

Total comments: 4

Patch Set 14 : review fixes #

Patch Set 15 : shared_library fix (was already committed, but then rolled out) #

Patch Set 16 : swap search path order for platformsdk/directx #

Created: 9 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+584 lines, -3 lines)			Patch
M	build/common.gypi	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	3 chunks	+3 lines, -3 lines	0 comments	Download
A	content/browser/gamepad/data_fetcher.h	View	1	1 chunk	+23 lines, -0 lines	0 comments	Download
A	content/browser/gamepad/data_fetcher_win.h	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+32 lines, -0 lines	0 comments	Download
A	content/browser/gamepad/data_fetcher_win.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+173 lines, -0 lines	0 comments	Download
A	content/browser/gamepad/gamepad_provider.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+78 lines, -0 lines	0 comments	Download
A	content/browser/gamepad/gamepad_provider.cc	View	1 2 3	1 chunk	+145 lines, -0 lines	0 comments	Download
A	content/browser/gamepad/gamepad_provider_unittest.cc	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+123 lines, -0 lines	0 comments	Download
M	content/content_browser.gypi	View	1 2 3 4 5 6 7 8 9 10 11 12	2 chunks	+6 lines, -0 lines	0 comments	Download
M	content/content_tests.gypi	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 22 (0 generated)

Expand Messages | Collapse Messages

jam

mostly nits please only add one reviewer instead of multiple, unless absolutely necessary (and then, ...

9 years, 1 month ago (2011-11-16 00:24:58 UTC) #2

Paweł Hajdan Jr.

Drive-by with testing related comments. I noticed patterns correlated with flakiness in the test code. ...

9 years, 1 month ago (2011-11-16 08:36:35 UTC) #3

scottmg

(other reviewers removed) http://codereview.chromium.org/8568029/diff/1/content/browser/gamepad/data_fetcher.cc File content/browser/gamepad/data_fetcher.cc (right): http://codereview.chromium.org/8568029/diff/1/content/browser/gamepad/data_fetcher.cc#newcode9 content/browser/gamepad/data_fetcher.cc:9: DataFetcher::~DataFetcher() { On 2011/11/16 00:24:58, John ...

9 years, 1 month ago (2011-11-16 18:04:35 UTC) #4

Paweł Hajdan Jr.

http://codereview.chromium.org/8568029/diff/1/content/browser/gamepad/gamepad_provider_unittest.cc File content/browser/gamepad/gamepad_provider_unittest.cc (right): http://codereview.chromium.org/8568029/diff/1/content/browser/gamepad/gamepad_provider_unittest.cc#newcode76 content/browser/gamepad/gamepad_provider_unittest.cc:76: base::PlatformThread::Sleep(300); On 2011/11/16 18:04:35, scottmg wrote: > On 2011/11/16 ...

9 years, 1 month ago (2011-11-17 09:40:59 UTC) #6

scottmg

On 2011/11/17 09:40:59, Paweł Hajdan Jr. wrote: > http://codereview.chromium.org/8568029/diff/1/content/browser/gamepad/gamepad_provider_unittest.cc > File content/browser/gamepad/gamepad_provider_unittest.cc (right): > > ...

9 years, 1 month ago (2011-11-17 17:52:31 UTC) #7

On 2011/11/17 09:40:59, Paweł Hajdan Jr. wrote:
>
http://codereview.chromium.org/8568029/diff/1/content/browser/gamepad/gamepad...
> File content/browser/gamepad/gamepad_provider_unittest.cc (right):
> 
>
http://codereview.chromium.org/8568029/diff/1/content/browser/gamepad/gamepad...
> content/browser/gamepad/gamepad_provider_unittest.cc:76:
> base::PlatformThread::Sleep(300);
> On 2011/11/16 18:04:35, scottmg wrote:
> > On 2011/11/16 08:36:35, Paweł Hajdan Jr. wrote:
> > > Why? Is it inherently timing-dependent, or should you wait on some event?
> > What's
> > > the purpose of this Sleep?
> > 
> > The background thread polls hardware every 16ms. There's (intentionally) no
> > explicit synchronization between hardware polling and the consumer of the
data
> > so there's no event available.
> > 
> > Thank you for pointing this out, I will need to keep an eye on this test to
> make
> > sure it doesn't cause flakyness.
> 
> If it's background thread polling every 16ms, it's very scheduler-dependent
> whether 300ms is the right amount of time to wait. Could you just wait for the
> first poll to finish?

Done.

http://codereview.chromium.org/8568029/diff/1/content/browser/gamepad/gamepad...
ittest.cc#newcode98
> content/browser/gamepad/gamepad_provider_unittest.cc:98:
> base::PlatformThread::YieldCurrentThread();
> On 2011/11/16 18:04:35, scottmg wrote:
> > On 2011/11/16 08:36:35, Paweł Hajdan Jr. wrote:
> > > What's the purpose of this? Tests generally run single-threaded, and this
> test
> > > doesn't seem to create any background threads (or maybe the code under
test
> > > does?).
> > 
> > That's right, the code under test creates a background thread.
> 
> Still, what's the purpose of this? Threads should be fairly scheduled anyway,
I
> don't see a reason for an explicit yield. Furthermore, is it possible that the
> loop will never finish? It's not obvious to me that there is no starvation
here.

I agree the explicit yield is not necessary, but it's intended as a
processor-burning-avoidance hint when there contention on read is detected.

Yes, starvation is possible as with any spin, but in reality the contention
should be extremely low. Added an EXPECT to that effect.

commit-bot: I haz the power

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

9 years, 1 month ago (2011-11-17 23:28:19 UTC) #8

commit-bot: I haz the power

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

9 years, 1 month ago (2011-11-18 00:10:53 UTC) #9

Paweł Hajdan Jr.

*Please* don't hit the CQ just because you think the problems are solved. This is ...

9 years, 1 month ago (2011-11-18 11:59:07 UTC) #10

scottmg

On 2011/11/18 11:59:07, Paweł Hajdan Jr. wrote: > *Please* don't hit the CQ just because ...

9 years, 1 month ago (2011-11-18 16:20:44 UTC) #11

On 2011/11/18 11:59:07, Paweł Hajdan Jr. wrote:
> *Please* don't hit the CQ just because you think the problems are solved. This
> is especially true in case of drive-by reviews.

My apologies, I thought that's what drive-by meant (i.e. they were suggestions
for improvements, rather than blocking the check in). Noted for future.

> 
>
http://codereview.chromium.org/8568029/diff/27001/content/browser/gamepad/gam...
> File content/browser/gamepad/gamepad_provider_unittest.cc (right):
> 
>
http://codereview.chromium.org/8568029/diff/27001/content/browser/gamepad/gam...
> content/browser/gamepad/gamepad_provider_unittest.cc:86:
> base::PlatformThread::Sleep(10);
> Sorry, this is still bad. Please wait for en _event_, i.e.
data_available.Wait()
> (base::WaitableEvent). Or maybe TimedWait if you want a timeout.

OK, will update.

>
http://codereview.chromium.org/8568029/diff/27001/content/browser/gamepad/gam...
> content/browser/gamepad/gamepad_provider_unittest.cc:108: EXPECT_LT(10,
> contention_count);
> This will still make the test hang if you're unlucky.

Ah, it looks like I meant ASSERT_LT then? I wanted the test to fail if that ever
happened.

> Is there really no other
> way to query this?
> Is it the way "production code" queries the buffer?

Yes that's how the production code queries it. In the case of the "production
code", it would probably make most sense to simply try the read only once, and
if there's contention, simply return the old data (i.e. the last valid read). In
the case of the test code here, I'm trying to test the logic of the contention
test, so looping seems appropriate to me.

On further reflection, it would be clearer to loop a fixed number of times and
verify that the data had been read after that number of times, and otherwise
fail the test. Does that seem OK to you?

> Suggestion: maybe instead of waiting for start marker and end marker to align,
> we can wait for some number of units in the buffer, and just take that? I
didn't
> read other code here, but I'm really suspicious when seeing loops like that.

Paweł Hajdan Jr.

http://codereview.chromium.org/8568029/diff/25002/content/browser/gamepad/gamepad_provider_unittest.cc File content/browser/gamepad/gamepad_provider_unittest.cc (right): http://codereview.chromium.org/8568029/diff/25002/content/browser/gamepad/gamepad_provider_unittest.cc#newcode99 content/browser/gamepad/gamepad_provider_unittest.cc:99: for (contention_count = 0; contention_count < 10; ++contention_count) { ...

9 years, 1 month ago (2011-11-21 12:09:37 UTC) #12

scottmg

On 2011/11/21 12:09:37, Paweł Hajdan Jr. wrote: > http://codereview.chromium.org/8568029/diff/25002/content/browser/gamepad/gamepad_provider_unittest.cc > File content/browser/gamepad/gamepad_provider_unittest.cc (right): > > ...

9 years, 1 month ago (2011-11-21 17:39:57 UTC) #13

commit-bot: I haz the power

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

9 years, 1 month ago (2011-11-22 17:26:31 UTC) #15

scottmg

cpu: could you have look at the delayload related code in data_fetcher_win.cc? The rest is ...

9 years, 1 month ago (2011-11-23 01:44:13 UTC) #17

cpu_(ooo_6.6-7.5)

lgtm with two nits http://codereview.chromium.org/8568029/diff/37001/content/browser/gamepad/data_fetcher_win.cc File content/browser/gamepad/data_fetcher_win.cc (right): http://codereview.chromium.org/8568029/diff/37001/content/browser/gamepad/data_fetcher_win.cc#newcode61 content/browser/gamepad/data_fetcher_win.cc:61: // Exception is not related ...

9 years, 1 month ago (2011-11-23 01:58:27 UTC) #18

scottmg

http://codereview.chromium.org/8568029/diff/37001/content/browser/gamepad/data_fetcher_win.cc File content/browser/gamepad/data_fetcher_win.cc (right): http://codereview.chromium.org/8568029/diff/37001/content/browser/gamepad/data_fetcher_win.cc#newcode61 content/browser/gamepad/data_fetcher_win.cc:61: // Exception is not related to delay loading On ...

9 years, 1 month ago (2011-11-23 02:10:09 UTC) #19

commit-bot: I haz the power

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

9 years, 1 month ago (2011-11-23 02:14:56 UTC) #20

commit-bot: I haz the power

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

9 years ago (2011-11-23 22:54:01 UTC) #21

Change committed as 111456

Expand Messages | Collapse Messages