URLRequestContext and disk cache for media files

Proposed change to support resource loading for media files.

Highlights of changes:
- Added methods to disk_cache::Entry:
  - Entry::PrepareTargetAsExternalFile(int index)
    Prepare a stream in an entry to use external file for storage.
  - Entry::GetExternalFile(int index)
    Get the external file backing the stream in the entry.
- Added a property "CacheType type_" to HttpCache, along with setter and getter.
  There shall be two cache types, COMMON_CACHE and MEDIA_CACHE for distinguishing between different purpose of HttpCache. We have this property to trigger special behavior for caching needs of media files.
- Added static methods to ChromeURLRequestContext
  - ChromeURLRequestContext::CreateOriginalForMedia
    Create a URLRequestContext for media files for the original profile.
  - ChromeURLRequestContext::CreateOffTheRecordForMedia
    Create a URLRequestContext for media files for off the record profile.
- Added method to Profile interface.
  - GetRequestContextForMedia
    To get the request context for media files from the context.

Design decissions:
- Enforce writing to external file by calling methods to Entry rather than construct Backend by a different flag.
  Since we only want a valid and full response to go into an external file rather than redirection response or erroneous response, we should let HttpCache::Transaction to decide when to have an external file for response data. We eliminate a lot of useless external cache files.
- Adding the CacheType enum and property to HttpCache, we could allow possible (?) future extensions to HttpCache to handle other different caching needs. And there's no need to add change constructors of HttpCache, but maybe we should add a specific constructor to accomodate a media HttpCache?
- Adding Profile::GetRequestContextForMedia()
  Since we will need to use this new request context in ResourceDispatcherHost, I think the best place to keep it is in the profile. Also we will expose to user that there's a separate cache for media, so it's better to expose it in the Profile level to allow settings to the media cache, e.g. max file size, etc.

[rvargas (doing something else), 2009-02-02 23:30:18]

Please add disk cache unit tests for the new code.

http://codereview.chromium.org/19747/diff/34/35
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/19747/diff/34/35#newcode17
Line 17: #include "net/disk_cache/backend_impl.h"
This file should not be here.

http://codereview.chromium.org/19747/diff/34/41
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/19747/diff/34/41#newcode151
Line 151: // Prepare a target stream as an external file, returns the file
handle if
nit: prepares

http://codereview.chromium.org/19747/diff/34/41#newcode155
Line 155: virtual base::PlatformFile PrepareTargetAsExternalFile(int index) = 0;
nit: Maybe UseExternalFile() is simpler.

http://codereview.chromium.org/19747/diff/34/41#newcode158
Line 158: virtual base::PlatformFile GetExternalPlatormFile(int index) = 0;
nit: May I suggest GetPlatformFile() instead.

http://codereview.chromium.org/19747/diff/34/42
File net/disk_cache/entry_impl.cc (right):

http://codereview.chromium.org/19747/diff/34/42#newcode277
Line 277: // size limitation. offset of buf_len could be negative numbers.
This comment is not implemented.

http://codereview.chromium.org/19747/diff/34/42#newcode379
Line 379: return base::kInvalidPlatformFileValue;
We could very well have an external file already. We can return that handle.

http://codereview.chromium.org/19747/diff/34/42#newcode576
Line 576: 
nit: remove

http://codereview.chromium.org/19747/diff/34/42#newcode670
Line 670: if (prepared_as_external_file_[index] || offset + buf_len >
kMaxBlockSize) {
If we do this, Flush() may end up creating an entry on a block file instead.

http://codereview.chromium.org/19747/diff/34/43
File net/disk_cache/entry_impl.h (right):

http://codereview.chromium.org/19747/diff/34/43#newcode44
Line 44: // be written to file directly.
nit: we should move whatever we consider relevant to the actual declaration, in
other words, to disk_cache.h. This just implements that interface.

http://codereview.chromium.org/19747/diff/34/43#newcode154
Line 154: bool prepared_as_external_file_[NUM_STREAMS];  // True if stream is
prepared
nit: maybe need_file_ is better.

http://codereview.chromium.org/19747/diff/34/46
File net/http/http_cache.h (right):

http://codereview.chromium.org/19747/diff/34/46#newcode51
Line 51: // The type of a HttpCache object, essential describe what a HttpCache
object
nit: an HttpCache object. Essentially describes

http://codereview.chromium.org/19747/diff/34/46#newcode54
Line 54: // A HttpCache object for common objects, e.g. html pages, images,
fonts,
nit: an HttpCache

http://codereview.chromium.org/19747/diff/34/46#newcode57
Line 57: // A cache system for media file, e.g. video and audio files. These
files
nit: same here

http://codereview.chromium.org/19747/diff/34/47
File net/http/http_response_info.h (right):

http://codereview.chromium.org/19747/diff/34/47#newcode50
Line 50: // in a standalone file, it's value is base::kInvalidPlatformFileValue.
nit: do you mean if it's not a standalone file?

[Alpha Left Google, 2009-02-04 06:08:23]

Refactored and changed some comments according to rvargas's notes.

[darin (slow to review), 2009-02-04 18:56:29]

how come there are no unit tests?

http://codereview.chromium.org/19747/diff/603/604
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/19747/diff/603/604#newcode82
Line 82: // for media files for off the record mode, we use the 'original'
profile
i'm not sure why this comment mentions off the record mode.

http://codereview.chromium.org/19747/diff/603/604#newcode127
Line 127: // or media files in OTR mode, we use the original context first.
Change this
did you mean "for" instead of "or" here?

http://codereview.chromium.org/19747/diff/603/610
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/19747/diff/603/610#newcode15
Line 15: #include "base/platform_file.h"
instead of including platform_file.h, can you just forward declare the
PlatformFile type?

http://codereview.chromium.org/19747/diff/603/610#newcode151
Line 151: // Prepares a target stream as an external file, returns the file
handle if
"returns a corresponding PlatformFile if successful..."

http://codereview.chromium.org/19747/diff/603/610#newcode158
Line 158: // So we need to cautious about using this option for preparing a
stream or
"need to [be] cautious"

http://codereview.chromium.org/19747/diff/603/610#newcode161
Line 161: // directly *without* buffering.
it seems like there should be some comment here explaining that upon success the
caller is responsible for closing the PlatformFile object.

http://codereview.chromium.org/19747/diff/603/611
File net/disk_cache/entry_impl.cc (right):

http://codereview.chromium.org/19747/diff/603/611#newcode298
Line 298: DCHECK(!user_buffers_[index].get());
Might also be written as:
  DCHECK(!need_file_[index] || !user_buffers_[index].get());

http://codereview.chromium.org/19747/diff/603/611#newcode397
Line 397: if (address.is_initialized() && address.is_separate_file()) {
nit: return early if not initialized or not a separate file.  that way the
remainder of the function can be un-indented.

http://codereview.chromium.org/19747/diff/603/614
File net/http/http_cache.cc (right):

http://codereview.chromium.org/19747/diff/603/614#newcode889
Line 889: // If the cache is for media files, we try to prepare the response
data
please put all of this new code into a helper function.  OnNetworkInfoAvailable
is already a long function!

http://codereview.chromium.org/19747/diff/603/615
File net/http/http_cache.h (right):

http://codereview.chromium.org/19747/diff/603/615#newcode53
Line 53: enum CacheType {
please just go with Type instead of CacheType.  no need for the redundant Cache
prefix.

http://codereview.chromium.org/19747/diff/603/615#newcode56
Line 56: COMMON_CACHE = 0,
please just go with COMMON and MEDIA here.  no need for the redundant CACHE
word.  these enums are already contained within HttpCache so "cache" is
implicit.

it may actually be better to go with TYPE_COMMON and TYPE_MEDIA.  but then i
would also change the Mode values to MODE_NORMAL, MODE_RECORD, and
MODE_PLAYBACK.

http://codereview.chromium.org/19747/diff/603/615#newcode104
Line 104: void set_type(CacheType type) { type_ = type; }
you need a newline above this new pair of setter/getter.  also a similar comment
as the one above set_mode might be good for consistency.

[rvargas (doing something else), 2009-02-04 19:20:40]

(Still missing the unit tests)

http://codereview.chromium.org/19747/diff/603/606
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/19747/diff/603/606#newcode205
Line 205: file_util::AppendToPath(&cache_path, chrome::kMediaCacheDirname);
You cannot use the same path for both types of cache. (normal and OffTheRecord)

http://codereview.chromium.org/19747/diff/603/610
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/19747/diff/603/610#newcode161
Line 161: // directly *without* buffering.
On 2009/02/04 18:56:29, darin wrote:
> it seems like there should be some comment here explaining that upon success
the
> caller is responsible for closing the PlatformFile object.

Actually, that's not the case with the current implementation so the comment
should say that the handle should not be closed, and should not be stored for
future use.

http://codereview.chromium.org/19747/diff/603/611
File net/disk_cache/entry_impl.cc (right):

http://codereview.chromium.org/19747/diff/603/611#newcode296
Line 296: // prepared as an external file.
nit: sorry about this, but the comment is more confusing that the code itself :(

http://codereview.chromium.org/19747/diff/603/611#newcode327
Line 327: if (!need_file_[index] && user_buffers_[index].get()) {
nit: This seems to be doing the same that the DCHECK above. Are you sure you
want to do both things?

http://codereview.chromium.org/19747/diff/603/611#newcode671
Line 671: address.is_separate_file()) {
It seems that if need_file_ is true, the other conditions must also be true. If
that is the case, use only the first one and DCHECK the other two inside the if.

http://codereview.chromium.org/19747/diff/603/611#newcode672
Line 672: return true;
Do we have to do this check here anyway? Maybe we can perform the check
sooner... on PrepareTarget() perhaps?.

http://codereview.chromium.org/19747/diff/603/614
File net/http/http_cache.cc (right):

http://codereview.chromium.org/19747/diff/603/614#newcode903
Line 903: kResponseContentIndex);
nit: can't this go on the previous line?

[Alpha Left Google, 2009-02-05 23:40:38]

I have cleaned it up and provided unit tests.

http://codereview.chromium.org/19747/diff/603/610
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/19747/diff/603/610#newcode15
Line 15: #include "base/platform_file.h"
base::PlatformFile is a typedef, so can't forward declare it.

On 2009/02/04 18:56:29, darin wrote:
> instead of including platform_file.h, can you just forward declare the
> PlatformFile type?

http://codereview.chromium.org/19747/diff/603/611
File net/disk_cache/entry_impl.cc (right):

http://codereview.chromium.org/19747/diff/603/611#newcode327
Line 327: if (!need_file_[index] && user_buffers_[index].get()) {
You are right, the DCHECK above is redundant.

On 2009/02/04 19:20:40, rvargas wrote:
> nit: This seems to be doing the same that the DCHECK above. Are you sure you
> want to do both things?

http://codereview.chromium.org/19747/diff/603/615
File net/http/http_cache.h (right):

http://codereview.chromium.org/19747/diff/603/615#newcode56
Line 56: COMMON_CACHE = 0,
I will go with COMMON and MEDIA first, will refactor Mode and Type in another
CL.

On 2009/02/04 18:56:29, darin wrote:
> please just go with COMMON and MEDIA here.  no need for the redundant CACHE
> word.  these enums are already contained within HttpCache so "cache" is
> implicit.
> 
> it may actually be better to go with TYPE_COMMON and TYPE_MEDIA.  but then i
> would also change the Mode values to MODE_NORMAL, MODE_RECORD, and
> MODE_PLAYBACK.

[rvargas (doing something else), 2009-02-06 00:51:49]

http://codereview.chromium.org/19747/diff/696/1632
File net/disk_cache/entry_impl.cc (right):

http://codereview.chromium.org/19747/diff/696/1632#newcode396
Line 396: DCHECK(cache_file);
this should not be a DCHECK. GetExternalFile can return NULL.

http://codereview.chromium.org/19747/diff/696/1634
File net/disk_cache/entry_unittest.cc (right):

http://codereview.chromium.org/19747/diff/696/1634#newcode814
Line 814: ASSERT_EQ(kDataSize, file->GetLength());
nit: You could also verify that the file size is 0 before writing.

http://codereview.chromium.org/19747/diff/696/1634#newcode846
Line 846: }
It would be nice to also test that without calling UseExternalFile you get an
invalid handle for small things, and a valid file handle for large things.

[Alpha Left Google, 2009-02-06 01:30:36]

One new unit test.

http://codereview.chromium.org/19747/diff/696/1634
File net/disk_cache/entry_unittest.cc (right):

http://codereview.chromium.org/19747/diff/696/1634#newcode814
Line 814: ASSERT_EQ(kDataSize, file->GetLength());
Done.

On 2009/02/06 00:51:49, rvargas wrote:
> nit: You could also verify that the file size is 0 before writing.

http://codereview.chromium.org/19747/diff/696/1634#newcode846
Line 846: }
Done.

On 2009/02/06 00:51:49, rvargas wrote:
> It would be nice to also test that without calling UseExternalFile you get an
> invalid handle for small things, and a valid file handle for large things.

[rvargas (doing something else), 2009-02-06 02:35:21]

LGTM

[darin (slow to review), 2009-02-18 07:29:56]

http://codereview.chromium.org/19747/diff/1649/1650
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/19747/diff/1649/1650#newcode92
Line 92: net::HttpCache* cache = new net::HttpCache(context->proxy_service_,
i think this needs to share the same network layer as the normal request
context.  otherwise, you'll end up having to show extra authentication prompts
to users if they are browsing media behind a site that requires HTTP
authentication.

it looks like we do not have a suitable HttpCache constructor.  the one that
takes both a network_layer and a disk_cache does not work because the disk_cache
needs to be allocated lazily on the IO thread from HttpCache's CreateTransaction
method.  i believe that form of the constructor only exists for unit testing. 
you'll probably need to add a HttpCache constructor that is a variation of the
first:  instead of a ProxyService parameter, you'd pass a HttpTransactionFactory
parameter.

one challenge, however, is that HttpCache holds an owning pointer to the
network_layer_.  hmm :(

it is really the HttpNetworkSession member of the HttpNetworkLayer that we want
to somehow share.  maybe we could just allocate a new HttpNetworkLayer that
points to the same HttpNetworkSession object.

http://codereview.chromium.org/19747/diff/1649/1651
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/19747/diff/1649/1651#newcode32
Line 32: // get called on UI thread. This method takes a profile and reuse the
nit: s/reuse/reuses/

http://codereview.chromium.org/19747/diff/1649/1655
File chrome/common/chrome_constants.h (right):

http://codereview.chromium.org/19747/diff/1649/1655#newcode30
Line 30: extern const FilePath::CharType kIncognitoMediaCacheDirname[];
hmm... i wonder if we want to say "incognito" in the code.  we say "off the
record" elsewhere.  it may be confusing to use both terms in the code.  (i'd
prefer to see all of them change to incognito, but that is probably outside the
scope of this CL.)

http://codereview.chromium.org/19747/diff/1649/1661
File net/http/http_cache.cc (right):

http://codereview.chromium.org/19747/diff/1649/1661#newcode847
Line 847: response_.response_data_file =
there are some cases where a Transaction's disk_entry becomes invalid or is
deleted.  at that point, it would seem that response_.response_data_file becomes
invalid.  how do you deal with that?

http://codereview.chromium.org/19747/diff/1649/1664
File net/http/http_response_info.h (right):

http://codereview.chromium.org/19747/diff/1649/1664#newcode50
Line 50: // not in a standalone file, it's value is
base::kInvalidPlatformFileValue.
it's -> its

[Alpha Left Google, 2009-02-18 21:31:53]

http://codereview.chromium.org/19747/diff/1649/1650
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/19747/diff/1649/1650#newcode92
Line 92: net::HttpCache* cache = new net::HttpCache(context->proxy_service_,
On 2009/02/18 07:29:56, darin wrote:
> i think this needs to share the same network layer as the normal request
> context.  otherwise, you'll end up having to show extra authentication prompts
> to users if they are browsing media behind a site that requires HTTP
> authentication.
> 
> it looks like we do not have a suitable HttpCache constructor.  the one that
> takes both a network_layer and a disk_cache does not work because the
disk_cache
> needs to be allocated lazily on the IO thread from HttpCache's
CreateTransaction
> method.  i believe that form of the constructor only exists for unit testing. 
> you'll probably need to add a HttpCache constructor that is a variation of the
> first:  instead of a ProxyService parameter, you'd pass a
HttpTransactionFactory
> parameter.
> 
> one challenge, however, is that HttpCache holds an owning pointer to the
> network_layer_.  hmm :(
> 
> it is really the HttpNetworkSession member of the HttpNetworkLayer that we
want
> to somehow share.  maybe we could just allocate a new HttpNetworkLayer that
> points to the same HttpNetworkSession object.

I've made the changes to HttpNetworkLayer for sharing the HttpNetworkSession,
but since HttpCache keeps a HttpTransactionFactory rather than an explicit
HttpNetworkLayer (for unit test purposes), a static cast of
HttpCache::network_layer() to HttpNetworkLayer is unavoidable here.

http://codereview.chromium.org/19747/diff/1649/1655
File chrome/common/chrome_constants.h (right):

http://codereview.chromium.org/19747/diff/1649/1655#newcode30
Line 30: extern const FilePath::CharType kIncognitoMediaCacheDirname[];
On 2009/02/18 07:29:56, darin wrote:
> hmm... i wonder if we want to say "incognito" in the code.  we say "off the
> record" elsewhere.  it may be confusing to use both terms in the code.  (i'd
> prefer to see all of them change to incognito, but that is probably outside
the
> scope of this CL.)

I'll change this to OffTheRecord.

http://codereview.chromium.org/19747/diff/1649/1661
File net/http/http_cache.cc (right):

http://codereview.chromium.org/19747/diff/1649/1661#newcode847
Line 847: response_.response_data_file =
On 2009/02/18 07:29:56, darin wrote:
> there are some cases where a Transaction's disk_entry becomes invalid or is
> deleted.  at that point, it would seem that response_.response_data_file
becomes
> invalid.  how do you deal with that?

response_.response_data_file_ will be duplicated once received by a resource
handler in ResourceDispatcherHost (e.g. MediaResourceHandler) for another
process. Once response_.response_data_file_ is filled in with the correct file
handle, HttpCache::HttpTransaction::HandleResult() is always called that would
finally reach ResourceHandler::OnResponseStarted() where the file handle is
duplicated immediately. There's only one exception in this data path when the
URLRequest is paused, then the file handle is not passed to ResourceHandler
immediately. Either we can make sure a media request is never paused before
OnResponseStarted, or we keep a duplicated file handle in
response_.response_data_file_. I think I'll simply not allow pause at the
moment, keeping a duplicated file handle will interfere the lifetime of the
disk_entry in disk_cache.

[Alpha Left Google, 2009-02-25 02:29:47]

http://codereview.chromium.org/19747/diff/1649/1661
File net/http/http_cache.cc (right):

http://codereview.chromium.org/19747/diff/1649/1661#newcode847
Line 847: response_.response_data_file =
On 2009/02/18 21:31:53, Alpha wrote:
> On 2009/02/18 07:29:56, darin wrote:
> > there are some cases where a Transaction's disk_entry becomes invalid or is
> > deleted.  at that point, it would seem that response_.response_data_file
> becomes
> > invalid.  how do you deal with that?
> 
> response_.response_data_file_ will be duplicated once received by a resource
> handler in ResourceDispatcherHost (e.g. MediaResourceHandler) for another
> process. Once response_.response_data_file_ is filled in with the correct file
> handle, HttpCache::HttpTransaction::HandleResult() is always called that would
> finally reach ResourceHandler::OnResponseStarted() where the file handle is
> duplicated immediately. There's only one exception in this data path when the
> URLRequest is paused, then the file handle is not passed to ResourceHandler
> immediately. Either we can make sure a media request is never paused before
> OnResponseStarted, or we keep a duplicated file handle in
> response_.response_data_file_. I think I'll simply not allow pause at the
> moment, keeping a duplicated file handle will interfere the lifetime of the
> disk_entry in disk_cache.

I just went through the code of ResourceDispatcherHost more carefully, it seems
there'll never be a case when ResourceHandler::OnResponseStarted() is called
while the request is paused, so usage of response_data_file within that method
is safe and we will send the handle immediately to renderer.

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

OK

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

On 2009/02/26 22:11:05, darin wrote:
> OK

Thanks for the thorough review. You are awesome! :)