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

content/browser/background_sync/background_sync_browsertest.cc

Index: content/browser/background_sync/background_sync_browsertest.cc
diff --git a/content/browser/background_sync/background_sync_browsertest.cc b/content/browser/background_sync/background_sync_browsertest.cc
index 9fe51e5417c22a03bee6b546cc3d3e676b425190..6671b219e37e313213254dae8f74af898efcf6a0 100644
--- a/content/browser/background_sync/background_sync_browsertest.cc
+++ b/content/browser/background_sync/background_sync_browsertest.cc
@@ -11,6 +11,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/task_runner_util.h"
 #include "content/browser/background_sync/background_sync_manager.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/service_worker/service_worker_context_wrapper.h"
@@ -28,6 +29,10 @@
 #include "net/base/network_change_notifier.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+#if defined(OS_ANDROID)
+#include "content/browser/android/background_sync_network_observer_android.h"
+#endif
+
 using net::NetworkChangeNotifier;
 
 namespace content {
@@ -106,6 +111,12 @@ class BackgroundSyncBrowserTest : public ContentBrowserTest {
     net::NetworkChangeNotifier::CreateMock();
 #endif
 
+#if defined(OS_ANDROID)
+    // On Android, the actual device connectivity status can interfere with some
+    // of the browser tests
+    BackgroundSyncNetworkObserverAndroid::DisableNotificationForTesting();

[jkarlin, 2015/09/19 00:16:07]

What if you added IgnoreNetworkChangeNotifierForTesting() to
BackgroundSyncNetworkObserver? Then you could get rid of the ifdefs in this file
and the SetTestNtoificationsOnly(true) line above. You could implement it by
simply not calling the callback from BSNO when notifications come in.

Alternatively, and I like this a bit more, you could make a
MockBackgroundSyncNetworkObserver and call
BackgroundSyncManager->SetNetworkObserver(mock) and just change the network as
necessary via the mock observer. Then the BackgroundSyncNetworkObservers
wouldn't need any ForTesting functions at all. The observers would need a
SetCallback function though so that the BSM could change the mock's callback in
SetNetworkObserver. 

[iclelland, 2015/09/21 18:49:30]

I'll try that approach; it might be cleaner than passing Disable..() up over JNI
to the Java object. It'll have to be a static method, so that all observers
(including those not yet created) are affected, though.
This is actually what I wanted to do, but it seemed more complicated than it
really should be, just for testing. I'd have to set the BSM behaviour before it
gets constructed.
The BSM creates the observer in its constructor, and on Android, on creation,
the Java BSNO needs to send the *actual* connectivity state down to the BSM.
This is fine in the real world, but in testing, if we've set the status to
offline in a regular tab, and then create a new incognito tab, the actual online
state will trigger the BSM code, and fire events that we don't want to happen
yet. We'd need to interfere with the construction process, with something like
an ObserverFactory injected into the BackgroundSyncContext, and it started to
look like a lot more complexity than was warranted for a test method.

[iclelland, 2015/09/22 13:18:15]

This is done now, in the latest patch set.

+#endif
+
     ContentBrowserTest::SetUp();
   }
 
@@ -113,6 +124,12 @@ class BackgroundSyncBrowserTest : public ContentBrowserTest {
     shell_ = incognito ? CreateOffTheRecordBrowser() : shell();
   }
 
+  BackgroundSyncContext* GetSyncContextFromShell(Shell* shell) {

[jkarlin, 2015/09/19 00:16:07]

Can you use this new function in OneShotPending?

[iclelland, 2015/09/21 18:49:30]

I could, but we need the storage partition separately in OneShotPending as well
(to get the ServiceWorkerContext), so it doesn't really seem to make anything
clearer.

+    StoragePartition* storage = BrowserContext::GetDefaultStoragePartition(
+        shell_->web_contents()->GetBrowserContext());
+    return storage->GetBackgroundSyncContext();
+  }
+
   void SetUpCommandLine(base::CommandLine* command_line) override {
     // TODO(jkarlin): Remove this once background sync is no longer
     // experimental.
@@ -128,10 +145,10 @@ class BackgroundSyncBrowserTest : public ContentBrowserTest {
         base::FilePath(FILE_PATH_LITERAL("content/test/data/"))));
     ASSERT_TRUE(https_server_->Start());
 
-    SetOnline(true);
-
     SetIncognitoMode(false);
 
+    SetOnline(true);
+
     ASSERT_TRUE(LoadTestPage(kDefaultTestURL));
 
     ContentBrowserTest::SetUpOnMainThread();
@@ -149,6 +166,9 @@ class BackgroundSyncBrowserTest : public ContentBrowserTest {
   }
 
   void SetOnline(bool online);

[jkarlin, 2015/09/19 00:16:07]

Please add comment noting that this runs asynchronously on the IO thread but we
don't need to wait for it to complete before running a background sync operation
since those also run on the IO thread.

+  void SetOnlineOnIOThread(
+      const scoped_refptr<BackgroundSyncContext>& sync_context,
+      bool online);
 
   // Returns true if the one-shot sync with tag is currently pending. Fails
   // (assertion failure) if the tag isn't registered.
@@ -172,14 +192,26 @@ class BackgroundSyncBrowserTest : public ContentBrowserTest {
 };
 
 void BackgroundSyncBrowserTest::SetOnline(bool online) {
+  ASSERT_TRUE(shell_);
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&BackgroundSyncBrowserTest::SetOnlineOnIOThread,
+                 base::Unretained(this),
+                 base::Unretained(GetSyncContextFromShell(shell_)), online));
+  base::RunLoop().RunUntilIdle();
+}
+
+void BackgroundSyncBrowserTest::SetOnlineOnIOThread(
+    const scoped_refptr<BackgroundSyncContext>& sync_context,
+    bool online) {
+  BackgroundSyncManager* sync_manager = sync_context->background_sync_manager();
+  BackgroundSyncNetworkObserver* network_observer =
+      sync_manager->GetNetworkObserverForTesting();
   if (online) {
-    NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests(
-        NetworkChangeNotifier::CONNECTION_WIFI);
+    network_observer->OnNetworkChanged(NetworkChangeNotifier::CONNECTION_WIFI);
   } else {
-    NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests(
-        NetworkChangeNotifier::CONNECTION_NONE);
+    network_observer->OnNetworkChanged(NetworkChangeNotifier::CONNECTION_NONE);
   }
-  base::RunLoop().RunUntilIdle();
 }
 
 bool BackgroundSyncBrowserTest::OneShotPending(const std::string& tag) {