Add Resource Handler for creating Streams to forward to extensions

Add Resource Handler for creating Streams to forward to extensions

BUG=171585
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189172

[Zachary Kuznia, 2013-03-13 09:06:49]

Please take a look.

Thanks,
-Zach

[kinuko, 2013-03-13 11:05:59]

Did a shallow review. The area's not very familiar to me but looks sane to me
(with my limited knowledge in the area)

https://codereview.chromium.org/12645004/diff/5001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/12645004/diff/5001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:99: //
Goes through the extension's file browser handlers and checks it there is one
nit: it -> if

https://codereview.chromium.org/12645004/diff/5001/content/browser/loader/res...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/12645004/diff/5001/content/browser/loader/res...
content/browser/loader/resource_request_info_impl.h:55: bool is_stream,
nit: it's probably ok since we already have is_download but in general adding
new bool parameters wouldn't be preferable

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

https://codereview.chromium.org/12645004/diff/5001/content/browser/loader/str...
content/browser/loader/stream_resource_handler.cc:67: }
nit: no need of { } for one-line body

https://codereview.chromium.org/12645004/diff/5001/content/browser/loader/str...
content/browser/loader/stream_resource_handler.cc:91: }
nit: ditto for { }

https://codereview.chromium.org/12645004/diff/5001/content/browser/loader/str...
content/browser/loader/stream_resource_handler.cc:101: if (urs.status() !=
net::URLRequestStatus::SUCCESS)
nit: return urs.status() == net::URLRequestStatus::SUCCESS

?

[Zachary Kuznia, 2013-03-13 11:13:10]

https://codereview.chromium.org/12645004/diff/5001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/12645004/diff/5001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:99: //
Goes through the extension's file browser handlers and checks it there is one
On 2013/03/13 11:05:59, kinuko wrote:
> nit: it -> if

Done.

https://codereview.chromium.org/12645004/diff/5001/content/browser/loader/res...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/12645004/diff/5001/content/browser/loader/res...
content/browser/loader/resource_request_info_impl.h:55: bool is_stream,
On 2013/03/13 11:05:59, kinuko wrote:
> nit: it's probably ok since we already have is_download but in general adding
> new bool parameters wouldn't be preferable

Would an enum be preferable?

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

https://codereview.chromium.org/12645004/diff/5001/content/browser/loader/str...
content/browser/loader/stream_resource_handler.cc:67: }
On 2013/03/13 11:05:59, kinuko wrote:
> nit: no need of { } for one-line body

Done.

https://codereview.chromium.org/12645004/diff/5001/content/browser/loader/str...
content/browser/loader/stream_resource_handler.cc:91: }
On 2013/03/13 11:05:59, kinuko wrote:
> nit: ditto for { }

Done.

https://codereview.chromium.org/12645004/diff/5001/content/browser/loader/str...
content/browser/loader/stream_resource_handler.cc:101: if (urs.status() !=
net::URLRequestStatus::SUCCESS)
On 2013/03/13 11:05:59, kinuko wrote:
> nit: return urs.status() == net::URLRequestStatus::SUCCESS
> 
> ?

Done.

[Zachary Kuznia, 2013-03-13 15:39:20]

Hi Darin,

Could you take a look at this?  This is the last patch to support the interim
solution for QuickOffice.  It replaces the DownloadThrottle solution for mime
type handling.  To keep this CL focused, I remove the throttle and associated
code in a follow-up change.

Some of the code you already reviewed in:
https://codereview.chromium.org/12212031/, so it already has the fixes from that
review.

Thanks,
-Zach

[darin (slow to review), 2013-03-14 23:27:54]

https://chromiumcodereview.appspot.com/12645004/diff/18001/chrome/browser/ren...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://chromiumcodereview.appspot.com/12645004/diff/18001/chrome/browser/ren...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:102:
bool CanHandleMimeType(const Extension* extension,
nit: ExtensionCanHandleMimeType

https://chromiumcodereview.appspot.com/12645004/diff/18001/chrome/browser/ren...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:131:
void DispatchEventOnUIThread(const std::string& mime_type,
this function name is fairly generic but the function is actually specific to
the particular event you want to dispatch.

one way to deal with this is to just create another source file that is domain
specific to streams.

another way would be to break this into two functions:

1-  DispatchEventToExtensionOnUIThread(
        int render_process_id,
        int render_view_id,
        const std::string& extension_id,
        const std::string& event_name,
        scoped_ptr<ListValue> args)

2-  Then build the ListValue on the IO thread.

https://chromiumcodereview.appspot.com/12645004/diff/18001/chrome/browser/ren...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:492: //
Go through the white-listed extensions and try to use them to intercept
should we assert that we were able to locate the extension again?

it seems like we are having to find the extension twice, once in
ShouldCreateStream and then again in OnStreamCreated.  that seems quite
non-ideal.  any way to avoid that?

https://chromiumcodereview.appspot.com/12645004/diff/18001/content/browser/lo...
File content/browser/loader/buffered_resource_handler.cc (right):

https://chromiumcodereview.appspot.com/12645004/diff/18001/content/browser/lo...
content/browser/loader/buffered_resource_handler.cc:328: GURL security_origin;
please break this code out into a helper function.  SelectNextHandler would be
too big with all of this code added to it.

https://chromiumcodereview.appspot.com/12645004/diff/18001/content/browser/lo...
File content/browser/loader/stream_resource_handler.cc (right):

https://chromiumcodereview.appspot.com/12645004/diff/18001/content/browser/lo...
content/browser/loader/stream_resource_handler.cc:96: const
net::URLRequestStatus& urs,
nit: urs -> status

try to avoid uncommon acronyms.

https://chromiumcodereview.appspot.com/12645004/diff/18001/content/browser/lo...
File content/browser/loader/stream_resource_handler.h (right):

https://chromiumcodereview.appspot.com/12645004/diff/18001/content/browser/lo...
content/browser/loader/stream_resource_handler.h:66: scoped_refptr<Stream>
stream() { return stream_; }
does this need to return scoped_refptr?  why not just a Stream*?  if it does,
then i'd just return |const scoped_refptr<Stream>&| instead.

https://chromiumcodereview.appspot.com/12645004/diff/18001/content/public/bro...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://chromiumcodereview.appspot.com/12645004/diff/18001/content/public/bro...
content/public/browser/resource_dispatcher_host_delegate.h:112: virtual bool
ShouldCreateStream(content::ResourceContext* resource_context,
nit: I'm struggling a bit with this name.  It doesn't really tell the whole
story.  This API is really asking if the resource load should be intercepted and
redirected to a stream that can be made available at the given URL.

How about a name like ShouldInterceptResourceAsStream?

Also, would it be reasonable to instead make OnStreamCreated vend a Stream
object?  Then, you could have the embedder (Chrome) use Stream-related APIs to
associate a Blob URL with the Stream.  That might make this system a bit more
general purpose / easier to understand.

[Zachary Kuznia, 2013-03-15 00:22:24]

https://chromiumcodereview.appspot.com/12645004/diff/18001/chrome/browser/ren...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://chromiumcodereview.appspot.com/12645004/diff/18001/chrome/browser/ren...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:102:
bool CanHandleMimeType(const Extension* extension,
On 2013/03/14 23:27:54, darin wrote:
> nit: ExtensionCanHandleMimeType

Done.

https://chromiumcodereview.appspot.com/12645004/diff/18001/chrome/browser/ren...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:131:
void DispatchEventOnUIThread(const std::string& mime_type,
On 2013/03/14 23:27:54, darin wrote:
> this function name is fairly generic but the function is actually specific to
> the particular event you want to dispatch.
> 
> one way to deal with this is to just create another source file that is domain
> specific to streams.
> 
> another way would be to break this into two functions:
> 
> 1-  DispatchEventToExtensionOnUIThread(
>         int render_process_id,
>         int render_view_id,
>         const std::string& extension_id,
>         const std::string& event_name,
>         scoped_ptr<ListValue> args)
> 
> 2-  Then build the ListValue on the IO thread.

Done.

https://chromiumcodereview.appspot.com/12645004/diff/18001/chrome/browser/ren...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:492: //
Go through the white-listed extensions and try to use them to intercept
On 2013/03/14 23:27:54, darin wrote:
> should we assert that we were able to locate the extension again?
> 
> it seems like we are having to find the extension twice, once in
> ShouldCreateStream and then again in OnStreamCreated.  that seems quite
> non-ideal.  any way to avoid that?

Removed most of this function and replaced it with target_id

https://chromiumcodereview.appspot.com/12645004/diff/18001/content/browser/lo...
File content/browser/loader/buffered_resource_handler.cc (right):

https://chromiumcodereview.appspot.com/12645004/diff/18001/content/browser/lo...
content/browser/loader/buffered_resource_handler.cc:328: GURL security_origin;
On 2013/03/14 23:27:54, darin wrote:
> please break this code out into a helper function.  SelectNextHandler would be
> too big with all of this code added to it.

Done.

https://chromiumcodereview.appspot.com/12645004/diff/18001/content/browser/lo...
File content/browser/loader/stream_resource_handler.cc (right):

https://chromiumcodereview.appspot.com/12645004/diff/18001/content/browser/lo...
content/browser/loader/stream_resource_handler.cc:96: const
net::URLRequestStatus& urs,
On 2013/03/14 23:27:54, darin wrote:
> nit: urs -> status
> 
> try to avoid uncommon acronyms.

Done.

https://chromiumcodereview.appspot.com/12645004/diff/18001/content/browser/lo...
File content/browser/loader/stream_resource_handler.h (right):

https://chromiumcodereview.appspot.com/12645004/diff/18001/content/browser/lo...
content/browser/loader/stream_resource_handler.h:66: scoped_refptr<Stream>
stream() { return stream_; }
On 2013/03/14 23:27:54, darin wrote:
> does this need to return scoped_refptr?  why not just a Stream*?  if it does,
> then i'd just return |const scoped_refptr<Stream>&| instead.

Done.

https://chromiumcodereview.appspot.com/12645004/diff/18001/content/public/bro...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://chromiumcodereview.appspot.com/12645004/diff/18001/content/public/bro...
content/public/browser/resource_dispatcher_host_delegate.h:112: virtual bool
ShouldCreateStream(content::ResourceContext* resource_context,
On 2013/03/14 23:27:54, darin wrote:
> nit: I'm struggling a bit with this name.  It doesn't really tell the whole
> story.  This API is really asking if the resource load should be intercepted
and
> redirected to a stream that can be made available at the given URL.
> 
> How about a name like ShouldInterceptResourceAsStream?
> 
> Also, would it be reasonable to instead make OnStreamCreated vend a Stream
> object?  Then, you could have the embedder (Chrome) use Stream-related APIs to
> associate a Blob URL with the Stream.  That might make this system a bit more
> general purpose / easier to understand.

Renamed.

The reason I only pass the stream URL instead of the object is that passing the
Stream object would require moving stream.{cc,h} to content/public/, which
didn't seem like a good idea until there was a concrete need for the embedder to
have the object.

[darin (slow to review), 2013-03-16 17:23:51]

https://chromiumcodereview.appspot.com/12645004/diff/25001/content/public/bro...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://chromiumcodereview.appspot.com/12645004/diff/25001/content/public/bro...
content/public/browser/resource_dispatcher_host_delegate.h:111: // be created
for the resource.
please document the fact that OnStreamCreated will be called if
ShouldInterceptResourceAsStream returns true.  also, document the fact that the
target_id parameter here will be forwarded to OnStreamCreated.  you might want
to explain what security_origin means in this context.

I think you should be really clear about the fact that this creates a Blob URL
associated with the given security_origin, and indicate that the Blob URL may
only be loaded once.

What happens if the Blob URL is never loaded?  Do we leak the stream data
forever?

[Zachary Kuznia, 2013-03-17 01:30:33]

https://chromiumcodereview.appspot.com/12645004/diff/25001/content/public/bro...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://chromiumcodereview.appspot.com/12645004/diff/25001/content/public/bro...
content/public/browser/resource_dispatcher_host_delegate.h:111: // be created
for the resource.
On 2013/03/16 17:23:51, darin wrote:
> please document the fact that OnStreamCreated will be called if
> ShouldInterceptResourceAsStream returns true.  also, document the fact that
the
> target_id parameter here will be forwarded to OnStreamCreated.  you might want
> to explain what security_origin means in this context.
> 
> I think you should be really clear about the fact that this creates a Blob URL
> associated with the given security_origin, and indicate that the Blob URL may
> only be loaded once.
> 
> What happens if the Blob URL is never loaded?  Do we leak the stream data
> forever?

I expanded these comments to explain this behavior.  I also added a 5 minute
read timeout to stream.{cc,h}.  Now, if the Stream spends more than 5 minutes
without a read observer, it unregisters itself and informer the WriteObserver
that it timed out.

[darin (slow to review), 2013-03-17 03:48:23]

On Sat, Mar 16, 2013 at 6:30 PM, <zork@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/12645004/diff/**
>
25001/content/public/browser/**resource_dispatcher_host_**delegate.h<https://chromiumcodereview.appspot.com/12645004/diff/25001/content/public/browser/resource_dispatcher_host_delegate.h>
> File content/public/browser/**resource_dispatcher_host_**delegate.h
> (right):
>
> https://chromiumcodereview.**appspot.com/12645004/diff/**
> 25001/content/public/browser/**resource_dispatcher_host_**
>
delegate.h#newcode111<https://chromiumcodereview.appspot.com/12645004/diff/25001/content/public/browser/resource_dispatcher_host_delegate.h#newcode111>
> content/public/browser/**resource_dispatcher_host_**delegate.h:111: // be
> created for the resource.
> On 2013/03/16 17:23:51, darin wrote:
>
>> please document the fact that OnStreamCreated will be called if
>> ShouldInterceptResourceAsStrea**m returns true.  also, document the fact
>>
> that the
>
>> target_id parameter here will be forwarded to OnStreamCreated.  you
>>
> might want
>
>> to explain what security_origin means in this context.
>>
>
>  I think you should be really clear about the fact that this creates a
>>
> Blob URL
>
>> associated with the given security_origin, and indicate that the Blob
>>
> URL may
>
>> only be loaded once.
>>
>
>  What happens if the Blob URL is never loaded?  Do we leak the stream
>>
> data
>
>> forever?
>>
>
> I expanded these comments to explain this behavior.  I also added a 5
> minute read timeout to stream.{cc,h}.  Now, if the Stream spends more
> than 5 minutes without a read observer, it unregisters itself and
> informer the WriteObserver that it timed out.
>
>
I'm not a fan of timeouts.  They lead to flaky behavior.  What if the
system is suspended after the stream is allocated but before it can be
requested by the target extension?

Is there any opportunity for the content API to vend an object that should
be retained by Chrome until such time as Chrome no longer cares about the
existence of the stream?

-Darin



>
https://chromiumcodereview.**appspot.com/12645004/<https://chromiumcodereview...
>

[Zachary Kuznia, 2013-03-17 04:35:07]

> I'm not a fan of timeouts.  They lead to flaky behavior.  What if the
> system is suspended after the stream is allocated but before it can be
> requested by the target extension?
> 
> Is there any opportunity for the content API to vend an object that should
> be retained by Chrome until such time as Chrome no longer cares about the
> existence of the stream?
> 
> -Darin

Once Streams are landed in WebKit, I'm going to change the API to vend a Stream
object that will do exactly this.  Until that lands, though, I'm not sure of a
good way to encapsulate the Stream in an object.  Chrome can't really track it
as a C++ object, because the lifetime of the Stream reader is in JavaScript in
the extension.

One possible solution is that Chrome could keep a list of URLs that have been
sent to each extension.  This would require two more APIs on the delegate:  One
to notify that a Stream URL has been consumed and doesn't need to be tracked
anymore, and one to notify content that a Stream URL should be freed, which
could be fired when an extension unloads.  However, this solution would still
effectively leak every Stream if the extension wasn't reading from the URLs, and
never got uninstalled.

In the case of suspending, wouldn't the Stream become invalid anyways, as the
connection to the server would be broken?  Or does Chrome automatically resume
such connections?

[darin (slow to review), 2013-03-17 05:02:04]

Good point about TCP interruptions.  Maybe consider downloads that are
small?

I agree this could get a lot simpler once we have real Stream objects
available to script.  However, even in that case there is still a "handoff"
time period between deciding to create a stream on the browser side and
getting the Stream object into JS.  During that handoff period, we'll still
have to keep track of the underlying stream until JS starts to retain it.
 That seems a bit similar to what we are talking about here.

What if we were to say that the underlying stream's lifetime is bound to
that of the extension we are about to spawn?  I mean, isn't the idea that
we send the stream URL to the extension?  So, if we were to monitor the
lifetime of the extension, then we could say that the stream URL can be
made invalid once the extension closes, right?


On Sat, Mar 16, 2013 at 9:35 PM, <zork@chromium.org> wrote:

>
>  I'm not a fan of timeouts.  They lead to flaky behavior.  What if the
>> system is suspended after the stream is allocated but before it can be
>> requested by the target extension?
>>
>
>  Is there any opportunity for the content API to vend an object that should
>> be retained by Chrome until such time as Chrome no longer cares about the
>> existence of the stream?
>>
>
>  -Darin
>>
>
> Once Streams are landed in WebKit, I'm going to change the API to vend a
> Stream
> object that will do exactly this.  Until that lands, though, I'm not sure
> of a
> good way to encapsulate the Stream in an object.  Chrome can't really
> track it
> as a C++ object, because the lifetime of the Stream reader is in
> JavaScript in
> the extension.
>
> One possible solution is that Chrome could keep a list of URLs that have
> been
> sent to each extension.  This would require two more APIs on the delegate:
>  One
> to notify that a Stream URL has been consumed and doesn't need to be
> tracked
> anymore, and one to notify content that a Stream URL should be freed, which
> could be fired when an extension unloads.  However, this solution would
> still
> effectively leak every Stream if the extension wasn't reading from the
> URLs, and
> never got uninstalled.
>
> In the case of suspending, wouldn't the Stream become invalid anyways, as
> the
> connection to the server would be broken?  Or does Chrome automatically
> resume
> such connections?
>
>
https://chromiumcodereview.**appspot.com/12645004/<https://chromiumcodereview...
>

[Zachary Kuznia, 2013-03-17 05:14:51]

On 2013/03/17 05:02:04, darin wrote:
> Good point about TCP interruptions.  Maybe consider downloads that are
> small?
> 
> I agree this could get a lot simpler once we have real Stream objects
> available to script.  However, even in that case there is still a "handoff"
> time period between deciding to create a stream on the browser side and
> getting the Stream object into JS.  During that handoff period, we'll still
> have to keep track of the underlying stream until JS starts to retain it.
>  That seems a bit similar to what we are talking about here.
> 
> What if we were to say that the underlying stream's lifetime is bound to
> that of the extension we are about to spawn?  I mean, isn't the idea that
> we send the stream URL to the extension?  So, if we were to monitor the
> lifetime of the extension, then we could say that the stream URL can be
> made invalid once the extension closes, right?
> 

Linking it to the extensions lifetime is what I meant by the solution where
Chrome tracks the URLs sent to each extension.  I would need to move Stream and
StreamRegistry to content/public, and add an event for a Stream being consumed. 
I'll update the patch with this solution, so you can see what it would look
like.

[darin (slow to review), 2013-03-17 05:58:45]

NOTE: We usually introduce interfaces in content/public/ corresponding to
concrete classes we want to expose through the content API.


On Sat, Mar 16, 2013 at 10:14 PM, <zork@chromium.org> wrote:

> On 2013/03/17 05:02:04, darin wrote:
>
>> Good point about TCP interruptions.  Maybe consider downloads that are
>> small?
>>
>
>  I agree this could get a lot simpler once we have real Stream objects
>> available to script.  However, even in that case there is still a
>> "handoff"
>> time period between deciding to create a stream on the browser side and
>> getting the Stream object into JS.  During that handoff period, we'll
>> still
>> have to keep track of the underlying stream until JS starts to retain it.
>>   That seems a bit similar to what we are talking about here.
>>
>
>  What if we were to say that the underlying stream's lifetime is bound to
>> that of the extension we are about to spawn?  I mean, isn't the idea that
>> we send the stream URL to the extension?  So, if we were to monitor the
>> lifetime of the extension, then we could say that the stream URL can be
>> made invalid once the extension closes, right?
>>
>
>
> Linking it to the extensions lifetime is what I meant by the solution where
> Chrome tracks the URLs sent to each extension.  I would need to move
> Stream and
> StreamRegistry to content/public, and add an event for a Stream being
> consumed.
> I'll update the patch with this solution, so you can see what it would look
> like.
>
>
https://chromiumcodereview.**appspot.com/12645004/<https://chromiumcodereview...
>

[Zachary Kuznia, 2013-03-17 06:07:54]

On 2013/03/17 05:58:45, darin wrote:
> NOTE: We usually introduce interfaces in content/public/ corresponding to
> concrete classes we want to expose through the content API.

I'll start with a Stream interface that has a Close() method, a url() method,
and an OnStreamUnregisterObserver.  The class in content/browser/streams/ will
become StreamImpl.

[darin (slow to review), 2013-03-17 06:45:02]

Had you considered just exposing an object, and letting the lifetime of
that object dictate the lifetime of the underlying stream storage?  What is
the purpose of OnStreamUnregisterObserver?


On Sat, Mar 16, 2013 at 11:07 PM, <zork@chromium.org> wrote:

> On 2013/03/17 05:58:45, darin wrote:
>
>> NOTE: We usually introduce interfaces in content/public/ corresponding to
>> concrete classes we want to expose through the content API.
>>
>
> I'll start with a Stream interface that has a Close() method, a url()
> method,
> and an OnStreamUnregisterObserver.  The class in content/browser/streams/
> will
> become StreamImpl.
>
>
https://chromiumcodereview.**appspot.com/12645004/<https://chromiumcodereview...
>

[Zachary Kuznia, 2013-03-17 19:00:35]

On 2013/03/17 06:45:02, darin wrote:
> Had you considered just exposing an object, and letting the lifetime of
> that object dictate the lifetime of the underlying stream storage?  What is
> the purpose of OnStreamUnregisterObserver?

I've now add StreamHandle, which has its lifetime tied to the Stream's lifetime.
 Additionally, the StreamsPrivateAPI now tracks all open Streams, and closes
them when the owning extension is unloaded.

[kinuko, 2013-03-18 00:28:17]

https://codereview.chromium.org/12645004/diff/49001/chrome/browser/extensions...
File chrome/browser/extensions/api/streams_private/streams_private_api.cc
(right):

https://codereview.chromium.org/12645004/diff/49001/chrome/browser/extensions...
chrome/browser/extensions/api/streams_private/streams_private_api.cc:90: if
(type == chrome::NOTIFICATION_EXTENSION_UNLOADED) {
(drive-by) UNLOADED event is dispatched when an extension is uninstalled (or
disabled in chrome://extensions/), which would not happen very frequently (less
frequently than chrome restarts). Is it the right place to handle this? (Though
we'd want to dealloc streams in uninstall case too)

[Zachary Kuznia, 2013-03-18 01:06:55]

https://codereview.chromium.org/12645004/diff/49001/chrome/browser/extensions...
File chrome/browser/extensions/api/streams_private/streams_private_api.cc
(right):

https://codereview.chromium.org/12645004/diff/49001/chrome/browser/extensions...
chrome/browser/extensions/api/streams_private/streams_private_api.cc:90: if
(type == chrome::NOTIFICATION_EXTENSION_UNLOADED) {
On 2013/03/18 00:28:17, kinuko wrote:
> (drive-by) UNLOADED event is dispatched when an extension is uninstalled (or
> disabled in chrome://extensions/), which would not happen very frequently
(less
> frequently than chrome restarts). Is it the right place to handle this?
(Though
> we'd want to dealloc streams in uninstall case too)

This is where chrome.omnibox, chrome.push_messaging, chrome.input.ime and a few
other APIs handle the event, so I felt it was best to continue that pattern.

[kinuko, 2013-03-18 01:46:17]

https://codereview.chromium.org/12645004/diff/49001/chrome/browser/extensions...
File chrome/browser/extensions/api/streams_private/streams_private_api.cc
(right):

https://codereview.chromium.org/12645004/diff/49001/chrome/browser/extensions...
chrome/browser/extensions/api/streams_private/streams_private_api.cc:90: if
(type == chrome::NOTIFICATION_EXTENSION_UNLOADED) {
On 2013/03/18 01:06:55, Zachary Kuznia wrote:
> On 2013/03/18 00:28:17, kinuko wrote:
> > (drive-by) UNLOADED event is dispatched when an extension is uninstalled (or
> > disabled in chrome://extensions/), which would not happen very frequently
> (less
> > frequently than chrome restarts). Is it the right place to handle this?
> (Though
> > we'd want to dealloc streams in uninstall case too)
> 
> This is where chrome.omnibox, chrome.push_messaging, chrome.input.ime and a
few
> other APIs handle the event, so I felt it was best to continue that pattern.

I agree that we should handle this case too... and I see we're also releasing
handles when this API instance goes away. So in the worst case streams could be
around until the profile exits?

[Zachary Kuznia, 2013-03-18 01:53:48]

On 2013/03/18 01:46:17, kinuko wrote:
>
https://codereview.chromium.org/12645004/diff/49001/chrome/browser/extensions...
> File chrome/browser/extensions/api/streams_private/streams_private_api.cc
> (right):
> 
>
https://codereview.chromium.org/12645004/diff/49001/chrome/browser/extensions...
> chrome/browser/extensions/api/streams_private/streams_private_api.cc:90: if
> (type == chrome::NOTIFICATION_EXTENSION_UNLOADED) {
> On 2013/03/18 01:06:55, Zachary Kuznia wrote:
> > On 2013/03/18 00:28:17, kinuko wrote:
> > > (drive-by) UNLOADED event is dispatched when an extension is uninstalled
(or
> > > disabled in chrome://extensions/), which would not happen very frequently
> > (less
> > > frequently than chrome restarts). Is it the right place to handle this?
> > (Though
> > > we'd want to dealloc streams in uninstall case too)
> > 
> > This is where chrome.omnibox, chrome.push_messaging, chrome.input.ime and a
> few
> > other APIs handle the event, so I felt it was best to continue that pattern.
> 
> I agree that we should handle this case too... and I see we're also releasing
> handles when this API instance goes away. So in the worst case streams could
be
> around until the profile exits?

That is correct.  If the extension holds the references to the Streams, they
won't get cleaned up until the extension goes away.

[darin (slow to review), 2013-03-19 06:12:50]

I think this is looking really great.  Lot's of feedback, but hopefully helpful
stuff...

https://codereview.chromium.org/12645004/diff/49001/chrome/browser/extensions...
File chrome/browser/extensions/api/streams_private/streams_private_api.cc
(right):

https://codereview.chromium.org/12645004/diff/49001/chrome/browser/extensions...
chrome/browser/extensions/api/streams_private/streams_private_api.cc:55:
event_args->Append(new base::StringValue(mime_type));
it feels like these could be parameters of the StreamHandle object

https://codereview.chromium.org/12645004/diff/49001/content/browser/loader/bu...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/12645004/diff/49001/content/browser/loader/bu...
content/browser/loader/buffered_resource_handler.cc:402:
host_->delegate()->ShouldInterceptResourceAsStream(info->GetContext(),
nit: return early to reduce overall indentation

https://codereview.chromium.org/12645004/diff/49001/content/browser/loader/bu...
content/browser/loader/buffered_resource_handler.cc:407: StreamContext*
stream_context =
you might consider modeling this code after CreateResourceHandlerForDownload,
which puts the handler allocation and delegate callback (DownloadStarting, but
in this case OnStreamCreated) on the ResourceDispatcherHostImpl.

https://codereview.chromium.org/12645004/diff/49001/content/browser/streams/s...
File content/browser/streams/stream.cc (right):

https://codereview.chromium.org/12645004/diff/49001/content/browser/streams/s...
content/browser/streams/stream.cc:95: registry_->UnregisterStream(url());
is there any possibility that SendCloseEvent could end up deleting |this|? 
should you consider calling UnregisterStream first to play it safe?

https://codereview.chromium.org/12645004/diff/49001/content/browser/streams/s...
content/browser/streams/stream.cc:125: write_observer_->OnClose(this);
ditto:  calling out to the write observer is calling out to foreign code.  how
do you know that |this| will still be valid memory upon return from OnClose?  do
we need to worry about dereferencing registry_?

https://codereview.chromium.org/12645004/diff/49001/content/browser/streams/s...
File content/browser/streams/stream_handle_impl.cc (right):

https://codereview.chromium.org/12645004/diff/49001/content/browser/streams/s...
content/browser/streams/stream_handle_impl.cc:12:
StreamHandleImpl::StreamHandleImpl(base::WeakPtr<Stream> stream)
nit: we normally pass WeakPtr using |const WeakPtr<...>&|

https://codereview.chromium.org/12645004/diff/49001/content/browser/streams/s...
content/browser/streams/stream_handle_impl.cc:24: void
StreamHandleImpl::SendCloseEvent() {
nit: Stream::CloseHandle and StreamHandle::SendCloseEvent sound very similar,
yet they are quite different.

There are really two states: 1) the StreamHandle gets destroyed, 2) the Stream
gets fully consumed (has no more data).

For #1, I think Stream::Close() makes sense.  For #2, maybe
StreamHandle::OnStreamConsumed().  Then, perhaps RegisterCloseCallback could be
renamed StreamHandle::NotifyWhenConsumed?

These are just naming suggestions.

https://codereview.chromium.org/12645004/diff/49001/content/browser/streams/s...
File content/browser/streams/stream_handle_impl.h (right):

https://codereview.chromium.org/12645004/diff/49001/content/browser/streams/s...
content/browser/streams/stream_handle_impl.h:34: base::Lock callback_lock_;
consider documenting what variables must be accessed with this lock held

https://codereview.chromium.org/12645004/diff/49001/content/public/browser/re...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/12645004/diff/49001/content/public/browser/re...
content/public/browser/resource_dispatcher_host_delegate.h:127: // Stream can be
read from the blob URL |stream_url|, but can only be read
nit: stream_url no longer exists
nit: you should define original_url

would it make sense for original_url and mime_type to be properties of
StreamHandle?

https://codereview.chromium.org/12645004/diff/49001/content/public/browser/st...
File content/public/browser/stream_handle.h (right):

https://codereview.chromium.org/12645004/diff/49001/content/public/browser/st...
content/public/browser/stream_handle.h:18: virtual const GURL& GetUrl() = 0;
nit: I'm more accustomed to seeing GetURL in chromium than GetUrl

https://codereview.chromium.org/12645004/diff/49001/content/public/browser/st...
content/public/browser/stream_handle.h:19: virtual void
RegisterCloseCallback(const base::Closure& callback) = 0;
nit: might want to add a comment explaining what a close callback is.

[Zachary Kuznia, 2013-03-19 06:59:35]

https://codereview.chromium.org/12645004/diff/49001/chrome/browser/extensions...
File chrome/browser/extensions/api/streams_private/streams_private_api.cc
(right):

https://codereview.chromium.org/12645004/diff/49001/chrome/browser/extensions...
chrome/browser/extensions/api/streams_private/streams_private_api.cc:55:
event_args->Append(new base::StringValue(mime_type));
On 2013/03/19 06:12:51, darin wrote:
> it feels like these could be parameters of the StreamHandle object

Done.

https://codereview.chromium.org/12645004/diff/49001/content/browser/loader/bu...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/12645004/diff/49001/content/browser/loader/bu...
content/browser/loader/buffered_resource_handler.cc:402:
host_->delegate()->ShouldInterceptResourceAsStream(info->GetContext(),
On 2013/03/19 06:12:51, darin wrote:
> nit: return early to reduce overall indentation

Done.

https://codereview.chromium.org/12645004/diff/49001/content/browser/loader/bu...
content/browser/loader/buffered_resource_handler.cc:407: StreamContext*
stream_context =
On 2013/03/19 06:12:51, darin wrote:
> you might consider modeling this code after CreateResourceHandlerForDownload,
> which puts the handler allocation and delegate callback (DownloadStarting, but
> in this case OnStreamCreated) on the ResourceDispatcherHostImpl.

Done.

https://codereview.chromium.org/12645004/diff/49001/content/browser/streams/s...
File content/browser/streams/stream.cc (right):

https://codereview.chromium.org/12645004/diff/49001/content/browser/streams/s...
content/browser/streams/stream.cc:95: registry_->UnregisterStream(url());
On 2013/03/19 06:12:51, darin wrote:
> is there any possibility that SendCloseEvent could end up deleting |this|? 
> should you consider calling UnregisterStream first to play it safe?

Done.

https://codereview.chromium.org/12645004/diff/49001/content/browser/streams/s...
content/browser/streams/stream.cc:125: write_observer_->OnClose(this);
On 2013/03/19 06:12:51, darin wrote:
> ditto:  calling out to the write observer is calling out to foreign code.  how
> do you know that |this| will still be valid memory upon return from OnClose? 
do
> we need to worry about dereferencing registry_?

Done.

https://codereview.chromium.org/12645004/diff/49001/content/browser/streams/s...
File content/browser/streams/stream_handle_impl.cc (right):

https://codereview.chromium.org/12645004/diff/49001/content/browser/streams/s...
content/browser/streams/stream_handle_impl.cc:12:
StreamHandleImpl::StreamHandleImpl(base::WeakPtr<Stream> stream)
On 2013/03/19 06:12:51, darin wrote:
> nit: we normally pass WeakPtr using |const WeakPtr<...>&|

Done.

https://codereview.chromium.org/12645004/diff/49001/content/browser/streams/s...
content/browser/streams/stream_handle_impl.cc:24: void
StreamHandleImpl::SendCloseEvent() {
On 2013/03/19 06:12:51, darin wrote:
> nit: Stream::CloseHandle and StreamHandle::SendCloseEvent sound very similar,
> yet they are quite different.
> 
> There are really two states: 1) the StreamHandle gets destroyed, 2) the Stream
> gets fully consumed (has no more data).
> 
> For #1, I think Stream::Close() makes sense.  For #2, maybe
> StreamHandle::OnStreamConsumed().  Then, perhaps RegisterCloseCallback could
be
> renamed StreamHandle::NotifyWhenConsumed?
> 
> These are just naming suggestions.

Done.

https://codereview.chromium.org/12645004/diff/49001/content/browser/streams/s...
File content/browser/streams/stream_handle_impl.h (right):

https://codereview.chromium.org/12645004/diff/49001/content/browser/streams/s...
content/browser/streams/stream_handle_impl.h:34: base::Lock callback_lock_;
On 2013/03/19 06:12:51, darin wrote:
> consider documenting what variables must be accessed with this lock held

Done.

https://codereview.chromium.org/12645004/diff/49001/content/public/browser/re...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/12645004/diff/49001/content/public/browser/re...
content/public/browser/resource_dispatcher_host_delegate.h:127: // Stream can be
read from the blob URL |stream_url|, but can only be read
On 2013/03/19 06:12:51, darin wrote:
> nit: stream_url no longer exists
> nit: you should define original_url
> 
> would it make sense for original_url and mime_type to be properties of
> StreamHandle?

Done.

https://codereview.chromium.org/12645004/diff/49001/content/public/browser/st...
File content/public/browser/stream_handle.h (right):

https://codereview.chromium.org/12645004/diff/49001/content/public/browser/st...
content/public/browser/stream_handle.h:18: virtual const GURL& GetUrl() = 0;
On 2013/03/19 06:12:51, darin wrote:
> nit: I'm more accustomed to seeing GetURL in chromium than GetUrl

Done.

https://codereview.chromium.org/12645004/diff/49001/content/public/browser/st...
content/public/browser/stream_handle.h:19: virtual void
RegisterCloseCallback(const base::Closure& callback) = 0;
On 2013/03/19 06:12:51, darin wrote:
> nit: might want to add a comment explaining what a close callback is.

Done.

[darin (slow to review), 2013-03-19 14:32:19]

LGTM

https://codereview.chromium.org/12645004/diff/60002/chrome/browser/extensions...
File chrome/browser/extensions/api/streams_private/streams_private_api.cc
(right):

https://codereview.chromium.org/12645004/diff/60002/chrome/browser/extensions...
chrome/browser/extensions/api/streams_private/streams_private_api.cc:71: void
StreamsPrivateAPI::OnStreamClosed(const std::string& extension_id,
nit: OnStreamConsumed?

Note: As I mentioned before, I probably wouldn't bother at all with the whole
callback to inform chrome that the stream was consumed.  It is not going to free
up very much memory.  It is not clear that the complexity is worth it.

[Zachary Kuznia, 2013-03-19 15:33:45]

On 2013/03/19 14:32:19, darin wrote:
> LGTM
> 
>
https://codereview.chromium.org/12645004/diff/60002/chrome/browser/extensions...
> File chrome/browser/extensions/api/streams_private/streams_private_api.cc
> (right):
> 
>
https://codereview.chromium.org/12645004/diff/60002/chrome/browser/extensions...
> chrome/browser/extensions/api/streams_private/streams_private_api.cc:71: void
> StreamsPrivateAPI::OnStreamClosed(const std::string& extension_id,
> nit: OnStreamConsumed?
> 
> Note: As I mentioned before, I probably wouldn't bother at all with the whole
> callback to inform chrome that the stream was consumed.  It is not going to
free
> up very much memory.  It is not clear that the complexity is worth it.

NotifyStreamConsumed has now been removed.

[darin (slow to review), 2013-03-19 16:34:06]

LGTM

[commit-bot: I haz the power, 2013-03-19 21:27:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/12645004/82006

[commit-bot: I haz the power, 2013-03-20 00:26:49]

Change committed as 189172