Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
734123002:
Mojify the battery status browsertests. (Closed)
DescriptionMojify the battery status browsertests.
This patch adds battery status browsertests that run against a test
implementation of BatteryMonitor, based on existing tests. The new
tests will run on all platforms regardless of the particular service
implementation.
BUG=420623
Committed: https://crrev.com/4526632ac82ea2bd0fca4f7a8adbb29566c13e54
Cr-Commit-Position: refs/heads/master@{#306847}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : Clang-format. #
Total comments: 4
Patch Set 3 : Address Ben's comments. #
Total comments: 6
Patch Set 4 : Address Tim's comments. #
Total comments: 4
Patch Set 5 : Rebase. #Patch Set 6 : Address Tim's comments: keep existing end-to-end tests. #
Total comments: 11
Patch Set 7 : Rebase. #Patch Set 8 : Address Tim's comments. #Patch Set 9 : Expose the test override in RenderProcessHostImpl instead of the client API. #Patch Set 10 : Rebase. #Patch Set 11 : Back to using the content client API now that it's landed. #
Messages
Total messages: 23 (5 generated)
ppi@chromium.org changed reviewers: + timvolodine@chromium.org
Patchset #1 (id:1) has been deleted
ppi@chromium.org changed reviewers: + qsr@chromium.org
Tim, could you review the test changes? Ben, could you review the OverridePerProcessMojoServices API?
https://codereview.chromium.org/734123002/diff/20001/content/browser/battery_... File content/browser/battery_status/battery_status_browsertest.cc (right): https://codereview.chromium.org/734123002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_browsertest.cc:29: BatteryUpdateCallbackList; format seems wrong, I expect 4 character indentation for continuation. There seems to have other issue like this below, could you clang-format this?
https://codereview.chromium.org/734123002/diff/20001/content/browser/battery_... File content/browser/battery_status/battery_status_browsertest.cc (right): https://codereview.chromium.org/734123002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_browsertest.cc:29: BatteryUpdateCallbackList; On 2014/11/18 09:24:44, qsr wrote: > format seems wrong, I expect 4 character indentation for continuation. > > There seems to have other issue like this below, could you clang-format this? Done.
LGTM with nits. https://codereview.chromium.org/734123002/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_browsertest.cc (right): https://codereview.chromium.org/734123002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_browsertest.cc:93: void TearDown() override { SetBrowserClientForTesting(old_client_); } Don't you want to use TearDownOnMainThread here? https://codereview.chromium.org/734123002/diff/40001/content/public/browser/c... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/734123002/diff/40001/content/public/browser/c... content/public/browser/content_browser_client.h:630: virtual void OverridePerProcessMojoServices(ServiceRegistry* registry); Maybe forward declare ServiceRegistry instead of importing it?
Thanks! https://codereview.chromium.org/734123002/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_browsertest.cc (right): https://codereview.chromium.org/734123002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_browsertest.cc:93: void TearDown() override { SetBrowserClientForTesting(old_client_); } On 2014/11/18 11:11:50, qsr wrote: > Don't you want to use TearDownOnMainThread here? Done. https://codereview.chromium.org/734123002/diff/40001/content/public/browser/c... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/734123002/diff/40001/content/public/browser/c... content/public/browser/content_browser_client.h:630: virtual void OverridePerProcessMojoServices(ServiceRegistry* registry); On 2014/11/18 11:11:50, qsr wrote: > Maybe forward declare ServiceRegistry instead of importing it? Done.
https://codereview.chromium.org/734123002/diff/60001/content/browser/battery_... File content/browser/battery_status/battery_status_browsertest.cc (right): https://codereview.chromium.org/734123002/diff/60001/content/browser/battery_... content/browser/battery_status/battery_status_browsertest.cc:46: class TestBatteryMonitor : public mojo::InterfaceImpl<device::BatteryMonitor> { The test was originally meant for end-to-end testing, so I don't really understand why you need to create a new mojo service for this. Is it not sufficient to set a fake battery manager in battery_status_service? that way there is also no need to keep a global list of callbacks.. also this browser_test does not test the normal path which is via the battery_status_service.. so if I break the service this end-to-end test will still pass. https://codereview.chromium.org/734123002/diff/60001/content/browser/battery_... content/browser/battery_status/battery_status_browsertest.cc:85: void SetUpCommandLine(CommandLine* command_line) override { I think we can remove this completely, since battery status is enabled by default now.
Thanks, Tim; ptal. https://codereview.chromium.org/734123002/diff/60001/content/browser/battery_... File content/browser/battery_status/battery_status_browsertest.cc (right): https://codereview.chromium.org/734123002/diff/60001/content/browser/battery_... content/browser/battery_status/battery_status_browsertest.cc:46: class TestBatteryMonitor : public mojo::InterfaceImpl<device::BatteryMonitor> { On 2014/11/18 18:55:02, timvolodine wrote: > The test was originally meant for end-to-end testing, so I don't really > understand why you need to create a new mojo service for this. Is it not > sufficient to set a fake battery manager in battery_status_service? that way > there is also no need to keep a global list of callbacks.. also this > browser_test does not test the normal path which is via the > battery_status_service.. so if I break the service this end-to-end test will > still pass. This change is needed to allow independent implementations of the service for particular platforms (see for instance https://codereview.chromium.org/641703002/ which is blocked on this). Current tests rely on details of the particular service implementation that we have in Chrome, so they will break once we swap a different implementation in. Surely, with this change the role of these tests changes - they are "less" end-to-end (previous ones also used fake BatteryManager though): now they are veryfing that whatever implementation of the service we swap in, it's being is correctly exposed to the renderer. Would you prefer that we keep the existing service-specific tests (and disable them on various platforms as we go) and add the new ones separately (as battery_monitor_browsertests)? Wdyt? https://codereview.chromium.org/734123002/diff/60001/content/browser/battery_... content/browser/battery_status/battery_status_browsertest.cc:85: void SetUpCommandLine(CommandLine* command_line) override { On 2014/11/18 18:55:02, timvolodine wrote: > I think we can remove this completely, since battery status is enabled by > default now. Done.
https://codereview.chromium.org/734123002/diff/60001/content/browser/battery_... File content/browser/battery_status/battery_status_browsertest.cc (right): https://codereview.chromium.org/734123002/diff/60001/content/browser/battery_... content/browser/battery_status/battery_status_browsertest.cc:46: class TestBatteryMonitor : public mojo::InterfaceImpl<device::BatteryMonitor> { On 2014/11/19 10:16:19, ppi wrote: > On 2014/11/18 18:55:02, timvolodine wrote: > > The test was originally meant for end-to-end testing, so I don't really > > understand why you need to create a new mojo service for this. Is it not > > sufficient to set a fake battery manager in battery_status_service? that way > > there is also no need to keep a global list of callbacks.. also this > > browser_test does not test the normal path which is via the > > battery_status_service.. so if I break the service this end-to-end test will > > still pass. > > This change is needed to allow independent implementations of the service for > particular platforms (see for instance > https://codereview.chromium.org/641703002/ which is blocked on this). > > Current tests rely on details of the particular service implementation that we > have in Chrome, so they will break once we swap a different implementation in. > > Surely, with this change the role of these tests changes - they are "less" > end-to-end (previous ones also used fake BatteryManager though): now they are > veryfing that whatever implementation of the service we swap in, it's being is > correctly exposed to the renderer. > > Would you prefer that we keep the existing service-specific tests (and disable > them on various platforms as we go) and add the new ones separately (as > battery_monitor_browsertests)? > > Wdyt? Ah I see, that makes more sense :). Yes in my opinion I think it's desirable to have end-to-end tests (they tend to break first if something goes wrong). So adding the new mojo ones separately and disabling the old ones as we go seems like the most stable approach.. https://codereview.chromium.org/734123002/diff/80001/content/browser/battery_... File content/browser/battery_status/battery_status_browsertest.cc (right): https://codereview.chromium.org/734123002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_browsertest.cc:6: #include "base/command_line.h" is this include needed? https://codereview.chromium.org/734123002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_browsertest.cc:13: #include "content/public/common/content_switches.h" and this?
Thanks, Tim! Ptal. https://codereview.chromium.org/734123002/diff/60001/content/browser/battery_... File content/browser/battery_status/battery_status_browsertest.cc (right): https://codereview.chromium.org/734123002/diff/60001/content/browser/battery_... content/browser/battery_status/battery_status_browsertest.cc:46: class TestBatteryMonitor : public mojo::InterfaceImpl<device::BatteryMonitor> { On 2014/11/19 14:11:28, timvolodine wrote: > On 2014/11/19 10:16:19, ppi wrote: > > On 2014/11/18 18:55:02, timvolodine wrote: > > > The test was originally meant for end-to-end testing, so I don't really > > > understand why you need to create a new mojo service for this. Is it not > > > sufficient to set a fake battery manager in battery_status_service? that way > > > there is also no need to keep a global list of callbacks.. also this > > > browser_test does not test the normal path which is via the > > > battery_status_service.. so if I break the service this end-to-end test will > > > still pass. > > > > This change is needed to allow independent implementations of the service for > > particular platforms (see for instance > > https://codereview.chromium.org/641703002/ which is blocked on this). > > > > Current tests rely on details of the particular service implementation that we > > have in Chrome, so they will break once we swap a different implementation in. > > > > Surely, with this change the role of these tests changes - they are "less" > > end-to-end (previous ones also used fake BatteryManager though): now they are > > veryfing that whatever implementation of the service we swap in, it's being is > > correctly exposed to the renderer. > > > > Would you prefer that we keep the existing service-specific tests (and disable > > them on various platforms as we go) and add the new ones separately (as > > battery_monitor_browsertests)? > > > > Wdyt? > > Ah I see, that makes more sense :). Yes in my opinion I think it's desirable to > have end-to-end tests (they tend to break first if something goes wrong). So > adding the new mojo ones separately and disabling the old ones as we go seems > like the most stable approach.. Done. https://codereview.chromium.org/734123002/diff/80001/content/browser/battery_... File content/browser/battery_status/battery_status_browsertest.cc (right): https://codereview.chromium.org/734123002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_browsertest.cc:6: #include "base/command_line.h" On 2014/11/19 14:11:29, timvolodine wrote: > is this include needed? Done. https://codereview.chromium.org/734123002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_browsertest.cc:13: #include "content/public/common/content_switches.h" On 2014/11/19 14:11:29, timvolodine wrote: > and this? Done.
thanks Przemek! lgtm modulo nits. https://codereview.chromium.org/734123002/diff/120001/content/browser/battery... File content/browser/battery_status/battery_monitor_impl_browsertest.cc (right): https://codereview.chromium.org/734123002/diff/120001/content/browser/battery... content/browser/battery_status/battery_monitor_impl_browsertest.cc:5: #include "base/synchronization/waitable_event.h" not sure if this is needed? https://codereview.chromium.org/734123002/diff/120001/content/browser/battery... File content/browser/battery_status/battery_monitor_integration_browsertest.cc (right): https://codereview.chromium.org/734123002/diff/120001/content/browser/battery... content/browser/battery_status/battery_monitor_integration_browsertest.cc:7: #include "base/synchronization/waitable_event.h" do we need this? https://codereview.chromium.org/734123002/diff/120001/content/browser/battery... content/browser/battery_status/battery_monitor_integration_browsertest.cc:21: #include "device/battery/battery_status_service.h" no need for this? https://codereview.chromium.org/734123002/diff/120001/content/browser/battery... content/browser/battery_status/battery_monitor_integration_browsertest.cc:49: class TestBatteryMonitor : public device::BatteryMonitor { nit: I think FakeBatteryMonitor is more common. https://codereview.chromium.org/734123002/diff/120001/content/browser/battery... content/browser/battery_status/battery_monitor_integration_browsertest.cc:116: // Set the fake battery manager to return predefined battery status values. "battery manager" -> "battery monitor"? https://codereview.chromium.org/734123002/diff/120001/content/browser/battery... content/browser/battery_status/battery_monitor_integration_browsertest.cc:134: // Set the fake battery manager to return default battery status values. "battery manager" -> "battery monitor"?
ppi@chromium.org changed reviewers: + jam@chromium.org
+Jam for OWNER approval. Jam, ptal. https://codereview.chromium.org/734123002/diff/120001/content/browser/battery... File content/browser/battery_status/battery_monitor_impl_browsertest.cc (right): https://codereview.chromium.org/734123002/diff/120001/content/browser/battery... content/browser/battery_status/battery_monitor_impl_browsertest.cc:5: #include "base/synchronization/waitable_event.h" On 2014/11/20 16:20:08, timvolodine wrote: > not sure if this is needed? Done. https://codereview.chromium.org/734123002/diff/120001/content/browser/battery... File content/browser/battery_status/battery_monitor_integration_browsertest.cc (right): https://codereview.chromium.org/734123002/diff/120001/content/browser/battery... content/browser/battery_status/battery_monitor_integration_browsertest.cc:21: #include "device/battery/battery_status_service.h" On 2014/11/20 16:20:08, timvolodine wrote: > no need for this? Done. https://codereview.chromium.org/734123002/diff/120001/content/browser/battery... content/browser/battery_status/battery_monitor_integration_browsertest.cc:49: class TestBatteryMonitor : public device::BatteryMonitor { On 2014/11/20 16:20:08, timvolodine wrote: > nit: I think FakeBatteryMonitor is more common. Done. https://codereview.chromium.org/734123002/diff/120001/content/browser/battery... content/browser/battery_status/battery_monitor_integration_browsertest.cc:116: // Set the fake battery manager to return predefined battery status values. On 2014/11/20 16:20:08, timvolodine wrote: > "battery manager" -> "battery monitor"? Done. https://codereview.chromium.org/734123002/diff/120001/content/browser/battery... content/browser/battery_status/battery_monitor_integration_browsertest.cc:134: // Set the fake battery manager to return default battery status values. On 2014/11/20 16:20:09, timvolodine wrote: > "battery manager" -> "battery monitor"? Done.
per http://www.chromium.org/developers/content-module/content-api, changes to content/public should only be done if an embedder uses them. in this case, this is all internal content code.
Thanks, Jam! At some point we will probably need this override for non-Chrome embedders wanting to override some of the browser services. Until then, I switched to a test hook exposed in RenderProcessHostImpl, does that lgty? Ptal.
On 2014/11/26 15:41:14, ppi wrote: > Thanks, Jam! > > At some point we will probably need this override for non-Chrome embedders > wanting to override some of the browser services. > > Until then, I switched to a test hook exposed in RenderProcessHostImpl, does > that lgty? > > Ptal. (excuse the delay, was ooo) can this be a non-static method, i.e. do you know ahead of time the RPH that you can call a method on? always better to avoid static methods/globals if possible
Thanks, Jam! As the content client override API landed in https://codereview.chromium.org/772433004/ , we no longer need to change render_frame_host_impl in this CL. This CL is now limited to (already reviewed) changes in tests.
The CQ bit was checked by ppi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/734123002/220001
Message was sent while issue was closed.
Committed patchset #11 (id:220001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/4526632ac82ea2bd0fca4f7a8adbb29566c13e54 Cr-Commit-Position: refs/heads/master@{#306847} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
