Chromium Code Reviews
Help | Chromium Project | Sign in
(354)

Issue 2745563005: Pref service: add support for tracked prefs.

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 6 days ago by Sam McNally
Modified:
1 day, 19 hours 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.

Description

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

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: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+315 lines, -118 lines) Patch
M chrome/browser/prefs/chrome_pref_service_factory.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/prefs/profile_pref_store_manager.h View 1 2 3 4 5 6 7 4 chunks +6 lines, -11 lines 0 comments Download
M chrome/browser/prefs/profile_pref_store_manager.cc View 1 2 3 4 5 6 7 8 chunks +43 lines, -84 lines 1 comment Download
M chrome/browser/prefs/profile_pref_store_manager_unittest.cc View 1 2 3 4 5 2 chunks +3 lines, -4 lines 0 comments Download
M components/user_prefs/tracked/BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M components/user_prefs/tracked/DEPS View 1 chunk +2 lines, -1 line 0 comments Download
M components/user_prefs/tracked/pref_hash_filter.h View 1 2 3 2 chunks +6 lines, -16 lines 0 comments Download
M services/preferences/BUILD.gn View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M services/preferences/DEPS View 1 chunk +1 line, -0 lines 0 comments Download
M services/preferences/persistent_pref_store_factory.cc View 1 2 3 4 5 6 7 3 chunks +40 lines, -0 lines 0 comments Download
M services/preferences/public/cpp/BUILD.gn View 1 2 2 chunks +6 lines, -1 line 0 comments Download
A services/preferences/public/cpp/tracked_persistent_pref_store_factory.h View 1 2 3 4 5 6 7 1 chunk +26 lines, -0 lines 0 comments Download
A services/preferences/public/cpp/tracked_persistent_pref_store_factory.cc View 1 2 3 4 5 6 7 1 chunk +129 lines, -0 lines 0 comments Download
M services/preferences/public/interfaces/preferences_configuration.mojom View 1 1 chunk +46 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 60 (45 generated)
Sam McNally
1 week, 6 days ago (2017-03-10 03:44:43 UTC) #5
tibell
https://codereview.chromium.org/2745563005/diff/20001/services/preferences/persistent_pref_store_factory.cc File services/preferences/persistent_pref_store_factory.cc (right): https://codereview.chromium.org/2745563005/diff/20001/services/preferences/persistent_pref_store_factory.cc#newcode78 services/preferences/persistent_pref_store_factory.cc:78: std::unique_ptr<PersistentPrefStoreImpl> CreateTrackedPersistentPrefStore( Does this method have to be kept ...
1 week, 6 days ago (2017-03-10 04:29:01 UTC) #6
Sam McNally
https://codereview.chromium.org/2745563005/diff/20001/services/preferences/persistent_pref_store_factory.cc File services/preferences/persistent_pref_store_factory.cc (right): https://codereview.chromium.org/2745563005/diff/20001/services/preferences/persistent_pref_store_factory.cc#newcode78 services/preferences/persistent_pref_store_factory.cc:78: std::unique_ptr<PersistentPrefStoreImpl> CreateTrackedPersistentPrefStore( On 2017/03/10 04:29:01, tibell wrote: > Does ...
1 week, 6 days ago (2017-03-10 05:04:44 UTC) #12
tibell
lgtm
1 week, 6 days ago (2017-03-10 05:08:11 UTC) #15
Sam McNally
+bauerb for //chrome/browser/prefs, //components/user_prefs/tracked and DEPS changes +mbarbella for the mojom
1 week, 3 days ago (2017-03-13 03:08:23 UTC) #23
Bernhard (OOO until Mar 27)
LGTM +Gab FYI
1 week, 3 days ago (2017-03-13 14:41:37 UTC) #26
gab
lg but a few things first (sorry, late to party, was OOO) https://codereview.chromium.org/2745563005/diff/60001/chrome/browser/prefs/profile_pref_store_manager.cc File chrome/browser/prefs/profile_pref_store_manager.cc ...
1 week, 2 days ago (2017-03-13 16:52:02 UTC) #28
Sam McNally
https://codereview.chromium.org/2745563005/diff/60001/chrome/browser/prefs/profile_pref_store_manager.cc File chrome/browser/prefs/profile_pref_store_manager.cc (left): https://codereview.chromium.org/2745563005/diff/60001/chrome/browser/prefs/profile_pref_store_manager.cc#oldcode138 chrome/browser/prefs/profile_pref_store_manager.cc:138: GetPrefHashStore(true), GetExternalVerificationPrefHashStorePair(), On 2017/03/13 16:52:02, gab (OOO until Mar ...
1 week, 2 days ago (2017-03-14 02:01:35 UTC) #37
Martin Barbella
mojom lgtm
1 week, 1 day ago (2017-03-14 23:30:37 UTC) #41
gab
Meta-comment: I think the mojo service should be ProfilePrefStoreManager. It's already the component responsible for ...
1 week, 1 day ago (2017-03-15 15:09:58 UTC) #42
Sam McNally
On 2017/03/15 15:09:58, gab wrote: > Meta-comment: I think the mojo service should be ProfilePrefStoreManager. ...
1 week ago (2017-03-15 23:11:55 UTC) #43
Sam McNally
https://codereview.chromium.org/2745563005/diff/140002/chrome/browser/prefs/profile_pref_store_manager.cc File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2745563005/diff/140002/chrome/browser/prefs/profile_pref_store_manager.cc#newcode67 chrome/browser/prefs/profile_pref_store_manager.cc:67: local_state_(local_state) {} On 2017/03/15 15:09:58, gab wrote: > Also ...
1 week ago (2017-03-16 07:13:48 UTC) #53
gab
Sorry for being slow and blocking here but I just don't see the benefit added ...
2 days, 23 hours ago (2017-03-20 16:36:26 UTC) #54
Sam McNally
On 2017/03/20 16:36:26, gab wrote: > Sorry for being slow and blocking here but I ...
2 days, 16 hours ago (2017-03-20 23:45:41 UTC) #57
gab
1 day, 19 hours ago (2017-03-21 21:19:22 UTC) #60
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld d1a128a62