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

chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc

Index: chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc
diff --git a/chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc b/chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc
index dad2c88da275738acefc4bcc29d449f86bd13496..331d786a05300e90d69087b2500fad60a079b658 100644
--- a/chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc
+++ b/chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc
@@ -14,6 +14,7 @@
 #include "chrome/browser/ui/autofill/create_card_unmask_prompt_view.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/test/test_browser_dialog.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "components/autofill/core/browser/autofill_test_utils.h"
 #include "components/autofill/core/browser/card_unmask_delegate.h"
@@ -22,13 +23,16 @@
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/test_utils.h"
-#include "ui/base/test/user_interactive_test_case.h"
 
 namespace autofill {
 
 namespace {
 
-enum class CreditCardExpiry : uint8_t { EXPIRED, VALID };
+// Forms of the dialog that can be invoked.
+enum class CreditCardExpiry {
+  EXPIRED,  // Shows additional month/year controls.
+  VALID,    // Shows only the CCV Textfield.
+};
 
 class TestCardUnmaskDelegate : public CardUnmaskDelegate {
  public:
@@ -85,12 +89,13 @@ class TestCardUnmaskPromptController : public CardUnmaskPromptControllerImpl {
   DISALLOW_COPY_AND_ASSIGN(TestCardUnmaskPromptController);
 };
 
-class CardUnmaskPromptViewBrowserTest : public InProcessBrowserTest {
+class CardUnmaskPromptViewBrowserTest : public DialogBrowserTest {
  public:
-  CardUnmaskPromptViewBrowserTest() : InProcessBrowserTest() {}
+  CardUnmaskPromptViewBrowserTest() {}
 
   ~CardUnmaskPromptViewBrowserTest() override {}
 
+  // InProcessBrowserTest:

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

Nit: You don't directly subclass InProcessBrowserTest (or TestDialogInterface,
below).  Say "// DialogBrowserTest:"

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

Done.

   void SetUpOnMainThread() override {
     runner_ = new content::MessageLoopRunner;
     contents_ = browser()->tab_strip_model()->GetActiveWebContents();
@@ -98,6 +103,15 @@ class CardUnmaskPromptViewBrowserTest : public InProcessBrowserTest {
     delegate_.reset(new TestCardUnmaskDelegate());
   }
 
+  // TestDialogInterface:
+  void ShowDialog(int index) override {
+    ShowUI(static_cast<CreditCardExpiry>(index));

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

I've been trying to think of ways to simplify the number of functions, types,
casts, etc. here.

What if ShowDialog() was simply given the std::string the user actually passed?

We could keep NameProvider() if we wanted to use it to provide usage messages to
the user and sanity-check their input, or we could eliminate it if those weren't
important.

In this case, ShowDialog() and ShowUI() could be combined into a function that
did something like:

    CreditCard card = (expiry == "expired") ? test::GetMaskedServerCard()
                                            : test::GetMaskedServerCardAmex();

...this would treat all strings other than "expired" as "valid".  If you had
removed input sanity-checking and needed to do it here, you could do something
like:

  void ShowDialog(const std::string& argument) override {
    const bool is_expired = argument == "expired";
    if (!is_expired && argument != "valid")
      return;

    ... use |is_expired|

In either case, callers that today do ShowUI(CreditCardExpiry::EXPIRED); could
do ShowDialog("expired");.  If you want to define a string constant for this
(and then use that constant in NameProvider()?) you could do that as well.

This allows us to combine these two functions, which is a pattern expect would
repeat in other cases we add support for.  It would eliminate the enum here. 
And it would eliminate the need for the caller to map the input string to an
index, meaning NameProvider() could return a set rather than a vector, which
would probably be more well-suited for things like sanity-checking the input is
within the set of valid strings.

These are not large differences, so it's not clear to me that this is wildly
better overall, but the current design feels boilerplate-y.

Another simpler change would be to change CreditCardExpiry from an enum class to
an enum, move the contents of ShowUI() into ShowDialog(), and call ShowDialog()
directly from the various callers.

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

I agree that the mapping in ShowDialog feels a bit clumsy :/. I do like the idea
of passing the string instead, but I think the net result will be more
boilerplate or redundant code in the test harness.

NameProvider() is important because it communicates/registers a list of
"available" dialog types in a central place. That is, one of the problems we
want to solve is that it's hard to be "systematic" when making a UI change --
Chrome has a lot of obscure dialogs that can do creative things (merging
permission requests comes to mind, or the "You are viewing a secure Google
Chrome page." version of the PageInfo bubble). Just registering the test
harness/class name doesn't communicate all the styles that should be checked to
someone unfamiliar with the code.

This doesn't rule out it being a std::set. However, given that we have a list of
strings "somewhere" (i.e. not just in a set of conditions) I wanted to reduce
the duplication of those strings (either as literals, or char[] constants). This
would likely mean ShowDialog(std::string) would do a lookup - e.g. using
NameProvider() itself to get an index, or in a supplementary mapping (**which
didn't need its own static initialization).

So, things might be neater if NameProvider returned a std::map with a known
mapped_type. However, since it's accessed during static initialization, we'd
need to be careful with what gets mapped. I think most client code would use an
`int`. So, providing `int` to ShowDialog just skips this this mapping. However,
it does then require that the mapped ints are contiguous, which isn't always
desirable.

See, e.g., the NameProvider for permissions prompts - _most_ but not all styles
are backed by a value from the content::PermissionType enum
https://codereview.chromium.org/2569703004/diff/40001/chrome/browser/permissi...
. This has both upsides and downsides: the code to make and map NameProvider
gets a little clumsy, but it _does_ force NameProvider to consider all the
different styles that may result from the various content::PermissionType
values.
Done.

+  }
+
+  static std::vector<std::string> NameProvider() {
+    return {"Expired", "Valid"};
+  }
+
   void ShowUI(CreditCardExpiry expired) {
     CardUnmaskPromptView* dialog =
         CreateCardUnmaskPromptView(controller(), contents());
@@ -126,20 +140,7 @@ class CardUnmaskPromptViewBrowserTest : public InProcessBrowserTest {
   DISALLOW_COPY_AND_ASSIGN(CardUnmaskPromptViewBrowserTest);
 };
 
-// Permanently disabled test used to invoke the UI for the card unmask prompt
-// with an expired credit card, which shows additional month/year controls.
-IN_PROC_BROWSER_TEST_F(CardUnmaskPromptViewBrowserTest,
-                       DISABLED_InvokeExpired) {
-  ShowUI(CreditCardExpiry::EXPIRED);
-  ::test::RunTestInteractively();
-}
-
-// Permanently disabled test used to invoke the UI for the card unmask prompt
-// with a valid credit card, which only shows the CCV Textfield.
-IN_PROC_BROWSER_TEST_F(CardUnmaskPromptViewBrowserTest, DISABLED_InvokeValid) {
-  ShowUI(CreditCardExpiry::VALID);
-  ::test::RunTestInteractively();
-}
+TEST_BROWSER_DIALOG(CardUnmaskPromptViewBrowserTest);
 
 IN_PROC_BROWSER_TEST_F(CardUnmaskPromptViewBrowserTest, DisplayUI) {
   ShowUI(CreditCardExpiry::EXPIRED);