Restored Pre-Harmony Bubble title margin metrics

chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc

Index: chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc
diff --git a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc
index 6baa1e945acc0fdc4910e921f6d1e19a392be9db..f16ef87339828ec93fe7669b8960c15197c81237 100644
--- a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc
+++ b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc
@@ -17,6 +17,7 @@
 #include "chrome/browser/ui/passwords/manage_passwords_test.h"
 #include "chrome/browser/ui/passwords/passwords_model_delegate.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/test/test_browser_dialog.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/passwords/manage_passwords_icon_views.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
@@ -352,3 +353,43 @@ IN_PROC_BROWSER_TEST_F(ManagePasswordsBubbleViewTest, AutoSigninNoFocus) {
   active_waiter.Wait();
   EXPECT_FALSE(IsBubbleShowing());
 }
+
+class ManagePasswordsBubbleDialogViewTest
+    : public SupportsTestDialog<ManagePasswordsBubbleViewTest> {

[tapted, 2017/04/18 05:26:34]

Note this file isn't compiled on Mac (but will be soon - see
https://codereview.chromium.org/2808823002/

And in fact ManagePasswordsBubbleViewTest::view() is not correct for MacViews
(and won't be called by this test, and in fact view() returns a
ManagePasswordsIcon which has "TODO(estade): remove this class from Cocoa as it
has been from Views.")

Basically, I don't think we need to inherit from ManagePasswordsBubbleViewTest
-- ManagePasswordsBubbleViewTest doesn't need to exist at all since ::view() is
never called except on Mac+Cocoa.

That's all a verbose way of saying we're not gaining any code-reuse by putting
this file in manage_passwords_bubble_view_interactive_uitest.cc rather than a
new file like manage_passwords_bubble_view_browsertest.cc which I think is the
correct thing to do.

manage_passwords_bubble_view_browsertest.cc probably should have existed long
ago -- some of the tests above don't need to be interactive_ui_tests - one that
does (the one using ui_test_utils::ClickOnView) is disabled for being flaky and
the tests that are doing things like

// Bubble should not be focused by default.
EXPECT_FALSE(ManagePasswordsBubbleView::manage_password_bubble()->GetFocusManager()->GetFocusedView());

are already broken on desktop linux since focus is asynchronous there (i.e. even
if the bubble _did_ take focus it won't happen until after a delay on Linux).

But that's a lot of Yaks. I think we just need a new file --
manage_passwords_bubble_view_browsertest.cc -- that has


#include "chrome/browser/ui/passwords/manage_passwords_test.h"
#include "chrome/browser/ui/test/test_browser_dialog.h"

class ManagePasswordsBubbleDialogViewTest : public
SupportsTestDialog<ManagePasswordsTest> {

 /* stuff below */



that new file --  manage_passwords_bubble_view_browsertest.cc -- won't run yet
on Mac, but we can move it after https://codereview.chromium.org/2808823002/
lands)

[kylix_rd, 2017/04/18 16:05:16]

I've created a new *_browsertest.cc which only contains the
ManagePasswordsBubbleDialogViewTest for now. We can look at moving whatever
tests are appropriate from the *_interactive_uitest.cc file.

+ public:
+  ManagePasswordsBubbleDialogViewTest() {}
+  ~ManagePasswordsBubbleDialogViewTest() override {}
+
+  void ShowDialog(const std::string& name) override {
+    if (name == "PendingPasswordBubble") {
+      SetupPendingPassword();
+    } else if (name == "AutomaticPasswordBubble") {
+      SetupAutomaticPassword();
+    } else if (name == "ManagePasswordBubble") {
+      SetupManagingPasswords();
+      ExecuteManagePasswordsCommand();
+    } else {
+      ADD_FAILURE() << "Unknown dialog type";
+      return;
+    }
+    EXPECT_TRUE(IsBubbleShowing());

[tapted, 2017/04/18 05:26:34]

I wouldn't bother copying the implementation of this since the machinery for
DialogBrowserTest will fail if a new Widget isn't Show()n, before ShowDialog()
returns.

[kylix_rd, 2017/04/18 16:05:16]

Acknowledged.

+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ManagePasswordsBubbleDialogViewTest);
+};
+
+IN_PROC_BROWSER_TEST_F(ManagePasswordsBubbleDialogViewTest,
+                       InvokeDialog_PendingPasswordBubble) {
+  RunDialog();
+}
+
+IN_PROC_BROWSER_TEST_F(ManagePasswordsBubbleDialogViewTest,
+                       InvokeDialog_AutomaticPasswordBubble) {
+  RunDialog();
+}
+
+IN_PROC_BROWSER_TEST_F(ManagePasswordsBubbleDialogViewTest,
+                       InvokeDialog_ManagePasswordBubble) {
+  RunDialog();
+}