Http Cache: Handle the asynchronous instantiation of the...

Http Cache: Handle the asynchronous instantiation of the
backend. We queue all trasnactions that reach Start before
we are done creating the disk cache.

BUG=26729
TEST=unittests.



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

[rvargas (doing something else), 2010-05-05 23:11:10]

Part of this change is a rename from NewEntry to PendingOp to be able to reuse
the infrastructure of queuing pending operations to the new case. I hope is not
too much noise.

http://codereview.chromium.org/2002002/diff/1/6
File net/http/http_cache.h (right):

http://codereview.chromium.org/2002002/diff/1/6#newcode31
net/http/http_cache.h:31: class HttpCacheTest_SimpleGET_WaitForBackend_Test;
see below.

http://codereview.chromium.org/2002002/diff/1/6#newcode189
net/http/http_cache.h:189: friend class
::HttpCacheTest_SimpleGET_WaitForBackend_Test;
I had a comment here about not using FRIEND_TEST, but I could use
FRIEND_TEST(::HttpCacheTest, SimpleGET_WaitForBackend);

I could also move the unit tests apart and put them inside the net namespace,
but I was trying to avoid that.

[eroman, 2010-05-06 00:21:10]

Some initial nits:

http://codereview.chromium.org/2002002/diff/1/3
File net/http/http_cache.cc (right):

http://codereview.chromium.org/2002002/diff/1/3#newcode317
net/http/http_cache.cc:317: for (; it != pending_queue.end(); ++it)
consider using STLDeleteElements(&pending_queue);

http://codereview.chromium.org/2002002/diff/1/3#newcode472
net/http/http_cache.cc:472: // See if this is a unit test. TODO(rvargas):
Clenaup this after removing the
typo "Cleanup"

http://codereview.chromium.org/2002002/diff/1/6
File net/http/http_cache.h (right):

http://codereview.chromium.org/2002002/diff/1/6#newcode189
net/http/http_cache.h:189: friend class
::HttpCacheTest_SimpleGET_WaitForBackend_Test;
On 2010/05/05 23:11:11, rvargas wrote:
> I had a comment here about not using FRIEND_TEST, but I could use
> FRIEND_TEST(::HttpCacheTest, SimpleGET_WaitForBackend);
> 
> I could also move the unit tests apart and put them inside the net namespace,
> but I was trying to avoid that.

My preference is friend test+ net namespace (in fact I generally like it when
unit-tests are in the net namespace, since it cuts down on extraneous net::).

Up to you.

http://codereview.chromium.org/2002002/diff/1/6#newcode323
net/http/http_cache.h:323: bool RemovePendingTransactionFromPendingOp(PendingOp*
entry,
nit: perhaps rename entry -> pending_op

http://codereview.chromium.org/2002002/diff/1/6#newcode324
net/http/http_cache.h:324: Transaction* trans);
nit: one extra space.

[rvargas (doing something else), 2010-05-06 01:02:08]

Thanks, updated.

http://codereview.chromium.org/2002002/diff/1/3
File net/http/http_cache.cc (right):

http://codereview.chromium.org/2002002/diff/1/3#newcode317
net/http/http_cache.cc:317: for (; it != pending_queue.end(); ++it)
I thought about that when I started writing it and somehow I forgot :(

On 2010/05/06 00:21:10, eroman wrote:
> consider using STLDeleteElements(&pending_queue);

[eroman, 2010-05-06 06:33:26]

lgtm after addressing the following comments:

http://codereview.chromium.org/2002002/diff/9001/10002
File net/http/http_cache.cc (right):

http://codereview.chromium.org/2002002/diff/9001/10002#newcode455
net/http/http_cache.cc:455: PendingOp* pending_op = GetPendingOp("");
Do you mention anywhere that key of "" corresponds with the backend creation?

I wander if it would be clearer to have a separate variable for this instead of
inserting into pending_ops_.

http://codereview.chromium.org/2002002/diff/9001/10002#newcode459
net/http/http_cache.cc:459: return ERR_IO_PENDING;
seems like |item| is going to leak if |!callback|. Is that an expected path?

http://codereview.chromium.org/2002002/diff/9001/10002#newcode467
net/http/http_cache.cc:467: // See if this is a unit test. TODO(rvargas):
Cleanup this after removing the
My personal preference would be to collapse all of the HttpCache constructors
down to 1, and have it take some "BackendFactory" class that enapsulates all the
relevant parameters. (and have different factories for in-memory, unit-tests,
etc.)

http://codereview.chromium.org/2002002/diff/9001/10002#newcode492
net/http/http_cache.cc:492: return ERR_FAILED;
Is this a reachable state? (Seems more like something for a DCHECK, I dunno).
Either way, I discourage the use of ERR_FAILED as it makes for cryptic user
error logs.

http://codereview.chromium.org/2002002/diff/9001/10002#newcode1073
net/http/http_cache.cc:1073: building_backend_ = false;
I suggest also resetting temp_backend_ to NULL, just to be safe.

http://codereview.chromium.org/2002002/diff/9001/10003
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/2002002/diff/9001/10003#newcode176
net/http/http_cache_transaction.cc:176: int rv = GetBackend();
I don't think this should be called GetBackend(), since it is doing more than
just getting the backend.

Can you just inline GetBackend() here? i.e. set the start state, and enter the
DoLoop?

http://codereview.chromium.org/2002002/diff/9001/10003#newcode544
net/http/http_cache_transaction.cc:544: DCHECK(result == OK || result ==
ERR_FAILED);
Could this check instead be DCHECK_NE(result, ERR_IO_PENDING) ?
Seems limiting to hardcode ERR_FAILED

http://codereview.chromium.org/2002002/diff/9001/10003#newcode1269
net/http/http_cache_transaction.cc:1269: int
HttpCache::Transaction::GetBackend() {
See suggestion above about removing this

http://codereview.chromium.org/2002002/diff/9001/10001
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/2002002/diff/9001/10001#newcode574
net/http/http_cache_unittest.cc:574: class MockDiskCacheNoCB: public
MockDiskCache {
nit: space around ':'

http://codereview.chromium.org/2002002/diff/9001/10001#newcode884
net/http/http_cache_unittest.cc:884: class HttpCacheTest: public testing::Test {
nit: space around ':'

http://codereview.chromium.org/2002002/diff/9001/10001#newcode913
net/http/http_cache_unittest.cc:913: EXPECT_TRUE(false);
How about:
 FAIL()
or
 EXPECT_TRUE(current_test_).

http://codereview.chromium.org/2002002/diff/9001/10001#newcode950
net/http/http_cache_unittest.cc:950: for (int i = 0; i < kNumTransactions; i++)
{
minor nit: I have seen both ++i and i++ in this file. I suggest sticking to one
style.

[rvargas (doing something else), 2010-05-06 20:58:56]

Thanks. Everything was addressed (I'll work on the factory on a separate cl)

http://codereview.chromium.org/2002002/diff/9001/10002
File net/http/http_cache.cc (right):

http://codereview.chromium.org/2002002/diff/9001/10002#newcode459
net/http/http_cache.cc:459: return ERR_IO_PENDING;
fixed.

On 2010/05/06 06:33:26, eroman wrote:
> seems like |item| is going to leak if |!callback|. Is that an expected path?

http://codereview.chromium.org/2002002/diff/9001/10002#newcode467
net/http/http_cache.cc:467: // See if this is a unit test. TODO(rvargas):
Cleanup this after removing the
I'll investigate that option on a follow-up.

On 2010/05/06 06:33:26, eroman wrote:
> My personal preference would be to collapse all of the HttpCache constructors
> down to 1, and have it take some "BackendFactory" class that enapsulates all
the
> relevant parameters. (and have different factories for in-memory, unit-tests,
> etc.)

http://codereview.chromium.org/2002002/diff/9001/10002#newcode492
net/http/http_cache.cc:492: return ERR_FAILED;
Yes, it is reachable (there is a test for that). However, the caller is
expecting ERR_FAILED and the error should never reach the user. See the comment
on the caller side.

On 2010/05/06 06:33:26, eroman wrote:
> Is this a reachable state? (Seems more like something for a DCHECK, I dunno).
> Either way, I discourage the use of ERR_FAILED as it makes for cryptic user
> error logs.

http://codereview.chromium.org/2002002/diff/9001/10003
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/2002002/diff/9001/10003#newcode544
net/http/http_cache_transaction.cc:544: DCHECK(result == OK || result ==
ERR_FAILED);
It could... but the real issue is that the code that we just called logically
returns bool, in that we don't care if there is any error, only if it worked or
not. In other words, it is restrictive to say that it must return ERR_FAILED,
but if that code changes to return something else the return code will be
discarded (so I think is better to DCHECK about it).

btw, that's the reason why I return ERR_FAILED, because that error code is not
intended to reach the user, just inform another layer that a failure happened
and should be dealt with.

On 2010/05/06 06:33:26, eroman wrote:
> Could this check instead be DCHECK_NE(result, ERR_IO_PENDING) ?
> Seems limiting to hardcode ERR_FAILED

http://codereview.chromium.org/2002002/diff/9001/10001
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/2002002/diff/9001/10001#newcode913
net/http/http_cache_unittest.cc:913: EXPECT_TRUE(false);
FAIL has the same problem as ASSERT_TRUE so I left it as
EXPECT_TRUE(current_test_) inside the if check.

On 2010/05/06 06:33:26, eroman wrote:
> How about:
>  FAIL()
> or
>  EXPECT_TRUE(current_test_).

[rvargas (doing something else), 2010-05-07 01:32:38]

I added the callback pointer to PendinOp to delete it inside the HttpCache
destructor if needed, as we discussed.

Thanks again.