Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2635153002:
Pref service: expose all read-only PrefStores through Mojo (Closed)
|
Created:
3 years, 11 months ago by tibell Modified:
3 years, 9 months ago CC:
Aaron Boodman, abarth-chromium, blundell+watchlist_chromium.org, chromium-reviews, darin (slow to review), droger+watchlist_chromium.org, jonross, qsr+mojo_chromium.org, sdefresne+watchlist_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPref service: expose all read-only PrefStores through Mojo
This will eventually replace the existing preferences service
implementation.
High-level overview:
PrefStoreImpl: Wraps a PrefStore to expose its Observer interface as a Mojo
interface.
PrefStoreClient: Implements the PrefStore interface and acts as a cache,
providing synchronous access to prefs stored in a PrefStoreImpl.
PrefStoreManagerImpl: Facilitates connections between PrefStoreClients and
PrefStoreImpls.
PrefStoreAdapter: Helper to tie the lifetime of the PrefStoreImpl
to the underlying PrefStore (which is indirectly held by the
PrefService).
BUG=654988
Review-Url: https://codereview.chromium.org/2635153002
Cr-Commit-Position: refs/heads/master@{#455919}
Committed: https://chromium.googlesource.com/chromium/src/+/11141bd8441dc816fa284bca287005251170aff8
Patch Set 1 #Patch Set 2 : Rename mojo interface and impls #Patch Set 3 : Chrome can now start #Patch Set 4 : Fix compile and improve docs #Patch Set 5 : Improve tests #Patch Set 6 : Fix nullptr crash #Patch Set 7 : Deal with deletions #Patch Set 8 : Split out the PrefStore::GetValues change #Patch Set 9 : Create and register service #
Total comments: 39
Patch Set 10 : Don't use a native enum in mojom #Patch Set 11 : Simplify mojom interface #Patch Set 12 : Make OnPrefChanged value nullable #Patch Set 13 : Make registration optional to support unit tests #Patch Set 14 : Address review comments #
Total comments: 3
Patch Set 15 : Use array instead of map, since keys weren't hashable in WTF #Patch Set 16 : Switch back to unordered_map #Patch Set 17 : Remove debug print #
Total comments: 4
Patch Set 18 : Move on top of crrev.com/2715153004 #Patch Set 19 : Address review comments #Patch Set 20 : Merge #
Total comments: 33
Patch Set 21 : Address review comments #
Total comments: 8
Patch Set 22 : Merge #Patch Set 23 : Split out non-services/ changes into separate CL #Patch Set 24 : Address review comments #Patch Set 25 : Call ConnectionBarrier::Create correctly #Patch Set 26 : Add test for PrefStoreClient starting out initialized #
Total comments: 2
Patch Set 27 : Address sammc's review comments #Patch Set 28 : Merge #Patch Set 29 : Merge #
Total comments: 6
Patch Set 30 : Address bauerb's review comments #Messages
Total messages: 164 (119 generated)
The CQ bit was checked by tibell@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_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Rename mojo interface and impls
The CQ bit was checked by tibell@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_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 tibell@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_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Chrome can now start
The CQ bit was checked by tibell@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Fix compile and improve docs
The CQ bit was checked by tibell@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 checked by tibell@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...
Description was changed from ========== WIP: Convert all PrefStores to use Mojo BUG=654988 ========== to ========== Prefs service: wrap all read-only PrefStores in Mojo impls This is a first refactoring towards having all prefs access be over Mojo and go through a central prefs manager that pref store implementations (e.g extension prefs, supervised prefs) can register with. Writable prefs, i.e. user prefs, aren't yet accessed using Mojo. High-level overview: PrefStore: Add a virtual GetValues method that lets us get the initial state, needed to implement PrefStoreImpl. Implement this method in all subclasses. PrefStoreImpl: Wraps a PrefStore to expose its Observer interface as a Mojo interface. PrefStoreClient: Implements the PrefStore interface and acts as a cache, providing synchronous access to prefs stored in a PrefStoreImpl. Provides a helper to set up all objects needed at browser startup, before fully transitioning to a single, centralized PrefStoreManagerImpl. PrefStoreManagerImpl: Facilitates connections between PrefStoreClients and PrefStoreImpls. Initial implementation is "dumb", but future iterations will use central store for a PrefStoreImpls to register in. BUG=654988 ==========
Description was changed from ========== Prefs service: wrap all read-only PrefStores in Mojo impls This is a first refactoring towards having all prefs access be over Mojo and go through a central prefs manager that pref store implementations (e.g extension prefs, supervised prefs) can register with. Writable prefs, i.e. user prefs, aren't yet accessed using Mojo. High-level overview: PrefStore: Add a virtual GetValues method that lets us get the initial state, needed to implement PrefStoreImpl. Implement this method in all subclasses. PrefStoreImpl: Wraps a PrefStore to expose its Observer interface as a Mojo interface. PrefStoreClient: Implements the PrefStore interface and acts as a cache, providing synchronous access to prefs stored in a PrefStoreImpl. Provides a helper to set up all objects needed at browser startup, before fully transitioning to a single, centralized PrefStoreManagerImpl. PrefStoreManagerImpl: Facilitates connections between PrefStoreClients and PrefStoreImpls. Initial implementation is "dumb", but future iterations will use central store for a PrefStoreImpls to register in. BUG=654988 ========== to ========== Prefs service: wrap all read-only PrefStores in Mojo impls This is a first refactoring towards having all prefs access be over Mojo and go through a central prefs manager that pref store implementations (e.g extension prefs, supervised prefs) can register with. Writable prefs, i.e. user prefs, aren't yet accessed using Mojo. High-level overview: PrefStore: Add a virtual GetValues method that lets us get the initial state, needed to implement PrefStoreImpl. Implement this method in all subclasses. PrefStoreImpl: Wraps a PrefStore to expose its Observer interface as a Mojo interface. PrefStoreClient: Implements the PrefStore interface and acts as a cache, providing synchronous access to prefs stored in a PrefStoreImpl. Provides a helper to set up all objects needed at browser startup, before fully transitioning to a single, centralized PrefStoreManagerImpl. PrefStoreManagerImpl: Facilitates connections between PrefStoreClients and PrefStoreImpls. Initial implementation is "dumb", but future iterations will use central store for a PrefStoreImpls to register in. BUG=654988 ==========
tibell@chromium.org changed reviewers: + sammc@chromium.org
This is a first working version, to serve as a starting point for discussion.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Fix nullptr crash
The CQ bit was checked by tibell@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Deal with deletions
The CQ bit was checked by tibell@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...)
Split out the PrefStore::GetValues change
Description was changed from ========== Prefs service: wrap all read-only PrefStores in Mojo impls This is a first refactoring towards having all prefs access be over Mojo and go through a central prefs manager that pref store implementations (e.g extension prefs, supervised prefs) can register with. Writable prefs, i.e. user prefs, aren't yet accessed using Mojo. High-level overview: PrefStore: Add a virtual GetValues method that lets us get the initial state, needed to implement PrefStoreImpl. Implement this method in all subclasses. PrefStoreImpl: Wraps a PrefStore to expose its Observer interface as a Mojo interface. PrefStoreClient: Implements the PrefStore interface and acts as a cache, providing synchronous access to prefs stored in a PrefStoreImpl. Provides a helper to set up all objects needed at browser startup, before fully transitioning to a single, centralized PrefStoreManagerImpl. PrefStoreManagerImpl: Facilitates connections between PrefStoreClients and PrefStoreImpls. Initial implementation is "dumb", but future iterations will use central store for a PrefStoreImpls to register in. BUG=654988 ========== to ========== Prefs service: expose all read-only PrefStores through Mojo High-level overview: PrefStoreImpl: Wraps a PrefStore to expose its Observer interface as a Mojo interface. PrefStoreClient: Implements the PrefStore interface and acts as a cache, providing synchronous access to prefs stored in a PrefStoreImpl. Provides a helper to set up all objects needed at browser startup, before fully transitioning to a single, centralized PrefStoreManagerImpl. PrefStoreManagerImpl: Facilitates connections between PrefStoreClients and PrefStoreImpls. Initial implementation is "dumb", but future iterations will use central store for a PrefStoreImpls to register in. BUG=654988 ==========
Create and register service
The CQ bit was checked by tibell@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Don't use a native enum in mojom
The CQ bit was checked by tibell@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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
https://codereview.chromium.org/2635153002/diff/160001/components/prefs/pref_... File components/prefs/pref_value_store.h (right): https://codereview.chromium.org/2635153002/diff/160001/components/prefs/pref_... components/prefs/pref_value_store.h:130: enum PrefStoreType { Types before constructors. https://codereview.chromium.org/2635153002/diff/160001/components/prefs/pref_... components/prefs/pref_value_store.h:265: return static_cast<std::size_t>(type); size_t for an enum with a -1? https://codereview.chromium.org/2635153002/diff/160001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2635153002/diff/160001/content/public/browser... content/public/browser/content_browser_client.h:688: // Registers per-browser context services to be loaded in the browser process per-browser-context https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... File services/preferences/public/cpp/pref_store_client.h (right): https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/pref_store_client.h:54: PrefValueMap prefs_; How about cached_prefs_? https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/pref_store_client.h:58: // Has the PrefStore we're observing been initialized? Is this necessary? https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... File services/preferences/public/cpp/pref_store_impl.cc (right): https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/pref_store_impl.cc:55: if (initialized_) { Do we want to catch success after failure as well as failure after success? https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/pref_store_impl.cc:57: return; No error handling after DCHECKing. https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... File services/preferences/public/cpp/pref_store_manager_impl.cc (right): https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.cc:14: class PrefStoreManagerImpl::ConnectionBarrier Why is this nested? https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.cc:43: ptr.second->AddObserver( This is perilous if the first ptr is closed. RefCounted starts with a refcount of 0. The Bind call brings it to 1, and then drops it back to 0, if the pipe is broken, deleting |this|. Do this work in a method instead with something external holding a ref. https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.cc:52: return {PrefValueStore::MANAGED_STORE}; Is this correct? https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.cc:55: void PrefStoreManagerImpl::ConnectionBarrier::OnConnect( This should be above DefaultStores(). https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... File services/preferences/public/cpp/tests/pref_store_client_unittest.cc (right): https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:43: PrefStoreObserverMock* observer() { return &observer_; } How about returning a PrefStoreObserverMock&? https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:46: bool Initialized() { return store_->initialized_; } initialized() https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:70: mojom::PrefStoreConnectionPtr connection_ptr = connection https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:81: } store_.reset(); https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:92: const std::string key("hey"); Can't use const char []? https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:111: EXPECT_TRUE(store()->GetValue(key, &value)); ASSERT_TRUE https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:112: EXPECT_NE(nullptr, value); ASSERT_TRUE(value) https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:113: EXPECT_TRUE(value->GetAsInteger(&actual_value)); ASSERT_TRUE
Simplify mojom interface
The CQ bit was checked by tibell@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: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by tibell@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by tibell@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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Address review comments
The CQ bit was checked by tibell@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...
PTAL https://codereview.chromium.org/2635153002/diff/160001/components/prefs/pref_... File components/prefs/pref_value_store.h (right): https://codereview.chromium.org/2635153002/diff/160001/components/prefs/pref_... components/prefs/pref_value_store.h:130: enum PrefStoreType { On 2017/02/24 04:14:53, Sam McNally wrote: > Types before constructors. Done. https://codereview.chromium.org/2635153002/diff/160001/components/prefs/pref_... components/prefs/pref_value_store.h:265: return static_cast<std::size_t>(type); On 2017/02/24 04:14:53, Sam McNally wrote: > size_t for an enum with a -1? Done. https://codereview.chromium.org/2635153002/diff/160001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2635153002/diff/160001/content/public/browser... content/public/browser/content_browser_client.h:688: // Registers per-browser context services to be loaded in the browser process On 2017/02/24 04:14:53, Sam McNally wrote: > per-browser-context Done. https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... File services/preferences/public/cpp/pref_store_client.h (right): https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/pref_store_client.h:54: PrefValueMap prefs_; On 2017/02/24 04:14:53, Sam McNally wrote: > How about cached_prefs_? Done. https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/pref_store_client.h:58: // Has the PrefStore we're observing been initialized? On 2017/02/24 04:14:53, Sam McNally wrote: > Is this necessary? Needed for IsInitializationComplete (part of the PrefStore interface). https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... File services/preferences/public/cpp/pref_store_impl.cc (right): https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/pref_store_impl.cc:55: if (initialized_) { On 2017/02/24 04:14:53, Sam McNally wrote: > Do we want to catch success after failure as well as failure after success? We could. We'd have to change initialized_ to a 3 state enum to do that. What do you think? https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/pref_store_impl.cc:57: return; On 2017/02/24 04:14:53, Sam McNally wrote: > No error handling after DCHECKing. Done. https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... File services/preferences/public/cpp/pref_store_manager_impl.cc (right): https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.cc:14: class PrefStoreManagerImpl::ConnectionBarrier On 2017/02/24 04:14:53, Sam McNally wrote: > Why is this nested? Done. https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.cc:43: ptr.second->AddObserver( On 2017/02/24 04:14:53, Sam McNally wrote: > This is perilous if the first ptr is closed. RefCounted starts with a refcount > of 0. The Bind call brings it to 1, and then drops it back to 0, if the pipe is > broken, deleting |this|. > > Do this work in a method instead with something external holding a ref. Done. https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.cc:52: return {PrefValueStore::MANAGED_STORE}; On 2017/02/24 04:14:53, Sam McNally wrote: > Is this correct? It's temporary in two senses: 1) I'm messing with it while testing the CL. 2) I will fill it out before submitting/final review, but later I believe we'll instead initialize this somewhere at the Chrome layer and pass the config to the service in Init. https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.cc:55: void PrefStoreManagerImpl::ConnectionBarrier::OnConnect( On 2017/02/24 04:14:53, Sam McNally wrote: > This should be above DefaultStores(). Done. https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... File services/preferences/public/cpp/tests/pref_store_client_unittest.cc (right): https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:43: PrefStoreObserverMock* observer() { return &observer_; } On 2017/02/24 04:14:54, Sam McNally wrote: > How about returning a PrefStoreObserverMock&? VerifyAndClearExpectations needs a pointer. https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:46: bool Initialized() { return store_->initialized_; } On 2017/02/24 04:14:53, Sam McNally wrote: > initialized() Done. https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:70: mojom::PrefStoreConnectionPtr connection_ptr = On 2017/02/24 04:14:54, Sam McNally wrote: > connection I like this naming pattern for InterfacePtrs. https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:81: } On 2017/02/24 04:14:53, Sam McNally wrote: > store_.reset(); Done. https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:92: const std::string key("hey"); On 2017/02/24 04:14:53, Sam McNally wrote: > Can't use const char []? Done. https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:111: EXPECT_TRUE(store()->GetValue(key, &value)); On 2017/02/24 04:14:54, Sam McNally wrote: > ASSERT_TRUE Not needed since the next test won't crash even if we don't. https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:112: EXPECT_NE(nullptr, value); On 2017/02/24 04:14:54, Sam McNally wrote: > ASSERT_TRUE(value) Done. https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:113: EXPECT_TRUE(value->GetAsInteger(&actual_value)); On 2017/02/24 04:14:53, Sam McNally wrote: > ASSERT_TRUE If we get here this comparison can't crash.
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_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Use array instead of map, since keys weren't hashable in WTF
The CQ bit was checked by tibell@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...
https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... File services/preferences/public/cpp/pref_store_impl.cc (right): https://codereview.chromium.org/2635153002/diff/160001/services/preferences/p... services/preferences/public/cpp/pref_store_impl.cc:55: if (initialized_) { On 2017/02/27 00:02:54, tibell wrote: > On 2017/02/24 04:14:53, Sam McNally wrote: > > Do we want to catch success after failure as well as failure after success? > > We could. We'd have to change initialized_ to a 3 state enum to do that. What do > you think? Ideally PrefStores should behave correctly, but in the mean time, I would prefer this treating success followed by failure and failure followed by success as incorrect. https://codereview.chromium.org/2635153002/diff/260001/services/preferences/p... File services/preferences/public/cpp/pref_store_manager_impl.cc (right): https://codereview.chromium.org/2635153002/diff/260001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.cc:13: namespace { https://codereview.chromium.org/2635153002/diff/260001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.cc:49: void Create(const PrefStorePtrs& pref_store_ptrs, ConnectionBarrier::Create()? Also, this doesn't seem to be used anywhere. https://codereview.chromium.org/2635153002/diff/260001/services/preferences/p... File services/preferences/public/interfaces/preferences.mojom (right): https://codereview.chromium.org/2635153002/diff/260001/services/preferences/p... services/preferences/public/interfaces/preferences.mojom:68: bool is_initialized; Can we get rid of this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Switch back to unordered_map
The CQ bit was checked by tibell@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...
Remove debug print
The CQ bit was checked by tibell@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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2635153002/diff/320001/components/sync_prefer... File components/sync_preferences/DEPS (right): https://codereview.chromium.org/2635153002/diff/320001/components/sync_prefer... components/sync_preferences/DEPS:7: "+services/preferences", /public https://codereview.chromium.org/2635153002/diff/320001/components/sync_prefer... components/sync_preferences/DEPS:8: "+services/service_manager", /public
Move on top of crbug.com/2715153004
The CQ bit was checked by tibell@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 checked by tibell@chromium.org to run a CQ dry run
https://codereview.chromium.org/2635153002/diff/320001/components/sync_prefer... File components/sync_preferences/DEPS (right): https://codereview.chromium.org/2635153002/diff/320001/components/sync_prefer... components/sync_preferences/DEPS:7: "+services/preferences", On 2017/02/28 02:59:55, Sam McNally wrote: > /public Done. https://codereview.chromium.org/2635153002/diff/320001/components/sync_prefer... components/sync_preferences/DEPS:8: "+services/service_manager", On 2017/02/28 02:59:55, Sam McNally wrote: > /public Done.
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_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Prefs service: expose all read-only PrefStores through Mojo High-level overview: PrefStoreImpl: Wraps a PrefStore to expose its Observer interface as a Mojo interface. PrefStoreClient: Implements the PrefStore interface and acts as a cache, providing synchronous access to prefs stored in a PrefStoreImpl. Provides a helper to set up all objects needed at browser startup, before fully transitioning to a single, centralized PrefStoreManagerImpl. PrefStoreManagerImpl: Facilitates connections between PrefStoreClients and PrefStoreImpls. Initial implementation is "dumb", but future iterations will use central store for a PrefStoreImpls to register in. BUG=654988 ========== to ========== Prefs service: expose all read-only PrefStores through Mojo High-level overview: PrefStoreImpl: Wraps a PrefStore to expose its Observer interface as a Mojo interface. PrefStoreClient: Implements the PrefStore interface and acts as a cache, providing synchronous access to prefs stored in a PrefStoreImpl. PrefStoreManagerImpl: Facilitates connections between PrefStoreClients and PrefStoreImpls. PrefStoreAdapter: Helper to tie the lifetime of the PrefStoreImpl to the underlying PrefStore (which is indirectly held by the PrefService). BUG=654988 ==========
Description was changed from ========== Prefs service: expose all read-only PrefStores through Mojo High-level overview: PrefStoreImpl: Wraps a PrefStore to expose its Observer interface as a Mojo interface. PrefStoreClient: Implements the PrefStore interface and acts as a cache, providing synchronous access to prefs stored in a PrefStoreImpl. PrefStoreManagerImpl: Facilitates connections between PrefStoreClients and PrefStoreImpls. PrefStoreAdapter: Helper to tie the lifetime of the PrefStoreImpl to the underlying PrefStore (which is indirectly held by the PrefService). BUG=654988 ========== to ========== Prefs service: expose all read-only PrefStores through Mojo This will eventually replace the existing preferences service implementation. Hidden behind a flag until then. High-level overview: PrefStoreImpl: Wraps a PrefStore to expose its Observer interface as a Mojo interface. PrefStoreClient: Implements the PrefStore interface and acts as a cache, providing synchronous access to prefs stored in a PrefStoreImpl. PrefStoreManagerImpl: Facilitates connections between PrefStoreClients and PrefStoreImpls. PrefStoreAdapter: Helper to tie the lifetime of the PrefStoreImpl to the underlying PrefStore (which is indirectly held by the PrefService). BUG=654988 ==========
Description was changed from ========== Prefs service: expose all read-only PrefStores through Mojo This will eventually replace the existing preferences service implementation. Hidden behind a flag until then. High-level overview: PrefStoreImpl: Wraps a PrefStore to expose its Observer interface as a Mojo interface. PrefStoreClient: Implements the PrefStore interface and acts as a cache, providing synchronous access to prefs stored in a PrefStoreImpl. PrefStoreManagerImpl: Facilitates connections between PrefStoreClients and PrefStoreImpls. PrefStoreAdapter: Helper to tie the lifetime of the PrefStoreImpl to the underlying PrefStore (which is indirectly held by the PrefService). BUG=654988 ========== to ========== Prefs service: expose all read-only PrefStores through Mojo This will eventually replace the existing preferences service implementation. Hidden behind a flag until then. High-level overview: PrefStoreImpl: Wraps a PrefStore to expose its Observer interface as a Mojo interface. PrefStoreClient: Implements the PrefStore interface and acts as a cache, providing synchronous access to prefs stored in a PrefStoreImpl. PrefStoreManagerImpl: Facilitates connections between PrefStoreClients and PrefStoreImpls. PrefStoreAdapter: Helper to tie the lifetime of the PrefStoreImpl to the underlying PrefStore (which is indirectly held by the PrefService). BUG=654988 ==========
Merge
The CQ bit was checked by tibell@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2635153002/diff/380001/mojo/public/tools/bind... File mojo/public/tools/bindings/chromium_bindings_configuration.gni (right): https://codereview.chromium.org/2635153002/diff/380001/mojo/public/tools/bind... mojo/public/tools/bindings/chromium_bindings_configuration.gni:31: "//services/preferences/public/cpp/typemaps.gni", This should be before services/resource_coordinator/public/cpp/typemaps.gni. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... File services/preferences/public/cpp/pref_store_adapter.cc (right): https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_adapter.cc:13: PrefStoreAdapter::~PrefStoreAdapter() = default; Definitions should be in the same order as declarations. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... File services/preferences/public/cpp/pref_store_client.cc (right): https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_client.cc:16: PrefStoreClient::~PrefStoreClient() = default; Definitions should be in the same order as declarations. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... File services/preferences/public/cpp/pref_store_client.h (right): https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_client.h:46: void OnConnected(); Remove. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... File services/preferences/public/cpp/pref_store_impl.cc (right): https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_impl.cc:48: observer->OnPrefChanged(key, base::Value::CreateNullValue()); nullptr? https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_impl.cc:70: callback.Run(std::move(connection)); Inline constructing |connection|. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... File services/preferences/public/cpp/pref_store_impl.h (right): https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_impl.h:25: static std::unique_ptr<PrefStoreImpl> Create( Methods after destructors. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_impl.h:42: // The backing store we observer for changes. This a |WeakPtr| because if we I think this comment is mostly no longer necessary. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_impl.h:54: bool initialized_; How about naming this |backing_pref_store_initialized_| or something similar? https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... File services/preferences/public/cpp/pref_store_manager_impl.cc (right): https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.cc:98: if (!pref_store_ptrs_.insert(std::make_pair(type, std::move(pref_store_ptr))) I think bool success = pref_store_ptrs_.insert(...).second; DCHECK(success) ...; is more common. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.cc:104: if (!pending_callbacks_.empty()) { Remove this? https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... File services/preferences/public/cpp/pref_store_manager_impl.h (right): https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.h:34: PrefStoreManagerImpl(PrefStoreTypes expected_pref_stores); explicit https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.h:37: // PrefStores used by Chrome by default. That's a layering violation. This should be somewhere in chrome/browser/prefs. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.h:70: using PrefStorePtrs = Types before methods. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... File services/preferences/public/cpp/tests/pref_store_client_unittest.cc (right): https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:55: void SetUp() override; Why are these out of line? https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:73: connection_ptr->initial_prefs = base::MakeUnique<base::DictionaryValue>(); Add tests for |initial_prefs| not being empty and |is_initialized| being true.
Address review comments
The CQ bit was checked by tibell@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...
PTAL. After you've had another look I will split out the non-services/preferences/ changes in a follow up CL to make it easier to review for Chrome owners. https://codereview.chromium.org/2635153002/diff/380001/mojo/public/tools/bind... File mojo/public/tools/bindings/chromium_bindings_configuration.gni (right): https://codereview.chromium.org/2635153002/diff/380001/mojo/public/tools/bind... mojo/public/tools/bindings/chromium_bindings_configuration.gni:31: "//services/preferences/public/cpp/typemaps.gni", On 2017/03/03 03:47:58, Sam McNally wrote: > This should be before services/resource_coordinator/public/cpp/typemaps.gni. Done. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... File services/preferences/public/cpp/pref_store_adapter.cc (right): https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_adapter.cc:13: PrefStoreAdapter::~PrefStoreAdapter() = default; On 2017/03/03 03:47:58, Sam McNally wrote: > Definitions should be in the same order as declarations. Done. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... File services/preferences/public/cpp/pref_store_client.cc (right): https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_client.cc:16: PrefStoreClient::~PrefStoreClient() = default; On 2017/03/03 03:47:58, Sam McNally wrote: > Definitions should be in the same order as declarations. Done. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... File services/preferences/public/cpp/pref_store_client.h (right): https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_client.h:46: void OnConnected(); On 2017/03/03 03:47:58, Sam McNally wrote: > Remove. Done. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... File services/preferences/public/cpp/pref_store_impl.cc (right): https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_impl.cc:48: observer->OnPrefChanged(key, base::Value::CreateNullValue()); On 2017/03/03 03:47:58, Sam McNally wrote: > nullptr? Done. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_impl.cc:70: callback.Run(std::move(connection)); On 2017/03/03 03:47:58, Sam McNally wrote: > Inline constructing |connection|. Done. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... File services/preferences/public/cpp/pref_store_impl.h (right): https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_impl.h:25: static std::unique_ptr<PrefStoreImpl> Create( On 2017/03/03 03:47:58, Sam McNally wrote: > Methods after destructors. Done. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_impl.h:42: // The backing store we observer for changes. This a |WeakPtr| because if we On 2017/03/03 03:47:58, Sam McNally wrote: > I think this comment is mostly no longer necessary. Done. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_impl.h:54: bool initialized_; On 2017/03/03 03:47:58, Sam McNally wrote: > How about naming this |backing_pref_store_initialized_| or something similar? Done. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... File services/preferences/public/cpp/pref_store_manager_impl.cc (right): https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.cc:98: if (!pref_store_ptrs_.insert(std::make_pair(type, std::move(pref_store_ptr))) On 2017/03/03 03:47:58, Sam McNally wrote: > I think > bool success = pref_store_ptrs_.insert(...).second; > DCHECK(success) ...; > is more common. Done. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.cc:104: if (!pending_callbacks_.empty()) { On 2017/03/03 03:47:58, Sam McNally wrote: > Remove this? Done. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... File services/preferences/public/cpp/pref_store_manager_impl.h (right): https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.h:34: PrefStoreManagerImpl(PrefStoreTypes expected_pref_stores); On 2017/03/03 03:47:58, Sam McNally wrote: > explicit Done. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.h:37: // PrefStores used by Chrome by default. On 2017/03/03 03:47:58, Sam McNally wrote: > That's a layering violation. This should be somewhere in chrome/browser/prefs. Done. I just removed this for now. We'll do the setup in another CL that hooks everything up. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.h:70: using PrefStorePtrs = On 2017/03/03 03:47:58, Sam McNally wrote: > Types before methods. Done. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... File services/preferences/public/cpp/tests/pref_store_client_unittest.cc (right): https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:55: void SetUp() override; On 2017/03/03 03:47:58, Sam McNally wrote: > Why are these out of line? Done. https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:73: connection_ptr->initial_prefs = base::MakeUnique<base::DictionaryValue>(); On 2017/03/03 03:47:58, Sam McNally wrote: > Add tests for |initial_prefs| not being empty and |is_initialized| being true. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Merge
The CQ bit was checked by tibell@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...
https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... File services/preferences/public/cpp/tests/pref_store_client_unittest.cc (right): https://codereview.chromium.org/2635153002/diff/380001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:73: connection_ptr->initial_prefs = base::MakeUnique<base::DictionaryValue>(); On 2017/03/07 00:52:52, tibell wrote: > On 2017/03/03 03:47:58, Sam McNally wrote: > > Add tests for |initial_prefs| not being empty and |is_initialized| being true. > > Done. I don't see one covering |is_initialized| being true. https://codereview.chromium.org/2635153002/diff/400001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences2_manifest.json (right): https://codereview.chromium.org/2635153002/diff/400001/chrome/browser/prefs/p... chrome/browser/prefs/preferences2_manifest.json:11: "requires": { Can you remove this section? https://codereview.chromium.org/2635153002/diff/400001/services/preferences/p... File services/preferences/public/cpp/pref_store_manager_impl.cc (right): https://codereview.chromium.org/2635153002/diff/400001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.cc:25: ConnectionBarrier(const PrefStorePtrs& pref_store_ptrs, Make this private. https://codereview.chromium.org/2635153002/diff/400001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.cc:106: make_scoped_refptr(new ConnectionBarrier(pref_store_ptrs_, callback)); ConnectionBarrier::Create https://codereview.chromium.org/2635153002/diff/400001/services/preferences/p... File services/preferences/public/cpp/preferences_struct_traits_macros.h (right): https://codereview.chromium.org/2635153002/diff/400001/services/preferences/p... services/preferences/public/cpp/preferences_struct_traits_macros.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. Delete.
Split out non-services/ changes into separate CL
The CQ bit was checked by tibell@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...
tibell@chromium.org changed reviewers: + bauerb@chromium.org, mbarbella@chromium.org
This is the ground work for the new preference service at uses the existing PrefService C++ library as a client side lib (i.e. allowing existing users of prefs to use a sync interface) but allows PrefService to be backed my a Mojo service. A follow-up CL will make the changes needed to browser/ etc to start this service and use it from the browser. bauerb@chromium.org: please review components/prefs mbarbella@chromium.org: please review the mojom sammc@chromium.org: please review everything
Description was changed from ========== Prefs service: expose all read-only PrefStores through Mojo This will eventually replace the existing preferences service implementation. Hidden behind a flag until then. High-level overview: PrefStoreImpl: Wraps a PrefStore to expose its Observer interface as a Mojo interface. PrefStoreClient: Implements the PrefStore interface and acts as a cache, providing synchronous access to prefs stored in a PrefStoreImpl. PrefStoreManagerImpl: Facilitates connections between PrefStoreClients and PrefStoreImpls. PrefStoreAdapter: Helper to tie the lifetime of the PrefStoreImpl to the underlying PrefStore (which is indirectly held by the PrefService). BUG=654988 ========== to ========== Prefs service: expose all read-only PrefStores through Mojo This will eventually replace the existing preferences service implementation. High-level overview: PrefStoreImpl: Wraps a PrefStore to expose its Observer interface as a Mojo interface. PrefStoreClient: Implements the PrefStore interface and acts as a cache, providing synchronous access to prefs stored in a PrefStoreImpl. PrefStoreManagerImpl: Facilitates connections between PrefStoreClients and PrefStoreImpls. PrefStoreAdapter: Helper to tie the lifetime of the PrefStoreImpl to the underlying PrefStore (which is indirectly held by the PrefService). BUG=654988 ==========
On 2017/03/07 01:55:57, tibell wrote: > This is the ground work for the new preference service at uses the existing > PrefService C++ library as a client side lib (i.e. allowing existing users of > prefs to use a sync interface) but allows PrefService to be backed my a Mojo > service. A follow-up CL will make the changes needed to browser/ etc to start > this service and use it from the browser. > > mailto:bauerb@chromium.org: please review components/prefs > mailto:mbarbella@chromium.org: please review the mojom > mailto:sammc@chromium.org: please review everything mbarbella@chromium.org: Once the pref stores are remote we need a way to talk about which pref store is connected where, hence exposing the enum in PrefValueStore.
Address review comments
The CQ bit was checked by tibell@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...
tibell@chromium.org changed reviewers: + rockot@chromium.org
PTAL rockot@chromium.org: Please review mojo/public/tools/bindings/chromium_bindings_configuration.gni https://codereview.chromium.org/2635153002/diff/400001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences2_manifest.json (right): https://codereview.chromium.org/2635153002/diff/400001/chrome/browser/prefs/p... chrome/browser/prefs/preferences2_manifest.json:11: "requires": { On 2017/03/07 01:47:13, Sam McNally wrote: > Can you remove this section? Done. https://codereview.chromium.org/2635153002/diff/400001/services/preferences/p... File services/preferences/public/cpp/pref_store_manager_impl.cc (right): https://codereview.chromium.org/2635153002/diff/400001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.cc:25: ConnectionBarrier(const PrefStorePtrs& pref_store_ptrs, On 2017/03/07 01:47:13, Sam McNally wrote: > Make this private. Done. https://codereview.chromium.org/2635153002/diff/400001/services/preferences/p... services/preferences/public/cpp/pref_store_manager_impl.cc:106: make_scoped_refptr(new ConnectionBarrier(pref_store_ptrs_, callback)); On 2017/03/07 01:47:13, Sam McNally wrote: > ConnectionBarrier::Create Done. https://codereview.chromium.org/2635153002/diff/400001/services/preferences/p... File services/preferences/public/cpp/preferences_struct_traits_macros.h (right): https://codereview.chromium.org/2635153002/diff/400001/services/preferences/p... services/preferences/public/cpp/preferences_struct_traits_macros.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/03/07 01:47:13, Sam McNally wrote: > Delete. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Call ConnectionBarrier::Create correctly
The CQ bit was checked by tibell@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...
Add test for PrefStoreClient starting out initialized
The CQ bit was checked by tibell@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/2635153002/diff/500001/services/preferences/p... File services/preferences/public/cpp/tests/pref_store_client_unittest.cc (right): https://codereview.chromium.org/2635153002/diff/500001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:183: EXPECT_TRUE(value->GetAsInteger(&actual_value)); Either ASSERT_TRUE here or initialize |actual_value|.
lgtm
https://codereview.chromium.org/2635153002/diff/500001/services/preferences/p... File services/preferences/public/cpp/tests/pref_store_client_unittest.cc (right): https://codereview.chromium.org/2635153002/diff/500001/services/preferences/p... services/preferences/public/cpp/tests/pref_store_client_unittest.cc:183: EXPECT_TRUE(value->GetAsInteger(&actual_value)); On 2017/03/07 03:38:08, Sam McNally wrote: > Either ASSERT_TRUE here or initialize |actual_value|. Done.
The CQ bit was checked by tibell@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_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
security lgtm
The CQ bit was checked by tibell@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...
bauerb@chromium.org: Just a quick heads-up that the CL you already approved (https://crrev.com/2740493002/) depends on this one, for which you're also a reviewer. Hope I didn't confuse things too much by splitting them up!
Merge
The CQ bit was checked by tibell@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.
Merge
The CQ bit was checked by tibell@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.
Description was changed from ========== Prefs service: expose all read-only PrefStores through Mojo This will eventually replace the existing preferences service implementation. High-level overview: PrefStoreImpl: Wraps a PrefStore to expose its Observer interface as a Mojo interface. PrefStoreClient: Implements the PrefStore interface and acts as a cache, providing synchronous access to prefs stored in a PrefStoreImpl. PrefStoreManagerImpl: Facilitates connections between PrefStoreClients and PrefStoreImpls. PrefStoreAdapter: Helper to tie the lifetime of the PrefStoreImpl to the underlying PrefStore (which is indirectly held by the PrefService). BUG=654988 ========== to ========== Pref service: expose all read-only PrefStores through Mojo This will eventually replace the existing preferences service implementation. High-level overview: PrefStoreImpl: Wraps a PrefStore to expose its Observer interface as a Mojo interface. PrefStoreClient: Implements the PrefStore interface and acts as a cache, providing synchronous access to prefs stored in a PrefStoreImpl. PrefStoreManagerImpl: Facilitates connections between PrefStoreClients and PrefStoreImpls. PrefStoreAdapter: Helper to tie the lifetime of the PrefStoreImpl to the underlying PrefStore (which is indirectly held by the PrefService). BUG=654988 ==========
lgtm https://codereview.chromium.org/2635153002/diff/560001/components/prefs/pref_... File components/prefs/pref_value_store.h (right): https://codereview.chromium.org/2635153002/diff/560001/components/prefs/pref_... components/prefs/pref_value_store.h:43: enum PrefStoreType { Historically we haven't exposed the exact pref store types on purpose, but I guess exposing it for the purpose of the Mojo service is probably okay, because that is tied very closely to prefs anyway. https://codereview.chromium.org/2635153002/diff/560001/components/prefs/pref_... components/prefs/pref_value_store.h:263: struct hash<PrefValueStore::PrefStoreType> { It might almost be worth making a macro out of this to allow hashing of enum classes in general... https://codereview.chromium.org/2635153002/diff/560001/services/preferences/p... File services/preferences/public/cpp/pref_store_adapter.h (right): https://codereview.chromium.org/2635153002/diff/560001/services/preferences/p... services/preferences/public/cpp/pref_store_adapter.h:17: // Ties the life time of a |PrefStoreImpl| to a |PrefStore|. Otherwise acts as a Nit: "lifetime" is a single word.
Address bauerb's review comments
https://codereview.chromium.org/2635153002/diff/560001/components/prefs/pref_... File components/prefs/pref_value_store.h (right): https://codereview.chromium.org/2635153002/diff/560001/components/prefs/pref_... components/prefs/pref_value_store.h:43: enum PrefStoreType { On 2017/03/09 09:25:22, Bernhard Bauer wrote: > Historically we haven't exposed the exact pref store types on purpose, but I > guess exposing it for the purpose of the Mojo service is probably okay, because > that is tied very closely to prefs anyway. Yeah. I also felt it was a bit unfortunate but I couldn't see a better way once we have store connecting to a central manager. We need a way to know which store connected. https://codereview.chromium.org/2635153002/diff/560001/components/prefs/pref_... components/prefs/pref_value_store.h:263: struct hash<PrefValueStore::PrefStoreType> { On 2017/03/09 09:25:22, Bernhard Bauer wrote: > It might almost be worth making a macro out of this to allow hashing of enum > classes in general... Agreed. I'll see what other Chromies think. https://codereview.chromium.org/2635153002/diff/560001/services/preferences/p... File services/preferences/public/cpp/pref_store_adapter.h (right): https://codereview.chromium.org/2635153002/diff/560001/services/preferences/p... services/preferences/public/cpp/pref_store_adapter.h:17: // Ties the life time of a |PrefStoreImpl| to a |PrefStore|. Otherwise acts as a On 2017/03/09 09:25:22, Bernhard Bauer wrote: > Nit: "lifetime" is a single word. Done.
The CQ bit was checked by tibell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, sammc@chromium.org, mbarbella@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2635153002/#ps580001 (title: "Address bauerb's review comments")
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": 580001, "attempt_start_ts": 1489100951830750,
"parent_rev": "e062cff433a3c2ee02cb9e45d480093a7b958272", "commit_rev":
"11141bd8441dc816fa284bca287005251170aff8"}
Message was sent while issue was closed.
Description was changed from ========== Pref service: expose all read-only PrefStores through Mojo This will eventually replace the existing preferences service implementation. High-level overview: PrefStoreImpl: Wraps a PrefStore to expose its Observer interface as a Mojo interface. PrefStoreClient: Implements the PrefStore interface and acts as a cache, providing synchronous access to prefs stored in a PrefStoreImpl. PrefStoreManagerImpl: Facilitates connections between PrefStoreClients and PrefStoreImpls. PrefStoreAdapter: Helper to tie the lifetime of the PrefStoreImpl to the underlying PrefStore (which is indirectly held by the PrefService). BUG=654988 ========== to ========== Pref service: expose all read-only PrefStores through Mojo This will eventually replace the existing preferences service implementation. High-level overview: PrefStoreImpl: Wraps a PrefStore to expose its Observer interface as a Mojo interface. PrefStoreClient: Implements the PrefStore interface and acts as a cache, providing synchronous access to prefs stored in a PrefStoreImpl. PrefStoreManagerImpl: Facilitates connections between PrefStoreClients and PrefStoreImpls. PrefStoreAdapter: Helper to tie the lifetime of the PrefStoreImpl to the underlying PrefStore (which is indirectly held by the PrefService). BUG=654988 Review-Url: https://codereview.chromium.org/2635153002 Cr-Commit-Position: refs/heads/master@{#455919} Committed: https://chromium.googlesource.com/chromium/src/+/11141bd8441dc816fa284bca2870... ==========
Message was sent while issue was closed.
Committed patchset #30 (id:580001) as https://chromium.googlesource.com/chromium/src/+/11141bd8441dc816fa284bca2870... |
