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

chrome/common/extensions/manifest_handler.h

 // Copyright (c) 2013 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.
 
 #ifndef CHROME_COMMON_EXTENSIONS_MANIFEST_HANDLER_H_
 #define CHROME_COMMON_EXTENSIONS_MANIFEST_HANDLER_H_
 
 #include <string>
 #include <vector>
 
 #include "base/string16.h"
 
 namespace base {
+class DictionaryValue;
 class Value;
 }
 
 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();
   virtual ~ManifestHandler();
 
   // Attempts to parse the manifest value.
   // Returns true on success or false on failure; if false, |error| will
   // be set to a failure message.
   virtual bool Parse(const base::Value* value,
                      Extension* extension,
@@ skipping 14 matching lines @@
   // Get the manifest handler associated with |key|, or NULL
   // if there is none.
   static ManifestHandler* Get(const std::string& key);
 
   // If the handler is not handling most of the keys, it may be
   // more efficient to have a list of keys to iterate over.
   // TODO(yoz): this isn't the long-term solution.
   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
 
 #endif  // CHROME_COMMON_EXTENSIONS_MANIFEST_HANDLER_H_