ChromeLaunchController does proper action when extension unloaded

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

 // Copyright 2013 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 "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
 
 #include <vector>
 
 #include "ash/ash_switches.h"
 #include "ash/desktop_background/desktop_background_controller.h"
@@ skipping 343 matching lines @@
     chrome::MultiUserWindowManager::GetInstance()->AddUser(profile);
   controller_->AdditionalUserAddedToSession(profile->GetOriginalProfile());
 }
 #endif
 
 ChromeLauncherController::ChromeLauncherController(Profile* profile,
                                                    ash::ShelfModel* model)
     : model_(model),
       item_delegate_manager_(NULL),
       profile_(profile),
+      extension_registry_(NULL),
       app_sync_ui_state_(NULL),
       ignore_persist_pinned_state_change_(false) {
   if (!profile_) {
     // If no profile was passed, we take the currently active profile and use it
     // as the owner of the current desktop.
     // Use the original profile as on chromeos we may get a temporary off the
     // record profile, unless in guest session (where off the record profile is
     // the right one).
     Profile* active_profile = ProfileManager::GetActiveUserProfile();
     profile_ = active_profile->IsGuestSession() ? active_profile :
@@ skipping 43 matching lines @@
   app_window_controller_.reset(new AppWindowLauncherController(this));
 #endif
 
   // Right now ash::Shell isn't created for tests.
   // TODO(mukai): Allows it to observe display change and write tests.
   if (ash::Shell::HasInstance()) {
     ash::Shell::GetInstance()->display_controller()->AddObserver(this);
     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() {
   // Reset the BrowserStatusMonitor as it has a weak pointer to this.
   browser_status_monitor_.reset();
 
   // Reset the app window controller here since it has a weak pointer to this.
   app_window_controller_.reset();
 
   for (std::set<ash::Shelf*>::iterator iter = shelves_.begin();
@@ skipping 724 matching lines @@
   // Force on-screen keyboard to reset.
   if (keyboard::IsKeyboardEnabled())
     ash::Shell::GetInstance()->CreateKeyboard();
 }
 
 void ChromeLauncherController::AdditionalUserAddedToSession(Profile* profile) {
   // Switch the running applications to the new user.
   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;
+  UpdateAppLaunchersFromPref();
+}
+
+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);
     }
-    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);
-      }
-
-      if (IsAppPinned(id)) {
-        if (unload_info->reason == UnloadedExtensionInfo::REASON_UNINSTALL) {
-          DoUnpinAppWithID(id);
-          app_icon_loader_->ClearImage(id);
-        } else {
-          app_icon_loader_->UpdateImage(id);
-        }
-      }
-      break;
-    }
-    default:
-      NOTREACHED() << "Unexpected notification type=" << type;
   }
 }
 
 void ChromeLauncherController::OnShelfAlignmentChanged(
     aura::Window* root_window) {
   const char* pref_value = NULL;
   switch (ash::Shell::GetInstance()->GetShelfAlignment(root_window)) {
     case ash::SHELF_ALIGNMENT_BOTTOM:
       pref_value = ash::kShelfAlignmentBottom;
       break;
@@ skipping 855 matching lines @@
   pref_change_registrar_.Add(
       prefs::kShelfPreferences,
       base::Bind(&ChromeLauncherController::SetShelfBehaviorsFromPrefs,
                  base::Unretained(this)));
 #if defined(OS_CHROMEOS)
   pref_change_registrar_.Add(
       prefs::kTouchVirtualKeyboardEnabled,
       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();
 }