[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

chrome/browser/permissions/permission_request_manager.cc

 // Copyright 2014 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/permissions/permission_request_manager.h"
 
 #include <algorithm>
 
 #include "base/command_line.h"
 #include "base/metrics/user_metrics_action.h"
@@ skipping 205 matching lines @@
     if (request == it->second) {
       it->second->RequestFinished();
       duplicate_requests_.erase(it);
       return;
     }
   }
 
   NOTREACHED();  // Callers should not cancel requests that are not pending.
 }
 
+void PermissionRequestManager::DismissBubble() {

[hcarmona, 2016/10/10 18:08:19]

|Closing| calls |FinalizeBubble| which hides the bubble. Can we use that
instead of creating a new method?

[dominickn, 2016/10/10 22:55:48]

This would work, and I did think of doing that. The main caveat is that
|Closing| is private and an inherited delegate method within
PermissionRequestManager, so BrowserWindowController can't currently access it.
I thought about exposing it - in that case, it would be the only public delegate
method exposed in this way. felt, do you have OWNER thoughts on this?

[hcarmona, 2016/10/11 14:45:33]

Casting to PermissionPrompt::Delegate will let you call |Closing|, this
way you don't need to expose the method.

Maybe having |DismissBubble| just call |Closing| so it's more obvious
that this is supposed to do the same thing?

However, I defer to felt@ on what is best to do here.

+  if (IsBubbleVisible())
+    Closing();
+  HideBubble();
+}
+
 void PermissionRequestManager::HideBubble() {
   // Disengage from the existing view if there is one.
   if (!view_)
     return;
 
   view_->SetDelegate(nullptr);
   view_->Hide();
   view_.reset();
 }
 
@@ skipping 152 matching lines @@
   view_->Show(requests_, accept_states_);
   PermissionUmaUtil::PermissionPromptShown(requests_);
   NotifyBubbleAdded();
 
   // If in testing mode, automatically respond to the bubble that was shown.
   if (auto_response_for_test_ != NONE)
     DoAutoResponseForTesting();
 }
 
 void PermissionRequestManager::FinalizeBubble() {
-  if (view_)
+  if (view_) {
     view_->Hide();
+#if !defined(OS_ANDROID)
+  } else if (web_contents() && !web_contents()->IsBeingDestroyed()) {
+    // Ensure that the view exists. It may have been deleted by HideBubble,

[hcarmona, 2016/10/10 18:08:19]

Good description. However, since your change is fixing this, can we
remove the crbug reference? Whenever I see a link to the bug tracker I
assume it's an active bug.

[dominickn, 2016/10/10 22:55:47]

Done.

+    // creating a race condition where if AddRequest is called before
+    // DisplayPendingRequests, no prompt will be displayed. See
+    // crbug.com/653498.
+    view_ = view_factory_.Run(web_contents());
+    view_->SetDelegate(this);
+#endif
+  }
 
   std::vector<PermissionRequest*>::iterator requests_iter;
   for (requests_iter = requests_.begin();
        requests_iter != requests_.end();
        requests_iter++) {
     RequestFinishedIncludingDuplicates(*requests_iter);
   }
   requests_.clear();
   accept_states_.clear();
   if (queued_requests_.size() || queued_frame_requests_.size())
@@ skipping 104 matching lines @@
     case DENY_ALL:
       Deny();
       break;
     case DISMISS:
       Closing();
       break;
     case NONE:
       NOTREACHED();
   }
 }