[Mac] Enable MacViews site settings bubble behind --enable-mac-views-dialogs.

chrome/browser/ui/views/browser_dialogs_views_mac.cc

+// Copyright 2015 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/browser_dialogs.h"
+
+#include "chrome/browser/ui/views/website_settings/website_settings_popup_view.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "ui/views/widget/widget_observer.h"
+
+// This file provides definitions of desktop browser dialog-creation methods for
+// Mac where a Cocoa browser is using Views dialogs.
+
+namespace {
+
+// Takes an anchor view for a bubble and deletes it when the bubble is closed.
+// It also closes the bubble when the parent WebContents is destroyed since the
+// anchor view does not have it's own Widget for the BubbleDelegateView to

[tapted, 2015/08/11 01:23:19]

nit: it's -> its

[jackhou1, 2015/08/11 03:09:23]

Done.

+// watch.

[tapted, 2015/08/11 01:23:19]

watch -> observe?

[jackhou1, 2015/08/11 03:09:24]

Done.

+class BubbleAnchorViewDeleter : public views::WidgetObserver,
+                                public content::WebContentsObserver {
+ public:
+  BubbleAnchorViewDeleter(views::View* anchor_view,
+                          views::Widget* widget,
+                          content::WebContents* web_contents)
+      : content::WebContentsObserver(web_contents),
+        anchor_view_(anchor_view),
+        widget_(widget) {
+    widget_->AddObserver(this);
+  }
+
+  ~BubbleAnchorViewDeleter() override {}
+
+  // WidgetObserver:
+  void OnWidgetDestroyed(views::Widget* widget) override {
+    widget_ = nullptr;

[tapted, 2015/08/11 01:23:19]

I think this needs to call `delete this`, and the anchor_view_ be stored in a
scoped_ptr. `delete this` will also stop observing the webcontents, so no need
to set widget_ = null (or even have the data member).

[jackhou1, 2015/08/11 03:09:24]

Done. Still need |widget_| to call Widget::Close().

+    delete anchor_view_;
+  }
+
+  // content::WebContentsObserver:
+  void WebContentsDestroyed() override {

[tapted, 2015/08/11 01:23:19]

Should any navigation do this too? Maybe that's not interesting for bubbles that
always dismiss themselves.

[jackhou1, 2015/08/11 03:09:23]

I think that would take focus away, which closes the bubble. Closing the parent
window doesn't focus it first.

+    if (widget_)
+      widget_->Close();
+  }
+
+ private:
+  views::View* anchor_view_;

[tapted, 2015/08/11 01:23:19]

when this becomes scoped_ptr, needs a note to say that it's never inserted into
a view hierarchy

[jackhou1, 2015/08/11 03:09:24]

Done.

+  views::Widget* widget_;
+
+  DISALLOW_COPY_AND_ASSIGN(BubbleAnchorViewDeleter);
+};
+
+}  // namespace
+
+namespace chrome {
+
+void ShowWebsiteSettingsBubbleViewsAtPoint(const gfx::Point& anchor_point,
+                                           Profile* profile,
+                                           content::WebContents* web_contents,
+                                           const GURL& url,
+                                           const content::SSLStatus& ssl) {
+  // The views bubble requires an anchor views::View. Create one using the
+  // anchor point. Since the view doesn't have a container Widget,
+  // View::ConvertPointToScreen will assume it's origin is 0,0. Create a rect

[tapted, 2015/08/11 01:23:19]

it's -> its

[jackhou1, 2015/08/11 03:09:23]

Done.

+  // where the midpoint of the bottom edge is at the anchor point.
+  gfx::Rect anchor_view_rect(0, 0, anchor_point.x() * 2, anchor_point.y());
+  // WebsiteSettingsPopupView assumes a margin of
+  // WebsiteSettingsPopupView::kLocationIconVerticalMargin.

[tapted, 2015/08/11 01:23:19]

This needs to be pulled out into a header file somewhere. (and it seems possible
the LocationIconVerticalMargin in website_settings_popup_view.cc should really
be referring to a constant used when creating the omnibox rahter than being
redefined in the anonymous namespace therE)

[jackhou1, 2015/08/11 03:09:24]

The margin is built into the assets. kLocationIconVerticalMargin is the only
definition I could find. zoom_button_view.cc hard codes it. So I've just make
kLocationIconVerticalMargin public.

+  anchor_view_rect.Inset(0, 0, 0, -5);
+
+  views::View* anchor_view = new views::View();

[tapted, 2015/08/11 01:23:19]

I think it would be nicer to do this directly into a scoped_ptr owned by
BubbleAnchorViewDeleter.

Maybe a better name then is BubbleAnchorViewAdapter. (i.e. this function becomes
merely

// BubbleAnchorViewAdapter deletes itself.
new BubbleAnchorViewAdapater(anchor_point, profile, web_contents, url, ssl);

[jackhou1, 2015/08/11 03:09:24]

Done.

+  anchor_view->SetBoundsRect(anchor_view_rect);

[tapted, 2015/08/11 01:23:19]

Maybe just

SetBounds(0, 0, anchor_point.x() * 2, anchor_point.y() +
WebsiteSettingsPopupView::kLocationIconVerticalMargin);

[jackhou1, 2015/08/11 03:09:24]

Done.

+  views::BubbleDelegateView* bubble_view = WebsiteSettingsPopupView::ShowPopup(
+      anchor_view, profile, web_contents, url, ssl);
+  new BubbleAnchorViewDeleter(anchor_view, bubble_view->GetWidget(),

[tapted, 2015/08/11 01:23:19]

(I think this is leaked now, but `delete this` should fix it..)

[jackhou1, 2015/08/11 03:09:23]

Done.

+                              web_contents);
+}
+
+}  // namespace chrome