[SyncFS] Add MetadataDatabase::UpdateTracker

chrome/browser/sync_file_system/drive_backend/metadata_database.cc

Index: chrome/browser/sync_file_system/drive_backend/metadata_database.cc
diff --git a/chrome/browser/sync_file_system/drive_backend/metadata_database.cc b/chrome/browser/sync_file_system/drive_backend/metadata_database.cc
index 98df266c11fd8f5129d1653f23c9f7484d337260..b37d578de922457587fa4a199e2acebbcd610475 100644
--- a/chrome/browser/sync_file_system/drive_backend/metadata_database.cc
+++ b/chrome/browser/sync_file_system/drive_backend/metadata_database.cc
@@ -710,7 +710,7 @@ void MetadataDatabase::PopulateFolder(const std::string& folder_id,
        itr != child_file_ids.end(); ++itr)
     CreateTrackerForParentAndFileID(*folder_tracker, *itr, batch.get());
   folder_tracker->set_needs_folder_listing(false);
-  if (!ShouldKeepDirty(*folder_tracker)) {
+  if (folder_tracker->dirty() && !ShouldKeepDirty(*folder_tracker)) {
     folder_tracker->set_dirty(false);
     dirty_trackers_.erase(folder_tracker);
   }
@@ -719,6 +719,63 @@ void MetadataDatabase::PopulateFolder(const std::string& folder_id,
   WriteToDatabase(batch.Pass(), callback);
 }
 
+void MetadataDatabase::UpdateTracker(int64 tracker_id,
+                                     const FileDetails& updated_details,
+                                     const SyncStatusCallback& callback) {
+  TrackerByID::iterator found = tracker_by_id_.find(tracker_id);
+  if (found == tracker_by_id_.end()) {
+    RunSoon(FROM_HERE, base::Bind(callback, SYNC_DATABASE_ERROR_NOT_FOUND));
+    return;
+  }
+
+  FileTracker* tracker = found->second;
+  DCHECK(tracker);
+
+  scoped_ptr<leveldb::WriteBatch> batch(new leveldb::WriteBatch);
+
+  // Local file is deleted.

[kinuko, 2013/08/27 07:47:05]

nit: the term 'local' is confusing here. Do you assume updated_details contain
local file change?  

[tzik, 2013/08/30 05:51:57]

Yes, the updated_details should contain latest local file state.

+  if (updated_details.deleted()) {
+    FileByID::iterator found = file_by_id_.find(tracker->file_id());
+    if (found == file_by_id_.end() || found->second->details().deleted()) {
+      // Both the tracker and metadata have the deleted flag, now it's safe to
+      // delete the |tracker|.
+      RemoveTracker(tracker->tracker_id(), batch.get());
+    } else {
+      // The local file is deleted, but corresponding remote file isn't.
+      // Put the tracker back to the initial state.
+      tracker->clear_synced_details();
+      tracker->set_dirty(true);
+      tracker->set_active(false);
+      PutTrackerToBatch(*tracker, batch.get());
+    }
+
+    WriteToDatabase(batch.Pass(), callback);
+    return;
+  }
+
+  // Check if the tracker was retitled.  If it was, update the title and its
+  // index in advance.
+  if (!tracker->has_synced_details() ||
+      tracker->synced_details().title() != updated_details.title()) {
+    UpdateTrackerTitle(tracker, updated_details.title(), batch.get());
+  }
+
+  *tracker->mutable_synced_details() = updated_details;
+
+  // Activate the tracker if:
+  //   - There is no active tracker that tracks |tracker->file_id()|.
+  //   - There is no active tracker that has the same |parent| and |title|.
+  if (!tracker->active() && CanActivateTracker(*tracker))
+    MakeTrackerActive(tracker->tracker_id(), batch.get());
+  if (tracker->dirty() && !ShouldKeepDirty(*tracker)) {
+    tracker->set_dirty(false);
+    dirty_trackers_.erase(tracker);
+  }
+  PutTrackerToBatch(*tracker, batch.get());
+
+  WriteToDatabase(batch.Pass(), callback);
+}
+
 MetadataDatabase::MetadataDatabase(base::SequencedTaskRunner* task_runner)
     : task_runner_(task_runner), weak_ptr_factory_(this) {
   DCHECK(task_runner);
@@ -960,9 +1017,21 @@ void MetadataDatabase::CreateTrackerForParentAndFileID(
   tracker_by_id_[tracker_id] = tracker.release();
 }
 
+void MetadataDatabase::RemoveTracker(int64 tracker_id,
+                                     leveldb::WriteBatch* batch) {
+  RemoveTrackerInternal(tracker_id, batch, false);
+}
+
 void MetadataDatabase::RemoveTrackerIgnoringSiblings(
     int64 tracker_id,
     leveldb::WriteBatch* batch) {
+  RemoveTrackerInternal(tracker_id, batch, true);
+}
+
+void MetadataDatabase::RemoveTrackerInternal(
+    int64 tracker_id,
+    leveldb::WriteBatch* batch,
+    bool ignore_name_conflicted_trackers) {

[kinuko, 2013/08/27 13:20:19]

nit: I'd name this bool parameter similar to the method name
RemoveTrackerIgnoringSiblings.

Or.. for simplicity could these three methods be combined into one with enum
params, say,

RemoveTracker(int tracker_id, RemoveTrackerMode mode, WriteBatch* batch);

enum RemoveTrackerMode {
  MARK_DIRTY_CONFLICTS_BY_PATH,
  IGNORE_CONFLICTS_BY_PATH,
};

[tzik, 2013/08/30 05:51:57]

Done.

   scoped_ptr<FileTracker> tracker(
       FindAndEraseItem(&tracker_by_id_, tracker_id));
   if (!tracker)
@@ -974,8 +1043,11 @@ void MetadataDatabase::RemoveTrackerIgnoringSiblings(
   EraseTrackerFromPathIndex(tracker.get());
 
   MarkTrackersDirtyByFileID(tracker->file_id(), batch);
-  // Do not mark the same path trackers as dirty, since the caller is deleting
-  // all its siblings.
+  if (!ignore_name_conflicted_trackers) {
+    MarkTrackersDirtyByPath(tracker->parent_tracker_id(),
+                            GetTrackerTitle(*tracker),
+                            batch);
+  }
   PutTrackerDeletionToBatch(tracker_id, batch);
 }
 
@@ -1138,7 +1210,27 @@ void MetadataDatabase::RecursiveMarkTrackerAsDirty(int64 root_tracker_id,
   }
 }
 
+bool MetadataDatabase::CanActivateTracker(const FileTracker& tracker) {
+  DCHECK(!tracker.active());
+  DCHECK_NE(service_metadata_->sync_root_tracker_id(), tracker.tracker_id());
+
+  if (HasActiveTrackerForFileID(tracker.file_id()))
+    return false;
+
+  if (tracker.app_id().empty())
+    return false;
+  if (!tracker.has_synced_details())
+    return false;
+  DCHECK(tracker.parent_tracker_id());
+
+  return !HasActiveTrackerForPath(tracker.parent_tracker_id(),
+                                  tracker.synced_details().title());
+}
+
 bool MetadataDatabase::ShouldKeepDirty(const FileTracker& tracker) const {
+  if (HasDisabledAppRoot(tracker))
+    return false;
+
   DCHECK(tracker.dirty());
   if (!tracker.has_synced_details())
     return true;
@@ -1146,17 +1238,18 @@ bool MetadataDatabase::ShouldKeepDirty(const FileTracker& tracker) const {
   FileByID::const_iterator found = file_by_id_.find(tracker.file_id());
   if (found == file_by_id_.end())
     return true;
-  const FileMetadata& file = *found->second;
+  const FileMetadata* file = found->second;
+  DCHECK(file);
 
   if (tracker.active()) {
     if (tracker.needs_folder_listing())
       return true;
-    if (tracker.synced_details().md5() != file.details().md5())
+    if (tracker.synced_details().md5() != file->details().md5())
       return true;
   }
 
   const FileDetails& local_details = tracker.synced_details();
-  const FileDetails& remote_details = file.details();
+  const FileDetails& remote_details = file->details();
 
   if (local_details.title() != remote_details.title())
     return true;
@@ -1166,6 +1259,75 @@ bool MetadataDatabase::ShouldKeepDirty(const FileTracker& tracker) const {
   return false;
 }
 
+bool MetadataDatabase::HasDisabledAppRoot(const FileTracker& tracker) const {
+  TrackerByAppID::const_iterator found =
+      app_root_by_app_id_.find(tracker.app_id());
+  if (found == app_root_by_app_id_.end())
+    return false;

[kinuko, 2013/08/27 13:20:19]

What does this case mean..?

[tzik, 2013/08/30 05:51:57]

This means the trackers is out of any app-root tree, e.g. a direct child of the
sync-root which is not a app-root.

+
+  const FileTracker* app_root_tracker = found->second;
+  DCHECK(app_root_tracker);
+  return app_root_tracker->tracker_kind() == TRACKER_KIND_DISABLED_APP_ROOT;
+}
+
+bool MetadataDatabase::HasActiveTrackerForFileID(
+    const std::string& file_id) const {
+  TrackersByFileID::const_iterator found = trackers_by_file_id_.find(file_id);
+  return found != trackers_by_file_id_.end() && found->second.has_active();
+}
+
+bool MetadataDatabase::HasActiveTrackerForPath(int64 parent_tracker_id,
+                                               const std::string& title) const {
+  TrackersByParentAndTitle::const_iterator found_by_parent =
+      trackers_by_parent_and_title_.find(parent_tracker_id);
+  if (found_by_parent == trackers_by_parent_and_title_.end())
+    return false;
+
+  const TrackersByTitle& trackers_by_title = found_by_parent->second;
+  TrackersByTitle::const_iterator found = trackers_by_title.find(title);
+  return found != trackers_by_title.end() && found->second.has_active();
+}
+
+void MetadataDatabase::UpdateTrackerTitle(FileTracker* tracker,
+                                          const std::string& new_title,
+                                          leveldb::WriteBatch* batch) {
+  int64 parent_id = tracker->parent_tracker_id();
+  std::string old_title = GetTrackerTitle(*tracker);
+  DCHECK_NE(old_title, new_title);
+  DCHECK(!new_title.empty());

[kinuko, 2013/08/27 13:20:19]

We don't check this in UpdateTracker.  Is new_title guaranteed to be a non-empty
string? (Does it always refer to a local file name?)

[tzik, 2013/08/30 05:51:57]

New title should not be empty unless the file is deleted locally.
And every title update is handled here. So, I think it's enough to check it
here.

[kinuko, 2013/08/30 06:03:46]

Ok, I assume title cannot be assigned to an empty string.

+
+  TrackersByTitle* trackers_by_title =
+      &trackers_by_parent_and_title_[parent_id];
+  TrackerSet* old_siblings = &(*trackers_by_title)[old_title];
+  TrackerSet* new_siblings = &(*trackers_by_title)[new_title];
+
+  old_siblings->Erase(tracker);
+  if (old_siblings->empty())
+    trackers_by_title->erase(old_title);
+  else
+    MarkTrackerSetDirty(old_siblings, batch);
+
+  if (tracker->active() && new_siblings->has_active()) {
+    // Inactivate existing active tracker.

[kinuko, 2013/08/27 13:20:19]

Why does the new one always win here?

[tzik, 2013/08/30 05:51:57]

The file is already updated locally here by the sync engine.
Deciding winner should be performed elsewhere in it.

+    FileTracker* obstacle = new_siblings->active_tracker();
+    new_siblings->Inactivate(obstacle);

[kinuko, 2013/08/27 13:20:19]

Do we need to check if the obstacle is KIND_REGULAR?

[tzik, 2013/08/30 05:51:57]

Done.

+
+    TrackerSet* chain = &trackers_by_file_id_[obstacle->file_id()];

[kinuko, 2013/08/27 13:20:19]

chain... ?

[tzik, 2013/08/30 05:51:57]

Done.

+    chain->Inactivate(obstacle);
+    MarkTrackerSetDirty(chain, batch);
+
+    obstacle->set_active(false);
+    PutTrackerToBatch(*obstacle, batch);
+
+    RemoveAllDescendantTrackers(obstacle->tracker_id(), batch);
+    new_siblings->Insert(tracker);

[kinuko, 2013/08/27 13:20:19]

duplicated with line 1327?

[tzik, 2013/08/30 05:51:57]

Done.

+  }
+
+  tracker->mutable_synced_details()->set_title(new_title);
+  new_siblings->Insert(tracker);
+  PutTrackerToBatch(*tracker, batch);
+}
+
 void MetadataDatabase::WriteToDatabase(scoped_ptr<leveldb::WriteBatch> batch,
                                        const SyncStatusCallback& callback) {
   base::PostTaskAndReplyWithResult(