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

Issue 2078893002: Add callback list to PrefModelAssociator after sync data is loaded (Closed)

Created:
4 years, 6 months ago by lshang
Modified:
4 years, 5 months ago
CC:
chromium-reviews, msramek+watch_chromium.org, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, raymes+watch_chromium.org, markusheintz_, chrome-apps-syd-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@migrate_domain_scoped_settings
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add callback list to PrefModelAssociator after sync data is loaded This is a follow up CL of https://codereview.chromium.org/1895993003. The migrated settings will be synced back because sync process happens after the construction of HostContentSettingsMap. In this CL, a callback list is added to PrefModelAssociator, which will gets registered with the migration code after construction of HostContentSettingsMap, and gets executed at the end of PrefModelAssociator::MergeDataAndStartSyncing. The follow up CL of this is https://codereview.chromium.org/2075103002. BUG=604612 Committed: https://crrev.com/2cf5c57f54ec0aa57d422f6f4f89dddd9c641051 Cr-Commit-Position: refs/heads/master@{#405669}

Patch Set 1 #

Patch Set 2 : #

Total comments: 18

Patch Set 3 : register callback outside of HostContentSettingsMap #

Patch Set 4 : remove unused things #

Total comments: 6

Patch Set 5 : address review comments #

Total comments: 9

Patch Set 6 : add a test for RegisterMergeDataFinishedCallback #

Total comments: 10

Patch Set 7 : revise test #

Total comments: 2

Patch Set 8 : use local variable #

Total comments: 2

Patch Set 9 : minor change #

Unified diffs Side-by-side diffs Delta from patch set Stats (+42 lines, -1 line) Patch
M components/syncable_prefs/pref_model_associator.h View 1 2 3 chunks +8 lines, -0 lines 0 comments Download
M components/syncable_prefs/pref_model_associator.cc View 1 2 3 4 2 chunks +12 lines, -0 lines 0 comments Download
M components/syncable_prefs/pref_service_syncable.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M components/syncable_prefs/pref_service_syncable.cc View 1 1 chunk +5 lines, -0 lines 0 comments Download
M components/syncable_prefs/pref_service_syncable_unittest.cc View 1 2 3 4 5 6 7 8 3 chunks +15 lines, -1 line 0 comments Download

Messages

Total messages: 42 (18 generated)
lshang
Hi Raymes, this is a follow up CL which adds callbacks in sync layer. PTAL ...
4 years, 5 months ago (2016-06-28 08:04:50 UTC) #10
raymes
We should add some tests for this - probably a test somewhere in the sync ...
4 years, 5 months ago (2016-06-29 03:50:01 UTC) #11
lshang
> We should add some tests for this - probably a test somewhere in the ...
4 years, 5 months ago (2016-06-30 05:03:36 UTC) #12
lshang
Hi Pavel, Raymes is OOO this week, could you take a look at this CL ...
4 years, 5 months ago (2016-07-04 01:26:44 UTC) #15
pavely
On 2016/07/04 01:26:44, Liu Shang wrote: > Hi Pavel, Raymes is OOO this week, could ...
4 years, 5 months ago (2016-07-06 22:11:31 UTC) #17
pavely
https://codereview.chromium.org/2078893002/diff/180001/components/syncable_prefs/pref_model_associator.cc File components/syncable_prefs/pref_model_associator.cc (right): https://codereview.chromium.org/2078893002/diff/180001/components/syncable_prefs/pref_model_associator.cc#newcode220 components/syncable_prefs/pref_model_associator.cc:220: callback_list_.clear(); I think it is possible that MergeDataAndStartSyncing is ...
4 years, 5 months ago (2016-07-06 22:11:56 UTC) #18
lshang
Thanks Pavel, please tell me if you have any further comments:-) https://codereview.chromium.org/2078893002/diff/180001/components/syncable_prefs/pref_model_associator.cc File components/syncable_prefs/pref_model_associator.cc (right): ...
4 years, 5 months ago (2016-07-07 08:26:05 UTC) #19
lshang
Also +bauerb@ for review, PTAL thanks!
4 years, 5 months ago (2016-07-07 08:26:49 UTC) #21
Bernhard Bauer
https://codereview.chromium.org/2078893002/diff/200001/chrome/browser/content_settings/host_content_settings_map_factory.cc File chrome/browser/content_settings/host_content_settings_map_factory.cc (right): https://codereview.chromium.org/2078893002/diff/200001/chrome/browser/content_settings/host_content_settings_map_factory.cc#newcode76 chrome/browser/content_settings/host_content_settings_map_factory.cc:76: // TODO(lshang): Make this not a comment when we ...
4 years, 5 months ago (2016-07-07 08:50:08 UTC) #22
raymes
> I currently commented out the registration code block in > HostContentSettingsMapFactory, I think we ...
4 years, 5 months ago (2016-07-13 03:20:20 UTC) #23
lshang
https://codereview.chromium.org/2078893002/diff/200001/chrome/browser/content_settings/host_content_settings_map_factory.cc File chrome/browser/content_settings/host_content_settings_map_factory.cc (right): https://codereview.chromium.org/2078893002/diff/200001/chrome/browser/content_settings/host_content_settings_map_factory.cc#newcode76 chrome/browser/content_settings/host_content_settings_map_factory.cc:76: // TODO(lshang): Make this not a comment when we ...
4 years, 5 months ago (2016-07-13 06:27:29 UTC) #25
lshang
On 2016/07/13 03:20:20, raymes wrote: > > I currently commented out the registration code block ...
4 years, 5 months ago (2016-07-13 06:28:40 UTC) #26
lshang
On 2016/07/13 03:20:20, raymes wrote: > > I currently commented out the registration code block ...
4 years, 5 months ago (2016-07-13 06:31:17 UTC) #27
Bernhard Bauer
https://codereview.chromium.org/2078893002/diff/220001/components/syncable_prefs/pref_service_syncable_unittest.cc File components/syncable_prefs/pref_service_syncable_unittest.cc (right): https://codereview.chromium.org/2078893002/diff/220001/components/syncable_prefs/pref_service_syncable_unittest.cc#newcode124 components/syncable_prefs/pref_service_syncable_unittest.cc:124: next_pref_remote_sync_node_id_ = 0; This is not necessary, BTW (each ...
4 years, 5 months ago (2016-07-13 08:31:53 UTC) #28
lshang
https://codereview.chromium.org/2078893002/diff/220001/components/syncable_prefs/pref_service_syncable_unittest.cc File components/syncable_prefs/pref_service_syncable_unittest.cc (right): https://codereview.chromium.org/2078893002/diff/220001/components/syncable_prefs/pref_service_syncable_unittest.cc#newcode124 components/syncable_prefs/pref_service_syncable_unittest.cc:124: next_pref_remote_sync_node_id_ = 0; On 2016/07/13 08:31:52, Bernhard Bauer wrote: ...
4 years, 5 months ago (2016-07-14 01:29:04 UTC) #29
raymes
https://codereview.chromium.org/2078893002/diff/240001/components/syncable_prefs/pref_service_syncable_unittest.cc File components/syncable_prefs/pref_service_syncable_unittest.cc (right): https://codereview.chromium.org/2078893002/diff/240001/components/syncable_prefs/pref_service_syncable_unittest.cc#newcode565 components/syncable_prefs/pref_service_syncable_unittest.cc:565: base::Bind(&CallbackFunc, &kNumCallbacks)); can kNumCallbacks be a local variable?
4 years, 5 months ago (2016-07-14 02:58:51 UTC) #30
lshang
https://codereview.chromium.org/2078893002/diff/240001/components/syncable_prefs/pref_service_syncable_unittest.cc File components/syncable_prefs/pref_service_syncable_unittest.cc (right): https://codereview.chromium.org/2078893002/diff/240001/components/syncable_prefs/pref_service_syncable_unittest.cc#newcode565 components/syncable_prefs/pref_service_syncable_unittest.cc:565: base::Bind(&CallbackFunc, &kNumCallbacks)); On 2016/07/14 02:58:51, raymes wrote: > can ...
4 years, 5 months ago (2016-07-14 04:10:24 UTC) #31
raymes
lgtm
4 years, 5 months ago (2016-07-14 04:34:09 UTC) #32
Bernhard Bauer
lgtm https://codereview.chromium.org/2078893002/diff/260001/components/syncable_prefs/pref_service_syncable_unittest.cc File components/syncable_prefs/pref_service_syncable_unittest.cc (right): https://codereview.chromium.org/2078893002/diff/260001/components/syncable_prefs/pref_service_syncable_unittest.cc#newcode49 components/syncable_prefs/pref_service_syncable_unittest.cc:49: void CallbackFunc(int* num) { Maybe call this method ...
4 years, 5 months ago (2016-07-14 10:29:18 UTC) #33
lshang
Thanks all! https://codereview.chromium.org/2078893002/diff/260001/components/syncable_prefs/pref_service_syncable_unittest.cc File components/syncable_prefs/pref_service_syncable_unittest.cc (right): https://codereview.chromium.org/2078893002/diff/260001/components/syncable_prefs/pref_service_syncable_unittest.cc#newcode49 components/syncable_prefs/pref_service_syncable_unittest.cc:49: void CallbackFunc(int* num) { On 2016/07/14 10:29:17, ...
4 years, 5 months ago (2016-07-15 01:09:07 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2078893002/280001
4 years, 5 months ago (2016-07-15 01:10:03 UTC) #37
commit-bot: I haz the power
Committed patchset #9 (id:280001)
4 years, 5 months ago (2016-07-15 01:36:47 UTC) #39
commit-bot: I haz the power
CQ bit was unchecked.
4 years, 5 months ago (2016-07-15 01:37:10 UTC) #40
commit-bot: I haz the power
4 years, 5 months ago (2016-07-15 01:38:49 UTC) #42
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/2cf5c57f54ec0aa57d422f6f4f89dddd9c641051
Cr-Commit-Position: refs/heads/master@{#405669}

Powered by Google App Engine
This is Rietveld 408576698