[DeviceService] Port VibrationManager to be hosted in Device Service

[DeviceService] Port VibrationManager to be hosted in Device Service

This CL ports VibrationManager from being hosted in //content/browser/
to be hosted in Device Service, and revises vibration layout tests
accorddingly.

BUG=686687
TEST=blink_tests
TBR=mvanouwerkerk@chromium.org
  for changes of device/vibration/BUILD.gn
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2755363002
Cr-Commit-Position: refs/heads/master@{#459370}
Committed: https://chromium.googlesource.com/chromium/src/+/235ea3af87ab499c13c04634c7311da9fe79a1fb

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

Description was changed from

==========
[DeviceService] Port VibrationManager to be hosted in Device Service

This CL ports VibrationManager from being hosted in //content/browser/
to be hosted in Device Service, and revises vibration layout tests
accorddingly.

BUG=686687
TEST=blink_tests
==========

to

==========
[DeviceService] Port VibrationManager to be hosted in Device Service

This CL ports VibrationManager from being hosted in //content/browser/
to be hosted in Device Service, and revises vibration layout tests
accorddingly.

BUG=686687
TEST=blink_tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[leonhsl(Using Gerrit), 2017-03-20 08:22:03]

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

[commit-bot: I haz the power, 2017-03-20 08:22:09]

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-20 08:26:56]

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

[commit-bot: I haz the power, 2017-03-20 08:26:57]

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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[leonhsl(Using Gerrit), 2017-03-20 08:46:06]

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

[commit-bot: I haz the power, 2017-03-20 08:46: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-20 09:02:03]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-03-20 11:00:18]

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

[commit-bot: I haz the power, 2017-03-20 11:00:19]

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

[leonhsl(Using Gerrit), 2017-03-21 02:05:49]

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

[commit-bot: I haz the power, 2017-03-21 02:06: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-21 03:35:22]

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

[commit-bot: I haz the power, 2017-03-21 03:35:23]

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

[leonhsl(Using Gerrit), 2017-03-21 07:11:51]

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

[commit-bot: I haz the power, 2017-03-21 07:12:17]

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-21 07:31:36]

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

[leonhsl(Using Gerrit), 2017-03-21 07:31:43]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-03-21 07:31: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-21 09:09:24]

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

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

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-03-21 14:12:07]

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

[leonhsl(Using Gerrit), 2017-03-21 14:12:07]

Hi, Colin, would you please just take a glance for now? I want to get your
confirmation step by step instead of sending out a big CL for review :)

You can see ps#1 has some red bots, it's because that I was trying to inject
mock VibrationManager per frame just like before, then I realized that
VibrationManager interface is being hosted in Device Service now, it does not
care about where its clients are positioned.
IIUC the blink connector is mocked per renderer process, once a mock
VibrationManager was injected, it can be consumed by all main/sub frames within
the same renderer process. So I changed to ps#2, which has made all bots green.
One question while I was checking around: If multiple places within one renderer
process tried to override the same interface XXX, maybe the last one should be
took as final? While currently BlinkConnectorImpl::AddOverrideForTesting() is
taking the first one, maybe any concerns here? Thanks.

[blundell, 2017-03-21 14:41:12]

On 2017/03/21 14:12:07, leonhsl wrote:
> Hi, Colin, would you please just take a glance for now? I want to get your
> confirmation step by step instead of sending out a big CL for review :)
> 

This looks great!

> You can see ps#1 has some red bots, it's because that I was trying to inject
> mock VibrationManager per frame just like before, then I realized that
> VibrationManager interface is being hosted in Device Service now, it does not
> care about where its clients are positioned.
> IIUC the blink connector is mocked per renderer process, once a mock
> VibrationManager was injected, it can be consumed by all main/sub frames
within
> the same renderer process. So I changed to ps#2, which has made all bots
green.
> One question while I was checking around: If multiple places within one
renderer
> process tried to override the same interface XXX, maybe the last one should be
> took as final? While currently BlinkConnectorImpl::AddOverrideForTesting() is
> taking the first one, maybe any concerns here? Thanks.

Yes, you're right: this is an inadvertent bug. I'll make a note to fix it if
that CL ends up going forward.

[leonhsl(Using Gerrit), 2017-03-22 06:59:18]

Patchset #3 (id:80001) has been deleted

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

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

[commit-bot: I haz the power, 2017-03-22 09:26: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-22 12:06:53]

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

[commit-bot: I haz the power, 2017-03-22 12:06:57]

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-22 13:08: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-22 13:08:37]

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-22 13:42:45]

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

[commit-bot: I haz the power, 2017-03-22 13:42:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_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-22 14:08:49]

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

[commit-bot: I haz the power, 2017-03-22 14:09:01]

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-22 14:10:52]

Patchset #3 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-03-22 14:12:47]

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

[commit-bot: I haz the power, 2017-03-22 14:12:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-23 02:03:53]

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

[commit-bot: I haz the power, 2017-03-23 02:04: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-23 02:09:08]

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

[commit-bot: I haz the power, 2017-03-23 02:09:10]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_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-23 07:24:33]

Patchset #1 (id:40001) has been deleted

[leonhsl(Using Gerrit), 2017-03-23 07:57:08]

Hi, Colin, this CL has 3 patch sets now:
  ps#1: Port VibrationManager into Device Service.

  ps#2: Move all vibrations codes from //device/ into //services/device/, with a
TODO left: Eliminate the dependency:
content/public/android::content_javatests ==>
//services/device/vibration/android:vibration_manager_java 
by replacing VibrationManagerImplTest.java with a service test.

  ps#3: Address the TODO left by ps#2: add a service test for testing
VibrationManager hosted in Device Service. And I've confirmed locally the newly
added service test runs OK.

Firstly, I need your help to confirm whether ps#3 is on a correct direction,
although currently it is not completely done: I need to enable C++ test code to
get data from VibrationManagerImpl.java via JNI, and more refinement.

And, I noticed that the 2 blocker CLs of this CL will be able to land soon, so
given that ps#1+ps#2 is already a big CL and ps#3 needs further work, I'm
considering to send out ps#2 for formal review and land it firstly, in the
meantime separate ps#3 to another CL to continue baking. Although landing ps#2
now would leave the TODO.. I suppose we can resolve it soon based on current
work of ps#3 :)
WDYT? Thanks!

[blundell, 2017-03-23 11:29:41]

On 2017/03/23 07:57:08, leonhsl wrote:
> Hi, Colin, this CL has 3 patch sets now:
>   ps#1: Port VibrationManager into Device Service.
> 
>   ps#2: Move all vibrations codes from //device/ into //services/device/, with
a
> TODO left: Eliminate the dependency:
> content/public/android::content_javatests ==>
> //services/device/vibration/android:vibration_manager_java 
> by replacing VibrationManagerImplTest.java with a service test.
> 
>   ps#3: Address the TODO left by ps#2: add a service test for testing
> VibrationManager hosted in Device Service. And I've confirmed locally the
newly
> added service test runs OK.
> 
> Firstly, I need your help to confirm whether ps#3 is on a correct direction,
> although currently it is not completely done: I need to enable C++ test code
to
> get data from VibrationManagerImpl.java via JNI, and more refinement.
> 
> And, I noticed that the 2 blocker CLs of this CL will be able to land soon, so
> given that ps#1+ps#2 is already a big CL and ps#3 needs further work, I'm
> considering to send out ps#2 for formal review and land it firstly, in the
> meantime separate ps#3 to another CL to continue baking. Although landing ps#2
> now would leave the TODO.. I suppose we can resolve it soon based on current
> work of ps#3 :)
> WDYT? Thanks!

Hi, this is awesome!

I would separate out PS1, PS2, and PS3 all into their own CLs.

I think that we should land PS3 before PS2, i.e., we should maintain the
property that a library is moved into the Device Service only when its GN
visibility can be restricted to //services/device:lib.

So concretely, I suggest sending out PS1 as its own CL now and inverting the
dependency between PS2 and PS3: i.e., develop PS3 as its own CL dependent on PS1
and then make PS2 its own CL dependent on PS3, so that PS2 can restrict the GN
visibility properly. Once you've done that, you can feel free to pipeline by
sending out PS2 for review even while still developing PS3.

PS3 looks good so far. The crucial thing is to bring up the service test itself,
of course. Thanks for working on this! It will be an awesome thing to have the
infra for Device Service service tests.

[leonhsl(Using Gerrit), 2017-03-23 13:53:20]

On 2017/03/23 11:29:41, blundell wrote:
> On 2017/03/23 07:57:08, leonhsl wrote:
> > Hi, Colin, this CL has 3 patch sets now:
> >   ps#1: Port VibrationManager into Device Service.
> > 
> >   ps#2: Move all vibrations codes from //device/ into //services/device/,
with
> a
> > TODO left: Eliminate the dependency:
> > content/public/android::content_javatests ==>
> > //services/device/vibration/android:vibration_manager_java 
> > by replacing VibrationManagerImplTest.java with a service test.
> > 
> >   ps#3: Address the TODO left by ps#2: add a service test for testing
> > VibrationManager hosted in Device Service. And I've confirmed locally the
> newly
> > added service test runs OK.
> > 
> > Firstly, I need your help to confirm whether ps#3 is on a correct direction,
> > although currently it is not completely done: I need to enable C++ test code
> to
> > get data from VibrationManagerImpl.java via JNI, and more refinement.
> > 
> > And, I noticed that the 2 blocker CLs of this CL will be able to land soon,
so
> > given that ps#1+ps#2 is already a big CL and ps#3 needs further work, I'm
> > considering to send out ps#2 for formal review and land it firstly, in the
> > meantime separate ps#3 to another CL to continue baking. Although landing
ps#2
> > now would leave the TODO.. I suppose we can resolve it soon based on current
> > work of ps#3 :)
> > WDYT? Thanks!
> 
> Hi, this is awesome!
> 
> I would separate out PS1, PS2, and PS3 all into their own CLs.
> 
> I think that we should land PS3 before PS2, i.e., we should maintain the
> property that a library is moved into the Device Service only when its GN
> visibility can be restricted to //services/device:lib.
> 
> So concretely, I suggest sending out PS1 as its own CL now and inverting the
> dependency between PS2 and PS3: i.e., develop PS3 as its own CL dependent on
PS1
> and then make PS2 its own CL dependent on PS3, so that PS2 can restrict the GN
> visibility properly. Once you've done that, you can feel free to pipeline by
> sending out PS2 for review even while still developing PS3.
> 
> PS3 looks good so far. The crucial thing is to bring up the service test
itself,
> of course. Thanks for working on this! It will be an awesome thing to have the
> infra for Device Service service tests.

Ah,, Thank you very much Colin! Seems I had a wrong feeling that the service
test creation must be together with or after moving codes into
//services/device/, actually once we achieved porting VibrationManager into
Device Service we can create the service test for it!
I see, I'll follow with your suggestions to continue. I'll send out ps#1 for
formal review now, tomorrow I'll separate ps#3 to a new CL B, and ps#2 to
another CL C depending on B.

[leonhsl(Using Gerrit), 2017-03-23 14:31:57]

Patchset #3 (id:140001) has been deleted

[leonhsl(Using Gerrit), 2017-03-23 14:32:14]

Patchset #2 (id:120001) has been deleted

[leonhsl(Using Gerrit), 2017-03-23 14:50:42]

leon.han@intel.com changed reviewers:
+ kinuko@chromium.org, mvanouwerkerk@chromium.org, tsepez@chromium.org

[leonhsl(Using Gerrit), 2017-03-23 14:50:42]

Hi, PTAL, Thanks!
  #Colin has done an initial review for this, and although I need rebase this CL
onto master once other 2 blocker CLs got landed(assuming soon today), the rebase
changes will be trivial, so I think current ps#1 is worth a formal review even
now.

blundell@: Overall review and OWNER review of //services/device/*

tsepez@: OWNER review of
  content/public/app/mojo/content_browser_manifest.json
  content/public/app/mojo/content_renderer_manifest.json
  services/device/manifest.json

kinuko@: OWNER review of //content/*

mvanouwerkerk@: OWNER review of 
  //third_party/WebKit/Source/modules/vibration/*

[blundell, 2017-03-23 15:07:24]

lgtm, thanks!

[Tom Sepez, 2017-03-23 16:37:23]

lgtm

[kinuko, 2017-03-24 00:51:29]

lgtm

[leonhsl(Using Gerrit), 2017-03-24 02:53:50]

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 02:54:09]

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 03:04:13]

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

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

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

[leonhsl(Using Gerrit), 2017-03-24 03:09:06]

Patchset #2 (id:160001) has been deleted

[leonhsl(Using Gerrit), 2017-03-24 03:10:37]

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 03:11:05]

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-24 04:58:17]

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

[leonhsl(Using Gerrit), 2017-03-24 04:58:18]

+haraken@, would you mind to help take the OWNER review of
//third_party/WebKit/Source/modules/? Thanks~

[haraken, 2017-03-24 05:03:35]

On 2017/03/24 04:58:18, leonhsl wrote:
> +haraken@, would you mind to help take the OWNER review of
> //third_party/WebKit/Source/modules/? Thanks~

WebKit LGTM

[commit-bot: I haz the power, 2017-03-24 05:45:08]

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

[commit-bot: I haz the power, 2017-03-24 05:45:09]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-03-24 06:08:57]

Description was changed from

==========
[DeviceService] Port VibrationManager to be hosted in Device Service

This CL ports VibrationManager from being hosted in //content/browser/
to be hosted in Device Service, and revises vibration layout tests
accorddingly.

BUG=686687
TEST=blink_tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[DeviceService] Port VibrationManager to be hosted in Device Service

This CL ports VibrationManager from being hosted in //content/browser/
to be hosted in Device Service, and revises vibration layout tests
accorddingly.

BUG=686687
TEST=blink_tests
TBR=mvanouwerkerk@chromium.org
  for changes of device/vibration/BUILD.gn
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[leonhsl(Using Gerrit), 2017-03-24 06:10:15]

Thanks all! Let me TBR mvanouwerkerk@ for the trivial changes of
//device/vibration/BUILD.gn and send this to CQ now.

[leonhsl(Using Gerrit), 2017-03-24 06:11:12]

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

[leonhsl(Using Gerrit), 2017-03-24 06:11:12]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
tsepez@chromium.org, blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/2755363002/#ps180001
(title: "Rebase only")

[commit-bot: I haz the power, 2017-03-24 06:11:31]

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 06:17:35]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490335872470580,
"parent_rev": "c098755ccb6890ee04b345381f07be5224b1e717", "commit_rev":
"235ea3af87ab499c13c04634c7311da9fe79a1fb"}

[commit-bot: I haz the power, 2017-03-24 06:18:19]

Description was changed from

==========
[DeviceService] Port VibrationManager to be hosted in Device Service

This CL ports VibrationManager from being hosted in //content/browser/
to be hosted in Device Service, and revises vibration layout tests
accorddingly.

BUG=686687
TEST=blink_tests
TBR=mvanouwerkerk@chromium.org
  for changes of device/vibration/BUILD.gn
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[DeviceService] Port VibrationManager to be hosted in Device Service

This CL ports VibrationManager from being hosted in //content/browser/
to be hosted in Device Service, and revises vibration layout tests
accorddingly.

BUG=686687
TEST=blink_tests
TBR=mvanouwerkerk@chromium.org
  for changes of device/vibration/BUILD.gn
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2755363002
Cr-Commit-Position: refs/heads/master@{#459370}
Committed:
https://chromium.googlesource.com/chromium/src/+/235ea3af87ab499c13c04634c731...
==========

[commit-bot: I haz the power, 2017-03-24 06:18:21]

Committed patchset #2 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/235ea3af87ab499c13c04634c731...