SyncFS: Fix wrong operation resolving in LocalSyncOperationResolver

Current implementation always does ADD_FILE operation even if
you update an existing file since LocalSyncOperationResolver
don't care of local metadata. This CL fixes the resolver behavior.


BUG=none
TEST=unit_tests --gtest_filter=\*LocalSyncOperationResolver\

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

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

4 files changed, 130 insertions, 47 deletions

diff --git a/chrome/browser/sync_file_system/drive_file_sync_service.cc b/chrome/browser/sync_file_system/drive_file_sync_service.cc
index ed50b64..57730f8 100644
--- a/chrome/browser/sync_file_system/drive_file_sync_service.cc
+++ b/chrome/browser/sync_file_system/drive_file_sync_service.cc
@@ -1064,10 +1064,11 @@ void DriveFileSyncService::ApplyLocalChangeInternal(
     param->drive_metadata.set_resource_id(remote_change.resource_id);
 
   LocalSyncOperationType operation =
-      LocalSyncOperationResolver::Resolve(local_file_change,
-                                          has_remote_change,
-                                          remote_change.change,
-                                          drive_metadata.conflicted());
+      LocalSyncOperationResolver::Resolve(
+          local_file_change,
+          has_remote_change,
+          remote_change.change,
+          param->has_drive_metadata ? &drive_metadata : NULL);
 
   DVLOG(1) << "ApplyLocalChange for " << url.DebugString()
            << " local_change:" << local_file_change.DebugString()
diff --git a/chrome/browser/sync_file_system/local_sync_operation_resolver.cc b/chrome/browser/sync_file_system/local_sync_operation_resolver.cc
index 9e95f88..220ccbf 100644
--- a/chrome/browser/sync_file_system/local_sync_operation_resolver.cc
+++ b/chrome/browser/sync_file_system/local_sync_operation_resolver.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/sync_file_system/local_sync_operation_resolver.h"
 
 #include "base/logging.h"
+#include "chrome/browser/sync_file_system/sync_file_system.pb.h"
 
 namespace sync_file_system {
 
@@ -12,11 +13,22 @@ LocalSyncOperationType LocalSyncOperationResolver::Resolve(
     const FileChange& local_file_change,
     bool has_remote_change,
     const FileChange& remote_file_change,
-    bool is_conflicting) {
+    DriveMetadata* drive_metadata) {
   // Invalid combination should never happen.
   if (has_remote_change && remote_file_change.IsTypeUnknown())
     return LOCAL_SYNC_OPERATION_FAIL;
 
+  bool is_conflicting = false;
+  SyncFileType remote_file_type_in_metadata = SYNC_FILE_TYPE_UNKNOWN;
+  if (drive_metadata != NULL) {
+    is_conflicting = drive_metadata->conflicted();
+    if (drive_metadata->type() ==
+        DriveMetadata_ResourceType_RESOURCE_TYPE_FILE)
+      remote_file_type_in_metadata = SYNC_FILE_TYPE_FILE;
+    else
+      remote_file_type_in_metadata = SYNC_FILE_TYPE_DIRECTORY;
+  }
+
   switch (local_file_change.change()) {
     case FileChange::FILE_CHANGE_ADD_OR_UPDATE:
       switch (local_file_change.file_type()) {
@@ -25,12 +37,15 @@ LocalSyncOperationType LocalSyncOperationResolver::Resolve(
               ? ResolveForAddOrUpdateFileInConflict(has_remote_change,
                                                     remote_file_change)
               : ResolveForAddOrUpdateFile(has_remote_change,
-                                          remote_file_change);
+                                          remote_file_change,
+                                          remote_file_type_in_metadata);
         case SYNC_FILE_TYPE_DIRECTORY:
           return is_conflicting
               ? ResolveForAddDirectoryInConflict(has_remote_change,
                                                  remote_file_change)
-              : ResolveForAddDirectory(has_remote_change, remote_file_change);
+              : ResolveForAddDirectory(has_remote_change,
+                                       remote_file_change,
+                                       remote_file_type_in_metadata);
         case SYNC_FILE_TYPE_UNKNOWN:
           NOTREACHED();
           return LOCAL_SYNC_OPERATION_FAIL;
@@ -41,13 +56,16 @@ LocalSyncOperationType LocalSyncOperationResolver::Resolve(
           return is_conflicting
               ? ResolveForDeleteFileInConflict(has_remote_change,
                                                remote_file_change)
-              : ResolveForDeleteFile(has_remote_change, remote_file_change);
+              : ResolveForDeleteFile(has_remote_change,
+                                     remote_file_change,
+                                     remote_file_type_in_metadata);
         case SYNC_FILE_TYPE_DIRECTORY:
           return is_conflicting
               ? ResolveForDeleteDirectoryInConflict(has_remote_change,
                                                     remote_file_change)
               : ResolveForDeleteDirectory(has_remote_change,
-                                          remote_file_change);
+                                          remote_file_change,
+                                          remote_file_type_in_metadata);
         case SYNC_FILE_TYPE_UNKNOWN:
           NOTREACHED();
           return LOCAL_SYNC_OPERATION_FAIL;
@@ -57,12 +75,22 @@ LocalSyncOperationType LocalSyncOperationResolver::Resolve(
   return LOCAL_SYNC_OPERATION_FAIL;
 }
 
-LocalSyncOperationType
-LocalSyncOperationResolver::ResolveForAddOrUpdateFile(
+LocalSyncOperationType LocalSyncOperationResolver::ResolveForAddOrUpdateFile(
     bool has_remote_change,
-    const FileChange& remote_file_change) {
-  if (!has_remote_change)
-    return LOCAL_SYNC_OPERATION_ADD_FILE;
+    const FileChange& remote_file_change,
+    SyncFileType remote_file_type_in_metadata) {
+  if (!has_remote_change) {
+    switch (remote_file_type_in_metadata) {
+      case SYNC_FILE_TYPE_UNKNOWN:
+        // Remote file or directory may not exist.
+        return LOCAL_SYNC_OPERATION_ADD_FILE;
+      case SYNC_FILE_TYPE_FILE:
+        return LOCAL_SYNC_OPERATION_UPDATE_FILE;
+      case SYNC_FILE_TYPE_DIRECTORY:
+        return LOCAL_SYNC_OPERATION_RESOLVE_TO_REMOTE;
+    }
+  }
+
   switch (remote_file_change.change()) {
     case FileChange::FILE_CHANGE_ADD_OR_UPDATE:
       if (remote_file_change.IsFile())
@@ -71,6 +99,7 @@ LocalSyncOperationResolver::ResolveForAddOrUpdateFile(
     case FileChange::FILE_CHANGE_DELETE:
       return LOCAL_SYNC_OPERATION_RESOLVE_TO_LOCAL;
   }
+
   NOTREACHED();
   return LOCAL_SYNC_OPERATION_FAIL;
 }
@@ -93,12 +122,22 @@ LocalSyncOperationResolver::ResolveForAddOrUpdateFileInConflict(
   return LOCAL_SYNC_OPERATION_FAIL;
 }
 
-LocalSyncOperationType
-LocalSyncOperationResolver::ResolveForAddDirectory(
+LocalSyncOperationType LocalSyncOperationResolver::ResolveForAddDirectory(
     bool has_remote_change,
-    const FileChange& remote_file_change) {
-  if (!has_remote_change)
-    return LOCAL_SYNC_OPERATION_ADD_DIRECTORY;
+    const FileChange& remote_file_change,
+    SyncFileType remote_file_type_in_metadata) {
+  if (!has_remote_change) {
+    switch (remote_file_type_in_metadata) {
+      case SYNC_FILE_TYPE_UNKNOWN:
+        // Remote file or directory may not exist.
+        return LOCAL_SYNC_OPERATION_ADD_DIRECTORY;
+      case SYNC_FILE_TYPE_FILE:
+        return LOCAL_SYNC_OPERATION_RESOLVE_TO_LOCAL;
+      case SYNC_FILE_TYPE_DIRECTORY:
+        return LOCAL_SYNC_OPERATION_NONE;
+    }
+  }
+
   switch (remote_file_change.change()) {
     case FileChange::FILE_CHANGE_ADD_OR_UPDATE:
       if (remote_file_change.IsFile())
@@ -109,6 +148,7 @@ LocalSyncOperationResolver::ResolveForAddDirectory(
         return LOCAL_SYNC_OPERATION_ADD_DIRECTORY;
       return LOCAL_SYNC_OPERATION_RESOLVE_TO_LOCAL;
   }
+
   NOTREACHED();
   return LOCAL_SYNC_OPERATION_FAIL;
 }
@@ -120,18 +160,29 @@ LocalSyncOperationResolver::ResolveForAddDirectoryInConflict(
   return LOCAL_SYNC_OPERATION_RESOLVE_TO_LOCAL;
 }
 
-LocalSyncOperationType
-LocalSyncOperationResolver::ResolveForDeleteFile(
+LocalSyncOperationType LocalSyncOperationResolver::ResolveForDeleteFile(
     bool has_remote_change,
-    const FileChange& remote_file_change) {
-  if (!has_remote_change)
-    return LOCAL_SYNC_OPERATION_DELETE_FILE;
+    const FileChange& remote_file_change,
+    SyncFileType remote_file_type_in_metadata) {
+  if (!has_remote_change) {
+    switch (remote_file_type_in_metadata) {
+      case SYNC_FILE_TYPE_UNKNOWN:
+        // Remote file or directory may not exist.
+        return LOCAL_SYNC_OPERATION_NONE;
+      case SYNC_FILE_TYPE_FILE:
+        return LOCAL_SYNC_OPERATION_DELETE_FILE;
+      case SYNC_FILE_TYPE_DIRECTORY:
+        return LOCAL_SYNC_OPERATION_RESOLVE_TO_REMOTE;
+    }
+  }
+
   switch (remote_file_change.change()) {
     case FileChange::FILE_CHANGE_ADD_OR_UPDATE:
       return LOCAL_SYNC_OPERATION_RESOLVE_TO_REMOTE;
     case FileChange::FILE_CHANGE_DELETE:
-      return LOCAL_SYNC_OPERATION_NONE;
+      return LOCAL_SYNC_OPERATION_DELETE_METADATA;
   }
+
   NOTREACHED();
   return LOCAL_SYNC_OPERATION_FAIL;
 }
@@ -152,13 +203,31 @@ LocalSyncOperationResolver::ResolveForDeleteFileInConflict(
   return LOCAL_SYNC_OPERATION_FAIL;
 }
 
-LocalSyncOperationType
-LocalSyncOperationResolver::ResolveForDeleteDirectory(
+LocalSyncOperationType LocalSyncOperationResolver::ResolveForDeleteDirectory(
     bool has_remote_change,
-    const FileChange& remote_file_change) {
-  if (!has_remote_change)
-    return LOCAL_SYNC_OPERATION_DELETE_DIRECTORY;
-  return LOCAL_SYNC_OPERATION_NONE;
+    const FileChange& remote_file_change,
+    SyncFileType remote_file_type_in_metadata) {
+  if (!has_remote_change) {
+    switch (remote_file_type_in_metadata) {
+      case SYNC_FILE_TYPE_UNKNOWN:
+        // Remote file or dircetory may not exist.
+        return LOCAL_SYNC_OPERATION_NONE;
+      case SYNC_FILE_TYPE_FILE:
+        return LOCAL_SYNC_OPERATION_RESOLVE_TO_REMOTE;
+      case SYNC_FILE_TYPE_DIRECTORY:
+        return LOCAL_SYNC_OPERATION_DELETE_DIRECTORY;
+    }
+  }
+
+  switch (remote_file_change.change()) {
+    case FileChange::FILE_CHANGE_ADD_OR_UPDATE:
+      return LOCAL_SYNC_OPERATION_RESOLVE_TO_REMOTE;
+    case FileChange::FILE_CHANGE_DELETE:
+      return LOCAL_SYNC_OPERATION_DELETE_METADATA;
+  }
+
+  NOTREACHED();
+  return LOCAL_SYNC_OPERATION_FAIL;
 }
 
 LocalSyncOperationType
diff --git a/chrome/browser/sync_file_system/local_sync_operation_resolver.h b/chrome/browser/sync_file_system/local_sync_operation_resolver.h
index 0993df3..8a59aca 100644
--- a/chrome/browser/sync_file_system/local_sync_operation_resolver.h
+++ b/chrome/browser/sync_file_system/local_sync_operation_resolver.h
@@ -11,6 +11,7 @@
 
 namespace sync_file_system {
 
+class DriveMetadata;
 class FileChange;
 
 enum LocalSyncOperationType {
@@ -33,30 +34,34 @@ class LocalSyncOperationResolver {
       const FileChange& local_file_change,
       bool has_remote_change,
       const FileChange& remote_file_change,
-      bool is_conflicting);
+      DriveMetadata* drive_metadata);
 
  private:
   static LocalSyncOperationType ResolveForAddOrUpdateFile(
       bool has_remote_change,
-      const FileChange& remote_file_change);
+      const FileChange& remote_file_change,
+      SyncFileType remote_file_type_in_metadata);
   static LocalSyncOperationType ResolveForAddOrUpdateFileInConflict(
       bool has_remote_change,
       const FileChange& remote_file_change);
   static LocalSyncOperationType ResolveForAddDirectory(
       bool has_remote_change,
-      const FileChange& remote_file_change);
+      const FileChange& remote_file_change,
+      SyncFileType remote_file_type_in_metadata);
   static LocalSyncOperationType ResolveForAddDirectoryInConflict(
       bool has_remote_change,
       const FileChange& remote_file_change);
   static LocalSyncOperationType ResolveForDeleteFile(
       bool has_remote_change,
-      const FileChange& remote_file_change);
+      const FileChange& remote_file_change,
+      SyncFileType remote_file_type_in_metadata);
   static LocalSyncOperationType ResolveForDeleteFileInConflict(
       bool has_remote_change,
       const FileChange& remote_file_change);
   static LocalSyncOperationType ResolveForDeleteDirectory(
       bool has_remote_change,
-      const FileChange& remote_file_change);
+      const FileChange& remote_file_change,
+      SyncFileType remote_file_type_in_metadata);
   static LocalSyncOperationType ResolveForDeleteDirectoryInConflict(
       bool has_remote_change,
       const FileChange& remote_file_change);
diff --git a/chrome/browser/sync_file_system/local_sync_operation_resolver_unittest.cc b/chrome/browser/sync_file_system/local_sync_operation_resolver_unittest.cc
index 97f8588..202c8a1 100644
--- a/chrome/browser/sync_file_system/local_sync_operation_resolver_unittest.cc
+++ b/chrome/browser/sync_file_system/local_sync_operation_resolver_unittest.cc
@@ -77,10 +77,12 @@ TEST_F(LocalSyncOperationResolverTest, ResolveForAddOrUpdateFile) {
   std::vector<Input> inputs = CreateInput();
 
   ASSERT_EQ(expected_types.size(), inputs.size());
+  // TODO(nhiroki): Fix inputs so that these tests can cover all cases.
   for (ExpectedTypes::size_type i = 0; i < expected_types.size(); ++i)
     EXPECT_EQ(expected_types[i],
               Resolver::ResolveForAddOrUpdateFile(
-                  inputs[i].has_remote_change, inputs[i].remote_file_change))
+                  inputs[i].has_remote_change, inputs[i].remote_file_change,
+                  SYNC_FILE_TYPE_UNKNOWN))
         << "Case " << i << ": (" << inputs[i].DebugString() << ")";
 }
 
@@ -117,10 +119,12 @@ TEST_F(LocalSyncOperationResolverTest, ResolveForAddDirectory) {
   std::vector<Input> inputs = CreateInput();
 
   ASSERT_EQ(expected_types.size(), inputs.size());
+  // TODO(nhiroki): Fix inputs so that these tests can cover all cases.
   for (ExpectedTypes::size_type i = 0; i < expected_types.size(); ++i)
     EXPECT_EQ(expected_types[i],
               Resolver::ResolveForAddDirectory(
-                  inputs[i].has_remote_change, inputs[i].remote_file_change))
+                  inputs[i].has_remote_change, inputs[i].remote_file_change,
+                  SYNC_FILE_TYPE_UNKNOWN))
         << "Case " << i << ": (" << inputs[i].DebugString() << ")";
 }
 
@@ -146,21 +150,23 @@ TEST_F(LocalSyncOperationResolverTest, ResolveForAddDirectoryInConflict) {
 
 TEST_F(LocalSyncOperationResolverTest, ResolveForDeleteFile) {
   const LocalSyncOperationType kExpectedTypes[] = {
-    LOCAL_SYNC_OPERATION_DELETE_FILE,
+    LOCAL_SYNC_OPERATION_NONE,
     LOCAL_SYNC_OPERATION_RESOLVE_TO_REMOTE,
     LOCAL_SYNC_OPERATION_RESOLVE_TO_REMOTE,
-    LOCAL_SYNC_OPERATION_NONE,
-    LOCAL_SYNC_OPERATION_NONE,
+    LOCAL_SYNC_OPERATION_DELETE_METADATA,
+    LOCAL_SYNC_OPERATION_DELETE_METADATA,
   };
 
   ExpectedTypes expected_types = CreateList(kExpectedTypes);
   std::vector<Input> inputs = CreateInput();
 
   ASSERT_EQ(expected_types.size(), inputs.size());
+  // TODO(nhiroki): Fix inputs so that these tests can cover all cases.
   for (ExpectedTypes::size_type i = 0; i < expected_types.size(); ++i)
     EXPECT_EQ(expected_types[i],
               Resolver::ResolveForDeleteFile(
-                  inputs[i].has_remote_change, inputs[i].remote_file_change))
+                  inputs[i].has_remote_change, inputs[i].remote_file_change,
+                  SYNC_FILE_TYPE_UNKNOWN))
         << "Case " << i << ": (" << inputs[i].DebugString() << ")";
 }
 
@@ -186,21 +192,23 @@ TEST_F(LocalSyncOperationResolverTest, ResolveForDeleteFileInConflict) {
 
 TEST_F(LocalSyncOperationResolverTest, ResolveForDeleteDirectory) {
   const LocalSyncOperationType kExpectedTypes[] = {
-    LOCAL_SYNC_OPERATION_DELETE_DIRECTORY,
-    LOCAL_SYNC_OPERATION_NONE,
-    LOCAL_SYNC_OPERATION_NONE,
-    LOCAL_SYNC_OPERATION_NONE,
     LOCAL_SYNC_OPERATION_NONE,
+    LOCAL_SYNC_OPERATION_RESOLVE_TO_REMOTE,
+    LOCAL_SYNC_OPERATION_RESOLVE_TO_REMOTE,
+    LOCAL_SYNC_OPERATION_DELETE_METADATA,
+    LOCAL_SYNC_OPERATION_DELETE_METADATA,
   };
 
   ExpectedTypes expected_types = CreateList(kExpectedTypes);
   std::vector<Input> inputs = CreateInput();
 
   ASSERT_EQ(expected_types.size(), inputs.size());
+  // TODO(nhiroki): Fix inputs so that these tests can cover all cases.
   for (ExpectedTypes::size_type i = 0; i < expected_types.size(); ++i)
     EXPECT_EQ(expected_types[i],
               Resolver::ResolveForDeleteDirectory(
-                  inputs[i].has_remote_change, inputs[i].remote_file_change))
+                  inputs[i].has_remote_change, inputs[i].remote_file_change,
+                  SYNC_FILE_TYPE_UNKNOWN))
         << "Case " << i << ": (" << inputs[i].DebugString() << ")";
 }