[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 32 matching lines @@
   virtual void Cancelled() OVERRIDE;
   virtual void RequestFinished() OVERRIDE;
 
  private:
   ChromeMidiPermissionContext* context_;
   const PermissionRequestID id_;
   GURL requesting_frame_;
   bool user_gesture_;
   std::string display_languages_;
   const content::BrowserContext::MidiSysExPermissionCallback& callback_;
+  bool is_finished_;
 
   DISALLOW_COPY_AND_ASSIGN(MidiPermissionRequest);
 };
 
 MidiPermissionRequest::MidiPermissionRequest(
     ChromeMidiPermissionContext* context,
     const PermissionRequestID& id,
     const GURL& requesting_frame,
     bool user_gesture,
     const std::string& display_languages,
     const content::BrowserContext::MidiSysExPermissionCallback& callback)
     : context_(context),
       id_(id),
       requesting_frame_(requesting_frame),
       user_gesture_(user_gesture),
       display_languages_(display_languages),
-      callback_(callback) {}
+      callback_(callback),
+      is_finished_(false) {}
 
-MidiPermissionRequest::~MidiPermissionRequest() {}
+MidiPermissionRequest::~MidiPermissionRequest() {
+  DCHECK(is_finished_);
+}
 
 int MidiPermissionRequest::GetIconID() const {
   return IDR_ALLOWED_MIDI_SYSEX;
 }
 
 base::string16 MidiPermissionRequest::GetMessageText() const {
   return l10n_util::GetStringFUTF16(
       IDS_MIDI_SYSEX_INFOBAR_QUESTION,
       net::FormatUrl(requesting_frame_.GetOrigin(), display_languages_));
 }
@@ skipping 11 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/21 20:15:13]

// This is called if the info-bar is dismissed.  Permission is neither granted
nor denied and we hope JS can deal with that.
?

(I suspect such a hope is poorly-founded; I can easily imagine use-cases where
the JS effectively blocks the app's functionality until the permission is
granted/denied and it would be good to give the JS a hint that that grant is
never coming)

[Greg Billock, 2014/05/21 22:46:42]

Yeah, me too. That's why I put in initially that cancellation would look like
denial. In retrospect, that seemed wrong since the dialog can cancel on tab
close and such. I don't know what the JS looks like for this call, or if there's
a good way to signal back a "request processed but still in limbo" kind of
thing.

[Ami GONE FROM CHROMIUM, 2014/05/21 23:47:34]

Shouldn't you find out before reverting the behavior?

[Greg Billock, 2014/05/22 01:50:00]

This (the change in behavior) is flagged off currently. Before it turns on we
need to coordinate with callers to make sure the termination cases are handled
the way they want. I'm trying to set them all to mimic the infobar to make that
easier.

-  context_->NotifyPermissionSet(id_, requesting_frame_, callback_, false);
 }
 
 void MidiPermissionRequest::RequestFinished() {
-  delete this;
+  is_finished_ = true;
+  // Deletes 'this'.
+  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/21 20:15:13]

Did you miss this comment?
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)?

[Greg Billock, 2014/05/21 22:46:42]

Ah, yes, I missed responding to that. Style guide is "never use linked_ptr", but
all requests are guaranteed to have RequestFinished called, so there's no leak.
(The permission bubble mgr is per-WC and this context is per-profile.)

[Ami GONE FROM CHROMIUM, 2014/05/21 23:47:34]

Where does it say that?
Then can you add the DCHECK?

[Greg Billock, 2014/05/22 01:50:00]

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

But I have no particular style agenda here. I'm not opposed to using linked_ptr
in the container -- it's kind of the sweet spot for it IMO. Mostly just telling
you the story of how we got where we are. :-) I want the code to look how you
want it to look.
sure. that's a good plan regardless of the container type.

[Ami GONE FROM CHROMIUM, 2014/05/22 02:56:33]

This is an unfortunate divergence between chromium style and google3 style; the
latter has embraced much of C++11, including std::unique_ptr.  Unfortunately
chromium has not done so, making that section of the style guide inapplicable to
chromium (there is no std::unique_ptr in C++98, which is effectively what
chromium uses).
https://code.google.com/p/google-styleguide/source/diff?spec=svn112&r=112&for...

Will raise on chromium-dev.

Until we move to C++11 there is no std:: alternative to linked_ptr, so I think
you should use linked_ptr here :)

   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);
+          bool inserted = pending_requests_.insert(
+              make_pair(id.ToString(), request)).second;
+          DCHECK(inserted) << "Duplicate id " << id.ToString();
+          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++) {
+    if (it->second == request) {
+      delete it->second;
+      pending_requests_.erase(it);
+      return;
+    }
+  }
+
+  NOTREACHED() << "Missing request";
+}
+
 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()) {
+    std::map<std::string, MidiPermissionRequest*>::iterator it =
+        pending_requests_.find(id.ToString());
+    MidiPermissionRequest* cancelling =
+        (it != pending_requests_.end()) ? it->second : NULL;
+
+    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);
 }