Small refactoring extension_toolbar_model.cc

chrome/browser/extensions/extension_toolbar_model.cc

 // Copyright (c) 2012 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/extensions/extension_toolbar_model.h"
 
 #include <string>
 
 #include "base/metrics/histogram.h"
 #include "base/metrics/histogram_base.h"
@@ skipping 166 matching lines @@
   if (!is_highlighting_) {
     prefs_->SetInteger(extensions::pref_names::kToolbarSize,
                        visible_icon_count_);
   }
 }
 
 void ExtensionToolbarModel::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
-  ExtensionService* extension_service =
-      extensions::ExtensionSystem::Get(profile_)->extension_service();
-  if (!extension_service || !extension_service->is_ready())
-    return;
-
-  if (type == chrome::NOTIFICATION_EXTENSIONS_READY) {
-    InitializeExtensionList(extension_service);
-    return;
-  }
-
-  const Extension* extension = NULL;
-  if (type == chrome::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED) {
-    extension = content::Details<extensions::UnloadedExtensionInfo>(
-        details)->extension;
-  } else if (type ==
-      chrome::NOTIFICATION_EXTENSION_BROWSER_ACTION_VISIBILITY_CHANGED) {
-    extension = extension_service->GetExtensionById(
-        *content::Details<const std::string>(details).ptr(), true);
-  } else {
-    extension = content::Details<const Extension>(details).ptr();
-  }
-  if (type == chrome::NOTIFICATION_EXTENSION_LOADED_DEPRECATED) {
-    // We don't want to add the same extension twice. It may have already been
-    // added by EXTENSION_BROWSER_ACTION_VISIBILITY_CHANGED below, if the user
-    // hides the browser action and then disables and enables the extension.
-    for (size_t i = 0; i < toolbar_items_.size(); i++) {
-      if (toolbar_items_[i].get() == extension)
-        return;  // Already exists.
+  switch (type) {
+    case chrome::NOTIFICATION_EXTENSIONS_READY: {
+      ExtensionService* extension_service =

[not at google - send to devlin, 2014/04/25 16:53:34]

it looks like this whole function early-exits if there's no extension service.
that shouldn't even really be happening since it's extension service that sends
these notifications, but my point is, this refactoring does change the existing
logic.

however, this class doesn't even need an extension service, just an extension
registry, apart from that "is_ready" check, and that you can get from here:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

so I'd make that your refactor here.
- what you have
- replace ES usage with ER and ready() and take it back to the start of Observe

[not at google - send to devlin, 2014/04/25 16:54:34]

another option: just pull the ES back to the start, don't use ER yet, but
DCHECK(ES) rather than guarding against it being null.

[limasdf, 2014/04/25 17:42:54]

I chose second method to keep exisitng code as much as I can.
Done.

+          ExtensionSystem::Get(profile_)->extension_service();
+      if (extension_service && extension_service->is_ready())
+        InitializeExtensionList(extension_service);
+      break;
     }
-    if (ExtensionActionAPI::GetBrowserActionVisibility(
-            extension_prefs_, extension->id())) {
-      AddExtension(extension);
+    case chrome::NOTIFICATION_EXTENSION_LOADED_DEPRECATED: {
+      const Extension* extension =
+          content::Details<const Extension>(details).ptr();
+      // We don't want to add the same extension twice. It may have already been
+      // added by EXTENSION_BROWSER_ACTION_VISIBILITY_CHANGED below, if the user
+      // hides the browser action and then disables and enables the extension.
+      for (size_t i = 0; i < toolbar_items_.size(); i++) {
+        if (toolbar_items_[i].get() == extension)
+          return;  // Already exists.
+      }
+      if (ExtensionActionAPI::GetBrowserActionVisibility(extension_prefs_,
+                                                         extension->id())) {
+        AddExtension(extension);
+      }
+      break;
     }
-  } else if (type == chrome::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED) {
-    RemoveExtension(extension);
-  } else if (type == chrome::NOTIFICATION_EXTENSION_UNINSTALLED) {
-    UninstalledExtension(extension);
-  } else if (type ==
-      chrome::NOTIFICATION_EXTENSION_BROWSER_ACTION_VISIBILITY_CHANGED) {
-    if (ExtensionActionAPI::GetBrowserActionVisibility(
-            extension_prefs_, extension->id())) {
-      AddExtension(extension);
-    } else {
+    case chrome::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED: {
+      const Extension* extension =
+          content::Details<extensions::UnloadedExtensionInfo>(details)
+              ->extension;
       RemoveExtension(extension);
+      break;
     }
-  } else {
-    NOTREACHED() << "Received unexpected notification";
+    case chrome::NOTIFICATION_EXTENSION_UNINSTALLED: {
+      const Extension* extension =
+          content::Details<const Extension>(details).ptr();
+      UninstalledExtension(extension);
+      break;
+    }
+    case chrome::NOTIFICATION_EXTENSION_BROWSER_ACTION_VISIBILITY_CHANGED: {
+      ExtensionService* extension_service =
+          ExtensionSystem::Get(profile_)->extension_service();
+      if (extension_service && extension_service->is_ready()) {
+        const Extension* extension = extension_service->GetExtensionById(
+            *content::Details<const std::string>(details).ptr(), true);
+        if (ExtensionActionAPI::GetBrowserActionVisibility(extension_prefs_,
+                                                           extension->id())) {
+          AddExtension(extension);
+        } else {
+          RemoveExtension(extension);
+        }
+      }
+      break;
+    }
+    default:
+      NOTREACHED() << "Received unexpected notification";
   }
 }
 
 size_t ExtensionToolbarModel::FindNewPositionFromLastKnownGood(
     const Extension* extension) {
   // See if we have last known good position for this extension.
   size_t new_index = 0;
   // Loop through the ID list of known positions, to count the number of visible
   // browser action icons preceding |extension|.
   for (ExtensionIdList::const_iterator iter_id = last_known_positions_.begin();
@@ skipping 341 matching lines @@
     is_highlighting_ = false;
     if (old_visible_icon_count_ != visible_icon_count_) {
       SetVisibleIconCount(old_visible_icon_count_);
       FOR_EACH_OBSERVER(Observer, observers_, VisibleCountChanged());
     }
     FOR_EACH_OBSERVER(Observer, observers_, HighlightModeChanged(false));
   }
 };
 
 }  // namespace extensions