Deletable file references.

Flesh out URLLoader's download_to_file function.
* tie the lifetime of the resulting temp file to the lifetime of the URLLoader
  (the plan is to later extend the lifetime of the temp file to support xhr.responseBlob)
* make it work in test_shell
* make it work for sync requests
* added OnDataDownloaded messages to report progress

A related BlobURL loading change.
* grab a reference to the blob early on to ensure it's still there when the 'job' is finally started.

TEST=manual and deletable_file_reference_unittest.cc
BUG=52486
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=60378

[michaeln, 2010-09-01 22:26:08]

This isn't really ready for review yet, but i wanted to send it out so people
could take a look at what i've got in mind and let me know if i'm missing
something.

wdyt?

http://codereview.chromium.org/3165062/diff/16001/14005
File base/deletable_file_reference.cc (right):

http://codereview.chromium.org/3165062/diff/16001/14005#newcode49
base/deletable_file_reference.cc:49: if (file_thread_)
should just be a required param

http://codereview.chromium.org/3165062/diff/16001/14009
File webkit/blob/blob_data.h (right):

http://codereview.chromium.org/3165062/diff/16001/14009#newcode70
webkit/blob/blob_data.h:70: DeletableFileReference::GetReference(file_path);
The trick is going to be to get this line to execute prior to
~RedirectToFileResourceHandler() when doing xhr.asBlob in workers.

http://codereview.chromium.org/3165062/diff/16001/14012
File webkit/glue/plugins/pepper_file_ref.cc (right):

http://codereview.chromium.org/3165062/diff/16001/14012#newcode136
webkit/glue/plugins/pepper_file_ref.cc:136: // TODO(michaeln): ipc to maybe
addRef the remote DeleteableFileReference
perhaps have an 'is_system_temp_file' parameter to conditionally send that ipc?

http://codereview.chromium.org/3165062/diff/16001/14012#newcode141
webkit/glue/plugins/pepper_file_ref.cc:141: //   ipc to maybe releaseRef the
remote DeleteableFileReference
... and if (is_system_temp_file) send the ipc to release it

[dumi, 2010-09-02 20:15:20]

LGTM, in general.

http://codereview.chromium.org/3165062/diff/16001/14005
File base/deletable_file_reference.cc (right):

http://codereview.chromium.org/3165062/diff/16001/14005#newcode42
base/deletable_file_reference.cc:42: DCHECK(!GetReference(path_));
could be replaced with

DCHECK(map()->find(path) == map()->end());

for a very small optimization.

http://codereview.chromium.org/3165062/diff/16001/14005#newcode43
base/deletable_file_reference.cc:43:
map()->insert(DeleteableFileMap::value_type(path_, this));
std::map's interface is just weird...

http://codereview.chromium.org/3165062/diff/16001/14005#newcode49
base/deletable_file_reference.cc:49: if (file_thread_)
On 2010/09/01 22:26:08, michaeln wrote:
> should just be a required param

i agree. add a DCHECK() to Get{OrCreate}Reference?

http://codereview.chromium.org/3165062/diff/16001/14006
File base/deletable_file_reference.h (right):

http://codereview.chromium.org/3165062/diff/16001/14006#newcode16
base/deletable_file_reference.h:16: // A refcounted wrapper around a FilePath
that schedules the file
probably need some comments that this class is not thread-safe.

http://codereview.chromium.org/3165062/diff/16001/14006#newcode36
base/deletable_file_reference.h:36: const FilePath& path,
base::MessageLoopProxy* file_thread);
pass in a scoped_refptr<base::MessageLoopProxy> instead of a raw pointer?

http://codereview.chromium.org/3165062/diff/16001/14012
File webkit/glue/plugins/pepper_file_ref.cc (right):

http://codereview.chromium.org/3165062/diff/16001/14012#newcode136
webkit/glue/plugins/pepper_file_ref.cc:136: // TODO(michaeln): ipc to maybe
addRef the remote DeleteableFileReference
On 2010/09/01 22:26:08, michaeln wrote:
> perhaps have an 'is_system_temp_file' parameter to conditionally send that
ipc?

we could also send it for all PP_FILESYSTEMTYPE_EXTERNAL files, and let the code
in the browser process figure out which ones are temporary.

[jianli, 2010-09-02 20:46:21]

Some comments.

http://codereview.chromium.org/3165062/diff/16001/14006
File base/deletable_file_reference.h (right):

http://codereview.chromium.org/3165062/diff/16001/14006#newcode21
base/deletable_file_reference.h:21: // for this patch exists returns NULL.
patch => path

http://codereview.chromium.org/3165062/diff/16001/14006#newcode22
base/deletable_file_reference.h:22: static DeletableFileReference*
GetReference(const FilePath& path);
Since this class is ref counted, probably it is better to return as
scoped_refptr<>.

http://codereview.chromium.org/3165062/diff/16001/14006#newcode26
base/deletable_file_reference.h:26: static DeletableFileReference*
GetOrCreateReference(
ditto.

http://codereview.chromium.org/3165062/diff/16001/14006#newcode39
base/deletable_file_reference.h:39: const FilePath path_;
Why adding const here?

http://codereview.chromium.org/3165062/diff/16001/14008
File chrome/browser/renderer_host/redirect_to_file_resource_handler.h (right):

http://codereview.chromium.org/3165062/diff/16001/14008#newcode83
chrome/browser/renderer_host/redirect_to_file_resource_handler.h:83: // an
additional reference to the DeletableFileReference.
This probably sounds too much insider knowledge for the clients. Could we
consider exposing a couple methods for this purpose, like KeepAlive() and
Revoke()?

http://codereview.chromium.org/3165062/diff/16001/14009
File webkit/blob/blob_data.h (right):

http://codereview.chromium.org/3165062/diff/16001/14009#newcode70
webkit/blob/blob_data.h:70: DeletableFileReference::GetReference(file_path);
On 2010/09/01 22:26:08, michaeln wrote:
> The trick is going to be to get this line to execute prior to
> ~RedirectToFileResourceHandler() when doing xhr.asBlob in workers. 

Can you explain it in more detail? Why do we need this to happen before
~RedirectToFileResourceHandler()?

Also, for other case, how do we avoid getting the file deleted?

[michaeln, 2010-09-02 21:39:39]

thnx for looking

http://codereview.chromium.org/3165062/diff/16001/14006
File base/deletable_file_reference.h (right):

http://codereview.chromium.org/3165062/diff/16001/14006#newcode16
base/deletable_file_reference.h:16: // A refcounted wrapper around a FilePath
that schedules the file
On 2010/09/02 20:15:20, dumi wrote:
> probably need some comments that this class is not thread-safe.

i think the opposite... unless otherwise noted, classes are NOT thread safe in
the chrome code base

http://codereview.chromium.org/3165062/diff/16001/14006#newcode22
base/deletable_file_reference.h:22: static DeletableFileReference*
GetReference(const FilePath& path);
On 2010/09/02 20:46:21, jianli wrote:
> Since this class is ref counted, probably it is better to return as
> scoped_refptr<>.

Will do.

http://codereview.chromium.org/3165062/diff/16001/14006#newcode36
base/deletable_file_reference.h:36: const FilePath& path,
base::MessageLoopProxy* file_thread);
On 2010/09/02 20:15:20, dumi wrote:
> pass in a scoped_refptr<base::MessageLoopProxy> instead of a raw pointer?

i don't see why, just more refcount churn

http://codereview.chromium.org/3165062/diff/16001/14006#newcode39
base/deletable_file_reference.h:39: const FilePath path_;
On 2010/09/02 20:46:21, jianli wrote:
> Why adding const here?

because it doesn't change, there is a seperate 'key' in the map that must
correspond with this value... i don't want it to be changeable and i'm
expressing that in the class definition

http://codereview.chromium.org/3165062/diff/16001/14008
File chrome/browser/renderer_host/redirect_to_file_resource_handler.h (right):

http://codereview.chromium.org/3165062/diff/16001/14008#newcode83
chrome/browser/renderer_host/redirect_to_file_resource_handler.h:83: // an
additional reference to the DeletableFileReference.
On 2010/09/02 20:46:21, jianli wrote:
> This probably sounds too much insider knowledge for the clients. Could we
> consider exposing a couple methods for this purpose, like KeepAlive() and
> Revoke()?

Maybe, I'll have to get further along to see about putting syntactic sugar on
this. If ResourceRequests produced "blobs" instead of just providing a file
path, this might be cleaner (but that would also have some problems with
figuring out when it would be OK to 'unregister' a blob produced by the
DownloadToBlobHandler.

http://codereview.chromium.org/3165062/diff/16001/14009
File webkit/blob/blob_data.h (right):

http://codereview.chromium.org/3165062/diff/16001/14009#newcode70
webkit/blob/blob_data.h:70: DeletableFileReference::GetReference(file_path);
> Can you explain it in more detail? Why do we need this to happen before
> ~RedirectToFileResourceHandler()?

If no other DeletableFileReference is taken, the file will get deleted when
RedirectToFileResourceHandler is deleted.

Our system design calls for this "raciness". ResourceRequest can produce a file,
the client has to wrap a Blob around that file on its own. The blob reference
will keep the file alive, but that blob reference has to be created prior to the
RedirectToFileResourceHandler that produced the temp file goes away.

> Also, for other case, how do we avoid getting the file deleted?

You mean the pepper case? A FileRef will keep the temp file alive (similar to
how a Blob keeps the temp file alive).

[michaeln, 2010-09-04 00:04:25]

http://codereview.chromium.org/3165062/diff/16001/14005
File base/deletable_file_reference.cc (right):

http://codereview.chromium.org/3165062/diff/16001/14005#newcode49
base/deletable_file_reference.cc:49: if (file_thread_)
On 2010/09/02 20:15:20, dumi wrote:
> On 2010/09/01 22:26:08, michaeln wrote:
> > should just be a required param
> 
> i agree. add a DCHECK() to Get{OrCreate}Reference?

Done.

http://codereview.chromium.org/3165062/diff/16001/14006
File base/deletable_file_reference.h (right):

http://codereview.chromium.org/3165062/diff/16001/14006#newcode21
base/deletable_file_reference.h:21: // for this patch exists returns NULL.
On 2010/09/02 20:46:21, jianli wrote:
> patch => path

Done.

http://codereview.chromium.org/3165062/diff/16001/14006#newcode22
base/deletable_file_reference.h:22: static DeletableFileReference*
GetReference(const FilePath& path);
On 2010/09/02 20:46:21, jianli wrote:
> Since this class is ref counted, probably it is better to return as
> scoped_refptr<>.

Done.

http://codereview.chromium.org/3165062/diff/16001/14006#newcode26
base/deletable_file_reference.h:26: static DeletableFileReference*
GetOrCreateReference(
On 2010/09/02 20:46:21, jianli wrote:
> ditto.

Done.

[michaeln, 2010-09-07 22:17:22]

http://codereview.chromium.org/3165062/diff/30001/31009
File webkit/glue/plugins/pepper_file_ref.cc (right):

http://codereview.chromium.org/3165062/diff/30001/31009#newcode136
webkit/glue/plugins/pepper_file_ref.cc:136: // TODO(michaeln): ipc to maybe
addRef the remote DeleteableFileReference
Q: Where would be a good place to define helper functions to send these IPCs to
the browser process on behalf of pepper FileRef objects? We're in the
'webkit_glue' here so i'm looking for a good place to poke holes out into
chrome? An instance method on WebURLLoader to send the addRef ipc could work,
but that doesn't help in the releaseRef case.

Some options...

1. Static methods on WebURLLoader could work for both the addRef and releaseRef
cases.

2. Add two new methods to WebKitClient for this purpose.

3. Implement this in the plugin case using chrome's Blob handling
infrastructure. So here in the ctor, call...
   registered_blob_id_ = make up a unique blob id;
   blobRegistry->registerBlob(registered_blob_id_, WebBlobData&)

I'm leaning towards options 3.

http://codereview.chromium.org/3165062/diff/30001/31009#newcode141
webkit/glue/plugins/pepper_file_ref.cc:141: //   ipc to maybe releaseRef the
remote DeleteableFileReference
... and here in the dtor, calls...
   blobRegistry->registerBlobURL(registered_blob_id_);

[dumi, 2010-09-07 23:24:03]

On Tue, Sep 7, 2010 at 3:17 PM, <michaeln@chromium.org> wrote:

>
> http://codereview.chromium.org/3165062/diff/30001/31009
> File webkit/glue/plugins/pepper_file_ref.cc (right):
>
> http://codereview.chromium.org/3165062/diff/30001/31009#newcode136
>
> webkit/glue/plugins/pepper_file_ref.cc:136: // TODO(michaeln): ipc to
> maybe addRef the remote DeleteableFileReference
> Q: Where would be a good place to define helper functions to send these
> IPCs to the browser process on behalf of pepper FileRef objects? We're
> in the 'webkit_glue' here so i'm looking for a good place to poke holes
> out into chrome? An instance method on WebURLLoader to send the addRef
> ipc could work, but that doesn't help in the releaseRef case.
>
> Some options...
>
> 1. Static methods on WebURLLoader could work for both the addRef and
> releaseRef cases.
>
> 2. Add two new methods to WebKitClient for this purpose.
>
> 3. Implement this in the plugin case using chrome's Blob handling
> infrastructure. So here in the ctor, call...
>   registered_blob_id_ = make up a unique blob id;
>   blobRegistry->registerBlob(registered_blob_id_, WebBlobData&)
>
> I'm leaning towards options 3.
>

there's webkit/glue/plugins/pepper_plugin_delegate.h, that's implemented by
chrome/renderer/pepper_plugin_delegate_impl.{h|cc}. we could add the call
there. also, for my patch, i've added a
chrome/renderer/pepper_message_filter.{h|cc} that is supposed to send
pepper's "open file" IPCs and process the responses. assuming darin will be
ok with this new message filter, you could use it to send your IPCs too.



> http://codereview.chromium.org/3165062/diff/30001/31009#newcode141
>
> webkit/glue/plugins/pepper_file_ref.cc:141: //   ipc to maybe releaseRef
> the remote DeleteableFileReference
> ... and here in the dtor, calls...
>   blobRegistry->registerBlobURL(registered_blob_id_);
>
>
> http://codereview.chromium.org/3165062/show
>

[michaeln, 2010-09-07 23:46:08]

> > 3. Implement this in the plugin case using chrome's Blob handling
> > infrastructure. So here in the ctor, call...
> >   registered_blob_id_ = make up a unique blob id;
> >   blobRegistry->registerBlob(registered_blob_id_, WebBlobData&)
> >
> > I'm leaning towards options 3.
> 
> there's webkit/glue/plugins/pepper_plugin_delegate.h, that's implemented by
> chrome/renderer/pepper_plugin_delegate_impl.{h|cc}. we could add the call
> there. also, for my patch, i've added a
> chrome/renderer/pepper_message_filter.{h|cc} that is supposed to send
> pepper's "open file" IPCs and process the responses. assuming darin will be
> ok with this new message filter, you could use it to send your IPCs too.

Thnx for the ptr dumi.

If we go with option 3 (overload blob infrastructure for this), i shouldn't need
to add any extra api/plumbing beyond what is already there for blobs. That's why
i'm leaning towards option 3.

I've run into a snap though... do we have a uuid/guid generator (or interface to
one) in the chrome code base?

[michaeln, 2010-09-07 23:51:37]

http://codereview.chromium.org/3165062/diff/22002/39009
File webkit/glue/plugins/pepper_file_ref.cc (right):

http://codereview.chromium.org/3165062/diff/22002/39009#newcode150
webkit/glue/plugins/pepper_file_ref.cc:150: registered_blob_id_ = GURL(); // how
do we generate guid's in chrome?
see... here's the snag

[dumi, 2010-09-07 23:55:40]

i don't think we have a ID generator. i usually use
base/atomic_sequence_num.h for this.

On Tue, Sep 7, 2010 at 4:51 PM, <michaeln@chromium.org> wrote:

>
> http://codereview.chromium.org/3165062/diff/22002/39009
>
> File webkit/glue/plugins/pepper_file_ref.cc (right):
>
> http://codereview.chromium.org/3165062/diff/22002/39009#newcode150
> webkit/glue/plugins/pepper_file_ref.cc:150: registered_blob_id_ =
> GURL(); // how do we generate guid's in chrome?
> see... here's the snag
>
>
> http://codereview.chromium.org/3165062/show
>

[michaeln, 2010-09-08 00:00:37]

On 2010/09/07 23:55:40, dumi wrote:
> i don't think we have a ID generator. i usually use
> base/atomic_sequence_num.h for this.

Of course i don't really need the "global" part of the unique id, but i do need
it to be unique across the entire browser (all processes).

[darin (slow to review), 2010-09-08 00:03:37]

http://codereview.chromium.org/3165062/diff/22002/39007
File webkit/glue/plugins/pepper_file_io.cc (right):

http://codereview.chromium.org/3165062/diff/22002/39007#newcode225
webkit/glue/plugins/pepper_file_io.cc:225: opened_file_ = file_ref;
i think you meant to assign NULL here

http://codereview.chromium.org/3165062/diff/46001/47002
File base/deletable_file_reference.cc (right):

http://codereview.chromium.org/3165062/diff/46001/47002#newcode37
base/deletable_file_reference.cc:37: DeletableFileReference* reference =
GetReference(path);
nit: you could optimize this code slightly by using the form of insert that
tells you if it found a match or not so you would only need to traverse the tree
once.

http://codereview.chromium.org/3165062/diff/46001/47006
File webkit/blob/blob_data.h (right):

http://codereview.chromium.org/3165062/diff/46001/47006#newcode89
webkit/blob/blob_data.h:89: scoped_refptr<DeletableFileReference>
deletable_file_reference_;
i'm not too crazy about this "behind the scenes" way of keeping the file path
valid.  a more explicit temp file manager seems better to me that would only be
used when a WebURLRequest has downloadToFile set.  we don't need this for every
blob in other words.

http://codereview.chromium.org/3165062/diff/46001/47009
File webkit/glue/plugins/pepper_file_ref.cc (right):

http://codereview.chromium.org/3165062/diff/46001/47009#newcode144
webkit/glue/plugins/pepper_file_ref.cc:144: if (fs_type_ ==
PP_FILESYSTEMTYPE_EXTERNAL) {
this seems like a pretty gross hack.  i'm also not convinced that this approach
is sufficient.  there is a race condition between the
RedirectToFileResourceHandler seeing OnRequestClosed and this FileRef object
getting created and sending the FilePath back up to the browser so that the file
can be retained.

i think a better approach would be to have destruction of the WebURLLoader
result in an IPC being sent up to the browser so that we can use that as a
signal that the associated temp file is no longer needed.

the pepper api already tells people to retain a reference to the Pepper
URLLoader to keep the temp file alive.

[michaeln, 2010-09-08 00:09:47]

On 2010/09/08 00:00:37, michaeln wrote:
> On 2010/09/07 23:55:40, dumi wrote:
> > i don't think we have a ID generator. i usually use
> > base/atomic_sequence_num.h for this.
> 
> Of course i don't really need the "global" part of the unique id, but i do
need
> it to be unique across the entire browser (all processes).

Is the browser-wide unique process id that's assinged by the browser process to
each child process readily available in the child process? Given that i could
easily construct a browser-wide unique tuple <processId, ++nextLocalId>.

[michaeln, 2010-09-08 00:23:01]

> http://codereview.chromium.org/3165062/diff/46001/47009#newcode144
> webkit/glue/plugins/pepper_file_ref.cc:144: if (fs_type_ ==
> PP_FILESYSTEMTYPE_EXTERNAL) {
> this seems like a pretty gross hack.  i'm also not convinced that this
approach
> is sufficient.  there is a race condition between the
> RedirectToFileResourceHandler seeing OnRequestClosed and this FileRef object
> getting created and sending the FilePath back up to the browser so that the
file
> can be retained.
> 
> i think a better approach would be to have destruction of the WebURLLoader
> result in an IPC being sent up to the browser so that we can use that as a
> signal that the associated temp file is no longer needed.
> 
> the pepper api already tells people to retain a reference to the Pepper
> URLLoader to keep the temp file alive.

Thanx for looking.

So long as we're happy with the "keep the URLLoader around for as long as you
need access to the file" semantics... SGTM.

[darin (slow to review), 2010-09-08 05:20:04]

On Tue, Sep 7, 2010 at 5:23 PM, <michaeln@chromium.org> wrote:

> http://codereview.chromium.org/3165062/diff/46001/47009#newcode144
>> webkit/glue/plugins/pepper_file_ref.cc:144: if (fs_type_ ==
>> PP_FILESYSTEMTYPE_EXTERNAL) {
>> this seems like a pretty gross hack.  i'm also not convinced that this
>>
> approach
>
>> is sufficient.  there is a race condition between the
>> RedirectToFileResourceHandler seeing OnRequestClosed and this FileRef
>> object
>> getting created and sending the FilePath back up to the browser so that
>> the
>>
> file
>
>> can be retained.
>>
>
>  i think a better approach would be to have destruction of the WebURLLoader
>> result in an IPC being sent up to the browser so that we can use that as a
>> signal that the associated temp file is no longer needed.
>>
>
>  the pepper api already tells people to retain a reference to the Pepper
>> URLLoader to keep the temp file alive.
>>
>
> Thanx for looking.
>
> So long as we're happy with the "keep the URLLoader around for as long as
> you
> need access to the file" semantics... SGTM.


Do you think it will make the WebCore side ugly?
-Darin



>
>
> http://codereview.chromium.org/3165062/show
>

[michaeln, 2010-09-08 05:38:14]

> Do you think it will make the WebCore side ugly?
> -Darin

I still have in mind to xfer ownership to a blob in the xhr.responseBlob case
(and maybe even in the pepper case).

I just don't quite see the relationship between URLRequest lifetime (and its
handlers) in the browser vs IPCResourceLoaderBridge lifetime in the child
process. It looks like there really isn't any coordination around end of life.
When the browser is done streaming the response back... the URLRequest (and
handlers) evaporate... and the IPCResourceLoaderBridge can go away at any time
independently (prior to or after URLRequest completion)... very loosely tied
together... at least that's what it looks like.

I kind of want an 'ack' sent back to the server in response to ResponseStarted
so I know that the client side will eventually get around to calling
"DoneWithTempFile" and have the URLRequest stay valid at least until then...
otherwise the client side may have walked away prior to seeing the
headers/filepath, but the browser may have already deleted the request (and
handlers).

This is gunky.

[michaeln, 2010-09-08 05:53:08]

> and have the URLRequest stay valid at least until then

at least until the ResponseStartedAck

... but i'm also very hesitant about change the lifecycle of these things

[michaeln, 2010-09-08 06:18:01]

Maybe we could effectively get that ACK w/o altering lifecycles.

// browser
0. handler creates a temp file and hold a ref until the handler is deleted, this
ref will protect it from deletion until at least that time.
1. send browser->client responseStarted
2. add a reference to the temp file to a per process collection of temp files
created on behalf of requests (per process to delete them upon child process
termination). The temp file is protected from deletion while in the collection.
3. send browser->client tempFileCreated message (this message does not follow
terminate in the client on an instance of IPCResourceLoaderBridge, it lands on a
singleton object)

// child
4. the client receives the responseStarted message and IPCResourceLoaderBridge
should wrap a blob around the file path if it wants to hold onto it.
5. the client receives the tempFileCreated message, and simply ACKs receipt of
that message, sends client->browser tempFileCreateACK (this message does not
terminate on an instance of a ResourceHandler chain) Note: this ACK gets sent
even if the bridge has been deleted already.

// browser
6. the server receives the Blob related messages and takes a reference to the
temp file protecting it from deletion.
7. the server receives the  tempFileCreatedACK and removes the temp file from
the per process collection in #2. Note: this step occurs if the request is still
active or not.

[michaeln, 2010-09-11 00:05:43]

Initially, this CL was just a simple DeletableFileReference class . The hope was
that that simple class could be worked into chrome and test_shell to manage the
lifetimes of temp files downloaded via URLLoaders. That system to do that is
somewhat less simple than the simple refcounted class.

There are two 'customers' for this stuff, pepper and xhr.responseBlob. Here's a
statement of what I'm shooting for with this CL.

* Add reasonable lifetime semantics to the recently added DownloadToFile
capability of WebURLLoader. The temp file should remain valid until WebURLLoader
is deleted. In terms of WebCore speak, the temp file should remain valid until
the ResourceHandle is deleted.

* If a WebCore Blob that references the temp file is registered prior to the
ResourceHandle having been deleted, the lifetime will be extended to the
lifetime of the Blob or any slices/clones of that Blob created thereafter.

[michaeln, 2010-09-11 00:07:10]

... and make that work in chrome and test_shell

[michaeln, 2010-09-14 01:31:34]

On 2010/09/11 00:05:43, michaeln wrote:
> There are two 'customers' for this stuff, pepper and xhr.responseBlob. Here's
a
> statement of what I'm shooting for with this CL.
> 
> * Add reasonable lifetime semantics to the recently added DownloadToFile
> capability of WebURLLoader. The temp file should remain valid until
WebURLLoader
> is deleted. In terms of WebCore speak, the temp file should remain valid until
> the ResourceHandle is deleted.
> 
> * If a WebCore Blob that references the temp file is registered prior to the
> ResourceHandle having been deleted, the lifetime will be extended to the
> lifetime of the Blob or any slices/clones of that Blob created thereafter.

That's a reasonable lifetime model for pepper's use case, but it doesn't work
for webcore's xhr.responseBlob. With sync xhr's, thre is no ResourceHandle
instance and the WebURLLoader created under the covers is deleted prior to
return from the static ResourceHandle::loadResourceSynchronously() method.

I'm going to bail on dealing with xhr.responseBlob in the CL and just focus on
the pepper use case.

[michaeln, 2010-09-15 01:52:35]

So I've backed up and just implemented the 'download_to_file' semantics for
chrome and test_shell where the downloaded file lives as long as the URLLoader
lives.

http://codereview.chromium.org/3165062/diff/46001/47002
File base/deletable_file_reference.cc (right):

http://codereview.chromium.org/3165062/diff/46001/47002#newcode37
base/deletable_file_reference.cc:37: DeletableFileReference* reference =
GetReference(path);
On 2010/09/08 00:03:37, darin wrote:
> nit: you could optimize this code slightly by using the form of insert that
> tells you if it found a match or not so you would only need to traverse the
tree
> once.

Done.

http://codereview.chromium.org/3165062/diff/125001/90014
File chrome/browser/renderer_host/resource_handler.h (right):

http://codereview.chromium.org/3165062/diff/125001/90014#newcode83
chrome/browser/renderer_host/resource_handler.h:83: virtual void
OnDataDownloaded(int request_id, int bytes_downloaded) {}
this s/b pure virtual and could use some doc comments

http://codereview.chromium.org/3165062/diff/125001/90020
File webkit/tools/test_shell/simple_resource_loader_bridge.cc (right):

http://codereview.chromium.org/3165062/diff/125001/90020#newcode311
webkit/tools/test_shell/simple_resource_loader_bridge.cc:311: path,
base::PLATFORM_FILE_CREATE | base::PLATFORM_FILE_WRITE);
what about errors?

[michaeln, 2010-09-15 03:45:25]

trybots are looking happy with this

[michaeln, 2010-09-16 03:16:21]

please take a look

[jianli, 2010-09-16 22:05:54]

http://codereview.chromium.org/3165062/diff/141001/79006
File base/deletable_file_reference.h (right):

http://codereview.chromium.org/3165062/diff/141001/79006#newcode20
base/deletable_file_reference.h:20: // Returns the DeletableFileReference for
the given path, if no reference
the => a

http://codereview.chromium.org/3165062/diff/141001/79006#newcode22
base/deletable_file_reference.h:22: static scoped_refptr<DeletableFileReference>
GetReference(
Maybe better to rename it to GetInstance?

http://codereview.chromium.org/3165062/diff/141001/79006#newcode27
base/deletable_file_reference.h:27: static scoped_refptr<DeletableFileReference>
GetOrCreateReference(
ditto.

http://codereview.chromium.org/3165062/diff/141001/79013
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/3165062/diff/141001/79013#newcode577
chrome/browser/renderer_host/resource_dispatcher_host.cc:577:
registered_temp_files_[receiver_id][request_id] = reference;
I did not see any place to set up and tear down the entry for a particular
receiver_id?

http://codereview.chromium.org/3165062/diff/141001/79014
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/3165062/diff/141001/79014#newcode270
chrome/browser/renderer_host/resource_dispatcher_host.h:270: // child process
and to defer deletion of the file until its
its => it's

http://codereview.chromium.org/3165062/diff/141001/79020
File chrome/common/resource_dispatcher.cc (right):

http://codereview.chromium.org/3165062/diff/141001/79020#newcode147
chrome/common/resource_dispatcher.cc:147: // Tell the browser process we're
deleted so it can reclaim resources its
its => it's

http://codereview.chromium.org/3165062/diff/141001/79020#newcode420
chrome/common/resource_dispatcher.cc:420: void
ResourceDispatcher::OnDownloadedData(const IPC::Message& message,
This seems to be very similar to OnReceivedData. Is it possible to merge these
two so that we can avoid some duplicate code?

http://codereview.chromium.org/3165062/diff/141001/79020#newcode437
chrome/common/resource_dispatcher.cc:437: if (data_len > 0) {
What does data_len == 0 mean?

http://codereview.chromium.org/3165062/diff/141001/79023
File webkit/tools/test_shell/simple_resource_loader_bridge.cc (right):

http://codereview.chromium.org/3165062/diff/141001/79023#newcode38
webkit/tools/test_shell/simple_resource_loader_bridge.cc:38: #include
"base/file_util.h"
Wrong order.

[michaeln, 2010-09-16 22:32:33]

thnx for looking, i'll ping you again when i have a new snapshot

http://codereview.chromium.org/3165062/diff/125002/113007
File chrome/browser/net/blob_url_request_job_factory.cc (right):

http://codereview.chromium.org/3165062/diff/125002/113007#newcode18
chrome/browser/net/blob_url_request_job_factory.cc:18:
ResourceDispatcherHost::InfoForRequest(request);
Fyi: this change was made so blob url requests would work even in the face of
deferred url job creation/starting that RDH can impose.

A function like the following would fail otherwise.

function xhrGetAsBlob() {
  if (!blob)
    buildABlob();
  var url = createBlobURL(blob);
  xhr = new XMLHttpRequest();
  xhr.open("GET", url, true);
  xhr.asBlob = true;
  xhr.onreadystatechange = function() { if (xhr.readyState == 4) {
alert('done'); blob = xhr.responseBlob; } }
  xhr.send();
  revokeBlobURL(url);
}

http://codereview.chromium.org/3165062/diff/141001/79006
File base/deletable_file_reference.h (right):

http://codereview.chromium.org/3165062/diff/141001/79006#newcode22
base/deletable_file_reference.h:22: static scoped_refptr<DeletableFileReference>
GetReference(
On 2010/09/16 22:05:55, jianli wrote:
> Maybe better to rename it to GetInstance?

Does that sound too singleton'ish? What would you think of simply Get() and
GetOrCreate()?

http://codereview.chromium.org/3165062/diff/141001/79007
File base/deletable_file_reference_unittest.cc (right):

http://codereview.chromium.org/3165062/diff/141001/79007#newcode14
base/deletable_file_reference_unittest.cc:14: ScopedTempDir temp_dir;
i guess i don't really need the ScopedTempDir here

http://codereview.chromium.org/3165062/diff/141001/79013
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/3165062/diff/141001/79013#newcode577
chrome/browser/renderer_host/resource_dispatcher_host.cc:577:
registered_temp_files_[receiver_id][request_id] = reference;
On 2010/09/16 22:05:55, jianli wrote:
> I did not see any place to set up and tear down the entry for a particular
> receiver_id?

Setup happens implicitly on first access to map[key], tear down happens in
CancelRequestsForProcess (down below).

http://codereview.chromium.org/3165062/diff/141001/79020
File chrome/common/resource_dispatcher.cc (right):

http://codereview.chromium.org/3165062/diff/141001/79020#newcode420
chrome/common/resource_dispatcher.cc:420: void
ResourceDispatcher::OnDownloadedData(const IPC::Message& message,
On 2010/09/16 22:05:55, jianli wrote:
> This seems to be very similar to OnReceivedData. Is it possible to merge these
> two so that we can avoid some duplicate code? 

I'll poke at eliminating the duplicate blocks of this form throughout the class.

   PendingRequestList::iterator it = pending_requests_.find(request_id);
   if (it == pending_requests_.end()) {
     // this might happen for kill()ed requests on the webkit end, so perhaps
     // it shouldn't be a warning...
     DLOG(WARNING) << "Got data for a nonexistent or finished request";
     return;
   }
   PendingRequestInfo& request_info = it->second;

http://codereview.chromium.org/3165062/diff/141001/79020#newcode437
chrome/common/resource_dispatcher.cc:437: if (data_len > 0) {
On 2010/09/16 22:05:55, jianli wrote:
> What does data_len == 0 mean?

Good question? I'll look into eliminating calls to this method with non-positive
values.

http://codereview.chromium.org/3165062/diff/141001/79023
File webkit/tools/test_shell/simple_resource_loader_bridge.cc (right):

http://codereview.chromium.org/3165062/diff/141001/79023#newcode38
webkit/tools/test_shell/simple_resource_loader_bridge.cc:38: #include
"base/file_util.h"
On 2010/09/16 22:05:55, jianli wrote:
> Wrong order.

ooops, thnx

[michaeln, 2010-09-16 23:48:01]

New snapshot after sync/merging and other changes mentioned below.

http://codereview.chromium.org/3165062/diff/141001/79006
File base/deletable_file_reference.h (right):

http://codereview.chromium.org/3165062/diff/141001/79006#newcode20
base/deletable_file_reference.h:20: // Returns the DeletableFileReference for
the given path, if no reference
On 2010/09/16 22:05:55, jianli wrote:
> the => a

Done.

http://codereview.chromium.org/3165062/diff/141001/79006#newcode22
base/deletable_file_reference.h:22: static scoped_refptr<DeletableFileReference>
GetReference(
On 2010/09/16 22:32:34, michaeln wrote:
> On 2010/09/16 22:05:55, jianli wrote:
> > Maybe better to rename it to GetInstance?
> 
> Does that sound too singleton'ish? What would you think of simply Get() and
> GetOrCreate()?

Done.

http://codereview.chromium.org/3165062/diff/141001/79007
File base/deletable_file_reference_unittest.cc (right):

http://codereview.chromium.org/3165062/diff/141001/79007#newcode14
base/deletable_file_reference_unittest.cc:14: ScopedTempDir temp_dir;
On 2010/09/16 22:32:34, michaeln wrote:
> i guess i don't really need the ScopedTempDir here

Done.

http://codereview.chromium.org/3165062/diff/141001/79014
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/3165062/diff/141001/79014#newcode270
chrome/browser/renderer_host/resource_dispatcher_host.h:270: // child process
and to defer deletion of the file until its
On 2010/09/16 22:05:55, jianli wrote:
> its => it's

Done.

http://codereview.chromium.org/3165062/diff/141001/79020
File chrome/common/resource_dispatcher.cc (right):

http://codereview.chromium.org/3165062/diff/141001/79020#newcode147
chrome/common/resource_dispatcher.cc:147: // Tell the browser process we're
deleted so it can reclaim resources its
On 2010/09/16 22:05:55, jianli wrote:
> its => it's

But.. the possessive term shouldn't contain punctuation.

http://codereview.chromium.org/3165062/diff/141001/79020#newcode420
chrome/common/resource_dispatcher.cc:420: void
ResourceDispatcher::OnDownloadedData(const IPC::Message& message,
On 2010/09/16 22:32:34, michaeln wrote:
> On 2010/09/16 22:05:55, jianli wrote:
> > This seems to be very similar to OnReceivedData. Is it possible to merge
these
> > two so that we can avoid some duplicate code? 
> 
> I'll poke at eliminating the duplicate blocks of this form throughout the
class.
> 
>    PendingRequestList::iterator it = pending_requests_.find(request_id);
>    if (it == pending_requests_.end()) {
>      // this might happen for kill()ed requests on the webkit end, so perhaps
>      // it shouldn't be a warning...
>      DLOG(WARNING) << "Got data for a nonexistent or finished request";
>      return;
>    }
>    PendingRequestInfo& request_info = it->second;

Done.

http://codereview.chromium.org/3165062/diff/141001/79020#newcode437
chrome/common/resource_dispatcher.cc:437: if (data_len > 0) {
On 2010/09/16 22:32:34, michaeln wrote:
> On 2010/09/16 22:05:55, jianli wrote:
> > What does data_len == 0 mean?
> 
> Good question? I'll look into eliminating calls to this method with
non-positive
> values.
> 

Done.

http://codereview.chromium.org/3165062/diff/141001/79023
File webkit/tools/test_shell/simple_resource_loader_bridge.cc (right):

http://codereview.chromium.org/3165062/diff/141001/79023#newcode38
webkit/tools/test_shell/simple_resource_loader_bridge.cc:38: #include
"base/file_util.h"
On 2010/09/16 22:05:55, jianli wrote:
> Wrong order.

Done.

[jianli, 2010-09-17 21:59:01]

LGTM.

http://codereview.chromium.org/3165062/diff/79024/146003
File base/deletable_file_reference.cc (right):

http://codereview.chromium.org/3165062/diff/79024/146003#newcode34
base/deletable_file_reference.cc:34: DeletableFileReference::GetOrCreate(
Can this line be appended to the previous line?

http://codereview.chromium.org/3165062/diff/79024/146004
File base/deletable_file_reference.h (right):

http://codereview.chromium.org/3165062/diff/79024/146004#newcode23
base/deletable_file_reference.h:23: const FilePath& path);
Can this line be appended to previous line?

[michaeln, 2010-09-17 22:04:31]

Thnx

http://codereview.chromium.org/3165062/diff/79024/146003
File base/deletable_file_reference.cc (right):

http://codereview.chromium.org/3165062/diff/79024/146003#newcode34
base/deletable_file_reference.cc:34: DeletableFileReference::GetOrCreate(
On 2010/09/17 21:59:01, jianli wrote:
> Can this line be appended to the previous line?

Done.

http://codereview.chromium.org/3165062/diff/79024/146004
File base/deletable_file_reference.h (right):

http://codereview.chromium.org/3165062/diff/79024/146004#newcode23
base/deletable_file_reference.h:23: const FilePath& path);
On 2010/09/17 21:59:01, jianli wrote:
> Can this line be appended to previous line?

Done.

[darin (slow to review), 2010-09-17 22:47:49]

very nice work getting all of this hooked up to testshell too!

http://codereview.chromium.org/3165062/diff/79024/146004
File base/deletable_file_reference.h (right):

http://codereview.chromium.org/3165062/diff/79024/146004#newcode18
base/deletable_file_reference.h:18: class DeletableFileReference : public
base::RefCounted<DeletableFileReference> {
i realize that you are placing this in base because it has no dependencies on
anything but base and it may actually be useful to others, but...  there's also
the competing pressure to not make base be a dumping ground.  if we don't need
something to be in base then we should not put it there.

how about putting this in src/webkit/fileapi/ instead?

why is this thread-safe ref counted?  it seems like it is implemented using a
std::map that is not protected by locks, so passing DeletableFileReference
between threads would probably be bad.

http://codereview.chromium.org/3165062/diff/79024/146011
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/3165062/diff/79024/146011#newcode520
chrome/browser/renderer_host/resource_dispatcher_host.cc:520: if
(request->url().SchemeIs(chrome::kBlobScheme) && context) {
BeginRequest is frighteningly large and complicated.  adding even more code to
it makes me cry.

http://codereview.chromium.org/3165062/diff/79024/146011#newcode888
chrome/browser/renderer_host/resource_dispatcher_host.cc:888:
registered_temp_files_.erase(child_id);
i see... this is the reason for the nested maps.

http://codereview.chromium.org/3165062/diff/79024/146012
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/3165062/diff/79024/146012#newcode452
chrome/browser/renderer_host/resource_dispatcher_host.h:452: typedef
std::map<int, DeletableFilesMap>
you wouldn't want to just use a GlobalRequestID as the key here?

http://codereview.chromium.org/3165062/diff/79024/146014
File chrome/browser/renderer_host/resource_dispatcher_host_request_info.h
(right):

http://codereview.chromium.org/3165062/diff/79024/146014#newcode23
chrome/browser/renderer_host/resource_dispatcher_host_request_info.h:23:
namespace webkit_blob { class BlobData; }
nit: usually written as:

namespace foo {
class Bar;
}

that way it does not need to be reformatted when we need to forward declare
something else in the "foo" namespace.

http://codereview.chromium.org/3165062/diff/79024/146017
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/3165062/diff/79024/146017#newcode327
chrome/common/render_messages_internal.h:327: 
nit: no new line here

http://codereview.chromium.org/3165062/diff/79024/146018
File chrome/common/resource_dispatcher.cc (right):

http://codereview.chromium.org/3165062/diff/79024/146018#newcode322
chrome/common/resource_dispatcher.cc:322:
ResourceDispatcher::GetPendingRequestInfo(int request_id) {
nice!

[michaeln, 2010-09-18 00:18:38]

haven't uploaded a new snapshot yet, just replying to comments

http://codereview.chromium.org/3165062/diff/79024/146004
File base/deletable_file_reference.h (right):

http://codereview.chromium.org/3165062/diff/79024/146004#newcode18
base/deletable_file_reference.h:18: class DeletableFileReference : public
base::RefCounted<DeletableFileReference> {
On 2010/09/17 22:47:50, darin wrote:
> how about putting this in src/webkit/fileapi/ instead?

Might be nice to have a webkit/base directory. There are utilities shared across
our modules in there, but no good home for such things. I'll see how well this
fits in the fileapi folder?

> why is this thread-safe ref counted?  it seems like it is implemented using a
> std::map that is not protected by locks, so passing DeletableFileReference
> between threads would probably be bad.

base::RefCounted<DeletableFileReference>

It is NOT thread-safe, yes passing between threads would be bad, that shouldn't
be happening.

http://codereview.chromium.org/3165062/diff/79024/146011
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/3165062/diff/79024/146011#newcode520
chrome/browser/renderer_host/resource_dispatcher_host.cc:520: if
(request->url().SchemeIs(chrome::kBlobScheme) && context) {
On 2010/09/17 22:47:50, darin wrote:
> BeginRequest is frighteningly large and complicated.  adding even more code to
> it makes me cry.

I know what you mean. Any suggestions?

http://codereview.chromium.org/3165062/diff/79024/146012
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/3165062/diff/79024/146012#newcode452
chrome/browser/renderer_host/resource_dispatcher_host.h:452: typedef
std::map<int, DeletableFilesMap>
On 2010/09/17 22:47:50, darin wrote:
> you wouldn't want to just use a GlobalRequestID as the key here?

makes it easier to deal with process died with the map of maps

http://codereview.chromium.org/3165062/diff/79024/146014
File chrome/browser/renderer_host/resource_dispatcher_host_request_info.h
(right):

http://codereview.chromium.org/3165062/diff/79024/146014#newcode23
chrome/browser/renderer_host/resource_dispatcher_host_request_info.h:23:
namespace webkit_blob { class BlobData; }
On 2010/09/17 22:47:50, darin wrote:
> nit: usually written as:
> 
> namespace foo {
> class Bar;
> }
> 
> that way it does not need to be reformatted when we need to forward declare
> something else in the "foo" namespace.

Done.

http://codereview.chromium.org/3165062/diff/79024/146017
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/3165062/diff/79024/146017#newcode327
chrome/common/render_messages_internal.h:327: 
On 2010/09/17 22:47:50, darin wrote:
> nit: no new line here

Done.

[michaeln, 2010-09-18 01:10:48]

> > how about putting this in src/webkit/fileapi/ instead?
> 
> Might be nice to have a webkit/base directory. There are utilities shared
across
> our modules in there, but no good home for such things. I'll see how well this
> fits in the fileapi folder?

I think webkit/blob might be a better place for it since it's to be used for
xhr.responseBlob.

I'll move it there for now.

[michaeln, 2010-09-18 01:54:57]

New snapshot with changes outlined in previous comments.

[michaeln, 2010-09-18 03:08:41]

hmmm... should we just smush webkit/blob and webkit/fileapi together?

[michaeln, 2010-09-18 06:25:38]

http://codereview.chromium.org/3165062/diff/148001/133019
File webkit/blob/deletable_file_reference_unittest.cc (right):

http://codereview.chromium.org/3165062/diff/148001/133019#newcode15
webkit/blob/deletable_file_reference_unittest.cc:15: MessageLoop loop;
that's wierd... something is different about test_shell_tests and
base_unit_tests such that putting a MessageLoop on the stack is doesn't work in
the former and does in the latter?

i have to poke at this test some more

[darin (slow to review), 2010-09-19 03:46:43]

probably... that is how it is in WebCore

On Fri, Sep 17, 2010 at 8:08 PM, <michaeln@chromium.org> wrote:

> hmmm... should we just smush webkit/blob and webkit/fileapi together?
>
>
> http://codereview.chromium.org/3165062/show
>

[darin (slow to review), 2010-09-19 03:48:03]

http://codereview.chromium.org/3165062/diff/148001/133019
File webkit/blob/deletable_file_reference_unittest.cc (right):

http://codereview.chromium.org/3165062/diff/148001/133019#newcode15
webkit/blob/deletable_file_reference_unittest.cc:15: MessageLoop loop;
yes, in test_shell_tests there is already a MessageLoop created for you.  maybe
that should only be done by the TestShellTest framework.

[michaeln, 2010-09-20 19:05:04]

On 2010/09/18 06:25:38, michaeln wrote:
> http://codereview.chromium.org/3165062/diff/148001/133019
> File webkit/blob/deletable_file_reference_unittest.cc (right):
> 
> http://codereview.chromium.org/3165062/diff/148001/133019#newcode15
> webkit/blob/deletable_file_reference_unittest.cc:15: MessageLoop loop;
> that's wierd... something is different about test_shell_tests and
> base_unit_tests such that putting a MessageLoop on the stack is doesn't work
in
> the former and does in the latter?
> 
> i have to poke at this test some more

New snapshot that uses MessageLoop::current() instead.

[michaeln, 2010-09-20 19:55:51]

New patch, synced and merged a conflict.

[darin (slow to review), 2010-09-21 07:03:09]

LGTM

[michaeln, 2010-09-24 20:31:18]

Ooops... there was a perf regression with this :(

[michaeln, 2010-09-24 20:31:44]

On 2010/09/24 20:31:18, michaeln wrote:
> Ooops... there was a perf regression with this :(

see http://code.google.com/p/chromium/issues/detail?id=56752

[darin (slow to review), 2010-09-24 20:42:16]

see my comment at the end of that bug.  did i understand correctly?
-darin

On Fri, Sep 24, 2010 at 1:31 PM, <michaeln@chromium.org> wrote:

> On 2010/09/24 20:31:18, michaeln wrote:
>
>> Ooops... there was a perf regression with this :(
>>
>
> see http://code.google.com/p/chromium/issues/detail?id=56752
>
>
> http://codereview.chromium.org/3165062/show
>

[michaeln, 2010-09-24 21:53:27]

> see my comment at the end of that bug.  did i understand correctly?

Yes, I think you did. Thanx for interpreting what that graph was saying.

    if (request_.download_to_file)  // This addition would avoid sending the IPC
in almost all cases.
      dispatcher_->message_sender()->Send(
          new ViewHostMsg_ResourceLoaderDeleted(request_id_));

[michaeln, 2010-09-24 23:38:38]

Per http://code.google.com/p/chromium/issues/detail?id=56752...

I'll resubmit this change with the addition of an extra line of code to
conditionally send the new IPC only when needed.

  if (request_.download_to_file)  // <--- this is the extra line
    dispatcher_->message_sender()->Send(
        new ViewHostMsg_ResourceLoaderDeleted(request_id_));

Since the semantics of the message are changing, I'll rename the message to
something along the lines of...
  ViewHostMsg_Resource_ReleaseDownloadedFile(request_id_);

[darin (slow to review), 2010-09-25 04:47:47]

On Fri, Sep 24, 2010 at 4:38 PM, <michaeln@chromium.org> wrote:

> Per http://code.google.com/p/chromium/issues/detail?id=56752...
>
> I'll resubmit this change with the addition of an extra line of code to
> conditionally send the new IPC only when needed.
>
>  if (request_.download_to_file)  // <--- this is the extra line
>
>    dispatcher_->message_sender()->Send(
>        new ViewHostMsg_ResourceLoaderDeleted(request_id_));
>
> Since the semantics of the message are changing, I'll rename the message to
> something along the lines of...
>  ViewHostMsg_Resource_ReleaseDownloadedFile(request_id_);


SGTM


>
>
> http://codereview.chromium.org/3165062/show
>