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

chrome/browser/ui/test/test_browser_dialog.cc

Index: chrome/browser/ui/test/test_browser_dialog.cc
diff --git a/chrome/browser/ui/test/test_browser_dialog.cc b/chrome/browser/ui/test/test_browser_dialog.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d18a3fa85c548d0a17b8e7a55429c3ac78ad3a98
--- /dev/null
+++ b/chrome/browser/ui/test/test_browser_dialog.cc
@@ -0,0 +1,208 @@
+// 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.
+
+#include "chrome/browser/ui/test/test_browser_dialog.h"
+
+#include "base/command_line.h"
+#include "base/message_loop/message_loop.h"
+#include "base/process/launch.h"
+#include "base/strings/string_split.h"
+#include "base/test/launcher/test_launcher.h"
+#include "base/test/test_switches.h"
+#include "chrome/browser/platform_util.h"
+#include "content/public/common/content_switches.h"
+#include "ui/base/test/user_interactive_test_case.h"
+#include "ui/base/ui_base_switches.h"
+#include "ui/compositor/compositor_switches.h"
+#include "ui/views/widget/widget.h"
+#include "ui/views/widget/widget_observer.h"
+
+using views::Widget;

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

Nit: You already explicitly qualify the majority of places below, so just
eliminate this and qualify the rest.

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

Done.

+
+namespace {
+
+// When present on the command line, runs the test indefinitely.
+const char kInteractiveSwitch[] = "interactive";

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

Nit: Prefer constexpr to const for compile-time constants.  (Several places)

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

Done.

+
+// Switch passed to TestBrowserDialog.Invoke indicating which registered dialog
+// to invoke.
+const char kDialogSwitch[] = "dialog";
+
+// Name of the test case generated by the TEST_BROWSER_DIALOG macro.
+const char kInvokeTestCase[] = "InvokeDefault";
+
+std::vector<std::string>& GetCases() {
+  CR_DEFINE_STATIC_LOCAL(std::vector<std::string>, all_cases, ());
+  return all_cases;
+}
+
+// An automatic action for WidgetCloser to post to the RunLoop.
+enum class DialogAction {
+  INTERACTIVE,  // Run interactively.
+  CLOSE_NOW,    // Call Widget::CloseNow().
+  CLOSE,        // Call Widget::Close() and empty the run loop.

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

Nit: I feel like either you should implement these in this CL, or not add them
until you do implement them.

If you do remove them for now, WidgetCloser()'s code should be simplified as
well.

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

Done (added a TODO in the enum class comment)

+  ACCEPT,       // Call DialogClientView::AcceptWindow().
+  CANCEL,       // Call DialogClientView::CancelWindow().
+};
+
+// Helper to break out of the nested run loop that runs a test dialog.
+class WidgetCloser : public views::WidgetObserver {
+ public:
+  WidgetCloser(Widget* widget, DialogAction action)
+      : widget_(widget), weak_ptr_factory_(this) {
+    widget->AddObserver(this);
+    auto action_function = &WidgetCloser::CloseNow;
+    switch (action) {
+      case DialogAction::INTERACTIVE:
+        return;
+      case DialogAction::CLOSE_NOW:
+        break;
+      default:
+        NOTIMPLEMENTED();
+    }
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(action_function, weak_ptr_factory_.GetWeakPtr()));
+  }
+
+  // WidgetObserver:
+  void OnWidgetDestroyed(Widget* widget) override {
+    widget_->RemoveObserver(this);
+    widget_ = nullptr;
+    base::MessageLoop::current()->QuitNow();
+  }
+
+ private:
+  void CloseNow() {
+    if (widget_)
+      widget_->CloseNow();
+  }
+
+  Widget* widget_;
+
+  base::WeakPtrFactory<WidgetCloser> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(WidgetCloser);
+};
+
+}  // namespace
+
+// static
+std::vector<std::string> TestDialogInterface::NameProvider() {
+  return {"Default"};
+}
+
+// static
+int TestDialogInterface::Register(const char* harness,
+                                  NameProviderFunction name_provider) {
+  const std::string separator = ".";

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

Nit: constexpr

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

Sadly std::basic_string invokes malloc for this under most ABIs. This was
largely so I could concatenate two char[]s, but I neatened it up a bit further
(i.e. `prefix = harness + std::string(".")`)

+  std::vector<std::string> names = name_provider();

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

Nit: Can inline below

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

Done.

+  std::vector<std::string>& all_cases = GetCases();
+  for (const std::string& name : names)
+    all_cases.push_back(harness + separator + name);
+  return 1;
+}
+
+// static
+void TestDialogInterface::TestBrowserDialogRun(
+    TestDialogInterface* harness,
+    const std::vector<std::string>& available_cases) {
+  const base::CommandLine& command_line =
+      *base::CommandLine::ForCurrentProcess();
+
+#if defined(OS_MACOSX)
+  // The rest of this method assumes the child dialog is toolkit-views. So, for
+  // Mac, it will only work if --secondary-ui-md is passed. When called via the
+  // process spawned by TestBrowserDialog.Invoke (along with --dialog=), this
+  // will always be the case. However, when the test runner invokes
+  // FooTestHarness.InvokeDefault as part of the test suite it won't be. Skip
+  // the test in that case.
+  if (!command_line.HasSwitch(switches::kExtendMdToSecondaryUi))

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

I'm worried this is going to break in the future.

It seems like really you don't care about --secondary-ui-md, you care about
toolkit=views.  Does --secondary-ui-md result in some of command-line flag being
auto-added which means that directly, which you can test for?  If not, what are
the consequences of just not having this ifdef and letting things break if
people try to run them the wrong way?

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

Since MD dialogs on Mac are only implemented using toolkit-views,
--secondary-ui-md on Mac means that a dialog will be toolkit-views. (There are
no longer any other `toolkit-views` runtime flags for Mac).

Without this ifdef, a Cocoa dialog will appear, which won't be picked up below,
and the test will fail: "Dialog not shown, or not a toolkit-views Dialog". (A
`WidgetCloser` that works for native Cocoa dialogs is feasible, but may have a
limited lifespan).

I see one concern - if `--secondary-ui-md` effectively becomes default on Mac
but the switch isn't removed at the same time, then this would bail out
needlessly and lose out on some test coverage.

I've changed this to instead just force IsSecondaryUiMaterial() to report `true`
on Mac (i.e. change the current process). This needed a tweak to
MaterialDesignControllerTestAPI. There's an added bonus: the trybots on Mac will
start getting test coverage of the `InvokeDefault` case immediately, like other
platforms.

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

And to be clear, this is a problem because:
* The test will run on the bots automatically, which don't pass this flag, so
just breaking in this case is Not OK
* You're trying to test Harmony UI outside the whole-shebang "Mac TOOLKIT_VIEWS"
build, so you can't #ifdef the test to bail when that's not set
I think that's probably a slightly better tradeoff.  All the solutions here have
downsides.

[tapted, 2016/12/16 04:21:53]

Yes (I considered defining a DISABLED_ test to spawn in the subprocess with
--also_run_disabled_tests, but that seemed like an easy/lazy way out. We're
already lacking some coverage of the UI code of various dialogs, so this will
help us get some more).
Yeah - we'll be launching Harmony/MD dialogs on Mac without any extra #defines.
TOOLKIT_VIEWS is already set on Mac in Release, and it allows dialogs to be
created using toolkit-views (e.g. all the Harmony dialogs).

There's a separate mac_views_browser .gn flag set that switches the entire
browser over to views as well. It works pretty well, but needs a lot more work.
It has a bot (http://go/macviewsbuilder), but it's not part of the CQ or
waterfall (and at this stage is mainly for catching compile regressions).

+    return;
+#endif
+
+  int dialog_index = 0;
+  std::string dialog_name = command_line.GetSwitchValueASCII(kDialogSwitch);
+  if (!dialog_name.empty()) {
+    std::vector<std::string> name_parts = base::SplitString(
+        dialog_name, ".", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);

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

I think instead of using SplitString, you should find() the first '.' in the
string, and return the substring either before or after it.  This will allow the
dialog name strings to contain '.'s if people want.

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

Done.

+    ASSERT_EQ(2u, name_parts.size());
+    const std::string& case_name = name_parts[1];

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

Nit: There are two blocks in this file that want to SplitString() the dialog
name, verify it has two parts, and then return one.  I suggest a helper
function.

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

Done.

+    auto it =
+        std::find(available_cases.begin(), available_cases.end(), case_name);
+    ASSERT_NE(it, available_cases.end());
+    dialog_index = std::distance(available_cases.begin(), it);
+  }
+
+  gfx::NativeView parent =
+      platform_util::GetViewForWindow(harness->DialogParent());
+  views::Widget::Widgets widgets_before;
+  views::Widget::GetAllChildWidgets(parent, &widgets_before);
+
+  harness->ShowDialog(dialog_index);
+  views::Widget::Widgets widgets_after;
+  views::Widget::GetAllChildWidgets(parent, &widgets_after);
+
+  EXPECT_GT(widgets_after.size(), widgets_before.size())
+      << "Dialog not shown, or not a toolkit-views Dialog.";
+  EXPECT_LE(widgets_after.size(), widgets_before.size() + 1)
+      << "More than one child dialog shown.";

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

Nit: It doesn't seem like the error messages add much.  I'd just remove these
and change the ASSERT below to ASSERT_EQ(1, added.size());.

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

Done.

+
+  auto added = base::STLSetDifference<std::vector<views::Widget*>>(
+      widgets_after, widgets_before);
+  ASSERT_FALSE(added.empty());
+  DialogAction action = DialogAction::CLOSE_NOW;
+  if (command_line.HasSwitch(kInteractiveSwitch))
+    action = DialogAction::INTERACTIVE;

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

Nit: Not shorter, but makes it clearer this is just a data type conversion, not
a control flow change:

  const DialogAction action = command_line.HasSwitch(kInteractiveSwitch)
                            ? DialogAction::INTERACTIVE
                            : DialogAction::CLOSE_NOW;

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

Done.

+
+  WidgetCloser closer(added[0], action);
+  ::test::RunTestInteractively();
+}
+
+// Adds a browser_test entry point into the dialog testing framework. Without a
+// --dialog specified, just lists the available dialogs.
+TEST(TestBrowserDialog, Invoke) {
+  const base::CommandLine& invoker = *base::CommandLine::ForCurrentProcess();
+  const std::string dialog_name = invoker.GetSwitchValueASCII(kDialogSwitch);
+
+  const std::vector<std::string>& all_cases = GetCases();
+  ASSERT_FALSE(all_cases.empty());
+
+  if (dialog_name.empty()) {
+    VLOG(0) << "\nPass one of the following after --" << kDialogSwitch << "=";
+    for (const std::string& name : GetCases())
+      std::cout << "\t" << name << "\n";

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

Did you mean to VLOG() this?  Or did you mean to cout the VLOG() above?

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

Ah, that's a better idea :).

I wanted only the first line of output "decorated" with line numbers and things,
but I can make an ostringstream and VLOG after. (Done.)

+  }
+
+  if (dialog_name.empty())
+    return;  // Nothing specified.

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

Nit: Put this in the above conditional.

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

Done.

+
+  auto it = std::find(all_cases.begin(), all_cases.end(), dialog_name);
+  ASSERT_NE(it, all_cases.end()) << "Dialog '" << dialog_name << "' not found.";
+
+  std::vector<std::string> name_parts = base::SplitString(
+      dialog_name, ".", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
+  ASSERT_EQ(2u, name_parts.size());
+  const std::string& harness_name = name_parts[0];
+
+  base::CommandLine command(invoker);
+
+  // Replace TestBrowserDialog.Invoke with |harness_name|.InvokeDefault.
+  command.AppendSwitchASCII(base::kGTestFilterFlag,
+                            harness_name + "." + kInvokeTestCase);
+
+  // Enable MacViews and Harmony by default.
+  command.AppendSwitch(switches::kExtendMdToSecondaryUi);

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

Same worry as earlier, but even more so.  It seems like it's the user's
responsibility to supply this if desirable.

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

Now removed with the change at the earlier comment.

+
+  // Disable timeouts and generate screen output if --interactive was specified.
+  if (command.HasSwitch(kInteractiveSwitch)) {
+    const std::string disable = "10000000";

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

This is an ugly way of disabling these timeouts.  It also doesn't match the
kAlmostInfiniteTimeoutMs value in test_timeouts.cc (too few zeroes).

I would prefer to modify that code to allow 0 or -1 to mean "no timeout at all",
remove the concept of "almost infinite timeout" from there and here, and then
pass that signal value.

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

kAlmostInfiniteTimeoutMs is tricky to remove since it's needed to return a valid
base::TimeDelta(). There's a pasture of yaks to wade through :/

* base::TimeDelta should have `constextpr` constructors
* TestTimeouts's static members can then be base::TimeDelta
* base::debug::BeingDebugged() should result in action timeouts being
base::TimeDelta::Max()
* InitializeTimeout() in test_timeouts.cc needs to beware not to overflow when
multiplying base::TimeDelta values (e.g. for MSAN builds)
* test_timeout.cc should be using int64_t constants

I've gone for a compromise that just adds 

// static
const char TestTimeouts::kNoTimeoutSwitchValue[] = "-1";

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

Feel free to file those yaks as bugs...

[tapted, 2016/12/16 04:21:53]

Filed http://crbug.com/674764 (which you've seen :).

+    command.AppendSwitchASCII(switches::kUiTestActionMaxTimeout, disable);
+    command.AppendSwitchASCII(switches::kTestLauncherTimeout, disable);
+    command.AppendSwitch(switches::kEnablePixelOutputInTests);
+  }
+
+  base::LaunchOptions options;
+  options.wait = true;
+  base::LaunchProcess(command, options);
+}