Add Access-Control-Allow-Origin: * header for extension resources specifically listed as web acce...

Add Access-Control-Allow-Origin: * header for extension resources specifically listed as web accessible.

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

[Aaron Boodman, 2012-01-10 23:40:33]

You need a test.

I am not sure if it is possible to test this in a unit test environment. Matt,
do you?

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

http://codereview.chromium.org/9152022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_protocols.cc:50: raw_headers.append(1,
'\0');
What's this?

[Matt Perry, 2012-01-10 23:47:24]

As for unit testing, I think it should be possible to register
ExtensionProtocolFactory and create a dummy URLRequest and watch what happens.

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

http://codereview.chromium.org/9152022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_protocols.cc:40: const std::string&
content_security_policy, const bool send_cors_header) {
nit: don't const-ify parameters that are POD types like bool.

[Cris Neckar, 2012-01-26 00:48:26]

On 2012/01/10 23:47:24, Matt Perry wrote:
> As for unit testing, I think it should be possible to register
> ExtensionProtocolFactory and create a dummy URLRequest and watch what happens.
> 
>
http://codereview.chromium.org/9152022/diff/1/chrome/browser/extensions/exten...
> File chrome/browser/extensions/extension_protocols.cc (right):
> 
>
http://codereview.chromium.org/9152022/diff/1/chrome/browser/extensions/exten...
> chrome/browser/extensions/extension_protocols.cc:40: const std::string&
> content_security_policy, const bool send_cors_header) {
> nit: don't const-ify parameters that are POD types like bool.

This will need to wait on https://bugs.webkit.org/show_bug.cgi?id=77041 as there
was a check preventing CORS requests for anything other than http schemes. Ill
submit a try for this once that lands and webkit rolls.

[Cris Neckar, 2012-01-26 00:48:43]

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

http://codereview.chromium.org/9152022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_protocols.cc:40: const std::string&
content_security_policy, const bool send_cors_header) {
On 2012/01/10 23:47:24, Matt Perry wrote:
> nit: don't const-ify parameters that are POD types like bool.

Done.

http://codereview.chromium.org/9152022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_protocols.cc:50: raw_headers.append(1,
'\0');
On 2012/01/10 23:40:33, Aaron Boodman wrote:
> What's this?

newline

[abarth-chromium, 2012-01-27 01:12:30]

LGTM (not sure if you want someone to do a more official review)

[Cris Neckar, 2012-01-27 01:19:46]

On 2012/01/27 01:12:30, abarth wrote:
> LGTM (not sure if you want someone to do a more official review)

Aaron should probably OK this as well.

[Aaron Boodman, 2012-01-27 22:36:46]

lgtm

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

http://codereview.chromium.org/9152022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_protocols.cc:50: raw_headers.append(1,
'\0');
On 2012/01/26 00:48:44, Cris Neckar wrote:
> On 2012/01/10 23:40:33, Aaron Boodman wrote:
> > What's this?
> 
> newline

Oh.

http://codereview.chromium.org/9152022/diff/15002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_protocols.cc (right):

http://codereview.chromium.org/9152022/diff/15002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_protocols.cc:118: response_info_.headers =
BuildHttpHeaders(content_security_policy, false);
Nit: use a named constant here for readability.

http://codereview.chromium.org/9152022/diff/15002/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/xhr_accessible_resource.html
(right):

http://codereview.chromium.org/9152022/diff/15002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/xhr_accessible_resource.html:1:
<script>
license header - we're inconsistent about this for test files, but I try to
remember.

http://codereview.chromium.org/9152022/diff/15002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/xhr_accessible_resource.html:4:
req.open("GET", "chrome-extension://ggmldgjhdenlnjjjmehkomheglpmijnf/test.png",
false);
80 cols

[Cris Neckar, 2012-01-27 22:58:23]

http://codereview.chromium.org/9152022/diff/15002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_protocols.cc (right):

http://codereview.chromium.org/9152022/diff/15002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_protocols.cc:118: response_info_.headers =
BuildHttpHeaders(content_security_policy, false);
On 2012/01/27 22:36:46, Aaron Boodman wrote:
> Nit: use a named constant here for readability.

Done.

http://codereview.chromium.org/9152022/diff/15002/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/xhr_accessible_resource.html
(right):

http://codereview.chromium.org/9152022/diff/15002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/xhr_accessible_resource.html:1:
<script>
On 2012/01/27 22:36:46, Aaron Boodman wrote:
> license header - we're inconsistent about this for test files, but I try to
> remember.

Done.

http://codereview.chromium.org/9152022/diff/15002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/xhr_accessible_resource.html:4:
req.open("GET", "chrome-extension://ggmldgjhdenlnjjjmehkomheglpmijnf/test.png",
false);
On 2012/01/27 22:36:46, Aaron Boodman wrote:
> 80 cols

Done.

[Aaron Boodman, 2012-01-28 01:16:57]

LGTM

[commit-bot: I haz the power, 2012-01-29 19:51:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cdn@chromium.org/9152022/21002

[commit-bot: I haz the power, 2012-01-29 21:43:26]

Change committed as 119646