AppCacheHost cache selection algorithm

* Fleshed out AppCacheHost class a fair amount, in particular the cache selection algorithm.

* Added some AppCacheHost unit tests.

* Introduced AppCacheRequestHandler class, which replaces the clunkyApp
CacheInterceptor::ExtraInfo struct. This impl is entirely skeletal
stubs for now.

TEST=appcache_unittest.cc, but really needs more
BUG=none

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

[michaeln, 2009-09-09 20:13:41]

Hi Jenn,

This is some work in progress that I'd like to clean up and get submitted before
taking on a next chunk of work. There's not too much here, but its close to a
reasonble checkpoint that could be submitted i think.

* Fleshed out AppCacheHost class a fair amount, the interface at least, the impl
is still entirely skeletal stubs

* Introduced AppCacheRequestHandler class, which replaces the clunky
AppCacheInterceptor::ExtraInfo struct. This impl is also entirely skeletal
stubs.

Also, in the act of reviewing this CL I think we can exchange thoughts on how we
expect loading an appcache/group from the database should work. This CL walks up
to that point and further work along these lines is very close to being blocked
on that. Brett's close to submitting his new and shiny sqlite wrapper classes so
there should be nothing in the way of digging into AppCacheDatabase next.

And... adding some folks to the cc'list to get some other eyes on our design
too.

[jennb, 2009-09-09 23:05:18]

Seems a bit odd to me that the URLRequest::UserData is more than just a
container for data.  Perhaps following the pattern of
ResourceDispatcherHostRequestInfo where the user data contains a pointer to the
handler is more intuitive?

http://codereview.chromium.org/192043/diff/1029/11
File webkit/appcache/appcache_backend_impl.cc (right):

http://codereview.chromium.org/192043/diff/1029/11#newcode16
Line 16: STLDeleteValues(&hosts_);
Cool. Learn something new.

http://codereview.chromium.org/192043/diff/1029/10
File webkit/appcache/appcache_backend_impl.h (right):

http://codereview.chromium.org/192043/diff/1029/10#newcode16
Line 16: class AppCacheBackendImpl {
As AppCacheBackend interface is now purely for proxying frontend to backend
requests, interface classes should be renamed AppCacheBackendProxy (and likewise
AppCacheFrontendProxy).  Each has a subclass for Chrome and for testing. Then
the *Impl classes can be simply renamed to AppCacheBackend and AppCacheFrontend.

http://codereview.chromium.org/192043/diff/1029/12
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/192043/diff/1029/12#newcode76
Line 76: frontend_->OnCacheSelected(host_id_, kNoCacheId, UNCACHED);
Is this line just a placeholder or do we also need host to
set_selected_cache(NULL) here?

http://codereview.chromium.org/192043/diff/1029/13
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/192043/diff/1029/13#newcode29
Line 29: class AppCacheHost : public base::SupportsWeakPtr<AppCacheHost> {
Cool. More new stuff for me!

http://codereview.chromium.org/192043/diff/1029/15
File webkit/appcache/appcache_interceptor.cc (right):

http://codereview.chromium.org/192043/diff/1029/15#newcode33
Line 33: AppCacheHost* host = service->GetBackend(process_id)->GetHost(host_id);
Should check if process_id is also bad?

http://codereview.chromium.org/192043/diff/1029/15#newcode40
Line 40: // Create a handler for this reqeust and associate it with the request.
typo reqeust

http://codereview.chromium.org/192043/diff/1029/16
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/192043/diff/1029/16#newcode15
Line 15: return true;
Is every request an Http or Https request?

http://codereview.chromium.org/192043/diff/1029/16#newcode93
Line 93: bool URLRequestHttpJob::ReadRawData(net::IOBuffer* buf, int buf_size,
I'm confused. How are these URLRequestHttpJob methods connected to
AppCacheRequestJob?

http://codereview.chromium.org/192043/diff/1029/17
File webkit/appcache/appcache_request_handler.h (right):

http://codereview.chromium.org/192043/diff/1029/17#newcode16
Line 16: class AppCacheHost;
Don't need this as you have the #include (or vice versa).

[michaeln, 2009-09-09 23:41:36]

no new snapshot yet...

http://codereview.chromium.org/192043/diff/1029/10
File webkit/appcache/appcache_backend_impl.h (right):

http://codereview.chromium.org/192043/diff/1029/10#newcode16
Line 16: class AppCacheBackendImpl {
I buy that. How 'bout i do that in a seperate CL?

http://codereview.chromium.org/192043/diff/1029/12
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/192043/diff/1029/12#newcode76
Line 76: frontend_->OnCacheSelected(host_id_, kNoCacheId, UNCACHED);
> Is this line just a placeholder or do we also need host to
> set_selected_cache(NULL) here?

Good catch, will do! 

Hmmm... maybe set_selected_cache should be what calls the frontend?

http://codereview.chromium.org/192043/diff/1029/13
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/192043/diff/1029/13#newcode29
Line 29: class AppCacheHost : public base::SupportsWeakPtr<AppCacheHost> {
On 2009/09/09 23:05:18, jennb wrote:
> Cool. More new stuff for me!

Darin just recently added this template class for the project.

http://codereview.chromium.org/192043/diff/1029/15
File webkit/appcache/appcache_interceptor.cc (right):

http://codereview.chromium.org/192043/diff/1029/15#newcode17
Line 17: request->SetUserData(instance(), handler);  // request takes ownership
We could put in an ExtraInfo object to 'hold' the handler... but is that really
needed... just an extra heap alloc.

http://codereview.chromium.org/192043/diff/1029/15#newcode33
Line 33: AppCacheHost* host = service->GetBackend(process_id)->GetHost(host_id);
On 2009/09/09 23:05:18, jennb wrote:
> Should check if process_id is also bad?

Don't think we need to. If the process_id was bad, we wouldn't have gotten this
far.

ResourceMessageFilter is on the stack here. That's where the process_id value
comes from. The RMF's AppCacheDispatcherHost is initted with that id (when RMF
is initted), and backend registration happens at that init time.

If there is no backend for this id... the browser process is confused, not the
child process.

http://codereview.chromium.org/192043/diff/1029/15#newcode40
Line 40: // Create a handler for this reqeust and associate it with the request.
On 2009/09/09 23:05:18, jennb wrote:
> typo reqeust

done, thnx

http://codereview.chromium.org/192043/diff/1029/16
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/192043/diff/1029/16#newcode15
Line 15: return true;
On 2009/09/09 23:05:18, jennb wrote:
> Is every request an Http or Https request?

I'll put a todo here for now... and return false... so none are :)

http://codereview.chromium.org/192043/diff/1029/16#newcode93
Line 93: bool URLRequestHttpJob::ReadRawData(net::IOBuffer* buf, int buf_size,
On 2009/09/09 23:05:18, jennb wrote:
> I'm confused. How are these URLRequestHttpJob methods connected to
> AppCacheRequestJob?

Not relevant yet...just some code snippet notes...  i'll remove this commented
out block prior to submitting.

http://codereview.chromium.org/192043/diff/1029/17
File webkit/appcache/appcache_request_handler.h (right):

http://codereview.chromium.org/192043/diff/1029/17#newcode16
Line 16: class AppCacheHost;
On 2009/09/09 23:05:18, jennb wrote:
> Don't need this as you have the #include (or vice versa).

Done.

[jennb, 2009-09-10 17:55:17]

On Wed, Sep 9, 2009 at 4:41 PM, <michaeln@chromium.org> wrote:

> no new snapshot yet...
>
>
> http://codereview.chromium.org/192043/diff/1029/10
> File webkit/appcache/appcache_backend_impl.h (right):
>
> http://codereview.chromium.org/192043/diff/1029/10#newcode16
> Line 16: class AppCacheBackendImpl {
> I buy that. How 'bout i do that in a seperate CL?


Separate CL is fine.


>
>
> http://codereview.chromium.org/192043/diff/1029/12
> File webkit/appcache/appcache_host.cc (right):
>
> http://codereview.chromium.org/192043/diff/1029/12#newcode76
> Line 76: frontend_->OnCacheSelected(host_id_, kNoCacheId, UNCACHED);
>
>> Is this line just a placeholder or do we also need host to
>> set_selected_cache(NULL) here?
>>
>
> Good catch, will do!
>
> Hmmm... maybe set_selected_cache should be what calls the frontend?
>

That's logical.


>
>
> http://codereview.chromium.org/192043/diff/1029/15
> File webkit/appcache/appcache_interceptor.cc (right):
>
> http://codereview.chromium.org/192043/diff/1029/15#newcode17
> Line 17: request->SetUserData(instance(), handler);  // request takes
> ownership
> We could put in an ExtraInfo object to 'hold' the handler... but is that
> really needed... just an extra heap alloc.


Let's leave it as now and see how the code looks when all is done.  If it's
not too convoluted, then I have no objections to making the UserData also
implement handler logic.


>
> http://codereview.chromium.org/192043/diff/1029/15#newcode33
> Line 33: AppCacheHost* host =
> service->GetBackend(process_id)->GetHost(host_id);
> On 2009/09/09 23:05:18, jennb wrote:
>
>> Should check if process_id is also bad?
>>
>
> Don't think we need to. If the process_id was bad, we wouldn't have
> gotten this far.
>

Sounds good.

Jenn


> http://codereview.chromium.org/192043
>
>

[michaeln, 2009-09-10 22:40:26]

Hi again jenn... new snapshot...

I've been poking at this a bit more and dug into the actual impl of AppCacheHost
to a degree. Specifically putting some stakes in the ground about how loading an
appcache from storage should look from its point of view.

Please take a close look at that part of things.

I'm positing the existence of a service->LoadCache(id, completion_callback). If
the cache is already loaded, the callback gets called immediately, otherwise a
the db thread will be tasked to load this cache (if its not already in the
process of loading that cache), and when complete the callback will be called.

I'm assuming a method with those semantics in this CL, but have not yet added
that method sig to AppCacheService (or AppCacheDatabase) or anywhere.

[michaeln, 2009-09-10 23:08:12]

blah... gcc comile errors... trouble declaring the tests as friends?

http://codereview.chromium.org/192043/diff/2004/2011
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/192043/diff/2004/2011#newcode69
Line 69: /* NOT YET
i still have to delete this

[michaeln, 2009-09-11 00:34:30]

new snapshot... put the unit tests in the appcache namespace and used
FRIEND_TEST macro to declare friendly tests

[michaeln, 2009-09-11 19:00:16]

New snapshot 8...

I've started work on unit tests for the AppCacheHost class too... the tests are
not ready to submit yet, but here's a preview.

[michaeln, 2009-09-11 22:40:25]

I intend to add a similar test case for each precondition case

- nothing selected (this one here)
- something selected
- selection pending, and the complete the selection

And observe we get the right callbacks and frontend method invocations.

A mockable service may be nice eventually, but since it the real class doesn't
actually do anything yet, not needed yet :)

[jennb, 2009-09-12 00:17:10]

Sorry took so long. Had to go look up new utils you used that were unfamiliar to
me.

http://codereview.chromium.org/192043/diff/9007/9020
File chrome/common/appcache/appcache_dispatcher_host.cc (right):

http://codereview.chromium.org/192043/diff/9007/9020#newcode49
Line 49: // TODO(michaeln): Handle the invlalid host id error condition,
probably
typo: invlalid

http://codereview.chromium.org/192043/diff/9007/9018
File webkit/appcache/appcache_group_unittest.cc (right):

http://codereview.chromium.org/192043/diff/9007/9018#newcode7
Line 7: #include "webkit/appcache/appcache_host.h"
My alpha mistake. Please fix for me? Thx.

http://codereview.chromium.org/192043/diff/9007/9010
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/192043/diff/9007/9010#newcode18
Line 18: : host_id_(host_id), pending_selected_cache_id_(kNoCacheId),
I had the indentation wrong for initializer lists. Should be : indented 4 lines,
then indent the following 2 more spaces to keep things lined up.  Could you fix
for me here? Thx.

http://codereview.chromium.org/192043/diff/9007/9010#newcode139
Line 139: void AppCacheHost::CacheLoadedCallback(AppCache* cache, int64
cache_id) {
Don't need cache_id param? Can get from cache->cache_id().

http://codereview.chromium.org/192043/diff/9007/9010#newcode149
Line 149: pending_get_status_callback_ = NULL;
Seems cleaner to clear the callback member vars in the place where the callback
gets invoked.  Also, calling *WithCallback from here makes the code convoluted
as I couldn't tell what scenario would have !is_selection_pending with a
callback set until I got down to this part of this file.

Might be cleaner like this (wdyt?)
FooWithCallback(callback, param) {
  DCHECK no callbacks set
  foo_callback_ = callback;
  foo_param = param;

  if selection pending
    return

  FooDoItNow();
}

FooDoItNow() {   // this is private member in class
  DCHECK expected callback set
  DCHECK !is_selection_pending
  do the foo thing now
  invoke callback
  clear foo_callback_ and param_ members
}

http://codereview.chromium.org/192043/diff/9007/9010#newcode176
Line 176: // TODO(michaeln): start a cache update
Don't want to trigger an update within this method.  It might be called by the
update method.

http://codereview.chromium.org/192043/diff/9007/9025
File webkit/appcache/appcache_host_unittest.cc (right):

http://codereview.chromium.org/192043/diff/9007/9025#newcode5
Line 5: #include "base/scoped_ptr.h"
Remember to modify test_shell.gyp for this new file.

http://codereview.chromium.org/192043/diff/9007/9025#newcode12
Line 12: namespace {
what's the empty anon namespace for?

http://codereview.chromium.org/192043/diff/9007/9025#newcode97
Line 97: host.GetStatusWithCallback(get_status_callback_.get(),
reinterpret_cast<void*>(1));
80 chars? i'm guessing if it wraps in rietveld, it's more than 80. some others
below.

http://codereview.chromium.org/192043/diff/9007/9015
File webkit/appcache/appcache_request_handler.h (right):

http://codereview.chromium.org/192043/diff/9007/9015#newcode43
Line 43: // Main vs sub resource loads are very different.
subresource (one word) to be consistent with above?

[jennb, 2009-09-12 02:01:45]

Since you have this open...

http://codereview.chromium.org/192043/diff/9007/9024
File webkit/appcache/appcache_service_unittest.cc (right):

http://codereview.chromium.org/192043/diff/9007/9024#newcode28
Line 28: new AppCacheGroup(&service, GURL::EmptyGURL());
Can you fix this indent to 4 space for me? Thx.

[michaeln, 2009-09-14 04:24:54]

http://codereview.chromium.org/192043/diff/8008/8015
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/192043/diff/8008/8015#newcode40
Line 40: // selection occurs, that extra reference should be dropped.
note to self: 
if (is_main_request_ && host_.get())
  host_->LoadCacheForMainResource(cache_id);

http://codereview.chromium.org/192043/diff/9007/9020
File chrome/common/appcache/appcache_dispatcher_host.cc (right):

http://codereview.chromium.org/192043/diff/9007/9020#newcode49
Line 49: // TODO(michaeln): Handle the invlalid host id error condition,
probably
On 2009/09/12 00:17:11, jennb wrote:
> typo: invlalid

Done.

http://codereview.chromium.org/192043/diff/9007/9018
File webkit/appcache/appcache_group_unittest.cc (right):

http://codereview.chromium.org/192043/diff/9007/9018#newcode7
Line 7: #include "webkit/appcache/appcache_host.h"
On 2009/09/12 00:17:11, jennb wrote:
> My alpha mistake. Please fix for me? Thx.

Done.

http://codereview.chromium.org/192043/diff/9007/9010
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/192043/diff/9007/9010#newcode18
Line 18: : host_id_(host_id), pending_selected_cache_id_(kNoCacheId),
On 2009/09/12 00:17:11, jennb wrote:
> I had the indentation wrong for initializer lists. Should be : indented 4
lines,
> then indent the following 2 more spaces to keep things lined up.  Could you
fix
> for me here? Thx.

Done.

http://codereview.chromium.org/192043/diff/9007/9010#newcode139
Line 139: void AppCacheHost::CacheLoadedCallback(AppCache* cache, int64
cache_id) {
> Don't need cache_id param? Can get from cache->cache_id().

If the load fails, cache will be NULL, but the id will indicate which cache load
failed.

http://codereview.chromium.org/192043/diff/9007/9010#newcode149
Line 149: pending_get_status_callback_ = NULL;
On 2009/09/12 00:17:11, jennb wrote:
> Seems cleaner to clear the callback member vars in the place where the
callback

Done.

http://codereview.chromium.org/192043/diff/9007/9010#newcode176
Line 176: // TODO(michaeln): start a cache update
On 2009/09/12 00:17:11, jennb wrote:
> Don't want to trigger an update within this method.  It might be called by the
> update method.

I had hijacked the set_selected_cache() for the cache selection logic. I've
refactored these methods a bit to make a clearer distinction about the types of
'associations' that we're establishing, and to keep that more seperate from the
cache selection algorithm.

Also see my review comments on your CL on this topic. WDYT?

public for use by UpdateJob...
  AssociateSelectedCache();
  AssociatePotentialCache();

private...
  FinishCacheSelection();

http://codereview.chromium.org/192043/diff/9007/9025
File webkit/appcache/appcache_host_unittest.cc (right):

http://codereview.chromium.org/192043/diff/9007/9025#newcode5
Line 5: #include "base/scoped_ptr.h"
On 2009/09/12 00:17:11, jennb wrote:
> Remember to modify test_shell.gyp for this new file.

I dont follow?

http://codereview.chromium.org/192043/diff/9007/9025#newcode12
Line 12: namespace {
On 2009/09/12 00:17:11, jennb wrote:
> what's the empty anon namespace for?

Done.

http://codereview.chromium.org/192043/diff/9007/9025#newcode97
Line 97: host.GetStatusWithCallback(get_status_callback_.get(),
reinterpret_cast<void*>(1));
On 2009/09/12 00:17:11, jennb wrote:
> 80 chars? i'm guessing if it wraps in rietveld, it's more than 80. some others
> below.

Done.

http://codereview.chromium.org/192043/diff/9007/9015
File webkit/appcache/appcache_request_handler.h (right):

http://codereview.chromium.org/192043/diff/9007/9015#newcode43
Line 43: // Main vs sub resource loads are very different.
On 2009/09/12 00:17:11, jennb wrote:
> subresource (one word) to be consistent with above?

Done.

http://codereview.chromium.org/192043/diff/9007/9024
File webkit/appcache/appcache_service_unittest.cc (right):

http://codereview.chromium.org/192043/diff/9007/9024#newcode28
Line 28: new AppCacheGroup(&service, GURL::EmptyGURL());
On 2009/09/12 02:01:45, jennb wrote:
> Can you fix this indent to 4 space for me? Thx.

Done.

[michaeln, 2009-09-14 18:27:38]

New snapshot 11... simplified how the distinction between potential and selected
is maintained.

[michaeln, 2009-09-14 20:01:47]

New snapshot 12/13...
* fixed some lint errors
* massaged AppCacheHost::SelectCache slightly for readability
* removed some uneeded comments
* fixed and unrelated compile error the try bot was showing for appcacheentry.h
(all of a sudden?)

[jennb, 2009-09-14 22:22:01]

http://codereview.chromium.org/192043/diff/1052/1066
File webkit/appcache/appcache.h (right):

http://codereview.chromium.org/192043/diff/1052/1066#newcode70
Line 70: // Use AppCacheHost::Associate<<>>Host() to manipulate host
association.
s/<<>>Host/<<>>Cache

http://codereview.chromium.org/192043/diff/1052/1072
File webkit/appcache/appcache_group.cc (right):

http://codereview.chromium.org/192043/diff/1052/1072#newcode75
Line 75: void AppCacheGroup::StartUpdateWithNewMasterEntry(
I prefer to let the update job decide whether or not it needs to create a new
cache so that the cache is not created unless determined necessary.

http://codereview.chromium.org/192043/diff/1052/1055
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/192043/diff/1052/1055#newcode82
Line 82: service_->MarkAsForeignEntry(cache_document_was_loaded_from,
document_url);
Suggest keeping the params in the same order for identically named methods.

http://codereview.chromium.org/192043/diff/1052/1055#newcode238
Line 238: // update job should have associated a cache prior to returning.
Not sure I agree with this yet. May be better to wait until necessary to
associate the host with an incomplete cache.

http://codereview.chromium.org/192043/diff/1052/1055#newcode258
Line 258: frontend_->OnCacheSelected(host_id_, kNoCacheId, GetStatus());
This would produce a non-UNCACHED status with kNoCacheId.  That seems
contradictory.

http://codereview.chromium.org/192043/diff/1052/1055#newcode268
Line 268: void AppCacheHost::AssociateCache(AppCache* cache) {
WDYT about droping AssociatePotentialCache and AssociateSelectedCache, instead
at the end of AssociateCache, always call frontend->OnCacheSelected() with the
real cache id.  (We'll need to assign a cache id to a brand new cache even
before it is complete so that we'll have a real cache id.  This eliminates the
problem of when to call frontend->OnCacheSelected() for newly created caches.)

http://codereview.chromium.org/192043/diff/1052/1055#newcode281
Line 281: group_ = associated_cache_->owning_group();
Hmm...I'll have to change my CL to give an incomplete cache an owning_group to
avoid messing this group_ assignment up.

http://codereview.chromium.org/192043/diff/1052/1056
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/192043/diff/1052/1056#newcode71
Line 71: void AssociatePotentialCache(AppCache* cache);
The potential vs selected cache separation seems to add unnecessary complexity. 
The code can easily check if the associated cache is incomplete and drop
requests to utilize the associated cache.

http://codereview.chromium.org/192043/diff/1052/1056#newcode116
Line 116: // The complete cache selected for use this host, if any.
typo

http://codereview.chromium.org/192043/diff/1052/1056#newcode119
Line 119: // The reference to our the group ensures the group exists
typo

http://codereview.chromium.org/192043/diff/1052/1070
File webkit/appcache/appcache_host_unittest.cc (right):

http://codereview.chromium.org/192043/diff/1052/1070#newcode176
Line 176: // The callback should not occur until we have an until
typo

http://codereview.chromium.org/192043/diff/1052/1070#newcode209
Line 209: // The callback should not occur until we have an until
typo

http://codereview.chromium.org/192043/diff/1052/1059
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/192043/diff/1052/1059#newcode34
Line 34: AppCacheRequestHandler::AppCacheRequestHandler(const GURL&
manifest_url,
Is manifest_url actually needed for anything?

http://codereview.chromium.org/192043/diff/1052/1074
File webkit/appcache/appcache_service.cc (right):

http://codereview.chromium.org/192043/diff/1052/1074#newcode67
Line 67: scoped_refptr<AppCacheGroup> group = GetGroup(manifest_url);
why scoped_refptr here?

http://codereview.chromium.org/192043/diff/1052/1075
File webkit/appcache/appcache_service.h (right):

http://codereview.chromium.org/192043/diff/1052/1075#newcode31
Line 31: virtual void CacheLoadedCallback(AppCache* cache, int64 cache_id) = 0;
Should add comments that cache/group being NULL in this and the next callback
means there was a load error.

[michaeln, 2009-09-15 00:05:04]

New snapshot coming soon...

http://codereview.chromium.org/192043/diff/1052/1066
File webkit/appcache/appcache.h (right):

http://codereview.chromium.org/192043/diff/1052/1066#newcode70
Line 70: // Use AppCacheHost::Associate<<>>Host() to manipulate host
association.
On 2009/09/14 22:22:01, jennb wrote:
> s/<<>>Host/<<>>Cache

Done.

http://codereview.chromium.org/192043/diff/1052/1072
File webkit/appcache/appcache_group.cc (right):

http://codereview.chromium.org/192043/diff/1052/1072#newcode75
Line 75: void AppCacheGroup::StartUpdateWithNewMasterEntry(
On 2009/09/14 22:22:01, jennb wrote:
> I prefer to let the update job decide whether or not it needs to create a new
> cache so that the cache is not created unless determined necessary.

Done.

http://codereview.chromium.org/192043/diff/1052/1055
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/192043/diff/1052/1055#newcode82
Line 82: service_->MarkAsForeignEntry(cache_document_was_loaded_from,
document_url);
On 2009/09/14 22:22:01, jennb wrote:
> Suggest keeping the params in the same order for identically named methods.

Done.

http://codereview.chromium.org/192043/diff/1052/1055#newcode238
Line 238: // update job should have associated a cache prior to returning.
On 2009/09/14 22:22:01, jennb wrote:
> Not sure I agree with this yet. May be better to wait until necessary to
> associate the host with an incomplete cache.

Ok, i'll back some of this out

http://codereview.chromium.org/192043/diff/1052/1055#newcode258
Line 258: frontend_->OnCacheSelected(host_id_, kNoCacheId, GetStatus());
On 2009/09/14 22:22:01, jennb wrote:
> This would produce a non-UNCACHED status with kNoCacheId.  That seems
> contradictory.

Yea... I'm unwinding the potential vs selected distinctions :)

http://codereview.chromium.org/192043/diff/1052/1055#newcode268
Line 268: void AppCacheHost::AssociateCache(AppCache* cache) {
Sounds good.

http://codereview.chromium.org/192043/diff/1052/1055#newcode281
Line 281: group_ = associated_cache_->owning_group();
that sounds right

http://codereview.chromium.org/192043/diff/1052/1056
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/192043/diff/1052/1056#newcode71
Line 71: void AssociatePotentialCache(AppCache* cache);
On 2009/09/14 22:22:01, jennb wrote:
> The potential vs selected cache separation seems to add unnecessary
complexity. 
> The code can easily check if the associated cache is incomplete and drop
> requests to utilize the associated cache.

Done.

http://codereview.chromium.org/192043/diff/1052/1056#newcode116
Line 116: // The complete cache selected for use this host, if any.
On 2009/09/14 22:22:01, jennb wrote:
> typo

Done.

http://codereview.chromium.org/192043/diff/1052/1056#newcode119
Line 119: // The reference to our the group ensures the group exists
On 2009/09/14 22:22:01, jennb wrote:
> typo

Done.

http://codereview.chromium.org/192043/diff/1052/1070
File webkit/appcache/appcache_host_unittest.cc (right):

http://codereview.chromium.org/192043/diff/1052/1070#newcode176
Line 176: // The callback should not occur until we have an until
On 2009/09/14 22:22:01, jennb wrote:
> typo

Done.

http://codereview.chromium.org/192043/diff/1052/1070#newcode209
Line 209: // The callback should not occur until we have an until
On 2009/09/14 22:22:01, jennb wrote:
> typo

Done.

http://codereview.chromium.org/192043/diff/1052/1059
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/192043/diff/1052/1059#newcode34
Line 34: AppCacheRequestHandler::AppCacheRequestHandler(const GURL&
manifest_url,
I'm going to revisit this in a later CL, for now i'm unwinding these ctors.

A handler has to wait until there is a cache selection prior to satisfying
subresource loads. It needs to know what cache to use (if any). I was thinking
of having the handler initiate cache/group loads independently to get notified
of load completion, and then query the host for its associated_cache().

I'm having second thoughts about that strategy. For now i'm going to stub this
out in AppCacheHost and just return NULL when we don't yet have a selection.

http://codereview.chromium.org/192043/diff/1052/1074
File webkit/appcache/appcache_service.cc (right):

http://codereview.chromium.org/192043/diff/1052/1074#newcode67
Line 67: scoped_refptr<AppCacheGroup> group = GetGroup(manifest_url);
On 2009/09/14 22:22:01, jennb wrote:
> why scoped_refptr here?

If client->GroupLoadCallback does not acquire a reference to a newly created
group below. The scope_refptr dtor will cause it to be deleted... so no leak.

http://codereview.chromium.org/192043/diff/1052/1075
File webkit/appcache/appcache_service.h (right):

http://codereview.chromium.org/192043/diff/1052/1075#newcode31
Line 31: virtual void CacheLoadedCallback(AppCache* cache, int64 cache_id) = 0;
On 2009/09/14 22:22:01, jennb wrote:
> Should add comments that cache/group being NULL in this and the next callback
> means there was a load error.

Done.

[michaeln, 2009-09-15 01:05:55]

New snapshot uploaded.

[jennb, 2009-09-15 04:26:08]

LGTM

If you can commit early tomorrow, I'll be able to sync these changes to my CL
before sending out a new snapshot.

Thanks!

http://codereview.chromium.org/192043/diff/11026/1122
File webkit/appcache/appcache_entry.h (right):

http://codereview.chromium.org/192043/diff/11026/1122#newcode8
Line 8: //#include "googleurl/src/gurl.h"
?? Local experiment accidentally included into this CL?

http://codereview.chromium.org/192043/diff/11026/1100
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/192043/diff/11026/1100#newcode166
Line 166: if (associated_cache() && associated_cache()->is_complete())
Can use the member var associated_cache_ directly in these 2 lines?

http://codereview.chromium.org/192043/diff/11026/1100#newcode253
Line 253: if (associated_cache_.get()) {
if (cache) - then you can move the OnCacheSelected call at line 259 into this if
clause.

[jennb, 2009-09-15 04:29:52]

LGTM

A couple more comments.

http://codereview.chromium.org/192043/diff/11026/1121
File webkit/appcache/appcache.cc (right):

http://codereview.chromium.org/192043/diff/11026/1121#newcode21
Line 21: if (cache_id_ != kUnknownCacheId)
Can delete. I'll be using a different ctor to create a cache during update. All
other uses of this ctor should be providing a valid cache_id - can DCHECK for
that if you like.

http://codereview.chromium.org/192043/diff/11026/1121#newcode28
Line 28: if (cache_id_ != kUnknownCacheId)
All appcache instances should have a valid cache_id (see my previous comment).

http://codereview.chromium.org/192043/diff/11026/1121#newcode61
Line 61: DCHECK(is_complete_);
Let's allow incomplete caches to have an id.  That way we can notify a host
about incomplete cache being selected.

[michaeln, 2009-09-15 17:08:09]

In a future CL, i think we should be able to make cache_id_ const. Class
invariants are good... especially this one since its sent over the wire to the
client. Once our id generators are in place, lets revisit.

[michaeln, 2009-09-15 18:36:46]

http://codereview.chromium.org/192043/diff/11026/1122
File webkit/appcache/appcache_entry.h (right):

http://codereview.chromium.org/192043/diff/11026/1122#newcode8
Line 8: //#include "googleurl/src/gurl.h"
On 2009/09/15 04:26:08, jennb wrote:
> ?? Local experiment accidentally included into this CL?

Done.

http://codereview.chromium.org/192043/diff/11026/1100
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/192043/diff/11026/1100#newcode166
Line 166: if (associated_cache() && associated_cache()->is_complete())
On 2009/09/15 04:26:08, jennb wrote:
> Can use the member var associated_cache_ directly in these 2 lines?

As a code reader, i prefer associated_host() to association_host_.get(), as far
as the compiler is concerned these are identical.

http://codereview.chromium.org/192043/diff/11026/1100#newcode253
Line 253: if (associated_cache_.get()) {
On 2009/09/15 04:26:08, jennb wrote:
> if (cache) - then you can move the OnCacheSelected call at line 259 into this
if
> clause.

Done.

http://codereview.chromium.org/192043/diff/8048/9148
File webkit/appcache/appcache.h (right):

http://codereview.chromium.org/192043/diff/8048/9148#newcode73
Line 73: const int64 cache_id_;
I've unwound the AssignId() method and instead have made the cache_id_ const.
Given our strategy, we should be able to either know a real id or to generate a
real id at ctor time. Your ctor to create a new cache given a manifest can
call...
  cache_id_(service->NewCacheId())
... in the initializer list.

[jennb, 2009-09-15 20:32:25]

On Tue, Sep 15, 2009 at 11:36 AM, <michaeln@chromium.org> wrote:

>
>
> http://codereview.chromium.org/192043/diff/11026/1100
> File webkit/appcache/appcache_host.cc (right):
>
> http://codereview.chromium.org/192043/diff/11026/1100#newcode166
> Line 166: if (associated_cache() && associated_cache()->is_complete())
> On 2009/09/15 04:26:08, jennb wrote:
>
>> Can use the member var associated_cache_ directly in these 2 lines?
>>
>
> As a code reader, i prefer associated_host() to association_host_.get(),
> as far as the compiler is concerned these are identical.


I don't think you need to use .get(). Should work with just
association_host_.


>
> http://codereview.chromium.org/192043/diff/8048/9148
> File webkit/appcache/appcache.h (right):
>
> http://codereview.chromium.org/192043/diff/8048/9148#newcode73
> Line 73: const int64 cache_id_;
> I've unwound the AssignId() method and instead have made the cache_id_
> const. Given our strategy, we should be able to either know a real id or
> to generate a real id at ctor time. Your ctor to create a new cache
> given a manifest can call...
>  cache_id_(service->NewCacheId())
> ... in the initializer list.
>
>
That works.

Jenn

> http://codereview.chromium.org/192043
>