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

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

Clean up some manifest unit tests.

BUG=159265
TBR=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182170

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

https://codereview.chromium.org/12042096/diff/2001/chrome/common/extensions/m...
File chrome/common/extensions/manifest_handler.h (right):

https://codereview.chromium.org/12042096/diff/2001/chrome/common/extensions/m...
chrome/common/extensions/manifest_handler.h:84: static void
Register(ManifestMultiKeyHandler* handler);
Notice this is different from the ManifestHandler::Register interface. I think
it's better this way; ManifestHandler should be like this.

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

https://codereview.chromium.org/12042096/diff/2001/chrome/common/extensions/a...
File chrome/common/extensions/api/extension_action/page_action_handler.cc
(right):

https://codereview.chromium.org/12042096/diff/2001/chrome/common/extensions/a...
chrome/common/extensions/api/extension_action/page_action_handler.cc:28:
scoped_ptr<ActionInfo> page_action_info;
nit: move this down where it's first used.

https://codereview.chromium.org/12042096/diff/2001/chrome/common/extensions/m...
File chrome/common/extensions/manifest_handler.h (right):

https://codereview.chromium.org/12042096/diff/2001/chrome/common/extensions/m...
chrome/common/extensions/manifest_handler.h:61: class ManifestMultiKeyHandler {
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]

https://codereview.chromium.org/12042096/diff/2001/chrome/common/extensions/m...
File chrome/common/extensions/manifest_handler.h (right):

https://codereview.chromium.org/12042096/diff/2001/chrome/common/extensions/m...
chrome/common/extensions/manifest_handler.h:61: class ManifestMultiKeyHandler {
On 2013/01/25 20:56:40, Matt Perry wrote:
> 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.

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.

[Joe Thomas, 2013-01-26 00:03:38]

Sorry, tried adding myself to the CC list, but I think it's going to the
Reviewers list. I can't remove myself now  :(

[Yoyo Zhou, 2013-01-26 01:29:32]

On 2013/01/25 20:56:40, Matt Perry wrote:
>
https://codereview.chromium.org/12042096/diff/2001/chrome/common/extensions/a...
> File chrome/common/extensions/api/extension_action/page_action_handler.cc
> (right):
> 
>
https://codereview.chromium.org/12042096/diff/2001/chrome/common/extensions/a...
> chrome/common/extensions/api/extension_action/page_action_handler.cc:28:
> scoped_ptr<ActionInfo> page_action_info;
> nit: move this down where it's first used.
> 
>
https://codereview.chromium.org/12042096/diff/2001/chrome/common/extensions/m...
> File chrome/common/extensions/manifest_handler.h (right):
> 
>
https://codereview.chromium.org/12042096/diff/2001/chrome/common/extensions/m...
> chrome/common/extensions/manifest_handler.h:61: class ManifestMultiKeyHandler
{
> 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.

If we do this, the interface of Parse should change to
Parse(extension, error)
since the alternative - Parse(manifest, extension, error) - is silly when the
manifest argument is always extension->manifest().

[Yoyo Zhou, 2013-01-29 00:20:21]

Followed up in
https://codereview.chromium.org/12084034/

[Yoyo Zhou, 2013-02-05 02:12:17]

Rebased on top of changes to support multi-key registration. Please have another
look.

[Yoyo Zhou, 2013-02-12 21:13:30]

On 2013/02/05 02:12:17, Yoyo Zhou wrote:
> Rebased on top of changes to support multi-key registration. Please have
another
> look.

ping

[Matt Perry, 2013-02-12 21:24:03]

Sorry, lost track of this one. LGTM

[Yoyo Zhou, 2013-02-13 00:54:39]

TBR=sky for some refactoring to UI code.

[commit-bot: I haz the power, 2013-02-13 00:58:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/12042096/45042

[commit-bot: I haz the power, 2013-02-13 09:22:54]

Change committed as 182170