Http cache: Second pass to move the HttpCache::Transaction...

Http cache: Second pass to move the HttpCache::Transaction
to a state machine.

BUG=26729
TEST=current unit tests.


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

[eroman, 2009-12-04 09:32:05]

lgtm

http://codereview.chromium.org/464028/diff/3001/3002
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/464028/diff/3001/3002#newcode460
net/http/http_cache_transaction.cc:460: int rv =
cache_->AddTransactionToEntry(new_entry_, this);
I am correct to expect that in future iterations this will be changed to take a
callback, so it can integrate more nicely with the state machine by setting a
next_state_ ?

(As an aside, you may want to reset next_state_ to something here just to avoid
confusion).

http://codereview.chromium.org/464028/diff/3001/3002#newcode576
net/http/http_cache_transaction.cc:576: case STATE_START_ADD_TO_ENTRY:
I suggest renaming these.

How about renaming:

STATE_START_ADD_TO_ENTRY --> STATE_ADD_TO_ENTRY
STATE_FINISH_ADD_TO_ENTRY --> STATE_ADD_TRANSACTION_TO_ENTRY

In future changes I suppose EntryAvailable() would become:

STATE_ADD_TRANSACTION_TO_ENTRY_COMPLETE

which is probably less awkward then

STATE_FINISH_ADD_TO_ENTRY_COMPLETE :)

[rvargas (doing something else), 2009-12-04 20:16:09]

http://codereview.chromium.org/464028/diff/3001/3002
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/464028/diff/3001/3002#newcode460
net/http/http_cache_transaction.cc:460: int rv =
cache_->AddTransactionToEntry(new_entry_, this);
On 2009/12/04 09:32:05, eroman wrote:
> I am correct to expect that in future iterations this will be changed to take
a
> callback, so it can integrate more nicely with the state machine by setting a
> next_state_ ?
> 
> (As an aside, you may want to reset next_state_ to something here just to
avoid
> confusion).
> 

That's an interesting idea (so no, it was not in plan). So far the idea was to
consider both AddToEntry and EntryAvailable as entry points for the transaction
so that basically the state is STATE_NONE when they are called. One problem with
passing a callback to the cache is that sometimes EntryAvailable is not the only
possible way to come back to this code: the cache may decide to call AddToEntry
again, and that would not be expected if we just provide a callback and keep the
SM in a busy state.

Let me see if I can come up with something for the next iteration. (If we end up
keeping this part the way it is, it will be less confusing once everything is
reordered so that there is a clear distinction between public methods for an
idle machine vs. internal state handlers)

http://codereview.chromium.org/464028/diff/3001/3002#newcode576
net/http/http_cache_transaction.cc:576: case STATE_START_ADD_TO_ENTRY:
On 2009/12/04 09:32:05, eroman wrote:
> I suggest renaming these.
> 
> How about renaming:
> 
> STATE_START_ADD_TO_ENTRY --> STATE_ADD_TO_ENTRY
> STATE_FINISH_ADD_TO_ENTRY --> STATE_ADD_TRANSACTION_TO_ENTRY
> 
> In future changes I suppose EntryAvailable() would become:
> 
> STATE_ADD_TRANSACTION_TO_ENTRY_COMPLETE
> 
> which is probably less awkward then
> 
> STATE_FINISH_ADD_TO_ENTRY_COMPLETE :)

I don't like that STATE_ADD_TO_ENTRY is basically the same as
STATE_ADD_TRANSACTION_TO_ENTRY.

I tried:
STATE_START_ADD_TO_ENTRY --> STATE_ADD_TO_ENTRY
STATE_FINISH_ADD_TO_ENTRY --> lack of a good name :(

STATE_START_ADD_TO_ENTRY --> STATE_GET_ENTRY**
STATE_FINISH_ADD_TO_ENTRY --> STATE_ADD_TO_ENTRY

** the logic being that right now all that this state does is to
doom/open/create the entry and jump to the other state, where the transaction is
actually added to the entry. Maybe STATE_INIT_ENTRY?. The only kind of weird
thing is that we'll have:

AddToEntry() {
  next_state_ = STATE_INIT_ENTRY;
  return DoLoop();
}

but from the point of view of the interface of the class, AddToEntry() makes
sense as a name because that's what we want to do, regardless of what's the
first step to do that.

[eroman, 2009-12-04 21:52:00]

http://codereview.chromium.org/464028/diff/3001/3002
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/464028/diff/3001/3002#newcode576
net/http/http_cache_transaction.cc:576: case STATE_START_ADD_TO_ENTRY:
On 2009/12/04 20:16:09, rvargas wrote:
> On 2009/12/04 09:32:05, eroman wrote:
> > I suggest renaming these.
> > 
> > How about renaming:
> > 
> > STATE_START_ADD_TO_ENTRY --> STATE_ADD_TO_ENTRY
> > STATE_FINISH_ADD_TO_ENTRY --> STATE_ADD_TRANSACTION_TO_ENTRY
> > 
> > In future changes I suppose EntryAvailable() would become:
> > 
> > STATE_ADD_TRANSACTION_TO_ENTRY_COMPLETE
> > 
> > which is probably less awkward then
> > 
> > STATE_FINISH_ADD_TO_ENTRY_COMPLETE :)
> 
> I don't like that STATE_ADD_TO_ENTRY is basically the same as
> STATE_ADD_TRANSACTION_TO_ENTRY.
> 
> I tried:
> STATE_START_ADD_TO_ENTRY --> STATE_ADD_TO_ENTRY
> STATE_FINISH_ADD_TO_ENTRY --> lack of a good name :(
> 
> STATE_START_ADD_TO_ENTRY --> STATE_GET_ENTRY**
> STATE_FINISH_ADD_TO_ENTRY --> STATE_ADD_TO_ENTRY
> 
> ** the logic being that right now all that this state does is to
> doom/open/create the entry and jump to the other state, where the transaction
is
> actually added to the entry. Maybe STATE_INIT_ENTRY?. The only kind of weird
> thing is that we'll have:
> 
> AddToEntry() {
>   next_state_ = STATE_INIT_ENTRY;
>   return DoLoop();
> }
> 
> but from the point of view of the interface of the class, AddToEntry() makes
> sense as a name because that's what we want to do, regardless of what's the
> first step to do that.
> 

I think anything other than START/FINISH would be an improvement.
(since it isn't confused with the _COMPLETE states then)

[rvargas (doing something else), 2009-12-05 01:35:13]

Thanks.

STATE_INIT_ENTRY it is.