Chromium Code Reviews
Help | Chromium Project | Sign in
(160)

Issue 983007: Http cache: Add support for a dedicated cache thread. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 11 months ago by rvargas
Modified:
3 years, 8 months ago
Reviewers:
michaeln, eroman
CC:
chromium-reviews, darin-cc_chromium.org, ben+cc_chromium.org
Visibility:
Public.

Description

Http cache: Add support for a dedicated cache thread. This is an interface-only change, nothing is really moving to another thread yet. BUG=26730 TEST=none Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=45974

Patch Set 1 #

Total comments: 1

Patch Set 2 : '' #

Patch Set 3 : '' #

Total comments: 2

Patch Set 4 : '' #

Total comments: 2

Patch Set 5 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+45 lines, -17 lines) Patch
M chrome/browser/net/chrome_url_request_context.cc View 1 2 3 4 3 chunks +3 lines, -3 lines 0 comments Download
M net/disk_cache/backend_impl.cc View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M net/disk_cache/disk_cache.h View 1 2 3 4 3 chunks +11 lines, -8 lines 0 comments Download
M net/http/http_cache.h View 1 2 3 4 6 chunks +14 lines, -2 lines 0 comments Download
M net/http/http_cache.cc View 1 2 3 4 3 chunks +11 lines, -0 lines 0 comments Download
M webkit/appcache/appcache_disk_cache.cc View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
M webkit/tools/test_shell/test_shell_request_context.cc View 3 4 1 chunk +1 line, -1 line 0 comments Download
Trybot results:
Commit:

Messages

Total messages: 11 (0 generated)
michaeln
i'll look closer tomorrow http://codereview.chromium.org/983007/diff/1/13 File webkit/appcache/appcache_disk_cache.cc (right): http://codereview.chromium.org/983007/diff/1/13#newcode108 webkit/appcache/appcache_disk_cache.cc:108: cache_type, cache_directory, cache_size, force, NULL, ...
4 years, 11 months ago (2010-03-18 06:20:59 UTC) #1
michaeln
LGTM Maybe update the description to indicate that your just putting in the programming interface ...
4 years, 11 months ago (2010-03-18 19:57:34 UTC) #2
michaeln
oh... i also had this question about the API that could be cleared up in ...
4 years, 11 months ago (2010-03-18 19:58:21 UTC) #3
rvargas
Michael, thanks for the review. Eric, your turn :) http://codereview.chromium.org/983007/diff/20001/21005 File net/disk_cache/disk_cache.h (right): http://codereview.chromium.org/983007/diff/20001/21005#newcode61 net/disk_cache/disk_cache.h:61: ...
4 years, 11 months ago (2010-03-18 20:47:57 UTC) #4
michaeln
> I updated the comment to show that passing NULL is not really supported (except ...
4 years, 11 months ago (2010-03-18 21:15:21 UTC) #5
michaeln
> Ok, thanx for the clarification. QQ: What if you were to pass in the ...
4 years, 11 months ago (2010-03-18 21:38:17 UTC) #6
rvargas
On 2010/03/18 21:38:17, michaeln wrote: > > Ok, thanx for the clarification. > > QQ: ...
4 years, 11 months ago (2010-03-18 21:50:26 UTC) #7
eroman
I wander if it wouldn't be better to simplify the constructors of HttpCache, and always ...
4 years, 11 months ago (2010-03-19 20:07:30 UTC) #8
rvargas
Interesting idea. I think we can collapse the constructors so that they always pass the ...
4 years, 11 months ago (2010-03-19 21:31:48 UTC) #9
eroman
As far as the issue initializing backend, won't the interface for backend need to be ...
4 years, 11 months ago (2010-03-19 23:48:32 UTC) #10
rvargas
4 years, 10 months ago (2010-04-29 19:42:44 UTC) #11
I finally checked in this cl.

To answer your last question, yes, the backend interface for creating the cache
is async, and all methods that could block are also async.

The flow goes like this:

1. ChromeURLRequestContext creates the http synchronously at startup.
2. With the first request, the http cache starts the creation of the backend
(when creating the HttpTransaction). It uses an internal completion callback.
3. All transactions that reach Start before the backend is created will be
queued by the http cache (seems safer than just bypassing the cache).
4. When the callback is notified, resume processing of queued TXs. Note that a
possible outcome of this step is that there is no backend at all (and the
session will proceed without a cache).

The downside is that anybody accessing directly the backend must implement a
callback to deal with the async creation. That's the case of the browser data
cleaner and the view-cache implementation.

Of course, this is yet to be implemented.

Thanks again.

On 2010/03/19 23:48:32, eroman wrote:
> As far as the issue initializing backend, won't the interface for backend need
> to be async, so that initialization could happen lazily during one of the
async
> calls?
> 
> Anyhow, I won't block this change from happening, just voicing my ideas.
> 
> LGTM.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87e6a26