Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2799603002:
Process TeamDrive change in change list. (Closed)
DescriptionProcess TeamDrive change in change list.
Creates or updates local folder entries of each Team Drive when changelist is received, so that files under Team Drive can link it
as their ancestor.
Full fetch of Team Drives is not included in this change.
TEST=unit_tests --gtest_filter=JobSchedulerTest.*:ResourceEntryConversionTest.*:ResourceMetadataTest.*:ChangeListProcessorTest.*
TEST=google_apis_unittests --gtest_filter=DriveAPIParserTest.*:DriveApiUrlGeneratorTest.*:DriveApiRequestsTest.*
BUG=none
Review-Url: https://codereview.chromium.org/2799603002
Cr-Commit-Position: refs/heads/master@{#463597}
Committed: https://chromium.googlesource.com/chromium/src/+/b1bbca356abfa6f8fdec3cd5815f18b164491a45
Patch Set 1 #Patch Set 2 : Add test case for updating Team Drive name. #Patch Set 3 : Handle add/replace in List Processor rather than in ResourceMetadata. #Patch Set 4 : Remove unused "title" message field. #Patch Set 5 : Add test for update. #
Total comments: 16
Patch Set 6 : Convert to ResourceEntry directly. Switch changeResource to normal class. #Patch Set 7 : Convert TeamDriveResource to ResourceEntry directly. #Patch Set 8 : Fix comment #Patch Set 9 : Avoid crash when receiving TD deletion entry. #
Total comments: 14
Patch Set 10 : Fix the test for removing td. #Patch Set 11 : Handle changes to Team Drive by the same path as files, including deletion. #
Total comments: 9
Patch Set 12 : Split ConvertTeamDriveResourceToResourceEntry. Remove unused code. #Patch Set 13 : Stop using a virtual resource ID for /team_drives/. #Patch Set 14 : Do not call SetParentLocalId when not needed. #
Total comments: 12
Patch Set 15 : . #Patch Set 16 : reflect review comments #Patch Set 17 : Remove unnecessary return value, as the TeamDriveResource conversion always succeeds. #Messages
Total messages: 62 (49 generated)
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Process TeamDrive change in change list. Creates local folder entries for each Team Drive upon change. Full fetch of Team Drives is not included in this change. TEST=unit_tests --gtest_filter=JobSchedulerTest.*:ResourceEntryConversionTest.*:ResourceMetadataTest.*:ChangeListProcessorTest.* TEST=google_apis_unittests --gtest_filter=DriveAPIParserTest.*:DriveApiUrlGeneratorTest.*:DriveApiRequestsTest.* BUG=none ========== to ========== Process TeamDrive change in change list. Creates local folder entries for each Team Drive upon change. Full fetch of Team Drives is not included in this change. Team Drive root folder resource entries differs from other file in these points: - local_id is same as ID in Drive. Creation of Team Drive offline is not supported now. - parent is always /team_drives TEST=unit_tests --gtest_filter=JobSchedulerTest.*:ResourceEntryConversionTest.*:ResourceMetadataTest.*:ChangeListProcessorTest.* TEST=google_apis_unittests --gtest_filter=DriveAPIParserTest.*:DriveApiUrlGeneratorTest.*:DriveApiRequestsTest.* BUG=none ==========
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Process TeamDrive change in change list. Creates local folder entries for each Team Drive upon change. Full fetch of Team Drives is not included in this change. Team Drive root folder resource entries differs from other file in these points: - local_id is same as ID in Drive. Creation of Team Drive offline is not supported now. - parent is always /team_drives TEST=unit_tests --gtest_filter=JobSchedulerTest.*:ResourceEntryConversionTest.*:ResourceMetadataTest.*:ChangeListProcessorTest.* TEST=google_apis_unittests --gtest_filter=DriveAPIParserTest.*:DriveApiUrlGeneratorTest.*:DriveApiRequestsTest.* BUG=none ========== to ========== Process TeamDrive change in change list. Creates local folder entries for each Team Drive upon change. Full fetch of Team Drives is not included in this change. Team Drive root folder resource entries differs from other file in these points: - local_id is same as ID in Drive. Creation of Team Drive offline is not supported now. - parent is always /team_drives - sync status (dirty/clean) is not handled (this will be required when we sync we support edit operation of Team Drive.) TEST=unit_tests --gtest_filter=JobSchedulerTest.*:ResourceEntryConversionTest.*:ResourceMetadataTest.*:ChangeListProcessorTest.* TEST=google_apis_unittests --gtest_filter=DriveAPIParserTest.*:DriveApiUrlGeneratorTest.*:DriveApiRequestsTest.* BUG=none ==========
Description was changed from ========== Process TeamDrive change in change list. Creates local folder entries for each Team Drive upon change. Full fetch of Team Drives is not included in this change. Team Drive root folder resource entries differs from other file in these points: - local_id is same as ID in Drive. Creation of Team Drive offline is not supported now. - parent is always /team_drives - sync status (dirty/clean) is not handled (this will be required when we sync we support edit operation of Team Drive.) TEST=unit_tests --gtest_filter=JobSchedulerTest.*:ResourceEntryConversionTest.*:ResourceMetadataTest.*:ChangeListProcessorTest.* TEST=google_apis_unittests --gtest_filter=DriveAPIParserTest.*:DriveApiUrlGeneratorTest.*:DriveApiRequestsTest.* BUG=none ========== to ========== Process TeamDrive change in change list. Creates or updates local folder entries of each Team Drive when changelist is received. Full fetch of Team Drives is not included in this change. Team Drive root folder resource entries differs from other file in these points: - local_id is same as ID in Drive. Creation of Team Drive offline is not supported now. - parent is always /team_drives - sync status (dirty/clean) is not handled (this will be required when we sync we support edit operation of Team Drive.) TEST=unit_tests --gtest_filter=JobSchedulerTest.*:ResourceEntryConversionTest.*:ResourceMetadataTest.*:ChangeListProcessorTest.* TEST=google_apis_unittests --gtest_filter=DriveAPIParserTest.*:DriveApiUrlGeneratorTest.*:DriveApiRequestsTest.* BUG=none ==========
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Process TeamDrive change in change list. Creates or updates local folder entries of each Team Drive when changelist is received. Full fetch of Team Drives is not included in this change. Team Drive root folder resource entries differs from other file in these points: - local_id is same as ID in Drive. Creation of Team Drive offline is not supported now. - parent is always /team_drives - sync status (dirty/clean) is not handled (this will be required when we sync we support edit operation of Team Drive.) TEST=unit_tests --gtest_filter=JobSchedulerTest.*:ResourceEntryConversionTest.*:ResourceMetadataTest.*:ChangeListProcessorTest.* TEST=google_apis_unittests --gtest_filter=DriveAPIParserTest.*:DriveApiUrlGeneratorTest.*:DriveApiRequestsTest.* BUG=none ========== to ========== Process TeamDrive change in change list. Creates or updates local folder entries of each Team Drive when changelist is received. Full fetch of Team Drives is not included in this change. TEST=unit_tests --gtest_filter=JobSchedulerTest.*:ResourceEntryConversionTest.*:ResourceMetadataTest.*:ChangeListProcessorTest.* TEST=google_apis_unittests --gtest_filter=DriveAPIParserTest.*:DriveApiUrlGeneratorTest.*:DriveApiRequestsTest.* BUG=none ==========
Description was changed from ========== Process TeamDrive change in change list. Creates or updates local folder entries of each Team Drive when changelist is received. Full fetch of Team Drives is not included in this change. TEST=unit_tests --gtest_filter=JobSchedulerTest.*:ResourceEntryConversionTest.*:ResourceMetadataTest.*:ChangeListProcessorTest.* TEST=google_apis_unittests --gtest_filter=DriveAPIParserTest.*:DriveApiUrlGeneratorTest.*:DriveApiRequestsTest.* BUG=none ========== to ========== Process TeamDrive change in change list. Creates or updates local folder entries of each Team Drive when changelist is received, so that files under Team Drive can link it as their ancestor. Full fetch of Team Drives is not included in this change. TEST=unit_tests --gtest_filter=JobSchedulerTest.*:ResourceEntryConversionTest.*:ResourceMetadataTest.*:ChangeListProcessorTest.* TEST=google_apis_unittests --gtest_filter=DriveAPIParserTest.*:DriveApiUrlGeneratorTest.*:DriveApiRequestsTest.* BUG=none ==========
yamaguchi@chromium.org changed reviewers: + hashimoto@chromium.org
ptal
https://codereview.chromium.org/2799603002/diff/80001/components/drive/change... File components/drive/change_list_processor_unittest.cc (right): https://codereview.chromium.org/2799603002/diff/80001/components/drive/change... components/drive/change_list_processor_unittest.cc:34: const int64_t kSecondResourceListChangestamp = 4567; You can use kBaseResourceListChangestamp + 1 instead of this. https://codereview.chromium.org/2799603002/diff/80001/components/drive/change... components/drive/change_list_processor_unittest.cc:694: metadata_->GetResourceEntryById(kTeamDriveId, &entry)); This GetResourceEntryById() is not needed? https://codereview.chromium.org/2799603002/diff/80001/components/drive/chrome... File components/drive/chromeos/change_list_processor.cc (right): https://codereview.chromium.org/2799603002/diff/80001/components/drive/chrome... components/drive/chromeos/change_list_processor.cc:85: if (items[i]->type() == google_apis::ChangeResource::TEAM_DRIVE) { I think Team Drive change can be also converted to ResourceEntry. Can't we just let ConvertChangeResourceToResourceEntry convert ChangeResource to ResourceEntry even when the type is TEAM_DRIVE? https://codereview.chromium.org/2799603002/diff/80001/components/drive/chrome... File components/drive/chromeos/resource_metadata.cc (right): https://codereview.chromium.org/2799603002/diff/80001/components/drive/chrome... components/drive/chromeos/resource_metadata.cc:125: FileError ResourceMetadata::AddTeamDrive(const std::string& id, Do we need these new methods? I think the same thing can be done with AddEntry() and RefreshEntry(). Also, don't we need to handle deletion? https://codereview.chromium.org/2799603002/diff/80001/components/drive/drive.... File components/drive/drive.proto (right): https://codereview.chromium.org/2799603002/diff/80001/components/drive/drive.... components/drive/drive.proto:162: message TeamDriveChange { IIUC this object never gets serialized. So this doesn't deserved to be a proto. https://codereview.chromium.org/2799603002/diff/80001/google_apis/drive/drive... File google_apis/drive/drive_api_parser.h (right): https://codereview.chromium.org/2799603002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_api_parser.h:413: void set_id(const std::string& id) { id_ = id; } Please put this after the getter (i.e. id()). https://codereview.chromium.org/2799603002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_api_parser.h:416: void set_name(const std::string& name) { name_ = name; } ditto. https://codereview.chromium.org/2799603002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_api_parser.h:864: void set_team_drive_id(const std::string& team_drive_id) { Please put this setter next to the other setters (e.g. set_change_id).
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Revised to convert TeamDrive changes to ResourceEntry directly. Would it be acceptable to add the logic to handle deletion of a Team Drive by a separate change? https://codereview.chromium.org/2799603002/diff/80001/components/drive/change... File components/drive/change_list_processor_unittest.cc (right): https://codereview.chromium.org/2799603002/diff/80001/components/drive/change... components/drive/change_list_processor_unittest.cc:34: const int64_t kSecondResourceListChangestamp = 4567; On 2017/04/06 10:23:36, hashimoto wrote: > You can use kBaseResourceListChangestamp + 1 instead of this. Done. https://codereview.chromium.org/2799603002/diff/80001/components/drive/change... components/drive/change_list_processor_unittest.cc:694: metadata_->GetResourceEntryById(kTeamDriveId, &entry)); On 2017/04/06 10:23:36, hashimoto wrote: > This GetResourceEntryById() is not needed? Right, it's not needed. https://codereview.chromium.org/2799603002/diff/80001/components/drive/chrome... File components/drive/chromeos/change_list_processor.cc (right): https://codereview.chromium.org/2799603002/diff/80001/components/drive/chrome... components/drive/chromeos/change_list_processor.cc:85: if (items[i]->type() == google_apis::ChangeResource::TEAM_DRIVE) { On 2017/04/06 10:23:36, hashimoto wrote: > I think Team Drive change can be also converted to ResourceEntry. > Can't we just let ConvertChangeResourceToResourceEntry convert ChangeResource to > ResourceEntry even when the type is TEAM_DRIVE? Changed to directly convert to ResourceEntry. However, I am still having those separated from files because those are always direct descendants of the Team Drives directory. https://codereview.chromium.org/2799603002/diff/80001/components/drive/chrome... File components/drive/chromeos/resource_metadata.cc (right): https://codereview.chromium.org/2799603002/diff/80001/components/drive/chrome... components/drive/chromeos/resource_metadata.cc:125: FileError ResourceMetadata::AddTeamDrive(const std::string& id, On 2017/04/06 10:23:36, hashimoto wrote: > Do we need these new methods? > I think the same thing can be done with AddEntry() and RefreshEntry(). > Also, don't we need to handle deletion? Changed to have them processed as normal Entries. https://codereview.chromium.org/2799603002/diff/80001/components/drive/drive.... File components/drive/drive.proto (right): https://codereview.chromium.org/2799603002/diff/80001/components/drive/drive.... components/drive/drive.proto:162: message TeamDriveChange { On 2017/04/06 10:23:36, hashimoto wrote: > IIUC this object never gets serialized. > So this doesn't deserved to be a proto. Changed not to use this intermediate data structure, as you suggested. https://codereview.chromium.org/2799603002/diff/80001/google_apis/drive/drive... File google_apis/drive/drive_api_parser.h (right): https://codereview.chromium.org/2799603002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_api_parser.h:413: void set_id(const std::string& id) { id_ = id; } On 2017/04/06 10:23:36, hashimoto wrote: > Please put this after the getter (i.e. id()). Done. https://codereview.chromium.org/2799603002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_api_parser.h:416: void set_name(const std::string& name) { name_ = name; } On 2017/04/06 10:23:37, hashimoto wrote: > ditto. Done. https://codereview.chromium.org/2799603002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_api_parser.h:864: void set_team_drive_id(const std::string& team_drive_id) { On 2017/04/06 10:23:36, hashimoto wrote: > Please put this setter next to the other setters (e.g. set_change_id). Done.
https://codereview.chromium.org/2799603002/diff/160001/components/drive/chang... File components/drive/change_list_processor_unittest.cc (right): https://codereview.chromium.org/2799603002/diff/160001/components/drive/chang... components/drive/change_list_processor_unittest.cc:717: change_lists[0]->set_largest_changestamp(kBaseResourceListChangestamp + 3); This is not a ChangeList which deletes something. To delete something, ResourceEntry in ChangeList should have "deleted" set to true. https://codereview.chromium.org/2799603002/diff/160001/components/drive/chrom... File components/drive/chromeos/change_list_processor.cc (right): https://codereview.chromium.org/2799603002/diff/160001/components/drive/chrom... components/drive/chromeos/change_list_processor.cc:86: if (items[i]->type() == google_apis::ChangeResource::TEAM_DRIVE) { 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_. https://codereview.chromium.org/2799603002/diff/160001/components/drive/chrom... components/drive/chromeos/change_list_processor.cc:499: } else if (parent_resource_id == util::kDriveTeamDrivesDirVirtualResourceId) { 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)? https://codereview.chromium.org/2799603002/diff/160001/components/drive/chrom... File components/drive/chromeos/resource_metadata.h (right): https://codereview.chromium.org/2799603002/diff/160001/components/drive/chrom... components/drive/chromeos/resource_metadata.h:69: FileError AddTeamDrive(const std::string& id, const std::string& name); No longer needed. https://codereview.chromium.org/2799603002/diff/160001/components/drive/chrom... components/drive/chromeos/resource_metadata.h:94: FileError RefreshTeamDrive(const std::string& id, const std::string& name); ditto. https://codereview.chromium.org/2799603002/diff/160001/google_apis/drive/driv... File google_apis/drive/drive_switches.h (right): https://codereview.chromium.org/2799603002/diff/160001/google_apis/drive/driv... google_apis/drive/drive_switches.h:19: std::string GetTeamDrivesIntegrationSwitchName(); Just put kEnableTeamDrives in this header like other xxx_switches.h files are doing.
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal Now I made it handle Team Drive removal as well. https://codereview.chromium.org/2799603002/diff/160001/components/drive/chang... File components/drive/change_list_processor_unittest.cc (right): https://codereview.chromium.org/2799603002/diff/160001/components/drive/chang... components/drive/change_list_processor_unittest.cc:717: change_lists[0]->set_largest_changestamp(kBaseResourceListChangestamp + 3); On 2017/04/07 07:56:41, hashimoto wrote: > This is not a ChangeList which deletes something. > To delete something, ResourceEntry in ChangeList should have "deleted" set to > true. Done. https://codereview.chromium.org/2799603002/diff/160001/components/drive/chrom... File components/drive/chromeos/change_list_processor.cc (right): https://codereview.chromium.org/2799603002/diff/160001/components/drive/chrom... components/drive/chromeos/change_list_processor.cc:86: if (items[i]->type() == google_apis::ChangeResource::TEAM_DRIVE) { On 2017/04/07 07:56:41, hashimoto wrote: > 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_. Done. https://codereview.chromium.org/2799603002/diff/160001/components/drive/chrom... components/drive/chromeos/change_list_processor.cc:499: } else if (parent_resource_id == util::kDriveTeamDrivesDirVirtualResourceId) { On 2017/04/07 07:56:41, hashimoto wrote: > 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)? 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. https://codereview.chromium.org/2799603002/diff/160001/components/drive/chrom... File components/drive/chromeos/resource_metadata.h (right): https://codereview.chromium.org/2799603002/diff/160001/components/drive/chrom... components/drive/chromeos/resource_metadata.h:69: FileError AddTeamDrive(const std::string& id, const std::string& name); On 2017/04/07 07:56:41, hashimoto wrote: > No longer needed. Done. https://codereview.chromium.org/2799603002/diff/160001/components/drive/chrom... components/drive/chromeos/resource_metadata.h:94: FileError RefreshTeamDrive(const std::string& id, const std::string& name); On 2017/04/07 07:56:41, hashimoto wrote: > ditto. Done. https://codereview.chromium.org/2799603002/diff/160001/google_apis/drive/driv... File google_apis/drive/drive_switches.h (right): https://codereview.chromium.org/2799603002/diff/160001/google_apis/drive/driv... google_apis/drive/drive_switches.h:19: std::string GetTeamDrivesIntegrationSwitchName(); On 2017/04/07 07:56:41, hashimoto wrote: > Just put kEnableTeamDrives in this header like other xxx_switches.h files are > doing. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2799603002/diff/160001/components/drive/chrom... File components/drive/chromeos/change_list_processor.cc (right): https://codereview.chromium.org/2799603002/diff/160001/components/drive/chrom... components/drive/chromeos/change_list_processor.cc:499: } else if (parent_resource_id == util::kDriveTeamDrivesDirVirtualResourceId) { On 2017/04/07 15:31:46, yamaguchi wrote: > On 2017/04/07 07:56:41, hashimoto wrote: > > 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)? > > 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. 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()? https://codereview.chromium.org/2799603002/diff/200001/components/drive/chrom... File components/drive/chromeos/change_list_processor.cc (right): https://codereview.chromium.org/2799603002/diff/200001/components/drive/chrom... components/drive/chromeos/change_list_processor.cc:23: #include "google_apis/drive/drive_switches.h" No longer needed. https://codereview.chromium.org/2799603002/diff/200001/components/drive/chrom... components/drive/chromeos/change_list_processor.cc:311: // TODO(yamaguchi): Apply this defensive logic to root directories of Why do you need to do this? https://codereview.chromium.org/2799603002/diff/200001/components/drive/chrom... File components/drive/chromeos/change_list_processor.h (right): https://codereview.chromium.org/2799603002/diff/200001/components/drive/chrom... components/drive/chromeos/change_list_processor.h:156: // Apply |change| to resource_metadata_. No longer needed. https://codereview.chromium.org/2799603002/diff/200001/components/drive/resou... File components/drive/resource_entry_conversion.cc (right): https://codereview.chromium.org/2799603002/diff/200001/components/drive/resou... components/drive/resource_entry_conversion.cc:149: bool ConvertTeamDriveChangeResourceToResourceEntry( For FileResource, we have ConvertFileResourceToResourceEntry. This can be ConvertTeamDriveResourceToResourceEntry in the same manner.
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2799603002/diff/160001/components/drive/chrom... File components/drive/chromeos/change_list_processor.cc (right): https://codereview.chromium.org/2799603002/diff/160001/components/drive/chrom... components/drive/chromeos/change_list_processor.cc:499: } else if (parent_resource_id == util::kDriveTeamDrivesDirVirtualResourceId) { On 2017/04/10 11:01:23, hashimoto wrote: > On 2017/04/07 15:31:46, yamaguchi wrote: > > On 2017/04/07 07:56:41, hashimoto wrote: > > > 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)? > > > > 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. > 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()? Thanks. Now I think it should work without introducing the virtual ID. https://codereview.chromium.org/2799603002/diff/200001/components/drive/chrom... File components/drive/chromeos/change_list_processor.cc (right): https://codereview.chromium.org/2799603002/diff/200001/components/drive/chrom... components/drive/chromeos/change_list_processor.cc:23: #include "google_apis/drive/drive_switches.h" On 2017/04/10 11:01:24, hashimoto wrote: > No longer needed. Done. https://codereview.chromium.org/2799603002/diff/200001/components/drive/chrom... components/drive/chromeos/change_list_processor.cc:311: // TODO(yamaguchi): Apply this defensive logic to root directories of On 2017/04/10 11:01:24, hashimoto wrote: > Why do you need to do this? According to crbug.com/297259, the root directory may be sent to client by Drive server's bug. The crbug thread says that, if we receive such data and don't drop such data here, all file will seemingly disappear from the directory list. (because it'll create directory like /others/root and then associate all files there). The same thing could happen with Team Drive root directories, because a root directory of Team Drive don't have parent resource ID. However I am not sure how frequently such a server bug happens. https://codereview.chromium.org/2799603002/diff/200001/components/drive/chrom... File components/drive/chromeos/change_list_processor.h (right): https://codereview.chromium.org/2799603002/diff/200001/components/drive/chrom... components/drive/chromeos/change_list_processor.h:156: // Apply |change| to resource_metadata_. On 2017/04/10 11:01:24, hashimoto wrote: > No longer needed. Done. https://codereview.chromium.org/2799603002/diff/200001/components/drive/resou... File components/drive/resource_entry_conversion.cc (right): https://codereview.chromium.org/2799603002/diff/200001/components/drive/resou... components/drive/resource_entry_conversion.cc:149: bool ConvertTeamDriveChangeResourceToResourceEntry( On 2017/04/10 11:01:24, hashimoto wrote: > For FileResource, we have ConvertFileResourceToResourceEntry. > This can be ConvertTeamDriveResourceToResourceEntry in the same manner. Splitted ConvertTeamDriveResourceToResourceEntry, but this still needs to access a field in ChangeResource. (because team_drive is not filled when deleting) The function will be reused when processing data from TeamDrives:list by another change later.
Message was sent while issue was closed.
https://codereview.chromium.org/2799603002/diff/200001/components/drive/chrom... File components/drive/chromeos/change_list_processor.cc (right): https://codereview.chromium.org/2799603002/diff/200001/components/drive/chrom... components/drive/chromeos/change_list_processor.cc:311: // TODO(yamaguchi): Apply this defensive logic to root directories of On 2017/04/11 07:08:07, yamaguchi wrote: > On 2017/04/10 11:01:24, hashimoto wrote: > > Why do you need to do this? > > According to crbug.com/297259, the root directory may be sent to client by Drive > server's bug. The crbug thread says that, if we receive such data and don't drop > such data here, all file will seemingly disappear from the directory list. > (because it'll create directory like /others/root and then associate all files > there). > The same thing could happen with Team Drive root directories, because a root > directory of Team Drive don't have parent resource ID. > However I am not sure how frequently such a server bug happens. We are doing this for My Drive root because the said bug actually happened. Are you seeing the said problem actually happening with Team Drives? If so, please fix it now, if not, no TODO is needed for it. https://codereview.chromium.org/2799603002/diff/260001/components/drive/chrom... File components/drive/chromeos/change_list_processor.cc (right): https://codereview.chromium.org/2799603002/diff/260001/components/drive/chrom... components/drive/chromeos/change_list_processor.cc:358: if (entry.parent_local_id().compare(util::kDriveTeamDrivesDirLocalId) != 0) { entry.parent_local_id() != util::kDriveTeamDrivesDirLocalId https://codereview.chromium.org/2799603002/diff/260001/components/drive/chrom... components/drive/chromeos/change_list_processor.cc:476: if (parent_resource_id.empty()) { Please merge the team drive handling logic to this function which handles entries with empty parent_resource_id. https://codereview.chromium.org/2799603002/diff/260001/components/drive/resou... File components/drive/resource_entry_conversion.cc (right): https://codereview.chromium.org/2799603002/diff/260001/components/drive/resou... components/drive/resource_entry_conversion.cc:143: out_entry->set_resource_id(input.id()); set_resource_id() is called with input.team_drive_id() so we don't need to do this here. https://codereview.chromium.org/2799603002/diff/260001/components/drive/resou... components/drive/resource_entry_conversion.cc:147: bool ConvertTeamDriveChangeResourceToResourceEntry( This code should be part of ConvertChangeResourceToResourceEntry. https://codereview.chromium.org/2799603002/diff/260001/components/drive/resou... components/drive/resource_entry_conversion.cc:159: if (!input.team_drive_id().empty()) { I don't think we need to do this check https://codereview.chromium.org/2799603002/diff/260001/components/drive/resou... components/drive/resource_entry_conversion.cc:162: if (converted.resource_id().empty()) and this (are you receiving Team Drive resource with empty resource ID from the real server?)
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2799603002/diff/260001/components/drive/chrom... File components/drive/chromeos/change_list_processor.cc (right): https://codereview.chromium.org/2799603002/diff/260001/components/drive/chrom... components/drive/chromeos/change_list_processor.cc:358: if (entry.parent_local_id().compare(util::kDriveTeamDrivesDirLocalId) != 0) { On 2017/04/11 07:55:14, hashimoto wrote: > entry.parent_local_id() != util::kDriveTeamDrivesDirLocalId Done. https://codereview.chromium.org/2799603002/diff/260001/components/drive/chrom... components/drive/chromeos/change_list_processor.cc:476: if (parent_resource_id.empty()) { On 2017/04/11 07:55:14, hashimoto wrote: > Please merge the team drive handling logic to this function which handles > entries with empty parent_resource_id. Done. https://codereview.chromium.org/2799603002/diff/260001/components/drive/resou... File components/drive/resource_entry_conversion.cc (right): https://codereview.chromium.org/2799603002/diff/260001/components/drive/resou... components/drive/resource_entry_conversion.cc:143: out_entry->set_resource_id(input.id()); On 2017/04/11 07:55:14, hashimoto wrote: > set_resource_id() is called with input.team_drive_id() so we don't need to do > this here. Done. This will be needed when we call this function directly to handle TeamDrive::List (which gives TeamDriveResource without ChangeResource) by the next change. I will revive this then (with enough description), but I agree that this is unnecessary now. https://codereview.chromium.org/2799603002/diff/260001/components/drive/resou... components/drive/resource_entry_conversion.cc:147: bool ConvertTeamDriveChangeResourceToResourceEntry( On 2017/04/11 07:55:14, hashimoto wrote: > This code should be part of ConvertChangeResourceToResourceEntry. Done. https://codereview.chromium.org/2799603002/diff/260001/components/drive/resou... components/drive/resource_entry_conversion.cc:159: if (!input.team_drive_id().empty()) { On 2017/04/11 07:55:14, hashimoto wrote: > I don't think we need to do this check Done. https://codereview.chromium.org/2799603002/diff/260001/components/drive/resou... components/drive/resource_entry_conversion.cc:162: if (converted.resource_id().empty()) On 2017/04/11 07:55:14, hashimoto wrote: > and this (are you receiving Team Drive resource with empty resource ID from the > real server?) Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/11 10:22:38, hashimoto wrote: > lgtm Thanks!
The CQ bit was checked by yamaguchi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1491908241798360,
"parent_rev": "c11680219c62c4276972d72d6cb0da91cb96b80e", "commit_rev":
"b1bbca356abfa6f8fdec3cd5815f18b164491a45"}
Message was sent while issue was closed.
Description was changed from ========== Process TeamDrive change in change list. Creates or updates local folder entries of each Team Drive when changelist is received, so that files under Team Drive can link it as their ancestor. Full fetch of Team Drives is not included in this change. TEST=unit_tests --gtest_filter=JobSchedulerTest.*:ResourceEntryConversionTest.*:ResourceMetadataTest.*:ChangeListProcessorTest.* TEST=google_apis_unittests --gtest_filter=DriveAPIParserTest.*:DriveApiUrlGeneratorTest.*:DriveApiRequestsTest.* BUG=none ========== to ========== Process TeamDrive change in change list. Creates or updates local folder entries of each Team Drive when changelist is received, so that files under Team Drive can link it as their ancestor. Full fetch of Team Drives is not included in this change. TEST=unit_tests --gtest_filter=JobSchedulerTest.*:ResourceEntryConversionTest.*:ResourceMetadataTest.*:ChangeListProcessorTest.* TEST=google_apis_unittests --gtest_filter=DriveAPIParserTest.*:DriveApiUrlGeneratorTest.*:DriveApiRequestsTest.* BUG=none Review-Url: https://codereview.chromium.org/2799603002 Cr-Commit-Position: refs/heads/master@{#463597} Committed: https://chromium.googlesource.com/chromium/src/+/b1bbca356abfa6f8fdec3cd5815f... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/b1bbca356abfa6f8fdec3cd5815f... |
