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 {
+
+RemoteSyncOperationResolver::RemoteSyncOperationType
+RemoteSyncOperationResolver::Resolve(const FileChange& remote_file_change,
+                                     const FileChangeList& local_changes,
+                                     SyncFileType local_file_type,
+                                     bool is_conflicting) {
+  if (remote_file_change.IsAddOrUpdate()) {

[kinuko, 2013/04/24 14:54:50]

nit: can we use switch/case instead?

[nhiroki, 2013/04/25 06:54:43]

Done.

+    if (remote_file_change.IsFile())
+      return is_conflicting
+          ? ResolveForAddOrUpdateFileInConflict(local_changes, local_file_type)
+          : ResolveForAddOrUpdateFile(local_changes, local_file_type);
+    DCHECK(remote_file_change.IsDirectory());
+    return is_conflicting
+        ? ResolveForAddDirectoryInConflict(local_changes, local_file_type)
+        : ResolveForAddDirectory(local_changes, local_file_type);
+  }
+
+  if (remote_file_change.IsDelete()) {
+    if (remote_file_change.IsFile())
+      return is_conflicting
+          ? ResolveForDeleteFileInConflict(local_changes, local_file_type)
+          : ResolveForDeleteFile(local_changes, local_file_type);
+    DCHECK(remote_file_change.IsDirectory());
+    return is_conflicting
+        ? ResolveForDeleteDirectoryInConflict(local_changes, local_file_type)
+        : ResolveForDeleteDirectory(local_changes, local_file_type);
+  }
+
+  NOTREACHED();
+  return REMOTE_SYNC_OPERATION_FAIL;
+}
+
+RemoteSyncOperationResolver::RemoteSyncOperationType
+RemoteSyncOperationResolver::ResolveForAddOrUpdateFile(
+    const FileChangeList& local_changes,
+    SyncFileType local_file_type) {
+  switch (local_file_type) {
+    case SYNC_FILE_TYPE_UNKNOWN:
+      if (local_changes.empty() ||
+          local_changes.list().back().IsDelete())

[kinuko, 2013/04/24 14:54:50]

You can add back() method to FileChangeList (we already have front())

[nhiroki, 2013/04/25 06:54:43]

Done.

+        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.list().back().IsAddOrUpdateFile())
+        return REMOTE_SYNC_OPERATION_CONFLICT;

[kinuko, 2013/04/24 14:54:50]

Does it make sense to change line 55-56 to:

DCHECK(local_changes.list().back().IsAddOrUpdate());
DCHECK(local_changes.list().back().IsFile());
return ...

Could other patterns come? Or you wrote this as if-clause because of testing,
i.e. to compare the return value with OPERATION_FAIL?  (I guess the latter is
yes)

Ok... how about having sanity checks like following in Resolve() (where we seem
to be able to have DCHECKs):

DCHECK(local_file_type == SYNC_FILE_TYPE_UNKNOWN ||
       local_changes.empty() ||
       local_file_type == local_changes.back().file_type())
  << local_file_type << " / " << local_changes.DebugString();

Probably we can extract the check part in a static method like:

bool IsValidCombination(local_changes, local_file_type);

and have DCHECK in Resolve() and return REMOTE_SYNC_OPERATION_FAIL at the
beginning of each sub-methods. Then the remaining switch/case body could become
slightly more simple?

[nhiroki, 2013/04/25 06:54:43]

Your idea looks good. It enables us to remove helper functions of FileChange.
Added IsValidCombination().

+      break;
+    case SYNC_FILE_TYPE_DIRECTORY:
+      if (local_changes.empty() ||
+          local_changes.list().back().IsAddDirectory())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL;
+      break;
+  }
+  return REMOTE_SYNC_OPERATION_FAIL;
+}
+
+RemoteSyncOperationResolver::RemoteSyncOperationType
+RemoteSyncOperationResolver::ResolveForAddOrUpdateFileInConflict(
+    const FileChangeList& local_changes,
+    SyncFileType local_file_type) {
+  switch (local_file_type) {
+    case SYNC_FILE_TYPE_UNKNOWN:
+      if (!local_changes.empty() &&
+          local_changes.list().back().IsDelete())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_REMOTE;
+      break;
+    case SYNC_FILE_TYPE_FILE:
+      if (local_changes.empty() ||
+          local_changes.list().back().IsAddOrUpdateFile())
+        return REMOTE_SYNC_OPERATION_CONFLICT;
+      break;
+    case SYNC_FILE_TYPE_DIRECTORY:
+      if (local_changes.empty() ||
+          local_changes.list().back().IsAddDirectory())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL;
+      break;
+  }
+  return REMOTE_SYNC_OPERATION_FAIL;
+}
+
+RemoteSyncOperationResolver::RemoteSyncOperationType
+RemoteSyncOperationResolver::ResolveForAddDirectory(
+    const FileChangeList& local_changes,
+    SyncFileType local_file_type) {
+  switch (local_file_type) {
+    case SYNC_FILE_TYPE_UNKNOWN:
+      if (local_changes.empty())
+        return REMOTE_SYNC_OPERATION_ADD_DIRECTORY;
+      if (local_changes.list().back().IsDelete())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_REMOTE;
+      break;
+    case SYNC_FILE_TYPE_FILE:
+      if (local_changes.empty() ||
+          local_changes.list().back().IsAddOrUpdateFile())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_REMOTE;
+      break;
+    case SYNC_FILE_TYPE_DIRECTORY:
+      if (local_changes.empty() ||
+          local_changes.list().back().IsAddDirectory())
+        return REMOTE_SYNC_OPERATION_NONE;
+      break;
+  }
+  return REMOTE_SYNC_OPERATION_FAIL;
+}
+
+RemoteSyncOperationResolver::RemoteSyncOperationType
+RemoteSyncOperationResolver::ResolveForAddDirectoryInConflict(
+    const FileChangeList& local_changes,
+    SyncFileType local_file_type) {
+  switch (local_file_type) {
+    case SYNC_FILE_TYPE_UNKNOWN:
+      if (!local_changes.empty() && local_changes.list().back().IsDelete())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_REMOTE;
+      break;
+    case SYNC_FILE_TYPE_FILE:
+      if (local_changes.empty() ||
+          local_changes.list().back().IsAddOrUpdateFile())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_REMOTE;
+      break;
+    case SYNC_FILE_TYPE_DIRECTORY:
+      if (!local_changes.empty() &&
+          local_changes.list().back().IsAddDirectory())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL;
+      break;
+  }
+  return REMOTE_SYNC_OPERATION_FAIL;
+}
+
+RemoteSyncOperationResolver::RemoteSyncOperationType
+RemoteSyncOperationResolver::ResolveForDeleteFile(
+    const FileChangeList& local_changes,
+    SyncFileType local_file_type) {
+  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.list().back().IsAddOrUpdateFile())
+        return REMOTE_SYNC_OPERATION_NONE;
+      break;
+    case SYNC_FILE_TYPE_DIRECTORY:
+      if (local_changes.empty() ||
+          local_changes.list().back().IsAddDirectory())
+        return REMOTE_SYNC_OPERATION_NONE;
+      break;
+  }
+  return REMOTE_SYNC_OPERATION_FAIL;
+}
+
+RemoteSyncOperationResolver::RemoteSyncOperationType
+RemoteSyncOperationResolver::ResolveForDeleteFileInConflict(
+    const FileChangeList& local_changes,
+    SyncFileType local_file_type) {
+  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.list().back().IsAddOrUpdateFile())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL;
+      break;
+    case SYNC_FILE_TYPE_DIRECTORY:
+      if (local_changes.empty() ||
+          local_changes.list().back().IsAddDirectory())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL;
+      break;
+  }
+  return REMOTE_SYNC_OPERATION_FAIL;
+}
+
+RemoteSyncOperationResolver::RemoteSyncOperationType
+RemoteSyncOperationResolver::ResolveForDeleteDirectory(
+    const FileChangeList& local_changes,
+    SyncFileType local_file_type) {
+  switch (local_file_type) {
+    case SYNC_FILE_TYPE_UNKNOWN:
+      if (local_changes.empty() ||
+          local_changes.list().back().IsDelete())
+        return REMOTE_SYNC_OPERATION_NONE;
+      break;
+    case SYNC_FILE_TYPE_FILE:
+      if (local_changes.empty() ||
+          local_changes.list().back().IsAddOrUpdateFile())
+        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.list().back().IsAddDirectory())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL;
+      break;
+  }
+  return REMOTE_SYNC_OPERATION_FAIL;
+}
+
+RemoteSyncOperationResolver::RemoteSyncOperationType
+RemoteSyncOperationResolver::ResolveForDeleteDirectoryInConflict(
+    const FileChangeList& local_changes,
+    SyncFileType local_file_type) {
+  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.list().back().IsAddOrUpdateFile())
+        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.list().back().IsAddDirectory())
+        return REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL;
+      break;
+  }
+  return REMOTE_SYNC_OPERATION_FAIL;
+}
+
+}  // namespace sync_file_system