PushMessagingBrowserTest for registration success and failure

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
new file mode 100644
index 0000000000000000000000000000000000000000..00153af09ab94674a7c17b8e0b286eaf246c0de7
--- /dev/null
+++ b/chrome/browser/services/gcm/push_messaging_browsertest.cc
@@ -0,0 +1,123 @@
+// 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 <string>
+
+#include "base/command_line.h"
+#include "chrome/browser/infobars/infobar_service.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/services/gcm/fake_gcm_profile_service.h"
+#include "chrome/browser/services/gcm/gcm_profile_service_factory.h"
+#include "chrome/browser/services/gcm/push_messaging_application_id.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "components/infobars/core/confirm_infobar_delegate.h"
+#include "components/infobars/core/infobar.h"
+#include "components/infobars/core/infobar_manager.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/common/content_switches.h"
+#include "content/public/test/browser_test_utils.h"
+
+namespace {
+class InfoBarObserver : public infobars::InfoBarManager::Observer {
+ public:
+  InfoBarObserver(Browser* browser, bool accept)
+      : infobar_service_(InfoBarService::FromWebContents(
+            browser->tab_strip_model()->GetActiveWebContents())),

[Peter Beverloo, 2014/10/17 14:04:07]

nit: I'd initialize this in the constructor's body since it's a non-trivial
initialization. That's preference, though.

[Michael van Ouwerkerk, 2014/10/17 15:28:01]

I don't mind strongly one way or the other. It's not trivial syntax but in the
end it's just a chain of getters. If there are no strong arguments either way
I'd rather leave it as is for now.

+        accept_(accept) {
+    infobar_service_->AddObserver(this);
+  }
+
+  virtual ~InfoBarObserver() { infobar_service_->RemoveObserver(this); }
+
+  // infobars::InfoBarManager::Observer
+  virtual void OnInfoBarAdded(infobars::InfoBar* infobar) override {
+    ConfirmInfoBarDelegate* delegate =
+        infobar->delegate()->AsConfirmInfoBarDelegate();
+    if (accept_)
+      delegate->Accept();
+    else
+      delegate->Cancel();
+  }
+
+ private:
+  InfoBarService* infobar_service_;
+  bool accept_;
+};
+}  // namespace
+
+class PushMessagingBrowserTest : public InProcessBrowserTest {
+ public:
+  PushMessagingBrowserTest() : gcm_service_(nullptr) {}
+  virtual ~PushMessagingBrowserTest() {}
+
+  // InProcessBrowserTest:

[Peter Beverloo, 2014/10/17 14:04:08]

micro nit: Perhaps group the three "InProcessBrowserTest:" comments into a
single "// InProcessBrowserTest overrides."?

[Michael van Ouwerkerk, 2014/10/17 15:28:01]

I find the current pattern easier to read. If you don't mind, I'd rather leave
it as is.

+  virtual void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(
+        switches::kEnableExperimentalWebPlatformFeatures);
+  }
+
+  // InProcessBrowserTest:
+  virtual void SetUp() override {
+    https_server_.reset(new net::SpawnedTestServer(
+        net::SpawnedTestServer::TYPE_HTTPS,
+        net::BaseTestServer::SSLOptions(
+            net::BaseTestServer::SSLOptions::CERT_OK),
+        base::FilePath(FILE_PATH_LITERAL("chrome/test/data/"))));
+    ASSERT_TRUE(https_server_->Start());
+
+    InProcessBrowserTest::SetUp();
+  }
+
+  // InProcessBrowserTest:
+  virtual void SetUpOnMainThread() override {
+    gcm_service_ = static_cast<gcm::FakeGCMProfileService*>(
+        gcm::GCMProfileServiceFactory::GetInstance()->SetTestingFactoryAndUse(
+            browser()->profile(), &gcm::FakeGCMProfileService::Build));
+    gcm_service_->set_collect(true);
+
+    ui_test_utils::NavigateToURL(
+        browser(), https_server_->GetURL("files/push_messaging/test.html"));
+  }
+
+  void RunScript(const std::string& script, const std::string& expected) {

[Peter Beverloo, 2014/10/17 14:04:08]

I would prefer if we could factor |expected| out of here. RunScript doesn't
sound like something that takes an expected result. Especially since it ASSERTs
instead of EXPECTs.

Perhaps:
  std::string RunScript(const std::string& script);
  std::string RegisterServiceWorker();
  std::string RegisterPush();

  EXPECT_EQ("ok", RegisterServiceWorker());

Alternatively, you could move the ASSERT_EQ() to the
Register{ServiceWorker,Push}AndExpect() instead.

[Michael van Ouwerkerk, 2014/10/17 15:28:01]

Done.

+    std::string result;
+    ASSERT_TRUE(content::ExecuteScriptAndExtractString(
+        browser()->tab_strip_model()->GetActiveWebContents()->GetMainFrame(),
+        script,
+        &result));
+    ASSERT_EQ(expected, result);
+  }
+
+  void RegisterServiceWorkerAndExpect(const std::string& expected) {
+    return RunScript("registerServiceWorker()", expected);
+  }
+
+  void RegisterPushAndExpect(const std::string& expected) {
+    return RunScript("registerPush('1234567890')", expected);

[Peter Beverloo, 2014/10/17 14:04:07]

nit: Will we always pass 1234567890 for tests, or does it make sense to test for
validity of sender Ids? I recall that you wanted to serialize it somewhere, but
it looks like we don't validate it anywhere at all. (Is it even validatable?)

Relatedly, why do we still pass up a senderId through the Blink API? It's even
passed to the browser process if the manifest key is not available. Mind if I
send you two patches to nuke that code path?

[Michael van Ouwerkerk, 2014/10/17 15:28:01]

The sender id is opaque from the browser perspective. We might eventually change
the API to handle multiple senders, but for now this seems like a clean
solution.
 
Sure, we should get rid of the non-manifest path. But as agreed, tests first.

+  }
+
+ protected:
+  scoped_ptr<net::SpawnedTestServer> https_server_;
+  gcm::FakeGCMProfileService* gcm_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(PushMessagingBrowserTest);
+};
+
+IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, RegisterSuccess) {
+  RegisterServiceWorkerAndExpect("ok");
+  InfoBarObserver accepting_observer(browser(), true);
+  RegisterPushAndExpect("https://android.googleapis.com/gcm/send - 1");

[Peter Beverloo, 2014/10/17 14:04:07]

Is there some kind of constant or method we can use to refer to the registration
end point rather than hard-coding it as a string in this test?

[Michael van Ouwerkerk, 2014/10/17 15:28:01]

Done.

+  gcm::PushMessagingApplicationId expected_id(https_server_->GetURL(""), 0L);
+  EXPECT_EQ(gcm_service_->last_registered_app_id(), expected_id.ToString());
+  EXPECT_EQ(gcm_service_->last_registered_sender_ids()[0], "1234567890");
+}
+
+IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, RegisterFailureNoPermission) {
+  RegisterServiceWorkerAndExpect("ok");
+  InfoBarObserver cancelling_observer(browser(), false);
+  RegisterPushAndExpect("AbortError - Registration failed - permission denied");
+}