app_shell: Extract extension runtime data from ExtensionService

chrome/browser/extensions/extension_service.cc

Index: chrome/browser/extensions/extension_service.cc
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index bd076a926f243e95105e8fad03b4b149f0d3cbe2..2341734e40b4ec1895e92241351b71fbdf0cb31b 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -82,6 +82,7 @@
 #include "extensions/browser/pending_extension_manager.h"
 #include "extensions/browser/pref_names.h"
 #include "extensions/browser/process_manager.h"
+#include "extensions/browser/runtime_data.h"
 #include "extensions/browser/update_observer.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/error_utils.h"
@@ -188,15 +189,6 @@ class SharedModuleProvider : public extensions::ManagementPolicy::Provider {
 
 }  // namespace
 
-ExtensionService::ExtensionRuntimeData::ExtensionRuntimeData()
-    : background_page_ready(false),
-      being_upgraded(false),
-      has_used_webrequest(false) {
-}
-
-ExtensionService::ExtensionRuntimeData::~ExtensionRuntimeData() {
-}
-
 // ExtensionService.
 
 void ExtensionService::CheckExternalUninstall(const std::string& id) {
@@ -739,7 +731,7 @@ void ExtensionService::ReloadExtension(const std::string extension_id) {
 
     path = current_extension->path();
     // BeingUpgraded is set back to false when the extension is added.
-    SetBeingUpgraded(current_extension, true);
+    system_->runtime_data()->SetBeingUpgraded(current_extension, true);
     DisableExtension(extension_id, Extension::DISABLE_RELOAD);
     reloading_extensions_.insert(extension_id);
   } else {
@@ -978,9 +970,9 @@ void ExtensionService::DisableExtension(
   if (!extension)
     return;
 
-  // Reset the background_page_ready flag
+  // Reset the background_page_ready flag.
   if (extensions::BackgroundInfo::HasBackgroundPage(extension))
-    extension_runtime_data_[extension->id()].background_page_ready = false;
+    system_->runtime_data()->SetBackgroundPageReady(extension, false);
 
   // Move it over to the disabled list. Don't send a second unload notification
   // for terminated extensions being disabled.
@@ -1590,7 +1582,7 @@ void ExtensionService::UnloadExtension(
   reloading_extensions_.erase(extension->id());
 
   // Clean up runtime data.
-  extension_runtime_data_.erase(extension_id);
+  system_->runtime_data()->Erase(extension);

[not at google - send to devlin, 2014/01/18 01:09:04]

I would expect RuntimeData to be able to manage cleanup itself by listening to
the UNLOAD notification.

[James Cook, 2014/01/18 02:01:18]

I didn't do that because NOTIFICATION_EXTENSION_UNLOADED is a chrome concept
that hasn't yet migrated to src/extensions. It's also not clear to me that doing
it on UNLOAD is equivalent to what's happening here -- for example, the
notification is not always sent below.  And it would change RuntimeData from a
simple wrapper around maps into something that needs a NotificationObserver,
NotificationRegistrar, etc.  And I have trouble reasoning about when things need
to get cleaned up relative to other listeners to the UNLOADED notification.

Should I change it to listen to the notification?

[not at google - send to devlin, 2014/01/21 18:47:47]

Ah right. I think the solution to this would be to add some sensible way to
observe changes to ExtensionRegistry. It's a bit tricky since I think there's
too much subtlety in the way that ExtensionService to, say, have
ExtensionRegistry implement "AddExtensionUnloadedObserver" itself. However I
could at least see an interim solution where ExtensionService fires the event.

I.e. when ExtensionService fires the EXTENSION_UNLOADED event it also calls
registry->OnUnloaded(Extension*), then add a method like
registry->{Add,Remove}UnloadObserver(Callback).

I feel kinda bad suggesting you should shave this yak now, but it's better than
trying to manually maintain this state across ExtensionService and RuntimeData.
Better to just bite the bullet and implement a more generic observation
mechanism now (and start using it).

You should probably check with appshell-team before actually following up on
this :)

 
   if (registry_->disabled_extensions().Contains(extension->id())) {
     registry_->RemoveDisabled(extension->id());
@@ -1720,7 +1712,7 @@ void ExtensionService::AddExtension(const Extension* extension) {
     if (!Manifest::IsUnpackedLocation(extension->location()))
       CHECK_GE(version_compare_result, 0);
   }
-  SetBeingUpgraded(extension, is_extension_upgrade);
+  system_->runtime_data()->SetBeingUpgraded(extension, is_extension_upgrade);
 
   // The extension is now loaded, remove its data from unloaded extension map.
   unloaded_extension_paths_.erase(extension->id());
@@ -1783,7 +1775,7 @@ void ExtensionService::AddExtension(const Extension* extension) {
       extension_sync_service_->SyncExtensionChangeIfNeeded(*extension);
     NotifyExtensionLoaded(extension);
   }
-  SetBeingUpgraded(extension, false);
+  system_->runtime_data()->SetBeingUpgraded(extension, false);
 }
 
 void ExtensionService::AddComponentExtension(const Extension* extension) {
@@ -2616,32 +2608,6 @@ ExtensionIdSet ExtensionService::GetAppIds() const {
   return result;
 }
 
-bool ExtensionService::IsBackgroundPageReady(const Extension* extension) const {
-  if (!extensions::BackgroundInfo::HasPersistentBackgroundPage(extension))
-    return true;
-  ExtensionRuntimeDataMap::const_iterator it =
-      extension_runtime_data_.find(extension->id());
-  return it == extension_runtime_data_.end() ? false :
-                                               it->second.background_page_ready;
-}
-
-void ExtensionService::SetBackgroundPageReady(const Extension* extension) {
-  DCHECK(extensions::BackgroundInfo::HasBackgroundPage(extension));
-  extension_runtime_data_[extension->id()].background_page_ready = true;
-}
-
-bool ExtensionService::IsBeingUpgraded(const Extension* extension) const {
-  ExtensionRuntimeDataMap::const_iterator it =
-      extension_runtime_data_.find(extension->id());
-  return it == extension_runtime_data_.end() ? false :
-                                               it->second.being_upgraded;
-}
-
-void ExtensionService::SetBeingUpgraded(const Extension* extension,
-                                        bool value) {
-  extension_runtime_data_[extension->id()].being_upgraded = value;
-}
-
 bool ExtensionService::IsBeingReloaded(
     const std::string& extension_id) const {
   return ContainsKey(extensions_being_reloaded_, extension_id);
@@ -2655,18 +2621,6 @@ void ExtensionService::SetBeingReloaded(const std::string& extension_id,
     extensions_being_reloaded_.erase(extension_id);
 }
 
-bool ExtensionService::HasUsedWebRequest(const Extension* extension) const {
-  ExtensionRuntimeDataMap::const_iterator it =
-      extension_runtime_data_.find(extension->id());
-  return it == extension_runtime_data_.end() ? false :
-                                               it->second.has_used_webrequest;
-}
-
-void ExtensionService::SetHasUsedWebRequest(const Extension* extension,
-                                            bool value) {
-  extension_runtime_data_[extension->id()].has_used_webrequest = value;
-}
-
 bool ExtensionService::ShouldEnableOnInstall(const Extension* extension) {
   // Extensions installed by policy can't be disabled. So even if a previous
   // installation disabled the extension, make sure it is now enabled.
@@ -2819,7 +2773,7 @@ void ExtensionService::UnloadAllExtensionsInternal() {
   profile_->GetExtensionSpecialStoragePolicy()->RevokeRightsForAllExtensions();
 
   registry_->ClearAll();
-  extension_runtime_data_.clear();
+  system_->runtime_data()->ClearAll();
 
   // TODO(erikkay) should there be a notification for this?  We can't use
   // EXTENSION_UNLOADED since that implies that the extension has been disabled