Queue appcache update if current update process is terminating.

TEST=update and group unittests added
BUG=none

Review URL: http://codereview.chromium.org/465132

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@34299 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 256 insertions, 3 deletions

diff --git a/webkit/appcache/appcache_group.cc b/webkit/appcache/appcache_group.cc
index 49c8af3..a9ad808 100644
--- a/webkit/appcache/appcache_group.cc
+++ b/webkit/appcache/appcache_group.cc
@@ -7,6 +7,7 @@
 #include <algorithm>
 
 #include "base/logging.h"
+#include "base/message_loop.h"
 #include "webkit/appcache/appcache.h"
 #include "webkit/appcache/appcache_host.h"
 #include "webkit/appcache/appcache_service.h"
@@ -15,6 +16,24 @@
 
 namespace appcache {
 
+class AppCacheGroup;
+
+// Use this helper class because we cannot make AppCacheGroup a derived class
+// of AppCacheHost::Observer as it would create a circular dependency between
+// AppCacheHost and AppCacheGroup.
+class AppCacheGroup::HostObserver : public AppCacheHost::Observer {
+ public:
+  explicit HostObserver(AppCacheGroup* group) : group_(group) {}
+
+  // Methods for AppCacheHost::Observer.
+  void OnCacheSelectionComplete(AppCacheHost* host) {}  // N/A
+  void OnDestructionImminent(AppCacheHost* host) {
+    group_->HostDestructionImminent(host);
+  }
+ private:
+  AppCacheGroup* group_;
+};
+
 AppCacheGroup::AppCacheGroup(AppCacheService* service,
                              const GURL& manifest_url,
                              int64 group_id)
@@ -24,13 +43,17 @@ AppCacheGroup::AppCacheGroup(AppCacheService* service,
       is_obsolete_(false),
       newest_complete_cache_(NULL),
       update_job_(NULL),
-      service_(service) {
+      service_(service),
+      restart_update_task_(NULL) {
   service_->storage()->working_set()->AddGroup(this);
+  host_observer_.reset(new HostObserver(this));
 }
 
 AppCacheGroup::~AppCacheGroup() {
   DCHECK(old_caches_.empty());
   DCHECK(!newest_complete_cache_);
+  DCHECK(!restart_update_task_);
+  DCHECK(queued_updates_.empty());
 
   if (update_job_)
     delete update_job_;
@@ -40,11 +63,18 @@ AppCacheGroup::~AppCacheGroup() {
 }
 
 void AppCacheGroup::AddUpdateObserver(UpdateObserver* observer) {
-  observers_.AddObserver(observer);
+  // If observer being added is a host that has been queued for later update,
+  // add observer to a different observer list.
+  AppCacheHost* host = static_cast<AppCacheHost*>(observer);
+  if (queued_updates_.find(host) != queued_updates_.end())
+    queued_observers_.AddObserver(observer);
+  else
+    observers_.AddObserver(observer);
 }
 
 void AppCacheGroup::RemoveUpdateObserver(UpdateObserver* observer) {
   observers_.RemoveObserver(observer);
+  queued_observers_.RemoveObserver(observer);
 }
 
 void AppCacheGroup::AddCache(AppCache* complete_cache) {
@@ -97,6 +127,79 @@ void AppCacheGroup::StartUpdateWithNewMasterEntry(
     update_job_ = new AppCacheUpdateJob(service_, this);
 
   update_job_->StartUpdate(host, new_master_resource);
+
+  // Run queued update immediately as an update has been started manually.
+  if (restart_update_task_) {
+    restart_update_task_->Cancel();
+    restart_update_task_ = NULL;
+    RunQueuedUpdates();
+  }
+}
+
+void AppCacheGroup::QueueUpdate(AppCacheHost* host,
+                                const GURL& new_master_resource) {
+  DCHECK(update_job_ && host && !new_master_resource.is_empty());
+  queued_updates_.insert(QueuedUpdates::value_type(host, new_master_resource));
+
+  // Need to know when host is destroyed.
+  host->AddObserver(host_observer_.get());
+
+  // If host is already observing for updates, move host to queued observers
+  // list so that host is not notified when the current update completes.
+  if (FindObserver(host, observers_)) {
+    observers_.RemoveObserver(host);
+    queued_observers_.AddObserver(host);
+  }
+}
+
+void AppCacheGroup::RunQueuedUpdates() {
+  if (restart_update_task_)
+    restart_update_task_ = NULL;
+
+  if (queued_updates_.empty())
+    return;
+
+  QueuedUpdates updates_to_run;
+  queued_updates_.swap(updates_to_run);
+  DCHECK(queued_updates_.empty());
+
+  for (QueuedUpdates::iterator it = updates_to_run.begin();
+       it != updates_to_run.end(); ++it) {
+    AppCacheHost* host = it->first;
+    host->RemoveObserver(host_observer_.get());
+    if (FindObserver(host, queued_observers_)) {
+      queued_observers_.RemoveObserver(host);
+      observers_.AddObserver(host);
+    }
+    StartUpdateWithNewMasterEntry(host, it->second);
+  }
+}
+
+bool AppCacheGroup::FindObserver(UpdateObserver* find_me,
+    const ObserverList<UpdateObserver>& observer_list) {
+  ObserverList<UpdateObserver>::Iterator it(observer_list);
+  UpdateObserver* obs;
+  while ((obs = it.GetNext()) != NULL) {
+    if (obs == find_me)
+      return true;
+  }
+  return false;
+}
+
+void AppCacheGroup::ScheduleUpdateRestart(int delay_ms) {
+  DCHECK(!restart_update_task_);
+  restart_update_task_ =
+    NewRunnableMethod(this, &AppCacheGroup::RunQueuedUpdates);
+  MessageLoop::current()->PostDelayedTask(FROM_HERE, restart_update_task_,
+                                          delay_ms);
+}
+
+void AppCacheGroup::HostDestructionImminent(AppCacheHost* host) {
+  queued_updates_.erase(host);
+  if (queued_updates_.empty() && restart_update_task_) {
+    restart_update_task_->Cancel();
+    restart_update_task_ = NULL;
+  }
 }
 
 void AppCacheGroup::SetUpdateStatus(UpdateStatus status) {
@@ -109,7 +212,17 @@ void AppCacheGroup::SetUpdateStatus(UpdateStatus status) {
     DCHECK(update_job_);
   } else {
     update_job_ = NULL;
+
+    // Check member variable before notifying observers about update finishing.
+    // Observers may remove reference to group, causing group to be deleted
+    // after the notifications. If there are queued updates, then the group
+    // will continue to exist.
+    bool restart_update = !queued_updates_.empty();
+
     FOR_EACH_OBSERVER(UpdateObserver, observers_, OnUpdateComplete(this));
+
+    if (restart_update)
+      ScheduleUpdateRestart(kUpdateRestartDelayMs);
   }
 }
 
diff --git a/webkit/appcache/appcache_group.h b/webkit/appcache/appcache_group.h
index efe8049..5cf3d99 100644
--- a/webkit/appcache/appcache_group.h
+++ b/webkit/appcache/appcache_group.h
@@ -5,10 +5,13 @@
 #ifndef WEBKIT_APPCACHE_APPCACHE_GROUP_H_
 #define WEBKIT_APPCACHE_APPCACHE_GROUP_H_
 
+#include <map>
 #include <vector>
 
 #include "base/observer_list.h"
 #include "base/ref_counted.h"
+#include "base/scoped_ptr.h"
+#include "base/task.h"
 #include "googleurl/src/gurl.h"
 #include "testing/gtest/include/gtest/gtest_prod.h"
 
@@ -18,6 +21,7 @@ class AppCache;
 class AppCacheHost;
 class AppCacheService;
 class AppCacheUpdateJob;
+class HostObserver;
 
 // Collection of application caches identified by the same manifest URL.
 // A group exists as long as it is in use by a host or is being updated.
@@ -76,6 +80,8 @@ class AppCacheGroup : public base::RefCounted<AppCacheGroup> {
                                      const GURL& new_master_resource);
 
  private:
+  class HostObserver;
+
   friend class AppCacheUpdateJob;
   friend class AppCacheUpdateJobTest;
   friend class base::RefCounted<AppCacheGroup>;
@@ -84,12 +90,23 @@ class AppCacheGroup : public base::RefCounted<AppCacheGroup> {
   ~AppCacheGroup();
 
   typedef std::vector<AppCache*> Caches;
+  typedef std::map<AppCacheHost*, GURL> QueuedUpdates;
+
+  static const int kUpdateRestartDelayMs = 1000;
 
   AppCacheUpdateJob* update_job() { return update_job_; }
   void SetUpdateStatus(UpdateStatus status);
 
   const Caches& old_caches() const { return old_caches_; }
 
+  // Update cannot be processed at this time. Queue it for a later run.
+  void QueueUpdate(AppCacheHost* host, const GURL& new_master_resource);
+  void RunQueuedUpdates();
+  bool FindObserver(UpdateObserver* find_me,
+                    const ObserverList<UpdateObserver>& observer_list);
+  void ScheduleUpdateRestart(int delay_ms);
+  void HostDestructionImminent(AppCacheHost* host);
+
   const int64 group_id_;
   const GURL manifest_url_;
   UpdateStatus update_status_;
@@ -110,8 +127,15 @@ class AppCacheGroup : public base::RefCounted<AppCacheGroup> {
   // List of objects observing this group.
   ObserverList<UpdateObserver> observers_;
 
+  // Updates that have been queued for the next run.
+  QueuedUpdates queued_updates_;
+  ObserverList<UpdateObserver> queued_observers_;
+  CancelableTask* restart_update_task_;
+  scoped_ptr<HostObserver> host_observer_;
+
   FRIEND_TEST(AppCacheGroupTest, StartUpdate);
   FRIEND_TEST(AppCacheGroupTest, CancelUpdate);
+  FRIEND_TEST(AppCacheGroupTest, QueueUpdate);
   FRIEND_TEST(AppCacheUpdateJobTest, AlreadyChecking);
   FRIEND_TEST(AppCacheUpdateJobTest, AlreadyDownloading);
   DISALLOW_COPY_AND_ASSIGN(AppCacheGroup);
diff --git a/webkit/appcache/appcache_group_unittest.cc b/webkit/appcache/appcache_group_unittest.cc
index 81280e7..384a38b 100644
--- a/webkit/appcache/appcache_group_unittest.cc
+++ b/webkit/appcache/appcache_group_unittest.cc
@@ -56,6 +56,21 @@ class TestUpdateObserver : public AppCacheGroup::UpdateObserver {
   bool group_has_cache_;
 };
 
+class TestAppCacheHost : public AppCacheHost {
+ public:
+  TestAppCacheHost(int host_id, AppCacheFrontend* frontend,
+                   AppCacheService* service)
+      : AppCacheHost(host_id, frontend, service),
+        update_completed_(false) {
+  }
+
+  virtual void OnUpdateComplete(AppCacheGroup* group) {
+    update_completed_ = true;
+  }
+
+  bool update_completed_;
+};
+
 class AppCacheGroupTest : public testing::Test {
 };
 
@@ -208,4 +223,52 @@ TEST(AppCacheGroupTest, CancelUpdate) {
   EXPECT_FALSE(observer.group_has_cache_);
 }
 
+TEST(AppCacheGroupTest, QueueUpdate) {
+  MockAppCacheService service;
+  scoped_refptr<AppCacheGroup> group =
+      new AppCacheGroup(&service, GURL("http://foo.com"), 111);
+
+  // Set state to checking to prevent update job from executing fetches.
+  group->update_status_ = AppCacheGroup::CHECKING;
+  group->StartUpdate();
+  EXPECT_TRUE(group->update_job_);
+
+  // Pretend group's update job is terminating so that next update is queued.
+  group->update_job_->internal_state_ = AppCacheUpdateJob::REFETCH_MANIFEST;
+  EXPECT_TRUE(group->update_job_->IsTerminating());
+
+  TestAppCacheFrontend frontend;
+  TestAppCacheHost host(1, &frontend, &service);
+  host.new_master_entry_url_ = GURL("http://foo.com/bar.txt");
+  group->StartUpdateWithNewMasterEntry(&host, host.new_master_entry_url_);
+  EXPECT_FALSE(group->queued_updates_.empty());
+
+  group->AddUpdateObserver(&host);
+  EXPECT_FALSE(group->FindObserver(&host, group->observers_));
+  EXPECT_TRUE(group->FindObserver(&host, group->queued_observers_));
+
+  // Delete update to cause it to complete. Verify no update complete notice
+  // sent to host.
+  delete group->update_job_;
+  EXPECT_EQ(AppCacheGroup::IDLE, group->update_status_);
+  EXPECT_TRUE(group->restart_update_task_);
+  EXPECT_FALSE(host.update_completed_);
+
+  // Start another update. Cancels task and will run queued updates.
+  group->update_status_ = AppCacheGroup::CHECKING;  // prevent actual fetches
+  group->StartUpdate();
+  EXPECT_TRUE(group->update_job_);
+  EXPECT_FALSE(group->restart_update_task_);
+  EXPECT_TRUE(group->queued_updates_.empty());
+  EXPECT_FALSE(group->update_job_->pending_master_entries_.empty());
+  EXPECT_FALSE(group->FindObserver(&host, group->queued_observers_));
+  EXPECT_TRUE(group->FindObserver(&host, group->observers_));
+
+  // Delete update to cause it to complete. Verify host is notified.
+  delete group->update_job_;
+  EXPECT_EQ(AppCacheGroup::IDLE, group->update_status_);
+  EXPECT_FALSE(group->restart_update_task_);
+  EXPECT_TRUE(host.update_completed_);
+}
+
 }  // namespace appcache
diff --git a/webkit/appcache/appcache_host.h b/webkit/appcache/appcache_host.h
index f154bae..13acdb0 100644
--- a/webkit/appcache/appcache_host.h
+++ b/webkit/appcache/appcache_host.h
@@ -178,6 +178,7 @@ class AppCacheHost : public AppCacheStorage::Delegate,
   FRIEND_TEST(AppCacheHostTest, FailedCacheLoad);
   FRIEND_TEST(AppCacheHostTest, FailedGroupLoad);
   FRIEND_TEST(AppCacheHostTest, SetSwappableCache);
+  FRIEND_TEST(AppCacheGroupTest, QueueUpdate);
   DISALLOW_COPY_AND_ASSIGN(AppCacheHost);
 };
 
diff --git a/webkit/appcache/appcache_update_job.cc b/webkit/appcache/appcache_update_job.cc
index 8cd752f..8d7ddc1 100644
--- a/webkit/appcache/appcache_update_job.cc
+++ b/webkit/appcache/appcache_update_job.cc
@@ -142,7 +142,7 @@ void AppCacheUpdateJob::StartUpdate(AppCacheHost* host,
 
     // Cannot add more to this update if already terminating.
     if (IsTerminating()) {
-      // TODO(jennb): requeue in group
+      group_->QueueUpdate(host, new_master_resource);
       return;
     }
 
diff --git a/webkit/appcache/appcache_update_job.h b/webkit/appcache/appcache_update_job.h
index 7c60ef1..304df03 100644
--- a/webkit/appcache/appcache_update_job.h
+++ b/webkit/appcache/appcache_update_job.h
@@ -15,6 +15,7 @@
 #include "base/task.h"
 #include "googleurl/src/gurl.h"
 #include "net/url_request/url_request.h"
+#include "testing/gtest/include/gtest/gtest_prod.h"
 #include "webkit/appcache/appcache.h"
 #include "webkit/appcache/appcache_host.h"
 #include "webkit/appcache/appcache_interfaces.h"
@@ -234,6 +235,7 @@ class AppCacheUpdateJob : public URLRequest::Delegate,
   net::CompletionCallbackImpl<AppCacheUpdateJob> manifest_data_write_callback_;
   net::CompletionCallbackImpl<AppCacheUpdateJob> manifest_data_read_callback_;
 
+  FRIEND_TEST(AppCacheGroupTest, QueueUpdate);
   DISALLOW_COPY_AND_ASSIGN(AppCacheUpdateJob);
 };
 
diff --git a/webkit/appcache/appcache_update_job_unittest.cc b/webkit/appcache/appcache_update_job_unittest.cc
index 70b980a..5679ef3 100644
--- a/webkit/appcache/appcache_update_job_unittest.cc
+++ b/webkit/appcache/appcache_update_job_unittest.cc
@@ -1890,6 +1890,52 @@ class AppCacheUpdateJobTest : public testing::Test,
     WaitForUpdateToFinish();
   }
 
+  void QueueMasterEntryTest() {
+    ASSERT_EQ(MessageLoop::TYPE_IO, MessageLoop::current()->type());
+
+    MakeService();
+    group_ = new AppCacheGroup(
+        service_.get(), http_server_->TestServerPage("files/manifest1"), 111);
+    AppCacheUpdateJob* update = new AppCacheUpdateJob(service_.get(), group_);
+    group_->update_job_ = update;
+
+    // Pretend update job has been running and is about to terminate.
+    group_->update_status_ = AppCacheGroup::DOWNLOADING;
+    update->internal_state_ = AppCacheUpdateJob::REFETCH_MANIFEST;
+    EXPECT_TRUE(update->IsTerminating());
+
+    // Start an update. Should be queued.
+    MockFrontend* frontend = MakeMockFrontend();
+    AppCacheHost* host = MakeHost(1, frontend);
+    host->new_master_entry_url_ =
+        http_server_->TestServerPage("files/explicit2");
+    update->StartUpdate(host, host->new_master_entry_url_);
+    EXPECT_TRUE(update->pending_master_entries_.empty());
+    EXPECT_FALSE(group_->queued_updates_.empty());
+
+    // Delete update, causing it to finish, which should trigger a new update
+    // for the queued host and master entry after a delay.
+    delete update;
+    EXPECT_TRUE(group_->restart_update_task_);
+
+    // Set up checks for when queued update job finishes.
+    do_checks_after_update_finished_ = true;
+    expect_group_obsolete_ = false;
+    expect_group_has_cache_ = true;
+    tested_manifest_ = MANIFEST1;
+    expect_extra_entries_.insert(AppCache::EntryMap::value_type(
+        host->new_master_entry_url_, AppCacheEntry(AppCacheEntry::MASTER)));
+    MockFrontend::HostIds ids1(1, host->host_id());
+    frontend->AddExpectedEvent(ids1, CHECKING_EVENT);
+    frontend->AddExpectedEvent(ids1, DOWNLOADING_EVENT);
+    frontend->AddExpectedEvent(ids1, PROGRESS_EVENT);
+    frontend->AddExpectedEvent(ids1, PROGRESS_EVENT);
+    frontend->AddExpectedEvent(ids1, CACHED_EVENT);
+
+    // Group status will be IDLE so cannot call WaitForUpdateToFinish.
+    group_->AddUpdateObserver(this);
+  }
+
   void WaitForUpdateToFinish() {
     if (group_->update_status() == AppCacheGroup::IDLE)
       UpdateFinished();
@@ -2458,4 +2504,8 @@ TEST_F(AppCacheUpdateJobTest, StartUpdateMidDownload) {
   RunTestOnIOThread(&AppCacheUpdateJobTest::StartUpdateMidDownloadTest);
 }
 
+TEST_F(AppCacheUpdateJobTest, QueueMasterEntry) {
+  RunTestOnIOThread(&AppCacheUpdateJobTest::QueueMasterEntryTest);
+}
+
 }  // namespace appcache