Convert tabs, windows, and extension APIs to feature system.

Convert tabs, windows, and extension APIs to feature system.

BUG=120069

[Aaron Boodman, 2012-04-07 07:49:11]

mpcomplete:
  chrome/browser/...

koz:
  chrome/renderer/...

kalman:
  chrome/common/...

https://chromiumcodereview.appspot.com/10025007/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/extension_api_unittest.cc (left):

https://chromiumcodereview.appspot.com/10025007/diff/1/chrome/common/extensio...
chrome/common/extensions/api/extension_api_unittest.cc:353: TEST(ExtensionAPI,
DefaultConfigurationFeatures) {
I should bring something like this back, but not sure what.

https://chromiumcodereview.appspot.com/10025007/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/windows.json (right):

https://chromiumcodereview.appspot.com/10025007/diff/1/chrome/common/extensio...
chrome/common/extensions/api/windows.json:56: "contexts": ["blessed_extension"],
It looks a bit odd repeating this everywhere, but I decided it was easier to
reason about when members of an API could not be more accessible than the API
itself. This creates problems for these three highly-coupled APIs, but it hsould
not be a problem in the general case.

https://chromiumcodereview.appspot.com/10025007/diff/1/chrome/renderer/extens...
File chrome/renderer/extensions/extension_dispatcher.cc (left):

https://chromiumcodereview.appspot.com/10025007/diff/1/chrome/renderer/extens...
chrome/renderer/extensions/extension_dispatcher.cc:880: bool
ExtensionDispatcher::CheckCurrentContextAccessToExtensionAPI(
We were checking access in two places in the renderer - once in the JS and once
here. This one didn't seem necessary.

[Aaron Boodman, 2012-04-07 07:50:59]

Matt: One note. extension_messages_browsertest.cc breaks with this because
chrome.extension.connect() is called from an non-extension context and is
therefore unavailable.

I don't see any obvious way to fix this. Do you think we have sufficient
coverage of connect that I can remove this test?

[Matt Perry, 2012-04-09 20:55:54]

As for the messaging test, I think we can kill it. It's covered by API tests.

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/browser/exte...
File chrome/browser/extensions/extension_event_router.cc (right):

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.cc:140:
static_cast<Feature::Context>(context_type))) {
this cast is unnecessary

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/browser/exte...
File chrome/browser/extensions/extension_event_router.h (right):

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.h:77: void
AddLazyEventListener(const std::string& event_name,
I think ExtensionEventRouter is the only caller of this function. If so, make it
private and rename it AddLazyEventListenerImpl.

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/browser/exte...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/browser/exte...
chrome/browser/extensions/extension_function_dispatcher.cc:283: LOG(ERROR) <<
"Access to extension API '" << params.name << "' denied.";
SendAccessDenied? this if body should be identical to the older one.

[not at google - send to devlin, 2012-04-10 02:04:27]

I started looking at this and got 1 diff in, but I need to jump on a plane in a
sec, I might have time to look at the rest in the airport lounge?

I'm only sending this out now because I wanted to add to the
extension_messages_browsertest discussion: if we delete this, we can delete all
that "SetTestExtensionID" crap in ExtensionDispatcher and friends.  That would
be *awesome*.  Perhaps do it in a separate change?  Should be pretty simple
surgery.

http://codereview.chromium.org/10025007/diff/6001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.cc (right):

http://codereview.chromium.org/10025007/diff/6001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.cc:205: << "contexts and will never
be available.";
Why not CHECK?

[jameskozianski, 2012-04-10 20:46:45]

chrome/renderer/... lgtm

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/renderer/ext...
File chrome/renderer/extensions/api_definitions_natives.h (right):

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/renderer/ext...
chrome/renderer/extensions/api_definitions_natives.h:26: // bool
IsMemberAllowed(string namespace, object memberSchema)
The signature here takes two arguments, but only one is used in the definition.

[not at google - send to devlin, 2012-04-10 23:55:40]

lgtm

http://codereview.chromium.org/10025007/diff/6001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.cc (right):

http://codereview.chromium.org/10025007/diff/6001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.cc:400: bool
ExtensionAPI::IsPrivileged(const std::string& full_name) {
Urk, it seems like "IsPrivileged" is a bit of a strange thing to expose, since
we're dealing with multiple types of Contexts and it's not clear from the name
what exactly that means anymore.

But probably not something to look at now.  And I'm too tired to think abou it.

http://codereview.chromium.org/10025007/diff/6001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.cc:655: iter !=
feature->dependencies()->end(); ++iter) {
nit: I think this would fit on 1 line. Certainly if you called it "it" instead
of "iter".

http://codereview.chromium.org/10025007/diff/6001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.cc:682: if (features_.find(*i) !=
features_.end()) {
This pattern is being used a lot. How about adding an IsControlledByFeatures()
method, then we also wouldn't need the comment that follows every check like
this.

http://codereview.chromium.org/10025007/diff/6001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api_feature.h (right):

http://codereview.chromium.org/10025007/diff/6001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api_feature.h:22: std::set<std::string>*
dependencies();
return const& ?

http://codereview.chromium.org/10025007/diff/6001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api_feature.h:27: // on demand for child
feature that lack their own spec.
I don't understand this comment...

http://codereview.chromium.org/10025007/diff/6001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api_unittest.cc (right):

http://codereview.chromium.org/10025007/diff/6001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api_unittest.cc:143: // module.
Don't understand comment... you *can* be less permissive than your module?

http://codereview.chromium.org/10025007/diff/6001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_permission_set.cc (left):

http://codereview.chromium.org/10025007/diff/6001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:68: "tabs.update",
yay!

http://codereview.chromium.org/10025007/diff/6001/chrome/common/extensions/fe...
File chrome/common/extensions/feature.h (right):

http://codereview.chromium.org/10025007/diff/6001/chrome/common/extensions/fe...
chrome/common/extensions/feature.h:109: // Returns true if the Feature contains
no special configuration.
what does "no special configuration" mean?

[Aaron Boodman, 2012-04-11 21:51:53]

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/browser/exte...
File chrome/browser/extensions/extension_event_router.cc (right):

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.cc:140:
static_cast<Feature::Context>(context_type))) {
On 2012/04/09 20:55:54, Matt Perry wrote:
> this cast is unnecessary

Done.

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/browser/exte...
File chrome/browser/extensions/extension_event_router.h (right):

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.h:77: void
AddLazyEventListener(const std::string& event_name,
On 2012/04/09 20:55:54, Matt Perry wrote:
> I think ExtensionEventRouter is the only caller of this function. If so, make
it
> private and rename it AddLazyEventListenerImpl.

No, runtime_api.cc calls it.

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/common/exten...
File chrome/common/extensions/api/extension_api.cc (right):

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/common/exten...
chrome/common/extensions/api/extension_api.cc:205: << "contexts and will never
be available.";
On 2012/04/10 02:04:27, kalman wrote:
> Why not CHECK?

Done.

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/common/exten...
chrome/common/extensions/api/extension_api.cc:655: iter !=
feature->dependencies()->end(); ++iter) {
On 2012/04/10 23:55:41, kalman wrote:
> nit: I think this would fit on 1 line. Certainly if you called it "it" instead
> of "iter".

Done.

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/common/exten...
chrome/common/extensions/api/extension_api.cc:682: if (features_.find(*i) !=
features_.end()) {
On 2012/04/10 23:55:41, kalman wrote:
> This pattern is being used a lot. How about adding an IsControlledByFeatures()
> method, then we also wouldn't need the comment that follows every check like
> this.

I like it. Done.

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/common/exten...
File chrome/common/extensions/api/extension_api_feature.h (right):

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/common/exten...
chrome/common/extensions/api/extension_api_feature.h:22: std::set<std::string>*
dependencies();
On 2012/04/10 23:55:41, kalman wrote:
> return const& ?

I want it to be mutable (e.g., for testing).

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/common/exten...
chrome/common/extensions/api/extension_api_feature.h:27: // on demand for child
feature that lack their own spec.
On 2012/04/10 23:55:41, kalman wrote:
> I don't understand this comment...

Done. Hopefully better now?

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/common/exten...
File chrome/common/extensions/api/extension_api_unittest.cc (right):

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/common/exten...
chrome/common/extensions/api/extension_api_unittest.cc:143: // module.
On 2012/04/10 23:55:41, kalman wrote:
> Don't understand comment... you *can* be less permissive than your module?

Tried to clarify.

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/common/exten...
File chrome/common/extensions/feature.h (right):

https://chromiumcodereview.appspot.com/10025007/diff/6001/chrome/common/exten...
chrome/common/extensions/feature.h:109: // Returns true if the Feature contains
no special configuration.
On 2012/04/10 23:55:41, kalman wrote:
> what does "no special configuration" mean?

Tried to clarify.

[Matt Perry, 2012-04-11 22:19:42]

lgtm

https://chromiumcodereview.appspot.com/10025007/diff/17002/chrome/browser/ext...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10025007/diff/17002/chrome/browser/ext...
chrome/browser/extensions/extension_function_dispatcher.cc:283: LOG(ERROR) <<
"Access to extension API '" << params.name << "' denied.";
Why is the LOG message different from the older code?