|
|
DescriptionParse TeamDrive resource inside ChangeList.
Changes:list can now return changes for Team Drive entries instead of
file/directory entries.
See https://developers.google.com/drive/v2/reference/changes/list
BUG=684274
TEST=google_apis_unittests --gtest_filter=DriveAPIParserTest.*
Review-Url: https://codereview.chromium.org/2748053005
Cr-Commit-Position: refs/heads/master@{#460010}
Committed: https://chromium.googlesource.com/chromium/src/+/c11f1ba110894a5188d40ab032e7f0c02f3ea279
Patch Set 1 #Patch Set 2 : Parse type field as well. #Patch Set 3 : rebase #Patch Set 4 : Indent array initializers by 2 #Patch Set 5 : Add field names to fetch. #Patch Set 6 : Revert unrelated change. #Patch Set 7 : Initialize with UNKNOWN. #Patch Set 8 : fix error #Patch Set 9 : rebase #
Total comments: 14
Patch Set 10 : Fix reflecting review comments #Patch Set 11 : DCHECK before reading resource-type-specific fields. #
Total comments: 2
Patch Set 12 : Use DCHECK_EQ wherever possible #Patch Set 13 : Check type before reading file() in fake_drive_service. #Patch Set 14 : Copy type field. #Patch Set 15 : Fill type field when generating ChangeResource in FakeDriveService. #Patch Set 16 : Set type field in all test that create ChangeResource object. #Patch Set 17 : Revert changes after patch set 15. Default type to FILE for backward compatibility. #Patch Set 18 : Initialize type field in many tests. #Patch Set 19 : rebase #
Total comments: 4
Patch Set 20 : Default the type field to UNKNOWN as it's not causing new test failures. #Messages
Total messages: 109 (89 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...
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 ========== Parse TeamDrive resource inside ChangeList. Changes:list can now return changes for Team Drive entries instead of file/directory entries. https://developers.google.com/drive/v2/reference/changes/list BUG=684274 TEST=google_apis_unittests --gtest_filter=DriveAPIParserTest.* ========== to ========== Parse TeamDrive resource inside ChangeList. Changes:list can now return changes for Team Drive entries instead of file/directory entries. See https://developers.google.com/drive/v2/reference/changes/list BUG=684274 TEST=google_apis_unittests --gtest_filter=DriveAPIParserTest.* ==========
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.
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.
yamaguchi@chromium.org changed reviewers: + hashimoto@chromium.org
ptal
Sorry for being late to respond. https://codereview.chromium.org/2748053005/diff/160001/components/drive/servi... File components/drive/service/drive_api_service.cc (right): https://codereview.chromium.org/2748053005/diff/160001/components/drive/servi... components/drive/service/drive_api_service.cc:125: "lastViewedByMeDate,shared),teamDrive(kind,id),teamDriveId," The "teamDrive" dictionary you are adding to test/data/drive/changelist.json contains fields other than "kind and id". Please be consistent. https://codereview.chromium.org/2748053005/diff/160001/google_apis/drive/driv... File google_apis/drive/drive_api_parser.cc (right): https://codereview.chromium.org/2748053005/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_parser.cc:218: const ChangeTypeMap kChangeTypeMap[] = { Please use constexpr. https://codereview.chromium.org/2748053005/diff/160001/google_apis/drive/driv... File google_apis/drive/drive_api_parser.h (right): https://codereview.chromium.org/2748053005/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_parser.h:777: // Filled only when type == FILE. What does "filled" here mean? Please use a more specific term (e.g. non-empty) to clarify. Does it make sense to add DCHEK about the value of |type| to this function? https://codereview.chromium.org/2748053005/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_parser.h:784: // Filled only when type == FILE. "Filled" here is ambiguous. If type != FILE, it's null, or FileResource w/ invalid values? Does it make sense to add DCHECK? https://codereview.chromium.org/2748053005/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_parser.h:789: // Filled only when type == TEAM_DRIVE. ditto. https://codereview.chromium.org/2748053005/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_parser.h:793: // Returns the ID of the Team Drive. Filled only when type == TEAM_DRIVE. ditto. https://codereview.chromium.org/2748053005/diff/160001/google_apis/drive/driv... File google_apis/drive/drive_api_parser_unittest.cc (right): https://codereview.chromium.org/2748053005/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_parser_unittest.cc:305: const TeamDriveCapabilities& capabilities = Lines after this look redundant as DriveAPIParserTest.TeamDriveResourceParser is already testing the same thing.
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/2748053005/diff/160001/components/drive/servi... File components/drive/service/drive_api_service.cc (right): https://codereview.chromium.org/2748053005/diff/160001/components/drive/servi... components/drive/service/drive_api_service.cc:125: "lastViewedByMeDate,shared),teamDrive(kind,id),teamDriveId," On 2017/03/22 06:40:30, hashimoto wrote: > The "teamDrive" dictionary you are adding to test/data/drive/changelist.json > contains fields other than "kind and id". > Please be consistent. Done. https://codereview.chromium.org/2748053005/diff/160001/google_apis/drive/driv... File google_apis/drive/drive_api_parser.cc (right): https://codereview.chromium.org/2748053005/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_parser.cc:218: const ChangeTypeMap kChangeTypeMap[] = { On 2017/03/22 06:40:30, hashimoto wrote: > Please use constexpr. Done. https://codereview.chromium.org/2748053005/diff/160001/google_apis/drive/driv... File google_apis/drive/drive_api_parser.h (right): https://codereview.chromium.org/2748053005/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_parser.h:777: // Filled only when type == FILE. On 2017/03/22 06:40:30, hashimoto wrote: > What does "filled" here mean? > Please use a more specific term (e.g. non-empty) to clarify. > > Does it make sense to add DCHEK about the value of |type| to this function? It means uninitialized. Also added DCHECK. https://codereview.chromium.org/2748053005/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_parser.h:784: // Filled only when type == FILE. On 2017/03/22 06:40:30, hashimoto wrote: > "Filled" here is ambiguous. > If type != FILE, it's null, or FileResource w/ invalid values? > > Does it make sense to add DCHECK? It'll be empty (owns nothing). Added DCHECK. https://codereview.chromium.org/2748053005/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_parser.h:789: // Filled only when type == TEAM_DRIVE. On 2017/03/22 06:40:30, hashimoto wrote: > ditto. Done. https://codereview.chromium.org/2748053005/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_parser.h:793: // Returns the ID of the Team Drive. Filled only when type == TEAM_DRIVE. On 2017/03/22 06:40:30, hashimoto wrote: > ditto. Done. https://codereview.chromium.org/2748053005/diff/160001/google_apis/drive/driv... File google_apis/drive/drive_api_parser_unittest.cc (right): https://codereview.chromium.org/2748053005/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_parser_unittest.cc:305: const TeamDriveCapabilities& capabilities = On 2017/03/22 06:40:30, hashimoto wrote: > Lines after this look redundant as DriveAPIParserTest.TeamDriveResourceParser is > already testing the same thing. Done.
https://codereview.chromium.org/2748053005/diff/200001/google_apis/drive/driv... File google_apis/drive/drive_api_parser.h (right): https://codereview.chromium.org/2748053005/diff/200001/google_apis/drive/driv... google_apis/drive/drive_api_parser.h:779: DCHECK(type_ == FILE); Please use DCHECK_EQ for better error message.
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/2748053005/diff/200001/google_apis/drive/driv... File google_apis/drive/drive_api_parser.h (right): https://codereview.chromium.org/2748053005/diff/200001/google_apis/drive/driv... google_apis/drive/drive_api_parser.h:779: DCHECK(type_ == FILE); On 2017/03/22 07:34:13, hashimoto wrote: > Please use DCHECK_EQ for better error message. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 yamaguchi@chromium.org
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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 yamaguchi@chromium.org
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: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
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: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
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 yamaguchi@chromium.org
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: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
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: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
The CQ bit was unchecked by yamaguchi@chromium.org
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: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
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.
Will you take a look again, because Patch set 12 had caused several existing tests crash (in unit_tests and browser_tests)?
https://codereview.chromium.org/2748053005/diff/360001/google_apis/drive/driv... File google_apis/drive/drive_api_parser.cc (right): https://codereview.chromium.org/2748053005/diff/360001/google_apis/drive/driv... google_apis/drive/drive_api_parser.cc:692: : change_id_(0), type_(FILE), deleted_(false) {} You are already fixing a number of tests. Do we still need this?
https://codereview.chromium.org/2748053005/diff/360001/google_apis/drive/driv... File google_apis/drive/drive_api_parser.cc (right): https://codereview.chromium.org/2748053005/diff/360001/google_apis/drive/driv... google_apis/drive/drive_api_parser.cc:692: : change_id_(0), type_(FILE), deleted_(false) {} On 2017/03/24 08:49:23, hashimoto wrote: > You are already fixing a number of tests. > Do we still need this? At least, 5 tests inside FileCacheTest fails if we make it default to UNKNOWN at the moment. So I would like to make this a TODO and fix later.
https://codereview.chromium.org/2748053005/diff/360001/google_apis/drive/driv... File google_apis/drive/drive_api_parser.cc (right): https://codereview.chromium.org/2748053005/diff/360001/google_apis/drive/driv... google_apis/drive/drive_api_parser.cc:692: : change_id_(0), type_(FILE), deleted_(false) {} On 2017/03/24 12:15:21, yamaguchi wrote: > On 2017/03/24 08:49:23, hashimoto wrote: > > You are already fixing a number of tests. > > Do we still need this? > > At least, 5 tests inside FileCacheTest fails if we make it default to UNKNOWN at > the moment. So I would like to make this a TODO and fix later. FileCache should have nothing to do with ChangeResource, as it only deals with ResourceEntry. How can it fail with this change?
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.
https://codereview.chromium.org/2748053005/diff/360001/google_apis/drive/driv... File google_apis/drive/drive_api_parser.cc (right): https://codereview.chromium.org/2748053005/diff/360001/google_apis/drive/driv... google_apis/drive/drive_api_parser.cc:692: : change_id_(0), type_(FILE), deleted_(false) {} On 2017/03/27 08:47:41, hashimoto wrote: > On 2017/03/24 12:15:21, yamaguchi wrote: > > On 2017/03/24 08:49:23, hashimoto wrote: > > > You are already fixing a number of tests. > > > Do we still need this? > > > > At least, 5 tests inside FileCacheTest fails if we make it default to UNKNOWN > at > > the moment. So I would like to make this a TODO and fix later. > > FileCache should have nothing to do with ChangeResource, as it only deals with > ResourceEntry. > How can it fail with this change? Now I found that those failures are not something introduced by this change. So I resolved the TODO now.
lgtm
The CQ bit was checked by yamaguchi@chromium.org
The CQ bit was unchecked by yamaguchi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
yamaguchi@chromium.org changed reviewers: + nhiroki@chromium.org
To nhiroki@: Will you review and approve the change for chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc?
sync_file_system lgtm
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": 380001, "attempt_start_ts": 1490674284797390, "parent_rev": "2cc9de46038c9ed3d3959cece913d02759e6b9df", "commit_rev": "c11f1ba110894a5188d40ab032e7f0c02f3ea279"}
Message was sent while issue was closed.
Description was changed from ========== Parse TeamDrive resource inside ChangeList. Changes:list can now return changes for Team Drive entries instead of file/directory entries. See https://developers.google.com/drive/v2/reference/changes/list BUG=684274 TEST=google_apis_unittests --gtest_filter=DriveAPIParserTest.* ========== to ========== Parse TeamDrive resource inside ChangeList. Changes:list can now return changes for Team Drive entries instead of file/directory entries. See https://developers.google.com/drive/v2/reference/changes/list BUG=684274 TEST=google_apis_unittests --gtest_filter=DriveAPIParserTest.* Review-Url: https://codereview.chromium.org/2748053005 Cr-Commit-Position: refs/heads/master@{#460010} Committed: https://chromium.googlesource.com/chromium/src/+/c11f1ba110894a5188d40ab032e7... ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001) as https://chromium.googlesource.com/chromium/src/+/c11f1ba110894a5188d40ab032e7... |