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

Issue 2745563005: Pref service: add support for tracked prefs. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 2 weeks ago by Sam McNally
Modified:
2 weeks, 3 days 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 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+427 lines, -282 lines) Patch
M chrome/browser/prefs/chrome_pref_service_factory.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +2 lines, -9 lines 0 comments Download
M chrome/browser/prefs/chrome_pref_service_factory.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 6 chunks +37 lines, -18 lines 0 comments Download
M chrome/browser/prefs/profile_pref_store_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 4 chunks +36 lines, -51 lines 0 comments Download
M chrome/browser/prefs/profile_pref_store_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 7 chunks +64 lines, -123 lines 0 comments Download
M chrome/browser/prefs/profile_pref_store_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 17 chunks +51 lines, -44 lines 1 comment Download
M chrome/browser/profiles/profile_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +0 lines, -11 lines 0 comments Download
M chrome/browser/profiles/profile_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +8 lines, -2 lines 0 comments Download
M services/preferences/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 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 8 9 10 11 12 13 3 chunks +8 lines, -1 line 0 comments Download
M services/preferences/persistent_pref_store_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +0 lines, -2 lines 0 comments Download
M services/preferences/persistent_pref_store_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -3 lines 0 comments Download
M services/preferences/persistent_pref_store_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
M services/preferences/public/interfaces/preferences_configuration.mojom View 1 2 3 4 5 6 7 8 9 10 2 chunks +22 lines, -0 lines 0 comments Download
M services/preferences/tracked/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -0 lines 0 comments Download
M services/preferences/tracked/pref_hash_filter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +5 lines, -5 lines 0 comments Download
M services/preferences/tracked/pref_hash_filter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +5 lines, -5 lines 0 comments Download
M services/preferences/tracked/pref_hash_filter_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 4 chunks +11 lines, -3 lines 0 comments Download
M services/preferences/tracked/segregated_pref_store.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +8 lines, -1 line 0 comments Download
M services/preferences/tracked/segregated_pref_store.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +4 lines, -2 lines 0 comments Download
M services/preferences/tracked/segregated_pref_store_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
A services/preferences/tracked/tracked_persistent_pref_store_factory.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +27 lines, -0 lines 0 comments Download
A services/preferences/tracked/tracked_persistent_pref_store_factory.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +133 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 137 (102 generated)
Sam McNally
1 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks ago (2017-03-10 05:04:44 UTC) #12
tibell
lgtm
1 month, 2 weeks 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 month, 1 week ago (2017-03-13 03:08:23 UTC) #23
Bernhard Bauer
LGTM +Gab FYI
1 month, 1 week 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 month, 1 week 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 month, 1 week ago (2017-03-14 02:01:35 UTC) #37
Martin Barbella
mojom lgtm
1 month, 1 week 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 month, 1 week 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 month, 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 month, 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 ...
1 month 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 ...
1 month ago (2017-03-20 23:45:41 UTC) #57
gab
On 2017/03/20 23:45:41, Sam McNally wrote: > On 2017/03/20 16:36:26, gab wrote: > > Sorry ...
1 month ago (2017-03-21 21:19:22 UTC) #60
tibell
Hi Gabriel! Since the pref service is otherwise ready to go (modulo some performance benchmarks) ...
3 weeks, 6 days ago (2017-03-28 02:31:42 UTC) #61
gab
To me separate GN targets = separate components = separate layers. This CL adds components/user_prefs ...
3 weeks, 5 days ago (2017-03-28 16:42:08 UTC) #62
Sam McNally
I've rebased this onto the move CL and moved all the tracked prefs implementation-touching parts ...
3 weeks, 3 days ago (2017-03-30 09:06:06 UTC) #86
gab
Thanks, I like this structure much better, Bernhard (or another mojo familiar person) can you ...
3 weeks, 3 days ago (2017-03-30 15:28:41 UTC) #89
Bernhard Bauer
I have to admit, I don't know enough about Mojo either to give detailed feedback. ...
3 weeks, 3 days ago (2017-03-30 20:11:57 UTC) #90
Sam McNally
On 2017/03/30 15:28:41, gab wrote: > Thanks, I like this structure much better, Bernhard (or ...
3 weeks, 3 days ago (2017-03-30 23:24:08 UTC) #95
tibell
lgtm Took another look at the Mojo setup. Still lgtm.
3 weeks, 3 days ago (2017-03-31 03:48:13 UTC) #98
gab
Just did another full pass, this is looking really nice now :), just a few ...
2 weeks, 6 days ago (2017-04-03 16:11:18 UTC) #107
Sam McNally
https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/chrome_pref_service_factory.cc File chrome/browser/prefs/chrome_pref_service_factory.cc (right): https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/chrome_pref_service_factory.cc#newcode467 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 ...
2 weeks, 6 days ago (2017-04-04 03:24:49 UTC) #113
gab
LGTM, woohoo! Thanks!! https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/chrome_pref_service_factory.cc File chrome/browser/prefs/chrome_pref_service_factory.cc (right): https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/chrome_pref_service_factory.cc#newcode467 chrome/browser/prefs/chrome_pref_service_factory.cc:467: mojo::MakeRequest(&reset_on_load_observer)); On 2017/04/04 03:24:49, Sam McNally ...
2 weeks, 5 days ago (2017-04-04 20:59:51 UTC) #114
Sam McNally
+mlerman for //chrome/browser/profiles https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/chrome_pref_service_factory.cc File chrome/browser/prefs/chrome_pref_service_factory.cc (right): https://codereview.chromium.org/2745563005/diff/530001/chrome/browser/prefs/chrome_pref_service_factory.cc#newcode467 chrome/browser/prefs/chrome_pref_service_factory.cc:467: mojo::MakeRequest(&reset_on_load_observer)); On 2017/04/04 20:59:51, gab wrote: ...
2 weeks, 5 days ago (2017-04-05 00:44:31 UTC) #116
Mike Lerman
profiles LGTM
2 weeks, 4 days ago (2017-04-05 13:57:55 UTC) #117
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2745563005/570001
2 weeks, 4 days ago (2017-04-05 22:43:23 UTC) #120
commit-bot: I haz the power
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_compile_dbg_ng/builds/389629)
2 weeks, 4 days ago (2017-04-05 22:49:32 UTC) #122
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2745563005/570001
2 weeks, 4 days ago (2017-04-05 22:52:02 UTC) #124
commit-bot: I haz the power
Committed patchset #19 (id:570001) as https://chromium.googlesource.com/chromium/src/+/d2e2fed00150274e4a6857c700dcb24f99793bd7
2 weeks, 4 days ago (2017-04-06 00:52:35 UTC) #127
foolip_UTC7
A revert of this CL (patchset #19 id:570001) has been created in https://codereview.chromium.org/2799043003/ by foolip@chromium.org. ...
2 weeks, 4 days ago (2017-04-06 06:26:25 UTC) #128
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2745563005/630001
2 weeks, 3 days ago (2017-04-06 07:36:52 UTC) #133
commit-bot: I haz the power
Committed patchset #21 (id:630001) as https://chromium.googlesource.com/chromium/src/+/cbfdf1af972416d91212925a613fb169a31a157a
2 weeks, 3 days ago (2017-04-06 08:39:55 UTC) #136
gab
2 weeks, 3 days ago (2017-04-06 16:00:07 UTC) #137
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?
Sign in to reply to this message.

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