Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2284613002:
[sensors] Android platform adaptation for Generic Sensor API (Closed)
|
Created:
4 years, 3 months ago by shalamov Modified:
4 years, 3 months ago Reviewers:
Ken Rockot(use gerrit already), ncarter (slow), Reilly Grant (use Gerrit), timvolodine, Ted C, Tom Sepez, jochen (gone - plz use gerrit) CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), darin-cc_chromium.org, jam, Mikhail, mlamouri+watch-content_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, maksims (do not use this acc) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[sensors] Android platform adaptation for Generic Sensor API
Authors: Alexander Shalamov, Mikhail Pozdnyakov
This patch introduces implementation of generic sensor interfaces
for Android platform. Support for following sensors is added:
ambient light, gyroscope, accelerometer, linear acceleration and magnetometer.
BUG=606766
Committed: https://crrev.com/a0d874185edcfa35107b3c05fa3dba412297bdc9
Cr-Commit-Position: refs/heads/master@{#419119}
Patch Set 1 : [sensors] Android platform adaptation for Generic Sensor API #
Total comments: 4
Patch Set 2 : Remove unneeded dependencies. #
Total comments: 1
Patch Set 3 : Remove redundant virtual specifiers #
Total comments: 2
Patch Set 4 : Rebase to master, remove obsolete gn deps. #Patch Set 5 : Rebased. #
Total comments: 34
Patch Set 6 : Fixes for Ted.C review comments + unit tests. #
Total comments: 13
Patch Set 7 : Fixes for Tim's comments. #
Total comments: 27
Patch Set 8 : Fixes for comments from Ted and Tim. #Patch Set 9 : Rebase to master. #Dependent Patchsets: Messages
Total messages: 57 (26 generated)
Patchset #1 (id:1) has been deleted
PTAL
https://codereview.chromium.org/2284613002/diff/20001/chrome/common/BUILD.gn File chrome/common/BUILD.gn (right): https://codereview.chromium.org/2284613002/diff/20001/chrome/common/BUILD.gn#... chrome/common/BUILD.gn:156: "//device/generic_sensor", why is this needed? https://codereview.chromium.org/2284613002/diff/20001/content/renderer/BUILD.gn File content/renderer/BUILD.gn (right): https://codereview.chromium.org/2284613002/diff/20001/content/renderer/BUILD.... content/renderer/BUILD.gn:57: "//device/generic_sensor/public/interfaces", why is this needed?
https://codereview.chromium.org/2284613002/diff/20001/chrome/common/BUILD.gn File chrome/common/BUILD.gn (right): https://codereview.chromium.org/2284613002/diff/20001/chrome/common/BUILD.gn#... chrome/common/BUILD.gn:156: "//device/generic_sensor", On 2016/08/29 09:28:25, jochen wrote: > why is this needed? Thanks for pointing out, forgot to remove those after rebase. https://codereview.chromium.org/2284613002/diff/20001/content/renderer/BUILD.gn File content/renderer/BUILD.gn (right): https://codereview.chromium.org/2284613002/diff/20001/content/renderer/BUILD.... content/renderer/BUILD.gn:57: "//device/generic_sensor/public/interfaces", On 2016/08/29 09:28:25, jochen wrote: > why is this needed? Done.
Mojom RS LGTM on adding two new types to enum.
content/ lgtm
https://codereview.chromium.org/2284613002/diff/40001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_android.h (right): https://codereview.chromium.org/2284613002/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_android.h:40: const PlatformSensorConfiguration& configuration) override; note that 'virtual' is unnecessary if 'override' is specified (and you'll get compile errors as follows if you try:) ../../device/generic_sensor/platform_sensor_android.h:41:3: error: [chromium-style] 'virtual' is redundant; 'override' implies 'virtual'. I've kicked off some tryjobs, please have a look at the results.
The CQ bit was checked by nick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm modulo nick's style comments.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
https://codereview.chromium.org/2284613002/diff/60001/content/public/android/... File content/public/android/BUILD.gn (right): https://codereview.chromium.org/2284613002/diff/60001/content/public/android/... content/public/android/BUILD.gn:39: "//device/generic_sensor:java", sorry for not catching this earlier, but I also don't see why this line is needed?
https://codereview.chromium.org/2284613002/diff/60001/content/public/android/... File content/public/android/BUILD.gn (right): https://codereview.chromium.org/2284613002/diff/60001/content/public/android/... content/public/android/BUILD.gn:39: "//device/generic_sensor:java", On 2016/08/30 11:50:31, jochen wrote: > sorry for not catching this earlier, but I also don't see why this line is > needed? Yes, not needed after jni was introduced. Thanks jochen!
The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
I only reviewed the java code. When you add reviewers, please be specific about what files you want them to review? In particular, I'm not an OWNER of any of this code, so I didn't really know to what extent to look at. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java (right): https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:28: * defined as 200000 microseconds. forgive me, but how is 5.0 related to 200000 microseconds? There is no comment in PlatformSensorConfiguration. I'm suspecting this is the number of polls per second, but that isn't clear from this comment. Maybe this is obvious to those in the sensor world as well, so maybe that level of knowledge is expected for users of this class. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:31: private final Sensor mSensor; I would add a space between the statics and the local variables. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:34: private final int mMinDelay; what are the units here? add a Suffix (Ms, Sec, etc..) https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:37: protected GenericPlatformSensor(int sensorType, int readingCount, SensorManager sensorManager, javadoc w/ descriptions of these variables. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:43: if (sensors.isEmpty()) throw new PlatformSensorException(); we have exposed very few exceptions in our code...I wonder if we really need this and whether this could be done closer to a native style. i.e. public static GenericPlatformSensor(int sensorType, int readingCount, ...) List<Sensor> sensors = mSensorManager.getSensorList(sensorType); if (sensors.isEmpty()) return null; return new GenericPlatformSensor(...); } Or just throw an IllegalStateException or something. I think a new exception type is too much https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:52: protected void fillSensorReadingData(SensorEvent event, ByteBuffer buffer) should this be private? https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:77: if (mNativePlatformSensorAndroid == 0 || mBuffer == null || mSensorReadingData == null) { how could this be called if mNativePlatformSensorAndroid == 0 Same thing for the buffer being null? Is it valid to set up a sensor with those being null? Isn't this driven by native? Seems like these are invalid states from the java side and we shouldn't check them. if anything, just add asserts: assert mNativePlatformSensorAndroid != 0; assert mBuffer != null; But the buffer check should happen when the buffer is constructed/passed in java rather than here. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:88: mSensorManager.unregisterListener(this); Hmm...should we actually be calling this: https://developer.android.com/reference/android/hardware/SensorManager.html#u..., android.hardware.Sensor) I guess it doesn't really matter since we only listen to one sensor and you can not easily build a version that handles multiple sensor types. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:100: if (configuration.getFrequency() > 0) { is <= 0 a valid frequency? Should we prevent that somewhere else? https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:115: mSensorReadingData.putDouble(event.timestamp * 0.000001d); Make 0.000001d a constant too https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java (right): https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:18: public abstract class PlatformSensor { seeing that we are in the generic_sensor directory, do we ever see any classes that extend this other than GenericPlatformSensor? If we don't see that happening in the coming month, then I feel this is doing more abstraction/indirection than is useful. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:19: protected long mNativePlatformSensorAndroid; I think this should be private. Then expose helper functions that call the native code. Ideally, native pointers are isolated to just the one file that does native calls. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:23: protected PlatformSensor() {} you want this protected to prevent sensors from being created outside of this package? https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:28: mBuffer = buffer; I would just have another abstract method that exposes buffer and sensorReadingData to subclasses (unless we combine them) https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:34: protected abstract int getReportingMode(); javadoc for all non-private methods https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensorConfiguration.java (right): https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensorConfiguration.java:15: final class PlatformSensorConfiguration { This seems like overkill as well. why don't we just pass the frequence as a double? https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensorProvider.java (right): https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensorProvider.java:31: mSensorsThread = new HandlerThread("SensorsHandlerThread"); what thread are sensors handled on for other platforms? Also, should we be terminating this thread at some point? Should we only run this if any sensors are active?
shalamov@: - because you are asking non API specific owners for reviews, could you please add at least one of them to the device/generic_sensor/OWNERS, I am thinking e.g. reillyg@ and/or tedchoc@. That way there is also more coverage if something goes wrong. - since the start of the implementation of this API there has been *zero* testing. This seems like very ad-hoc way of working. Please add tests! Not sure why other reviewers have not mentioned this, but we usually have unit tests, instrumentation tests on Android and browser tests.
On 2016/09/02 14:07:24, timvolodine wrote: > shalamov@: > > - because you are asking non API specific owners for reviews, > could you please add at least one of them to the > device/generic_sensor/OWNERS, I am thinking e.g. reillyg@ > and/or tedchoc@. That way there is also more coverage if > something goes wrong. > > - since the start of the implementation of this API there has > been *zero* testing. This seems like very ad-hoc way of working. > Please add tests! Not sure why other reviewers have not mentioned > this, but we usually have unit tests, instrumentation tests on > Android and browser tests. @maksims (in CC) developed unit tests for device/generic_sensors base classes and will upload CL shortly. For the blink side, we have layout tests that mock concrete sensor backends and used to test binding code. Unfortunately we need bindings for concrete sensor e.g. AmbientLight before tests can be uploaded for review. I will add java unit tests for this CL early next week, after I fix review comments from Ted. Thanks for raising your concerns.
On 2016/09/02 14:51:13, shalamov wrote: > On 2016/09/02 14:07:24, timvolodine wrote: > > shalamov@: > > > > - because you are asking non API specific owners for reviews, > > could you please add at least one of them to the > > device/generic_sensor/OWNERS, I am thinking e.g. reillyg@ > > and/or tedchoc@. That way there is also more coverage if > > something goes wrong. > > > > - since the start of the implementation of this API there has > > been *zero* testing. This seems like very ad-hoc way of working. > > Please add tests! Not sure why other reviewers have not mentioned > > this, but we usually have unit tests, instrumentation tests on > > Android and browser tests. > > @maksims (in CC) developed unit tests for device/generic_sensors base classes > and will upload CL shortly. For the blink side, we have layout tests that mock > concrete sensor backends and used to test binding code. Unfortunately we need > bindings for concrete sensor e.g. AmbientLight before tests can be uploaded for > review. > I will add java unit tests for this CL early next week, after I fix review > comments from Ted. Thanks for raising your concerns. sounds good, thanks!
Patchset #6 (id:140001) has been deleted
alexander.shalamov@intel.com changed reviewers: + timvolodine@chromium.org
Thanks for comments Ted. Sorry for not being explicit about what should be checked. I remember you gave me very valuable comments last time you reviewed java code as well as this time. Tim, I added java unit tests, could you please check whether those are sufficient? https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java (right): https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:28: * defined as 200000 microseconds. On 2016/09/01 00:12:36, Ted C wrote: > forgive me, but how is 5.0 related to 200000 microseconds? There is no comment > in PlatformSensorConfiguration. > > I'm suspecting this is the number of polls per second, but that isn't clear from > this comment. Maybe this is obvious to those in the sensor world as well, so > maybe that level of knowledge is expected for users of this class. Done. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:31: private final Sensor mSensor; On 2016/09/01 00:12:36, Ted C wrote: > I would add a space between the statics and the local variables. Done. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:34: private final int mMinDelay; On 2016/09/01 00:12:36, Ted C wrote: > what are the units here? add a Suffix (Ms, Sec, etc..) Done. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:37: protected GenericPlatformSensor(int sensorType, int readingCount, SensorManager sensorManager, On 2016/09/01 00:12:35, Ted C wrote: > javadoc w/ descriptions of these variables. Done. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:43: if (sensors.isEmpty()) throw new PlatformSensorException(); On 2016/09/01 00:12:35, Ted C wrote: > we have exposed very few exceptions in our code...I wonder if we really need > this and whether this could be done closer to a native style. > > i.e. > > public static GenericPlatformSensor(int sensorType, int readingCount, ...) > List<Sensor> sensors = mSensorManager.getSensorList(sensorType); > if (sensors.isEmpty()) return null; > return new GenericPlatformSensor(...); > } > > Or just throw an IllegalStateException or something. I think a new exception > type is too much Done. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:52: protected void fillSensorReadingData(SensorEvent event, ByteBuffer buffer) On 2016/09/01 00:12:36, Ted C wrote: > should this be private? Done. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:77: if (mNativePlatformSensorAndroid == 0 || mBuffer == null || mSensorReadingData == null) { On 2016/09/01 00:12:35, Ted C wrote: > how could this be called if mNativePlatformSensorAndroid == 0 > > Same thing for the buffer being null? Is it valid to set up a sensor with those > being null? > > Isn't this driven by native? Seems like these are invalid states from the java > side and we shouldn't check them. > > if anything, just add asserts: > > assert mNativePlatformSensorAndroid != 0; > assert mBuffer != null; > > But the buffer check should happen when the buffer is constructed/passed in java > rather than here. Done. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:88: mSensorManager.unregisterListener(this); On 2016/09/01 00:12:36, Ted C wrote: > Hmm...should we actually be calling this: > https://developer.android.com/reference/android/hardware/SensorManager.html#u..., > android.hardware.Sensor) > > I guess it doesn't really matter since we only listen to one sensor and you can > not easily build a version that handles multiple sensor types. Done. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:100: if (configuration.getFrequency() > 0) { On 2016/09/01 00:12:36, Ted C wrote: > is <= 0 a valid frequency? Should we prevent that somewhere else? Checked in native side, cannot happen. Removed. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java:115: mSensorReadingData.putDouble(event.timestamp * 0.000001d); On 2016/09/01 00:12:36, Ted C wrote: > Make 0.000001d a constant too Done. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java (right): https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:18: public abstract class PlatformSensor { On 2016/09/01 00:12:36, Ted C wrote: > seeing that we are in the generic_sensor directory, do we ever see any classes > that extend this other than GenericPlatformSensor? > > If we don't see that happening in the coming month, then I feel this is doing > more abstraction/indirection than is useful. Agree, removed abstraction, if in the future non Sensor Framework, sensors would be needed, can be added back. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:19: protected long mNativePlatformSensorAndroid; On 2016/09/01 00:12:36, Ted C wrote: > I think this should be private. Then expose helper functions that call the > native code. Ideally, native pointers are isolated to just the one file that > does native calls. Done. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:23: protected PlatformSensor() {} On 2016/09/01 00:12:36, Ted C wrote: > you want this protected to prevent sensors from being created outside of this > package? added public static create() that returns null in case sensor cannot be created, will keep it as protected. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:28: mBuffer = buffer; On 2016/09/01 00:12:36, Ted C wrote: > I would just have another abstract method that exposes buffer and > sensorReadingData to subclasses (unless we combine them) not needed, since I merged classes. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:34: protected abstract int getReportingMode(); On 2016/09/01 00:12:36, Ted C wrote: > javadoc for all non-private methods Done. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensorConfiguration.java (right): https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensorConfiguration.java:15: final class PlatformSensorConfiguration { On 2016/09/01 00:12:36, Ted C wrote: > This seems like overkill as well. why don't we just pass the frequence as a > double? Removed, will consider adding it back once we have more data to be passed in configuration. https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensorProvider.java (right): https://codereview.chromium.org/2284613002/diff/120001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensorProvider.java:31: mSensorsThread = new HandlerThread("SensorsHandlerThread"); On 2016/09/01 00:12:36, Ted C wrote: > what thread are sensors handled on for other platforms? > > Also, should we be terminating this thread at some point? Should we only run > this if any sensors are active? Added code that starts / stops thread when there are active sensors.
Thanks! A few comments below. I see the other *_android.cc implementations are mostly glue for java, so not interesting to test. Also looks like other .cc unittests are covered in a separate patch. https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java (right): https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:27: public class PlatformSensor implements SensorEventListener, PlatformSensorNotifier { would it make sense to have some tests for PlatformSensor? or is this covered by the PlatformSensorProviderTest? https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... File device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java (right): https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:123: public void testSensorStartStop() { - can we have a test with multiple sensors? - e.g. stop one sensor and check that there are still active sensors, or that the sensor thread has been cleaned up. https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:143: .registerListener(any(SensorEventListener.class), any(Sensor.class), anyInt(), is it also possible to test that unregisterListener is actually called when sensors are stopped? https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:203: public void testSensorBufferFromEvent() throws NoSuchFieldException, IllegalAccessException { what does this test? maybe add a comment? https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:210: // System.out.format("Event values[0] %f", event.values[0]); should this be here? https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_android.cc (right): https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_android.cc:40: return PlatformSensorProviderAndroid::GetInstance(); Looks a bit awkward, having two singletons.. It would arguably be better to have one singleton instance somewhere owning platform provider instances, that way would also be easier to inject providers for testing e.g. in browsertests. I guess the refactoring can be done in a separate CL though.
https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java (right): https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:27: public class PlatformSensor implements SensorEventListener, PlatformSensorNotifier { On 2016/09/06 17:44:37, timvolodine wrote: > would it make sense to have some tests for PlatformSensor? or is this covered by > the PlatformSensorProviderTest? They are in the same file PlatformSensorProviderTest.java, should I rename it? I was thinking to split it, but it would require to duplicate all mocking code. https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... File device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java (right): https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:123: public void testSensorStartStop() { On 2016/09/06 17:44:37, timvolodine wrote: > - can we have a test with multiple sensors? > - e.g. stop one sensor and check that there are still active sensors, or that > the sensor thread has been cleaned up. Done. https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:143: .registerListener(any(SensorEventListener.class), any(Sensor.class), anyInt(), On 2016/09/06 17:44:37, timvolodine wrote: > is it also possible to test that unregisterListener is actually called when > sensors are stopped? Done. https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:203: public void testSensorBufferFromEvent() throws NoSuchFieldException, IllegalAccessException { On 2016/09/06 17:44:37, timvolodine wrote: > what does this test? maybe add a comment? Added comments. https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:210: // System.out.format("Event values[0] %f", event.values[0]); On 2016/09/06 17:44:37, timvolodine wrote: > should this be here? Done. https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_android.cc (right): https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_android.cc:40: return PlatformSensorProviderAndroid::GetInstance(); On 2016/09/06 17:44:38, timvolodine wrote: > Looks a bit awkward, having two singletons.. It would arguably be better to have > one singleton instance somewhere owning platform provider instances, that way > would also be easier to inject providers for testing e.g. in browsertests. I > guess the refactoring can be done in a separate CL though. I couldn't figure out how to return singleton from PlatformSensorProvider::GetInstance(), since Singleton class defines static Type* get(); as private. We still have one Singleton, per platform, it just needs to be returned by concrete PlatformSensorProviderAndroid.
thanks, PlatformSensorProviderTest.java -- lgtm % comments below https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java (right): https://codereview.chromium.org/2284613002/diff/160001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:27: public class PlatformSensor implements SensorEventListener, PlatformSensorNotifier { On 2016/09/07 13:53:42, shalamov wrote: > On 2016/09/06 17:44:37, timvolodine wrote: > > would it make sense to have some tests for PlatformSensor? or is this covered > by > > the PlatformSensorProviderTest? > > They are in the same file PlatformSensorProviderTest.java, should I rename it? I > was thinking to split it, but it would require to duplicate all mocking code. oh I see, maybe just rename it to PlatformSensorAndProviderTest.java or something like that. https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensorNotifier.java (right): https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensorNotifier.java:10: interface PlatformSensorNotifier { Looking at the delta wrt the latest upload, looks like you've silently added the PlatformSensorNotifier class? Maybe this is a small change, but in general (significant) changes after reviews are best done in follow-up patches, or at least please provide explanation when uploading a new patch. https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... File device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java (right): https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:79: }) is the indentation here correct? (looks a bit weird) https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:333: .unregisterListener(any(SensorEventListener.class), any(Sensor.class)); Regarding multiple sensors I was actually hoping for a provider test that there are no active sensors and the sensor thread is stopped/null in the end (or something equivalent). Does that make sense? The register/unregister seems to be kind of covered by the testSensorStartStop test already.
https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java (right): https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:112: protected void initPlatformSensorAndroid(long nativePlatformSensorAndroid, ByteBuffer buffer) { looks like this one could be private https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:177: protected boolean startSensor(double frequency) { just to make sure the frequency is sanity checked for reasonable values at some point during its pipeline? Want to make sure the site can't pass -1 or some ridiculously high frequency that kills your phone. https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:192: mCurrentPollingFrequency = frequency; any reason this is under the !sensorStarted check above? https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:198: if (mCurrentPollingFrequency == 0) return; should we not be registering if frequency is 0 above? https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:243: protected native void nativeNotifyPlatformSensorReadingChanged( Should these be private? Also, we put native methods at the bottom. https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensorNotifier.java (right): https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensorNotifier.java:12: public void sensorReadingChanged(); public is standard on interfaces, so you don't actually need this. But, I question whether we actually need this. Seems like we could isolate this in the test code: something along the lines of: TestPlatformSensor extends PlatformSensor { @Override public void sensorReadingChanged() { // testing code } @Override public void sensorError() { // testing code } } If we can do something like that, we're reducing the clutter of the production code. https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... File device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java (right): https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:79: }) On 2016/09/07 14:51:06, timvolodine wrote: > is the indentation here correct? (looks a bit weird) I would do: doAnswer( new Answer<List<Sensor>>() { // stuff }) .when(mSensorManager) .getSensorList(...); Or just create the Answer outside of the call to make it cleaner. https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:338: SensorEvent mockEvent = mock(SensorEvent.class); This seems more complicated than it needs to be. Why can't this be: float values[] = new float[readingValuesNum]; for (int i = 0; i < readingValuesNum; ++i) { values[i] = (float) (i + 1.0f + MILLISECONDS_IN_NANOSECOND); } SensorEvent event = new SensorEvent(values); event.timestamp = PLATFORM_SENSOR_TIMESTAMP; return event; Really, really try to avoid reflection
https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java (right): https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:177: protected boolean startSensor(double frequency) { On 2016/09/07 18:12:23, Ted C wrote: > just to make sure the frequency is sanity checked for reasonable values at some > point during its pipeline? Want to make sure the site can't pass -1 or some > ridiculously high frequency that kills your phone. It is checked on native side, during mojo type conversion and in PlatformSensorConfiguration https://cs.chromium.org/chromium/src/device/generic_sensor/public/cpp/sensor_... https://cs.chromium.org/chromium/src/device/generic_sensor/public/cpp/platfor... https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:192: mCurrentPollingFrequency = frequency; On 2016/09/07 18:12:23, Ted C wrote: > any reason this is under the !sensorStarted check above? If sensor failed to start with given configuration, then mCurrentPollingFrequency should not be equal to what was provided to startSensor(), that was the reason why I moved it after. https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:198: if (mCurrentPollingFrequency == 0) return; On 2016/09/07 18:12:23, Ted C wrote: > should we not be registering if frequency is 0 above? Native side will never provide 0 frequency, but I can add check if you want. https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensorNotifier.java (right): https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensorNotifier.java:10: interface PlatformSensorNotifier { On 2016/09/07 14:51:06, timvolodine wrote: > Looking at the delta wrt the latest upload, looks like you've silently added the > PlatformSensorNotifier class? > > Maybe this is a small change, but in general (significant) changes after reviews > are best done in follow-up patches, or at least please provide explanation when > uploading a new patch. Unfurtunately this is required by to abstract native methods in PlatformSensor, since Mockito cannot mock native methods. Without this interface important parts of PlatformSensor cannot be tested, e.g. OnSensorChanged, fillBuffer, exceptions and notifications to platform side. You have more experience with java code, maybe there is a better way to mock native methods? https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... File device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java (right): https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:79: }) On 2016/09/07 18:12:24, Ted C wrote: > On 2016/09/07 14:51:06, timvolodine wrote: > > is the indentation here correct? (looks a bit weird) > > I would do: > > doAnswer( > new Answer<List<Sensor>>() { > // stuff > }) > .when(mSensorManager) > .getSensorList(...); > > Or just create the Answer outside of the call to make it cleaner. Thanks, I will format it manually. https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:333: .unregisterListener(any(SensorEventListener.class), any(Sensor.class)); On 2016/09/07 14:51:06, timvolodine wrote: > Regarding multiple sensors I was actually hoping for a provider test that there > are no active sensors and the sensor thread is stopped/null in the end (or > something equivalent). Does that make sense? > > The register/unregister seems to be kind of covered by the testSensorStartStop > test already. Mockito / JUnit tests are run on 'testing thread', when new thread starts, mockito raises exception when verify or other methods are called. One solution is to add interface for thread management that will be used by PlatformSensorProvider and then mock it. What do you think? https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:338: SensorEvent mockEvent = mock(SensorEvent.class); On 2016/09/07 18:12:24, Ted C wrote: > This seems more complicated than it needs to be. > > Why can't this be: > > float values[] = new float[readingValuesNum]; > for (int i = 0; i < readingValuesNum; ++i) { > values[i] = (float) (i + 1.0f + MILLISECONDS_IN_NANOSECOND); > } > SensorEvent event = new SensorEvent(values); > event.timestamp = PLATFORM_SENSOR_TIMESTAMP; > return event; > > Really, really try to avoid reflection That's what I tried in the beginning, but it looks like SensorEvent constructor is not accessible "SensorEvent(int) is not public in SensorEvent; cannot be accessed from outside package." Is there a better way to do it without reflection? I can make wrapper method that takes contents of SensorEvent and then test it, would that be acceptable?
https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... File device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java (right): https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:338: SensorEvent mockEvent = mock(SensorEvent.class); On 2016/09/07 18:49:18, shalamov wrote: > On 2016/09/07 18:12:24, Ted C wrote: > > This seems more complicated than it needs to be. > > > > Why can't this be: > > > > float values[] = new float[readingValuesNum]; > > for (int i = 0; i < readingValuesNum; ++i) { > > values[i] = (float) (i + 1.0f + MILLISECONDS_IN_NANOSECOND); > > } > > SensorEvent event = new SensorEvent(values); > > event.timestamp = PLATFORM_SENSOR_TIMESTAMP; > > return event; > > > > Really, really try to avoid reflection > > That's what I tried in the beginning, but it looks like SensorEvent constructor > is not accessible "SensorEvent(int) is not public in SensorEvent; cannot be > accessed from outside package." Is there a better way to do it without > reflection? I can make wrapper method that takes contents of SensorEvent and > then test it, would that be acceptable? Oh android...can you use reflection to just make the constructor public instead? I think the combination of reflection and mocks seems a bit much if we can avoid it. And maybe leave a comment about why we need to do this so you don't get silly questions like I gave you already.
https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... File device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java (right): https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:333: .unregisterListener(any(SensorEventListener.class), any(Sensor.class)); On 2016/09/07 18:49:18, shalamov wrote: > On 2016/09/07 14:51:06, timvolodine wrote: > > Regarding multiple sensors I was actually hoping for a provider test that > there > > are no active sensors and the sensor thread is stopped/null in the end (or > > something equivalent). Does that make sense? > > > > The register/unregister seems to be kind of covered by the testSensorStartStop > > test already. > > Mockito / JUnit tests are run on 'testing thread', when new thread starts, > mockito raises exception when verify or other methods are called. One solution > is to add interface for thread management that will be used by > PlatformSensorProvider and then mock it. What do you think? Would a simpler approach to just check "mSensorsThread == null" work here? maybe by exposing a @VisibleForTesting method isSensorThreadActive() or something like that. I am pushing for this kind of test because it seems to be an important part of logic. In the general context recreating a thread is not necessarily the most viable option. What happens for example when a page goes invisible and sensors are 'suspended'? would the thread be destroyed? I am mentioning this for future consideration, this can be considered/addressed later on when the core parts are in place.
https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensorNotifier.java (right): https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensorNotifier.java:10: interface PlatformSensorNotifier { On 2016/09/07 18:49:18, shalamov wrote: > On 2016/09/07 14:51:06, timvolodine wrote: > > Looking at the delta wrt the latest upload, looks like you've silently added > the > > PlatformSensorNotifier class? > > > > Maybe this is a small change, but in general (significant) changes after > reviews > > are best done in follow-up patches, or at least please provide explanation > when > > uploading a new patch. > > Unfurtunately this is required by to abstract native methods in PlatformSensor, > since Mockito cannot mock native methods. Without this interface important parts > of PlatformSensor cannot be tested, e.g. OnSensorChanged, fillBuffer, exceptions > and notifications to platform side. You have more experience with java code, > maybe there is a better way to mock native methods? oh sorry I didn't realize it was for testing only.. If that's the case maybe try to encapsulate this interface inside the test or the PlatformSensor itself if possible. I was assuming this was some new 'functionality' or refactoring. Also see Ted's comment.
- Removed PlatformSensorNotifier. Native callbacks are tested by overriden methods in TestPlatformSensor. - Added reflection code that makes SensorEvent constructor accessible instead of removing final specifier from SensorEvent.values. - Added TestPlatformSensorProvider that overrides thread management methods, so that thread start / stop can be tested. - Renamed PlatformSensorProviderTest.java to PlatformSensorAndProviderTest.java https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java (right): https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:112: protected void initPlatformSensorAndroid(long nativePlatformSensorAndroid, ByteBuffer buffer) { On 2016/09/07 18:12:23, Ted C wrote: > looks like this one could be private I'm using it for unit tests, is there a way to make it private and still expose for unit tests? https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:243: protected native void nativeNotifyPlatformSensorReadingChanged( On 2016/09/07 18:12:23, Ted C wrote: > Should these be private? > > Also, we put native methods at the bottom. Done. https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... File device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensorNotifier.java (right): https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensorNotifier.java:10: interface PlatformSensorNotifier { On 2016/09/08 12:51:03, timvolodine wrote: > On 2016/09/07 18:49:18, shalamov wrote: > > On 2016/09/07 14:51:06, timvolodine wrote: > > > Looking at the delta wrt the latest upload, looks like you've silently added > > the > > > PlatformSensorNotifier class? > > > > > > Maybe this is a small change, but in general (significant) changes after > > reviews > > > are best done in follow-up patches, or at least please provide explanation > > when > > > uploading a new patch. > > > > Unfurtunately this is required by to abstract native methods in > PlatformSensor, > > since Mockito cannot mock native methods. Without this interface important > parts > > of PlatformSensor cannot be tested, e.g. OnSensorChanged, fillBuffer, > exceptions > > and notifications to platform side. You have more experience with java code, > > maybe there is a better way to mock native methods? > > oh sorry I didn't realize it was for testing only.. If that's the case maybe try > to encapsulate this interface inside the test or the PlatformSensor itself if > possible. I was assuming this was some new 'functionality' or refactoring. Also > see Ted's comment. Done. https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... File device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java (right): https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:79: }) On 2016/09/07 18:12:24, Ted C wrote: > On 2016/09/07 14:51:06, timvolodine wrote: > > is the indentation here correct? (looks a bit weird) > > I would do: > > doAnswer( > new Answer<List<Sensor>>() { > // stuff > }) > .when(mSensorManager) > .getSensorList(...); > > Or just create the Answer outside of the call to make it cleaner. Done. https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:333: .unregisterListener(any(SensorEventListener.class), any(Sensor.class)); On 2016/09/08 12:35:06, timvolodine wrote: > On 2016/09/07 18:49:18, shalamov wrote: > > On 2016/09/07 14:51:06, timvolodine wrote: > > > Regarding multiple sensors I was actually hoping for a provider test that > > there > > > are no active sensors and the sensor thread is stopped/null in the end (or > > > something equivalent). Does that make sense? > > > > > > The register/unregister seems to be kind of covered by the > testSensorStartStop > > > test already. > > > > Mockito / JUnit tests are run on 'testing thread', when new thread starts, > > mockito raises exception when verify or other methods are called. One solution > > is to add interface for thread management that will be used by > > PlatformSensorProvider and then mock it. What do you think? > > Would a simpler approach to just check "mSensorsThread == null" work here? maybe > by exposing a @VisibleForTesting method isSensorThreadActive() or something like > that. > > I am pushing for this kind of test because it seems to be an important part of > logic. In the general context recreating a thread is not necessarily the most > viable option. What happens for example when a page goes invisible and sensors > are 'suspended'? would the thread be destroyed? I am mentioning this for future > consideration, this can be considered/addressed later on when the core parts are > in place. Done. https://codereview.chromium.org/2284613002/diff/180001/device/generic_sensor/... device/generic_sensor/android/junit/src/org/chromium/device/sensors/PlatformSensorProviderTest.java:338: SensorEvent mockEvent = mock(SensorEvent.class); On 2016/09/07 20:08:12, Ted C wrote: > On 2016/09/07 18:49:18, shalamov wrote: > > On 2016/09/07 18:12:24, Ted C wrote: > > > This seems more complicated than it needs to be. > > > > > > Why can't this be: > > > > > > float values[] = new float[readingValuesNum]; > > > for (int i = 0; i < readingValuesNum; ++i) { > > > values[i] = (float) (i + 1.0f + MILLISECONDS_IN_NANOSECOND); > > > } > > > SensorEvent event = new SensorEvent(values); > > > event.timestamp = PLATFORM_SENSOR_TIMESTAMP; > > > return event; > > > > > > Really, really try to avoid reflection > > > > That's what I tried in the beginning, but it looks like SensorEvent > constructor > > is not accessible "SensorEvent(int) is not public in SensorEvent; cannot be > > accessed from outside package." Is there a better way to do it without > > reflection? I can make wrapper method that takes contents of SensorEvent and > > then test it, would that be acceptable? > > Oh android...can you use reflection to just make the constructor public > instead? I think the combination of reflection and mocks seems a bit > much if we can avoid it. > > And maybe leave a comment about why we need to do this so you don't > get silly questions like I gave you already. Done.
java - lgtm
The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jochen@ could you please take a look at content/public/android/BUILD.gn I started testing blink bindings to see if everything works on device and there are problems if dependency is not added to content/public/android/BUILD.gn. Unfortunately there is no java library like "device_java" where all device java classes could be placed and then content would depend on it. As you can see, other device modules add corresponding java components to content/public/android/BUILD.gn as well.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/09 06:15:51, shalamov wrote: > jochen@ could you please take a look at content/public/android/BUILD.gn > > I started testing blink bindings to see if everything works on device and there > are problems if dependency is not added to content/public/android/BUILD.gn. > > Unfortunately there is no java library like "device_java" where all device java > classes could be placed and then content would depend on it. As you can see, > other device modules add corresponding java components to > content/public/android/BUILD.gn as well. timvolodine@ jochen@ do you have more comments for this CL?
The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alexander.shalamov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, tsepez@chromium.org, reillyg@chromium.org, jochen@chromium.org, timvolodine@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2284613002/#ps220001 (title: "Rebase to master.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== [sensors] Android platform adaptation for Generic Sensor API Authors: Alexander Shalamov, Mikhail Pozdnyakov This patch introduces implementation of generic sensor interfaces for Android platform. Support for following sensors is added: ambient light, gyroscope, accelerometer, linear acceleration and magnetometer. BUG=606766 ========== to ========== [sensors] Android platform adaptation for Generic Sensor API Authors: Alexander Shalamov, Mikhail Pozdnyakov This patch introduces implementation of generic sensor interfaces for Android platform. Support for following sensors is added: ambient light, gyroscope, accelerometer, linear acceleration and magnetometer. BUG=606766 Committed: https://crrev.com/a0d874185edcfa35107b3c05fa3dba412297bdc9 Cr-Commit-Position: refs/heads/master@{#419119} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/a0d874185edcfa35107b3c05fa3dba412297bdc9 Cr-Commit-Position: refs/heads/master@{#419119} |
