drive: Check "modificationDate" property of a change before applying it

Updating a locally edited entry with an older change results in unnecessarily overwriting users' edit.

Add "modificationDate" field to ChangeResource. (Corresponding API documentation: https://developers.google.com/drive/v2/reference/changes#resource)
Request "modificationDate"
Use "modificationDate" for conflict resolution in ChangeListProcessor.

BUG=350398

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

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

10 files changed, 92 insertions, 11 deletions

diff --git a/chrome/browser/chromeos/drive/change_list_processor.cc b/chrome/browser/chromeos/drive/change_list_processor.cc
index a56e5a64..b84a620 100644
--- a/chrome/browser/chromeos/drive/change_list_processor.cc
+++ b/chrome/browser/chromeos/drive/change_list_processor.cc
@@ -53,6 +53,22 @@ class ChangeListToEntryMapUMAStats {
   int num_shared_with_me_entries_;
 };
 
+// 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;
+}
+
 }  // namespace
 
 std::string DirectoryFetchInfo::ToString() const {
@@ -69,15 +85,20 @@ 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]))
+                               &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() {}
@@ -126,6 +147,8 @@ 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);
@@ -303,8 +326,11 @@ 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,
@@ -321,9 +347,22 @@ FileError ChangeListProcessor::ApplyEntry(const ResourceEntry& entry) {
     error = resource_metadata_->GetResourceEntryById(local_id, &existing_entry);
 
   switch (error) {
-    case FILE_ERROR_OK:  // Entry exists and needs to be refreshed.
-      new_entry.set_local_id(local_id);
-      error = resource_metadata_->RefreshEntry(new_entry);
+    case FILE_ERROR_OK:
+      if (ShouldApplyChange(existing_entry, new_entry, modification_date)) {
+        // Entry exists and needs to be refreshed.
+        new_entry.set_local_id(local_id);
+        error = resource_metadata_->RefreshEntry(new_entry);
+      } else {
+        if (entry.file_info().is_directory()) {
+          // No need to refresh, but update the changestamp.
+          new_entry = existing_entry;
+          new_entry.mutable_directory_specific_info()->set_changestamp(
+              new_entry.directory_specific_info().changestamp());
+          error = resource_metadata_->RefreshEntry(new_entry);
+        }
+        DVLOG(1) << "Change was discarded for: "
+                 << resource_metadata_->GetFilePath(local_id).value();
+      }
       break;
     case FILE_ERROR_NOT_FOUND: {  // Adding a new entry.
       std::string local_id;
diff --git a/chrome/browser/chromeos/drive/change_list_processor.h b/chrome/browser/chromeos/drive/change_list_processor.h
index 57926bf..5a219c1 100644
--- a/chrome/browser/chromeos/drive/change_list_processor.h
+++ b/chrome/browser/chromeos/drive/change_list_processor.h
@@ -78,6 +78,12 @@ 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_; }
 
@@ -88,6 +94,7 @@ 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_;
 
@@ -134,6 +141,8 @@ 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.
@@ -151,6 +160,7 @@ 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 07ab4f6..18e5908 100644
--- a/chrome/browser/chromeos/drive/change_list_processor_unittest.cc
+++ b/chrome/browser/chromeos/drive/change_list_processor_unittest.cc
@@ -603,6 +603,7 @@ 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;
diff --git a/chrome/browser/drive/drive_api_service.cc b/chrome/browser/drive/drive_api_service.cc
index d09aa02..cf81889 100644
--- a/chrome/browser/drive/drive_api_service.cc
+++ b/chrome/browser/drive/drive_api_service.cc
@@ -120,9 +120,8 @@ const char kChangeListFields[] =
     "kind,items(file(kind,id,title,createdDate,sharedWithMeDate,"
     "mimeType,md5Checksum,fileSize,labels/trashed,imageMediaMetadata/width,"
     "imageMediaMetadata/height,imageMediaMetadata/rotation,etag,"
-    "parents/parentLink,alternateLink,"
-    "modifiedDate,lastViewedByMeDate,shared),deleted,id,fileId),nextLink,"
-    "largestChangeId";
+    "parents/parentLink,alternateLink,modifiedDate,lastViewedByMeDate,shared),"
+    "deleted,id,fileId,modificationDate),nextLink,largestChangeId";
 
 // Callback invoked when the parsing of resource list is completed,
 // regardless whether it is succeeded or not.
diff --git a/chrome/browser/drive/drive_api_util.cc b/chrome/browser/drive/drive_api_util.cc
index 5c0eadb..5ac7222 100644
--- a/chrome/browser/drive/drive_api_util.cc
+++ b/chrome/browser/drive/drive_api_util.cc
@@ -482,6 +482,7 @@ ConvertChangeResourceToResourceEntry(
   // If |is_deleted()| returns true, the file is removed from Drive.
   entry->set_removed(change_resource.is_deleted());
   entry->set_changestamp(change_resource.change_id());
+  entry->set_modification_date(change_resource.modification_date());
 
   return entry.Pass();
 }
diff --git a/chrome/test/data/drive/changelist.json b/chrome/test/data/drive/changelist.json
index 009e45c..1f17785 100644
--- a/chrome/test/data/drive/changelist.json
+++ b/chrome/test/data/drive/changelist.json
@@ -63,7 +63,8 @@
     "editable": true,
     "shared": false,
     "writersCanShare": true
-   }
+   },
+   "modificationDate": "2012-07-27T04:54:11.030Z"
   },
   {
    "kind": "drive#change",
@@ -121,7 +122,8 @@
     "editable": true,
     "shared": true,
     "writersCanShare": true
-   }
+   },
+   "modificationDate": "2012-07-27T05:43:20.269Z"
   },
   {
    "kind": "drive#change",
@@ -173,14 +175,16 @@
     "editable": true,
     "shared": false,
     "writersCanShare": true
-   }
+   },
+   "modificationDate": "2012-07-27T04:43:18.507Z"
   },
   {
     "kind": "drive#change",
     "id": "8430",
     "fileId": "ABCv7G8yEYAWHc3Y5X0hMSkJYXYZ",
     "selfLink": "https://www.googleapis.com/drive/v2/changes/16429",
-    "deleted": true
+    "deleted": true,
+    "modificationDate": "2012-07-27T12:34:56.789Z"
   }
  ]
 }
diff --git a/google_apis/drive/drive_api_parser.cc b/google_apis/drive/drive_api_parser.cc
index 018c37d..384db45 100644
--- a/google_apis/drive/drive_api_parser.cc
+++ b/google_apis/drive/drive_api_parser.cc
@@ -125,6 +125,7 @@ const char kFileKind[] = "drive#file";
 const char kTitle[] = "title";
 const char kMimeType[] = "mimeType";
 const char kCreatedDate[] = "createdDate";
+const char kModificationDate[] = "modificationDate";
 const char kModifiedDate[] = "modifiedDate";
 const char kModifiedByMeDate[] = "modifiedByMeDate";
 const char kLastViewedByMeDate[] = "lastViewedByMeDate";
@@ -559,6 +560,9 @@ void ChangeResource::RegisterJSONConverter(
   converter->RegisterBoolField(kDeleted, &ChangeResource::deleted_);
   converter->RegisterCustomValueField(kFile, &ChangeResource::file_,
                                       &CreateFileResourceFromValue);
+  converter->RegisterCustomField<base::Time>(
+      kModificationDate, &ChangeResource::modification_date_,
+      &util::GetTimeFromString);
 }
 
 // static
diff --git a/google_apis/drive/drive_api_parser.h b/google_apis/drive/drive_api_parser.h
index 75f2792..e51b864 100644
--- a/google_apis/drive/drive_api_parser.h
+++ b/google_apis/drive/drive_api_parser.h
@@ -730,6 +730,9 @@ class ChangeResource {
   const FileResource* file() const { return file_.get(); }
   FileResource* mutable_file() { return file_.get(); }
 
+  // Returns the time of this modification.
+  const base::Time& modification_date() const { return modification_date_; }
+
   void set_change_id(int64 change_id) {
     change_id_ = change_id;
   }
@@ -742,6 +745,9 @@ class ChangeResource {
   void set_file(scoped_ptr<FileResource> file) {
     file_ = file.Pass();
   }
+  void set_modification_date(const base::Time& modification_date) {
+    modification_date_ = modification_date;
+  }
 
  private:
   friend class base::internal::RepeatedMessageConverter<ChangeResource>;
@@ -755,6 +761,7 @@ class ChangeResource {
   std::string file_id_;
   bool deleted_;
   scoped_ptr<FileResource> file_;
+  base::Time modification_date_;
 
   DISALLOW_COPY_AND_ASSIGN(ChangeResource);
 };
diff --git a/google_apis/drive/drive_api_parser_unittest.cc b/google_apis/drive/drive_api_parser_unittest.cc
index 413e7ef..fd9b0c3 100644
--- a/google_apis/drive/drive_api_parser_unittest.cc
+++ b/google_apis/drive/drive_api_parser_unittest.cc
@@ -254,6 +254,7 @@ TEST(DriveAPIParserTest, ChangeListParser) {
   EXPECT_EQ("1Pc8jzfU1ErbN_eucMMqdqzY3eBm0v8sxXm_1CtLxABC", change1.file_id());
   EXPECT_EQ(change1.file_id(), change1.file()->file_id());
   EXPECT_FALSE(change1.file()->shared());
+  EXPECT_EQ(change1.file()->modified_date(), change1.modification_date());
 
   const ChangeResource& change2 = *changelist->items()[1];
   EXPECT_EQ(8424, change2.change_id());
@@ -261,6 +262,7 @@ TEST(DriveAPIParserTest, ChangeListParser) {
   EXPECT_EQ("0B4v7G8yEYAWHUmRrU2lMS2hLABC", change2.file_id());
   EXPECT_EQ(change2.file_id(), change2.file()->file_id());
   EXPECT_TRUE(change2.file()->shared());
+  EXPECT_EQ(change2.file()->modified_date(), change2.modification_date());
 
   const ChangeResource& change3 = *changelist->items()[2];
   EXPECT_EQ(8429, change3.change_id());
@@ -268,12 +270,17 @@ TEST(DriveAPIParserTest, ChangeListParser) {
   EXPECT_EQ("0B4v7G8yEYAWHYW1OcExsUVZLABC", change3.file_id());
   EXPECT_EQ(change3.file_id(), change3.file()->file_id());
   EXPECT_FALSE(change3.file()->shared());
+  EXPECT_EQ(change3.file()->modified_date(), change3.modification_date());
 
   // Deleted entry.
   const ChangeResource& change4 = *changelist->items()[3];
   EXPECT_EQ(8430, change4.change_id());
   EXPECT_EQ("ABCv7G8yEYAWHc3Y5X0hMSkJYXYZ", change4.file_id());
   EXPECT_TRUE(change4.is_deleted());
+  base::Time modification_time;
+  ASSERT_TRUE(util::GetTimeFromString("2012-07-27T12:34:56.789Z",
+                                      &modification_time));
+  EXPECT_EQ(modification_time, change4.modification_date());
 }
 
 TEST(DriveAPIParserTest, HasKind) {
diff --git a/google_apis/drive/gdata_wapi_parser.h b/google_apis/drive/gdata_wapi_parser.h
index a7ac493..eddd0e1 100644
--- a/google_apis/drive/gdata_wapi_parser.h
+++ b/google_apis/drive/gdata_wapi_parser.h
@@ -464,6 +464,10 @@ class ResourceEntry : public CommonMetadata {
   // If doesn't exist, then equals -1.
   int64 image_rotation() const { return image_rotation_; }
 
+  // The time of this modification.
+  // Note: This property is filled only when converted from ChangeResource.
+  const base::Time& modification_date() const { return modification_date_; }
+
   // Text version of resource entry kind. Returns an empty string for
   // unknown entry kind.
   std::string GetEntryKindText() const;
@@ -557,6 +561,9 @@ class ResourceEntry : public CommonMetadata {
   void set_image_rotation(int64 image_rotation) {
     image_rotation_ = image_rotation;
   }
+  void set_modification_date(const base::Time& modification_date) {
+    modification_date_ = modification_date;
+  }
 
   // Fills the remaining fields where JSONValueConverter cannot catch.
   // Currently, sets |kind_| and |labels_| based on the |categories_| in the
@@ -595,6 +602,8 @@ class ResourceEntry : public CommonMetadata {
   int64 image_height_;
   int64 image_rotation_;
 
+  base::Time modification_date_;
+
   DISALLOW_COPY_AND_ASSIGN(ResourceEntry);
 };