drive: Use modification date for conflict resolution

Add ResourceEntry::modification_date.
Call set_modification_date() in every place which sets metadata_edit_state to DIRTY.
Compare modification_date in ChangeListProcessor.

BUG=363498
TEST=unit_tests

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

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

10 files changed, 61 insertions, 33 deletions

diff --git a/chrome/browser/chromeos/drive/change_list_processor.cc b/chrome/browser/chromeos/drive/change_list_processor.cc
index 5ef384d..ac01e49 100644
--- a/chrome/browser/chromeos/drive/change_list_processor.cc
+++ b/chrome/browser/chromeos/drive/change_list_processor.cc
@@ -54,19 +54,12 @@ class ChangeListToEntryMapUMAStats {
 };
 
 // Returns true if it's OK to overwrite the local entry with the remote one.
-// |modification_date| is the time of the modification whose result is
-// |remote_entry|.
 bool ShouldApplyChange(const ResourceEntry& local_entry,
-                       const ResourceEntry& remote_entry,
-                       const base::Time& modification_date) {
-  // TODO(hashimoto): Implement more sophisticated conflict resolution.
-
-  // If the entry is locally available and is newly created in this change,
-  // No need to overwrite the local entry.
-  base::Time creation_time =
-      base::Time::FromInternalValue(remote_entry.file_info().creation_time());
-  const bool entry_is_new = creation_time == modification_date;
-  return !entry_is_new;
+                       const ResourceEntry& remote_entry) {
+  if (local_entry.metadata_edit_state() == ResourceEntry::CLEAN)
+    return true;
+  return base::Time::FromInternalValue(remote_entry.modification_date()) >
+      base::Time::FromInternalValue(local_entry.modification_date());
 }
 
 }  // namespace
@@ -85,20 +78,16 @@ ChangeList::ChangeList(const google_apis::ResourceList& resource_list)
 
   entries_.resize(resource_list.entries().size());
   parent_resource_ids_.resize(resource_list.entries().size());
-  modification_dates_.resize(resource_list.entries().size());
   size_t entries_index = 0;
   for (size_t i = 0; i < resource_list.entries().size(); ++i) {
     if (ConvertToResourceEntry(*resource_list.entries()[i],
                                &entries_[entries_index],
                                &parent_resource_ids_[entries_index])) {
-      modification_dates_[entries_index] =
-          resource_list.entries()[i]->modification_date();
       ++entries_index;
     }
   }
   entries_.resize(entries_index);
   parent_resource_ids_.resize(entries_index);
-  modification_dates_.resize(entries_index);
 }
 
 ChangeList::~ChangeList() {}
@@ -147,8 +136,6 @@ FileError ChangeListProcessor::Apply(
         if (entry->shared_with_me())
           uma_stats.IncrementNumSharedWithMeEntries();
       }
-      modification_date_map_[entry->resource_id()] =
-          change_list->modification_dates()[i];
       parent_resource_id_map_[entry->resource_id()] =
           change_list->parent_resource_ids()[i];
       entry_map_[entry->resource_id()].Swap(entry);
@@ -326,11 +313,8 @@ FileError ChangeListProcessor::ApplyEntryMap(
 FileError ChangeListProcessor::ApplyEntry(const ResourceEntry& entry) {
   DCHECK(!entry.deleted());
   DCHECK(parent_resource_id_map_.count(entry.resource_id()));
-  DCHECK(modification_date_map_.count(entry.resource_id()));
   const std::string& parent_resource_id =
       parent_resource_id_map_[entry.resource_id()];
-  const base::Time& modification_date =
-      modification_date_map_[entry.resource_id()];
 
   ResourceEntry new_entry(entry);
   FileError error = SetParentLocalIdOfEntry(resource_metadata_, &new_entry,
@@ -348,7 +332,7 @@ FileError ChangeListProcessor::ApplyEntry(const ResourceEntry& entry) {
 
   switch (error) {
     case FILE_ERROR_OK:
-      if (ShouldApplyChange(existing_entry, new_entry, modification_date)) {
+      if (ShouldApplyChange(existing_entry, new_entry)) {
         // Entry exists and needs to be refreshed.
         new_entry.set_local_id(local_id);
         error = resource_metadata_->RefreshEntry(new_entry);
diff --git a/chrome/browser/chromeos/drive/change_list_processor.h b/chrome/browser/chromeos/drive/change_list_processor.h
index 56a8fb3..dfaabc3 100644
--- a/chrome/browser/chromeos/drive/change_list_processor.h
+++ b/chrome/browser/chromeos/drive/change_list_processor.h
@@ -78,12 +78,6 @@ class ChangeList {
   std::vector<std::string>* mutable_parent_resource_ids() {
     return &parent_resource_ids_;
   }
-  const std::vector<base::Time>& modification_dates() const {
-    return modification_dates_;
-  }
-  std::vector<base::Time>* mutable_modification_dates() {
-    return &modification_dates_;
-  }
   const GURL& next_url() const { return next_url_; }
   int64 largest_changestamp() const { return largest_changestamp_; }
 
@@ -94,7 +88,6 @@ class ChangeList {
  private:
   std::vector<ResourceEntry> entries_;
   std::vector<std::string> parent_resource_ids_;
-  std::vector<base::Time> modification_dates_;
   GURL next_url_;
   int64 largest_changestamp_;
 
@@ -140,8 +133,6 @@ class ChangeListProcessor {
       ResourceEntryMap;
   typedef std::map<std::string /* resource_id */,
                    std::string /* parent_resource_id*/> ParentResourceIdMap;
-  typedef std::map<std::string /* resource_id */,
-                   base::Time /* modification_date */> ModificationDateMap;
 
   // Applies the pre-processed metadata from entry_map_ onto the resource
   // metadata. |about_resource| must not be null.
@@ -159,7 +150,6 @@ class ChangeListProcessor {
 
   ResourceEntryMap entry_map_;
   ParentResourceIdMap parent_resource_id_map_;
-  ModificationDateMap modification_date_map_;
   std::set<base::FilePath> changed_dirs_;
 
   DISALLOW_COPY_AND_ASSIGN(ChangeListProcessor);
diff --git a/chrome/browser/chromeos/drive/change_list_processor_unittest.cc b/chrome/browser/chromeos/drive/change_list_processor_unittest.cc
index c5bbc87..daaabc8 100644
--- a/chrome/browser/chromeos/drive/change_list_processor_unittest.cc
+++ b/chrome/browser/chromeos/drive/change_list_processor_unittest.cc
@@ -594,7 +594,6 @@ TEST_F(ChangeListProcessorTest, SharedFilesWithNoParentInFeed) {
   new_file.set_resource_id("new_file_id");
   change_lists[0]->mutable_entries()->push_back(new_file);
   change_lists[0]->mutable_parent_resource_ids()->push_back("nonexisting");
-  change_lists[0]->mutable_modification_dates()->push_back(base::Time());
   change_lists[0]->set_largest_changestamp(kBaseResourceListChangestamp + 1);
 
   std::set<base::FilePath> changed_dirs;
@@ -606,5 +605,50 @@ TEST_F(ChangeListProcessorTest, SharedFilesWithNoParentInFeed) {
       util::GetDriveGrandRootPath().AppendASCII("other/new_file"), &entry));
 }
 
+TEST_F(ChangeListProcessorTest, ModificationDate) {
+  // Prepare metadata.
+  EXPECT_EQ(FILE_ERROR_OK,
+            ApplyFullResourceList(ParseChangeList(kBaseResourceListFile)));
+
+  // Create change lists with a new file.
+  ScopedVector<ChangeList> change_lists;
+  change_lists.push_back(new ChangeList);
+
+  const base::Time now = base::Time::Now();
+  ResourceEntry new_file_remote;
+  new_file_remote.set_title("new_file_remote");
+  new_file_remote.set_resource_id("new_file_id");
+  new_file_remote.set_modification_date(now.ToInternalValue());
+
+  change_lists[0]->mutable_entries()->push_back(new_file_remote);
+  change_lists[0]->mutable_parent_resource_ids()->push_back(kRootId);
+  change_lists[0]->set_largest_changestamp(kBaseResourceListChangestamp + 1);
+
+  // Add the same file locally, but with a different name, a dirty metadata
+  // state, and a newer modification date.
+  ResourceEntry root;
+  EXPECT_EQ(FILE_ERROR_OK, metadata_->GetResourceEntryByPath(
+      util::GetDriveMyDriveRootPath(), &root));
+
+  ResourceEntry new_file_local;
+  new_file_local.set_resource_id(new_file_remote.resource_id());
+  new_file_local.set_parent_local_id(root.local_id());
+  new_file_local.set_title("new_file_local");
+  new_file_local.set_metadata_edit_state(ResourceEntry::DIRTY);
+  new_file_local.set_modification_date(
+      (now + base::TimeDelta::FromSeconds(1)).ToInternalValue());
+  std::string local_id;
+  EXPECT_EQ(FILE_ERROR_OK, metadata_->AddEntry(new_file_local, &local_id));
+
+  // Apply the change.
+  std::set<base::FilePath> changed_dirs;
+  EXPECT_EQ(FILE_ERROR_OK, ApplyChangeList(change_lists.Pass(), &changed_dirs));
+
+  // The change is rejected due to the old modification date.
+  ResourceEntry entry;
+  EXPECT_EQ(FILE_ERROR_OK, metadata_->GetResourceEntryById(local_id, &entry));
+  EXPECT_EQ(new_file_local.title(), entry.title());
+}
+
 }  // namespace internal
 }  // namespace drive
diff --git a/chrome/browser/chromeos/drive/drive.proto b/chrome/browser/chromeos/drive/drive.proto
index 7495077..9d12674 100644
--- a/chrome/browser/chromeos/drive/drive.proto
+++ b/chrome/browser/chromeos/drive/drive.proto
@@ -114,6 +114,9 @@ message ResourceEntry {
 
   // Indicates whether this entry's metadata is edited locally or not.
   optional EditState metadata_edit_state = 16;
+
+  // The time of the last modification.
+  optional int64 modification_date = 18;
 }
 
 // Container for the header part of ResourceMetadata.
diff --git a/chrome/browser/chromeos/drive/file_system/copy_operation.cc b/chrome/browser/chromeos/drive/file_system/copy_operation.cc
index efa9109..3479c1f 100644
--- a/chrome/browser/chromeos/drive/file_system/copy_operation.cc
+++ b/chrome/browser/chromeos/drive/file_system/copy_operation.cc
@@ -132,6 +132,7 @@ FileError TryToCopyLocally(internal::ResourceMetadata* metadata,
   entry.mutable_file_specific_info()->set_content_mime_type(
       params->src_entry.file_specific_info().content_mime_type());
   entry.set_metadata_edit_state(ResourceEntry::DIRTY);
+  entry.set_modification_date(base::Time::Now().ToInternalValue());
   entry.mutable_file_info()->set_last_modified(
       params->preserve_last_modified ?
       params->src_entry.file_info().last_modified() : now);
@@ -260,6 +261,7 @@ FileError LocalWorkForTransferJsonGdocFile(
     entry.set_title(params->new_title);
     entry.set_parent_local_id(params->parent_local_id);
     entry.set_metadata_edit_state(ResourceEntry::DIRTY);
+    entry.set_modification_date(base::Time::Now().ToInternalValue());
     error = metadata->RefreshEntry(entry);
     if (error == FILE_ERROR_OK)
       params->changed_path = metadata->GetFilePath(local_id);
diff --git a/chrome/browser/chromeos/drive/file_system/create_directory_operation.cc b/chrome/browser/chromeos/drive/file_system/create_directory_operation.cc
index 0452c5f..39f5742 100644
--- a/chrome/browser/chromeos/drive/file_system/create_directory_operation.cc
+++ b/chrome/browser/chromeos/drive/file_system/create_directory_operation.cc
@@ -39,6 +39,7 @@ FileError CreateDirectoryRecursively(
   entry.mutable_file_info()->set_last_accessed(now.ToInternalValue());
   entry.set_parent_local_id(parent_local_id);
   entry.set_metadata_edit_state(ResourceEntry::DIRTY);
+  entry.set_modification_date(base::Time::Now().ToInternalValue());
 
   std::string local_id;
   FileError error = metadata->AddEntry(entry, &local_id);
diff --git a/chrome/browser/chromeos/drive/file_system/create_file_operation.cc b/chrome/browser/chromeos/drive/file_system/create_file_operation.cc
index 7fe7068..e9efe6b 100644
--- a/chrome/browser/chromeos/drive/file_system/create_file_operation.cc
+++ b/chrome/browser/chromeos/drive/file_system/create_file_operation.cc
@@ -57,6 +57,7 @@ FileError UpdateLocalState(internal::ResourceMetadata* metadata,
   entry->set_title(file_path.BaseName().AsUTF8Unsafe());
   entry->set_parent_local_id(parent.local_id());
   entry->set_metadata_edit_state(ResourceEntry::DIRTY);
+  entry->set_modification_date(base::Time::Now().ToInternalValue());
   entry->mutable_file_specific_info()->set_content_mime_type(mime_type);
 
   std::string local_id;
diff --git a/chrome/browser/chromeos/drive/file_system/move_operation.cc b/chrome/browser/chromeos/drive/file_system/move_operation.cc
index 1d6dd32..2210c6e 100644
--- a/chrome/browser/chromeos/drive/file_system/move_operation.cc
+++ b/chrome/browser/chromeos/drive/file_system/move_operation.cc
@@ -52,6 +52,7 @@ FileError UpdateLocalState(internal::ResourceMetadata* metadata,
   entry.set_title(new_title);
   entry.set_parent_local_id(parent_entry.local_id());
   entry.set_metadata_edit_state(ResourceEntry::DIRTY);
+  entry.set_modification_date(base::Time::Now().ToInternalValue());
   error = metadata->RefreshEntry(entry);
   if (error != FILE_ERROR_OK)
     return error;
diff --git a/chrome/browser/chromeos/drive/file_system/touch_operation.cc b/chrome/browser/chromeos/drive/file_system/touch_operation.cc
index 914312c..bb1b23d 100644
--- a/chrome/browser/chromeos/drive/file_system/touch_operation.cc
+++ b/chrome/browser/chromeos/drive/file_system/touch_operation.cc
@@ -38,6 +38,7 @@ FileError UpdateLocalState(internal::ResourceMetadata* metadata,
   if (!last_modified_time.is_null())
     file_info->set_last_modified(last_modified_time.ToInternalValue());
   entry.set_metadata_edit_state(ResourceEntry::DIRTY);
+  entry.set_modification_date(base::Time::Now().ToInternalValue());
   return metadata->RefreshEntry(entry);
 }
 
diff --git a/chrome/browser/chromeos/drive/resource_entry_conversion.cc b/chrome/browser/chromeos/drive/resource_entry_conversion.cc
index 1f479b6..1e13e6c 100644
--- a/chrome/browser/chromeos/drive/resource_entry_conversion.cc
+++ b/chrome/browser/chromeos/drive/resource_entry_conversion.cc
@@ -47,6 +47,7 @@ bool ConvertToResourceEntry(const google_apis::ResourceEntry& input,
   converted.set_title(input.title());
   converted.set_base_name(util::NormalizeFileName(converted.title()));
   converted.set_resource_id(input.resource_id());
+  converted.set_modification_date(input.modification_date().ToInternalValue());
 
   // Gets parent Resource ID. On drive.google.com, a file can have multiple
   // parents or no parent, but we are forcing a tree-shaped structure (i.e. no