IPC messages and changes to ResourceLoaderBridge to support resource loading for media

Highlights of changes:
1. Added entry to ResourceResponseHead so that it contains
   either a base::PlatformFile (OS_WIN) or
   base::FileDescriptor (OS_POSIX) for passing the file
   handle from browser to renderer process.
2. Also added IPC messages for reporting download progress
   and ACK message for it. ResourceLoaderBridge::Peer::OnDownloadProgress
   is added so that the peer is notified of the download
   progress in the renderer process.
3. Load flag to kick start the resource loading for media
   files. LOAD_MEDIA_RESOURCE is added so that
   ResourceDispatcherHost knows how to use a different
   ResourceHandler for handling media resource request.

[Alpha Left Google, 2009-02-25 22:30:21]

This is a proposed change to support media resource loading using file handle
for transportation of data. There's one additional decision that we need to
make, now I have LOAD_MEDIA_RESOURCE added in load_flags.h for identifying media
resource loading in ResourceDispatcherHost, but there's also ResourceType::MEDIA
that we can use for the same purpose, that would be a better way to do this?
With this change and the previous one for disk cache and request context, I can
work on a MediaResourceHandler in ResourceDispatcherHost that does the real
work.

[darin (slow to review), 2009-02-26 22:26:55]

http://codereview.chromium.org/27168/diff/18/19
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/27168/diff/18/19#newcode821
Line 821: response->response_head.response_data_file =
request->response_data_file();
i think you need to use DuplicateHandle here to make it possible for the
renderer to read this HANDLE from within the sandbox.

http://codereview.chromium.org/27168/diff/18/22
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/27168/diff/18/22#newcode178
Line 178: IPC_MESSAGE_ROUTED3(ViewMsg_Resource_DownloadProgress,
please add a comment explaining when this is used.

it may be valid for size to be unknown.  consider the case of a web server
streaming a media file using Transfer-Encoding: chunked.  it is common practice
to use a size of -1 to indicate the unknown size value.  we should probably
document that here and make sure it works :)

http://codereview.chromium.org/27168/diff/18/22#newcode1127
Line 1127: // Sent when the renderer process to acknowledge reception of an
grammar-nit: "Sent by the renderer process to acknowledge receipt of a
DownloadProgress message."

http://codereview.chromium.org/27168/diff/18/22#newcode1132
Line 1132: // Sent when the renderer process to acknowledge receiption of an
this one could use the same.

http://codereview.chromium.org/27168/diff/18/25
File net/base/load_flags.h (right):

http://codereview.chromium.org/27168/diff/18/25#newcode73
Line 73: LOAD_MEDIA_RESOURCE = 1 << 16,
please pick a name for this flag that corresponds better to what it actually
does.  LOAD_AS_FILE is a good choice.  or, maybe LOAD_ENABLE_DOWNLOAD_FILE would
be better.  then, i'd also give this a value of 1 << 7 and put it next to
LOAD_ENABLE_UPLOAD_PROGRESS.

http://codereview.chromium.org/27168/diff/18/26
File net/http/http_response_info.h (right):

http://codereview.chromium.org/27168/diff/18/26#newcode55
Line 55: base::PlatformFile response_data_file;
this seems to duplicate what i saw in the other CL.  which CL is the right one
for me to be reviewing?

http://codereview.chromium.org/27168/diff/18/28
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/27168/diff/18/28#newcode99
Line 99: virtual void OnDownloadProgress(uint64 position, uint64 size) {}
we don't need another callback for this.  each call to OnReceivedData is
effectively reporting download progress.

we probably do not even need LOAD_MEDIA_RESOURCE then.

[Alpha Left Google, 2009-02-26 23:03:26]

http://codereview.chromium.org/27168/diff/18/19
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/27168/diff/18/19#newcode821
Line 821: response->response_head.response_data_file =
request->response_data_file();
On 2009/02/26 22:26:56, darin wrote:
> i think you need to use DuplicateHandle here to make it possible for the
> renderer to read this HANDLE from within the sandbox.

This response will be passed to ResourceHandler immediately, I will have a
MediaResourceHandler that handles this.

http://codereview.chromium.org/27168/diff/18/22
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/27168/diff/18/22#newcode178
Line 178: IPC_MESSAGE_ROUTED3(ViewMsg_Resource_DownloadProgress,
On 2009/02/26 22:26:56, darin wrote:
> please add a comment explaining when this is used.
> 
> it may be valid for size to be unknown.  consider the case of a web server
> streaming a media file using Transfer-Encoding: chunked.  it is common
practice
> to use a size of -1 to indicate the unknown size value.  we should probably
> document that here and make sure it works :)

Done.

http://codereview.chromium.org/27168/diff/18/22#newcode1127
Line 1127: // Sent when the renderer process to acknowledge reception of an
On 2009/02/26 22:26:56, darin wrote:
> grammar-nit: "Sent by the renderer process to acknowledge receipt of a
> DownloadProgress message."

Done.

http://codereview.chromium.org/27168/diff/18/22#newcode1132
Line 1132: // Sent when the renderer process to acknowledge receiption of an
On 2009/02/26 22:26:56, darin wrote:
> this one could use the same.

Done.

http://codereview.chromium.org/27168/diff/18/25
File net/base/load_flags.h (right):

http://codereview.chromium.org/27168/diff/18/25#newcode73
Line 73: LOAD_MEDIA_RESOURCE = 1 << 16,
On 2009/02/26 22:26:56, darin wrote:
> please pick a name for this flag that corresponds better to what it actually
> does.  LOAD_AS_FILE is a good choice.  or, maybe LOAD_ENABLE_DOWNLOAD_FILE
would
> be better.  then, i'd also give this a value of 1 << 7 and put it next to
> LOAD_ENABLE_UPLOAD_PROGRESS.

I'll take this away.

http://codereview.chromium.org/27168/diff/18/26
File net/http/http_response_info.h (right):

http://codereview.chromium.org/27168/diff/18/26#newcode55
Line 55: base::PlatformFile response_data_file;
On 2009/02/26 22:26:56, darin wrote:
> this seems to duplicate what i saw in the other CL.  which CL is the right one
> for me to be reviewing?

Sorry for the confusion, this change does not belong to this CL, it's just to
make this CL standalone (compilable), I'll remove this.

http://codereview.chromium.org/27168/diff/18/28
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/27168/diff/18/28#newcode99
Line 99: virtual void OnDownloadProgress(uint64 position, uint64 size) {}
On 2009/02/26 22:26:56, darin wrote:
> we don't need another callback for this.  each call to OnReceivedData is
> effectively reporting download progress.
> 
> we probably do not even need LOAD_MEDIA_RESOURCE then.

Method removed.

[darin (slow to review), 2009-02-26 23:23:34]

http://codereview.chromium.org/27168/diff/1027/50
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/27168/diff/1027/50#newcode113
Line 113: // NULL and serves as a notification of download progress using |len|.
doh, i think i gave you bad advice previously.  i had mistakenly thought this
was URLRequest::Delegate that you were adding the OnDownloadProgress method to. 
however, this is ResourceLoaderBridge, and i think OnDownloadProgress is
actually more appropriate.  sorry about that!

and now i understand why you were trying to enable stream as file mode for
ResourceType::MEDIA.  instead, i think it would be better to use the load flag
as you were originally doing.

we should also make sure that the URLRequest layer understands that load flag.

[Alpha Left Google, 2009-02-26 23:43:55]

http://codereview.chromium.org/27168/diff/1027/50
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/27168/diff/1027/50#newcode113
Line 113: // NULL and serves as a notification of download progress using |len|.
On 2009/02/26 23:23:34, darin wrote:
> doh, i think i gave you bad advice previously.  i had mistakenly thought this
> was URLRequest::Delegate that you were adding the OnDownloadProgress method
to. 
> however, this is ResourceLoaderBridge, and i think OnDownloadProgress is
> actually more appropriate.  sorry about that!
> 
> and now i understand why you were trying to enable stream as file mode for
> ResourceType::MEDIA.  instead, i think it would be better to use the load flag
> as you were originally doing.
> 
> we should also make sure that the URLRequest layer understands that load flag.

Done. I should have explained it better. :) URLRequest actually doesn't have
much to do with LOAD_ENABLE_DOWNLOAD_FILE, it's ResourceDispatcherHost that
takes care of the flag and gives URLRequest a media URLRequestContext supported
by a special cache.

[darin (slow to review), 2009-02-26 23:55:50]

> Done. I should have explained it better. :) URLRequest actually doesn't have
> much to do with LOAD_ENABLE_DOWNLOAD_FILE, it's ResourceDispatcherHost that
> takes care of the flag and gives URLRequest a media URLRequestContext
supported
> by a special cache.

That makes sense.  It may also be a good idea to assert somewhere that this load
flag is always set for requests made using that special URLRequestContext.

[Alpha Left Google, 2009-02-27 19:46:10]

On 2009/02/26 23:55:50, darin wrote:
> > Done. I should have explained it better. :) URLRequest actually doesn't have
> > much to do with LOAD_ENABLE_DOWNLOAD_FILE, it's ResourceDispatcherHost that
> > takes care of the flag and gives URLRequest a media URLRequestContext
> supported
> > by a special cache.
> 
> That makes sense.  It may also be a good idea to assert somewhere that this
load
> flag is always set for requests made using that special URLRequestContext.

Done. I put an assert in HttpCache::HttpTransaction so that whenever HttpCache
of type MEDIA is used, LOAD_ENABLE_DOWNLOAD_FILE is always set.

[darin (slow to review), 2009-02-27 22:31:36]

Looks good, but...

http://codereview.chromium.org/27168/diff/1053/65
File net/http/http_cache.cc (right):

http://codereview.chromium.org/27168/diff/1053/65#newcode608
Line 608: DCHECK(cache_->type() != HttpCache::MEDIA ||
we should also assert that LOAD_ENABLE_DOWNLOAD_FILE is not set when the type is
not MEDIA.

http://codereview.chromium.org/27168/diff/1053/67
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/27168/diff/1053/67#newcode98
Line 98: // unknown and |size| will be -1 in that case.
will be uint64(-1) you mean, right?  maybe that should just say kunit64max

[Alpha Left Google, 2009-02-27 22:49:00]

http://codereview.chromium.org/27168/diff/1053/65
File net/http/http_cache.cc (right):

http://codereview.chromium.org/27168/diff/1053/65#newcode608
Line 608: DCHECK(cache_->type() != HttpCache::MEDIA ||
On 2009/02/27 22:31:36, darin wrote:
> we should also assert that LOAD_ENABLE_DOWNLOAD_FILE is not set when the type
is
> not MEDIA.

Done. I used an "if (cache_->type() == HttpCache::MEDIA)" s tatement to decide
which DCHECK to have, I think it's clearer this way.

http://codereview.chromium.org/27168/diff/1053/67
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/27168/diff/1053/67#newcode98
Line 98: // unknown and |size| will be -1 in that case.
On 2009/02/27 22:31:36, darin wrote:
> will be uint64(-1) you mean, right?  maybe that should just say kunit64max

Changed the comment to say kuint64max.

[agl, 2009-03-05 17:47:37]

http://codereview.chromium.org/27168/diff/1056/69
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/27168/diff/1056/69#newcode818
Line 818: response->response_head.response_data_file.fd =
request->response_data_file();
Note that the file descriptor *must* be valid at the time when the message is
sent. It's a fatal error otherwise. If in doubt, dup the descriptor here and set
auto close to true. From the logs of the failing processes, it appears that this
file descriptor may have been -1 or some such?

If the file descriptor is invalid, you cannot serialise it. Use IPC::Maybe to
solve this issue.

(In general, you can always ask me to review anything involving FileDescriptor
since it has subtleties which the Windows world doesn't have.)

[Alpha Left Google, 2009-03-05 23:47:31]

agl, darin, rvargas: Please look into http_cache.cc where the file handle is
duplicated.

http://codereview.chromium.org/27168/diff/4016/3042
File net/http/http_cache.cc (right):

http://codereview.chromium.org/27168/diff/4016/3042#newcode822
Line 822: response_.response_data_file = dup(response_.response_data_file);
In case of linux and mac, we can't tell whether a file handle is closed or not,
and sending a closed file handle through IPC is going to crash everything. So I
do this to make sure the lifetime of file handle leaving the net stack is not
controlled by the cache. In the case of windows, we will use DuplicateHandle and
send the duplicated handle, it will simply fail if the handle is closed. I'm
tempted to duplicate the handle here for windows too, any ideas?

[rvargas (doing something else), 2009-03-06 03:46:03]

http://codereview.chromium.org/27168/diff/4019/4027
File net/http/http_cache.cc (right):

http://codereview.chromium.org/27168/diff/4019/4027#newcode822
Line 822: response_.response_data_file = dup(response_.response_data_file);
I don't know how the actual transport of the handle across ipc happens on posix
or whether RDH will perform a targeted second duplication to the appropriate
renderer before sending the message (as the windows code will do), but if that
is not the case, an important part of this feature is that the renderer receives
a handle with lower privileges than the handle provided by the disk cache.

Reading the docs for dup(), the second handle will share almost everything with
the original (file offsets) so if the handle received by the renderer has the
same characteristics  that will be a problem.

Also, it seems like dup will simply fail (EBADF) if the handle was closed so we
could perform a single dup on the RDH and know if the handle is valid or not.

[agl, 2009-03-06 05:09:02]

NACK

To be clear: it looks like this code is trying to pass a handle to a cache file
from the browser to the renderer. Probably for faster and random access for
media files, right?

If this is the case, then I don't see the cross-process DuplicateHandle for the
Windows code. The renderer cannot DuplicateHandle from the browser, so it should
be in here somewhere, no?

For the POSIX case, see the comments inline. Also note rvargas's comment: the
descriptor passed to the renderer should be read-only. This needs to be
specified at open(); one cannot dup() a descriptor to have less authority.

To be clear, the descriptor which arrives in the renderer has /equal/ authority,
and /shares/ file offset values etc. The only difference is the close-on-exec
flag, which is private. Thus, if the browser also uses the descriptor for
anything other than mmap(), bad things may occur.

Re EBADF: For Windows and POSIX, if it's *possible* for the program to trigger
EBADF (or the Windows equiv), the code is BROKEN. EBADF means that the
handle/descriptor number was invalid. Since the browser is multithreaded, and
threads share the descriptor table, that means that you are racing the other
threads which may install a different, valid descriptor in that slot.

http://codereview.chromium.org/27168/diff/4019/4020
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/27168/diff/4019/4020#newcode818
Line 818: // Structure of ResourceResponseHead depends on the platform, so we do
it
nit: this comment is superfluous

http://codereview.chromium.org/27168/diff/4019/4020#newcode822
Line 822: response->response_head.response_data_file.auto_close = false;
This should be true I believe. If true, the IPC system will close the file
descriptor after it has been sent. Otherwise, I think it will leak.

http://codereview.chromium.org/27168/diff/4019/4027
File net/http/http_cache.cc (right):

http://codereview.chromium.org/27168/diff/4019/4027#newcode821
Line 821: if (response_.response_data_file != base::kInvalidPlatformFileValue) {
This cannot be correct.

Either: response_.response_data_file may be closed before the IPC message is
transmitted, in which case both Windows and POSIX need to duplicate the handle.
OR, it's ensured that it remains open in which case neither should duplicate it.

[Alpha Left Google, 2009-03-06 05:57:54]

On 2009/03/06 05:09:02, agl wrote:
> NACK
> 
> To be clear: it looks like this code is trying to pass a handle to a cache
file
> from the browser to the renderer. Probably for faster and random access for
> media files, right?
>

Correct.
 
> If this is the case, then I don't see the cross-process DuplicateHandle for
the
> Windows code. The renderer cannot DuplicateHandle from the browser, so it
should
> be in here somewhere, no?

DuplicateHandle in Windows will be called by a RDH resource handler. The RDH
resource handler (say MediaResourceHandler) is responsible for sending the file
handle through IPC and thus duplicate the file handle there. In Windows we will
use DuplicateHandle in the RDH handler and send the duplicated handle, in Linux
we may send the file handle directly. The problem is that there are some cases
the file handle may be closed but the RDH handler doesn't know about it. In case
of Windows, DuplicateHandle will give an error, we can handle this gracefully.
In case of Linux, sending a closed handle will crash, and we don't have the
proper indication. The purpose of dup() in http_cache.cc is to gurantee an
opened file handle for RDH in Linux (for not crashing).

> 
> For the POSIX case, see the comments inline. Also note rvargas's comment: the
> descriptor passed to the renderer should be read-only. This needs to be
> specified at open(); one cannot dup() a descriptor to have less authority.
> 
> To be clear, the descriptor which arrives in the renderer has /equal/
authority,
> and /shares/ file offset values etc. The only difference is the close-on-exec
> flag, which is private. Thus, if the browser also uses the descriptor for
> anything other than mmap(), bad things may occur.
> 

If the file handle arriving in renderer shares also the offset then it will be a
problem. Because neither side is using mmap(). We need to find an alternative
for Linux.

> Re EBADF: For Windows and POSIX, if it's *possible* for the program to trigger
> EBADF (or the Windows equiv), the code is BROKEN. EBADF means that the
> handle/descriptor number was invalid. Since the browser is multithreaded, and
> threads share the descriptor table, that means that you are racing the other
> threads which may install a different, valid descriptor in that slot.
> 
> http://codereview.chromium.org/27168/diff/4019/4020
> File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):
> 
> http://codereview.chromium.org/27168/diff/4019/4020#newcode818
> Line 818: // Structure of ResourceResponseHead depends on the platform, so we
do
> it
> nit: this comment is superfluous
> 
> http://codereview.chromium.org/27168/diff/4019/4020#newcode822
> Line 822: response->response_head.response_data_file.auto_close = false;
> This should be true I believe. If true, the IPC system will close the file
> descriptor after it has been sent. Otherwise, I think it will leak.
> 
> http://codereview.chromium.org/27168/diff/4019/4027
> File net/http/http_cache.cc (right):
> 
> http://codereview.chromium.org/27168/diff/4019/4027#newcode821
> Line 821: if (response_.response_data_file != base::kInvalidPlatformFileValue)
{
> This cannot be correct.
> 
> Either: response_.response_data_file may be closed before the IPC message is
> transmitted, in which case both Windows and POSIX need to duplicate the
handle.
> OR, it's ensured that it remains open in which case neither should duplicate
it.

[Alpha Left Google, 2009-03-06 17:32:10]

On 2009/03/06 05:57:54, Alpha wrote:
> On 2009/03/06 05:09:02, agl wrote:
> > NACK
> > 
> > To be clear: it looks like this code is trying to pass a handle to a cache
> file
> > from the browser to the renderer. Probably for faster and random access for
> > media files, right?
> >
> 
> Correct.
>  
> > If this is the case, then I don't see the cross-process DuplicateHandle for
> the
> > Windows code. The renderer cannot DuplicateHandle from the browser, so it
> should
> > be in here somewhere, no?
> 
> DuplicateHandle in Windows will be called by a RDH resource handler. The RDH
> resource handler (say MediaResourceHandler) is responsible for sending the
file
> handle through IPC and thus duplicate the file handle there. In Windows we
will
> use DuplicateHandle in the RDH handler and send the duplicated handle, in
Linux
> we may send the file handle directly. The problem is that there are some cases
> the file handle may be closed but the RDH handler doesn't know about it. In
case
> of Windows, DuplicateHandle will give an error, we can handle this gracefully.
> In case of Linux, sending a closed handle will crash, and we don't have the
> proper indication. The purpose of dup() in http_cache.cc is to gurantee an
> opened file handle for RDH in Linux (for not crashing).
> 
> > 
> > For the POSIX case, see the comments inline. Also note rvargas's comment:
the
> > descriptor passed to the renderer should be read-only. This needs to be
> > specified at open(); one cannot dup() a descriptor to have less authority.
> > 
> > To be clear, the descriptor which arrives in the renderer has /equal/
> authority,
> > and /shares/ file offset values etc. The only difference is the
close-on-exec
> > flag, which is private. Thus, if the browser also uses the descriptor for
> > anything other than mmap(), bad things may occur.
> > 
> 
> If the file handle arriving in renderer shares also the offset then it will be
a
> problem. Because neither side is using mmap(). We need to find an alternative

Since dup() creates a handle with the same privileges and same offset, I think
we should create a complete new file handle with read only when we ask for a
file handle from disk_cache.

> for Linux.
> 
> > Re EBADF: For Windows and POSIX, if it's *possible* for the program to
trigger
> > EBADF (or the Windows equiv), the code is BROKEN. EBADF means that the
> > handle/descriptor number was invalid. Since the browser is multithreaded,
and
> > threads share the descriptor table, that means that you are racing the other
> > threads which may install a different, valid descriptor in that slot.
> > 
> > http://codereview.chromium.org/27168/diff/4019/4020
> > File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):
> > 
> > http://codereview.chromium.org/27168/diff/4019/4020#newcode818
> > Line 818: // Structure of ResourceResponseHead depends on the platform, so
we
> do
> > it
> > nit: this comment is superfluous
> > 
> > http://codereview.chromium.org/27168/diff/4019/4020#newcode822
> > Line 822: response->response_head.response_data_file.auto_close = false;
> > This should be true I believe. If true, the IPC system will close the file
> > descriptor after it has been sent. Otherwise, I think it will leak.
> > 
> > http://codereview.chromium.org/27168/diff/4019/4027
> > File net/http/http_cache.cc (right):
> > 
> > http://codereview.chromium.org/27168/diff/4019/4027#newcode821
> > Line 821: if (response_.response_data_file !=
base::kInvalidPlatformFileValue)
> {
> > This cannot be correct.
> > 
> > Either: response_.response_data_file may be closed before the IPC message is
> > transmitted, in which case both Windows and POSIX need to duplicate the
> handle.
> > OR, it's ensured that it remains open in which case neither should duplicate
> it.

[Alpha Left Google, 2009-03-07 02:06:54]

agl, rvargas: entry_impl.cc, disk_cache.h
You will find code of opening the cache file in entry_impl.cc and returning a
completely new file handle. Obtaining a new set of read file handle make the
code much simpler and coherent across platforms.
MediaResourceHandler is the actual place of sending the handle to IPC.

[agl, 2009-03-09 18:02:31]

rvargas still to check the cache code.

http://codereview.chromium.org/27168/diff/4072/3079
File chrome/browser/renderer_host/media_resource_handler.cc (right):

http://codereview.chromium.org/27168/diff/4072/3079#newcode44
Line 44: // In POSIX platform, we will just set auto_close to true, and the IPC
"In POSIX platform" -> "On POSIX"
s/will just //

http://codereview.chromium.org/27168/diff/4072/3079#newcode53
Line 53: // In Windows, we will duplicate the file handle for renderer process
and
s/In/On/, s/will //, s/for /for the /

http://codereview.chromium.org/27168/diff/4072/3079#newcode67
Line 67: response->response_head.response_data_file =
I think this line is redundant.

http://codereview.chromium.org/27168/diff/4072/3079#newcode87
Line 87: // behavior except that data is not sent by just the progress of
download.
Please reword this comment, I'm not sure what you're trying to say here.

http://codereview.chromium.org/27168/diff/4072/3080
File chrome/browser/renderer_host/media_resource_handler.h (right):

http://codereview.chromium.org/27168/diff/4072/3080#newcode15
Line 15: // asynchronously and try to work with file for response data if
possible,
s/try/tries, period after "possible"

http://codereview.chromium.org/27168/diff/4072/3080#newcode16
Line 16: // if a response data file is not available, it just redirects calls to
s/if/If/, s/just//

http://codereview.chromium.org/27168/diff/4072/3084
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/27168/diff/4072/3084#newcode340
Line 340: // specific to media for handling it because these resources had
specific
s/had/have/

http://codereview.chromium.org/27168/diff/4072/3091
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/27168/diff/4072/3091#newcode175
Line 175: // |index|. Value other than base::kInvalidPlatformFileValue is a
success and
s/Value/Values/, s/is a success/are successful/,

http://codereview.chromium.org/27168/diff/4072/3092
File net/disk_cache/entry_impl.cc (right):

http://codereview.chromium.org/27168/diff/4072/3092#newcode380
Line 380: if (backend_->CreateExternalFile(&address)) {
nit: complement this condition and handle the return here, so that the body of
the function has less indentation?

[rvargas (doing something else), 2009-03-09 20:15:48]

http://codereview.chromium.org/27168/diff/4072/3079
File chrome/browser/renderer_host/media_resource_handler.cc (right):

http://codereview.chromium.org/27168/diff/4072/3079#newcode6
Line 6: #include "chrome/browser/renderer_host/media_resource_handler.h"
nit: this should be the first include.

http://codereview.chromium.org/27168/diff/4072/3079#newcode98
Line 98: return handler_->OnReadCompleted(request_id, bytes_read);
Are you sure that you don't want to call the previous handler when you send your
own message (DownloadProgress)? I  guess that will end up in a series of
OnWillRead() calls without an OnReadCompleted(), and maybe they're not so happy
about that.

http://codereview.chromium.org/27168/diff/4072/3080
File chrome/browser/renderer_host/media_resource_handler.h (right):

http://codereview.chromium.org/27168/diff/4072/3080#newcode13
Line 13: // Used to complete an media resource request in response to resource
nit: an -> a

http://codereview.chromium.org/27168/diff/4072/3091
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/27168/diff/4072/3091#newcode163
Line 163: // Caller should never close the file handle returned by this method,
since
Now the caller needs to close this handle.

http://codereview.chromium.org/27168/diff/4072/3092
File net/disk_cache/entry_impl.cc (right):

http://codereview.chromium.org/27168/diff/4072/3092#newcode379
Line 379: return base::kInvalidPlatformFileValue;
I don't think you need to change this function. Returning directly the file from
the disk cache (as you do now) would be a mistake.

http://codereview.chromium.org/27168/diff/4072/3092#newcode402
Line 402: return base::CreatePlatformFile(backend_->GetFileName(address),
There is a problem with the current implementation of CreatePlatformFile for
Windows. In this case, we need FILE_SHARE_DELETE to be set... you can do that
with this CL or file a separate bug.

[Alpha Left Google, 2009-03-09 21:28:40]

http://codereview.chromium.org/27168/diff/4072/3079
File chrome/browser/renderer_host/media_resource_handler.cc (right):

http://codereview.chromium.org/27168/diff/4072/3079#newcode6
Line 6: #include "chrome/browser/renderer_host/media_resource_handler.h"
On 2009/03/09 20:15:49, rvargas wrote:
> nit: this should be the first include.

Done.

http://codereview.chromium.org/27168/diff/4072/3079#newcode44
Line 44: // In POSIX platform, we will just set auto_close to true, and the IPC
On 2009/03/09 18:02:31, agl wrote:
> "In POSIX platform" -> "On POSIX"
> s/will just //

Done.

http://codereview.chromium.org/27168/diff/4072/3079#newcode53
Line 53: // In Windows, we will duplicate the file handle for renderer process
and
On 2009/03/09 18:02:31, agl wrote:
> s/In/On/, s/will //, s/for /for the /

Done.

http://codereview.chromium.org/27168/diff/4072/3079#newcode67
Line 67: response->response_head.response_data_file =
On 2009/03/09 18:02:31, agl wrote:
> I think this line is redundant.

Done.

http://codereview.chromium.org/27168/diff/4072/3079#newcode87
Line 87: // behavior except that data is not sent by just the progress of
download.
On 2009/03/09 18:02:31, agl wrote:
> Please reword this comment, I'm not sure what you're trying to say here.

Re-written it.

http://codereview.chromium.org/27168/diff/4072/3079#newcode98
Line 98: return handler_->OnReadCompleted(request_id, bytes_read);
On 2009/03/09 20:15:49, rvargas wrote:
> Are you sure that you don't want to call the previous handler when you send
your
> own message (DownloadProgress)? I  guess that will end up in a series of
> OnWillRead() calls without an OnReadCompleted(), and maybe they're not so
happy
> about that. 

Calling to handler_->OnReadCompleted which essentially is calling to
AsyncResourceHandler::OnReadCompleted would cause an extra IPC with shared
memory handle to be sent, which is not necessary. WillSendData increments the
pending data count, which is decremented by
ViewHostMsg_Resource_DownloadProgressACK, so it works the same as if a normal
request IPC.

http://codereview.chromium.org/27168/diff/4072/3080
File chrome/browser/renderer_host/media_resource_handler.h (right):

http://codereview.chromium.org/27168/diff/4072/3080#newcode13
Line 13: // Used to complete an media resource request in response to resource
On 2009/03/09 20:15:49, rvargas wrote:
> nit: an -> a

Done.

http://codereview.chromium.org/27168/diff/4072/3080#newcode15
Line 15: // asynchronously and try to work with file for response data if
possible,
On 2009/03/09 18:02:31, agl wrote:
> s/try/tries, period after "possible"

Done.

http://codereview.chromium.org/27168/diff/4072/3080#newcode16
Line 16: // if a response data file is not available, it just redirects calls to
On 2009/03/09 18:02:31, agl wrote:
> s/if/If/, s/just//

Done.

http://codereview.chromium.org/27168/diff/4072/3084
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/27168/diff/4072/3084#newcode340
Line 340: // specific to media for handling it because these resources had
specific
On 2009/03/09 18:02:31, agl wrote:
> s/had/have/

Done.

http://codereview.chromium.org/27168/diff/4072/3091
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/27168/diff/4072/3091#newcode163
Line 163: // Caller should never close the file handle returned by this method,
since
On 2009/03/09 20:15:49, rvargas wrote:
> Now the caller needs to close this handle.

Done.

http://codereview.chromium.org/27168/diff/4072/3091#newcode175
Line 175: // |index|. Value other than base::kInvalidPlatformFileValue is a
success and
On 2009/03/09 18:02:31, agl wrote:
> s/Value/Values/, s/is a success/are successful/,

Done.

http://codereview.chromium.org/27168/diff/4072/3092
File net/disk_cache/entry_impl.cc (right):

http://codereview.chromium.org/27168/diff/4072/3092#newcode379
Line 379: return base::kInvalidPlatformFileValue;
On 2009/03/09 20:15:49, rvargas wrote:
> I don't think you need to change this function. Returning directly the file
from
> the disk cache (as you do now) would be a mistake.

Make sense. Reverted.

http://codereview.chromium.org/27168/diff/4072/3092#newcode380
Line 380: if (backend_->CreateExternalFile(&address)) {
On 2009/03/09 18:02:31, agl wrote:
> nit: complement this condition and handle the return here, so that the body of
> the function has less indentation?

Done since reverted.

http://codereview.chromium.org/27168/diff/4072/3092#newcode402
Line 402: return base::CreatePlatformFile(backend_->GetFileName(address),
On 2009/03/09 20:15:49, rvargas wrote:
> There is a problem with the current implementation of CreatePlatformFile for
> Windows. In this case, we need FILE_SHARE_DELETE to be set... you can do that
> with this CL or file a separate bug.

I will open up another CL for this issue. This CL is pretty big already..

[rvargas (doing something else), 2009-03-09 22:02:59]

http://codereview.chromium.org/27168/diff/4072/3079
File chrome/browser/renderer_host/media_resource_handler.cc (right):

http://codereview.chromium.org/27168/diff/4072/3079#newcode98
Line 98: return handler_->OnReadCompleted(request_id, bytes_read);
On 2009/03/09 21:28:40, Alpha wrote:
> On 2009/03/09 20:15:49, rvargas wrote:
> > Are you sure that you don't want to call the previous handler when you send
> your
> > own message (DownloadProgress)? I  guess that will end up in a series of
> > OnWillRead() calls without an OnReadCompleted(), and maybe they're not so
> happy
> > about that. 
> 
> Calling to handler_->OnReadCompleted which essentially is calling to
> AsyncResourceHandler::OnReadCompleted would cause an extra IPC with shared
> memory handle to be sent, which is not necessary. WillSendData increments the
> pending data count, which is decremented by
> ViewHostMsg_Resource_DownloadProgressACK, so it works the same as if a normal
> request IPC.

What I'm saying is that it would be nice if the handler could work chained to
any other handler, and the handler after you may be expecting the calls to be
paired.

In particular, if AsyncResourceHandler is after you, there is an implied
assumption that the buffer allocated on OnWillRead will be freed by the call to
OnReadCompleted (or OnResponseCompleted). You could be hitting a DCHECK that
attempts to make sure that there is no read_buffer_.

[rvargas (doing something else), 2009-03-09 22:10:21]

Sorry about this... a couple of extra small nits.

http://codereview.chromium.org/27168/diff/3103/3117
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/27168/diff/3103/3117#newcode163
Line 163: // Caller should close the file handle returned by this method or
there will
nit: Caller -> The caller

http://codereview.chromium.org/27168/diff/3103/3117#newcode174
Line 174: // |index|. Values other than base::kInvalidPlatformFileValue is
successful
nit: is -> are

http://codereview.chromium.org/27168/diff/3103/3117#newcode175
Line 175: // and the file handle should be managed by caller, i.e. caller should
close
nit caller -> the caller

http://codereview.chromium.org/27168/diff/3103/3118
File net/disk_cache/entry_impl.cc (right):

http://codereview.chromium.org/27168/diff/3103/3118#newcode384
Line 384: base::PLATFORM_FILE_READ |
nit: indent this 4 spaces.

[Alpha Left Google, 2009-03-09 22:21:55]

http://codereview.chromium.org/27168/diff/4072/3079
File chrome/browser/renderer_host/media_resource_handler.cc (right):

http://codereview.chromium.org/27168/diff/4072/3079#newcode98
Line 98: return handler_->OnReadCompleted(request_id, bytes_read);
On 2009/03/09 22:02:59, rvargas wrote:
> On 2009/03/09 21:28:40, Alpha wrote:
> > On 2009/03/09 20:15:49, rvargas wrote:
> > > Are you sure that you don't want to call the previous handler when you
send
> > your
> > > own message (DownloadProgress)? I  guess that will end up in a series of
> > > OnWillRead() calls without an OnReadCompleted(), and maybe they're not so
> > happy
> > > about that. 
> > 
> > Calling to handler_->OnReadCompleted which essentially is calling to
> > AsyncResourceHandler::OnReadCompleted would cause an extra IPC with shared
> > memory handle to be sent, which is not necessary. WillSendData increments
the
> > pending data count, which is decremented by
> > ViewHostMsg_Resource_DownloadProgressACK, so it works the same as if a
normal
> > request IPC.
> 
> What I'm saying is that it would be nice if the handler could work chained to
> any other handler, and the handler after you may be expecting the calls to be
> paired.
> 
> In particular, if AsyncResourceHandler is after you, there is an implied
> assumption that the buffer allocated on OnWillRead will be freed by the call
to
> OnReadCompleted (or OnResponseCompleted). You could be hitting a DCHECK that
> attempts to make sure that there is no read_buffer_.

Good catch. :) You are absolutely right here.

http://codereview.chromium.org/27168/diff/3103/3117
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/27168/diff/3103/3117#newcode163
Line 163: // Caller should close the file handle returned by this method or
there will
On 2009/03/09 22:10:21, rvargas wrote:
> nit: Caller -> The caller

Done.

http://codereview.chromium.org/27168/diff/3103/3117#newcode174
Line 174: // |index|. Values other than base::kInvalidPlatformFileValue is
successful
On 2009/03/09 22:10:21, rvargas wrote:
> nit: is -> are

Done.

http://codereview.chromium.org/27168/diff/3103/3117#newcode175
Line 175: // and the file handle should be managed by caller, i.e. caller should
close
On 2009/03/09 22:10:21, rvargas wrote:
> nit caller -> the caller

Done.

http://codereview.chromium.org/27168/diff/3103/3118
File net/disk_cache/entry_impl.cc (right):

http://codereview.chromium.org/27168/diff/3103/3118#newcode384
Line 384: base::PLATFORM_FILE_READ |
On 2009/03/09 22:10:21, rvargas wrote:
> nit: indent this 4 spaces.

Done.

[rvargas (doing something else), 2009-03-11 04:19:35]

Sorry, I didn't realized you changed the code with the last message.

LGTM. The other option you can explore if you see too many messages going on is
to not forward the OnReadCompleted but avoid asking for another buffer next time
(not calling OnWillRead).