Isolated bug fixes from CL 385104 to address the following:

  - using references for collections  
  - only getting request response code if request was successful
  - writing response info for URL fetches that do not contain any data
  - only bothering with reading/writing response if response code is 2xx

TEST=added test and data files to verify empty file still gets response written to storage
BUG=none

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

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

4 files changed, 97 insertions, 28 deletions

diff --git a/webkit/appcache/appcache_update_job.cc b/webkit/appcache/appcache_update_job.cc
index efcd065..83190d0 100644
--- a/webkit/appcache/appcache_update_job.cc
+++ b/webkit/appcache/appcache_update_job.cc
@@ -34,7 +34,6 @@ class UpdateJobInfo :  public URLRequest::UserData {
         retry_503_attempts_(0),
         update_job_(NULL),
         request_(NULL),
-        wrote_response_info_(false),
         ALLOW_THIS_IN_INITIALIZER_LIST(write_callback_(
             this, &UpdateJobInfo::OnWriteComplete)) {
   }
@@ -61,7 +60,6 @@ class UpdateJobInfo :  public URLRequest::UserData {
   scoped_ptr<AppCacheResponseWriter> response_writer_;
   AppCacheUpdateJob* update_job_;
   URLRequest* request_;
-  bool wrote_response_info_;
   net::CompletionCallbackImpl<UpdateJobInfo> write_callback_;
 };
 
@@ -183,10 +181,26 @@ void AppCacheUpdateJob::FetchManifest(bool is_first_fetch) {
 }
 
 void AppCacheUpdateJob::OnResponseStarted(URLRequest *request) {
-  if (request->status().is_success())
-    ReadResponseData(request);
-  else
+  if (request->status().is_success() &&
+      (request->GetResponseCode() / 100) == 2) {
+    // Write response info to storage for URL fetches. Wait for async write
+    // completion before reading any response data.
+    UpdateJobInfo* info =
+        static_cast<UpdateJobInfo*>(request->GetUserData(this));
+    if (info->type_ == UpdateJobInfo::URL_FETCH) {
+      info->SetUpResponseWriter(
+          service_->storage()->CreateResponseWriter(manifest_url_),
+          this, request);
+      scoped_refptr<HttpResponseInfoIOBuffer> io_buffer =
+          new HttpResponseInfoIOBuffer(
+              new net::HttpResponseInfo(request->response_info()));
+      info->response_writer_->WriteInfo(io_buffer, &info->write_callback_);
+    } else {
+      ReadResponseData(request);
+    }
+  } else {
     OnResponseCompleted(request);
+  }
 }
 
 void AppCacheUpdateJob::ReadResponseData(URLRequest* request) {
@@ -236,11 +250,7 @@ bool AppCacheUpdateJob::ConsumeResponseData(URLRequest* request,
       manifest_data_.append(info->buffer_->data(), bytes_read);
       break;
     case UpdateJobInfo::URL_FETCH:
-      if (!info->response_writer_.get()) {
-        info->SetUpResponseWriter(
-            service_->storage()->CreateResponseWriter(manifest_url_),
-            this, request);
-      }
+      DCHECK(info->response_writer_.get());
       info->response_writer_->WriteData(info->buffer_, bytes_read,
                                         &info->write_callback_);
       return false;  // wait for async write completion to continue reading
@@ -264,15 +274,6 @@ void AppCacheUpdateJob::OnWriteResponseComplete(int result,
     return;
   }
 
-  if (!info->wrote_response_info_) {
-    info->wrote_response_info_ = true;
-    scoped_refptr<HttpResponseInfoIOBuffer> io_buffer =
-        new HttpResponseInfoIOBuffer(
-            new net::HttpResponseInfo(request->response_info()));
-    info->response_writer_->WriteInfo(io_buffer, &info->write_callback_);
-    return;
-  }
-
   ReadResponseData(request);
 }
 
@@ -426,11 +427,10 @@ void AppCacheUpdateJob::ContinueHandleManifestFetchCompleted(bool changed) {
   // Associate all pending master hosts with the newly created cache.
   for (PendingMasters::iterator it = pending_master_entries_.begin();
        it != pending_master_entries_.end(); ++it) {
-    PendingHosts hosts = it->second;
+    PendingHosts& hosts = it->second;
     for (PendingHosts::iterator host_it = hosts.begin();
          host_it != hosts.end(); ++host_it) {
-      AppCacheHost* host = *host_it;
-      host->AssociateCache(inprogress_cache_);
+      (*host_it)->AssociateCache(inprogress_cache_);
     }
   }
 
@@ -447,7 +447,8 @@ void AppCacheUpdateJob::HandleUrlFetchCompleted(URLRequest* request) {
   pending_url_fetches_.erase(url);
   ++url_fetches_completed_;
 
-  int response_code = request->GetResponseCode();
+  int response_code = request->status().is_success()
+      ? request->GetResponseCode() : -1;
   AppCacheEntry& entry = url_file_list_.find(url)->second;
 
   UpdateJobInfo* info =
@@ -508,7 +509,8 @@ void AppCacheUpdateJob::HandleManifestRefetchCompleted(URLRequest* request) {
   DCHECK(internal_state_ == REFETCH_MANIFEST);
   manifest_url_request_ = NULL;
 
-  int response_code = request->GetResponseCode();
+  int response_code = request->status().is_success()
+      ? request->GetResponseCode() : -1;
   if (response_code == 304 || manifest_data_ == manifest_refetch_data_) {
     // Only need to store response in storage if manifest is not already an
     // an entry in the cache.
@@ -601,11 +603,10 @@ void AppCacheUpdateJob::NotifyAllPendingMasterHosts(EventID event_id) {
   HostNotifier host_notifier;
   for (PendingMasters::iterator it = pending_master_entries_.begin();
        it != pending_master_entries_.end(); ++it) {
-    PendingHosts hosts = it->second;
+    PendingHosts& hosts = it->second;
     for (PendingHosts::iterator host_it = hosts.begin();
          host_it != hosts.end(); ++host_it) {
-      AppCacheHost* host = *host_it;
-      host_notifier.AddHost(host);
+      host_notifier.AddHost(*host_it);
     }
   }
 
diff --git a/webkit/appcache/appcache_update_job_unittest.cc b/webkit/appcache/appcache_update_job_unittest.cc
index aae7ef8..d7b695a 100644
--- a/webkit/appcache/appcache_update_job_unittest.cc
+++ b/webkit/appcache/appcache_update_job_unittest.cc
@@ -917,6 +917,37 @@ class AppCacheUpdateJobTest : public testing::Test,
     WaitForUpdateToFinish();
   }
 
+  void EmptyFileTest() {
+    ASSERT_EQ(MessageLoop::TYPE_IO, MessageLoop::current()->type());
+
+    MakeService();
+    group_ = new AppCacheGroup(service_.get(),
+        http_server_->TestServerPage("files/empty-file-manifest"));
+    AppCacheUpdateJob* update = new AppCacheUpdateJob(service_.get(), group_);
+    group_->update_job_ = update;
+
+    AppCache* cache = MakeCacheForGroup(service_->storage()->NewCacheId(), 22);
+    MockFrontend* frontend = MakeMockFrontend();
+    AppCacheHost* host = MakeHost(1, frontend);
+    host->AssociateCache(cache);
+
+    update->StartUpdate(host, GURL::EmptyGURL());
+    EXPECT_TRUE(update->manifest_url_request_ != NULL);
+
+    // Set up checks for when update job finishes.
+    do_checks_after_update_finished_ = true;
+    expect_group_obsolete_ = false;
+    expect_group_has_cache_ = true;
+    tested_manifest_ = EMPTY_FILE_MANIFEST;
+    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, UPDATE_READY_EVENT);
+
+    WaitForUpdateToFinish();
+  }
+
   void RetryRequestTest() {
     ASSERT_EQ(MessageLoop::TYPE_IO, MessageLoop::current()->type());
 
@@ -1355,6 +1386,9 @@ class AppCacheUpdateJobTest : public testing::Test,
       case EMPTY_MANIFEST:
         VerifyEmptyManifest(group_->newest_complete_cache());
         break;
+      case EMPTY_FILE_MANIFEST:
+        VerifyEmptyFileManifest(group_->newest_complete_cache());
+        break;
       case NONE:
       default:
         break;
@@ -1440,7 +1474,7 @@ class AppCacheUpdateJobTest : public testing::Test,
   }
 
   void VerifyEmptyManifest(AppCache* cache) {
-    ASSERT_TRUE(cache!= NULL);
+    ASSERT_TRUE(cache != NULL);
     EXPECT_EQ(group_, cache->owning_group());
     EXPECT_TRUE(cache->is_complete());
 
@@ -1458,6 +1492,30 @@ class AppCacheUpdateJobTest : public testing::Test,
     EXPECT_TRUE(cache->update_time_ > base::TimeTicks());
   }
 
+  void VerifyEmptyFileManifest(AppCache* cache) {
+    ASSERT_TRUE(cache != NULL);
+    EXPECT_EQ(group_, cache->owning_group());
+    EXPECT_TRUE(cache->is_complete());
+
+    EXPECT_EQ(size_t(2), cache->entries().size());
+    AppCacheEntry* entry = cache->GetEntry(
+        http_server_->TestServerPage("files/empty-file-manifest"));
+    ASSERT_TRUE(entry);
+    EXPECT_EQ(AppCacheEntry::MANIFEST, entry->types());
+
+    entry = cache->GetEntry(
+        http_server_->TestServerPage("files/empty1"));
+    ASSERT_TRUE(entry);
+    EXPECT_EQ(AppCacheEntry::EXPLICIT, entry->types());
+    EXPECT_TRUE(entry->has_response_id());
+
+    EXPECT_TRUE(cache->fallback_namespaces_.empty());
+    EXPECT_TRUE(cache->online_whitelist_namespaces_.empty());
+    EXPECT_FALSE(cache->online_whitelist_all_);
+
+    EXPECT_TRUE(cache->update_time_ > base::TimeTicks());
+  }
+
  private:
   // Various manifest files used in this test.
   enum TestedManifest {
@@ -1465,6 +1523,7 @@ class AppCacheUpdateJobTest : public testing::Test,
     MANIFEST1,
     MANIFEST_MERGED_TYPES,
     EMPTY_MANIFEST,
+    EMPTY_FILE_MANIFEST,
   };
 
   static scoped_ptr<base::Thread> io_thread_;
@@ -1635,6 +1694,10 @@ TEST_F(AppCacheUpdateJobTest, EmptyManifest) {
   RunTestOnIOThread(&AppCacheUpdateJobTest::EmptyManifestTest);
 }
 
+TEST_F(AppCacheUpdateJobTest, EmptyFile) {
+  RunTestOnIOThread(&AppCacheUpdateJobTest::EmptyFileTest);
+}
+
 TEST_F(AppCacheUpdateJobTest, RetryRequest) {
   RunTestOnIOThread(&AppCacheUpdateJobTest::RetryRequestTest);
 }
diff --git a/webkit/appcache/data/appcache_unittest/empty-file-manifest b/webkit/appcache/data/appcache_unittest/empty-file-manifest
new file mode 100644
index 0000000..e58b0c0
--- /dev/null
+++ b/webkit/appcache/data/appcache_unittest/empty-file-manifest
@@ -0,0 +1,3 @@
+CACHE MANIFEST
+empty1
+
diff --git a/webkit/appcache/data/appcache_unittest/empty-file-manifest.mock-http-headers b/webkit/appcache/data/appcache_unittest/empty-file-manifest.mock-http-headers
new file mode 100644
index 0000000..6e904b9
--- /dev/null
+++ b/webkit/appcache/data/appcache_unittest/empty-file-manifest.mock-http-headers
@@ -0,0 +1,2 @@
+HTTP/1.1 200 OK
+Content-type: text/cache-manifest