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

content/browser/background_sync/background_sync_manager_unittest.cc

 // Copyright 2015 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 "content/browser/background_sync/background_sync_manager.h"
 
 #include "base/files/scoped_temp_dir.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/power_monitor/power_monitor.h"
 #include "base/power_monitor/power_monitor_source.h"
 #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"
 #include "content/browser/service_worker/embedded_worker_test_helper.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/browser/service_worker/service_worker_storage.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "net/base/network_change_notifier.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ skipping 199 matching lines @@
     sync_options_1_.network_state = NETWORK_STATE_ONLINE;
     sync_options_1_.power_state = POWER_STATE_AUTO;
 
     sync_options_2_.tag = "bar";
     sync_options_2_.periodicity = SYNC_ONE_SHOT;
     sync_options_2_.network_state = NETWORK_STATE_ONLINE;
     sync_options_2_.power_state = POWER_STATE_AUTO;
   }
 
   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));
 
     power_monitor_source_ = new TestPowerSource();
     // power_monitor_ takes ownership of power_monitor_source.
     power_monitor_.reset(new base::PowerMonitor(
         scoped_ptr<base::PowerMonitorSource>(power_monitor_source_)));
 
     SetOnBatteryPower(false);
 
     SetupBackgroundSyncManager();
 
     // Wait for storage to finish initializing before registering service
     // workers.
     base::RunLoop().RunUntilIdle();
     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;
     helper_->context()->RegisterServiceWorker(
         GURL(kPattern1), GURL(kScript1), NULL,
         base::Bind(&RegisterServiceWorkerCallback, &called_1,
                    &sw_registration_id_1_));
 
     helper_->context()->RegisterServiceWorker(
         GURL(kPattern2), GURL(kScript2), NULL,
@@ skipping 33 matching lines @@
   }
 
   void RemoveWindowClients() {
     sw_registration_1_->active_version()->RemoveControllee(host_1_.get());
     sw_registration_2_->active_version()->RemoveControllee(host_2_.get());
   }
 
   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) {
     power_monitor_source_->GeneratePowerStateEvent(on_battery_power);
     base::RunLoop().RunUntilIdle();
   }
 
   void StatusAndRegistrationCallback(
       bool* was_called,
       BackgroundSyncStatus status,
@@ skipping 1291 matching lines @@
 }
 
 TEST_F(BackgroundSyncManagerTest, UnregisterSucceedsWithoutWindow) {
   EXPECT_TRUE(Register(sync_options_1_));
   RemoveWindowClients();
   EXPECT_TRUE(Unregister(callback_registration_handle_.get()));
   EXPECT_FALSE(GetRegistration(sync_options_1_));
 }
 
 }  // namespace content