Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(137)

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

Created:
1 year, 1 month ago by Sam McNally
Modified:
1 year 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

Dependent Patchsets:

Messages

Total messages: 137 (102 generated)
Sam McNally
1 year, 1 month 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 year, 1 month 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 year, 1 month ago (2017-03-10 05:04:44 UTC) #12
tibell
lgtm
1 year, 1 month 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 year, 1 month ago (2017-03-13 03:08:23 UTC) #23
Bernhard Bauer
LGTM +Gab FYI
1 year, 1 month 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 year, 1 month 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 year, 1 month ago (2017-03-14 02:01:35 UTC) #37
Martin Barbella
mojom lgtm
1 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 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 year, 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 year 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) ...
1 year 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 ...
1 year 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 ...
1 year 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 ...
1 year 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. ...
1 year 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 ...
1 year ago (2017-03-30 23:24:08 UTC) #95
tibell
lgtm Took another look at the Mojo setup. Still lgtm.
1 year ago (2017-03-31 03:48:13 UTC) #98
gab
Just did another full pass, this is looking really nice now :), just a few ...
1 year 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 ...
1 year 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 ...
1 year 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: ...
1 year ago (2017-04-05 00:44:31 UTC) #116
Mike Lerman
profiles LGTM
1 year 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
1 year 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)
1 year 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
1 year 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
1 year ago (2017-04-06 00:52:35 UTC) #127
foolip
A revert of this CL (patchset #19 id:570001) has been created in https://codereview.chromium.org/2799043003/ by foolip@chromium.org. ...
1 year 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
1 year 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
1 year ago (2017-04-06 08:39:55 UTC) #136
gab
1 year 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?

Powered by Google App Engine
This is Rietveld 408576698