[Extensions cleanup] Remove UpdatedExtensionPermissionsInfo struct

Previously, NotifyExtensionPermissionsUpdated used the
NotificationService which could only receive a single object. Thus we
had a struct to pass the information. This is not the case anymore, so
we can just pass all three of the params to the observer methods.

This also renames and updates the "reason" struct.

No functionality changed.

Bug: 1343623
Change-Id: I68224dfcaa2e314e64e8110a58dd6252342b2cc8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3813965
Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
Commit-Queue: Emilia Paz <emiliapaz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1033636}

chrome/browser/background/background_application_list_model.cc

@@ -48,7 +48,6 @@ using extensions::ExtensionRegistry;
 using extensions::ExtensionSet;
 using extensions::PermissionSet;
 using extensions::UnloadedExtensionReason;
-using extensions::UpdatedExtensionPermissionsInfo;
 using extensions::mojom::APIPermissionID;
 
 class ExtensionNameComparator {
@@ -363,22 +362,26 @@ void BackgroundApplicationListModel::OnBackgroundContentsServiceDestroying() {
 }
 
 void BackgroundApplicationListModel::OnExtensionPermissionsUpdated(
-    const extensions::UpdatedExtensionPermissionsInfo& info) {
-  if (info.permissions.HasAPIPermission(APIPermissionID::kBackground) ||
+    const extensions::Extension& extension,
+    const extensions::PermissionSet& permissions,
+    extensions::PermissionsManager::UpdateReason reason) {
+  if (permissions.HasAPIPermission(APIPermissionID::kBackground) ||
       (base::FeatureList::IsEnabled(features::kOnConnectNative) &&
-       info.permissions.HasAPIPermission(
-           APIPermissionID::kTransientBackground))) {
-    switch (info.reason) {
-      case UpdatedExtensionPermissionsInfo::ADDED:
-      case UpdatedExtensionPermissionsInfo::REMOVED:
+       permissions.HasAPIPermission(APIPermissionID::kTransientBackground))) {
+    switch (reason) {
+      case extensions::PermissionsManager::UpdateReason::kAdded:
+      case extensions::PermissionsManager::UpdateReason::kRemoved:
         Update();
-        if (IsBackgroundApp(*info.extension, profile_)) {
-          AssociateApplicationData(info.extension);
+        if (IsBackgroundApp(extension, profile_)) {
+          AssociateApplicationData(&extension);
         } else {
-          DissociateApplicationData(info.extension);
+          DissociateApplicationData(&extension);
         }
         break;
-      default:
+      case extensions::PermissionsManager::UpdateReason::kPolicy:
+        // Policy changes are only used for host permissions, so the
+        // "background"
+        // permission would never be present in  permissions .
         NOTREACHED();
     }
   }

chrome/browser/background/background_application_list_model.h

@@ -31,7 +31,6 @@ class ImageSkia;
 
 namespace extensions {
 class ExtensionRegistry;
-struct UpdatedExtensionPermissionsInfo;
 }  // namespace extensions
 
 // Model for list of Background Applications associated with a Profile (i.e.
@@ -156,7 +155,9 @@ class BackgroundApplicationListModel
 
   // extensions::PermissionsManager::Observer:
   void OnExtensionPermissionsUpdated(
-      const extensions::UpdatedExtensionPermissionsInfo& info) override;
+      const extensions::Extension& extension,
+      const extensions::PermissionSet& permissions,
+      extensions::PermissionsManager::UpdateReason reason) override;
 
   // Intended to be called when extension system is ready.
   void OnExtensionSystemReady();

chrome/browser/extensions/api/developer_private/developer_private_api.cc

@@ -656,9 +656,11 @@ void DeveloperPrivateEventRouter::OnUserPermissionsSettingsChanged(
 }
 
 void DeveloperPrivateEventRouter::OnExtensionPermissionsUpdated(
-    const UpdatedExtensionPermissionsInfo& info) {
+    const Extension& extension,
+    const PermissionSet& permissions,
+    PermissionsManager::UpdateReason reason) {
   BroadcastItemStateChanged(developer::EVENT_TYPE_PERMISSIONS_CHANGED,
-                            info.extension->id());
+                            extension.id());
 }
 
 void DeveloperPrivateEventRouter::OnProfilePrefChanged() {

chrome/browser/extensions/api/developer_private/developer_private_api.h

@@ -143,7 +143,9 @@ class DeveloperPrivateEventRouter : public ExtensionRegistryObserver,
   void OnUserPermissionsSettingsChanged(
       const PermissionsManager::UserPermissionsSettings& settings) override;
   void OnExtensionPermissionsUpdated(
-      const UpdatedExtensionPermissionsInfo& info) override;
+      const Extension& extension,
+      const PermissionSet& permissions,
+      PermissionsManager::UpdateReason reason) override;
 
   // Handles a profile preference change.
   void OnProfilePrefChanged();

chrome/browser/extensions/permissions_updater.cc

@@ -651,29 +651,27 @@ void PermissionsUpdater::NotifyPermissionsUpdated(
     return;
   }
 
-  UpdatedExtensionPermissionsInfo::Reason reason;
+  PermissionsManager::UpdateReason reason;
   events::HistogramValue histogram_value = events::UNKNOWN;
   const char* event_name = NULL;
   Profile* profile = Profile::FromBrowserContext(browser_context);
 
   if (event_type == REMOVED) {
-    reason = UpdatedExtensionPermissionsInfo::REMOVED;
+    reason = PermissionsManager::UpdateReason::kRemoved;
     histogram_value = events::PERMISSIONS_ON_REMOVED;
     event_name = permissions::OnRemoved::kEventName;
   } else if (event_type == ADDED) {
-    reason = UpdatedExtensionPermissionsInfo::ADDED;
+    reason = PermissionsManager::UpdateReason::kAdded;
     histogram_value = events::PERMISSIONS_ON_ADDED;
     event_name = permissions::OnAdded::kEventName;
   } else {
     DCHECK_EQ(POLICY, event_type);
-    reason = UpdatedExtensionPermissionsInfo::POLICY;
+    reason = PermissionsManager::UpdateReason::kPolicy;
   }
 
   // Notify other APIs or interested parties.
-  UpdatedExtensionPermissionsInfo info =
-      UpdatedExtensionPermissionsInfo(extension.get(), *changed, reason);
   PermissionsManager::Get(browser_context)
-      ->NotifyExtensionPermissionsUpdated(info);
+      ->NotifyExtensionPermissionsUpdated(*extension, *changed, reason);
 
   // Send the new permissions to the renderers.
   for (RenderProcessHost::iterator host_iterator(

chrome/browser/extensions/permissions_updater_unittest.cc

@@ -158,10 +158,12 @@ TEST_F(PermissionsUpdaterTest, GrantAndRevokeOptionalPermissions) {
         .GrantOptionalPermissions(*extension, delta, base::DoNothing());
     waiter.WaitForExtensionPermissionsUpdate(base::BindOnce(
         [](scoped_refptr<const Extension> extension, PermissionSet* delta,
-           const UpdatedExtensionPermissionsInfo& info) {
-          ASSERT_EQ(extension.get(), info.extension);
-          ASSERT_EQ(UpdatedExtensionPermissionsInfo::ADDED, info.reason);
-          ASSERT_EQ(*delta, info.permissions);
+           const Extension& actual_extension,
+           const PermissionSet& actual_permissions,
+           PermissionsManager::UpdateReason actual_reason) {
+          ASSERT_EQ(extension.get()->id(), actual_extension.id());
+          ASSERT_EQ(*delta, actual_permissions);
+          ASSERT_EQ(PermissionsManager::UpdateReason::kAdded, actual_reason);
         },
         extension, &delta));
 
@@ -194,10 +196,12 @@ TEST_F(PermissionsUpdaterTest, GrantAndRevokeOptionalPermissions) {
                                    base::DoNothing());
     waiter.WaitForExtensionPermissionsUpdate(base::BindOnce(
         [](scoped_refptr<const Extension> extension, PermissionSet* delta,
-           const UpdatedExtensionPermissionsInfo& info) {
-          ASSERT_EQ(extension.get(), info.extension);
-          ASSERT_EQ(UpdatedExtensionPermissionsInfo::REMOVED, info.reason);
-          ASSERT_EQ(*delta, info.permissions);
+           const Extension& actual_extension,
+           const PermissionSet& actual_permissions,
+           PermissionsManager::UpdateReason actual_reason) {
+          ASSERT_EQ(extension.get()->id(), actual_extension.id());
+          ASSERT_EQ(*delta, actual_permissions);
+          ASSERT_EQ(PermissionsManager::UpdateReason::kRemoved, actual_reason);
         },
         extension, &delta));
 

chrome/browser/ui/toolbar/toolbar_actions_model.cc

@@ -130,10 +130,12 @@ void ToolbarActionsModel::OnExtensionManagementSettingsChanged() {
 }
 
 void ToolbarActionsModel::OnExtensionPermissionsUpdated(
-    const extensions::UpdatedExtensionPermissionsInfo& info) {
-  if (HasAction(info.extension->id())) {
+    const extensions::Extension& extension,
+    const extensions::PermissionSet& permissions,
+    extensions::PermissionsManager::UpdateReason reason) {
+  if (HasAction(extension.id())) {
     for (Observer& observer : observers_)
-      observer.OnToolbarActionUpdated(info.extension->id());
+      observer.OnToolbarActionUpdated(extension.id());
   }
 }
 

chrome/browser/ui/toolbar/toolbar_actions_model.h

@@ -157,7 +157,9 @@ class ToolbarActionsModel : public extensions::ExtensionActionAPI::Observer,
 
   // extensions::PermissionsManager::Observer:
   void OnExtensionPermissionsUpdated(
-      const extensions::UpdatedExtensionPermissionsInfo& info) override;
+      const extensions::Extension& extension,
+      const extensions::PermissionSet& permissions,
+      extensions::PermissionsManager::UpdateReason reason) override;
 
   // KeyedService:
   void Shutdown() override;

extensions/browser/permissions_manager.cc

@@ -185,12 +185,6 @@ KeyedService* PermissionsManagerFactory::BuildServiceInstanceFor(
 
 }  // namespace
 
-UpdatedExtensionPermissionsInfo::UpdatedExtensionPermissionsInfo(
-    const Extension* extension,
-    const PermissionSet& permissions,
-    Reason reason)
-    : reason(reason), extension(extension), permissions(permissions) {}
-
 // Implementation of UserPermissionsSettings.
 PermissionsManager::UserPermissionsSettings::UserPermissionsSettings() =
     default;
@@ -573,9 +567,11 @@ PermissionsManager::GetEffectivePermissionsToGrant(
 }
 
 void PermissionsManager::NotifyExtensionPermissionsUpdated(
-    const UpdatedExtensionPermissionsInfo& info) {
+    const Extension& extension,
+    const PermissionSet& permissions,
+    UpdateReason reason) {
   for (Observer& observer : observers_) {
-    observer.OnExtensionPermissionsUpdated(info);
+    observer.OnExtensionPermissionsUpdated(extension, permissions, reason);
   }
 }
 

extensions/browser/permissions_manager.h

@@ -30,29 +30,6 @@ class ExtensionPrefs;
 class Extension;
 class PermissionSet;
 
-// TODO(crbug.com/1343623): This no longer needs to be a struct.
-struct UpdatedExtensionPermissionsInfo {
-  enum Reason {
-    ADDED,    // The permissions were added to the extension.
-    REMOVED,  // The permissions were removed from the extension.
-    POLICY,   // The policy that affects permissions was updated.
-  };
-
-  Reason reason;
-
-  // The extension whose permissions have changed.
-  raw_ptr<const Extension> extension;
-
-  // The permissions that have changed. For Reason::ADDED, this would contain
-  // only the permissions that have added, and for Reason::REMOVED, this would
-  // only contain the removed permissions.
-  const PermissionSet& permissions;
-
-  UpdatedExtensionPermissionsInfo(const Extension* extension,
-                                  const PermissionSet& permissions,
-                                  Reason reason);
-};
-
 // Class for managing user-scoped extension permissions.
 // Includes blocking all extensions from running on a site and automatically
 // running all extensions on a site.
@@ -108,12 +85,22 @@ class PermissionsManager : public KeyedService {
     kCustomizeByExtension,
   };
 
+  enum class UpdateReason {
+    // Permissions were added to the extension.
+    kAdded,
+    // Permissions were removed from the extension.
+    kRemoved,
+    // Policy that affects permissions was updated.
+    kPolicy,
+  };
+
   class Observer {
    public:
     virtual void OnUserPermissionsSettingsChanged(
         const UserPermissionsSettings& settings) {}
-    virtual void OnExtensionPermissionsUpdated(
-        const UpdatedExtensionPermissionsInfo& info) {}
+    virtual void OnExtensionPermissionsUpdated(const Extension& extension,
+                                               const PermissionSet& permissions,
+                                               UpdateReason reason) {}
   };
 
   explicit PermissionsManager(content::BrowserContext* browser_context);
@@ -194,8 +181,9 @@ class PermissionsManager : public KeyedService {
 
   // Notifies `observers_` that the permissions have been updated for an
   // extension.
-  void NotifyExtensionPermissionsUpdated(
-      const UpdatedExtensionPermissionsInfo& info);
+  void NotifyExtensionPermissionsUpdated(const Extension& extension,
+                                         const PermissionSet& permissions,
+                                         UpdateReason reason);
 
   // Adds or removes observers.
   void AddObserver(Observer* observer);

extensions/test/permissions_manager_waiter.cc

@@ -22,7 +22,10 @@ void PermissionsManagerWaiter::WaitForExtensionPermissionsUpdate() {
 }
 
 void PermissionsManagerWaiter::WaitForExtensionPermissionsUpdate(
-    base::OnceCallback<void(const UpdatedExtensionPermissionsInfo&)> callback) {
+    base::OnceCallback<void(const Extension& extension,
+                            const PermissionSet& permissions,
+                            PermissionsManager::UpdateReason reason)>
+        callback) {
   extension_permissions_update_callback_ = std::move(callback);
   WaitForExtensionPermissionsUpdate();
 }
@@ -33,9 +36,12 @@ void PermissionsManagerWaiter::OnUserPermissionsSettingsChanged(
 }
 
 void PermissionsManagerWaiter::OnExtensionPermissionsUpdated(
-    const UpdatedExtensionPermissionsInfo& info) {
+    const Extension& extension,
+    const PermissionSet& permissions,
+    PermissionsManager::UpdateReason reason) {
   if (extension_permissions_update_callback_) {
-    std::move(extension_permissions_update_callback_).Run(info);
+    std::move(extension_permissions_update_callback_)
+        .Run(extension, permissions, reason);
   }
   extension_permissions_update_run_loop_.Quit();
 }

extensions/test/permissions_manager_waiter.h

@@ -13,6 +13,10 @@ namespace extensions {
 
 class PermissionsManagerWaiter : public PermissionsManager::Observer {
  public:
+  using ExtensionPermissionUpdateCallback =
+      base::OnceCallback<void(const Extension& extension,
+                              const PermissionSet& permissions,
+                              PermissionsManager::UpdateReason reason)>;
   explicit PermissionsManagerWaiter(PermissionsManager* manager);
   PermissionsManagerWaiter(const PermissionsManagerWaiter&) = delete;
   const PermissionsManagerWaiter& operator=(const PermissionsManagerWaiter&) =
@@ -22,25 +26,25 @@ class PermissionsManagerWaiter : public PermissionsManager::Observer {
   // Waits until permissions change.
   void WaitForUserPermissionsSettingsChange();
   void WaitForExtensionPermissionsUpdate();
-  // `callback` is called after waiting for an update.
+  // Waits until extension permissions change and then calls `callback`.
   void WaitForExtensionPermissionsUpdate(
-      base::OnceCallback<void(const UpdatedExtensionPermissionsInfo&)>
-          callback);
+      ExtensionPermissionUpdateCallback callback);
 
  private:
   // PermissionsManager::Observer:
   void OnUserPermissionsSettingsChanged(
       const PermissionsManager::UserPermissionsSettings& settings) override;
   void OnExtensionPermissionsUpdated(
-      const UpdatedExtensionPermissionsInfo& info) override;
+      const Extension& extension,
+      const PermissionSet& permissions,
+      PermissionsManager::UpdateReason reason) override;
 
   base::RunLoop user_permissions_settings_changed_run_loop_;
   base::RunLoop extension_permissions_update_run_loop_;
   base::ScopedObservation<extensions::PermissionsManager,
                           extensions::PermissionsManager::Observer>
       manager_observation_{this};
-  base::OnceCallback<void(const UpdatedExtensionPermissionsInfo&)>
-      extension_permissions_update_callback_;
+  ExtensionPermissionUpdateCallback extension_permissions_update_callback_;
 };
 
 }  // namespace extensions