Process TeamDrive change in change list.

components/drive/chromeos/change_list_processor.cc

Index: components/drive/chromeos/change_list_processor.cc
diff --git a/components/drive/chromeos/change_list_processor.cc b/components/drive/chromeos/change_list_processor.cc
index 3eb96bb1c81a47f9d7de103166f40b752d7b951f..66d0d43cdd4ba223619aa58cf8d40f112c1a212f 100644
--- a/components/drive/chromeos/change_list_processor.cc
+++ b/components/drive/chromeos/change_list_processor.cc
@@ -20,6 +20,7 @@
 #include "components/drive/file_system_core_util.h"
 #include "components/drive/resource_entry_conversion.h"
 #include "google_apis/drive/drive_api_parser.h"
+#include "google_apis/drive/drive_switches.h"
 
 namespace drive {
 namespace internal {
@@ -77,17 +78,24 @@ ChangeList::ChangeList(const google_apis::ChangeList& change_list)
   const std::vector<std::unique_ptr<google_apis::ChangeResource>>& items =
       change_list.items();
   entries_.resize(items.size());
+  team_drives_.resize(items.size());
   parent_resource_ids_.resize(items.size());
   size_t entries_index = 0;
+  size_t team_drives_index = 0;
   for (size_t i = 0; i < items.size(); ++i) {
-    if (ConvertChangeResourceToResourceEntry(
-            *items[i],
-            &entries_[entries_index],
-            &parent_resource_ids_[entries_index])) {
+    if (items[i]->type() == google_apis::ChangeResource::TEAM_DRIVE) {

[hashimoto, 2017/04/07 07:56:41]

Can't we do this check in ConvertTeamDriveChangeResourceToResourceEntry?
Can't we merge ConvertChangeResourceToResourceEntry to
ConvertTeamDriveChangeResourceToResourceEntry?

This way, we can get rid of team_drives_ by storing all entires in entries_.

[yamaguchi, 2017/04/07 15:31:46]

Done.

+      if (ConvertTeamDriveChangeResourceToResourceEntry(
+              *items[i], &team_drives_[team_drives_index])) {
+        ++team_drives_index;
+      }
+    } else if (ConvertChangeResourceToResourceEntry(
+                   *items[i], &entries_[entries_index],
+                   &parent_resource_ids_[entries_index])) {
       ++entries_index;
     }
   }
   entries_.resize(entries_index);
+  team_drives_.resize(team_drives_index);
   parent_resource_ids_.resize(entries_index);
 }
 
@@ -149,6 +157,15 @@ FileError ChangeListProcessor::Apply(
   for (size_t i = 0; i < change_lists.size(); ++i) {
     ChangeList* change_list = change_lists[i].get();
 
+    const std::vector<ResourceEntry>& team_drives = change_list->team_drives();
+    for (size_t i = 0; i < team_drives.size(); ++i) {
+      if (team_drives.at(i).deleted())
+        // TODO(yamaguchi): Handle deleting change of Team Drives. Deletion
+        // entries of Team Drive should be filtered in ApplyEntryMap().
+        continue;
+      ApplyTeamDriveChange(team_drives.at(i));
+    }
+
     std::vector<ResourceEntry>* entries = change_list->mutable_entries();
     for (size_t i = 0; i < entries->size(); ++i) {
       ResourceEntry* entry = &(*entries)[i];
@@ -196,6 +213,13 @@ FileError ChangeListProcessor::Apply(
   return FILE_ERROR_OK;
 }
 
+FileError ChangeListProcessor::ApplyTeamDriveChange(
+    const ResourceEntry& change) {
+  parent_resource_id_map_[change.resource_id()] =
+      util::kDriveTeamDrivesDirVirtualResourceId;
+  return ApplyEntry(change);
+}
+
 FileError ChangeListProcessor::ApplyEntryMap(
     int64_t changestamp,
     std::unique_ptr<google_apis::AboutResource> about_resource) {
@@ -308,6 +332,8 @@ FileError ChangeListProcessor::ApplyEntryMap(
     for (size_t i = 0; i < entries.size(); ++i) {
       // Skip root entry in the change list. We don't expect servers to send
       // root entry, but we should better be defensive (see crbug.com/297259).
+      // TODO(yamaguchi): Apply this defensive logic to root directories of
+      // every Team Drive as well.
       ResourceEntryMap::iterator it = entries[i];
       if (it->first != root.resource_id()) {
         FileError error = ApplyEntry(it->second);
@@ -348,6 +374,7 @@ FileError ChangeListProcessor::ApplyEntryMap(
 
 FileError ChangeListProcessor::ApplyEntry(const ResourceEntry& entry) {
   DCHECK(!entry.deleted());
+  DCHECK(!entry.resource_id().empty());
   DCHECK(parent_resource_id_map_.count(entry.resource_id()));
   const std::string& parent_resource_id =
       parent_resource_id_map_[entry.resource_id()];
@@ -469,6 +496,8 @@ FileError ChangeListProcessor::SetParentLocalIdOfEntry(
   if (parent_resource_id.empty()) {
     // Entries without parents should go under "other" directory.
     parent_local_id = util::kDriveOtherDirLocalId;
+  } else if (parent_resource_id == util::kDriveTeamDrivesDirVirtualResourceId) {

[hashimoto, 2017/04/07 07:56:41]

Instead of introducing this "virtual resource ID", how about doing nothing when
|entry|'s parent_local_id is already set (or when |entry|'s
parent_local_id==kDriveTeamDrivesDirLocalId)?

[yamaguchi, 2017/04/07 15:31:46]

Changed to insert the virtual resource ID to the "/team_drives" entry in
resource_metadata.cc instead.
I think this would be more simple as TeamDrive entries can also be processed by
existing logic evenly for this kind of localID<->recourceID lookup in other
places as well.

[hashimoto, 2017/04/10 11:01:23]

The idea of "virtual resource ID" sounds awkward to me.
Is there any logic which needs to be changed to handle team drive entries
without introducing this virtual ID other than SetParentLocalIdOfEntry()?

[yamaguchi, 2017/04/11 07:08:07]

Thanks. Now I think it should work without introducing the virtual ID.

+    parent_local_id = util::kDriveTeamDrivesDirLocalId;
   } else {
     FileError error = resource_metadata->GetIdByResourceId(
         parent_resource_id, &parent_local_id);