Add support for redirecting ResourceHandlers to a blob: URL

Add support for redirecting ResourceHandlers to a blob: URL

This allows us to redirect a connection to a source that can be read from an
extension that has been registered as a mime type handler.

BUG=171585

[Zachary Kuznia, 2013-02-06 12:01:39]

Hi Darin,

This is the updated patch that hooks the Stream up to a blob: URL.

Please take a look.

Thanks,
-Zach

[darin (slow to review), 2013-02-13 09:18:12]

First pass of feedback... sorry again for the delays!!

https://codereview.chromium.org/12212031/diff/7003/content/browser/loader/str...
File content/browser/loader/stream_resource_handler.cc (right):

https://codereview.chromium.org/12212031/diff/7003/content/browser/loader/str...
content/browser/loader/stream_resource_handler.cc:83: if
(stream_->should_defer()) {
nit: instead of should_defer(), I'd use a name that is more connected to the
AddData() method.  You want to ask the question: "Can I add more data?"  So, a
method name like CanAddData would work.

https://codereview.chromium.org/12212031/diff/7003/content/browser/resource_c...
File content/browser/resource_context_impl.h (right):

https://codereview.chromium.org/12212031/diff/7003/content/browser/resource_c...
content/browser/resource_context_impl.h:13: class ChromeStreamContext;
I see where you got the Chrome* prefix from.  I suspect this may be a
side-effect of the Blob code originally existing in src/chrome.  I think it was
moved into src/content because it is a core feature of the platform.  There is
probably a TODO to drop the prefix.

https://codereview.chromium.org/12212031/diff/7003/content/browser/storage_pa...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/12212031/diff/7003/content/browser/storage_pa...
content/browser/storage_partition_impl_map.cc:102: } else {
nit: no need for else after return.

https://codereview.chromium.org/12212031/diff/7003/content/browser/streams/ch...
File content/browser/streams/chrome_stream.cc (right):

https://codereview.chromium.org/12212031/diff/7003/content/browser/streams/ch...
content/browser/streams/chrome_stream.cc:52: int buf_size,
it feels like there is some redundancy here with ByteStream{Reader,Writer}:

https://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/download/...

https://codereview.chromium.org/12212031/diff/7003/content/browser/streams/ch...
File content/browser/streams/chrome_stream.h (right):

https://codereview.chromium.org/12212031/diff/7003/content/browser/streams/ch...
content/browser/streams/chrome_stream.h:23: class ChromeStreamObserver {
nit: We don't usually name classes with a Chrome* prefix.  I realize this was
probably to match the "chrome-stream:" URL scheme we had discussed.

nit: I recommend creating a separate header file for the *Observer class.  There
will be cases where someone will only need to include the Observer interface
definition in their header file.

https://codereview.chromium.org/12212031/diff/7003/content/browser/streams/ch...
content/browser/streams/chrome_stream.h:48: void
AddObserver(ChromeStreamObserver* observer);
It seems like the observer interface is used both to notify the producer of data
that they can add more data as well as to notify the consumer(s) of data that
they can get more data.  But, given the signature of ReadRawData, I'm not sure
that this class really supports multiple readers.  Plus, I remember in our
discussions you said the stream would only have one reader because it was
intended to be temporary.

It'd be good to document the intended processing model in this header file.

I might look at separating the consumer notifications from the producer
notifications.  It seems potentially awkward to have those notifications merged
into a single interface.

https://codereview.chromium.org/12212031/diff/7003/content/browser/streams/ch...
File content/browser/streams/chrome_stream_registry.cc (right):

https://codereview.chromium.org/12212031/diff/7003/content/browser/streams/ch...
content/browser/streams/chrome_stream_registry.cc:45: void
ChromeStreamRegistry::OnStreamComplete(ChromeStream* stream) {
so does OnStreamComplete mean that the stream was fully consumed?  maybe the
name of this event should be a bit more specific.  OnStreamConsumed?

however, if you were to do this without the general purpose Observer interface,
then it might just make sense to have the Stream class directly call a function
on the StreamRegistry to unregister the stream.

[Zachary Kuznia, 2013-02-13 16:37:14]

https://codereview.chromium.org/12212031/diff/7003/content/browser/loader/str...
File content/browser/loader/stream_resource_handler.cc (right):

https://codereview.chromium.org/12212031/diff/7003/content/browser/loader/str...
content/browser/loader/stream_resource_handler.cc:83: if
(stream_->should_defer()) {
On 2013/02/13 09:18:12, darin wrote:
> nit: instead of should_defer(), I'd use a name that is more connected to the
> AddData() method.  You want to ask the question: "Can I add more data?"  So, a
> method name like CanAddData would work.

Done.

https://codereview.chromium.org/12212031/diff/7003/content/browser/resource_c...
File content/browser/resource_context_impl.h (right):

https://codereview.chromium.org/12212031/diff/7003/content/browser/resource_c...
content/browser/resource_context_impl.h:13: class ChromeStreamContext;
On 2013/02/13 09:18:12, darin wrote:
> I see where you got the Chrome* prefix from.  I suspect this may be a
> side-effect of the Blob code originally existing in src/chrome.  I think it
was
> moved into src/content because it is a core feature of the platform.  There is
> probably a TODO to drop the prefix.

bufdo %s/ChromeStream/Stream/g

https://codereview.chromium.org/12212031/diff/7003/content/browser/storage_pa...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/12212031/diff/7003/content/browser/storage_pa...
content/browser/storage_partition_impl_map.cc:102: } else {
On 2013/02/13 09:18:12, darin wrote:
> nit: no need for else after return.

Done.

https://codereview.chromium.org/12212031/diff/7003/content/browser/streams/ch...
File content/browser/streams/chrome_stream.cc (right):

https://codereview.chromium.org/12212031/diff/7003/content/browser/streams/ch...
content/browser/streams/chrome_stream.cc:52: int buf_size,
On 2013/02/13 09:18:12, darin wrote:
> it feels like there is some redundancy here with ByteStream{Reader,Writer}:
> 
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/download/...

Modified this class to use ByteStream.

https://codereview.chromium.org/12212031/diff/7003/content/browser/streams/ch...
File content/browser/streams/chrome_stream.h (right):

https://codereview.chromium.org/12212031/diff/7003/content/browser/streams/ch...
content/browser/streams/chrome_stream.h:23: class ChromeStreamObserver {
On 2013/02/13 09:18:12, darin wrote:
> nit: We don't usually name classes with a Chrome* prefix.  I realize this was
> probably to match the "chrome-stream:" URL scheme we had discussed.
> 
> nit: I recommend creating a separate header file for the *Observer class. 
There
> will be cases where someone will only need to include the Observer interface
> definition in their header file.

Done.

https://codereview.chromium.org/12212031/diff/7003/content/browser/streams/ch...
content/browser/streams/chrome_stream.h:48: void
AddObserver(ChromeStreamObserver* observer);
On 2013/02/13 09:18:12, darin wrote:
> It seems like the observer interface is used both to notify the producer of
data
> that they can add more data as well as to notify the consumer(s) of data that
> they can get more data.  But, given the signature of ReadRawData, I'm not sure
> that this class really supports multiple readers.  Plus, I remember in our
> discussions you said the stream would only have one reader because it was
> intended to be temporary.
> 
> It'd be good to document the intended processing model in this header file.
> 
> I might look at separating the consumer notifications from the producer
> notifications.  It seems potentially awkward to have those notifications
merged
> into a single interface.

Split the observers into StreamReadObserver and StreamWriteObserver.
Moved observers into separate files
Added comments to describe this class

https://codereview.chromium.org/12212031/diff/7003/content/browser/streams/ch...
File content/browser/streams/chrome_stream_registry.cc (right):

https://codereview.chromium.org/12212031/diff/7003/content/browser/streams/ch...
content/browser/streams/chrome_stream_registry.cc:45: void
ChromeStreamRegistry::OnStreamComplete(ChromeStream* stream) {
On 2013/02/13 09:18:12, darin wrote:
> so does OnStreamComplete mean that the stream was fully consumed?  maybe the
> name of this event should be a bit more specific.  OnStreamConsumed?
> 
> however, if you were to do this without the general purpose Observer
interface,
> then it might just make sense to have the Stream class directly call a
function
> on the StreamRegistry to unregister the stream.

Changed to OnStreamConsumed and now this is called directly.