Select a more appropiate appcache based on the opener or the parent of the new document.

Select a more appropiate appcache based on the opener or the parent or the target frame of the new document. The change determines a 'preferred manifest url' in these cases:

* <iframe> loading, we prefer the parent frame's manifest url
* window.open() loading, we prefer the opener frame's manifest url
* href clicking to navigate a frame in place, we prefer the manifest url of the document in the frame when the navigation starts

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

[michaeln, 2011-04-11 15:49:44]

hi again, this is a follow up the the earlier appcache CL (plumbing) that you
looked at. in this CL the newly available numbers are put to use. please take a
look

[adamk, 2011-04-11 20:45:16]

Mostly style stuff, I'll appreciate jennb's comments on the meat of the change
itself (though your rules sound reasonable to me).

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.cc:242: AppCacheHost*
AppCacheHost::GetSpawningHost() {
I don't see a test for this method, can you write one?

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.cc:355:
DCHECK(cache->owning_group()->manifest_url() == preferred_manifest_url_);
Nit: DCHECK_EQ will give better error messages if this fails to hold.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.cc:376: DCHECK(group->manifest_url() ==
preferred_manifest_url_);
DCHECK_EQ

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.h:78: AppCacheHost* GetSpawningHost();
Can this be a const AppChacheHost*?  I think you should have a comment for this
method noting under what conditions it returns NULL.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.h:79: const GURL& preferred_manifest_url() {
return preferred_manifest_url_; }
You can make this a const method.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage.h (right):

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage.h:117: const GURL& url, const GURL&
preferred_manifest_url,
I believe the style guide asks that if you need to split lines, each argument
have its own line.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:517:
group_->set_creation_time(group_record_.creation_time);
Is this related to the preferred manifest url change?  It looks unrelated...

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:540:
FindMainResponseTask(AppCacheStorageImpl* storage, const GURL& url,
Nit: while you're here, may as well follow that same style rule.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:574: struct SortByCacheId
"struct" here isn't helping much, since you have private stuff anyway: maybe
clearer to just use class with a "public:" section?

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:579: SortByCacheId(int64 preferred_id,
const std::set<int64>* in_use_ids)
Minor point, but why not just a const ref?  Instances of this class never last
more than a single expression anyway...

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:643: bool IsValidForMainResponse(const
AppCacheDatabase::EntryRecord& entry_record,
I'd name this slightly differently, since it's not clear from the name that it
sets manifest_url: how about ValidateForMainResponse()?  Other naming
suggestions welcome, I'd probably have a better name in mind if I knew this
domain better. Would FindManifestUrlInDatabase make any sense?

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:661: // is under specified, and just
plain not fully understood. This needs
Grammar nit: s/under specified/underspecified/

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:717:
std::vector<AppCacheDatabase::FallbackNameSpaceRecord*> preferred_fallbacks;
I think this would actually be easier to read if you didn't have to repeat the
type three times, and just did "preferred_fallbacks, inuse_fallbacks,
other_fallbacks" as part of the same statement.  Alternatively, a typedef of
this vector type could make the individual lines easier to read.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:756:
std::vector<AppCacheDatabase::FallbackNameSpaceRecord*>::const_iterator iter2;
The fact that this is called "iter2" suggests to me that a helper function might
be in order...this is a lot of code to cram into one function.

My suggestion would be to split the bucketing into a helper: input is
all_fallbacks, output is ordered_fallbacks.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:772: // We didn't find antything.
s/antything/anything/

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:1136: return;
Nit: indentation

[jennb, 2011-04-12 00:07:56]

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.h:76: //  that created this host is used to break
ties.
indentation

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.h:162: // Information about the host that created
this one, the manifest
nit: replace comma with semicolon

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_re...
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_re...
webkit/appcache/appcache_request_handler.cc:193: AppCacheHost* spawning_host =
ResourceType::IsSharedWorker(resource_type_) ?
How does SharedWorker affect which host to use as spawning_host?  I'm not
familiar with the significance.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:516: if
(group_->creation_time().is_null())
Why only set the group's creation time if cache is not the
newest_complete_cache?  As long as group exists, we should try to ensure it's
creation time is set.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:591: else if
(in_use_ids_->find(entry.cache_id) != in_use_ids_->end())
It would be nice if in_use_ids was sorted by LRU so that we can give more weight
to the more recently used id.  (Not critical.)

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:643: bool IsValidForMainResponse(const
AppCacheDatabase::EntryRecord& entry_record,
On 2011/04/11 20:45:16, adamk wrote:
> I'd name this slightly differently, since it's not clear from the name that it
> sets manifest_url: how about ValidateForMainResponse()?  Other naming
> suggestions welcome, I'd probably have a better name in mind if I knew this
> domain better. Would FindManifestUrlInDatabase make any sense?

This could be 2 functions as it's doing 2 completely unrelated tasks: 1)
checking if the entry can be used for a main response and 2) extracting the
manifest_url from the entry that gets used.  Alternatively, I'm trying to come
up with a single function name that encompasses both of these concepts, but
nothing sounds good.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:685:
std::vector<AppCacheDatabase::EntryRecord> entries;
This routine is rather long.  You could break out the code into 2 subroutines:
one for searching in the regular entries, and one for searching fallback
namespaces.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:690: SortByCacheId(preferred_cache_id,
&cache_ids_in_use_));
SortByCacheId makes me think the IDs are being sorted.  As we're sorting caches
by preference, perhaps rename to SortByCachePreference?

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:740: // Assemble into a single ordered
list.
Instead of building one single list out of the three buckets, you could also
make the find-first-valid-entry for-loop a helper function and call it 3 times,
once for each bucket-list until you find a valid fallback entry.  Might make
this easier to read.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:1172: AppCacheGroup* group,  const
GURL& url, Delegate* delegate) {
extra space before const

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.h (right):

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.h:104: // Sometimes we can respond without
having to query the database.
This comment got separated from its function below.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl_unittest.cc (right):

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:875: // the working set to
similulate appearing as "in use".
simulate

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:878:
MakeMultipleHitCacheAndGroup(kManifestUrl3, 3);
What logic ensures that only this one remains in the working set?

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:890: // Add and entry for
kEntryUrl
Add an entry

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:900: // Add and entry for the
manifestUrl
Add an entry

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:939: // Conduct the another
test perferring kManifestUrl
Conduct another test

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:1002: void
Verify_FindMainResponseWithMultipleHits5() {
Do we have test coverage for these cases:  1) multiple in-use explicit entries
and 2) multiple in-use fallback namespaces?

[michaeln1, 2011-04-12 00:29:29]

Thnx for looking, i'll take a stab at breaking FindMainResponseTask::Run
into smaller pieces (thnx for the suggestions about how to slice it up) and
let you know when i've got a new CL to look at.

On Mon, Apr 11, 2011 at 5:07 PM, <jennb@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
> File webkit/appcache/appcache_host.h (right):
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
> webkit/appcache/appcache_host.h:76: //  that created this host is used
> to break ties.
> indentation
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
> webkit/appcache/appcache_host.h:162: // Information about the host that
> created this one, the manifest
> nit: replace comma with semicolon
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_re...
> File webkit/appcache/appcache_request_handler.cc (right):
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_re...
> webkit/appcache/appcache_request_handler.cc:193: AppCacheHost*
> spawning_host = ResourceType::IsSharedWorker(resource_type_) ?
> How does SharedWorker affect which host to use as spawning_host?  I'm
> not familiar with the significance.
>
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
> File webkit/appcache/appcache_storage_impl.cc (right):
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
> webkit/appcache/appcache_storage_impl.cc:516: if
> (group_->creation_time().is_null())
> Why only set the group's creation time if cache is not the
> newest_complete_cache?  As long as group exists, we should try to ensure
> it's creation time is set.
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
> webkit/appcache/appcache_storage_impl.cc:591: else if
> (in_use_ids_->find(entry.cache_id) != in_use_ids_->end())
> It would be nice if in_use_ids was sorted by LRU so that we can give
> more weight to the more recently used id.  (Not critical.)
>
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
> webkit/appcache/appcache_storage_impl.cc:643: bool
> IsValidForMainResponse(const AppCacheDatabase::EntryRecord&
> entry_record,
> On 2011/04/11 20:45:16, adamk wrote:
>
>> I'd name this slightly differently, since it's not clear from the name
>>
> that it
>
>> sets manifest_url: how about ValidateForMainResponse()?  Other naming
>> suggestions welcome, I'd probably have a better name in mind if I knew
>>
> this
>
>> domain better. Would FindManifestUrlInDatabase make any sense?
>>
>
> This could be 2 functions as it's doing 2 completely unrelated tasks: 1)
> checking if the entry can be used for a main response and 2) extracting
> the manifest_url from the entry that gets used.  Alternatively, I'm
> trying to come up with a single function name that encompasses both of
> these concepts, but nothing sounds good.
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
> webkit/appcache/appcache_storage_impl.cc:685:
> std::vector<AppCacheDatabase::EntryRecord> entries;
> This routine is rather long.  You could break out the code into 2
> subroutines: one for searching in the regular entries, and one for
> searching fallback namespaces.
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
> webkit/appcache/appcache_storage_impl.cc:690:
> SortByCacheId(preferred_cache_id, &cache_ids_in_use_));
> SortByCacheId makes me think the IDs are being sorted.  As we're sorting
> caches by preference, perhaps rename to SortByCachePreference?
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
> webkit/appcache/appcache_storage_impl.cc:740: // Assemble into a single
> ordered list.
> Instead of building one single list out of the three buckets, you could
> also make the find-first-valid-entry for-loop a helper function and call
> it 3 times, once for each bucket-list until you find a valid fallback
> entry.  Might make this easier to read.
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
> webkit/appcache/appcache_storage_impl.cc:1172: AppCacheGroup* group,
> const GURL& url, Delegate* delegate) {
> extra space before const
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
> File webkit/appcache/appcache_storage_impl.h (right):
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
> webkit/appcache/appcache_storage_impl.h:104: // Sometimes we can respond
> without having to query the database.
> This comment got separated from its function below.
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
> File webkit/appcache/appcache_storage_impl_unittest.cc (right):
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
> webkit/appcache/appcache_storage_impl_unittest.cc:875: // the working
> set to similulate appearing as "in use".
> simulate
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
> webkit/appcache/appcache_storage_impl_unittest.cc:878:
> MakeMultipleHitCacheAndGroup(kManifestUrl3, 3);
> What logic ensures that only this one remains in the working set?
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
> webkit/appcache/appcache_storage_impl_unittest.cc:890: // Add and entry
> for kEntryUrl
> Add an entry
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
> webkit/appcache/appcache_storage_impl_unittest.cc:900: // Add and entry
> for the manifestUrl
> Add an entry
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
> webkit/appcache/appcache_storage_impl_unittest.cc:939: // Conduct the
> another test perferring kManifestUrl
> Conduct another test
>
>
>
http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
> webkit/appcache/appcache_storage_impl_unittest.cc:1002: void
> Verify_FindMainResponseWithMultipleHits5() {
> Do we have test coverage for these cases:  1) multiple in-use explicit
> entries and 2) multiple in-use fallback namespaces?
>
>
> http://codereview.chromium.org/6727006/
>

[michaeln, 2011-04-14 23:41:23]

I still haven't split FindMainResponseTask::Run() into bite sized chunks, but i
think i've addressed the other comments in some way.

I'll upload a new snapshot with additional mods to FindMainResponseTask::Run()
soon'ish.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.cc:355:
DCHECK(cache->owning_group()->manifest_url() == preferred_manifest_url_);
On 2011/04/11 20:45:16, adamk wrote:
> Nit: DCHECK_EQ will give better error messages if this fails to hold.

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.cc:376: DCHECK(group->manifest_url() ==
preferred_manifest_url_);
On 2011/04/11 20:45:16, adamk wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.h:76: //  that created this host is used to break
ties.
On 2011/04/12 00:07:56, jennb wrote:
> indentation

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.h:79: const GURL& preferred_manifest_url() {
return preferred_manifest_url_; }
On 2011/04/11 20:45:16, adamk wrote:
> You can make this a const method.

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.h:162: // Information about the host that created
this one, the manifest
On 2011/04/12 00:07:56, jennb wrote:
> nit: replace comma with semicolon

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_re...
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_re...
webkit/appcache/appcache_request_handler.cc:193: AppCacheHost* spawning_host =
ResourceType::IsSharedWorker(resource_type_) ?
On 2011/04/12 00:07:56, jennb wrote:
> How does SharedWorker affect which host to use as spawning_host?  I'm not
> familiar with the significance.

In the current system, the main resource of a shared worker is loaded by the
context (and its appcacheHost) that first creates that worker. This is very
unlike other 'main-resources' (pages) which are the first resource loaded by the
empty context which the page will populate.

So in the SharedWorker case, the spawning host is the page that's creating the
worker == the page that loading the main resource for the shared worker = host_.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage.h (right):

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage.h:117: const GURL& url, const GURL&
preferred_manifest_url,
On 2011/04/11 20:45:16, adamk wrote:
> I believe the style guide asks that if you need to split lines, each argument
> have its own line.

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:516: if
(group_->creation_time().is_null())
On 2011/04/12 00:07:56, jennb wrote:
> Why only set the group's creation time if cache is not the
> newest_complete_cache?  As long as group exists, we should try to ensure it's
> creation time is set.

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:517:
group_->set_creation_time(group_record_.creation_time);
On 2011/04/11 20:45:16, adamk wrote:
> Is this related to the preferred manifest url change?  It looks unrelated...

This is unrelated, i just noticed the very minor TODO in such close proxity and
decided to take care of it.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:540:
FindMainResponseTask(AppCacheStorageImpl* storage, const GURL& url,
On 2011/04/11 20:45:16, adamk wrote:
> Nit: while you're here, may as well follow that same style rule.

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:574: struct SortByCacheId
On 2011/04/11 20:45:16, adamk wrote:
> "struct" here isn't helping much, since you have private stuff anyway: maybe
> clearer to just use class with a "public:" section?

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:579: SortByCacheId(int64 preferred_id,
const std::set<int64>* in_use_ids)
On 2011/04/11 20:45:16, adamk wrote:
> Minor point, but why not just a const ref?  Instances of this class never last
> more than a single expression anyway...

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:591: else if
(in_use_ids_->find(entry.cache_id) != in_use_ids_->end())
On 2011/04/12 00:07:56, jennb wrote:
> It would be nice if in_use_ids was sorted by LRU so that we can give more
weight
> to the more recently used id.  (Not critical.)

I'm sure the logic to disambiguate between multiple hits can be refined more,
but I'd like to leave more along those lines for later CLs. 

Maybe when it comes to LRU'ness, that heuristic could be more useful to apply to
caches that are not in use? If it's in use, it's been very recently used (a page
is open on it now).

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:661: // is under specified, and just
plain not fully understood. This needs
On 2011/04/11 20:45:16, adamk wrote:
> Grammar nit: s/under specified/underspecified/

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:690: SortByCacheId(preferred_cache_id,
&cache_ids_in_use_));
On 2011/04/12 00:07:56, jennb wrote:
> SortByCacheId makes me think the IDs are being sorted.  As we're sorting
caches
> by preference, perhaps rename to SortByCachePreference?

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:772: // We didn't find antything.
On 2011/04/11 20:45:16, adamk wrote:
> s/antything/anything/

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.h (right):

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.h:104: // Sometimes we can respond without
having to query the database.
On 2011/04/12 00:07:56, jennb wrote:
> This comment got separated from its function below.

This was intentional since both methods are involved in circumventing the db
query.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl_unittest.cc (right):

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:875: // the working set to
similulate appearing as "in use".
On 2011/04/12 00:07:56, jennb wrote:
> simulate

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:878:
MakeMultipleHitCacheAndGroup(kManifestUrl3, 3);
On 2011/04/12 00:07:56, jennb wrote:
> What logic ensures that only this one remains in the working set?

The helper methods stores a ref to the cache_ and group_ in a data member. Only
the last created has that ref applied since each successive call to the method
overwrites the previous methods results in those data members, So the only one
left in memory (ala in the working set) is the last one.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:890: // Add and entry for
kEntryUrl
On 2011/04/12 00:07:56, jennb wrote:
> Add an entry

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:900: // Add and entry for the
manifestUrl
On 2011/04/12 00:07:56, jennb wrote:
> Add an entry

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:939: // Conduct the another
test perferring kManifestUrl
On 2011/04/12 00:07:56, jennb wrote:
> Conduct another test

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:1002: void
Verify_FindMainResponseWithMultipleHits5() {
On 2011/04/12 00:07:56, jennb wrote:
> Do we have test coverage for these cases:  1) multiple in-use explicit entries
> and 2) multiple in-use fallback namespaces?

No, i've just got one entry per bucket. Do you think the additional coverage is
needed?

[michaeln, 2011-04-15 02:07:00]

PTAL

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:643: bool IsValidForMainResponse(const
AppCacheDatabase::EntryRecord& entry_record,
On 2011/04/11 20:45:16, adamk wrote:
> I'd name this slightly differently, since it's not clear from the name that it
> sets manifest_url: how about ValidateForMainResponse()?  Other naming
> suggestions welcome, I'd probably have a better name in mind if I knew this
> domain better. Would FindManifestUrlInDatabase make any sense?

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:717:
std::vector<AppCacheDatabase::FallbackNameSpaceRecord*> preferred_fallbacks;
On 2011/04/11 20:45:16, adamk wrote:
> I think this would actually be easier to read if you didn't have to repeat the
> type three times, and just did "preferred_fallbacks, inuse_fallbacks,
> other_fallbacks" as part of the same statement.  Alternatively, a typedef of
> this vector type could make the individual lines easier to read.

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:740: // Assemble into a single ordered
list.
On 2011/04/12 00:07:56, jennb wrote:
> Instead of building one single list out of the three buckets, you could also
> make the find-first-valid-entry for-loop a helper function and call it 3
times,
> once for each bucket-list until you find a valid fallback entry.  Might make
> this easier to read.

Done.

http://codereview.chromium.org/6727006/diff/38027/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:756:
std::vector<AppCacheDatabase::FallbackNameSpaceRecord*>::const_iterator iter2;
On 2011/04/11 20:45:16, adamk wrote:
> The fact that this is called "iter2" suggests to me that a helper function
might
> be in order...this is a lot of code to cram into one function.
> 
> My suggestion would be to split the bucketing into a helper: input is
> all_fallbacks, output is ordered_fallbacks.

Done, got rid of iter2 but did the split in a different way.

[adamk, 2011-04-15 17:20:51]

lgtm, with a request for an additional comment.

http://codereview.chromium.org/6727006/diff/47006/webkit/appcache/appcache_ho...
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/6727006/diff/47006/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.h:78: const AppCacheHost* GetSpawningHost() const;
Please add a comment to this method explaining under what conditions it might
return NULL.

[michaeln, 2011-04-15 19:33:34]

http://codereview.chromium.org/6727006/diff/47006/webkit/appcache/appcache_ho...
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/6727006/diff/47006/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.h:78: const AppCacheHost* GetSpawningHost() const;
On 2011/04/15 17:20:51, adamk wrote:
> Please add a comment to this method explaining under what conditions it might
> return NULL.

Done.

  // Called prior to the main resource load. When the system contains multiple
  // candidates for a main resource load, the appcache preferred by the host
  // that created this host is used to break ties.
  void SetSpawningHostId(int spawning_process_id, int spawning_host_id);

  // May return NULL if the spawning host context has been closed, or if a
  // spawning host context was never identified.
  const AppCacheHost* GetSpawningHost() const;

[jennb, 2011-04-15 20:16:06]

LGTM