Remove pointer from PermissionPromptAndroid to InfoBar

chrome/browser/permissions/permission_prompt_android.cc

Index: chrome/browser/permissions/permission_prompt_android.cc
diff --git a/chrome/browser/permissions/permission_prompt_android.cc b/chrome/browser/permissions/permission_prompt_android.cc
index 607b369d1ffb12e17b167edae6c5859d124d7b15..1df3c5b4f7dcebef2533b0ea6f07a88d51225b3e 100644
--- a/chrome/browser/permissions/permission_prompt_android.cc
+++ b/chrome/browser/permissions/permission_prompt_android.cc
@@ -6,26 +6,16 @@
 
 #include "base/memory/ptr_util.h"
 #include "chrome/browser/infobars/infobar_service.h"
-#include "chrome/browser/media/webrtc/media_stream_devices_controller.h"
 #include "chrome/browser/permissions/grouped_permission_infobar_delegate_android.h"
 #include "chrome/browser/permissions/permission_request.h"
-#include "chrome/browser/permissions/permission_request_id.h"
-#include "components/infobars/core/infobar.h"
-#include "content/public/browser/web_contents.h"
 
 PermissionPromptAndroid::PermissionPromptAndroid(
     content::WebContents* web_contents)
-    : web_contents_(web_contents), delegate_(nullptr), infobar_(nullptr) {
+    : web_contents_(web_contents), delegate_(nullptr) {
   DCHECK(web_contents);
 }
 
-PermissionPromptAndroid::~PermissionPromptAndroid() {
-  if (infobar_) {
-    GroupedPermissionInfoBarDelegate* infobar_delegate =
-        static_cast<GroupedPermissionInfoBarDelegate*>(infobar_->delegate());
-    infobar_delegate->PermissionPromptDestroyed();
-  }
-}
+PermissionPromptAndroid::~PermissionPromptAndroid() {}
 
 void PermissionPromptAndroid::SetDelegate(Delegate* delegate) {
   delegate_ = delegate;
@@ -40,8 +30,8 @@ void PermissionPromptAndroid::Show(
     return;
 
   requests_ = requests;
-  infobar_ = GroupedPermissionInfoBarDelegate::Create(this, infobar_service,
-                                                      requests[0]->GetOrigin());
+  GroupedPermissionInfoBarDelegate::Create(this, infobar_service,
+                                           requests[0]->GetOrigin());
 }
 
 bool PermissionPromptAndroid::CanAcceptRequestUpdate() {
@@ -49,16 +39,16 @@ bool PermissionPromptAndroid::CanAcceptRequestUpdate() {
 }
 
 void PermissionPromptAndroid::Hide() {
-  InfoBarService* infobar_service =
-      InfoBarService::FromWebContents(web_contents_);
-  if (infobar_ && infobar_service) {
-    infobar_service->RemoveInfoBar(infobar_);
-  }
-  infobar_ = nullptr;
+  // No-op. Hide() is called in three cases:
+  // - After the user resolves a prompt. The infobar manager will hide the
+  // prompt for us.
+  // - DidFinishNavigation, etc. The infobar manager handles this.
+  // - PermissionManager::CancelRequest(). As CanAcceptRequestUpdate() returns
+  // false, it doesn't call into here and instead dummies out the callbacks.

[benwells, 2017/03/20 03:00:43]

This feels brittle - e.g. someone might add a new call to Hide and things won't
work.

Can you think of a better solution that doesn't rely on callers of Hide all
being unnecessary for infobars?

[Timothy Loh, 2017/03/20 04:00:41]

If we don't remove this pointer, then we'd need to add more logic to correctly
maintain the state of the pointer (of note is we'd need to add an
InfoBarManager::Observer so we can know when it's removed due to navigation).
And we'd probably want to add a flag to Hide() since it's weird for the
PermissionRequestManager to also try and remove infobars when the InfoBarManager
should really be in charge of it.

It seems to me that it's simpler to remove this pointer than to try and make it
actually hide the infobar for future potential usages (YAGNI).

 }
 
 bool PermissionPromptAndroid::IsVisible() {
-  return infobar_ != nullptr;
+  return !requests_.empty();
 }
 
 void PermissionPromptAndroid::UpdateAnchorPosition() {