Push API: Return cached registration if available.

chrome/browser/services/gcm/push_messaging_browsertest.cc

Index: chrome/browser/services/gcm/push_messaging_browsertest.cc
diff --git a/chrome/browser/services/gcm/push_messaging_browsertest.cc b/chrome/browser/services/gcm/push_messaging_browsertest.cc
index e799e766749d7bf3e497753641809cfc80573367..45a3ae7a0683590cfce0d67b44819c36e468ff37 100644
--- a/chrome/browser/services/gcm/push_messaging_browsertest.cc
+++ b/chrome/browser/services/gcm/push_messaging_browsertest.cc
@@ -143,7 +143,20 @@ class PushMessagingBrowserTest : public InProcessBrowserTest {
 
   void loadTestPage() {
     ui_test_utils::NavigateToURL(
-        browser(), https_server_->GetURL("files/push_messaging/test.html"));
+        browser(),
+        https_server_->GetURL("files/push_messaging/test.html"));
+  }
+
+  void LoadSubscope1TestPage() {

[Michael van Ouwerkerk, 2014/12/11 17:12:53]

These methods repeat a lot of boilerplate and do not get called all that often.
How about instead adding an overload for loadTestPage that accepts a url, and
making loadTestPage call its overload?

[johnme, 2014/12/11 18:32:33]

Done (fixed the capitalization while I was at it ;)

+    ui_test_utils::NavigateToURL(
+        browser(),
+        https_server_->GetURL("files/push_messaging/subscope1/test.html"));
+  }
+
+  void LoadSubscope2TestPage() {
+    ui_test_utils::NavigateToURL(
+        browser(),
+        https_server_->GetURL("files/push_messaging/subscope2/test.html"));
   }
 
   bool RunScript(const std::string& script, std::string* result) {
@@ -153,6 +166,9 @@ class PushMessagingBrowserTest : public InProcessBrowserTest {
         result);
   }
 
+  void TryToRegisterSuccessfully(
+      const std::string& expected_push_registration_id);
+
   net::SpawnedTestServer* https_server() const { return https_server_.get(); }
 
   FakeGCMProfileService* gcm_service() const { return gcm_service_; }
@@ -169,20 +185,27 @@ class PushMessagingBrowserTest : public InProcessBrowserTest {
   DISALLOW_COPY_AND_ASSIGN(PushMessagingBrowserTest);
 };
 
-IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, RegisterSuccess) {
+void PushMessagingBrowserTest::TryToRegisterSuccessfully(
+    const std::string& expected_push_registration_id) {
   std::string script_result;
 
-  ASSERT_TRUE(RunScript("registerServiceWorker()", &script_result));
-  ASSERT_EQ("ok - service worker registered", script_result);
+  EXPECT_TRUE(RunScript("registerServiceWorker()", &script_result));
+  EXPECT_EQ("ok - service worker registered", script_result);
 
   InfoBarResponder accepting_responder(browser(), true);
-  ASSERT_TRUE(RunScript("requestNotificationPermission()", &script_result));
-  ASSERT_EQ("permission status - granted", script_result);
+  EXPECT_TRUE(RunScript("requestNotificationPermission()", &script_result));
+  EXPECT_EQ("permission status - granted", script_result);
 
-  ASSERT_TRUE(RunScript("registerPush()", &script_result));
-  EXPECT_EQ(std::string(kPushMessagingEndpoint) + " - 1-0", script_result);
+  EXPECT_TRUE(RunScript("registerPush()", &script_result));
+  EXPECT_EQ(std::string(kPushMessagingEndpoint) + " - "
+            + expected_push_registration_id, script_result);
+}
 
-  PushMessagingApplicationId app_id(https_server()->GetURL(""), 0L);
+IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, RegisterSuccess) {
+  TryToRegisterSuccessfully("1-0" /* expected_push_registration_id */);
+
+  PushMessagingApplicationId app_id(https_server()->GetURL(""),
+                                    0LL /* service_worker_registration_id */);

[Michael van Ouwerkerk, 2014/12/11 17:12:53]

Is the rest of this file doing something significantly different by using only
one L ? Let's make it all consistent.

[johnme, 2014/12/11 18:32:33]

Done.

   EXPECT_EQ(app_id.ToString(), gcm_service()->last_registered_app_id());
   EXPECT_EQ("1234567890", gcm_service()->last_registered_sender_ids()[0]);
 }
@@ -233,18 +256,66 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, RegisterFailureNoSenderId) {
             script_result);
 }
 
-IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, PushEventSuccess) {
+IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, RegisterPersisted) {
   std::string script_result;
 
+  // An app ID for each Service Worker registration ID we'll use.
+  PushMessagingApplicationId app_id_sw0(https_server()->GetURL(""), 0LL);
+  PushMessagingApplicationId app_id_sw1(https_server()->GetURL(""), 1LL);
+  PushMessagingApplicationId app_id_sw2(https_server()->GetURL(""), 2LL);
+
+  // First, test that Service Worker registration IDs are assigned in order of
+  // registering the Service Workers, and the (fake) push registration ids are
+  // assigned in order of push registration (even when these orders are
+  // different).
+
+  TryToRegisterSuccessfully("1-0" /* expected_push_registration_id */);
+  EXPECT_EQ(app_id_sw0.ToString(), gcm_service()->last_registered_app_id());
+
+  LoadSubscope1TestPage();
   ASSERT_TRUE(RunScript("registerServiceWorker()", &script_result));
   ASSERT_EQ("ok - service worker registered", script_result);
 
-  InfoBarResponder accepting_responder(browser(), true);
-  ASSERT_TRUE(RunScript("requestNotificationPermission();", &script_result));
-  ASSERT_EQ("permission status - granted", script_result);
+  LoadSubscope2TestPage();
+  ASSERT_TRUE(RunScript("registerServiceWorker()", &script_result));
+  ASSERT_EQ("ok - service worker registered", script_result);
 
-  ASSERT_TRUE(RunScript("registerPush()", &script_result));
-  EXPECT_EQ(std::string(kPushMessagingEndpoint) + " - 1-0", script_result);
+  // Note that we need to reload the page after registering, otherwise
+  // navigator.serviceWorker.ready is going to be resolved with the parent
+  // Service Worker which still controls the page.
+  LoadSubscope2TestPage();
+  TryToRegisterSuccessfully("1-1" /* expected_push_registration_id */);
+  EXPECT_EQ(app_id_sw2.ToString(), gcm_service()->last_registered_app_id());
+
+  LoadSubscope1TestPage();
+  TryToRegisterSuccessfully("1-2" /* expected_push_registration_id */);
+  EXPECT_EQ(app_id_sw1.ToString(), gcm_service()->last_registered_app_id());
+
+  // Now test that the Service Worker registration IDs and push registration IDs
+  // generated above were persisted to SW storage, by checking that they are
+  // unchanged despite requesting them in a different order.
+  // TODO(johnme): Ideally we would restart the browser at this point to check
+  // they were persisted to disk, but that's not currently possible since the
+  // test server uses random port numbers for each test (even PRE_Foo and Foo),
+  // so we wouldn't be able to load the test pages with the same origin.
+
+  LoadSubscope1TestPage();
+  TryToRegisterSuccessfully("1-2" /* expected_push_registration_id */);
+  EXPECT_EQ(app_id_sw1.ToString(), gcm_service()->last_registered_app_id());
+
+  LoadSubscope2TestPage();
+  TryToRegisterSuccessfully("1-1" /* expected_push_registration_id */);
+  EXPECT_EQ(app_id_sw1.ToString(), gcm_service()->last_registered_app_id());
+
+  loadTestPage();
+  TryToRegisterSuccessfully("1-0" /* expected_push_registration_id */);
+  EXPECT_EQ(app_id_sw1.ToString(), gcm_service()->last_registered_app_id());
+}
+
+IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, PushEventSuccess) {
+  std::string script_result;
+
+  TryToRegisterSuccessfully("1-0" /* expected_push_registration_id */);
 
   PushMessagingApplicationId app_id(https_server()->GetURL(""), 0L);
   EXPECT_EQ(app_id.ToString(), gcm_service()->last_registered_app_id());
@@ -270,15 +341,7 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, PushEventSuccess) {
 IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, PushEventNoServiceWorker) {
   std::string script_result;
 
-  ASSERT_TRUE(RunScript("registerServiceWorker()", &script_result));
-  ASSERT_EQ("ok - service worker registered", script_result);
-
-  InfoBarResponder accepting_responder(browser(), true);
-  ASSERT_TRUE(RunScript("requestNotificationPermission();", &script_result));
-  ASSERT_EQ("permission status - granted", script_result);
-
-  ASSERT_TRUE(RunScript("registerPush()", &script_result));
-  EXPECT_EQ(std::string(kPushMessagingEndpoint) + " - 1-0", script_result);
+  TryToRegisterSuccessfully("1-0" /* expected_push_registration_id */);
 
   PushMessagingApplicationId app_id(https_server()->GetURL(""), 0L);
   EXPECT_EQ(app_id.ToString(), gcm_service()->last_registered_app_id());
@@ -319,15 +382,7 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, PushEventNoServiceWorker) {
 IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, PushEventNoPermission) {
   std::string script_result;
 
-  ASSERT_TRUE(RunScript("registerServiceWorker()", &script_result));
-  ASSERT_EQ("ok - service worker registered", script_result);
-
-  InfoBarResponder accepting_responder(browser(), true);
-  ASSERT_TRUE(RunScript("requestNotificationPermission();", &script_result));
-  ASSERT_EQ("permission status - granted", script_result);
-
-  ASSERT_TRUE(RunScript("registerPush()", &script_result));
-  EXPECT_EQ(std::string(kPushMessagingEndpoint) + " - 1-0", script_result);
+  TryToRegisterSuccessfully("1-0" /* expected_push_registration_id */);
 
   PushMessagingApplicationId app_id(https_server()->GetURL(""), 0L);
   EXPECT_EQ(app_id.ToString(), gcm_service()->last_registered_app_id());