Add a browser test for the views external protocol dialog.

chrome/browser/ui/views/external_protocol_dialog_browsertest.cc

+// Copyright 2017 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 "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/external_protocol_dialog_delegate.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/views/external_protocol_dialog.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/browser/render_view_host.h"
+#include "content/public/browser/web_contents.h"
+#include "ui/views/controls/message_box_view.h"
+#include "url/gurl.h"
+
+namespace test {
+
+class ExternalProtocolDialogTestApi {
+ public:
+  explicit ExternalProtocolDialogTestApi(ExternalProtocolDialog* dialog)
+      : dialog_(dialog) {}
+
+  void SetCheckBoxSelected(bool checked) {
+    dialog_->message_box_view_->SetCheckBoxSelected(checked);
+  }
+
+ private:
+  ExternalProtocolDialog* dialog_;
+};

[msw, 2017/01/05 01:32:16]

nit: DISALLOW_COPY_AND_ASSIGN

[dominickn, 2017/01/05 02:00:50]

Done.

+
+}  // namespace test
+
+class TestExternalProtocolDialogDelegate

[msw, 2017/01/05 01:32:16]

nit: add a short comment?

[dominickn, 2017/01/05 02:00:50]

Done.

+    : public ExternalProtocolDialogDelegate {
+ public:
+  TestExternalProtocolDialogDelegate(const GURL& url,
+                                     int render_process_host_id,
+                                     int routing_id,
+                                     bool* called,
+                                     bool* accept,
+                                     bool* dont_block)
+      : ExternalProtocolDialogDelegate(url, render_process_host_id, routing_id),
+        called_(called),
+        accept_(accept),
+        dont_block_(dont_block) {}
+
+  // ExternalProtocolDialogDelegate:
+  void DoAccept(const GURL& url, bool dont_block) const override {

[msw, 2017/01/05 01:32:16]

q: Should these overrides call the base class impls?

[dominickn, 2017/01/05 02:00:50]

It's not really important because this test is designed to ensure that the
dialog class tells the delegate the correct values (i.e. |dont_block|).

Calling the base impl also has the somewhat comical side effect of actually
trying to launch the provided URL if the dialog was Accepted. That means it
opens your default browser and sends it to google.com. (I've changed the URL to
be a telnet:// to ensure it actually is an external one now).

[msw, 2017/01/05 15:57:53]

Acknowledged.

+    *called_ = true;
+    *accept_ = true;
+    *dont_block_ = dont_block;
+  }
+
+  void DoCancel(const GURL& url, bool dont_block) const override {
+    *called_ = true;
+    *accept_ = false;
+    *dont_block_ = dont_block;
+  }
+
+ private:
+  bool* called_;
+  bool* accept_;
+  bool* dont_block_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestExternalProtocolDialogDelegate);
+};
+
+class ExternalProtocolDialogBrowserTest : public InProcessBrowserTest {
+ public:
+  ExternalProtocolDialogBrowserTest() {}
+
+  void ShowDialog() {
+    content::WebContents* web_contents =
+        browser()->tab_strip_model()->GetActiveWebContents();
+    int render_process_host_id = web_contents->GetRenderProcessHost()->GetID();
+    int routing_id = web_contents->GetRenderViewHost()->GetRoutingID();
+    dialog_ = new ExternalProtocolDialog(
+        base::MakeUnique<TestExternalProtocolDialogDelegate>(
+            GURL("https://www.google.com"), render_process_host_id, routing_id,
+            &called_, &accept_, &dont_block_),
+        render_process_host_id, routing_id);
+  }
+
+  void SetChecked(bool checked) {
+    test::ExternalProtocolDialogTestApi(dialog_).SetCheckBoxSelected(checked);
+  }
+
+ protected:
+  ExternalProtocolDialog* dialog_ = nullptr;
+  bool called_ = false;
+  bool accept_ = false;
+  bool dont_block_ = false;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ExternalProtocolDialogBrowserTest);
+};
+
+IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest, TestAccept) {
+  ShowDialog();
+  EXPECT_TRUE(dialog_->Accept());
+  EXPECT_TRUE(called_);
+  EXPECT_TRUE(accept_);
+  EXPECT_FALSE(dont_block_);
+}
+
+IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest,
+                       TestAcceptWithChecked) {
+  ShowDialog();
+  SetChecked(true);
+  EXPECT_TRUE(dialog_->Accept());
+  EXPECT_TRUE(called_);
+  EXPECT_TRUE(accept_);
+  EXPECT_TRUE(dont_block_);
+}
+
+IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest, TestCancel) {

[msw, 2017/01/05 01:32:15]

It seems like we should have an explicit flag for cancel, so it can be
distinguished from the test cases that just close the dialog without accepting
or cancelling it.

[dominickn, 2017/01/05 02:00:50]

Close() is overridden to call DoCancel() (see crrev.com/2559783003); the test
here is that Close() never sets |dont_block| to be true despite the state of the
checkbox.

[msw, 2017/01/05 15:57:53]

Ack. I still suggest testing for |cancel|

+  ShowDialog();
+  EXPECT_TRUE(dialog_->Cancel());
+  EXPECT_TRUE(called_);
+  EXPECT_FALSE(accept_);
+  EXPECT_FALSE(dont_block_);
+}
+
+IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest,
+                       TestCancelWithChecked) {
+  ShowDialog();
+  SetChecked(true);
+  EXPECT_TRUE(dialog_->Cancel());
+  EXPECT_TRUE(called_);
+  EXPECT_FALSE(accept_);
+  EXPECT_TRUE(dont_block_);
+}
+
+IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest, TestClose) {
+  ShowDialog();
+  EXPECT_TRUE(dialog_->Close());
+  EXPECT_TRUE(called_);
+  EXPECT_FALSE(accept_);
+  EXPECT_FALSE(dont_block_);
+}
+
+IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest,
+                       TestCloseWithChecked) {
+  ShowDialog();
+  SetChecked(true);
+  EXPECT_TRUE(dialog_->Close());
+  EXPECT_TRUE(called_);
+  EXPECT_FALSE(accept_);
+  EXPECT_FALSE(dont_block_);
+}