Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
58953003:
[SyncFS] Refine RemoteToLocalSyncer resolver (Closed)
|
Created:
7 years, 1 month ago by tzik Modified:
7 years, 1 month ago CC:
chromium-reviews, nhiroki+watch_chromium.org, kinuko+watch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[SyncFS] Refine RemoteToLocalSyncer resolver
* Unify analyzing part and dispatching part
* Complete missing cases handling
* Add DCHECKs
BUG=240165
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234370
Patch Set 1 #Patch Set 2 : split largest_changeid part to other CL #Patch Set 3 : fix #Patch Set 4 : +DCHECK #Patch Set 5 : drop unused field #
Total comments: 4
Patch Set 6 : rebase #
Total comments: 18
Patch Set 7 : rebase #Patch Set 8 : buildfix #
Messages
Total messages: 15 (0 generated)
PTL
lgtm Can you please summarize why you needed to change from boolean flags to this design in the description? thx (+nhiroki for optional second review) https://codereview.chromium.org/58953003/diff/110001/chrome/browser/sync_file... File chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h (right): https://codereview.chromium.org/58953003/diff/110001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:48: // Dispatches remote change to handleing functions or to SyncCompleted() handleing -> handler or handling https://codereview.chromium.org/58953003/diff/110001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:53: // # The file is listed in a folder soon before this operation. soon before -> you mean 'right before' ? https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... File chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.cc (right): https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.cc:217: << dirty_tracker_->file_id(); Is this a valid (possible) case or notreached? https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.cc:263: callback); We just finish sync after fetching remote details in missing remote metadata case? (We don't call SyncCompleted so this just appears to be noop in terms of sync and we should be fine?) https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... File chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h (right): https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:70: // Else, if the tracker have different title between its synced metadata and nit: has different titles https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:167: scoped_ptr<FileTracker> dirty_tracker_; nit: Why was this changed to scoped_ptr? Looks like it's just adding more DCHECKs? Or will we reset this somewhere during the sync?
Looks good. https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... File chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.cc (right): https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.cc:188: << remote_details.file_kind(); nit: Line 184 says "remote and local", but line 187 says "local.file_kind() vs remote.file_kind()". There seems to be an inconsistent order (remote->local vs local->remote). I'd prefer to make them consistent or to specify labels like this: << " type: (local)" << synced_details.file_kind() << " vs (remote)" << remote_details.file_kind(); https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... File chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h (right): https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:48: // Dispatches remote change to handleing functions or to SyncCompleted() handleing -> handling https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:57: // - Dispatch to HandleInactiveFile() to resolve offline solvable dirtiness. HandleInactiveFile() -> HandleInactiveTracker() ? https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:65: // - Else, the file should be a unsupported active file. This should not a unsupported -> an unsupported https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:86: // - Dispatch to HandleFolderListing() HandleFolderListing() -> HandleFolderContentListing()
> Can you please summarize why you needed to change from boolean flags to this > design in the description? thx The refinement itself is needed for complete the resolver for all case. For migration from boolean variables, both previous analyze/dispatch functions have basically same logic, since we need to check metadata status to check variable availability. So, I'd unify them to make it less error prone. https://codereview.chromium.org/58953003/diff/110001/chrome/browser/sync_file... File chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h (right): https://codereview.chromium.org/58953003/diff/110001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:48: // Dispatches remote change to handleing functions or to SyncCompleted() On 2013/11/09 23:33:25, kinuko (OOO until Nov 13) wrote: > handleing -> handler or handling Done. https://codereview.chromium.org/58953003/diff/110001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:53: // # The file is listed in a folder soon before this operation. On 2013/11/09 23:33:25, kinuko (OOO until Nov 13) wrote: > soon before -> you mean 'right before' ? Done. https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... File chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.cc (right): https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.cc:188: << remote_details.file_kind(); On 2013/11/11 03:37:17, nhiroki wrote: > nit: Line 184 says "remote and local", but line 187 says "local.file_kind() vs > remote.file_kind()". There seems to be an inconsistent order (remote->local vs > local->remote). > > I'd prefer to make them consistent or to specify labels like this: > > << " type: (local)" << synced_details.file_kind() << " vs (remote)" > << remote_details.file_kind(); Done. https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.cc:217: << dirty_tracker_->file_id(); On 2013/11/09 23:33:25, kinuko (OOO until Nov 13) wrote: > Is this a valid (possible) case or notreached? Fixed. It should be NOTREACHED. https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.cc:263: callback); On 2013/11/09 23:33:25, kinuko (OOO until Nov 13) wrote: > We just finish sync after fetching remote details in missing remote metadata > case? (We don't call SyncCompleted so this just appears to be noop in terms of > sync and we should be fine?) Changed to invoke SyncCompleted. Finishing sync without actual sync operation should be fine, it should be handled in next sync phase. https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... File chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h (right): https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:48: // Dispatches remote change to handleing functions or to SyncCompleted() On 2013/11/11 03:37:17, nhiroki wrote: > handleing -> handling Done. https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:57: // - Dispatch to HandleInactiveFile() to resolve offline solvable dirtiness. On 2013/11/11 03:37:17, nhiroki wrote: > HandleInactiveFile() -> HandleInactiveTracker() ? Done. https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:65: // - Else, the file should be a unsupported active file. This should not On 2013/11/11 03:37:17, nhiroki wrote: > a unsupported -> an unsupported Done. https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:70: // Else, if the tracker have different title between its synced metadata and On 2013/11/09 23:33:25, kinuko (OOO until Nov 13) wrote: > nit: has different titles Done. https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:86: // - Dispatch to HandleFolderListing() On 2013/11/11 03:37:17, nhiroki wrote: > HandleFolderListing() -> HandleFolderContentListing() Done. https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:167: scoped_ptr<FileTracker> dirty_tracker_; On 2013/11/09 23:33:25, kinuko (OOO until Nov 13) wrote: > nit: Why was this changed to scoped_ptr? Looks like it's just adding more > DCHECKs? Or will we reset this somewhere during the sync? It's just for adding DCHECK to assert we follow the call chain in right order.
lgtm
On 2013/11/11 04:45:34, tzik wrote: > > Can you please summarize why you needed to change from boolean flags to this > > design in the description? thx > > The refinement itself is needed for complete the resolver for all case. > For migration from boolean variables, both previous analyze/dispatch functions > have basically same logic, since we need to check metadata status to check > variable availability. > So, I'd unify them to make it less error prone. pls write that in the CL description (my comment's intention was to make each CL have decent meaningful context, not only for my own understanding), thx. > https://codereview.chromium.org/58953003/diff/110001/chrome/browser/sync_file... > File chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h > (right): > > https://codereview.chromium.org/58953003/diff/110001/chrome/browser/sync_file... > chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:48: // > Dispatches remote change to handleing functions or to SyncCompleted() > On 2013/11/09 23:33:25, kinuko (OOO until Nov 13) wrote: > > handleing -> handler or handling > > Done. > > https://codereview.chromium.org/58953003/diff/110001/chrome/browser/sync_file... > chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:53: // > # The file is listed in a folder soon before this operation. > On 2013/11/09 23:33:25, kinuko (OOO until Nov 13) wrote: > > soon before -> you mean 'right before' ? > > Done. > > https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... > File chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.cc > (right): > > https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... > chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.cc:188: << > remote_details.file_kind(); > On 2013/11/11 03:37:17, nhiroki wrote: > > nit: Line 184 says "remote and local", but line 187 says "local.file_kind() vs > > remote.file_kind()". There seems to be an inconsistent order (remote->local vs > > local->remote). > > > > I'd prefer to make them consistent or to specify labels like this: > > > > << " type: (local)" << synced_details.file_kind() << " vs (remote)" > > << remote_details.file_kind(); > > Done. > > https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... > chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.cc:217: << > dirty_tracker_->file_id(); > On 2013/11/09 23:33:25, kinuko (OOO until Nov 13) wrote: > > Is this a valid (possible) case or notreached? > > Fixed. > It should be NOTREACHED. > > https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... > chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.cc:263: > callback); > On 2013/11/09 23:33:25, kinuko (OOO until Nov 13) wrote: > > We just finish sync after fetching remote details in missing remote metadata > > case? (We don't call SyncCompleted so this just appears to be noop in terms of > > sync and we should be fine?) > > Changed to invoke SyncCompleted. > Finishing sync without actual sync operation should be fine, it should be > handled in next sync phase. > > https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... > File chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h > (right): > > https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... > chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:48: // > Dispatches remote change to handleing functions or to SyncCompleted() > On 2013/11/11 03:37:17, nhiroki wrote: > > handleing -> handling > > Done. > > https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... > chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:57: // > - Dispatch to HandleInactiveFile() to resolve offline solvable dirtiness. > On 2013/11/11 03:37:17, nhiroki wrote: > > HandleInactiveFile() -> HandleInactiveTracker() ? > > Done. > > https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... > chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:65: // > - Else, the file should be a unsupported active file. This should not > On 2013/11/11 03:37:17, nhiroki wrote: > > a unsupported -> an unsupported > > Done. > > https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... > chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:70: // > Else, if the tracker have different title between its synced metadata and > On 2013/11/09 23:33:25, kinuko (OOO until Nov 13) wrote: > > nit: has different titles > > Done. > > https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... > chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:86: // > - Dispatch to HandleFolderListing() > On 2013/11/11 03:37:17, nhiroki wrote: > > HandleFolderListing() -> HandleFolderContentListing() > > Done. > > https://codereview.chromium.org/58953003/diff/130001/chrome/browser/sync_file... > chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h:167: > scoped_ptr<FileTracker> dirty_tracker_; > On 2013/11/09 23:33:25, kinuko (OOO until Nov 13) wrote: > > nit: Why was this changed to scoped_ptr? Looks like it's just adding more > > DCHECKs? Or will we reset this somewhere during the sync? > > It's just for adding DCHECK to assert we follow the call chain in right order.
On 2013/11/11 05:02:23, kinuko (OOO until Nov 13) wrote: > On 2013/11/11 04:45:34, tzik wrote: > > > Can you please summarize why you needed to change from boolean flags to this > > > design in the description? thx > > > > The refinement itself is needed for complete the resolver for all case. > > For migration from boolean variables, both previous analyze/dispatch functions > > have basically same logic, since we need to check metadata status to check > > variable availability. > > So, I'd unify them to make it less error prone. > > pls write that in the CL description (my comment's intention was to make each CL > have decent meaningful context, not only for my own understanding), thx. Ah, I see. Done!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tzik@chromium.org/58953003/290001
Sorry for I got bad news for ya. Compile failed with a clobber build on win7_aura. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tzik@chromium.org/58953003/580001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tzik@chromium.org/58953003/580001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tzik@chromium.org/58953003/580001
Message was sent while issue was closed.
Change committed as 234370 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
