Renderer reading side of gamepad

Renderer reading side of gamepad

The is the glue code that reads from the shared memory buffer on the renderer
side. The writer is a background thread the browser. The data here is returned
back up into WebKit code via the Chromium port.

The writer on the browser side was in this patch:
http://codereview.chromium.org/8568029/

And the rest of the related gamepad changes are here:
http://codereview.chromium.org/8345027/

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

[scottmg, 2011-11-24 23:06:20]

GamepadUtil::SampleGamepads is the fairly subtle shared memory reading
contention-detection behaviour we had discussed before. LMK if you want to
review in person.

[scottmg, 2011-11-24 23:15:01]

Should have added for reference: writer to shared memory is landed in
content/browser/gamepad/gamepad_provider.cc, GamepadProvider::DoPoll().

[darin (slow to review), 2011-11-28 17:35:49]

http://codereview.chromium.org/8689011/diff/5001/content/renderer/gamepad_uti...
File content/renderer/gamepad_util.cc (right):

http://codereview.chromium.org/8689011/diff/5001/content/renderer/gamepad_uti...
content/renderer/gamepad_util.cc:14: using namespace content;
nit: the google c++ style guide forbids 'using namespace foo'

note: i think that the contents of this file should live in the content
namespace since you are in the content/ subdir.

http://codereview.chromium.org/8689011/diff/5001/content/renderer/gamepad_uti...
content/renderer/gamepad_util.cc:17: GamepadUtil::GamepadUtil() {
please come up with a better name for this class.  GamepadUtil is overly
generic.  we have too many of these "Util" classes in chromium.

how about GamepadReader or GamepadSharedMemoryReader?

http://codereview.chromium.org/8689011/diff/5001/content/renderer/gamepad_uti...
content/renderer/gamepad_util.cc:42: end =
base::subtle::Acquire_Load(&gamepad_hardware_buffer_->end_marker);
i had expected to see a memory barrier here.  i admit, i'm not an expert on
these topics though.

i'm curious why you designed this to use Acquire_Load for the end and start
markers.  that doesn't equate to a memory barrier on x86.

http://codereview.chromium.org/8689011/diff/5001/content/renderer/gamepad_uti...
content/renderer/gamepad_util.cc:43: memcpy(&read_into,
&gamepad_hardware_buffer_->buffer, sizeof(gamepads));
i'm not the expert on our dynamic annotations system, but you may want to
sprinkle some ANNOTATE_HAPPENS_{BEFORE,AFTER} macros in here.

you may also want to get this reviewed by timurrrr.  it may be wise to factor
this chunk of sensitive code out into a separate function.

http://codereview.chromium.org/8689011/diff/5001/content/renderer/gamepad_uti...
content/renderer/gamepad_util.cc:72: const unsigned primaryInteractionButtons =
4;
nit: use kFooBar for google c++ style constants.

http://codereview.chromium.org/8689011/diff/5001/content/renderer/gamepad_uti...
content/renderer/gamepad_util.cc:84: RenderThread::Get()->Send(new
GamepadHostMsg_StopPolling());
note: i'm not sure if you are doing this yet, but you'll want the browser-side
code to monitor renderer process crashes so that you can also StopPolling when a
renderer dies unexpectedly.

[scottmg, 2011-11-28 20:02:08]

http://codereview.chromium.org/8689011/diff/5001/content/renderer/gamepad_uti...
File content/renderer/gamepad_util.cc (right):

http://codereview.chromium.org/8689011/diff/5001/content/renderer/gamepad_uti...
content/renderer/gamepad_util.cc:14: using namespace content;
On 2011/11/28 17:35:49, darin wrote:
> nit: the google c++ style guide forbids 'using namespace foo'
> 
> note: i think that the contents of this file should live in the content
> namespace since you are in the content/ subdir.

Done.

http://codereview.chromium.org/8689011/diff/5001/content/renderer/gamepad_uti...
content/renderer/gamepad_util.cc:17: GamepadUtil::GamepadUtil() {
On 2011/11/28 17:35:49, darin wrote:
> please come up with a better name for this class.  GamepadUtil is overly
> generic.  we have too many of these "Util" classes in chromium.
> 
> how about GamepadReader or GamepadSharedMemoryReader?

Done.

http://codereview.chromium.org/8689011/diff/5001/content/renderer/gamepad_uti...
content/renderer/gamepad_util.cc:42: end =
base::subtle::Acquire_Load(&gamepad_hardware_buffer_->end_marker);
On 2011/11/28 17:35:49, darin wrote:
> i had expected to see a memory barrier here.  i admit, i'm not an expert on
> these topics though.
> 
> i'm curious why you designed this to use Acquire_Load for the end and start
> markers.  that doesn't equate to a memory barrier on x86.

I'm afraid that's probably a bug then. I thought I'd understood the semantics of
acquire/release but perhaps I'm confused -- will revisit.

http://codereview.chromium.org/8689011/diff/5001/content/renderer/gamepad_uti...
content/renderer/gamepad_util.cc:43: memcpy(&read_into,
&gamepad_hardware_buffer_->buffer, sizeof(gamepads));
On 2011/11/28 17:35:49, darin wrote:
> i'm not the expert on our dynamic annotations system, but you may want to
> sprinkle some ANNOTATE_HAPPENS_{BEFORE,AFTER} macros in here.
> 
> you may also want to get this reviewed by timurrrr.  it may be wise to factor
> this chunk of sensitive code out into a separate function.

OK, will do.

http://codereview.chromium.org/8689011/diff/5001/content/renderer/gamepad_uti...
content/renderer/gamepad_util.cc:72: const unsigned primaryInteractionButtons =
4;
On 2011/11/28 17:35:49, darin wrote:
> nit: use kFooBar for google c++ style constants.

Done.

http://codereview.chromium.org/8689011/diff/5001/content/renderer/gamepad_uti...
content/renderer/gamepad_util.cc:84: RenderThread::Get()->Send(new
GamepadHostMsg_StopPolling());
On 2011/11/28 17:35:49, darin wrote:
> note: i'm not sure if you are doing this yet, but you'll want the browser-side
> code to monitor renderer process crashes so that you can also StopPolling when
a
> renderer dies unexpectedly.

After talking to John I have some other work to do on the background thread.
Thanks for the pointer, I'll make sure to do this.

[scottmg, 2011-11-28 22:38:58]

> > i'm curious why you designed this to use Acquire_Load for the end and start
> > markers.  that doesn't equate to a memory barrier on x86.
> 
> I'm afraid that's probably a bug then. I thought I'd understood the semantics
of
> acquire/release but perhaps I'm confused -- will revisit.

jbates pointed me to the fact that this construct is called a seqlock
(http://en.wikipedia.org/wiki/Seqlock). I'm going to pull the synchronization
parts out into base/synchronization/seqlock.h, which will keep the tricky bits
localized (and out of this higher-level gamepad code).

[darin (slow to review), 2011-11-28 22:58:35]

On Mon, Nov 28, 2011 at 2:38 PM, <scottmg@chromium.org> wrote:

> > i'm curious why you designed this to use Acquire_Load for the end and
>> start
>> > markers.  that doesn't equate to a memory barrier on x86.
>>
>
>  I'm afraid that's probably a bug then. I thought I'd understood the
>> semantics
>>
> of
>
>> acquire/release but perhaps I'm confused -- will revisit.
>>
>
> jbates pointed me to the fact that this construct is called a seqlock
>
(http://en.wikipedia.org/wiki/**Seqlock<http://en.wikipedia.org/wiki/Seqlock>).
> I'm going to pull the synchronization
> parts out into base/synchronization/seqlock.**h, which will keep the
> tricky bits
> localized (and out of this higher-level gamepad code).
>

Sounds great to me!


>
>
http://codereview.chromium.**org/8689011/<http://codereview.chromium.org/8689...
>

[jbates, 2011-11-29 00:05:26]

http://codereview.chromium.org/8689011/diff/11004/base/synchronization/seqlock.h
File base/synchronization/seqlock.h (right):

http://codereview.chromium.org/8689011/diff/11004/base/synchronization/seqloc...
base/synchronization/seqlock.h:49: subtle::Barrier_AtomicIncrement(&sequence_,
1);
I'm not sure why there's BarrierAtomicIncrement and not
Acquire/ReleaseAtomicIncrement, but I think it may be cleaner to use
Acquire/Release style to get the correct barrier ordering. Since there is only
one writer, how about this:
subtle::Acquire_Store(&sequence_, sequence_ + 1);

http://codereview.chromium.org/8689011/diff/11004/base/synchronization/seqloc...
base/synchronization/seqlock.h:57: subtle::Barrier_AtomicIncrement(&sequence_,
1);
And this:
subtle::Release_Store(&sequence_, sequence_ + 1);

[scottmg, 2011-11-29 00:39:58]

On 2011/11/29 00:05:26, jbates wrote:
>
http://codereview.chromium.org/8689011/diff/11004/base/synchronization/seqlock.h
> File base/synchronization/seqlock.h (right):
> 
>
http://codereview.chromium.org/8689011/diff/11004/base/synchronization/seqloc...
> base/synchronization/seqlock.h:49: subtle::Barrier_AtomicIncrement(&sequence_,
> 1);
> I'm not sure why there's BarrierAtomicIncrement and not
> Acquire/ReleaseAtomicIncrement, but I think it may be cleaner to use
> Acquire/Release style to get the correct barrier ordering. Since there is only
> one writer, how about this:
> subtle::Acquire_Store(&sequence_, sequence_ + 1);
> 
>
http://codereview.chromium.org/8689011/diff/11004/base/synchronization/seqloc...
> base/synchronization/seqlock.h:57: subtle::Barrier_AtomicIncrement(&sequence_,
> 1);
> And this:
> subtle::Release_Store(&sequence_, sequence_ + 1);

I believe you're right. I would prefer to stick to the simpler AtomicIncrement
(both an acquire and release barrier) as a conservative first attempt, and defer
your suggested improvement to a future change, if that seems reasonable.

[jbates, 2011-11-29 00:49:00]

On 2011/11/29 00:39:58, scottmg wrote:
> On 2011/11/29 00:05:26, jbates wrote:
> >
>
http://codereview.chromium.org/8689011/diff/11004/base/synchronization/seqlock.h
> > File base/synchronization/seqlock.h (right):
> > 
> >
>
http://codereview.chromium.org/8689011/diff/11004/base/synchronization/seqloc...
> > base/synchronization/seqlock.h:49:
subtle::Barrier_AtomicIncrement(&sequence_,
> > 1);
> > I'm not sure why there's BarrierAtomicIncrement and not
> > Acquire/ReleaseAtomicIncrement, but I think it may be cleaner to use
> > Acquire/Release style to get the correct barrier ordering. Since there is
only
> > one writer, how about this:
> > subtle::Acquire_Store(&sequence_, sequence_ + 1);
> > 
> >
>
http://codereview.chromium.org/8689011/diff/11004/base/synchronization/seqloc...
> > base/synchronization/seqlock.h:57:
subtle::Barrier_AtomicIncrement(&sequence_,
> > 1);
> > And this:
> > subtle::Release_Store(&sequence_, sequence_ + 1);
> 
> I believe you're right. I would prefer to stick to the simpler AtomicIncrement
> (both an acquire and release barrier) as a conservative first attempt, and
defer
> your suggested improvement to a future change, if that seems reasonable.

SGTM

[darin (slow to review), 2011-11-30 18:34:37]

http://codereview.chromium.org/8689011/diff/10008/content/browser/gamepad/gam...
File content/browser/gamepad/gamepad_provider_unittest.cc (right):

http://codereview.chromium.org/8689011/diff/10008/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_provider_unittest.cc:12: 
nit: A 'using WebKit::WebGamepads" might be nice here.

http://codereview.chromium.org/8689011/diff/10008/content/common/gamepad_seql...
File content/common/gamepad_seqlock.h (right):

http://codereview.chromium.org/8689011/diff/10008/content/common/gamepad_seql...
content/common/gamepad_seqlock.h:11: namespace gamepad {
I think we probably want new code in the content/ module to be in the content::
namespace.  Jam confirms.

http://codereview.chromium.org/8689011/diff/10008/content/common/gamepad_seql...
content/common/gamepad_seqlock.h:34: base::subtle::Atomic32 ReadBegin() {
why inline these functions?

http://codereview.chromium.org/8689011/diff/10008/content/common/gamepad_seql...
File content/common/gamepad_seqlock_unittest.cc (right):

http://codereview.chromium.org/8689011/diff/10008/content/common/gamepad_seql...
content/common/gamepad_seqlock_unittest.cc:84:
base::subtle::Release_Store(&ready, kNumReaderThreads);
Would it be possible to implement |ready| using atomic_ref_count.h instead? 
That might help make this code easier to understand.  As is, I'm forced to fully
internalize the base::subtle:: methods in order to read this code, but if you
can express this just in terms of reference counting, then it might make the
code more readable.

[scottmg, 2011-11-30 19:43:31]

http://codereview.chromium.org/8689011/diff/10008/content/browser/gamepad/gam...
File content/browser/gamepad/gamepad_provider_unittest.cc (right):

http://codereview.chromium.org/8689011/diff/10008/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_provider_unittest.cc:12: 
On 2011/11/30 18:34:37, darin wrote:
> nit: A 'using WebKit::WebGamepads" might be nice here.

Done.

http://codereview.chromium.org/8689011/diff/10008/content/common/gamepad_seql...
File content/common/gamepad_seqlock.h (right):

http://codereview.chromium.org/8689011/diff/10008/content/common/gamepad_seql...
content/common/gamepad_seqlock.h:11: namespace gamepad {
On 2011/11/30 18:34:37, darin wrote:
> I think we probably want new code in the content/ module to be in the
content::
> namespace.  Jam confirms.

Phew, that turned out to be a lot of places (not content::gamepad::, just
content::, right?).

Couple classes deserved to be renamed when living in content:: (Provider ->
GamepadProvider, DataFetcher -> GamepadDataFetcher) so there's some (more)
noise. Seems tidier overall though.

http://codereview.chromium.org/8689011/diff/10008/content/common/gamepad_seql...
content/common/gamepad_seqlock.h:34: base::subtle::Atomic32 ReadBegin() {
On 2011/11/30 18:34:37, darin wrote:
> why inline these functions?

4/5 seemed trivial enough (other than ReadBegin), but there's not a major
benefit either i guess: moved out of line.

http://codereview.chromium.org/8689011/diff/10008/content/common/gamepad_seql...
File content/common/gamepad_seqlock_unittest.cc (right):

http://codereview.chromium.org/8689011/diff/10008/content/common/gamepad_seql...
content/common/gamepad_seqlock_unittest.cc:84:
base::subtle::Release_Store(&ready, kNumReaderThreads);
On 2011/11/30 18:34:37, darin wrote:
> Would it be possible to implement |ready| using atomic_ref_count.h instead? 
> That might help make this code easier to understand.  As is, I'm forced to
fully
> internalize the base::subtle:: methods in order to read this code, but if you
> can express this just in terms of reference counting, then it might make the
> code more readable.

Better, thanks. Done.

[darin (slow to review), 2011-11-30 21:06:22]

LGTM

http://codereview.chromium.org/8689011/diff/21004/content/browser/gamepad/dat...
File content/browser/gamepad/data_fetcher.h (right):

http://codereview.chromium.org/8689011/diff/21004/content/browser/gamepad/dat...
content/browser/gamepad/data_fetcher.h:14: class GamepadDataFetcher {
I hate to create more work for you, but the name of the file should match the
name of the class.  (Remember to update the include guards too.)

If you want to stage these changes that's fine.

http://codereview.chromium.org/8689011/diff/21004/content/browser/gamepad/gam...
File content/browser/gamepad/gamepad_provider_unittest.cc (right):

http://codereview.chromium.org/8689011/diff/21004/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_provider_unittest.cc:19: class MockDataFetcher :
public content::GamepadDataFetcher {
I think it is also OK for all of the stuff in this file to live in the content::
namespace.  That way you don't need all of the awkward using directives or the
content:: prefixes.

See for example:  net/base/net_log_unittest.cc

http://codereview.chromium.org/8689011/diff/21004/content/common/gamepad_seql...
File content/common/gamepad_seqlock_unittest.cc (right):

http://codereview.chromium.org/8689011/diff/21004/content/common/gamepad_seql...
content/common/gamepad_seqlock_unittest.cc:35: while
(AtomicRefCountIsZero(ready_)) {}
Should this call PlatformThread::YieldCurrentThread()?

[scottmg, 2011-11-30 21:22:39]

thanks for quick (re-)review.

http://codereview.chromium.org/8689011/diff/21004/content/browser/gamepad/dat...
File content/browser/gamepad/data_fetcher.h (right):

http://codereview.chromium.org/8689011/diff/21004/content/browser/gamepad/dat...
content/browser/gamepad/data_fetcher.h:14: class GamepadDataFetcher {
On 2011/11/30 21:06:22, darin wrote:
> I hate to create more work for you, but the name of the file should match the
> name of the class.  (Remember to update the include guards too.)
> 
> If you want to stage these changes that's fine.

OK, will that do as part of moving things to gamepad/ subdirs so that there's
just one change renaming/moving everything around.

http://codereview.chromium.org/8689011/diff/21004/content/browser/gamepad/gam...
File content/browser/gamepad/gamepad_provider_unittest.cc (right):

http://codereview.chromium.org/8689011/diff/21004/content/browser/gamepad/gam...
content/browser/gamepad/gamepad_provider_unittest.cc:19: class MockDataFetcher :
public content::GamepadDataFetcher {
On 2011/11/30 21:06:22, darin wrote:
> I think it is also OK for all of the stuff in this file to live in the
content::
> namespace.  That way you don't need all of the awkward using directives or the
> content:: prefixes.
> 
> See for example:  net/base/net_log_unittest.cc

Done.

http://codereview.chromium.org/8689011/diff/21004/content/common/gamepad_seql...
File content/common/gamepad_seqlock_unittest.cc (right):

http://codereview.chromium.org/8689011/diff/21004/content/common/gamepad_seql...
content/common/gamepad_seqlock_unittest.cc:35: while
(AtomicRefCountIsZero(ready_)) {}
On 2011/11/30 21:06:22, darin wrote:
> Should this call PlatformThread::YieldCurrentThread()?

Not necessary to be correct, but no reason to burn CPU either. Done.

[commit-bot: I haz the power, 2011-11-30 21:24:02]

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

[commit-bot: I haz the power, 2011-11-30 21:24:09]

Can't apply patch for file content/content_tests.gypi.
While running patch -p1 --forward --force;
patching file content/content_tests.gypi
Hunk #1 FAILED at 223.
1 out of 1 hunk FAILED -- saving rejects to file content/content_tests.gypi.rej

[commit-bot: I haz the power, 2011-11-30 22:11:27]

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

[commit-bot: I haz the power, 2011-12-01 00:01:40]

Change committed as 112345