Add a TestBrowserDialog helper class for testing browser dialogs in a consistent way.

chrome/browser/ui/test/test_browser_dialog.h

+// Copyright 2016 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.
+
+#ifndef CHROME_BROWSER_UI_TEST_TEST_BROWSER_DIALOG_H_
+#define CHROME_BROWSER_UI_TEST_TEST_BROWSER_DIALOG_H_
+
+#include <string>
+#include <vector>
+
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/gfx/native_widget_types.h"
+
+// TEST_BROWSER_DIALOG provides a way to register an InProcessBrowserTest
+// testing harness with a framework that collects the set of all Chrome browser
+// dialogs available, and provides a way to invoke them "interactively", in a
+// consistent way. E.g. This allows screenshots to be generated easily, with the

[Peter Kasting, 2016/12/15 00:28:55]

Nit: Just remove "E.g."

[tapted, 2016/12/15 06:08:22]

Done.

+// same test data, to assist with UI review. It also provides a registry of
+// dialogs so they can be systematically checked for subtle changes and
+// regressions.
+//
+// To use TEST_BROWSER_DIALOG, a test harness inherits from DialogBrowserTest

[Peter Kasting, 2016/12/15 00:28:55]

Nit: inherits -> should inherit

[tapted, 2016/12/15 06:08:22]

Done.

+// rather than InProcessBrowserTest. If the dialog under test has only a single
+// mode of operation, the only other requirement on the test harness is an
+// override:
+//
+// class FooDialogTest : public DialogBrowserTest {
+//  public:
+//   ..
+//   // TestDialogInterface:

[Peter Kasting, 2016/12/15 00:28:56]

Nit: TestDialogInterface -> DialogBrowserTest

[tapted, 2016/12/15 06:08:22]

Done.

+//   void ShowDialog(int index) override {
+//     /* Show dialog attached to browser() and leave it open. */
+//   }
+//   ..
+// };
+//
+// then in the foo_dialog_browsertest.cc, define:
+//
+//   TEST_BROWSER_DIALOG(FooDialogTest);
+//
+// This adds a test case "FooDialogTest.InvokeDefault", and registers with the
+// dialog testing framework. The dialog is shown and immediately closed (the
+// test harness does not need to close it). The "InvokeDefault" test case is run
+// as part of the regular test suite.
+//
+//

[Peter Kasting, 2016/12/15 00:28:55]

Nit: Not clear to me why some places have two blank lines instead of one.

[tapted, 2016/12/15 06:08:22]

Got rid of the double-blank lines. (I think I was using them if a ~<code> block
was notionally at the end of a paragraph rather than inline, but I'm not fussed
and I like consistency :)).

+// To get a list of all available dialogs, run the `TestBrowserDialog.Invoke`
+// test case without other arguments. I.e.
+//
+//   browser_tests --gtest_filter=TestBrowserDialog.Invoke
+//
+//
+// Dialogs listed can be shown interactively using the --dialog argument. E.g.
+//
+//   browser_tests --gtest_filter=TestBrowserDialog.Invoke --interactive \
+//       --dialog=FooDialogTest.Default
+//
+//
+// Here the name "Default" is provided by the default implementation of a
+// NameProvider() static method. A test harness can invoke multiple styles by
+// declaring its own NameProvider that returns a vector of strings. E.g.
+//
+//   // static
+//   std::vector<std::string> FooDialogTest::NameProvider() {
+//     return {"Expired", "Valid"};
+//   }
+//
+// This registers multiple dialogs for `TestBrowserDialog.Invoke`. The |index|
+// given to the ShowDialog() override will correspond to the name provided via
+// the --dialog= switch.
+#define TEST_BROWSER_DIALOG(Harness)                                          \

[Peter Kasting, 2016/12/15 00:28:55]

Nit: Blank line above this so the comment is visually separated from the content

[tapted, 2016/12/15 06:08:22]

Done.

+  int Harness##_register_names =                                              \
+      TestDialogInterface::Register(#Harness, &Harness::NameProvider);        \

[Peter Kasting, 2016/12/15 00:28:56]

This violates the Google style guide, but I don't know another way of
accomplishing what you're trying to do.

Basically, you're modifying global state from a function that's called during
static initialization, which is banned, and which is the only real way for
TestDialogInterface to enumerate all the names without actually instantiating
each relevant harness, which isn't desirable either.

It would be nice to eliminate this, fix the style guide violation, and allow
harnesses to use virtual overrides instead of this weird static override thing. 
But I don't see an obvious way to do it.

[tapted, 2016/12/15 06:08:22]

Yeah, I was aware of this, but it wasn't a casual decision :/. gtest's
TEST[_FOO] macros actually do the same thing. (I hadn't checked earlier, but
assumed this was the mechanism involved. Sure enough, gtest-internal.h does:

// Helper macro for defining tests.
#define GTEST_TEST_(test_case_name, test_name, parent_class, parent_id)\
...
::testing::TestInfo* const GTEST_TEST_CLASS_NAME_(test_case_name, test_name)\
  ::test_info_ =\
    ::testing::internal::MakeAndRegisterTestInfo(\..)

So, I think this could be a specific use-case where it is informally accepted.
E.g., the approach here wouldn't be adding static initializers to any
translation unit that doesn't already have them for their own TEST_F macros.

+  IN_PROC_BROWSER_TEST_F(Harness, InvokeDefault) {                            \
+    TestBrowserDialogRun(this, Harness::NameProvider());                      \
+  }
+
+class TestDialogInterface {
+ public:
+  using NameProviderFunction = std::vector<std::string> (*)();

[Peter Kasting, 2016/12/15 00:28:55]

Nit: Unless I am mistaken, if you declare this type as a function rather than a
function pointer:

  using NameProviderFunction = std::vector<std::string> ();

Then in test files using this, you can declare NameProvider() as:

  NameProviderFunction NameProvider;

This makes it more clear that this function's signature is defined by this type,
and will slightly help with maintenance if for some reason this type changed
later.

You still have to define the function using the full type, I believe, which is a
bit inconvenient.

[tapted, 2016/12/15 06:08:23]

Done.

[Peter Kasting, 2016/12/15 08:12:28]

I realize now this suggestion didn't actually help much since for the overriding
class in this CL, you define the "override" inline in the class declaration, and
I don't think this syntax can be used there:

  NameProviderFunction NameProvider {
    ...
  }

And probably future people will do something similar.

Oh well :/.  I guess this is available in case anyone is like me and likes doing
things out-of-line even in their test code.

+
+  // Provides a single name, "Default". Provide a "static" override of this to
+  // register multiple dialog styles with the dialog testing framework.
+  static std::vector<std::string> NameProvider();
+
+  // Show the dialog corresponding to |index| of the names supplied by
+  // NameProvider().
+  virtual void ShowDialog(int index) = 0;
+
+  // The window that owns the dialogs. Used to find where the dialog appears.
+  virtual gfx::NativeWindow DialogParent() = 0;
+
+  // Called by the static initializer to register names. Only the
+  // TEST_BROWSER_DIALOG macro calls this.
+  static int Register(const char* harness, NameProviderFunction name_provider);
+
+  // Called by the generated test case to invoke the dialog(s).
+  static void TestBrowserDialogRun(
+      TestDialogInterface* harness,
+      const std::vector<std::string>& available_cases);
+};
+
+// Helper to mix in a TestDialogInterface to an existing test harness. |Base|
+// must be a descendant of InProcessBrowserTest.
+template <class Base>
+class SupportsTestDialog : public Base, public TestDialogInterface {
+ public:
+  SupportsTestDialog() {}
+
+  // TestDialogInterface:
+  gfx::NativeWindow DialogParent() override {
+    return this->browser()->window()->GetNativeWindow();

[Peter Kasting, 2016/12/15 00:28:56]

Nit: Can't see why this-> is needed here.  Does it not compile without it?

[tapted, 2016/12/15 06:08:22]

It's needed because browser() is a member of a template argument. Without it,
the compiler ejects "error: use of undeclared identifier 'browser'". An
alternative is

    return Base::browser()->window()->GetNativeWindow();

(Of course, my penchant is for "this->", but I don't feel strongly about it).

[Peter Kasting, 2016/12/15 08:12:28]

Ah, that makes sense.  I figured it was due to templates but I couldn't
immediately think why it was important.

I agree that this-> is better than Base:: in this case, since I at first misread
Base:: as meaning "this function is static".

+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SupportsTestDialog);
+};
+
+using DialogBrowserTest = SupportsTestDialog<InProcessBrowserTest>;
+
+#endif  // CHROME_BROWSER_UI_TEST_TEST_BROWSER_DIALOG_H_