Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12771008:
Implement java-side browser sensor polling for device motion and device orientation DOM events. (Closed)
|
Created:
7 years, 9 months ago by timvolodine Modified:
7 years, 8 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionImplement the java-side browser sensor polling for device motion and device orientation DOM events.
BUG=135804
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190639
Patch Set 1 #
Total comments: 50
Patch Set 2 : taking into account the comments #
Total comments: 24
Patch Set 3 : fixing diff and comments #
Total comments: 85
Patch Set 4 : fixed comments #
Total comments: 54
Patch Set 5 : fixed comments #
Total comments: 16
Patch Set 6 : yet another patch #Patch Set 7 : added tests and mocks #
Total comments: 37
Patch Set 8 : fixed comments and trybots #
Total comments: 16
Patch Set 9 : fixed Marcus's comments #
Total comments: 2
Messages
Total messages: 37 (0 generated)
Some initial comments. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... File content/browser/device_orientation/data_fetcher_impl_android.cc (right): https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:10: #include "jni/DeviceMotionAndOrientation_jni.h" What's happening to the DeviceOrientation.java file? You added a new file, but didn't remove this file. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:22: // constants used for JNI calls to java Please use proper sentences and grammar. As previously commented, I'd prefer to expose this in a header given that external callers will need it. It's not platform specific either, so it should (eventually?) end up in the service's file/class. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:88: // TODO: copy into shared memory buffer This doesn't add a lot. Since it shouldn't be called yet, just add NOTIMPLEMENTED()? https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:93: } Dito to line 88, please add the NOTIMPLEMENTED() everywhere for clarity. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:99: // TODO: remove this method later, this is for compatibility now nit: "// TODO: Remove this method when all callers supply the required event type."? https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:102: //return Start(DEVICE_MOTION, rate_in_milliseconds); nit: I'd prefer not to commit commented out code. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:115: // TODO: possibly remove this method, or make it stop everything In which case would we want to do that? https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... File content/browser/device_orientation/data_fetcher_impl_android.h (right): https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.h:41: void GotAcceleration(JNIEnv*, jobject, For clarity, I'd prefer to either remove the empty lines between these four methods *or* add the "Called from Java via JNI." comment above all of them, given that it applies to all. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.h:42: double x, double y, double z); nit: indent should be equal to JNIEnv* on line 41. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.h:45: double x, double y, double z); nit: indent should be equal to JNIEnv* on line 45. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.h:48: double alpha, double beta, double gamma); nit: indent should be equal to JNIEnv* on line 47. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.h:59: bool Start(int spec_event_type, int rate_in_milliseconds); I'm not fond of having an int as the parameter, it's unreadable. Can we add an enum to represent which event type should start polling? https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.h:59: bool Start(int spec_event_type, int rate_in_milliseconds); naming nit: I don't like "spec_event_type", as the "spec_" prefix doesn't add any value. When we have an enum, maybe just "event_type"? https://codereview.chromium.org/12771008/diff/1/content/content_jni.gypi File content/content_jni.gypi (right): https://codereview.chromium.org/12771008/diff/1/content/content_jni.gypi#newc... content/content_jni.gypi:21: 'public/android/java/src/org/chromium/content/browser/DeviceOrientation.java', We're not using this file anymore now, are we? https://codereview.chromium.org/12771008/diff/1/content/public/android/java/s... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. It's impossible to review changes in this file given that everything shows up as new. Isn't there a way to get rietveld to recognize it's a copy, and thereby only show changes?
Some comments from my side, very inline with Peter's https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... File content/browser/device_orientation/data_fetcher_impl_android.cc (right): https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:10: #include "jni/DeviceMotionAndOrientation_jni.h" yes plese mv oldName newName, that way you remove it and as a nice side effects the right diffs will show up in retrieveid making it way easier to review. On 2013/03/11 17:57:25, Peter Beverloo wrote: > What's happening to the DeviceOrientation.java file? You added a new file, but > didn't remove this file. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:24: enum DeviceMotionOrientationEvents { This enum should probably be public, you've worked around this by using an int in the Start/Stop methods but that is not very elegant. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:88: // TODO: copy into shared memory buffer Please add an owner to the TODO. I think it should at least log something. Alternatively you could provide a mock implementation and use it in tests. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:102: //return Start(DEVICE_MOTION, rate_in_milliseconds); +1 On 2013/03/11 17:57:25, Peter Beverloo wrote: > nit: I'd prefer not to commit commented out code. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:105: bool DataFetcherImplAndroid::Start(int spec_event_type, It does not really feel we need both Start methods, in fact it is going to be confusing. Can't you just change the callers? If you cannot afford to do that because there are two many please add a TODO and do it in a separate CL. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:115: // TODO: possibly remove this method, or make it stop everything On 2013/03/11 17:57:25, Peter Beverloo wrote: > In which case would we want to do that? I'd remove it. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:119: //Stop(DEVICE_MOTION); commented line... https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... File content/browser/device_orientation/data_fetcher_impl_android.h (right): https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.h:59: bool Start(int spec_event_type, int rate_in_milliseconds); Yeah let's with an enum on C++ on Java we should keep int constants since enums are expensive. On 2013/03/11 17:57:25, Peter Beverloo wrote: > I'm not fond of having an int as the parameter, it's unreadable. Can we add an > enum to represent which event type should start polling? https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.h:61: void Stop(); I am not too fond on method overrides, but if deemed necessary please add a big bunch of documentation explaining both cases. https://codereview.chromium.org/12771008/diff/1/content/content_jni.gypi File content/content_jni.gypi (right): https://codereview.chromium.org/12771008/diff/1/content/content_jni.gypi#newc... content/content_jni.gypi:21: 'public/android/java/src/org/chromium/content/browser/DeviceOrientation.java', On 2013/03/11 17:57:25, Peter Beverloo wrote: > We're not using this file anymore now, are we? +1
thanks for the comments, uploaded a new patch taking them into account https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... File content/browser/device_orientation/data_fetcher_impl_android.cc (right): https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:10: #include "jni/DeviceMotionAndOrientation_jni.h" On 2013/03/11 17:57:25, Peter Beverloo wrote: > What's happening to the DeviceOrientation.java file? You added a new file, but > didn't remove this file. Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:10: #include "jni/DeviceMotionAndOrientation_jni.h" On 2013/03/11 18:25:50, Miguel Garcia wrote: > yes plese mv oldName newName, that way you remove it and as a nice side effects > the right diffs will show up in retrieveid making it way easier to review. > > On 2013/03/11 17:57:25, Peter Beverloo wrote: > > What's happening to the DeviceOrientation.java file? You added a new file, > but > > didn't remove this file. > Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:22: // constants used for JNI calls to java actually reusing the DeviceData::Type enum now to reduce code duplication On 2013/03/11 17:57:25, Peter Beverloo wrote: > Please use proper sentences and grammar. As previously commented, I'd prefer to > expose this in a header given that external callers will need it. It's not > platform specific either, so it should (eventually?) end up in the service's > file/class. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:24: enum DeviceMotionOrientationEvents { On 2013/03/11 18:25:50, Miguel Garcia wrote: > This enum should probably be public, you've worked around this by using an int > in the Start/Stop methods but that is not very elegant. Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:88: // TODO: copy into shared memory buffer On 2013/03/11 17:57:25, Peter Beverloo wrote: > This doesn't add a lot. Since it shouldn't be called yet, just add > NOTIMPLEMENTED()? Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:88: // TODO: copy into shared memory buffer On 2013/03/11 18:25:50, Miguel Garcia wrote: > Please add an owner to the TODO. I think it should at least log something. > Alternatively you could provide a mock implementation and use it in tests. Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:93: } On 2013/03/11 17:57:25, Peter Beverloo wrote: > Dito to line 88, please add the NOTIMPLEMENTED() everywhere for clarity. Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:99: // TODO: remove this method later, this is for compatibility now On 2013/03/11 17:57:25, Peter Beverloo wrote: > nit: "// TODO: Remove this method when all callers supply the required event > type."? Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:102: //return Start(DEVICE_MOTION, rate_in_milliseconds); On 2013/03/11 17:57:25, Peter Beverloo wrote: > nit: I'd prefer not to commit commented out code. Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:102: //return Start(DEVICE_MOTION, rate_in_milliseconds); On 2013/03/11 18:25:50, Miguel Garcia wrote: > +1 > On 2013/03/11 17:57:25, Peter Beverloo wrote: > > nit: I'd prefer not to commit commented out code. > Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:105: bool DataFetcherImplAndroid::Start(int spec_event_type, On 2013/03/11 18:25:50, Miguel Garcia wrote: > It does not really feel we need both Start methods, in fact it is going to be > confusing. Can't you just change the callers? > > If you cannot afford to do that because there are two many please add a TODO and > do it in a separate CL. Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:115: // TODO: possibly remove this method, or make it stop everything On 2013/03/11 18:25:50, Miguel Garcia wrote: > On 2013/03/11 17:57:25, Peter Beverloo wrote: > > In which case would we want to do that? > I'd remove it. Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:115: // TODO: possibly remove this method, or make it stop everything potentially in the destructor.. On 2013/03/11 17:57:25, Peter Beverloo wrote: > In which case would we want to do that? https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.cc:119: //Stop(DEVICE_MOTION); On 2013/03/11 18:25:50, Miguel Garcia wrote: > commented line... Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... File content/browser/device_orientation/data_fetcher_impl_android.h (right): https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.h:41: void GotAcceleration(JNIEnv*, jobject, On 2013/03/11 17:57:25, Peter Beverloo wrote: > For clarity, I'd prefer to either remove the empty lines between these four > methods *or* add the "Called from Java via JNI." comment above all of them, > given that it applies to all. Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.h:42: double x, double y, double z); On 2013/03/11 17:57:25, Peter Beverloo wrote: > nit: indent should be equal to JNIEnv* on line 41. Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.h:45: double x, double y, double z); On 2013/03/11 17:57:25, Peter Beverloo wrote: > nit: indent should be equal to JNIEnv* on line 45. Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.h:48: double alpha, double beta, double gamma); On 2013/03/11 17:57:25, Peter Beverloo wrote: > nit: indent should be equal to JNIEnv* on line 47. Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.h:59: bool Start(int spec_event_type, int rate_in_milliseconds); On 2013/03/11 17:57:25, Peter Beverloo wrote: > I'm not fond of having an int as the parameter, it's unreadable. Can we add an > enum to represent which event type should start polling? Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.h:59: bool Start(int spec_event_type, int rate_in_milliseconds); On 2013/03/11 17:57:25, Peter Beverloo wrote: > naming nit: I don't like "spec_event_type", as the "spec_" prefix doesn't add > any value. When we have an enum, maybe just "event_type"? Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.h:59: bool Start(int spec_event_type, int rate_in_milliseconds); On 2013/03/11 18:25:50, Miguel Garcia wrote: > Yeah let's with an enum on C++ on Java we should keep int constants since enums > are expensive. > > On 2013/03/11 17:57:25, Peter Beverloo wrote: > > I'm not fond of having an int as the parameter, it's unreadable. Can we add an > > enum to represent which event type should start polling? > Done. https://codereview.chromium.org/12771008/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_android.h:61: void Stop(); On 2013/03/11 18:25:50, Miguel Garcia wrote: > I am not too fond on method overrides, but if deemed necessary please add a big > bunch of documentation explaining both cases. Done. https://codereview.chromium.org/12771008/diff/1/content/content_jni.gypi File content/content_jni.gypi (right): https://codereview.chromium.org/12771008/diff/1/content/content_jni.gypi#newc... content/content_jni.gypi:21: 'public/android/java/src/org/chromium/content/browser/DeviceOrientation.java', On 2013/03/11 17:57:25, Peter Beverloo wrote: > We're not using this file anymore now, are we? Done. https://codereview.chromium.org/12771008/diff/1/content/content_jni.gypi#newc... content/content_jni.gypi:21: 'public/android/java/src/org/chromium/content/browser/DeviceOrientation.java', On 2013/03/11 18:25:50, Miguel Garcia wrote: > On 2013/03/11 17:57:25, Peter Beverloo wrote: > > We're not using this file anymore now, are we? > > +1 Done. https://codereview.chromium.org/12771008/diff/1/content/public/android/java/s... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/03/11 17:57:25, Peter Beverloo wrote: > It's impossible to review changes in this file given that everything shows up as > new. Isn't there a way to get rietveld to recognize it's a copy, and thereby > only show changes? Done.
https://codereview.chromium.org/12771008/diff/9001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/9001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. I did a mv old new, but still no proper diff with the old DeviceOrientation.java, trying to figure out how to do this in rietveld.
https://codereview.chromium.org/12771008/diff/9001/content/browser/device_ori... File content/browser/device_orientation/data_fetcher_impl_android.cc (right): https://codereview.chromium.org/12771008/diff/9001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_android.cc:46: Stop(DeviceData::kTypeOrientation); nit: add a TODO to cover that this should shut down the activated data types, not just orientation. https://codereview.chromium.org/12771008/diff/9001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_android.cc:83: // TODO(timvolodine): copy into shared memory buffer It's fine to get rid of the TODO now that there's a NOTIMPLEMENTED() call. https://codereview.chromium.org/12771008/diff/9001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_android.cc:103: event_type, nit: don't we need a cast here for strictness? Please run the try-bots :-). https://codereview.chromium.org/12771008/diff/9001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_android.cc:111: event_type); nit: dito. https://codereview.chromium.org/12771008/diff/9001/content/browser/device_ori... File content/browser/device_orientation/data_fetcher_impl_android.h (right): https://codereview.chromium.org/12771008/diff/9001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_android.h:55: // bool Start(int rate_in_milliseconds); Please don't include commented out code. https://codereview.chromium.org/12771008/diff/9001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_android.h:56: bool Start(DeviceData::Type event_type, int rate_in_milliseconds); While device data definitely is not the correct place to put this enum (given that we'll be getting rid of it), since the service does not exist yet I guess this would be ok. Nonetheless, please add a TODO comment to clarify that this needs to be moved to the service once it's there. https://codereview.chromium.org/12771008/diff/9001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_android.h:58: // void Stop(); Dito. https://codereview.chromium.org/12771008/diff/9001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/9001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Ok. This still is unreviewable unfortunately.. You can ask around in #chromium if you can't find a way.
https://codereview.chromium.org/12771008/diff/9001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/9001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. I agree, I am adding some comments but please send us the diff in some way (if needed just put both files some where so we can tkdiff them) On 2013/03/12 11:26:39, Peter Beverloo wrote: > Ok. This still is unreviewable unfortunately.. You can ask around in #chromium > if you can't find a way. https://codereview.chromium.org/12771008/diff/9001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. copyright 2013 https://codereview.chromium.org/12771008/diff/9001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:70: private static enum EventType { no enums in Java please, use int constants instead https://codereview.chromium.org/12771008/diff/9001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:71: DEVICE_ORIENTATION(0), also the C++ version has a TEST element as well
incorporated new comments and uploaded a new patch with the correct diff for the java class https://codereview.chromium.org/12771008/diff/9001/content/browser/device_ori... File content/browser/device_orientation/data_fetcher_impl_android.cc (right): https://codereview.chromium.org/12771008/diff/9001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_android.cc:46: Stop(DeviceData::kTypeOrientation); On 2013/03/12 11:26:39, Peter Beverloo wrote: > nit: add a TODO to cover that this should shut down the activated data types, > not just orientation. Done. https://codereview.chromium.org/12771008/diff/9001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_android.cc:83: // TODO(timvolodine): copy into shared memory buffer On 2013/03/12 11:26:39, Peter Beverloo wrote: > It's fine to get rid of the TODO now that there's a NOTIMPLEMENTED() call. Done. https://codereview.chromium.org/12771008/diff/9001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_android.cc:103: event_type, rate_in_milliseconds below does not use any casts, so I was originally basing on that.. will check with the bots On 2013/03/12 11:26:39, Peter Beverloo wrote: > nit: don't we need a cast here for strictness? Please run the try-bots :-). https://codereview.chromium.org/12771008/diff/9001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_android.cc:111: event_type); On 2013/03/12 11:26:39, Peter Beverloo wrote: > nit: dito. Done. https://codereview.chromium.org/12771008/diff/9001/content/browser/device_ori... File content/browser/device_orientation/data_fetcher_impl_android.h (right): https://codereview.chromium.org/12771008/diff/9001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_android.h:55: // bool Start(int rate_in_milliseconds); On 2013/03/12 11:26:39, Peter Beverloo wrote: > Please don't include commented out code. Done. https://codereview.chromium.org/12771008/diff/9001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_android.h:56: bool Start(DeviceData::Type event_type, int rate_in_milliseconds); On 2013/03/12 11:26:39, Peter Beverloo wrote: > While device data definitely is not the correct place to put this enum (given > that we'll be getting rid of it), since the service does not exist yet I guess > this would be ok. Nonetheless, please add a TODO comment to clarify that this > needs to be moved to the service once it's there. Done. https://codereview.chromium.org/12771008/diff/9001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_android.h:58: // void Stop(); On 2013/03/12 11:26:39, Peter Beverloo wrote: > Dito. Done. https://codereview.chromium.org/12771008/diff/9001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/9001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/03/12 11:26:39, Peter Beverloo wrote: > Ok. This still is unreviewable unfortunately.. You can ask around in #chromium > if you can't find a way. Done. https://codereview.chromium.org/12771008/diff/9001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/03/12 11:40:51, Miguel Garcia wrote: > I agree, I am adding some comments but please send us the diff in some way (if > needed just put both files some where so we can tkdiff them) > > > On 2013/03/12 11:26:39, Peter Beverloo wrote: > > Ok. This still is unreviewable unfortunately.. You can ask around in > #chromium > > if you can't find a way. > Done. https://codereview.chromium.org/12771008/diff/9001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/03/12 11:40:51, Miguel Garcia wrote: > copyright 2013 Done. https://codereview.chromium.org/12771008/diff/9001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/03/12 11:22:40, timvolodine1 wrote: > I did a mv old new, but still no proper diff with the old > DeviceOrientation.java, trying to figure out how to do this in rietveld. Done.
https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... File content/browser/device_orientation/data_fetcher_impl_android.cc (right): https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.cc:36: DataFetcher* DataFetcherImplAndroid::Create() { don't you want to at least change the interface now and pass an argument with the types we want? Then on the implementation you can Start only the kTypeOtrientation and throw unimplemented for the rest. https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.cc:104: event_type, what happened to the cast suggestion from Peter? https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:72: private static enum EventType { what happened to my comment about not using an enum? https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:74: DEVICE_MOTION(1); test? https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:97: // keep an array of active event types for speed because it is used in @see # @see # ? where is it used? https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:103: } can you do this in a for loop so we don't forget to add new types eventually? https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:169: boolean dispatchDeviceOrientation = I would change this a bit so that the switch just fills in the array, then we do a boundary check (important) then decide what method to call once we have the sensor data. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:193: break; extra blank line https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:213: // Unexpected can you log/assert something here? https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:217: if (dispatchDeviceOrientation) I think the style guide says to use brackets on all ifs https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:317: void gotAcceleration(double x, double y, double z) { I wonder if we could not simplify this by having one single method that takes the parameter you want (acceleration,accel + gravity, rotation rate or orientation) and the coordinate/angles that would avoid having 4 methods everywhere. What do you think Peter? https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:317: void gotAcceleration(double x, double y, double z) { can these methods be private or public if meant to be called from somewhere, package private seems useless in this context
https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... File content/browser/device_orientation/data_fetcher_impl_android.cc (right): https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.cc:34: // TODO(timvolodine): figure out how to modify this method in case of What does this mean? Figuring something out does not imply actually making the change. The TODO should read "Modify this method to be able to distinguish device motion from orientation.". https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.cc:46: // TODO(timvolodine): stop all active event types, not only This TODO should read "Support device motion as well. Only stop the active event type(s).". https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.cc:104: event_type, On 2013/03/12 17:00:26, Miguel Garcia wrote: > what happened to the cast suggestion from Peter? Try-jobs are running, let's see what they say. Nonetheless, I prefer to just static_cast<> them to an integer given that I think it's cleaner. https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.cc:112: event_type); Dito to my reply to Miguel's comment on line 104. https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... File content/browser/device_orientation/data_fetcher_impl_android.h (right): https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.h:55: // TODO(timvolodine): move the DeviceData::Type enum to the service class Should we also add this to the DeviceData class? https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:28: * Android implementation of the device motion and orientation API. nit: s/API/APIs/. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:56: // Gyroscope "The angular speed at which the device is rotating around the body frame." https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:57: private float[] mGyroVector; Let's spell out the full name: mGyroscopeVector. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:72: private static enum EventType { Please use int constants instead of enums (enums imply a significant weight on the dex files). https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:95: EventType.DEVICE_MOTION.value, DEVICE_MOTION_SENSORS); For line 82 to 95: I think this is much too verbose. We know that the accelerometer is necessary for both events, and then there are event-specific sensors. It would be really easy and much clearer to just implement this with an if-statement or two in start() and stop, which would take away the need to have all these sets. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:97: // keep an array of active event types for speed because it is used in @see # nit: Grammar. You seem to be missing a reference here. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:98: private boolean[] mIsSpecEventActive = new boolean[EventType.values().length]; As I mentioned in the native code review, please don't use the "Spec" prefix. Since there are only two event types with no new ones planned, I think it'd be nicer to just have two booleans. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:103: } On 2013/03/12 17:00:26, Miguel Garcia wrote: > can you do this in a for loop so we don't forget to add new types eventually? I think we should just have two separate members. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:112: * @param specEventType type of event to listen to, can either 0 for device nit: grammar (capital). s/can either/can be either/. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:137: * @param specEventType type of event to listen to, can either 0 for device nit: dito to line 112. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:172: mIsSpecEventActive[EventType.DEVICE_MOTION.value]; When we have two separate booleans for the events, we don't need have this at all. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:180: if (dispatchDeviceMotion) nit: Please include brackets for readability. The style guide prohibits multiple-line if statements (two, in this case) without brackets (see the "Use Standard Brace Style" section). https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:185: if (mAccelerationVector == null) { We're not using mAccelerationVector anywhere outside this block. Please don't copy everything and just invoke gotAcceleration(event.values[0], ...); https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:190: if (dispatchDeviceMotion) Why do we have this if statement here? We'll only receive linear acceleration sensor results if device motion events are being listened to. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:195: case Sensor.TYPE_GYROSCOPE: This whole block: dito to the comments on line 185 and 190. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:217: if (dispatchDeviceOrientation) nit: dito to the comment on line 180. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:275: unregisterForSensors(sensorTypes); Won't this fail if device orientation registered first (and the accelerometer and gravity sensors are available), then device motion registers, we're trying to register new sensors and can't register a linear acceleration sensor? In that case, we'd unregister all of them again, also losing device orientation coverage. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:299: Sensor defaultSensor = sensorManager.getDefaultSensor(type); I looked into doing this change myself, but I'm a bit wary because getDefaultSensor mentions it can combine the values of multiple sensors, which we're already doing. Furthermore, the implementation in Android right now is a stub, it does exactly the same to what we are doing right now. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:317: void gotAcceleration(double x, double y, double z) { On 2013/03/12 17:00:26, Miguel Garcia wrote: > I wonder if we could not simplify this by having one single method that takes > the parameter you want (acceleration,accel + gravity, rotation rate or > orientation) and the coordinate/angles that would avoid having 4 methods > everywhere. What do you think Peter? We cannot be sure in regards to which sensors we can receive data from, as such, I think having four separate methods would be best. That allows us to determine that we have to pass NULL to the event in the renderer process. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:317: void gotAcceleration(double x, double y, double z) { On 2013/03/12 17:00:26, Miguel Garcia wrote: > can these methods be private or public if meant to be called from somewhere, > package private seems useless in this context They should be private. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:396: * linear acceleration w/o gravity nit: Please use proper grammar and spell out "without". The nativeGotOrientation part could use a similar comment. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:403: * acceleration including gravity nit: dito. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:410: * gyroscope nit: dito.
fixed comments, uploaded new patch https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... File content/browser/device_orientation/data_fetcher_impl_android.cc (right): https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.cc:34: // TODO(timvolodine): figure out how to modify this method in case of On 2013/03/12 17:11:22, Peter Beverloo wrote: > What does this mean? Figuring something out does not imply actually making the > change. The TODO should read "Modify this method to be able to distinguish > device motion from orientation.". Done. https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.cc:36: DataFetcher* DataFetcherImplAndroid::Create() { This probably goes out of scope of this CL, because this intended to be mainly java-side. We'll need to figure out how to best handle both types as the current ::Create() approach seems not very appropriate anymore. On 2013/03/12 17:00:26, Miguel Garcia wrote: > don't you want to at least change the interface now and pass an argument with > the types we want? Then on the implementation you can Start only the > kTypeOtrientation and throw unimplemented for the rest. https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.cc:46: // TODO(timvolodine): stop all active event types, not only On 2013/03/12 17:11:22, Peter Beverloo wrote: > This TODO should read "Support device motion as well. Only stop the active event > type(s).". Done. https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.cc:104: event_type, On 2013/03/12 17:00:26, Miguel Garcia wrote: > what happened to the cast suggestion from Peter? Done. https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.cc:104: event_type, ok, added static_cast<jint> On 2013/03/12 17:11:22, Peter Beverloo wrote: > On 2013/03/12 17:00:26, Miguel Garcia wrote: > > what happened to the cast suggestion from Peter? > > Try-jobs are running, let's see what they say. Nonetheless, I prefer to just > static_cast<> them to an integer given that I think it's cleaner. https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.cc:112: event_type); On 2013/03/12 17:11:22, Peter Beverloo wrote: > Dito to my reply to Miguel's comment on line 104. Done. https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... File content/browser/device_orientation/data_fetcher_impl_android.h (right): https://codereview.chromium.org/12771008/diff/24001/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.h:55: // TODO(timvolodine): move the DeviceData::Type enum to the service class On 2013/03/12 17:11:22, Peter Beverloo wrote: > Should we also add this to the DeviceData class? Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:28: * Android implementation of the device motion and orientation API. On 2013/03/12 17:11:22, Peter Beverloo wrote: > nit: s/API/APIs/. Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:56: // Gyroscope On 2013/03/12 17:11:22, Peter Beverloo wrote: > "The angular speed at which the device is rotating around the body frame." Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:57: private float[] mGyroVector; On 2013/03/12 17:11:22, Peter Beverloo wrote: > Let's spell out the full name: mGyroscopeVector. Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:72: private static enum EventType { sorry seem to have missed that while uploading new diff On 2013/03/12 17:00:26, Miguel Garcia wrote: > what happened to my comment about not using an enum? https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:72: private static enum EventType { On 2013/03/12 17:11:22, Peter Beverloo wrote: > Please use int constants instead of enums (enums imply a significant weight on > the dex files). Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:74: DEVICE_MOTION(1); On 2013/03/12 17:00:26, Miguel Garcia wrote: > test? hmm not sure, is this N/A anymore (?) https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:95: EventType.DEVICE_MOTION.value, DEVICE_MOTION_SENSORS); On 2013/03/12 17:11:22, Peter Beverloo wrote: > For line 82 to 95: I think this is much too verbose. We know that the > accelerometer is necessary for both events, and then there are event-specific > sensors. It would be really easy and much clearer to just implement this with > an if-statement or two in start() and stop, which would take away the need to > have all these sets. Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:97: // keep an array of active event types for speed because it is used in @see # On 2013/03/12 17:00:26, Miguel Garcia wrote: > @see # ? > > where is it used? Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:97: // keep an array of active event types for speed because it is used in @see # On 2013/03/12 17:11:22, Peter Beverloo wrote: > nit: Grammar. You seem to be missing a reference here. Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:98: private boolean[] mIsSpecEventActive = new boolean[EventType.values().length]; On 2013/03/12 17:11:22, Peter Beverloo wrote: > As I mentioned in the native code review, please don't use the "Spec" prefix. > Since there are only two event types with no new ones planned, I think it'd be > nicer to just have two booleans. Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:103: } On 2013/03/12 17:11:22, Peter Beverloo wrote: > On 2013/03/12 17:00:26, Miguel Garcia wrote: > > can you do this in a for loop so we don't forget to add new types eventually? > > I think we should just have two separate members. Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:103: } On 2013/03/12 17:00:26, Miguel Garcia wrote: > can you do this in a for loop so we don't forget to add new types eventually? Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:112: * @param specEventType type of event to listen to, can either 0 for device On 2013/03/12 17:11:22, Peter Beverloo wrote: > nit: grammar (capital). s/can either/can be either/. Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:137: * @param specEventType type of event to listen to, can either 0 for device On 2013/03/12 17:11:22, Peter Beverloo wrote: > nit: dito to line 112. Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:169: boolean dispatchDeviceOrientation = On 2013/03/12 17:00:26, Miguel Garcia wrote: > I would change this a bit so that the switch just fills in the array, then we do > a boundary check (important) then decide what method to call once we have the > sensor data. Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:172: mIsSpecEventActive[EventType.DEVICE_MOTION.value]; On 2013/03/12 17:11:22, Peter Beverloo wrote: > When we have two separate booleans for the events, we don't need have this at > all. Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:180: if (dispatchDeviceMotion) On 2013/03/12 17:11:22, Peter Beverloo wrote: > nit: Please include brackets for readability. The style guide prohibits > multiple-line if statements (two, in this case) without brackets (see the "Use > Standard Brace Style" section). Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:185: if (mAccelerationVector == null) { On 2013/03/12 17:11:22, Peter Beverloo wrote: > We're not using mAccelerationVector anywhere outside this block. Please don't > copy everything and just invoke gotAcceleration(event.values[0], ...); Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:190: if (dispatchDeviceMotion) On 2013/03/12 17:11:22, Peter Beverloo wrote: > Why do we have this if statement here? We'll only receive linear acceleration > sensor results if device motion events are being listened to. Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:193: break; On 2013/03/12 17:00:26, Miguel Garcia wrote: > extra blank line Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:195: case Sensor.TYPE_GYROSCOPE: On 2013/03/12 17:11:22, Peter Beverloo wrote: > This whole block: dito to the comments on line 185 and 190. Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:213: // Unexpected It feels a bit scary if something goes wrong and we would log at @50Hz.. On 2013/03/12 17:00:26, Miguel Garcia wrote: > can you log/assert something here? https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:217: if (dispatchDeviceOrientation) On 2013/03/12 17:00:26, Miguel Garcia wrote: > I think the style guide says to use brackets on all ifs Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:217: if (dispatchDeviceOrientation) On 2013/03/12 17:11:22, Peter Beverloo wrote: > nit: dito to the comment on line 180. Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:275: unregisterForSensors(sensorTypes); this method is always called from start() with a list of inactive sensors only, so when unregistering no active sensor should be touched. There is a different issue here though: the method returns a boolean on success, while the definition of success is different for device orientation/device motion. I have added a flag and a doc to reflect that logic. On 2013/03/12 17:11:22, Peter Beverloo wrote: > Won't this fail if device orientation registered first (and the accelerometer > and gravity sensors are available), then device motion registers, we're trying > to register new sensors and can't register a linear acceleration sensor? In > that case, we'd unregister all of them again, also losing device orientation > coverage. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:299: Sensor defaultSensor = sensorManager.getDefaultSensor(type); ok, so should I revert to the previous solution with get(0)? also it sounds more logical to use sensor fusion (if available) as it is as close to hardware as possible and could possibly produce more accurate results... e.g. the current implementation of orientation is noisy in chrome, while that of firefox is smooth, I suspect they use Sensor.TYPE_ORIENTATION which seems to use sensor fusion.. anyway this is probably out of scope of this patch. On 2013/03/12 17:11:22, Peter Beverloo wrote: > I looked into doing this change myself, but I'm a bit wary because > getDefaultSensor mentions it can combine the values of multiple sensors, which > we're already doing. Furthermore, the implementation in Android right now is a > stub, it does exactly the same to what we are doing right now. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:317: void gotAcceleration(double x, double y, double z) { On 2013/03/12 17:00:26, Miguel Garcia wrote: > I wonder if we could not simplify this by having one single method that takes > the parameter you want (acceleration,accel + gravity, rotation rate or > orientation) and the coordinate/angles that would avoid having 4 methods > everywhere. What do you think Peter? Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:317: void gotAcceleration(double x, double y, double z) { On 2013/03/12 17:00:26, Miguel Garcia wrote: > can these methods be private or public if meant to be called from somewhere, > package private seems useless in this context Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:317: void gotAcceleration(double x, double y, double z) { On 2013/03/12 17:11:22, Peter Beverloo wrote: > On 2013/03/12 17:00:26, Miguel Garcia wrote: > > I wonder if we could not simplify this by having one single method that takes > > the parameter you want (acceleration,accel + gravity, rotation rate or > > orientation) and the coordinate/angles that would avoid having 4 methods > > everywhere. What do you think Peter? > > We cannot be sure in regards to which sensors we can receive data from, as such, > I think having four separate methods would be best. That allows us to determine > that we have to pass NULL to the event in the renderer process. Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:317: void gotAcceleration(double x, double y, double z) { On 2013/03/12 17:11:22, Peter Beverloo wrote: > On 2013/03/12 17:00:26, Miguel Garcia wrote: > > can these methods be private or public if meant to be called from somewhere, > > package private seems useless in this context > > They should be private. Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:396: * linear acceleration w/o gravity On 2013/03/12 17:11:22, Peter Beverloo wrote: > nit: Please use proper grammar and spell out "without". The > nativeGotOrientation part could use a similar comment. Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:403: * acceleration including gravity On 2013/03/12 17:11:22, Peter Beverloo wrote: > nit: dito. Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:410: * gyroscope On 2013/03/12 17:11:22, Peter Beverloo wrote: > nit: dito. Done.
Some comments on the java side, this is starting to look good https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:96: * @param eventType type of event to listen to, can be either 0 for device can you refer to the constants you created instead of 0/1. That would avoid having to explain what 0 and 1 mean. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:108: sensorsToActivate.removeAll(mActiveSensors); Since registerForSensors already filters out active sensors why not just call registerForSensors(DEVICE_ORIENTATION_SENSORS, rateInMilliseconds, true) ? https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:114: success = registerForSensors(sensorsToActivate, rateInMilliseconds, false); can you explain why we allow missing sensors on motion but not on orientation? https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:132: * We strictly guarantee that nativeGotOrientation() will not be called how about the other nativeGot* methods? https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:156: unregisterForSensors(sensorsToDeactivate); I don't understand all these set operations can't you simply do ? case orientation: unregisterForSensors(mDeviceMotionIsActive ? DEVICE_MOTION_SENSORS : mActiveSensors); case motion: unregisterForSensors(mDeviceOrientationIsActive > DEVICE_ORIENTATION_SENSORS : mActiveSensors); https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:175: if (mDeviceMotionIsActive) { why don't you use the same pattern than before, you collect the results (and what you need to do) in the switch statement then later on you do call whatever native methods need to be called. This way half of the case: statements call a native method and the other half don't making it pretty confusing. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:277: private boolean registerForSensors(Iterable<Integer> sensorTypes, int rateInMilliseconds, can this be called registerSensors while you are at it? https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:290: mActiveSensors.add(sensorType); should'nt you be checkout if the sensor activation has worked before adding it to the list?
https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:213: // Unexpected On 2013/03/13 12:32:24, timvolodine wrote: > It feels a bit scary if something goes wrong and we would log at @50Hz.. > On 2013/03/12 17:00:26, Miguel Garcia wrote: > > can you log/assert something here? > Maybe. We know that something is wrong when we listen to an unidentified sensor, so it shouldn't happen. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:299: Sensor defaultSensor = sensorManager.getDefaultSensor(type); On 2013/03/13 12:32:24, timvolodine wrote: > ok, so should I revert to the previous solution with get(0)? > also it sounds more logical to use sensor fusion (if available) as it is as > close to hardware as possible and could possibly produce more accurate > results... e.g. the current implementation of orientation is noisy in chrome, > while that of firefox is smooth, I suspect they use Sensor.TYPE_ORIENTATION > which seems to use sensor fusion.. anyway this is probably out of scope of this > patch. > > On 2013/03/12 17:11:22, Peter Beverloo wrote: > > I looked into doing this change myself, but I'm a bit wary because > > getDefaultSensor mentions it can combine the values of multiple sensors, which > > we're already doing. Furthermore, the implementation in Android right now is > a > > stub, it does exactly the same to what we are doing right now. > I do think we should revert until we properly understand what it will do. Right now it's undefined and we're doing our own calculations, whereas a combination sensor (maybe similar to TYPE_ORIENTATION) will do it for us. By using getDefaultSensor() the behavior will change unexpectedly when Android implements this. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:299: Sensor defaultSensor = sensorManager.getDefaultSensor(type); Can you file an issue to track looking in to switching to TYPE_ORIENTATION? https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:15: import com.google.common.collect.ImmutableMap; This is now unused. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:24: import java.util.Map; This is now unused. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:54: private float[] mAccelerationVector; Should we perhaps use this member instead of mGravityVector for the accelerometer results? I think it has a better name. In either case, we don't need both of them. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:57: private float[] mGyroscopeVector; We don't need this member. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:96: * @param eventType type of event to listen to, can be either 0 for device s/type/Type/. Sentences begin with a capital. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:108: sensorsToActivate.removeAll(mActiveSensors); On 2013/03/13 13:37:09, Miguel Garcia wrote: > Since registerForSensors already filters out active sensors why not just call > registerForSensors(DEVICE_ORIENTATION_SENSORS, rateInMilliseconds, true) ? +1. Dito on line 113. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:114: success = registerForSensors(sensorsToActivate, rateInMilliseconds, false); On 2013/03/13 13:37:09, Miguel Garcia wrote: > can you explain why we allow missing sensors on motion but not on orientation? The specification doesn't mandate that either sensor has to exist. We pass NULL in the IDL if the implementation can't provide the data. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:117: return false; Can we assert here as this shouldn't happen? https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:130: * @param eventType type of event to listen to, can be either 0 for device s/type/Type/. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:151: return; Dito: Can we assert here as well? https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:175: if (mDeviceMotionIsActive) { On 2013/03/13 13:37:09, Miguel Garcia wrote: > why don't you use the same pattern than before, you collect the results (and > what you need to do) in the switch statement then later on you do call whatever > native methods need to be called. This way half of the case: statements call a > native method and the other half don't making it pretty confusing. I think this is the right approach. We don't need to do further operations on the accelerometer, linear acceleration or gyroscope data for Device Motion. Device Orientation requires us to know both the accelerometer and magnetic field data, which is why we call it later on. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:185: mAccelerationVector[2]); What fills mAccelerationVector? :-). This should call getAcceleration with the event.values[{0, 1, 2}] data. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:191: gotRotationRate(mGyroscopeVector[0], mGyroscopeVector[1], mGyroscopeVector[2]); Dito to line 184. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:272: * @param rateInMilliseconds intented delay (in ms) between sensor readings. s/intented/Intended/. Can you spell out ms? Ideally we'd move away from these approximations in favor of specifying SENSOR_DELAY_*. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:310: Sensor defaultSensor = sensorManager.getDefaultSensor(type); Please see my replies in regards to this on the old patch set. I left two comments there.
uploaded new patch https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:299: Sensor defaultSensor = sensorManager.getDefaultSensor(type); On 2013/03/13 14:02:47, Peter Beverloo wrote: > On 2013/03/13 12:32:24, timvolodine wrote: > > ok, so should I revert to the previous solution with get(0)? > > also it sounds more logical to use sensor fusion (if available) as it is as > > close to hardware as possible and could possibly produce more accurate > > results... e.g. the current implementation of orientation is noisy in chrome, > > while that of firefox is smooth, I suspect they use Sensor.TYPE_ORIENTATION > > which seems to use sensor fusion.. anyway this is probably out of scope of > this > > patch. > > > > On 2013/03/12 17:11:22, Peter Beverloo wrote: > > > I looked into doing this change myself, but I'm a bit wary because > > > getDefaultSensor mentions it can combine the values of multiple sensors, > which > > > we're already doing. Furthermore, the implementation in Android right now > is > > a > > > stub, it does exactly the same to what we are doing right now. > > > > I do think we should revert until we properly understand what it will do. Right > now it's undefined and we're doing our own calculations, whereas a combination > sensor (maybe similar to TYPE_ORIENTATION) will do it for us. By using > getDefaultSensor() the behavior will change unexpectedly when Android implements > this. Done. https://codereview.chromium.org/12771008/diff/24001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:299: Sensor defaultSensor = sensorManager.getDefaultSensor(type); On 2013/03/13 14:02:47, Peter Beverloo wrote: > Can you file an issue to track looking in to switching to TYPE_ORIENTATION? will do https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:15: import com.google.common.collect.ImmutableMap; On 2013/03/13 14:02:47, Peter Beverloo wrote: > This is now unused. Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:24: import java.util.Map; On 2013/03/13 14:02:47, Peter Beverloo wrote: > This is now unused. Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:54: private float[] mAccelerationVector; On 2013/03/13 14:02:47, Peter Beverloo wrote: > Should we perhaps use this member instead of mGravityVector for the > accelerometer results? I think it has a better name. In either case, we don't > need both of them. Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:57: private float[] mGyroscopeVector; On 2013/03/13 14:02:47, Peter Beverloo wrote: > We don't need this member. Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:96: * @param eventType type of event to listen to, can be either 0 for device On 2013/03/13 13:37:09, Miguel Garcia wrote: > can you refer to the constants you created instead of 0/1. That would avoid > having to explain what 0 and 1 mean. Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:96: * @param eventType type of event to listen to, can be either 0 for device On 2013/03/13 14:02:47, Peter Beverloo wrote: > s/type/Type/. Sentences begin with a capital. Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:108: sensorsToActivate.removeAll(mActiveSensors); I've changed the registerForSensors method a bit, so this is now possible. Otherwise in the case with DEVICE_MOTION we might unregister too many sensors upon failure. On 2013/03/13 13:37:09, Miguel Garcia wrote: > Since registerForSensors already filters out active sensors why not just call > registerForSensors(DEVICE_ORIENTATION_SENSORS, rateInMilliseconds, true) ? https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:108: sensorsToActivate.removeAll(mActiveSensors); On 2013/03/13 14:02:47, Peter Beverloo wrote: > On 2013/03/13 13:37:09, Miguel Garcia wrote: > > Since registerForSensors already filters out active sensors why not just call > > registerForSensors(DEVICE_ORIENTATION_SENSORS, rateInMilliseconds, true) ? > > +1. Dito on line 113. Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:114: success = registerForSensors(sensorsToActivate, rateInMilliseconds, false); as Peter said, it's the spec On 2013/03/13 13:37:09, Miguel Garcia wrote: > can you explain why we allow missing sensors on motion but not on orientation? https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:114: success = registerForSensors(sensorsToActivate, rateInMilliseconds, false); On 2013/03/13 14:02:47, Peter Beverloo wrote: > On 2013/03/13 13:37:09, Miguel Garcia wrote: > > can you explain why we allow missing sensors on motion but not on orientation? > > The specification doesn't mandate that either sensor has to exist. We pass NULL > in the IDL if the implementation can't provide the data. Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:117: return false; On 2013/03/13 14:02:47, Peter Beverloo wrote: > Can we assert here as this shouldn't happen? Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:130: * @param eventType type of event to listen to, can be either 0 for device On 2013/03/13 14:02:47, Peter Beverloo wrote: > s/type/Type/. Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:132: * We strictly guarantee that nativeGotOrientation() will not be called On 2013/03/13 13:37:09, Miguel Garcia wrote: > how about the other nativeGot* methods? Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:151: return; On 2013/03/13 14:02:47, Peter Beverloo wrote: > Dito: Can we assert here as well? Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:156: unregisterForSensors(sensorsToDeactivate); the set of sensors for both events overlaps, so we need to be careful not to unregister too much, hence all these set operations On 2013/03/13 13:37:09, Miguel Garcia wrote: > I don't understand all these set operations > can't you simply do ? > > case orientation: > unregisterForSensors(mDeviceMotionIsActive ? DEVICE_MOTION_SENSORS : > mActiveSensors); > case motion: > unregisterForSensors(mDeviceOrientationIsActive > DEVICE_ORIENTATION_SENSORS : > mActiveSensors); https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:175: if (mDeviceMotionIsActive) { On 2013/03/13 14:02:47, Peter Beverloo wrote: > On 2013/03/13 13:37:09, Miguel Garcia wrote: > > why don't you use the same pattern than before, you collect the results (and > > what you need to do) in the switch statement then later on you do call > whatever > > native methods need to be called. This way half of the case: statements call a > > native method and the other half don't making it pretty confusing. > > I think this is the right approach. We don't need to do further operations on > the accelerometer, linear acceleration or gyroscope data for Device Motion. > Device Orientation requires us to know both the accelerometer and magnetic field > data, which is why we call it later on. Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:175: if (mDeviceMotionIsActive) { agree with Peter's comment, we don't need to compute anything for the motion case.. On 2013/03/13 13:37:09, Miguel Garcia wrote: > why don't you use the same pattern than before, you collect the results (and > what you need to do) in the switch statement then later on you do call whatever > native methods need to be called. This way half of the case: statements call a > native method and the other half don't making it pretty confusing. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:185: mAccelerationVector[2]); On 2013/03/13 14:02:47, Peter Beverloo wrote: > What fills mAccelerationVector? :-). This should call getAcceleration with the > event.values[{0, 1, 2}] data. Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:191: gotRotationRate(mGyroscopeVector[0], mGyroscopeVector[1], mGyroscopeVector[2]); On 2013/03/13 14:02:47, Peter Beverloo wrote: > Dito to line 184. Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:272: * @param rateInMilliseconds intented delay (in ms) between sensor readings. On 2013/03/13 14:02:47, Peter Beverloo wrote: > s/intented/Intended/. Can you spell out ms? Ideally we'd move away from these > approximations in favor of specifying SENSOR_DELAY_*. Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:277: private boolean registerForSensors(Iterable<Integer> sensorTypes, int rateInMilliseconds, On 2013/03/13 13:37:09, Miguel Garcia wrote: > can this be called registerSensors while you are at it? Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:290: mActiveSensors.add(sensorType); right Done. On 2013/03/13 13:37:09, Miguel Garcia wrote: > should'nt you be checkout if the sensor activation has worked before adding it > to the list? https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:310: Sensor defaultSensor = sensorManager.getDefaultSensor(type); On 2013/03/13 14:02:47, Peter Beverloo wrote: > Please see my replies in regards to this on the old patch set. I left two > comments there. Done.
I think that all the set operations can be simplified significantly, let me know what you think of my suggestion ok? https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:114: success = registerForSensors(sensorsToActivate, rateInMilliseconds, false); Yeas I know I meant adding a comment explaining exactly that :) so a random reader understands why On 2013/03/13 15:29:49, timvolodine wrote: > as Peter said, it's the spec > On 2013/03/13 13:37:09, Miguel Garcia wrote: > > can you explain why we allow missing sensors on motion but not on orientation? > https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:128: * Stop listening for particular sensor events. Always succeeds. I think you need to upgrade this java doc. Something like "Stop listening events for a given event type, it ensures that sensors for overlapping event types are not disabled if they are still in use by a different type." https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:156: unregisterForSensors(sensorsToDeactivate); Would it feel more natural to have 3 sets? common_sensors orientation_only_sensors motion_only_sensors. Then all the register/unregister code will become way simpler On 2013/03/13 15:29:49, timvolodine wrote: > the set of sensors for both events overlaps, so we need to be careful not to > unregister too much, hence all these set operations > On 2013/03/13 13:37:09, Miguel Garcia wrote: > > I don't understand all these set operations > > can't you simply do ? > > > > case orientation: > > unregisterForSensors(mDeviceMotionIsActive ? DEVICE_MOTION_SENSORS : > > mActiveSensors); > > case motion: > > unregisterForSensors(mDeviceOrientationIsActive > DEVICE_ORIENTATION_SENSORS > : > > mActiveSensors); > https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:175: if (mDeviceMotionIsActive) { ok On 2013/03/13 15:29:49, timvolodine wrote: > agree with Peter's comment, we don't need to compute anything for the motion > case.. > On 2013/03/13 13:37:09, Miguel Garcia wrote: > > why don't you use the same pattern than before, you collect the results (and > > what you need to do) in the switch statement then later on you do call > whatever > > native methods need to be called. This way half of the case: statements call a > > native method and the other half don't making it pretty confusing. >
This indeed is a lot better, thank you. A few more comments -- mostly nits!, on top of what Miguel already submitted. After these are addressed we can ask Marcus to take a look. https://codereview.chromium.org/12771008/diff/40001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/40001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:48: // The gravity vector expressed in the body frame. This should read "The acceleration vector including gravity expressed in the body frame.". https://codereview.chromium.org/12771008/diff/40001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:92: * DEVICE_MOTION. nit: 11-space ident? What should this be aligned with? Line 90 seems to do this wrong as well. https://codereview.chromium.org/12771008/diff/40001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:102: true); nit: indent with eight spaces, so equal to the "=" character in line 101. https://codereview.chromium.org/12771008/diff/40001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:123: * DEVICE_MOTION. nit: 11 space indent? https://codereview.chromium.org/12771008/diff/40001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:168: mAccelerationVector.length); nit: 8 space indent, so with the first "r". https://codereview.chromium.org/12771008/diff/40001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:171: mAccelerationVector[2]); nit: 8 space indent, so with the second "e". https://codereview.chromium.org/12771008/diff/40001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:175: gotAcceleration(event.values[0], event.values[1], event.values[2]); Should we protect this with if(mDeviceMotionIsActive)? Dito with line 178. It shouldn't hurt in either case. https://codereview.chromium.org/12771008/diff/40001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:309: getHandler()); nit: 8 space indent.
https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:114: success = registerForSensors(sensorsToActivate, rateInMilliseconds, false); On 2013/03/13 16:04:29, Miguel Garcia wrote: > Yeas I know I meant adding a comment explaining exactly that :) so a random > reader understands why > > On 2013/03/13 15:29:49, timvolodine wrote: > > as Peter said, it's the spec > > On 2013/03/13 13:37:09, Miguel Garcia wrote: > > > can you explain why we allow missing sensors on motion but not on > orientation? > > > Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:128: * Stop listening for particular sensor events. Always succeeds. On 2013/03/13 16:04:29, Miguel Garcia wrote: > I think you need to upgrade this java doc. Something like > > "Stop listening events for a given event type, it ensures that sensors for > overlapping event types are not disabled if they are still in use by a different > type." Done. https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:156: unregisterForSensors(sensorsToDeactivate); that'll probably work too, but not sure if it would simplify the code much. We'll still need active sensors, sensorsToDeactivate=orientation_only - common_sensors or something like that... On 2013/03/13 16:04:29, Miguel Garcia wrote: > Would it feel more natural to have 3 sets? > common_sensors > orientation_only_sensors > motion_only_sensors. > > Then all the register/unregister code will become way simpler > > On 2013/03/13 15:29:49, timvolodine wrote: > > the set of sensors for both events overlaps, so we need to be careful not to > > unregister too much, hence all these set operations > > On 2013/03/13 13:37:09, Miguel Garcia wrote: > > > I don't understand all these set operations > > > can't you simply do ? > > > > > > case orientation: > > > unregisterForSensors(mDeviceMotionIsActive ? DEVICE_MOTION_SENSORS : > > > mActiveSensors); > > > case motion: > > > unregisterForSensors(mDeviceOrientationIsActive > > DEVICE_ORIENTATION_SENSORS > > : > > > mActiveSensors); > > > https://codereview.chromium.org/12771008/diff/35001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:175: if (mDeviceMotionIsActive) { On 2013/03/13 16:04:29, Miguel Garcia wrote: > ok > On 2013/03/13 15:29:49, timvolodine wrote: > > agree with Peter's comment, we don't need to compute anything for the motion > > case.. > > On 2013/03/13 13:37:09, Miguel Garcia wrote: > > > why don't you use the same pattern than before, you collect the results (and > > > what you need to do) in the switch statement then later on you do call > > whatever > > > native methods need to be called. This way half of the case: statements call > a > > > native method and the other half don't making it pretty confusing. > > > Done. https://codereview.chromium.org/12771008/diff/40001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/40001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:48: // The gravity vector expressed in the body frame. On 2013/03/13 16:18:57, Peter Beverloo wrote: > This should read "The acceleration vector including gravity expressed in the > body frame.". Done. https://codereview.chromium.org/12771008/diff/40001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:92: * DEVICE_MOTION. aligned with last row for readability. On 2013/03/13 16:18:57, Peter Beverloo wrote: > nit: 11-space ident? What should this be aligned with? Line 90 seems to do > this wrong as well. https://codereview.chromium.org/12771008/diff/40001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:102: true); On 2013/03/13 16:18:57, Peter Beverloo wrote: > nit: indent with eight spaces, so equal to the "=" character in line 101. Done. https://codereview.chromium.org/12771008/diff/40001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:123: * DEVICE_MOTION. aligned to last line On 2013/03/13 16:18:57, Peter Beverloo wrote: > nit: 11 space indent? https://codereview.chromium.org/12771008/diff/40001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:168: mAccelerationVector.length); On 2013/03/13 16:18:57, Peter Beverloo wrote: > nit: 8 space indent, so with the first "r". Done. https://codereview.chromium.org/12771008/diff/40001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:171: mAccelerationVector[2]); On 2013/03/13 16:18:57, Peter Beverloo wrote: > nit: 8 space indent, so with the second "e". Done. https://codereview.chromium.org/12771008/diff/40001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:175: gotAcceleration(event.values[0], event.values[1], event.values[2]); On 2013/03/13 16:18:57, Peter Beverloo wrote: > Should we protect this with if(mDeviceMotionIsActive)? Dito with line 178. It > shouldn't hurt in either case. Done. https://codereview.chromium.org/12771008/diff/40001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:309: getHandler()); On 2013/03/13 16:18:57, Peter Beverloo wrote: > nit: 8 space indent. Done.
added tests patch should be complete now
Some extra comments on the test https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... File content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java (right): https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:32: */ Do you need a full instrumentation test for this? You don't seem to be using any UI or anything like that. https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:42: mDeviceMotionAndOrientation = MockDeviceMotionAndOrientation.getInstance(); line over 80, some more below, please run the clang formatter https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:276: static class MockSensorManager implements DeviceMotionAndOrientation.SensorManagerProxy { please move this to a separate class. We will end up adding extra functionality and become ummanageable. Alternative can we use EasyMock for this?
looking forward to see this landing! a few comments and nits below, I hope it'll simplify things a bit: https://codereview.chromium.org/12771008/diff/40002/content/browser/device_or... File content/browser/device_orientation/data_fetcher_impl_android.cc (right): https://codereview.chromium.org/12771008/diff/40002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.cc:101: return Java_DeviceMotionAndOrientation_start(AttachCurrentThread(), nit: preferred way for callers is to move the first param to the next line indented by +4, and put as many params per line as possible... (for declarations, it's better to keep left-aligned as it was...) https://codereview.chromium.org/12771008/diff/40002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.cc:111: device_orientation_.obj(), nit: as above.. https://codereview.chromium.org/12771008/diff/40002/content/browser/device_or... File content/browser/device_orientation/device_data.h (right): https://codereview.chromium.org/12771008/diff/40002/content/browser/device_or... content/browser/device_orientation/device_data.h:23: kTypeOrientation = 0, nit: is this initialization necessary? https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:33: private static final String LOGTAG = "DeviceMotionAndOrientation"; nit: normally this is just "TAG" https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:79: Set<Integer> mActiveSensors = Sets.newHashSet(); nit: final here, 69 and 73 https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:154: mNativePtr = 0; shouldn't the native ptr only go away if there are no more active sensors? https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:265: private void setActiveEventType(int eventType, boolean value) { nit: this would read better as "setEventTypeActive", otherwise it seems that only one can be active at a time. https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:284: boolean failOnMissingSensor) { nit: alignment https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:307: List<Sensor> sensors = getSensorManagerProxy().getSensorList(sensorType); all usages of getSensorList() check for empty and then use sensors.get(0).. how about just making it "Sensor getSensor()" then return null if it's empty and use it directly otherwise? or even better: it seems that this is used only to then pass it again to un/registerListener... so how about pushing all this logic to SensorManagerProxy itself, make "getSensorList" an internal method on the impl, and let the un/registerListener take a Integer type rather than a Sensor? https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:446: public List<Sensor> getSensorList(int type); see above, I think this can be moved out of the interface and as a private method in SensorManagerProxyImpl if the un/register methods take an "int type" rather than a Sensor.. https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... File content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java (right): https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:42: mDeviceMotionAndOrientation = MockDeviceMotionAndOrientation.getInstance(); On 2013/03/19 15:12:00, Miguel Garcia wrote: > line over 80, some more below, please run the clang formatter java is 100cols, so it's fine. :) however, please note that this is not a mock, since it's actually extending the class under test (for a minute, I was confused on why are we testing a mock? :) please name it a "DeviceMotionAndOrientationForTests".. (the MockSensorManager is actually a mock, so leave it as is). https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:47: public void testRegisterSensors_DeviceMotion() { nit: remove the "_DeviceMotion" everywhere, the name is already scoped to this class.
thanks for comments, fixed most of them replies below https://codereview.chromium.org/12771008/diff/40002/content/browser/device_or... File content/browser/device_orientation/data_fetcher_impl_android.cc (right): https://codereview.chromium.org/12771008/diff/40002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.cc:101: return Java_DeviceMotionAndOrientation_start(AttachCurrentThread(), On 2013/03/19 15:22:36, bulach wrote: > nit: preferred way for callers is to move the first param to the next line > indented by +4, and put as many params per line as possible... (for > declarations, it's better to keep left-aligned as it was...) Done. https://codereview.chromium.org/12771008/diff/40002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.cc:111: device_orientation_.obj(), On 2013/03/19 15:22:36, bulach wrote: > nit: as above.. Done. https://codereview.chromium.org/12771008/diff/40002/content/browser/device_or... File content/browser/device_orientation/device_data.h (right): https://codereview.chromium.org/12771008/diff/40002/content/browser/device_or... content/browser/device_orientation/device_data.h:23: kTypeOrientation = 0, On 2013/03/19 15:22:36, bulach wrote: > nit: is this initialization necessary? maybe not, but wanted to make sure it's ok when called through JNI to java. This Class is intended to go anyway so will revisit this issue then. https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:33: private static final String LOGTAG = "DeviceMotionAndOrientation"; On 2013/03/19 15:22:36, bulach wrote: > nit: normally this is just "TAG" Done. https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:79: Set<Integer> mActiveSensors = Sets.newHashSet(); On 2013/03/19 15:22:36, bulach wrote: > nit: final here, 69 and 73 Done. https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:154: mNativePtr = 0; On 2013/03/19 15:22:36, bulach wrote: > shouldn't the native ptr only go away if there are no more active sensors? Done. https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:265: private void setActiveEventType(int eventType, boolean value) { On 2013/03/19 15:22:36, bulach wrote: > nit: this would read better as "setEventTypeActive", otherwise it seems that > only one can be active at a time. Done. https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:284: boolean failOnMissingSensor) { On 2013/03/19 15:22:36, bulach wrote: > nit: alignment Done. https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:307: List<Sensor> sensors = getSensorManagerProxy().getSensorList(sensorType); nice point, simplifies the code indeed -- done! On 2013/03/19 15:22:36, bulach wrote: > all usages of getSensorList() check for empty and then use sensors.get(0).. > how about just making it "Sensor getSensor()" then return null if it's empty and > use it directly otherwise? > or even better: it seems that this is used only to then pass it again to > un/registerListener... so how about pushing all this logic to SensorManagerProxy > itself, make "getSensorList" an internal method on the impl, and let the > un/registerListener take a Integer type rather than a Sensor? https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:446: public List<Sensor> getSensorList(int type); On 2013/03/19 15:22:36, bulach wrote: > see above, I think this can be moved out of the interface and as a private > method in SensorManagerProxyImpl if the un/register methods take an "int type" > rather than a Sensor.. Done. https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... File content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java (right): https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:32: */ not sure what else to use in this case, was basing my test on ImportantFileWriterAndroidTest.. On 2013/03/19 15:12:00, Miguel Garcia wrote: > Do you need a full instrumentation test for this? You don't seem to be using any > UI or anything like that. https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:42: mDeviceMotionAndOrientation = MockDeviceMotionAndOrientation.getInstance(); On 2013/03/19 15:22:36, bulach wrote: > On 2013/03/19 15:12:00, Miguel Garcia wrote: > > line over 80, some more below, please run the clang formatter > > java is 100cols, so it's fine. :) > > however, please note that this is not a mock, since it's actually extending the > class under test (for a minute, I was confused on why are we testing a mock? :) > > please name it a "DeviceMotionAndOrientationForTests".. > (the MockSensorManager is actually a mock, so leave it as is). Done. https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:42: mDeviceMotionAndOrientation = MockDeviceMotionAndOrientation.getInstance(); On 2013/03/19 15:12:00, Miguel Garcia wrote: > line over 80, some more below, please run the clang formatter Done. https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:47: public void testRegisterSensors_DeviceMotion() { this refers to testing of sensors related to "device motion" as opposed to "device orientation" below. If I remove the suffix not sure how to distinguish the two tests. On 2013/03/19 15:22:36, bulach wrote: > nit: remove the "_DeviceMotion" everywhere, the name is already scoped to this > class. https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:276: static class MockSensorManager implements DeviceMotionAndOrientation.SensorManagerProxy { not sure if moving to a separate class will simplify things much in this case, none of the other tests is doing this so this could just 'pollute' the directory.. Initially I looked into using EasyMock/mockito but unfortunately this is not supported/used in chromium so eventually need all these mocks.. On 2013/03/19 15:12:00, Miguel Garcia wrote: > please move this to a separate class. We will end up adding extra functionality > and become ummanageable. Alternative can we use EasyMock for this?
https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... File content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java (right): https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:32: */ It feels that a plain AndroidTestCase would do. On 2013/03/19 16:50:08, timvolodine wrote: > not sure what else to use in this case, was basing my test on > ImportantFileWriterAndroidTest.. > On 2013/03/19 15:12:00, Miguel Garcia wrote: > > Do you need a full instrumentation test for this? You don't seem to be using > any > > UI or anything like that. > https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:42: mDeviceMotionAndOrientation = MockDeviceMotionAndOrientation.getInstance(); Ups, my bad On 2013/03/19 15:22:36, bulach wrote: > On 2013/03/19 15:12:00, Miguel Garcia wrote: > > line over 80, some more below, please run the clang formatter > > java is 100cols, so it's fine. :) > > however, please note that this is not a mock, since it's actually extending the > class under test (for a minute, I was confused on why are we testing a mock? :) > > please name it a "DeviceMotionAndOrientationForTests".. > (the MockSensorManager is actually a mock, so leave it as is). https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:276: static class MockSensorManager implements DeviceMotionAndOrientation.SensorManagerProxy { It's a pity that no mocking system is supported. Marcus would it be a problem if we move this to it's own file? I am not sure I buy the pollution argument since this is already in the javatests space. On 2013/03/19 16:50:08, timvolodine wrote: > not sure if moving to a separate class will simplify things much in this case, > none of the other tests is doing this so this could just 'pollute' the > directory.. > Initially I looked into using EasyMock/mockito but unfortunately this is not > supported/used in chromium so eventually need all these mocks.. > > On 2013/03/19 15:12:00, Miguel Garcia wrote: > > please move this to a separate class. We will end up adding extra > functionality > > and become ummanageable. Alternative can we use EasyMock for this? >
https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... File content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java (right): https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:32: */ ok, that works also -- done. On 2013/03/19 17:06:13, Miguel Garcia wrote: > It feels that a plain AndroidTestCase would do. > > On 2013/03/19 16:50:08, timvolodine wrote: > > not sure what else to use in this case, was basing my test on > > ImportantFileWriterAndroidTest.. > > On 2013/03/19 15:12:00, Miguel Garcia wrote: > > > Do you need a full instrumentation test for this? You don't seem to be using > > any > > > UI or anything like that. > > > https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:42: mDeviceMotionAndOrientation = MockDeviceMotionAndOrientation.getInstance(); On 2013/03/19 17:06:13, Miguel Garcia wrote: > Ups, my bad > > On 2013/03/19 15:22:36, bulach wrote: > > On 2013/03/19 15:12:00, Miguel Garcia wrote: > > > line over 80, some more below, please run the clang formatter > > > > java is 100cols, so it's fine. :) > > > > however, please note that this is not a mock, since it's actually extending > the > > class under test (for a minute, I was confused on why are we testing a mock? > :) > > > > please name it a "DeviceMotionAndOrientationForTests".. > > (the MockSensorManager is actually a mock, so leave it as is). > Done. https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:276: static class MockSensorManager implements DeviceMotionAndOrientation.SensorManagerProxy { If we move the MockSensorManager to a separate file than we should probably move the MockDeviceMotionAndOrientation as well. In my opinion this would not improve readability. In its current state the test is self-contained similar to the the other tests. On 2013/03/19 17:06:13, Miguel Garcia wrote: > It's a pity that no mocking system is supported. > > Marcus would it be a problem if we move this to it's own file? I am not sure I > buy the pollution argument since this is already in the javatests space. > > > On 2013/03/19 16:50:08, timvolodine wrote: > > not sure if moving to a separate class will simplify things much in this case, > > none of the other tests is doing this so this could just 'pollute' the > > directory.. > > Initially I looked into using EasyMock/mockito but unfortunately this is not > > supported/used in chromium so eventually need all these mocks.. > > > > On 2013/03/19 15:12:00, Miguel Garcia wrote: > > > please move this to a separate class. We will end up adding extra > > functionality > > > and become ummanageable. Alternative can we use EasyMock for this? > > >
lgtm https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... File content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java (right): https://codereview.chromium.org/12771008/diff/40002/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:276: static class MockSensorManager implements DeviceMotionAndOrientation.SensorManagerProxy { Alright, I don't feel too strongly about it. On 2013/03/19 17:31:55, timvolodine wrote: > If we move the MockSensorManager to a separate file than we should probably move > the MockDeviceMotionAndOrientation as well. In my opinion this would not improve > readability. In its current state the test is self-contained similar to the the > other tests. > > On 2013/03/19 17:06:13, Miguel Garcia wrote: > > It's a pity that no mocking system is supported. > > > > Marcus would it be a problem if we move this to it's own file? I am not sure I > > buy the pollution argument since this is already in the javatests space. > > > > > > On 2013/03/19 16:50:08, timvolodine wrote: > > > not sure if moving to a separate class will simplify things much in this > case, > > > none of the other tests is doing this so this could just 'pollute' the > > > directory.. > > > Initially I looked into using EasyMock/mockito but unfortunately this is not > > > supported/used in chromium so eventually need all these mocks.. > > > > > > On 2013/03/19 15:12:00, Miguel Garcia wrote: > > > > please move this to a separate class. We will end up adding extra > > > functionality > > > > and become ummanageable. Alternative can we use EasyMock for this? > > > > > >
lgtm Thanks for putting up with all the comments!
lgtm, just some nits and one question on the mNativePtr below, feel free to go ahead once these are addressed. https://codereview.chromium.org/12771008/diff/52019/content/browser/device_or... File content/browser/device_orientation/data_fetcher_impl_android.cc (right): https://codereview.chromium.org/12771008/diff/52019/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.cc:98: bool DataFetcherImplAndroid::Start(DeviceData::Type event_type, nit: move the param to the next line.. https://codereview.chromium.org/12771008/diff/52019/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/52019/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:155: if (!mActiveSensors.isEmpty()) { I think the ! is unecessary, that is, mNativePtr should only be nullified if mActiveSensors is empty? https://codereview.chromium.org/12771008/diff/52019/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:319: boolean registerForSensorType(int type, int rateInMilliseconds) { nit: private? https://codereview.chromium.org/12771008/diff/52019/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:449: private SensorManager mSensorManager; nit: final https://codereview.chromium.org/12771008/diff/52019/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:472: return mSensorManager.getSensorList(type); this function is not very helpful, just call mSensorManager.getSensorList(type); directly on the two places above.. https://codereview.chromium.org/12771008/diff/52019/content/public/android/ja... File content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java (right): https://codereview.chromium.org/12771008/diff/52019/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:47: public void testRegisterSensors_DeviceMotion() { ahn, I see the need for the suffix... however, in java there's no "_", so just call it "testRegisterSensorsDeviceMotion" (ditto for the ones below) https://codereview.chromium.org/12771008/diff/52019/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:171: mDeviceMotionAndOrientation.verifyCalls("gotAccelerationIncludingGravity"+"gotOrientation"); nit: spaces between + https://codereview.chromium.org/12771008/diff/52019/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:214: static class DeviceMotionAndOrientationForTests extends DeviceMotionAndOrientation { won't make much difference, but could mark this and the following class and all its members and functions as private, since it's only accessed here..
https://codereview.chromium.org/12771008/diff/52019/content/browser/device_or... File content/browser/device_orientation/data_fetcher_impl_android.cc (right): https://codereview.chromium.org/12771008/diff/52019/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_android.cc:98: bool DataFetcherImplAndroid::Start(DeviceData::Type event_type, On 2013/03/19 18:51:06, bulach wrote: > nit: move the param to the next line.. Done. https://codereview.chromium.org/12771008/diff/52019/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://codereview.chromium.org/12771008/diff/52019/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:155: if (!mActiveSensors.isEmpty()) { oops -- fixed On 2013/03/19 18:51:06, bulach wrote: > I think the ! is unecessary, that is, mNativePtr should only be nullified if > mActiveSensors is empty? https://codereview.chromium.org/12771008/diff/52019/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:319: boolean registerForSensorType(int type, int rateInMilliseconds) { On 2013/03/19 18:51:06, bulach wrote: > nit: private? Done. https://codereview.chromium.org/12771008/diff/52019/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:449: private SensorManager mSensorManager; On 2013/03/19 18:51:06, bulach wrote: > nit: final Done. https://codereview.chromium.org/12771008/diff/52019/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:472: return mSensorManager.getSensorList(type); On 2013/03/19 18:51:06, bulach wrote: > this function is not very helpful, just call mSensorManager.getSensorList(type); > directly on the two places above.. Done. https://codereview.chromium.org/12771008/diff/52019/content/public/android/ja... File content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java (right): https://codereview.chromium.org/12771008/diff/52019/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:47: public void testRegisterSensors_DeviceMotion() { On 2013/03/19 18:51:06, bulach wrote: > ahn, I see the need for the suffix... however, in java there's no "_", so just > call it "testRegisterSensorsDeviceMotion" (ditto for the ones below) Done. https://codereview.chromium.org/12771008/diff/52019/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:171: mDeviceMotionAndOrientation.verifyCalls("gotAccelerationIncludingGravity"+"gotOrientation"); On 2013/03/19 18:51:06, bulach wrote: > nit: spaces between + Done. https://codereview.chromium.org/12771008/diff/52019/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:214: static class DeviceMotionAndOrientationForTests extends DeviceMotionAndOrientation { Done. However this was not possible in some places because of the super class access privileges On 2013/03/19 18:51:06, bulach wrote: > won't make much difference, but could mark this and the following class and all > its members and functions as private, since it's only accessed here..
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/12771008/21002
Retried try job too often on linux_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/12771008/21002
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=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/12771008/21002
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...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/12771008/21002
Message was sent while issue was closed.
Change committed as 190639
Message was sent while issue was closed.
drive by FYI, in case you happen to still be working on this file... https://chromiumcodereview.appspot.com/12771008/diff/21002/content/public/and... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://chromiumcodereview.appspot.com/12771008/diff/21002/content/public/and... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:360: private Handler getHandler() { this whole function could be private Handler getHandler() { synchronized (mHandlerLock) { if (mHandler == null) { HandlerThead thread = new HandlerThread(); thread.start(); mHandler = new Handler(thread.getLooper()); // blocks on thread start } return mHandler; } } // and then delete setHandler()
Message was sent while issue was closed.
thanks! new patch uploaded at: https://codereview.chromium.org/14352008/ https://chromiumcodereview.appspot.com/12771008/diff/21002/content/public/and... File content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java (right): https://chromiumcodereview.appspot.com/12771008/diff/21002/content/public/and... content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:360: private Handler getHandler() { On 2013/04/17 21:02:26, joth wrote: > this whole function could be > private Handler getHandler() { > synchronized (mHandlerLock) { > if (mHandler == null) { > HandlerThead thread = new HandlerThread(); > thread.start(); > mHandler = new Handler(thread.getLooper()); // blocks on thread start > } > return mHandler; > } > } > > // and then delete setHandler() Done. |
