view-cache: Refactor ViewCacheHelper and ViewHttpCacheJobFactory...

view-cache: Refactor ViewCacheHelper and ViewHttpCacheJobFactory
to use asynchronous interfaces.

BUG=26730
TEST=unittest



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=48438

[rvargas (doing something else), 2010-05-25 19:23:23]

http://codereview.chromium.org/2168004/diff/1/2
File chrome/browser/net/view_http_cache_job_factory.cc (right):

http://codereview.chromium.org/2168004/diff/1/2#newcode70
chrome/browser/net/view_http_cache_job_factory.cc:70: bool
ViewHttpCacheJob::ReadRawData(net::IOBuffer* buf, int buf_size,
We probably want to move at some point to a world where the whole cache is not
dumped in one pass (at Start()) but the renderer receives a limited number of
entries at a time... or we could do some searches or time range dumps...

http://codereview.chromium.org/2168004/diff/1/5
File net/url_request/view_cache_helper.h (right):

http://codereview.chromium.org/2168004/diff/1/5#newcode47
net/url_request/view_cache_helper.h:47: int GetStatisticsHTML(URLRequestContext*
context, std::string* out,
I should probably delete this method and the related states as it is not used by
anybody. Let me know if you think we should keep it.

http://codereview.chromium.org/2168004/diff/1/5#newcode96
net/url_request/view_cache_helper.h:96: scoped_refptr<URLRequestContext>
context_;
This is the cleanest thing I could come up. I'm not that happy about it but it
may be good enough (at least for now).

[eroman, 2010-05-25 21:23:54]

http://codereview.chromium.org/2168004/diff/1/4
File net/url_request/view_cache_helper.cc (right):

http://codereview.chromium.org/2168004/diff/1/4#newcode63
net/url_request/view_cache_helper.cc:63: const std::string& url_prefix) {
nit:indent

http://codereview.chromium.org/2168004/diff/1/5
File net/url_request/view_cache_helper.h (right):

http://codereview.chromium.org/2168004/diff/1/5#newcode47
net/url_request/view_cache_helper.h:47: int GetStatisticsHTML(URLRequestContext*
context, std::string* out,
On 2010/05/25 19:23:23, rvargas wrote:
> I should probably delete this method and the related states as it is not used
by
> anybody. Let me know if you think we should keep it.

If you don't think it is useful then remove it.
But note that the functionality is currently being used by
chrome/browser/dom_ui/net_internals_ui.cc

(shows up on the HTTP Cache tab of about:net-internals).

[rvargas (doing something else), 2010-05-25 22:19:24]

Thanks

http://codereview.chromium.org/2168004/diff/1/4
File net/url_request/view_cache_helper.cc (right):

http://codereview.chromium.org/2168004/diff/1/4#newcode63
net/url_request/view_cache_helper.cc:63: const std::string& url_prefix) {
On 2010/05/25 21:23:54, eroman wrote:
> nit:indent

Done.

http://codereview.chromium.org/2168004/diff/1/5
File net/url_request/view_cache_helper.h (right):

http://codereview.chromium.org/2168004/diff/1/5#newcode47
net/url_request/view_cache_helper.h:47: int GetStatisticsHTML(URLRequestContext*
context, std::string* out,
On 2010/05/25 21:23:54, eroman wrote:
> On 2010/05/25 19:23:23, rvargas wrote:
> > I should probably delete this method and the related states as it is not
used
> by
> > anybody. Let me know if you think we should keep it.
> 
> If you don't think it is useful then remove it.
> But note that the functionality is currently being used by
> chrome/browser/dom_ui/net_internals_ui.cc
> 
> (shows up on the HTTP Cache tab of about:net-internals).

I removed the code... I wrote it because I thought it was used by net-internals,
but the new implementation doesn't use this helper.

[eroman, 2010-05-25 23:36:04]

lgtm

http://codereview.chromium.org/2168004/diff/8001/9003
File net/url_request/view_cache_helper.cc (right):

http://codereview.chromium.org/2168004/diff/8001/9003#newcode91
net/url_request/view_cache_helper.cc:91: context_ = context;
I think we should DCHECK(context), and not allow calling this with a NULL
context.

http://codereview.chromium.org/2168004/diff/8001/9003#newcode175
net/url_request/view_cache_helper.cc:175: HandleResult(rv);
This isn't consistent with how we do the other state machines.
(DoLoop() never executes the callback, but lets the caller do that).

Can you move the HandleResult() to the callers?

There isn't much point returning the |rv| if the callback has already been
invoked on it.

http://codereview.chromium.org/2168004/diff/8001/9003#newcode182
net/url_request/view_cache_helper.cc:182: if (!context_)
Same comment as before, I don't think we should allow context to be NULL.

http://codereview.chromium.org/2168004/diff/8001/9003#newcode215
net/url_request/view_cache_helper.cc:215: return
disk_cache_->OpenNextEntry(&iter_, &entry_, &cache_callback_);
Is this iteration stable across mutations to the disk cache? What happens if the
disk cache is being changed while we are iterating?

http://codereview.chromium.org/2168004/diff/8001/9003#newcode269
net/url_request/view_cache_helper.cc:269:
data_->append("<pre>RESPONSE_INFO_TRUNCATED</pre>");
nit: why not use spaces instead of underscores?

http://codereview.chromium.org/2168004/diff/8001/9003#newcode277
net/url_request/view_cache_helper.cc:277: while
(response.headers->EnumerateHeaderLines(&iter, &name, &value)) {
I wander if this could be replaced with
HttpResponseHeaders::GetNormalizedResponseHeaders()

http://codereview.chromium.org/2168004/diff/8001/9003#newcode298
net/url_request/view_cache_helper.cc:298: buf_len_ = 1;
What does this mean?

http://codereview.chromium.org/2168004/diff/8001/9004
File net/url_request/view_cache_helper.h (right):

http://codereview.chromium.org/2168004/diff/8001/9004#newcode37
net/url_request/view_cache_helper.h:37: // or the object is destroyed.
Can you explain that empty string for |key| has a special meaning?

In fact ideally, I think those should be separated out into two separate public
methods.

[rvargas (doing something else), 2010-05-26 20:10:15]

Thanks.

I added a new file with unit tests.

http://codereview.chromium.org/2168004/diff/8001/9003
File net/url_request/view_cache_helper.cc (right):

http://codereview.chromium.org/2168004/diff/8001/9003#newcode91
net/url_request/view_cache_helper.cc:91: context_ = context;
On 2010/05/25 23:36:04, eroman wrote:
> I think we should DCHECK(context), and not allow calling this with a NULL
> context.

Done.

http://codereview.chromium.org/2168004/diff/8001/9003#newcode175
net/url_request/view_cache_helper.cc:175: HandleResult(rv);
On 2010/05/25 23:36:04, eroman wrote:
> This isn't consistent with how we do the other state machines.
> (DoLoop() never executes the callback, but lets the caller do that).
> 
> Can you move the HandleResult() to the callers?
> 
> There isn't much point returning the |rv| if the callback has already been
> invoked on it.

I removed HandleResult and moved the DoCallback call to OnIoComplete but then I
realized what's the issue with that approach.

At first sight both ways are pretty much the same: A single call to DoCallback 
after checking for ERR_PENDING either from our completion routine or from the
loop itself.

But, I want a way to make sure that certain things are always performed when
returning to the user (either synchronously or asynchronously): I want to make
sure that we don't return ERR_FAILED, and that we release the request context as
soon as possible. That means that now the checks have to be performed at two
different places: the completion code and the class entry point (for a sync
return). One option is to keep HandleResult and call it from two places, so no
big deal in this case... but in general, if the class has n methods that go into
the state machine, we now have to call this code from n+1 places. So it seems
better to just do this at the end of DoLoop so that it is in just one place.

I realize most of the other state machines don't need something like this (there
are no checks or cleanup), but if we really want to have all the state machines
looking exactly the same (I'm not sold on that one if we have a switch
statement), maybe it is better to have a generic cleanup point.

So, for now, I removed the return value of HandleResult.

http://codereview.chromium.org/2168004/diff/8001/9003#newcode182
net/url_request/view_cache_helper.cc:182: if (!context_)
On 2010/05/25 23:36:04, eroman wrote:
> Same comment as before, I don't think we should allow context to be NULL.

Done.

http://codereview.chromium.org/2168004/diff/8001/9003#newcode215
net/url_request/view_cache_helper.cc:215: return
disk_cache_->OpenNextEntry(&iter_, &entry_, &cache_callback_);
On 2010/05/25 23:36:04, eroman wrote:
> Is this iteration stable across mutations to the disk cache? What happens if
the
> disk cache is being changed while we are iterating?

No, it is not. It depends on what actually gets modified, but possible side
effects include duplicated entries or an early termination, coupled with blurry
vision and headaches.

http://codereview.chromium.org/2168004/diff/8001/9003#newcode269
net/url_request/view_cache_helper.cc:269:
data_->append("<pre>RESPONSE_INFO_TRUNCATED</pre>");
On 2010/05/25 23:36:04, eroman wrote:
> nit: why not use spaces instead of underscores?

This is the actual name of the flag, not a description of the event. I have a
thing about displaying generic messages that are not properly translated to any
other language, so having it this way allows me to ignore the issue.

http://codereview.chromium.org/2168004/diff/8001/9003#newcode277
net/url_request/view_cache_helper.cc:277: while
(response.headers->EnumerateHeaderLines(&iter, &name, &value)) {
On 2010/05/25 23:36:04, eroman wrote:
> I wander if this could be replaced with
> HttpResponseHeaders::GetNormalizedResponseHeaders()

We want to show what we actually have stored, not a sanitized version (collapsed
etc).

http://codereview.chromium.org/2168004/diff/8001/9003#newcode298
net/url_request/view_cache_helper.cc:298: buf_len_ = 1;
On 2010/05/25 23:36:04, eroman wrote:
> What does this mean?

This was an attempt to keep the logic simple by allocating a 1 byte buffer
(instead of trying 0 bytes), knowing that the read will return 0 bytes. I
changed the logic to avoid the alloc/read.

http://codereview.chromium.org/2168004/diff/8001/9004
File net/url_request/view_cache_helper.h (right):

http://codereview.chromium.org/2168004/diff/8001/9004#newcode37
net/url_request/view_cache_helper.h:37: // or the object is destroyed.
On 2010/05/25 23:36:04, eroman wrote:
> Can you explain that empty string for |key| has a special meaning?
> 
> In fact ideally, I think those should be separated out into two separate
public
> methods.

Done.

[eroman, 2010-05-27 19:34:38]

lgtm.

thanks for adding the unittest!