diff options
author | lalitm <lalitm@google.com> | 2015-09-16 10:30:31 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-09-16 17:31:17 +0000 |
commit | 152f7f2870e2745de968e9dc21b1ee34099b967c (patch) | |
tree | 7c967ab3dc3224659fffe2fd8e2a66074015e6ca | |
parent | 592f75b3055eae9e909d6993d2b1977977aeabbc (diff) | |
download | chromium_src-152f7f2870e2745de968e9dc21b1ee34099b967c.zip chromium_src-152f7f2870e2745de968e9dc21b1ee34099b967c.tar.gz chromium_src-152f7f2870e2745de968e9dc21b1ee34099b967c.tar.bz2 |
permissions: switch from explicitly passing queue controller to callbacks
Explicitly passing a raw QueueController object is extremely bad design
as it leads to inflexibility plus questions about lifetimes and ownership
of the pointer.
Moreover, with the new multiple permissions work, queue controller is
disappearing so using a callback means its replacement can be dropped
in without any modifications to the delegates.
This CL depends on https://codereview.chromium.org/1343553003/
This CL is also a part of a group of CLs:
(1) https://codereview.chromium.org/1332293002 (this)
(2) https://codereview.chromium.org/1337903002
(3) https://codereview.chromium.org/1332063003
BUG=516626
Review URL: https://codereview.chromium.org/1332293002
Cr-Commit-Position: refs/heads/master@{#349151}
13 files changed, 120 insertions, 131 deletions
diff --git a/chrome/browser/geolocation/geolocation_infobar_delegate.cc b/chrome/browser/geolocation/geolocation_infobar_delegate.cc index f727ebc..497b87f 100644 --- a/chrome/browser/geolocation/geolocation_infobar_delegate.cc +++ b/chrome/browser/geolocation/geolocation_infobar_delegate.cc @@ -12,26 +12,24 @@ #include "grit/theme_resources.h" #include "ui/base/l10n/l10n_util.h" - // static infobars::InfoBar* GeolocationInfoBarDelegate::Create( InfoBarService* infobar_service, - PermissionQueueController* controller, - const PermissionRequestID& id, const GURL& requesting_frame, - const std::string& display_languages) { + const std::string& display_languages, + const PermissionSetCallback& callback) { return infobar_service->AddInfoBar(infobar_service->CreateConfirmInfoBar( scoped_ptr<ConfirmInfoBarDelegate>(new GeolocationInfoBarDelegate( - controller, id, requesting_frame, display_languages)))); + requesting_frame, display_languages, callback)))); } GeolocationInfoBarDelegate::GeolocationInfoBarDelegate( - PermissionQueueController* controller, - const PermissionRequestID& id, const GURL& requesting_frame, - const std::string& display_languages) - : PermissionInfobarDelegate(controller, id, requesting_frame, - CONTENT_SETTINGS_TYPE_GEOLOCATION), + const std::string& display_languages, + const PermissionSetCallback& callback) + : PermissionInfobarDelegate(requesting_frame, + CONTENT_SETTINGS_TYPE_GEOLOCATION, + callback), requesting_frame_(requesting_frame), display_languages_(display_languages) { } diff --git a/chrome/browser/geolocation/geolocation_infobar_delegate.h b/chrome/browser/geolocation/geolocation_infobar_delegate.h index fb4a7da..1afa546 100644 --- a/chrome/browser/geolocation/geolocation_infobar_delegate.h +++ b/chrome/browser/geolocation/geolocation_infobar_delegate.h @@ -9,25 +9,24 @@ #include "chrome/browser/permissions/permission_infobar_delegate.h" - // GeolocationInfoBarDelegates are created by the -// GeolocationInfoBarQueueController to control the display +// PermissionQueueController to control the display // and handling of geolocation permission infobars to the user. class GeolocationInfoBarDelegate : public PermissionInfobarDelegate { public: // Creates a geolocation infobar and delegate and adds the infobar to // |infobar_service|. Returns the infobar if it was successfully added. - static infobars::InfoBar* Create(InfoBarService* infobar_service, - PermissionQueueController* controller, - const PermissionRequestID& id, - const GURL& requesting_frame, - const std::string& display_languages); + static infobars::InfoBar* Create( + InfoBarService* infobar_service, + const GURL& requesting_frame, + const std::string& display_languages, + const PermissionSetCallback& callback); private: - GeolocationInfoBarDelegate(PermissionQueueController* controller, - const PermissionRequestID& id, - const GURL& requesting_frame, - const std::string& display_languages); + GeolocationInfoBarDelegate( + const GURL& requesting_frame, + const std::string& display_languages, + const PermissionSetCallback& callback); ~GeolocationInfoBarDelegate() override; // PermissionInfoBarDelegate: diff --git a/chrome/browser/media/midi_permission_infobar_delegate.cc b/chrome/browser/media/midi_permission_infobar_delegate.cc index 264f392..ee8000e 100644 --- a/chrome/browser/media/midi_permission_infobar_delegate.cc +++ b/chrome/browser/media/midi_permission_infobar_delegate.cc @@ -14,23 +14,21 @@ // static infobars::InfoBar* MidiPermissionInfoBarDelegate::Create( InfoBarService* infobar_service, - PermissionQueueController* controller, - const PermissionRequestID& id, const GURL& requesting_frame, const std::string& display_languages, - ContentSettingsType type) { + ContentSettingsType type, + const PermissionSetCallback& callback) { return infobar_service->AddInfoBar(infobar_service->CreateConfirmInfoBar( scoped_ptr<ConfirmInfoBarDelegate>(new MidiPermissionInfoBarDelegate( - controller, id, requesting_frame, display_languages, type)))); + requesting_frame, display_languages, type, callback)))); } MidiPermissionInfoBarDelegate::MidiPermissionInfoBarDelegate( - PermissionQueueController* controller, - const PermissionRequestID& id, const GURL& requesting_frame, const std::string& display_languages, - ContentSettingsType type) - : PermissionInfobarDelegate(controller, id, requesting_frame, type), + ContentSettingsType type, + const PermissionSetCallback& callback) + : PermissionInfobarDelegate(requesting_frame, type, callback), requesting_frame_(requesting_frame), display_languages_(display_languages) { } diff --git a/chrome/browser/media/midi_permission_infobar_delegate.h b/chrome/browser/media/midi_permission_infobar_delegate.h index fcf82da..5818f2d 100644 --- a/chrome/browser/media/midi_permission_infobar_delegate.h +++ b/chrome/browser/media/midi_permission_infobar_delegate.h @@ -10,7 +10,6 @@ #include "components/content_settings/core/common/content_settings_types.h" class GURL; -class PermissionQueueController; class InfoBarService; // MidiPermissionInfoBarDelegates are created by the @@ -20,19 +19,19 @@ class MidiPermissionInfoBarDelegate : public PermissionInfobarDelegate { public: // Creates a MIDI permission infobar and delegate and adds the infobar to // |infobar_service|. Returns the infobar if it was successfully added. - static infobars::InfoBar* Create(InfoBarService* infobar_service, - PermissionQueueController* controller, - const PermissionRequestID& id, - const GURL& requesting_frame, - const std::string& display_languages, - ContentSettingsType type); + static infobars::InfoBar* Create( + InfoBarService* infobar_service, + const GURL& requesting_frame, + const std::string& display_languages, + ContentSettingsType type, + const PermissionSetCallback& callback); private: - MidiPermissionInfoBarDelegate(PermissionQueueController* controller, - const PermissionRequestID& id, - const GURL& requesting_frame, - const std::string& display_languages, - ContentSettingsType type); + MidiPermissionInfoBarDelegate( + const GURL& requesting_frame, + const std::string& display_languages, + ContentSettingsType type, + const PermissionSetCallback& callback); ~MidiPermissionInfoBarDelegate() override; // ConfirmInfoBarDelegate: diff --git a/chrome/browser/media/protected_media_identifier_infobar_delegate.cc b/chrome/browser/media/protected_media_identifier_infobar_delegate.cc index e23934e..780324a 100644 --- a/chrome/browser/media/protected_media_identifier_infobar_delegate.cc +++ b/chrome/browser/media/protected_media_identifier_infobar_delegate.cc @@ -5,8 +5,6 @@ #include "chrome/browser/media/protected_media_identifier_infobar_delegate.h" #include "chrome/browser/infobars/infobar_service.h" -#include "chrome/browser/permissions/permission_queue_controller.h" -#include "chrome/browser/permissions/permission_request_id.h" #include "chrome/common/url_constants.h" #include "chrome/grit/generated_resources.h" #include "components/infobars/core/infobar.h" @@ -18,25 +16,24 @@ // static infobars::InfoBar* ProtectedMediaIdentifierInfoBarDelegate::Create( InfoBarService* infobar_service, - PermissionQueueController* controller, - const PermissionRequestID& id, const GURL& requesting_frame, - const std::string& display_languages) { + const std::string& display_languages, + const PermissionSetCallback& callback) { return infobar_service->AddInfoBar(infobar_service->CreateConfirmInfoBar( scoped_ptr<ConfirmInfoBarDelegate>( new ProtectedMediaIdentifierInfoBarDelegate( - controller, id, requesting_frame, display_languages)))); + requesting_frame, display_languages, callback)))); } ProtectedMediaIdentifierInfoBarDelegate:: ProtectedMediaIdentifierInfoBarDelegate( - PermissionQueueController* controller, - const PermissionRequestID& id, const GURL& requesting_frame, - const std::string& display_languages) + const std::string& display_languages, + const PermissionSetCallback& callback) : PermissionInfobarDelegate( - controller, id, requesting_frame, - CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER), + requesting_frame, + CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER, + callback), requesting_frame_(requesting_frame), display_languages_(display_languages) { } diff --git a/chrome/browser/media/protected_media_identifier_infobar_delegate.h b/chrome/browser/media/protected_media_identifier_infobar_delegate.h index 7f0f52d..ef8acc0 100644 --- a/chrome/browser/media/protected_media_identifier_infobar_delegate.h +++ b/chrome/browser/media/protected_media_identifier_infobar_delegate.h @@ -9,7 +9,6 @@ #include "chrome/browser/permissions/permission_infobar_delegate.h" -class PermissionQueueController; class InfoBarService; class ProtectedMediaIdentifierInfoBarDelegate @@ -18,17 +17,17 @@ class ProtectedMediaIdentifierInfoBarDelegate // Creates a protected media identifier infobar and delegate and adds the // infobar to |infobar_service|. Returns the infobar if it was successfully // added. - static infobars::InfoBar* Create(InfoBarService* infobar_service, - PermissionQueueController* controller, - const PermissionRequestID& id, - const GURL& requesting_frame, - const std::string& display_languages); + static infobars::InfoBar* Create( + InfoBarService* infobar_service, + const GURL& requesting_frame, + const std::string& display_languages, + const PermissionSetCallback& callback); protected: - ProtectedMediaIdentifierInfoBarDelegate(PermissionQueueController* controller, - const PermissionRequestID& id, - const GURL& requesting_frame, - const std::string& display_languages); + ProtectedMediaIdentifierInfoBarDelegate( + const GURL& requesting_frame, + const std::string& display_languages, + const PermissionSetCallback& callback); ~ProtectedMediaIdentifierInfoBarDelegate() override; private: diff --git a/chrome/browser/notifications/notification_permission_infobar_delegate.cc b/chrome/browser/notifications/notification_permission_infobar_delegate.cc index 01970d2..f5d9873 100644 --- a/chrome/browser/notifications/notification_permission_infobar_delegate.cc +++ b/chrome/browser/notifications/notification_permission_infobar_delegate.cc @@ -15,24 +15,23 @@ // static infobars::InfoBar* NotificationPermissionInfobarDelegate::Create( InfoBarService* infobar_service, - PermissionQueueController* controller, - const PermissionRequestID& id, const GURL& requesting_frame, - const std::string& display_languages) { + const std::string& display_languages, + const base::Callback<void(bool, bool)>& callback) { return infobar_service->AddInfoBar(infobar_service->CreateConfirmInfoBar( scoped_ptr<ConfirmInfoBarDelegate>( - new NotificationPermissionInfobarDelegate(controller, id, - requesting_frame, - display_languages)))); + new NotificationPermissionInfobarDelegate(requesting_frame, + display_languages, + callback)))); } NotificationPermissionInfobarDelegate::NotificationPermissionInfobarDelegate( - PermissionQueueController* controller, - const PermissionRequestID& id, const GURL& requesting_frame, - const std::string& display_languages) - : PermissionInfobarDelegate(controller, id, requesting_frame, - CONTENT_SETTINGS_TYPE_NOTIFICATIONS), + const std::string& display_languages, + const base::Callback<void(bool, bool)>& callback) + : PermissionInfobarDelegate(requesting_frame, + CONTENT_SETTINGS_TYPE_NOTIFICATIONS, + callback), requesting_frame_(requesting_frame), display_languages_(display_languages) {} diff --git a/chrome/browser/notifications/notification_permission_infobar_delegate.h b/chrome/browser/notifications/notification_permission_infobar_delegate.h index 4d70ec0..f541631 100644 --- a/chrome/browser/notifications/notification_permission_infobar_delegate.h +++ b/chrome/browser/notifications/notification_permission_infobar_delegate.h @@ -14,16 +14,16 @@ class NotificationPermissionInfobarDelegate : public PermissionInfobarDelegate { // Creates a Notification permission infobar and delegate and adds the // infobar to |infobar_service|. Returns the infobar if it was successfully // added. - static infobars::InfoBar* Create(InfoBarService* infobar_service, - PermissionQueueController* controller, - const PermissionRequestID& id, - const GURL& requesting_frame, - const std::string& display_languages); + static infobars::InfoBar* Create( + InfoBarService* infobar_service, + const GURL& requesting_frame, + const std::string& display_languages, + const PermissionSetCallback& callback); private: - NotificationPermissionInfobarDelegate(PermissionQueueController* controller, - const PermissionRequestID& id, - const GURL& requesting_frame, - const std::string& display_languages); + NotificationPermissionInfobarDelegate( + const GURL& requesting_frame, + const std::string& display_languages, + const PermissionSetCallback& callback); ~NotificationPermissionInfobarDelegate() override; // PermissionInfoBarDelegate: diff --git a/chrome/browser/permissions/permission_infobar_delegate.cc b/chrome/browser/permissions/permission_infobar_delegate.cc index 418f03c..f91c995 100644 --- a/chrome/browser/permissions/permission_infobar_delegate.cc +++ b/chrome/browser/permissions/permission_infobar_delegate.cc @@ -5,7 +5,6 @@ #include "chrome/browser/permissions/permission_infobar_delegate.h" #include "chrome/browser/permissions/permission_context_uma_util.h" -#include "chrome/browser/permissions/permission_queue_controller.h" #include "chrome/grit/generated_resources.h" #include "components/infobars/core/infobar.h" #include "ui/base/l10n/l10n_util.h" @@ -16,13 +15,13 @@ PermissionInfobarDelegate::~PermissionInfobarDelegate() { } PermissionInfobarDelegate::PermissionInfobarDelegate( - PermissionQueueController* controller, - const PermissionRequestID& id, const GURL& requesting_origin, - ContentSettingsType type) - : controller_(controller), id_(id), requesting_origin_(requesting_origin), + ContentSettingsType type, + const base::Callback<void(bool, bool)>& callback) + : requesting_origin_(requesting_origin), action_taken_(false), - type_(type) { + type_(type), + callback_(callback) { } infobars::InfoBarDelegate::Type @@ -58,9 +57,5 @@ bool PermissionInfobarDelegate::Cancel() { void PermissionInfobarDelegate::SetPermission(bool update_content_setting, bool allowed) { action_taken_ = true; - controller_->OnPermissionSet( - id_, requesting_origin_, - InfoBarService::WebContentsFromInfoBar( - infobar())->GetLastCommittedURL().GetOrigin(), - update_content_setting, allowed); + callback_.Run(update_content_setting, allowed); } diff --git a/chrome/browser/permissions/permission_infobar_delegate.h b/chrome/browser/permissions/permission_infobar_delegate.h index 14a2fe8..35fc49f 100644 --- a/chrome/browser/permissions/permission_infobar_delegate.h +++ b/chrome/browser/permissions/permission_infobar_delegate.h @@ -6,13 +6,11 @@ #define CHROME_BROWSER_PERMISSIONS_PERMISSION_INFOBAR_DELEGATE_H_ #include "chrome/browser/infobars/infobar_service.h" -#include "chrome/browser/permissions/permission_request_id.h" #include "components/content_settings/core/common/content_settings_types.h" #include "components/infobars/core/confirm_infobar_delegate.h" #include "content/public/browser/web_contents.h" class NavigationDetails; -class PermissionQueueController; // Base class for permission infobars, it implements the default behavior // so that the accept/deny buttons grant/deny the relevant permission. @@ -21,13 +19,13 @@ class PermissionQueueController; class PermissionInfobarDelegate : public ConfirmInfoBarDelegate { public: + using PermissionSetCallback = base::Callback<void(bool, bool)>; ContentSettingsType content_setting() const { return type_; } protected: - PermissionInfobarDelegate(PermissionQueueController* controller, - const PermissionRequestID& id, - const GURL& requesting_origin, - ContentSettingsType type); + PermissionInfobarDelegate(const GURL& requesting_origin, + ContentSettingsType type, + const PermissionSetCallback& callback); ~PermissionInfobarDelegate() override; private: @@ -41,11 +39,10 @@ class PermissionInfobarDelegate : public ConfirmInfoBarDelegate { void SetPermission(bool update_content_setting, bool allowed); - PermissionQueueController* controller_; // not owned by us - const PermissionRequestID id_; GURL requesting_origin_; bool action_taken_; ContentSettingsType type_; + const PermissionSetCallback callback_; DISALLOW_COPY_AND_ASSIGN(PermissionInfobarDelegate); }; diff --git a/chrome/browser/permissions/permission_queue_controller.cc b/chrome/browser/permissions/permission_queue_controller.cc index 9d4b0d1..269e7b2 100644 --- a/chrome/browser/permissions/permission_queue_controller.cc +++ b/chrome/browser/permissions/permission_queue_controller.cc @@ -12,6 +12,7 @@ #include "chrome/browser/media/midi_permission_infobar_delegate.h" #include "chrome/browser/notifications/notification_permission_infobar_delegate.h" #include "chrome/browser/permissions/permission_context_uma_util.h" +#include "chrome/browser/permissions/permission_request_id.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/storage/durable_storage_permission_infobar_delegate.h" #include "chrome/browser/tab_contents/tab_util.h" @@ -114,35 +115,45 @@ void PermissionQueueController::PendingInfobarRequest::RunCallback( void PermissionQueueController::PendingInfobarRequest::CreateInfoBar( PermissionQueueController* controller, const std::string& display_languages) { + // Controller can be Unretained because the lifetime of the infobar + // is tied to that of the queue controller. Before QueueController + // is destroyed, all requests will be cancelled and so all delegates + // will be destroyed. + PermissionInfobarDelegate::PermissionSetCallback callback = + base::Bind(&PermissionQueueController::OnPermissionSet, + base::Unretained(controller), + id_, + requesting_frame_, + embedder_); switch (type_) { case CONTENT_SETTINGS_TYPE_GEOLOCATION: infobar_ = GeolocationInfoBarDelegate::Create( - GetInfoBarService(id_), controller, id_, requesting_frame_, - display_languages); + GetInfoBarService(id_), requesting_frame_, + display_languages, callback); break; #if defined(ENABLE_NOTIFICATIONS) case CONTENT_SETTINGS_TYPE_NOTIFICATIONS: infobar_ = NotificationPermissionInfobarDelegate::Create( - GetInfoBarService(id_), controller, id_, requesting_frame_, - display_languages); + GetInfoBarService(id_), requesting_frame_, + display_languages, callback); break; #endif // ENABLE_NOTIFICATIONS case CONTENT_SETTINGS_TYPE_MIDI_SYSEX: infobar_ = MidiPermissionInfoBarDelegate::Create( - GetInfoBarService(id_), controller, id_, requesting_frame_, - display_languages, type_); + GetInfoBarService(id_), requesting_frame_, + display_languages, type_, callback); break; #if defined(OS_ANDROID) || defined(OS_CHROMEOS) case CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER: infobar_ = ProtectedMediaIdentifierInfoBarDelegate::Create( - GetInfoBarService(id_), controller, id_, requesting_frame_, - display_languages); + GetInfoBarService(id_), requesting_frame_, + display_languages, callback); break; #endif case CONTENT_SETTINGS_TYPE_DURABLE_STORAGE: infobar_ = DurableStoragePermissionInfoBarDelegate::Create( - GetInfoBarService(id_), controller, id_, requesting_frame_, - display_languages, type_); + GetInfoBarService(id_), requesting_frame_, + display_languages, type_, callback); break; default: NOTREACHED(); diff --git a/chrome/browser/storage/durable_storage_permission_infobar_delegate.cc b/chrome/browser/storage/durable_storage_permission_infobar_delegate.cc index 6657a49..1d2eac7 100644 --- a/chrome/browser/storage/durable_storage_permission_infobar_delegate.cc +++ b/chrome/browser/storage/durable_storage_permission_infobar_delegate.cc @@ -16,25 +16,23 @@ // static infobars::InfoBar* DurableStoragePermissionInfoBarDelegate::Create( InfoBarService* infobar_service, - PermissionQueueController* controller, - const PermissionRequestID& id, const GURL& requesting_frame, const std::string& display_languages, - ContentSettingsType type) { + ContentSettingsType type, + const PermissionSetCallback& callback) { return infobar_service->AddInfoBar( infobar_service->CreateConfirmInfoBar(scoped_ptr<ConfirmInfoBarDelegate>( new DurableStoragePermissionInfoBarDelegate( - controller, id, requesting_frame, display_languages, type)))); + requesting_frame, display_languages, type, callback)))); } DurableStoragePermissionInfoBarDelegate:: DurableStoragePermissionInfoBarDelegate( - PermissionQueueController* controller, - const PermissionRequestID& id, const GURL& requesting_frame, const std::string& display_languages, - ContentSettingsType type) - : PermissionInfobarDelegate(controller, id, requesting_frame, type), + ContentSettingsType type, + const PermissionSetCallback& callback) + : PermissionInfobarDelegate(requesting_frame, type, callback), requesting_frame_(requesting_frame), display_languages_(display_languages) { } diff --git a/chrome/browser/storage/durable_storage_permission_infobar_delegate.h b/chrome/browser/storage/durable_storage_permission_infobar_delegate.h index d8f54f3..7d9b98d 100644 --- a/chrome/browser/storage/durable_storage_permission_infobar_delegate.h +++ b/chrome/browser/storage/durable_storage_permission_infobar_delegate.h @@ -10,7 +10,6 @@ #include "components/content_settings/core/common/content_settings_types.h" class GURL; -class PermissionQueueController; class InfoBarService; class DurableStoragePermissionInfoBarDelegate @@ -19,19 +18,19 @@ class DurableStoragePermissionInfoBarDelegate // Creates a DurableStorage permission infobar and delegate and adds the // infobar to // |infobar_service|. Returns the infobar if it was successfully added. - static infobars::InfoBar* Create(InfoBarService* infobar_service, - PermissionQueueController* controller, - const PermissionRequestID& id, - const GURL& requesting_frame, - const std::string& display_languages, - ContentSettingsType type); + static infobars::InfoBar* Create( + InfoBarService* infobar_service, + const GURL& requesting_frame, + const std::string& display_languages, + ContentSettingsType type, + const PermissionSetCallback& callback); private: - DurableStoragePermissionInfoBarDelegate(PermissionQueueController* controller, - const PermissionRequestID& id, - const GURL& requesting_frame, - const std::string& display_languages, - ContentSettingsType type); + DurableStoragePermissionInfoBarDelegate( + const GURL& requesting_frame, + const std::string& display_languages, + ContentSettingsType type, + const PermissionSetCallback& callback); ~DurableStoragePermissionInfoBarDelegate() override = default; base::string16 GetMessageText() const override; |