gdata: Remove a werid behavior from GDataCacheMetadata::UpdateCache()

Though it's not documented in the function comment, UpdateCache()
had ability to remove an existing cache entry. This is more like a
surprising behavior. This patch removes the behavior.

Along the way, rename the function to be more descriptive.
Also rename RemoveFromCache() to RemoveCacheEntry().

BUG=136921
TEST=out/Release/unit_tests --gtest_filter=GData*

Review URL: https://chromiumcodereview.appspot.com/10690154

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

chrome/browser/chromeos/gdata/gdata_cache.cc

@@ -882,7 +882,7 @@ void GDataCache::Store(const std::string& resource_id,
     // Now that file operations have completed, update cache map.
     new_cache_entry.SetPresent(true);
     new_cache_entry.SetPersistent(sub_dir_type == CACHE_TYPE_PERSISTENT);
-    metadata_->UpdateCache(resource_id, new_cache_entry);
+    metadata_->AddOrUpdateCacheEntry(resource_id, new_cache_entry);
   }
 }
 
@@ -971,7 +971,7 @@ void GDataCache::Pin(const std::string& resource_id,
     // Now that file operations have completed, update cache map.
     new_cache_entry.SetPinned(true);
     new_cache_entry.SetPersistent(sub_dir_type == CACHE_TYPE_PERSISTENT);
-    metadata_->UpdateCache(resource_id, new_cache_entry);
+    metadata_->AddOrUpdateCacheEntry(resource_id, new_cache_entry);
   }
 }
 
@@ -1049,10 +1049,15 @@ void GDataCache::Unpin(const std::string& resource_id,
 
   if (*error == base::PLATFORM_FILE_OK) {
     // Now that file operations have completed, update cache map.
-    GDataCacheEntry new_cache_entry(md5, cache_entry->cache_state());
-    new_cache_entry.SetPinned(false);
-    new_cache_entry.SetPersistent(sub_dir_type == CACHE_TYPE_PERSISTENT);
-    metadata_->UpdateCache(resource_id, new_cache_entry);
+    if (cache_entry->IsPresent()) {
+      GDataCacheEntry new_cache_entry(md5, cache_entry->cache_state());
+      new_cache_entry.SetPinned(false);
+      new_cache_entry.SetPersistent(sub_dir_type == CACHE_TYPE_PERSISTENT);
+      metadata_->AddOrUpdateCacheEntry(resource_id, new_cache_entry);
+    } else {
+      // Remove the existing entry if we are unpinning a non-present file.
+      metadata_->RemoveCacheEntry(resource_id);
+    }
   }
 }
 
@@ -1117,7 +1122,7 @@ void GDataCache::SetMountedState(const FilePath& file_path,
   if (*error == base::PLATFORM_FILE_OK) {
     // Now that cache operation is complete, update cache map
     new_cache_entry.SetPersistent(dest_subdir == CACHE_TYPE_PERSISTENT);
-    metadata_->UpdateCache(resource_id, new_cache_entry);
+    metadata_->AddOrUpdateCacheEntry(resource_id, new_cache_entry);
   }
 }
 
@@ -1222,7 +1227,7 @@ void GDataCache::MarkDirty(const std::string& resource_id,
     GDataCacheEntry new_cache_entry(md5, cache_entry->cache_state());
     new_cache_entry.SetDirty(true);
     new_cache_entry.SetPersistent(sub_dir_type == CACHE_TYPE_PERSISTENT);
-    metadata_->UpdateCache(resource_id, new_cache_entry);
+    metadata_->AddOrUpdateCacheEntry(resource_id, new_cache_entry);
   }
 }
 
@@ -1370,7 +1375,7 @@ void GDataCache::ClearDirty(const std::string& resource_id,
     GDataCacheEntry new_cache_entry(md5, cache_entry->cache_state());
     new_cache_entry.SetDirty(false);
     new_cache_entry.SetPersistent(sub_dir_type == CACHE_TYPE_PERSISTENT);
-    metadata_->UpdateCache(resource_id, new_cache_entry);
+    metadata_->AddOrUpdateCacheEntry(resource_id, new_cache_entry);
   }
 }
 
@@ -1379,9 +1384,8 @@ void GDataCache::Remove(const std::string& resource_id,
   AssertOnSequencedWorkerPool();
   DCHECK(error);
 
-  // MD5 is not passed into RemoveFromCache and hence
-  // RemoveFromCacheOnBlockingPool, because we would delete all cache files
-  // corresponding to <resource_id> regardless of the md5.
+  // MD5 is not passed into RemoveCacheEntry because we would delete all
+  // cache files corresponding to <resource_id> regardless of the md5.
   // So, search for entry in cache without taking md5 into account.
   scoped_ptr<GDataCacheEntry> cache_entry =
       GetCacheEntry(resource_id, std::string());
@@ -1434,7 +1438,7 @@ void GDataCache::Remove(const std::string& resource_id,
   }
 
   // Now that all file operations have completed, remove from cache map.
-  metadata_->RemoveFromCache(resource_id);
+  metadata_->RemoveCacheEntry(resource_id);
 
   *error = base::PLATFORM_FILE_OK;
 }

chrome/browser/chromeos/gdata/gdata_cache_metadata.cc

@@ -174,36 +174,25 @@ void GDataCacheMetadataMap::Initialize(
   DVLOG(1) << "Directory scan finished";
 }
 
-void GDataCacheMetadataMap::UpdateCache(
+void GDataCacheMetadataMap::AddOrUpdateCacheEntry(
     const std::string& resource_id,
     const GDataCacheEntry& cache_entry) {
   AssertOnSequencedWorkerPool();
 
   CacheMap::iterator iter = cache_map_.find(resource_id);
   if (iter == cache_map_.end()) {  // New resource, create new entry.
-    // Makes no sense to create new entry if cache state is NONE.
-    DCHECK(cache_entry.cache_state() != CACHE_STATE_NONE);
-    if (cache_entry.cache_state() != CACHE_STATE_NONE) {
-      cache_map_.insert(std::make_pair(resource_id, cache_entry));
-      DVLOG(1) << "Added res_id=" << resource_id
-               << ", " << cache_entry.ToString();
-    }
+    cache_map_.insert(std::make_pair(resource_id, cache_entry));
+    DVLOG(1) << "Added resource_id=" << resource_id
+             << ", " << cache_entry.ToString();
   } else {  // Resource exists.
-    // If cache state is NONE, delete entry from cache map.
-    if (cache_entry.cache_state() == CACHE_STATE_NONE) {
-      DVLOG(1) << "Deleting res_id=" << resource_id
-               << ", " << iter->second.ToString();
-      cache_map_.erase(iter);
-    } else {  // Otherwise, update entry in cache map.
-      iter->second.set_md5(cache_entry.md5());
-      iter->second.set_cache_state(cache_entry.cache_state());
-      DVLOG(1) << "Updated res_id=" << resource_id
-               << ", " << iter->second.ToString();
-    }
+    iter->second.set_md5(cache_entry.md5());
+    iter->second.set_cache_state(cache_entry.cache_state());
+    DVLOG(1) << "Updated resource_id=" << resource_id
+             << ", " << iter->second.ToString();
   }
 }
 
-void GDataCacheMetadataMap::RemoveFromCache(const std::string& resource_id) {
+void GDataCacheMetadataMap::RemoveCacheEntry(const std::string& resource_id) {
   AssertOnSequencedWorkerPool();
 
   CacheMap::iterator iter = cache_map_.find(resource_id);

chrome/browser/chromeos/gdata/gdata_cache_metadata.h

@@ -35,13 +35,13 @@ class GDataCacheMetadata {
       const base::SequencedWorkerPool::SequenceToken& sequence_token);
   virtual ~GDataCacheMetadata();
 
-  // Updates cache map with entry corresponding to |resource_id|.
-  // Creates new entry if it doesn't exist, otherwise update the entry.
-  virtual void UpdateCache(const std::string& resource_id,
-                           const GDataCacheEntry& cache_entry) = 0;
+  // Adds a new cache entry corresponding to |resource_id| if it doesn't
+  // exist, otherwise update the existing entry.
+  virtual void AddOrUpdateCacheEntry(const std::string& resource_id,
+                                     const GDataCacheEntry& cache_entry) = 0;
 
   // Removes entry corresponding to |resource_id| from cache map.
-  virtual void RemoveFromCache(const std::string& resource_id) = 0;
+  virtual void RemoveCacheEntry(const std::string& resource_id) = 0;
 
   // Returns the cache entry for file corresponding to |resource_id| and |md5|
   // if entry exists in cache map.  Otherwise, returns NULL.
@@ -83,10 +83,10 @@ class GDataCacheMetadataMap : public GDataCacheMetadata {
   void Initialize(const std::vector<FilePath>& cache_paths);
 
   // GDataCacheMetadata overrides:
-  virtual void UpdateCache(
+  virtual void AddOrUpdateCacheEntry(
       const std::string& resource_id,
       const GDataCacheEntry& cache_entry) OVERRIDE;
-  virtual void RemoveFromCache(const std::string& resource_id) OVERRIDE;
+  virtual void RemoveCacheEntry(const std::string& resource_id) OVERRIDE;
   virtual scoped_ptr<GDataCacheEntry> GetCacheEntry(
       const std::string& resource_id,
       const std::string& md5) OVERRIDE;

chrome/browser/chromeos/gdata/gdata_cache_metadata_unittest.cc

@@ -142,7 +142,7 @@ TEST_F(GDataCacheMetadataMapTest, CacheTest) {
       GDataCache::CACHE_TYPE_PERSISTENT;
   int test_cache_state = (CACHE_STATE_PRESENT |
                           CACHE_STATE_PERSISTENT);
-  metadata_->UpdateCache(
+  metadata_->AddOrUpdateCacheEntry(
       test_resource_id,
       GDataCacheEntry(test_file_md5, test_cache_state));
 
@@ -174,7 +174,7 @@ TEST_F(GDataCacheMetadataMapTest, CacheTest) {
   test_file_md5 = "test_file_md5_2";
   test_sub_dir_type = GDataCache::CACHE_TYPE_TMP;
   test_cache_state = CACHE_STATE_PINNED;
-  metadata_->UpdateCache(
+  metadata_->AddOrUpdateCacheEntry(
       test_resource_id,
       GDataCacheEntry(test_file_md5, test_cache_state));
 
@@ -196,7 +196,7 @@ TEST_F(GDataCacheMetadataMapTest, CacheTest) {
   test_file_md5 = "test_file_md5_3";
   test_sub_dir_type = GDataCache::CACHE_TYPE_TMP;
   test_cache_state = CACHE_STATE_DIRTY;
-  metadata_->UpdateCache(
+  metadata_->AddOrUpdateCacheEntry(
       test_resource_id,
       GDataCacheEntry(test_file_md5, test_cache_state));
 
@@ -221,7 +221,7 @@ TEST_F(GDataCacheMetadataMapTest, CacheTest) {
   EXPECT_EQ(test_file_md5, cache_entry->md5());
 
   // Remove the entry.
-  metadata_->RemoveFromCache(test_resource_id);
+  metadata_->RemoveCacheEntry(test_resource_id);
   cache_entry =
       metadata_->GetCacheEntry(test_resource_id, std::string()).Pass();
   EXPECT_FALSE(cache_entry.get());
@@ -231,7 +231,7 @@ TEST_F(GDataCacheMetadataMapTest, CacheTest) {
   test_file_md5 = "test_file_md5_4";
   test_sub_dir_type = GDataCache::CACHE_TYPE_TMP;
   test_cache_state = CACHE_STATE_PRESENT;
-  metadata_->UpdateCache(
+  metadata_->AddOrUpdateCacheEntry(
       test_resource_id,
       GDataCacheEntry(test_file_md5, test_cache_state));
 
@@ -242,18 +242,6 @@ TEST_F(GDataCacheMetadataMapTest, CacheTest) {
   EXPECT_EQ(test_file_md5, cache_entry->md5());
   EXPECT_EQ(test_sub_dir_type, GDataCache::GetSubDirectoryType(*cache_entry));
   EXPECT_EQ(test_cache_state, cache_entry->cache_state());
-
-  // Update with CACHE_STATE_NONE should evict the entry.
-  test_file_md5 = "test_file_md5_5";
-  test_sub_dir_type = GDataCache::CACHE_TYPE_TMP;
-  test_cache_state = CACHE_STATE_NONE;
-  metadata_->UpdateCache(
-      test_resource_id,
-      GDataCacheEntry(test_file_md5, test_cache_state));
-
-  cache_entry =
-      metadata_->GetCacheEntry(test_resource_id, std::string()).Pass();
-  EXPECT_FALSE(cache_entry.get());
 }
 
 TEST_F(GDataCacheMetadataMapTest, Initialization) {