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

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

This makes it possible to support multi-key handlers using the same interface.
Fixes a bug with default behavior for manifest handlers; previously, HasNoKey would be called even for inappropriate extension types.

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

[Yoyo Zhou, 2013-01-29 00:19:57]

This is the revised interface to ManifestHandler, per discussion in
https://chromiumcodereview.appspot.com/12042096/

[Matt Perry, 2013-01-29 00:58:47]

Mostly LGTM - some small comments

https://codereview.chromium.org/12084034/diff/1/chrome/common/extensions/exte...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/12084034/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension.cc:2395:
ManifestHandler::GetHandlers(manifest());
Rather than returning a temporary set that the caller then has to iterate
through, how about providing a ManifestHandler::Parse(manifest()) method that
does it for you?

https://codereview.chromium.org/12084034/diff/1/chrome/common/extensions/mani...
File chrome/common/extensions/manifest.cc (right):

https://codereview.chromium.org/12084034/diff/1/chrome/common/extensions/mani...
chrome/common/extensions/manifest.cc:128: return CanAccessPath(path) &&
value_->GetDictionary(path, out_value);
implement these in terms of the non-const versions (use temporaries)

https://codereview.chromium.org/12084034/diff/1/chrome/common/extensions/mani...
File chrome/common/extensions/manifest_handler.h (right):

https://codereview.chromium.org/12084034/diff/1/chrome/common/extensions/mani...
chrome/common/extensions/manifest_handler.h:31: // are present.
Could you add a sentence or two on when this would be useful? I didn't
understand until I dug through and looked at examples.

[Yoyo Zhou, 2013-01-29 04:32:41]

https://codereview.chromium.org/12084034/diff/1/chrome/common/extensions/exte...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/12084034/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension.cc:2395:
ManifestHandler::GetHandlers(manifest());
On 2013/01/29 00:58:47, Matt Perry wrote:
> Rather than returning a temporary set that the caller then has to iterate
> through, how about providing a ManifestHandler::Parse(manifest()) method that
> does it for you?

Sure.

https://codereview.chromium.org/12084034/diff/1/chrome/common/extensions/mani...
File chrome/common/extensions/manifest.cc (right):

https://codereview.chromium.org/12084034/diff/1/chrome/common/extensions/mani...
chrome/common/extensions/manifest.cc:128: return CanAccessPath(path) &&
value_->GetDictionary(path, out_value);
On 2013/01/29 00:58:47, Matt Perry wrote:
> implement these in terms of the non-const versions (use temporaries)

Done - let me know if this casting looks reasonable.

https://codereview.chromium.org/12084034/diff/1/chrome/common/extensions/mani...
File chrome/common/extensions/manifest_handler.h (right):

https://codereview.chromium.org/12084034/diff/1/chrome/common/extensions/mani...
chrome/common/extensions/manifest_handler.h:31: // are present.
On 2013/01/29 00:58:47, Matt Perry wrote:
> Could you add a sentence or two on when this would be useful? I didn't
> understand until I dug through and looked at examples.

Done.

[Matt Perry, 2013-01-29 20:59:25]

lgtm

https://codereview.chromium.org/12084034/diff/15004/chrome/common/extensions/...
File chrome/common/extensions/manifest_handler.cc (right):

https://codereview.chromium.org/12084034/diff/15004/chrome/common/extensions/...
chrome/common/extensions/manifest_handler.cc:43:
handler->AlwaysParseForType(extension->GetType()))
Why create a temporary set? Can't you just call Parse() inline here?

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

https://codereview.chromium.org/12084034/diff/15004/chrome/common/extensions/...
File chrome/common/extensions/manifest_handler.cc (right):

https://codereview.chromium.org/12084034/diff/15004/chrome/common/extensions/...
chrome/common/extensions/manifest_handler.cc:43:
handler->AlwaysParseForType(extension->GetType()))
On 2013/01/29 20:59:26, Matt Perry wrote:
> Why create a temporary set? Can't you just call Parse() inline here?

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]

https://codereview.chromium.org/12084034/diff/15004/chrome/common/extensions/...
File chrome/common/extensions/manifest_handler.cc (right):

https://codereview.chromium.org/12084034/diff/15004/chrome/common/extensions/...
chrome/common/extensions/manifest_handler.cc:43:
handler->AlwaysParseForType(extension->GetType()))
On 2013/01/29 21:01:05, Yoyo Zhou wrote:
> On 2013/01/29 20:59:26, Matt Perry wrote:
> > Why create a temporary set? Can't you just call Parse() inline here?
> 
> Because a handler can be registered for more than one key, and we're iterating
> over keys here.

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]

https://codereview.chromium.org/12084034/diff/15004/chrome/common/extensions/...
File chrome/common/extensions/manifest_handler.cc (right):

https://codereview.chromium.org/12084034/diff/15004/chrome/common/extensions/...
chrome/common/extensions/manifest_handler.cc:43:
handler->AlwaysParseForType(extension->GetType()))
On 2013/01/29 21:03:59, Matt Perry wrote:
> On 2013/01/29 21:01:05, Yoyo Zhou wrote:
> > On 2013/01/29 20:59:26, Matt Perry wrote:
> > > Why create a temporary set? Can't you just call Parse() inline here?
> > 
> > Because a handler can be registered for more than one key, and we're
iterating
> > over keys here.
> 
> Ah, I see. This could become a problem if you ever have a multi-key handler
that
> returns true for AlwaysParseForType.

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

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

By the way, I was thinking it would be more correct to use the feature system
instead of manually specifying which extension types matter for which keys. I'll
add a TODO for it.

https://codereview.chromium.org/12084034/diff/15004/chrome/common/extensions/...
File chrome/common/extensions/manifest_handler.cc (right):

https://codereview.chromium.org/12084034/diff/15004/chrome/common/extensions/...
chrome/common/extensions/manifest_handler.cc:43:
handler->AlwaysParseForType(extension->GetType()))
On 2013/01/29 21:03:59, Matt Perry wrote:
> On 2013/01/29 21:01:05, Yoyo Zhou wrote:
> > On 2013/01/29 20:59:26, Matt Perry wrote:
> > > Why create a temporary set? Can't you just call Parse() inline here?
> > 
> > Because a handler can be registered for more than one key, and we're
iterating
> > over keys here.
> 
> Ah, I see. This could become a problem if you ever have a multi-key handler
that
> returns true for AlwaysParseForType.

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).

[commit-bot: I haz the power, 2013-01-29 21:26:22]

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

[commit-bot: I haz the power, 2013-01-30 00:01:20]

Change committed as 179470