Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2745563005:
Pref service: add support for tracked prefs. (Closed)
|
Created:
3 years, 9 months ago by Sam McNally Modified:
3 years, 8 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, droger+watchlist_chromium.org, viettrungluu+watch_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), chrome-apps-syd-reviews_chromium.org, gab Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPref service: add support for tracked prefs.
This:
- Adds TrackedPersistentPrefStoreConfiguration: a mojo struct containing
the necessary state for constructing a PrefStore identical to the one
constructed in ProfilePrefStoreManager::CreateProfilePrefStore().
- Moves the enums used to configure tracked prefs into the mojom.
- Extracts the creation of tracked pref store from
ProfilePrefStoreManager::CreateProfilePrefStore() into
prefs::CreateTrackedPersistentPrefStore().
- Changes prefs::CreatePersistentPrefStore() so an appropriate
configuration results in prefs::CreateTrackedPersistentPrefStore()
being called to create the backing PrefStore.
BUG=654988
Review-Url: https://codereview.chromium.org/2745563005
Cr-Original-Commit-Position: refs/heads/master@{#462301}
Committed: https://chromium.googlesource.com/chromium/src/+/d2e2fed00150274e4a6857c700dcb24f99793bd7
Review-Url: https://codereview.chromium.org/2745563005
Cr-Commit-Position: refs/heads/master@{#462395}
Committed: https://chromium.googlesource.com/chromium/src/+/cbfdf1af972416d91212925a613fb169a31a157a
Patch Set 1 : #
Total comments: 4
Patch Set 2 : #Patch Set 3 : around go the deps #
Total comments: 12
Patch Set 4 : rebase onto https://codereview.chromium.org/2751603002/ #Patch Set 5 : #
Total comments: 6
Patch Set 6 : #Patch Set 7 : rebase #
Total comments: 4
Patch Set 8 : #
Total comments: 10
Patch Set 9 : rebase #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : rebase #Patch Set 14 : #
Total comments: 2
Patch Set 15 : #Patch Set 16 : #Patch Set 17 : #Patch Set 18 : rebase #
Total comments: 14
Patch Set 19 : #Patch Set 20 : rebase #Patch Set 21 : deflake tests #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 137 (102 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
https://codereview.chromium.org/2745563005/diff/20001/services/preferences/pe... File services/preferences/persistent_pref_store_factory.cc (right): https://codereview.chromium.org/2745563005/diff/20001/services/preferences/pe... services/preferences/persistent_pref_store_factory.cc:78: std::unique_ptr<PersistentPrefStoreImpl> CreateTrackedPersistentPrefStore( Does this method have to be kept in sync with some other method? If so can we at least have a comment pointing at the other one? Could the code instead be shared somehow? https://codereview.chromium.org/2745563005/diff/20001/services/preferences/pu... File services/preferences/public/interfaces/preferences_configuration.mojom (right): https://codereview.chromium.org/2745563005/diff/20001/services/preferences/pu... services/preferences/public/interfaces/preferences_configuration.mojom:20: struct TrackedPersistentPrefStoreConfiguration { Could you refer to the C++ function taking all these parameters in a comment so a reader could look it up if needed?
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 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 ========== Pref service: add support for tracked prefs. This: - Adds TrackedPersistentPrefStoreConfiguration: a mojo struct containing the necessary state for constructing a PrefStore identical to the one constructed in ProfilePrefStoreManager::CreateProfilePrefStore(). - Moves the enums used to configure tracked prefs into the mojom. - Duplicates the PersistentPrefStore creation from ProfilePrefStoreManager::CreateProfilePrefStore() into prefs::CreatePersistentPrefStore() so that an identically-configured tracked pref store will be configured the same. BUG=654988 ========== to ========== Pref service: add support for tracked prefs. This: - Adds TrackedPersistentPrefStoreConfiguration: a mojo struct containing the necessary state for constructing a PrefStore identical to the one constructed in ProfilePrefStoreManager::CreateProfilePrefStore(). - Moves the enums used to configure tracked prefs into the mojom. - Extracts the creation of tracked pref store from ProfilePrefStoreManager::CreateProfilePrefStore() into prefs::CreateTrackedPersistentPrefStore(). - Changes prefs::CreatePersistentPrefStore() so an appropriate configuration results in prefs::CreateTrackedPersistentPrefStore() being called to create the backing PrefStore. BUG=654988 ==========
https://codereview.chromium.org/2745563005/diff/20001/services/preferences/pe... File services/preferences/persistent_pref_store_factory.cc (right): https://codereview.chromium.org/2745563005/diff/20001/services/preferences/pe... services/preferences/persistent_pref_store_factory.cc:78: std::unique_ptr<PersistentPrefStoreImpl> CreateTrackedPersistentPrefStore( On 2017/03/10 04:29:01, tibell wrote: > Does this method have to be kept in sync with some other method? If so can we at > least have a comment pointing at the other one? Could the code instead be shared > somehow? Extracted out the common parts. https://codereview.chromium.org/2745563005/diff/20001/services/preferences/pu... File services/preferences/public/interfaces/preferences_configuration.mojom (right): https://codereview.chromium.org/2745563005/diff/20001/services/preferences/pu... services/preferences/public/interfaces/preferences_configuration.mojom:20: struct TrackedPersistentPrefStoreConfiguration { On 2017/03/10 04:29:01, tibell wrote: > Could you refer to the C++ function taking all these parameters in a comment so > a reader could look it up if needed? Done.
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_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: This issue passed the CQ dry run.
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 //chrome/browser/prefs, //components/user_prefs/tracked and DEPS changes +mbarbella for the mojom
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_...)
LGTM +Gab FYI
gab@chromium.org changed reviewers: + gab@chromium.org
lg but a few things first (sorry, late to party, was OOO) https://codereview.chromium.org/2745563005/diff/60001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager.cc (left): https://codereview.chromium.org/2745563005/diff/60001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:138: GetPrefHashStore(true), GetExternalVerificationPrefHashStorePair(), It's weird to drop usage of GetExternalVerificationPrefHashStorePair() here but keep it in InitializePrefsFromMasterPrefs() (or rather, I'm not a fan of passing all the params by value instead of a few distinct prefs/tracked objects). https://codereview.chromium.org/2745563005/diff/60001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:150: // TODO(gab): Remove kDeprecatedProtectedPreferencesFilename as an alternate Could you do this cleanup in a precursor CL (and also remove the interface for providing an alternate filename to JsonPrefStore's constructor -- pretty sure it's still only used by this)? https://codereview.chromium.org/2745563005/diff/60001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2745563005/diff/60001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:97: const scoped_refptr<base::SequencedTaskRunner>& io_task_runner, C++11 adjustment: Take scoped_refptr by value and std::move() below. https://codereview.chromium.org/2745563005/diff/60001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:100: std::unique_ptr<PrefFilter> pref_filter; Unused? https://codereview.chromium.org/2745563005/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_filter.h (left): https://codereview.chromium.org/2745563005/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.h:48: enum EnforcementLevel { NO_ENFORCEMENT, ENFORCE_ON_LOAD }; Can we turn these into "enum class" in a precursor CL and forward all of the deltas caused by this to that CL (this one is already big enough like this and it's hard to review with all the no-op diffs caused by this). https://codereview.chromium.org/2745563005/diff/60001/services/preferences/pu... File services/preferences/public/cpp/tracked_persistent_pref_store_factory.cc (right): https://codereview.chromium.org/2745563005/diff/60001/services/preferences/pu... services/preferences/public/cpp/tracked_persistent_pref_store_factory.cc:58: ::PersistentPrefStore* CreateTrackedPersistentPrefStore( Why the :: prefix? Explicit global namespace is unusual for Chromium classes (usually done to highlight calling third-party APIs)
Patchset #4 (id:80001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) 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...)
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
https://codereview.chromium.org/2745563005/diff/60001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager.cc (left): https://codereview.chromium.org/2745563005/diff/60001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:138: GetPrefHashStore(true), GetExternalVerificationPrefHashStorePair(), On 2017/03/13 16:52:02, gab (OOO until Mar 13) wrote: > It's weird to drop usage of GetExternalVerificationPrefHashStorePair() here but > keep it in InitializePrefsFromMasterPrefs() (or rather, I'm not a fan of passing > all the params by value instead of a few distinct prefs/tracked objects). In the longer term, the configuration will only be sent over mojo so it pretty much has to be data and delegates. https://codereview.chromium.org/2745563005/diff/60001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:150: // TODO(gab): Remove kDeprecatedProtectedPreferencesFilename as an alternate On 2017/03/13 16:52:01, gab (OOO until Mar 13) wrote: > Could you do this cleanup in a precursor CL (and also remove the interface for > providing an alternate filename to JsonPrefStore's constructor -- pretty sure > it's still only used by this)? https://codereview.chromium.org/2751603002/ https://codereview.chromium.org/2745563005/diff/60001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2745563005/diff/60001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:97: const scoped_refptr<base::SequencedTaskRunner>& io_task_runner, On 2017/03/13 16:52:02, gab (OOO until Mar 13) wrote: > C++11 adjustment: Take scoped_refptr by value and std::move() below. Done. https://codereview.chromium.org/2745563005/diff/60001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:100: std::unique_ptr<PrefFilter> pref_filter; On 2017/03/13 16:52:02, gab (OOO until Mar 13) wrote: > Unused? Done. https://codereview.chromium.org/2745563005/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_filter.h (left): https://codereview.chromium.org/2745563005/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.h:48: enum EnforcementLevel { NO_ENFORCEMENT, ENFORCE_ON_LOAD }; On 2017/03/13 16:52:02, gab (OOO until Mar 13) wrote: > Can we turn these into "enum class" in a precursor CL and forward all of the > deltas caused by this to that CL (this one is already big enough like this and > it's hard to review with all the no-op diffs caused by this). https://codereview.chromium.org/2752533002/ https://codereview.chromium.org/2745563005/diff/60001/services/preferences/pu... File services/preferences/public/cpp/tracked_persistent_pref_store_factory.cc (right): https://codereview.chromium.org/2745563005/diff/60001/services/preferences/pu... services/preferences/public/cpp/tracked_persistent_pref_store_factory.cc:58: ::PersistentPrefStore* CreateTrackedPersistentPrefStore( On 2017/03/13 16:52:02, gab (OOO until Mar 13) wrote: > Why the :: prefix? Explicit global namespace is unusual for Chromium classes > (usually done to highlight calling third-party APIs) Removed it.
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.
mojom lgtm
Meta-comment: I think the mojo service should be ProfilePrefStoreManager. It's already the component responsible for handing off tracked pref stores -- and that explains why I thought the initial separation of concerns was weird as it forces passing intermediary data which is supposed to be an implementation detail and is now exposed on a public interface. (still posting pending nits but they become irrelevant if we change course I think -- and otherwise I'm still not done with the review since I aborted per the above) https://codereview.chromium.org/2745563005/diff/140002/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2745563005/diff/140002/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:67: local_state_(local_state) {} Also just realized this param is no longer used. https://codereview.chromium.org/2745563005/diff/140002/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:102: io_task_runner, std::unique_ptr<PrefFilter>()); std::move here as well (since returning reference is no longer needed) https://codereview.chromium.org/2745563005/diff/140002/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:127: PrefHashFilter(GetPrefHashStore(), inline GetPrefHashStore() here as this is now the only caller
On 2017/03/15 15:09:58, gab wrote: > Meta-comment: I think the mojo service should be ProfilePrefStoreManager. It's > already the component responsible for handing off tracked pref stores -- and > that explains why I thought the initial separation of concerns was weird as it > forces passing intermediary data which is supposed to be an implementation > detail and is now exposed on a public interface. > The functionality in ProfilePrefStoreManager::CreateProfilePrefStore() is being duplicated in the mojo service (mostly by extracting out prefs::CreateTrackedPersistentPrefStore()). In order to keep the pref service below the //chrome layer, all of the configuration needs to be passed in over a mojo interface. Currently ProfilePrefStoreManager::CreateProfilePrefStore() pulls its configuration from fields, constants and parameters; this extracts out a function that takes all the configuration as parameters and adds a mojo struct containing those parameters. Put another way, ProfilePrefStoreManager both decides what sort of pref store to use and creates it; in the future, ProfilePrefStoreManager (or perhaps something else in //chrome/browser/prefs) will decide what sort of pref store and ask the pref service (over mojo) to create it. The functionality duplication is so we can have both the old and the new working in parallel.
Patchset #6 (id:170001) 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...
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...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2745563005/diff/140002/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2745563005/diff/140002/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:67: local_state_(local_state) {} On 2017/03/15 15:09:58, gab wrote: > Also just realized this param is no longer used. Done. https://codereview.chromium.org/2745563005/diff/140002/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:102: io_task_runner, std::unique_ptr<PrefFilter>()); On 2017/03/15 15:09:58, gab wrote: > std::move here as well (since returning reference is no longer needed) Done. https://codereview.chromium.org/2745563005/diff/140002/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:127: PrefHashFilter(GetPrefHashStore(), On 2017/03/15 15:09:58, gab wrote: > inline GetPrefHashStore() here as this is now the only caller Done.
Sorry for being slow and blocking here but I just don't see the benefit added by this change... The way I understand it mojo is supposed to help componentize things yet this CL only appears to increase the "spaghetti-level" around this code..? Maybe my below two meta comments can help guide why I'm feeling this way? IMO there should be a clear tree architecture but right now some nodes use the service which isn't itself a leaf (reaches back into classes like PrefHashFilter also used by higher nodes). https://codereview.chromium.org/2745563005/diff/210001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2745563005/diff/210001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:114: validation_delegate, on_reset_on_load); I'm not a fan of this mumbo-jumbo passing of arguments... is this typical of mojo interfaces? It's usually frowned upon in Chromium to have methods that take numerous parameters of the same type (e.g. multiple bool methods were refactored into enums over the recent years) -- it makes it hard to read and error-prone. Can we perhaps build the config struct client side (explicitly setting its members by name) and send that over? https://codereview.chromium.org/2745563005/diff/210001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:125: PrefHashFilter( I find it weird to have classes like PrefHashFilter used on both sides (i.e. here and in the service). IMO any tracked-pref class used in the service should be moved as an implementation detail of it (if that helps as a first pass: I'm fine adding a temporary dependency from the existing code to services/preferences/ while the calling code is moved to the service)
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...
On 2017/03/20 16:36:26, gab wrote: > Sorry for being slow and blocking here but I just don't see the benefit added by > this change... > > The way I understand it mojo is supposed to help componentize things yet this CL > only appears to increase the "spaghetti-level" around this code..? > > Maybe my below two meta comments can help guide why I'm feeling this way? IMO > there should be a clear tree architecture but right now some nodes use the > service which isn't itself a leaf (reaches back into classes like PrefHashFilter > also used by higher nodes). > I agree this is a bit unfortunate. We're targeting building a usable pref service in parallel with the existing code. Once the pref service is the one way everyone gets at prefs, we can hide all the implementation details away. I would prefer not to move it into a new location if we're just going to move it again once the external uses are cleaned up. https://codereview.chromium.org/2745563005/diff/210001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2745563005/diff/210001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:114: validation_delegate, on_reset_on_load); On 2017/03/20 16:36:26, gab wrote: > I'm not a fan of this mumbo-jumbo passing of arguments... is this typical of > mojo interfaces? It's usually frowned upon in Chromium to have methods that take > numerous parameters of the same type (e.g. multiple bool methods were refactored > into enums over the recent years) -- it makes it hard to read and error-prone. > > Can we perhaps build the config struct client side (explicitly setting its > members by name) and send that over? Done. The struct doesn't quite match what this class wants to send because the non-data parts are expected to be mojo interfaces, but that's only going to be an issue until the transition is complete. https://codereview.chromium.org/2745563005/diff/210001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:125: PrefHashFilter( On 2017/03/20 16:36:26, gab wrote: > I find it weird to have classes like PrefHashFilter used on both sides (i.e. > here and in the service). > > IMO any tracked-pref class used in the service should be moved as an > implementation detail of it (if that helps as a first pass: I'm fine adding a > temporary dependency from the existing code to services/preferences/ while the > calling code is moved to the service) That's the plan. The tracked-pref implementation should be internal to the service; only the configuration should be public. However, in the immediate future, while we transition from using prefs directly to using prefs via the service, various parts like this one will have to be shared.
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...)
On 2017/03/20 23:45:41, Sam McNally wrote: > On 2017/03/20 16:36:26, gab wrote: > > Sorry for being slow and blocking here but I just don't see the benefit added > by > > this change... > > > > The way I understand it mojo is supposed to help componentize things yet this > CL > > only appears to increase the "spaghetti-level" around this code..? > > > > Maybe my below two meta comments can help guide why I'm feeling this way? IMO > > there should be a clear tree architecture but right now some nodes use the > > service which isn't itself a leaf (reaches back into classes like > PrefHashFilter > > also used by higher nodes). > > > I agree this is a bit unfortunate. We're targeting building a usable pref > service in parallel with the existing code. Once the pref service is the one way > everyone gets at prefs, we can hide all the implementation details away. I would > prefer not to move it into a new location if we're just going to move it again > once the external uses are cleaned up. I don't see how the service is being built in "parallel"? The way I see it this CL fully moves the instantiations into the service (whether the service is a live mojo service or just a method call for now I don't know -- and it doesn't matter from my point of view -- what I care about is where the code lives and avoiding spaghetti calls back and forth between code layers). https://codereview.chromium.org/2745563005/diff/230001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2745563005/diff/230001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:126: PrefHashFilter( After this CL this is the only instantiation of PrefHashFilter outside of the service, if we servicify this call as well then PrefHashFilter can be made an impl detail right away.
Hi Gabriel! Since the pref service is otherwise ready to go (modulo some performance benchmarks) and people are waiting to use it in mash, I want to make sure I understand the concerns here. I read your comments above a couple of times but I can't quite put my finger on your main concerns. Here are some possible readings: * components/prefs and services/preferences get intertwined - I think we should basically see them as one thing (e.g. no layering between the two). Things in components/prefs that aren't publicly used or hard to move (like components/pref_service.h) will eventually migrate into services/preferences. (All service implementations should eventually live in services/.) * The migration lands us in intermediate states before it's done - I don't think there's a way to avoid this. We can't land everything in one giant CL and we need to keep working as more parts of it are migrated to use the service instead of accessing the preferences directly so developing behind the flag is the only option. -- Johan
To me separate GN targets = separate components = separate layers. This CL adds components/user_prefs -> services/preferences -> components/user_prefs "spaghetti". As highlighted in a comment below, this code is fragile. Furthermore, the failure mode is catastrophic, i.e. if you get a seed wrong you can instantly wipe the entire Chrome Canary population's settings when settings protection erroneously kicks in... (which is not recoverable by design since an escape hatch would be the first thing attackers would use...). One of the proposal I was making to avoid spaghetti code is to move all classes that should only be used by the service to services/preferences/ right away and have a temporary DEPS from components/prefs -> non-public part of services/preferences/ (off the top of my head you can probably move all of components/user_prefs/tracked/). That way we at least keep things unidirectional. I understand that we can't do it in one giga CL but I prefer the above option (I've seen too many cases of moving-on-when-it-works-and-never-coming-back-to-fix-it over the years, not saying I don't trust you personally, that's just how software development tends to happen and I prefer to do the move cleanly first instead of making it work first with a promise of fixing the mess later). I also don't see this "flag" you keep referring to? The way I see it this code *is* redirected in this CL? Maybe the service isn't live and it just runs synchronously barring this "flag", but that doesn't change the code architecture (which is what I'm after). Left a few more comments while skimming through once more. Thanks for your patience, Gab https://codereview.chromium.org/2745563005/diff/230001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2745563005/diff/230001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:172: prefs::mojom::TrackedPreferenceValidationDelegatePtr validation_delegate_ptr; Unused? https://codereview.chromium.org/2745563005/diff/230001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:173: return prefs::mojom::TrackedPersistentPrefStoreConfiguration::New( Where are things like ::New() documented? I dug around to figure out whether this was a smart or raw pointer... First looked at https://www.chromium.org/developers/design-documents/mojo then at https://www.chromium.org/developers/design-documents/mojo/mojom-syntax eventually resolved to code searching a similar struct https://cs.chromium.org/search/?q=SimplePersistentPrefStoreConfiguration::New... and clicking through eventually figured out this is a StructPtr which internally holds a std::unique_ptr... Feedback: the mojo semantics are all very obscure to me... https://codereview.chromium.org/2745563005/diff/230001/services/preferences/p... File services/preferences/persistent_pref_store_factory.cc (right): https://codereview.chromium.org/2745563005/diff/230001/services/preferences/p... services/preferences/persistent_pref_store_factory.cc:60: on_reset_on_load), Why are the last two params special cased from the config? https://codereview.chromium.org/2745563005/diff/230001/services/preferences/p... services/preferences/persistent_pref_store_factory.cc:73: if (configuration->is_tracked_configuration()) { Purely observational, no action required. To keep things positive: I admit that while the mojom magic is fairly obscure at first, it's kind of neat to have all this auto generated code :) https://codereview.chromium.org/2745563005/diff/230001/services/preferences/p... File services/preferences/public/cpp/tracked_persistent_pref_store_factory.cc (right): https://codereview.chromium.org/2745563005/diff/230001/services/preferences/p... services/preferences/public/cpp/tracked_persistent_pref_store_factory.cc:89: config->seed, config->legacy_device_id, config->registry_path, This is the wrong seed, the external hash store uses a separate seed. This highlights my concern about tweaking this code and enhances my desire for it to move as-is as much as possible (e.g. can we avoid duplicating GetExternalVerificationPrefHashStorePair()?)
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 #11 (id:290001) has been deleted
Patchset #11 (id:310001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) 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-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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_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_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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 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 #16 (id:430001) has been deleted
Patchset #15 (id:410001) has been deleted
Patchset #14 (id:390001) has been deleted
I've rebased this onto the move CL and moved all the tracked prefs implementation-touching parts from ProfilePrefStoreManager into //services/preferences/tracked, using the mojo config struct to communicate the configuration. https://codereview.chromium.org/2745563005/diff/230001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2745563005/diff/230001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:172: prefs::mojom::TrackedPreferenceValidationDelegatePtr validation_delegate_ptr; On 2017/03/28 16:42:08, gab wrote: > Unused? Done. https://codereview.chromium.org/2745563005/diff/230001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:173: return prefs::mojom::TrackedPersistentPrefStoreConfiguration::New( On 2017/03/28 16:42:08, gab wrote: > Where are things like ::New() documented? I dug around to figure out whether > this was a smart or raw pointer... > > First looked at > https://www.chromium.org/developers/design-documents/mojo > > then at > https://www.chromium.org/developers/design-documents/mojo/mojom-syntax > > eventually resolved to code searching a similar struct > https://cs.chromium.org/search/?q=SimplePersistentPrefStoreConfiguration::New... > > and clicking through eventually figured out this is a StructPtr which internally > holds a std::unique_ptr... > > > Feedback: the mojo semantics are all very obscure to me... It does appear that mojo structs and unions are not documented (at least not anywhere obvious). https://codereview.chromium.org/2745563005/diff/230001/services/preferences/p... File services/preferences/persistent_pref_store_factory.cc (right): https://codereview.chromium.org/2745563005/diff/230001/services/preferences/p... services/preferences/persistent_pref_store_factory.cc:60: on_reset_on_load), On 2017/03/28 16:42:08, gab wrote: > Why are the last two params special cased from the config? Changed them to not be. https://codereview.chromium.org/2745563005/diff/230001/services/preferences/p... File services/preferences/public/cpp/tracked_persistent_pref_store_factory.cc (right): https://codereview.chromium.org/2745563005/diff/230001/services/preferences/p... services/preferences/public/cpp/tracked_persistent_pref_store_factory.cc:89: config->seed, config->legacy_device_id, config->registry_path, On 2017/03/28 16:42:08, gab wrote: > This is the wrong seed, the external hash store uses a separate seed. > > This highlights my concern about tweaking this code and enhances my desire for > it to move as-is as much as possible (e.g. can we avoid duplicating > GetExternalVerificationPrefHashStorePair()?) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, I like this structure much better, Bernhard (or another mojo familiar person) can you please have another look, the mojo stuff has changed a lot and I don't feel comfortable reviewing that... For one, it seems convoluted to hook a simple Closure callback (i.e. on_reset_on_load parameter -> bound into ResetOnLoadObserverImpl which is a prefs::mojom::ResetOnLoadObserver -> bound via a mojo::MakeStrongBinding (don't know what those do..) -> passed into prefs::mojom::TrackedPersistentPrefStoreConfiguration -> on service side bind another on_reset_on_load to ForwardToResetOnLoadObserver() which unwinds the whole thing when invoked :-O!
I have to admit, I don't know enough about Mojo either to give detailed feedback. I agree that passing the reset_on_load callback seems more complicated now than it used to be, but maybe that's just Mojo for ya… :-/ https://codereview.chromium.org/2745563005/diff/450001/services/preferences/t... File services/preferences/tracked/tracked_persistent_pref_store_factory.cc (right): https://codereview.chromium.org/2745563005/diff/450001/services/preferences/t... services/preferences/tracked/tracked_persistent_pref_store_factory.cc:139: configuration->tracking_configuration, base::Closure(), NULL, Nit: nullptr instead of NULL
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 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...
On 2017/03/30 15:28:41, gab wrote: > Thanks, I like this structure much better, Bernhard (or another mojo familiar > person) can you please have another look, the mojo stuff has changed a lot and I > don't feel comfortable reviewing that... > > For one, it seems convoluted to hook a simple Closure callback (i.e. > on_reset_on_load parameter -> bound into ResetOnLoadObserverImpl which is a > prefs::mojom::ResetOnLoadObserver -> bound via a mojo::MakeStrongBinding (don't > know what those do..) -> passed into > prefs::mojom::TrackedPersistentPrefStoreConfiguration -> on service side bind > another on_reset_on_load to ForwardToResetOnLoadObserver() which unwinds the > whole thing when invoked :-O! I've removed the unnecessary wrapping. I was going to come back to that in a later CL, but since you asked. https://codereview.chromium.org/2745563005/diff/450001/services/preferences/t... File services/preferences/tracked/tracked_persistent_pref_store_factory.cc (right): https://codereview.chromium.org/2745563005/diff/450001/services/preferences/t... services/preferences/tracked/tracked_persistent_pref_store_factory.cc:139: configuration->tracking_configuration, base::Closure(), NULL, On 2017/03/30 20:11:57, Bernhard Bauer wrote: > Nit: nullptr instead of NULL Done.
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_...)
lgtm Took another look at the Mojo setup. Still 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: 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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Just did another full pass, this is looking really nice now :), just a few things and we're good to go! https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/c... File chrome/browser/prefs/chrome_pref_service_factory.cc (right): https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/c... chrome/browser/prefs/chrome_pref_service_factory.cc:467: mojo::MakeRequest(&reset_on_load_observer)); General Mojo question (since I can't find documentation for this either): what does this do? And how would I know to write this as a non-mojo guru? i.e. why aren't we just passing base::MakeUnique<ResetOnLoadObserverImpl>(profile_path) in CreateProfilePrefStore() below? https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:171: std::move(tracking_configuration_), reporting_ids_count_, seed_, This move means this ProfilePrefStoreManager can only be used once to CreateProfilePrefStore(). While this is true in practice, it wasn't previously a requirement. The other two moves below are parameters so that's okay, but consuming members is something else.. Could we instead pass the tracking_configuration as a parameter to CreateProfilePrefStore() as well? https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right): https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:624: INSTANTIATE_TEST_CASE_P(ProfilePrefStoreManagerTest, Add a comment to document what the bool toggles (being a service or not looks like?) https://codereview.chromium.org/2745563005/diff/530001/services/preferences/t... File services/preferences/tracked/segregated_pref_store.cc (right): https://codereview.chromium.org/2745563005/diff/530001/services/preferences/t... services/preferences/tracked/segregated_pref_store.cc:60: validation_delegate_(std::move(validation_delegate)) { This member is unused? https://codereview.chromium.org/2745563005/diff/530001/services/preferences/t... File services/preferences/tracked/tracked_persistent_pref_store_factory.h (right): https://codereview.chromium.org/2745563005/diff/530001/services/preferences/t... services/preferences/tracked/tracked_persistent_pref_store_factory.h:20: base::SequencedWorkerPool* worker_pool); This is running service side, right? (i.e. not a /public interface) Is it really appropriate to take a base::SequencedWorkerPool* here? Seems this ties this service to the calling process and also to this executing while the refcount on |worker_pool| is still up? Does it always execute synchronously and I'm just not seeing it?
Patchset #19 (id:550001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/c... File chrome/browser/prefs/chrome_pref_service_factory.cc (right): https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/c... chrome/browser/prefs/chrome_pref_service_factory.cc:467: mojo::MakeRequest(&reset_on_load_observer)); On 2017/04/03 16:11:17, gab wrote: > General Mojo question (since I can't find documentation for this either): what > does this do? And how would I know to write this as a non-mojo guru? > > i.e. why aren't we just passing > base::MakeUnique<ResetOnLoadObserverImpl>(profile_path) in > CreateProfilePrefStore() below? There are docs for this now: https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindi.... In particular, this is the pattern described in https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindi.... We want to pass to the service the client end of a mojo pipe pointing at the ResetOnLoadObserverImpl (in this case using a StrongBinding to manage the impl lifetime). https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:171: std::move(tracking_configuration_), reporting_ids_count_, seed_, On 2017/04/03 16:11:17, gab wrote: > This move means this ProfilePrefStoreManager can only be used once to > CreateProfilePrefStore(). While this is true in practice, it wasn't previously a > requirement. The other two moves below are parameters so that's okay, but > consuming members is something else.. > > Could we instead pass the tracking_configuration as a parameter to > CreateProfilePrefStore() as well? Done. https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right): https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:624: INSTANTIATE_TEST_CASE_P(ProfilePrefStoreManagerTest, On 2017/04/03 16:11:17, gab wrote: > Add a comment to document what the bool toggles (being a service or not looks > like?) Done. https://codereview.chromium.org/2745563005/diff/530001/services/preferences/t... File services/preferences/tracked/segregated_pref_store.cc (right): https://codereview.chromium.org/2745563005/diff/530001/services/preferences/t... services/preferences/tracked/segregated_pref_store.cc:60: validation_delegate_(std::move(validation_delegate)) { On 2017/04/03 16:11:17, gab wrote: > This member is unused? It's used by the PrefHashFilters attached to the two PrefStores. They need somewhere external to manage its lifetime. Added a comment. https://codereview.chromium.org/2745563005/diff/530001/services/preferences/t... File services/preferences/tracked/tracked_persistent_pref_store_factory.h (right): https://codereview.chromium.org/2745563005/diff/530001/services/preferences/t... services/preferences/tracked/tracked_persistent_pref_store_factory.h:20: base::SequencedWorkerPool* worker_pool); On 2017/04/03 16:11:17, gab wrote: > This is running service side, right? (i.e. not a /public interface) Yes. > > Is it really appropriate to take a base::SequencedWorkerPool* here? Seems this > ties this service to the calling process and also to this executing while the > refcount on |worker_pool| is still up? Does it always execute synchronously and > I'm just not seeing it? When the service is used, |worker_pool| is provided by the service. In the short term, the service runs in-process and the end-session shutdown procedure needs to be able to find the task runner used to write prefs so it can wait for that to complete.
LGTM, woohoo! Thanks!! https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/c... File chrome/browser/prefs/chrome_pref_service_factory.cc (right): https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/c... chrome/browser/prefs/chrome_pref_service_factory.cc:467: mojo::MakeRequest(&reset_on_load_observer)); On 2017/04/04 03:24:49, Sam McNally wrote: > On 2017/04/03 16:11:17, gab wrote: > > General Mojo question (since I can't find documentation for this either): what > > does this do? And how would I know to write this as a non-mojo guru? > > > > i.e. why aren't we just passing > > base::MakeUnique<ResetOnLoadObserverImpl>(profile_path) in > > CreateProfilePrefStore() below? > > There are docs for this now: > https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindi.... > In particular, this is the pattern described in > https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindi.... > > We want to pass to the service the client end of a mojo pipe pointing at the > ResetOnLoadObserverImpl (in this case using a StrongBinding to manage the impl > lifetime). Awesome, thanks! PS: Guess this is easy now as this OnResetOnLoad() override makes a static call, but lifetime would be more complex if that object needed to be teared down before something else it owned, right? What would you do then? Refcount its state? https://codereview.chromium.org/2745563005/diff/530001/services/preferences/t... File services/preferences/tracked/tracked_persistent_pref_store_factory.h (right): https://codereview.chromium.org/2745563005/diff/530001/services/preferences/t... services/preferences/tracked/tracked_persistent_pref_store_factory.h:20: base::SequencedWorkerPool* worker_pool); On 2017/04/04 03:24:49, Sam McNally wrote: > On 2017/04/03 16:11:17, gab wrote: > > This is running service side, right? (i.e. not a /public interface) > > Yes. > > > > Is it really appropriate to take a base::SequencedWorkerPool* here? Seems this > > ties this service to the calling process and also to this executing while the > > refcount on |worker_pool| is still up? Does it always execute synchronously > and > > I'm just not seeing it? > > When the service is used, |worker_pool| is provided by the service. In the short > term, the service runs in-process and the end-session shutdown procedure needs > to be able to find the task runner used to write prefs so it can wait for that > to complete. Ok, that works for now then, medium term SequencedWorkerPool will be replaced by base/task_scheduler anyways which has an implicit instance in every process and won't need to be passed around :) Where is the code where "|worker_pool| is provided by the service"? (trying to understand the mojo paradigms as much as possible :))
sammc@chromium.org changed reviewers: + mlerman@chromium.org
+mlerman for //chrome/browser/profiles https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/c... File chrome/browser/prefs/chrome_pref_service_factory.cc (right): https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/c... chrome/browser/prefs/chrome_pref_service_factory.cc:467: mojo::MakeRequest(&reset_on_load_observer)); On 2017/04/04 20:59:51, gab wrote: > On 2017/04/04 03:24:49, Sam McNally wrote: > > On 2017/04/03 16:11:17, gab wrote: > > > General Mojo question (since I can't find documentation for this either): > what > > > does this do? And how would I know to write this as a non-mojo guru? > > > > > > i.e. why aren't we just passing > > > base::MakeUnique<ResetOnLoadObserverImpl>(profile_path) in > > > CreateProfilePrefStore() below? > > > > There are docs for this now: > > > https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindi.... > > In particular, this is the pattern described in > > > https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindi.... > > > > We want to pass to the service the client end of a mojo pipe pointing at the > > ResetOnLoadObserverImpl (in this case using a StrongBinding to manage the impl > > lifetime). > > Awesome, thanks! > > PS: Guess this is easy now as this OnResetOnLoad() override makes a static call, > but lifetime would be more complex if that object needed to be teared down > before something else it owned, right? What would you do then? Refcount its > state? Probably something like how it was managed before: ProfileImpl would own both the implementation and a Binding. Then when the shared state goes away, the mojo connection would break. https://codereview.chromium.org/2745563005/diff/530001/services/preferences/t... File services/preferences/tracked/tracked_persistent_pref_store_factory.h (right): https://codereview.chromium.org/2745563005/diff/530001/services/preferences/t... services/preferences/tracked/tracked_persistent_pref_store_factory.h:20: base::SequencedWorkerPool* worker_pool); On 2017/04/04 20:59:51, gab wrote: > On 2017/04/04 03:24:49, Sam McNally wrote: > > On 2017/04/03 16:11:17, gab wrote: > > > This is running service side, right? (i.e. not a /public interface) > > > > Yes. > > > > > > Is it really appropriate to take a base::SequencedWorkerPool* here? Seems > this > > > ties this service to the calling process and also to this executing while > the > > > refcount on |worker_pool| is still up? Does it always execute synchronously > > and > > > I'm just not seeing it? > > > > When the service is used, |worker_pool| is provided by the service. In the > short > > term, the service runs in-process and the end-session shutdown procedure needs > > to be able to find the task runner used to write prefs so it can wait for that > > to complete. > > Ok, that works for now then, medium term SequencedWorkerPool will be replaced by > base/task_scheduler anyways which has an implicit instance in every process and > won't need to be passed around :) > > Where is the code where "|worker_pool| is provided by the service"? (trying to > understand the mojo paradigms as much as possible :)) The main class of the service PrefStoreManagerImpl has a ref to a SequencedWorkerPool, which it passes into the common PersistentPrefStore factory function, which forwards it to this function if tracked prefs are enabled. PrefStoreManagerImpl receives its SequencedWorkerPool at construction time (which chrome provides since it's running in the browser process).
profiles LGTM
The CQ bit was checked by sammc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, mbarbella@chromium.org, tibell@chromium.org Link to the patchset: https://codereview.chromium.org/2745563005/#ps570001 (title: " ")
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
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by sammc@chromium.org
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": 570001, "attempt_start_ts": 1491432689920860,
"parent_rev": "103ae538bcf760e23ad6307dc5d33583ec877b89", "commit_rev":
"d2e2fed00150274e4a6857c700dcb24f99793bd7"}
Message was sent while issue was closed.
Description was changed from ========== Pref service: add support for tracked prefs. This: - Adds TrackedPersistentPrefStoreConfiguration: a mojo struct containing the necessary state for constructing a PrefStore identical to the one constructed in ProfilePrefStoreManager::CreateProfilePrefStore(). - Moves the enums used to configure tracked prefs into the mojom. - Extracts the creation of tracked pref store from ProfilePrefStoreManager::CreateProfilePrefStore() into prefs::CreateTrackedPersistentPrefStore(). - Changes prefs::CreatePersistentPrefStore() so an appropriate configuration results in prefs::CreateTrackedPersistentPrefStore() being called to create the backing PrefStore. BUG=654988 ========== to ========== Pref service: add support for tracked prefs. This: - Adds TrackedPersistentPrefStoreConfiguration: a mojo struct containing the necessary state for constructing a PrefStore identical to the one constructed in ProfilePrefStoreManager::CreateProfilePrefStore(). - Moves the enums used to configure tracked prefs into the mojom. - Extracts the creation of tracked pref store from ProfilePrefStoreManager::CreateProfilePrefStore() into prefs::CreateTrackedPersistentPrefStore(). - Changes prefs::CreatePersistentPrefStore() so an appropriate configuration results in prefs::CreateTrackedPersistentPrefStore() being called to create the backing PrefStore. BUG=654988 Review-Url: https://codereview.chromium.org/2745563005 Cr-Commit-Position: refs/heads/master@{#462301} Committed: https://chromium.googlesource.com/chromium/src/+/d2e2fed00150274e4a6857c700dc... ==========
Message was sent while issue was closed.
Committed patchset #19 (id:570001) as https://chromium.googlesource.com/chromium/src/+/d2e2fed00150274e4a6857c700dc...
Message was sent while issue was closed.
A revert of this CL (patchset #19 id:570001) has been created in https://codereview.chromium.org/2799043003/ by foolip@chromium.org. The reason for reverting is: ProfilePrefStoreManagerTest/ProfilePrefStoreManagerTest.UnprotectedToProtected/1 became flaky shortly after this landed. BUG=708901.
Message was sent while issue was closed.
Description was changed from ========== Pref service: add support for tracked prefs. This: - Adds TrackedPersistentPrefStoreConfiguration: a mojo struct containing the necessary state for constructing a PrefStore identical to the one constructed in ProfilePrefStoreManager::CreateProfilePrefStore(). - Moves the enums used to configure tracked prefs into the mojom. - Extracts the creation of tracked pref store from ProfilePrefStoreManager::CreateProfilePrefStore() into prefs::CreateTrackedPersistentPrefStore(). - Changes prefs::CreatePersistentPrefStore() so an appropriate configuration results in prefs::CreateTrackedPersistentPrefStore() being called to create the backing PrefStore. BUG=654988 Review-Url: https://codereview.chromium.org/2745563005 Cr-Commit-Position: refs/heads/master@{#462301} Committed: https://chromium.googlesource.com/chromium/src/+/d2e2fed00150274e4a6857c700dc... ========== to ========== Pref service: add support for tracked prefs. This: - Adds TrackedPersistentPrefStoreConfiguration: a mojo struct containing the necessary state for constructing a PrefStore identical to the one constructed in ProfilePrefStoreManager::CreateProfilePrefStore(). - Moves the enums used to configure tracked prefs into the mojom. - Extracts the creation of tracked pref store from ProfilePrefStoreManager::CreateProfilePrefStore() into prefs::CreateTrackedPersistentPrefStore(). - Changes prefs::CreatePersistentPrefStore() so an appropriate configuration results in prefs::CreateTrackedPersistentPrefStore() being called to create the backing PrefStore. BUG=654988 Review-Url: https://codereview.chromium.org/2745563005 Cr-Commit-Position: refs/heads/master@{#462301} Committed: https://chromium.googlesource.com/chromium/src/+/d2e2fed00150274e4a6857c700dc... ==========
Patchset #20 (id:590001) has been deleted
The CQ bit was checked by sammc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, gab@chromium.org, mlerman@chromium.org, mbarbella@chromium.org, tibell@chromium.org Link to the patchset: https://codereview.chromium.org/2745563005/#ps630001 (title: "deflake tests")
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": 630001, "attempt_start_ts": 1491464189584970,
"parent_rev": "13f391f98453cf27e1f355c27982969a9745eb1b", "commit_rev":
"cbfdf1af972416d91212925a613fb169a31a157a"}
Message was sent while issue was closed.
Description was changed from ========== Pref service: add support for tracked prefs. This: - Adds TrackedPersistentPrefStoreConfiguration: a mojo struct containing the necessary state for constructing a PrefStore identical to the one constructed in ProfilePrefStoreManager::CreateProfilePrefStore(). - Moves the enums used to configure tracked prefs into the mojom. - Extracts the creation of tracked pref store from ProfilePrefStoreManager::CreateProfilePrefStore() into prefs::CreateTrackedPersistentPrefStore(). - Changes prefs::CreatePersistentPrefStore() so an appropriate configuration results in prefs::CreateTrackedPersistentPrefStore() being called to create the backing PrefStore. BUG=654988 Review-Url: https://codereview.chromium.org/2745563005 Cr-Commit-Position: refs/heads/master@{#462301} Committed: https://chromium.googlesource.com/chromium/src/+/d2e2fed00150274e4a6857c700dc... ========== to ========== Pref service: add support for tracked prefs. This: - Adds TrackedPersistentPrefStoreConfiguration: a mojo struct containing the necessary state for constructing a PrefStore identical to the one constructed in ProfilePrefStoreManager::CreateProfilePrefStore(). - Moves the enums used to configure tracked prefs into the mojom. - Extracts the creation of tracked pref store from ProfilePrefStoreManager::CreateProfilePrefStore() into prefs::CreateTrackedPersistentPrefStore(). - Changes prefs::CreatePersistentPrefStore() so an appropriate configuration results in prefs::CreateTrackedPersistentPrefStore() being called to create the backing PrefStore. BUG=654988 Review-Url: https://codereview.chromium.org/2745563005 Cr-Original-Commit-Position: refs/heads/master@{#462301} Committed: https://chromium.googlesource.com/chromium/src/+/d2e2fed00150274e4a6857c700dc... Review-Url: https://codereview.chromium.org/2745563005 Cr-Commit-Position: refs/heads/master@{#462395} Committed: https://chromium.googlesource.com/chromium/src/+/cbfdf1af972416d91212925a613f... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:630001) as https://chromium.googlesource.com/chromium/src/+/cbfdf1af972416d91212925a613f...
Message was sent while issue was closed.
https://codereview.chromium.org/2745563005/diff/630001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right): https://codereview.chromium.org/2745563005/diff/630001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:283: RelaunchPrefService(); Add a comment here and below about why relaunching on init and destroy is necessary? i.e. to hint at whoever will remove JsonPrefStore's usage of SequencedWorkerPool at whether they can remove this hack? |
