Mojify the battery status browsertests.

content/browser/battery_status/battery_status_browsertest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/callback_list.h"
 #include "base/command_line.h"
+#include "base/lazy_instance.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/thread_task_runner_handle.h"
+#include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/content_client.h"
 #include "content/public/common/content_switches.h"
+#include "content/public/common/service_registry.h"
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
+#include "content/shell/browser/shell_content_browser_client.h"
+#include "device/battery/battery_monitor.mojom.h"
 #include "device/battery/battery_status_manager.h"
 #include "device/battery/battery_status_service.h"
 
 namespace content {
 
 namespace {
 
-class FakeBatteryManager : public device::BatteryStatusManager {
+typedef base::CallbackList<void(const device::BatteryStatus&)>
+    BatteryUpdateCallbackList;
+typedef BatteryUpdateCallbackList::Subscription BatteryUpdateSubscription;
+
+// Global battery state used in the tests.
+device::BatteryStatus g_battery_status;
+// Global list of test battery monitors to notify when |g_battery_status|
+// changes.
+base::LazyInstance<BatteryUpdateCallbackList> g_callback_list =
+    LAZY_INSTANCE_INITIALIZER;
+
+// Updates the global battery state and notifies existing test monitors.
+void UpdateBattery(const device::BatteryStatus& battery_status) {
+  g_battery_status = battery_status;
+  g_callback_list.Get().Notify(battery_status);
+}
+
+class TestBatteryMonitor : public mojo::InterfaceImpl<device::BatteryMonitor> {

[timvolodine, 2014/11/18 18:55:02]

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.

[ppi, 2014/11/19 10:16:19]

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?

[timvolodine, 2014/11/19 14:11:28]

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

[ppi, 2014/11/19 18:38:49]

Done.

  public:
-  explicit FakeBatteryManager(
-      const device::BatteryStatusService::BatteryUpdateCallback& callback)
-      : callback_(callback), battery_status_available_(true), started_(false) {}
-  ~FakeBatteryManager() override {}
-
-  // Methods from BatteryStatusManager.
-  bool StartListeningBatteryChange() override {
-    started_ = true;
-    if (battery_status_available_)
-      InvokeUpdateCallback();
-    return battery_status_available_;
-  }
-
-  void StopListeningBatteryChange() override {}
-
-  void InvokeUpdateCallback() {
-    // Invoke asynchronously to mimic the OS-specific battery managers.
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
-        base::Bind(callback_, status_));
-  }
-
-  void set_battery_status(const device::BatteryStatus& status) {
-    status_ = status;
-  }
-
-  void set_battery_status_available(bool value) {
-    battery_status_available_ = value;
-  }
-
-  bool started() {
-    return started_;
+  static void Create(mojo::InterfaceRequest<BatteryMonitor> request) {
+    BindToRequest(new TestBatteryMonitor(), &request);
   }
 
  private:
-  device::BatteryStatusService::BatteryUpdateCallback callback_;
-  bool battery_status_available_;
-  bool started_;
-  device::BatteryStatus status_;
+  TestBatteryMonitor() {}
+  ~TestBatteryMonitor() override {}
 
-  DISALLOW_COPY_AND_ASSIGN(FakeBatteryManager);
+  // mojo::InterfaceImpl<..> methods:
+  void OnConnectionEstablished() override {
+    DidChange(g_battery_status);
+    subscription_ = g_callback_list.Get().Add(
+        base::Bind(&TestBatteryMonitor::DidChange, base::Unretained(this)));
+  }
+
+  void DidChange(const device::BatteryStatus& battery_status) {
+    device::BatteryStatusPtr status(device::BatteryStatus::New());
+    *status = battery_status;
+    client()->DidChange(status.Pass());
+  }
+
+  scoped_ptr<BatteryUpdateSubscription> subscription_;
+};
+
+// Overrides the default battery monitor implementation with the test
+// implementation declared above.
+class TestContentBrowserClient : public ContentBrowserClient {
+ public:
+  void OverridePerProcessMojoServices(ServiceRegistry* registry) override {
+    registry->AddService(base::Bind(&TestBatteryMonitor::Create));
+  }
 };
 
 class BatteryStatusBrowserTest : public ContentBrowserTest  {
  public:
-    BatteryStatusBrowserTest()
-      : battery_manager_(NULL),
-        battery_service_(NULL) {
-  }
+  BatteryStatusBrowserTest() {}
 
     void SetUpCommandLine(CommandLine* command_line) override {

[timvolodine, 2014/11/18 18:55:02]

I think we can remove this completely, since battery status is enabled by
default now.

[ppi, 2014/11/19 10:16:19]

Done.

     command_line->AppendSwitch(
         switches::kEnableExperimentalWebPlatformFeatures);
   }
 
   void SetUpOnMainThread() override {
-    battery_service_ = device::BatteryStatusService::GetInstance();
-
-    // We keep a raw pointer to the FakeBatteryManager, which we expect to
-    // remain valid for the lifetime of the BatteryStatusService.
-    scoped_ptr<FakeBatteryManager> battery_manager(new FakeBatteryManager(
-        battery_service_->GetUpdateCallbackForTesting()));
-    battery_manager_ = battery_manager.get();
-
-    battery_service_->SetBatteryManagerForTesting(
-        battery_manager.Pass());
+    old_client_ = SetBrowserClientForTesting(&test_client_);
   }
 
-  void TearDown() override {
-    battery_service_->SetBatteryManagerForTesting(
-        scoped_ptr<device::BatteryStatusManager>());
-    battery_manager_ = NULL;
-  }
-
-  FakeBatteryManager* battery_manager() {
-    return battery_manager_;
+  void TearDownOnMainThread() override {
+    SetBrowserClientForTesting(old_client_);
   }
 
  private:
-  FakeBatteryManager* battery_manager_;
-  device::BatteryStatusService* battery_service_;
+  TestContentBrowserClient test_client_;
+  ContentBrowserClient* old_client_;
 
   DISALLOW_COPY_AND_ASSIGN(BatteryStatusBrowserTest);
 };
 
 IN_PROC_BROWSER_TEST_F(BatteryStatusBrowserTest, BatteryManagerDefaultValues) {
-  // Set the fake battery manager to return false on start. From JavaScript
-  // request a promise for the battery status information and once it resolves
-  // check the default values and navigate to #pass.
-  battery_manager()->set_battery_status_available(false);
+  // From JavaScript request a promise for the battery status information and
+  // once it resolves check the default values and navigate to #pass.
+  UpdateBattery(device::BatteryStatus());
   GURL test_url = GetTestUrl(
       "battery_status", "battery_status_default_test.html");
   NavigateToURLBlockUntilNavigationsComplete(shell(), test_url, 2);
   EXPECT_EQ("pass", shell()->web_contents()->GetLastCommittedURL().ref());
-  EXPECT_TRUE(battery_manager()->started());
 }
 
 IN_PROC_BROWSER_TEST_F(BatteryStatusBrowserTest, BatteryManagerResolvePromise) {
   // Set the fake battery manager to return predefined battery status values.
   // From JavaScript request a promise for the battery status information and
   // once it resolves check the values and navigate to #pass.
   device::BatteryStatus status;
   status.charging = true;
   status.charging_time = 100;
   status.discharging_time = std::numeric_limits<double>::infinity();
   status.level = 0.5;
-  battery_manager()->set_battery_status(status);
+  UpdateBattery(status);
 
   GURL test_url = GetTestUrl(
       "battery_status", "battery_status_promise_resolution_test.html");
   NavigateToURLBlockUntilNavigationsComplete(shell(), test_url, 2);
   EXPECT_EQ("pass", shell()->web_contents()->GetLastCommittedURL().ref());
-  EXPECT_TRUE(battery_manager()->started());
 }
 
 IN_PROC_BROWSER_TEST_F(BatteryStatusBrowserTest,
     BatteryManagerWithEventListener) {
   // Set the fake battery manager to return default battery status values.
   // From JavaScript request a promise for the battery status information.
   // Once it resolves add an event listener for battery level change. Set
   // battery level to 0.6 and invoke update. Check that the event listener
   // is invoked with the correct value for level and navigate to #pass.
   device::BatteryStatus status;
-  battery_manager()->set_battery_status(status);
+  UpdateBattery(status);
 
   TestNavigationObserver same_tab_observer(shell()->web_contents(), 2);
   GURL test_url = GetTestUrl(
       "battery_status", "battery_status_event_listener_test.html");
   shell()->LoadURL(test_url);
   same_tab_observer.Wait();
   EXPECT_EQ("resolved", shell()->web_contents()->GetLastCommittedURL().ref());
 
   TestNavigationObserver same_tab_observer2(shell()->web_contents(), 1);
   status.level = 0.6;
-  battery_manager()->set_battery_status(status);
-  battery_manager()->InvokeUpdateCallback();
+  UpdateBattery(status);
   same_tab_observer2.Wait();
   EXPECT_EQ("pass", shell()->web_contents()->GetLastCommittedURL().ref());
-  EXPECT_TRUE(battery_manager()->started());
 }
 
 }  //  namespace
 
 }  //  namespace content