[DeviceService] Add end-to-end browsertest for PowerMonitor

[DeviceService] Add end-to-end browsertest for PowerMonitor

This CL is to confirm that in a real browser environment the renderer
process CAN succeed in connecting to Device Service to bind PowerMonitor
interface, and, via which renderer process CAN get power state change
correctly.

Follow-up CL will set up similar tests for gpu/utility child processes.

BUG=715985
TEST=content_browsertests

Review-Url: https://codereview.chromium.org/2870373002
Cr-Commit-Position: refs/heads/master@{#478928}
Committed: https://chromium.googlesource.com/chromium/src/+/a0c5fa3e6bb48c503df220d9c8ac946598f12215

[leonhsl(Using Gerrit), 2017-05-10 08:40:23]

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

[commit-bot: I haz the power, 2017-05-10 08:40:54]

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-05-10 09:16:00]

Description was changed from

==========
[DeviceService] Add end-to-end browsertest for PowerMonitor

BUG=715985
TEST=content_browsertests
==========

to

==========
[DeviceService] Add end-to-end browsertest for PowerMonitor

This CL is to confirm that in a real browser environment the browser child
process CAN succeed in connecting to Device Service to bind PowerMonitor
interface.

BUG=715985
TEST=content_browsertests
==========

[commit-bot: I haz the power, 2017-05-10 09:20:24]

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

[commit-bot: I haz the power, 2017-05-10 09:20:24]

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

[leonhsl(Using Gerrit), 2017-05-10 09:33:28]

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

[leonhsl(Using Gerrit), 2017-05-10 09:33:29]

Hi, Colin, this CL just adds a end-to-end test for renderer process, I tried to
also cover other child processes(gpu etc.), but content_browsertests seems does
not start gpu process on startup, and I'm not clear how to trigger starting it.
And starting utility/plugin process also needs additional work. 

As all these child processes share the same ChildThreadImpl implementation,
maybe this CL is enough now? I'm eager to your suggestions/comments:) Thanks.

[leonhsl(Using Gerrit), 2017-05-10 09:35:47]

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

[commit-bot: I haz the power, 2017-05-10 09:36:25]

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-05-10 09:36:36]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-05-10 11:23:41]

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

[commit-bot: I haz the power, 2017-05-10 11:23:42]

Dry run: This issue passed the CQ dry run.

[blundell, 2017-05-10 14:54:56]

This is a great start! It would be great if we could add tests that would catch
both the regressions that we've seen. I believe that this test would catch the
second (where ChildThreadImpl was working with an empty InterfaceProvider), but
not the first (where the client didn't hold on to the PowerMonitor connection,
causing the PowerMonitor impl to immediately die). Is that correct?

Also, it would indeed be great to add another process type, but I don't know
anything about doing that in a browsertest. jam@ is likely a good starting point
there. We shouldn't let that block us getting these tests in though for sure.

[leonhsl(Using Gerrit), 2017-05-11 02:22:42]

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

[commit-bot: I haz the power, 2017-05-11 02:23:39]

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

On 2017/05/10 14:54:56, blundell wrote:
> This is a great start! It would be great if we could add tests that would
catch
> both the regressions that we've seen. I believe that this test would catch the
> second (where ChildThreadImpl was working with an empty InterfaceProvider),
but
> not the first (where the client didn't hold on to the PowerMonitor connection,
> causing the PowerMonitor impl to immediately die). Is that correct?
> 
The first regression we met before is caused by internal implementation of the
PowerMonitor impl, the client would connect to PowerMonitor, just call
AddClient() on it and then close PowerMonitor connection, this is expected
behavior, while PowerMonitor impl should not have used StrongBinding to put its
lifecycle controlled by this mojo connection. So I think the service test in
power_monitor_message_broadcaster_unittest.cc should be able to catch such
regression now. 

But it's true that the service test can NOT catch the error that under real
browser environment ChildThreadImpl may not hold PowerMonitorClient impl
correctly(not long enough, so that child processes can't get power monitor
notifications correctly), I'll consider more about how to verify this happening
inside child processes.. Power monitor has no corresponding javascript APIs just
like battery/vibration to enable us to confirm some information via
html/javascript..

[leonhsl(Using Gerrit), 2017-05-11 03:38:14]

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

[leonhsl(Using Gerrit), 2017-05-11 03:38:14]

Hi, John, we want to start up some child processes like gpu/utility/plugin in a
browser test, to verify some behavior inside our Device Service, do you have any
idea how to achieve this? Some samples in existing browser tests will be very
helpful :-) Thanks!

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

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

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

Dry run: This issue passed the CQ dry run.

[jam, 2017-05-11 14:55:23]

On 2017/05/11 03:38:14, leonhsl wrote:
> Hi, John, we want to start up some child processes like gpu/utility/plugin in
a
> browser test, to verify some behavior inside our Device Service, do you have
any
> idea how to achieve this? Some samples in existing browser tests will be very
> helpful :-) Thanks!

Can you provide more information? What do you want to run in the child process?

[leonhsl(Using Gerrit), 2017-05-12 00:44:46]

On 2017/05/11 14:55:23, jam wrote:
> On 2017/05/11 03:38:14, leonhsl wrote:
> > Hi, John, we want to start up some child processes like gpu/utility/plugin
in
> a
> > browser test, to verify some behavior inside our Device Service, do you have
> any
> > idea how to achieve this? Some samples in existing browser tests will be
very
> > helpful :-) Thanks!
> 
> Can you provide more information? What do you want to run in the child
process?

We want to verify the following 2 behaviors: 
  1. Child processes(renderer/gpu/utility/plugin) on startup SHOULD&CAN bind
PowerMonitor interface to Device Service.
  2. Child processes CAN receive power change notifications when power status
changed in browser process.

Above 1 only requires to start these child processes from a browser test, ps#2
of this CL just starts a renderer process to verify this, but I have no idea how
to start other child processes in browser test.
Above 2 requires to pick&verify some power related information of child process
from a browser test.

[jam, 2017-05-15 13:57:46]

On 2017/05/12 00:44:46, leonhsl wrote:
> On 2017/05/11 14:55:23, jam wrote:
> > On 2017/05/11 03:38:14, leonhsl wrote:
> > > Hi, John, we want to start up some child processes like gpu/utility/plugin
> in
> > a
> > > browser test, to verify some behavior inside our Device Service, do you
have
> > any
> > > idea how to achieve this? Some samples in existing browser tests will be
> very
> > > helpful :-) Thanks!
> > 
> > Can you provide more information? What do you want to run in the child
> process?
> 
> We want to verify the following 2 behaviors: 
>   1. Child processes(renderer/gpu/utility/plugin) on startup SHOULD&CAN bind
> PowerMonitor interface to Device Service.
>   2. Child processes CAN receive power change notifications when power status
> changed in browser process.
> 
> Above 1 only requires to start these child processes from a browser test, ps#2
> of this CL just starts a renderer process to verify this, but I have no idea
how
> to start other child processes in browser test.
> Above 2 requires to pick&verify some power related information of child
process
> from a browser test.

Got it, so the way to do this would be to define some test IPCs. we really only
have old IPC in content/shell/common/shell_messages.h, but you should create a
new shell_messages.mojom for this (cause we should avoid creating new IPCs to
test mojo code!). Then you can write code in content/shell/{utility, gpu,
renderer} to dispatch these messages and send back verification.

[leonhsl(Using Gerrit), 2017-05-16 09:13:33]

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

[commit-bot: I haz the power, 2017-05-16 09:13:56]

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-05-16 09:21:47]

Hi, Colin, John, now ps#3 can verify power state change of renderer process by
adding the new mojo interface PowerMonitorTest, PTAnL, Thanks!

[commit-bot: I haz the power, 2017-05-16 10:47:03]

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

[commit-bot: I haz the power, 2017-05-16 10:47:04]

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

[blundell, 2017-05-16 13:28:42]

Thanks, this looks great!

https://codereview.chromium.org/2870373002/diff/60001/content/browser/power_m...
File content/browser/power_monitor_browsertest.cc (right):

https://codereview.chromium.org/2870373002/diff/60001/content/browser/power_m...
content/browser/power_monitor_browsertest.cc:111:
IN_PROC_BROWSER_TEST_F(PowerMonitorTest, RequestOnceFromOneRendererProcess) {
John, would we able to set up a similar test for the GPU/utility processes?

https://codereview.chromium.org/2870373002/diff/60001/content/public/test/pow...
File content/public/test/power_monitor_test.mojom (right):

https://codereview.chromium.org/2870373002/diff/60001/content/public/test/pow...
content/public/test/power_monitor_test.mojom:7: interface PowerMonitorTest {
nit: This could use a comment.

[leonhsl(Using Gerrit), 2017-05-20 12:49:08]

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

[commit-bot: I haz the power, 2017-05-20 12:49:20]

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-05-20 14:14:56]

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

[commit-bot: I haz the power, 2017-05-20 14:14:57]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-05-22 12:11:57]

https://codereview.chromium.org/2870373002/diff/60001/content/browser/power_m...
File content/browser/power_monitor_browsertest.cc (right):

https://codereview.chromium.org/2870373002/diff/60001/content/browser/power_m...
content/browser/power_monitor_browsertest.cc:111:
IN_PROC_BROWSER_TEST_F(PowerMonitorTest, RequestOnceFromOneRendererProcess) {
On 2017/05/16 13:28:42, blundell wrote:
> John, would we able to set up a similar test for the GPU/utility processes?

Friendly ping John, how could we start up GPU/utility processes in content
browsertests? Thanks.

[blundell, 2017-05-22 12:14:52]

On 2017/05/22 12:11:57, leonhsl wrote:
>
https://codereview.chromium.org/2870373002/diff/60001/content/browser/power_m...
> File content/browser/power_monitor_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2870373002/diff/60001/content/browser/power_m...
> content/browser/power_monitor_browsertest.cc:111:
> IN_PROC_BROWSER_TEST_F(PowerMonitorTest, RequestOnceFromOneRendererProcess) {
> On 2017/05/16 13:28:42, blundell wrote:
> > John, would we able to set up a similar test for the GPU/utility processes?
> 
> Friendly ping John, how could we start up GPU/utility processes in content
> browsertests? Thanks.

I think we could also land this CL as-is and take that discussion to a bug for a
followup.

[leonhsl(Using Gerrit), 2017-05-22 12:39:54]

On 2017/05/22 12:14:52, blundell wrote:
> On 2017/05/22 12:11:57, leonhsl wrote:
> >
>
https://codereview.chromium.org/2870373002/diff/60001/content/browser/power_m...
> > File content/browser/power_monitor_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2870373002/diff/60001/content/browser/power_m...
> > content/browser/power_monitor_browsertest.cc:111:
> > IN_PROC_BROWSER_TEST_F(PowerMonitorTest, RequestOnceFromOneRendererProcess)
{
> > On 2017/05/16 13:28:42, blundell wrote:
> > > John, would we able to set up a similar test for the GPU/utility
processes?
> > 
> > Friendly ping John, how could we start up GPU/utility processes in content
> > browsertests? Thanks.
> 
> I think we could also land this CL as-is and take that discussion to a bug for
a
> followup.

Hi, Colin, I agree we proceed step by step, and CCd John about this in original
issue crbug.com/715985.
Hi, John, PTAL at this CL firstly, so we can at least get one browsertest ready
to defend against potential regressions which have happened before ;-)

[leonhsl(Using Gerrit), 2017-05-28 09:45:58]

Friendly ping John, Thanks.

[jam, 2017-06-06 22:20:29]

really sorry for the delay, I missed this. please IM or send me direct emails if
I don't respond quickly.

https://codereview.chromium.org/2870373002/diff/60001/content/browser/power_m...
File content/browser/power_monitor_browsertest.cc (right):

https://codereview.chromium.org/2870373002/diff/60001/content/browser/power_m...
content/browser/power_monitor_browsertest.cc:111:
IN_PROC_BROWSER_TEST_F(PowerMonitorTest, RequestOnceFromOneRendererProcess) {
On 2017/05/22 12:11:57, leonhsl (OOO until 09 June) wrote:
> On 2017/05/16 13:28:42, blundell wrote:
> > John, would we able to set up a similar test for the GPU/utility processes?
> 
> Friendly ping John, how could we start up GPU/utility processes in content
> browsertests? Thanks.

Since the GPU process is always running, content/shell can also add embedder
interfaces in the GPU process to respond to testing IPCs. For utility, also do
the same.

https://codereview.chromium.org/2870373002/diff/60001/content/public/test/pow...
File content/public/test/power_monitor_test.mojom (right):

https://codereview.chromium.org/2870373002/diff/60001/content/public/test/pow...
content/public/test/power_monitor_test.mojom:7: interface PowerMonitorTest {
nit: move this to content/shell/common

[leonhsl(Using Gerrit), 2017-06-09 06:51:18]

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

[commit-bot: I haz the power, 2017-06-09 06:51:27]

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-06-09 06:53:39]

Description was changed from

==========
[DeviceService] Add end-to-end browsertest for PowerMonitor

This CL is to confirm that in a real browser environment the browser child
process CAN succeed in connecting to Device Service to bind PowerMonitor
interface.

BUG=715985
TEST=content_browsertests
==========

to

==========
[DeviceService] Add end-to-end browsertest for PowerMonitor

This CL is to confirm that in a real browser environment the renderer
process CAN succeed in connecting to Device Service to bind PowerMonitor
interface, and, via which renderer process CAN get power state change
correctly.

BUG=715985
TEST=content_browsertests
==========

[commit-bot: I haz the power, 2017-06-09 06:55:16]

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

[commit-bot: I haz the power, 2017-06-09 06:55:17]

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_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[leonhsl(Using Gerrit), 2017-06-09 06:59:25]

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

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

Patchset #5 (id:100001) has been deleted

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

Description was changed from

==========
[DeviceService] Add end-to-end browsertest for PowerMonitor

This CL is to confirm that in a real browser environment the renderer
process CAN succeed in connecting to Device Service to bind PowerMonitor
interface, and, via which renderer process CAN get power state change
correctly.

BUG=715985
TEST=content_browsertests
==========

to

==========
[DeviceService] Add end-to-end browsertest for PowerMonitor

This CL is to confirm that in a real browser environment the renderer
process CAN succeed in connecting to Device Service to bind PowerMonitor
interface, and, via which renderer process CAN get power state change
correctly.

Follow-up CL will set up similar tests for gpu/utility child processes.

BUG=715985
TEST=content_browsertests
==========

[leonhsl(Using Gerrit), 2017-06-09 07:05:53]

Hi, John, Colin, PTAnL at ps#5, Thanks!

https://codereview.chromium.org/2870373002/diff/60001/content/browser/power_m...
File content/browser/power_monitor_browsertest.cc (right):

https://codereview.chromium.org/2870373002/diff/60001/content/browser/power_m...
content/browser/power_monitor_browsertest.cc:111:
IN_PROC_BROWSER_TEST_F(PowerMonitorTest, RequestOnceFromOneRendererProcess) {
On 2017/06/06 22:20:28, jam wrote:
> On 2017/05/22 12:11:57, leonhsl (OOO until 09 June) wrote:
> > On 2017/05/16 13:28:42, blundell wrote:
> > > John, would we able to set up a similar test for the GPU/utility
processes?
> > 
> > Friendly ping John, how could we start up GPU/utility processes in content
> > browsertests? Thanks.
> 
> Since the GPU process is always running, content/shell can also add embedder
> interfaces in the GPU process to respond to testing IPCs. For utility, also do
> the same.

Thanks! I'd like to land this CL(only for renderer process) firstly, then
investigate more and set up similar tests for gpu/utility with follow-up CL.

https://codereview.chromium.org/2870373002/diff/60001/content/public/test/pow...
File content/public/test/power_monitor_test.mojom (right):

https://codereview.chromium.org/2870373002/diff/60001/content/public/test/pow...
content/public/test/power_monitor_test.mojom:7: interface PowerMonitorTest {
On 2017/06/06 22:20:28, jam wrote:
> nit: move this to content/shell/common

Done.

https://codereview.chromium.org/2870373002/diff/60001/content/public/test/pow...
content/public/test/power_monitor_test.mojom:7: interface PowerMonitorTest {
On 2017/05/16 13:28:42, blundell wrote:
> nit: This could use a comment.

Done.

[commit-bot: I haz the power, 2017-06-09 08:07:31]

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

[commit-bot: I haz the power, 2017-06-09 08:07:32]

Dry run: This issue passed the CQ dry run.

[jam, 2017-06-13 01:43:44]

lgtm

[leonhsl(Using Gerrit), 2017-06-13 02:27:15]

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

[commit-bot: I haz the power, 2017-06-13 02:27:34]

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-06-13 02:35:19]

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

[commit-bot: I haz the power, 2017-06-13 02:35:21]

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

[leonhsl(Using Gerrit), 2017-06-13 02:38:24]

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

[leonhsl(Using Gerrit), 2017-06-13 02:38:25]

Hi, Mike, would you please help to take an OWNER review for adding
content/shell/common/power_monitor_test.mojom? Thanks.

[Mike West, 2017-06-13 07:41:13]

mojo LGTM.

[leonhsl(Using Gerrit), 2017-06-13 07:45:50]

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

[commit-bot: I haz the power, 2017-06-13 07:46:01]

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-06-13 08:14:14]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497339950355220,
"parent_rev": "22e5033f53dfb9e1e5792aad8bfec89d19afc5fc", "commit_rev":
"a0c5fa3e6bb48c503df220d9c8ac946598f12215"}

[commit-bot: I haz the power, 2017-06-13 08:14:29]

Description was changed from

==========
[DeviceService] Add end-to-end browsertest for PowerMonitor

This CL is to confirm that in a real browser environment the renderer
process CAN succeed in connecting to Device Service to bind PowerMonitor
interface, and, via which renderer process CAN get power state change
correctly.

Follow-up CL will set up similar tests for gpu/utility child processes.

BUG=715985
TEST=content_browsertests
==========

to

==========
[DeviceService] Add end-to-end browsertest for PowerMonitor

This CL is to confirm that in a real browser environment the renderer
process CAN succeed in connecting to Device Service to bind PowerMonitor
interface, and, via which renderer process CAN get power state change
correctly.

Follow-up CL will set up similar tests for gpu/utility child processes.

BUG=715985
TEST=content_browsertests

Review-Url: https://codereview.chromium.org/2870373002
Cr-Commit-Position: refs/heads/master@{#478928}
Committed:
https://chromium.googlesource.com/chromium/src/+/a0c5fa3e6bb48c503df220d9c8ac...
==========

[commit-bot: I haz the power, 2017-06-13 08:14:31]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/a0c5fa3e6bb48c503df220d9c8ac...