Convert app_bindings.js to the schema_generated_bindings.js infrastructure.

Convert app_bindings.js to the schema_generated_bindings.js infrastructure.
This involves opening up *all* custom bindings to web pages, where access is
controlled content-script style using a URL-matches property in the API schema.


BUG=104100, 117282
TEST=unit_tests,browser_tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125811

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127391

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127899

[not at google - send to devlin, 2012-02-24 07:12:34]

appNotifications and webstore stuff upcoming, then we can close that bug!

[not at google - send to devlin, 2012-02-24 10:41:52]

Actually, this failed a few tryjobs so I think it needs a bit more work. I'll
ping when it's ready again.

R=koz, Aaron Boodman

[not at google - send to devlin, 2012-02-27 00:55:13]

Just needed to fix Stubs test, good to go!

[koz (OOO until 15th September), 2012-02-27 03:04:11]

lgtm

https://chromiumcodereview.appspot.com/9460002/diff/1/chrome/common/extension...
File chrome/common/extensions/api/extension_api_unittest.cc (right):

https://chromiumcodereview.appspot.com/9460002/diff/1/chrome/common/extension...
chrome/common/extensions/api/extension_api_unittest.cc:123:
"chrome-extension://fakeextension"));
nit: This seems redundant to have the same data passed in to two different
tests. Perhaps consider unifying GetSchemasForURLContains() and MatchesURL()?

https://chromiumcodereview.appspot.com/9460002/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/9460002/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/extension_dispatcher.cc:342: return context_type !=
ChromeV8Context::UNPRIVILEGED;
Invert this conditional and remove this comment?

https://chromiumcodereview.appspot.com/9460002/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/extension_dispatcher.cc:543: return
ChromeV8Context::BLESSED;
Nice.

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

http://codereview.chromium.org/9460002/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api_unittest.cc (right):

http://codereview.chromium.org/9460002/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api_unittest.cc:123:
"chrome-extension://fakeextension"));
On 2012/02/27 03:04:11, koz wrote:
> nit: This seems redundant to have the same data passed in to two different
> tests. Perhaps consider unifying GetSchemasForURLContains() and MatchesURL()?

Yeah, it's ok, you didn't need to soften that with "nit". I was lazy. Done.

http://codereview.chromium.org/9460002/diff/1/chrome/renderer/extensions/exte...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/9460002/diff/1/chrome/renderer/extensions/exte...
chrome/renderer/extensions/extension_dispatcher.cc:342: return context_type !=
ChromeV8Context::UNPRIVILEGED;
On 2012/02/27 03:04:11, koz wrote:
> Invert this conditional and remove this comment?

Right. Good point.

http://codereview.chromium.org/9460002/diff/1/chrome/renderer/extensions/exte...
chrome/renderer/extensions/extension_dispatcher.cc:543: return
ChromeV8Context::BLESSED;
On 2012/02/27 03:04:11, koz wrote:
> Nice.

Cheers.

[Aaron Boodman, 2012-02-27 23:32:31]

extension_dispatcher.cc is kinda a mess, but I guess this will get better once
we have the feature stuff for apis.

http://codereview.chromium.org/9460002/diff/4006/chrome/common/extensions/api...
File chrome/common/extensions/api/app.json (right):

http://codereview.chromium.org/9460002/diff/4006/chrome/common/extensions/api...
chrome/common/extensions/api/app.json:6: "matches": [ "<all_urls>" ],
Can you think of a more descriptive property name? It should be clear that this
describes the URL that this API will be exposed to.

http://codereview.chromium.org/9460002/diff/4006/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.h (right):

http://codereview.chromium.org/9460002/diff/4006/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.h:17: #include
"chrome/common/extensions/url_pattern_set.h"
:) I love it when classes can be reused so nicely.

http://codereview.chromium.org/9460002/diff/4006/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.h:119: std::set<std::string>
completely_unprivileged_apis_;
Why do we keep these sets, rather than computing them at runtime?

http://codereview.chromium.org/9460002/diff/4006/chrome/renderer/extensions/a...
File chrome/renderer/extensions/app_bindings.h (right):

http://codereview.chromium.org/9460002/diff/4006/chrome/renderer/extensions/a...
chrome/renderer/extensions/app_bindings.h:26: int dependency_count,
I think that you can combine this line with the one above.

http://codereview.chromium.org/9460002/diff/4006/chrome/renderer/extensions/c...
File chrome/renderer/extensions/chrome_v8_context.h (right):

http://codereview.chromium.org/9460002/diff/4006/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_v8_context.h:31: // which are iframes holding
web accessible resources).
The parenthetical here is not correct... there is a difference in how we treat
chrome-extension:// URLs that are in their assigned process, and
chrome-extension:// URLs that are in some other process.

The iframe situation you mention here would be the second situation - a
chrome-extension:// URL in some other process (in this case a web renderer).

http://codereview.chromium.org/9460002/diff/4006/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_v8_context.h:32: BLESSED,
I know you aren't crazy about privileged/unprivileged nomenclature, but it is
what we have right now. It would be better to be consistent.

(FWIW: I like "blessed" -- that better indicates that the contexts are
specifically marked as being able to do stuff... I would be OK changing the
nomenclature throughout to blessed/unblessed. But we should do that all at
once).

http://codereview.chromium.org/9460002/diff/4006/chrome/renderer/extensions/e...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/9460002/diff/4006/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_dispatcher.cc:107: // Unrestricted
extension-related v8-extensions.
The restricted_v8_extensions_ set seems mostly-outdated now. Is your plan to
remove it soon?

http://codereview.chromium.org/9460002/diff/4006/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_dispatcher.cc:539: if
(extensions_.ExtensionBindingsAllowed(ExtensionURLInfo(
ExtensionBindingsAllowed is now outdated and should eventually be removed.

For what you're doing here, it is not quite correct. The logic should be (in
psuedo-code):

GetContextType(int extension_group, WebFrame* frame)
  if (group == CONTENT_SCRIPTS)
    return CONTENT_SCRIPTS

  Extension* extension = extensions_.GetByURL(ExtensionURLInfo(...));
  if (extension && (IsExtensionActive(extension->id()) ||
IsApplicationActive(extension->id()))
    return BLESSED;
  }

  return UNBLESSED;
}

It would probably be good also to combine the active_extension_ids_ and
active_app_ids_ sets and just let the callers that care check the extension type
themselves. I think in most cases the callers won't care.

[not at google - send to devlin, 2012-02-28 05:00:04]

(p.s. I removed that unnecessary logging line but didn't want to pollute code
review history with just changing that one line)

so yeah, koz you may want to take another look too.

http://codereview.chromium.org/9460002/diff/4006/chrome/common/extensions/api...
File chrome/common/extensions/api/app.json (right):

http://codereview.chromium.org/9460002/diff/4006/chrome/common/extensions/api...
chrome/common/extensions/api/app.json:6: "matches": [ "<all_urls>" ],
On 2012/02/27 23:32:31, Aaron Boodman wrote:
> Can you think of a more descriptive property name? It should be clear that
this
> describes the URL that this API will be exposed to.

I kinda like "matches" because it is sort of analogous to content scripts. At a
stretch (but not too much of a stretch). But yeah, content scripts run on pages
that match "matches", and likewise this API is available to pages that match
"matches".

What about "availableTo" or "availableToURLs"?

http://codereview.chromium.org/9460002/diff/4006/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.h (right):

http://codereview.chromium.org/9460002/diff/4006/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.h:119: std::set<std::string>
completely_unprivileged_apis_;
On 2012/02/27 23:32:31, Aaron Boodman wrote:
> Why do we keep these sets, rather than computing them at runtime?

... as in lazily? I don't understand what you mean...

(The sets are like caches so that you don't need to search the whole schema
space every time you need to query those methods.)

http://codereview.chromium.org/9460002/diff/4006/chrome/renderer/extensions/a...
File chrome/renderer/extensions/app_bindings.h (right):

http://codereview.chromium.org/9460002/diff/4006/chrome/renderer/extensions/a...
chrome/renderer/extensions/app_bindings.h:26: int dependency_count,
On 2012/02/27 23:32:31, Aaron Boodman wrote:
> I think that you can combine this line with the one above.

Done.

Oh... this will all be deleted after koz's CL.

http://codereview.chromium.org/9460002/diff/4006/chrome/renderer/extensions/c...
File chrome/renderer/extensions/chrome_v8_context.h (right):

http://codereview.chromium.org/9460002/diff/4006/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_v8_context.h:31: // which are iframes holding
web accessible resources).
On 2012/02/27 23:32:31, Aaron Boodman wrote:
> The parenthetical here is not correct... there is a difference in how we treat
> chrome-extension:// URLs that are in their assigned process, and
> chrome-extension:// URLs that are in some other process.
> 
> The iframe situation you mention here would be the second situation - a
> chrome-extension:// URL in some other process (in this case a web renderer).

Done.

http://codereview.chromium.org/9460002/diff/4006/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_v8_context.h:32: BLESSED,
On 2012/02/27 23:32:31, Aaron Boodman wrote:
> I know you aren't crazy about privileged/unprivileged nomenclature, but it is
> what we have right now. It would be better to be consistent.
> 
> (FWIW: I like "blessed" -- that better indicates that the contexts are
> specifically marked as being able to do stuff... I would be OK changing the
> nomenclature throughout to blessed/unblessed. But we should do that all at
> once).

Ok, reverted, and I'll do an audit when I have time.

http://codereview.chromium.org/9460002/diff/4006/chrome/renderer/extensions/e...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/9460002/diff/4006/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_dispatcher.cc:107: // Unrestricted
extension-related v8-extensions.
On 2012/02/27 23:32:31, Aaron Boodman wrote:
> The restricted_v8_extensions_ set seems mostly-outdated now. Is your plan to
> remove it soon?

Yep, well, it's going to go away when koz's CL goes in anyway.

http://codereview.chromium.org/9460002/diff/4006/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_dispatcher.cc:539: if
(extensions_.ExtensionBindingsAllowed(ExtensionURLInfo(
On 2012/02/27 23:32:31, Aaron Boodman wrote:
> ExtensionBindingsAllowed is now outdated and should eventually be removed.
> 
> For what you're doing here, it is not quite correct. The logic should be (in
> psuedo-code):
> 
> GetContextType(int extension_group, WebFrame* frame)
>   if (group == CONTENT_SCRIPTS)
>     return CONTENT_SCRIPTS
> 
>   Extension* extension = extensions_.GetByURL(ExtensionURLInfo(...));
>   if (extension && (IsExtensionActive(extension->id()) ||
> IsApplicationActive(extension->id()))
>     return BLESSED;
>   }
> 
>   return UNBLESSED;
> }
> 
> It would probably be good also to combine the active_extension_ids_ and
> active_app_ids_ sets and just let the callers that care check the extension
type
> themselves. I think in most cases the callers won't care.

Ah. I think what we *really* need is 4 context types.
 - privileged extension pages
 - content scripts
 - unprivileged extension pages
 - web pages

With your suggested change, unprivileged extension pages and web pages would be
lumped into the same category, but this isn't correct; those unprivileged
extension pages need access to sendMessage and stuff, which web pages don't.

So I added the 4th type... and left the ExtensionBindingsAllowed check here. Is
there a better alternative?

Incidentally, is "unprivileged" the best name? What kind of pages are
unprivileged, should it be like OUT_OF_PROCESS_FOO or something like that?

In other news, I checked where IsApplicationActive and ActivateApplication is
used, and some code can be deleted, so I deleted it. You'll need to take another
look at it though.

[Aaron Boodman, 2012-02-29 01:03:57]

http://codereview.chromium.org/9460002/diff/4006/chrome/common/extensions/api...
File chrome/common/extensions/api/app.json (right):

http://codereview.chromium.org/9460002/diff/4006/chrome/common/extensions/api...
chrome/common/extensions/api/app.json:6: "matches": [ "<all_urls>" ],
On 2012/02/28 05:00:04, kalman wrote:
> On 2012/02/27 23:32:31, Aaron Boodman wrote:
> > Can you think of a more descriptive property name? It should be clear that
> this
> > describes the URL that this API will be exposed to.
> 
> I kinda like "matches" because it is sort of analogous to content scripts. At
a
> stretch (but not too much of a stretch). But yeah, content scripts run on
pages
> that match "matches", and likewise this API is available to pages that match
> "matches".
> 
> What about "availableTo" or "availableToURLs"?

OK, I can see your argument. "matches" is fine.

http://codereview.chromium.org/9460002/diff/4006/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.h (right):

http://codereview.chromium.org/9460002/diff/4006/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.h:119: std::set<std::string>
completely_unprivileged_apis_;
On 2012/02/28 05:00:04, kalman wrote:
> On 2012/02/27 23:32:31, Aaron Boodman wrote:
> > Why do we keep these sets, rather than computing them at runtime?
> 
> ... as in lazily? I don't understand what you mean...
> 
> (The sets are like caches so that you don't need to search the whole schema
> space every time you need to query those methods.)

I see. Since computing these top two is hard, I can see caching them. But
computing url_matching_apis_ is easy, and it seems weird to have a cache of
that... Can you just do that at runtime (each time it is needed)?

http://codereview.chromium.org/9460002/diff/11002/chrome/renderer/extensions/...
File chrome/renderer/extensions/chrome_v8_context.h (right):

http://codereview.chromium.org/9460002/diff/11002/chrome/renderer/extensions/...
chrome/renderer/extensions/chrome_v8_context.h:32: PRIVILEGED,
PRIVILEGED_EXTENSION_PAGE,
UNPRIVILEGED_EXTENSION_PAGE,
OTHER_WEB_PAGE,
CONTENT_SCRIPT

?

http://codereview.chromium.org/9460002/diff/11002/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/9460002/diff/11002/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_dispatcher.cc:305: std::string
custom_binding_api_name =
If you make the change I suggested in schema_generated_bindings.cc, would it be
possible to replace this code with a call to ExtensionAPI?

return ExtensionAPI::GetInstance()->IsModuleAvailable(ContextType, Extension,
GURL)

http://codereview.chromium.org/9460002/diff/11002/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_dispatcher.cc:525:
ChromeV8Context::ContextType ExtensionDispatcher::GetContextType(
Naming suggestion: ClassifyJavaScriptContext ?

http://codereview.chromium.org/9460002/diff/11002/chrome/renderer/extensions/...
File chrome/renderer/extensions/schema_generated_bindings.cc (right):

http://codereview.chromium.org/9460002/diff/11002/chrome/renderer/extensions/...
chrome/renderer/extensions/schema_generated_bindings.cc:157:
ExtensionAPI::GetInstance()->GetSchemasForExtension(
Is it possible to consolidate this knowledge into ExtensionAPI?

ExtensionAPI::GetSchemasForContext(ContextType, Extension*, GURL)

Also, it seems like it would simplify things to add the new context type
"web_page" (or maybe "other_web_page") to the api.json. So the app declaration
would be:

other_web_page=true, matches=["<all_urls>"]

That way the implementation of GetSchemasForContext could just check each
requirement of the schema node against what was passed in, and not have a
special case for WEB_PAGE.

[not at google - send to devlin, 2012-03-05 07:46:54]

Ok, this patch changed *a lot*.

Mostly because I merged it with koz's unsubmitted change (the big module one)
which is still failing a couple of tests (and I think the ones that this change
is failing are due to that, but we'll find out soon I guess). This is a pretty
big merge, the intent is the same of course, but some of the mechanisms
(particularly in extension_dispatcher.cc) are now quite different.

Also, as expected it allowed some of that permission checking logic to be
simplified. As I mentioned in a comment, now there's just a GetAvailableAPIs
call exposed from ExtensionAPI, so that's centralised.

http://codereview.chromium.org/9460002/diff/4006/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.h (right):

http://codereview.chromium.org/9460002/diff/4006/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.h:119: std::set<std::string>
completely_unprivileged_apis_;
On 2012/02/29 01:03:58, Aaron Boodman wrote:
> On 2012/02/28 05:00:04, kalman wrote:
> > On 2012/02/27 23:32:31, Aaron Boodman wrote:
> > > Why do we keep these sets, rather than computing them at runtime?
> > 
> > ... as in lazily? I don't understand what you mean...
> > 
> > (The sets are like caches so that you don't need to search the whole schema
> > space every time you need to query those methods.)
> 
> I see. Since computing these top two is hard, I can see caching them. But
> computing url_matching_apis_ is easy, and it seems weird to have a cache of
> that... Can you just do that at runtime (each time it is needed)?

Oh, the reason I cached url_matching_apis is so that we wouldn't have to parse
the URL pattern every time.

http://codereview.chromium.org/9460002/diff/11002/chrome/renderer/extensions/...
File chrome/renderer/extensions/chrome_v8_context.h (right):

http://codereview.chromium.org/9460002/diff/11002/chrome/renderer/extensions/...
chrome/renderer/extensions/chrome_v8_context.h:32: PRIVILEGED,
On 2012/02/29 01:03:58, Aaron Boodman wrote:
> PRIVILEGED_EXTENSION_PAGE,
> UNPRIVILEGED_EXTENSION_PAGE,
> OTHER_WEB_PAGE,
> CONTENT_SCRIPT
> 
> ?

Done.

As discussed: just used the Features version of this, added WEB_PAGE to that.

http://codereview.chromium.org/9460002/diff/11002/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/9460002/diff/11002/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_dispatcher.cc:305: std::string
custom_binding_api_name =
On 2012/02/29 01:03:58, Aaron Boodman wrote:
> If you make the change I suggested in schema_generated_bindings.cc, would it
be
> possible to replace this code with a call to ExtensionAPI?
> 
> return ExtensionAPI::GetInstance()->IsModuleAvailable(ContextType, Extension,
> GURL)

Done... kinda.

I consolidated all the logic in ExtensionAPI, and this all became a lot simpler
after koz's change.

The only method that deals with any of this is now
ExtensionAPI::GetAvailableAPIs. It's up to the callers to do what they want with
this. IsModuleAvailable is implemented as GetAvailableAPIs().count(api).

http://codereview.chromium.org/9460002/diff/11002/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_dispatcher.cc:525:
ChromeV8Context::ContextType ExtensionDispatcher::GetContextType(
On 2012/02/29 01:03:58, Aaron Boodman wrote:
> Naming suggestion: ClassifyJavaScriptContext ?

Done.

http://codereview.chromium.org/9460002/diff/11002/chrome/renderer/extensions/...
File chrome/renderer/extensions/schema_generated_bindings.cc (right):

http://codereview.chromium.org/9460002/diff/11002/chrome/renderer/extensions/...
chrome/renderer/extensions/schema_generated_bindings.cc:157:
ExtensionAPI::GetInstance()->GetSchemasForExtension(
On 2012/02/29 01:03:58, Aaron Boodman wrote:
> Is it possible to consolidate this knowledge into ExtensionAPI?
> 
> ExtensionAPI::GetSchemasForContext(ContextType, Extension*, GURL)

Done. This is much nicer (I actually made it just GetAvailableAPIs which returns
a set -- much simpler).

> 
> Also, it seems like it would simplify things to add the new context type
> "web_page" (or maybe "other_web_page") to the api.json. So the app declaration
> would be:
> 
> other_web_page=true, matches=["<all_urls>"]
> 
> That way the implementation of GetSchemasForContext could just check each
> requirement of the schema node against what was passed in, and not have a
> special case for WEB_PAGE.

I'm not sure if that does simplify it. Hmm. Well, actually, once the Features
work for ExtensionAPI is done I think that for consistency it would probably
make sense to have this in the availableTo thing. I don't think it does right
now, though... the presence of a "matches" property implies web_page, but what
would the presence of web_page without matches imply?

Perhaps I'm remembering / looking at this the wrong way.

http://codereview.chromium.org/9460002/diff/17001/chrome/renderer/extensions/...
File chrome/renderer/extensions/schema_generated_bindings.cc (right):

http://codereview.chromium.org/9460002/diff/17001/chrome/renderer/extensions/...
chrome/renderer/extensions/schema_generated_bindings.cc:121: // TODO(kalman):
can we just cache this in the ChromeV8Context instance?
I added this TODO for the duration of the code review. Haven't really thought it
through properly.

[Aaron Boodman, 2012-03-06 01:17:21]

nice

http://codereview.chromium.org/9460002/diff/18034/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

http://codereview.chromium.org/9460002/diff/18034/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:120: if (type
!= Extension::TYPE_THEME)
I prefer a whitelist here to the blacklist.

I guess it would be even better to do it with a switch and for the default case
CHECK(false) so we don't forget to update this.

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

http://codereview.chromium.org/9460002/diff/18034/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.cc:245: const base::ListValue* list,
Why move? Just makes things hard to review.

http://codereview.chromium.org/9460002/diff/18034/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.h (right):

http://codereview.chromium.org/9460002/diff/18034/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.h:81: void InsertAllowedAPIs(
This is still "Getting" allowed APIs. The insertion is just an implementation
detail.

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
File chrome/renderer/extensions/app_bindings.cc (left):

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
chrome/renderer/extensions/app_bindings.cc:84: // TODO(aa): GARRR - why is there
IsExtensionActive and IsApplicationActive!?
:) - Thanks I really appreciate these little cleanups.

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_dispatcher.cc:691: return
Feature::UNPRIVILEGED_CONTEXT;
OK, but this name is terrible. After this CL, can you rename these so they make
more sense?

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_dispatcher.h (right):

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_dispatcher.h:61: // Finds the extension ID
for the current context. This is determined from
s/current/specified/

Also, comment should say what, not how:

Finds the extension ID for the JavaScript context associated with the specified
frame and isolated world. If world_id is zero, finds the extension ID associated
with the main world's JavaScript context. If the JavaScript context isn't from
an extension, returns empty string.

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_dispatcher.h:152: // Returns the type of
context that should be created, given an extension ID
Returns the Feature::Context type for a JavaScript context.

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
File chrome/renderer/extensions/schema_generated_bindings.cc (right):

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
chrome/renderer/extensions/schema_generated_bindings.cc:121: // TODO(kalman):
can we just cache this in the ChromeV8Context instance?
What is the value in that. This should only be getting called once per context
anyway.

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
chrome/renderer/extensions/schema_generated_bindings.cc:136:
v8::Persistent<v8::Context> context(v8::Context::New());
Not your bug, but we should just create one utility context that we reuse. The
way this is now, we can end up creating lots of contexts which is kinda a waste.

[not at google - send to devlin, 2012-03-06 03:53:00]

http://codereview.chromium.org/9460002/diff/18034/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

http://codereview.chromium.org/9460002/diff/18034/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:120: if (type
!= Extension::TYPE_THEME)
On 2012/03/06 01:17:21, Aaron Boodman wrote:
> I prefer a whitelist here to the blacklist.
> 
> I guess it would be even better to do it with a switch and for the default
case
> CHECK(false) so we don't forget to update this.

Done.

(added NOTREACHED() to a version of the patch that i'll upload next round of
comments... though clang will actually check this I believe).

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

http://codereview.chromium.org/9460002/diff/18034/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.cc:245: const base::ListValue* list,
On 2012/03/06 01:17:21, Aaron Boodman wrote:
> Why move? Just makes things hard to review.

Dunno. Moved back.

Oh, because I have a thing about util methods being below those that call them
(with the exception of anonymous ones for obvious reasons). I try to resist but
I guess that one slipped through self censorship.

http://codereview.chromium.org/9460002/diff/18034/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.h (right):

http://codereview.chromium.org/9460002/diff/18034/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.h:81: void InsertAllowedAPIs(
On 2012/03/06 01:17:21, Aaron Boodman wrote:
> This is still "Getting" allowed APIs. The insertion is just an implementation
> detail.

Done.

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_dispatcher.cc:691: return
Feature::UNPRIVILEGED_CONTEXT;
On 2012/03/06 01:17:21, Aaron Boodman wrote:
> OK, but this name is terrible. After this CL, can you rename these so they
make
> more sense?

Yep. Added TODO to feature.h

Does s/PRIVILEGED/BLESSED_EXTENSION/ sound right?

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_dispatcher.h (right):

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_dispatcher.h:61: // Finds the extension ID
for the current context. This is determined from
On 2012/03/06 01:17:21, Aaron Boodman wrote:
> s/current/specified/
> 
> Also, comment should say what, not how:
> 
> Finds the extension ID for the JavaScript context associated with the
specified
> frame and isolated world. If world_id is zero, finds the extension ID
associated
> with the main world's JavaScript context. If the JavaScript context isn't from
> an extension, returns empty string.

Done.

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_dispatcher.h:152: // Returns the type of
context that should be created, given an extension ID
On 2012/03/06 01:17:21, Aaron Boodman wrote:
> Returns the Feature::Context type for a JavaScript context.

Done.

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
File chrome/renderer/extensions/schema_generated_bindings.cc (right):

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
chrome/renderer/extensions/schema_generated_bindings.cc:121: // TODO(kalman):
can we just cache this in the ChromeV8Context instance?
On 2012/03/06 01:17:21, Aaron Boodman wrote:
> What is the value in that. This should only be getting called once per context
> anyway.

Because it's getting called many times per context: each time IsAPIAllowed
(native call, see ExtensionDispatcher and setup_bindings.js) is called involves
calculating the allowed APIs. This is pretty unfortunate, in fact, it gets
called num_allowed_apis times (+ once more here).

The other way to avoid this is to just forget about the extra safety of
IsAPIAllowed (since in reality it's called only from setup_bindings.js in a loop
over APIs that are allowed).

Stepping back, the check in setup_bindings.js should really be put on require()
somehow, else it's only a half-baked security mechanism.

Anyway, in conclusion, I'm most in favour of deleting IsAPIAllowed.

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
chrome/renderer/extensions/schema_generated_bindings.cc:136:
v8::Persistent<v8::Context> context(v8::Context::New());
On 2012/03/06 01:17:21, Aaron Boodman wrote:
> Not your bug, but we should just create one utility context that we reuse. The
> way this is now, we can end up creating lots of contexts which is kinda a
waste.

Chatting to koz, he says it shouldn't be using Context at all.  Should this just
be fixed in a later patch?

[koz (OOO until 15th September), 2012-03-06 04:06:57]

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

http://codereview.chromium.org/9460002/diff/18034/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.cc:292:
RemovePrivilegedAPIs(result.get());
This logic is beautiful ;_;

http://codereview.chromium.org/9460002/diff/18034/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.cc:298: for (std::map<std::string,
URLPatternSet>::const_iterator i =
tiny nit: I feel like this part should be lifted into a function (eg:
GetAPIsMatchingURL(result.get()) so it reads like the other cases.

http://codereview.chromium.org/9460002/diff/18034/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.h (right):

http://codereview.chromium.org/9460002/diff/18034/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.h:92: const std::set<std::string>&
reference,
reference -> excluding?

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
File chrome/renderer/extensions/schema_generated_bindings.cc (right):

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
chrome/renderer/extensions/schema_generated_bindings.cc:136:
v8::Persistent<v8::Context> context(v8::Context::New());
On 2012/03/06 01:17:21, Aaron Boodman wrote:
> Not your bug, but we should just create one utility context that we reuse. The
> way this is now, we can end up creating lots of contexts which is kinda a
waste.

I'm not familiar with this usage of Context - isn't creating and disposing one
here a no-op because we don't execute any JS? As such I think those lines can be
deleted.

[Aaron Boodman, 2012-03-06 08:18:12]

lgtm

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_dispatcher.cc:691: return
Feature::UNPRIVILEGED_CONTEXT;
On 2012/03/06 03:53:00, kalman wrote:
> On 2012/03/06 01:17:21, Aaron Boodman wrote:
> > OK, but this name is terrible. After this CL, can you rename these so they
> make
> > more sense?
> 
> Yep. Added TODO to feature.h
> 
> Does s/PRIVILEGED/BLESSED_EXTENSION/ sound right?

Yes.

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
File chrome/renderer/extensions/schema_generated_bindings.cc (right):

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
chrome/renderer/extensions/schema_generated_bindings.cc:121: // TODO(kalman):
can we just cache this in the ChromeV8Context instance?
On 2012/03/06 03:53:00, kalman wrote:
> On 2012/03/06 01:17:21, Aaron Boodman wrote:
> > What is the value in that. This should only be getting called once per
context
> > anyway.
> 
> Because it's getting called many times per context: each time IsAPIAllowed
> (native call, see ExtensionDispatcher and setup_bindings.js) is called
involves
> calculating the allowed APIs. This is pretty unfortunate, in fact, it gets
> called num_allowed_apis times (+ once more here).
> 
> The other way to avoid this is to just forget about the extra safety of
> IsAPIAllowed (since in reality it's called only from setup_bindings.js in a
loop
> over APIs that are allowed).
> 
> Stepping back, the check in setup_bindings.js should really be put on
require()
> somehow, else it's only a half-baked security mechanism.
> 
> Anyway, in conclusion, I'm most in favour of deleting IsAPIAllowed.

Completely agree about removing IsAPIAllowed.

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
chrome/renderer/extensions/schema_generated_bindings.cc:136:
v8::Persistent<v8::Context> context(v8::Context::New());
On 2012/03/06 04:06:58, koz wrote:
> On 2012/03/06 01:17:21, Aaron Boodman wrote:
> > Not your bug, but we should just create one utility context that we reuse.
The
> > way this is now, we can end up creating lots of contexts which is kinda a
> waste.
> 
> I'm not familiar with this usage of Context - isn't creating and disposing one
> here a no-op because we don't execute any JS? As such I think those lines can
be
> deleted.

Explained to kalman in irc.

[not at google - send to devlin, 2012-03-06 11:36:46]

Rebasing now.

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

http://codereview.chromium.org/9460002/diff/18034/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.cc:292:
RemovePrivilegedAPIs(result.get());
On 2012/03/06 04:06:58, koz wrote:
> This logic is beautiful ;_;

Cheers ;_;

http://codereview.chromium.org/9460002/diff/18034/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.cc:298: for (std::map<std::string,
URLPatternSet>::const_iterator i =
On 2012/03/06 04:06:58, koz wrote:
> tiny nit: I feel like this part should be lifted into a function (eg:
> GetAPIsMatchingURL(result.get()) so it reads like the other cases.

Cool, looks better.

http://codereview.chromium.org/9460002/diff/18034/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.h (right):

http://codereview.chromium.org/9460002/diff/18034/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.h:92: const std::set<std::string>&
reference,
On 2012/03/06 04:06:58, koz wrote:
> reference -> excluding?

Done.

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
File chrome/renderer/extensions/schema_generated_bindings.cc (right):

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
chrome/renderer/extensions/schema_generated_bindings.cc:121: // TODO(kalman):
can we just cache this in the ChromeV8Context instance?
On 2012/03/06 08:18:12, Aaron Boodman wrote:
> On 2012/03/06 03:53:00, kalman wrote:
> > On 2012/03/06 01:17:21, Aaron Boodman wrote:
> > > What is the value in that. This should only be getting called once per
> context
> > > anyway.
> > 
> > Because it's getting called many times per context: each time IsAPIAllowed
> > (native call, see ExtensionDispatcher and setup_bindings.js) is called
> involves
> > calculating the allowed APIs. This is pretty unfortunate, in fact, it gets
> > called num_allowed_apis times (+ once more here).
> > 
> > The other way to avoid this is to just forget about the extra safety of
> > IsAPIAllowed (since in reality it's called only from setup_bindings.js in a
> loop
> > over APIs that are allowed).
> > 
> > Stepping back, the check in setup_bindings.js should really be put on
> require()
> > somehow, else it's only a half-baked security mechanism.
> > 
> > Anyway, in conclusion, I'm most in favour of deleting IsAPIAllowed.
> 
> Completely agree about removing IsAPIAllowed.

Done. Also means that ContextInfoNativeHandler can be entirely deleted.

http://codereview.chromium.org/9460002/diff/18034/chrome/renderer/extensions/...
chrome/renderer/extensions/schema_generated_bindings.cc:136:
v8::Persistent<v8::Context> context(v8::Context::New());
On 2012/03/06 08:18:12, Aaron Boodman wrote:
> On 2012/03/06 04:06:58, koz wrote:
> > On 2012/03/06 01:17:21, Aaron Boodman wrote:
> > > Not your bug, but we should just create one utility context that we reuse.
> The
> > > way this is now, we can end up creating lots of contexts which is kinda a
> > waste.
> > 
> > I'm not familiar with this usage of Context - isn't creating and disposing
one
> > here a no-op because we don't execute any JS? As such I think those lines
can
> be
> > deleted.
> 
> Explained to kalman in irc.

TODO added.

[commit-bot: I haz the power, 2012-03-09 05:12:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/9460002/41001

[commit-bot: I haz the power, 2012-03-09 06:38:56]

Change committed as 125811

[commit-bot: I haz the power, 2012-03-17 23:14:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/9460002/44001

[commit-bot: I haz the power, 2012-03-18 02:05:27]

Change committed as 127391

[not at google - send to devlin, 2012-03-20 03:32:13]

koz/aa: PTAL.

Koz is up-to-date with where this patch is now because we discussed it at
length, but, Aaron -- yeah, it looks like we can't load
schema_generated_bindings (due to its size, and its dependencies, and the fact
that it's JavaScript) in every web page. So, as a compromise, I revert that part
of the change and went back to manually setting up chrome.app.

Later we should revisit getting around this restriction. It will probably need
some combination of lazy loading of APIs and JavaScript files, combined with a
nontrivial refactoring of how the whole binding generation mechanism works.

https://chromiumcodereview.appspot.com/9460002/diff/44001/chrome/renderer/ext...
File chrome/renderer/extensions/schema_generated_bindings.cc (right):

https://chromiumcodereview.appspot.com/9460002/diff/44001/chrome/renderer/ext...
chrome/renderer/extensions/schema_generated_bindings.cc:121: // TODO(kalman):
can we just cache this in the ChromeV8Context instance?
I deleted this TODO because it *was* no longer relevant given we were only
looking at the available APIs here. However, now they're also being queried in
extension_dispatcher.cc, so perhaps we should revisit something like this. 
WDYT?

https://chromiumcodereview.appspot.com/9460002/diff/51001/chrome/renderer/ext...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/9460002/diff/51001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:475: }
This is what setup_bindings.js used to do.

https://chromiumcodereview.appspot.com/9460002/diff/51001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:486: }
So this is now the crux of the change. For performance reasons we can't load
schema_generated_bindings in every web page, so we still use the handwritten
app.js (now called... app_custom_bindings.js).

However, at least now we get things like: declarative/sensible integration with
the permissions system, fewer special cases, a nice abstraction for running APIs
in web pages, and the possibility (when we reconcile the performance
consequences) of fully converting app.js later.

https://chromiumcodereview.appspot.com/9460002/diff/51001/chrome/renderer/ext...
File chrome/renderer/extensions/schema_generated_bindings.cc (right):

https://chromiumcodereview.appspot.com/9460002/diff/51001/chrome/renderer/ext...
chrome/renderer/extensions/schema_generated_bindings.cc:158: 
This is basically http://codereview.chromium.org/9699049/ rebased into this
change.

https://chromiumcodereview.appspot.com/9460002/diff/51001/chrome/renderer/res...
File chrome/renderer/resources/extensions/app_custom_bindings.js (right):

https://chromiumcodereview.appspot.com/9460002/diff/51001/chrome/renderer/res...
chrome/renderer/resources/extensions/app_custom_bindings.js:15: };
So basically the same as how it was earlier.

Just not as weirdly written.

https://chromiumcodereview.appspot.com/9460002/diff/51001/chrome/renderer/res...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://chromiumcodereview.appspot.com/9460002/diff/51001/chrome/renderer/res...
chrome/renderer/resources/extensions/schema_generated_bindings.js:537: return;
sigh...

[koz (OOO until 15th September), 2012-03-20 04:43:05]

lgtm

https://chromiumcodereview.appspot.com/9460002/diff/44001/chrome/renderer/ext...
File chrome/renderer/extensions/schema_generated_bindings.cc (right):

https://chromiumcodereview.appspot.com/9460002/diff/44001/chrome/renderer/ext...
chrome/renderer/extensions/schema_generated_bindings.cc:121: // TODO(kalman):
can we just cache this in the ChromeV8Context instance?
On 2012/03/20 03:32:13, kalman wrote:
> I deleted this TODO because it *was* no longer relevant given we were only
> looking at the available APIs here. However, now they're also being queried in
> extension_dispatcher.cc, so perhaps we should revisit something like this. 
> WDYT?

I can't think of any particularly good reason not to.

https://chromiumcodereview.appspot.com/9460002/diff/44001/chrome/renderer/ext...
chrome/renderer/extensions/schema_generated_bindings.cc:142: // TODO(kalman):
this caching is actually useless now, because
"this caching" -> "the caching in GetV8SchemaForAPI"?

Or change GetV8SchemaForAPI() to GetAndCacheV8SchemaForAPI().

[not at google - send to devlin, 2012-03-20 07:02:05]

https://chromiumcodereview.appspot.com/9460002/diff/44001/chrome/renderer/ext...
File chrome/renderer/extensions/schema_generated_bindings.cc (right):

https://chromiumcodereview.appspot.com/9460002/diff/44001/chrome/renderer/ext...
chrome/renderer/extensions/schema_generated_bindings.cc:121: // TODO(kalman):
can we just cache this in the ChromeV8Context instance?
On 2012/03/20 04:43:05, koz wrote:
> On 2012/03/20 03:32:13, kalman wrote:
> > I deleted this TODO because it *was* no longer relevant given we were only
> > looking at the available APIs here. However, now they're also being queried
in
> > extension_dispatcher.cc, so perhaps we should revisit something like this. 
> > WDYT?
> 
> I can't think of any particularly good reason not to.

Bahhhhhhhhhhhhhhhhhhh turns out this is hard because of the fake extension ID
created in extension_messages_browsertest.cc (ExtensionMessagesOnConnect and
friend(s)). Which need to be known because parts of code is predicated on that
crap.

I'm really really sick of those tests though, once again they're silently
failing for no good reason (well, because I'm forgetting to check for
IsTestExtensionID in some place... grrrrrr).

Ok, I'm going to fix that freaking test tomorrow. Surely it can't be hard to
just create a fake extension then make sure it's added to the extension set??
Surely???

https://chromiumcodereview.appspot.com/9460002/diff/44001/chrome/renderer/ext...
chrome/renderer/extensions/schema_generated_bindings.cc:142: // TODO(kalman):
this caching is actually useless now, because
On 2012/03/20 04:43:05, koz wrote:
> "this caching" -> "the caching in GetV8SchemaForAPI"?
> 
> Or change GetV8SchemaForAPI() to GetAndCacheV8SchemaForAPI().

I've actually fixed this in a follow-up http://codereview.chromium.org/9616055/

(and then messed it up again. but I will un-mess it soon)

[koz (OOO until 15th September), 2012-03-20 23:04:18]

https://chromiumcodereview.appspot.com/9460002/diff/44001/chrome/renderer/ext...
File chrome/renderer/extensions/schema_generated_bindings.cc (right):

https://chromiumcodereview.appspot.com/9460002/diff/44001/chrome/renderer/ext...
chrome/renderer/extensions/schema_generated_bindings.cc:121: // TODO(kalman):
can we just cache this in the ChromeV8Context instance?
On 2012/03/20 07:02:05, kalman wrote:
> On 2012/03/20 04:43:05, koz wrote:
> > On 2012/03/20 03:32:13, kalman wrote:
> > > I deleted this TODO because it *was* no longer relevant given we were only
> > > looking at the available APIs here. However, now they're also being
queried
> in
> > > extension_dispatcher.cc, so perhaps we should revisit something like this.

> > > WDYT?
> > 
> > I can't think of any particularly good reason not to.
> 
> Bahhhhhhhhhhhhhhhhhhh turns out this is hard because of the fake extension ID
> created in extension_messages_browsertest.cc (ExtensionMessagesOnConnect and
> friend(s)). Which need to be known because parts of code is predicated on that
> crap.
> 
> I'm really really sick of those tests though, once again they're silently
> failing for no good reason (well, because I'm forgetting to check for
> IsTestExtensionID in some place... grrrrrr).
> 
> Ok, I'm going to fix that freaking test tomorrow. Surely it can't be hard to
> just create a fake extension then make sure it's added to the extension set??
> Surely???

Surely!

[not at google - send to devlin, 2012-03-21 02:20:57]

Ok, I've made those damn tests pass. Going to try submitting this again and see
where the perf bots end up.

[commit-bot: I haz the power, 2012-03-21 02:21:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/9460002/62001

[commit-bot: I haz the power, 2012-03-21 04:09:25]

Change committed as 127899