Allow <audio> to work in extension

Allow <audio> to work in extension

BUG=22152
TEST=use <audio> extension with relative paths

We used to exclude file:// for checks for valid HTTP response but
since chrome extension resources use chrome-extension:// as scheme
response from such scheme require valid HTTP response.

This change changes the logic to limit the check for valid HTTP
response only for http:// and https:// schemes. This impose a
strong assumption that file://, ftp:// and new protocols need to
know how to handle range requests or at least know how to fail.

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

[Alpha Left Google, 2009-09-18 08:11:26]

bug for ftp:// filed as http://crbug.com/22231

[awong, 2009-09-18 19:13:58]

Still have some concerns.

http://codereview.chromium.org/216022/diff/3001/4001
File chrome/renderer/renderer_glue.cc (right):

http://codereview.chromium.org/216022/diff/3001/4001#newcode216
Line 216: if (url.SchemeIsFile() || url.SchemeIs("http") ||
url.SchemeIs("https") ||
We don't support ftp?

http://codereview.chromium.org/216022/diff/3001/4001#newcode217
Line 217: url.SchemeIs("chrome-extension"))
aren't there constants for these strings?

http://codereview.chromium.org/216022/diff/3001/4002
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/216022/diff/3001/4002#newcode212
Line 212: Stop();
Should this still return true?  When can this function return false?

http://codereview.chromium.org/216022/diff/3001/4002#newcode231
Line 231: // response for HTTP and HTTPS protocol.
I like that we have a specific filter of supported protocols, but I'm still
uncomfortable with the current code structure because we are not strongly tying
the condition for when we need to verify data and what data is permitted
through.

I would prefer a full white-list here in the deep validation code.  The cost of
accidentally adding a new scheme into the IsProtocolSupportedForMedia, but
forgetting to add validation is possibly fairly large, so we should make it hard
to do that via code structure.

[Alpha Left Google, 2009-09-18 22:14:33]

http://codereview.chromium.org/216022/diff/3001/4001
File chrome/renderer/renderer_glue.cc (right):

http://codereview.chromium.org/216022/diff/3001/4001#newcode216
Line 216: if (url.SchemeIsFile() || url.SchemeIs("http") ||
url.SchemeIs("https") ||
On 2009/09/18 19:13:59, awong wrote:
> We don't support ftp?

No, we don't support ftp.

http://codereview.chromium.org/216022/diff/3001/4001#newcode217
Line 217: url.SchemeIs("chrome-extension"))
On 2009/09/18 19:13:59, awong wrote:
> aren't there constants for these strings?

Done.

http://codereview.chromium.org/216022/diff/3001/4002
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/216022/diff/3001/4002#newcode212
Line 212: Stop();
On 2009/09/18 19:13:59, awong wrote:
> Should this still return true?  When can this function return false?

Done.

http://codereview.chromium.org/216022/diff/3001/4002#newcode231
Line 231: // response for HTTP and HTTPS protocol.
On 2009/09/18 19:13:59, awong wrote:
> I like that we have a specific filter of supported protocols, but I'm still
> uncomfortable with the current code structure because we are not strongly
tying
> the condition for when we need to verify data and what data is permitted
> through.
> 
> I would prefer a full white-list here in the deep validation code.  The cost
of
> accidentally adding a new scheme into the IsProtocolSupportedForMedia, but
> forgetting to add validation is possibly fairly large, so we should make it
hard
> to do that via code structure.

By doing this, we would be adding a bit of protocol checks in the glue code that
similar to
IsProtocolTrustedForMedia() or something like that..

I fear that:
1. We are splitting the logic a bit too much
2. Complicating the glue layer to knows a bit internal of the media engine

Consider that the check is used to guard only HTTP/HTTPS protocol, we should be
specific about these two protocols. And when we add new protocols like maybe
rtsp://, the validation would be completely different. Given that adding a white
list for "not validating HTTP headers" seems to be doing exactly the same thing
here except less compatible for furture changes. And if we fear changing
IsProtocolSupportedForMedia() would cause harm, we can add extra comments there
to explain.

[scherkus (not reviewing), 2009-09-18 22:23:17]

http://codereview.chromium.org/216022/diff/3006/4006
File chrome/renderer/renderer_glue.cc (right):

http://codereview.chromium.org/216022/diff/3006/4006#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
copyright year

http://codereview.chromium.org/216022/diff/3006/4007
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/216022/diff/3006/4007#newcode493
Line 493: // Saves the url.
saves -> save

http://codereview.chromium.org/216022/diff/3006/4008
File webkit/glue/webkit_glue.h (right):

http://codereview.chromium.org/216022/diff/3006/4008#newcode202
Line 202: bool IsProtocolSupportedForMedia(const GURL& url);
function/comment mismatch

perhaps it should be renamed IsRangeRequestSupportedForProtocol() ?

[Alpha Left Google, 2009-09-18 22:56:10]

http://codereview.chromium.org/216022/diff/3006/4008
File webkit/glue/webkit_glue.h (right):

http://codereview.chromium.org/216022/diff/3006/4008#newcode202
Line 202: bool IsProtocolSupportedForMedia(const GURL& url);
On 2009/09/18 22:23:17, scherkus wrote:
> function/comment mismatch
> 
> perhaps it should be renamed IsRangeRequestSupportedForProtocol() ?

I tend to keep it as is.. it is used to guard un-wanted schemes for media, I
should change the comment to drop the range request part.

[scherkus (not reviewing), 2009-09-19 00:58:25]

LGTM