MacViews: support Views permission bubble

chrome/browser/ui/views/website_settings/permissions_bubble_view.cc

Index: chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
diff --git a/chrome/browser/ui/views/website_settings/permissions_bubble_view.cc b/chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
index 000a4de2e6e36b9dcf5bd1d7154aed5819ee8948..8cf12c6ee78a7fdced4d552bccd134effb590287 100644
--- a/chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
+++ b/chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
@@ -149,6 +149,7 @@ class PermissionsBubbleDialogDelegateView
  public:
   PermissionsBubbleDialogDelegateView(
       views::View* anchor_view,
+      gfx::Point anchor_point,

[tapted, 2016/05/03 12:02:38]

nit: const reference

[Elly Fong-Jones, 2016/05/10 21:21:02]

Done.

       views::BubbleBorder::Arrow anchor_arrow,
       PermissionBubbleViewViews* owner,
       const std::vector<PermissionBubbleRequest*>& requests,
@@ -191,6 +192,7 @@ class PermissionsBubbleDialogDelegateView
 
 PermissionsBubbleDialogDelegateView::PermissionsBubbleDialogDelegateView(
     views::View* anchor_view,
+    gfx::Point anchor_point,
     views::BubbleBorder::Arrow anchor_arrow,
     PermissionBubbleViewViews* owner,
     const std::vector<PermissionBubbleRequest*>& requests,
@@ -200,6 +202,9 @@ PermissionsBubbleDialogDelegateView::PermissionsBubbleDialogDelegateView(
       multiple_requests_(requests.size() > 1) {
   DCHECK(!requests.empty());
 
+  if (!anchor_view)
+    SetAnchorRect(gfx::Rect(anchor_point, gfx::Size()));

[tapted, 2016/05/03 12:02:38]

(see later comment about UpdateAnchor() -- it might be possible to just call
UpdateAnchor() rather than passing in anchor_point to the constructor as well)

[Elly Fong-Jones, 2016/05/10 21:21:02]

It's not safe to call UpdateAnchor() yet at this point, and it seems if I don't
do this here, the view ends up anchored at (0,0). I am not 100% sure why this
is, since UpdateAnchor should get called soon after this constructor runs.

[tapted, 2016/05/11 07:06:18]

Sorry, I meant, instead of 

bubble_delegate_ = new PermissionsBubbleDialogDelegateView(
anchor_delegate_->GetAnchorView(), anchor_delegate_->GetAnchorPoint(),
GetAnchorArrow(), this, requests, values);

we should be able to do 

bubble_delegate_ = new PermissionsBubbleDialogDelegateView(this, requests,
values);
bubble_delegate_->UpdateAnchor(anchor_delegate_->GetAnchorView(),
anchor_delegate_->GetAnchorPoint(), GetAnchorArrow());

+
   set_close_on_deactivate(false);
 
   SetLayoutManager(new views::BoxLayout(views::BoxLayout::kVertical, 0, 0,
@@ -375,23 +380,39 @@ void PermissionsBubbleDialogDelegateView::UpdateAnchor(
 //////////////////////////////////////////////////////////////////////////////
 // PermissionBubbleViewViews
 
-PermissionBubbleViewViews::PermissionBubbleViewViews(Browser* browser)
+PermissionBubbleViewViews::PermissionBubbleViewViews(
+    Browser* browser,
+    std::unique_ptr<AnchorDelegate> anchor_delegate)
     : browser_(browser),
       delegate_(nullptr),
-      bubble_delegate_(nullptr) {
+      bubble_delegate_(nullptr),
+      anchor_delegate_(std::move(anchor_delegate)) {
   DCHECK(browser);
+  DCHECK(browser->window());
 }
 
 PermissionBubbleViewViews::~PermissionBubbleViewViews() {
 }
 
-// static
-std::unique_ptr<PermissionBubbleView> PermissionBubbleView::Create(
-    Browser* browser) {
-  return base::WrapUnique(new PermissionBubbleViewViews(browser));
-}
+// On Mac, there is a separate Cocoa-specific definition of this function.
+#if !defined(OS_MACOSX)

[tapted, 2016/05/03 12:02:39]

This approach won't work with the mac_views_browser build -- there we need to
anchor off a toolkit-views browser still, but on Mac. But once this is in its
own file, this should be easily doable with gyp variables

[Elly Fong-Jones, 2016/05/10 21:21:02]

Done.

I moved this class, and the CreateAnchorDelegate function, *and* the
PermissionBubbleView::Create views variant into a separate file, which I made
"non mac views".

+class ViewAnchorDelegate : public PermissionBubbleViewViews::AnchorDelegate {

[tapted, 2016/05/03 12:02:38]

Perhaps ViewsBrowserAnchorDelegate?

It should have a comment describing it too. Like

// Permits a toolkit-views permissions bubble to be anchored off a toolkit-views
BrowserView.

[Elly Fong-Jones, 2016/05/10 21:21:02]

Done.

+ public:
+  explicit ViewAnchorDelegate(Browser* browser);
+  ~ViewAnchorDelegate() override;
+  views::View* GetAnchorView() override;

[tapted, 2016/05/03 12:02:38]

nit: 
// AnchorDelegate:

[Elly Fong-Jones, 2016/05/10 21:21:02]

Done.

+  gfx::Point GetAnchorPoint() override;
+  gfx::NativeView GetParentView() override;
 
-views::View* PermissionBubbleViewViews::GetAnchorView() {
+ private:
+  Browser* browser_;
+};

[tapted, 2016/05/03 12:02:38]

nit: DISALLOW_COPY_AND_ASSIGN(..)

[Elly Fong-Jones, 2016/05/10 21:21:02]

Done.

+
+ViewAnchorDelegate::ViewAnchorDelegate(Browser* browser) : browser_(browser) {}
+
+ViewAnchorDelegate::~ViewAnchorDelegate() {}
+
+views::View* ViewAnchorDelegate::GetAnchorView() {
   BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser_);
 
   if (browser_->SupportsWindowFeature(Browser::FEATURE_LOCATIONBAR))
@@ -403,6 +424,34 @@ views::View* PermissionBubbleViewViews::GetAnchorView() {
   return browser_view->top_container();
 }
 
+gfx::Point ViewAnchorDelegate::GetAnchorPoint() {
+  return gfx::Point();
+}
+
+gfx::NativeView ViewAnchorDelegate::GetParentView() {
+  views::Widget* widget = views::Widget::GetWidgetForNativeWindow(
+      browser_->window()->GetNativeWindow());
+  return widget->GetNativeView();
+}
+
+// static
+std::unique_ptr<PermissionBubbleView> PermissionBubbleView::Create(
+    Browser* browser) {
+  unique_ptr<ViewAnchorDelegate> anchor_delegate(browser);
+  return base::WrapUnique(
+      new PermissionBubbleViewViews(browser, std::move(anchor_delegate)));
+}
+
+#endif
+
+views::View* PermissionBubbleViewViews::GetAnchorView() {
+  return anchor_delegate_->GetAnchorView();
+}
+
+gfx::Point PermissionBubbleViewViews::GetAnchorPoint() {
+  return anchor_delegate_->GetAnchorPoint();
+}
+
 views::BubbleBorder::Arrow PermissionBubbleViewViews::GetAnchorArrow() {
   if (browser_->SupportsWindowFeature(Browser::FEATURE_LOCATIONBAR))
     return views::BubbleBorder::TOP_LEFT;
@@ -420,12 +469,11 @@ void PermissionBubbleViewViews::Show(
     bubble_delegate_->CloseBubble();
 
   bubble_delegate_ = new PermissionsBubbleDialogDelegateView(
-      GetAnchorView(), GetAnchorArrow(), this, requests, values);
+      GetAnchorView(), GetAnchorPoint(), GetAnchorArrow(), this, requests,
+      values);
 
   // Set |parent_window| because some valid anchors can become hidden.
-  views::Widget* widget = views::Widget::GetWidgetForNativeWindow(
-      browser_->window()->GetNativeWindow());
-  bubble_delegate_->set_parent_window(widget->GetNativeView());
+  bubble_delegate_->set_parent_window(anchor_delegate_->GetParentView());

[tapted, 2016/05/03 12:02:38]

I don't think we need AnchorDelegate::GetParentView(), instead do:

platform_util::GetViewForWindow(browser_->window()->GetNativeWindow());

[Elly Fong-Jones, 2016/05/10 21:21:02]

Done.

 
   views::BubbleDialogDelegateView::CreateBubble(bubble_delegate_)->Show();
   bubble_delegate_->SizeToContents();