Implement Device Orientation using shared memory in content/renderer.

Implement Device Orientation using shared memory in content/renderer.

This CL features:
- extracted code in base class device_sensor_event_pump
- implementation of device_motion_event_pump and device_orientation_event_pump
- new unit test for device_orientation_event_pump
- added device_orientation_event_pump to platform, i.e. can be activated using SetDeviceOrientationListener().
- IPC messages, corresponding filter and hardware buffer implementation.

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

[timvolodine, 2013-07-26 11:01:36]

Hi Darin,

Would you mind having a look at this CL?
It is a migration of device orientation to shared memory in content/renderer and
is largely similar to what we did for device motion.

thanks
Tim

[Michael van Ouwerkerk, 2013-07-26 15:14:33]

https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
File content/browser/renderer_host/device_orientation_browser_message_filter.cc
(right):

https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
content/browser/renderer_host/device_orientation_browser_message_filter.cc:16:
DeviceOrientationBrowserMessageFilter::
You can shorten this class name to DeviceOrientationMessageFilter if it is the
only filter for device orientation.

[timvolodine, 2013-07-30 15:14:08]

https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
File content/browser/renderer_host/device_orientation_browser_message_filter.cc
(right):

https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
content/browser/renderer_host/device_orientation_browser_message_filter.cc:16:
DeviceOrientationBrowserMessageFilter::
On 2013/07/26 15:14:33, Michael van Ouwerkerk wrote:
> You can shorten this class name to DeviceOrientationMessageFilter if it is the
> only filter for device orientation.

yeah, there is already a message filter in the browser/device_orientation/
directory, which I'll remove once everything has been ported to the new
approach.

[timvolodine, 2013-07-30 16:19:39]

Hi Darin,

Any chance you could have a look at this CL?

thanks,
Tim

[darin (slow to review), 2013-07-31 19:27:28]

https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
File content/browser/renderer_host/device_orientation_browser_message_filter.h
(right):

https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
content/browser/renderer_host/device_orientation_browser_message_filter.h:6:
#define
CONTENT_BROWSER_RENDERER_HOST_DEVICE_ORIENTATION_BROWSER_MESSAGE_FILTER_H_
is there a reason why this file doesn't exist in
content/browser/device_orientation/ alongside the related code?  note how
resource_message_filter.{h,cc} live in content/browser/loader/.

https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
content/browser/renderer_host/device_orientation_browser_message_filter.h:9:
#include "base/memory/shared_memory.h"
nit: looks like this include is not needed

https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
content/browser/renderer_host/device_orientation_browser_message_filter.h:17:
class DeviceOrientationBrowserMessageFilter : public BrowserMessageFilter {
I think this class should be named DeviceOrientationMessageFilter.  You can drop
the Browser word.  See other BrowserMessageFilter subclasses for reference.

https://chromiumcodereview.appspot.com/20707002/diff/1/content/common/device_...
File content/common/device_orientation_hardware_buffer.h (right):

https://chromiumcodereview.appspot.com/20707002/diff/1/content/common/device_...
content/common/device_orientation_hardware_buffer.h:6: #define
CONTENT_COMMON_DEVICE_ORIENTATION_HARDWARE_BUFFER_H_
nit: should we have a content/common/device_orientation/ subdirectory?

https://chromiumcodereview.appspot.com/20707002/diff/1/content/renderer/devic...
File content/renderer/device_orientation/device_motion_event_pump.h (right):

https://chromiumcodereview.appspot.com/20707002/diff/1/content/renderer/devic...
content/renderer/device_orientation/device_motion_event_pump.h:9: #include
"content/renderer/device_orientation/device_sensor_event_pump.h"
Hmm, so "device sensor" is the term we are using as the generalization of device
motion and device orientation?  It seems like there are some other places in the
code where "device orientation" is used to contain things specific to "device
motion".  Should we standardize on using the term "device sensor" everywhere
applicable?  Is that your plan?  I realize it may take several CLs to implement
such a plan.

[timvolodine, 2013-08-01 15:31:36]

https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
File content/browser/renderer_host/device_orientation_browser_message_filter.h
(right):

https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
content/browser/renderer_host/device_orientation_browser_message_filter.h:6:
#define
CONTENT_BROWSER_RENDERER_HOST_DEVICE_ORIENTATION_BROWSER_MESSAGE_FILTER_H_
On 2013/07/31 19:27:28, darin wrote:
> is there a reason why this file doesn't exist in
> content/browser/device_orientation/ alongside the related code?  note how
> resource_message_filter.{h,cc} live in content/browser/loader/.

AFAIK there is no reason, so moved it there.
I needed to resolve some naming conflicts though because of the old
implementation.
(+looks like gamepad_browser_message_filter.{h,cc} also needs to move to
browser/gamepad)

https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
content/browser/renderer_host/device_orientation_browser_message_filter.h:9:
#include "base/memory/shared_memory.h"
On 2013/07/31 19:27:28, darin wrote:
> nit: looks like this include is not needed

Done.

https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
content/browser/renderer_host/device_orientation_browser_message_filter.h:17:
class DeviceOrientationBrowserMessageFilter : public BrowserMessageFilter {
On 2013/07/31 19:27:28, darin wrote:
> I think this class should be named DeviceOrientationMessageFilter.  You can
drop
> the Browser word.  See other BrowserMessageFilter subclasses for reference.

Done.

https://chromiumcodereview.appspot.com/20707002/diff/1/content/common/device_...
File content/common/device_orientation_hardware_buffer.h (right):

https://chromiumcodereview.appspot.com/20707002/diff/1/content/common/device_...
content/common/device_orientation_hardware_buffer.h:6: #define
CONTENT_COMMON_DEVICE_ORIENTATION_HARDWARE_BUFFER_H_
On 2013/07/31 19:27:28, darin wrote:
> nit: should we have a content/common/device_orientation/ subdirectory?

yes, think this is a good idea, added content/common/device_orientation/ and
moved some files there.

https://chromiumcodereview.appspot.com/20707002/diff/1/content/renderer/devic...
File content/renderer/device_orientation/device_motion_event_pump.h (right):

https://chromiumcodereview.appspot.com/20707002/diff/1/content/renderer/devic...
content/renderer/device_orientation/device_motion_event_pump.h:9: #include
"content/renderer/device_orientation/device_sensor_event_pump.h"
On 2013/07/31 19:27:28, darin wrote:
> Hmm, so "device sensor" is the term we are using as the generalization of
device
> motion and device orientation?  It seems like there are some other places in
the
> code where "device orientation" is used to contain things specific to "device
> motion".  Should we standardize on using the term "device sensor" everywhere
> applicable?  Is that your plan?  I realize it may take several CLs to
implement
> such a plan.

On the Blink side we have DeviceSensorEventController and
DeviceSensorEventDipatcher, so DeviceSensorEventPump seemed like a good option
here.

At some point Adam coined the term 'kinesthetics' e.g.
DeviceKinestheticsController, but I think it sounds a bit far fetched..

At the browser-side in content/browser/device_orientation/ I am planning to use
DeviceInertialSensorService as a generalized class to supply both
motion/orientation data.

In the chromium directory tree we have been using ../device_orientation/..,
probably because the spec is called 'DeviceOrientation Event Specification',
however it also covers motion and compass events.

So something like ../device_sensor/.. would probably be more sensible.
../device_inertial_sensor/.. sounds a bit too specific, although strictly
speaking covers our case (see
http://en.wikipedia.org/wiki/Inertial_measurement_unit).

So at this point I think I would prefer 'device sensor' as the main term (except
when it get close to the hardware where I would call it 'inertial sensor').

Does this make sense?

(maybe we can make this into a thread discussion.. anyway the renaming should
not delay this CL)

[darin (slow to review), 2013-08-01 15:57:07]

On 2013/08/01 15:31:36, timvolodine wrote:
>
https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
> File content/browser/renderer_host/device_orientation_browser_message_filter.h
> (right):
> 
>
https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
> content/browser/renderer_host/device_orientation_browser_message_filter.h:6:
> #define
> CONTENT_BROWSER_RENDERER_HOST_DEVICE_ORIENTATION_BROWSER_MESSAGE_FILTER_H_
> On 2013/07/31 19:27:28, darin wrote:
> > is there a reason why this file doesn't exist in
> > content/browser/device_orientation/ alongside the related code?  note how
> > resource_message_filter.{h,cc} live in content/browser/loader/.
> 
> AFAIK there is no reason, so moved it there.
> I needed to resolve some naming conflicts though because of the old
> implementation.
> (+looks like gamepad_browser_message_filter.{h,cc} also needs to move to
> browser/gamepad)
> 
>
https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
> content/browser/renderer_host/device_orientation_browser_message_filter.h:9:
> #include "base/memory/shared_memory.h"
> On 2013/07/31 19:27:28, darin wrote:
> > nit: looks like this include is not needed
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
> content/browser/renderer_host/device_orientation_browser_message_filter.h:17:
> class DeviceOrientationBrowserMessageFilter : public BrowserMessageFilter {
> On 2013/07/31 19:27:28, darin wrote:
> > I think this class should be named DeviceOrientationMessageFilter.  You can
> drop
> > the Browser word.  See other BrowserMessageFilter subclasses for reference.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/20707002/diff/1/content/common/device_...
> File content/common/device_orientation_hardware_buffer.h (right):
> 
>
https://chromiumcodereview.appspot.com/20707002/diff/1/content/common/device_...
> content/common/device_orientation_hardware_buffer.h:6: #define
> CONTENT_COMMON_DEVICE_ORIENTATION_HARDWARE_BUFFER_H_
> On 2013/07/31 19:27:28, darin wrote:
> > nit: should we have a content/common/device_orientation/ subdirectory?
> 
> yes, think this is a good idea, added content/common/device_orientation/ and
> moved some files there.
> 
>
https://chromiumcodereview.appspot.com/20707002/diff/1/content/renderer/devic...
> File content/renderer/device_orientation/device_motion_event_pump.h (right):
> 
>
https://chromiumcodereview.appspot.com/20707002/diff/1/content/renderer/devic...
> content/renderer/device_orientation/device_motion_event_pump.h:9: #include
> "content/renderer/device_orientation/device_sensor_event_pump.h"
> On 2013/07/31 19:27:28, darin wrote:
> > Hmm, so "device sensor" is the term we are using as the generalization of
> device
> > motion and device orientation?  It seems like there are some other places in
> the
> > code where "device orientation" is used to contain things specific to
"device
> > motion".  Should we standardize on using the term "device sensor" everywhere
> > applicable?  Is that your plan?  I realize it may take several CLs to
> implement
> > such a plan.
> 
> On the Blink side we have DeviceSensorEventController and
> DeviceSensorEventDipatcher, so DeviceSensorEventPump seemed like a good option
> here.
> 
> At some point Adam coined the term 'kinesthetics' e.g.
> DeviceKinestheticsController, but I think it sounds a bit far fetched..
> 
> At the browser-side in content/browser/device_orientation/ I am planning to
use
> DeviceInertialSensorService as a generalized class to supply both
> motion/orientation data.
> 
> In the chromium directory tree we have been using ../device_orientation/..,
> probably because the spec is called 'DeviceOrientation Event Specification',
> however it also covers motion and compass events.
> 
> So something like ../device_sensor/.. would probably be more sensible.
> ../device_inertial_sensor/.. sounds a bit too specific, although strictly
> speaking covers our case (see
> http://en.wikipedia.org/wiki/Inertial_measurement_unit).
> 
> So at this point I think I would prefer 'device sensor' as the main term
(except
> when it get close to the hardware where I would call it 'inertial sensor').
> 
> Does this make sense?
> 
> (maybe we can make this into a thread discussion.. anyway the renaming should
> not delay this CL)

I think "device sensor" or "inertial sensor" could work as the catch-all term.

No need for such a renaming to block this CL!

[darin (slow to review), 2013-08-01 16:00:36]

LGTM

[timvolodine, 2013-08-01 16:00:45]

On 2013/08/01 15:57:07, darin wrote:
> On 2013/08/01 15:31:36, timvolodine wrote:
> >
>
https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
> > File
content/browser/renderer_host/device_orientation_browser_message_filter.h
> > (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
> > content/browser/renderer_host/device_orientation_browser_message_filter.h:6:
> > #define
> > CONTENT_BROWSER_RENDERER_HOST_DEVICE_ORIENTATION_BROWSER_MESSAGE_FILTER_H_
> > On 2013/07/31 19:27:28, darin wrote:
> > > is there a reason why this file doesn't exist in
> > > content/browser/device_orientation/ alongside the related code?  note how
> > > resource_message_filter.{h,cc} live in content/browser/loader/.
> > 
> > AFAIK there is no reason, so moved it there.
> > I needed to resolve some naming conflicts though because of the old
> > implementation.
> > (+looks like gamepad_browser_message_filter.{h,cc} also needs to move to
> > browser/gamepad)
> > 
> >
>
https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
> > content/browser/renderer_host/device_orientation_browser_message_filter.h:9:
> > #include "base/memory/shared_memory.h"
> > On 2013/07/31 19:27:28, darin wrote:
> > > nit: looks like this include is not needed
> > 
> > Done.
> > 
> >
>
https://chromiumcodereview.appspot.com/20707002/diff/1/content/browser/render...
> >
content/browser/renderer_host/device_orientation_browser_message_filter.h:17:
> > class DeviceOrientationBrowserMessageFilter : public BrowserMessageFilter {
> > On 2013/07/31 19:27:28, darin wrote:
> > > I think this class should be named DeviceOrientationMessageFilter.  You
can
> > drop
> > > the Browser word.  See other BrowserMessageFilter subclasses for
reference.
> > 
> > Done.
> > 
> >
>
https://chromiumcodereview.appspot.com/20707002/diff/1/content/common/device_...
> > File content/common/device_orientation_hardware_buffer.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/20707002/diff/1/content/common/device_...
> > content/common/device_orientation_hardware_buffer.h:6: #define
> > CONTENT_COMMON_DEVICE_ORIENTATION_HARDWARE_BUFFER_H_
> > On 2013/07/31 19:27:28, darin wrote:
> > > nit: should we have a content/common/device_orientation/ subdirectory?
> > 
> > yes, think this is a good idea, added content/common/device_orientation/ and
> > moved some files there.
> > 
> >
>
https://chromiumcodereview.appspot.com/20707002/diff/1/content/renderer/devic...
> > File content/renderer/device_orientation/device_motion_event_pump.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/20707002/diff/1/content/renderer/devic...
> > content/renderer/device_orientation/device_motion_event_pump.h:9: #include
> > "content/renderer/device_orientation/device_sensor_event_pump.h"
> > On 2013/07/31 19:27:28, darin wrote:
> > > Hmm, so "device sensor" is the term we are using as the generalization of
> > device
> > > motion and device orientation?  It seems like there are some other places
in
> > the
> > > code where "device orientation" is used to contain things specific to
> "device
> > > motion".  Should we standardize on using the term "device sensor"
everywhere
> > > applicable?  Is that your plan?  I realize it may take several CLs to
> > implement
> > > such a plan.
> > 
> > On the Blink side we have DeviceSensorEventController and
> > DeviceSensorEventDipatcher, so DeviceSensorEventPump seemed like a good
option
> > here.
> > 
> > At some point Adam coined the term 'kinesthetics' e.g.
> > DeviceKinestheticsController, but I think it sounds a bit far fetched..
> > 
> > At the browser-side in content/browser/device_orientation/ I am planning to
> use
> > DeviceInertialSensorService as a generalized class to supply both
> > motion/orientation data.
> > 
> > In the chromium directory tree we have been using ../device_orientation/..,
> > probably because the spec is called 'DeviceOrientation Event Specification',
> > however it also covers motion and compass events.
> > 
> > So something like ../device_sensor/.. would probably be more sensible.
> > ../device_inertial_sensor/.. sounds a bit too specific, although strictly
> > speaking covers our case (see
> > http://en.wikipedia.org/wiki/Inertial_measurement_unit).
> > 
> > So at this point I think I would prefer 'device sensor' as the main term
> (except
> > when it get close to the hardware where I would call it 'inertial sensor').
> > 
> > Does this make sense?
> > 
> > (maybe we can make this into a thread discussion.. anyway the renaming
should
> > not delay this CL)
> 
> I think "device sensor" or "inertial sensor" could work as the catch-all term.
> 
> No need for such a renaming to block this CL!

sure, renaming can be done later..

due to moving files to content/common/device_orientation/.. this CL has grown
somewhat, hope this is still ok for you. Let me know if not.

[timvolodine, 2013-08-02 13:51:06]

+Chris for IPC security review.

[palmer, 2013-08-02 19:10:37]

LGTM

https://chromiumcodereview.appspot.com/20707002/diff/20001/content/browser/de...
File content/browser/device_orientation/message_filter.h (right):

https://chromiumcodereview.appspot.com/20707002/diff/20001/content/browser/de...
content/browser/device_orientation/message_filter.h:5: #ifndef
CONTENT_BROWSER_DEVICE_ORIENTATION_MESSAGE_FILTER_OLD_H_
But file name has not changed?

https://chromiumcodereview.appspot.com/20707002/diff/20001/content/common/dev...
File content/common/device_orientation/device_orientation_messages.h (right):

https://chromiumcodereview.appspot.com/20707002/diff/20001/content/common/dev...
content/common/device_orientation/device_orientation_messages.h:16: // handles
that will hold the data from the hardware. See
Typo: "handle", singular

[timvolodine, 2013-08-05 15:26:47]

https://codereview.chromium.org/20707002/diff/20001/content/browser/device_or...
File content/browser/device_orientation/message_filter.h (right):

https://codereview.chromium.org/20707002/diff/20001/content/browser/device_or...
content/browser/device_orientation/message_filter.h:5: #ifndef
CONTENT_BROWSER_DEVICE_ORIENTATION_MESSAGE_FILTER_OLD_H_
On 2013/08/02 19:10:37, Chromium Palmer wrote:
> But file name has not changed?

Done.
(reverted to the old #ifndef, new filters have a different guard anyway). This
file is meant to be deleted soon..

https://codereview.chromium.org/20707002/diff/20001/content/common/device_ori...
File content/common/device_orientation/device_orientation_messages.h (right):

https://codereview.chromium.org/20707002/diff/20001/content/common/device_ori...
content/common/device_orientation/device_orientation_messages.h:16: // handles
that will hold the data from the hardware. See
On 2013/08/02 19:10:37, Chromium Palmer wrote:
> Typo: "handle", singular

Done.

[commit-bot: I haz the power, 2013-08-05 19:56:42]

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

[commit-bot: I haz the power, 2013-08-05 21:17:20]

Change committed as 215680