[SyncFileSystem] Separate out ApplyLocalChange from DriveFileSyncService.

[SyncFileSystem] Separate out ApplyLocalChange from DriveFileSyncService.

This CL introduces drive::LocalChangeProcessorDelegate to handle
 callback chain for ApplyLocalChange::ApplyLocalChange.

BUG=240588
TEST=existing test

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

[tzik, 2013-05-16 06:30:43]

PTL

[kinuko, 2013-05-16 06:57:49]

looks good, is it possible for you to separate out semantic changes in conflict
handling from this CL?  It's much easier to review if this change only contains
mechanical refactoring.

https://codereview.chromium.org/14977008/diff/19001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/apply_local_change_delegate.cc
(right):

https://codereview.chromium.org/14977008/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/apply_local_change_delegate.cc:29:
ApplyLocalChangeDelegate::ApplyLocalChangeDelegate(
nit: method order different from .h

https://codereview.chromium.org/14977008/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/apply_local_change_delegate.cc:57:
base::Bind(&OwnDelegateInstance, base::Owned(delegate), callback);
I understand this class's very self-contained and looks clean, but as we chatted
offline it concerns me that we lose lifetime control once we launched. (E.g. we
may want to cancel an ongoing sync task, and in the current code there's no way
to do so other than killing SyncService).

https://codereview.chromium.org/14977008/diff/19001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/apply_local_change_delegate.h
(right):

https://codereview.chromium.org/14977008/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/apply_local_change_delegate.h:24: class
ApplyLocalChangeDelegate {
ApplyLocalChangeDelete::ApplyLocalChange() ...

LocalChangeProcessorDelegate::ApplyLocalChange() maybe?

[tzik, 2013-05-16 11:06:14]

Updated!

https://codereview.chromium.org/14977008/diff/19001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/apply_local_change_delegate.cc
(right):

https://codereview.chromium.org/14977008/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/apply_local_change_delegate.cc:29:
ApplyLocalChangeDelegate::ApplyLocalChangeDelegate(
On 2013/05/16 06:57:49, kinuko wrote:
> nit: method order different from .h

Done.

https://codereview.chromium.org/14977008/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/apply_local_change_delegate.cc:57:
base::Bind(&OwnDelegateInstance, base::Owned(delegate), callback);
On 2013/05/16 06:57:49, kinuko wrote:
> I understand this class's very self-contained and looks clean, but as we
chatted
> offline it concerns me that we lose lifetime control once we launched. (E.g.
we
> may want to cancel an ongoing sync task, and in the current code there's no
way
> to do so other than killing SyncService).

Done. I added |running_local_sync_task_| to DriveFileSyncService to cancel the
task.

https://codereview.chromium.org/14977008/diff/19001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/apply_local_change_delegate.h
(right):

https://codereview.chromium.org/14977008/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/apply_local_change_delegate.h:24: class
ApplyLocalChangeDelegate {
On 2013/05/16 06:57:49, kinuko wrote:
> ApplyLocalChangeDelete::ApplyLocalChange() ...
> 
> LocalChangeProcessorDelegate::ApplyLocalChange() maybe?

Done.

[kinuko, 2013-05-16 13:37:37]

Looking good (still), some more comments.

https://codereview.chromium.org/14977008/diff/28001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/local_change_processor_delegate.cc
(right):

https://codereview.chromium.org/14977008/diff/28001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/local_change_processor_delegate.cc:447:
const SyncStatusCallback& callback) {
This could do more than resetting MD5 if the given resource_id and type is
different from the original one, right?   I expect code like following by this
method name:

void ResetMD5(const SyncStatusCallback& callback) {
   drive_metadata_.set_md5_checksum(std::string());
   metadata_store()->UpdateEntry(url_, drive_metadata_, callback);
}

Otherwise (if the caller wants to do more than resetting md5) the caller should
just call UpdateMetadata().

https://codereview.chromium.org/14977008/diff/28001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/local_change_processor_delegate.cc:544:
remote_file_type, remote_modification_time)) {
nit: can you move this method calling before switch()?

https://codereview.chromium.org/14977008/diff/28001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/local_change_processor_delegate.h
(right):

https://codereview.chromium.org/14977008/diff/28001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/local_change_processor_delegate.h:24:
class LocalChangeProcessorDelegate {
Can you add a class-level comment? Also please note about the lifetime of an
instance and warning that any async method of this class first check if
sync_service_ is null or not.

https://codereview.chromium.org/14977008/diff/28001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/local_change_processor_delegate.h:89:
const SyncStatusCallback& callback);
Can we include 'Metadata' in all metadata-related method names?  E.g.

ResetMetadataMD5
SetMetadataToBeFetched
SetMetadataConflict

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

https://codereview.chromium.org/14977008/diff/28001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1495:
DriveFileSyncService::SyncFileTypeToDriveMetadataResourceType(
nit: maybe we can factor out these util methods into a separate class later. (No
need to do it in this CL)

[tzik, 2013-05-17 05:14:10]

Updated! PTAL

https://codereview.chromium.org/14977008/diff/28001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/local_change_processor_delegate.cc
(right):

https://codereview.chromium.org/14977008/diff/28001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/local_change_processor_delegate.cc:447:
const SyncStatusCallback& callback) {
On 2013/05/16 13:37:37, kinuko wrote:
> This could do more than resetting MD5 if the given resource_id and type is
> different from the original one, right?   I expect code like following by this
> method name:
> 
> void ResetMD5(const SyncStatusCallback& callback) {
>    drive_metadata_.set_md5_checksum(std::string());
>    metadata_store()->UpdateEntry(url_, drive_metadata_, callback);
> }
> 
> Otherwise (if the caller wants to do more than resetting md5) the caller
should
> just call UpdateMetadata().

Done.

https://codereview.chromium.org/14977008/diff/28001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/local_change_processor_delegate.cc:544:
remote_file_type, remote_modification_time)) {
On 2013/05/16 13:37:37, kinuko wrote:
> nit: can you move this method calling before switch()?

Done.

https://codereview.chromium.org/14977008/diff/28001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/local_change_processor_delegate.h
(right):

https://codereview.chromium.org/14977008/diff/28001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/local_change_processor_delegate.h:24:
class LocalChangeProcessorDelegate {
On 2013/05/16 13:37:37, kinuko wrote:
> Can you add a class-level comment? Also please note about the lifetime of an
> instance and warning that any async method of this class first check if
> sync_service_ is null or not.

Done.

https://codereview.chromium.org/14977008/diff/28001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/local_change_processor_delegate.h:89:
const SyncStatusCallback& callback);
On 2013/05/16 13:37:37, kinuko wrote:
> Can we include 'Metadata' in all metadata-related method names?  E.g.
> 
> ResetMetadataMD5
> SetMetadataToBeFetched
> SetMetadataConflict

Done.

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

https://codereview.chromium.org/14977008/diff/28001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1495:
DriveFileSyncService::SyncFileTypeToDriveMetadataResourceType(
On 2013/05/16 13:37:37, kinuko wrote:
> nit: maybe we can factor out these util methods into a separate class later.
(No
> need to do it in this CL)

Done.

[nhiroki, 2013-05-17 06:02:14]

Cool! Thank you for working on this. LGTM

https://codereview.chromium.org/14977008/diff/46001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/local_change_processor_delegate.cc
(right):

https://codereview.chromium.org/14977008/diff/46001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/local_change_processor_delegate.cc:618: } 
// drive
nit: "// namespace drive"?

https://codereview.chromium.org/14977008/diff/46001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/local_change_processor_delegate.h
(right):

https://codereview.chromium.org/14977008/diff/46001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/local_change_processor_delegate.h:22:
class APIUtil;
nit: you may want to add a blank line between 21 and 22.

[kinuko, 2013-05-17 06:19:23]

Ok, lgtm.

nit: Please wrap change description around 72-80 col or so:
https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-de...

[tzik, 2013-05-17 07:49:25]

Thanks!

https://codereview.chromium.org/14977008/diff/46001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/local_change_processor_delegate.cc
(right):

https://codereview.chromium.org/14977008/diff/46001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/local_change_processor_delegate.cc:618: } 
// drive
On 2013/05/17 06:02:14, nhiroki wrote:
> nit: "// namespace drive"?

Done.

https://codereview.chromium.org/14977008/diff/46001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/local_change_processor_delegate.h
(right):

https://codereview.chromium.org/14977008/diff/46001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/local_change_processor_delegate.h:22:
class APIUtil;
On 2013/05/17 06:02:14, nhiroki wrote:
> nit: you may want to add a blank line between 21 and 22.

Done.

[commit-bot: I haz the power, 2013-05-17 07:49:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/14977008/50001

[commit-bot: I haz the power, 2013-05-17 13:51:08]

Change committed as 200809