[BackgroundSync] Trigger Background Sync events when Chrome is backgrounded on Android

content/browser/background_sync/background_sync_manager_unittest.cc

Index: content/browser/background_sync/background_sync_manager_unittest.cc
diff --git a/content/browser/background_sync/background_sync_manager_unittest.cc b/content/browser/background_sync/background_sync_manager_unittest.cc
index 22e3c76940c6c2f9b69477cfa681cf276a247ab0..3056594e8845bab0c02f8d65894a56be94ebbf45 100644
--- a/content/browser/background_sync/background_sync_manager_unittest.cc
+++ b/content/browser/background_sync/background_sync_manager_unittest.cc
@@ -12,6 +12,7 @@
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/thread_task_runner_handle.h"
+#include "content/browser/background_sync/background_sync_network_observer.h"
 #include "content/browser/background_sync/background_sync_registration_handle.h"
 #include "content/browser/background_sync/background_sync_status.h"
 #include "content/browser/browser_thread_impl.h"
@@ -231,6 +232,9 @@ class BackgroundSyncManagerTest : public testing::Test {
   }
 
   void SetUp() override {
+    // Don't let the tests be confused by the real-world device connectivity
+    BackgroundSyncNetworkObserver::SetIgnoreNetworkChangeNotifierForTests(true);
+
     helper_.reset(
         new EmbeddedWorkerTestHelper(base::FilePath(), kRenderProcessId));
 
@@ -249,6 +253,12 @@ class BackgroundSyncManagerTest : public testing::Test {
     RegisterServiceWorkers();
   }
 
+  void TearDown() override {
+    // Restore the network observer functionality for subsequent tests

[jkarlin, 2015/09/24 17:51:30]

I don't believe this is necessary? Each test runs independently.

[iclelland, 2015/09/24 20:37:19]

They're not completely isolated, and this is a process-wide global setting. In
content_unittests_apk, they are run in blocks of 256 tests at a time, and not
resetting this was causing the BackgroundSyncNetworkObserver unit tests to fail
if they ran after these ones. This makes these tests cleaner, by undoing the
modifications that SetUp makes to the environment.

[jkarlin, 2015/09/24 23:33:12]

Acknowledged.

+    BackgroundSyncNetworkObserver::SetIgnoreNetworkChangeNotifierForTests(
+        false);
+  }
+
   void RegisterServiceWorkers() {
     bool called_1 = false;
     bool called_2 = false;
@@ -302,7 +312,12 @@ class BackgroundSyncManagerTest : public testing::Test {
   void SetNetwork(net::NetworkChangeNotifier::ConnectionType connection_type) {
     net::NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests(
         connection_type);
-    base::RunLoop().RunUntilIdle();
+    if (test_background_sync_manager_) {
+      BackgroundSyncNetworkObserver* network_observer =
+          test_background_sync_manager_->GetNetworkObserverForTesting();
+      network_observer->NotifyManagerIfNetworkChanged(connection_type);

[jkarlin, 2015/09/24 17:51:30]

The name "NotifyManagerIfNetworkChanged" doesn't seem to fit here. I'd expect
more of a "SetNetworkTypeForTesting()" or something.

[iclelland, 2015/09/24 20:37:19]

This code is actually calling the BSNO method which triggers its observers if
the network state has changed. It's the same method that is called by the
network_observer object's OnNetworkChanged(), except that OnNetworkChanged can
be stopped by using SetIgnoreNetworkChangeNotifierForTests, and this method
always calls down to the observers.

[jkarlin, 2015/09/24 23:33:12]

I realize that, but the name seems strange from this context. But not a strong
preference on my part.

[iclelland, 2015/09/25 13:05:01]

Would something like OnNetworkChangedImpl or OnNetworkChangedInternal be clearer
here? It signifies that we're not calling the public interface, we're directly
calling the method that does the work (skipping some of the checks for testing)

[jkarlin, 2015/09/25 13:08:57]

Yes, OnNetworkChangedImpl would do nicely.

+      base::RunLoop().RunUntilIdle();
+    }
   }
 
   void SetOnBatteryPower(bool on_battery_power) {