Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1324853004:
Add browsertests for VibrationManager java impl on android. (Closed)
|
Created:
5 years, 3 months ago by leonhsl(Using Gerrit) Modified:
5 years, 1 month ago CC:
chromium-reviews, darin-cc_chromium.org, haltonhuo, ppi Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd browsertests for VibrationManager java impl on android.
On android VibrationManager mojo service is implemented directly with Java,
this CL adds browsertests to confirm VibrationManagerImpl behaviors.
BUG=
TEST=ContentShellTest instrumentation test PASS on android device.
Committed: https://crrev.com/ce8363056d9bd861fd768ad026104ddd0be544e9
Cr-Commit-Position: refs/heads/master@{#358286}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address review comments #
Total comments: 9
Patch Set 3 : Address comments #Patch Set 4 : Address comments #Patch Set 5 : Rebase only #Patch Set 6 : Fix trybot failure #Patch Set 7 : Fix trybot failure... #
Total comments: 12
Patch Set 8 : Address comments #
Total comments: 2
Patch Set 9 : Address comments #
Total comments: 6
Patch Set 10 : Avoid static ref to android service #
Messages
Total messages: 68 (21 generated)
leon.han@intel.com changed reviewers: + timvolodine@chromium.org
Hi, Tim, This CL adds android instrumentation test for VibrationManagerImpl, as it depends on ContentShellTestBase which resides under //content, I put the test code into //content/public/android/javatests together with other content shell test codes. PTAL, Thanks~
thanks leon.han@! I think we should aim to reduce the amount of test related code in production, hence a couple of comments regarding injecting as a fake service.. https://codereview.chromium.org/1324853004/diff/1/content/public/android/java... File content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java (right): https://codereview.chromium.org/1324853004/diff/1/content/public/android/java... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:74: @Feature({"VibrationManagerImpl"}) @Feature({"Vibration"}) would probably be more meaningful https://codereview.chromium.org/1324853004/diff/1/content/public/android/java... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:93: "Did not get vibrate mMilliSeconds correctly", fakeWrapper.mMilliSeconds, 3000); put 3000 before fakeWrapper.mMilliSeconds, that way the potential error message would be meaningful (assuming 3000 is the expected value) https://codereview.chromium.org/1324853004/diff/1/device/vibration/android/ja... File device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java (right): https://codereview.chromium.org/1324853004/diff/1/device/vibration/android/ja... device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java:36: public static class AndroidVibratorWrapper { normally we try to minimize test-related code in production, so it's best to define something like this in the ..Test file. https://codereview.chromium.org/1324853004/diff/1/device/vibration/android/ja... device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java:53: public static void setVibratorWrapperForTesting(AndroidVibratorWrapper wrapper) { I think ideally the way to test this would be to inject a fake vibration manager service into java registry during the test. The fake service would derive from this class and set mVibrator android service to a mock implementation.
adding ppi@: do we have a specific way to test java-only mojo services? I remember you worked on Battery API java mojo service but not sure if there was any testing involved.
ppi@chromium.org changed reviewers: + ppi@chromium.org
For battery afair we had java unittests and c++ browsertests (working transparently against both C++ and Java backend implementations).
On 2015/09/17 18:16:24, ppi wrote: > For battery afair we had java unittests and c++ browsertests (working > transparently against both C++ and Java backend implementations). Yeah I've created c++ browsertests (transparent against both C++ and Java implementations) for Vibration mojo service on https://codereview.chromium.org/1312123003/. As for java unittests, I think Vibration java impl is simple maybe unittests would be trivial. So I tried to figure out a way to create java browsertests for VibrationManagerImpl, and now is investigating how to override the vibration service with a VibrationManagerImpl subclass according Tim's comments.
Thanks a lot for review comments. https://codereview.chromium.org/1324853004/diff/1/content/public/android/java... File content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java (right): https://codereview.chromium.org/1324853004/diff/1/content/public/android/java... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:74: @Feature({"VibrationManagerImpl"}) On 2015/09/11 16:40:15, timvolodine wrote: > @Feature({"Vibration"}) would probably be more meaningful Done. https://codereview.chromium.org/1324853004/diff/1/content/public/android/java... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:93: "Did not get vibrate mMilliSeconds correctly", fakeWrapper.mMilliSeconds, 3000); On 2015/09/11 16:40:15, timvolodine wrote: > put 3000 before fakeWrapper.mMilliSeconds, that way the potential error message > would be meaningful (assuming 3000 is the expected value) Done. https://codereview.chromium.org/1324853004/diff/1/device/vibration/android/ja... File device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java (right): https://codereview.chromium.org/1324853004/diff/1/device/vibration/android/ja... device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java:36: public static class AndroidVibratorWrapper { On 2015/09/11 16:40:15, timvolodine wrote: > normally we try to minimize test-related code in production, so it's best to > define something like this in the ..Test file. As bellowing discussion. https://codereview.chromium.org/1324853004/diff/1/device/vibration/android/ja... device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java:53: public static void setVibratorWrapperForTesting(AndroidVibratorWrapper wrapper) { On 2015/09/11 16:40:15, timvolodine wrote: > I think ideally the way to test this would be to inject a fake vibration manager > service into java registry during the test. The fake service would derive from > this class and set mVibrator android service to a mock implementation. ContentBrowserClient provides OverrideRenderProcessMojoServices() which allows to override to inject new C++ mojo service into *ServiceRegistry* of RenderProcessHost. But as for java mojo services, they're added into the *ServiceRegistry* via JNI function registerProcessHostServices() in ServiceRegistrar.java. Java test code based on ContentShellTestBase class have no entry to override the registering process. Although I investigated around lots, did not find a good point to do this... If possible would you please help to give some hints~~
Soft ping~
I'm not sure if I understand the issue at hand. If we have C++ browsertests that work transparently for both implementations, and in particular, run on Android agains the Java impl then that sounds good. One more thing to consider would be Java unittests for details of the Java implementation but it seems you have already considered that. What is the goal of this CL? Is it testing anything that can't be tested using either the C++ browser test or Java unittest?
On 2015/10/06 21:24:50, ppi wrote: > I'm not sure if I understand the issue at hand. If we have C++ browsertests that > work transparently for both implementations, and in particular, run on Android > agains the Java impl then that sounds good. One more thing to consider would be > Java unittests for details of the Java implementation but it seems you have > already considered that. > > What is the goal of this CL? Is it testing anything that can't be tested using > either the C++ browser test or Java unittest? C++ browsertests only verifies that VibrationManager mojo service is *Exposed* correctly to the renderer by overriding the service impl with a completely fake one wrote in C++, even on android. That is to say, the VibrationManager impl never got a chance to run really.. This CL can trigger a real run of the VibrationManager java impl, and verify that the java impl *Behavior* is correct.
Thanks for the explanation. It makes sense to me, and so does the approach taken. Adding an interface to the production code so that we can mock the system data source for testing seems reasonable too. Tim is the proper owner/reviewer for that, but as far as the approach goes I don't have better ideas and this lgtm.
On 2015/10/08 20:43:01, ppi wrote: > Thanks for the explanation. It makes sense to me, and so does the approach > taken. Adding an interface to the production code so that we can mock the system > data source for testing seems reasonable too. > > Tim is the proper owner/reviewer for that, but as far as the approach goes I > don't have better ideas and this lgtm. Thanks a lot for acceptance~
going through stale patches ;)... yes looks like we don't have a way to inject java impl into the registry for testing. Hence no way of injecting fake vibration manager derived from the VibrationManagerImpl... anyway just a few comments below to hopefully simplify things. https://codereview.chromium.org/1324853004/diff/20001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java (right): https://codereview.chromium.org/1324853004/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:35: private static class FakeAndroidVibratorWrapper can we simply inject a FakeVibrator which extends android.os.Vibrator? https://codereview.chromium.org/1324853004/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:77: (Vibrator) getActivity().getSystemService(Context.VIBRATOR_SERVICE)); why the need to supply a real system vibrator for tests? https://codereview.chromium.org/1324853004/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:93: "Did not get vibrate mMilliSeconds correctly", 3000, fakeWrapper.mMilliSeconds); is this is correctly formatted? https://codereview.chromium.org/1324853004/diff/20001/device/vibration/androi... File device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java (right): https://codereview.chromium.org/1324853004/diff/20001/device/vibration/androi... device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java:36: public static class AndroidVibratorWrapper { can we just get rid of AndroidVibratorWrapper and simply use "Vibrator" instead?
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1324853004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1324853004/40001
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1324853004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1324853004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1324853004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1324853004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1324853004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1324853004/100001
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1324853004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1324853004/120001
Thanks a lot for kindly review~ Addressed comments and uploaded patch#7, sorry for so many uploads... https://codereview.chromium.org/1324853004/diff/20001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java (right): https://codereview.chromium.org/1324853004/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:35: private static class FakeAndroidVibratorWrapper On 2015/10/16 17:41:24, timvolodine wrote: > can we simply inject a FakeVibrator which extends android.os.Vibrator? Yeah I thought about this idea before but found that android.os.Vibrator is an abstract class which holds a private constructor, usually its instance for impl is got via Context.getSystemService() API. So we can't extend it.. https://codereview.chromium.org/1324853004/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:77: (Vibrator) getActivity().getSystemService(Context.VIBRATOR_SERVICE)); On 2015/10/16 17:41:24, timvolodine wrote: > why the need to supply a real system vibrator for tests? Yeah no need for this, modified. https://codereview.chromium.org/1324853004/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:93: "Did not get vibrate mMilliSeconds correctly", 3000, fakeWrapper.mMilliSeconds); On 2015/10/16 17:41:24, timvolodine wrote: > is this is correctly formatted? It is formatted by 'git cl format', seems in this case the 8 spaces indent is correct. https://codereview.chromium.org/1324853004/diff/20001/device/vibration/androi... File device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java (right): https://codereview.chromium.org/1324853004/diff/20001/device/vibration/androi... device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java:36: public static class AndroidVibratorWrapper { On 2015/10/16 17:41:24, timvolodine wrote: > can we just get rid of AndroidVibratorWrapper and simply use "Vibrator" instead? As we can't extend os.android.Vibrator, so, to enable injection, we have to use the wrapper..
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm modulo comments below. thanks! https://codereview.chromium.org/1324853004/diff/20001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java (right): https://codereview.chromium.org/1324853004/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:35: private static class FakeAndroidVibratorWrapper On 2015/10/19 10:27:37, leon.han wrote: > On 2015/10/16 17:41:24, timvolodine wrote: > > can we simply inject a FakeVibrator which extends android.os.Vibrator? > > Yeah I thought about this idea before but found that android.os.Vibrator is an > abstract class which holds a private constructor, usually its instance for impl > is got via Context.getSystemService() API. So we can't extend it.. ah sorry didn't realize the Vibrator was not meant to be subclassed outside android framework. :/ https://codereview.chromium.org/1324853004/diff/120001/content/public/android... File content/public/android/BUILD.gn (right): https://codereview.chromium.org/1324853004/diff/120001/content/public/android... content/public/android/BUILD.gn:188: "//device/vibration/android:vibration_manager_android", no similar gyp changes? https://codereview.chromium.org/1324853004/diff/120001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java (right): https://codereview.chromium.org/1324853004/diff/120001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:74: VibrationManagerImpl.setVibratorWrapperForTesting(fakeWrapper); put these 2 lines in setUp()? applies to the second test as well https://codereview.chromium.org/1324853004/diff/120001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:77: assertFalse(fakeWrapper.mCancelled); maybe also these two lines as well https://codereview.chromium.org/1324853004/diff/120001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:79: loadNewShell(URL_VIBRATOR_VIBRATE); nit: empty line after for readability https://codereview.chromium.org/1324853004/diff/120001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:101: VibrationManagerImpl.setVibratorWrapperForTesting(fakeWrapper); same here https://codereview.chromium.org/1324853004/diff/120001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:104: assertFalse(fakeWrapper.mCancelled); and here
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1324853004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1324853004/140001
Thanks a lot for great help! ;) Addressed comments and uploaded patch set#8. https://codereview.chromium.org/1324853004/diff/120001/content/public/android... File content/public/android/BUILD.gn (right): https://codereview.chromium.org/1324853004/diff/120001/content/public/android... content/public/android/BUILD.gn:188: "//device/vibration/android:vibration_manager_android", On 2015/10/20 13:16:22, timvolodine wrote: > no similar gyp changes? Yeah gyp has no need to do so. gyp has no "content_javatests" target but instead it embeds the build into "//content/content_tests.gypi:content_shell_test_apk", which has already dependency on //device/vibration. https://codereview.chromium.org/1324853004/diff/120001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java (right): https://codereview.chromium.org/1324853004/diff/120001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:74: VibrationManagerImpl.setVibratorWrapperForTesting(fakeWrapper); On 2015/10/20 13:16:22, timvolodine wrote: > put these 2 lines in setUp()? applies to the second test as well Done. https://codereview.chromium.org/1324853004/diff/120001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:77: assertFalse(fakeWrapper.mCancelled); On 2015/10/20 13:16:22, timvolodine wrote: > maybe also these two lines as well Done. https://codereview.chromium.org/1324853004/diff/120001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:79: loadNewShell(URL_VIBRATOR_VIBRATE); On 2015/10/20 13:16:22, timvolodine wrote: > nit: empty line after for readability Done. https://codereview.chromium.org/1324853004/diff/120001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:101: VibrationManagerImpl.setVibratorWrapperForTesting(fakeWrapper); On 2015/10/20 13:16:22, timvolodine wrote: > same here Done. https://codereview.chromium.org/1324853004/diff/120001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:104: assertFalse(fakeWrapper.mCancelled); On 2015/10/20 13:16:22, timvolodine wrote: > and here Done.
still lgtm with a comment. also, you'd probably need other owners to lgtm. https://codereview.chromium.org/1324853004/diff/140001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java (right): https://codereview.chromium.org/1324853004/diff/140001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:65: VibrationManagerImpl.setVibratorWrapperForTesting(mFakeWrapper); hmm does that actually work? probably better to create a new fake vibrator in setUp as well..
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 leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1324853004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1324853004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Addressed comments and upload again. Thanks a lot~ https://codereview.chromium.org/1324853004/diff/140001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java (right): https://codereview.chromium.org/1324853004/diff/140001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:65: VibrationManagerImpl.setVibratorWrapperForTesting(mFakeWrapper); On 2015/10/22 11:26:13, timvolodine wrote: > hmm does that actually work? probably better to create a new fake vibrator in > setUp as well.. Done.
leon.han@intel.com changed reviewers: + jam@chromium.org
Hi, jam@, Would you please take a look for Owner review? Thanks~
On 2015/10/25 05:59:20, leon.han wrote: > Hi, jam@, Would you please take a look for Owner review? Thanks~ please pick a more specific owner. i.e. from device/OWNERS and content/public/android/OWNERS
On 2015/10/26 17:18:52, jam wrote: > On 2015/10/25 05:59:20, leon.han wrote: > > Hi, jam@, Would you please take a look for Owner review? Thanks~ > > please pick a more specific owner. i.e. from device/OWNERS and > content/public/android/OWNERS Understood. Thanks~
leon.han@intel.com changed reviewers: + dtrainor@chromium.org - jam@chromium.org
Hi, dtrainor@, would you please help do Owner review for //content/public/android, Thanks~
leon.han@intel.com changed reviewers: + tedchoc@chromium.org
Added tedchoc@ . Hi, Ted, would you please help to take a look as //content/public/android Owner? Thanks a lot~
https://chromiumcodereview.appspot.com/1324853004/diff/160001/device/vibratio... File device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java (right): https://chromiumcodereview.appspot.com/1324853004/diff/160001/device/vibratio... device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java:61: (Vibrator) context.getSystemService(Context.VIBRATOR_SERVICE)); If this is a static, should we make *sure* this is the application context we're calling this on? I'm not 100% sure it's a problem, but I wouldn't want to leak an activity forever here.
https://codereview.chromium.org/1324853004/diff/160001/content/public/android... File content/public/android/BUILD.gn (right): https://codereview.chromium.org/1324853004/diff/160001/content/public/android... content/public/android/BUILD.gn:188: "//device/vibration/android:vibration_manager_android", are these actually needed? It looks like they are transitvely included by content_java_test_support content_java_test_support -> //content/public/android:content_java -> "//device/vibration/android:vibration_manager_android" && "//device/vibration:mojo_bindings_java", https://codereview.chromium.org/1324853004/diff/160001/device/vibration/andro... File device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java (right): https://codereview.chromium.org/1324853004/diff/160001/device/vibration/andro... device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java:61: (Vibrator) context.getSystemService(Context.VIBRATOR_SERVICE)); On 2015/11/02 16:43:51, David Trainor wrote: > If this is a static, should we make *sure* this is the application context we're > calling this on? I'm not 100% sure it's a problem, but I wouldn't want to leak > an activity forever here. If you want to avoid keeping a static like this, you could make the Wrapper not actually take the Vibrator in the constructor but in both methods. Then the wrapper/helper/delegate could be overridden with something that does not operate on it. And you're not keeping a static ref to a service. public static class AndroidVibratorWrapper { public void vibrate(Vibrator vibrator, long milliseconds) { ... } public void cancel(Vibrator vibrator) { ... } }
https://codereview.chromium.org/1324853004/diff/160001/content/public/android... File content/public/android/BUILD.gn (right): https://codereview.chromium.org/1324853004/diff/160001/content/public/android... content/public/android/BUILD.gn:188: "//device/vibration/android:vibration_manager_android", On 2015/11/02 23:16:47, Ted C wrote: > are these actually needed? It looks like they are transitvely included by > content_java_test_support > > content_java_test_support -> //content/public/android:content_java -> > "//device/vibration/android:vibration_manager_android" && > "//device/vibration:mojo_bindings_java", Yeah the deps are there.. But without this change the android gn trybot always failed because VibrationManagerImpl class definition can't be located.. https://codereview.chromium.org/1324853004/diff/160001/device/vibration/andro... File device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java (right): https://codereview.chromium.org/1324853004/diff/160001/device/vibration/andro... device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java:61: (Vibrator) context.getSystemService(Context.VIBRATOR_SERVICE)); On 2015/11/02 23:16:47, Ted C wrote: > On 2015/11/02 16:43:51, David Trainor wrote: > > If this is a static, should we make *sure* this is the application context > we're > > calling this on? I'm not 100% sure it's a problem, but I wouldn't want to > leak > > an activity forever here. > > If you want to avoid keeping a static like this, you could make the Wrapper not > actually take the Vibrator in the constructor but in both methods. Then the > wrapper/helper/delegate could be overridden with something that does not operate > on it. And you're not keeping a static ref to a service. > > public static class AndroidVibratorWrapper { > public void vibrate(Vibrator vibrator, long milliseconds) { > ... > } > > public void cancel(Vibrator vibrator) { > ... > } > } Hi, David, Ted, Thank you very much for helpful advice~ VibrationManagerImpl is a mojo service impl which will be created via a predefined/fixed route, it is guaranteed that the context here is always the application context. Although it wouldn't be a problem, maybe we should take Ted's advice to avoid keeping a static ref to an android service? It would be more flexible for possible future changes. Hi, Tim, Ppi, WDYT? Thanks~
https://codereview.chromium.org/1324853004/diff/160001/device/vibration/andro... File device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java (right): https://codereview.chromium.org/1324853004/diff/160001/device/vibration/andro... device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java:61: (Vibrator) context.getSystemService(Context.VIBRATOR_SERVICE)); On 2015/11/03 08:56:26, leon.han wrote: > On 2015/11/02 23:16:47, Ted C wrote: > > On 2015/11/02 16:43:51, David Trainor wrote: > > > If this is a static, should we make *sure* this is the application context > > we're > > > calling this on? I'm not 100% sure it's a problem, but I wouldn't want to > > leak > > > an activity forever here. > > > > If you want to avoid keeping a static like this, you could make the Wrapper > not > > actually take the Vibrator in the constructor but in both methods. Then the > > wrapper/helper/delegate could be overridden with something that does not > operate > > on it. And you're not keeping a static ref to a service. > > > > public static class AndroidVibratorWrapper { > > public void vibrate(Vibrator vibrator, long milliseconds) { > > ... > > } > > > > public void cancel(Vibrator vibrator) { > > ... > > } > > } > > Hi, David, Ted, Thank you very much for helpful advice~ > VibrationManagerImpl is a mojo service impl which will be created via a > predefined/fixed route, it is guaranteed that the context here is always the > application context. > Although it wouldn't be a problem, maybe we should take Ted's advice to avoid > keeping a static ref to an android service? It would be more flexible for > possible future changes. > > Hi, Tim, Ppi, WDYT? Thanks~ Ted's suggestion sg to me.
On 2015/11/03 13:27:11, ppi wrote: > https://codereview.chromium.org/1324853004/diff/160001/device/vibration/andro... > File > device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java > (right): > > https://codereview.chromium.org/1324853004/diff/160001/device/vibration/andro... > device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java:61: > (Vibrator) context.getSystemService(Context.VIBRATOR_SERVICE)); > On 2015/11/03 08:56:26, leon.han wrote: > > On 2015/11/02 23:16:47, Ted C wrote: > > > On 2015/11/02 16:43:51, David Trainor wrote: > > > > If this is a static, should we make *sure* this is the application context > > > we're > > > > calling this on? I'm not 100% sure it's a problem, but I wouldn't want to > > > leak > > > > an activity forever here. > > > > > > If you want to avoid keeping a static like this, you could make the Wrapper > > not > > > actually take the Vibrator in the constructor but in both methods. Then the > > > wrapper/helper/delegate could be overridden with something that does not > > operate > > > on it. And you're not keeping a static ref to a service. > > > > > > public static class AndroidVibratorWrapper { > > > public void vibrate(Vibrator vibrator, long milliseconds) { > > > ... > > > } > > > > > > public void cancel(Vibrator vibrator) { > > > ... > > > } > > > } > > > > Hi, David, Ted, Thank you very much for helpful advice~ > > VibrationManagerImpl is a mojo service impl which will be created via a > > predefined/fixed route, it is guaranteed that the context here is always the > > application context. > > Although it wouldn't be a problem, maybe we should take Ted's advice to avoid > > keeping a static ref to an android service? It would be more flexible for > > possible future changes. > > > > Hi, Tim, Ppi, WDYT? Thanks~ > > Ted's suggestion sg to me. sgtm
Uploaded patchset#10 to address Ted's comments. PTAL, Thanks~
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1324853004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1324853004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm!
lgtm
The CQ bit was checked by leon.han@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from ppi@chromium.org, timvolodine@chromium.org Link to the patchset: https://codereview.chromium.org/1324853004/#ps180001 (title: "Avoid static ref to android service")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1324853004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1324853004/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/ce8363056d9bd861fd768ad026104ddd0be544e9 Cr-Commit-Position: refs/heads/master@{#358286} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
