Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11187021:
Add RemoteFileSyncService interface (Closed)
|
Created:
8 years, 2 months ago by tzik Modified:
8 years, 1 month ago CC:
chromium-reviews, tzik+watch_chromium.org, kinuko+watch Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd RemoteFileSyncService interface for Sync FileSystem.
BUG=156041
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=163805
Patch Set 1 #Patch Set 2 : multi-provider #Patch Set 3 : +DCHECK #
Total comments: 19
Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 18
Patch Set 6 : '' #
Total comments: 8
Patch Set 7 : rebased onto separate CL #
Total comments: 2
Patch Set 8 : rebase & fix comment #
Total comments: 4
Patch Set 9 : rebase & address comments #Patch Set 10 : rebase again #
Messages
Total messages: 16 (0 generated)
PTAL
http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... File chrome/browser/sync_file_system/remote_file_provider.h (right): http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:27: virtual bool deleted() = 0; Is it possible to reuse fileapi::FileChange for this purpose? Does a remote change have distinction between kind=file or folder? http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:34: RemoteFileProvider* remote_file_provider) = 0; Maybe... for now I don't think we need this argument? http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:43: virtual void SetObserver(RemoteFileObserver* observer) = 0; Can you add a comment about the expected lifetime of the |observer|? (E.g. the observer needs to be alive etc) Also if we're using usual (non-thread-safe / non-task-runner-safe) observer pattern I think AddObserver()/RemoveObserver() (used together with base::ObserverList) is more common pattern http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:52: // After this method is called, GetRemoteChange() doesn't return remote-side ... GetRemoteChange() won't include ... ? http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:60: virtual scoped_ptr<RemoteChange> GetRemoteChange() = 0; Returns a remote change Can you also comment what happens if it's called when HasRemoteChange == false? http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:62: // Notifies RemoveFileProvider of |change| is applied local filesystem. 'Notifies' may be confusing with observer related methods. "Tells this RemoteFileProvider that the |change| has been applied to the local side." ...? Or maybe simply we can name it DidProcessRemoteChange(...) (Let's be consistent with terms in the method names: Change or RemoteChange) http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:66: // invoked with status and temporary file. Please comment if the temporary file needs to be discarded on the caller side or not. http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:76: // Upon completion, |callback| is invoked with status. Please comment if the caller needs to make sure file_path is unmodified while upload is ongoing. http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... File chrome/browser/sync_file_system/sync_file_system_service.cc (right): http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/sync_file_system_service.cc:103: void SyncFileSystemService::MayStartRemoteSync() { May -> Maybe ? http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... File chrome/browser/sync_file_system/sync_file_system_service.h (right): http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/sync_file_system_service.h:75: std::set<RemoteFileProvider*> pending_providers_; I haven't looked closely at the .cc file but what this 'pending' means?
Thanks! Updated. http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... File chrome/browser/sync_file_system/remote_file_provider.h (right): http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:27: virtual bool deleted() = 0; On 2012/10/17 09:22:57, kinuko wrote: > Is it possible to reuse fileapi::FileChange for this purpose? > Does a remote change have distinction between kind=file or folder? I think it's true iff we use single provider. When we advance to multi provider at some point, we'll need to add some extra data for each change (e.g. md5, resource id, changestamp), and might want to hide them from SyncService. http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:34: RemoteFileProvider* remote_file_provider) = 0; On 2012/10/17 09:22:57, kinuko wrote: > Maybe... for now I don't think we need this argument? It's correct if we start from single provider, as we chatted. http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:43: virtual void SetObserver(RemoteFileObserver* observer) = 0; On 2012/10/17 09:22:57, kinuko wrote: > Can you add a comment about the expected lifetime of the |observer|? (E.g. the > observer needs to be alive etc) > > Also if we're using usual (non-thread-safe / non-task-runner-safe) observer > pattern I think AddObserver()/RemoveObserver() (used together with > base::ObserverList) is more common pattern Done. http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:60: virtual scoped_ptr<RemoteChange> GetRemoteChange() = 0; On 2012/10/17 09:22:57, kinuko wrote: > Returns a remote change > > Can you also comment what happens if it's called when HasRemoteChange == false? Done. http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:62: // Notifies RemoveFileProvider of |change| is applied local filesystem. On 2012/10/17 09:22:57, kinuko wrote: > 'Notifies' may be confusing with observer related methods. > > "Tells this RemoteFileProvider that the |change| has been applied to the local > side." ...? > > Or maybe simply we can name it DidProcessRemoteChange(...) > > (Let's be consistent with terms in the method names: Change or RemoteChange) Done. http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:66: // invoked with status and temporary file. On 2012/10/17 09:22:57, kinuko wrote: > Please comment if the temporary file needs to be discarded on the caller side or > not. Done. http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:76: // Upon completion, |callback| is invoked with status. On 2012/10/17 09:22:57, kinuko wrote: > Please comment if the caller needs to make sure file_path is unmodified while > upload is ongoing. Done. http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... File chrome/browser/sync_file_system/sync_file_system_service.cc (right): http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/sync_file_system_service.cc:103: void SyncFileSystemService::MayStartRemoteSync() { On 2012/10/17 09:22:57, kinuko wrote: > May -> Maybe ? Done. http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... File chrome/browser/sync_file_system/sync_file_system_service.h (right): http://codereview.chromium.org/11187021/diff/1005/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/sync_file_system_service.h:75: std::set<RemoteFileProvider*> pending_providers_; On 2012/10/17 09:22:57, kinuko wrote: > I haven't looked closely at the .cc file but what this 'pending' means? Removed. In this version of patchset, I considered multiprovider case, and stored providers that have pending changes here.
http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... File chrome/browser/sync_file_system/remote_file_provider.h (right): http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:34: // This class represents the changelist of the remote files. This class provides access to the remote side changes. ? http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:43: // remote file changes for |origin|. Registers |origin| to track remote side changes for the |origin|. ? http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:44: virtual void RegisterOrigin(const GURL& origin) = 0; On the second thought I may prefer having this on RemoteFileProvider. Or we should make it clearer if each method is for a current change or for the provider itself. Also in the current interface it's unclear when we populate a next change... I think I still like a simpler traditional design like following: class RemoteChange { bool IsDeletion(); bool IsDirectory(); FileSystemURL& GetURL(); }; class RemoteFileSyncService { explicit RemoteFileSyncService(RemoteChangeObserver* observer); void RegisterOriginForTrackChanges(origin); void UnregisterOriginForTrackChanges(origin); // Returns a next available remote change. // This returns NULL if no changes are available. // The returned change becomes invalid once FinalizeChange() is called on this // change or after this service gets killed. // Also calling this again without finalizing or ungetting the returned change // (by calling FinalizeChange or UngetChange) is invalid. // The ownership of the returned change is owned by this service and must not // be deleted by the caller. RemoteChange* GetNextChange(); // Finalizes the given change. // This must be called before next GetNextChange() is called. void FinalizeChange(RemoteChange*); // Ungets the given change. This is useful when the consumer of the change // is not ready for process and wants to revisit the change later. void UngetChange(RemoteChange*); // ... }; WDYT? Let me know if I'm missing something. (I changed some class names or reverted some name changes; let's talk offline about them... sorry for not being really decisive) http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:48: // remote file changes for |origin|. Unregisters |origin| so that this provider no longer returns changes for the |origin|. http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:52: virtual bool HasRemoteChange() = 0; nit: Change -> Changes ? http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:60: virtual bool IsDeletionChange() = 0; How do we know if a change is for a file or a directory? http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:75: class RemoteFileProvider { For consistency & since this interface is still specific to the sync service can we rename this to RemoteFileSyncService? (Or RemoteFileSyncBackend maybe) http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... File chrome/browser/sync_file_system/sync_file_system_service.cc (right): http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/sync_file_system_service.cc:54: DCHECK(remote_file_provider_); Until we resolve the TODO on line 158 don't we hit this assertion? http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/sync_file_system_service.cc:57: app_url, file_system_context, callback); Hmm... while you're there can you also change this to app_url.GetOrigin() ? http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/sync_file_system_service.cc:105: base::Bind(&SyncFileSystemService::DidDownloadFile, AsWeakPtr())); We also need to handle directory cases. http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... File chrome/browser/sync_file_system/sync_file_system_service.h (right): http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/sync_file_system_service.h:52: typedef std::map<std::string, RemoteFileProvider*> RemoteFileProviderMap; Not necessary for now? http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/sync_file_system_service.h:68: bool sync_is_running_; remote_sync_is_running_ at least for now?
Thanks for reviewing, Kinuko-san. I'm addressing your comment. http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... File chrome/browser/sync_file_system/remote_file_provider.h (right): http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:60: virtual bool IsDeletionChange() = 0; On 2012/10/18 13:54:32, kinuko wrote: > How do we know if a change is for a file or a directory? Change data from Drive server seems not containing mime-type, which is needed to determine the file was file or directory. Is it needed for proceeding RemoteSync? If it's needed, we can add a field to database entry to store that.
Ok I think I have an alternative proposal. The problem is that the remote
sync service wants to control the sync flow since most of sync detail needs
to be hidden inside the remote service, while the current model assumes the
central service takes the overall control.
I think maybe we can reverse the role. What about a model like following?
// This interface is to be implemented/backed by LocalSyncFileService.
class RemoteChangeProcessor {
typedef Callback<void(SyncStatusCode status,
ChangeList& change)> PrepareSyncCallback;
// This must be called before processing the change for the |url|.
// This tries to lock the target |url| and returns the local changes
// if any.
virtual void PrepareSync(
const FileSystemURL& url,
const PrepareSyncCallback& callback) = 0;
// Call this to apply the remote change. If the change type is
// ADD_OR_UPDATE for a file, |local_path| needs to point to a
// local file path that contains the remote file image.
virtual void ApplyChange(
const FileChange& change,
const FilePath& local_path,
const FileSystemURL& url,
const StatusCallback& callback) = 0;
};
class RemoteChangeObserver {
virtual void OnRemoteChangeAvailable(int64 pending_changes);
};
class RemoteFileSyncService {
explicit RemoteFileSyncService(RemoteChangeObserver* observer);
void RegisterOriginForTrackChanges(origin);
void UnregisterOriginForTrackChanges(origin);
// Called by the sync engine to process one remote change.
// After a change is processed |callback| will be called (to return
// the control to the sync engine).
void ProcessChange(RemoteChangeProcessor* processor,
StatusCallback& callback);
};
The sync engine determines when to call remote_service->ProcessChange based
on the # of pending changes/operations in local/remote side.
In this way most of the logic you've wrote in
SyncFileSystemService::MaybeStartRemoteSync() can be implemented in
RemoteFileSyncService::ProcessChange in a more straightforward way.
WDYT?
On Fri, Oct 19, 2012 at 1:32 PM, <tzik@chromium.org> wrote:
> Thanks for reviewing, Kinuko-san. I'm addressing your comment.
>
>
>
> http://codereview.chromium.**org/11187021/diff/2008/chrome/**
>
browser/sync_file_system/**remote_file_provider.h<http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_system/remote_file_provider.h>
> File chrome/browser/sync_file_**system/remote_file_provider.h (right):
>
> http://codereview.chromium.**org/11187021/diff/2008/chrome/**
>
browser/sync_file_system/**remote_file_provider.h#**newcode60<http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_system/remote_file_provider.h#newcode60>
> chrome/browser/sync_file_**system/remote_file_provider.h:**60: virtual
> bool
> IsDeletionChange() = 0;
> On 2012/10/18 13:54:32, kinuko wrote:
>
>> How do we know if a change is for a file or a directory?
>>
>
> Change data from Drive server seems not containing mime-type, which is
> needed to determine the file was file or directory.
> Is it needed for proceeding RemoteSync?
>
> If it's needed, we can add a field to database entry to store that.
>
>
http://codereview.chromium.**org/11187021/<http://codereview.chromium.org/111...
>
On 2012/10/19 14:27:24, kinuko wrote:
> Ok I think I have an alternative proposal. The problem is that the remote
> sync service wants to control the sync flow since most of sync detail needs
> to be hidden inside the remote service, while the current model assumes the
> central service takes the overall control.
> I think maybe we can reverse the role. What about a model like following?
>
> // This interface is to be implemented/backed by LocalSyncFileService.
> class RemoteChangeProcessor {
> typedef Callback<void(SyncStatusCode status,
> ChangeList& change)> PrepareSyncCallback;
>
> // This must be called before processing the change for the |url|.
> // This tries to lock the target |url| and returns the local changes
> // if any.
> virtual void PrepareSync(
> const FileSystemURL& url,
> const PrepareSyncCallback& callback) = 0;
>
> // Call this to apply the remote change. If the change type is
> // ADD_OR_UPDATE for a file, |local_path| needs to point to a
> // local file path that contains the remote file image.
> virtual void ApplyChange(
> const FileChange& change,
> const FilePath& local_path,
> const FileSystemURL& url,
> const StatusCallback& callback) = 0;
> };
>
> class RemoteChangeObserver {
> virtual void OnRemoteChangeAvailable(int64 pending_changes);
> };
>
> class RemoteFileSyncService {
> explicit RemoteFileSyncService(RemoteChangeObserver* observer);
> void RegisterOriginForTrackChanges(origin);
> void UnregisterOriginForTrackChanges(origin);
>
> // Called by the sync engine to process one remote change.
> // After a change is processed |callback| will be called (to return
> // the control to the sync engine).
> void ProcessChange(RemoteChangeProcessor* processor,
> StatusCallback& callback);
> };
>
> The sync engine determines when to call remote_service->ProcessChange based
> on the # of pending changes/operations in local/remote side.
> In this way most of the logic you've wrote in
> SyncFileSystemService::MaybeStartRemoteSync() can be implemented in
> RemoteFileSyncService::ProcessChange in a more straightforward way.
>
> WDYT?
Created a skeleton changeset along this line:
http://codereview.chromium.org/11234025/
> On Fri, Oct 19, 2012 at 1:32 PM, <mailto:tzik@chromium.org> wrote:
>
> > Thanks for reviewing, Kinuko-san. I'm addressing your comment.
> >
> >
> >
> > http://codereview.chromium.**org/11187021/diff/2008/chrome/**
> >
>
browser/sync_file_system/**remote_file_provider.h<http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_system/remote_file_provider.h>
> > File chrome/browser/sync_file_**system/remote_file_provider.h (right):
> >
> > http://codereview.chromium.**org/11187021/diff/2008/chrome/**
> >
>
browser/sync_file_system/**remote_file_provider.h#**newcode60<http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_system/remote_file_provider.h#newcode60>
> > chrome/browser/sync_file_**system/remote_file_provider.h:**60: virtual
> > bool
> > IsDeletionChange() = 0;
> > On 2012/10/18 13:54:32, kinuko wrote:
> >
> >> How do we know if a change is for a file or a directory?
> >>
> >
> > Change data from Drive server seems not containing mime-type, which is
> > needed to determine the file was file or directory.
> > Is it needed for proceeding RemoteSync?
> >
> > If it's needed, we can add a field to database entry to store that.
> >
> >
>
http://codereview.chromium.**org/11187021/%3Chttp://codereview.chromium.org/1...>
> >
Updated. http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... File chrome/browser/sync_file_system/remote_file_provider.h (right): http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:43: // remote file changes for |origin|. On 2012/10/18 13:54:32, kinuko wrote: > Registers |origin| to track remote side changes for the |origin|. ? Done. http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:48: // remote file changes for |origin|. On 2012/10/18 13:54:32, kinuko wrote: > Unregisters |origin| so that this provider no longer returns changes for the > |origin|. Done. http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/remote_file_provider.h:75: class RemoteFileProvider { On 2012/10/18 13:54:32, kinuko wrote: > For consistency & since this interface is still specific to the sync service can > we rename this to RemoteFileSyncService? > (Or RemoteFileSyncBackend maybe) Done. http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... File chrome/browser/sync_file_system/sync_file_system_service.cc (right): http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/sync_file_system_service.cc:57: app_url, file_system_context, callback); On 2012/10/18 13:54:32, kinuko wrote: > Hmm... while you're there can you also change this to app_url.GetOrigin() ? Done. http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... File chrome/browser/sync_file_system/sync_file_system_service.h (right): http://codereview.chromium.org/11187021/diff/2008/chrome/browser/sync_file_sy... chrome/browser/sync_file_system/sync_file_system_service.h:52: typedef std::map<std::string, RemoteFileProvider*> RemoteFileProviderMap; On 2012/10/18 13:54:32, kinuko wrote: > Not necessary for now? Done.
http://codereview.chromium.org/11187021/diff/14002/chrome/browser/sync_file_s... File chrome/browser/sync_file_system/remote_file_sync_service.h (right): http://codereview.chromium.org/11187021/diff/14002/chrome/browser/sync_file_s... chrome/browser/sync_file_system/remote_file_sync_service.h:27: void RemoveObserver(RemoteChangeObserver* observer); It might be ok to leave them pure virtual too.. then we don't need the protected methods as well. http://codereview.chromium.org/11187021/diff/14002/chrome/browser/sync_file_s... chrome/browser/sync_file_system/remote_file_sync_service.h:39: StatusCallback& callback) = 0; I think this should also let the caller know what URL it has worked on (to show status & to handle suspended sync properly). In local service case I'm thinking about using this callback type: typedef base::Callback<void(fileapi::SyncStatusCode status, fileapi::FileSystemURL& url)> SyncCompletionCallback; http://codereview.chromium.org/11234025/diff/12/chrome/browser/sync_file_syst... http://codereview.chromium.org/11187021/diff/14002/chrome/browser/sync_file_s... File chrome/browser/sync_file_system/sync_file_system_callbacks.h (right): http://codereview.chromium.org/11187021/diff/14002/chrome/browser/sync_file_s... chrome/browser/sync_file_system/sync_file_system_callbacks.h:12: typedef base::Callback<void(fileapi::SyncStatusCode)> StatusCallback; Can you move this under webkit/fileapi/syncable and split it into a separate patch (for a new bug)? Probably we can also put SyncCompletionCallback here too (see other comments) http://codereview.chromium.org/11187021/diff/14002/chrome/browser/sync_file_s... File chrome/browser/sync_file_system/sync_file_system_service.cc (right): http://codereview.chromium.org/11187021/diff/14002/chrome/browser/sync_file_s... chrome/browser/sync_file_system/sync_file_system_service.cc:30: // remote_file_service_->RemoveObserver(this); Not sure if this line is really necessary...
http://codereview.chromium.org/11187021/diff/14002/chrome/browser/sync_file_s... File chrome/browser/sync_file_system/remote_file_sync_service.h (right): http://codereview.chromium.org/11187021/diff/14002/chrome/browser/sync_file_s... chrome/browser/sync_file_system/remote_file_sync_service.h:27: void RemoveObserver(RemoteChangeObserver* observer); On 2012/10/23 08:12:14, kinuko wrote: > It might be ok to leave them pure virtual too.. then we don't need the protected > methods as well. Done. http://codereview.chromium.org/11187021/diff/14002/chrome/browser/sync_file_s... chrome/browser/sync_file_system/remote_file_sync_service.h:39: StatusCallback& callback) = 0; On 2012/10/23 08:12:14, kinuko wrote: > I think this should also let the caller know what URL it has worked on (to show > status & to handle suspended sync properly). > > In local service case I'm thinking about using this callback type: > typedef base::Callback<void(fileapi::SyncStatusCode status, > fileapi::FileSystemURL& url)> SyncCompletionCallback; > > http://codereview.chromium.org/11234025/diff/12/chrome/browser/sync_file_syst... Done. http://codereview.chromium.org/11187021/diff/14002/chrome/browser/sync_file_s... File chrome/browser/sync_file_system/sync_file_system_callbacks.h (right): http://codereview.chromium.org/11187021/diff/14002/chrome/browser/sync_file_s... chrome/browser/sync_file_system/sync_file_system_callbacks.h:12: typedef base::Callback<void(fileapi::SyncStatusCode)> StatusCallback; On 2012/10/23 08:12:14, kinuko wrote: > Can you move this under webkit/fileapi/syncable and split it into a separate > patch (for a new bug)? > > Probably we can also put SyncCompletionCallback here too (see other comments) Done at http://codereview.chromium.org/11265002/ http://codereview.chromium.org/11187021/diff/14002/chrome/browser/sync_file_s... File chrome/browser/sync_file_system/sync_file_system_service.cc (right): http://codereview.chromium.org/11187021/diff/14002/chrome/browser/sync_file_s... chrome/browser/sync_file_system/sync_file_system_service.cc:30: // remote_file_service_->RemoveObserver(this); On 2012/10/23 08:12:14, kinuko wrote: > Not sure if this line is really necessary... Done.
lgtm Please update the issue description! http://codereview.chromium.org/11187021/diff/22001/chrome/browser/sync_file_s... File chrome/browser/sync_file_system/sync_file_system_service.cc (right): http://codereview.chromium.org/11187021/diff/22001/chrome/browser/sync_file_s... chrome/browser/sync_file_system/sync_file_system_service.cc:53: remote_file_service_->RegisterOriginForTrackingChanges(app_url.GetOrigin()); I think this line should be added when you add implementation. We can just leave TODO comment like: // TODO(tzik): Call remote_file_service_->RegisterOriginForTrackingChanges() here
Thanks for reviewing, Kinuko-san. Lei, could you take a look as OWNERS of chrome/? http://codereview.chromium.org/11187021/diff/22001/chrome/browser/sync_file_s... File chrome/browser/sync_file_system/sync_file_system_service.cc (right): http://codereview.chromium.org/11187021/diff/22001/chrome/browser/sync_file_s... chrome/browser/sync_file_system/sync_file_system_service.cc:53: remote_file_service_->RegisterOriginForTrackingChanges(app_url.GetOrigin()); On 2012/10/24 05:17:21, kinuko wrote: > I think this line should be added when you add implementation. We can just leave > TODO comment like: > // TODO(tzik): Call remote_file_service_->RegisterOriginForTrackingChanges() > here Done.
lgtm http://codereview.chromium.org/11187021/diff/27001/chrome/browser/sync_file_s... File chrome/browser/sync_file_system/remote_file_sync_service.h (right): http://codereview.chromium.org/11187021/diff/27001/chrome/browser/sync_file_s... chrome/browser/sync_file_system/remote_file_sync_service.h:9: #include "base/observer_list.h" nit: not needed http://codereview.chromium.org/11187021/diff/27001/webkit/fileapi/syncable/sy... File webkit/fileapi/syncable/sync_callbacks.h (right): http://codereview.chromium.org/11187021/diff/27001/webkit/fileapi/syncable/sy... webkit/fileapi/syncable/sync_callbacks.h:17: } // namespace sync_file_system nit: wrong namespace in the comment
Thanks! Updated! http://codereview.chromium.org/11187021/diff/27001/chrome/browser/sync_file_s... File chrome/browser/sync_file_system/remote_file_sync_service.h (right): http://codereview.chromium.org/11187021/diff/27001/chrome/browser/sync_file_s... chrome/browser/sync_file_system/remote_file_sync_service.h:9: #include "base/observer_list.h" On 2012/10/24 05:53:19, Lei Zhang wrote: > nit: not needed Done. http://codereview.chromium.org/11187021/diff/27001/webkit/fileapi/syncable/sy... File webkit/fileapi/syncable/sync_callbacks.h (right): http://codereview.chromium.org/11187021/diff/27001/webkit/fileapi/syncable/sy... webkit/fileapi/syncable/sync_callbacks.h:17: } // namespace sync_file_system On 2012/10/24 05:53:19, Lei Zhang wrote: > nit: wrong namespace in the comment Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tzik@chromium.org/11187021/31003
Retried try job too often for step(s) compile |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
