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

webkit/appcache/appcache_storage_impl.cc

Index: webkit/appcache/appcache_storage_impl.cc
===================================================================
--- webkit/appcache/appcache_storage_impl.cc	(revision 80669)
+++ webkit/appcache/appcache_storage_impl.cc	(working copy)
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -509,11 +509,12 @@
 
 void AppCacheStorageImpl::StoreGroupAndCacheTask::RunCompleted() {
   if (success_) {
-    // TODO(kkanetkar): Add to creation time when that's enabled.
     storage_->origins_with_groups_.insert(group_->manifest_url().GetOrigin());
     if (cache_ != group_->newest_complete_cache()) {
       cache_->set_complete(true);
       group_->AddCache(cache_);
+      if (group_->creation_time().is_null())

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

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.

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

Done.

+        group_->set_creation_time(group_record_.creation_time);

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

Is this related to the preferred manifest url change?  It looks unrelated...

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

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

     }
     group_->AddNewlyDeletableResponseIds(&newly_deletable_response_ids_);
   }
@@ -537,8 +538,11 @@
 class AppCacheStorageImpl::FindMainResponseTask : public DatabaseTask {
  public:
   FindMainResponseTask(AppCacheStorageImpl* storage, const GURL& url,

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

Nit: while you're here, may as well follow that same style rule.

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

Done.

+                       const GURL& preferred_manifest_url,
                        const AppCacheWorkingSet::GroupMap* groups_in_use)
-      : DatabaseTask(storage), url_(url), cache_id_(kNoCacheId) {
+      : DatabaseTask(storage), url_(url),
+        preferred_manifest_url_(preferred_manifest_url),
+        cache_id_(kNoCacheId) {
     if (groups_in_use) {
       for (AppCacheWorkingSet::GroupMap::const_iterator it =
                groups_in_use->begin();
@@ -556,6 +560,7 @@
   virtual void RunCompleted();
 
   GURL url_;
+  GURL preferred_manifest_url_;
   std::set<int64> cache_ids_in_use_;
   AppCacheEntry entry_;
   AppCacheEntry fallback_entry_;
@@ -566,6 +571,31 @@
 
 // Helpers for FindMainResponseTask::Run()
 namespace {
+struct SortByCacheId

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

"struct" here isn't helping much, since you have private stuff anyway: maybe
clearer to just use class with a "public:" section?

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

Done.

+    : public std::binary_function<
+        AppCacheDatabase::EntryRecord,
+        AppCacheDatabase::EntryRecord,
+        bool> {
+  SortByCacheId(int64 preferred_id, const std::set<int64>* in_use_ids)

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

Minor point, but why not just a const ref?  Instances of this class never last
more than a single expression anyway...

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

Done.

+      : preferred_id_(preferred_id), in_use_ids_(in_use_ids) {
+  }
+  bool operator()(
+      const AppCacheDatabase::EntryRecord& lhs,
+      const AppCacheDatabase::EntryRecord& rhs) {
+    return compute_value(lhs) > compute_value(rhs);
+  }
+ private:
+  int compute_value(const AppCacheDatabase::EntryRecord& entry) {
+    if (entry.cache_id == preferred_id_)
+      return 100;
+    else if (in_use_ids_->find(entry.cache_id) != in_use_ids_->end())

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

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.)

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

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).

+      return 50;
+    return 0;
+  }
+  int64 preferred_id_;
+  const std::set<int64>* in_use_ids_;
+};
+
 bool SortByLength(
     const AppCacheDatabase::FallbackNameSpaceRecord& lhs,
     const AppCacheDatabase::FallbackNameSpaceRecord& rhs) {
@@ -609,99 +639,138 @@
   WhiteListMap namespaces_map_;
   AppCacheDatabase* database_;
 };
+
+bool IsValidForMainResponse(const AppCacheDatabase::EntryRecord& entry_record,

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

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?

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

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.

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

Done.

+                            AppCacheDatabase* database,
+                            GURL* manifest_url_out) {
+  if (entry_record.flags & AppCacheEntry::FOREIGN)
+    return false;
+  AppCacheDatabase::GroupRecord group_record;
+  if (!database->FindGroupForCache(entry_record.cache_id, &group_record)) {
+    NOTREACHED() << "A cache without a group is not expected.";
+    return false;
+  }
+  *manifest_url_out = group_record.manifest_url;
+  return true;
+}
+
 }  // namespace
 
 void AppCacheStorageImpl::FindMainResponseTask::Run() {
-  // We have a bias for hits from caches that are in use.
+  // NOTE: The heuristics around choosing amoungst multiple candidates
+  // is under specified, and just plain not fully understood. This needs

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

Grammar nit: s/under specified/underspecified/

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

Done.

+  // to be refined.
 
-  // TODO(michaeln): The heuristics around choosing amoungst
-  // multiple candidates is under specified, and just plain
-  // not fully understood. Refine these over time. In particular,
-  // * prefer candidates from newer caches
-  // * take into account the cache associated with the document
-  //   that initiated the navigation
-  // * take into account the cache associated with the document
-  //   currently residing in the frame being navigated
+  // The 'preferred_manifest_url' is the url of the manifest associated
+  // with the page that opened or embedded the page being loaded now.
+  // We have a strong preference to use resources from that cache.
+  // We also have a lesser bias to use resources from caches that are currently
+  // being used by other unrelated pages.
+  // TODO(michaeln): come up with a 'preferred_manifest_url' in more cases
+  // - when navigating a frame whose current contents are from an appcache
+  // - when clicking an href in a frame that is appcached
+  int64 preferred_cache_id = kNoCacheId;
+  if (!preferred_manifest_url_.is_empty()) {
+    AppCacheDatabase::GroupRecord preferred_group;
+    AppCacheDatabase::CacheRecord preferred_cache;
+    if (database_->FindGroupForManifestUrl(
+            preferred_manifest_url_, &preferred_group) &&
+        database_->FindCacheForGroup(
+            preferred_group.group_id, &preferred_cache)) {
+      preferred_cache_id = preferred_cache.cache_id;
+    }
+  }
 
-  // First look for an exact match. We don't worry about whether
-  // the containing cache is in-use in this loop because the
-  // storage class's FindResponseForMainRequest method does that
-  // as a pre-optimization.
+  // First look for an exact match.
   std::vector<AppCacheDatabase::EntryRecord> entries;

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

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. 

   if (database_->FindEntriesForUrl(url_, &entries) && !entries.empty()) {
+    // Sort them in order of preference, from the preferred_cache first,
+    // followed by hits from caches that are 'in use', then the rest.
+    std::sort(entries.begin(), entries.end(),
+              SortByCacheId(preferred_cache_id, &cache_ids_in_use_));

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

SortByCacheId makes me think the IDs are being sorted.  As we're sorting caches
by preference, perhaps rename to SortByCachePreference?

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

Done.

+
+    // Take the first with a valid entry.
     std::vector<AppCacheDatabase::EntryRecord>::iterator iter;
     for (iter = entries.begin(); iter < entries.end(); ++iter) {
-      if (iter->flags & AppCacheEntry::FOREIGN)
+      if (!IsValidForMainResponse(*iter, database_, &manifest_url_))
         continue;
-
-      AppCacheDatabase::GroupRecord group_record;
-      if (!database_->FindGroupForCache(iter->cache_id, &group_record)) {
-        NOTREACHED() << "A cache without a group is not expected.";
-        continue;
-      }
       entry_ = AppCacheEntry(iter->flags, iter->response_id);
       cache_id_ = iter->cache_id;
-      manifest_url_ = group_record.manifest_url;
-      return;
+      return;  // We found an exact match.
     }
   }
 
   // No exact matches, look at the fallback namespaces for this origin.
-  std::vector<AppCacheDatabase::FallbackNameSpaceRecord> fallbacks;
-  if (!database_->FindFallbackNameSpacesForOrigin(url_.GetOrigin(), &fallbacks)
-      || fallbacks.empty()) {
+  std::vector<AppCacheDatabase::FallbackNameSpaceRecord> all_fallbacks;
+  if (!database_->FindFallbackNameSpacesForOrigin(
+          url_.GetOrigin(), &all_fallbacks)
+      || all_fallbacks.empty()) {
     return;
   }
 
-  // Sort by namespace url string length, longest to shortest,
-  // since longer matches trump when matching a url to a namespace.
-  std::sort(fallbacks.begin(), fallbacks.end(), SortByLength);
+  // Sort them by length, longer matches within the same cache/bucket take
+  // precedence.
+  std::sort(all_fallbacks.begin(), all_fallbacks.end(), SortByLength);
 
-  bool has_candidate = false;
-  GURL candidate_fallback_namespace;
+  // Filter the list and bin them into buckets.
+  NetworkNamespaceHelper network_namespace_helper(database_);
+  std::vector<AppCacheDatabase::FallbackNameSpaceRecord*> preferred_fallbacks;

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

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.

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

Done.

+  std::vector<AppCacheDatabase::FallbackNameSpaceRecord*> inuse_fallbacks;
+  std::vector<AppCacheDatabase::FallbackNameSpaceRecord*> other_fallbacks;
   std::vector<AppCacheDatabase::FallbackNameSpaceRecord>::iterator iter;
-  NetworkNamespaceHelper network_namespace_helper(database_);
-  for (iter = fallbacks.begin(); iter < fallbacks.end(); ++iter) {
-    // Skip this fallback namespace if the requested url falls into a network
-    // namespace of the containing appcache.
+  for (iter = all_fallbacks.begin(); iter < all_fallbacks.end(); ++iter) {
+    // Skip those that aren't a prefix match.
+    if (!StartsWithASCII(url_.spec(), iter->namespace_url.spec(), true))
+      continue;
+
+    // Skip fallback namespaces where the requested url falls into a network
+    // namespace of its containing appcache.
     if (network_namespace_helper.IsInNetworkNamespace(url_, iter->cache_id))
       continue;
 
-    if (has_candidate &&
-        (candidate_fallback_namespace.spec().length() >
-         iter->namespace_url.spec().length())) {
-      break;  // Stop iterating since longer namespace prefix matches win.
-    }
+    // Bin them into one of our three buckets.
+    if (iter->cache_id == preferred_cache_id)
+      preferred_fallbacks.push_back(&(*iter));
+    else if (cache_ids_in_use_.find(iter->cache_id) != cache_ids_in_use_.end())
+      inuse_fallbacks.push_back(&(*iter));
+    else
+      other_fallbacks.push_back(&(*iter));
+  }
 
-    if (StartsWithASCII(url_.spec(), iter->namespace_url.spec(), true)) {
-      bool is_cache_in_use = cache_ids_in_use_.find(iter->cache_id) !=
-                             cache_ids_in_use_.end();
+  // Assemble into a single ordered list.

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

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.

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

Done.

+  std::vector<AppCacheDatabase::FallbackNameSpaceRecord*> ordered_fallbacks;
+  if (!preferred_fallbacks.empty())
+    ordered_fallbacks.insert(ordered_fallbacks.end(),
+                             preferred_fallbacks.begin(),
+                             preferred_fallbacks.end());
+  if (!inuse_fallbacks.empty())
+    ordered_fallbacks.insert(ordered_fallbacks.end(),
+                             inuse_fallbacks.begin(),
+                             inuse_fallbacks.end());
+  if (!other_fallbacks.empty())
+    ordered_fallbacks.insert(ordered_fallbacks.end(),
+                             other_fallbacks.begin(),
+                             other_fallbacks.end());
 
-      bool take_new_candidate = !has_candidate || is_cache_in_use;
-
-      AppCacheDatabase::EntryRecord entry_record;
-      if (take_new_candidate &&
-          database_->FindEntry(iter->cache_id, iter->fallback_entry_url,
-                               &entry_record)) {
-        if (entry_record.flags & AppCacheEntry::FOREIGN)
-          continue;
-        AppCacheDatabase::GroupRecord group_record;
-        if (!database_->FindGroupForCache(iter->cache_id, &group_record)) {
-          NOTREACHED() << "A cache without a group is not expected.";
-          continue;
-        }
-        cache_id_ = iter->cache_id;
-        fallback_url_ = iter->fallback_entry_url;
-        manifest_url_ = group_record.manifest_url;
-        fallback_entry_ = AppCacheEntry(
-            entry_record.flags, entry_record.response_id);
-        if (is_cache_in_use)
-          break;  // Stop iterating since we favor hits from in-use caches.
-        candidate_fallback_namespace = iter->namespace_url;
-        has_candidate = true;
-      }
+  // Take the first with a valid entry.
+  std::vector<AppCacheDatabase::FallbackNameSpaceRecord*>::const_iterator iter2;

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

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.

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

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

+  for (iter2 = ordered_fallbacks.begin(); iter2 < ordered_fallbacks.end();
+       ++iter2) {
+    AppCacheDatabase::EntryRecord entry_record;
+    if (database_->FindEntry((*iter2)->cache_id, (*iter2)->fallback_entry_url,
+                             &entry_record)) {
+      if (!IsValidForMainResponse(entry_record, database_, &manifest_url_))
+        continue;
+      cache_id_ = (*iter2)->cache_id;
+      fallback_url_ = (*iter2)->fallback_entry_url;
+      fallback_entry_ = AppCacheEntry(
+          entry_record.flags, entry_record.response_id);
+      return;  // We found a matching fallback entry.
     }
   }
+
+  // We didn't find antything.

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

s/antything/anything/

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

Done.

+  DCHECK(cache_id_ == kNoCacheId && manifest_url_.is_empty());
 }
 
 void AppCacheStorageImpl::FindMainResponseTask::RunCompleted() {
@@ -1038,7 +1107,8 @@
 }
 
 void AppCacheStorageImpl::FindResponseForMainRequest(
-    const GURL& url, Delegate* delegate) {
+    const GURL& url, const GURL& preferred_manifest_url,
+    Delegate* delegate) {
   DCHECK(delegate);
 
   const GURL* url_ptr = &url;
@@ -1057,22 +1127,23 @@
   const AppCacheWorkingSet::GroupMap* groups_in_use =
       working_set()->GetGroupsInOrigin(origin);
   if (groups_in_use) {
-    for (AppCacheWorkingSet::GroupMap::const_iterator it =
-            groups_in_use->begin();
-         it != groups_in_use->end(); ++it) {
-      AppCacheGroup* group = it->second;
-      AppCache* cache = group->newest_complete_cache();
-      if (group->is_obsolete() || !cache)
-        continue;
-
-      AppCacheEntry* entry = cache->GetEntry(*url_ptr);
-      if (entry && !entry->IsForeign()) {
-        ScheduleSimpleTask(method_factory_.NewRunnableMethod(
-            &AppCacheStorageImpl::DeliverShortCircuitedFindMainResponse,
-            url, *entry, make_scoped_refptr(group), make_scoped_refptr(cache),
-            make_scoped_refptr(GetOrCreateDelegateReference(delegate))));
-        return;
+    if (!preferred_manifest_url.is_empty()) {
+      AppCacheWorkingSet::GroupMap::const_iterator found =
+          groups_in_use->find(preferred_manifest_url);
+      if (found != groups_in_use->end() &&
+          FindResponseForMainRequestInGroup(
+              found->second, *url_ptr, delegate)) {
+          return;

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

Nit: indentation

       }
+    } else {
+      for (AppCacheWorkingSet::GroupMap::const_iterator it =
+              groups_in_use->begin();
+           it != groups_in_use->end(); ++it) {
+        if (FindResponseForMainRequestInGroup(
+                it->second, *url_ptr, delegate)) {
+          return;
+        }
+      }
     }
   }
 
@@ -1091,11 +1162,29 @@
 
   // We have to query the database, schedule a database task to do so.
   scoped_refptr<FindMainResponseTask> task(
-      new FindMainResponseTask(this, *url_ptr, groups_in_use));
+      new FindMainResponseTask(this, *url_ptr, preferred_manifest_url,
+                               groups_in_use));
   task->AddDelegate(GetOrCreateDelegateReference(delegate));
   task->Schedule();
 }
 
+bool AppCacheStorageImpl::FindResponseForMainRequestInGroup(
+    AppCacheGroup* group,  const GURL& url, Delegate* delegate) {

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

extra space before const

+  AppCache* cache = group->newest_complete_cache();
+  if (group->is_obsolete() || !cache)
+    return false;
+
+  AppCacheEntry* entry = cache->GetEntry(url);
+  if (!entry || entry->IsForeign())
+    return false;
+
+  ScheduleSimpleTask(method_factory_.NewRunnableMethod(
+      &AppCacheStorageImpl::DeliverShortCircuitedFindMainResponse,
+      url, *entry, make_scoped_refptr(group), make_scoped_refptr(cache),
+      make_scoped_refptr(GetOrCreateDelegateReference(delegate))));
+  return true;
+}
+
 void AppCacheStorageImpl::DeliverShortCircuitedFindMainResponse(
     const GURL& url, AppCacheEntry found_entry,
     scoped_refptr<AppCacheGroup> group, scoped_refptr<AppCache> cache,