[DeviceService] Add service tests for VibrationManager.

[DeviceService] Add service tests for VibrationManager.

VibrationManagerImpl.java is in //device/vibration while the
VibrationManagerImplTest.java is still in //content,
to be able to hide all //device/vibration impl inside Device Service,
we must eliminate this concrete dependency on //device/vibration from
content layer.

As VibrationManagerImplTest.java is an integration test based on
content shell test infra, we can't move it outside of //content.
So we consider to use other tests to replace it, while the renderer-side
testing has already been covered by layout tests created at
https://codereview.chromium.org/2731953003/, so we create this CL to
cover the VibrationManager interface impl testing:
 - creates an infra for Device Service service tests.
 - adds a new service test for VibrationManager interface.

Then after this CL we'll be able to hide all vibration impl inside
Device Service with a follow-up CL.

BUG=689379
TEST=service_unittests

Review-Url: https://codereview.chromium.org/2774783003
Cr-Commit-Position: refs/heads/master@{#463612}
Committed: https://chromium.googlesource.com/chromium/src/+/9c17bee62f1e16224fabb7c83bbeaab719015761

[leonhsl(Using Gerrit), 2017-03-24 11:48:36]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 11:48:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 11:52:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 11:52:23]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[leonhsl(Using Gerrit), 2017-03-26 07:23:01]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-26 07:23:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-26 07:24:41]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-26 07:24:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-26 07:25:36]

Patchset #2 (id:20001) has been deleted

[leonhsl(Using Gerrit), 2017-03-26 07:25:44]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-03-26 07:38:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-26 07:38:12]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[leonhsl(Using Gerrit), 2017-03-26 09:21:16]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-26 09:21:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-26 09:38:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-26 09:38:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[leonhsl(Using Gerrit), 2017-03-26 10:57:39]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-26 10:57:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-26 12:02:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-26 12:02:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[leonhsl(Using Gerrit), 2017-03-26 14:44:08]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-26 14:44:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-26 14:59:28]

Patchset #1 (id:40001) has been deleted

[leonhsl(Using Gerrit), 2017-03-26 14:59:48]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-26 14:59:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-26 15:00:15]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-03-26 15:36:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-26 15:36:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[leonhsl(Using Gerrit), 2017-03-27 02:25:01]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 02:25:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-27 02:41:23]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 02:41:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-27 02:52:07]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 02:52:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-27 02:54:31]

Patchset #1 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-03-27 04:14:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 04:14:43]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[leonhsl(Using Gerrit), 2017-03-27 04:16:30]

Description was changed from

==========
[DeviceService] Add service tests for VibrationManager.

BUG=689379
TEST=service_unittests
==========

to

==========
[DeviceService] Add service tests for VibrationManager.

VibrationManagerImpl.java is in //device/vibration while the
VibrationManagerImplTest.java is still in //content,
to be able to hide all //device/vibration impl inside Device Service,
we must eliminate this concrete dependency on //device/vibration from
content layer.

As VibrationManagerImplTest.java is an integration test based on
content shell test infra, we can't move it outside of //content.
So we consider to use other tests to replace it, while the renderer-side
testing has already been covered by layout tests created at
https://codereview.chromium.org/2731953003/, so we create this CL to
cover the VibrationManager interface impl testing:
 - creates an infra for Device Service service tests.
 - adds a new service test for VibrationManager interface.

Then after this CL we'll be able to hide all vibration impl inside
Device Service with a follow-up CL.

BUG=689379
TEST=service_unittests
==========

[leonhsl(Using Gerrit), 2017-03-27 04:18:48]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 04:19:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 05:35:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 05:35:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[leonhsl(Using Gerrit), 2017-03-27 05:49:14]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 05:49:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-27 06:01:08]

Patchset #3 (id:140001) has been deleted

[leonhsl(Using Gerrit), 2017-03-27 06:01:34]

Patchset #1 (id:100001) has been deleted

[leonhsl(Using Gerrit), 2017-03-27 06:49:30]

Patchset #2 (id:130013) has been deleted

[leonhsl(Using Gerrit), 2017-03-27 07:00:44]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 07:00:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-27 07:02:20]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[leonhsl(Using Gerrit), 2017-03-27 07:02:26]

Patchset #2 (id:170001) has been deleted

[commit-bot: I haz the power, 2017-03-27 07:02:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-27 07:04:59]

Patchset #1 (id:120001) has been deleted

[leonhsl(Using Gerrit), 2017-03-27 08:03:37]

Patchset #2 (id:210001) has been deleted

[leonhsl(Using Gerrit), 2017-03-27 08:16:02]

leon.han@intel.com changed reviewers:
+ blundell@chromium.org

[leonhsl(Using Gerrit), 2017-03-27 08:16:03]

Hi, Colin, I thought ps#1 should be able to work well but our service_unittests
is actually failing on Android bot. Because of some reasons of my Dev env, now
I'm still preparing the env to run service_unittests locally by myself so that I
can see more useful logs to find out the root cause.

Anyway PTAL if you have time :) I suspect we need some other work to enable JNI
to run well within a gtest apk.. I'm checking around.

[leonhsl(Using Gerrit), 2017-03-28 05:34:07]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 05:34:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-28 05:36:30]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[leonhsl(Using Gerrit), 2017-03-28 05:36:39]

Patchset #2 (id:230001) has been deleted

[leonhsl(Using Gerrit), 2017-03-28 05:36:56]

Patchset #1 (id:190001) has been deleted

[commit-bot: I haz the power, 2017-03-28 05:36:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 05:41:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 05:41:18]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[leonhsl(Using Gerrit), 2017-03-28 05:50:35]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[leonhsl(Using Gerrit), 2017-03-28 05:50:54]

Patchset #1 (id:250001) has been deleted

[commit-bot: I haz the power, 2017-03-28 05:51:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 06:25:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 06:25:35]

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...)

[leonhsl(Using Gerrit), 2017-03-28 06:29:38]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[leonhsl(Using Gerrit), 2017-03-28 06:29:49]

Patchset #1 (id:270001) has been deleted

[commit-bot: I haz the power, 2017-03-28 06:29:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-28 07:27:52]

Hi, Colin, I've solved the failure on Android bot and updated CL, PTAL, Thanks!

https://codereview.chromium.org/2774783003/diff/290001/services/device/androi...
File services/device/android/register_jni.cc (right):

https://codereview.chromium.org/2774783003/diff/290001/services/device/androi...
services/device/android/register_jni.cc:17: const
base::android::RegistrationMethod kRegisteredMethods[] = {
We must register these JNIs to enable
InterfaceRegistrar::createInterfaceRegistryForContext to work well when running
service_unittests apk on Android.

Of course when Device Service is embedded by content_browser, content_browser
also registers these JNIs, but I think duplicated registration here has no bad
effect and Device Service should register all its necessary JNIs by itself.
WDYT? Thanks.

[commit-bot: I haz the power, 2017-03-28 08:55:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 08:55:53]

Dry run: This issue passed the CQ dry run.

[blundell, 2017-03-28 12:40:47]

This looks great!

https://codereview.chromium.org/2774783003/diff/290001/services/device/androi...
File services/device/android/register_jni.cc (right):

https://codereview.chromium.org/2774783003/diff/290001/services/device/androi...
services/device/android/register_jni.cc:17: const
base::android::RegistrationMethod kRegisteredMethods[] = {
On 2017/03/28 07:27:52, leonhsl wrote:
> We must register these JNIs to enable
> InterfaceRegistrar::createInterfaceRegistryForContext to work well when
running
> service_unittests apk on Android.
> 
> Of course when Device Service is embedded by content_browser, content_browser
> also registers these JNIs, but I think duplicated registration here has no bad
> effect and Device Service should register all its necessary JNIs by itself.
> WDYT? Thanks.

It seems like we should put this somewhere inside //mojo where it will just
naturally be depended on by any target that needs it, similar to e.g.
base_jni_registrar.* in //base. Is there some reason that won't work?

https://codereview.chromium.org/2774783003/diff/290001/services/device/device...
File services/device/device_service_test_base.cc (right):

https://codereview.chromium.org/2774783003/diff/290001/services/device/device...
services/device/device_service_test_base.cc:44: // TODO(leonhsl): Prepare
file/io task runner for Device Service.
Can we do this in this CL?

[leonhsl(Using Gerrit), 2017-03-29 06:32:01]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 06:32:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 07:11:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 07:11:32]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[leonhsl(Using Gerrit), 2017-03-29 07:37:16]

Hi, Colin, PTAnL at ps#2, the red trybots now are not caused by this CL, Thanks!

https://codereview.chromium.org/2774783003/diff/290001/services/device/androi...
File services/device/android/register_jni.cc (right):

https://codereview.chromium.org/2774783003/diff/290001/services/device/androi...
services/device/android/register_jni.cc:17: const
base::android::RegistrationMethod kRegisteredMethods[] = {
On 2017/03/28 12:40:47, blundell wrote:
> On 2017/03/28 07:27:52, leonhsl wrote:
> > We must register these JNIs to enable
> > InterfaceRegistrar::createInterfaceRegistryForContext to work well when
> running
> > service_unittests apk on Android.
> > 
> > Of course when Device Service is embedded by content_browser,
content_browser
> > also registers these JNIs, but I think duplicated registration here has no
bad
> > effect and Device Service should register all its necessary JNIs by itself.
> > WDYT? Thanks.
> 
> It seems like we should put this somewhere inside //mojo where it will just
> naturally be depended on by any target that needs it, similar to e.g.
> base_jni_registrar.* in //base. Is there some reason that won't work?

To make sure I understand the suggestion clearly:

When any one target X needs to use JNIs of //base, usually
  - target X depends on //base which packages base_jni_registrar.* and needed
C++ sources, and calls base::android::RegisterJni() on initiation.
  - corresponding android_library target X_java depends on //base:base_java
which packages needed java sources.

For Mojo case, it's similar that
  - X needs to depend on //mojo/android:libsystem_java which packages needed C++
sources, and calls both mojo::android::RegisterCoreImpl() and
mojo::android::RegisterWatcherImpl().
  - android_library X_java needs to depend on //mojo/android:system_java which
packages needed java sources.

So, the only difference with //base case is that //mojo/android:libsystem_java
is not exposing a mojo::android::RegisterJni() API consolidating calls to
mojo::android::RegisterCoreImpl() and mojo::android::RegisterWatcherImpl().

Do you mean that we'd better provide such a mojo::android::RegisterJni()?
Thanks!
If so I'll create another CL to do this task because it will also touch other
//mojo/android users unrelated with Device Service.

https://codereview.chromium.org/2774783003/diff/290001/services/device/device...
File services/device/device_service_test_base.cc (right):

https://codereview.chromium.org/2774783003/diff/290001/services/device/device...
services/device/device_service_test_base.cc:44: // TODO(leonhsl): Prepare
file/io task runner for Device Service.
On 2017/03/28 12:40:47, blundell wrote:
> Can we do this in this CL?

Done. Before I was considering to do this when we actually need them in future
test case development :)

[leonhsl(Using Gerrit), 2017-03-29 07:37:30]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 07:37:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 09:20:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 09:20:05]

Dry run: This issue passed the CQ dry run.

[blundell, 2017-03-29 10:05:13]

On 2017/03/29 07:37:16, leonhsl wrote:
> Hi, Colin, PTAnL at ps#2, the red trybots now are not caused by this CL,
Thanks!
> 
>
https://codereview.chromium.org/2774783003/diff/290001/services/device/androi...
> File services/device/android/register_jni.cc (right):
> 
>
https://codereview.chromium.org/2774783003/diff/290001/services/device/androi...
> services/device/android/register_jni.cc:17: const
> base::android::RegistrationMethod kRegisteredMethods[] = {
> On 2017/03/28 12:40:47, blundell wrote:
> > On 2017/03/28 07:27:52, leonhsl wrote:
> > > We must register these JNIs to enable
> > > InterfaceRegistrar::createInterfaceRegistryForContext to work well when
> > running
> > > service_unittests apk on Android.
> > > 
> > > Of course when Device Service is embedded by content_browser,
> content_browser
> > > also registers these JNIs, but I think duplicated registration here has no
> bad
> > > effect and Device Service should register all its necessary JNIs by
itself.
> > > WDYT? Thanks.
> > 
> > It seems like we should put this somewhere inside //mojo where it will just
> > naturally be depended on by any target that needs it, similar to e.g.
> > base_jni_registrar.* in //base. Is there some reason that won't work?
> 
> To make sure I understand the suggestion clearly:
> 
> When any one target X needs to use JNIs of //base, usually
>   - target X depends on //base which packages base_jni_registrar.* and needed
> C++ sources, and calls base::android::RegisterJni() on initiation.
>   - corresponding android_library target X_java depends on //base:base_java
> which packages needed java sources.
> 
> For Mojo case, it's similar that
>   - X needs to depend on //mojo/android:libsystem_java which packages needed
C++
> sources, and calls both mojo::android::RegisterCoreImpl() and
> mojo::android::RegisterWatcherImpl().
>   - android_library X_java needs to depend on //mojo/android:system_java which
> packages needed java sources.
> 
> So, the only difference with //base case is that //mojo/android:libsystem_java
> is not exposing a mojo::android::RegisterJni() API consolidating calls to
> mojo::android::RegisterCoreImpl() and mojo::android::RegisterWatcherImpl().
> 
> Do you mean that we'd better provide such a mojo::android::RegisterJni()?
> Thanks!
> If so I'll create another CL to do this task because it will also touch other
> //mojo/android users unrelated with Device Service.

Ah I see, I had somewhat misunderstood the situation. Two thoughts:

(1) Yes, what you're suggesting seems like a good idea.
(2) I don't think that the Device Service should be responsible for registering
the core Mojo JNI, since if we're using the Device Service, that already implies
that the app in question is using Mojo. For this test context, I would register
the JNI in //services/service_manager/public/cpp/test/run_all_service_tests.cc
assuming that that works.
> 
>
https://codereview.chromium.org/2774783003/diff/290001/services/device/device...
> File services/device/device_service_test_base.cc (right):
> 
>
https://codereview.chromium.org/2774783003/diff/290001/services/device/device...
> services/device/device_service_test_base.cc:44: // TODO(leonhsl): Prepare
> file/io task runner for Device Service.
> On 2017/03/28 12:40:47, blundell wrote:
> > Can we do this in this CL?
> 
> Done. Before I was considering to do this when we actually need them in future
> test case development :)

[leonhsl(Using Gerrit), 2017-03-30 10:03:55]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 10:04:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-30 10:25:29]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 10:25:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-30 12:24:21]

> Ah I see, I had somewhat misunderstood the situation. Two thoughts:
> 
> (1) Yes, what you're suggesting seems like a good idea.

Uploaded another CL to do this work:
https://codereview.chromium.org/2784493003#ps40001

> (2) I don't think that the Device Service should be responsible for
registering
> the core Mojo JNI, since if we're using the Device Service, that already
implies
> that the app in question is using Mojo. For this test context, I would
register
> the JNI in //services/service_manager/public/cpp/test/run_all_service_tests.cc
> assuming that that works.

Uploaded ps#4 to let service_test app itself register mojo JNIs, PTAnL, Thanks!

[commit-bot: I haz the power, 2017-03-30 12:47:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 12:47:19]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[leonhsl(Using Gerrit), 2017-03-30 12:53:08]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 12:53:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 15:14:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 15:14:59]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[blundell, 2017-03-31 11:22:30]

lgtm, thanks!

https://codereview.chromium.org/2774783003/diff/310001/services/device/device...
File services/device/device_service_test_base.cc (right):

https://codereview.chromium.org/2774783003/diff/310001/services/device/device...
services/device/device_service_test_base.cc:74: base::Thread io_thread_;
nit: These should be moved above line 72 so that they outlive
|device_service_context_|.

[leonhsl(Using Gerrit), 2017-03-31 12:14:44]

leon.han@intel.com changed reviewers:
+ avi@chromium.org, rockot@chromium.org

[leonhsl(Using Gerrit), 2017-03-31 12:14:46]

PTAL, Thanks!

Ken: OWNER review of services/* and device/*
  # services/device/* have been stamped by Colin.

Avi: OWNER review of content/*

https://codereview.chromium.org/2774783003/diff/310001/services/device/device...
File services/device/device_service_test_base.cc (right):

https://codereview.chromium.org/2774783003/diff/310001/services/device/device...
services/device/device_service_test_base.cc:74: base::Thread io_thread_;
On 2017/03/31 11:22:30, blundell wrote:
> nit: These should be moved above line 72 so that they outlive
> |device_service_context_|.

Acknowledged.

[Avi (use Gerrit), 2017-03-31 14:21:40]

lgtm

stampy

[leonhsl(Using Gerrit), 2017-04-05 02:41:44]

leon.han@intel.com changed reviewers:
+ jam@chromium.org, timvolodine@chromium.org

[leonhsl(Using Gerrit), 2017-04-05 02:41:45]

+jam@ +timvolodine@ Would you PTAL at this? Thanks!

John: 
OWNER review for 
services/device/BUILD.gn
services/service_manager/public/cpp/test/BUILD.gn
services/service_manager/public/cpp/test/run_all_service_tests.cc

Tim:  
OWNER review for device/vibration/*

[timvolodine, 2017-04-05 17:16:29]

Hi Leon, regarding device/vibration a question below:

https://codereview.chromium.org/2774783003/diff/350001/device/vibration/andro...
File
device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java
(right):

https://codereview.chromium.org/2774783003/diff/350001/device/vibration/andro...
device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java:35:
private static boolean sVibrateCancelledForTesting = false;
Is there any particular reason for eliminating the Wrapper here? Exposing
testable primitive values / static methods (as below) in production code here
and in other places does not seem very clean and scalable..

[leonhsl(Using Gerrit), 2017-04-06 03:19:16]

Hi, Tim, Thanks a lot for review!

https://codereview.chromium.org/2774783003/diff/350001/device/vibration/andro...
File
device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java
(right):

https://codereview.chromium.org/2774783003/diff/350001/device/vibration/andro...
device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java:35:
private static boolean sVibrateCancelledForTesting = false;
On 2017/04/05 17:16:28, timvolodine wrote:
> Is there any particular reason for eliminating the Wrapper here? Exposing
> testable primitive values / static methods (as below) in production code here
> and in other places does not seem very clean and scalable..

We were creating/setting this AndroidVibratorWrapper from Java testing code
before, but now we're in C++ testing code, seems it's not so easy to do that
like before, so I chose such a way considering that the 2 static testing
variables are so simple and with small size..

[timvolodine, 2017-04-07 10:39:07]

some more comments:

https://codereview.chromium.org/2774783003/diff/350001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java
(left):

https://codereview.chromium.org/2774783003/diff/350001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:29:
public class VibrationManagerImplTest {
If there is no way to provide a similar end-to-end test in device/ then this
test should probably stay where it is..

https://codereview.chromium.org/2774783003/diff/350001/device/vibration/andro...
File
device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java
(right):

https://codereview.chromium.org/2774783003/diff/350001/device/vibration/andro...
device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java:64:
mVibrator.vibrate(sanitizedMilliseconds);
Will the device actually vibrate when testing?

https://codereview.chromium.org/2774783003/diff/350001/device/vibration/andro...
device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java:66:
setVibrateMilliSecondsForTesting(sanitizedMilliseconds);
I believe we generally try to avoid mixing production and explicit testing code.

https://codereview.chromium.org/2774783003/diff/350001/services/device/vibrat...
File services/device/vibration/vibration_manager_impl_unittest.cc (right):

https://codereview.chromium.org/2774783003/diff/350001/services/device/vibrat...
services/device/vibration/vibration_manager_impl_unittest.cc:21: class
VibrationManagerImplTest : public DeviceServiceTestBase {
If I understand correctly the idea here is to test the mojo impl setting of
value and execution of callback.. perhaps the existing wrapper in the java code
could be reused for that purpose with a java test (can we do that?) since the
default impl doesn't do much anyway. Although the test would seem somewhat
trivial (if I am not missing anything)..

https://codereview.chromium.org/2774783003/diff/350001/services/device/vibrat...
services/device/vibration/vibration_manager_impl_unittest.cc:71: Vibrate(10000);
Does this mean the Android device on which the test runs will actually vibrate?

[leonhsl(Using Gerrit), 2017-04-08 00:51:01]

Thanks Tim a lot for review!

https://codereview.chromium.org/2774783003/diff/350001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java
(left):

https://codereview.chromium.org/2774783003/diff/350001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/VibrationManagerImplTest.java:29:
public class VibrationManagerImplTest {
On 2017/04/07 10:39:06, timvolodine wrote:
> If there is no way to provide a similar end-to-end test in device/ then this
> test should probably stay where it is..

If we keep this test here we have to make a dependency on
//services/device/vibration from content java test target,, which breaks the
rule that //services/device/vibration impl should be hidden inside device
service.

https://codereview.chromium.org/2774783003/diff/350001/device/vibration/andro...
File
device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java
(right):

https://codereview.chromium.org/2774783003/diff/350001/device/vibration/andro...
device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java:64:
mVibrator.vibrate(sanitizedMilliseconds);
On 2017/04/07 10:39:07, timvolodine wrote:
> Will the device actually vibrate when testing?

Nope :) The test app service_unittests has no Vibration permission. I think we
just want to verify VibrationManager mojo interface impl inside Device Service
so I did not add that permission additionally.

https://codereview.chromium.org/2774783003/diff/350001/device/vibration/andro...
device/vibration/android/java/src/org/chromium/device/vibration/VibrationManagerImpl.java:66:
setVibrateMilliSecondsForTesting(sanitizedMilliseconds);
On 2017/04/07 10:39:07, timvolodine wrote:
> I believe we generally try to avoid mixing production and explicit testing
code.

Yeah.. Having no better idea to handle such case though..

https://codereview.chromium.org/2774783003/diff/350001/services/device/vibrat...
File services/device/vibration/vibration_manager_impl_unittest.cc (right):

https://codereview.chromium.org/2774783003/diff/350001/services/device/vibrat...
services/device/vibration/vibration_manager_impl_unittest.cc:21: class
VibrationManagerImplTest : public DeviceServiceTestBase {
On 2017/04/07 10:39:07, timvolodine wrote:
> If I understand correctly the idea here is to test the mojo impl setting of
> value and execution of callback.. perhaps the existing wrapper in the java
code
> could be reused for that purpose with a java test (can we do that?) since the
> default impl doesn't do much anyway. Although the test would seem somewhat
> trivial (if I am not missing anything)..

Yeah we're trying to verify the behavior of VibrationManager mojo impl, and we
also want to verify Device Service is hosting the VibrationManager interface
correctly. So we create this test as a service test: On startup
DeviceServiceTestBase starts a background Service Manager and makes Device
Service ready to be started, then on line30 in this file we're acting as a
client to try to consume VibrationManager interface from Device Service, means a
real Device Service instance will be started to receive our requests to trigger
VibrationManager mojo functions.

https://codereview.chromium.org/2774783003/diff/350001/services/device/vibrat...
services/device/vibration/vibration_manager_impl_unittest.cc:71: Vibrate(10000);
On 2017/04/07 10:39:07, timvolodine wrote:
> Does this mean the Android device on which the test runs will actually
vibrate?

No, because the test app has no Vibration permission.

[jam, 2017-04-10 14:53:56]

lgtm

[Ken Rockot(use gerrit already), 2017-04-10 21:29:32]

https://codereview.chromium.org/2774783003/diff/350001/services/BUILD.gn
File services/BUILD.gn (right):

https://codereview.chromium.org/2774783003/diff/350001/services/BUILD.gn#newc...
services/BUILD.gn:27: "//services/device:java",
Can this not just be a public_deps in //services/device:tests? Is this some
weird deficiency in our dependency system for Java targets? It's unfortunate
that you have to make any changes to this target's deps when you're not making
any other direct changes to the target.

[leonhsl(Using Gerrit), 2017-04-11 03:34:52]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 03:35:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-04-11 03:39:18]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 03:39:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 05:01:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 05:01:13]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-04-11 05:07:41]

On 2017/04/10 21:29:32, Ken Rockot wrote:
> https://codereview.chromium.org/2774783003/diff/350001/services/BUILD.gn
> File services/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2774783003/diff/350001/services/BUILD.gn#newc...
> services/BUILD.gn:27: "//services/device:java",
> Can this not just be a public_deps in //services/device:tests? Is this some
> weird deficiency in our dependency system for Java targets? It's unfortunate
> that you have to make any changes to this target's deps when you're not making
> any other direct changes to the target.

Thank you Ken! Yes ps#6 proves that your suggestion is right. Sorry I
misunderstood that java library target must always be depended by java
library/apk targets,, so I put //services/device:java into the deps of
service_unittests, which is a java apk target for android build.

[Ken Rockot(use gerrit already), 2017-04-11 05:11:37]

lgtm

[leonhsl(Using Gerrit), 2017-04-11 08:23:09]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 08:23:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-04-11 08:32:02]

Uploaded ps#7 to address a comment from Colin which I missed before...

https://codereview.chromium.org/2774783003/diff/310001/services/device/device...
File services/device/device_service_test_base.cc (right):

https://codereview.chromium.org/2774783003/diff/310001/services/device/device...
services/device/device_service_test_base.cc:74: base::Thread io_thread_;
On 2017/03/31 12:14:45, leonhsl wrote:
> On 2017/03/31 11:22:30, blundell wrote:
> > nit: These should be moved above line 72 so that they outlive
> > |device_service_context_|.
> 
> Acknowledged.

Done.

[commit-bot: I haz the power, 2017-04-11 09:42:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 09:42:23]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-04-11 13:41:22]

The CQ bit was checked by leon.han@intel.com

[leonhsl(Using Gerrit), 2017-04-11 13:41:23]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
jam@chromium.org, blundell@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2774783003/#ps410001
(title: "Address comments from blundell@")

[commit-bot: I haz the power, 2017-04-11 13:41:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 13:47:39]

CQ is committing da patch.

Bot data: {"patchset_id": 410001, "attempt_start_ts": 1491918082996060,
"parent_rev": "5f11985eb39f27ae2a405dde3d964d2ab02bb992", "commit_rev":
"9c17bee62f1e16224fabb7c83bbeaab719015761"}

[commit-bot: I haz the power, 2017-04-11 13:48:27]

Description was changed from

==========
[DeviceService] Add service tests for VibrationManager.

VibrationManagerImpl.java is in //device/vibration while the
VibrationManagerImplTest.java is still in //content,
to be able to hide all //device/vibration impl inside Device Service,
we must eliminate this concrete dependency on //device/vibration from
content layer.

As VibrationManagerImplTest.java is an integration test based on
content shell test infra, we can't move it outside of //content.
So we consider to use other tests to replace it, while the renderer-side
testing has already been covered by layout tests created at
https://codereview.chromium.org/2731953003/, so we create this CL to
cover the VibrationManager interface impl testing:
 - creates an infra for Device Service service tests.
 - adds a new service test for VibrationManager interface.

Then after this CL we'll be able to hide all vibration impl inside
Device Service with a follow-up CL.

BUG=689379
TEST=service_unittests
==========

to

==========
[DeviceService] Add service tests for VibrationManager.

VibrationManagerImpl.java is in //device/vibration while the
VibrationManagerImplTest.java is still in //content,
to be able to hide all //device/vibration impl inside Device Service,
we must eliminate this concrete dependency on //device/vibration from
content layer.

As VibrationManagerImplTest.java is an integration test based on
content shell test infra, we can't move it outside of //content.
So we consider to use other tests to replace it, while the renderer-side
testing has already been covered by layout tests created at
https://codereview.chromium.org/2731953003/, so we create this CL to
cover the VibrationManager interface impl testing:
 - creates an infra for Device Service service tests.
 - adds a new service test for VibrationManager interface.

Then after this CL we'll be able to hide all vibration impl inside
Device Service with a follow-up CL.

BUG=689379
TEST=service_unittests

Review-Url: https://codereview.chromium.org/2774783003
Cr-Commit-Position: refs/heads/master@{#463612}
Committed:
https://chromium.googlesource.com/chromium/src/+/9c17bee62f1e16224fabb7c83bbe...
==========

[commit-bot: I haz the power, 2017-04-11 13:48:29]

Committed patchset #7 (id:410001) as
https://chromium.googlesource.com/chromium/src/+/9c17bee62f1e16224fabb7c83bbe...

[please use gerrit instead, 2017-04-11 14:45:37]

A revert of this CL (patchset #7 id:410001) has been created in
https://codereview.chromium.org/2815623003/ by rouslan@chromium.org.

The reason for reverting is: Appears to have broken service_unittests on Nexus
5X..