Implement Android shared memory data fetcher for Device Motion.

Implement Android shared memory data fetcher for Device Motion.

This CL implements the shared memory data fetcher for devices running on Android. The data fetchers acquires data from sensors (e.g. accelerometer, gyroscope) and puts it into a shared memory buffer. Because the sensor data is push-based on Android there is no need for an extra polling thread to fetch the data at regular intervals.

This CL features:
- interface for shared memory data fetchers for Device Motion.
- implementation of the Android shared memory data fetcher.
- unit tests for the Android shared memory data fetcher.
- implementation of the empty data fetcher for other platforms.
- modification of the existing Android data fetcher to be a singleton, such that it is compatible with the current Device Orientation implementation.

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

[bulach, 2013-07-04 08:23:35]

thanks tim, good stuff! some comments below, I think the last one will hopefully
simplify things a bit:

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
File content/browser/device_orientation/data_fetcher_impl_android.cc (right):

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_impl_android.cc:25: :
device_motion_buffer_(0),
nit: s/0/NULL/

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_impl_android.cc:31: void
DataFetcherImplAndroid::Init(JNIEnv* env) {
nit: not related to your patch, but rather than Init, this should be called
"Register"..

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_impl_android.cc:54: // the
active event type(s).
the first part of the TODO seem done :)

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_impl_android.cc:177:
device_motion_buffer_->data.allAvailableSensorsAreActive = true;
just checking:
once it has all three datatypes, the buffer will be ready.
then, if it updates only one of them, the "other side" would still consider the
whole set "fresh" ? or should this flag be per sensor?

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_impl_android.cc:186:
receivedMotionData[0] = receivedMotionData[1] = receivedMotionData[2] = 0;
nit:
memset(received_motion_data, 0, sizeof(received_motion_data));

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_impl_android.cc:202:
receivedMotionData[0] = receivedMotionData[1] = receivedMotionData[2] = 0;
nit: memset

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
File content/browser/device_orientation/data_fetcher_impl_android.h (right):

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_impl_android.h:62: protected:
why is it protected? styleguide forbids non-private data members..

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_impl_android.h:85: int
receivedMotionData[3];
nit: received_motion_data.
also, I think using the indices to map the type could be done more explicitly,
something like:

enum {
  DATA_ACCELERATION = 0,
  DATA_ACCELERATION_INCLUDING_GRAVITY = 1,
  DATA_ROTATION_RATE = 2,
  DATA_MAX = 3,
};
int received_motioned_data[DATA_MAX];

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
File content/browser/device_orientation/data_fetcher_impl_android_unittest.cc
(right):

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_impl_android_unittest.cc:65:
fetcher.StopFetchingDeviceMotionData();
nit: for completeness, could add this after the Stop call in these tests..

 ASSERT_FALSE(buffer_->data.allAvailableSensorsAreActive);

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
File content/browser/device_orientation/data_fetcher_platform.cc (right):

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_platform.cc:11: :
device_motion_buffer_(0) {
nit: NULL

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
File content/browser/device_orientation/data_fetcher_shared_memory.h (right):

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_shared_memory.h:17: class
DataFetcherSharedMemory {
in the "vibration API" review, jam@ pointed out to:
http://dev.chromium.org/developers/design-documents/conventions-and-patterns-...

I think this falls into the case described "
Small platform differences in the header, larger ones in the implementation:
split the implementation"

so this data_fetcher_shared_memory.h would have a corresponding .cc for all
platforms, and one specific for data_fetcher_shared_memory_android.cc with a
different implementation. this would avoid the need for typedef and the extra
indirection at data_fetcher_platform.h/cc..

[timvolodine, 2013-07-04 12:02:52]

thanks Marcus, replies below.

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
File content/browser/device_orientation/data_fetcher_impl_android.cc (right):

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_impl_android.cc:25: :
device_motion_buffer_(0),
On 2013/07/04 08:23:36, bulach wrote:
> nit: s/0/NULL/

Done.

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_impl_android.cc:31: void
DataFetcherImplAndroid::Init(JNIEnv* env) {
On 2013/07/04 08:23:36, bulach wrote:
> nit: not related to your patch, but rather than Init, this should be called
> "Register"..

Done.

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_impl_android.cc:54: // the
active event type(s).
On 2013/07/04 08:23:36, bulach wrote:
> the first part of the TODO seem done :)

Done.

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_impl_android.cc:177:
device_motion_buffer_->data.allAvailableSensorsAreActive = true;
On 2013/07/04 08:23:36, bulach wrote:
> just checking:
> once it has all three datatypes, the buffer will be ready.
> then, if it updates only one of them, the "other side" would still consider
the
> whole set "fresh" ? or should this flag be per sensor?

This flag is actually intended for the cases where some of the sensors are not
available (i.e. it is possible for allAvailableSensorsAreActive to be true while
no motion data can be provided in which case Blink would eventually receive an
event with null values). So this flag is not related to freshness rather that
the data is in a sense complete and good to read for the other side.

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_impl_android.cc:186:
receivedMotionData[0] = receivedMotionData[1] = receivedMotionData[2] = 0;
On 2013/07/04 08:23:36, bulach wrote:
> nit:
> memset(received_motion_data, 0, sizeof(received_motion_data));

Done.

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_impl_android.cc:202:
receivedMotionData[0] = receivedMotionData[1] = receivedMotionData[2] = 0;
On 2013/07/04 08:23:36, bulach wrote:
> nit: memset

Done.

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
File content/browser/device_orientation/data_fetcher_impl_android.h (right):

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_impl_android.h:62: protected:
On 2013/07/04 08:23:36, bulach wrote:
> why is it protected? styleguide forbids non-private data members..

I needed a couple of methods to be protected for easier testing (i.e. in
data_fetcher_impl_android_unittest). I've made the data members private now.

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_impl_android.h:85: int
receivedMotionData[3];
On 2013/07/04 08:23:36, bulach wrote:
> nit: received_motion_data.
> also, I think using the indices to map the type could be done more explicitly,
> something like:
> 
> enum {
>   DATA_ACCELERATION = 0,
>   DATA_ACCELERATION_INCLUDING_GRAVITY = 1,
>   DATA_ROTATION_RATE = 2,
>   DATA_MAX = 3,
> };
> int received_motioned_data[DATA_MAX];

Done.

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
File content/browser/device_orientation/data_fetcher_impl_android_unittest.cc
(right):

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_impl_android_unittest.cc:65:
fetcher.StopFetchingDeviceMotionData();
On 2013/07/04 08:23:36, bulach wrote:
> nit: for completeness, could add this after the Stop call in these tests..
> 
>  ASSERT_FALSE(buffer_->data.allAvailableSensorsAreActive);

Done.

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
File content/browser/device_orientation/data_fetcher_platform.cc (right):

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_platform.cc:11: :
device_motion_buffer_(0) {
On 2013/07/04 08:23:36, bulach wrote:
> nit: NULL

Done.

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
File content/browser/device_orientation/data_fetcher_shared_memory.h (right):

https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/dev...
content/browser/device_orientation/data_fetcher_shared_memory.h:17: class
DataFetcherSharedMemory {
On 2013/07/04 08:23:36, bulach wrote:
> in the "vibration API" review, jam@ pointed out to:
>
http://dev.chromium.org/developers/design-documents/conventions-and-patterns-...
> 
> I think this falls into the case described "
> Small platform differences in the header, larger ones in the implementation:
> split the implementation"
> 
> so this data_fetcher_shared_memory.h would have a corresponding .cc for all
> platforms, and one specific for data_fetcher_shared_memory_android.cc with a
> different implementation. this would avoid the need for typedef and the extra
> indirection at data_fetcher_platform.h/cc..

hmm, yes that looks like the normal way to go. However in this case I think
typedefs are still needed because we need to instantiate a particular singleton
fetcher using a static instance() method. Not sure how I would do this using the
approach with split implementations..
The current approach with typedefs is based on how the gamepad solves this
problem, which I think is cleaner as compared to the DataFetcherFactory in
provider_impl..

[bulach, 2013-07-05 11:42:44]

lgtm

thanks tim! question: the instance() method should be implemented by the
platform-specific code in the .cc file, right? callers of instance() should only
rely on the platform-agnostic API as defined in the .h..
I guess this could all be simplified by:

data_fetcher_shared_memory.h:
-- declare / define DataFetcherSharedMemory class, with just method declaration.

data_fetcher_shared_memory_default.cc:
-- "empty" / simple implementation for all methods defined in
data_fetcher_shared_memory.h.
-- this .cc will be compiled for all platforms, except android (exclude this
file via GYP for android).
-- the instance() method will be implemented here, returning an instance of
DataFetcherSharedMemory().. or perhaps even better, NULL for platforms that
don't implement this..

data_fetcher_shared_memory_android.cc:
-- concrete implementation of DataFetcherSharedMemory specifically for android.
-- this .cc will be compiled for android only (since it has _android suffix, GYP
already knows how to exclude it for other platforms)
-- instance() returns a new DataFetcherSharedMemory, which is implemented here..

would that work?

[bulach, 2013-07-05 11:43:14]

sorry, I didn't mean lgtm :) would like to discuss this a bit further, just
pressed the wrong button.. :-/ really sorry.

[timvolodine, 2013-07-05 12:26:41]

On 2013/07/05 11:43:14, bulach wrote:
> sorry, I didn't mean lgtm :) would like to discuss this a bit further, just
> pressed the wrong button.. :-/ really sorry.

no worries :).
btw this is new: lgtm by accident ;)

[timvolodine, 2013-07-05 13:22:43]

On 2013/07/05 11:42:44, bulach wrote:
> lgtm
> 
> thanks tim! question: the instance() method should be implemented by the
> platform-specific code in the .cc file, right? callers of instance() should
only
> rely on the platform-agnostic API as defined in the .h..
> I guess this could all be simplified by:
> 
> data_fetcher_shared_memory.h:
> -- declare / define DataFetcherSharedMemory class, with just method
declaration.
> 
> data_fetcher_shared_memory_default.cc:
> -- "empty" / simple implementation for all methods defined in
> data_fetcher_shared_memory.h.
> -- this .cc will be compiled for all platforms, except android (exclude this
> file via GYP for android).
> -- the instance() method will be implemented here, returning an instance of
> DataFetcherSharedMemory().. or perhaps even better, NULL for platforms that
> don't implement this..
> 
> data_fetcher_shared_memory_android.cc:
> -- concrete implementation of DataFetcherSharedMemory specifically for
android.
> -- this .cc will be compiled for android only (since it has _android suffix,
GYP
> already knows how to exclude it for other platforms)
> -- instance() returns a new DataFetcherSharedMemory, which is implemented
here..
> 
> would that work?

That would be the classical split implementation approach if I understand
correctly.

In this case we still need to support the 'old' implementation of
DeviceOrientation, such that DataFetcherImplAndroid should be both a DataFetcher
and DataFetcherSharedMemory, which I implemented as multiple inheritance. I
agree that in the end this probably should be implemented as you described, but
the problem is that we have non-shared memory fetcher methods as well and they
all need to be instantiated by the same static instance() method which cannot be
virtual. The described approach would be perfectly possible once the device
orientation also switches to the shared memory implementation, so the current
platform implementation is a temporary measure in that respect.

I think an alternative could be to put shared memory methods in data_fetcher
(although I tried to keep shared memory fetcher and the original fetcher methods
separate as much as possible for clarity), but that would involve a major
refactoring as the current implementation of device orientation does not follow
the suggested split-implementation approach.

I've added device_motion_provider.{h,cc} where the fetcher actually gets
instantiated, these files are for illustration, because they overlap with
another CL and are technically not part of this CL :/..

[timvolodine, 2013-07-08 13:40:38]

refactored the platform dependent code to use the suggested split-implementation
approach.
please have a look!

[bulach, 2013-07-09 09:03:29]

good stuff, thanks! just some nits below and some questions about the
"instance()" methods:

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_impl_android.cc (right):

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.cc:54:
DataFetcherImplAndroid::~DataFetcherImplAndroid() {
question: the destructor will never be called since this is relying on
CR_DEFINE_STATIC_LOCAL, right? should this be NOTREACHED() ?

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.cc:169:
DCHECK(NeedsPolling());
would NOTREACHED(); be better?

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.cc:175: +
received_motion_data_[DATA_ACCELERATION_INCLUDING_GRAVITY]
nit: + and == always at the end of the previous line

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.cc:192:
memset(received_motion_data_, 0, sizeof(received_motion_data_));
192-194 is the same as 207-209, perhaps "ClearInternalBuffers()" would help?

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_impl_android.h (right):

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.h:55: //
Implementation of the new API using shared memory.
nit: just the name the base class, "new" will get "old" :)

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.h:57: virtual bool
NeedsPolling() { return false; }
nit: better move the implementation to the .cc file.

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.h:79:
DATA_ACCELERATION = 0,
nit: I guess this would be clearer as RECEIVED_MOTION_DATA_...

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_impl_android_unittest.cc
(right):

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android_unittest.cc:22: int
rate_in_milliseconds) OVERRIDE {
nit: align the "int" with the ( above

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android_unittest.cc:30:
void SetNumberActiveDeviceMotionSensors(int numberActiveSensors) {
nit: param name, number_active_sensors

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android_unittest.cc:39:
public:
nit: can remove the "public" label here..

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_shared_memory.h (right):

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_shared_memory.h:24: // If this
fetcher needsPolling() is true, this method will update the
nit: s/needs/Needs/ here and below..

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_shared_memory.h:38: virtual
~DataFetcherSharedMemory();
since the only way to get one object is via "instance()", the destructor will
never be called, and no callers should try to destroy one, right? in that case,
make this destructor private?

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_shared_memory.h:42: 
nit: private:
(data members and the DISALLOW_* macros can't be protected)

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_shared_memory_default.cc
(right):

https://codereview.chromium.org/18572014/diff/24001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_shared_memory_default.cc:25:
DCHECK(NeedsPolling());
nit: NOTREACHED() ?

[timvolodine, 2013-07-09 18:59:56]

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
File content/browser/device_orientation/data_fetcher_impl_android.cc (right):

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android.cc:54:
DataFetcherImplAndroid::~DataFetcherImplAndroid() {
On 2013/07/09 09:03:29, bulach wrote:
> question: the destructor will never be called since this is relying on
> CR_DEFINE_STATIC_LOCAL, right? should this be NOTREACHED() ?

hmm, very good point. This is actually a bug.

The CR_DEFINE_STATIC_LOCAL apparently uses new to allocate on heap, so the
destructor is never called. To add to the complexity the provider_impl.cc (for
device orientation) does not know that the fetcher is a singleton now, and tries
to destroy the fetcher because it thinks it owns it in a scoped_ptr :/.
I've modified provider_impl.cc slightly and also added instance to all ports
(mac and windows). Also added some code to properly shutdown sensors on exit.

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android.cc:169:
DCHECK(NeedsPolling());
On 2013/07/09 09:03:29, bulach wrote:
> would NOTREACHED(); be better?

Done.

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android.cc:175: +
received_motion_data_[DATA_ACCELERATION_INCLUDING_GRAVITY]
On 2013/07/09 09:03:29, bulach wrote:
> nit: + and == always at the end of the previous line

Done.

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android.cc:192:
memset(received_motion_data_, 0, sizeof(received_motion_data_));
On 2013/07/09 09:03:29, bulach wrote:
> 192-194 is the same as 207-209, perhaps "ClearInternalBuffers()" would help?

Done.

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
File content/browser/device_orientation/data_fetcher_impl_android.h (right):

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android.h:55: //
Implementation of the new API using shared memory.
On 2013/07/09 09:03:29, bulach wrote:
> nit: just the name the base class, "new" will get "old" :)

Done.

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android.h:57: virtual bool
NeedsPolling() { return false; }
On 2013/07/09 09:03:29, bulach wrote:
> nit: better move the implementation to the .cc file.

Done.

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android.h:79:
DATA_ACCELERATION = 0,
On 2013/07/09 09:03:29, bulach wrote:
> nit: I guess this would be clearer as RECEIVED_MOTION_DATA_...

Done.

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
File content/browser/device_orientation/data_fetcher_impl_android_unittest.cc
(right):

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android_unittest.cc:22: int
rate_in_milliseconds) OVERRIDE {
On 2013/07/09 09:03:29, bulach wrote:
> nit: align the "int" with the ( above

Done.

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android_unittest.cc:30:
void SetNumberActiveDeviceMotionSensors(int numberActiveSensors) {
On 2013/07/09 09:03:29, bulach wrote:
> nit: param name, number_active_sensors

Done.

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android_unittest.cc:39:
public:
On 2013/07/09 09:03:29, bulach wrote:
> nit: can remove the "public" label here..

Done.

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
File content/browser/device_orientation/data_fetcher_shared_memory.h (right):

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
content/browser/device_orientation/data_fetcher_shared_memory.h:24: // If this
fetcher needsPolling() is true, this method will update the
On 2013/07/09 09:03:29, bulach wrote:
> nit: s/needs/Needs/ here and below..

Done.

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
content/browser/device_orientation/data_fetcher_shared_memory.h:38: virtual
~DataFetcherSharedMemory();
On 2013/07/09 09:03:29, bulach wrote:
> since the only way to get one object is via "instance()", the destructor will
> never be called, and no callers should try to destroy one, right? in that
case,
> make this destructor private?

Done.

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
content/browser/device_orientation/data_fetcher_shared_memory.h:42: 
On 2013/07/09 09:03:29, bulach wrote:
> nit: private:
> (data members and the DISALLOW_* macros can't be protected)

Done.

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
File content/browser/device_orientation/data_fetcher_shared_memory_default.cc
(right):

https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/de...
content/browser/device_orientation/data_fetcher_shared_memory_default.cc:25:
DCHECK(NeedsPolling());
On 2013/07/09 09:03:29, bulach wrote:
> nit: NOTREACHED() ?

Done.

[bulach, 2013-07-10 16:59:41]

thanks tim! this patch is highlighting a few more issues with the existing code
in terms of ownership of the fetcher.. :-/ whilst this is a good thing, I think
it'd be better for us to split this, fix the ownership model, then have this
patch just implementing the Device Motion itself? more inline, please let me
know your thoughts:

https://codereview.chromium.org/18572014/diff/37001/content/browser/browser_m...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/18572014/diff/37001/content/browser/browser_m...
content/browser/browser_main_loop.cc:469:
DataFetcherImplAndroid::Register(base::android::AttachCurrentThread());
sorry, I just noticed this is in the wrong place :)
this registration should be done here instead:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/an...

https://codereview.chromium.org/18572014/diff/37001/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_impl_android.cc (right):

https://codereview.chromium.org/18572014/diff/37001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.cc:165: // -----
New shared memory API methods
s/New// :) it won't be new for too long..

https://codereview.chromium.org/18572014/diff/37001/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_impl_android.h (right):

https://codereview.chromium.org/18572014/diff/37001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.h:15:
template<typename T> struct DefaultSingletonTraits;
nit: LeakySingletonTraits

https://codereview.chromium.org/18572014/diff/37001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.h:40: // Needs to
be thread-safe, because accessed from a thread in provider_impl.cc
nit: s/a thread/different threads/

https://codereview.chromium.org/18572014/diff/37001/content/browser/device_or...
File content/browser/device_orientation/provider_impl.cc (right):

https://codereview.chromium.org/18572014/diff/37001/content/browser/device_or...
content/browser/device_orientation/provider_impl.cc:78: #if defined(OS_ANDROID)
// TODO(timvolodine): Remove this platform dependency once
// orientation also switches to shared memory implementation.

https://codereview.chromium.org/18572014/diff/37001/content/browser/device_or...
content/browser/device_orientation/provider_impl.cc:101: DataFetcher* fetcher =
factory();
hmm... factory() normally means it'll create an object, and the caller takes
ownership.. in this case, it'd be better as something like
GetFetcherInstance()... all Fetchers would have to change, since their "Create"
methods and this factory don't make much sense anymore...

however, thinking more about this:
it seems we're completely changing the ownership model of the fetcher.
it'd probably be better to do all of that in a separate patch, then this one can
focus only on implementing the Android data fetcher... wdyt?

https://codereview.chromium.org/18572014/diff/37001/content/browser/device_or...
content/browser/device_orientation/provider_impl.cc:106: // Pass ownership of
fetcher to provider_.
remove comment?

[timvolodine, 2013-07-11 14:31:34]

thanks Marcus! replies below.

https://chromiumcodereview.appspot.com/18572014/diff/37001/content/browser/br...
File content/browser/browser_main_loop.cc (right):

https://chromiumcodereview.appspot.com/18572014/diff/37001/content/browser/br...
content/browser/browser_main_loop.cc:469:
DataFetcherImplAndroid::Register(base::android::AttachCurrentThread());
On 2013/07/10 16:59:41, bulach wrote:
> sorry, I just noticed this is in the wrong place :)
> this registration should be done here instead:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/an...

ok, I've pulled out this change into a separate CL, think would be easier to get
it reviewed separately.

https://chromiumcodereview.appspot.com/18572014/diff/37001/content/browser/de...
File content/browser/device_orientation/data_fetcher_impl_android.cc (right):

https://chromiumcodereview.appspot.com/18572014/diff/37001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android.cc:165: // -----
New shared memory API methods
On 2013/07/10 16:59:41, bulach wrote:
> s/New// :) it won't be new for too long..

Done.

https://chromiumcodereview.appspot.com/18572014/diff/37001/content/browser/de...
File content/browser/device_orientation/data_fetcher_impl_android.h (right):

https://chromiumcodereview.appspot.com/18572014/diff/37001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android.h:15:
template<typename T> struct DefaultSingletonTraits;
On 2013/07/10 16:59:41, bulach wrote:
> nit: LeakySingletonTraits

apparently this needs to be DefaultSingletonTraits, cannot get it to compile
otherwise.

https://chromiumcodereview.appspot.com/18572014/diff/37001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android.h:40: // Needs to
be thread-safe, because accessed from a thread in provider_impl.cc
On 2013/07/10 16:59:41, bulach wrote:
> nit: s/a thread/different threads/

Done.

https://chromiumcodereview.appspot.com/18572014/diff/37001/content/browser/de...
File content/browser/device_orientation/provider_impl.cc (right):

https://chromiumcodereview.appspot.com/18572014/diff/37001/content/browser/de...
content/browser/device_orientation/provider_impl.cc:78: #if defined(OS_ANDROID)
On 2013/07/10 16:59:41, bulach wrote:
> // TODO(timvolodine): Remove this platform dependency once
> // orientation also switches to shared memory implementation.

N/A anymore.
Done.

https://chromiumcodereview.appspot.com/18572014/diff/37001/content/browser/de...
content/browser/device_orientation/provider_impl.cc:101: DataFetcher* fetcher =
factory();
On 2013/07/10 16:59:41, bulach wrote:
> hmm... factory() normally means it'll create an object, and the caller takes
> ownership.. in this case, it'd be better as something like
> GetFetcherInstance()... all Fetchers would have to change, since their
"Create"
> methods and this factory don't make much sense anymore...
> 
> however, thinking more about this:
> it seems we're completely changing the ownership model of the fetcher.
> it'd probably be better to do all of that in a separate patch, then this one
can
> focus only on implementing the Android data fetcher... wdyt?

Sure. I've refactored the code and added a new class for handling the device
orientation implementation in a transparent way, as discussed yesterday. The
other fetchers and the device orientation provider do not have to change in this
case.

https://chromiumcodereview.appspot.com/18572014/diff/37001/content/browser/de...
content/browser/device_orientation/provider_impl.cc:106: // Pass ownership of
fetcher to provider_.
On 2013/07/10 16:59:41, bulach wrote:
> remove comment?

N/A anymore.
Done.

[bulach, 2013-07-12 14:00:41]

thanks tim! a few questions below, let me know your thoughts:

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_impl_android.cc (right):

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.cc:7: #include
<string.h>
nit: \n

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_impl_android.h (right):

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.h:15:
template<typename T> struct DefaultSingletonTraits;
I don't quite understand why the Leaky version wouldn't compile?

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.h:30: // shared
memory implementation.
would this be the case? I think this class will remain as is to bridge from
C/C++ to java, right?

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.h:54: // once it is
implemented.
I think 52-54 can be dropped? :)

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.h:63: virtual void
StopFetchingDeviceMotionData();
hmm... is "NeedsPolling()" necessary, since this class is no longer an
interface? also, make all of these non-virtual?

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_orientation_android.cc
(right):

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
content/browser/device_orientation/data_fetcher_orientation_android.cc:24:
return fetcher.release();
nit: .pass() once it return scoped_ptr

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_orientation_android.h
(right):

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
content/browser/device_orientation/data_fetcher_orientation_android.h:17: static
DataFetcher* Create();
return a scoped_ptr<DataFecther>

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_shared_memory_android.cc
(right):

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
content/browser/device_orientation/data_fetcher_shared_memory_android.cc:17:
return DataFetcherImplAndroid::GetInstance()->NeedsPolling();
nit: I think this can return false directly..

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
content/browser/device_orientation/data_fetcher_shared_memory_android.cc:26:
DeviceMotionHardwareBuffer* buffer) {
nit: indent +2

[timvolodine, 2013-07-12 15:22:17]

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_impl_android.cc (right):

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.cc:7: #include
<string.h>
On 2013/07/12 14:00:41, bulach wrote:
> nit: \n

Done.

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_impl_android.h (right):

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.h:15:
template<typename T> struct DefaultSingletonTraits;
On 2013/07/12 14:00:41, bulach wrote:
> I don't quite understand why the Leaky version wouldn't compile?

If I change this to Leaky the below friend declaration gives an error that it is
not defined. If I put Leaky in the friend declaration, the compiler will
complain that the destructor/constructor of fetcher is protected. I think this
is because Leaky is a subclass of Default, so Default needs to be friend of
fetcher to have access to constructor/destructor of fetcher.

In chromium some files just include singleton.h in the header to avoid this
template<> declaration.

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.h:30: // shared
memory implementation.
On 2013/07/12 14:00:41, bulach wrote:
> would this be the case? I think this class will remain as is to bridge from
> C/C++ to java, right?

yes, but the class can be simplified once device orientation switches to shared
memory approach. I've added a comment to reflect that.

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.h:54: // once it is
implemented.
On 2013/07/12 14:00:41, bulach wrote:
> I think 52-54 can be dropped? :)

Done.

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.h:63: virtual void
StopFetchingDeviceMotionData();
On 2013/07/12 14:00:41, bulach wrote:
> hmm... is "NeedsPolling()" necessary, since this class is no longer an
> interface? also, make all of these non-virtual?

Done.

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_orientation_android.cc
(right):

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
content/browser/device_orientation/data_fetcher_orientation_android.cc:24:
return fetcher.release();
On 2013/07/12 14:00:41, bulach wrote:
> nit: .pass() once it return scoped_ptr

let's handle this in a separate CL, because I think this change will impact some
refactoring in provider_impl.

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_orientation_android.h
(right):

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
content/browser/device_orientation/data_fetcher_orientation_android.h:17: static
DataFetcher* Create();
On 2013/07/12 14:00:41, bulach wrote:
> return a scoped_ptr<DataFecther>

provider_impl.cc uses a raw pointer (which is later scoped). I would probably
need to change other fetcher as well, which I think goes beyond this CL..

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_shared_memory_android.cc
(right):

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
content/browser/device_orientation/data_fetcher_shared_memory_android.cc:17:
return DataFetcherImplAndroid::GetInstance()->NeedsPolling();
On 2013/07/12 14:00:41, bulach wrote:
> nit: I think this can return false directly..

Done.

https://codereview.chromium.org/18572014/diff/23003/content/browser/device_or...
content/browser/device_orientation/data_fetcher_shared_memory_android.cc:26:
DeviceMotionHardwareBuffer* buffer) {
On 2013/07/12 14:00:41, bulach wrote:
> nit: indent +2

Done.

[bulach, 2013-07-12 16:07:43]

lgtm, thanks tim! just some nits and suggestions for the follow up..

https://codereview.chromium.org/18572014/diff/59001/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_impl_android.h (right):

https://codereview.chromium.org/18572014/diff/59001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android.h:28: // once
Device Orientation switches to shared memory implementation.
thanks! just as an idea for the follow up: this class and file should probably
be renamed to SensorManagerAndroid.. DataFetcher is now an entirely separate
concept.

https://codereview.chromium.org/18572014/diff/59001/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_impl_android_unittest.cc
(right):

https://codereview.chromium.org/18572014/diff/59001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android_unittest.cc:9:
#include "content/browser/device_orientation/data_fetcher_impl_android.h"
nit: I just learned the file under test should be in line 5 followed by another
\n, just like the non-unittest file does.

https://codereview.chromium.org/18572014/diff/59001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android_unittest.cc:16:
class MockDataFetcherImplAndroid : public DataFetcherImplAndroid {
nit: this should be named as "Fake" instead of "Mock".

see more in gmock documentation:
"""
Note: It is easy to confuse the term fake objects with mock objects. Fakes and
mocks actually mean very different things in the Test-Driven Development (TDD)
community:

Fake objects have working implementations, but usually take some shortcut
(perhaps to make the operations less expensive), which makes them not suitable
for production. An in-memory file system would be an example of a fake.
Mocks are objects pre-programmed with expectations, which form a specification
of the calls they are expected to receive.
"""

https://codereview.chromium.org/18572014/diff/59001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_impl_android_unittest.cc:22: int
rate_in_milliseconds) OVERRIDE {
nit: indent

https://codereview.chromium.org/18572014/diff/59001/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_shared_memory.h (right):

https://codereview.chromium.org/18572014/diff/59001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_shared_memory.h:41:
DeviceMotionHardwareBuffer* device_motion_buffer_;
I think this is no longer needed here?

https://codereview.chromium.org/18572014/diff/59001/content/browser/device_or...
File content/browser/device_orientation/data_fetcher_shared_memory_android.cc
(right):

https://codereview.chromium.org/18572014/diff/59001/content/browser/device_or...
content/browser/device_orientation/data_fetcher_shared_memory_android.cc:27:
device_motion_buffer_ = buffer;
nit: as above..

[timvolodine, 2013-07-12 17:43:56]

thanks Marcus!

https://chromiumcodereview.appspot.com/18572014/diff/59001/content/browser/de...
File content/browser/device_orientation/data_fetcher_impl_android.h (right):

https://chromiumcodereview.appspot.com/18572014/diff/59001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android.h:28: // once
Device Orientation switches to shared memory implementation.
On 2013/07/12 16:07:43, bulach wrote:
> thanks! just as an idea for the follow up: this class and file should probably
> be renamed to SensorManagerAndroid.. DataFetcher is now an entirely separate
> concept.

ack, good idea.

https://chromiumcodereview.appspot.com/18572014/diff/59001/content/browser/de...
File content/browser/device_orientation/data_fetcher_impl_android_unittest.cc
(right):

https://chromiumcodereview.appspot.com/18572014/diff/59001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android_unittest.cc:9:
#include "content/browser/device_orientation/data_fetcher_impl_android.h"
On 2013/07/12 16:07:43, bulach wrote:
> nit: I just learned the file under test should be in line 5 followed by
another
> \n, just like the non-unittest file does.

Done.

https://chromiumcodereview.appspot.com/18572014/diff/59001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android_unittest.cc:16:
class MockDataFetcherImplAndroid : public DataFetcherImplAndroid {
On 2013/07/12 16:07:43, bulach wrote:
> nit: this should be named as "Fake" instead of "Mock".
> 
> see more in gmock documentation:
> """
> Note: It is easy to confuse the term fake objects with mock objects. Fakes and
> mocks actually mean very different things in the Test-Driven Development (TDD)
> community:
> 
> Fake objects have working implementations, but usually take some shortcut
> (perhaps to make the operations less expensive), which makes them not suitable
> for production. An in-memory file system would be an example of a fake.
> Mocks are objects pre-programmed with expectations, which form a specification
> of the calls they are expected to receive.
> """

Done.

https://chromiumcodereview.appspot.com/18572014/diff/59001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android_unittest.cc:22: int
rate_in_milliseconds) OVERRIDE {
On 2013/07/12 16:07:43, bulach wrote:
> nit: indent

Done.

https://chromiumcodereview.appspot.com/18572014/diff/59001/content/browser/de...
File content/browser/device_orientation/data_fetcher_shared_memory.h (right):

https://chromiumcodereview.appspot.com/18572014/diff/59001/content/browser/de...
content/browser/device_orientation/data_fetcher_shared_memory.h:41:
DeviceMotionHardwareBuffer* device_motion_buffer_;
On 2013/07/12 16:07:43, bulach wrote:
> I think this is no longer needed here?

we still actually need it in data_fetcher_shared_memory_default...

https://chromiumcodereview.appspot.com/18572014/diff/59001/content/browser/de...
File content/browser/device_orientation/data_fetcher_shared_memory_android.cc
(right):

https://chromiumcodereview.appspot.com/18572014/diff/59001/content/browser/de...
content/browser/device_orientation/data_fetcher_shared_memory_android.cc:27:
device_motion_buffer_ = buffer;
On 2013/07/12 16:07:43, bulach wrote:
> nit: as above..

here it's not really used but it is still needed in
data_fetcher_shared_memory_default. This is due to temporary nature of having
device motion using shared memory and device orientation working in the old way.

[timvolodine, 2013-07-12 17:45:33]

extra approval needed for
content/content_browser.gypi,
content/content_tests/gypi

[piman, 2013-07-12 18:06:45]

LGTM+nit

https://chromiumcodereview.appspot.com/18572014/diff/67001/content/browser/de...
File content/browser/device_orientation/data_fetcher_impl_android.h (right):

https://chromiumcodereview.appspot.com/18572014/diff/67001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android.h:87: int
number_active_device_motion_sensors_;
new field needs initialization.

[timvolodine, 2013-07-12 18:33:45]

thanks!

https://chromiumcodereview.appspot.com/18572014/diff/67001/content/browser/de...
File content/browser/device_orientation/data_fetcher_impl_android.h (right):

https://chromiumcodereview.appspot.com/18572014/diff/67001/content/browser/de...
content/browser/device_orientation/data_fetcher_impl_android.h:87: int
number_active_device_motion_sensors_;
On 2013/07/12 18:06:45, piman wrote:
> new field needs initialization.

Done.

[commit-bot: I haz the power, 2013-07-12 19:57:54]

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

[commit-bot: I haz the power, 2013-07-12 21:26:01]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

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

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

[commit-bot: I haz the power, 2013-07-13 04:23:09]

Change committed as 211539