permissions: allow delegate callbacks during dialog close

Right now, PermissionRequestManager requires that the PermissionPrompt
not call any of the PermissionPrompt::Delegate methods if the
PermissionPrompt is being destroyed by the PermissionRequestManager
itself. This mainly happens when the user switches tabs, in which case
the PRM wants to destroy the bubble but not treat the permission request
as handled yet.

This CL changes the contract of the PermissionPrompt: now the
PermissionPrompt always calls the notification methods on
PermissionPrompt::Delegate, and it is up to the PRM to ignore these
callbacks if they are not relevant. This centralizes knowledge of this
PRM behavior in a single place (PRM itself) and simplifies
PermissionPromptBubbleView.

Bug: 1011446
Change-Id: Ie023ca572848e26f63cb9287ee216dd738f32473
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2039730Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#739394}

chrome/browser/permissions/permission_request_manager.cc

@@ -6,6 +6,7 @@
 
 #include <algorithm>
 
+#include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/containers/circular_deque.h"
@@ -330,6 +331,8 @@ PermissionRequestManager::GetDisplayNameOrOrigin() {
 }
 
 void PermissionRequestManager::Accept() {
+  if (deleting_bubble_)
+    return;
   DCHECK(view_);
   std::vector<permissions::PermissionRequest*>::iterator requests_iter;
   for (requests_iter = requests_.begin(); requests_iter != requests_.end();
@@ -340,6 +343,8 @@ void PermissionRequestManager::Accept() {
 }
 
 void PermissionRequestManager::Deny() {
+  if (deleting_bubble_)
+    return;
   DCHECK(view_);
 
   // Suppress any further prompts in this WebContents, from any origin, until
@@ -365,6 +370,8 @@ void PermissionRequestManager::Deny() {
 }
 
 void PermissionRequestManager::Closing() {
+  if (deleting_bubble_)
+    return;
   DCHECK(view_);
   std::vector<permissions::PermissionRequest*>::iterator requests_iter;
   for (requests_iter = requests_.begin();
@@ -465,7 +472,10 @@ void PermissionRequestManager::ShowBubble() {
 
 void PermissionRequestManager::DeleteBubble() {
   DCHECK(view_);
-  view_.reset();
+  {
+    base::AutoReset<bool> deleting(&deleting_bubble_, true);
+    view_.reset();
+  }
   NotifyBubbleRemoved();
 }
 

chrome/browser/permissions/permission_request_manager.h

@@ -247,6 +247,11 @@ class PermissionRequestManager
   // |notification_permission_ui_selector_| or we are using the normal UI.
   base::Optional<QuietUiReason> current_request_quiet_ui_reason_;
 
+  // Whether the bubble is being destroyed by this class, rather than in
+  // response to a UI event. In this case, callbacks from the bubble itself
+  // should be ignored.
+  bool deleting_bubble_ = false;
+
   base::WeakPtrFactory<PermissionRequestManager> weak_factory_{this};
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 };

chrome/browser/ui/views/permission_bubble/permission_prompt_bubble_view.cc

@@ -46,6 +46,13 @@ PermissionPromptBubbleView::PermissionPromptBubbleView(
       ui::DIALOG_BUTTON_CANCEL, l10n_util::GetStringUTF16(IDS_PERMISSION_DENY));
   set_close_on_deactivate(false);
 
+  DialogDelegate::set_accept_callback(base::BindOnce(
+      &PermissionPrompt::Delegate::Accept, base::Unretained(delegate)));
+  DialogDelegate::set_cancel_callback(base::BindOnce(
+      &PermissionPrompt::Delegate::Deny, base::Unretained(delegate)));
+  DialogDelegate::set_close_callback(base::BindOnce(
+      &PermissionPrompt::Delegate::Closing, base::Unretained(delegate)));
+
   SetLayoutManager(std::make_unique<views::BoxLayout>(
       views::BoxLayout::Orientation::kVertical, gfx::Insets(),
       ChromeLayoutProvider::Get()->GetDistanceMetric(
@@ -99,11 +106,6 @@ void PermissionPromptBubbleView::UpdateAnchorPosition() {
   SetArrow(configuration.bubble_arrow);
 }
 
-void PermissionPromptBubbleView::CloseWithoutNotifyingDelegate() {
-  notify_delegate_ = false;
-  GetWidget()->CloseWithReason(views::Widget::ClosedReason::kUnspecified);
-}
-
 void PermissionPromptBubbleView::AddedToWidget() {
   if (name_or_origin_.is_origin) {
     // There is a risk of URL spoofing from origins that are too wide to fit in
@@ -122,24 +124,6 @@ base::string16 PermissionPromptBubbleView::GetWindowTitle() const {
                                     name_or_origin_.name_or_origin);
 }
 
-bool PermissionPromptBubbleView::Cancel() {
-  if (notify_delegate_)
-    delegate_->Deny();
-  return true;
-}
-
-bool PermissionPromptBubbleView::Accept() {
-  if (notify_delegate_)
-    delegate_->Accept();
-  return true;
-}
-
-bool PermissionPromptBubbleView::Close() {
-  if (notify_delegate_)
-    delegate_->Closing();
-  return true;
-}
-
 void PermissionPromptBubbleView::Show() {
   DCHECK(browser_->window());
 

chrome/browser/ui/views/permission_bubble/permission_prompt_bubble_view.h

@@ -22,20 +22,10 @@ class PermissionPromptBubbleView : public views::BubbleDialogDelegateView {
   // bubble_anchor_util::GetPageInfoAnchorConfiguration.
   void UpdateAnchorPosition();
 
-  // Closes the bubble without notifying |delegate_|. Called when the
-  // controlling PermissionPrompt is removed from the permission system, and so
-  // the delegate will have lost the relevant reference. This can happen when
-  // the user changes tabs or initiates a navigation without interacting with
-  // the UI.
-  void CloseWithoutNotifyingDelegate();
-
   // views::BubbleDialogDelegateView:
   void AddedToWidget() override;
   bool ShouldShowCloseButton() const override;
   base::string16 GetWindowTitle() const override;
-  bool Cancel() override;
-  bool Accept() override;
-  bool Close() override;
 
  private:
   void AddPermissionRequestLine(permissions::PermissionRequest* request);
@@ -48,10 +38,6 @@ class PermissionPromptBubbleView : public views::BubbleDialogDelegateView {
   // The requesting domain's name or origin.
   const PermissionPrompt::DisplayNameOrOrigin name_or_origin_;
 
-  // Whether to notify |delegate_| of a decision. Set to false when
-  // CloseWithNotifyingDelegate is called; see the comment on that method.
-  bool notify_delegate_ = true;
-
   DISALLOW_COPY_AND_ASSIGN(PermissionPromptBubbleView);
 };
 

chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc

@@ -47,7 +47,7 @@ PermissionPromptImpl::PermissionPromptImpl(Browser* browser,
 
 PermissionPromptImpl::~PermissionPromptImpl() {
   if (prompt_bubble_)
-    prompt_bubble_->CloseWithoutNotifyingDelegate();
+    prompt_bubble_->GetWidget()->Close();
 
   if (showing_quiet_prompt_) {
     // Hides the quiet prompt.