[Media,Geolocation] Add permission bubble cancellation.

chrome/browser/media/chrome_midi_permission_context.cc

 // Copyright 2013 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/media/chrome_midi_permission_context.h"
 
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
 #include "chrome/browser/content_settings/permission_queue_controller.h"
 #include "chrome/browser/content_settings/permission_request_id.h"
@@ skipping 82 matching lines @@
 }
 
 void MidiPermissionRequest::PermissionGranted() {
   context_->NotifyPermissionSet(id_, requesting_frame_, callback_, true);
 }
 
 void MidiPermissionRequest::PermissionDenied() {
   context_->NotifyPermissionSet(id_, requesting_frame_, callback_, false);
 }
 
 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)
     : profile_(profile),
       shutting_down_(false) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 }
 
 ChromeMidiPermissionContext::~ChromeMidiPermissionContext() {
   DCHECK(!permission_queue_controller_);

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

Would you
DCHECK(pending_requests_.empty());
or would you expect that it's possible that there are still pending requests, in
which case my linked_ptr comment is actually a bug-fix (leak)?

   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 }
 
 void ChromeMidiPermissionContext::Shutdown() {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   permission_queue_controller_.reset();
   shutting_down_ = true;
 }
 
 void ChromeMidiPermissionContext::RequestMidiSysExPermission(
@@ skipping 64 matching lines @@
       PermissionDecided(id, requesting_frame, embedder, callback, false);
       break;
     case CONTENT_SETTING_ALLOW:
       PermissionDecided(id, requesting_frame, embedder, callback, true);
       break;
     default:
       if (PermissionBubbleManager::Enabled()) {
         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;
       }
 
       // TODO(gbillock): Delete this and the infobar delegate when
       // we're using only bubbles. crbug.com/337458
       GetQueueController()->CreateInfoBarRequest(
           id, requesting_frame, embedder, std::string(), base::Bind(
               &ChromeMidiPermissionContext::NotifyPermissionSet,
               base::Unretained(this), id, requesting_frame, callback));
@@ skipping 32 matching lines @@
 
 PermissionQueueController* ChromeMidiPermissionContext::GetQueueController() {
   if (!permission_queue_controller_) {
     permission_queue_controller_.reset(
         new PermissionQueueController(profile_,
                                       CONTENT_SETTINGS_TYPE_MIDI_SYSEX));
   }
   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);
 }