Chromium Code Reviews
|
|
Chromium Code Reviews
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. |
DescriptionAdd 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 #
Messages
Total messages: 42 (18 generated)
Description was changed from ========== Add migration callback to syncable prefs after sync merge with migrate consist changes with migrate BUG= ========== to ========== Add migration callback to syncable prefs after sync We used to do migration code in the constructor of HostContentSettingsMap, and found that old settings will be synced back during sync process. In this CL a callback is added in PrefModelAssociator::MergeDataAndStartSyncing, which load sync data. BUG=604612 ==========
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add migration callback to syncable prefs after sync We used to do migration code in the constructor of HostContentSettingsMap, and found that old settings will be synced back during sync process. In this CL a callback is added in PrefModelAssociator::MergeDataAndStartSyncing, which load sync data. BUG=604612 ========== to ========== Add migration callback 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 gets registered in constructor of HostContentSettingsMap and gets executed at the end of PrefModelAssociator::MergeDataAndStartSyncing. BUG=604612 ==========
lshang@chromium.org changed reviewers: + raymes@chromium.org
Hi Raymes, this is a follow up CL which adds callbacks in sync layer. PTAL thanks!
We should add some tests for this - probably a test somewhere in the sync code (pref_model_associator_unittest maybe?). Also we can add a test in host_content_settings_map_unittest if we pass the registration function as suggested below. https://codereview.chromium.org/2078893002/diff/140001/chrome/browser/downloa... File chrome/browser/download/download_request_limiter_unittest.cc (right): https://codereview.chromium.org/2078893002/diff/140001/chrome/browser/downloa... chrome/browser/download/download_request_limiter_unittest.cc:154: false /* guest_profile */, nullptr /* PrefServiceSyncable* */); nit: usually just put the name of the variable in the comment, rather than the type https://codereview.chromium.org/2078893002/diff/140001/components/content_set... File components/content_settings/core/browser/cookie_settings_unittest.cc (right): https://codereview.chromium.org/2078893002/diff/140001/components/content_set... components/content_settings/core/browser/cookie_settings_unittest.cc:33: nullptr /* PrefServiceSyncable* */); nit: same here https://codereview.chromium.org/2078893002/diff/140001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2078893002/diff/140001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:185: base::Unretained(this))); We should probably use a WeakPtr here instead of base::Unretained(this). That will ensure that the callback doesn't get called if this object is destroyed. I think we could also just pass "this", which would keep this object alive until after the callback has run, but in this case we don't need to keep it alive and it's probably better not to. https://codereview.chromium.org/2078893002/diff/140001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/2078893002/diff/140001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:75: syncable_prefs::PrefServiceSyncable* pref_service); I'm wondering if we can just pass in a base::Callback here register_pref_sync_finished_callback register_pref_sync_finished_callback.Run(base::Bind(...)); That would eliminate the dependency on syncable_prefs and make testing a lot easier. We could even register the callback outside this class (without passing anything in), but I think this is slightly better because it means that the client code is forced to think about registering such a callback. What do you think? https://codereview.chromium.org/2078893002/diff/140001/components/signin/ios/... File components/signin/ios/browser/account_consistency_service_unittest.mm (right): https://codereview.chromium.org/2078893002/diff/140001/components/signin/ios/... components/signin/ios/browser/account_consistency_service_unittest.mm:137: nullptr /* PrefServiceSyncable* */); nit: same here (should just be pref_service) https://codereview.chromium.org/2078893002/diff/140001/components/syncable_pr... File components/syncable_prefs/pref_model_associator.cc (right): https://codereview.chromium.org/2078893002/diff/140001/components/syncable_pr... components/syncable_prefs/pref_model_associator.cc:160: void PrefModelAssociator::RegisterMergeDataFinishedCallback( Did you look into whether we should use PrefServiceSyncableObserver instead (you had mentioned this)? What were your thoughts? https://codereview.chromium.org/2078893002/diff/140001/components/syncable_pr... components/syncable_prefs/pref_model_associator.cc:162: callback_list_.push_back(callback); We should make sure that we only add this if we expect MergeDataAndStartSyncing to be called in the future. I think we can use models_associated_ here? if (!models_associated_) callback_list_.push_back(callback); else callback.Run(); https://codereview.chromium.org/2078893002/diff/140001/components/syncable_pr... File components/syncable_prefs/pref_model_associator.h (right): https://codereview.chromium.org/2078893002/diff/140001/components/syncable_pr... components/syncable_prefs/pref_model_associator.h:202: std::list<base::Closure> callback_list_; std::vector is usually preferable https://codereview.chromium.org/2078893002/diff/140001/ios/chrome/browser/con... File ios/chrome/browser/content_settings/host_content_settings_map_factory.cc (right): https://codereview.chromium.org/2078893002/diff/140001/ios/chrome/browser/con... ios/chrome/browser/content_settings/host_content_settings_map_factory.cc:47: false /* guest_profile */, nullptr /* PrefServiceSyncable* */)); nit: same here
> We should add some tests for this - probably a test somewhere in the sync code (pref_model_associator_unittest maybe?). > Also we can add a test in host_content_settings_map_unittest if we pass the registration function as suggested below. I currently commented out the registration code block in HostContentSettingsMapFactory, I think we won't want it to start migrating settings from this CL. Maybe we can add tests when we hook up all the migration code and sync callbacks in the next CL? Are you OK with this? https://codereview.chromium.org/2078893002/diff/140001/chrome/browser/downloa... File chrome/browser/download/download_request_limiter_unittest.cc (right): https://codereview.chromium.org/2078893002/diff/140001/chrome/browser/downloa... chrome/browser/download/download_request_limiter_unittest.cc:154: false /* guest_profile */, nullptr /* PrefServiceSyncable* */); On 2016/06/29 03:50:01, raymes wrote: > nit: usually just put the name of the variable in the comment, rather than the > type We've changed to not pass in the pointer, so no need to address this. https://codereview.chromium.org/2078893002/diff/140001/components/content_set... File components/content_settings/core/browser/cookie_settings_unittest.cc (right): https://codereview.chromium.org/2078893002/diff/140001/components/content_set... components/content_settings/core/browser/cookie_settings_unittest.cc:33: nullptr /* PrefServiceSyncable* */); On 2016/06/29 03:50:01, raymes wrote: > nit: same here We've changed to not pass in the pointer, so no need to address this. https://codereview.chromium.org/2078893002/diff/140001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2078893002/diff/140001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:185: base::Unretained(this))); On 2016/06/29 03:50:01, raymes wrote: > We should probably use a WeakPtr here instead of base::Unretained(this). That > will ensure that the callback doesn't get called if this object is destroyed. > > I think we could also just pass "this", which would keep this object alive until > after the callback has run, but in this case we don't need to keep it alive and > it's probably better not to. Done. https://codereview.chromium.org/2078893002/diff/140001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/2078893002/diff/140001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:75: syncable_prefs::PrefServiceSyncable* pref_service); On 2016/06/29 03:50:01, raymes wrote: > I'm wondering if we can just pass in a base::Callback here > register_pref_sync_finished_callback > > register_pref_sync_finished_callback.Run(base::Bind(...)); > > That would eliminate the dependency on syncable_prefs and make testing a lot > easier. > > We could even register the callback outside this class (without passing anything > in), but I think this is slightly better because it means that the client code > is forced to think about registering such a callback. What do you think? Done. Yeah as we discussed, I think register the callback outside this class sounds better because that will make the constructor of HostContentSettingsMap cleaner. I've moved it to HostContentSettingsMapFactory. And I also commented out the registration code block for now since we don't really want it to be called in this CL. we should hook all things up in the next CL when we change the scoping type. https://codereview.chromium.org/2078893002/diff/140001/components/signin/ios/... File components/signin/ios/browser/account_consistency_service_unittest.mm (right): https://codereview.chromium.org/2078893002/diff/140001/components/signin/ios/... components/signin/ios/browser/account_consistency_service_unittest.mm:137: nullptr /* PrefServiceSyncable* */); On 2016/06/29 03:50:01, raymes wrote: > nit: same here (should just be pref_service) We've changed to not pass in the pointer, so no need to address this. https://codereview.chromium.org/2078893002/diff/140001/components/syncable_pr... File components/syncable_prefs/pref_model_associator.cc (right): https://codereview.chromium.org/2078893002/diff/140001/components/syncable_pr... components/syncable_prefs/pref_model_associator.cc:160: void PrefModelAssociator::RegisterMergeDataFinishedCallback( On 2016/06/29 03:50:01, raymes wrote: > Did you look into whether we should use PrefServiceSyncableObserver instead (you > had mentioned this)? What were your thoughts? PrefServiceSyncableObserver will get called in MergeDataAndStartSyncing() and StopSyncing(), and StopSyncing() will be called in a list of cases: - User signed out - Data type disabled by user through settings - User enabled passphrase encryption on remote device, but hasn't entered it on local device yet - Datatype encountered error - Browser process shutdown I also added some log to test this, and it didn't output as I expect (maybe for some reason, I didn't dig too much). I think it doesn't deserve so much extra concern or handling of cases to reuse PrefServiceSyncableObserver, compared to just add a new list of callbacks to pref_model_associator and these callbacks only get called in MergeDataAndStartSyncing(). https://codereview.chromium.org/2078893002/diff/140001/components/syncable_pr... components/syncable_prefs/pref_model_associator.cc:162: callback_list_.push_back(callback); On 2016/06/29 03:50:01, raymes wrote: > We should make sure that we only add this if we expect MergeDataAndStartSyncing > to be called in the future. I think we can use models_associated_ here? > > if (!models_associated_) > callback_list_.push_back(callback); > else > callback.Run(); Done. https://codereview.chromium.org/2078893002/diff/140001/components/syncable_pr... File components/syncable_prefs/pref_model_associator.h (right): https://codereview.chromium.org/2078893002/diff/140001/components/syncable_pr... components/syncable_prefs/pref_model_associator.h:202: std::list<base::Closure> callback_list_; On 2016/06/29 03:50:01, raymes wrote: > std::vector is usually preferable Done. https://codereview.chromium.org/2078893002/diff/140001/ios/chrome/browser/con... File ios/chrome/browser/content_settings/host_content_settings_map_factory.cc (right): https://codereview.chromium.org/2078893002/diff/140001/ios/chrome/browser/con... ios/chrome/browser/content_settings/host_content_settings_map_factory.cc:47: false /* guest_profile */, nullptr /* PrefServiceSyncable* */)); On 2016/06/29 03:50:01, raymes wrote: > nit: same here Same as above, don't need to address this after we change to not pass in it.
Description was changed from ========== Add migration callback 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 gets registered in constructor of HostContentSettingsMap and gets executed at the end of PrefModelAssociator::MergeDataAndStartSyncing. BUG=604612 ========== to ========== Add migration callback 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 gets registered after constructor of HostContentSettingsMap and gets executed at the end of PrefModelAssociator::MergeDataAndStartSyncing. BUG=604612 ==========
lshang@chromium.org changed reviewers: + pavely@chromium.org
Hi Pavel, Raymes is OOO this week, could you take a look at this CL first from sync perspective? Thanks!
Description was changed from ========== Add migration callback 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 gets registered after constructor of HostContentSettingsMap and gets executed at the end of PrefModelAssociator::MergeDataAndStartSyncing. BUG=604612 ========== to ========== Add migration callback 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 gets registered after construction of HostContentSettingsMap and gets executed at the end of PrefModelAssociator::MergeDataAndStartSyncing. BUG=604612 ==========
On 2016/07/04 01:26:44, Liu Shang wrote: > Hi Pavel, Raymes is OOO this week, could you take a look at this CL first from > sync perspective? Thanks! I left couple comments. Keep in mind, I'm not an owner of these files, you should add owner to review.
https://codereview.chromium.org/2078893002/diff/180001/components/syncable_pr... File components/syncable_prefs/pref_model_associator.cc (right): https://codereview.chromium.org/2078893002/diff/180001/components/syncable_pr... components/syncable_prefs/pref_model_associator.cc:220: callback_list_.clear(); I think it is possible that MergeDataAndStartSyncing is called multiple times for a given object. Scenario would be: - Sign-in, enable syncing everything (First time MDaSS is called) - Disable syncing settings (StopSyncing is called) - Enable syncing settings (Second time MDaSS is called) https://codereview.chromium.org/2078893002/diff/180001/components/syncable_pr... components/syncable_prefs/pref_model_associator.cc:227: If you want to call a callback only for successful merge you need to move code here. https://codereview.chromium.org/2078893002/diff/180001/components/syncable_pr... File components/syncable_prefs/pref_model_associator.h (right): https://codereview.chromium.org/2078893002/diff/180001/components/syncable_pr... components/syncable_prefs/pref_model_associator.h:202: std::vector<base::Closure> callback_list_; Check out CallbackList class. Maybe that's what you need here.
Thanks Pavel, please tell me if you have any further comments:-) https://codereview.chromium.org/2078893002/diff/180001/components/syncable_pr... File components/syncable_prefs/pref_model_associator.cc (right): https://codereview.chromium.org/2078893002/diff/180001/components/syncable_pr... components/syncable_prefs/pref_model_associator.cc:220: callback_list_.clear(); On 2016/07/06 22:11:56, pavely wrote: > I think it is possible that MergeDataAndStartSyncing is called multiple times > for a given object. Scenario would be: > - Sign-in, enable syncing everything (First time MDaSS is called) > - Disable syncing settings (StopSyncing is called) > - Enable syncing settings (Second time MDaSS is called) I think in this case even though the migration code will be called twice, user's settings will not change again on the second time, because all the domain scoped content settings have already been migrated to origin scoped styles and it will output the same result no matter how many times it be called later on. We will also register a pref to control this in the following CL, to make sure that callback in sync only gets called once. https://codereview.chromium.org/2078893002/diff/180001/components/syncable_pr... components/syncable_prefs/pref_model_associator.cc:227: On 2016/07/06 22:11:55, pavely wrote: > If you want to call a callback only for successful merge you need to move code > here. Done. https://codereview.chromium.org/2078893002/diff/180001/components/syncable_pr... File components/syncable_prefs/pref_model_associator.h (right): https://codereview.chromium.org/2078893002/diff/180001/components/syncable_pr... components/syncable_prefs/pref_model_associator.h:202: std::vector<base::Closure> callback_list_; On 2016/07/06 22:11:56, pavely wrote: > Check out CallbackList class. Maybe that's what you need here. I did try to use CallbackList here at first, and add Subscription in HostContentSettingsMap, which will deregister the callback when listener (here is HostContentSettingsMap) gets destroyed. Problem is PrefModelAssociator gets destroyed first when sync is done, and HostContentSettingsMap will last for longer time and gets destroyed when browser closes, that will cause core dumped since the callback list has already been destroyed. As in: https://cs.chromium.org/chromium/src/base/callback_list.h?sq=package:chromium...
lshang@chromium.org changed reviewers: + bauerb@chromium.org
Also +bauerb@ for review, PTAL thanks!
https://codereview.chromium.org/2078893002/diff/200001/chrome/browser/content... File chrome/browser/content_settings/host_content_settings_map_factory.cc (right): https://codereview.chromium.org/2078893002/diff/200001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_factory.cc:76: // TODO(lshang): Make this not a comment when we change scoping type and hook up The comment should be indented at the same level as the surrounding code, which is two spaces here. https://codereview.chromium.org/2078893002/diff/200001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_factory.cc:78: /* syncable_prefs::PrefServiceSyncable* pref_service = And this comment should use the regular double-slash style. https://codereview.chromium.org/2078893002/diff/200001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_factory.cc:80: if (pref_service) Add braces, as the body spans more than a single line. https://codereview.chromium.org/2078893002/diff/200001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_factory.cc:82: &HostContentSettingsMap::MigrateDomainScopedSettings, settings_map));*/ You are going to have to figure out the lifetime story here with regards to |settings_map|. Can you guarantee that |settings_map| is alive when the callback runs? If so, you can use base::Unretained(). If not, you'll have to find some other way.
> I currently commented out the registration code block in > HostContentSettingsMapFactory, I think we won't want it to start migrating > settings from this CL. Maybe we can add tests when we hook up all the migration > code and sync callbacks in the next CL? Are you OK with this? I think we should add a test for the sync code in this CL. I think we can remove the commented-out code in this CL and just add it in later when it's needed. https://codereview.chromium.org/2078893002/diff/200001/chrome/browser/content... File chrome/browser/content_settings/host_content_settings_map_factory.cc (right): https://codereview.chromium.org/2078893002/diff/200001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_factory.cc:82: &HostContentSettingsMap::MigrateDomainScopedSettings, settings_map));*/ I don't think we can make that guarantee (that I know of). I think we should get a weak pointer from the HostContentSettingsMap.
Description was changed from ========== Add migration callback 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 gets registered after construction of HostContentSettingsMap and gets executed at the end of PrefModelAssociator::MergeDataAndStartSyncing. BUG=604612 ========== to ========== 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 ==========
https://codereview.chromium.org/2078893002/diff/200001/chrome/browser/content... File chrome/browser/content_settings/host_content_settings_map_factory.cc (right): https://codereview.chromium.org/2078893002/diff/200001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_factory.cc:76: // TODO(lshang): Make this not a comment when we change scoping type and hook up On 2016/07/07 08:50:08, Bernhard Bauer wrote: > The comment should be indented at the same level as the surrounding code, which > is two spaces here. I moved this code block to the following CL, so this comment is deleted there. https://codereview.chromium.org/2078893002/diff/200001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_factory.cc:78: /* syncable_prefs::PrefServiceSyncable* pref_service = On 2016/07/07 08:50:08, Bernhard Bauer wrote: > And this comment should use the regular double-slash style. Also with this comment sign. https://codereview.chromium.org/2078893002/diff/200001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_factory.cc:80: if (pref_service) On 2016/07/07 08:50:08, Bernhard Bauer wrote: > Add braces, as the body spans more than a single line. Done in the following CL. https://codereview.chromium.org/2078893002/diff/200001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_factory.cc:82: &HostContentSettingsMap::MigrateDomainScopedSettings, settings_map));*/ On 2016/07/13 03:20:20, raymes wrote: > I don't think we can make that guarantee (that I know of). I think we should get > a weak pointer from the HostContentSettingsMap. Done.
On 2016/07/13 03:20:20, raymes wrote: > > I currently commented out the registration code block in > > HostContentSettingsMapFactory, I think we won't want it to start migrating > > settings from this CL. Maybe we can add tests when we hook up all the > migration > > code and sync callbacks in the next CL? Are you OK with this? > > I think we should add a test for the sync code in this CL. I think we can remove > the commented-out code in this CL and just add it in later when it's needed. > > https://codereview.chromium.org/2078893002/diff/200001/chrome/browser/content... > File chrome/browser/content_settings/host_content_settings_map_factory.cc > (right): > > https://codereview.chromium.org/2078893002/diff/200001/chrome/browser/content... > chrome/browser/content_settings/host_content_settings_map_factory.cc:82: > &HostContentSettingsMap::MigrateDomainScopedSettings, settings_map));*/ > I don't think we can make that guarantee (that I know of). I think we should get > a weak pointer from the HostContentSettingsMap. I added a simple test in pref_service_syncable to verify that the registered method gets called in MergeDataAndStartSyncing.
On 2016/07/13 03:20:20, raymes wrote: > > I currently commented out the registration code block in > > HostContentSettingsMapFactory, I think we won't want it to start migrating > > settings from this CL. Maybe we can add tests when we hook up all the > migration > > code and sync callbacks in the next CL? Are you OK with this? > > I think we should add a test for the sync code in this CL. I think we can remove > the commented-out code in this CL and just add it in later when it's needed. > Also the commented-out code is moved to the follow-up CL https://codereview.chromium.org/2075103002, I'll test more on that so it's not ready yet for now.
https://codereview.chromium.org/2078893002/diff/220001/components/syncable_pr... File components/syncable_prefs/pref_service_syncable_unittest.cc (right): https://codereview.chromium.org/2078893002/diff/220001/components/syncable_pr... components/syncable_prefs/pref_service_syncable_unittest.cc:124: next_pref_remote_sync_node_id_ = 0; This is not necessary, BTW (each test gets a new fixture insteance). https://codereview.chromium.org/2078893002/diff/220001/components/syncable_pr... components/syncable_prefs/pref_service_syncable_unittest.cc:125: callback_test_num = 0; I would initialize this in the constructor. https://codereview.chromium.org/2078893002/diff/220001/components/syncable_pr... components/syncable_prefs/pref_service_syncable_unittest.cc:208: void CallbackFunc() { callback_test_num += 1; } Nit: you could use ++. https://codereview.chromium.org/2078893002/diff/220001/components/syncable_pr... components/syncable_prefs/pref_service_syncable_unittest.cc:225: int callback_test_num; Nit: |num_callbacks|? Alternatively, you could use a static function (in an anonymous namespace) that takes an int* and increments it, and then bind it with the address of a variable on the stack. That way, you don't need to have any state in the fixture. https://codereview.chromium.org/2078893002/diff/220001/components/syncable_pr... components/syncable_prefs/pref_service_syncable_unittest.cc:564: EXPECT_EQ(callback_test_num, 0); The expected value goes first, for nicer error messages in case of failure.
https://codereview.chromium.org/2078893002/diff/220001/components/syncable_pr... File components/syncable_prefs/pref_service_syncable_unittest.cc (right): https://codereview.chromium.org/2078893002/diff/220001/components/syncable_pr... 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: > This is not necessary, BTW (each test gets a new fixture insteance). Done. https://codereview.chromium.org/2078893002/diff/220001/components/syncable_pr... components/syncable_prefs/pref_service_syncable_unittest.cc:125: callback_test_num = 0; On 2016/07/13 08:31:52, Bernhard Bauer wrote: > I would initialize this in the constructor. Done. Also added an underscore at the end of this variable's name since it's a member of the class. Oh this is deleted later since I moved it to the anonymous namespace. https://codereview.chromium.org/2078893002/diff/220001/components/syncable_pr... components/syncable_prefs/pref_service_syncable_unittest.cc:208: void CallbackFunc() { callback_test_num += 1; } On 2016/07/13 08:31:52, Bernhard Bauer wrote: > Nit: you could use ++. Done. https://codereview.chromium.org/2078893002/diff/220001/components/syncable_pr... components/syncable_prefs/pref_service_syncable_unittest.cc:225: int callback_test_num; On 2016/07/13 08:31:52, Bernhard Bauer wrote: > Nit: |num_callbacks|? > > Alternatively, you could use a static function (in an anonymous namespace) that > takes an int* and increments it, and then bind it with the address of a variable > on the stack. That way, you don't need to have any state in the fixture. Done. The alternative is really cool! https://codereview.chromium.org/2078893002/diff/220001/components/syncable_pr... components/syncable_prefs/pref_service_syncable_unittest.cc:564: EXPECT_EQ(callback_test_num, 0); On 2016/07/13 08:31:52, Bernhard Bauer wrote: > The expected value goes first, for nicer error messages in case of failure. Done.
https://codereview.chromium.org/2078893002/diff/240001/components/syncable_pr... File components/syncable_prefs/pref_service_syncable_unittest.cc (right): https://codereview.chromium.org/2078893002/diff/240001/components/syncable_pr... components/syncable_prefs/pref_service_syncable_unittest.cc:565: base::Bind(&CallbackFunc, &kNumCallbacks)); can kNumCallbacks be a local variable?
https://codereview.chromium.org/2078893002/diff/240001/components/syncable_pr... File components/syncable_prefs/pref_service_syncable_unittest.cc (right): https://codereview.chromium.org/2078893002/diff/240001/components/syncable_pr... components/syncable_prefs/pref_service_syncable_unittest.cc:565: base::Bind(&CallbackFunc, &kNumCallbacks)); On 2016/07/14 02:58:51, raymes wrote: > can kNumCallbacks be a local variable? Done.
lgtm
lgtm https://codereview.chromium.org/2078893002/diff/260001/components/syncable_pr... File components/syncable_prefs/pref_service_syncable_unittest.cc (right): https://codereview.chromium.org/2078893002/diff/260001/components/syncable_pr... components/syncable_prefs/pref_service_syncable_unittest.cc:49: void CallbackFunc(int* num) { Maybe call this method "increment"?
Thanks all! https://codereview.chromium.org/2078893002/diff/260001/components/syncable_pr... File components/syncable_prefs/pref_service_syncable_unittest.cc (right): https://codereview.chromium.org/2078893002/diff/260001/components/syncable_pr... components/syncable_prefs/pref_service_syncable_unittest.cc:49: void CallbackFunc(int* num) { On 2016/07/14 10:29:17, Bernhard Bauer wrote: > Maybe call this method "increment"? Done.
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2078893002/#ps280001 (title: "minor change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:280001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/2cf5c57f54ec0aa57d422f6f4f89dddd9c641051 Cr-Commit-Position: refs/heads/master@{#405669} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
