drive: Do not clear dirty bit if someone is writing to the file

Keeping the cache entry dirty to make sure that the edited file will eventually be uploaded.

BUG=326054
TEST=unit_tests

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

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

chrome/browser/chromeos/drive/file_system/update_operation.cc

@@ -95,7 +95,9 @@ FileError UpdateFileLocalState(
   if (drive_file_path->empty())
     return FILE_ERROR_NOT_FOUND;
 
-  // Clear the dirty bit if we have updated an existing file.
+  // Do not clear the dirty bit if someone is writing to the file.
+  if (cache->IsOpenedForWrite(local_id))
+    return FILE_ERROR_OK;
   return cache->ClearDirty(local_id, entry.file_specific_info().md5());
 }
 

chrome/browser/chromeos/drive/file_system/update_operation_unittest.cc

@@ -5,6 +5,8 @@
 #include "chrome/browser/chromeos/drive/file_system/update_operation.h"
 
 #include "base/callback_helpers.h"
+#include "base/file_util.h"
+#include "base/md5.h"
 #include "base/task_runner_util.h"
 #include "chrome/browser/chromeos/drive/file_cache.h"
 #include "chrome/browser/chromeos/drive/file_system/operation_test_base.h"
@@ -20,72 +22,68 @@ namespace file_system {
 class UpdateOperationTest : public OperationTestBase {
  protected:
   virtual void SetUp() OVERRIDE {
-   OperationTestBase::SetUp();
-   operation_.reset(new UpdateOperation(blocking_task_runner(),
-                                        observer(),
-                                        scheduler(),
-                                        metadata(),
-                                        cache()));
- }
-
- scoped_ptr<UpdateOperation> operation_;
+    OperationTestBase::SetUp();
+    operation_.reset(new UpdateOperation(blocking_task_runner(),
+                                         observer(),
+                                         scheduler(),
+                                         metadata(),
+                                         cache()));
+  }
+
+  // Stores |content| to the cache and mark it as dirty.
+  FileError StoreAndMarkDirty(const std::string& local_id,
+                              const std::string& content) {
+    base::FilePath path;
+    if (!base::CreateTemporaryFileInDir(temp_dir(), &path) ||
+        !google_apis::test_util::WriteStringToFile(path, content))
+      return FILE_ERROR_FAILED;
+
+    // Store the file to cache.
+    FileError error = FILE_ERROR_FAILED;
+    base::PostTaskAndReplyWithResult(
+        blocking_task_runner(),
+        FROM_HERE,
+        base::Bind(&internal::FileCache::Store,
+                   base::Unretained(cache()),
+                   local_id, base::MD5String(content), path,
+                   internal::FileCache::FILE_OPERATION_COPY),
+        google_apis::test_util::CreateCopyResultCallback(&error));
+    test_util::RunBlockingPoolTask();
+    if (error != FILE_ERROR_OK)
+      return error;
+
+    // Add the dirty bit.
+    error = FILE_ERROR_FAILED;
+    scoped_ptr<base::ScopedClosureRunner> file_closer;
+    base::PostTaskAndReplyWithResult(
+        blocking_task_runner(),
+        FROM_HERE,
+        base::Bind(&internal::FileCache::OpenForWrite,
+                   base::Unretained(cache()),
+                   local_id,
+                   &file_closer),
+        google_apis::test_util::CreateCopyResultCallback(&error));
+    test_util::RunBlockingPoolTask();
+    return error;
+  }
+
+  scoped_ptr<UpdateOperation> operation_;
 };
 
-TEST_F(UpdateOperationTest, UpdateFileByLocalId_PersistentFile) {
+TEST_F(UpdateOperationTest, UpdateFileByLocalId) {
   const base::FilePath kFilePath(FILE_PATH_LITERAL("drive/root/File 1.txt"));
   const std::string kResourceId("file:2_file_resource_id");
-  const std::string kMd5("3b4382ebefec6e743578c76bbd0575ce");
-
-  const base::FilePath kTestFile = temp_dir().Append(FILE_PATH_LITERAL("foo"));
-  const std::string kTestFileContent = "I'm being uploaded! Yay!";
-  google_apis::test_util::WriteStringToFile(kTestFile, kTestFileContent);
 
   const std::string local_id = GetLocalId(kFilePath);
   EXPECT_FALSE(local_id.empty());
 
-  // Pin the file so it'll be store in "persistent" directory.
-  FileError error = FILE_ERROR_FAILED;
-  base::PostTaskAndReplyWithResult(
-      blocking_task_runner(),
-      FROM_HERE,
-      base::Bind(&internal::FileCache::Pin,
-                 base::Unretained(cache()),
-                 local_id),
-      google_apis::test_util::CreateCopyResultCallback(&error));
-  test_util::RunBlockingPoolTask();
-  EXPECT_EQ(FILE_ERROR_OK, error);
-
-  // First store a file to cache.
-  error = FILE_ERROR_FAILED;
-  base::PostTaskAndReplyWithResult(
-      blocking_task_runner(),
-      FROM_HERE,
-      base::Bind(&internal::FileCache::Store,
-                 base::Unretained(cache()),
-                 local_id, kMd5, kTestFile,
-                 internal::FileCache::FILE_OPERATION_COPY),
-      google_apis::test_util::CreateCopyResultCallback(&error));
-  test_util::RunBlockingPoolTask();
-  EXPECT_EQ(FILE_ERROR_OK, error);
-
-  // Add the dirty bit.
-  error = FILE_ERROR_FAILED;
-  scoped_ptr<base::ScopedClosureRunner> file_closer;
-  base::PostTaskAndReplyWithResult(
-      blocking_task_runner(),
-      FROM_HERE,
-      base::Bind(&internal::FileCache::OpenForWrite,
-                 base::Unretained(cache()),
-                 local_id,
-                 &file_closer),
-      google_apis::test_util::CreateCopyResultCallback(&error));
-  test_util::RunBlockingPoolTask();
-  EXPECT_EQ(FILE_ERROR_OK, error);
+  const std::string kTestFileContent = "I'm being uploaded! Yay!";
+  EXPECT_EQ(FILE_ERROR_OK, StoreAndMarkDirty(local_id, kTestFileContent));
 
   int64 original_changestamp = fake_service()->largest_changestamp();
 
   // The callback will be called upon completion of UpdateFileByLocalId().
-  error = FILE_ERROR_FAILED;
+  FileError error = FILE_ERROR_FAILED;
   operation_->UpdateFileByLocalId(
       local_id,
       ClientContext(USER_INITIATED),
@@ -139,46 +137,17 @@ TEST_F(UpdateOperationTest, UpdateFileByLocalId_NonexistentFile) {
 TEST_F(UpdateOperationTest, UpdateFileByLocalId_Md5) {
   const base::FilePath kFilePath(FILE_PATH_LITERAL("drive/root/File 1.txt"));
   const std::string kResourceId("file:2_file_resource_id");
-  const std::string kMd5("3b4382ebefec6e743578c76bbd0575ce");
-
-  const base::FilePath kTestFile = temp_dir().Append(FILE_PATH_LITERAL("foo"));
-  const std::string kTestFileContent = "I'm being uploaded! Yay!";
-  google_apis::test_util::WriteStringToFile(kTestFile, kTestFileContent);
 
   const std::string local_id = GetLocalId(kFilePath);
   EXPECT_FALSE(local_id.empty());
 
-  // First store a file to cache.
-  FileError error = FILE_ERROR_FAILED;
-  base::PostTaskAndReplyWithResult(
-      blocking_task_runner(),
-      FROM_HERE,
-      base::Bind(&internal::FileCache::Store,
-                 base::Unretained(cache()),
-                 local_id, kMd5, kTestFile,
-                 internal::FileCache::FILE_OPERATION_COPY),
-      google_apis::test_util::CreateCopyResultCallback(&error));
-  test_util::RunBlockingPoolTask();
-  EXPECT_EQ(FILE_ERROR_OK, error);
-
-  // Add the dirty bit.
-  error = FILE_ERROR_FAILED;
-  scoped_ptr<base::ScopedClosureRunner> file_closer;
-  base::PostTaskAndReplyWithResult(
-      blocking_task_runner(),
-      FROM_HERE,
-      base::Bind(&internal::FileCache::OpenForWrite,
-                 base::Unretained(cache()),
-                 local_id,
-                 &file_closer),
-      google_apis::test_util::CreateCopyResultCallback(&error));
-  test_util::RunBlockingPoolTask();
-  EXPECT_EQ(FILE_ERROR_OK, error);
+  const std::string kTestFileContent = "I'm being uploaded! Yay!";
+  EXPECT_EQ(FILE_ERROR_OK, StoreAndMarkDirty(local_id, kTestFileContent));
 
   int64 original_changestamp = fake_service()->largest_changestamp();
 
   // The callback will be called upon completion of UpdateFileByLocalId().
-  error = FILE_ERROR_FAILED;
+  FileError error = FILE_ERROR_FAILED;
   operation_->UpdateFileByLocalId(
       local_id,
       ClientContext(USER_INITIATED),
@@ -218,6 +187,7 @@ TEST_F(UpdateOperationTest, UpdateFileByLocalId_Md5) {
   EXPECT_FALSE(cache_entry.is_dirty());
 
   // Again mark the cache file dirty.
+  scoped_ptr<base::ScopedClosureRunner> file_closer;
   error = FILE_ERROR_FAILED;
   base::PostTaskAndReplyWithResult(
       blocking_task_runner(),
@@ -289,5 +259,81 @@ TEST_F(UpdateOperationTest, UpdateFileByLocalId_Md5) {
   EXPECT_LE(original_changestamp, fake_service()->largest_changestamp());
 }
 
+TEST_F(UpdateOperationTest, UpdateFileByLocalId_OpenedForWrite) {
+  const base::FilePath kFilePath(FILE_PATH_LITERAL("drive/root/File 1.txt"));
+  const std::string kResourceId("file:2_file_resource_id");
+
+  const std::string local_id = GetLocalId(kFilePath);
+  EXPECT_FALSE(local_id.empty());
+
+  const std::string kTestFileContent = "I'm being uploaded! Yay!";
+  EXPECT_EQ(FILE_ERROR_OK, StoreAndMarkDirty(local_id, kTestFileContent));
+
+  // Emulate a situation where someone is writing to the file.
+  scoped_ptr<base::ScopedClosureRunner> file_closer;
+  FileError error = FILE_ERROR_FAILED;
+  base::PostTaskAndReplyWithResult(
+      blocking_task_runner(),
+      FROM_HERE,
+      base::Bind(&internal::FileCache::OpenForWrite,
+                 base::Unretained(cache()),
+                 local_id,
+                 &file_closer),
+      google_apis::test_util::CreateCopyResultCallback(&error));
+  test_util::RunBlockingPoolTask();
+  EXPECT_EQ(FILE_ERROR_OK, error);
+
+  // Update. This should not clear the dirty bit.
+  error = FILE_ERROR_FAILED;
+  operation_->UpdateFileByLocalId(
+      local_id,
+      ClientContext(USER_INITIATED),
+      UpdateOperation::RUN_CONTENT_CHECK,
+      google_apis::test_util::CreateCopyResultCallback(&error));
+  test_util::RunBlockingPoolTask();
+  EXPECT_EQ(FILE_ERROR_OK, error);
+
+  // Make sure that the cache is still dirty.
+  bool success = false;
+  FileCacheEntry cache_entry;
+  base::PostTaskAndReplyWithResult(
+      blocking_task_runner(),
+      FROM_HERE,
+      base::Bind(&internal::FileCache::GetCacheEntry,
+                 base::Unretained(cache()),
+                 local_id,
+                 &cache_entry),
+      google_apis::test_util::CreateCopyResultCallback(&success));
+  test_util::RunBlockingPoolTask();
+  EXPECT_TRUE(success);
+  EXPECT_TRUE(cache_entry.is_dirty());
+
+  // Close the file.
+  file_closer.reset();
+
+  // Update. This should clear the dirty bit.
+  error = FILE_ERROR_FAILED;
+  operation_->UpdateFileByLocalId(
+      local_id,
+      ClientContext(USER_INITIATED),
+      UpdateOperation::RUN_CONTENT_CHECK,
+      google_apis::test_util::CreateCopyResultCallback(&error));
+  test_util::RunBlockingPoolTask();
+  EXPECT_EQ(FILE_ERROR_OK, error);
+
+  // Make sure that the cache is no longer dirty.
+  base::PostTaskAndReplyWithResult(
+      blocking_task_runner(),
+      FROM_HERE,
+      base::Bind(&internal::FileCache::GetCacheEntry,
+                 base::Unretained(cache()),
+                 local_id,
+                 &cache_entry),
+      google_apis::test_util::CreateCopyResultCallback(&success));
+  test_util::RunBlockingPoolTask();
+  EXPECT_TRUE(success);
+  EXPECT_FALSE(cache_entry.is_dirty());
+}
+
 }  // namespace file_system
 }  // namespace drive