Small refactoring extension_toolbar_model.cc

chrome/browser/extensions/extension_toolbar_model.cc

Index: chrome/browser/extensions/extension_toolbar_model.cc
diff --git a/chrome/browser/extensions/extension_toolbar_model.cc b/chrome/browser/extensions/extension_toolbar_model.cc
index 74e8280992a878c0df0072d31c4e260259126eb2..ddd442c8986d969cd19c2c84a7597eef37bfa2d8 100644
--- a/chrome/browser/extensions/extension_toolbar_model.cc
+++ b/chrome/browser/extensions/extension_toolbar_model.cc
@@ -184,53 +184,60 @@ 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";
   }
 }