MockAppCacheStorage implemenation

MockAppCacheStorage implemention

This is a quick and easy 'mock' implementation of the storage interface
that doesn't put anything to disk.

We simply add an extra reference to objects when they're put in storage,
and remove the extra reference when they are removed from storage.
Responses are never really removed from the in-memory disk cache.
Delegate callbacks are made asyncly to appropiately mimic what will
happen with a real disk-backed storage impl that involves IO on a
background thread.

This is for use in unit tests and to initially bring up the appcache
related layout tests.

TEST=mock_appcache_storage_unittest.cc
BUG=none


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

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

Existing unit tests pass with this. Remaining todo...

* work up unittests for this new code

* maybe add hooks to simulate failures w/o having to derive and override a
virtual method

[michaeln, 2009-10-21 22:43:50]

http://codereview.chromium.org/300043/diff/1/4
File webkit/appcache/mock_appcache_storage.cc (right):

http://codereview.chromium.org/300043/diff/1/4#newcode66
Line 66: // caches only have one ref
ah... i want to do this todo too

[michaeln, 2009-10-21 23:08:13]

> http://codereview.chromium.org/300043/diff/1/4#newcode66
> Line 66: // caches only have one ref
> ah... i want to do this todo too

New snapshot with that in it.

[michaeln, 2009-10-21 23:29:54]

http://codereview.chromium.org/300043/diff/6005/6007
File webkit/appcache/appcache_storage.h (right):

http://codereview.chromium.org/300043/diff/6005/6007#newcode298
Line 298: // TODO(michaeln): Maybe?
maybe not?

[michaeln, 2009-10-22 06:27:26]

i still need to clean this up and add unit tests... no need to look yet

[michaeln, 2009-10-22 22:13:35]

Hi Jenn, 

I think the new code is ready for review, i'm still working up unit tests, but i
don't expect to change the production code part of this CL (modulo anything the
tests may turn up).

[michaeln, 2009-10-22 22:25:19]

New snapshot, stubs for the handful of tests i think i need to write, but
they're not written yet.

[michaeln, 2009-10-23 04:02:42]

New snapshots, added unit tests

[michaeln, 2009-10-23 04:49:07]

other than the gcc'ism... i think this is ready to go... please take a look when
you can

http://codereview.chromium.org/300043/diff/15002/15010
File webkit/appcache/mock_appcache_storage_unittest.cc (right):

http://codereview.chromium.org/300043/diff/15002/15010#newcode250
Line 250: EXPECT_EQ(1, storage->stored_caches_.size());
bah... signed/unsigned comparison warnings (treated as errors) on gcc... that's
why red tries on mac/linux

i'll take care of that

[michaeln, 2009-10-23 05:00:50]

http://codereview.chromium.org/300043/diff/15002/15005
File webkit/appcache/mock_appcache_storage.cc (right):

http://codereview.chromium.org/300043/diff/15002/15005#newcode252
Line 252: bool MockAppCacheStorage::ShouldGroupLoadAppearAsync(
should i add these comments here...

http://codereview.chromium.org/300043/diff/15002/15005#newcode254
Line 254: if (!group)
we'll have to query the database to see if a group for the manifest_url exists
on disk... so async.

http://codereview.chromium.org/300043/diff/15002/15005#newcode257
Line 257: if (!group->newest_complete_cache())
groups w/o a newest cache can't be stored, so if its in the working_set, return
it immediately... not async.

http://codereview.chromium.org/300043/diff/15002/15005#newcode260
Line 260: if (ShouldCacheLoadAppearAsync(group->newest_complete_cache()))
if any of this group's caches don't require an 'async' appearance... neither
does the group since you can't have a ref to a cache w/o it having a ref to its
group.

in the 'real' impl, if the cache is in the working set... so is its group... so
not async

http://codereview.chromium.org/300043/diff/15002/15005#newcode277
Line 277: return IsCacheStored(cache) && cache->HasOneRef();
if the 'stored' ref is the only ref, real storage will have to load from the
database... so async

[jennb, 2009-10-23 18:27:34]

Didn't get to look at mock_appcache_storage_unittest.cc yet.  Will get to it
later today.

http://codereview.chromium.org/300043/diff/15002/15008
File webkit/appcache/appcache_storage_unittest.cc (right):

http://codereview.chromium.org/300043/diff/15002/15008#newcode9
Line 9: #include "webkit/appcache/appcache_storage.h"
alpha

http://codereview.chromium.org/300043/diff/15002/15008#newcode17
Line 17: class MockStorageDelegate : public AppCacheStorage::Delegate {
Easier to read if this class was not nested inside AppCacheStorageTest?

http://codereview.chromium.org/300043/diff/15002/15008#newcode22
Line 22: TEST_F(AppCacheStorageTest, AddRemoveCache) {
TEST_F still needed if AppCacheStorageTest is empty class?

http://codereview.chromium.org/300043/diff/15002/15005
File webkit/appcache/mock_appcache_storage.cc (right):

http://codereview.chromium.org/300043/diff/15002/15005#newcode19
Line 19: // We simply add an extra reference to object's when they're put in
storage,
s/object's/objects

http://codereview.chromium.org/300043/diff/15002/15005#newcode23
Line 23: // happen with real a disk-backed storage impl that involves IO on a
typo

http://codereview.chromium.org/300043/diff/15002/15005#newcode124
Line 124: // We don't bother with removing responses from mock storage.
Hmm...then I can't put code in my unittests to make sure I've properly cleaned
up storage on failed updates.

http://codereview.chromium.org/300043/diff/15002/15005#newcode129
Line 129: AppCache* cache = working_set_.GetCache(id);
Should be loading from stored_caches_ instead of working set (if code assumes
cache being loaded was previously stored).

http://codereview.chromium.org/300043/diff/15002/15005#newcode136
Line 136: scoped_refptr<AppCacheGroup> group =
working_set_.GetGroup(manifest_url);
Should look in stored_groups_ instead of working set.

http://codereview.chromium.org/300043/diff/15002/15005#newcode138
Line 138: group = new AppCacheGroup(service_, manifest_url);
Add comments why newly created group does not get added to stored groups here.

http://codereview.chromium.org/300043/diff/15002/15005#newcode196
Line 196: // Copy the collection prior to removal, on final release
Make this a subroutine rather than duplicate this code?

http://codereview.chromium.org/300043/diff/15002/15005#newcode211
Line 211: void MockAppCacheStorage::ScheduleTask(Task* task) {
Why do tasks need to be scheduled like this as opposed to a simple PostTask?

http://codereview.chromium.org/300043/diff/15002/15005#newcode248
Line 248: working_set()->RemoveGroup(group);
This breaks the 1:1 relationship between an instance existing and its presence
in the working_set.

http://codereview.chromium.org/300043/diff/15002/15005#newcode252
Line 252: bool MockAppCacheStorage::ShouldGroupLoadAppearAsync(
On 2009/10/23 05:00:50, michaeln wrote:
> should i add these comments here...

Yes, including your comments in the code will help.

http://codereview.chromium.org/300043/diff/15002/15005#newcode257
Line 257: if (!group->newest_complete_cache())
On 2009/10/23 05:00:50, michaeln wrote:
> groups w/o a newest cache can't be stored, so if its in the working_set,
return
> it immediately... not async.

Would you ever get here as any stored group will have a newest cache?

http://codereview.chromium.org/300043/diff/15002/15005#newcode263
Line 263: const AppCacheGroup::Caches& old_caches = group->old_caches();
Would a group being loaded have any old caches?  Up above, old caches are all
removed once a newest cache is stored.

http://codereview.chromium.org/300043/diff/15002/15006
File webkit/appcache/mock_appcache_storage.h (right):

http://codereview.chromium.org/300043/diff/15002/15006#newcode65
Line 65: bool IsCacheStored(const AppCache* cache) {
Putting IsCacheStored, IsGroupStored, IsResponseStored up in appcache_storage.h
would allow unittests to easily verify storage contains expected items or not
without doing a load/read to see if item is in storage.  Unittests don't know
they're working with MockAppCacheStorage so it needs to be in AppCacheStorage.

http://codereview.chromium.org/300043/diff/15002/15006#newcode72
Line 72: bool ShouldGroupLoadAppearAsync(const AppCacheGroup* group);
Perhaps some comments explaining these? Odd naming...makes me think these are
only here for test simulations.

http://codereview.chromium.org/300043/diff/15002/15010
File webkit/appcache/mock_appcache_storage_unittest.cc (right):

http://codereview.chromium.org/300043/diff/15002/15010#newcode332
Line 332: TEST_F(MockAppCacheStorageTest,  FindNoMainResponse) {
extra space

[michaeln, 2009-10-23 19:42:08]

http://codereview.chromium.org/300043/diff/15002/15008
File webkit/appcache/appcache_storage_unittest.cc (right):

http://codereview.chromium.org/300043/diff/15002/15008#newcode9
Line 9: #include "webkit/appcache/appcache_storage.h"
On 2009/10/23 18:27:34, jennb wrote:
> alpha

Done.

http://codereview.chromium.org/300043/diff/15002/15008#newcode17
Line 17: class MockStorageDelegate : public AppCacheStorage::Delegate {
On 2009/10/23 18:27:34, jennb wrote:
> Easier to read if this class was not nested inside AppCacheStorageTest?

A long winded answer :) I've been doing this for a few reasons.

1) to be consistent with the others

2) to hide each others different MockStorageDelegate class from one another

3) the biggest reason the others do this is... so the impl of the MockDelegate's
inline methods can access data members of the outer class w/o having to see full
declaration of the AppCacheStorageTest class. 

That last reason doesn't apply to this one... but this one being consistent with
the others where that last reason matters.

http://codereview.chromium.org/300043/diff/15002/15008#newcode22
Line 22: TEST_F(AppCacheStorageTest, AddRemoveCache) {
On 2009/10/23 18:27:34, jennb wrote:
> TEST_F still needed if AppCacheStorageTest is empty class?

Again, just to be consistent with our other tests.

http://codereview.chromium.org/300043/diff/15002/15005
File webkit/appcache/mock_appcache_storage.cc (right):

http://codereview.chromium.org/300043/diff/15002/15005#newcode19
Line 19: // We simply add an extra reference to object's when they're put in
storage,
On 2009/10/23 18:27:34, jennb wrote:
> s/object's/objects

Done.

http://codereview.chromium.org/300043/diff/15002/15005#newcode23
Line 23: // happen with real a disk-backed storage impl that involves IO on a
On 2009/10/23 18:27:34, jennb wrote:
> typo

Done.

http://codereview.chromium.org/300043/diff/15002/15005#newcode124
Line 124: // We don't bother with removing responses from mock storage.
On 2009/10/23 18:27:34, jennb wrote:
> Hmm...then I can't put code in my unittests to make sure I've properly cleaned
> up storage on failed updates.

You could, add a new data member to MockAppCacheStorage...
  std::vector<int64> doomed_responses_;
... add items to it here, then examine the contents of that collection in your
unit tests.

I can add that collection now if you like.

http://codereview.chromium.org/300043/diff/15002/15005#newcode136
Line 136: scoped_refptr<AppCacheGroup> group =
working_set_.GetGroup(manifest_url);
On 2009/10/23 18:27:34, jennb wrote:
> Should look in stored_groups_ instead of working set.

Don't think that's the behavior we want here. Consider two documents, both refer
to <html manifest=foo>, get opened in proximity to one another (say a page and
its iframe). Both of those are going to try to start an update. They want a
reference to the same Group even though it hasn't been committed to disk yet.

http://codereview.chromium.org/300043/diff/15002/15005#newcode196
Line 196: // Copy the collection prior to removal, on final release
On 2009/10/23 18:27:34, jennb wrote:
> Make this a subroutine rather than duplicate this code?

will do

http://codereview.chromium.org/300043/diff/15002/15005#newcode211
Line 211: void MockAppCacheStorage::ScheduleTask(Task* task) {
On 2009/10/23 18:27:34, jennb wrote:
> Why do tasks need to be scheduled like this as opposed to a simple PostTask?

Because of the extra refs held by the 'task' objects. We have delete semantics
for the 'storage' class, and in the storage classes dtor we have these nice
assertions that things have been cleaned up (all refs dropped). I don't want to
give up on those assertions as they make it real clear real fast when we
introduce a 'leak'.

If the 'tasks' were not deleted upon deletion of this class, those assertions
would fire.

http://codereview.chromium.org/300043/diff/15002/15005#newcode248
Line 248: working_set()->RemoveGroup(group);
On 2009/10/23 18:27:34, jennb wrote:
> This breaks the 1:1 relationship between an instance existing and its presence
> in the working_set.

Remember long ago, i was interested in group_id :)

http://codereview.chromium.org/300043/diff/15002/15005#newcode257
Line 257: if (!group->newest_complete_cache())

> Would you ever get here as any stored group will have a newest cache?

This can get called for unstored groups.

http://codereview.chromium.org/300043/diff/15002/15006
File webkit/appcache/mock_appcache_storage.h (right):

http://codereview.chromium.org/300043/diff/15002/15006#newcode65
Line 65: bool IsCacheStored(const AppCache* cache) {
I don't want to make those part of the real api unless there's a real use case
outside of unittests. Those may not be easy questions to answer syncly for real.

http://codereview.chromium.org/300043/diff/15002/15006#newcode72
Line 72: bool ShouldGroupLoadAppearAsync(const AppCacheGroup* group);
On 2009/10/23 18:27:34, jennb wrote:
> Perhaps some comments explaining these? Odd naming...makes me think these are
> only here for test simulations.

I'll add some comments. These are here so the 'mock' impl will behave asyncly in
ways that (i anticipate at least) a real impl would. This is not just here for
unit tests, here so AppCacheUpdateJob and AppCacheHost and
AppCacheRequestHandler (callers of the storage layer) will get responses out of
the 'mock' class just like they will get out of the 'real' impl.

http://codereview.chromium.org/300043/diff/15002/15010
File webkit/appcache/mock_appcache_storage_unittest.cc (right):

http://codereview.chromium.org/300043/diff/15002/15010#newcode250
Line 250: EXPECT_EQ(1, storage->stored_caches_.size());
On 2009/10/23 04:49:07, michaeln wrote:
> bah... signed/unsigned comparison warnings (treated as errors) on gcc...
that's
> why red tries on mac/linux
> 
> i'll take care of that

Done.

http://codereview.chromium.org/300043/diff/15002/15010#newcode332
Line 332: TEST_F(MockAppCacheStorageTest,  FindNoMainResponse) {
On 2009/10/23 18:27:34, jennb wrote:
> extra space

Done.

[michaeln, 2009-10-23 21:09:25]

New snapshot...

http://codereview.chromium.org/300043/diff/15002/15005
File webkit/appcache/mock_appcache_storage.cc (right):

http://codereview.chromium.org/300043/diff/15002/15005#newcode124
Line 124: // We don't bother with removing responses from mock storage.
> I can add that collection now if you like.

Done

http://codereview.chromium.org/300043/diff/15002/15005#newcode129
Line 129: AppCache* cache = working_set_.GetCache(id);
On 2009/10/23 18:27:34, jennb wrote:
> Should be loading from stored_caches_ instead of working set (if code assumes
> cache being loaded was previously stored).

See earlier comments. I think this 'mock' impl is faithfully reproducing the
correct/desired behavior.

http://codereview.chromium.org/300043/diff/15002/15005#newcode138
Line 138: group = new AppCacheGroup(service_, manifest_url);
On 2009/10/23 18:27:34, jennb wrote:
> Add comments why newly created group does not get added to stored groups here.

Done.

http://codereview.chromium.org/300043/diff/15002/15005#newcode196
Line 196: // Copy the collection prior to removal, on final release
On 2009/10/23 18:27:34, jennb wrote:
> Make this a subroutine rather than duplicate this code?

Done.

http://codereview.chromium.org/300043/diff/15002/15005#newcode252
Line 252: bool MockAppCacheStorage::ShouldGroupLoadAppearAsync(
On 2009/10/23 18:27:34, jennb wrote:
> On 2009/10/23 05:00:50, michaeln wrote:
> > should i add these comments here...
> 
> Yes, including your comments in the code will help.

Done.

http://codereview.chromium.org/300043/diff/15002/15005#newcode263
Line 263: const AppCacheGroup::Caches& old_caches = group->old_caches();
On 2009/10/23 18:27:34, jennb wrote:
> Would a group being loaded have any old caches?  Up above, old caches are all
> removed once a newest cache is stored.

They are removed, but they can linger in memory for as long as they remain in
use.

http://codereview.chromium.org/300043/diff/15002/15006
File webkit/appcache/mock_appcache_storage.h (right):

http://codereview.chromium.org/300043/diff/15002/15006#newcode72
Line 72: bool ShouldGroupLoadAppearAsync(const AppCacheGroup* group);
On 2009/10/23 19:42:08, michaeln wrote:
> On 2009/10/23 18:27:34, jennb wrote:
> > Perhaps some comments explaining these? Odd naming...makes me think these
are
> > only here for test simulations.
> 
> I'll add some comments. These are here so the 'mock' impl will behave asyncly
in
> ways that (i anticipate at least) a real impl would. This is not just here for
> unit tests, here so AppCacheUpdateJob and AppCacheHost and
> AppCacheRequestHandler (callers of the storage layer) will get responses out
of
> the 'mock' class just like they will get out of the 'real' impl.

Done, comments

Any naming suggestions?

[michaeln, 2009-10-23 21:12:42]

I've taken care of these things locally.

http://codereview.chromium.org/300043/diff/16002/16005
File webkit/appcache/mock_appcache_storage.cc (right):

http://codereview.chromium.org/300043/diff/16002/16005#newcode144
Line 144: // Newly created groups are not put on the stored_groups collection
s/b in the

http://codereview.chromium.org/300043/diff/16002/16005#newcode249
Line 249: 
i'll remove the blank line

http://codereview.chromium.org/300043/diff/16002/16006
File webkit/appcache/mock_appcache_storage.h (right):

http://codereview.chromium.org/300043/diff/16002/16006#newcode46
Line 46: typedef std::set<int64> DoomedRepsonseIds;
doh... typo... i'll take care of this

[jennb, 2009-10-23 22:22:23]

http://codereview.chromium.org/300043/diff/16002/16005
File webkit/appcache/mock_appcache_storage.cc (right):

http://codereview.chromium.org/300043/diff/16002/16005#newcode276
Line 276: // If any of this group's caches don't require an 'async' appearance,
neither
If any of the caches require async, the group itself must also be async. The
negatives in the text don't match the code.

http://codereview.chromium.org/300043/diff/16002/16006
File webkit/appcache/mock_appcache_storage.h (right):

http://codereview.chromium.org/300043/diff/16002/16006#newcode92
Line 92: DoomedRepsonseIds doomed_response_ids_;
Guess this works for unittests, but now the unittests have to be aware of
MockAppCacheStorage.

http://codereview.chromium.org/300043/diff/16002/16010
File webkit/appcache/mock_appcache_storage_unittest.cc (right):

http://codereview.chromium.org/300043/diff/16002/16010#newcode10
Line 10: #include "webkit/appcache/appcache_response.h"
alpha

http://codereview.chromium.org/300043/diff/16002/16010#newcode65
Line 65: MockAppCacheStorageTest* test_;
Unused member var

http://codereview.chromium.org/300043/diff/16002/16010#newcode111
Line 111: EXPECT_TRUE(delegate.loaded_group_.get());
Also check that newly created group is not stored and has no cache?

http://codereview.chromium.org/300043/diff/16002/16010#newcode132
Line 132: delegate.loaded_manifest_url_ = GURL();
Use GURL::EmptyGURL()?

http://codereview.chromium.org/300043/diff/16002/16010#newcode208
Line 208: // Hold our refs to simulate the UpdateJob holding these refs.
UpdateJob doesn't hold a ref to the group.

http://codereview.chromium.org/300043/diff/16002/16010#newcode250
Line 250: EXPECT_EQ(size_t(1), storage->stored_caches_.size());
This GCC warning is why I use a local var 'size_t expected = 1' as we're not
supposed to use C-style casts and C++ style is  a lot of typing to get around
this warning.

http://codereview.chromium.org/300043/diff/16002/16010#newcode275
Line 275: // appears to be "stored" and "not currently in use".
"not currently in use" comment incorrect here. group and cache are both stored
and have > 1 ref.

http://codereview.chromium.org/300043/diff/16002/16010#newcode304
Line 304: EXPECT_EQ(size_t(0), storage->stored_caches_.size());
EXPECT_TRUE(storage->stored_caches_.empty());

http://codereview.chromium.org/300043/diff/16002/16010#newcode329
Line 329: EXPECT_TRUE(cache->GetEntry(entry_url)->IsForeign());
Also check that entry IsExplicit to make sure that wasn't clobbered?

http://codereview.chromium.org/300043/diff/16002/16010#newcode332
Line 332: TEST_F(MockAppCacheStorageTest, FindNoMainResponse) {
Is there a way to test finding a main response successfully yet?

[michaeln, 2009-10-23 22:50:23]

New snapshot.

http://codereview.chromium.org/300043/diff/16002/16005
File webkit/appcache/mock_appcache_storage.cc (right):

http://codereview.chromium.org/300043/diff/16002/16005#newcode276
Line 276: // If any of this group's caches don't require an 'async' appearance,
neither
On 2009/10/23 22:22:24, jennb wrote:
> If any of the caches require async, the group itself must also be async. The
> negatives in the text don't match the code.

"If any of the caches require async, the group itself must also be async."

That's not correct. The 'group' can be in memory w/o all of its caches in
memory. A 'cache' can't be in memory w/o also having its group in memory.

So some caches in a group may need to appear 'async', while the group itself
does not. But if any of the group's caches can be loaded syncly, so must the
group.

I'll try to reword the comment.

http://codereview.chromium.org/300043/diff/16002/16006
File webkit/appcache/mock_appcache_storage.h (right):

http://codereview.chromium.org/300043/diff/16002/16006#newcode92
Line 92: DoomedRepsonseIds doomed_response_ids_;
On 2009/10/23 22:22:24, jennb wrote:
> Guess this works for unittests, but now the unittests have to be aware of
> MockAppCacheStorage.

I think that's OK. That's part of why we have a 'mock'... for testing.

http://codereview.chromium.org/300043/diff/16002/16010
File webkit/appcache/mock_appcache_storage_unittest.cc (right):

http://codereview.chromium.org/300043/diff/16002/16010#newcode10
Line 10: #include "webkit/appcache/appcache_response.h"
On 2009/10/23 22:22:24, jennb wrote:
> alpha

Done... bah... i don't know my abc's :)

http://codereview.chromium.org/300043/diff/16002/16010#newcode65
Line 65: MockAppCacheStorageTest* test_;
On 2009/10/23 22:22:24, jennb wrote:
> Unused member var

removed

http://codereview.chromium.org/300043/diff/16002/16010#newcode111
Line 111: EXPECT_TRUE(delegate.loaded_group_.get());
On 2009/10/23 22:22:24, jennb wrote:
> Also check that newly created group is not stored and has no cache?

Done.

http://codereview.chromium.org/300043/diff/16002/16010#newcode132
Line 132: delegate.loaded_manifest_url_ = GURL();
On 2009/10/23 22:22:24, jennb wrote:
> Use GURL::EmptyGURL()?

Either way is valid.

http://codereview.chromium.org/300043/diff/16002/16010#newcode208
Line 208: // Hold our refs to simulate the UpdateJob holding these refs.
On 2009/10/23 22:22:24, jennb wrote:
> UpdateJob doesn't hold a ref to the group.

Changed the comment.

  // Hold a ref to the cache simulate the UpdateJob holding that ref,
  // and hold a ref to the group to simulate the CachHost holding that ref.

http://codereview.chromium.org/300043/diff/16002/16010#newcode250
Line 250: EXPECT_EQ(size_t(1), storage->stored_caches_.size());
On 2009/10/23 22:22:24, jennb wrote:
> This GCC warning is why I use a local var 'size_t expected = 1' as we're not
> supposed to use C-style casts and C++ style is  a lot of typing to get around
> this warning.

This is a minor nuisance all right... note i'm not using a C-style case... i'm
using a C++ style ctor :)

http://codereview.chromium.org/300043/diff/16002/16010#newcode275
Line 275: // appears to be "stored" and "not currently in use".
On 2009/10/23 22:22:24, jennb wrote:
> "not currently in use" comment incorrect here. group and cache are both stored
> and have > 1 ref.

Done.

http://codereview.chromium.org/300043/diff/16002/16010#newcode304
Line 304: EXPECT_EQ(size_t(0), storage->stored_caches_.size());
On 2009/10/23 22:22:24, jennb wrote:
> EXPECT_TRUE(storage->stored_caches_.empty());

Done.

http://codereview.chromium.org/300043/diff/16002/16010#newcode329
Line 329: EXPECT_TRUE(cache->GetEntry(entry_url)->IsForeign());
On 2009/10/23 22:22:24, jennb wrote:
> Also check that entry IsExplicit to make sure that wasn't clobbered?

Done.

http://codereview.chromium.org/300043/diff/16002/16010#newcode332
Line 332: TEST_F(MockAppCacheStorageTest, FindNoMainResponse) {
On 2009/10/23 22:22:24, jennb wrote:
> Is there a way to test finding a main response successfully yet?

Not yet, I'm leaving that work as part of the AppCacheRequesdtHandler CL. That's
why i'm just testing if the none found code path functions as intended.

[jennb, 2009-10-23 23:01:33]

LGTM

Rest are small and so is your figuring out the ShouldGroupLoadAppearAsync
comment/code correction.

http://codereview.chromium.org/300043/diff/16002/16010
File webkit/appcache/mock_appcache_storage_unittest.cc (right):

http://codereview.chromium.org/300043/diff/16002/16010#newcode250
Line 250: EXPECT_EQ(size_t(1), storage->stored_caches_.size());
On 2009/10/23 22:50:23, michaeln wrote:
> This is a minor nuisance all right... note i'm not using a C-style case... i'm
> using a C++ style ctor :)

;-)  I'll have to remember that for next time I hit this.

http://codereview.chromium.org/300043/diff/18003/18011#newcode212
Line 212: EXPECT_TRUE(storage->stored_caches_.empty());
CacheHost

[michaeln, 2009-10-24 00:32:11]

Thnx

http://codereview.chromium.org/300043/diff/18003/18011
File webkit/appcache/mock_appcache_storage_unittest.cc (right):

http://codereview.chromium.org/300043/diff/18003/18011#newcode212
Line 212: // and hold a ref to the group to simulate the CachHost holding that
ref.
On 2009/10/23 23:01:33, jennb wrote:
> CacheHost

Done.

[michaeln, 2009-10-24 00:36:06]

http://codereview.chromium.org/300043/diff/16013/16016
File webkit/appcache/mock_appcache_storage.cc (right):

http://codereview.chromium.org/300043/diff/16013/16016#newcode276
Line 276: // if loading the newest cache should appear async, so to must this
changed locally to... "so too must the"