Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2778643002:
Pref service: Filter updates from read-only pref stores. (Closed)
|
Created:
3 years, 9 months ago by Sam McNally Modified:
3 years, 8 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPref service: Filter updates from read-only pref stores.
This changes prefs::PrefStoreImpl to restrict pref change observations
to only the prefs registered by the client and forwards that data from
PrefStoreManagerImpl through prefs::mojom::PrefStore::AddObserver()
calls.
BUG=654988
Review-Url: https://codereview.chromium.org/2778643002
Cr-Commit-Position: refs/heads/master@{#460269}
Committed: https://chromium.googlesource.com/chromium/src/+/29914e3f0bc84b0ec570b086a17bc41254f4cc34
Patch Set 1 : #
Total comments: 32
Patch Set 2 : #Patch Set 3 : #Depends on Patchset: Messages
Total messages: 53 (46 generated)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
sammc@chromium.org changed reviewers: + tibell@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pr... File services/preferences/pref_service_factory_unittest.cc (right): https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pr... services/preferences/pref_service_factory_unittest.cc:239: below_user_prefs_pref_store()->SetValue( Add: // This update is needed to check that the change to kKey has propagated even though we will not observe it change. or something similar. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... File services/preferences/public/cpp/pref_store_impl.cc (right): https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/pref_store_impl.cc:80: for (auto& observer : observers_) const https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/pref_store_impl.cc:83: for (auto& observer : observers_) const https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/pref_store_impl.cc:94: for (auto& observer : observers_) const https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... File services/preferences/public/cpp/tests/pref_store_impl_unittest.cc (right): https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:30: MOCK_METHOD1(OnInitializationCompleted, void(bool succeeded)); Can drop |succeeded| here for consistency with the above method. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:83: void CreateImpl(scoped_refptr<PrefStore> backing_pref_store, Could this just return the PrefStore instead? Less state to worry about in the test. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:133: .Times(1) Times(1) is the default so you can remove. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:150: .Times(1) ditto https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:164: ASSERT_TRUE(pref_store()->IsInitializationComplete()); Maybe add a TODO here too to check what the backing pref store does in this case (i.e. is having values enough to be considered initialized)? https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:193: EXPECT_CALL(observer, OnPrefValueChanged(kKey)) Could this be lifted out into a ExpectPrefChange(PrefStore, const string&) helper? https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:194: .Times(1) Remove https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:218: .Times(1) Remove https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:239: .Times(1) Remove https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:269: .Times(1) Remove https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:294: .Times(1) Remove https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:324: .Times(1) Remove
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sammc@chromium.org changed reviewers: + bauerb@chromium.org, mbarbella@chromium.org
+bauerb for //components/sync_preferences +mbarbella for the mojom https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pr... File services/preferences/pref_service_factory_unittest.cc (right): https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pr... services/preferences/pref_service_factory_unittest.cc:239: below_user_prefs_pref_store()->SetValue( On 2017/03/27 03:37:55, tibell wrote: > Add: > > // This update is needed to check that the change to kKey has propagated even > though we will not observe it change. > > or something similar. Done. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... File services/preferences/public/cpp/pref_store_impl.cc (right): https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/pref_store_impl.cc:80: for (auto& observer : observers_) On 2017/03/27 03:37:56, tibell wrote: > const Done. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/pref_store_impl.cc:83: for (auto& observer : observers_) On 2017/03/27 03:37:56, tibell wrote: > const Done. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/pref_store_impl.cc:94: for (auto& observer : observers_) On 2017/03/27 03:37:56, tibell wrote: > const Done. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... File services/preferences/public/cpp/tests/pref_store_impl_unittest.cc (right): https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:30: MOCK_METHOD1(OnInitializationCompleted, void(bool succeeded)); On 2017/03/27 03:37:56, tibell wrote: > Can drop |succeeded| here for consistency with the above method. Done. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:83: void CreateImpl(scoped_refptr<PrefStore> backing_pref_store, On 2017/03/27 03:37:56, tibell wrote: > Could this just return the PrefStore instead? Less state to worry about in the > test. Different tests use different PrefStores. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:133: .Times(1) On 2017/03/27 03:37:56, tibell wrote: > Times(1) is the default so you can remove. Done. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:150: .Times(1) On 2017/03/27 03:37:56, tibell wrote: > ditto Done. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:164: ASSERT_TRUE(pref_store()->IsInitializationComplete()); On 2017/03/27 03:37:56, tibell wrote: > Maybe add a TODO here too to check what the backing pref store does in this case > (i.e. is having values enough to be considered initialized)? As discussed, this isn't testing that the presence of values causes this to be initialized; ValueMapPrefStore is always initialized so the client should also always be initialized. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:193: EXPECT_CALL(observer, OnPrefValueChanged(kKey)) On 2017/03/27 03:37:56, tibell wrote: > Could this be lifted out into a > > ExpectPrefChange(PrefStore, const string&) > > helper? Done. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:194: .Times(1) On 2017/03/27 03:37:56, tibell wrote: > Remove Done. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:218: .Times(1) On 2017/03/27 03:37:56, tibell wrote: > Remove Done. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:239: .Times(1) On 2017/03/27 03:37:56, tibell wrote: > Remove Done. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:269: .Times(1) On 2017/03/27 03:37:56, tibell wrote: > Remove Done. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:294: .Times(1) On 2017/03/27 03:37:56, tibell wrote: > Remove Done. https://codereview.chromium.org/2778643002/diff/20001/services/preferences/pu... services/preferences/public/cpp/tests/pref_store_impl_unittest.cc:324: .Times(1) On 2017/03/27 03:37:56, tibell wrote: > Remove Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sync_preferences/ LGTM.
mojom lgtm
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CQ has no permission to schedule in bucket master.tryserver.chromium.win
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sammc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tibell@chromium.org Link to the patchset: https://codereview.chromium.org/2778643002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490759539484210,
"parent_rev": "3ab8e7fc26c16ec0c127b67d4f75a216eb85eb64", "commit_rev":
"29914e3f0bc84b0ec570b086a17bc41254f4cc34"}
Message was sent while issue was closed.
Description was changed from ========== Pref service: Filter updates from read-only pref stores. This changes prefs::PrefStoreImpl to restrict pref change observations to only the prefs registered by the client and forwards that data from PrefStoreManagerImpl through prefs::mojom::PrefStore::AddObserver() calls. BUG=654988 ========== to ========== Pref service: Filter updates from read-only pref stores. This changes prefs::PrefStoreImpl to restrict pref change observations to only the prefs registered by the client and forwards that data from PrefStoreManagerImpl through prefs::mojom::PrefStore::AddObserver() calls. BUG=654988 Review-Url: https://codereview.chromium.org/2778643002 Cr-Commit-Position: refs/heads/master@{#460269} Committed: https://chromium.googlesource.com/chromium/src/+/29914e3f0bc84b0ec570b086a17b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/chromium/src/+/29914e3f0bc84b0ec570b086a17b... |
