Http cache: Add a few more states to the transaction SM....

Http cache: Add a few more states to the transaction SM.

Now we have states for writing and reading the
cached headers.

BUG=26729, 18055
TEST=covered by current unit tests


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

[rvargas (doing something else), 2009-12-16 22:07:40]

http://codereview.chromium.org/500009/diff/1/2
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/500009/diff/1/2#newcode98
net/http/http_cache_transaction.cc:98: static const int kTruncatedEnry = 1;
Typo fixed.

[eroman, 2009-12-17 00:00:09]

LGTM after considering my remove of kTruncatedEnry suggestion.

http://codereview.chromium.org/500009/diff/1/2
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/500009/diff/1/2#newcode587
net/http/http_cache_transaction.cc:587: case STATE_SUCCESSFUL_SEND_REQUEST:
I am not convinced this needs to be a state (since we don't jump to it from
anywhere else).

How about just calling the DoSuccessfulSendRequest() function directly from
DoSendRequestComplete() ?

http://codereview.chromium.org/500009/diff/1/2#newcode1551
net/http/http_cache_transaction.cc:1551: rv = OK;  // !kTruncatedEnry.
The use of kTruncatedEntry is pretty subtle, I strongly discourage it.

I can think of two alternatives:

(1) Create a cross-product of the states. So split the
STATE_CACHE_WRITE_RESPONSE INTO two states:
   STATE_CACHE_WRITE_RESPONSE     // non truncated case
   STATE_CACHE_WRITE_RESPONSE_FOR_TRUNCATED_ENTRY

These could both simply call out to a helper that takes a |bool truncated|.

(2) Set a member variable which you can test in DoCacheWriteResponse().
(Although as with all the member variables, you would need to pay careful
attention to reset it when doing a restart).

http://codereview.chromium.org/500009/diff/1/2#newcode1559
net/http/http_cache_transaction.cc:1559: DCHECK(!response_206_);
nit: I suggest a more verbose name, perhaps "server_responded_206", or making it
into an |int server_response_code|

http://codereview.chromium.org/500009/diff/1/2#newcode1594
net/http/http_cache_transaction.cc:1594: return (truncated_ ? kTruncatedEnry :
OK);
See earlier comment about hopefully getting rid of kTruncatedEntry.

http://codereview.chromium.org/500009/diff/1/2#newcode1597
net/http/http_cache_transaction.cc:1597: 
nit: remove extra newline.

[rvargas (doing something else), 2009-12-17 01:49:07]

Thanks.

http://codereview.chromium.org/500009/diff/1/2
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/500009/diff/1/2#newcode587
net/http/http_cache_transaction.cc:587: case STATE_SUCCESSFUL_SEND_REQUEST:
On 2009/12/17 00:00:09, eroman wrote:
> I am not convinced this needs to be a state (since we don't jump to it from
> anywhere else).

I'm on the fence in this one. I've been going back and forth about changing more
code to be states even though it is not really involved in asynchronous
operations so it could be still be implemented as regular function calls. For
instance, BeginCacheRead, BeginPartialCacheValidation,
BeginExternallyConditionalizedRequest and ValidateEntryHeadersAndContinue could
also become states because they also represent one particular step in the
sequence of actions to complete the transaction... and they execute when the
transaction itself is already running the SM.

On one hand this reduces somewhat the debug friendliness of the code (stack
traces are almost the same all the time), but we are already well into that
situation so it's not like we gain much by leaving a few functions in the stack.
On the other hand, when we add proper tracing to the code, having functionality
moved into states gives us automatic visibility into code that would otherwise
be hidden behind regular function calls. Besides, it reduces the mixed-mode
feeling of some code implemented one way and other code in another way (within
the same class).

http://codereview.chromium.org/500009/diff/1/2#newcode1551
net/http/http_cache_transaction.cc:1551: rv = OK;  // !kTruncatedEnry.
On 2009/12/17 00:00:09, eroman wrote:
> The use of kTruncatedEntry is pretty subtle, I strongly discourage it.

Thanks. I went with door number one.

By the way, would have been acceptable to you if the state required explicit
values depending on the intention (i.e. kTruncate kDontTruncate) to avoid the
overloading of net::OK? I'm asking because the planned solution to bring
EntryAvailable (with a callback) into the state machine also relies on the
return code given by the cache, although in that case it will be a negative
value, and exposed in the list of net error codes.

http://codereview.chromium.org/500009/diff/1/2#newcode1559
net/http/http_cache_transaction.cc:1559: DCHECK(!response_206_);
On 2009/12/17 00:00:09, eroman wrote:
> nit: I suggest a more verbose name, perhaps "server_responded_206", or making
it
> into an |int server_response_code|

The name will probably change again soon... right now it's not "really" what the
server returns so we cannot upgrade it to int.

[eroman, 2009-12-17 05:21:41]

LGTM

> By the way, would have been acceptable to you if the state required explicit
> values depending on the intention (i.e. kTruncate kDontTruncate) to avoid the
> overloading of net::OK?

Anything other than an error-code seems weird to me.

http://codereview.chromium.org/500009/diff/1004/7
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/500009/diff/1004/7#newcode98
net/http/http_cache_transaction.cc:98: static const int kTruncatedEntry = 1;
This can be deleted now yes?

[rvargas (doing something else), 2009-12-17 18:30:12]

Thanks.

http://codereview.chromium.org/500009/diff/1004/7
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/500009/diff/1004/7#newcode98
net/http/http_cache_transaction.cc:98: static const int kTruncatedEntry = 1;
On 2009/12/17 05:21:42, eroman wrote:
> This can be deleted now yes?

Removed. Thanks!.