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

chrome/browser/ui/test/test_browser_dialog.cc

+// 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 "base/test/test_timeouts.h"
+#include "chrome/browser/platform_util.h"
+#include "content/public/common/content_switches.h"
+#include "ui/base/material_design/material_design_controller.h"
+#include "ui/base/test/material_design_controller_test_api.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"
+
+namespace {
+
+// When present on the command line, runs the test indefinitely.
+constexpr char kInteractiveSwitch[] = "interactive";
+
+// Switch passed to TestBrowserDialog.Invoke indicating which registered dialog
+// to invoke.
+constexpr char kDialogSwitch[] = "dialog";
+
+// Name of the test case generated by the TEST_BROWSER_DIALOG macro.
+constexpr 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.
+// TODO(tapted): Explore asynchronous Widget::Close() and DialogClientView::
+// {Accept,Cancel}Window() approaches to test other dialog lifetimes.
+enum class DialogAction {
+  INTERACTIVE,  // Run interactively.
+  CLOSE_NOW,    // Call Widget::CloseNow().
+};
+
+// Helper to break out of the nested run loop that runs a test dialog.
+class WidgetCloser : public views::WidgetObserver {
+ public:
+  WidgetCloser(views::Widget* widget, DialogAction action)
+      : widget_(widget), weak_ptr_factory_(this) {
+    widget->AddObserver(this);
+    if (action == DialogAction::INTERACTIVE)
+      return;
+
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&WidgetCloser::CloseNow, weak_ptr_factory_.GetWeakPtr()));
+  }
+
+  // WidgetObserver:
+  void OnWidgetDestroyed(views::Widget* widget) override {
+    widget_->RemoveObserver(this);
+    widget_ = nullptr;
+    base::MessageLoop::current()->QuitNow();
+  }
+
+ private:
+  void CloseNow() {
+    if (widget_)
+      widget_->CloseNow();
+  }
+
+  views::Widget* widget_;
+
+  base::WeakPtrFactory<WidgetCloser> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(WidgetCloser);
+};
+
+// Splits a --dialog= switch argument into |harness_name| and |test_case_name|.
+void SplitDialogDescription(const std::string& argument,
+                            std::string* harness_name,
+                            std::string* test_case_name) {

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

Nit: I don't know whether it's better or worse than this approach, but another
way would be to declare like:

std::string SplitArgument(const std::string& argument, bool harness);

And return one name or the other.  I admit that the use of bool here is not
wonderful, but since this is basically a file-scope helper, so what.

This would be about the same code within the function, but would be slightly
nicer to call.

Up to you.

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

Yeah I considered this too - flip-flopped a bit. I guess it felt kinda like
"coincidence" that the two callers only wanted one or the other part. A
bool/enum argument kinda hides the fact that it's a split+discard. But also
encapsulation is good too, so my feelings are mixed. Kept it the same (because
inertia).

+  std::string::size_type dot = argument.find('.');
+  if (harness_name)
+    *harness_name = argument.substr(0, dot);
+  if (test_case_name && dot != std::string::npos)
+    *test_case_name = argument.substr(dot + 1);
+}
+
+}  // namespace
+
+// static
+std::vector<std::string> TestDialogInterface::NameProvider() {
+  return {"Default"};
+}
+
+// static
+int TestDialogInterface::Register(const char* harness,
+                                  NameProviderFunction name_provider) {
+  const std::string prefix = harness + std::string(".");
+  std::vector<std::string>& all_cases = GetCases();
+  for (const std::string& name : name_provider())
+    all_cases.push_back(prefix + 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. Without this, a
+  // Cocoa dialog will be created, which TestDialogInterface doesn't support.
+  // Force SecondaryUiMaterial() on Mac to get coverage on the bots. Leave it
+  // optional elsewhere so that the non-MD dialog can be invoked to compare.
+  ui::test::MaterialDesignControllerTestAPI md_test_api(
+      ui::MaterialDesignController::GetMode());
+  md_test_api.SetSecondaryUiMaterial(true);
+#endif
+
+  int dialog_index = 0;
+  std::string dialog_name = command_line.GetSwitchValueASCII(kDialogSwitch);
+  if (!dialog_name.empty()) {
+    std::string case_name;
+    SplitDialogDescription(dialog_name, nullptr, &case_name);
+    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);
+
+  auto added = base::STLSetDifference<std::vector<views::Widget*>>(
+      widgets_after, widgets_before);
+
+  // This can fail if no dialog was shown, if the dialog shown wasn't a toolkit-
+  // views dialog, or if more than one child dialog was shown.
+  ASSERT_EQ(1u, added.size());
+
+  const DialogAction action = command_line.HasSwitch(kInteractiveSwitch)
+                                  ? DialogAction::INTERACTIVE
+                                  : DialogAction::CLOSE_NOW;
+
+  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()) {
+    std::ostringstream case_list;
+    for (const std::string& name : GetCases())
+      case_list << "\t" << name << "\n";

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

Nit: Unless I overlooked something, you have no need for ostringstream, just do:

    std::string case_list;
    for (const std::string& name : GetCases())
       case_list += '\t' + name + '\n';
    VLOG(0) << ... << case_list;

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

Done.

+    VLOG(0) << "\nPass one of the following after --" << kDialogSwitch << "=\n"
+            << case_list.str();
+    return;  // Nothing provided (just list options).

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

Nit: I found this comment more confusing than helpful.

If you think this is really unclear, I would replace this comment with a comment
above the whole conditional saying "if no dialog was specified, print a usage
message and exit" or similar.

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

Done (just added "and exits" to the function-level comment)

+  }
+
+  auto it = std::find(all_cases.begin(), all_cases.end(), dialog_name);
+  ASSERT_NE(it, all_cases.end()) << "Dialog '" << dialog_name << "' not found.";
+
+  std::string harness_name;
+  SplitDialogDescription(dialog_name, &harness_name, nullptr);
+
+  base::CommandLine command(invoker);
+
+  // Replace TestBrowserDialog.Invoke with |harness_name|.InvokeDefault.
+  command.AppendSwitchASCII(base::kGTestFilterFlag,
+                            harness_name + "." + kInvokeTestCase);
+
+  base::LaunchOptions options;
+
+  // Disable timeouts and generate screen output if --interactive was specified.
+  if (command.HasSwitch(kInteractiveSwitch)) {
+    command.AppendSwitchASCII(switches::kUiTestActionMaxTimeout,
+                              TestTimeouts::kNoTimeoutSwitchValue);
+    command.AppendSwitchASCII(switches::kTestLauncherTimeout,
+                              TestTimeouts::kNoTimeoutSwitchValue);
+    command.AppendSwitch(switches::kEnablePixelOutputInTests);
+  } else {
+    options.wait = true;

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

I realised it was pretty pointless removing the timeouts on the subprocess if
they were still in effect on the parent. Leaving LaunchOptions::wait as false
means the subprocess outputs goes to an interactive shell, but the browser won't
die unexpectedly while you're trying to take a screenshot.

+  }
+
+  base::LaunchProcess(command, options);
+}