Implement the content/renderer and content/browser part of the Device Motion API.

Implement the content/renderer and content/browser part of the Device Motion API.
This patch has a partial implementation of provider and does not include any device motion fetchers.

The patch features:
- chromium-side event pump
- shared memory reader and buffer representation
- IPC messages and filters for shared memory communication
- service class to keep track of listening renderers

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

[darin (slow to review), 2013-05-16 19:49:15]

high level comment:  how about creating a renderer/device_orientation/ directory
to mirror the one on the browser side?  that way you can have an OWNERS file
too.

[jamesr, 2013-05-16 19:53:05]

It looks like the data being passed around is around 24 bytes and the pump rate
is 40ms - are you sure that the complicated shared memory system + atomics is
worth the complexity vs just passing this down via the normal IPC mechanism when
it changes?

https://codereview.chromium.org/14678012/diff/1/content/common/device_motion_...
File content/common/device_motion_hardware_buffer.h (right):

https://codereview.chromium.org/14678012/diff/1/content/common/device_motion_...
content/common/device_motion_hardware_buffer.h:13: /*
we pretty much never use /* style comments in chromium

https://codereview.chromium.org/14678012/diff/1/content/common/device_motion_...
content/common/device_motion_hardware_buffer.h:25: GamepadSeqLock sequence;
Gamepad? What does this have to do with game pad?

https://codereview.chromium.org/14678012/diff/1/content/renderer/device_motio...
File content/renderer/device_motion_event_pump.cc (right):

https://codereview.chromium.org/14678012/diff/1/content/renderer/device_motio...
content/renderer/device_motion_event_pump.cc:22: {
This is chromium - the { goes on the previous line here and in all other
functions

https://codereview.chromium.org/14678012/diff/1/content/renderer/device_motio...
content/renderer/device_motion_event_pump.cc:45:
base::Bind(&DeviceMotionEventPump::fire, base::Unretained(this)));
unretained? what prevents this task from running after the pump is torn down?

I think you may want a base::RepeatingTimer here

https://codereview.chromium.org/14678012/diff/1/content/renderer/device_motio...
content/renderer/device_motion_event_pump.cc:52: LOG(INFO) << "stop
listening\n";
This is way too useless to spam up the INFO log.  If you want to have a way to
crank logging up for this system, consider a VLOG() or DVLOG()

https://codereview.chromium.org/14678012/diff/1/content/renderer/device_motio...
content/renderer/device_motion_event_pump.cc:61: return true;
why does this function take a data parameter if it doesn't use it?

https://codereview.chromium.org/14678012/diff/1/content/renderer/device_motio...
content/renderer/device_motion_event_pump.cc:84:
base::Bind(&DeviceMotionEventPump::fire, base::Unretained(this)),
same thing here re the unretained

https://codereview.chromium.org/14678012/diff/1/content/renderer/device_motio...
File content/renderer/device_motion_event_pump.h (right):

https://codereview.chromium.org/14678012/diff/1/content/renderer/device_motio...
content/renderer/device_motion_event_pump.h:28: const static double
pump_delay_millis = 40;
put the definition in a translation unit or you'll get multiple copies. why does
this have to be public?

https://codereview.chromium.org/14678012/diff/1/content/renderer/render_threa...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/14678012/diff/1/content/renderer/render_threa...
content/renderer/render_thread_impl.cc:746: //     
command_line.HasSwitch(switches::kEnableDeviceMotion));
huh?

https://codereview.chromium.org/14678012/diff/1/content/renderer/renderer_web...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/14678012/diff/1/content/renderer/renderer_web...
content/renderer/renderer_webkitplatformsupport_impl.cc:904: if
(!device_motion_event_pump_.get())
just

if (!device_motion_event_pump_)
  ...

[jamesr, 2013-05-16 19:58:21]

https://codereview.chromium.org/14678012/diff/1/content/common/device_motion_...
File content/common/device_motion_messages.h (right):

https://codereview.chromium.org/14678012/diff/1/content/common/device_motion_...
content/common/device_motion_messages.h:21:
IPC_SYNC_MESSAGE_CONTROL0_1(DeviceMotionHostMsg_StartPolling,
Why synchronous?

https://codereview.chromium.org/14678012/diff/1/content/common/device_motion_...
content/common/device_motion_messages.h:24:
IPC_SYNC_MESSAGE_CONTROL0_0(DeviceMotionHostMsg_StopPolling)
Ditto - why synchronous?

[timvolodine, 2013-05-20 18:12:37]

On 2013/05/16 19:49:15, darin wrote:
> high level comment:  how about creating a renderer/device_orientation/
directory
> to mirror the one on the browser side?  that way you can have an OWNERS file
> too.

Done.

[timvolodine, 2013-05-20 18:18:13]

On 2013/05/16 19:53:05, jamesr wrote:
> It looks like the data being passed around is around 24 bytes and the pump
rate
> is 40ms - are you sure that the complicated shared memory system + atomics is
> worth the complexity vs just passing this down via the normal IPC mechanism
when
> it changes?

It's actually 82 bytes to be precise (when packed). There is probably something
to say for both approaches, with the main advantage of shared memory being that
it is shared between all renderers. Also in the case of device motion multiple
sensors are involved and updated independently in case of Android (is push
based). To avoid too many updates we need a polling kind of system anyway.

[timvolodine, 2013-05-20 18:18:47]

thanks for comments!

https://codereview.chromium.org/14678012/diff/1/content/common/device_motion_...
File content/common/device_motion_hardware_buffer.h (right):

https://codereview.chromium.org/14678012/diff/1/content/common/device_motion_...
content/common/device_motion_hardware_buffer.h:13: /*
On 2013/05/16 19:53:05, jamesr wrote:
> we pretty much never use /* style comments in chromium

it is actually used in gamepad_hardware_buffer, on which this class is based.
I've changed it to // comments on each line.

https://codereview.chromium.org/14678012/diff/1/content/common/device_motion_...
content/common/device_motion_hardware_buffer.h:25: GamepadSeqLock sequence;
On 2013/05/16 19:53:06, jamesr wrote:
> Gamepad? What does this have to do with game pad?

As a first step the GamepadLock can be used for shared buffer sync. In a
subsequent step we can implement a generalization of this (as noted in the
GamepadLock itself).

Also, does it make sense to subdivide content/common into
content/common/device_orientation, or maybe content/common/shared_memory to put
the shared mem related code e.g. locks there?

https://codereview.chromium.org/14678012/diff/1/content/common/device_motion_...
File content/common/device_motion_messages.h (right):

https://codereview.chromium.org/14678012/diff/1/content/common/device_motion_...
content/common/device_motion_messages.h:21:
IPC_SYNC_MESSAGE_CONTROL0_1(DeviceMotionHostMsg_StartPolling,
On 2013/05/16 19:58:21, jamesr wrote:
> Why synchronous?

we need a valid handle to the shared memory from the browser before we start
polling.

The actual call is not completely synchronous anyway, in the sense that we don't
wait for sensors to start firing, just a shared memory buffer handle needs to be
returned.

https://codereview.chromium.org/14678012/diff/1/content/common/device_motion_...
content/common/device_motion_messages.h:24:
IPC_SYNC_MESSAGE_CONTROL0_0(DeviceMotionHostMsg_StopPolling)
On 2013/05/16 19:58:21, jamesr wrote:
> Ditto - why synchronous?

this can be async. Done.

https://codereview.chromium.org/14678012/diff/1/content/renderer/device_motio...
File content/renderer/device_motion_event_pump.cc (right):

https://codereview.chromium.org/14678012/diff/1/content/renderer/device_motio...
content/renderer/device_motion_event_pump.cc:22: {
On 2013/05/16 19:53:06, jamesr wrote:
> This is chromium - the { goes on the previous line here and in all other
> functions

Done.

https://codereview.chromium.org/14678012/diff/1/content/renderer/device_motio...
content/renderer/device_motion_event_pump.cc:45:
base::Bind(&DeviceMotionEventPump::fire, base::Unretained(this)));
On 2013/05/16 19:53:06, jamesr wrote:
> unretained? what prevents this task from running after the pump is torn down?
> 
> I think you may want a base::RepeatingTimer here

switched to RepeatedTimer, looks like this is exactly what is needed.

https://codereview.chromium.org/14678012/diff/1/content/renderer/device_motio...
content/renderer/device_motion_event_pump.cc:52: LOG(INFO) << "stop
listening\n";
On 2013/05/16 19:53:06, jamesr wrote:
> This is way too useless to spam up the INFO log.  If you want to have a way to
> crank logging up for this system, consider a VLOG() or DVLOG()

Done.

https://codereview.chromium.org/14678012/diff/1/content/renderer/device_motio...
content/renderer/device_motion_event_pump.cc:61: return true;
On 2013/05/16 19:53:06, jamesr wrote:
> why does this function take a data parameter if it doesn't use it?

Done.

https://codereview.chromium.org/14678012/diff/1/content/renderer/device_motio...
content/renderer/device_motion_event_pump.cc:84:
base::Bind(&DeviceMotionEventPump::fire, base::Unretained(this)),
On 2013/05/16 19:53:06, jamesr wrote:
> same thing here re the unretained

Done.

https://codereview.chromium.org/14678012/diff/1/content/renderer/device_motio...
File content/renderer/device_motion_event_pump.h (right):

https://codereview.chromium.org/14678012/diff/1/content/renderer/device_motio...
content/renderer/device_motion_event_pump.h:28: const static double
pump_delay_millis = 40;
On 2013/05/16 19:53:06, jamesr wrote:
> put the definition in a translation unit or you'll get multiple copies. why
does
> this have to be public?

Done.

https://codereview.chromium.org/14678012/diff/1/content/renderer/render_threa...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/14678012/diff/1/content/renderer/render_threa...
content/renderer/render_thread_impl.cc:746: //     
command_line.HasSwitch(switches::kEnableDeviceMotion));
On 2013/05/16 19:53:06, jamesr wrote:
> huh?

Done.

https://codereview.chromium.org/14678012/diff/1/content/renderer/renderer_web...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/14678012/diff/1/content/renderer/renderer_web...
content/renderer/renderer_webkitplatformsupport_impl.cc:904: if
(!device_motion_event_pump_.get())
On 2013/05/16 19:53:06, jamesr wrote:
> just
> 
> if (!device_motion_event_pump_)
>   ...

Done.

[timvolodine, 2013-05-28 15:31:43]

Hi Darin,
could you please have a look at this chromium-side patch as well ?

thanks,
Tim

[darin (slow to review), 2013-05-29 06:41:53]

Just a first pass code review...

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
File content/renderer/device_orientation/device_motion_event_pump.cc (right):

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_event_pump.cc:48: this,
&DeviceMotionEventPump::fireEvent);
nit: fix indentation

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_event_pump.cc:68:
NOTIMPLEMENTED();
add a comment?  why is DeviceMotionEventPump extending from
WebDeviceMotionListener if it is not going to implement this method?

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
File content/renderer/device_orientation/device_motion_event_pump.h (right):

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_event_pump.h:30: virtual void
setListener(WebKit::WebDeviceMotionListener*) OVERRIDE;
s/setListener/SetListener/?  not an OVERRIDE.  does it need to be virtual?

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_event_pump.h:32: virtual void
didChangeDeviceMotion(
nit: add a comment indicating that this is the implementation of
WebDeviceMotionListener

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_event_pump.h:35: virtual void
setDeviceMotionReader(DeviceMotionSharedMemoryReader*);
why virtual?

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_event_pump.h:41: void
startFetchingDeviceMotion();
nit: use google C++ style for method naming

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
File content/renderer/device_orientation/device_motion_shared_memory_reader.cc
(right):

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_shared_memory_reader.cc:61:
CHECK(RenderThread::Get()->Send(
are you sure you need to CHECK here?

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_shared_memory_reader.cc:62:
new DeviceMotionHostMsg_StartPolling(&renderer_shared_memory_handle_)));
Can you do this using an asynchronous IPC instead?  This is just adding jank to
the main thread for no apparent benefit other than perhaps making this code
simpler.  Instead, it seems like you could pair this StartPolling IPC with a
DeviceMotionMsg_DidStartPolling IPC that gets sent back to you.

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_shared_memory_reader.cc:72:
CHECK(memory);
i have seen cases where it is difficult to map a large segment of shared memory
into the renderer.  maybe that'll come up rarely in this case.

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_shared_memory_reader.cc:75: 
nit: kill gratuitous new line

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_shared_memory_reader.cc:79:
CHECK(RenderThread::Get()->Send(new DeviceMotionHostMsg_StopPolling()));
why do you need to CHECK here?  this could fail during renderer / browser
shutdown.  are you sure you want that to be regarded as a renderer crash?

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
File content/renderer/device_orientation/device_motion_shared_memory_reader.h
(right):

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_shared_memory_reader.h:24:
virtual bool latestDeviceMotionData(WebKit::WebDeviceMotionData& data);
nit: use google C++ style for naming methods

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_shared_memory_reader.h:24:
virtual bool latestDeviceMotionData(WebKit::WebDeviceMotionData& data);
nit add a verb here... like "Query" or "Get"

[timvolodine, 2013-05-29 18:59:06]

Thanks for the initial comments, I appreciate that you are making time for this
:)

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
File content/renderer/device_orientation/device_motion_event_pump.cc (right):

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_event_pump.cc:48: this,
&DeviceMotionEventPump::fireEvent);
On 2013/05/29 06:41:53, darin wrote:
> nit: fix indentation

Done.

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_event_pump.cc:68:
NOTIMPLEMENTED();
On 2013/05/29 06:41:53, darin wrote:
> add a comment?  why is DeviceMotionEventPump extending from
> WebDeviceMotionListener if it is not going to implement this method?

actually the listener interface is no longer relevant here, so removed it.
Done.

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
File content/renderer/device_orientation/device_motion_event_pump.h (right):

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_event_pump.h:30: virtual void
setListener(WebKit::WebDeviceMotionListener*) OVERRIDE;
On 2013/05/29 06:41:53, darin wrote:
> s/setListener/SetListener/?  not an OVERRIDE.  does it need to be virtual?

Done.

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_event_pump.h:32: virtual void
didChangeDeviceMotion(
On 2013/05/29 06:41:53, darin wrote:
> nit: add a comment indicating that this is the implementation of
> WebDeviceMotionListener

Done.

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_event_pump.h:35: virtual void
setDeviceMotionReader(DeviceMotionSharedMemoryReader*);
On 2013/05/29 06:41:53, darin wrote:
> why virtual?

Done.

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_event_pump.h:41: void
startFetchingDeviceMotion();
On 2013/05/29 06:41:53, darin wrote:
> nit: use google C++ style for method naming

Done.

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
File content/renderer/device_orientation/device_motion_shared_memory_reader.cc
(right):

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_shared_memory_reader.cc:61:
CHECK(RenderThread::Get()->Send(
On 2013/05/29 06:41:53, darin wrote:
> are you sure you need to CHECK here?

hmm, I thought the reasoning for this was that the sync call should not fail.
but maybe I should make it more flexible and just return silently if this
happens.

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_shared_memory_reader.cc:62:
new DeviceMotionHostMsg_StartPolling(&renderer_shared_memory_handle_)));
On 2013/05/29 06:41:53, darin wrote:
> Can you do this using an asynchronous IPC instead?  This is just adding jank
to
> the main thread for no apparent benefit other than perhaps making this code
> simpler.  Instead, it seems like you could pair this StartPolling IPC with a
> DeviceMotionMsg_DidStartPolling IPC that gets sent back to you.

I was basing this IPC messaging on how the gamepad is implemented. The idea here
is to obtain a shared memory handle from the browser only (not to wait until
sensors spin up). I did a couple of measurements and it appears the sync call
takes approx 1.5 milliseconds. Is this worth an async call ? If you think it is
I'll add a somewhat redesigned messaging structure, but will need to adapt the
code to passing a renderer process id, because now the messaging is
renderer-agnostic so to speak.

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_shared_memory_reader.cc:72:
CHECK(memory);
On 2013/05/29 06:41:53, darin wrote:
> i have seen cases where it is difficult to map a large segment of shared
memory
> into the renderer.  maybe that'll come up rarely in this case.

I actually borrowed that code from the gamepad implementation..
maybe I should make it DCHECK and add an if to exit silently.

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_shared_memory_reader.cc:75: 
On 2013/05/29 06:41:53, darin wrote:
> nit: kill gratuitous new line

Done.

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_shared_memory_reader.cc:79:
CHECK(RenderThread::Get()->Send(new DeviceMotionHostMsg_StopPolling()));
On 2013/05/29 06:41:53, darin wrote:
> why do you need to CHECK here?  this could fail during renderer / browser
> shutdown.  are you sure you want that to be regarded as a renderer crash?

Done. since the StopPolling message is async now (it used to be sync).

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
File content/renderer/device_orientation/device_motion_shared_memory_reader.h
(right):

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_shared_memory_reader.h:24:
virtual bool latestDeviceMotionData(WebKit::WebDeviceMotionData& data);
On 2013/05/29 06:41:53, darin wrote:
> nit: use google C++ style for naming methods

Done.

https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
content/renderer/device_orientation/device_motion_shared_memory_reader.h:24:
virtual bool latestDeviceMotionData(WebKit::WebDeviceMotionData& data);
On 2013/05/29 06:41:53, darin wrote:
> nit add a verb here... like "Query" or "Get"

Done.

[timvolodine, 2013-05-30 15:34:02]

On 2013/05/29 18:59:06, timvolodine wrote:
> Thanks for the initial comments, I appreciate that you are making time for
this
> :)
> 
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> File content/renderer/device_orientation/device_motion_event_pump.cc (right):
> 
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> content/renderer/device_orientation/device_motion_event_pump.cc:48: this,
> &DeviceMotionEventPump::fireEvent);
> On 2013/05/29 06:41:53, darin wrote:
> > nit: fix indentation
> 
> Done.
> 
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> content/renderer/device_orientation/device_motion_event_pump.cc:68:
> NOTIMPLEMENTED();
> On 2013/05/29 06:41:53, darin wrote:
> > add a comment?  why is DeviceMotionEventPump extending from
> > WebDeviceMotionListener if it is not going to implement this method?
> 
> actually the listener interface is no longer relevant here, so removed it.
> Done.
> 
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> File content/renderer/device_orientation/device_motion_event_pump.h (right):
> 
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> content/renderer/device_orientation/device_motion_event_pump.h:30: virtual
void
> setListener(WebKit::WebDeviceMotionListener*) OVERRIDE;
> On 2013/05/29 06:41:53, darin wrote:
> > s/setListener/SetListener/?  not an OVERRIDE.  does it need to be virtual?
> 
> Done.
> 
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> content/renderer/device_orientation/device_motion_event_pump.h:32: virtual
void
> didChangeDeviceMotion(
> On 2013/05/29 06:41:53, darin wrote:
> > nit: add a comment indicating that this is the implementation of
> > WebDeviceMotionListener
> 
> Done.
> 
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> content/renderer/device_orientation/device_motion_event_pump.h:35: virtual
void
> setDeviceMotionReader(DeviceMotionSharedMemoryReader*);
> On 2013/05/29 06:41:53, darin wrote:
> > why virtual?
> 
> Done.
> 
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> content/renderer/device_orientation/device_motion_event_pump.h:41: void
> startFetchingDeviceMotion();
> On 2013/05/29 06:41:53, darin wrote:
> > nit: use google C++ style for method naming
> 
> Done.
> 
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> File content/renderer/device_orientation/device_motion_shared_memory_reader.cc
> (right):
> 
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> content/renderer/device_orientation/device_motion_shared_memory_reader.cc:62:
> new DeviceMotionHostMsg_StartPolling(&renderer_shared_memory_handle_)));
> On 2013/05/29 06:41:53, darin wrote:
> > Can you do this using an asynchronous IPC instead?  This is just adding jank
> to
> > the main thread for no apparent benefit other than perhaps making this code
> > simpler.  Instead, it seems like you could pair this StartPolling IPC with a
> > DeviceMotionMsg_DidStartPolling IPC that gets sent back to you.
> 
> I was basing this IPC messaging on how the gamepad is implemented. The idea
here
> is to obtain a shared memory handle from the browser only (not to wait until
> sensors spin up). I did a couple of measurements and it appears the sync call
> takes approx 1.5 milliseconds. Is this worth an async call ? If you think it
is
> I'll add a somewhat redesigned messaging structure, but will need to adapt the
> code to passing a renderer process id, because now the messaging is
> renderer-agnostic so to speak.
> 
> 

Regarding the async messaging via the platform: is there a way to get the
RenderViewImpl* from the platform, i.e. renderer_webkitplatformsupport.h? To
receive messages in the renderer I would need to register a RenderViewObserver
(i.e. an event pump) with the RenderView.

I have not found any examples in platform that are actual RenderViewObservers,
they seems to be mostly owned by render_view_impl. In the platform the
Hyphenator for example is a RenderProcessObserver and hence listens to control
messages from the browser.

What is your opinion on this should the DeviceMotionMsg_DidStartPolling messages
be control or routed?

[timvolodine, 2013-05-31 17:31:41]

On 2013/05/30 15:34:02, timvolodine wrote:
> On 2013/05/29 18:59:06, timvolodine wrote:
> > Thanks for the initial comments, I appreciate that you are making time for
> this
> > :)
> > 
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > File content/renderer/device_orientation/device_motion_event_pump.cc
(right):
> > 
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > content/renderer/device_orientation/device_motion_event_pump.cc:48: this,
> > &DeviceMotionEventPump::fireEvent);
> > On 2013/05/29 06:41:53, darin wrote:
> > > nit: fix indentation
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > content/renderer/device_orientation/device_motion_event_pump.cc:68:
> > NOTIMPLEMENTED();
> > On 2013/05/29 06:41:53, darin wrote:
> > > add a comment?  why is DeviceMotionEventPump extending from
> > > WebDeviceMotionListener if it is not going to implement this method?
> > 
> > actually the listener interface is no longer relevant here, so removed it.
> > Done.
> > 
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > File content/renderer/device_orientation/device_motion_event_pump.h (right):
> > 
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > content/renderer/device_orientation/device_motion_event_pump.h:30: virtual
> void
> > setListener(WebKit::WebDeviceMotionListener*) OVERRIDE;
> > On 2013/05/29 06:41:53, darin wrote:
> > > s/setListener/SetListener/?  not an OVERRIDE.  does it need to be virtual?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > content/renderer/device_orientation/device_motion_event_pump.h:32: virtual
> void
> > didChangeDeviceMotion(
> > On 2013/05/29 06:41:53, darin wrote:
> > > nit: add a comment indicating that this is the implementation of
> > > WebDeviceMotionListener
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > content/renderer/device_orientation/device_motion_event_pump.h:35: virtual
> void
> > setDeviceMotionReader(DeviceMotionSharedMemoryReader*);
> > On 2013/05/29 06:41:53, darin wrote:
> > > why virtual?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > content/renderer/device_orientation/device_motion_event_pump.h:41: void
> > startFetchingDeviceMotion();
> > On 2013/05/29 06:41:53, darin wrote:
> > > nit: use google C++ style for method naming
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > File
content/renderer/device_orientation/device_motion_shared_memory_reader.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> >
content/renderer/device_orientation/device_motion_shared_memory_reader.cc:62:
> > new DeviceMotionHostMsg_StartPolling(&renderer_shared_memory_handle_)));
> > On 2013/05/29 06:41:53, darin wrote:
> > > Can you do this using an asynchronous IPC instead?  This is just adding
jank
> > to
> > > the main thread for no apparent benefit other than perhaps making this
code
> > > simpler.  Instead, it seems like you could pair this StartPolling IPC with
a
> > > DeviceMotionMsg_DidStartPolling IPC that gets sent back to you.
> > 
> > I was basing this IPC messaging on how the gamepad is implemented. The idea
> here
> > is to obtain a shared memory handle from the browser only (not to wait until
> > sensors spin up). I did a couple of measurements and it appears the sync
call
> > takes approx 1.5 milliseconds. Is this worth an async call ? If you think it
> is
> > I'll add a somewhat redesigned messaging structure, but will need to adapt
the
> > code to passing a renderer process id, because now the messaging is
> > renderer-agnostic so to speak.
> > 
> > 
> 
> Regarding the async messaging via the platform: is there a way to get the
> RenderViewImpl* from the platform, i.e. renderer_webkitplatformsupport.h? To
> receive messages in the renderer I would need to register a RenderViewObserver
> (i.e. an event pump) with the RenderView.
> 
> I have not found any examples in platform that are actual RenderViewObservers,
> they seems to be mostly owned by render_view_impl. In the platform the
> Hyphenator for example is a RenderProcessObserver and hence listens to control
> messages from the browser.
> 
> What is your opinion on this should the DeviceMotionMsg_DidStartPolling
messages
> be control or routed?

Hi Darin,

Can we go with the 'gamepad-equivalent' sync approach for now? I'll add the
async messaging later. That way it would also be easier to work in smaller
increments.

thanks,
Tim

[timvolodine, 2013-06-03 18:59:46]

On 2013/05/31 17:31:41, timvolodine wrote:
> On 2013/05/30 15:34:02, timvolodine wrote:
> > On 2013/05/29 18:59:06, timvolodine wrote:
> > > Thanks for the initial comments, I appreciate that you are making time for
> > this
> > > :)
> > > 
> > >
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > > File content/renderer/device_orientation/device_motion_event_pump.cc
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > > content/renderer/device_orientation/device_motion_event_pump.cc:48: this,
> > > &DeviceMotionEventPump::fireEvent);
> > > On 2013/05/29 06:41:53, darin wrote:
> > > > nit: fix indentation
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > > content/renderer/device_orientation/device_motion_event_pump.cc:68:
> > > NOTIMPLEMENTED();
> > > On 2013/05/29 06:41:53, darin wrote:
> > > > add a comment?  why is DeviceMotionEventPump extending from
> > > > WebDeviceMotionListener if it is not going to implement this method?
> > > 
> > > actually the listener interface is no longer relevant here, so removed it.
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > > File content/renderer/device_orientation/device_motion_event_pump.h
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > > content/renderer/device_orientation/device_motion_event_pump.h:30: virtual
> > void
> > > setListener(WebKit::WebDeviceMotionListener*) OVERRIDE;
> > > On 2013/05/29 06:41:53, darin wrote:
> > > > s/setListener/SetListener/?  not an OVERRIDE.  does it need to be
virtual?
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > > content/renderer/device_orientation/device_motion_event_pump.h:32: virtual
> > void
> > > didChangeDeviceMotion(
> > > On 2013/05/29 06:41:53, darin wrote:
> > > > nit: add a comment indicating that this is the implementation of
> > > > WebDeviceMotionListener
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > > content/renderer/device_orientation/device_motion_event_pump.h:35: virtual
> > void
> > > setDeviceMotionReader(DeviceMotionSharedMemoryReader*);
> > > On 2013/05/29 06:41:53, darin wrote:
> > > > why virtual?
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > > content/renderer/device_orientation/device_motion_event_pump.h:41: void
> > > startFetchingDeviceMotion();
> > > On 2013/05/29 06:41:53, darin wrote:
> > > > nit: use google C++ style for method naming
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > > File
> content/renderer/device_orientation/device_motion_shared_memory_reader.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/14678012/diff/16001/content/renderer/device_o...
> > >
> content/renderer/device_orientation/device_motion_shared_memory_reader.cc:62:
> > > new DeviceMotionHostMsg_StartPolling(&renderer_shared_memory_handle_)));
> > > On 2013/05/29 06:41:53, darin wrote:
> > > > Can you do this using an asynchronous IPC instead?  This is just adding
> jank
> > > to
> > > > the main thread for no apparent benefit other than perhaps making this
> code
> > > > simpler.  Instead, it seems like you could pair this StartPolling IPC
with
> a
> > > > DeviceMotionMsg_DidStartPolling IPC that gets sent back to you.
> > > 
> > > I was basing this IPC messaging on how the gamepad is implemented. The
idea
> > here
> > > is to obtain a shared memory handle from the browser only (not to wait
until
> > > sensors spin up). I did a couple of measurements and it appears the sync
> call
> > > takes approx 1.5 milliseconds. Is this worth an async call ? If you think
it
> > is
> > > I'll add a somewhat redesigned messaging structure, but will need to adapt
> the
> > > code to passing a renderer process id, because now the messaging is
> > > renderer-agnostic so to speak.
> > > 
> > > 
> > 
> > Regarding the async messaging via the platform: is there a way to get the
> > RenderViewImpl* from the platform, i.e. renderer_webkitplatformsupport.h? To
> > receive messages in the renderer I would need to register a
RenderViewObserver
> > (i.e. an event pump) with the RenderView.
> > 
> > I have not found any examples in platform that are actual
RenderViewObservers,
> > they seems to be mostly owned by render_view_impl. In the platform the
> > Hyphenator for example is a RenderProcessObserver and hence listens to
control
> > messages from the browser.
> > 
> > What is your opinion on this should the DeviceMotionMsg_DidStartPolling
> messages
> > be control or routed?
> 
> Hi Darin,
> 
> Can we go with the 'gamepad-equivalent' sync approach for now? I'll add the
> async messaging later. That way it would also be easier to work in smaller
> increments.
> 
> thanks,
> Tim

Again wrt async messaging:
Upon further inspection it seems logical to do an approach similar to Hyphenator
using *control* messages to signal DeviceMotionMsg_DidStartPolling. A Platform
is associated with a RenderThread and hence there should be only one
DeviceMotionEventPump per RenderProcess, no RenderViews need to be involved.
Correct me if my understanding is wrong here.

[timvolodine, 2013-06-12 18:12:14]

uploaded a 'complete' solution with an android fetcher.

(contains some debugging LOG statements which will be removed)

[timvolodine, 2013-06-13 20:09:23]

added async messaging for obtaining a shared memory handle from the browser.
added tri-state status to device_motion_event_pump.

[timvolodine, 2013-06-14 19:29:59]

friendly ping :)

[jamesr, 2013-06-14 19:30:59]

On 2013/06/14 19:29:59, timvolodine wrote:
> friendly ping :)

Could you specify who you are pinging? There are 4 reviewers on the reviewers
list.

[timvolodine, 2013-06-14 21:00:12]

On 2013/06/14 19:30:59, jamesr wrote:
> On 2013/06/14 19:29:59, timvolodine wrote:
> > friendly ping :)
> 
> Could you specify who you are pinging? There are 4 reviewers on the reviewers
> list.

yes sorry. I actually meant to ping Darin, he said he would have a look..

[darin (slow to review), 2013-06-17 05:27:46]

Sorry for the delay in getting this feedback out.

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
File content/renderer/device_orientation/device_motion_event_pump.cc (right):

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_event_pump.cc:39:
reader_.reset(reader);
since this is a transfer of ownership, the argument type should be
scoped_ptr<DeviceMotionSharedMemoryReader>, and the callsite should use the
.Pass() method of scoped_ptr<T>.

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_event_pump.cc:44: LOG(INFO) <<
"PUMP start fetching";
nit: are both of these log statements necessary?

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_event_pump.cc:60: LOG(INFO) <<
"PUMP stop fetching";
ditto: can you do with only one log statement?  do you need these log statements
compiled into release builds?

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_event_pump.cc:65:
DCHECK((state_ == PENDING_START && !timer_.IsRunning())
nit: chromium style is to put the "||" operator at the end of the previous line

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_event_pump.cc:77: LOG(INFO) <<
"PUMP fire";
nit: it seems like there could be a lot of these log statements in the log file.
 is this really necessary in release builds?

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_event_pump.cc:99:
SetDeviceMotionReader(new DeviceMotionSharedMemoryReader);
nit: the code might be clearer if you just directly call reader_.reset(new
DeviceMotionSharedMemoryReader) here.  since you are null testing reader_ here,
you might as well also assign to it directly.

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
File content/renderer/device_orientation/device_motion_shared_memory_reader.cc
(right):

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_shared_memory_reader.cc:23: //
Only try to read this many times before failing to avoid waiting here
it looks like you mostly just copy-pasted code from here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/g...

instead, it seems like you should refactor this code so that it can be shared.

we should probably come up with a descriptive name for this way of using
shared memory, and then turn that into a class, which both DeviceMotion
and Gamepad code can share.

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_shared_memory_reader.cc:40:
LOG(INFO) << "PUMP CONTENTION = " << contention_count;
nit: indentation... also, is this really needed in release builds?

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_shared_memory_reader.cc:68: if
(void *memory = renderer_shared_memory_->memory()) {
nit: indentation

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
File content/renderer/device_orientation/device_motion_shared_memory_reader.h
(right):

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_shared_memory_reader.h:24:
virtual bool GetLatestDeviceMotionData(WebKit::WebDeviceMotionData& data);
nit: google c++ style disallows non-const reference arguments.  data should be
passed by pointer instead.

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/r...
File content/renderer/render_thread_impl.cc (right):

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/r...
content/renderer/render_thread_impl.cc:723: //  if
(command_line.HasSwitch(switches::kEnableDeviceMotion))
nit: unintended change?

[timvolodine, 2013-06-17 20:10:55]

thanks for the comments!

refactored the reader using templates to be more generic, see
shared_memory_seqlock_{reader,buffer}. Think this is an appropriate name since
it's a non-blocking shared memory reader using a seq-lock.

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
File content/renderer/device_orientation/device_motion_event_pump.cc (right):

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_event_pump.cc:39:
reader_.reset(reader);
On 2013/06/17 05:27:46, darin wrote:
> since this is a transfer of ownership, the argument type should be
> scoped_ptr<DeviceMotionSharedMemoryReader>, and the callsite should use the
> .Pass() method of scoped_ptr<T>.

Done.

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_event_pump.cc:44: LOG(INFO) <<
"PUMP start fetching";
On 2013/06/17 05:27:46, darin wrote:
> nit: are both of these log statements necessary?

sorry this was for debugging, removed all extra logging, also below.

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_event_pump.cc:60: LOG(INFO) <<
"PUMP stop fetching";
On 2013/06/17 05:27:46, darin wrote:
> ditto: can you do with only one log statement?  do you need these log
statements
> compiled into release builds?

Done.

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_event_pump.cc:65:
DCHECK((state_ == PENDING_START && !timer_.IsRunning())
On 2013/06/17 05:27:46, darin wrote:
> nit: chromium style is to put the "||" operator at the end of the previous
line

Done.

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_event_pump.cc:77: LOG(INFO) <<
"PUMP fire";
On 2013/06/17 05:27:46, darin wrote:
> nit: it seems like there could be a lot of these log statements in the log
file.
>  is this really necessary in release builds?

Done.

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_event_pump.cc:99:
SetDeviceMotionReader(new DeviceMotionSharedMemoryReader);
On 2013/06/17 05:27:46, darin wrote:
> nit: the code might be clearer if you just directly call reader_.reset(new
> DeviceMotionSharedMemoryReader) here.  since you are null testing reader_
here,
> you might as well also assign to it directly.

Done.

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
File content/renderer/device_orientation/device_motion_shared_memory_reader.cc
(right):

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_shared_memory_reader.cc:23: //
Only try to read this many times before failing to avoid waiting here
On 2013/06/17 05:27:46, darin wrote:
> it looks like you mostly just copy-pasted code from here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/g...
> 
> instead, it seems like you should refactor this code so that it can be shared.
> 
> we should probably come up with a descriptive name for this way of using
> shared memory, and then turn that into a class, which both DeviceMotion
> and Gamepad code can share.

yes, here the code is mostly the same as for gamepad. I've templated this class
and called it shared_memory_seqlock_reader. Also note the the actual buffer is
now also templated (and the generic version is called
shared_memory_seqlock_buffer). That way the generic classes can be reused by
Gamepad as well.

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_shared_memory_reader.cc:40:
LOG(INFO) << "PUMP CONTENTION = " << contention_count;
On 2013/06/17 05:27:46, darin wrote:
> nit: indentation... also, is this really needed in release builds?

Done.

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_shared_memory_reader.cc:68: if
(void *memory = renderer_shared_memory_->memory()) {
On 2013/06/17 05:27:46, darin wrote:
> nit: indentation

Done.

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
File content/renderer/device_orientation/device_motion_shared_memory_reader.h
(right):

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/d...
content/renderer/device_orientation/device_motion_shared_memory_reader.h:24:
virtual bool GetLatestDeviceMotionData(WebKit::WebDeviceMotionData& data);
On 2013/06/17 05:27:46, darin wrote:
> nit: google c++ style disallows non-const reference arguments.  data should be
> passed by pointer instead.

Done.

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/r...
File content/renderer/render_thread_impl.cc (right):

https://chromiumcodereview.appspot.com/14678012/diff/40001/content/renderer/r...
content/renderer/render_thread_impl.cc:723: //  if
(command_line.HasSwitch(switches::kEnableDeviceMotion))
On 2013/06/17 05:27:46, darin wrote:
> nit: unintended change?

Done.

[darin (slow to review), 2013-06-17 20:23:44]

Some quick comments focused on the seqlock code.

https://chromiumcodereview.appspot.com/14678012/diff/51001/content/common/sha...
File content/common/shared_memory_seqlock_buffer.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/51001/content/common/sha...
content/common/shared_memory_seqlock_buffer.h:23: GamepadSeqLock sequence;
nit: seems like GamepadSeqLock needs to be renamed, right?

https://chromiumcodereview.appspot.com/14678012/diff/51001/content/common/sha...
content/common/shared_memory_seqlock_buffer.h:24: Data buffer;
nit: maybe "buffer" should be named "data" to match the name of the data type? 
plus, that way you can say that the buffer's data is given by the "data" member
variable.  otherwise, you have the buffer's data is given by the "buffer" member
variable, and you end up with code like "buffer->buffer" which tends to be a bit
awkward.

https://chromiumcodereview.appspot.com/14678012/diff/51001/content/renderer/d...
File content/renderer/device_orientation/shared_memory_seqlock_reader.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/51001/content/renderer/d...
content/renderer/device_orientation/shared_memory_seqlock_reader.h:6: #define
CONTENT_RENDERER_SHARED_MEMORY_SEQLOCK_READER_H_
I'm confused to see this file in the device_orientation directory since you are
trying to make it generic.

https://chromiumcodereview.appspot.com/14678012/diff/51001/content/renderer/d...
content/renderer/device_orientation/shared_memory_seqlock_reader.h:20: virtual
~SharedMemorySeqLockReader() { }
why the virtual functions?

https://chromiumcodereview.appspot.com/14678012/diff/51001/content/renderer/d...
content/renderer/device_orientation/shared_memory_seqlock_reader.h:22: virtual
bool GetLatestDeviceMotionData(Data* data) {
nit: maybe this function should be named GetLatestData (or GetSnapshotOfData)?

https://chromiumcodereview.appspot.com/14678012/diff/51001/content/renderer/d...
content/renderer/device_orientation/shared_memory_seqlock_reader.h:25: if
(!base::SharedMemory::IsHandleValid(renderer_shared_memory_handle_))
It seems like the guts of this function does not need to be a template.  The
guts of this function just need to know sizeof(Data) and the address of
read_into.

I recommend creating a non-template helper class, that you can instantiate as a
member variable, for the guts so that the build system doesn't have to duplicate
this code in the resulting Chrome binary.  I would put everything I can into
that class, and then just have this template class delegate to it as
appropriate.

https://chromiumcodereview.appspot.com/14678012/diff/51001/content/renderer/d...
content/renderer/device_orientation/shared_memory_seqlock_reader.h:75: };
nit: probably want to add DISALLOW_COPY_AND_ASSIGN here

[timvolodine, 2013-06-18 16:44:31]

refactoring out the templates in shared_memory_seqlock_reader seems somewhat
challenging, I've uploaded some new code for that, please also see comments
below.

https://chromiumcodereview.appspot.com/14678012/diff/51001/content/common/sha...
File content/common/shared_memory_seqlock_buffer.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/51001/content/common/sha...
content/common/shared_memory_seqlock_buffer.h:23: GamepadSeqLock sequence;
On 2013/06/17 20:23:44, darin wrote:
> nit: seems like GamepadSeqLock needs to be renamed, right?

Done.

renamed to OneWriterSeqLock.

https://chromiumcodereview.appspot.com/14678012/diff/51001/content/common/sha...
content/common/shared_memory_seqlock_buffer.h:24: Data buffer;
On 2013/06/17 20:23:44, darin wrote:
> nit: maybe "buffer" should be named "data" to match the name of the data type?

> plus, that way you can say that the buffer's data is given by the "data"
member
> variable.  otherwise, you have the buffer's data is given by the "buffer"
member
> variable, and you end up with code like "buffer->buffer" which tends to be a
bit
> awkward.

absolutely agree, Done.

https://chromiumcodereview.appspot.com/14678012/diff/51001/content/renderer/d...
File content/renderer/device_orientation/shared_memory_seqlock_reader.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/51001/content/renderer/d...
content/renderer/device_orientation/shared_memory_seqlock_reader.h:6: #define
CONTENT_RENDERER_SHARED_MEMORY_SEQLOCK_READER_H_
On 2013/06/17 20:23:44, darin wrote:
> I'm confused to see this file in the device_orientation directory since you
are
> trying to make it generic.

Done.

https://chromiumcodereview.appspot.com/14678012/diff/51001/content/renderer/d...
content/renderer/device_orientation/shared_memory_seqlock_reader.h:20: virtual
~SharedMemorySeqLockReader() { }
On 2013/06/17 20:23:44, darin wrote:
> why the virtual functions?

in the refactored version there is the base class to avoid template replication.
The destructor is therefore virtual.

https://chromiumcodereview.appspot.com/14678012/diff/51001/content/renderer/d...
content/renderer/device_orientation/shared_memory_seqlock_reader.h:22: virtual
bool GetLatestDeviceMotionData(Data* data) {
On 2013/06/17 20:23:44, darin wrote:
> nit: maybe this function should be named GetLatestData (or GetSnapshotOfData)?

Done.

https://chromiumcodereview.appspot.com/14678012/diff/51001/content/renderer/d...
content/renderer/device_orientation/shared_memory_seqlock_reader.h:25: if
(!base::SharedMemory::IsHandleValid(renderer_shared_memory_handle_))
On 2013/06/17 20:23:44, darin wrote:
> It seems like the guts of this function does not need to be a template.  The
> guts of this function just need to know sizeof(Data) and the address of
> read_into.
> 
> I recommend creating a non-template helper class, that you can instantiate as
a
> member variable, for the guts so that the build system doesn't have to
duplicate
> this code in the resulting Chrome binary.  I would put everything I can into
> that class, and then just have this template class delegate to it as
> appropriate.

Actually the guts also needs the buffer variable because it uses its lock to
read the data. The buffer itself is also templated so there is a problem here.
I've tried putting as much as I could in a base non-templated ancestor class to
avoid templated code duplication, maybe I am missing something but it looks like
it's not worth the effort to define that parent class. I've uploaded the code
anyway for you to have a look.

https://chromiumcodereview.appspot.com/14678012/diff/51001/content/renderer/d...
content/renderer/device_orientation/shared_memory_seqlock_reader.h:75: };
On 2013/06/17 20:23:44, darin wrote:
> nit: probably want to add DISALLOW_COPY_AND_ASSIGN here

Done.

[timvolodine, 2013-06-19 18:28:51]

Did some more thinking about this and eventually factored out most of the
templated reader class code (i.e. shared_memory_seqlock_reader) :).

For this to work I had to introduce a base class for the
content/common/shared_memory_seqlock_buffer.h.

[timvolodine, 2013-06-24 22:38:42]

Hi Scott,

we ended up touching the gamepad implementation slightly in this patch, could
you please have a look at the relevant pieces to see if they are ok.

thanks,
Tim

[scottmg, 2013-06-24 23:24:38]

the lock renaming seems a mechanical change so that seems fine, but i think the
reader side could be either improved to be shared, or alternatively
de-genericized to be device motion specific.

(also... i guess this was covered long ago, but it looks like the data is only
82 bytes
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
maybe just regular ipc would be sufficient?)

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/sha...
File content/common/shared_memory_seqlock_buffer.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/sha...
content/common/shared_memory_seqlock_buffer.h:13: // which does the hardware
polling, and the consumers of the device motion
being that this is currently device motion specific, it has a pretty generic
name. or will the gamepad code be refactored to use it also?

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/sha...
content/common/shared_memory_seqlock_buffer.h:27: bool is_ready_for_read;
(unrelated to looking at gamepad changes, but) this field sort of scares me.
should the "data is valid" field not be a member of the templated contained
Data?

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/renderer/s...
File content/renderer/shared_memory_seqlock_reader.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/renderer/s...
content/renderer/shared_memory_seqlock_reader.h:15: class
SharedMemorySeqLockReader : private SharedMemorySeqLockReaderBase {
was the intention that this be the generic reader? it seems you need to rewrite
renderer/gamepad_shared_memory_reader in that case, otherwise probably name it
something more device motion specific.

[darin (slow to review), 2013-06-24 23:40:34]

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/browser/re...
File content/browser/renderer_host/device_motion_browser_message_filter.cc
(right):

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/browser/re...
content/browser/renderer_host/device_motion_browser_message_filter.cc:39: if
(!is_started_) {
nit: Consider returning early when is_started_ is already true.  That way the
body of the function doesn't have to be indented.  This is just a general style
suggestion (not required).

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/browser/re...
content/browser/renderer_host/device_motion_browser_message_filter.cc:45:
NOTREACHED();
Note that in release builds, this NOTREACHED will be silent.  There will be no
record of spurious DeviceMotionHostMsg_{Start,Stop}Polling messages as seen in
the field.  That seems mostly OK, but it could also be reasonable to crash the
renderer when it seems to give bad messages.

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/one...
File content/common/one_writer_seqlock.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/one...
content/common/one_writer_seqlock.h:31: class CONTENT_EXPORT OneWriterSeqLock {
nit: Since you didn't freshly write these files, consider using a lower
--similiarity option with "git cl upload".  That way history will be preserved.

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/sha...
File content/common/shared_memory_seqlock_buffer.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/sha...
content/common/shared_memory_seqlock_buffer.h:31: class
SharedMemorySeqLockBuffer : public SharedMemorySeqLockBufferBase {
nit: Since there are no methods here, it seems like there isn't much of a
benefit to factoring out a non-template *Base class here.

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/renderer/s...
File content/renderer/shared_memory_seqlock_reader.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/renderer/s...
content/renderer/shared_memory_seqlock_reader.h:11: 
nit: it is common to put the *Base class here inside an internal:: namespace.

[timvolodine, 2013-06-25 03:36:06]

fixed comments + removed data fetcher related code to make this CL shorter and
easier to review.

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/browser/re...
File content/browser/renderer_host/device_motion_browser_message_filter.cc
(right):

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/browser/re...
content/browser/renderer_host/device_motion_browser_message_filter.cc:39: if
(!is_started_) {
On 2013/06/24 23:40:34, darin wrote:
> nit: Consider returning early when is_started_ is already true.  That way the
> body of the function doesn't have to be indented.  This is just a general
style
> suggestion (not required).

Done.

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/browser/re...
content/browser/renderer_host/device_motion_browser_message_filter.cc:45:
NOTREACHED();
On 2013/06/24 23:40:34, darin wrote:
> Note that in release builds, this NOTREACHED will be silent.  There will be no
> record of spurious DeviceMotionHostMsg_{Start,Stop}Polling messages as seen in
> the field.  That seems mostly OK, but it could also be reasonable to crash the
> renderer when it seems to give bad messages.

Ack.
Added a DCHECK to catch those messages in debug.

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/one...
File content/common/one_writer_seqlock.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/one...
content/common/one_writer_seqlock.h:31: class CONTENT_EXPORT OneWriterSeqLock {
On 2013/06/24 23:40:34, darin wrote:
> nit: Since you didn't freshly write these files, consider using a lower
> --similiarity option with "git cl upload".  That way history will be
preserved.

Done.

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/sha...
File content/common/shared_memory_seqlock_buffer.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/sha...
content/common/shared_memory_seqlock_buffer.h:13: // which does the hardware
polling, and the consumers of the device motion
On 2013/06/24 23:24:38, scottmg wrote:
> being that this is currently device motion specific, it has a pretty generic
> name. or will the gamepad code be refactored to use it also?

Yes, the idea is to use this for the gamepad code as well in a separate patch
though.
I have modified the comments appropriately.

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/sha...
content/common/shared_memory_seqlock_buffer.h:27: bool is_ready_for_read;
On 2013/06/24 23:24:38, scottmg wrote:
> (unrelated to looking at gamepad changes, but) this field sort of scares me.
> should the "data is valid" field not be a member of the templated contained
> Data?

ok, I've moved this field to WebDeviceMotionData, since it is actually Device
Motion specific.
It seems reasonable to assume that the generalized buffer should only contain a
lock and data. That way it will also be applicable to gamepad.

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/sha...
content/common/shared_memory_seqlock_buffer.h:31: class
SharedMemorySeqLockBuffer : public SharedMemorySeqLockBufferBase {
On 2013/06/24 23:40:34, darin wrote:
> nit: Since there are no methods here, it seems like there isn't much of a
> benefit to factoring out a non-template *Base class here.

The base buffer class is used by the shared_memory_seqlock_reader_base which is
a non-templated class, hence the need for a non-templated buffer class.

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/renderer/s...
File content/renderer/shared_memory_seqlock_reader.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/renderer/s...
content/renderer/shared_memory_seqlock_reader.h:11: 
On 2013/06/24 23:40:34, darin wrote:
> nit: it is common to put the *Base class here inside an internal:: namespace.

Done.

https://chromiumcodereview.appspot.com/14678012/diff/61001/content/renderer/s...
content/renderer/shared_memory_seqlock_reader.h:15: class
SharedMemorySeqLockReader : private SharedMemorySeqLockReaderBase {
On 2013/06/24 23:24:38, scottmg wrote:
> was the intention that this be the generic reader? it seems you need to
rewrite
> renderer/gamepad_shared_memory_reader in that case, otherwise probably name it
> something more device motion specific.

The new reader class largely implements the original gamepad reader code, except
for the sync IPC and some gamepad specifics like gestures. I think it should not
be too hard to refactor gamepad reader using this more generic reader class by
replacing the common code with calls to this implementation,

[timvolodine, 2013-06-26 01:00:11]

On 2013/06/25 03:36:06, timvolodine wrote:
> fixed comments + removed data fetcher related code to make this CL shorter and
> easier to review.
> 
>
https://chromiumcodereview.appspot.com/14678012/diff/61001/content/browser/re...
> File content/browser/renderer_host/device_motion_browser_message_filter.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/14678012/diff/61001/content/browser/re...
> content/browser/renderer_host/device_motion_browser_message_filter.cc:39: if
> (!is_started_) {
> On 2013/06/24 23:40:34, darin wrote:
> > nit: Consider returning early when is_started_ is already true.  That way
the
> > body of the function doesn't have to be indented.  This is just a general
> style
> > suggestion (not required).
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/14678012/diff/61001/content/browser/re...
> content/browser/renderer_host/device_motion_browser_message_filter.cc:45:
> NOTREACHED();
> On 2013/06/24 23:40:34, darin wrote:
> > Note that in release builds, this NOTREACHED will be silent.  There will be
no
> > record of spurious DeviceMotionHostMsg_{Start,Stop}Polling messages as seen
in
> > the field.  That seems mostly OK, but it could also be reasonable to crash
the
> > renderer when it seems to give bad messages.
> 
> Ack.
> Added a DCHECK to catch those messages in debug.
> 
>
https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/one...
> File content/common/one_writer_seqlock.h (right):
> 
>
https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/one...
> content/common/one_writer_seqlock.h:31: class CONTENT_EXPORT OneWriterSeqLock
{
> On 2013/06/24 23:40:34, darin wrote:
> > nit: Since you didn't freshly write these files, consider using a lower
> > --similiarity option with "git cl upload".  That way history will be
> preserved.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/sha...
> File content/common/shared_memory_seqlock_buffer.h (right):
> 
>
https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/sha...
> content/common/shared_memory_seqlock_buffer.h:13: // which does the hardware
> polling, and the consumers of the device motion
> On 2013/06/24 23:24:38, scottmg wrote:
> > being that this is currently device motion specific, it has a pretty generic
> > name. or will the gamepad code be refactored to use it also?
> 
> Yes, the idea is to use this for the gamepad code as well in a separate patch
> though.
> I have modified the comments appropriately.
> 
>
https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/sha...
> content/common/shared_memory_seqlock_buffer.h:27: bool is_ready_for_read;
> On 2013/06/24 23:24:38, scottmg wrote:
> > (unrelated to looking at gamepad changes, but) this field sort of scares me.
> > should the "data is valid" field not be a member of the templated contained
> > Data?
> 
> ok, I've moved this field to WebDeviceMotionData, since it is actually Device
> Motion specific.
> It seems reasonable to assume that the generalized buffer should only contain
a
> lock and data. That way it will also be applicable to gamepad.
> 
>
https://chromiumcodereview.appspot.com/14678012/diff/61001/content/common/sha...
> content/common/shared_memory_seqlock_buffer.h:31: class
> SharedMemorySeqLockBuffer : public SharedMemorySeqLockBufferBase {
> On 2013/06/24 23:40:34, darin wrote:
> > nit: Since there are no methods here, it seems like there isn't much of a
> > benefit to factoring out a non-template *Base class here.
> 
> The base buffer class is used by the shared_memory_seqlock_reader_base which
is
> a non-templated class, hence the need for a non-templated buffer class.
> 
>
https://chromiumcodereview.appspot.com/14678012/diff/61001/content/renderer/s...
> File content/renderer/shared_memory_seqlock_reader.h (right):
> 
>
https://chromiumcodereview.appspot.com/14678012/diff/61001/content/renderer/s...
> content/renderer/shared_memory_seqlock_reader.h:11: 
> On 2013/06/24 23:40:34, darin wrote:
> > nit: it is common to put the *Base class here inside an internal::
namespace.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/14678012/diff/61001/content/renderer/s...
> content/renderer/shared_memory_seqlock_reader.h:15: class
> SharedMemorySeqLockReader : private SharedMemorySeqLockReaderBase {
> On 2013/06/24 23:24:38, scottmg wrote:
> > was the intention that this be the generic reader? it seems you need to
> rewrite
> > renderer/gamepad_shared_memory_reader in that case, otherwise probably name
it
> > something more device motion specific.
> 
> The new reader class largely implements the original gamepad reader code,
except
> for the sync IPC and some gamepad specifics like gestures. I think it should
not
> be too hard to refactor gamepad reader using this more generic reader class by
> replacing the common code with calls to this implementation,

Hi Darin, could we have another iteration || lgtm on this patch please ?

[darin (slow to review), 2013-06-27 17:33:25]

https://chromiumcodereview.appspot.com/14678012/diff/73001/content/browser/de...
File content/browser/device_orientation/device_motion_service.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/73001/content/browser/de...
content/browser/device_orientation/device_motion_service.h:47: void Terminate();
I'm trying to understand this method.  It looks like the idea is that you could
have cases where you have existing consumers but you want to interrupt or stop
the background thread.  Is that right?  Can you help me understand when this
gets used?

https://chromiumcodereview.appspot.com/14678012/diff/73001/content/common/gam...
File content/common/gamepad_hardware_buffer.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/73001/content/common/gam...
content/common/gamepad_hardware_buffer.h:26: struct GamepadHardwareBuffer {
should there be a TODO about making this utilize SharedMemorySeqLockBuffer?

https://chromiumcodereview.appspot.com/14678012/diff/73001/content/common/sha...
File content/common/shared_memory_seqlock_buffer.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/73001/content/common/sha...
content/common/shared_memory_seqlock_buffer.h:21: class
SharedMemorySeqLockBufferBase {
Why do we need to carve out a Base class in this case?  Since there aren't any
methods here, it seems reasonable to just give SharedMemorySeqLockBuffer<T> a
OneWriterSeqLock member.

It seems like the Gamepad buffer class could be trivially changed to be a
specialization of SharedMemorySeqLockBuffer<T>.  Why not just do that as part of
this CL?

https://chromiumcodereview.appspot.com/14678012/diff/73001/content/renderer/s...
File content/renderer/shared_memory_seqlock_reader.cc (right):

https://chromiumcodereview.appspot.com/14678012/diff/73001/content/renderer/s...
content/renderer/shared_memory_seqlock_reader.cc:25: bool
SharedMemorySeqLockReaderBase::FetchFromBuffer(
Is there a path to changing GamepadSharedMemoryReader to use
SharedMemorySeqLockReader<T> too?  Are you saving that for a separate CL?

[timvolodine, 2013-06-27 21:08:43]

https://chromiumcodereview.appspot.com/14678012/diff/73001/content/browser/de...
File content/browser/device_orientation/device_motion_service.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/73001/content/browser/de...
content/browser/device_orientation/device_motion_service.h:47: void Terminate();
On 2013/06/27 17:33:26, darin wrote:
> I'm trying to understand this method.  It looks like the idea is that you
could
> have cases where you have existing consumers but you want to interrupt or stop
> the background thread.  Is that right?  Can you help me understand when this
> gets used?

This method comes from a similar class for gamepad. It is called from
BrowserMainLoop::ShutdownThreadsAndCleanUp. I've added a Terminate() call to the
DeviceMotionService there. It is my understanding that the idea here is to
cleanly terminate all running threads and sensors when the Browser process shuts
down and there are still some existing consumers. In the current implementation
for Android there is no polling thread because Android is push-based and the
fetcher writes to the buffer directly without any need for browser-side polling.
However it is still beneficial to terminate the fetcher (and the sensors) in a
predictable fashion just after the I/O browser thread shuts down. Otherwise it
is not exactly clear when the destructor for fetcher will be called since
DeviceMotionService is a singleton.

https://chromiumcodereview.appspot.com/14678012/diff/73001/content/common/gam...
File content/common/gamepad_hardware_buffer.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/73001/content/common/gam...
content/common/gamepad_hardware_buffer.h:26: struct GamepadHardwareBuffer {
On 2013/06/27 17:33:26, darin wrote:
> should there be a TODO about making this utilize SharedMemorySeqLockBuffer?

Done.

https://chromiumcodereview.appspot.com/14678012/diff/73001/content/common/sha...
File content/common/shared_memory_seqlock_buffer.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/73001/content/common/sha...
content/common/shared_memory_seqlock_buffer.h:21: class
SharedMemorySeqLockBufferBase {
On 2013/06/27 17:33:26, darin wrote:
> Why do we need to carve out a Base class in this case?  Since there aren't any
> methods here, it seems reasonable to just give SharedMemorySeqLockBuffer<T> a
> OneWriterSeqLock member.
> 
> It seems like the Gamepad buffer class could be trivially changed to be a
> specialization of SharedMemorySeqLockBuffer<T>.  Why not just do that as part
of
> this CL?

ok, I've removed this base class and added an extra lock-parameter to
SharedMemorySeqLockReaderBase::FetchFromBuffer to keep it generic.

Regarding Gamepad I think this is best done in a separate CL. Gamepad buffer
uses "buffer" as a name for "data", so some renaming will need to happen
touching multiple files (thinking about fetchers and test files), which will
'pollute' this CL.

https://chromiumcodereview.appspot.com/14678012/diff/73001/content/renderer/s...
File content/renderer/shared_memory_seqlock_reader.cc (right):

https://chromiumcodereview.appspot.com/14678012/diff/73001/content/renderer/s...
content/renderer/shared_memory_seqlock_reader.cc:25: bool
SharedMemorySeqLockReaderBase::FetchFromBuffer(
On 2013/06/27 17:33:26, darin wrote:
> Is there a path to changing GamepadSharedMemoryReader to use
> SharedMemorySeqLockReader<T> too?  Are you saving that for a separate CL?

Yes, I think this is best done in a separate CL. If the refactoring happens to
break Gamepad it would also be easier to revert.

As to the refactoring of the gamepad, the idea is to call
SharedMemorySeqLockReader from GamepadSharedMemoryReader at appropriate places.
The GamepadSharedMemoryReader itself will probably still need to exist because
there is some gamepad-specific code like for handling gestures..

[darin (slow to review), 2013-06-27 21:43:33]

On 2013/06/27 21:08:43, timvolodine wrote:
>
https://chromiumcodereview.appspot.com/14678012/diff/73001/content/browser/de...
> File content/browser/device_orientation/device_motion_service.h (right):
> 
>
https://chromiumcodereview.appspot.com/14678012/diff/73001/content/browser/de...
> content/browser/device_orientation/device_motion_service.h:47: void
Terminate();
> On 2013/06/27 17:33:26, darin wrote:
> > I'm trying to understand this method.  It looks like the idea is that you
> could
> > have cases where you have existing consumers but you want to interrupt or
stop
> > the background thread.  Is that right?  Can you help me understand when this
> > gets used?
> 
> This method comes from a similar class for gamepad. It is called from
> BrowserMainLoop::ShutdownThreadsAndCleanUp. I've added a Terminate() call to
the
> DeviceMotionService there. It is my understanding that the idea here is to
> cleanly terminate all running threads and sensors when the Browser process
shuts
> down and there are still some existing consumers. In the current
implementation
> for Android there is no polling thread because Android is push-based and the
> fetcher writes to the buffer directly without any need for browser-side
polling.
> However it is still beneficial to terminate the fetcher (and the sensors) in a
> predictable fashion just after the I/O browser thread shuts down. Otherwise it
> is not exactly clear when the destructor for fetcher will be called since
> DeviceMotionService is a singleton.

Maybe a slightly more canonical name for this method would be "Shutdown"
instead?
That helps make it clear that the intent is to shutdown the service.  Would it
be
wise to record this state in a flag so that if AddConsumer happens to be called
again that we won't try to re-start the thread?


>
https://chromiumcodereview.appspot.com/14678012/diff/73001/content/common/sha...
> File content/common/shared_memory_seqlock_buffer.h (right):
> 
>
https://chromiumcodereview.appspot.com/14678012/diff/73001/content/common/sha...
> content/common/shared_memory_seqlock_buffer.h:21: class
> SharedMemorySeqLockBufferBase {
> On 2013/06/27 17:33:26, darin wrote:
> > Why do we need to carve out a Base class in this case?  Since there aren't
any
> > methods here, it seems reasonable to just give SharedMemorySeqLockBuffer<T>
a
> > OneWriterSeqLock member.
> > 
> > It seems like the Gamepad buffer class could be trivially changed to be a
> > specialization of SharedMemorySeqLockBuffer<T>.  Why not just do that as
part
> of
> > this CL?
> 
> ok, I've removed this base class and added an extra lock-parameter to
> SharedMemorySeqLockReaderBase::FetchFromBuffer to keep it generic.
> 
> Regarding Gamepad I think this is best done in a separate CL. Gamepad buffer
> uses "buffer" as a name for "data", so some renaming will need to happen
> touching multiple files (thinking about fetchers and test files), which will
> 'pollute' this CL.

OK


>
https://chromiumcodereview.appspot.com/14678012/diff/73001/content/renderer/s...
> File content/renderer/shared_memory_seqlock_reader.cc (right):
> 
>
https://chromiumcodereview.appspot.com/14678012/diff/73001/content/renderer/s...
> content/renderer/shared_memory_seqlock_reader.cc:25: bool
> SharedMemorySeqLockReaderBase::FetchFromBuffer(
> On 2013/06/27 17:33:26, darin wrote:
> > Is there a path to changing GamepadSharedMemoryReader to use
> > SharedMemorySeqLockReader<T> too?  Are you saving that for a separate CL?
> 
> Yes, I think this is best done in a separate CL. If the refactoring happens to
> break Gamepad it would also be easier to revert.

OK


> As to the refactoring of the gamepad, the idea is to call
> SharedMemorySeqLockReader from GamepadSharedMemoryReader at appropriate
places.
> The GamepadSharedMemoryReader itself will probably still need to exist because
> there is some gamepad-specific code like for handling gestures..

Ah, makes sense.

Thanks for explaining this stuff!  LGTM

[timvolodine, 2013-06-28 00:08:33]

Thanks Darin!

> Maybe a slightly more canonical name for this method would be "Shutdown"
> instead?
> That helps make it clear that the intent is to shutdown the service.  Would it
> be
> wise to record this state in a flag so that if AddConsumer happens to be
called
> again that we won't try to re-start the thread?
> 

Done.

[commit-bot: I haz the power, 2013-07-10 10:42:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timvolodine@chromium.org/14678012/126001

[commit-bot: I haz the power, 2013-07-10 10:57:05]

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

[timvolodine, 2013-07-10 11:04:44]

Hi Chris,

apparently I also need your lgtm for the
content/common/device_motion_messages.h.
Could you please approve asap?

thanks,
Tim

[timvolodine, 2013-07-10 11:19:07]

Hi Chris,

Looks like Chris Evans is on vacation and I also need your lgtm for the
content/common/device_motion_messages.h.
Could you please approve asap?

thanks,
Tim

[palmer, 2013-07-10 18:23:48]

IPC security LGTM

https://chromiumcodereview.appspot.com/14678012/diff/126001/content/browser/d...
File content/browser/device_orientation/device_motion_provider.cc (right):

https://chromiumcodereview.appspot.com/14678012/diff/126001/content/browser/d...
content/browser/device_orientation/device_motion_provider.cc:15: CHECK(res);
Should the browser really crash in this case? I suppose it's probably going to
die soon anyway, if this fails, but I want to make sure that's what you intend.

https://chromiumcodereview.appspot.com/14678012/diff/126001/content/browser/d...
File content/browser/device_orientation/device_motion_provider.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/126001/content/browser/d...
content/browser/device_orientation/device_motion_provider.h:15: //
TODO(timvolodine) Implement this class properly.
What does "properly" mean? Why not do it now? :)

https://chromiumcodereview.appspot.com/14678012/diff/126001/content/browser/d...
content/browser/device_orientation/device_motion_provider.h:39: // FIXME: add
member variable,
Use TODO(timvolodine) instead of FIXME.

[timvolodine, 2013-07-10 21:17:50]

thanks!

https://chromiumcodereview.appspot.com/14678012/diff/126001/content/browser/d...
File content/browser/device_orientation/device_motion_provider.cc (right):

https://chromiumcodereview.appspot.com/14678012/diff/126001/content/browser/d...
content/browser/device_orientation/device_motion_provider.cc:15: CHECK(res);
On 2013/07/10 18:23:48, Chromium Palmer wrote:
> Should the browser really crash in this case? I suppose it's probably going to
> die soon anyway, if this fails, but I want to make sure that's what you
intend.

hmm, yes it will die in this case. I've added a comment to look into possibly
making this non-fatal.

https://chromiumcodereview.appspot.com/14678012/diff/126001/content/browser/d...
File content/browser/device_orientation/device_motion_provider.h (right):

https://chromiumcodereview.appspot.com/14678012/diff/126001/content/browser/d...
content/browser/device_orientation/device_motion_provider.h:15: //
TODO(timvolodine) Implement this class properly.
On 2013/07/10 18:23:48, Chromium Palmer wrote:
> What does "properly" mean? Why not do it now? :)

because the data fetcher is not there yet, it's being implemented as part of a
separate CL (see https://codereview.chromium.org/18572014/).
I've modified the comment to be more specific ;).

https://chromiumcodereview.appspot.com/14678012/diff/126001/content/browser/d...
content/browser/device_orientation/device_motion_provider.h:39: // FIXME: add
member variable,
On 2013/07/10 18:23:48, Chromium Palmer wrote:
> Use TODO(timvolodine) instead of FIXME.

Done.

[commit-bot: I haz the power, 2013-07-10 21:29:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timvolodine@chromium.org/14678012/141001

[commit-bot: I haz the power, 2013-07-10 21:49:41]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2013-07-10 23:07:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timvolodine@chromium.org/14678012/157001

[commit-bot: I haz the power, 2013-07-11 08:01:00]

Change committed as 211043