ChromeLaunchController does proper action when extension unloaded

chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc

Index: chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
diff --git a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc b/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
index 40b62086503f1adaeaca61a850869bd518f7db68..dd11b0a49a88e8491cacb310d2536b5d7f87a2aa 100644
--- a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
+++ b/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
@@ -361,6 +361,7 @@ ChromeLauncherController::ChromeLauncherController(Profile* profile,
     : model_(model),
       item_delegate_manager_(NULL),
       profile_(profile),
+      extension_registry_(NULL),
       app_sync_ui_state_(NULL),
       ignore_persist_pinned_state_change_(false) {
   if (!profile_) {
@@ -424,15 +425,6 @@ ChromeLauncherController::ChromeLauncherController(Profile* profile,
     item_delegate_manager_ =
         ash::Shell::GetInstance()->shelf_item_delegate_manager();
   }
-
-  notification_registrar_.Add(

[James Cook, 2014/09/12 20:35:35]

+skuhne -- I'm not sure about the old code here.

ChromeLauncherController can be used with multiple profiles. The original code
here only monitors the first profile for extension load/unload events. That
seems a little odd -- what if the user switches profiles then uninstalls an
extension/app from the second profile?

Your change observes both the original profile and future profiles. That seems
more correct to me.

[Mr4D (OOO till 08-26), 2014/09/12 23:12:11]

A closer look reviles that everything should be as it was - but - it has
actually a problem:

CloseWindowedAppsFromRemovedExtension(id) should really be
CloseWindowedAppsFromRemovedExtension(id, profile).

and only browsers which are matching the id AND the profile should get closed
since each user might have the app installed, but only one does uninstall.

Pinning and unpinning should also only happen if the profile in question is the
current profile.

-      this,
-      extensions::NOTIFICATION_EXTENSION_LOADED_DEPRECATED,
-      content::Source<Profile>(profile_));
-  notification_registrar_.Add(
-      this,
-      extensions::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED,
-      content::Source<Profile>(profile_));
 }
 
 ChromeLauncherController::~ChromeLauncherController() {
@@ -1177,46 +1169,37 @@ void ChromeLauncherController::AdditionalUserAddedToSession(Profile* profile) {
   app_window_controller_->AdditionalUserAddedToSession(profile);
 }
 
-void ChromeLauncherController::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  switch (type) {
-    case extensions::NOTIFICATION_EXTENSION_LOADED_DEPRECATED: {
-      const Extension* extension =
-          content::Details<const Extension>(details).ptr();
-      if (IsAppPinned(extension->id())) {
-        // Clear and re-fetch to ensure icon is up-to-date.
-        app_icon_loader_->ClearImage(extension->id());
-        app_icon_loader_->FetchImage(extension->id());
-      }
+void ChromeLauncherController::OnExtensionLoaded(
+    content::BrowserContext* browser_context,
+    const extensions::Extension* extension) {
+  if (IsAppPinned(extension->id())) {
+    // Clear and re-fetch to ensure icon is up-to-date.
+    app_icon_loader_->ClearImage(extension->id());
+    app_icon_loader_->FetchImage(extension->id());
+  }
 
-      UpdateAppLaunchersFromPref();
-      break;
-    }
-    case extensions::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED: {
-      const content::Details<UnloadedExtensionInfo>& unload_info(details);
-      const Extension* extension = unload_info->extension;
-      const std::string& id = extension->id();
-      // Since we might have windowed apps of this type which might have
-      // outstanding locks which needs to be removed.
-      if (GetShelfIDForAppID(id) &&
-          unload_info->reason == UnloadedExtensionInfo::REASON_UNINSTALL) {
-        CloseWindowedAppsFromRemovedExtension(id);
-      }
+  UpdateAppLaunchersFromPref();
+}
 
-      if (IsAppPinned(id)) {
-        if (unload_info->reason == UnloadedExtensionInfo::REASON_UNINSTALL) {
-          DoUnpinAppWithID(id);
-          app_icon_loader_->ClearImage(id);
-        } else {
-          app_icon_loader_->UpdateImage(id);
-        }
-      }
-      break;
+void ChromeLauncherController::OnExtensionUnloaded(
+    content::BrowserContext* browser_context,
+    const extensions::Extension* extension,
+    extensions::UnloadedExtensionInfo::Reason reason) {
+  const std::string& id = extension->id();
+  // Since we might have windowed apps of this type which might have
+  // outstanding locks which needs to be removed.
+  if (GetShelfIDForAppID(id) &&
+      reason == UnloadedExtensionInfo::REASON_UNINSTALL) {

[Mr4D (OOO till 08-26), 2014/09/12 23:12:11]

You should forward the profile to CloseWindowedAppsFromRemovedExtension so that
only browsers with a matching profile get closed.

[limasdf, 2014/09/13 18:31:01]

Done.

+    CloseWindowedAppsFromRemovedExtension(id);
+  }
+
+  if (IsAppPinned(id)) {

[Mr4D (OOO till 08-26), 2014/09/12 23:12:11]

DoUnpinAppWithID() should only be called if the passed profile is profile(),
since user a's icons should not get closed when user b's applications are
shutting down due to e.g. uninstall.

The app_icon_loader_ on the other hand is a different topic since it does not
distinguish between users (at least not yet).

I guess that ideal would be to have one loader per user, but there is a comment
above stating that that might be not easy. Since this is not a "frequent and
high resource intense" action I guess that we might get away for the time being
if we could check if any user has the id installed before clearing the image.

[limasdf, 2014/09/13 18:31:01]

:: DoUnpinAppWithID() should only be called if the passed profile is profile(),

Done.

On 2014/09/12 23:12:11, Mr4D wrote:

+    if (reason == UnloadedExtensionInfo::REASON_UNINSTALL) {
+      DoUnpinAppWithID(id);
+      app_icon_loader_->ClearImage(id);
+    } else {
+      app_icon_loader_->UpdateImage(id);
     }
-    default:
-      NOTREACHED() << "Unexpected notification type=" << type;
   }
 }
 
@@ -2092,12 +2075,19 @@ void ChromeLauncherController::AttachProfile(Profile* profile) {
       base::Bind(&ChromeLauncherController::SetVirtualKeyboardBehaviorFromPrefs,
                  base::Unretained(this)));
 #endif  // defined(OS_CHROMEOS)
+
+  extension_registry_ = extensions::ExtensionRegistry::Get(profile_);

[James Cook, 2014/09/12 20:35:35]

Can you get rid of the extension_registry_ member and just use
ExtensionRegistry::Get(profile_)->AddObserver() and RemoveObserver()? That might
make it a little more clear.

[limasdf, 2014/09/12 20:53:00]

Done.

+  extension_registry_->AddObserver(this);
 }
 
 void ChromeLauncherController::ReleaseProfile() {
   if (app_sync_ui_state_)
     app_sync_ui_state_->RemoveObserver(this);
 
+  if (extension_registry_)
+    extension_registry_->RemoveObserver(this);
+  extension_registry_ = NULL;
+
   PrefServiceSyncable::FromProfile(profile_)->RemoveObserver(this);
 
   pref_change_registrar_.RemoveAll();