Extensions: run "custom bindings" v8-extensions in content scripts.

Extensions: run "custom bindings" v8-extensions in content scripts.

Currently they're only run in "privileged" extension processes (e.g. background pages).

As part of this, plug some places where API bindings are set up unnecessarily in content scripts: firstly in ExtensionDispatcher, where the "custom bindings" v8-extensions are allowed to run in content scripts, and secondly in SchemaGeneratedBindings where schemas for privileged APIs are sent to content scripts.


BUG=80310
TEST=browser_tests


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

[not at google - send to devlin, 2012-02-15 07:50:51]

P.S. There are some really nice patches coming after this.

[koz (OOO until 15th September), 2012-02-15 23:43:11]

lgtm

https://chromiumcodereview.appspot.com/9403006/diff/6001/chrome/common/extens...
File chrome/common/extensions/api/extension_api.h (right):

https://chromiumcodereview.appspot.com/9403006/diff/6001/chrome/common/extens...
chrome/common/extensions/api/extension_api.h:43: // Gets whether *every* path in
the API is privileged. This will be false for
nit: "Gets whether" -> "Returns whether"

https://chromiumcodereview.appspot.com/9403006/diff/6001/chrome/renderer/exte...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/9403006/diff/6001/chrome/renderer/exte...
chrome/renderer/extensions/extension_dispatcher.cc:281: // There is a patch in
progress (koz) to properly plumb this through.
TODO(koz): Plumb extension_group through to AllowScriptExtension() from WebKit.

https://chromiumcodereview.appspot.com/9403006/diff/6001/chrome/renderer/exte...
chrome/renderer/extensions/extension_dispatcher.cc:332: custom_binding_api_name,
*extension, is_content_script);
(aside) It'll be nice when we can factor this logic out so it becomes possible
to write tests like this:

CheckInjection(CONTENT_SCRIPT, NON_BLESSED_URL,
&apis_extension_has_permission_for, "storage", false);

Which would enable you to test the plugging up done by this change.

[not at google - send to devlin, 2012-02-16 00:06:27]

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

http://codereview.chromium.org/9403006/diff/6001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.h:43: // Gets whether *every* path in
the API is privileged. This will be false for
On 2012/02/15 23:43:11, koz wrote:
> nit: "Gets whether" -> "Returns whether"

Done.

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

http://codereview.chromium.org/9403006/diff/6001/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_dispatcher.cc:281: // There is a patch in
progress (koz) to properly plumb this through.
On 2012/02/15 23:43:11, koz wrote:
> TODO(koz): Plumb extension_group through to AllowScriptExtension() from
WebKit.

Done.

http://codereview.chromium.org/9403006/diff/6001/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_dispatcher.cc:332: custom_binding_api_name,
*extension, is_content_script);
On 2012/02/15 23:43:11, koz wrote:
> (aside) It'll be nice when we can factor this logic out so it becomes possible
> to write tests like this:
> 
> CheckInjection(CONTENT_SCRIPT, NON_BLESSED_URL,
> &apis_extension_has_permission_for, "storage", false);
> 
> Which would enable you to test the plugging up done by this change.

Yeah I'm hoping/expecting that once your require() change(s) go in we'll be able
to centralise the logic here and in SchemaGeneratedBindings (which are quite
simple, but I couldn't come up with a convenient/sensible way to share it with
the current structure of the code, and it's not worth refactoring since it's all
about to change soon anyway).

[Aaron Boodman, 2012-02-16 00:17:13]

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

http://codereview.chromium.org/9403006/diff/6001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.cc:184: std::string api_name;
This is going to be replaced with Feature stuff eventually, right?

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

http://codereview.chromium.org/9403006/diff/6001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.h:40: // content scripts, or other
low-privileged processes.
nit: last word would be better as "contexts".

http://codereview.chromium.org/9403006/diff/6001/chrome/renderer/extensions/c...
File chrome/renderer/extensions/chrome_v8_context.cc (right):

http://codereview.chromium.org/9403006/diff/6001/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_v8_context.cc:38: << "  frame: " <<
web_frame_;
Add the context_script bit to this log line?

http://codereview.chromium.org/9403006/diff/6001/chrome/renderer/extensions/c...
File chrome/renderer/extensions/chrome_v8_context_set_unittest.cc (right):

http://codereview.chromium.org/9403006/diff/6001/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_v8_context_set_unittest.cc:25: new
ChromeV8Context(v8_context, frame, extension_id, false);
Boolean flags stink for readability. Consider adding a ContextType enum. Current
value: frame, content script. In the future there could also be "worker".

[not at google - send to devlin, 2012-02-16 01:45:53]

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

http://codereview.chromium.org/9403006/diff/6001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.cc:184: std::string api_name;
On 2012/02/16 00:17:13, Aaron Boodman wrote:
> This is going to be replaced with Feature stuff eventually, right?

mos def.

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

http://codereview.chromium.org/9403006/diff/6001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.h:40: // content scripts, or other
low-privileged processes.
On 2012/02/16 00:17:13, Aaron Boodman wrote:
> nit: last word would be better as "contexts".

Done.

http://codereview.chromium.org/9403006/diff/6001/chrome/renderer/extensions/c...
File chrome/renderer/extensions/chrome_v8_context.cc (right):

http://codereview.chromium.org/9403006/diff/6001/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_v8_context.cc:38: << "  frame: " <<
web_frame_;
On 2012/02/16 00:17:13, Aaron Boodman wrote:
> Add the context_script bit to this log line?

Done.

http://codereview.chromium.org/9403006/diff/6001/chrome/renderer/extensions/c...
File chrome/renderer/extensions/chrome_v8_context_set_unittest.cc (right):

http://codereview.chromium.org/9403006/diff/6001/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_v8_context_set_unittest.cc:25: new
ChromeV8Context(v8_context, frame, extension_id, false);
On 2012/02/16 00:17:13, Aaron Boodman wrote:
> Boolean flags stink for readability. Consider adding a ContextType enum.
Current
> value: frame, content script. In the future there could also be "worker".

Done.

I'm not sure it's entirely right though. In ExtensionDispatcher where this is
created, there are more than just content scripts and frames, right? Currently
frame -> "other".

[Aaron Boodman, 2012-02-16 02:01:52]

lgtm

http://codereview.chromium.org/9403006/diff/6001/chrome/renderer/extensions/c...
File chrome/renderer/extensions/chrome_v8_context_set_unittest.cc (right):

http://codereview.chromium.org/9403006/diff/6001/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_v8_context_set_unittest.cc:25: new
ChromeV8Context(v8_context, frame, extension_id, false);
On 2012/02/16 01:45:53, kalman wrote:
> On 2012/02/16 00:17:13, Aaron Boodman wrote:
> > Boolean flags stink for readability. Consider adding a ContextType enum.
> Current
> > value: frame, content script. In the future there could also be "worker".
> 
> Done.
> 
> I'm not sure it's entirely right though. In ExtensionDispatcher where this is
> created, there are more than just content scripts and frames, right? Currently
> frame -> "other".

That's a good point. I'm not sure if this is called for workers. You can rename
it to other if it pleases you. Or NotContentScript :).

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

http://codereview.chromium.org/9403006/diff/1030/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_v8_context.h:55: bool context_type() const {
"Ha ha! Got you!"

signed,
C++

[commit-bot: I haz the power, 2012-02-16 03:05:36]

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

[commit-bot: I haz the power, 2012-02-16 05:32:50]

Change committed as 122243