Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(74)

Issue 2746023002: Pref service: enable for user prefs in chrome behind a flag. (Closed)

Created:
3 years, 9 months ago by Sam McNally
Modified:
3 years, 8 months ago
CC:
Aaron Boodman, abarth-chromium, chrome-apps-syd-reviews_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, qsr+mojo_chromium.org, rginda+watch_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Pref service: enable for user prefs in chrome behind a flag. This: - changes ProfilePrefStoreManager to, when the PrefService feature is enabled, connect to the pref service, send it the user prefs configuration and create a PrefStore that is backed by the pref service - changes PrefHashFilter to discard its on_reset_on_load callback after the point where it may call it; when running in the perf service, the callback owns a mojo connection that can be torn down at that point - changes BrowserProcessImpl to wait for a task to run on the IO thread before waiting for blocking IO to complete for each profile; the pref service runs on the IO thread so waiting for a task to run there is necessary - fixes a bug in PrefServiceSyncableFactory where enabling the pref service would crash if any non-user pref stores are null BUG=654988 Review-Url: https://codereview.chromium.org/2746023002 Cr-Commit-Position: refs/heads/master@{#460273} Committed: https://chromium.googlesource.com/chromium/src/+/2a1451c0b54e230d212035525fc6b373abea7f5b

Patch Set 1 : On for testing #

Patch Set 2 : #

Total comments: 8

Patch Set 3 : rebase #

Patch Set 4 : #

Patch Set 5 : rebase off https://codereview.chromium.org/2745563005/ #

Patch Set 6 : rebase #

Patch Set 7 : #

Patch Set 8 : #

Total comments: 2

Patch Set 9 : rebase #

Patch Set 10 : #

Patch Set 11 : rebase #

Patch Set 12 : no, the other pref service #

Unified diffs Side-by-side diffs Delta from patch set Stats (+250 lines, -45 lines) Patch
M chrome/browser/browser_process_impl.cc View 1 2 3 4 5 6 7 8 9 10 6 chunks +31 lines, -9 lines 0 comments Download
M chrome/browser/chrome_content_browser_manifest_overlay.json View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/prefs/chrome_pref_service_factory.cc View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/browser/prefs/profile_pref_store_manager.h View 1 2 3 4 5 6 7 8 4 chunks +12 lines, -1 line 0 comments Download
M chrome/browser/prefs/profile_pref_store_manager.cc View 1 2 3 4 5 6 7 8 4 chunks +27 lines, -2 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 21 chunks +158 lines, -27 lines 0 comments Download
M chrome/browser/profiles/profile_browsertest.cc View 2 chunks +6 lines, -0 lines 0 comments Download
M components/sync_preferences/pref_service_syncable_factory.cc View 1 chunk +1 line, -1 line 0 comments Download
M components/user_prefs/tracked/pref_hash_filter.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M components/user_prefs/tracked/pref_hash_filter.cc View 1 chunk +1 line, -0 lines 0 comments Download
M mojo/public/cpp/bindings/sync_call_restrictions.h View 1 2 3 4 5 6 2 chunks +7 lines, -0 lines 0 comments Download
M services/preferences/public/cpp/persistent_pref_store_client.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -0 lines 0 comments Download

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 113 (96 generated)
Sam McNally
Please review the prefs changes.
3 years, 9 months ago (2017-03-14 11:10:47 UTC) #35
Sam McNally
+mlerman for //chrome/browser/profiles +sky for chrome/browser/browser_process_impl.cc +bauerb for //chrome/browser/prefs, //components/sync_preferences and //components/user_prefs/tracked +jam for //content ...
3 years, 9 months ago (2017-03-15 01:46:24 UTC) #40
sky
https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser_process_impl.cc File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser_process_impl.cc#newcode528 chrome/browser/browser_process_impl.cc:528: base::Time start = base::Time::Now(); Use timeticks? https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser_process_impl.cc#newcode529 chrome/browser/browser_process_impl.cc:529: if ...
3 years, 9 months ago (2017-03-15 15:19:17 UTC) #41
Mike Lerman
On 2017/03/15 15:19:17, sky wrote: > https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser_process_impl.cc > File chrome/browser/browser_process_impl.cc (right): > > https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser_process_impl.cc#newcode528 > ...
3 years, 9 months ago (2017-03-15 16:59:31 UTC) #43
Sam McNally
https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser_process_impl.cc File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser_process_impl.cc#newcode528 chrome/browser/browser_process_impl.cc:528: base::Time start = base::Time::Now(); On 2017/03/15 15:19:17, sky wrote: ...
3 years, 9 months ago (2017-03-16 06:58:22 UTC) #52
Bernhard Bauer
prefs LGTM
3 years, 9 months ago (2017-03-17 16:44:56 UTC) #55
Martin Barbella
json lgtm
3 years, 9 months ago (2017-03-17 17:28:29 UTC) #56
Sam McNally
jam, anthonyvd, sky: ping
3 years, 9 months ago (2017-03-21 23:15:53 UTC) #59
jam
Sorry for the delay, I missed this cl. In the future, please IM me if ...
3 years, 9 months ago (2017-03-22 00:03:48 UTC) #62
Sam McNally
On 2017/03/22 00:03:48, jam wrote: > Sorry for the delay, I missed this cl. In ...
3 years, 9 months ago (2017-03-22 05:51:16 UTC) #69
jam
lgtm https://codereview.chromium.org/2746023002/diff/320001/services/preferences/public/cpp/persistent_pref_store_client.cc File services/preferences/public/cpp/persistent_pref_store_client.cc (right): https://codereview.chromium.org/2746023002/diff/320001/services/preferences/public/cpp/persistent_pref_store_client.cc#newcode75 services/preferences/public/cpp/persistent_pref_store_client.cc:75: base::ThreadRestrictions::AssertWaitAllowed(); nit: curious why you added this? mojo::SyncCallRestrictions ...
3 years, 9 months ago (2017-03-22 14:58:13 UTC) #72
sky
LGTM
3 years, 9 months ago (2017-03-22 16:27:02 UTC) #73
anthonyvd
Sorry about that, c/b/profiles lgtm
3 years, 9 months ago (2017-03-24 16:23:34 UTC) #74
Sam McNally
https://codereview.chromium.org/2746023002/diff/320001/services/preferences/public/cpp/persistent_pref_store_client.cc File services/preferences/public/cpp/persistent_pref_store_client.cc (right): https://codereview.chromium.org/2746023002/diff/320001/services/preferences/public/cpp/persistent_pref_store_client.cc#newcode75 services/preferences/public/cpp/persistent_pref_store_client.cc:75: base::ThreadRestrictions::AssertWaitAllowed(); On 2017/03/22 14:58:12, jam wrote: > nit: curious ...
3 years, 8 months ago (2017-03-28 23:06:22 UTC) #93
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/2746023002/460001
3 years, 8 months ago (2017-03-29 03:44:35 UTC) #109
commit-bot: I haz the power
Committed patchset #12 (id:460001) as https://chromium.googlesource.com/chromium/src/+/2a1451c0b54e230d212035525fc6b373abea7f5b
3 years, 8 months ago (2017-03-29 04:15:01 UTC) #112
chanli1
3 years, 8 months ago (2017-03-29 17:08:35 UTC) #113
Message was sent while issue was closed.
On 2017/03/29 04:15:01, commit-bot: I haz the power wrote:
> Committed patchset #12 (id:460001) as
>
https://chromium.googlesource.com/chromium/src/+/2a1451c0b54e230d212035525fc6...

Hello,

Could you help me confirm whether or not this CL could cause
ProfilePrefStoreManagerTest/ProfilePrefStoreManagerTest.UnprotectedToProtected/0
to be flaky?

For more information:
https://findit-for-me.appspot.com/waterfall/check-flake?key=ag9zfmZpbmRpdC1mb...

Thank you!

Powered by Google App Engine
This is Rietveld 408576698