Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(40)

Issue 13986011: SyncFS: Introduce RemoteSyncOperationResolver for directory operation support (Closed)

Created:
7 years, 8 months ago by nhiroki
Modified:
7 years, 7 months ago
Reviewers:
kinuko, Lei Zhang, tzik
CC:
chromium-reviews, tzik+watch_chromium.org, kinuko+watch, darin-cc_chromium.org, calvinlo
Visibility:
Public.

Description

SyncFS: Introduce RemoteSyncOperationResolver for directory operation support To support directory operations in SyncFileSystem, this patch introduces RemoteSyncOperationResolver to determine remote operation type. BUG=231852 TEST=unit_tests, manual tests Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197245

Patch Set 1 : #

Total comments: 6

Patch Set 2 : add RemoteSyncOperationResolver and unit tests #

Total comments: 12

Patch Set 3 : review fix #

Total comments: 6

Patch Set 4 : rebase #

Patch Set 5 : review fix #

Total comments: 12

Patch Set 6 : review fix #

Unified diffs Side-by-side diffs Delta from patch set Stats (+698 lines, -62 lines) Patch
M chrome/browser/sync_file_system/drive_file_sync_service.h View 1 2 3 2 chunks +0 lines, -17 lines 0 comments Download
M chrome/browser/sync_file_system/drive_file_sync_service.cc View 1 2 3 4 4 chunks +16 lines, -45 lines 0 comments Download
A chrome/browser/sync_file_system/remote_sync_operation_resolver.h View 1 2 1 chunk +82 lines, -0 lines 0 comments Download
A chrome/browser/sync_file_system/remote_sync_operation_resolver.cc View 1 2 3 4 5 1 chunk +237 lines, -0 lines 0 comments Download
A chrome/browser/sync_file_system/remote_sync_operation_resolver_unittest.cc View 1 2 3 4 5 1 chunk +359 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M webkit/fileapi/syncable/file_change.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 22 (0 generated)
nhiroki
Hi super-syncers! Can you review this? Thanks!
7 years, 8 months ago (2013-04-23 11:57:53 UTC) #1
kinuko
some high-level comments https://codereview.chromium.org/13986011/diff/5001/chrome/browser/sync_file_system/drive_file_sync_service.cc File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/13986011/diff/5001/chrome/browser/sync_file_system/drive_file_sync_service.cc#newcode1569 chrome/browser/sync_file_system/drive_file_sync_service.cc:1569: bool missing_local_file) { It looks this ...
7 years, 8 months ago (2013-04-24 02:32:32 UTC) #2
kinuko
https://codereview.chromium.org/13986011/diff/5001/chrome/browser/sync_file_system/drive_file_sync_service.cc File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/13986011/diff/5001/chrome/browser/sync_file_system/drive_file_sync_service.cc#newcode1569 chrome/browser/sync_file_system/drive_file_sync_service.cc:1569: bool missing_local_file) { On 2013/04/24 02:32:32, kinuko wrote: > ...
7 years, 8 months ago (2013-04-24 02:42:29 UTC) #3
tzik
lgtm
7 years, 8 months ago (2013-04-24 03:39:07 UTC) #4
nhiroki
Thanks for your review. This patchset#2 factors out ResolveSyncOperationType function to RemoteSyncOperationResolver class, and adds ...
7 years, 8 months ago (2013-04-24 13:29:05 UTC) #5
kinuko
Phew... I seem to have made you to produce a lot more code. Replying to ...
7 years, 8 months ago (2013-04-24 14:07:59 UTC) #6
kinuko
Looking good, thanks for adding all these tests. I think we can probably reduce the ...
7 years, 8 months ago (2013-04-24 14:54:50 UTC) #7
nhiroki
Thanks! Updated. https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_system/remote_sync_operation_resolver.cc File chrome/browser/sync_file_system/remote_sync_operation_resolver.cc (right): https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_system/remote_sync_operation_resolver.cc#newcode16 chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:16: if (remote_file_change.IsAddOrUpdate()) { On 2013/04/24 14:54:50, kinuko ...
7 years, 8 months ago (2013-04-25 06:54:43 UTC) #8
kinuko
https://codereview.chromium.org/13986011/diff/24001/chrome/browser/sync_file_system/remote_sync_operation_resolver.cc File chrome/browser/sync_file_system/remote_sync_operation_resolver.cc (right): https://codereview.chromium.org/13986011/diff/24001/chrome/browser/sync_file_system/remote_sync_operation_resolver.cc#newcode15 chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:15: if (local_file_type == SYNC_FILE_TYPE_UNKNOWN || I think this one ...
7 years, 8 months ago (2013-04-25 08:34:10 UTC) #9
nhiroki
Updated, thanks! https://codereview.chromium.org/13986011/diff/24001/chrome/browser/sync_file_system/remote_sync_operation_resolver.cc File chrome/browser/sync_file_system/remote_sync_operation_resolver.cc (right): https://codereview.chromium.org/13986011/diff/24001/chrome/browser/sync_file_system/remote_sync_operation_resolver.cc#newcode15 chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:15: if (local_file_type == SYNC_FILE_TYPE_UNKNOWN || On 2013/04/25 ...
7 years, 8 months ago (2013-04-25 10:29:27 UTC) #10
kinuko
Thanks, a few more comments. https://codereview.chromium.org/13986011/diff/4004/chrome/browser/sync_file_system/remote_sync_operation_resolver.cc File chrome/browser/sync_file_system/remote_sync_operation_resolver.cc (right): https://codereview.chromium.org/13986011/diff/4004/chrome/browser/sync_file_system/remote_sync_operation_resolver.cc#newcode17 chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:17: return !(is_conflicting && local_file_type ...
7 years, 8 months ago (2013-04-25 14:07:54 UTC) #11
nhiroki
PTAL https://codereview.chromium.org/13986011/diff/4004/chrome/browser/sync_file_system/remote_sync_operation_resolver.cc File chrome/browser/sync_file_system/remote_sync_operation_resolver.cc (right): https://codereview.chromium.org/13986011/diff/4004/chrome/browser/sync_file_system/remote_sync_operation_resolver.cc#newcode17 chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:17: return !(is_conflicting && local_file_type != SYNC_FILE_TYPE_FILE); On 2013/04/25 ...
7 years, 8 months ago (2013-04-26 06:04:00 UTC) #12
kinuko
lgtm Fix later if we find new problems :)
7 years, 8 months ago (2013-04-26 06:12:07 UTC) #13
nhiroki
+jam@ Hi jam, could you review for chrome/chrome_tests_unit.gypi? Thanks.
7 years, 8 months ago (2013-04-26 06:16:52 UTC) #14
jam
On 2013/04/26 06:16:52, nhiroki wrote: > +jam@ > > Hi jam, could you review for ...
7 years, 8 months ago (2013-04-26 15:43:27 UTC) #15
nhiroki
On 2013/04/26 15:43:27, jam wrote: > On 2013/04/26 06:16:52, nhiroki wrote: > > +jam@ > ...
7 years, 7 months ago (2013-04-29 15:51:22 UTC) #16
nhiroki
+thestig@ Hi Lei, could you review chrome/chrome_tests_gypi? Thanks!
7 years, 7 months ago (2013-04-29 15:58:32 UTC) #17
Lei Zhang
lgtm
7 years, 7 months ago (2013-04-29 19:44:07 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nhiroki@chromium.org/13986011/36001
7 years, 7 months ago (2013-04-29 19:44:19 UTC) #19
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=140955
7 years, 7 months ago (2013-04-29 22:46:42 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nhiroki@chromium.org/13986011/36001
7 years, 7 months ago (2013-04-30 01:49:20 UTC) #21
commit-bot: I haz the power
7 years, 7 months ago (2013-04-30 03:17:28 UTC) #22
Message was sent while issue was closed.
Change committed as 197245

Powered by Google App Engine
This is Rietveld 408576698