SyncFS: Add resource type field into DriveMetadata

SyncFS: Add resource type field into DriveMetadata


BUG=231273
TEST=pass all existing tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194552

[nhiroki, 2013-04-16 10:50:15]

Can you review this? Thanks!

[kinuko, 2013-04-16 11:14:45]

https://codereview.chromium.org/13929012/diff/3001/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/sync_file_system.proto (right):

https://codereview.chromium.org/13929012/diff/3001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/sync_file_system.proto:22: optional bool
is_folder = 5;
maybe [default = false] ?

Alternatively we can add a type field with enum like

enum ResourceType {
 FILE = 0;
 FOLDER = 1;
}
optional ResourceType type = 5 [default = FILE];

[nhiroki, 2013-04-16 11:37:07]

Updated, thanks!

https://codereview.chromium.org/13929012/diff/3001/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/sync_file_system.proto (right):

https://codereview.chromium.org/13929012/diff/3001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/sync_file_system.proto:22: optional bool
is_folder = 5;
On 2013/04/16 11:14:45, kinuko wrote:
> maybe [default = false] ?
> 
> Alternatively we can add a type field with enum like
> 
> enum ResourceType {
>  FILE = 0;
>  FOLDER = 1;
> }
> optional ResourceType type = 5 [default = FILE];

Done.

[kinuko, 2013-04-16 12:10:19]

lgtm if the protobuf change looks good to tzik@

https://codereview.chromium.org/13929012/diff/13003/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/13929012/diff/13003/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1141:
DCHECK_EQ(DriveMetadata::FILE, param->drive_metadata.type());
It feels slightly weird to have these occasional DCHECKs while we don't have
actual code around.

[tzik, 2013-04-16 15:02:49]

lgtm

https://codereview.chromium.org/13929012/diff/13003/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/sync_file_system.proto (right):

https://codereview.chromium.org/13929012/diff/13003/chrome/browser/sync_file_...
chrome/browser/sync_file_system/sync_file_system.proto:24: FOLDER = 1;
RESOURCE_TYPE_FILE and RESOURCE_TYPE_FOLDER for better readability?
DriveMetadata::FILE does not tell the type of the value.

[nhiroki, 2013-04-17 02:24:51]

Addressed comments, thanks!

https://codereview.chromium.org/13929012/diff/13003/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/13929012/diff/13003/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1141:
DCHECK_EQ(DriveMetadata::FILE, param->drive_metadata.type());
On 2013/04/16 12:10:19, kinuko wrote:
> It feels slightly weird to have these occasional DCHECKs while we don't have
> actual code around.

Okay, recalled them for now.

https://codereview.chromium.org/13929012/diff/13003/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/sync_file_system.proto (right):

https://codereview.chromium.org/13929012/diff/13003/chrome/browser/sync_file_...
chrome/browser/sync_file_system/sync_file_system.proto:24: FOLDER = 1;
On 2013/04/16 15:02:49, tzik wrote:
> RESOURCE_TYPE_FILE and RESOURCE_TYPE_FOLDER for better readability?
> DriveMetadata::FILE does not tell the type of the value.

Done.

[nhiroki, 2013-04-17 09:19:29]

Committed patchset #4 manually as r194552 (presubmit successful).