AppCacheResponse storage implementation

AppCacheResponse storage implementation

* classes AppCacheResponseReader and AppCacheResponseWriter
* also implements AppCacheStorage.LoadResponseInfo()
* using a memory-backed disk_cache for now, so not really on disk yet

TEST=appcache_response_unittest.cc
BUG=none



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

[jennb, 2009-10-15 18:53:57]

http://codereview.chromium.org/269062/diff/20/5011
File webkit/appcache/appcache_response.cc (right):

http://codereview.chromium.org/269062/diff/20/5011#newcode60
Line 60: io_context_.release()->detach();
when does the io_context_ get destroyed?

http://codereview.chromium.org/269062/diff/20/5011#newcode66
Line 66: void AppCacheResponseIO::OnIOComplete(int result) {
I wonder if we should use a different name (InvokeIOCompleteCallback?) for this
method than the name of the method subclasses override.  Will make it more
obvious that DeferredIOComplete calls the subclass method.

http://codereview.chromium.org/269062/diff/20/5011#newcode126
Line 126: Entry* entry = OpenEntryIfNeeded();
Should OpenEntryIfNeeded go in ReadData to make it analogous to ReadInfo?

http://codereview.chromium.org/269062/diff/20/5011#newcode139
Line 139: // Completed sucessfully syncrhonously, defer so our callers
typo

http://codereview.chromium.org/269062/diff/20/5011#newcode178
Line 178: return net::ERR_CACHE_READ_FAILURE;
Is there a WRITE_FAILURE equivalent for this err?

http://codereview.chromium.org/269062/diff/20/5011#newcode204
Line 204: Entry* entry = CreateEntryIfNeeded();
Ditto earlier question about whether this should go in WriteData instead.

http://codereview.chromium.org/269062/diff/20/5010
File webkit/appcache/appcache_response.h (right):

http://codereview.chromium.org/269062/diff/20/5010#newcode69
Line 69: // even after the entry has been cose.
typo

http://codereview.chromium.org/269062/diff/20/5007
File webkit/appcache/appcache_storage.h (right):

http://codereview.chromium.org/269062/diff/20/5007#newcode160
Line 160: it->get()->delegate->func_and_args;                          \
Need to check if DelegateReference was canceled?

http://codereview.chromium.org/269062/diff/20/5007#newcode165
Line 165: // allow all pending callbacks to that delegate be easily canceled.
typo

[michaeln, 2009-10-15 20:08:21]

http://codereview.chromium.org/269062/diff/20/5011
File webkit/appcache/appcache_response.cc (right):

http://codereview.chromium.org/269062/diff/20/5011#newcode60
Line 60: io_context_.release()->detach();
On 2009/10/15 18:53:58, jennb wrote:
> when does the io_context_ get destroyed?

There are two different code paths.
1) in the AppCacheResponseIO dtor (when there is no io pending), its in a
scoped_ptr
2) when disk_cache finally gets around to calling the raw io completion proc
sometime after the AppCacheResponseIO instance is gone

    void OnRawIOComplete(int result) {
      if (!response_io_)
        delete this;  // <----- here
      else
        response_io_->OnIOComplete(result);
    }

http://codereview.chromium.org/269062/diff/20/5011#newcode66
Line 66: void AppCacheResponseIO::OnIOComplete(int result) {
On 2009/10/15 18:53:58, jennb wrote:
> I wonder if we should use a different name (InvokeIOCompleteCallback?) for
this
> method than the name of the method subclasses override.  Will make it more
> obvious that DeferredIOComplete calls the subclass method.

Sure... i'll make OnIOComplete pure virtual in the base class, and redifine this
as a helper method... maybe InvokeUserCompletionCallback?

http://codereview.chromium.org/269062/diff/20/5011#newcode126
Line 126: Entry* entry = OpenEntryIfNeeded();
On 2009/10/15 18:53:58, jennb wrote:
> Should OpenEntryIfNeeded go in ReadData to make it analogous to ReadInfo?

We could, but ReadData itself doesn't need to use the 'entry' instance, so its
kind of out of place.

Maybe Read/WriteRaw shouldn't be calling Open/CreateEntryIfNeeded at all, it
just DCHECKS(entry_).

I could make that change.

http://codereview.chromium.org/269062/diff/20/5011#newcode178
Line 178: return net::ERR_CACHE_READ_FAILURE;
On 2009/10/15 18:53:58, jennb wrote:
> Is there a WRITE_FAILURE equivalent for this err?

I'll check

http://codereview.chromium.org/269062/diff/20/5010
File webkit/appcache/appcache_response.h (right):

http://codereview.chromium.org/269062/diff/20/5010#newcode83
Line 83: delete this;
doh...

else
  response_io_->DoSomething

http://codereview.chromium.org/269062/diff/20/5007
File webkit/appcache/appcache_storage.h (right):

http://codereview.chromium.org/269062/diff/20/5007#newcode160
Line 160: it->get()->delegate->func_and_args;                          \
On 2009/10/15 18:53:58, jennb wrote:
> Need to check if DelegateReference was canceled?

Yes thank you

if (it->get()->delegate)
  it->get()->delegate->func_and_args;

http://codereview.chromium.org/269062/diff/20/5007#newcode165
Line 165: // allow all pending callbacks to that delegate be easily canceled.
On 2009/10/15 18:53:58, jennb wrote:
> typo

cancelled vs canceled, or somethign (sic:) else?

http://codereview.chromium.org/269062/diff/20/5008
File webkit/appcache/mock_appcache_storage.cc (right):

http://codereview.chromium.org/269062/diff/20/5008#newcode39
Line 39: void MockAppCacheStorage::LoadResponseInfo(
maybe hoist this method into the base class?

[jennb, 2009-10-15 20:21:21]

On Thu, Oct 15, 2009 at 1:08 PM, <michaeln@chromium.org> wrote:

> Reviewers: jennb, rvargas,
>
>
>
> http://codereview.chromium.org/269062/diff/20/5007#newcode165
> Line 165: // allow all pending callbacks to that delegate be easily
> canceled.
> On 2009/10/15 18:53:58, jennb wrote:
>
>> typo
>>
>
> cancelled vs canceled, or somethign (sic:) else?
>

"delegate be easily" -> "delegate to be easily"
I noticed cancelled was misspelled, but it is consistently so all over the
place and by many people so I don't nitpick that one. :-)


>
> http://codereview.chromium.org/269062/diff/20/5008
> File webkit/appcache/mock_appcache_storage.cc (right):
>
> http://codereview.chromium.org/269062/diff/20/5008#newcode39
> Line 39: void MockAppCacheStorage::LoadResponseInfo(
> maybe hoist this method into the base class?
>
>
Sure if it seems like there's nothing special about the mock impl.

DEPS file:
I was wondering if a comment would be nice to explain that we need to add
net to the include_rules in order to use disk_cache?

Jenn

[michaeln, 2009-10-15 20:33:23]

Uploaded a new snapshot with changes outlined in a previous repsponse.

> "delegate be easily" -> "delegate to be easily"

oh... thnx

> DEPS file

Maybe I could make the DEPS run more specific...

include_rules = [
  "+net/disk_cache",
]

[rvargas (doing something else), 2009-10-15 23:30:08]

First three files so far.

http://codereview.chromium.org/269062/diff/12001/11007
File webkit/appcache/appcache_response.h (right):

http://codereview.chromium.org/269062/diff/12001/11007#newcode9
Line 9: #include "base/logging.h"
Could you remove this?. We try to keep logging.h out of headers files to speedup
build time. (of course this means moving DCHECKS to cc files)

http://codereview.chromium.org/269062/diff/12001/11007#newcode69
Line 69: // even after the entry has been cose.
nit: typo cose

http://codereview.chromium.org/269062/diff/12001/11007#newcode70
Line 70: class IOContext {
If there are no extra resources that have to be preserved until the callback, I
think that this class may be replaced with a net::CancelableCompletionCallback

http://codereview.chromium.org/269062/diff/12001/11007#newcode91
Line 91: explicit AppCacheResponseIO(
no need for explicit here.

http://codereview.chromium.org/269062/diff/12001/11007#newcode95
Line 95: void DeferredIOComplete(int result);
nit: it would be nice to have some description here about what this method does.

http://codereview.chromium.org/269062/diff/13001/13007
File webkit/appcache/appcache_response.cc (right):

http://codereview.chromium.org/269062/diff/13001/13007#newcode28
Line 28: }
You may want to keep up to here on an anonymous namespace.

http://codereview.chromium.org/269062/diff/13001/13007#newcode72
Line 72: void AppCacheResponseIO::DeferredIOComplete(int result) {
Any reason not to post OnIOComplete() directly?

http://codereview.chromium.org/269062/diff/13001/13007#newcode79
Line 79: scoped_refptr<HttpResponseInfoIOBuffer> info_buffer;
You probably don't need to grab extra references to the buffers (doing work to
make sure they are valid during the callback)... if the callers need them they
should be holding a reference.

http://codereview.chromium.org/269062/diff/13001/13007#newcode97
Line 97: Entry* entry = OpenEntryIfNeeded();
I guess you want this to be entry_

http://codereview.chromium.org/269062/diff/13001/13007#newcode105
Line 105: return ReadRaw(kResponseInfoIndex, 0, buffer_.get(), size);
nit: no need to use get() for a scoped_refptr, unless you want to be very
explicit about it.

http://codereview.chromium.org/269062/diff/13001/13007#newcode114
Line 114: Entry* entry = OpenEntryIfNeeded();
same here

http://codereview.chromium.org/269062/diff/13001/13007#newcode120
Line 120: if (read_position_ + buf_len > range_length_) {
This may be an integer overflow... don't know if we care.

http://codereview.chromium.org/269062/diff/13001/13007#newcode164
Line 164: disk_cache_->CreateEntry(response_key(response_id_), &entry_);
OpenEntry

http://codereview.chromium.org/269062/diff/13001/13007#newcode228
Line 228: // DCHECK(we wrote the amount we wanted to write);
nit: TODO?

[michaeln, 2009-10-16 01:26:17]

New snapshot

http://codereview.chromium.org/269062/diff/12001/11007
File webkit/appcache/appcache_response.h (right):

http://codereview.chromium.org/269062/diff/12001/11007#newcode69
Line 69: // even after the entry has been cose.
On 2009/10/15 23:30:08, rvargas wrote:
> nit: typo cose

doesn't matter anymore

http://codereview.chromium.org/269062/diff/12001/11007#newcode70
Line 70: class IOContext {
On 2009/10/15 23:30:08, rvargas wrote:
> If there are no extra resources that have to be preserved until the callback,
I
> think that this class may be replaced with a net::CancelableCompletionCallback

sweet... thnx... done

http://codereview.chromium.org/269062/diff/13001/13007
File webkit/appcache/appcache_response.cc (right):

http://codereview.chromium.org/269062/diff/13001/13007#newcode28
Line 28: }
On 2009/10/15 23:30:08, rvargas wrote:
> You may want to keep up to here on an anonymous namespace.

Done.

http://codereview.chromium.org/269062/diff/13001/13007#newcode72
Line 72: void AppCacheResponseIO::DeferredIOComplete(int result) {
On 2009/10/15 23:30:08, rvargas wrote:
> Any reason not to post OnIOComplete() directly?

Its a virtual method, don't think i can take the address of it in
CallDeferredIOCompleteSoon.

This wart bugs me too... i'll see what i can do about it.

http://codereview.chromium.org/269062/diff/13001/13007#newcode79
Line 79: scoped_refptr<HttpResponseInfoIOBuffer> info_buffer;
Good point, i was thinking i have to hold the reference for the IOBuffer that we
make (for the ReadInfo case). But we don't since the caller doesn't ever see
that anyway.

Done

http://codereview.chromium.org/269062/diff/13001/13007#newcode97
Line 97: Entry* entry = OpenEntryIfNeeded();
On 2009/10/15 23:30:08, rvargas wrote:
> I guess you want this to be entry_

The assignment to entry_ actually happens in Open/CreateIfNeeded, so this local
was intentional. I could change this to read...

if (!OpenEntryIfNeeded())
  return net::ERR;
entry_->DoSomething();

... and have Open/CreateEntryIfNeeded return a bool.

http://codereview.chromium.org/269062/diff/13001/13007#newcode105
Line 105: return ReadRaw(kResponseInfoIndex, 0, buffer_.get(), size);
On 2009/10/15 23:30:08, rvargas wrote:
> nit: no need to use get() for a scoped_refptr, unless you want to be very
> explicit about it.

I tend to be explicit, but i get this comment a lot :)

http://codereview.chromium.org/269062/diff/13001/13007#newcode120
Line 120: if (read_position_ + buf_len > range_length_) {
On 2009/10/15 23:30:08, rvargas wrote:
> This may be an integer overflow... don't know if we care.

I'll put a TODO here.

http://codereview.chromium.org/269062/diff/13001/13007#newcode164
Line 164: disk_cache_->CreateEntry(response_key(response_id_), &entry_);
On 2009/10/15 23:30:08, rvargas wrote:
> OpenEntry

Done, thanx

http://codereview.chromium.org/269062/diff/13001/13007#newcode228
Line 228: // DCHECK(we wrote the amount we wanted to write);
On 2009/10/15 23:30:08, rvargas wrote:
> nit: TODO?

Done and I'll fill in the DCHECK prior to submitting.

[rvargas (doing something else), 2009-10-16 21:33:09]

I feel I would have to read way more code to do a real review of anything beyond
appcache_storage. Let me know if you want me to do that.

http://codereview.chromium.org/269062/diff/1013/1015
File webkit/appcache/appcache_storage.h (right):

http://codereview.chromium.org/269062/diff/1013/1015#newcode13
Line 13: #include "base/logging.h"
Same thing here.

http://codereview.chromium.org/269062/diff/14001/1028
File webkit/appcache/appcache_response.cc (right):

http://codereview.chromium.org/269062/diff/14001/1028#newcode20
Line 20: // Disk cache entry data indices.
nit: empty line after namespace (and at the end of the block).

http://codereview.chromium.org/269062/diff/14001/1024
File webkit/appcache/appcache_storage.h (right):

http://codereview.chromium.org/269062/diff/14001/1024#newcode160
Line 160: #define FOR_EACH_DELEGATE(delegates, func_and_args)                \
Can we move this to a cc file?

[michaeln, 2009-10-16 23:33:57]

> I feel I would have to read way more code to do a real
> review of anything beyond appcache_storage. Let me know
> if you want me to do that.

appcache_storage and appcache_response are is plenty... thnx!


New snapshot
* deltas mostly in unit tests
* i have a 'harness' worked out, but still working on the tests themselves

http://codereview.chromium.org/269062/diff/12001/11007
File webkit/appcache/appcache_response.h (right):

http://codereview.chromium.org/269062/diff/12001/11007#newcode9
Line 9: #include "base/logging.h"
On 2009/10/15 23:30:08, rvargas wrote:
> Could you remove this?. We try to keep logging.h out of headers files to
speedup
> build time. (of course this means moving DCHECKS to cc files)

Done

I wasn't aware of keeping logging.h out of headers :( There are probably several
occurrences of that in this appcache dir.

http://codereview.chromium.org/269062/diff/1013/1015
File webkit/appcache/appcache_storage.h (right):

http://codereview.chromium.org/269062/diff/1013/1015#newcode13
Line 13: #include "base/logging.h"
On 2009/10/16 21:33:09, rvargas wrote:
> Same thing here.

Will do... although it pains me to not be able to use DCHECK in a .h file :(

http://codereview.chromium.org/269062/diff/14001/1028
File webkit/appcache/appcache_response.cc (right):

http://codereview.chromium.org/269062/diff/14001/1028#newcode20
Line 20: // Disk cache entry data indices.
On 2009/10/16 21:33:09, rvargas wrote:
> nit: empty line after namespace (and at the end of the block).

Done.

http://codereview.chromium.org/269062/diff/14001/1024
File webkit/appcache/appcache_storage.h (right):

http://codereview.chromium.org/269062/diff/14001/1024#newcode160
Line 160: #define FOR_EACH_DELEGATE(delegates, func_and_args)                \
> Can we move this to a cc file?

Not sure i can w/o replicating it in multiple .cc files. This macro will also be
used by MockAppCacheStorage and AppCacheStorageImpl... two derived classes.

[rvargas (doing something else), 2009-10-17 00:31:36]

> > Can we move this to a cc file?
> 
> Not sure i can w/o replicating it in multiple .cc files. This macro will also
be
> used by MockAppCacheStorage and AppCacheStorageImpl... two derived classes.

Yeah, I thought that when looking at the mocked version. I just don't like
macros exposed on headers, but I guess that's fine.

Ping me when the tests are finished.

[michaeln, 2009-10-17 06:51:18]

> Yeah, I thought that when looking at the mocked version. I just don't like
> macros exposed on headers, but I guess that's fine.

I'll keep this in mind when i get to additional usages of this macro, and look
for opportunities to get it out of the .h file.

> Ping me when the tests are finished.

k

[michaeln, 2009-10-19 20:02:11]

New snapshot

* considerably more work on the unittests
* only very small changes outside of the unittest.cc file... added a ctors to
HttpResponseInfoIOBuffer

I'm have to think thru what additional unittests to add still? I wanted to
upload what i've got as the 'harness' is fairly complete, and the three tests
that i do have cover a fair amount of ground... the three tests are...

1. DelegateReferences - test that 'delegate' book keeping is correct
2. LoadResponseInfo_Miss - basic test
3. LoadResponseInfo_Hit - this one covers a lot of ground, it write a response
to storage, then reads the response info back out using LoadResponseInfo.

Missing from the test suite.
* a test the reads response the response body
* reading a range only
* other stuff?

I don't think i need to test multiple concurrent AppCacheResponseReaders since
the DiskCache test suite should have that covered.

[michaeln, 2009-10-19 20:28:42]

http://codereview.chromium.org/269062/diff/19001/20009
File webkit/appcache/appcache_response_unittest.cc (right):

http://codereview.chromium.org/269062/diff/19001/20009#newcode24
Line 24: // Test Harness
-------------------------------------------------------------
I could refactor this some...

class AppCacheResponseTestHarness;
class AppCacheReponseTest;

http://codereview.chromium.org/269062/diff/19001/20009#newcode116
Line 116:
method_factory_.NewRunnableMethod(&AppCacheResponseTest::TestFinishedUnwound));
blah... line length... i'll take care of this

http://codereview.chromium.org/269062/diff/19001/20009#newcode233
Line 233: // Wrappers to call AppCacheResponseReader/Writer Read and Write
methods
maybe put in the 'harness', plus need to add covers for Read operations when i
do a 'Read' oriented test(s)

http://codereview.chromium.org/269062/diff/19001/20009#newcode254
Line 254: // AppCacheResponseReader / Writer completion callbacks
maybe put in the 'harness'

http://codereview.chromium.org/269062/diff/19001/20009#newcode276
Line 276: // HttpResponseInfo utils
maybe put in the 'harness'

[michaeln, 2009-10-20 03:35:55]

New snapshot with read tests...

  void WriteThenVariouslyReadResponse() {
    // This tests involves multiple async steps.
    // 1. First, write a large body using multiple writes, we don't bother
    //    with a response head for this this test.
    // 2. Read the entire body, using multiple reads
    // 3. Read the entire body, using one read.
    // 4. Attempt to read beyond the EOF.
    // 5. Reader just a range.
    // 6. Attempt to read beyond EOF of a range.
    
    // Push tasks in reverse order
    PushNextTask(method_factory_.NewRunnableMethod(
       &AppCacheResponseTest::ReadPastEOF));
    PushNextTask(method_factory_.NewRunnableMethod(
       &AppCacheResponseTest::ReadRange));
    PushNextTask(method_factory_.NewRunnableMethod(
       &AppCacheResponseTest::ReadPastEOF));
    PushNextTask(method_factory_.NewRunnableMethod(
       &AppCacheResponseTest::ReadAllAtOnce));
    PushNextTask(method_factory_.NewRunnableMethod(
       &AppCacheResponseTest::ReadInBlocks));
    PushNextTask(method_factory_.NewRunnableMethod(
       &AppCacheResponseTest::WriteOutBlocks));
    
    // Get them going.
    ScheduleNextTask();
  }

[michaeln, 2009-10-20 04:53:03]

http://codereview.chromium.org/269062/diff/23004/24014
File webkit/appcache/appcache_response_unittest.cc (right):

http://codereview.chromium.org/269062/diff/23004/24014#newcode5
Line 5: #include "base/pickle.h"
alpha ordering is fixed locally, there are also trailing whitespaces in here...
i'll take care of those

http://codereview.chromium.org/269062/diff/23004/24014#newcode223
Line 223: WriteBasicResponse()
ooops;

[michaeln, 2009-10-20 07:00:44]

http://codereview.chromium.org/269062/diff/23004/24014
File webkit/appcache/appcache_response_unittest.cc (right):

http://codereview.chromium.org/269062/diff/23004/24014#newcode199
Line 199: const int64 kResponseId = 111;
maybe should be using...
  static const int64 kNoSuchResponseId = 123;
... defined at the class level since this is used elsewhere too

http://codereview.chromium.org/269062/diff/23004/24014#newcode363
Line 363: result = reader_->ReadData(read_buffer_, kBlockSize, &read_callback_);
hmmm... maybe ReadData() and ReadInfo() should always (for everything) return
asyncly... currently *some* errors are reported syncly.

http://codereview.chromium.org/269062/diff/23004/24014#newcode437
Line 437: int GetHttpResponseInfoSize(const net::HttpResponseInfo* info) {
bah... this depends details of how the impl actually serializes things, the
values kSkip and kTruncated... minor leakage... i could rearrange things to make
this particular pickling static AppCacheResponseIO method calls so the tests
would be resilient to changes (unlikely) here.

void AppCacheResponseIO::PickleHttpResponseInfo(&pickle, info);
void AppCacheResponseIO::UnpickleHttpResponseInfo(pickle, info);

http://codereview.chromium.org/269062/diff/23004/24014#newcode447
Line 447: void FillData(char value, char* data, int data_len) {
maybe some flavor of memset can do this in on line?

http://codereview.chromium.org/269062/diff/23004/24014#newcode452
Line 452: bool CheckData(char value, const char* data, int data_len) {
maybe some flavor of memcmp can do this in one line?

http://codereview.chromium.org/269062/diff/23004/24014#newcode500
Line 500: 
what other tests (if any) do i need for the initial commit?

[rvargas (doing something else), 2009-10-20 18:32:22]

http://codereview.chromium.org/269062/diff/23004/24014
File webkit/appcache/appcache_response_unittest.cc (right):

http://codereview.chromium.org/269062/diff/23004/24014#newcode257
Line 257: // 5. Reader just a range.
nit: typo

http://codereview.chromium.org/269062/diff/23004/24014#newcode336
Line 336: reader_->ReadData(
I think it's important to EXPECT_EQ() the return value of all functions that
you're testing.

http://codereview.chromium.org/269062/diff/23004/24014#newcode380
Line 380: new net::WrappedIOBuffer(body), strlen(body)));
nit: maybe it is better to use const std::string& as the argument instead of
using an implied strlen(body)

http://codereview.chromium.org/269062/diff/23004/24014#newcode437
Line 437: int GetHttpResponseInfoSize(const net::HttpResponseInfo* info) {
On 2009/10/20 07:00:44, michaeln wrote:
> bah... this depends details of how the impl actually serializes things, the
> values kSkip and kTruncated... minor leakage... i could rearrange things to
make
> this particular pickling static AppCacheResponseIO method calls so the tests
> would be resilient to changes (unlikely) here.
> 
> void AppCacheResponseIO::PickleHttpResponseInfo(&pickle, info);
> void AppCacheResponseIO::UnpickleHttpResponseInfo(pickle, info);

Unless you have real users of AppCacheResponseIO::PickleHttpResponseInfo I
wouldn't bother too much with this.

http://codereview.chromium.org/269062/diff/23004/24014#newcode500
Line 500: 
On 2009/10/20 07:00:44, michaeln wrote:
> what other tests (if any) do i need for the initial commit?

A test for deleting a reader or writer while io is pending would be good, and
something testing the behavior of IsXxPending().

[michaeln, 2009-10-20 20:39:46]

> A test for deleting a reader or writer while io is pending would be good, and
> something testing the behavior of IsXxPending().

I've been working up both of those exactly :)

[jennb, 2009-10-20 21:27:31]

http://codereview.chromium.org/269062/diff/23004/24011
File webkit/appcache/appcache_response.cc (right):

http://codereview.chromium.org/269062/diff/23004/24011#newcode8
Line 8: #include "base/logging.h"
alpha

http://codereview.chromium.org/269062/diff/23004/24011#newcode24
Line 24: kResponseInfoIndex,
enum should look like MACROS

http://codereview.chromium.org/269062/diff/23004/24010
File webkit/appcache/appcache_response.h (right):

http://codereview.chromium.org/269062/diff/23004/24010#newcode125
Line 125: DCHECK(!IsReadPending() && !read_position_);
DCHECK in .h

http://codereview.chromium.org/269062/diff/23004/24014
File webkit/appcache/appcache_response_unittest.cc (right):

http://codereview.chromium.org/269062/diff/23004/24014#newcode157
Line 157: // TODO(michaeln): maybe this one belongs in
appcache_storage_unittest.cc
yeah, makes more sense in storage unittest

http://codereview.chromium.org/269062/diff/23004/24014#newcode208
Line 208: EXPECT_TRUE(!storage_delegate_->loaded_info_.get());
replace with EXPECT_FALSE and drop '!'?

http://codereview.chromium.org/269062/diff/23004/24014#newcode238
Line 238: int written_size = GetHttpResponseInfoSize(
Instead of verifying the size, what about comparing the contents so we can have
some check that the info is the same?

http://codereview.chromium.org/269062/diff/23004/24014#newcode253
Line 253: //    with a response head for this this test.
typo

http://codereview.chromium.org/269062/diff/23004/24014#newcode299
Line 299: for (int i = 0; i < kNumBlocks; ++i) {
Count backwards instead:  for (int i = kNumBlocks - 1; i >=0; --i)  and just
pass 'i' to ReadOneBlock?

http://codereview.chromium.org/269062/diff/23004/24014#newcode345
Line 345: reader_->SetReadRange(kBlockSize, kBlockSize);
Also test passing a block that is bigger/smaller than the range length?

http://codereview.chromium.org/269062/diff/23004/24014#newcode361
Line 361: EXPECT_TRUE(result < 0);
Do you want to check for a specific error result?  Ditto below.

http://codereview.chromium.org/269062/diff/23004/24014#newcode363
Line 363: result = reader_->ReadData(read_buffer_, kBlockSize, &read_callback_);
On 2009/10/20 07:00:44, michaeln wrote:
> hmmm... maybe ReadData() and ReadInfo() should always (for everything) return
> asyncly... currently *some* errors are reported syncly.

Let's leave it for now and revisit once we get real code using ReadData/Info to
see which is easier.

http://codereview.chromium.org/269062/diff/23004/24014#newcode500
Line 500: 
On 2009/10/20 07:00:44, michaeln wrote:
> what other tests (if any) do i need for the initial commit?

Is there any way to simulate some of the error conditions on writing - failing
to create entry, write failing - to check behavior in those scenarios?

http://codereview.chromium.org/269062/diff/23004/24009
File webkit/appcache/mock_appcache_storage.h (right):

http://codereview.chromium.org/269062/diff/23004/24009#newcode62
Line 62: DCHECK(disk_cache_.get());
DCHECK in .h file

[michaeln, 2009-10-20 21:46:42]

I have some lint cleanup and minor .h file touching up to do and trailing
whitespace removal type tasks.

Also I was planning on making read/write always return asyncly no matter what to
make life a little easier when coding up callers of these interfaces.

And ricardo, i'll try taking the addr of the virtual method, but i don't think
that will fly, if it doesn't fly, are you ok with leaving XxxRawCompltion stuff
in place?

Otherwise, i think i'm done with this CL.

http://codereview.chromium.org/269062/diff/23004/24011
File webkit/appcache/appcache_response.cc (right):

http://codereview.chromium.org/269062/diff/23004/24011#newcode8
Line 8: #include "base/logging.h"
On 2009/10/20 21:27:32, jennb wrote:
> alpha

Done.

http://codereview.chromium.org/269062/diff/23004/24011#newcode24
Line 24: kResponseInfoIndex,
On 2009/10/20 21:27:32, jennb wrote:
> enum should look like MACROS

You're right, they should. I copy pasted these exactly from http_cache.cc which
also uses these same index values for the same purpose. Should i deviate from
that, or keep it matching here?

http://codereview.chromium.org/269062/diff/23004/24010
File webkit/appcache/appcache_response.h (right):

http://codereview.chromium.org/269062/diff/23004/24010#newcode36
Line 36: // TODO(michaeln): should the ctor/dtor be hidden from public view?
any opinions?

http://codereview.chromium.org/269062/diff/23004/24010#newcode125
Line 125: DCHECK(!IsReadPending() && !read_position_);
> DCHECK in .h

I'll search and destroy these, but not done yet.

http://codereview.chromium.org/269062/diff/23004/24014
File webkit/appcache/appcache_response_unittest.cc (right):

http://codereview.chromium.org/269062/diff/23004/24014#newcode257
Line 257: // 5. Reader just a range.
On 2009/10/20 18:32:23, rvargas wrote:
> nit: typo

Done.

http://codereview.chromium.org/269062/diff/23004/24014#newcode336
Line 336: reader_->ReadData(
I'm planning to change the interface so there is no return value from the these
methods, return values will always be delivered thru the callback, and always
asyncly.

http://codereview.chromium.org/269062/diff/23004/24014#newcode361
Line 361: EXPECT_TRUE(result < 0);
On 2009/10/20 21:27:32, jennb wrote:
> Do you want to check for a specific error result?  Ditto below.

I don't want too strong of a dependency on disk_cache details, this particular
error code may be returned by appcache specific code, if it is i'll test for the
exactl value we return.

http://codereview.chromium.org/269062/diff/23004/24014#newcode363
Line 363: result = reader_->ReadData(read_buffer_, kBlockSize, &read_callback_);
On 2009/10/20 21:27:32, jennb wrote:
> On 2009/10/20 07:00:44, michaeln wrote:
> > hmmm... maybe ReadData() and ReadInfo() should always (for everything)
return
> > asyncly... currently *some* errors are reported syncly.
> 
> Let's leave it for now and revisit once we get real code using ReadData/Info
to
> see which is easier.

As is, when we code up callers of this, we'll have to handle both types of
return scenarios... sync and async. I can change it here easily so the callsites
can skip the sync return path altogether and just do the async path... would
that save time?

http://codereview.chromium.org/269062/diff/23004/24014#newcode380
Line 380: new net::WrappedIOBuffer(body), strlen(body)));
On 2009/10/20 18:32:23, rvargas wrote:
> nit: maybe it is better to use const std::string& as the argument instead of
> using an implied strlen(body)

I'm gonna leave this nit as is... just too simple as it is.

http://codereview.chromium.org/269062/diff/23004/24014#newcode437
Line 437: int GetHttpResponseInfoSize(const net::HttpResponseInfo* info) {
k... leaving as is then

http://codereview.chromium.org/269062/diff/23004/24014#newcode447
Line 447: void FillData(char value, char* data, int data_len) {
changed to use memset

http://codereview.chromium.org/269062/diff/23004/24014#newcode500
Line 500: 
On 2009/10/20 18:32:23, rvargas wrote:
> On 2009/10/20 07:00:44, michaeln wrote:
> > what other tests (if any) do i need for the initial commit?
> 
> A test for deleting a reader or writer while io is pending would be good, and
> something testing the behavior of IsXxPending().

Done

http://codereview.chromium.org/269062/diff/23004/24014#newcode500
Line 500: 

> Is there any way to simulate some of the error conditions on writing - failing
> to create entry, write failing - to check behavior in those scenarios?

Ricardo, any way to poke the disk_cache to simulate errors?

http://codereview.chromium.org/269062/diff/23004/24009
File webkit/appcache/mock_appcache_storage.h (right):

http://codereview.chromium.org/269062/diff/23004/24009#newcode62
Line 62: DCHECK(disk_cache_.get());
On 2009/10/20 21:27:32, jennb wrote:
> DCHECK in .h file

Ditto

[michaeln, 2009-10-20 22:50:24]

New snapshot... but have a few other items to take care of.
* DCHECKs out of .h files :(
* trailing whitespace removed

http://codereview.chromium.org/269062/diff/23004/24014
File webkit/appcache/appcache_response_unittest.cc (right):

http://codereview.chromium.org/269062/diff/23004/24014#newcode157
Line 157: // TODO(michaeln): maybe this one belongs in
appcache_storage_unittest.cc
On 2009/10/20 21:27:32, jennb wrote:
> yeah, makes more sense in storage unittest

How about I leave the TODO for now and take care of moving it over when we add a
appcache_storage_unittest.cc file.

http://codereview.chromium.org/269062/diff/23004/24014#newcode238
Line 238: int written_size = GetHttpResponseInfoSize(
On 2009/10/20 21:27:32, jennb wrote:
> Instead of verifying the size, what about comparing the contents so we can
have
> some check that the info is the same?

Will do.

http://codereview.chromium.org/269062/diff/23004/24014#newcode253
Line 253: //    with a response head for this this test.
On 2009/10/20 21:27:32, jennb wrote:
> typo

Done.

http://codereview.chromium.org/269062/diff/23004/24014#newcode299
Line 299: for (int i = 0; i < kNumBlocks; ++i) {
On 2009/10/20 21:27:32, jennb wrote:
> Count backwards instead:  for (int i = kNumBlocks - 1; i >=0; --i)  and just
> pass 'i' to ReadOneBlock?

Do you feel this should be changed, i'll change it if so, but it feels like a
half-dozen vs six.

http://codereview.chromium.org/269062/diff/23004/24014#newcode345
Line 345: reader_->SetReadRange(kBlockSize, kBlockSize);
On 2009/10/20 21:27:32, jennb wrote:
> Also test passing a block that is bigger/smaller than the range length?

So where the range extends beyond the EOF... will do.

[rvargas (doing something else), 2009-10-20 22:58:32]

> And ricardo, i'll try taking the addr of the virtual method, but i don't think
> that will fly, if it doesn't fly, are you ok with leaving XxxRawCompltion
stuff
> in place?

I'm fine with the current way of doing things.

> > enum should look like MACROS
> 
> You're right, they should. I copy pasted these exactly from http_cache.cc
which
> also uses these same index values for the same purpose. Should i deviate from
> that, or keep it matching here?

We want them to shout (don't ask me why)

> http://codereview.chromium.org/269062/diff/23004/24010#newcode36
> Line 36: // TODO(michaeln): should the ctor/dtor be hidden from public view?
> any opinions?

probably yes, given that it is refcounted.

> > Is there any way to simulate some of the error conditions on writing -
failing
> > to create entry, write failing - to check behavior in those scenarios?
> 
> Ricardo, any way to poke the disk_cache to simulate errors?

Not from the real backend. http_cache_unittests has a mocked version that can
inject errors.

[rvargas (doing something else), 2009-10-20 22:58:59]

http://codereview.chromium.org/269062/diff/25003/25012
File webkit/appcache/appcache_response_unittest.cc (right):

http://codereview.chromium.org/269062/diff/25003/25012#newcode110
Line 110: delete task_stack_.top().first;
nit: having the implementation of every method inlined with the declaration
means that now I have to scroll another 500 lines to get to the declaration of
this member... only to find out that there is no explanation for the bool part
of the pair :(... of course, I could just search the uses of the variable to
figure out what it means, but it's extra time and effort. (just saying...)

http://codereview.chromium.org/269062/diff/25003/25012#newcode155
Line 155: task->Run();
You have to delete this task.

[jennb, 2009-10-20 23:07:30]

On Tue, Oct 20, 2009 at 3:50 PM, <michaeln@chromium.org> wrote:

>
>
> http://codereview.chromium.org/269062/diff/23004/24014#newcode157
> Line 157: // TODO(michaeln): maybe this one belongs in
> appcache_storage_unittest.cc
> On 2009/10/20 21:27:32, jennb wrote:
>
>> yeah, makes more sense in storage unittest
>>
>
> How about I leave the TODO for now and take care of moving it over when
> we add a appcache_storage_unittest.cc file.
>

That's fine with me.


>
> http://codereview.chromium.org/269062/diff/23004/24014#newcode299
> Line 299: for (int i = 0; i < kNumBlocks; ++i) {
> On 2009/10/20 21:27:32, jennb wrote:
>
>> Count backwards instead:  for (int i = kNumBlocks - 1; i >=0; --i)
>>
> and just
>
>> pass 'i' to ReadOneBlock?
>>
>
> Do you feel this should be changed, i'll change it if so, but it feels
> like a half-dozen vs six.
>

IMHO, changing the for loop would make it more obvious (both for reading and
writing blocks). Took me a second look to see why you were passing
"kNumBlocks - i", which then led to doublechecking for off-by-one errors
which are often associated with these calcs.  Hmm...now that I look again,
are the blocks counting from 0 or 1?


> http://codereview.chromium.org/269062/diff/23004/24014#newcode345
> Line 345: reader_->SetReadRange(kBlockSize, kBlockSize);
> On 2009/10/20 21:27:32, jennb wrote:
>
>> Also test passing a block that is bigger/smaller than the range
>>
> length?
>
> So where the range extends beyond the EOF... will do.


Also when the read buffer is bigger/smaller than the range to make sure that
the amount to read is adjusted by the range.

[michaeln, 2009-10-20 23:20:00]

New snapshot...
* additional ReadRange tests
* compare info structures more deeply

> >> Count backwards instead:  for (int i = kNumBlocks - 1; i >=0; --i)
> 
> IMHO, changing the for loop would make it more obvious (both for reading and
> writing blocks). Took me a second look to see why you were passing
> "kNumBlocks - i", which then led to doublechecking for off-by-one errors
> which are often associated with these calcs.  Hmm...now that I look again,
> are the blocks counting from 0 or 1?

I have it as is because when i see a for loop that counts backwards i have to
puzzle out how many times the loop executes, whereas for (i=0; i<num; ++i) is
obvious. So we trade what's obvious and what's not... half-dozen vs six :)

Blocks are numbered from 1.

[jennb, 2009-10-20 23:33:35]

LGTM if trybots and rvargas agree.

Remember to update the description.

[michaeln, 2009-10-21 01:57:15]

New snapshot.

* Made reader and writer return async'ly via the callback for all IO cases, the
return value for ReadXxx() and WriteXxx() methods are now void. Updated doc
comments.

* Cleaned up ScheduleIOCompletionCallback stuff (per ricardo. The following
works fine even though OnIOComplete is pure virtual in AppCacheResponseIO.

void AppCacheResponseIO::ScheduleIOCompletionCallback(int result) {
  MessageLoop::current()->PostTask(FROM_HERE,
      method_factory_.NewRunnableMethod(
          &AppCacheResponseIO::OnIOComplete, result));
}

All that remains is to sync and merge.

[rvargas (doing something else), 2009-10-21 02:16:01]

LGTM after fixing the leak.

[michaeln, 2009-10-21 02:49:01]

On 2009/10/21 02:16:01, rvargas wrote:
> LGTM after fixing the leak.

Thnx... leak fixed, sync'd and merged

[rvargas (doing something else), 2009-10-21 02:53:38]

OK, Thanks.

[michaeln, 2009-10-21 17:25:35]

new snapshot...

http://codereview.chromium.org/269062/diff/26005/26011
File webkit/appcache/appcache_response.cc (right):

http://codereview.chromium.org/269062/diff/26005/26011#newcode209
Line 209: buffer_ = new net::WrappedIOBuffer(
whoa... can't use a wrapped buffer here... we need to keep the pickle around for
a while...

added helper class WrappedPickleIOBuffer

[rvargas (doing something else), 2009-10-21 17:40:59]

LGTM

http://codereview.chromium.org/269062/diff/31001/23069
File webkit/appcache/appcache_response_unittest.cc (right):

http://codereview.chromium.org/269062/diff/31001/23069#newcode635
Line 635: // wait a moment to verify no callbacks
nit: Capitalize wait and end with period.