drive: Stop using FileCacheEntry related methods in FileCache

Remove ResourceMetadataStorage::PutCacheEntry, RemoveCacheEntry and GetCacheEntryIterator. (GetCacheEntry is kept for tests)
FileCache::ClearAll is only responsible to delete files.

BUG=275271
TEST=unit_tests

Review URL: https://codereview.chromium.org/296463002

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

chrome/browser/chromeos/drive/file_cache.h

@@ -114,7 +114,7 @@ class FileCache {
   // Removes the specified cache entry and delete cache files if available.
   FileError Remove(const std::string& id);
 
-  // Removes all the files in the cache directory and cache entries in DB.
+  // Removes all the files in the cache directory.
   bool ClearAll();
 
   // Initializes the cache. Returns true on success.

chrome/browser/chromeos/drive/resource_metadata.cc

@@ -387,8 +387,17 @@ FileError ResourceMetadata::RefreshEntry(const ResourceEntry& entry) {
   if (!new_parent.file_info().is_directory())
     return FILE_ERROR_NOT_A_DIRECTORY;
 
+  // Do not overwrite cache states.
+  // Cache state should be changed via FileCache.
+  ResourceEntry updated_entry(entry);
+  if (old_entry.file_specific_info().has_cache_state()) {
+    *updated_entry.mutable_file_specific_info()->mutable_cache_state() =
+        old_entry.file_specific_info().cache_state();
+  } else if (updated_entry.file_specific_info().has_cache_state()) {
+    updated_entry.mutable_file_specific_info()->clear_cache_state();
+  }
   // Remove from the old parent and add it to the new parent with the new data.
-  return PutEntryUnderDirectory(entry);
+  return PutEntryUnderDirectory(updated_entry);
 }
 
 FileError ResourceMetadata::GetSubDirectoriesRecursively(

chrome/browser/chromeos/drive/resource_metadata_storage.cc

@@ -240,62 +240,6 @@ bool ResourceMetadataStorage::Iterator::HasError() const {
   return !it_->status().ok();
 }
 
-ResourceMetadataStorage::CacheEntryIterator::CacheEntryIterator(
-    scoped_ptr<leveldb::Iterator> it) : it_(it.Pass()) {
-  base::ThreadRestrictions::AssertIOAllowed();
-  DCHECK(it_);
-
-  it_->SeekToFirst();
-  AdvanceInternal();
-}
-
-ResourceMetadataStorage::CacheEntryIterator::~CacheEntryIterator() {
-  base::ThreadRestrictions::AssertIOAllowed();
-}
-
-bool ResourceMetadataStorage::CacheEntryIterator::IsAtEnd() const {
-  base::ThreadRestrictions::AssertIOAllowed();
-  return !it_->Valid();
-}
-
-const std::string& ResourceMetadataStorage::CacheEntryIterator::GetID() const {
-  base::ThreadRestrictions::AssertIOAllowed();
-  DCHECK(!IsAtEnd());
-  return id_;
-}
-
-const FileCacheEntry&
-ResourceMetadataStorage::CacheEntryIterator::GetValue() const {
-  base::ThreadRestrictions::AssertIOAllowed();
-  DCHECK(!IsAtEnd());
-  return entry_;
-}
-
-void ResourceMetadataStorage::CacheEntryIterator::Advance() {
-  base::ThreadRestrictions::AssertIOAllowed();
-  DCHECK(!IsAtEnd());
-
-  it_->Next();
-  AdvanceInternal();
-}
-
-bool ResourceMetadataStorage::CacheEntryIterator::HasError() const {
-  base::ThreadRestrictions::AssertIOAllowed();
-  return !it_->status().ok();
-}
-
-void ResourceMetadataStorage::CacheEntryIterator::AdvanceInternal() {
-  for (; it_->Valid(); it_->Next()) {
-    // Skip unparsable broken entries.
-    // TODO(hashimoto): Broken entries should be cleaned up at some point.
-    if (IsCacheEntryKey(it_->key()) &&
-        entry_.ParseFromArray(it_->value().data(), it_->value().size())) {
-      id_ = GetIdFromCacheEntryKey(it_->key());
-      break;
-    }
-  }
-}
-
 // static
 bool ResourceMetadataStorage::UpgradeOldDB(
     const base::FilePath& directory_path,
@@ -665,6 +609,8 @@ FileError ResourceMetadataStorage::PutEntry(const ResourceEntry& entry) {
       return FILE_ERROR_FAILED;
     }
     batch.Put(GetCacheEntryKey(id), serialized_entry);
+  } else {
+    batch.Delete(GetCacheEntryKey(id));
   }
 
   // Put the entry itself.
@@ -708,6 +654,8 @@ FileError ResourceMetadataStorage::GetEntry(const std::string& id,
   if (cache_error == FILE_ERROR_OK) {
     *out_entry->mutable_file_specific_info()->mutable_cache_state() =
         cache_entry;
+  } else {
+    out_entry->mutable_file_specific_info()->clear_cache_state();
   }
   return FILE_ERROR_OK;
 }
@@ -785,24 +733,6 @@ FileError ResourceMetadataStorage::GetChildren(
   return LevelDBStatusToFileError(it->status());
 }
 
-FileError ResourceMetadataStorage::PutCacheEntry(const std::string& id,
-                                                 const FileCacheEntry& entry) {
-  base::ThreadRestrictions::AssertIOAllowed();
-  DCHECK(!id.empty());
-
-  std::string serialized_entry;
-  if (!entry.SerializeToString(&serialized_entry)) {
-    DLOG(ERROR) << "Failed to serialize the entry.";
-    return FILE_ERROR_FAILED;
-  }
-
-  const leveldb::Status status = resource_map_->Put(
-      leveldb::WriteOptions(),
-      leveldb::Slice(GetCacheEntryKey(id)),
-      leveldb::Slice(serialized_entry));
-  return LevelDBStatusToFileError(status);
-}
-
 FileError ResourceMetadataStorage::GetCacheEntry(const std::string& id,
                                                  FileCacheEntry* out_entry) {
   base::ThreadRestrictions::AssertIOAllowed();
@@ -819,25 +749,6 @@ FileError ResourceMetadataStorage::GetCacheEntry(const std::string& id,
       FILE_ERROR_OK : FILE_ERROR_FAILED;
 }
 
-FileError ResourceMetadataStorage::RemoveCacheEntry(const std::string& id) {
-  base::ThreadRestrictions::AssertIOAllowed();
-  DCHECK(!id.empty());
-
-  const leveldb::Status status = resource_map_->Delete(
-      leveldb::WriteOptions(),
-      leveldb::Slice(GetCacheEntryKey(id)));
-  return LevelDBStatusToFileError(status);
-}
-
-scoped_ptr<ResourceMetadataStorage::CacheEntryIterator>
-ResourceMetadataStorage::GetCacheEntryIterator() {
-  base::ThreadRestrictions::AssertIOAllowed();
-
-  scoped_ptr<leveldb::Iterator> it(
-      resource_map_->NewIterator(leveldb::ReadOptions()));
-  return make_scoped_ptr(new CacheEntryIterator(it.Pass()));
-}
-
 ResourceMetadataStorage::RecoveredCacheInfo::RecoveredCacheInfo()
     : is_dirty(false) {}
 

chrome/browser/chromeos/drive/resource_metadata_storage.h

@@ -166,18 +166,9 @@ class ResourceMetadataStorage {
   FileError GetChildren(const std::string& parent_id,
                         std::vector<std::string>* children);
 
-  // Puts the cache entry to this storage.
-  FileError PutCacheEntry(const std::string& id, const FileCacheEntry& entry);
-
   // Gets a cache entry stored in this storage.
   FileError GetCacheEntry(const std::string& id, FileCacheEntry* out_entry);
 
-  // Removes a cache entry from this storage.
-  FileError RemoveCacheEntry(const std::string& id);
-
-  // Returns an object to iterate over cache entries stored in this storage.
-  scoped_ptr<CacheEntryIterator> GetCacheEntryIterator();
-
   // Returns the local ID associated with the given resource ID.
   FileError GetIdByResourceId(const std::string& resource_id,
                               std::string* out_id);

chrome/browser/chromeos/drive/resource_metadata_storage_unittest.cc

@@ -156,14 +156,6 @@ TEST_F(ResourceMetadataStorageTest, Iterator) {
     EXPECT_EQ(FILE_ERROR_OK, storage_->PutEntry(entry));
   }
 
-  // Insert some cache entries.
-  std::map<std::string, FileCacheEntry> cache_entries;
-  cache_entries[keys[0]].set_md5("aaaaaa");
-  cache_entries[keys[1]].set_md5("bbbbbb");
-  for (std::map<std::string, FileCacheEntry>::iterator it =
-           cache_entries.begin(); it != cache_entries.end(); ++it)
-    EXPECT_EQ(FILE_ERROR_OK, storage_->PutCacheEntry(it->first, it->second));
-
   // Iterate and check the result.
   std::map<std::string, ResourceEntry> found_entries;
   scoped_ptr<ResourceMetadataStorage::Iterator> it = storage_->GetIterator();
@@ -179,72 +171,6 @@ TEST_F(ResourceMetadataStorageTest, Iterator) {
     EXPECT_EQ(1U, found_entries.count(keys[i]));
 }
 
-TEST_F(ResourceMetadataStorageTest, PutCacheEntry) {
-  FileCacheEntry entry;
-  const std::string key1 = "abcdefg";
-  const std::string key2 = "abcd";
-  const std::string md5_1 = "foo";
-  const std::string md5_2 = "bar";
-
-  // Put cache entries.
-  entry.set_md5(md5_1);
-  EXPECT_EQ(FILE_ERROR_OK, storage_->PutCacheEntry(key1, entry));
-  entry.set_md5(md5_2);
-  EXPECT_EQ(FILE_ERROR_OK, storage_->PutCacheEntry(key2, entry));
-
-  // Get cache entires.
-  EXPECT_EQ(FILE_ERROR_OK, storage_->GetCacheEntry(key1, &entry));
-  EXPECT_EQ(md5_1, entry.md5());
-  EXPECT_EQ(FILE_ERROR_OK, storage_->GetCacheEntry(key2, &entry));
-  EXPECT_EQ(md5_2, entry.md5());
-
-  // Remove cache entries.
-  EXPECT_EQ(FILE_ERROR_OK, storage_->RemoveCacheEntry(key1));
-  EXPECT_EQ(FILE_ERROR_NOT_FOUND, storage_->GetCacheEntry(key1, &entry));
-
-  EXPECT_EQ(FILE_ERROR_OK, storage_->RemoveCacheEntry(key2));
-  EXPECT_EQ(FILE_ERROR_NOT_FOUND, storage_->GetCacheEntry(key2, &entry));
-}
-
-TEST_F(ResourceMetadataStorageTest, CacheEntryIterator) {
-  // Prepare data.
-  std::map<std::string, FileCacheEntry> entries;
-  FileCacheEntry cache_entry;
-
-  cache_entry.set_md5("aA");
-  entries["entry1"] = cache_entry;
-  cache_entry.set_md5("bB");
-  entries["entry2"] = cache_entry;
-  cache_entry.set_md5("cC");
-  entries["entry3"] = cache_entry;
-  cache_entry.set_md5("dD");
-  entries["entry4"] = cache_entry;
-
-  for (std::map<std::string, FileCacheEntry>::iterator it = entries.begin();
-       it != entries.end(); ++it)
-    EXPECT_EQ(FILE_ERROR_OK, storage_->PutCacheEntry(it->first, it->second));
-
-  // Insert some dummy entries.
-  ResourceEntry entry;
-  entry.set_local_id("entry1");
-  EXPECT_EQ(FILE_ERROR_OK, storage_->PutEntry(entry));
-  entry.set_local_id("entry2");
-  EXPECT_EQ(FILE_ERROR_OK, storage_->PutEntry(entry));
-
-  // Iterate and check the result.
-  scoped_ptr<ResourceMetadataStorage::CacheEntryIterator> it =
-      storage_->GetCacheEntryIterator();
-  ASSERT_TRUE(it);
-  size_t num_entries = 0;
-  for (; !it->IsAtEnd(); it->Advance()) {
-    EXPECT_EQ(1U, entries.count(it->GetID()));
-    EXPECT_EQ(entries[it->GetID()].md5(), it->GetValue().md5());
-    ++num_entries;
-  }
-  EXPECT_FALSE(it->HasError());
-  EXPECT_EQ(entries.size(), num_entries);
-}
-
 TEST_F(ResourceMetadataStorageTest, GetIdByResourceId) {
   const std::string local_id = "local_id";
   const std::string resource_id = "resource_id";
@@ -308,13 +234,6 @@ TEST_F(ResourceMetadataStorageTest, GetChildren) {
     }
   }
 
-  // Put some dummy cache entries.
-  for (size_t i = 0; i < arraysize(parents_id); ++i) {
-    FileCacheEntry cache_entry;
-    EXPECT_EQ(FILE_ERROR_OK,
-              storage_->PutCacheEntry(parents_id[i], cache_entry));
-  }
-
   // Try to get children.
   for (size_t i = 0; i < children_name_id.size(); ++i) {
     std::vector<std::string> children;
@@ -465,8 +384,6 @@ TEST_F(ResourceMetadataStorageTest, IncompatibleDB_Unknown) {
   ResourceEntry entry;
   entry.set_local_id(key1);
   EXPECT_EQ(FILE_ERROR_OK, storage_->PutEntry(entry));
-  FileCacheEntry cache_entry;
-  EXPECT_EQ(FILE_ERROR_OK, storage_->PutCacheEntry(key1, cache_entry));
 
   // Set newer version, upgrade and reopen DB.
   SetDBVersion(ResourceMetadataStorage::kDBVersion + 1);
@@ -483,7 +400,6 @@ TEST_F(ResourceMetadataStorageTest, IncompatibleDB_Unknown) {
             storage_->GetLargestChangestamp(&largest_changestamp));
   EXPECT_EQ(0, largest_changestamp);
   EXPECT_EQ(FILE_ERROR_NOT_FOUND, storage_->GetEntry(key1, &entry));
-  EXPECT_EQ(FILE_ERROR_NOT_FOUND, storage_->GetCacheEntry(key1, &cache_entry));
 }
 
 TEST_F(ResourceMetadataStorageTest, WrongPath) {
@@ -498,19 +414,12 @@ TEST_F(ResourceMetadataStorageTest, WrongPath) {
 }
 
 TEST_F(ResourceMetadataStorageTest, RecoverCacheEntriesFromTrashedResourceMap) {
-  // Put some cache entries.
-  FileCacheEntry cache_entry;
-  cache_entry.set_md5("md5_foo");
-  EXPECT_EQ(FILE_ERROR_OK, storage_->PutCacheEntry("id_foo", cache_entry));
-  cache_entry.set_md5("md5_bar");
-  cache_entry.set_is_dirty(true);
-  EXPECT_EQ(FILE_ERROR_OK, storage_->PutCacheEntry("id_bar", cache_entry));
-
   // Put entry with id_foo.
   ResourceEntry entry;
   entry.set_local_id("id_foo");
   entry.set_base_name("foo");
   entry.set_title("foo");
+  entry.mutable_file_specific_info()->mutable_cache_state()->set_md5("md5_foo");
   EXPECT_EQ(FILE_ERROR_OK, storage_->PutEntry(entry));
 
   // Put entry with id_bar as a id_foo's child.
@@ -518,6 +427,8 @@ TEST_F(ResourceMetadataStorageTest, RecoverCacheEntriesFromTrashedResourceMap) {
   entry.set_parent_local_id("id_foo");
   entry.set_base_name("bar");
   entry.set_title("bar");
+  entry.mutable_file_specific_info()->mutable_cache_state()->set_md5("md5_bar");
+  entry.mutable_file_specific_info()->mutable_cache_state()->set_is_dirty(true);
   EXPECT_EQ(FILE_ERROR_OK, storage_->PutEntry(entry));
 
   // Remove parent-child relationship to make the DB invalid.
@@ -596,11 +507,6 @@ TEST_F(ResourceMetadataStorageTest, CheckValidity) {
   PutChild(key2, name3, key3);
   EXPECT_TRUE(CheckValidity());
 
-  // Add some cache entries.
-  FileCacheEntry cache_entry;
-  EXPECT_EQ(FILE_ERROR_OK, storage_->PutCacheEntry(key1, cache_entry));
-  EXPECT_EQ(FILE_ERROR_OK, storage_->PutCacheEntry(key2, cache_entry));
-
   // Remove key2.
   RemoveChild(key1, name2);
   EXPECT_FALSE(CheckValidity());

chrome/browser/chromeos/drive/resource_metadata_unittest.cc

@@ -352,6 +352,35 @@ TEST_F(ResourceMetadataTest, RefreshEntry_ResourceIDCheck) {
             resource_metadata_->RefreshEntry(new_entry));
 }
 
+TEST_F(ResourceMetadataTest, RefreshEntry_DoNotOverwriteCacheState) {
+  ResourceEntry entry;
+  EXPECT_EQ(FILE_ERROR_OK, resource_metadata_->GetResourceEntryByPath(
+      base::FilePath::FromUTF8Unsafe("drive/root/dir1/file4"), &entry));
+
+  // Try to set MD5 with RefreshEntry.
+  entry.mutable_file_specific_info()->mutable_cache_state()->set_md5("md5");
+  EXPECT_EQ(FILE_ERROR_OK, resource_metadata_->RefreshEntry(entry));
+
+  // Cache state is unchanged.
+  EXPECT_EQ(FILE_ERROR_OK, resource_metadata_->GetResourceEntryByPath(
+      base::FilePath::FromUTF8Unsafe("drive/root/dir1/file4"), &entry));
+  EXPECT_TRUE(entry.file_specific_info().cache_state().md5().empty());
+
+  // Pin the file.
+  EXPECT_EQ(FILE_ERROR_OK, cache_->Pin(entry.local_id()));
+
+  // Try to clear the cache state with RefreshEntry.
+  EXPECT_EQ(FILE_ERROR_OK, resource_metadata_->GetResourceEntryByPath(
+      base::FilePath::FromUTF8Unsafe("drive/root/dir1/file4"), &entry));
+  entry.mutable_file_specific_info()->clear_cache_state();
+  EXPECT_EQ(FILE_ERROR_OK, resource_metadata_->RefreshEntry(entry));
+
+  // Cache state is not cleared.
+  EXPECT_EQ(FILE_ERROR_OK, resource_metadata_->GetResourceEntryByPath(
+      base::FilePath::FromUTF8Unsafe("drive/root/dir1/file4"), &entry));
+  EXPECT_TRUE(entry.file_specific_info().cache_state().is_pinned());
+}
+
 TEST_F(ResourceMetadataTest, GetSubDirectoriesRecursively) {
   std::set<base::FilePath> sub_directories;