Implements part of Device Motion in the Renderer

Implements part of Device Motion in the Renderer

Implements Device Motion in the renderer and the message filter for Device
Motion in the browser.

This depends on WebKit patch at https://bugs.webkit.org/show_bug.cgi?id=89197.


BUG=59201
TEST=browser_tests:DeviceOrientationBrowserTest.BasicMotionTest

[bulach, 2012-07-02 12:47:56]

thanks amy!

a few nits below, but most important the comment about the switching the flag
from enabled to disabled. also, it may be better at some point to have a
"Sensor" API that would allow us to introduce more sensor datatypes without
having to touch so many files in between..

http://codereview.chromium.org/10698046/diff/1/content/browser/device_orienta...
File content/browser/device_orientation/device_orientation_browsertest.cc
(right):

http://codereview.chromium.org/10698046/diff/1/content/browser/device_orienta...
content/browser/device_orientation/device_orientation_browsertest.cc:21:
explicit MockProvider(const Motion& motion, const Orientation& orientation)
nit: "explicit" is no longer needed..

http://codereview.chromium.org/10698046/diff/1/content/browser/device_orienta...
content/browser/device_orientation/device_orientation_browsertest.cc:69: Motion
kTestMotion;
nit: "k" is used for constants, in this case just kTestOrientation..
perhaps it's clearer to:

const Orientation kTestOrientation;
Motion test_motion;
test_motion.SetFooBar..();

http://codereview.chromium.org/10698046/diff/1/content/browser/device_orienta...
File content/browser/device_orientation/motion.h (right):

http://codereview.chromium.org/10698046/diff/1/content/browser/device_orienta...
content/browser/device_orientation/motion.h:32: Motion(const Motion&);
is this copy constructor necessary? if possible, it's probably best to avoid it,
i.e., mark it as explicit and copy it where needed.. and then if possible, also
add a "DISALLOW_COPY_AND_ASSIGN(Motion)" as the last line in the private
section..

http://codereview.chromium.org/10698046/diff/1/content/browser/device_orienta...
content/browser/device_orientation/motion.h:48: void setAccelerationX(double
accelerationX) {
nit: wrong case, needs to s/set/Set/ and s/can/Can/ (and a few others below.. :)
or rather, set_acceleration_x / can_provide_acceleration_x / acceleration_x as
these methods are all simple inlined getters/setters..

http://codereview.chromium.org/10698046/diff/1/content/browser/device_orienta...
File content/browser/device_orientation/motion_message_filter.cc (right):

http://codereview.chromium.org/10698046/diff/1/content/browser/device_orienta...
content/browser/device_orientation/motion_message_filter.cc:43: IPC::Sender*
sender_;  // Weak pointer.
not sure how is this a weak pointer in this context?

http://codereview.chromium.org/10698046/diff/1/content/browser/device_orienta...
content/browser/device_orientation/motion_message_filter.cc:91:
motion.canProvideRotationRateGamma();
nit: from 72-91, need an extra indent for the continuation lines..

http://codereview.chromium.org/10698046/diff/1/content/browser/device_orienta...
content/browser/device_orientation/motion_message_filter.cc:120: this);
nit: move this line to 119, and indent it just by 4..

http://codereview.chromium.org/10698046/diff/1/content/browser/device_orienta...
File content/browser/device_orientation/orientation_message_filter.cc (right):

http://codereview.chromium.org/10698046/diff/1/content/browser/device_orienta...
content/browser/device_orientation/orientation_message_filter.cc:36: virtual
void OnOrientationUpdate(const Orientation& orientation);
nit: unrelated to your patch, but please fix the indentation from 28-36 (needs
-2).

http://codereview.chromium.org/10698046/diff/1/content/browser/device_orienta...
content/browser/device_orientation/orientation_message_filter.cc:97:
render_view_id, this);
nit: indent +2

http://codereview.chromium.org/10698046/diff/1/content/browser/device_orienta...
File content/browser/device_orientation/provider_impl.h (right):

http://codereview.chromium.org/10698046/diff/1/content/browser/device_orienta...
content/browser/device_orientation/provider_impl.h:44: OrientationObserver*
observer) OVERRIDE;
nit: indent +2

http://codereview.chromium.org/10698046/diff/1/content/public/common/content_...
File content/public/common/content_switches.h (left):

http://codereview.chromium.org/10698046/diff/1/content/public/common/content_...
content/public/common/content_switches.h:91: CONTENT_EXPORT extern const char
kEnableDeviceMotion[];
I think we normally keep the kEnableDeviceMotion until late in the process (that
is, keep disable by default during development..). once everything is ready and
tested, we then switch to kDisable (i.e, enabled by default once it all done..)

[aousterh, 2012-07-04 13:31:54]

Thanks for the comments! I responded to them, made the changes you suggested,
and also rebased onto master.

https://chromiumcodereview.appspot.com/10698046/diff/1/content/browser/device...
File content/browser/device_orientation/device_orientation_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10698046/diff/1/content/browser/device...
content/browser/device_orientation/device_orientation_browsertest.cc:21:
explicit MockProvider(const Motion& motion, const Orientation& orientation)
On 2012/07/02 12:47:56, bulach wrote:
> nit: "explicit" is no longer needed..

Done.

https://chromiumcodereview.appspot.com/10698046/diff/1/content/browser/device...
content/browser/device_orientation/device_orientation_browsertest.cc:69: Motion
kTestMotion;
On 2012/07/02 12:47:56, bulach wrote:
> nit: "k" is used for constants, in this case just kTestOrientation..
> perhaps it's clearer to:
> 
> const Orientation kTestOrientation;
> Motion test_motion;
> test_motion.SetFooBar..();

Done.

https://chromiumcodereview.appspot.com/10698046/diff/1/content/browser/device...
File content/browser/device_orientation/motion.h (right):

https://chromiumcodereview.appspot.com/10698046/diff/1/content/browser/device...
content/browser/device_orientation/motion.h:32: Motion(const Motion&);
On 2012/07/02 12:47:56, bulach wrote:
> is this copy constructor necessary? if possible, it's probably best to avoid
it,
> i.e., mark it as explicit and copy it where needed.. and then if possible,
also
> add a "DISALLOW_COPY_AND_ASSIGN(Motion)" as the last line in the private
> section..

The copy constructor is used in device_orientation_browsertest and will probably
be used in the provider (as is the case for orientation). I'll also add in the
assignment operator, since the style guide says that that should be defined if
the copy constructor is defined.

https://chromiumcodereview.appspot.com/10698046/diff/1/content/browser/device...
content/browser/device_orientation/motion.h:48: void setAccelerationX(double
accelerationX) {
On 2012/07/02 12:47:56, bulach wrote:
> nit: wrong case, needs to s/set/Set/ and s/can/Can/ (and a few others below..
:)
> or rather, set_acceleration_x / can_provide_acceleration_x / acceleration_x as
> these methods are all simple inlined getters/setters..

Done.

https://chromiumcodereview.appspot.com/10698046/diff/1/content/browser/device...
File content/browser/device_orientation/motion_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10698046/diff/1/content/browser/device...
content/browser/device_orientation/motion_message_filter.cc:43: IPC::Sender*
sender_;  // Weak pointer.
On 2012/07/02 12:47:56, bulach wrote:
> not sure how is this a weak pointer in this context?

The sender isn't owned by the MotionMessageFilter (I think it's owned by the
Channel Proxy, which keeps a scoped pointer to it), so this pointer doesn't
effect its lifetime.

https://chromiumcodereview.appspot.com/10698046/diff/1/content/browser/device...
content/browser/device_orientation/motion_message_filter.cc:91:
motion.canProvideRotationRateGamma();
On 2012/07/02 12:47:56, bulach wrote:
> nit: from 72-91, need an extra indent for the continuation lines..

Done.

https://chromiumcodereview.appspot.com/10698046/diff/1/content/browser/device...
content/browser/device_orientation/motion_message_filter.cc:120: this);
On 2012/07/02 12:47:56, bulach wrote:
> nit: move this line to 119, and indent it just by 4..

Done.

https://chromiumcodereview.appspot.com/10698046/diff/1/content/browser/device...
File content/browser/device_orientation/orientation_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10698046/diff/1/content/browser/device...
content/browser/device_orientation/orientation_message_filter.cc:36: virtual
void OnOrientationUpdate(const Orientation& orientation);
On 2012/07/02 12:47:56, bulach wrote:
> nit: unrelated to your patch, but please fix the indentation from 28-36 (needs
> -2).

Done.

https://chromiumcodereview.appspot.com/10698046/diff/1/content/browser/device...
content/browser/device_orientation/orientation_message_filter.cc:97:
render_view_id, this);
On 2012/07/02 12:47:56, bulach wrote:
> nit: indent +2

Done.

https://chromiumcodereview.appspot.com/10698046/diff/1/content/browser/device...
File content/browser/device_orientation/provider_impl.h (right):

https://chromiumcodereview.appspot.com/10698046/diff/1/content/browser/device...
content/browser/device_orientation/provider_impl.h:44: OrientationObserver*
observer) OVERRIDE;
On 2012/07/02 12:47:56, bulach wrote:
> nit: indent +2

Done.

https://chromiumcodereview.appspot.com/10698046/diff/1/content/public/common/...
File content/public/common/content_switches.h (left):

https://chromiumcodereview.appspot.com/10698046/diff/1/content/public/common/...
content/public/common/content_switches.h:91: CONTENT_EXPORT extern const char
kEnableDeviceMotion[];
On 2012/07/02 12:47:56, bulach wrote:
> I think we normally keep the kEnableDeviceMotion until late in the process
(that
> is, keep disable by default during development..). once everything is ready
and
> tested, we then switch to kDisable (i.e, enabled by default once it all
done..)

Oh ok, makes sense. Done.

[Steve Block, 2012-07-04 13:58:24]

It looks like Marcus has this under control!

http://codereview.chromium.org/10698046/diff/1031/content/browser/device_orie...
File content/browser/device_orientation/motion_message_filter.cc (right):

http://codereview.chromium.org/10698046/diff/1031/content/browser/device_orie...
content/browser/device_orientation/motion_message_filter.cc:35: virtual void
OnMotionUpdate(const Motion& motion);
weird indentation

http://codereview.chromium.org/10698046/diff/1031/content/browser/device_orie...
File content/browser/device_orientation/provider.h (right):

http://codereview.chromium.org/10698046/diff/1031/content/browser/device_orie...
content/browser/device_orientation/provider.h:28: class OrientationObserver {
You might want to do the Oberver -> OrientationObserver rename as a separate
prerequisite change to reduce the size of this diff.

[aousterh, 2012-08-13 10:25:58]

This new patch set implements the revised DeviceMotion API in WebKit, reflecting
the change in the WebKit patch (https://bugs.webkit.org/show_bug.cgi?id=89197)
from passing DeviceMotionData through the WebKit layer to through the Platform
layer instead.

This patch also rebases onto master, so a lot of the diff from the previous
patch set can be attributed to the rebase. The refactoring of ProviderImpl
(https://chromiumcodereview.appspot.com/10755002/) is responsible for most of
those changes.

[hans, 2012-08-13 13:35:19]

LGTM, just a couple of nits.

Since this is blocked on the WebKit patch, maybe we should split off and land
the parts that aren't that tightly related to the WebKit code:

I think for example the changes to DeviceData, the Motion class, and the
Provider unit test could be split off and landed as a separate patch.

http://codereview.chromium.org/10698046/diff/15001/content/browser/device_ori...
File content/browser/device_orientation/device_orientation_browsertest.cc
(right):

http://codereview.chromium.org/10698046/diff/15001/content/browser/device_ori...
content/browser/device_orientation/device_orientation_browsertest.cc:85: //
crbug.com/113952
this crbug.com comment is obsolete; feel free to remove it while you're editing
here anyway

http://codereview.chromium.org/10698046/diff/15001/content/browser/device_ori...
File content/browser/device_orientation/motion_message_filter.cc (right):

http://codereview.chromium.org/10698046/diff/15001/content/browser/device_ori...
content/browser/device_orientation/motion_message_filter.cc:16: : 
MessageFilter(DeviceData::kTypeMotion) {
ultra nit: looks like an extra space between the : and the M

http://codereview.chromium.org/10698046/diff/15001/content/browser/device_ori...
File content/browser/device_orientation/observer_delegate.cc (right):

http://codereview.chromium.org/10698046/diff/15001/content/browser/device_ori...
content/browser/device_orientation/observer_delegate.cc:38: DeviceData*
ObserverDelegate::EmptyDeviceData(DeviceData::Type type) {
maybe a switch here would be better:

switch (type) {
  case DeviceData::kTypeMotion:
    return new Motion();
  case DeviceData::kTypeOrientation:
    return new Orientation();
}
NOTREACHED();
return NULL;

This way we'll get a warning if a new device data type is added and someone
forgets to add a case to the switch.

http://codereview.chromium.org/10698046/diff/15001/content/browser/device_ori...
File content/browser/device_orientation/provider_unittest.cc (right):

http://codereview.chromium.org/10698046/diff/15001/content/browser/device_ori...
content/browser/device_orientation/provider_unittest.cc:611: // Tests that
Motion events always fire, even if the motion is unchanged.
i was just about to ask for a test like that :)

http://codereview.chromium.org/10698046/diff/15001/content/renderer/device_mo...
File content/renderer/device_motion_dispatcher.cc (right):

http://codereview.chromium.org/10698046/diff/15001/content/renderer/device_mo...
content/renderer/device_motion_dispatcher.cc:28: OnDeviceMotionUpdated)
i think this might fit on one line and still be < 80 columns

http://codereview.chromium.org/10698046/diff/15001/content/renderer/device_mo...
content/renderer/device_motion_dispatcher.cc:48: return last_motion_;
ultra nit: indent should be two spaces, but it looks like there's three here

[aousterh, 2012-08-16 15:23:49]

The parts of this patch that do not depend on the WebKit patch have been pulled
into a different patch (https://chromiumcodereview.appspot.com/10823310/).

I also rebased and made a few modifications to adjust to changes in the API in
the corresponding WebKit patch.

https://chromiumcodereview.appspot.com/10698046/diff/15001/content/browser/de...
File content/browser/device_orientation/device_orientation_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10698046/diff/15001/content/browser/de...
content/browser/device_orientation/device_orientation_browsertest.cc:85: //
crbug.com/113952
On 2012/08/13 13:35:19, hans wrote:
> this http://crbug.com comment is obsolete; feel free to remove it while you're
editing
> here anyway

Done.

https://chromiumcodereview.appspot.com/10698046/diff/15001/content/browser/de...
File content/browser/device_orientation/motion_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10698046/diff/15001/content/browser/de...
content/browser/device_orientation/motion_message_filter.cc:16: : 
MessageFilter(DeviceData::kTypeMotion) {
On 2012/08/13 13:35:19, hans wrote:
> ultra nit: looks like an extra space between the : and the M

Done.

https://chromiumcodereview.appspot.com/10698046/diff/15001/content/browser/de...
File content/browser/device_orientation/observer_delegate.cc (right):

https://chromiumcodereview.appspot.com/10698046/diff/15001/content/browser/de...
content/browser/device_orientation/observer_delegate.cc:38: DeviceData*
ObserverDelegate::EmptyDeviceData(DeviceData::Type type) {
On 2012/08/13 13:35:19, hans wrote:
> maybe a switch here would be better:
> 
> switch (type) {
>   case DeviceData::kTypeMotion:
>     return new Motion();
>   case DeviceData::kTypeOrientation:
>     return new Orientation();
> }
> NOTREACHED();
> return NULL;
> 
> This way we'll get a warning if a new device data type is added and someone
> forgets to add a case to the switch.

Done.

https://chromiumcodereview.appspot.com/10698046/diff/15001/content/renderer/d...
File content/renderer/device_motion_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10698046/diff/15001/content/renderer/d...
content/renderer/device_motion_dispatcher.cc:28: OnDeviceMotionUpdated)
On 2012/08/13 13:35:19, hans wrote:
> i think this might fit on one line and still be < 80 columns

Done.

https://chromiumcodereview.appspot.com/10698046/diff/15001/content/renderer/d...
content/renderer/device_motion_dispatcher.cc:48: return last_motion_;
On 2012/08/13 13:35:19, hans wrote:
> ultra nit: indent should be two spaces, but it looks like there's three here

Done.

[hans, 2012-08-16 16:38:40]

Just one comment, otherwise still lgtm.

I guess it's time to bring in the owners reviewers.

https://chromiumcodereview.appspot.com/10698046/diff/26003/content/renderer/d...
File content/renderer/device_motion_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10698046/diff/26003/content/renderer/d...
content/renderer/device_motion_dispatcher.cc:54: 
should last_motion_ be reset here before we start initializing it?

[aousterh, 2012-08-16 17:21:05]

Thanks Hans.

jam: can you do an OWNERS review for content/ and ipc/? Thanks!

https://chromiumcodereview.appspot.com/10698046/diff/26003/content/renderer/d...
File content/renderer/device_motion_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10698046/diff/26003/content/renderer/d...
content/renderer/device_motion_dispatcher.cc:54: 
On 2012/08/16 16:38:40, hans wrote:
> should last_motion_ be reset here before we start initializing it?

Good point. Done.

[hans, 2012-08-20 13:38:41]

Re-uploading this patch here: https://chromiumcodereview.appspot.com/10827415/