Change manifest handler interface to always (implicitly) pass the entire manifest to handlers.

chrome/common/extensions/manifest_handler.cc

Index: chrome/common/extensions/manifest_handler.cc
diff --git a/chrome/common/extensions/manifest_handler.cc b/chrome/common/extensions/manifest_handler.cc
index a88b475e16791823f3700da32bc2d07defeaac18..9d2a9b5502773421f5965afeb9f80470f50365ab 100644
--- a/chrome/common/extensions/manifest_handler.cc
+++ b/chrome/common/extensions/manifest_handler.cc
@@ -8,6 +8,7 @@
 
 #include "base/lazy_instance.h"
 #include "base/memory/linked_ptr.h"
+#include "chrome/common/extensions/manifest.h"
 
 namespace extensions {
 
@@ -17,8 +18,7 @@ class ManifestHandlerRegistry {
  public:
   void RegisterManifestHandler(const std::string& key,
                                ManifestHandler* handler);
-  ManifestHandler* GetManifestHandler(const std::string& key);
-  std::vector<std::string> GetManifestHandlerKeys();
+  bool ParseExtension(Extension* extension, string16* error);
 
  private:
   friend struct base::DefaultLazyInstanceTraits<ManifestHandlerRegistry>;
@@ -33,25 +33,26 @@ void ManifestHandlerRegistry::RegisterManifestHandler(
   handlers_[key] = make_linked_ptr(handler);
 }
 
-ManifestHandler* ManifestHandlerRegistry::GetManifestHandler(
-    const std::string& key) {
-  ManifestHandlerMap::iterator iter = handlers_.find(key);
-  if (iter != handlers_.end())
-    return iter->second.get();
-  // TODO(yoz): The NOTREACHED only makes sense as long as
-  // GetManifestHandlerKeys is how we're getting the available
-  // manifest handlers.
-  NOTREACHED();
-  return NULL;
-}
-
-std::vector<std::string> ManifestHandlerRegistry::GetManifestHandlerKeys() {
-  std::vector<std::string> keys;
+bool ManifestHandlerRegistry::ParseExtension(Extension* extension,
+                                             string16* error) {
+  std::set<ManifestHandler*> handler_set;
   for (ManifestHandlerMap::iterator iter = handlers_.begin();
        iter != handlers_.end(); ++iter) {
-    keys.push_back(iter->first);
+    ManifestHandler* handler = iter->second.get();
+    if (extension->manifest()->HasPath(iter->first) ||
+        handler->AlwaysParseForType(extension->GetType()))

[Matt Perry, 2013/01/29 20:59:26]

Why create a temporary set? Can't you just call Parse() inline here?

[Yoyo Zhou, 2013/01/29 21:01:05]

Because a handler can be registered for more than one key, and we're iterating
over keys here.

[Matt Perry, 2013/01/29 21:03:59]

Ah, I see. This could become a problem if you ever have a multi-key handler that
returns true for AlwaysParseForType.

[Matt Perry, 2013/01/29 21:08:05]

Actually, nevermind, that's the point of using a set. Carry on!

[Yoyo Zhou, 2013/01/29 21:08:30]

How so? The logic should be the same as for a single-key handler:
- If AlwaysParseForType is false, then it's called (once) as long as one of the
keys is there.
- If AlwaysParseForType is true, then it's always called (once).

+    handler_set.insert(iter->second.get());
   }
-  return keys;
+
+  // TODO(yoz): Some handlers may depend on other handlers having already
+  // parsed their keys. Reorder the handlers so that handlers needed earlier
+  // come first in the returned container.
+  for (std::set<ManifestHandler*>::iterator iter = handler_set.begin();
+       iter != handler_set.end(); ++iter) {
+    if (!(*iter)->Parse(extension, error))
+      return false;
+  }
+  return true;
 }
 
 static base::LazyInstance<ManifestHandlerRegistry> g_registry =
@@ -65,8 +66,8 @@ ManifestHandler::ManifestHandler() {
 ManifestHandler::~ManifestHandler() {
 }
 
-bool ManifestHandler::HasNoKey(Extension* extension, string16* error) {
-  return true;
+bool ManifestHandler::AlwaysParseForType(Extension::Type type) {
+  return false;
 }
 
 // static
@@ -76,13 +77,8 @@ void ManifestHandler::Register(const std::string& key,
 }
 
 // static
-ManifestHandler* ManifestHandler::Get(const std::string& key) {
-  return g_registry.Get().GetManifestHandler(key);
-}
-
-// static
-std::vector<std::string> ManifestHandler::GetKeys() {
-  return g_registry.Get().GetManifestHandlerKeys();
+bool ManifestHandler::ParseExtension(Extension* extension, string16* error) {
+  return g_registry.Get().ParseExtension(extension, error);
 }
 
 }  // namespace extensions