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 8065016: [Sync] Refactor non-frontend DTC to handle new API properly. (Closed)

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

Description

[Sync] Refactor non-frontend DTC to handle new API properly. We now support disconnecting the syncableservice from the syncer via it's sync change processor. AutofillProfile has been modified to support this. As a result of the refactor and this disconnect functionality, we don't need to block sync shutdown on datatypes implemented the new API, even if they don't run on the UI thread. From here on, datatypes that are not on the UI thread should have their controller inherit from NewNonFrontendDataTypeController, and should have their syncable service take ownership of the sync change processor it receives. A remaining TODO is to apply these changes to UI thread based datatypes. This involves having them all take ownership of their sync change processors (this change has their datatype controllers take ownership instead) and creating a new parent datatype controller class that is API aware, allowing us to remove the SyncableServiceAdapater completely. See crbug.com/100114. BUG=96889 TEST=unit_tests, integration tests Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105404

Patch Set 1 #

Patch Set 2 : Proper diff #

Patch Set 3 : Working patch #

Patch Set 4 : Self review #

Total comments: 2

Patch Set 5 : Refactor SharedChangeProcessor out of GenericChangeProcessor #

Patch Set 6 : Rebase and self review #

Total comments: 16

Patch Set 7 : Kill stray character #

Patch Set 8 : Some comments addressed #

Total comments: 2

Patch Set 9 : Reffffactor #

Total comments: 54

Patch Set 10 : Address comments #

Patch Set 11 : Address comments + rebase #

Patch Set 12 : Self Review #

Patch Set 13 : Try harder trybot (rebase). #

Total comments: 30

Patch Set 14 : Rebase against Ilya's patch + address comments #

Patch Set 15 : Self review #

Total comments: 20

Patch Set 16 : Address final comments, rebase against actual patch #

Patch Set 17 : g++ is a hater #

Patch Set 18 : Fix race #

Patch Set 19 : Rebase++++ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1618 lines, -193 lines) Patch
M chrome/browser/sync/api/sync_change_processor.h View 1 2 3 10 3 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/sync/api/sync_change_processor.cc View 1 10 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/sync/api/syncable_service.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +6 lines, -1 line 0 comments Download
M chrome/browser/sync/glue/app_data_type_controller.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/sync/glue/app_data_type_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +7 lines, -1 line 0 comments Download
M chrome/browser/sync/glue/autofill_data_type_controller.cc View 1 2 3 4 5 10 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/sync/glue/autofill_profile_data_type_controller.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +45 lines, -8 lines 0 comments Download
M chrome/browser/sync/glue/autofill_profile_data_type_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +99 lines, -10 lines 0 comments Download
M chrome/browser/sync/glue/change_processor.h View 1 2 3 4 10 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/browser/sync/glue/change_processor.cc View 1 2 3 4 10 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/sync/glue/extension_data_type_controller.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/sync/glue/extension_data_type_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +6 lines, -1 line 0 comments Download
M chrome/browser/sync/glue/frontend_data_type_controller.cc View 1 2 3 4 5 10 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/sync/glue/generic_change_processor.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +24 lines, -11 lines 0 comments Download
M chrome/browser/sync/glue/generic_change_processor.cc View 1 2 3 4 5 6 7 8 9 10 7 chunks +26 lines, -10 lines 0 comments Download
A chrome/browser/sync/glue/new_non_frontend_data_type_controller.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +75 lines, -0 lines 0 comments Download
A chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +220 lines, -0 lines 0 comments Download
A chrome/browser/sync/glue/new_non_frontend_data_type_controller_mock.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +64 lines, -0 lines 0 comments Download
A chrome/browser/sync/glue/new_non_frontend_data_type_controller_mock.cc View 1 10 1 chunk +13 lines, -0 lines 0 comments Download
A chrome/browser/sync/glue/new_non_frontend_data_type_controller_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +404 lines, -0 lines 0 comments Download
M chrome/browser/sync/glue/non_frontend_data_type_controller.h View 1 10 1 chunk +6 lines, -2 lines 0 comments Download
M chrome/browser/sync/glue/non_frontend_data_type_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 12 8 chunks +33 lines, -18 lines 0 comments Download
M chrome/browser/sync/glue/preference_data_type_controller.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/sync/glue/preference_data_type_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +6 lines, -2 lines 0 comments Download
M chrome/browser/sync/glue/search_engine_data_type_controller.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/sync/glue/search_engine_data_type_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +7 lines, -2 lines 0 comments Download
A chrome/browser/sync/glue/shared_change_processor.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +113 lines, -0 lines 0 comments Download
A chrome/browser/sync/glue/shared_change_processor.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +129 lines, -0 lines 0 comments Download
A chrome/browser/sync/glue/shared_change_processor_mock.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +48 lines, -0 lines 0 comments Download
A chrome/browser/sync/glue/shared_change_processor_mock.cc View 1 2 3 4 5 6 7 8 10 1 chunk +15 lines, -0 lines 0 comments Download
A chrome/browser/sync/glue/shared_change_processor_ref.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +38 lines, -0 lines 0 comments Download
A chrome/browser/sync/glue/shared_change_processor_ref.cc View 1 2 3 4 5 6 7 10 1 chunk +23 lines, -0 lines 0 comments Download
M chrome/browser/sync/glue/syncable_service_adapter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +5 lines, -2 lines 0 comments Download
M chrome/browser/sync/profile_sync_factory.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 4 chunks +26 lines, -7 lines 0 comments Download
M chrome/browser/sync/profile_sync_factory_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +9 lines, -4 lines 0 comments Download
M chrome/browser/sync/profile_sync_factory_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 9 chunks +44 lines, -32 lines 0 comments Download
M chrome/browser/sync/profile_sync_factory_mock.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +10 lines, -5 lines 0 comments Download
M chrome/browser/sync/profile_sync_service.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/sync/profile_sync_service_autofill_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 7 chunks +24 lines, -19 lines 0 comments Download
M chrome/browser/sync/profile_sync_service_mock.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/sync/profile_sync_service_preference_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/browser/webdata/autofill_profile_syncable_service.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/webdata/autofill_profile_syncable_service.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 10 chunks +12 lines, -14 lines 0 comments Download
M chrome/browser/webdata/autofill_profile_syncable_service_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 6 chunks +24 lines, -27 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +6 lines, -0 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
Nicolas Zea
GeorgeY for autofill profile specific parts (in particular the profile_sync_factory, autofill profile syncable service + ...
9 years, 2 months ago (2011-10-04 00:00:02 UTC) #1
akalin
http://codereview.chromium.org/8065016/diff/4002/chrome/browser/sync/glue/generic_change_processor.h File chrome/browser/sync/glue/generic_change_processor.h (right): http://codereview.chromium.org/8065016/diff/4002/chrome/browser/sync/glue/generic_change_processor.h#newcode39 chrome/browser/sync/glue/generic_change_processor.h:39: public base::RefCountedThreadSafe<GenericChangeProcessor> { Okay, as you probably expected, I ...
9 years, 2 months ago (2011-10-06 05:33:29 UTC) #2
akalin
On 2011/10/06 05:33:29, akalin wrote: > http://codereview.chromium.org/8065016/diff/4002/chrome/browser/sync/glue/generic_change_processor.h > File chrome/browser/sync/glue/generic_change_processor.h (right): > > http://codereview.chromium.org/8065016/diff/4002/chrome/browser/sync/glue/generic_change_processor.h#newcode39 > ...
9 years, 2 months ago (2011-10-06 15:50:43 UTC) #3
Nicolas Zea
Good idea, PTAL. http://codereview.chromium.org/8065016/diff/4002/chrome/browser/sync/glue/generic_change_processor.h File chrome/browser/sync/glue/generic_change_processor.h (right): http://codereview.chromium.org/8065016/diff/4002/chrome/browser/sync/glue/generic_change_processor.h#newcode39 chrome/browser/sync/glue/generic_change_processor.h:39: public base::RefCountedThreadSafe<GenericChangeProcessor> { On 2011/10/06 05:33:29, ...
9 years, 2 months ago (2011-10-06 22:10:54 UTC) #4
akalin
Few more high-level comments http://codereview.chromium.org/8065016/diff/13001/chrome/browser/sync/glue/generic_change_processor.cc File chrome/browser/sync/glue/generic_change_processor.cc (right): http://codereview.chromium.org/8065016/diff/13001/chrome/browser/sync/glue/generic_change_processor.cc#newcode257 chrome/browser/sync/glue/generic_change_processor.cc:257: local_service_ = local_service; may as ...
9 years, 2 months ago (2011-10-07 20:51:16 UTC) #5
Nicolas Zea
Had some clarification questions on a couple comments. http://codereview.chromium.org/8065016/diff/13001/chrome/browser/sync/glue/generic_change_processor.cc File chrome/browser/sync/glue/generic_change_processor.cc (right): http://codereview.chromium.org/8065016/diff/13001/chrome/browser/sync/glue/generic_change_processor.cc#newcode257 chrome/browser/sync/glue/generic_change_processor.cc:257: local_service_ ...
9 years, 2 months ago (2011-10-07 22:05:51 UTC) #6
akalin
http://codereview.chromium.org/8065016/diff/13001/chrome/browser/sync/glue/generic_change_processor.cc File chrome/browser/sync/glue/generic_change_processor.cc (right): http://codereview.chromium.org/8065016/diff/13001/chrome/browser/sync/glue/generic_change_processor.cc#newcode257 chrome/browser/sync/glue/generic_change_processor.cc:257: local_service_ = local_service; On 2011/10/07 22:05:52, nzea wrote: > ...
9 years, 2 months ago (2011-10-07 22:18:28 UTC) #7
akalin
http://codereview.chromium.org/8065016/diff/21001/chrome/browser/sync/glue/shared_change_processor.cc File chrome/browser/sync/glue/shared_change_processor.cc (right): http://codereview.chromium.org/8065016/diff/21001/chrome/browser/sync/glue/shared_change_processor.cc#newcode79 chrome/browser/sync/glue/shared_change_processor.cc:79: return GenericChangeProcessor::ProcessSyncChanges(from_here, list_of_changes); it may suffice to store the ...
9 years, 2 months ago (2011-10-07 22:40:05 UTC) #8
GeorgeY
On 2011/10/07 22:40:05, akalin wrote: > http://codereview.chromium.org/8065016/diff/21001/chrome/browser/sync/glue/shared_change_processor.cc > File chrome/browser/sync/glue/shared_change_processor.cc (right): > > http://codereview.chromium.org/8065016/diff/21001/chrome/browser/sync/glue/shared_change_processor.cc#newcode79 > ...
9 years, 2 months ago (2011-10-10 23:45:25 UTC) #9
akalin
Looking better, but more comments http://codereview.chromium.org/8065016/diff/33002/chrome/browser/sync/api/syncable_service.h File chrome/browser/sync/api/syncable_service.h (right): http://codereview.chromium.org/8065016/diff/33002/chrome/browser/sync/api/syncable_service.h#newcode23 chrome/browser/sync/api/syncable_service.h:23: public base::SupportsWeakPtr<SyncableService> { I ...
9 years, 2 months ago (2011-10-11 22:41:07 UTC) #10
Nicolas Zea
All comments addressed. Additionally, I've removed the usage of WeakHandles (just sticking with WeakPtrs since ...
9 years, 2 months ago (2011-10-12 04:24:19 UTC) #11
akalin
Few more comments, but I think you need to wait for isherman's autofill patch to ...
9 years, 2 months ago (2011-10-12 19:54:20 UTC) #12
Ilya Sherman
On 2011/10/12 19:54:20, akalin wrote: > Few more comments, but I think you need to ...
9 years, 2 months ago (2011-10-12 20:25:57 UTC) #13
Nicolas Zea
All comments addressed, PTAL http://codereview.chromium.org/8065016/diff/41001/chrome/browser/extensions/extension_service.h File chrome/browser/extensions/extension_service.h (right): http://codereview.chromium.org/8065016/diff/41001/chrome/browser/extensions/extension_service.h#newcode80 chrome/browser/extensions/extension_service.h:80: public base::SupportsWeakPtr<ExtensionServiceInterface> { On 2011/10/12 ...
9 years, 2 months ago (2011-10-12 23:47:42 UTC) #14
akalin
LGTM after comments http://codereview.chromium.org/8065016/diff/41001/chrome/browser/sync/glue/app_data_type_controller.h File chrome/browser/sync/glue/app_data_type_controller.h (right): http://codereview.chromium.org/8065016/diff/41001/chrome/browser/sync/glue/app_data_type_controller.h#newcode41 chrome/browser/sync/glue/app_data_type_controller.h:41: scoped_ptr<GenericChangeProcessor> change_processor_; On 2011/10/12 23:47:43, nzea ...
9 years, 2 months ago (2011-10-13 00:08:15 UTC) #15
Nicolas Zea
comments addressed, committing once trybots go green. http://codereview.chromium.org/8065016/diff/45002/chrome/browser/sync/api/syncable_service_mock.h File chrome/browser/sync/api/syncable_service_mock.h (right): http://codereview.chromium.org/8065016/diff/45002/chrome/browser/sync/api/syncable_service_mock.h#newcode10 chrome/browser/sync/api/syncable_service_mock.h:10: #include "base/memory/weak_ptr.h" ...
9 years, 2 months ago (2011-10-13 01:16:14 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zea@chromium.org/8065016/55001
9 years, 2 months ago (2011-10-13 22:53:09 UTC) #17
commit-bot: I haz the power
9 years, 2 months ago (2011-10-13 22:53:36 UTC) #18
Can't apply patch for file chrome/browser/prefs/pref_model_associator.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/prefs/pref_model_associator.cc
Hunk #1 FAILED at 412.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/prefs/pref_model_associator.cc.rej

Powered by Google App Engine
This is Rietveld 408576698