Add stub functions for chrome.* APIs in content scripts. ...

Add stub functions for chrome.* APIs in content scripts. 

Most of the extensions API is not supported in content scripts for security 
purposes. Instead the typical pattern is to use messaging between content 
scripts and a background page which executes them on behalf of the script.
This is not immediately obvious to many extension developers, so this
change adds an error message to help them better understand what's going on.


BUG=26128
TEST=Try writing a content script that uses some of the extensions API stuff 
in chrome.tabs, chrome.windows, etc. You should see errors in the console 
telling you that it isn't supported in content scripts.



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

[Aaron Boodman, 2009-11-05 19:10:49]

Drive by

http://codereview.chromium.org/366024/diff/18/2002
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/366024/diff/18/2002#newcode26
Line 26: if (chrome.test && !chrome.test.getAPIDefinition) {
Maybe move this down closer to where the other custom apis are setup?

http://codereview.chromium.org/366024/diff/18/2002#newcode26
Line 26: if (chrome.test && !chrome.test.getAPIDefinition) {
Also, is the check for !chrome.test.getAPIDefinition necessary? Do we come
through here twice?

http://codereview.chromium.org/366024/diff/18/2003
File chrome/renderer/resources/renderer_extension_bindings.js (right):

http://codereview.chromium.org/366024/diff/18/2003#newcode208
Line 208: chrome.setupExtensionApiStubs = function() {
Can you make this a local function and call it from initExtension() instead? I'd
like to have as few of these things hanging off of chrome.* as possible.

http://codereview.chromium.org/366024/diff/18/2003#newcode211
Line 211: var not_supported_suffix = " is not supported in content scripts. " +
Nit: JavaScript style is camelCase.

http://codereview.chromium.org/366024/diff/18/2003#newcode214
Line 214: function not_supported_func(name) {
Camel case. Also, functions should be verbs, and abbreviations are discouraged,
so createNotSupportedFunction?

http://codereview.chromium.org/366024/diff/18/2006
File chrome/test/data/extensions/api_test/stubs/background.html (right):

http://codereview.chromium.org/366024/diff/18/2006#newcode6
Line 6: // this: [["tabs"], ["extension", "getViews"]]. A 1-element array is an
Nit: A more self documenting way to implement this would be something like:

[
  { namespace: "foo" }
  { namespace: "bar",
    names: ["baz", "hotdog"] }
]

But just an idea.

http://codereview.chromium.org/366024/diff/18/2006#newcode8
Line 8: function get_names_to_test() {
camelCase.

http://codereview.chromium.org/366024/diff/18/2006#newcode10
Line 10: var api = JSON.parse(chrome.test.getAPIDefinition());
I like this, elegant way to test this feature.

http://codereview.chromium.org/366024/diff/18/2006#newcode11
Line 11: for (var i in api) {
Nit: "i" is usually used for a counter. "prop" is a common name to use in this
situation.

http://codereview.chromium.org/366024/diff/18/2006#newcode40
Line 40: chrome.extension.onConnect.addListener(function(port) {
Maybe worth using Matt's new onRequest ?

[asargent_no_longer_on_chrome, 2009-11-12 23:17:59]

I ended up rewriting this a bit in the following ways:

-Added a 'unprivileged' property in extensions_api.json, which if missing from a
namespace/function/event/property means that it is assumed to be privileged.

-Made the browser test parse the extensions_api.json and find the names to test
based on the 'unprivileged' 

-Addressed Aaron's review feedback to fix style issues.

I looked into parsing the extension_api.json at runtime to dynamically create
the list of privileged apis to generate stubs for, but the naive approach was
much too slow. The best way would probably be to do something in the build
process and spit out the list somewhere, so I've left that as a future
enhancement if it gets cumbersome to add new apis to the privileged list.

http://codereview.chromium.org/366024/diff/18/2002
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/366024/diff/18/2002#newcode26
Line 26: if (chrome.test && !chrome.test.getAPIDefinition) {
On 2009/11/05 19:10:49, Aaron Boodman wrote:
> Maybe move this down closer to where the other custom apis are setup?

Done.

http://codereview.chromium.org/366024/diff/18/2002#newcode26
Line 26: if (chrome.test && !chrome.test.getAPIDefinition) {
On 2009/11/05 19:10:49, Aaron Boodman wrote:
> Also, is the check for !chrome.test.getAPIDefinition necessary? Do we come
> through here twice?
Nope. Fixed.

http://codereview.chromium.org/366024/diff/18/2003
File chrome/renderer/resources/renderer_extension_bindings.js (right):

http://codereview.chromium.org/366024/diff/18/2003#newcode208
Line 208: chrome.setupExtensionApiStubs = function() {
On 2009/11/05 19:10:49, Aaron Boodman wrote:
> Can you make this a local function and call it from initExtension() instead?
I'd
> like to have as few of these things hanging off of chrome.* as possible.

Done.

http://codereview.chromium.org/366024/diff/18/2003#newcode211
Line 211: var not_supported_suffix = " is not supported in content scripts. " +
On 2009/11/05 19:10:49, Aaron Boodman wrote:
> Nit: JavaScript style is camelCase.

Done.

http://codereview.chromium.org/366024/diff/18/2003#newcode214
Line 214: function not_supported_func(name) {
On 2009/11/05 19:10:49, Aaron Boodman wrote:
> Camel case. Also, functions should be verbs, and abbreviations are
discouraged,
> so createNotSupportedFunction?

changed to createStub()

http://codereview.chromium.org/366024/diff/18/2006
File chrome/test/data/extensions/api_test/stubs/background.html (right):

http://codereview.chromium.org/366024/diff/18/2006#newcode6
Line 6: // this: [["tabs"], ["extension", "getViews"]]. A 1-element array is an
On 2009/11/05 19:10:49, Aaron Boodman wrote:
> Nit: A more self documenting way to implement this would be something like:
> 
> [
>   { namespace: "foo" }
>   { namespace: "bar",
>     names: ["baz", "hotdog"] }
> ]
> 
> But just an idea.

Changed to just using a flat list of dot-separated strings, which turned out to
be simpler for other reasons anyway.

http://codereview.chromium.org/366024/diff/18/2006#newcode8
Line 8: function get_names_to_test() {
On 2009/11/05 19:10:49, Aaron Boodman wrote:
> camelCase.

Done.

http://codereview.chromium.org/366024/diff/18/2006#newcode11
Line 11: for (var i in api) {
On 2009/11/05 19:10:49, Aaron Boodman wrote:
> Nit: "i" is usually used for a counter. "prop" is a common name to use in this
> situation.

Done.

http://codereview.chromium.org/366024/diff/18/2006#newcode40
Line 40: chrome.extension.onConnect.addListener(function(port) {
On 2009/11/05 19:10:49, Aaron Boodman wrote:
> Maybe worth using Matt's new onRequest ?

Done.

[rafaelw, 2009-11-13 02:44:25]

lgtm

http://codereview.chromium.org/366024/diff/8001/9002
File chrome/renderer/resources/renderer_extension_bindings.js (right):

http://codereview.chromium.org/366024/diff/8001/9002#newcode212
Line 212: function createStub(name) {
One idea: I think this could more compactly be implemented recursively:

function createStub(anchor, name) {
  var parts = name.split(".");
  if (parts.length == 1) {
    anchor[name].__defineGetter__(name, function() { ... });
  } else {
    var next = parts[0];
    anchor[next] = anchor[next] || {};
    createStub(anchor[next], name.substring(next.length + 1));
  }
}

and call createStub(chrome, privileged[i]) below.

just a suggestion.

http://codereview.chromium.org/366024/diff/8001/9002#newcode239
Line 239: 
nit: remove one newline here.

http://codereview.chromium.org/366024/diff/8001/9008
File chrome/test/data/extensions/api_test/stubs/content_script.js (right):

http://codereview.chromium.org/366024/diff/8001/9008#newcode9
Line 9: for (var moduleIndex in apis) {
array forEach is now supported in v8. use

apis.forEach(function(module) {
});

http://codereview.chromium.org/366024/diff/8001/9008#newcode17
Line 17: for (var section in {"functions":1, "events":1}) {
["functions", "events"].forEach(function(section) {
});

http://codereview.chromium.org/366024/diff/8001/9008#newcode20
Line 20: for (var i in module[section]) {
module[section].forEach(function(functionOrEvent) {

});

http://codereview.chromium.org/366024/diff/8001/9008#newcode23
Line 23: if ("unprivileged" in entry) {
if (entry.unprivileged)

http://codereview.chromium.org/366024/diff/8001/9008#newcode31
Line 31: if ("properties" in module) {
if (module.properties)

http://codereview.chromium.org/366024/diff/8001/9008#newcode34
Line 34: if ("unprivileged" in module.properties[propName]) {
module.properties[propName].unprivileged

http://codereview.chromium.org/366024/diff/8001/9008#newcode133
Line 133: for (var i = 0; i < privilegedPaths.length; i++) {
privilegedPaths.forEach

http://codereview.chromium.org/366024/diff/8001/9008#newcode140
Line 140: for (var i = 0; i < unprivilegedPaths.length; i++) {
unprivilegedPaths.forEach

[asargent_no_longer_on_chrome, 2009-11-13 19:40:30]

http://codereview.chromium.org/366024/diff/8001/9002
File chrome/renderer/resources/renderer_extension_bindings.js (right):

http://codereview.chromium.org/366024/diff/8001/9002#newcode212
Line 212: function createStub(name) {
On 2009/11/13 02:44:25, rafaelw wrote:
> One idea: I think this could more compactly be implemented recursively:
> 
> function createStub(anchor, name) {
>   var parts = name.split(".");
>   if (parts.length == 1) {
>     anchor[name].__defineGetter__(name, function() { ... });
>   } else {
>     var next = parts[0];
>     anchor[next] = anchor[next] || {};
>     createStub(anchor[next], name.substring(next.length + 1));
>   }
> }
> 
> and call createStub(chrome, privileged[i]) below.
> 
> just a suggestion.

I tried this but the recursive version is actually a little more complicated
than your sample code because of needing to get the error output correct (you'd
need a third parameter or something to build up the error string for eg
chrome.extension.getBackgroundPage). So, leaving it as is.

http://codereview.chromium.org/366024/diff/8001/9002#newcode239
Line 239: 
On 2009/11/13 02:44:25, rafaelw wrote:
> nit: remove one newline here.

Done.

http://codereview.chromium.org/366024/diff/8001/9008
File chrome/test/data/extensions/api_test/stubs/content_script.js (right):

http://codereview.chromium.org/366024/diff/8001/9008#newcode9
Line 9: for (var moduleIndex in apis) {
On 2009/11/13 02:44:25, rafaelw wrote:
> array forEach is now supported in v8. use
> 
> apis.forEach(function(module) {
> });

Done.

http://codereview.chromium.org/366024/diff/8001/9008#newcode17
Line 17: for (var section in {"functions":1, "events":1}) {
On 2009/11/13 02:44:25, rafaelw wrote:
> ["functions", "events"].forEach(function(section) {
> });
That is much cleaner. Done.

http://codereview.chromium.org/366024/diff/8001/9008#newcode20
Line 20: for (var i in module[section]) {
On 2009/11/13 02:44:25, rafaelw wrote:
> module[section].forEach(function(functionOrEvent) {
> 
> });

Done.

http://codereview.chromium.org/366024/diff/8001/9008#newcode23
Line 23: if ("unprivileged" in entry) {
On 2009/11/13 02:44:25, rafaelw wrote:
> if (entry.unprivileged)

Done.

http://codereview.chromium.org/366024/diff/8001/9008#newcode31
Line 31: if ("properties" in module) {
On 2009/11/13 02:44:25, rafaelw wrote:
> if (module.properties)

Done.

http://codereview.chromium.org/366024/diff/8001/9008#newcode34
Line 34: if ("unprivileged" in module.properties[propName]) {
On 2009/11/13 02:44:25, rafaelw wrote:
> module.properties[propName].unprivileged

Done.

http://codereview.chromium.org/366024/diff/8001/9008#newcode133
Line 133: for (var i = 0; i < privilegedPaths.length; i++) {
On 2009/11/13 02:44:25, rafaelw wrote:
> privilegedPaths.forEach

Done.

http://codereview.chromium.org/366024/diff/8001/9008#newcode140
Line 140: for (var i = 0; i < unprivilegedPaths.length; i++) {
On 2009/11/13 02:44:25, rafaelw wrote:
> unprivilegedPaths.forEach
> 

Done.

[Aaron Boodman, 2009-11-13 21:12:54]

Defer to raf.