[Extensions] Remove NOTIFICATION_EXTENSION_COMMAND_[ADDED|REMOVED]

The NotificationService is deprecated. Remove two notifications from it:
NOTIFICATION_EXTENSION_COMMAND_ADDED and
NOTIFICATION_EXTENSION_COMMAND_REMOVED. These (as the names imply) were
used to notify of when an extension command was added or removed, but
can be substituted with the CommandService::Observer.

This also fixes a bad cast bug.

Bug: 411569
Change-Id: I7b2f0a1cf006a80d62571d20cf8a20968f2d0ac0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2155248Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#761093}

chrome/browser/extensions/api/commands/command_service.cc

@@ -69,14 +69,6 @@ bool IsForCurrentPlatform(const std::string& key) {
                           base::CompareCase::SENSITIVE);
 }
 
-std::string StripCurrentPlatform(const std::string& key) {
-  DCHECK(IsForCurrentPlatform(key));
-  std::string result = key;
-  base::ReplaceFirstSubstringAfterOffset(
-      &result, 0, Command::CommandPlatform() + ":", base::StringPiece());
-  return result;
-}
-
 // Merge |suggested_key_prefs| into the saved preferences for the extension. We
 // merge rather than overwrite to preserve existing was_assigned preferences.
 void MergeSuggestedKeyPrefs(
@@ -115,6 +107,8 @@ CommandService::CommandService(content::BrowserContext* context)
 }
 
 CommandService::~CommandService() {
+  for (auto& observer : observers_)
+    observer.OnCommandServiceDestroying();
 }
 
 static base::LazyInstance<BrowserContextKeyedAPIFactory<CommandService>>::
@@ -236,19 +230,9 @@ bool CommandService::AddKeybindingPref(
                          std::move(suggested_key_prefs));
 
   // Fetch the newly-updated command, and notify the observers.
-  for (auto& observer : observers_) {
-    observer.OnExtensionCommandAdded(
-        extension_id, FindCommandByName(extension_id, command_name));
-  }
-
-  // TODO(devlin): Deprecate this notification in favor of the observers.
-  std::pair<const std::string, const std::string> details =
-      std::make_pair(extension_id, command_name);
-  content::NotificationService::current()->Notify(
-      extensions::NOTIFICATION_EXTENSION_COMMAND_ADDED,
-      content::Source<Profile>(profile_),
-      content::Details<std::pair<const std::string, const std::string> >(
-          &details));
+  Command command = FindCommandByName(extension_id, command_name);
+  for (auto& observer : observers_)
+    observer.OnExtensionCommandAdded(extension_id, command);
 
   return true;
 }
@@ -653,14 +637,6 @@ void CommandService::RemoveKeybindingPrefs(const std::string& extension_id,
        it != keys_to_remove.end(); ++it) {
     std::string key = *it;
     bindings->Remove(key, NULL);
-
-    // TODO(devlin): Deprecate this notification in favor of the observers.
-    ExtensionCommandRemovedDetails details(extension_id, command_name,
-                                           StripCurrentPlatform(key));
-    content::NotificationService::current()->Notify(
-        extensions::NOTIFICATION_EXTENSION_COMMAND_REMOVED,
-        content::Source<Profile>(profile_),
-        content::Details<ExtensionCommandRemovedDetails>(&details));
   }
 
   for (const Command& removed_command : removed_commands) {

chrome/browser/extensions/api/commands/command_service.h

@@ -78,6 +78,10 @@ class CommandService : public BrowserContextKeyedAPI,
     // Called when an extension command is removed.
     virtual void OnExtensionCommandRemoved(const std::string& extension_id,
                                            const Command& command) {}
+
+    // Called when the CommandService is being destroyed.
+    virtual void OnCommandServiceDestroying() {}
+
    protected:
     virtual ~Observer() {}
   };

chrome/browser/extensions/extension_keybinding_registry.cc

@@ -14,7 +14,6 @@
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/media_keys_listener_manager.h"
 #include "extensions/browser/event_router.h"
-#include "extensions/browser/notification_types.h"
 #include "extensions/common/extension_set.h"
 #include "extensions/common/manifest_constants.h"
 
@@ -39,14 +38,7 @@ ExtensionKeybindingRegistry::ExtensionKeybindingRegistry(
       delegate_(delegate),
       shortcut_handling_suspended_(false) {
   extension_registry_observer_.Add(ExtensionRegistry::Get(browser_context_));
-
-  Profile* profile = Profile::FromBrowserContext(browser_context_);
-  registrar_.Add(this,
-                 extensions::NOTIFICATION_EXTENSION_COMMAND_ADDED,
-                 content::Source<Profile>(profile->GetOriginalProfile()));
-  registrar_.Add(this,
-                 extensions::NOTIFICATION_EXTENSION_COMMAND_REMOVED,
-                 content::Source<Profile>(profile->GetOriginalProfile()));
+  command_service_observer_.Add(CommandService::Get(browser_context_));
   media_keys_listener_ = ui::MediaKeysListener::Create(
       this, ui::MediaKeysListener::Scope::kFocused);
 }
@@ -241,43 +233,44 @@ void ExtensionKeybindingRegistry::OnExtensionUnloaded(
     RemoveExtensionKeybinding(extension, std::string());
 }
 
-void ExtensionKeybindingRegistry::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  switch (type) {
-    case extensions::NOTIFICATION_EXTENSION_COMMAND_ADDED:
-    case extensions::NOTIFICATION_EXTENSION_COMMAND_REMOVED: {
-      ExtensionCommandRemovedDetails* payload =
-          content::Details<ExtensionCommandRemovedDetails>(details).ptr();
-
+void ExtensionKeybindingRegistry::OnExtensionCommandAdded(
+    const std::string& extension_id,
+    const Command& command) {
   const Extension* extension = ExtensionRegistry::Get(browser_context_)
                                    ->enabled_extensions()
-                                       .GetByID(payload->extension_id);
+                                   .GetByID(extension_id);
   // During install and uninstall the extension won't be found. We'll catch
-      // those events above, with the LOADED/UNLOADED, so we ignore this event.
-      if (!extension)
+  // those events above, with the OnExtension[Unloaded|Loaded], so we ignore
+  // this event.
+  if (!extension || !ExtensionMatchesFilter(extension))
     return;
 
-      if (ExtensionMatchesFilter(extension)) {
-        if (type == extensions::NOTIFICATION_EXTENSION_COMMAND_ADDED) {
-          // Component extensions triggers OnExtensionLoaded for extension
+  // Component extensions trigger OnExtensionLoaded() for extension
   // installs as well as loads. This can cause adding of multiple key
   // targets.
   if (extension->location() == Manifest::COMPONENT)
     return;
 
-          AddExtensionKeybindings(extension, payload->command_name);
-        } else {
-          RemoveExtensionKeybinding(extension, payload->command_name);
-        }
-      }
-      break;
-    }
-    default:
-      NOTREACHED();
-      break;
-  }
+  AddExtensionKeybindings(extension, command.command_name());
+}
+
+void ExtensionKeybindingRegistry::OnExtensionCommandRemoved(
+    const std::string& extension_id,
+    const Command& command) {
+  const Extension* extension = ExtensionRegistry::Get(browser_context_)
+                                   ->enabled_extensions()
+                                   .GetByID(extension_id);
+  // During install and uninstall the extension won't be found. We'll catch
+  // those events above, with the OnExtension[Unloaded|Loaded], so we ignore
+  // this event.
+  if (!extension || !ExtensionMatchesFilter(extension))
+    return;
+
+  RemoveExtensionKeybinding(extension, command.command_name());
+}
+
+void ExtensionKeybindingRegistry::OnCommandServiceDestroying() {
+  command_service_observer_.RemoveAll();
 }
 
 void ExtensionKeybindingRegistry::OnMediaKeysAccelerator(

chrome/browser/extensions/extension_keybinding_registry.h

@@ -13,10 +13,7 @@
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/scoped_observer.h"
-#include "content/public/browser/notification_details.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
-#include "content/public/browser/notification_source.h"
+#include "chrome/browser/extensions/api/commands/command_service.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_registry_observer.h"
 #include "ui/base/accelerators/media_keys_listener.h"
@@ -37,7 +34,7 @@ class Extension;
 // The ExtensionKeybindingRegistry is a class that handles the cross-platform
 // logic for keyboard accelerators. See platform-specific implementations for
 // implementation details for each platform.
-class ExtensionKeybindingRegistry : public content::NotificationObserver,
+class ExtensionKeybindingRegistry : public CommandService::Observer,
                                     public ExtensionRegistryObserver,
                                     public ui::MediaKeysListener::Delegate {
  public:
@@ -134,10 +131,12 @@ class ExtensionKeybindingRegistry : public content::NotificationObserver,
   content::BrowserContext* browser_context() const { return browser_context_; }
 
  private:
-  // Overridden from content::NotificationObserver:
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override;
+  // extensions::CommandService::Observer:
+  void OnExtensionCommandAdded(const std::string& extension_id,
+                               const Command& command) override;
+  void OnExtensionCommandRemoved(const std::string& extension_id,
+                                 const Command& command) override;
+  void OnCommandServiceDestroying() override;
 
   // ExtensionRegistryObserver implementation.
   void OnExtensionLoaded(content::BrowserContext* browser_context,
@@ -162,9 +161,6 @@ class ExtensionKeybindingRegistry : public content::NotificationObserver,
   // Returns true if any media keys are registered.
   bool IsListeningToAnyMediaKeys() const;
 
-  // The content notification registrar for listening to extension events.
-  content::NotificationRegistrar registrar_;
-
   content::BrowserContext* browser_context_;
 
   // What extensions to register keybindings for.
@@ -188,6 +184,9 @@ class ExtensionKeybindingRegistry : public content::NotificationObserver,
   ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>
       extension_registry_observer_{this};
 
+  ScopedObserver<CommandService, CommandService::Observer>
+      command_service_observer_{this};
+
   // Keeps track of whether shortcut handling is currently suspended. Shortcuts
   // are suspended briefly while capturing which shortcut to assign to an
   // extension command in the Config UI. If handling isn't suspended while

chrome/browser/ui/views/extensions/extension_action_platform_delegate_views.cc

@@ -20,9 +20,6 @@
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 #include "chrome/common/extensions/api/extension_action/action_info.h"
 #include "chrome/common/extensions/command.h"
-#include "content/public/browser/notification_details.h"
-#include "content/public/browser/notification_source.h"
-#include "extensions/browser/notification_types.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest_constants.h"
 #include "ui/views/view.h"
@@ -39,14 +36,8 @@ ExtensionActionPlatformDelegate::Create(
 ExtensionActionPlatformDelegateViews::ExtensionActionPlatformDelegateViews(
     ExtensionActionViewController* controller)
     : controller_(controller) {
-  content::NotificationSource notification_source = content::Source<Profile>(
-      controller_->browser()->profile()->GetOriginalProfile());
-  registrar_.Add(this,
-                 extensions::NOTIFICATION_EXTENSION_COMMAND_ADDED,
-                 notification_source);
-  registrar_.Add(this,
-                 extensions::NOTIFICATION_EXTENSION_COMMAND_REMOVED,
-                 notification_source);
+  command_service_observer_.Add(
+      extensions::CommandService::Get(controller_->browser()->profile()));
 }
 
 ExtensionActionPlatformDelegateViews::~ExtensionActionPlatformDelegateViews() {
@@ -91,25 +82,42 @@ void ExtensionActionPlatformDelegateViews::ShowContextMenu() {
       view, view->GetKeyboardContextMenuLocation(), ui::MENU_SOURCE_NONE);
 }
 
-void ExtensionActionPlatformDelegateViews::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  DCHECK(type == extensions::NOTIFICATION_EXTENSION_COMMAND_ADDED ||
-         type == extensions::NOTIFICATION_EXTENSION_COMMAND_REMOVED);
-  extensions::ExtensionCommandRemovedDetails* payload =
-      content::Details<extensions::ExtensionCommandRemovedDetails>(details)
-          .ptr();
-  if (controller_->extension()->id() == payload->extension_id &&
-      (payload->command_name ==
-           extensions::manifest_values::kBrowserActionCommandEvent ||
-       payload->command_name ==
-           extensions::manifest_values::kPageActionCommandEvent)) {
-    if (type == extensions::NOTIFICATION_EXTENSION_COMMAND_ADDED)
+void ExtensionActionPlatformDelegateViews::OnExtensionCommandAdded(
+    const std::string& extension_id,
+    const extensions::Command& command) {
+  if (extension_id != controller_->extension()->id())
+    return;  // Not this action's extension.
+
+  if (command.command_name() !=
+          extensions::manifest_values::kBrowserActionCommandEvent &&
+      command.command_name() !=
+          extensions::manifest_values::kPageActionCommandEvent) {
+    // Not an action-related command.
+    return;
+  }
+
   RegisterCommand();
-    else
-      UnregisterCommand(true);
+}
+
+void ExtensionActionPlatformDelegateViews::OnExtensionCommandRemoved(
+    const std::string& extension_id,
+    const extensions::Command& command) {
+  if (extension_id != controller_->extension()->id())
+    return;
+
+  if (command.command_name() !=
+          extensions::manifest_values::kBrowserActionCommandEvent &&
+      command.command_name() !=
+          extensions::manifest_values::kPageActionCommandEvent) {
+    // Not an action-related command.
+    return;
   }
+
+  UnregisterCommand(/*only_if_removed=*/true);
+}
+
+void ExtensionActionPlatformDelegateViews::OnCommandServiceDestroying() {
+  command_service_observer_.RemoveAll();
 }
 
 bool ExtensionActionPlatformDelegateViews::AcceleratorPressed(

chrome/browser/ui/views/extensions/extension_action_platform_delegate_views.h

@@ -6,9 +6,9 @@
 #define CHROME_BROWSER_UI_VIEWS_EXTENSIONS_EXTENSION_ACTION_PLATFORM_DELEGATE_VIEWS_H_
 
 #include "base/macros.h"
+#include "base/scoped_observer.h"
+#include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/ui/extensions/extension_action_platform_delegate.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
 #include "ui/base/accelerators/accelerator.h"
 
 class ToolbarActionViewDelegateViews;
@@ -23,7 +23,7 @@ class ToolbarActionViewDelegateViews;
 // the views::View wrapper.
 class ExtensionActionPlatformDelegateViews
     : public ExtensionActionPlatformDelegate,
-      public content::NotificationObserver,
+      public extensions::CommandService::Observer,
       public ui::AcceleratorTarget {
  public:
   ExtensionActionPlatformDelegateViews(
@@ -39,10 +39,12 @@ class ExtensionActionPlatformDelegateViews
       ExtensionActionViewController::PopupShowAction show_action) override;
   void ShowContextMenu() override;
 
-  // content::NotificationObserver:
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override;
+  // extensions::CommandService::Observer:
+  void OnExtensionCommandAdded(const std::string& extension_id,
+                               const extensions::Command& command) override;
+  void OnExtensionCommandRemoved(const std::string& extension_id,
+                                 const extensions::Command& command) override;
+  void OnCommandServiceDestroying() override;
 
   // ui::AcceleratorTarget:
   bool AcceleratorPressed(const ui::Accelerator& accelerator) override;
@@ -62,7 +64,9 @@ class ExtensionActionPlatformDelegateViews
   // for (to show the popup).
   std::unique_ptr<ui::Accelerator> action_keybinding_;
 
-  content::NotificationRegistrar registrar_;
+  ScopedObserver<extensions::CommandService,
+                 extensions::CommandService::Observer>
+      command_service_observer_{this};
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionActionPlatformDelegateViews);
 };

chrome/browser/ui/views/extensions/extension_keybinding_registry_views.h

@@ -57,9 +57,6 @@ class ExtensionKeybindingRegistryViews
   // accelerators with. Not owned by us.
   views::FocusManager* focus_manager_;
 
-  // The content notification registrar for listening to extension events.
-  content::NotificationRegistrar registrar_;
-
   DISALLOW_COPY_AND_ASSIGN(ExtensionKeybindingRegistryViews);
 };
 

extensions/browser/BUILD.gn

@@ -282,7 +282,6 @@ jumbo_source_set("browser_sources") {
     "media_capture_util.h",
     "mojo/keep_alive_impl.cc",
     "mojo/keep_alive_impl.h",
-    "notification_types.cc",
     "notification_types.h",
     "null_app_sorting.cc",
     "null_app_sorting.h",

extensions/browser/notification_types.cc

-// Copyright 2015 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 "extensions/browser/notification_types.h"
-
-namespace extensions {
-ExtensionCommandRemovedDetails::ExtensionCommandRemovedDetails(
-    const std::string& extension_id,
-    const std::string& command_name,
-    const std::string& accelerator)
-    : extension_id(extension_id),
-      command_name(command_name),
-      accelerator(accelerator) {
-}
-}  // namespace extensions

extensions/browser/notification_types.h

@@ -106,17 +106,6 @@ enum NotificationType {
   // Sent when a background page is ready so other components can load.
   NOTIFICATION_EXTENSION_BACKGROUND_PAGE_READY,
 
-  // Sent when an extension command has been removed. The source is the
-  // BrowserContext* and the details is an ExtensionCommandRemovedDetails
-  // consisting of std::strings representing an extension ID, the name of the
-  // command being removed, and the accelerator associated with the command.
-  NOTIFICATION_EXTENSION_COMMAND_REMOVED,
-
-  // Sent when an extension command has been added. The source is the
-  // BrowserContext* and the details is a std::pair of two std::string objects
-  // (an extension ID and the name of the command being added).
-  NOTIFICATION_EXTENSION_COMMAND_ADDED,
-
   // Sent by an extension to notify the browser about the results of a unit
   // test.
   NOTIFICATION_EXTENSION_TEST_PASSED,
@@ -168,16 +157,6 @@ enum NotificationType {
   NOTIFICATION_EXTENSIONS_END
 };
 
-struct ExtensionCommandRemovedDetails {
-  ExtensionCommandRemovedDetails(const std::string& extension_id,
-                                 const std::string& command_name,
-                                 const std::string& accelerator);
-
-  std::string extension_id;
-  std::string command_name;
-  std::string accelerator;
-};
-
 // **
 // ** NOTICE
 // **