SyncFS: Introduce RemoteSyncOperationResolver for directory operation support

chrome/browser/sync_file_system/remote_sync_operation_resolver.cc

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/sync_file_system/remote_sync_operation_resolver.h"
+
+#include "base/logging.h"
+
+namespace sync_file_system {
+
+namespace {
+
+bool IsValidCombination(const FileChangeList& local_changes,
+                        SyncFileType local_file_type) {
+  if (local_file_type == SYNC_FILE_TYPE_UNKNOWN ||

[kinuko, 2013/04/25 08:34:10]

I think this one can be probably more improved. Looks like we have really many
common non-valid cases.

I'd write like following (have tried this in my local env):

bool IsValidCombination(const FileChangeList& local_changes,
                        SyncFileType local_file_type,
                        bool is_conflicting) {
  if (local_changes.empty()) {
    // We never leave directory or non-existing entry in a conflicting state.
    return !(is_conflicting && local_file_type != SYNC_FILE_TYPE_FILE);
  }
  switch (local_file_type) {
    case SYNC_FILE_TYPE_UNKNOWN:
      return local_changes.back().IsDelete();
    case SYNC_FILE_TYPE_FILE:
    case SYNC_FILE_TYPE_DIRECTORY:
      return !local_changes.back().IsDelete() &&
             local_changes.back().file_type() == local_file_type;
  }
  return false;
}

Then we should be able to get rid of most of if conditionals in the sub-methods.
 And if it makes some tests fail (it actually does), the test expectations could
be wrong.

[nhiroki, 2013/04/25 10:29:27]

Done! I found some wrong expectations in the design doc. I fixed the doc and
reflected it in this patch.

+      local_changes.empty() ||
+      local_file_type == local_changes.back().file_type())
+    return true;
+  return false;
+}
+
+}  // namespace
+
+RemoteSyncOperationType
+RemoteSyncOperationResolver::Resolve(const FileChange& remote_file_change,
+                                     const FileChangeList& local_changes,
+                                     SyncFileType local_file_type,
+                                     bool is_conflicting) {
+  DCHECK(IsValidCombination(local_changes, local_file_type));

[kinuko, 2013/04/25 08:34:10]

Hmm, maybe this one's a bit too verbose... sorry could you remove this line? (We
can DCHECK to see if Resolve's return value is not FAIL)

[nhiroki, 2013/04/25 10:29:27]

Done.

+
+  switch (remote_file_change.change()) {
+    case FileChange::FILE_CHANGE_ADD_OR_UPDATE:
+      switch (remote_file_change.file_type()) {
+        case SYNC_FILE_TYPE_FILE:
+          return is_conflicting
+              ? ResolveForAddOrUpdateFileInConflict(local_changes,
+                                                    local_file_type)
+              : ResolveForAddOrUpdateFile(local_changes, local_file_type);
+        case SYNC_FILE_TYPE_DIRECTORY:
+          return is_conflicting
+              ? ResolveForAddDirectoryInConflict(local_changes, local_file_type)
+              : ResolveForAddDirectory(local_changes, local_file_type);
+        case SYNC_FILE_TYPE_UNKNOWN:
+          NOTREACHED();
+          return REMOTE_SYNC_OPERATION_FAIL;
+      }
+      break;
+    case FileChange::FILE_CHANGE_DELETE:
+      switch (remote_file_change.file_type()) {
+        case SYNC_FILE_TYPE_FILE:
+          return is_conflicting
+              ? ResolveForDeleteFileInConflict(local_changes, local_file_type)
+              : ResolveForDeleteFile(local_changes, local_file_type);
+        case SYNC_FILE_TYPE_DIRECTORY:
+          return is_conflicting
+              ? ResolveForDeleteDirectoryInConflict(local_changes,
+                                                    local_file_type)
+              : ResolveForDeleteDirectory(local_changes, local_file_type);
+        case SYNC_FILE_TYPE_UNKNOWN:
+          NOTREACHED();
+          return REMOTE_SYNC_OPERATION_FAIL;
+      }
+      break;
+  }
+  NOTREACHED();
+  return REMOTE_SYNC_OPERATION_FAIL;
+}
+
+RemoteSyncOperationType
+RemoteSyncOperationResolver::ResolveForAddOrUpdateFile(
+    const FileChangeList& local_changes,
+    SyncFileType local_file_type) {
+  // Invalid combination should never happen.
+  if (!IsValidCombination(local_changes, local_file_type))
+    return REMOTE_SYNC_OPERATION_FAIL;
+
+  switch (local_file_type) {
+    case SYNC_FILE_TYPE_UNKNOWN:
+      if (local_changes.empty() || local_changes.back().IsDelete())
+        return REMOTE_SYNC_OPERATION_ADD_FILE;
+      break;
+    case SYNC_FILE_TYPE_FILE:
+      if (local_changes.empty())
+        return REMOTE_SYNC_OPERATION_UPDATE_FILE;
+      if (local_changes.back().IsAddOrUpdate())
+        return REMOTE_SYNC_OPERATION_CONFLICT;
+      break;
+    case SYNC_FILE_TYPE_DIRECTORY:
+      if (local_changes.empty() || local_changes.back().IsAddOrUpdate())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL;

[kinuko, 2013/04/25 08:34:10]

Can we add a comment:
// Currently we always prioritize directories over files.

[nhiroki, 2013/04/25 10:29:27]

Done.

+      break;
+  }
+  return REMOTE_SYNC_OPERATION_FAIL;
+}
+
+RemoteSyncOperationType
+RemoteSyncOperationResolver::ResolveForAddOrUpdateFileInConflict(
+    const FileChangeList& local_changes,
+    SyncFileType local_file_type) {
+  // Invalid combination should never happen.
+  if (!IsValidCombination(local_changes, local_file_type))
+    return REMOTE_SYNC_OPERATION_FAIL;
+
+  switch (local_file_type) {
+    case SYNC_FILE_TYPE_UNKNOWN:
+      if (!local_changes.empty() && local_changes.back().IsDelete())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_REMOTE;
+      break;
+    case SYNC_FILE_TYPE_FILE:
+      if (local_changes.empty() || local_changes.back().IsAddOrUpdate())
+        return REMOTE_SYNC_OPERATION_CONFLICT;
+      break;
+    case SYNC_FILE_TYPE_DIRECTORY:
+      if (local_changes.empty() || local_changes.back().IsAddOrUpdate())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL;
+      break;
+  }
+  return REMOTE_SYNC_OPERATION_FAIL;
+}
+
+RemoteSyncOperationType
+RemoteSyncOperationResolver::ResolveForAddDirectory(
+    const FileChangeList& local_changes,
+    SyncFileType local_file_type) {
+  // Invalid combination should never happen.
+  if (!IsValidCombination(local_changes, local_file_type))
+    return REMOTE_SYNC_OPERATION_FAIL;
+
+  switch (local_file_type) {
+    case SYNC_FILE_TYPE_UNKNOWN:
+      if (local_changes.empty())
+        return REMOTE_SYNC_OPERATION_ADD_DIRECTORY;
+      if (local_changes.back().IsDelete())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_REMOTE;
+      break;
+    case SYNC_FILE_TYPE_FILE:
+      if (local_changes.empty() || local_changes.back().IsAddOrUpdate())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_REMOTE;
+      break;
+    case SYNC_FILE_TYPE_DIRECTORY:
+      if (local_changes.empty() || local_changes.back().IsAddOrUpdate())
+        return REMOTE_SYNC_OPERATION_NONE;
+      break;
+  }
+  return REMOTE_SYNC_OPERATION_FAIL;
+}
+
+RemoteSyncOperationType
+RemoteSyncOperationResolver::ResolveForAddDirectoryInConflict(
+    const FileChangeList& local_changes,
+    SyncFileType local_file_type) {
+  // Invalid combination should never happen.
+  if (!IsValidCombination(local_changes, local_file_type))
+    return REMOTE_SYNC_OPERATION_FAIL;
+
+  switch (local_file_type) {
+    case SYNC_FILE_TYPE_UNKNOWN:
+      if (!local_changes.empty() && local_changes.back().IsDelete())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_REMOTE;
+      break;
+    case SYNC_FILE_TYPE_FILE:
+      if (local_changes.empty() || local_changes.back().IsAddOrUpdate())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_REMOTE;
+      break;
+    case SYNC_FILE_TYPE_DIRECTORY:
+      if (!local_changes.empty() && local_changes.back().IsAddOrUpdate())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL;
+      break;
+  }
+  return REMOTE_SYNC_OPERATION_FAIL;
+}
+
+RemoteSyncOperationType
+RemoteSyncOperationResolver::ResolveForDeleteFile(
+    const FileChangeList& local_changes,
+    SyncFileType local_file_type) {
+  // Invalid combination should never happen.
+  if (!IsValidCombination(local_changes, local_file_type))
+    return REMOTE_SYNC_OPERATION_FAIL;
+
+  switch (local_file_type) {
+    case SYNC_FILE_TYPE_UNKNOWN:
+      return REMOTE_SYNC_OPERATION_DELETE_METADATA;
+    case SYNC_FILE_TYPE_FILE:
+      if (local_changes.empty())
+        return REMOTE_SYNC_OPERATION_DELETE_FILE;
+      if (local_changes.back().IsAddOrUpdate())
+        return REMOTE_SYNC_OPERATION_NONE;
+      break;
+    case SYNC_FILE_TYPE_DIRECTORY:
+      if (local_changes.empty() || local_changes.back().IsAddOrUpdate())
+        return REMOTE_SYNC_OPERATION_NONE;
+      break;
+  }
+  return REMOTE_SYNC_OPERATION_FAIL;
+}
+
+RemoteSyncOperationType
+RemoteSyncOperationResolver::ResolveForDeleteFileInConflict(
+    const FileChangeList& local_changes,
+    SyncFileType local_file_type) {
+  // Invalid combination should never happen.
+  if (!IsValidCombination(local_changes, local_file_type))
+    return REMOTE_SYNC_OPERATION_FAIL;
+
+  switch (local_file_type) {
+    case SYNC_FILE_TYPE_UNKNOWN:
+      return REMOTE_SYNC_OPERATION_DELETE_METADATA;
+    case SYNC_FILE_TYPE_FILE:
+      if (local_changes.empty() || local_changes.back().IsAddOrUpdate())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL;
+      break;
+    case SYNC_FILE_TYPE_DIRECTORY:
+      if (local_changes.empty() || local_changes.back().IsAddOrUpdate())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL;
+      break;
+  }
+  return REMOTE_SYNC_OPERATION_FAIL;
+}
+
+RemoteSyncOperationType
+RemoteSyncOperationResolver::ResolveForDeleteDirectory(
+    const FileChangeList& local_changes,
+    SyncFileType local_file_type) {
+  // Invalid combination should never happen.
+  if (!IsValidCombination(local_changes, local_file_type))
+    return REMOTE_SYNC_OPERATION_FAIL;
+
+  switch (local_file_type) {
+    case SYNC_FILE_TYPE_UNKNOWN:
+      if (local_changes.empty() || local_changes.back().IsDelete())
+        return REMOTE_SYNC_OPERATION_NONE;
+      break;
+    case SYNC_FILE_TYPE_FILE:
+      if (local_changes.empty() || local_changes.back().IsAddOrUpdate())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL;
+      break;
+    case SYNC_FILE_TYPE_DIRECTORY:
+      if (local_changes.empty())
+        return REMOTE_SYNC_OPERATION_DELETE_DIRECTORY;
+      if (local_changes.back().IsAddOrUpdate())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL;
+      break;
+  }
+  return REMOTE_SYNC_OPERATION_FAIL;
+}
+
+RemoteSyncOperationType
+RemoteSyncOperationResolver::ResolveForDeleteDirectoryInConflict(
+    const FileChangeList& local_changes,
+    SyncFileType local_file_type) {
+  // Invalid combination should never happen.
+  if (!IsValidCombination(local_changes, local_file_type))
+    return REMOTE_SYNC_OPERATION_FAIL;
+
+  switch (local_file_type) {
+    case SYNC_FILE_TYPE_UNKNOWN:
+      return REMOTE_SYNC_OPERATION_DELETE_METADATA;
+    case SYNC_FILE_TYPE_FILE:
+      if (local_changes.empty() || local_changes.back().IsAddOrUpdate())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL;
+      break;
+    case SYNC_FILE_TYPE_DIRECTORY:
+      if (local_changes.empty())
+        return REMOTE_SYNC_OPERATION_DELETE_DIRECTORY;
+      if (local_changes.back().IsAddOrUpdate())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL;
+      break;
+  }
+  return REMOTE_SYNC_OPERATION_FAIL;
+}
+
+}  // namespace sync_file_system