Introduce background.scripts feature for extension manifests.

Introduce background.scripts feature for extension manifests.

This optimizes for the common use case where background pages
just include a reference to one or more script files and no
additional HTML.

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

[Aaron Boodman, 2012-01-09 20:15:48]

I will also update the docs in a separate change.

http://codereview.chromium.org/9150008/diff/1/chrome/test/data/extensions/api...
File chrome/test/data/extensions/api_test/alert/manifest.json (right):

http://codereview.chromium.org/9150008/diff/1/chrome/test/data/extensions/api...
chrome/test/data/extensions/api_test/alert/manifest.json:7: "scripts":
["test.js"]
I will convert the rest of these in a separate change.

[Matt Perry, 2012-01-09 21:12:17]

lgtm

http://codereview.chromium.org/9150008/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_protocols.cc (right):

http://codereview.chromium.org/9150008/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_protocols.cc:105: class
GeneratedBackgroundPageJob : public net::URLRequestSimpleJob {
Please add a class comment

http://codereview.chromium.org/9150008/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/9150008/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.cc:382:
extension_filenames::kGeneratedBackgroundPageFilename);
nit: braces around multi-line if/else

http://codereview.chromium.org/9150008/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.cc:1284: if (!background_scripts_.empty())
Maybe it should be an error to have both a background.scripts and
background.page key.

[commit-bot: I haz the power, 2012-01-10 01:25:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aa@chromium.org/9150008/10001

[Mihai Parparita -not on Chrome, 2012-01-10 01:51:37]

http://codereview.chromium.org/9150008/diff/10001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/9150008/diff/10001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:1222:
background_scripts_.push_back(script);
Drive by: Do you want to validate that these are relative paths only? It looks
like "background": {"scripts": ["http://www.mysite.com/script.js"]] will result
in a <script> tag pointed at that URL being generated. Though maybe that's OK,
because of CSP?

[Mihai Parparita -not on Chrome, 2012-01-10 01:53:37]

Actually, I'm not sure if CSP will prevent that script from being loaded,
since this manifest feature is available to v1 too.

Mihai

On Mon, Jan 9, 2012 at 5:51 PM, <mihaip@chromium.org> wrote:

>
> http://codereview.chromium.**org/9150008/diff/10001/chrome/**
>
common/extensions/extension.cc<http://codereview.chromium.org/9150008/diff/10001/chrome/common/extensions/extension.cc>
> File chrome/common/extensions/**extension.cc (right):
>
> http://codereview.chromium.**org/9150008/diff/10001/chrome/**
>
common/extensions/extension.**cc#newcode1222<http://codereview.chromium.org/9150008/diff/10001/chrome/common/extensions/extension.cc#newcode1222>
> chrome/common/extensions/**extension.cc:1222:
> background_scripts_.push_back(**script);
> Drive by: Do you want to validate that these are relative paths only? It
> looks like "background": {"scripts":
> ["http://www.mysite.com/**script.js <http://www.mysite.com/script.js>"]]
> will result in a <script> tag
> pointed at that URL being generated. Though maybe that's OK, because of
> CSP?
>
>
http://codereview.chromium.**org/9150008/<http://codereview.chromium.org/9150...
>

[commit-bot: I haz the power, 2012-01-10 02:38:34]

List of reviewers changed. mihaip@chromium.org did a drive-by without LGTM'ing!

[Aaron Boodman, 2012-01-10 20:11:51]

Good call. Fixed.

[Mihai Parparita -not on Chrome, 2012-01-10 20:15:04]

LGTM

[commit-bot: I haz the power, 2012-01-10 21:10:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aa@chromium.org/9150008/4007

[commit-bot: I haz the power, 2012-01-10 23:25:35]

Change committed as 117110