Create end-to-end browsertest for PowerMonitor |
||||||
Issue descriptionChildThreadImpl's usage of the DeviceService's PowerMonitor Mojo interface has broken in two separate regressions: (1) Incorrectly dropping the remote end of the impl on the floor when the impl used StrongBinding ( crbug.com/696197 ) (2) Moving the Device Service away from implementing OnConnect() while the client still relied on that implementation, resulting in the client having an empty remote InterfaceProvider ( crbug.com/714206 ) We need to add an end-to-end browsertest of this functionality that at a minimum is sufficient to catch the two above regressions.
,
Apr 28 2017
Hi, Colin, I suggest we create a service test to do this work, WDYT? Thanks!
1,
The service test will create a device::PowerMonitorBroadcastSource by passing a service_manager::Connector*, by which it can talk with a real instance of Device Service. This abstracts ChildThreadImpl's usage in production code.
2,
The test is to verify that device::PowerMonitorBroadcastSource CAN receive notifications correctly from Device Service when system power state changed, with a precondition that the Connector* is already valid and usable.
3,
This test does not cover the verification that the Connector* of every ChildThreadImpl(renderer, gpu, utility etc.) is valid and usable. That should be covered by other tests general for service connection between browser and {renderer,gpu,utility} etc.
4,
This test is sufficient to catch the two regressions described in this bug's original report.
,
May 2 2017
Hi Han Leon, Those tests sound like a good idea! I think we should also have an end-to-end browsertest given that (a) we've seen regressions of this functionality that are specifically related to code in //content/child and (b) it should be easy to bring up the end-to-end test once we have the right infra in place.
,
May 2 2017
Got it. I'll firstly start to create the service test, then create the end-to-end browsertest once we get ready the new infra tracked by issue 717377 .
,
May 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6561c2f12d7c6d68613da7500643b300b923e15b commit 6561c2f12d7c6d68613da7500643b300b923e15b Author: leon.han <leon.han@intel.com> Date: Wed May 03 09:56:17 2017 [DeviceService] Revise PowerMonitorMessageBroadcasterTest to be a service test Currently PowerMonitorMessageBroadcasterTest is just an unit test for PowerMonitorMessageBroadcaster class, this CL revises it to be a service test: - Keep all the existing test cases verifying internal impl logic of PowerMonitorMessageBroadcaster. - In addition, verify that PowerMonitorBroadcastSource(in power monitor client library) can interact correctly with the PowerMonitorMessageBroadcaster instantiated inside a real Device Service instance. BUG= 715985 TEST=service_unittests Review-Url: https://codereview.chromium.org/2857913002 Cr-Commit-Position: refs/heads/master@{#468929} [modify] https://crrev.com/6561c2f12d7c6d68613da7500643b300b923e15b/services/device/power_monitor/power_monitor_message_broadcaster_unittest.cc [modify] https://crrev.com/6561c2f12d7c6d68613da7500643b300b923e15b/services/device/unittest_manifest.json
,
May 22 2017
Hi, John, https://codereview.chromium.org/2870373002 just adds the test against renderer process, and hopefully we can also add similar tests for gpu/utility processes as part of work for this issue.
,
Jun 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a0c5fa3e6bb48c503df220d9c8ac946598f12215 commit a0c5fa3e6bb48c503df220d9c8ac946598f12215 Author: leon.han <leon.han@intel.com> Date: Tue Jun 13 08:14:11 2017 [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} [add] https://crrev.com/a0c5fa3e6bb48c503df220d9c8ac946598f12215/content/browser/power_monitor_browsertest.cc [modify] https://crrev.com/a0c5fa3e6bb48c503df220d9c8ac946598f12215/content/shell/BUILD.gn [modify] https://crrev.com/a0c5fa3e6bb48c503df220d9c8ac946598f12215/content/shell/browser/content_shell_renderer_manifest_overlay.json [add] https://crrev.com/a0c5fa3e6bb48c503df220d9c8ac946598f12215/content/shell/common/power_monitor_test.mojom [modify] https://crrev.com/a0c5fa3e6bb48c503df220d9c8ac946598f12215/content/shell/renderer/shell_content_renderer_client.cc [modify] https://crrev.com/a0c5fa3e6bb48c503df220d9c8ac946598f12215/content/test/BUILD.gn
,
Jun 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/18ea620c7031007948cc97031ce516d5512c5271 commit 18ea620c7031007948cc97031ce516d5512c5271 Author: Han Leon <leon.han@intel.com> Date: Sat Jun 24 07:17:31 2017 [DeviceService] Add PowerMonitor browsertest against utility process This CL is to confirm that in a real browser environment the utility process CAN succeed in connecting to Device Service to bind PowerMonitor interface, and, via which utility process CAN get power state change correctly. Follow-up CL will set up similar tests for gpu process. BUG= 715985 TEST=content_browsertests TBR=tsepez@chromium.org for content/shell/browser/content_shell_utility_manifest_overlay.json Change-Id: If4c61de1aad8031ce996fe307ccbe0e04d8acccb Reviewed-on: https://chromium-review.googlesource.com/535057 Commit-Queue: Han Leon <leon.han@intel.com> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#482139} [modify] https://crrev.com/18ea620c7031007948cc97031ce516d5512c5271/content/browser/power_monitor_browsertest.cc [modify] https://crrev.com/18ea620c7031007948cc97031ce516d5512c5271/content/shell/BUILD.gn [modify] https://crrev.com/18ea620c7031007948cc97031ce516d5512c5271/content/shell/browser/content_shell_utility_manifest_overlay.json [add] https://crrev.com/18ea620c7031007948cc97031ce516d5512c5271/content/shell/common/power_monitor_test_impl.cc [add] https://crrev.com/18ea620c7031007948cc97031ce516d5512c5271/content/shell/common/power_monitor_test_impl.h [modify] https://crrev.com/18ea620c7031007948cc97031ce516d5512c5271/content/shell/renderer/shell_content_renderer_client.cc [modify] https://crrev.com/18ea620c7031007948cc97031ce516d5512c5271/content/shell/utility/shell_content_utility_client.cc
,
Jun 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6fa15937d2a3f46d5757b09b5dea1a185b899d44 commit 6fa15937d2a3f46d5757b09b5dea1a185b899d44 Author: Han Leon <leon.han@intel.com> Date: Wed Jun 28 02:46:04 2017 [DeviceService] Add PowerMonitor browsertest against gpu process This CL is to confirm that in a real browser environment the gpu process CAN succeed in connecting to Device Service to bind PowerMonitor interface, and, via which gpu process CAN get power state change correctly. BUG= 715985 TEST=content_browsertests Change-Id: I2371ba0e2c2b50f99ef9356aa395a3f7eeef4870 Reviewed-on: https://chromium-review.googlesource.com/547672 Reviewed-by: Colin Blundell <blundell@chromium.org> Reviewed-by: Tom Sepez <tsepez@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Commit-Queue: Han Leon <leon.han@intel.com> Cr-Commit-Position: refs/heads/master@{#482861} [modify] https://crrev.com/6fa15937d2a3f46d5757b09b5dea1a185b899d44/content/browser/power_monitor_browsertest.cc [modify] https://crrev.com/6fa15937d2a3f46d5757b09b5dea1a185b899d44/content/shell/BUILD.gn [modify] https://crrev.com/6fa15937d2a3f46d5757b09b5dea1a185b899d44/content/shell/app/shell_main_delegate.cc [modify] https://crrev.com/6fa15937d2a3f46d5757b09b5dea1a185b899d44/content/shell/app/shell_main_delegate.h [modify] https://crrev.com/6fa15937d2a3f46d5757b09b5dea1a185b899d44/content/shell/browser/OWNERS [add] https://crrev.com/6fa15937d2a3f46d5757b09b5dea1a185b899d44/content/shell/browser/content_shell_gpu_manifest_overlay.json [modify] https://crrev.com/6fa15937d2a3f46d5757b09b5dea1a185b899d44/content/shell/browser/shell_content_browser_client.cc [add] https://crrev.com/6fa15937d2a3f46d5757b09b5dea1a185b899d44/content/shell/gpu/DEPS [add] https://crrev.com/6fa15937d2a3f46d5757b09b5dea1a185b899d44/content/shell/gpu/shell_content_gpu_client.cc [add] https://crrev.com/6fa15937d2a3f46d5757b09b5dea1a185b899d44/content/shell/gpu/shell_content_gpu_client.h [modify] https://crrev.com/6fa15937d2a3f46d5757b09b5dea1a185b899d44/content/shell/shell_resources.grd
,
Jun 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/37aeb2da175acaa5a99b176130b2e0fdc9fabcd4 commit 37aeb2da175acaa5a99b176130b2e0fdc9fabcd4 Author: Ben Wells <benwells@chromium.org> Date: Wed Jun 28 04:12:12 2017 Revert "[DeviceService] Add PowerMonitor browsertest against gpu process" This reverts commit 6fa15937d2a3f46d5757b09b5dea1a185b899d44. Reason for revert: new test times out on Mac 10.11 bot: https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.11%20Tests/builds/14487 Original change's description: > [DeviceService] Add PowerMonitor browsertest against gpu process > > This CL is to confirm that in a real browser environment the gpu > process CAN succeed in connecting to Device Service to bind PowerMonitor > interface, and, via which gpu process CAN get power state change > correctly. > > BUG= 715985 > TEST=content_browsertests > > Change-Id: I2371ba0e2c2b50f99ef9356aa395a3f7eeef4870 > Reviewed-on: https://chromium-review.googlesource.com/547672 > Reviewed-by: Colin Blundell <blundell@chromium.org> > Reviewed-by: Tom Sepez <tsepez@chromium.org> > Reviewed-by: John Abd-El-Malek <jam@chromium.org> > Commit-Queue: Han Leon <leon.han@intel.com> > Cr-Commit-Position: refs/heads/master@{#482861} TBR=jam@chromium.org,blundell@chromium.org,tsepez@chromium.org,leon.han@intel.com Change-Id: I663c4a8846c0617c916858e290fc90c448af5a5a No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 715985 Reviewed-on: https://chromium-review.googlesource.com/551138 Reviewed-by: Ben Wells <benwells@chromium.org> Commit-Queue: Ben Wells <benwells@chromium.org> Cr-Commit-Position: refs/heads/master@{#482877} [modify] https://crrev.com/37aeb2da175acaa5a99b176130b2e0fdc9fabcd4/content/browser/power_monitor_browsertest.cc [modify] https://crrev.com/37aeb2da175acaa5a99b176130b2e0fdc9fabcd4/content/shell/BUILD.gn [modify] https://crrev.com/37aeb2da175acaa5a99b176130b2e0fdc9fabcd4/content/shell/app/shell_main_delegate.cc [modify] https://crrev.com/37aeb2da175acaa5a99b176130b2e0fdc9fabcd4/content/shell/app/shell_main_delegate.h [modify] https://crrev.com/37aeb2da175acaa5a99b176130b2e0fdc9fabcd4/content/shell/browser/OWNERS [delete] https://crrev.com/f2b81512b889f0b85a0e5f418aa5ee93396965ca/content/shell/browser/content_shell_gpu_manifest_overlay.json [modify] https://crrev.com/37aeb2da175acaa5a99b176130b2e0fdc9fabcd4/content/shell/browser/shell_content_browser_client.cc [delete] https://crrev.com/f2b81512b889f0b85a0e5f418aa5ee93396965ca/content/shell/gpu/DEPS [delete] https://crrev.com/f2b81512b889f0b85a0e5f418aa5ee93396965ca/content/shell/gpu/shell_content_gpu_client.cc [delete] https://crrev.com/f2b81512b889f0b85a0e5f418aa5ee93396965ca/content/shell/gpu/shell_content_gpu_client.h [modify] https://crrev.com/37aeb2da175acaa5a99b176130b2e0fdc9fabcd4/content/shell/shell_resources.grd
,
Jul 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/43528ec69cbfa04acb0e67906d7b53e7bcd064e6 commit 43528ec69cbfa04acb0e67906d7b53e7bcd064e6 Author: Han Leon <leon.han@intel.com> Date: Fri Jul 07 02:54:57 2017 Reland 1: [DeviceService] Add PowerMonitor browsertest against gpu process The original CL https://chromium-review.googlesource.com/c/547672/ was reverted because of timeout of PowerMonitorTest.TestGpuProcess on https://build.chromium.org/p/chromium.mac/builders/Mac10.11%20Tests/builds/14487 Original CL description: This CL is to confirm that in a real browser environment the gpu process CAN succeed in connecting to Device Service to bind PowerMonitor interface, and, via which gpu process CAN get power state change correctly. BUG= 715985 TEST=content_browsertests TBR=jam@chromium.org Change-Id: Ib9f4bd0ad7b779f61d13eb7fbee9b2ab56d3d354 Reviewed-on: https://chromium-review.googlesource.com/554339 Commit-Queue: Han Leon <leon.han@intel.com> Reviewed-by: Colin Blundell <blundell@chromium.org> Reviewed-by: Tom Sepez <tsepez@chromium.org> Cr-Commit-Position: refs/heads/master@{#484812} [modify] https://crrev.com/43528ec69cbfa04acb0e67906d7b53e7bcd064e6/content/browser/power_monitor_browsertest.cc [modify] https://crrev.com/43528ec69cbfa04acb0e67906d7b53e7bcd064e6/content/shell/BUILD.gn [modify] https://crrev.com/43528ec69cbfa04acb0e67906d7b53e7bcd064e6/content/shell/app/shell_main_delegate.cc [modify] https://crrev.com/43528ec69cbfa04acb0e67906d7b53e7bcd064e6/content/shell/app/shell_main_delegate.h [modify] https://crrev.com/43528ec69cbfa04acb0e67906d7b53e7bcd064e6/content/shell/browser/OWNERS [add] https://crrev.com/43528ec69cbfa04acb0e67906d7b53e7bcd064e6/content/shell/browser/content_shell_gpu_manifest_overlay.json [modify] https://crrev.com/43528ec69cbfa04acb0e67906d7b53e7bcd064e6/content/shell/browser/shell_content_browser_client.cc [add] https://crrev.com/43528ec69cbfa04acb0e67906d7b53e7bcd064e6/content/shell/gpu/DEPS [add] https://crrev.com/43528ec69cbfa04acb0e67906d7b53e7bcd064e6/content/shell/gpu/shell_content_gpu_client.cc [add] https://crrev.com/43528ec69cbfa04acb0e67906d7b53e7bcd064e6/content/shell/gpu/shell_content_gpu_client.h [modify] https://crrev.com/43528ec69cbfa04acb0e67906d7b53e7bcd064e6/content/shell/shell_resources.grd
,
Jul 12 2017
,
Nov 7 2017
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by leon....@intel.com
, Apr 28 2017Owner: leon....@intel.com