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

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

Created:
1 year, 3 months ago by Sam McNally
Modified:
1 year, 2 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.

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, 3 months 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, 3 months 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, 3 months ago (2017-03-10 05:04:44 UTC) #12
tibell
lgtm
1 year, 3 months 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, 3 months ago (2017-03-13 03:08:23 UTC) #23
Bernhard Bauer
LGTM +Gab FYI
1 year, 3 months 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, 3 months 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, 3 months ago (2017-03-14 02:01:35 UTC) #37
Martin Barbella
mojom lgtm
1 year, 3 months 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, 3 months 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, 3 months 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, 3 months 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, 3 months 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, 3 months 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, 3 months 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, 2 months 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, 2 months 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, 2 months 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, 2 months 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, 2 months 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, 2 months ago (2017-03-30 23:24:08 UTC) #95
tibell
lgtm Took another look at the Mojo setup. Still lgtm.
1 year, 2 months 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, 2 months 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, 2 months 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, 2 months 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, 2 months ago (2017-04-05 00:44:31 UTC) #116
Mike Lerman
profiles LGTM
1 year, 2 months 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, 2 months 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, 2 months 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, 2 months 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, 2 months 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, 2 months 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, 2 months 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, 2 months ago (2017-04-06 08:39:55 UTC) #136
gab
1 year, 2 months 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