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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 7926001: [Sync] Move change-related methods out of SyncManager::Observer (Closed)

Created:
9 years, 3 months ago by akalin
Modified:
9 years, 3 months ago
CC:
chromium-reviews, Raghu Simha, ncarter (slow), Paweł Hajdan Jr., tim (not reviewing), idana
Visibility:
Public.

Description

[Sync] Move change-related methods out of SyncManager::Observer Create new classes SyncManager::Change{Delegate,Observer} to handle and observe changes. Make SyncBackendRegistrar the SyncManager::ChangeDelegate. Make SyncBackendHost call RefreshEncryption asynchronously and wait for it. Also make the initialization state machine more explicit. Remove the hack done to make use of observers_ in SyncManager::SyncInternal almost thread-safe. Comment which SyncManager member functions can be called from any thread. Introduce helper struct WriteTransactionInfo and have DirectoryChangeDelegate take that instead of just an EntryKernelMutationMap. Beef up thread safety checks in SyncBackendHost. Limit number of ChangeRecord -> value conversions done. Move ChangeRecord unit tests into their own file. BUG=89658, 85481 TEST= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102169

Patch Set 1 #

Total comments: 1

Patch Set 2 : sync to head #

Patch Set 3 : Simplify test, fix checkdeps #

Patch Set 4 : Do RefreshEncryption asynchronously #

Patch Set 5 : Fix bug in async encryption, sync to head #

Total comments: 6

Patch Set 6 : Address comments #

Patch Set 7 : Fix unit tests #

Total comments: 4

Patch Set 8 : Address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+794 lines, -607 lines) Patch
M chrome/browser/sync/glue/sync_backend_host.h View 1 2 3 10 chunks +40 lines, -35 lines 0 comments Download
M chrome/browser/sync/glue/sync_backend_host.cc View 1 2 3 4 5 6 7 16 chunks +130 lines, -70 lines 0 comments Download
M chrome/browser/sync/glue/sync_backend_registrar.h View 4 chunks +22 lines, -7 lines 0 comments Download
M chrome/browser/sync/glue/sync_backend_registrar.cc View 1 2 3 4 5 3 chunks +44 lines, -15 lines 0 comments Download
M chrome/browser/sync/glue/sync_backend_registrar_unittest.cc View 1 2 3 4 5 6 7 9 chunks +45 lines, -18 lines 0 comments Download
M chrome/browser/sync/internal_api/change_record.h View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/sync/internal_api/change_record.cc View 1 2 3 4 5 2 chunks +5 lines, -18 lines 0 comments Download
A chrome/browser/sync/internal_api/change_record_unittest.cc View 1 2 3 4 5 1 chunk +137 lines, -0 lines 0 comments Download
M chrome/browser/sync/internal_api/sync_manager.h View 1 2 3 4 5 11 chunks +78 lines, -32 lines 0 comments Download
M chrome/browser/sync/internal_api/sync_manager.cc View 1 2 3 4 5 45 chunks +125 lines, -141 lines 0 comments Download
M chrome/browser/sync/internal_api/syncapi_unittest.cc View 1 2 3 4 5 6 7 9 chunks +13 lines, -148 lines 0 comments Download
M chrome/browser/sync/js/js_sync_manager_observer.h View 1 2 3 4 5 1 chunk +7 lines, -3 lines 0 comments Download
M chrome/browser/sync/js/js_sync_manager_observer.cc View 1 2 3 4 5 6 7 2 chunks +27 lines, -6 lines 0 comments Download
M chrome/browser/sync/js/js_sync_manager_observer_unittest.cc View 1 2 3 4 5 6 7 5 chunks +4 lines, -42 lines 0 comments Download
M chrome/browser/sync/js/js_transaction_observer.h View 1 2 3 4 5 1 chunk +2 lines, -4 lines 0 comments Download
M chrome/browser/sync/js/js_transaction_observer.cc View 1 2 3 4 5 6 7 1 chunk +4 lines, -17 lines 0 comments Download
M chrome/browser/sync/profile_sync_service.h View 1 2 3 4 5 6 7 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/browser/sync/profile_sync_service.cc View 1 2 3 4 5 6 7 1 chunk +0 lines, -8 lines 0 comments Download
M chrome/browser/sync/syncable/directory_change_delegate.h View 1 2 3 4 5 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/sync/syncable/syncable.h View 1 2 3 4 5 6 7 5 chunks +47 lines, -17 lines 0 comments Download
M chrome/browser/sync/syncable/syncable.cc View 1 2 3 4 5 6 7 5 chunks +47 lines, -9 lines 0 comments Download
M chrome/browser/sync/syncable/transaction_observer.h View 1 2 3 4 5 1 chunk +2 lines, -4 lines 0 comments Download
M chrome/browser/sync/test/null_directory_change_delegate.h View 1 2 3 4 5 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/sync/test/null_directory_change_delegate.cc View 1 2 3 4 5 1 chunk +6 lines, -4 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
akalin
+tim for review
9 years, 3 months ago (2011-09-16 02:14:38 UTC) #1
akalin
http://codereview.chromium.org/7926001/diff/1/chrome/browser/sync/glue/sync_backend_registrar.h File chrome/browser/sync/glue/sync_backend_registrar.h (right): http://codereview.chromium.org/7926001/diff/1/chrome/browser/sync/glue/sync_backend_registrar.h#newcode35 chrome/browser/sync/glue/sync_backend_registrar.h:35: class SyncBackendRegistrar : public ModelSafeWorkerRegistrar, not sure if SyncBackendRegistrar ...
9 years, 3 months ago (2011-09-16 02:20:20 UTC) #2
akalin
Synced to head, added async call of RefreshEncryption after consulting with zea.
9 years, 3 months ago (2011-09-16 23:01:48 UTC) #3
akalin
Ping!
9 years, 3 months ago (2011-09-19 17:38:44 UTC) #4
tim (not reviewing)
http://codereview.chromium.org/7926001/diff/7001/chrome/browser/sync/glue/sync_backend_registrar.cc File chrome/browser/sync/glue/sync_backend_registrar.cc (right): http://codereview.chromium.org/7926001/diff/7001/chrome/browser/sync/glue/sync_backend_registrar.cc#newcode260 chrome/browser/sync/glue/sync_backend_registrar.cc:260: return NULL; nit / consistency - I'm seeing braceless ...
9 years, 3 months ago (2011-09-20 16:04:59 UTC) #5
akalin
All comments address, PTAL. http://codereview.chromium.org/7926001/diff/7001/chrome/browser/sync/glue/sync_backend_registrar.cc File chrome/browser/sync/glue/sync_backend_registrar.cc (right): http://codereview.chromium.org/7926001/diff/7001/chrome/browser/sync/glue/sync_backend_registrar.cc#newcode260 chrome/browser/sync/glue/sync_backend_registrar.cc:260: return NULL; On 2011/09/20 16:04:59, ...
9 years, 3 months ago (2011-09-20 21:14:42 UTC) #6
tim (not reviewing)
lgtm http://codereview.chromium.org/7926001/diff/18002/chrome/browser/sync/js/js_transaction_observer.cc File chrome/browser/sync/js/js_transaction_observer.cc (right): http://codereview.chromium.org/7926001/diff/18002/chrome/browser/sync/js/js_transaction_observer.cc#newcode59 chrome/browser/sync/js/js_transaction_observer.cc:59: write_transaction_info.Get().ToValue(kMutationLimit)); nit - indent http://codereview.chromium.org/7926001/diff/18002/chrome/browser/sync/syncable/syncable.h File chrome/browser/sync/syncable/syncable.h (right): ...
9 years, 3 months ago (2011-09-21 16:12:03 UTC) #7
akalin
9 years, 3 months ago (2011-09-21 19:49:38 UTC) #8 
Addressed all comments, checking in after trybots.

http://codereview.chromium.org/7926001/diff/18002/chrome/browser/sync/js/js_t...
File chrome/browser/sync/js/js_transaction_observer.cc (right):

http://codereview.chromium.org/7926001/diff/18002/chrome/browser/sync/js/js_t...
chrome/browser/sync/js/js_transaction_observer.cc:59:
write_transaction_info.Get().ToValue(kMutationLimit));
On 2011/09/21 16:12:03, timsteele wrote:
> nit - indent

Done.

http://codereview.chromium.org/7926001/diff/18002/chrome/browser/sync/syncabl...
File chrome/browser/sync/syncable/syncable.h (right):

http://codereview.chromium.org/7926001/diff/18002/chrome/browser/sync/syncabl...
chrome/browser/sync/syncable/syncable.h:570: // a WriteTransaction has a writer
tag describing which body of code is doing
On 2011/09/21 16:12:03, timsteele wrote:
> nit 'A WriteTransaction'

Done.

Powered by Google App Engine
This is Rietveld 408576698