[Push] Register: require user-visible to be present in the manifest

chrome/browser/services/gcm/push_messaging_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 <map>
 #include <string>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 123 matching lines @@
     gcm_service_ = static_cast<FakeGCMProfileService*>(
         GCMProfileServiceFactory::GetInstance()->SetTestingFactoryAndUse(
             browser()->profile(), &FakeGCMProfileService::Build));
     gcm_service_->set_collect(true);
 
     loadTestPage();
 
     InProcessBrowserTest::SetUpOnMainThread();
   }
 
-  void loadTestPage() {
+  virtual void loadTestPage() {
     ui_test_utils::NavigateToURL(
         browser(), https_server_->GetURL("files/push_messaging/test.html"));
   }
 
   bool RunScript(const std::string& script, std::string* result) {
     return content::ExecuteScriptAndExtractString(
         browser()->tab_strip_model()->GetActiveWebContents()->GetMainFrame(),
         script,
         result);
   }
 
   net::SpawnedTestServer* https_server() const { return https_server_.get(); }
 
   FakeGCMProfileService* gcm_service() const { return gcm_service_; }
 
   PushMessagingServiceImpl* push_service() {
     return static_cast<PushMessagingServiceImpl*>(
         gcm_service_->push_messaging_service());
   }
 
- private:
+ protected:

[Michael van Ouwerkerk, 2014/12/10 18:36:33]

This should still be private. We have public getters for the server and service.

[Miguel Garcia, 2014/12/11 11:37:08]

Done.

   scoped_ptr<net::SpawnedTestServer> https_server_;
   FakeGCMProfileService* gcm_service_;
 
   DISALLOW_COPY_AND_ASSIGN(PushMessagingBrowserTest);
 };
 
-IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, RegisterSuccess) {
+class PushMessagingBadManifestBrowserTest : public PushMessagingBrowserTest {
+  void loadTestPage() override {

[Michael van Ouwerkerk, 2014/12/10 18:36:33]

Instead of subclassing, you could add a loadTestPage method that accepts a url
for the page to load. Just call it at the start of your test method to load a
different page. I don't think it's overly expensive to load an extra page, but
if you do, we can talk about other approaches.

Calling loadTestPage with a specific url at the start of the test also seems
easier to read than using a different test class, I tend to gloss over those.

[Miguel Garcia, 2014/12/11 11:37:08]

mm not sure I agree here, the way this is structured we now have a page per test
which seems sensible and removes a lot of boiler plate code (having to load the
page on every test case.


On 2014/12/10 18:36:33, Michael van Ouwerkerk wrote:

+    ui_test_utils::NavigateToURL(
+        browser(),
+        https_server_->GetURL("files/push_messaging/test_bad_manifest.html"));
+  };
+};
+
+IN_PROC_BROWSER_TEST_F(PushMessagingBadManifestBrowserTest,
+                       RegisterFailsNotVisibleMessages) {
   std::string script_result;
 
   ASSERT_TRUE(RunScript("registerServiceWorker()", &script_result));
+  ASSERT_EQ("ok - service worker registered", script_result);
+  ASSERT_TRUE(RunScript("registerPush()", &script_result));
+  EXPECT_EQ("AbortError - Registration failed - permission denied",
+            script_result);
+}
+
+IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest,
+                       RegisterSuccessNotificationsGranted) {
+  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", script_result);
 
   PushMessagingApplicationId app_id(https_server()->GetURL(""), 0L);
   EXPECT_EQ(app_id.ToString(), gcm_service()->last_registered_app_id());
   EXPECT_EQ("1234567890", gcm_service()->last_registered_sender_ids()[0]);
 }
 
 IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest,
-                       RegisterFailureNoPushPermission) {
+                       RegisterSuccessNotificationsPrompt) {
   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("registerPush()", &script_result));
-  EXPECT_EQ("AbortError - Registration failed - permission denied",
-            script_result);
+  EXPECT_EQ(std::string(kPushMessagingEndpoint) + " - 1", script_result);
+
+  PushMessagingApplicationId app_id(https_server()->GetURL(""), 0L);
+  EXPECT_EQ(app_id.ToString(), gcm_service()->last_registered_app_id());
+  EXPECT_EQ("1234567890", gcm_service()->last_registered_sender_ids()[0]);
 }
 
 IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest,
                        RegisterFailureNotificationsBlocked) {
   std::string script_result;
 
   ASSERT_TRUE(RunScript("registerServiceWorker()", &script_result));
   ASSERT_EQ("ok - service worker registered", script_result);
 
   InfoBarResponder cancelling_responder(browser(), false);
@@ skipping 194 matching lines @@
 
   ASSERT_TRUE(RunScript("registerPush()", &script_result));
   EXPECT_EQ("AbortError - Registration failed - permission denied",
             script_result);
 
   ASSERT_TRUE(RunScript("hasPermission()", &script_result));
   EXPECT_EQ("permission status - denied", script_result);
 }
 
 }  // namespace gcm