MacViews: Allows the toolkit-views Manage Passwords Dialog to be used

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

Index: chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
diff --git a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
index 31b70dac9c8aff19cd40e29dffdb63dfc4cca9b8..5afefd123e054daedf69f80ce0e201bc42fa3b95 100644
--- a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
+++ b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
@@ -11,10 +11,10 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/exclusive_access/fullscreen_controller.h"
 #include "chrome/browser/ui/passwords/password_dialog_prompts.h"
 #include "chrome/browser/ui/passwords/passwords_model_delegate.h"
-#include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/harmony/layout_delegate.h"
 #include "chrome/browser/ui/views/passwords/credentials_item_view.h"
 #include "chrome/browser/ui/views/passwords/credentials_selection_view.h"
@@ -25,6 +25,7 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/base/resource/resource_bundle.h"
+#include "ui/base/ui_features.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/native_theme/native_theme.h"
@@ -40,11 +41,16 @@
 #include "ui/views/layout/layout_constants.h"
 #include "ui/views/widget/widget.h"
 
+#if !defined(OS_MACOSX) || BUILDFLAG(MAC_VIEWS_BROWSER)
+#include "chrome/browser/ui/views/frame/browser_view.h"
+#endif
+
 #if defined(OS_WIN)
 #include "chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.h"
 #endif
 
 int ManagePasswordsBubbleView::auto_signin_toast_timeout_ = 3;
+bool g_bubble_shown_ = false;
 
 // Helpers --------------------------------------------------------------------
 
@@ -199,10 +205,12 @@ class ManagePasswordsBubbleView::AutoSigninView
   // views::ButtonListener:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
 
+#if !defined(OS_MACOSX) || BUILDFLAG(MAC_VIEWS_BROWSER)
   // views::WidgetObserver:
   // Tracks the state of the browser window.
   void OnWidgetActivationChanged(views::Widget* widget, bool active) override;
   void OnWidgetClosing(views::Widget* widget) override;
+#endif
 
   void OnTimer();
   static base::TimeDelta GetTimeout() {
@@ -236,10 +244,16 @@ ManagePasswordsBubbleView::AutoSigninView::AutoSigninView(
   Browser* browser =
       chrome::FindBrowserWithWebContents(parent_->web_contents());
   DCHECK(browser);
+
+// On MacOS the BrowserView should but does not yet exist. Auto-close of the
+// auto-sign-in dialog is not implemented for a Cocoa browser when a bubble is
+// shown in a non-active browser window.

[tapted, 2017/04/12 05:08:42]

Can we say "This matches the current Mac behavior with Cocoa password bubbles."

(i.e. does it match? -- And do we also match on the focus-stealing behavior when
the bubble is shown?)

E.g. if the bubbles never auto-close on Mac, then moving the #endif down below
when the timer starts is probably the right thing to do.

[varkha, 2017/04/12 09:16:43]

I think it is safer to do more of the same that other platforms do. 
http://crbug.com/468281 was more about the bubble getting dismissed so the
solution added IsActive() check and resuming the timer on re-activation. I think
lack of automatic re-activation is OK but removing the check for IsActive()
could mean the bubble is sometimes not shown when it should.

+// See http://crbug.com/468281.
+#if !defined(OS_MACOSX) || BUILDFLAG(MAC_VIEWS_BROWSER)
   BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser);
   observed_browser_.Add(browser_view->GetWidget());
-
-  if (browser_view->IsActive())
+#endif
+  if (browser->window()->IsActive())
     timer_.Start(FROM_HERE, GetTimeout(), this, &AutoSigninView::OnTimer);
 }
 
@@ -248,6 +262,7 @@ void ManagePasswordsBubbleView::AutoSigninView::ButtonPressed(
   NOTREACHED();
 }
 
+#if !defined(OS_MACOSX) || BUILDFLAG(MAC_VIEWS_BROWSER)

[tapted, 2017/04/12 05:08:42]

Although it's dead code, I think it's safer if we leave this stuff compiled in.
E.g. in case someone adds a partial fix - they won't forget to restore these
steps as well.

[varkha, 2017/04/12 09:16:43]

Done.

 void ManagePasswordsBubbleView::AutoSigninView::OnWidgetActivationChanged(
     views::Widget* widget, bool active) {
   if (active && !timer_.IsRunning())
@@ -258,6 +273,7 @@ void ManagePasswordsBubbleView::AutoSigninView::OnWidgetClosing(
     views::Widget* widget) {
   observed_browser_.RemoveAll();
 }
+#endif
 
 void ManagePasswordsBubbleView::AutoSigninView::OnTimer() {
   parent_->model()->OnAutoSignInToastTimeout();
@@ -698,6 +714,7 @@ void ManagePasswordsBubbleView::UpdatePendingView::StyledLabelLinkClicked(
 ManagePasswordsBubbleView* ManagePasswordsBubbleView::manage_passwords_bubble_ =
     NULL;
 
+#if !defined(OS_MACOSX) || BUILDFLAG(MAC_VIEWS_BROWSER)
 // static
 void ManagePasswordsBubbleView::ShowBubble(
     content::WebContents* web_contents,
@@ -720,7 +737,7 @@ void ManagePasswordsBubbleView::ShowBubble(
     }
   }
   manage_passwords_bubble_ = new ManagePasswordsBubbleView(
-      web_contents, anchor_view, reason);
+      web_contents, anchor_view, gfx::Point(), reason);
 
   if (is_fullscreen)
     manage_passwords_bubble_->set_parent_window(web_contents->GetNativeView());
@@ -740,6 +757,7 @@ void ManagePasswordsBubbleView::ShowBubble(
 
   manage_passwords_bubble_->ShowForReason(reason);
 }
+#endif  // !OS_MACOSX || MAC_VIEWS_BROWSER
 
 // static
 void ManagePasswordsBubbleView::CloseCurrentBubble() {
@@ -754,6 +772,11 @@ void ManagePasswordsBubbleView::ActivateBubble() {
   manage_passwords_bubble_->GetWidget()->Activate();
 }
 
+// static
+bool ManagePasswordsBubbleView::IsBubbleShown() {
+  return g_bubble_shown_;
+}
+
 content::WebContents* ManagePasswordsBubbleView::web_contents() const {
   return model_.GetWebContents();
 }
@@ -761,8 +784,12 @@ content::WebContents* ManagePasswordsBubbleView::web_contents() const {
 ManagePasswordsBubbleView::ManagePasswordsBubbleView(
     content::WebContents* web_contents,
     views::View* anchor_view,
+    const gfx::Point& anchor_point,
     DisplayReason reason)
-    : LocationBarBubbleDelegateView(anchor_view, web_contents),
+    : LocationBarBubbleDelegateView(anchor_view,
+                                    anchor_point,
+                                    views::BubbleBorder::TOP_RIGHT,
+                                    web_contents),
       model_(PasswordsModelDelegateFromWebContents(web_contents),
              reason == AUTOMATIC ? ManagePasswordsBubbleModel::AUTOMATIC
                                  : ManagePasswordsBubbleModel::USER_ACTION),
@@ -774,9 +801,11 @@ ManagePasswordsBubbleView::ManagePasswordsBubbleView(
       gfx::Insets(LayoutDelegate::Get()->GetMetric(
                       LayoutDelegate::Metric::PANEL_CONTENT_MARGIN),
                   side_margin, 0, side_margin));
+  g_bubble_shown_ = true;
 }
 
 ManagePasswordsBubbleView::~ManagePasswordsBubbleView() {
+  g_bubble_shown_ = false;
   if (manage_passwords_bubble_ == this)

[tapted, 2017/04/12 05:08:42]

huh - I just noticed this. Can we implement IsBubbleShown() as simply
`!!manage_passwords_bubble_`?

[varkha, 2017/04/12 09:16:43]

Done.

     manage_passwords_bubble_ = NULL;
 }