Move page action manifest parsing out of Extension; the first multi-key manifest handler.

chrome/common/extensions/manifest_handler.h

Index: chrome/common/extensions/manifest_handler.h
diff --git a/chrome/common/extensions/manifest_handler.h b/chrome/common/extensions/manifest_handler.h
index f9797d3bd7db5ef061ad35fa1431eed2da2ce031..a8c73c7f3b369f42fe8b1aaa6532769153398b75 100644
--- a/chrome/common/extensions/manifest_handler.h
+++ b/chrome/common/extensions/manifest_handler.h
@@ -11,6 +11,7 @@
 #include "base/string16.h"
 
 namespace base {
+class DictionaryValue;
 class Value;
 }
 
@@ -18,6 +19,9 @@ namespace extensions {
 
 class Extension;
 
+// Base class for extension APIs to implement to parse data they care about
+// from the manifest. This only supports parsing from a single manifest key.
+// If multiple keys need to be passed as input, use ManifestMultiKeyHandler.
 class ManifestHandler {
  public:
   ManifestHandler();
@@ -52,6 +56,40 @@ class ManifestHandler {
   static std::vector<std::string> GetKeys();
 };
 
+// Like ManifestHandler, but used when the parsing of manifest data requires
+// input from multiple keys in the manifest.
+class ManifestMultiKeyHandler {

[Matt Perry, 2013/01/25 20:56:40]

It seems unnecessary to differentiate between single-key and multi-key manifest
handlers. At the least, single-key is just a special case of multi-key.

Idea:
- manifest handler code should keep a map of (single) key to the handler that
can parse that key. a handler can register for more than one key, but multiple
handlers can't register for the same key.
- manifest handler code keeps a list of every manifest handler.
- extension.cc doesn't worry about keys. it just iterates through every manifest
handler registered, giving it the manifest to do its share of the parsing.

Then you won't need the concept of multi-key parsers, and key sets. You
mentioned in IM that the downside to this is handlers could look at keys they
didn't register for. I think that risk is worth the difference in complexity. We
can enforce that through code reviews.

[Devlin, 2013/01/25 21:50:55]

Another thought:
- Have only one type of handler, which has a list of keys which it parses (as
ManifestMultiKeyHandler does now).
- Have ManifestHandler code store a list of every ManifestHandler.
- Have extension.cc get a (reference to a) list of all the ManifestHandlers,
rather than all the keys, and send each handler a value with the appropriate
keys.

This allows us to limit what each handler can see, have only one ManifestHandler
class, and even saves us a little bit of time from the current implementation,
since we don't have to do ManifestHandler::Get() lookups.

Just an idea - there may be downsides I'm not seeing, but I figured I'd throw it
out there.

+ public:
+  typedef std::vector<const char*> KeySet;
+
+  ManifestMultiKeyHandler();
+  virtual ~ManifestMultiKeyHandler();
+
+  // Attempts to parse the manifest values. |dict_value| maps the registered
+  // keys from key_set() to their values from the manifest. If a key is absent
+  // from the the manifest, it will be absent from |dict_value|.
+  // Returns true on success or false on failure; if false, |error| will
+  // be set to a failure message.
+  virtual bool Parse(const base::DictionaryValue* dict_value,
+                     Extension* extension,
+                     string16* error) = 0;
+
+  // Returns the key set this handler should be registered for.
+  virtual const KeySet key_set() = 0;
+
+  // Associate |handler| with its key set in the manifest. Takes ownership
+  // of |handler|.
+  // WARNING: Manifest handlers registered only in the browser process
+  // are not available to renderers.
+  static void Register(ManifestMultiKeyHandler* handler);

[Yoyo Zhou, 2013/01/25 00:28:23]

Notice this is different from the ManifestHandler::Register interface. I think
it's better this way; ManifestHandler should be like this.

+
+  // Get the manifest handler associated with |key_set|, or NULL
+  // if there is none.
+  static ManifestMultiKeyHandler* Get(const KeySet& key_set);
+
+  // Get all key sets associated with manifest multi-key handlers.
+  static std::vector<KeySet> GetKeySets();
+};
 
 }  // namespace extensions