[Media,Geolocation] Add permission bubble cancellation.

chrome/browser/media/chrome_midi_permission_context.cc

Index: chrome/browser/media/chrome_midi_permission_context.cc
diff --git a/chrome/browser/media/chrome_midi_permission_context.cc b/chrome/browser/media/chrome_midi_permission_context.cc
index 223609ad9425d865a03c714459a49fe40b143e86..c29f1081e5ac948ab1eadded5b50c17433bf0047 100644
--- a/chrome/browser/media/chrome_midi_permission_context.cc
+++ b/chrome/browser/media/chrome_midi_permission_context.cc
@@ -101,11 +101,11 @@ void MidiPermissionRequest::PermissionDenied() {
 }
 
 void MidiPermissionRequest::Cancelled() {

[Ami GONE FROM CHROMIUM, 2014/05/20 17:09:53]

NOTREACHED?

[Greg Billock, 2014/05/21 19:12:34]

This is invoked if the user cancels the permission bubble. I'm changing this to
match infobar behavior for now instead of treating it as a rejected permission
request, but it's available if you want to do something different.

-  context_->NotifyPermissionSet(id_, requesting_frame_, callback_, false);
 }
 
 void MidiPermissionRequest::RequestFinished() {
-  delete this;
+  // Deletes 'this'.

[Ami GONE FROM CHROMIUM, 2014/05/20 17:09:53]

IWBN to ensure that this is the only way ~MPR() can be called; feel like putting
a 
bool is_finished_;
member on MPR that is ctor'd to false, set to true here, and DCHECK'd in the
dtor?

[Greg Billock, 2014/05/21 19:12:34]

sounds good, done.

On 2014/05/20 17:09:53, Ami Fischman wrote:

+  context_->RequestFinished(this);
 }
 
 ChromeMidiPermissionContext::ChromeMidiPermissionContext(Profile* profile)
@@ -200,10 +200,12 @@ void ChromeMidiPermissionContext::DecidePermission(
         PermissionBubbleManager* bubble_manager =
             PermissionBubbleManager::FromWebContents(web_contents);
         if (bubble_manager) {
-          bubble_manager->AddRequest(new MidiPermissionRequest(
+          MidiPermissionRequest* request = new MidiPermissionRequest(
               this, id, requesting_frame, user_gesture,
               profile_->GetPrefs()->GetString(prefs::kAcceptLanguages),
-              callback));
+              callback);
+          pending_requests_[id.ToString()] = request;

[Ami GONE FROM CHROMIUM, 2014/05/20 17:09:53]

This depends on id.ToString() being unique.  Is that true?  Would you mind
greatly
bool inserted = pending_requests_.insert(make_pair(id.ToString(),
request)).second;
DCHECK(inserted) << "Duplicate id: " << id;
?

[Greg Billock, 2014/05/21 19:12:34]

It would be nice to key the map on PermissionRequestID, but that's not possible.
:-( I toyed with trying to make that work, but I think this ends up being fine.
The reason is that the bridge_id_ of the PRID is unique per-instance and is
included in the string. (And indeed, the operation of the class is predicated on
being able to tell PRIDs apart.) Adding a DCHECK on it seems fine, though.

+          bubble_manager->AddRequest(request);
         }
         return;
       }
@@ -256,11 +258,43 @@ PermissionQueueController* ChromeMidiPermissionContext::GetQueueController() {
   return permission_queue_controller_.get();
 }
 
+void ChromeMidiPermissionContext::RequestFinished(
+    MidiPermissionRequest* request) {
+  std::map<std::string, MidiPermissionRequest*>::iterator it;
+  for (it = pending_requests_.begin(); it != pending_requests_.end(); it++) {

[Ami GONE FROM CHROMIUM, 2014/05/20 17:09:53]

This is a map; why are you iterating??

Put another way: you don't seem to use the fact that the map is keyed by ID.
This might as well be a set of pairs of id & pointer.

If you do want to keep it as a map, then l.280-287 simplify to:
std::map<std::string, MidiPermissionRequest*>::iterator it =
    pending_requests_.find(id.ToString());
MidiPermissionRequest* cancelling = 
    it != pending_requests_.end() ? it->second : NULL;

[Greg Billock, 2014/05/21 19:12:34]

This iteration is searching by value.

The one below could change, though.

On map vs list<pair>, I think a map is more intelligible.

On 2014/05/20 17:09:53, Ami Fischman wrote:

+    if (it->second == request) {
+      delete it->second;
+      pending_requests_.erase(it);
+      return;
+    }
+  }

[Ami GONE FROM CHROMIUM, 2014/05/20 17:09:53]

NOTREACHED() << "Missing request!";
?

[Greg Billock, 2014/05/21 19:12:34]

Yes. That's correct.

+}
+
 void ChromeMidiPermissionContext::CancelPendingInfobarRequest(
     const PermissionRequestID& id) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   if (shutting_down_)
     return;
-  // TODO(gbillock): Add support for cancellation to permission bubbles.
+
+  if (PermissionBubbleManager::Enabled()) {
+    MidiPermissionRequest* cancelling = NULL;
+    std::map<std::string, MidiPermissionRequest*>::iterator it;
+    for (it = pending_requests_.begin(); it != pending_requests_.end(); it++) {
+      if (it->first == id.ToString()) {
+        cancelling = it->second;
+        break;
+      }
+    }
+
+    content::WebContents* web_contents = tab_util::GetWebContentsByID(
+        id.render_process_id(), id.render_view_id());
+    if (cancelling != NULL && web_contents != NULL &&
+        PermissionBubbleManager::FromWebContents(web_contents) != NULL) {
+      PermissionBubbleManager::FromWebContents(web_contents)->
+          CancelRequest(cancelling);
+    }
+    return;
+  }
+
   GetQueueController()->CancelInfoBarRequest(id);
 }