Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2092453002:
Mojom interface for Preferences (Closed)
|
Created:
4 years, 6 months ago by jonross Modified:
4 years, 1 month 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), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMojom interface for Preferences
This change creates a mojom interface for Preferences. This will allow chrome/browser to continue to manager preferences. While allowing clients in other processes to access them.
PrefObserverStore has been created to abstract away the interface from clients. Instead this can be used by clients as a normal PrefStore, with synchronous access to preferences.
TEST=PrefObserverStoreTest, also manual testing with shelf preferences (not in this review)
BUG=622347
Committed: https://crrev.com/63b021f2555ac1094961fc6f82de77b7ac31b312
Cr-Commit-Position: refs/heads/master@{#429166}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Rebase #
Total comments: 4
Patch Set 3 : Move to services #
Total comments: 4
Patch Set 4 : Sample #Patch Set 5 : Rebase #Patch Set 6 : Remove unneeded file #Patch Set 7 : Remove example #Patch Set 8 : Address Review Comments #
Total comments: 20
Patch Set 9 : Rebase #Patch Set 10 : Use new wrapper types #
Total comments: 3
Patch Set 11 : Rebase #
Total comments: 4
Patch Set 12 : Remove component export #Patch Set 13 : Rebase #Patch Set 14 : Switch to using DictionaryValue #
Total comments: 8
Patch Set 15 : Rebase #Patch Set 16 : Address Review Comments #
Total comments: 3
Patch Set 17 : Rebase #Patch Set 18 : Unify Prefs naming #Patch Set 19 : Expand Unittests #
Total comments: 3
Patch Set 20 : Update test to provide Ptr directly #
Total comments: 6
Patch Set 21 : Update ReportValueChanged #
Total comments: 2
Patch Set 22 : Const ref nit #Messages
Total messages: 65 (15 generated)
https://codereview.chromium.org/2092453002/diff/1/components/prefs/DEPS File components/prefs/DEPS (right): https://codereview.chromium.org/2092453002/diff/1/components/prefs/DEPS#newcode3 components/prefs/DEPS:3: "+services/shell/public/cpp", This is actually a new file, not sure why its showing a diff https://codereview.chromium.org/2092453002/diff/1/components/prefs/public/int... File components/prefs/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2092453002/diff/1/components/prefs/public/int... components/prefs/public/interfaces/BUILD.gn:9: "preferences.mojom", new file, not sure why it is showing a diff
jonross@chromium.org changed reviewers: + sadrul@chromium.org
Hey Sadrul, Could you take a look? This review includes the mojom for preferences. Along with an initial implementation of a PrefStore. Thanks, Jon
I think this should live in a separate place (e.g. //services/preferences/public/[cpp|interfaces]/) https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... File components/prefs/pref_observer_store.cc (right): https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... components/prefs/pref_observer_store.cc:35: return false; Maybe start with a DCHECK() https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... components/prefs/pref_observer_store.cc:36: Should this also check for initialized_? https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... components/prefs/pref_observer_store.cc:79: if (keys_.find(key) == keys_.end()) How do you add a new preference? https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... components/prefs/pref_observer_store.cc:80: return; DCHECK()? https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... File components/prefs/pref_observer_store.h (right): https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... components/prefs/pref_observer_store.h:40: void Init(const std::set<mojo::String>& keys); Can this be std::vector<std::string> instead?
https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... File components/prefs/pref_observer_store.cc (right): https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... components/prefs/pref_observer_store.cc:36: On 2016/06/23 17:30:17, sadrul wrote: > Should this also check for initialized_? Possibly. Depends on how we wish for clients to handle the non-init state during startup. If we expect them to not ask for values until receiving OnInitializedCompleted, then definitely. https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... components/prefs/pref_observer_store.cc:79: if (keys_.find(key) == keys_.end()) On 2016/06/23 17:30:17, sadrul wrote: > How do you add a new preference? First pass there is no method. I had planned to follow up by allowing the PrefStore to begin observing additional settings at runtime. Do we have cases where user actions generate brand new preferences? https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... File components/prefs/pref_observer_store.h (right): https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... components/prefs/pref_observer_store.h:40: void Init(const std::set<mojo::String>& keys); On 2016/06/23 17:30:17, sadrul wrote: > Can this be std::vector<std::string> instead? I had went with mojo::String as I was seeing this error with testing code I was working on: error: no viable conversion from 'const set<mojo::String>' to 'const set<std::string>' I know that I can force the conversion via: To<std::set<std::string>> however at the time I didn't see a disadvantage to just keeping mojo::String on both sides.
https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... File components/prefs/pref_observer_store.cc (right): https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... components/prefs/pref_observer_store.cc:36: On 2016/06/23 17:30:17, sadrul wrote: > Should this also check for initialized_? Possibly. Depends on how we wish for clients to handle the non-init state during startup. If we expect them to not ask for values until receiving OnInitializedCompleted, then definitely. https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... components/prefs/pref_observer_store.cc:79: if (keys_.find(key) == keys_.end()) On 2016/06/23 17:30:17, sadrul wrote: > How do you add a new preference? First pass there is no method. I had planned to follow up by allowing the PrefStore to begin observing additional settings at runtime. Do we have cases where user actions generate brand new preferences? https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... File components/prefs/pref_observer_store.h (right): https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... components/prefs/pref_observer_store.h:40: void Init(const std::set<mojo::String>& keys); On 2016/06/23 17:30:17, sadrul wrote: > Can this be std::vector<std::string> instead? I had went with mojo::String as I was seeing this error with testing code I was working on: error: no viable conversion from 'const set<mojo::String>' to 'const set<std::string>' I know that I can force the conversion via: To<std::set<std::string>> however at the time I didn't see a disadvantage to just keeping mojo::String on both sides.
Hey Sadrul, I've moved the api to services/preferences. Could you take a look?
Patchset #4 (id:60001) has been deleted
I've added a small sample of usage for demonstration purposes. I'll revert it before landing. Something similar, but complete, will land later. Reimplementing the shelf prefs. chrome/browser - handles being prefs::mojom::PreferencesManager ash/sysui - instantiates PrefObserverStore, which handles the mojo connection
On 2016/06/28 22:05:44, jonross wrote: > I've added a small sample of usage for demonstration purposes. I'll revert it > before landing. Something similar, but complete, will land later. Reimplementing > the shelf prefs. > > chrome/browser - handles being prefs::mojom::PreferencesManager > > ash/sysui - instantiates PrefObserverStore, which handles the mojo connection Hey Sadrul, could you PTAL?
You can remove the sample API usage from the CL. (and since sysui is actually going away, you would need to hook up //ash/mus to use the new pref service). Looks like all of the files are in //services/preferences/public/, which is meant to be the API for clients to use. So it's unclear what part of the code is going to run in the service itself. Can you clarify that? https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... File components/prefs/pref_observer_store.cc (right): https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... components/prefs/pref_observer_store.cc:36: On 2016/06/24 15:13:06, jonross wrote: > On 2016/06/23 17:30:17, sadrul wrote: > > Should this also check for initialized_? > > Possibly. Depends on how we wish for clients to handle the non-init state during > startup. > > If we expect them to not ask for values until receiving OnInitializedCompleted, > then definitely. When does it receive OnInitializedCompleted()? I think it's better to check for |initialized_| here. https://codereview.chromium.org/2092453002/diff/20001/components/prefs/pref_o... File components/prefs/pref_observer_store.cc (right): https://codereview.chromium.org/2092453002/diff/20001/components/prefs/pref_o... components/prefs/pref_observer_store.cc:35: return false; DCHECK()? https://codereview.chromium.org/2092453002/diff/20001/components/prefs/pref_o... components/prefs/pref_observer_store.cc:44: return; DCHECK() here too. It's difficult to debug when some request is silently ignored. DCHECK(keys_.find(key) != keys_.end()) << " Pref '" << key << "' was not included in Init()"; https://codereview.chromium.org/2092453002/diff/40001/services/preferences/BU... File services/preferences/BUILD.gn (right): https://codereview.chromium.org/2092453002/diff/40001/services/preferences/BU... services/preferences/BUILD.gn:8: "//services/preferences/public/cpp/tests:preferences_unittests", You shouldn't need the ':preferences_unittests' part. https://codereview.chromium.org/2092453002/diff/40001/services/preferences/pu... File services/preferences/public/cpp/tests/BUILD.gn (right): https://codereview.chromium.org/2092453002/diff/40001/services/preferences/pu... services/preferences/public/cpp/tests/BUILD.gn:15: test("preferences_unittests") { Make sure to add this in some bots so the tests are actually run.
On 2016/07/19 15:08:53, sadrul wrote: > You can remove the sample API usage from the CL. (and since sysui is actually > going away, you would need to hook up //ash/mus to use the new pref service). > > Looks like all of the files are in //services/preferences/public/, which is > meant to be the API for clients to use. So it's unclear what part of the code is > going to run in the service itself. Can you clarify that? The chrome process would run the PreferenceManager portion. Other processes desiring to receive data would run their own PreferenceObserver
https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... File components/prefs/pref_observer_store.cc (right): https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... components/prefs/pref_observer_store.cc:36: On 2016/07/19 15:08:53, sadrul wrote: > On 2016/06/24 15:13:06, jonross wrote: > > On 2016/06/23 17:30:17, sadrul wrote: > > > Should this also check for initialized_? > > > > Possibly. Depends on how we wish for clients to handle the non-init state > during > > startup. > > > > If we expect them to not ask for values until receiving > OnInitializedCompleted, > > then definitely. > > When does it receive OnInitializedCompleted()? > > I think it's better to check for |initialized_| here. OnInitializedCompleted() is called from OnPreferencesChanged(), the first time preferences are received from the PrefManager
https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... File components/prefs/pref_observer_store.cc (right): https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... components/prefs/pref_observer_store.cc:36: On 2016/07/19 15:08:53, sadrul wrote: > On 2016/06/24 15:13:06, jonross wrote: > > On 2016/06/23 17:30:17, sadrul wrote: > > > Should this also check for initialized_? > > > > Possibly. Depends on how we wish for clients to handle the non-init state > during > > startup. > > > > If we expect them to not ask for values until receiving > OnInitializedCompleted, > > then definitely. > > When does it receive OnInitializedCompleted()? > > I think it's better to check for |initialized_| here. OnInitializedCompleted() is called from OnPreferencesChanged(), the first time preferences are received from the PrefManager
https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... File components/prefs/pref_observer_store.cc (right): https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... components/prefs/pref_observer_store.cc:35: return false; On 2016/06/23 17:30:17, sadrul wrote: > Maybe start with a DCHECK() Done. https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... components/prefs/pref_observer_store.cc:36: On 2016/07/19 17:24:11, jonross wrote: > On 2016/07/19 15:08:53, sadrul wrote: > > On 2016/06/24 15:13:06, jonross wrote: > > > On 2016/06/23 17:30:17, sadrul wrote: > > > > Should this also check for initialized_? > > > > > > Possibly. Depends on how we wish for clients to handle the non-init state > > during > > > startup. > > > > > > If we expect them to not ask for values until receiving > > OnInitializedCompleted, > > > then definitely. > > > > When does it receive OnInitializedCompleted()? > > > > I think it's better to check for |initialized_| here. > > OnInitializedCompleted() is called from OnPreferencesChanged(), the first time > preferences are received from the PrefManager Done. https://codereview.chromium.org/2092453002/diff/1/components/prefs/pref_obser... components/prefs/pref_observer_store.cc:80: return; On 2016/06/23 17:30:17, sadrul wrote: > DCHECK()? Done. https://codereview.chromium.org/2092453002/diff/20001/components/prefs/pref_o... File components/prefs/pref_observer_store.cc (right): https://codereview.chromium.org/2092453002/diff/20001/components/prefs/pref_o... components/prefs/pref_observer_store.cc:35: return false; On 2016/07/19 15:08:53, sadrul wrote: > DCHECK()? Done. https://codereview.chromium.org/2092453002/diff/20001/components/prefs/pref_o... components/prefs/pref_observer_store.cc:44: return; On 2016/07/19 15:08:53, sadrul wrote: > DCHECK() here too. It's difficult to debug when some request is silently > ignored. > > DCHECK(keys_.find(key) != keys_.end()) << " Pref '" << key << "' was not > included in Init()"; Done. https://codereview.chromium.org/2092453002/diff/40001/services/preferences/BU... File services/preferences/BUILD.gn (right): https://codereview.chromium.org/2092453002/diff/40001/services/preferences/BU... services/preferences/BUILD.gn:8: "//services/preferences/public/cpp/tests:preferences_unittests", On 2016/07/19 15:08:53, sadrul wrote: > You shouldn't need the ':preferences_unittests' part. Done. https://codereview.chromium.org/2092453002/diff/40001/services/preferences/pu... File services/preferences/public/cpp/tests/BUILD.gn (right): https://codereview.chromium.org/2092453002/diff/40001/services/preferences/pu... services/preferences/public/cpp/tests/BUILD.gn:15: test("preferences_unittests") { On 2016/07/19 15:08:53, sadrul wrote: > Make sure to add this in some bots so the tests are actually run. Do we have a prefered set of bots for mus work?
https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.cc (right): https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:14: connector->ConnectToInterface("exe:chrome", &prefs_manager_ptr_); Send the |prefs_manager_ptr_| from the caller, instead of from here. (see https://codereview.chromium.org/1992443002/#msg24) https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:21: keys_ = keys; DCHECK(!initialized_) https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:56: // can appropriately set the value on the PreferenceManger. Looks like the callers are expected to call ReportValueChanged when they change some value in the returned base::Value, so we should be good (i.e. we don't need to introduce anything that tracks whether base::Value changed)? https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:80: // TODO(jonross): replace with struct traits for base::Value Do we need to check |initialized_| here, and queue the updates until we are initialized? https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:86: value->GetAsInteger(&val); Check return value https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:92: value->GetAsString(&str); ditto https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:111: base::Value* value; value = nullptr; https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... File services/preferences/public/cpp/tests/run_all_unittests.cc (right): https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/cpp/tests/run_all_unittests.cc:15: base::Bind(&base::TestSuite::Run, base::Unretained(&test_suite))); Instead of adding this file, can you add a dependency on //mojo/edk/test:run_all_unittests instead? https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... File services/preferences/public/interfaces/preferences.mojom (right): https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/interfaces/preferences.mojom:21: }; Do you need this if you use use_new_wrapper_types? https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/interfaces/preferences.mojom:26: }; ditto
https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.cc (right): https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:14: connector->ConnectToInterface("exe:chrome", &prefs_manager_ptr_); On 2016/07/25 15:32:18, sadrul wrote: > Send the |prefs_manager_ptr_| from the caller, instead of from here. (see > https://codereview.chromium.org/1992443002/#msg24) Done. https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:21: keys_ = keys; On 2016/07/25 15:32:18, sadrul wrote: > DCHECK(!initialized_) Done. https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:56: // can appropriately set the value on the PreferenceManger. On 2016/07/25 15:32:17, sadrul wrote: > Looks like the callers are expected to call ReportValueChanged when they change > some value in the returned base::Value, so we should be good (i.e. we don't need > to introduce anything that tracks whether base::Value changed)? Yeah that seems to be true. I'll update this to forward on the mutable value with that expectation. https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:80: // TODO(jonross): replace with struct traits for base::Value On 2016/07/25 15:32:18, sadrul wrote: > Do we need to check |initialized_| here, and queue the updates until we are > initialized? This is a possible way to handle it. Another is to send the signals to the PrefsManager, and allow it to accept/reject setting changes as desired. We could also further look to the WTClient model, where the client tracks all inflight changes in its local state, reverts on failures. https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:86: value->GetAsInteger(&val); On 2016/07/25 15:32:18, sadrul wrote: > Check return value Done. https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:92: value->GetAsString(&str); On 2016/07/25 15:32:17, sadrul wrote: > ditto Done. https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:111: base::Value* value; On 2016/07/25 15:32:17, sadrul wrote: > value = nullptr; Done. https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... File services/preferences/public/cpp/tests/run_all_unittests.cc (right): https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/cpp/tests/run_all_unittests.cc:15: base::Bind(&base::TestSuite::Run, base::Unretained(&test_suite))); On 2016/07/25 15:32:18, sadrul wrote: > Instead of adding this file, can you add a dependency on > //mojo/edk/test:run_all_unittests instead? Done. https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... File services/preferences/public/interfaces/preferences.mojom (right): https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/interfaces/preferences.mojom:21: }; On 2016/07/25 15:32:18, sadrul wrote: > Do you need this if you use use_new_wrapper_types? Done. https://codereview.chromium.org/2092453002/diff/150001/services/preferences/p... services/preferences/public/interfaces/preferences.mojom:26: }; On 2016/07/25 15:32:18, sadrul wrote: > ditto Done.
jonross@chromium.org changed reviewers: + ben@chromium.org, erg@chromium.org
Adding in ben@ and erg@ for their thoughts. This change is to introduce a preference mojom to communicate preferences between the chrome process, and client processes. Patch 6 shows a short demonstration of usage. I plan to follow this change with a full implementation of PreferenceManager within chrome/browser. Once ash/sysui is removed I will replace the current shelf preferences using this, as the first client impl. PTAL, thanks.
https://codereview.chromium.org/2092453002/diff/190001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.h (right): https://codereview.chromium.org/2092453002/diff/190001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.h:30: // Currently this does not support RemoveValue or GetMutableValue. In the case of GetMutableValue, there are few enough callers to GetMutableValue (about 20) that converting those to not use GetMutableValue might be easier than implementing it. https://codereview.chromium.org/2092453002/diff/190001/services/preferences/p... File services/preferences/public/interfaces/preferences.mojom (right): https://codereview.chromium.org/2092453002/diff/190001/services/preferences/p... services/preferences/public/interfaces/preferences.mojom:16: }; If you're only supporting integers and strings here, how do you support maps/arrays? If you aren't supporting maps/arrays, how are you dealing with nested preferences, which are the vast majority of the preferences? (Amazingly, none of the prefs read or written in c/b/u/ash/chrome_launcher_prefs.cc are written like this. This interface is probably Good Enough for ash, though it won't be good enough for most anything else.) Keys in the preference system, of the form <one>.<two>.<three> actually map to: { "one": { "two": { "three": 0, } } } Asking the preference system for one.two.three returns 0, one.two returns { "three": 0 } and so on. Setting and returning dictionaries that contain multiple keys (or entire subtrees!) is used all over the place; I just plugged a few random preferences form my Preferences file into the code to check. ("default_search_provider" sets whole hierarchies to the top level preference, "extensions.chrome_url_overrides" isn't a full path, but one which gets set with a complex value.) I could see this pattern continuing in a mojoified world: observe "default_search_provider" to receive that entire preference tree, with child preferences.
https://codereview.chromium.org/2092453002/diff/190001/services/preferences/p... File services/preferences/public/interfaces/preferences.mojom (right): https://codereview.chromium.org/2092453002/diff/190001/services/preferences/p... services/preferences/public/interfaces/preferences.mojom:16: }; On 2016/07/26 21:11:58, Elliot Glaysher wrote: > If you're only supporting integers and strings here, how do you support > maps/arrays? If you aren't supporting maps/arrays, how are you dealing with > nested preferences, which are the vast majority of the preferences? > > (Amazingly, none of the prefs read or written in > c/b/u/ash/chrome_launcher_prefs.cc are written like this. This interface is > probably Good Enough for ash, though it won't be good enough for most anything > else.) > > Keys in the preference system, of the form <one>.<two>.<three> actually map to: > > { > "one": { > "two": { > "three": 0, > } > } > } > > Asking the preference system for one.two.three returns 0, one.two returns { > "three": 0 } and so on. > > Setting and returning dictionaries that contain multiple keys (or entire > subtrees!) is used all over the place; I just plugged a few random preferences > form my Preferences file into the code to check. ("default_search_provider" sets > whole hierarchies to the top level preference, "extensions.chrome_url_overrides" > isn't a full path, but one which gets set with a complex value.) I could see > this pattern continuing in a mojoified world: observe "default_search_provider" > to receive that entire preference tree, with child preferences. I started with int/string to cover the test case of the shelf. What I would like to to is to build strut_traits for the various base::Value subclasses. Which would allow for the passing of the more complicated dictionaries of preferences.
(Q: Are you expecting a full review from me, or did you just ask for thoughts?)
On 2016/07/27 17:06:55, Elliot Glaysher wrote: > (Q: Are you expecting a full review from me, or did you just ask for thoughts?) Just for thoughts, I knew that you had looked into this previously. Sadrul has been principal reviewer, and ben@ has owners for these files. Thanks.
ben@ could you PTAL?
I think fundamentally this looks good. https://codereview.chromium.org/2092453002/diff/210001/services/preferences/p... File services/preferences/public/cpp/preferences_export.h (right): https://codereview.chromium.org/2092453002/diff/210001/services/preferences/p... services/preferences/public/cpp/preferences_export.h:8: #if defined(COMPONENT_BUILD) why do you need this given that the client lib is a source_set, not a component? https://codereview.chromium.org/2092453002/diff/210001/services/preferences/p... File services/preferences/public/interfaces/preferences.mojom (right): https://codereview.chromium.org/2092453002/diff/210001/services/preferences/p... services/preferences/public/interfaces/preferences.mojom:9: // we can pass base::Value via mojo. FYI, Ken tells me there's already param traits support for base::Value in mojo/common.
https://codereview.chromium.org/2092453002/diff/210001/services/preferences/p... File services/preferences/public/cpp/preferences_export.h (right): https://codereview.chromium.org/2092453002/diff/210001/services/preferences/p... services/preferences/public/cpp/preferences_export.h:8: #if defined(COMPONENT_BUILD) On 2016/09/08 17:40:08, Ben Goodger (Google) wrote: > why do you need this given that the client lib is a source_set, not a component? Yeah this isn't needed with this as a source_set. removed https://codereview.chromium.org/2092453002/diff/210001/services/preferences/p... File services/preferences/public/interfaces/preferences.mojom (right): https://codereview.chromium.org/2092453002/diff/210001/services/preferences/p... services/preferences/public/interfaces/preferences.mojom:9: // we can pass base::Value via mojo. On 2016/09/08 17:40:08, Ben Goodger (Google) wrote: > FYI, Ken tells me there's already param traits support for base::Value in > mojo/common. Yeah there are apparently some of the base::Value subclasses there. I'll add the others and update this mojom
On 2016/09/09 15:55:36, jonross wrote: > https://codereview.chromium.org/2092453002/diff/210001/services/preferences/p... > File services/preferences/public/cpp/preferences_export.h (right): > > https://codereview.chromium.org/2092453002/diff/210001/services/preferences/p... > services/preferences/public/cpp/preferences_export.h:8: #if > defined(COMPONENT_BUILD) > On 2016/09/08 17:40:08, Ben Goodger (Google) wrote: > > why do you need this given that the client lib is a source_set, not a > component? > > Yeah this isn't needed with this as a source_set. removed > > https://codereview.chromium.org/2092453002/diff/210001/services/preferences/p... > File services/preferences/public/interfaces/preferences.mojom (right): > > https://codereview.chromium.org/2092453002/diff/210001/services/preferences/p... > services/preferences/public/interfaces/preferences.mojom:9: // we can pass > base::Value via mojo. > On 2016/09/08 17:40:08, Ben Goodger (Google) wrote: > > FYI, Ken tells me there's already param traits support for base::Value in > > mojo/common. > > Yeah there are apparently some of the base::Value subclasses there. > > I'll add the others and update this mojom Actually the base::DictionaryValue mojom is enough to handle the entire uses case. Since we were sending a map of string keys to base::Values anyways, this can be encompassed by using a DictionaryValue to begin with. I've updated the mojom based on this, removing the additional mojom types I had added. Now the prefs service and manager will use a DictionaryValue to track interactions of preferences that are desired. This also allows for more complex values to be passed with the current mojom, as the values themselves can be other DictionaryValues. sadrul@ ben@ thoughts?
looks good. https://codereview.chromium.org/2092453002/diff/270001/services/preferences/p... File services/preferences/public/cpp/BUILD.gn (right): https://codereview.chromium.org/2092453002/diff/270001/services/preferences/p... services/preferences/public/cpp/BUILD.gn:9: "preferences_export.h", Doesn't looks like this file is in the CL? But do you actually need to export? https://codereview.chromium.org/2092453002/diff/270001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.cc (right): https://codereview.chromium.org/2092453002/diff/270001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:45: SetValueOnPreferenceManager(key, value.get()); It'd be nice to not notify the server if the value doesn't actually change. (e.g. https://cs.chromium.org/chromium/src/components/prefs/value_map_pref_store.cc... doesn't notify the observers if the value didn't change). Let's leave a TODO here. https://codereview.chromium.org/2092453002/diff/270001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:94: ValueMapPrefStore::SetValue(it.key(), it.value().CreateDeepCopy(), 0); Add a comment here that we deliberately call the parent here to avoid notifying the server again. https://codereview.chromium.org/2092453002/diff/270001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.h (right): https://codereview.chromium.org/2092453002/diff/270001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.h:34: PrefObserverStore(prefs::mojom::PreferenceManagerPtr prefs_manager_ptr); explicit
https://codereview.chromium.org/2092453002/diff/270001/services/preferences/p... File services/preferences/public/cpp/BUILD.gn (right): https://codereview.chromium.org/2092453002/diff/270001/services/preferences/p... services/preferences/public/cpp/BUILD.gn:9: "preferences_export.h", On 2016/10/06 14:09:41, sadrul wrote: > Doesn't looks like this file is in the CL? But do you actually need to export? It's not anymore, and export not needed for source_set. Missed removing it here. https://codereview.chromium.org/2092453002/diff/270001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.cc (right): https://codereview.chromium.org/2092453002/diff/270001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:45: SetValueOnPreferenceManager(key, value.get()); On 2016/10/06 14:09:41, sadrul wrote: > It'd be nice to not notify the server if the value doesn't actually change. > (e.g. > https://cs.chromium.org/chromium/src/components/prefs/value_map_pref_store.cc... > doesn't notify the observers if the value didn't change). Let's leave a TODO > here. Done. https://codereview.chromium.org/2092453002/diff/270001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:94: ValueMapPrefStore::SetValue(it.key(), it.value().CreateDeepCopy(), 0); On 2016/10/06 14:09:41, sadrul wrote: > Add a comment here that we deliberately call the parent here to avoid notifying > the server again. Done. https://codereview.chromium.org/2092453002/diff/270001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.h (right): https://codereview.chromium.org/2092453002/diff/270001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.h:34: PrefObserverStore(prefs::mojom::PreferenceManagerPtr prefs_manager_ptr); On 2016/10/06 14:09:41, sadrul wrote: > explicit Done.
lgtm
On 2016/10/07 03:36:48, sadrul wrote: > lgtm ben@ would you be able to provide an owners review for this change? I've updated the mojom to use base::DictionaryValue directly. Which simplifies it. Thanks, Jon
jamescook@chromium.org changed reviewers: + jamescook@chromium.org
https://codereview.chromium.org/2092453002/diff/310001/services/preferences/p... File services/preferences/public/interfaces/preferences.mojom (right): https://codereview.chromium.org/2092453002/diff/310001/services/preferences/p... services/preferences/public/interfaces/preferences.mojom:11: interface PreferenceObserver { drive-by naming question: Is there any particular reason that the directory is "preference*s*", the interfaces are "preference", and the methods are "preference*s*" again? It seems inconsistent.
https://codereview.chromium.org/2092453002/diff/310001/services/preferences/p... File services/preferences/public/interfaces/preferences.mojom (right): https://codereview.chromium.org/2092453002/diff/310001/services/preferences/p... services/preferences/public/interfaces/preferences.mojom:11: interface PreferenceObserver { On 2016/10/11 17:08:29, James Cook wrote: > drive-by naming question: Is there any particular reason that the directory is > "preference*s*", the interfaces are "preference", and the methods are > "preference*s*" again? It seems inconsistent. Nope, just drift from when this was in the components/prefs/ dir, and there was a Preference struct here. For consistency I'll update these interfaces to "preferences"
https://codereview.chromium.org/2092453002/diff/310001/services/preferences/p... File services/preferences/public/interfaces/preferences.mojom (right): https://codereview.chromium.org/2092453002/diff/310001/services/preferences/p... services/preferences/public/interfaces/preferences.mojom:11: interface PreferenceObserver { On 2016/10/11 22:05:13, jonross wrote: > On 2016/10/11 17:08:29, James Cook wrote: > > drive-by naming question: Is there any particular reason that the directory is > > "preference*s*", the interfaces are "preference", and the methods are > > "preference*s*" again? It seems inconsistent. > > Nope, just drift from when this was in the components/prefs/ dir, and there was > a Preference struct here. > > For consistency I'll update these interfaces to "preferences" Done.
ben@ would you be able to provide an owner's review for this change? Thanks, Jon
lgtm
The CQ bit was checked by jonross@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2092453002/#ps370001 (title: "Expand Unittests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Mojom interface for Preferences This change creates a mojom interface for Preferences. This will allow chrome/browser to continue to manager preferences. While allowing clients in other processes to access them. PrefObserverStore has been created to abstract away the interface from clients. Instead this can be used by clients as a normal PrefStore, with synchronous access to preferences. TEST=PrefObserverStoreTest, also manual testing with shelf preferences (not in this review) BUG=622347 ========== to ========== Mojom interface for Preferences This change creates a mojom interface for Preferences. This will allow chrome/browser to continue to manager preferences. While allowing clients in other processes to access them. PrefObserverStore has been created to abstract away the interface from clients. Instead this can be used by clients as a normal PrefStore, with synchronous access to preferences. TEST=PrefObserverStoreTest, also manual testing with shelf preferences (not in this review) BUG=622347 ==========
jonross@chromium.org changed reviewers: + dcheng@chromium.org
On 2016/10/25 13:31:09, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Hi dcheng@, Could you provide a security owners review for: services/preferences/public/interfaces/preferences.mojom It is a new mojom to allow for preferences to be communicated between chrome and other services, while running in mus. Thanks, Jon
https://codereview.chromium.org/2092453002/diff/370001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.h (right): https://codereview.chromium.org/2092453002/diff/370001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.h:72: prefs::mojom::PreferencesManager* prefs_manager_; Can the unit test just pass in an actual prefs::mojom::PreferencesManagerPtr? It should be pretty easy; look at https://cs.chromium.org/chromium/src/url/mojo/url_gurl_struct_traits_unittest... for one example.
https://codereview.chromium.org/2092453002/diff/370001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.h (right): https://codereview.chromium.org/2092453002/diff/370001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.h:72: prefs::mojom::PreferencesManager* prefs_manager_; On 2016/10/26 22:17:32, dcheng wrote: > Can the unit test just pass in an actual prefs::mojom::PreferencesManagerPtr? It > should be pretty easy; look at > https://cs.chromium.org/chromium/src/url/mojo/url_gurl_struct_traits_unittest... > for one example. The tests can be switched over to instantiating a prefs::mojom::PreferencesManagerPtr. However that would begin pulling in dependencies on actually writing messages to the pipes. With certain tests needing to manage advancing the MessageLoop. I had thought to keep the scope of these tests more limited. Thoughts on that trade-off?
On 2016/10/27 19:29:34, jonross wrote: > https://codereview.chromium.org/2092453002/diff/370001/services/preferences/p... > File services/preferences/public/cpp/pref_observer_store.h (right): > > https://codereview.chromium.org/2092453002/diff/370001/services/preferences/p... > services/preferences/public/cpp/pref_observer_store.h:72: > prefs::mojom::PreferencesManager* prefs_manager_; > On 2016/10/26 22:17:32, dcheng wrote: > > Can the unit test just pass in an actual prefs::mojom::PreferencesManagerPtr? > It > > should be pretty easy; look at > > > https://cs.chromium.org/chromium/src/url/mojo/url_gurl_struct_traits_unittest... > > for one example. > > The tests can be switched over to instantiating a > prefs::mojom::PreferencesManagerPtr. However that would begin pulling in > dependencies on actually writing messages to the pipes. With certain tests > needing to manage advancing the MessageLoop. > > I had thought to keep the scope of these tests more limited. > > Thoughts on that trade-off? I think it's better to put the complexity in the tests. (Random musing: I wonder if it's possible to make Mojo return a special testing proxy that would do the message loop spinning when Mojo methods are invoked. I'm not sure if that's actually a good idea or not though...)
On 2016/10/27 21:38:23, dcheng wrote: > On 2016/10/27 19:29:34, jonross wrote: > > > https://codereview.chromium.org/2092453002/diff/370001/services/preferences/p... > > File services/preferences/public/cpp/pref_observer_store.h (right): > > > > > https://codereview.chromium.org/2092453002/diff/370001/services/preferences/p... > > services/preferences/public/cpp/pref_observer_store.h:72: > > prefs::mojom::PreferencesManager* prefs_manager_; > > On 2016/10/26 22:17:32, dcheng wrote: > > > Can the unit test just pass in an actual > prefs::mojom::PreferencesManagerPtr? > > It > > > should be pretty easy; look at > > > > > > https://cs.chromium.org/chromium/src/url/mojo/url_gurl_struct_traits_unittest... > > > for one example. > > > > The tests can be switched over to instantiating a > > prefs::mojom::PreferencesManagerPtr. However that would begin pulling in > > dependencies on actually writing messages to the pipes. With certain tests > > needing to manage advancing the MessageLoop. > > > > I had thought to keep the scope of these tests more limited. > > > > Thoughts on that trade-off? > > I think it's better to put the complexity in the tests. > > (Random musing: I wonder if it's possible to make Mojo return a special testing > proxy that would do the message loop spinning when Mojo methods are invoked. I'm > not sure if that's actually a good idea or not though...) I was diving deeper into this, and noticed a behaviour difference between MessageReceiver::Accept and MessageReceiverWithResponder::AcceptWithResponder Here in preferences the generated mojo code uses Accept, which fires off the message as a task. In the example you linked (url_gurl_struct_traits_unittest.cc) the generated mojo uses AcceptWithResonder which triggers the message directly. This differentiation can be dangerous with tests. While I don't think we need a special testing proxy, it would be good to get this documented for the next time tests need to be written. I'll switch over to the Ptr being made in the tests.
Patchset #20 (id:390001) has been deleted
https://codereview.chromium.org/2092453002/diff/370001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.h (right): https://codereview.chromium.org/2092453002/diff/370001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.h:72: prefs::mojom::PreferencesManager* prefs_manager_; On 2016/10/27 19:29:34, jonross wrote: > On 2016/10/26 22:17:32, dcheng wrote: > > Can the unit test just pass in an actual prefs::mojom::PreferencesManagerPtr? > It > > should be pretty easy; look at > > > https://cs.chromium.org/chromium/src/url/mojo/url_gurl_struct_traits_unittest... > > for one example. > > The tests can be switched over to instantiating a > prefs::mojom::PreferencesManagerPtr. However that would begin pulling in > dependencies on actually writing messages to the pipes. With certain tests > needing to manage advancing the MessageLoop. > > I had thought to keep the scope of these tests more limited. > > Thoughts on that trade-off? Done.
https://codereview.chromium.org/2092453002/diff/410001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.cc (right): https://codereview.chromium.org/2092453002/diff/410001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:24: prefs_manager_ptr_->AddObserver(std::move(pref_array), Does this std::move() have an effect? I think the generated stub will take this argument by const ref. Also, I think mojo already has array traits that allow the array to be populated from a set, so maybe this isn't even needed. https://codereview.chromium.org/2092453002/diff/410001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.h (right): https://codereview.chromium.org/2092453002/diff/410001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.h:27: // received from the prefs::mojom::PreferenceManager. Upon this completion any Nit: Presumably, "this completion" is referring to the first notification of OnPreferencesChanged? But from the way the comment is worded, I'm not 100% sure if this is supposed to happen when we construct it / init it, or if it just happens asynchronously.
https://codereview.chromium.org/2092453002/diff/410001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.h (right): https://codereview.chromium.org/2092453002/diff/410001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.h:50: bool GetMutableValue(const std::string& key, base::Value** value) override; Btw, how is this supposed to work in conjunction with the preference notifications?
Patchset #21 (id:430001) has been deleted
https://codereview.chromium.org/2092453002/diff/410001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.cc (right): https://codereview.chromium.org/2092453002/diff/410001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:24: prefs_manager_ptr_->AddObserver(std::move(pref_array), On 2016/10/31 19:42:46, dcheng wrote: > Does this std::move() have an effect? I think the generated stub will take this > argument by const ref. > > Also, I think mojo already has array traits that allow the array to be populated > from a set, so maybe this isn't even needed. The move is unnecessary, and removed. For mojo, there are array traits for sets. However that does not seem to be compiled in for methods. AddObserver(array<string> preferences, PreferencesObserver client); is converted to virtual void AddObserver(const std::vector<std::string>& preferences, PreferencesObserverPtr client) = 0; https://codereview.chromium.org/2092453002/diff/410001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.h (right): https://codereview.chromium.org/2092453002/diff/410001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.h:27: // received from the prefs::mojom::PreferenceManager. Upon this completion any On 2016/10/31 19:42:46, dcheng wrote: > Nit: Presumably, "this completion" is referring to the first notification of > OnPreferencesChanged? But from the way the comment is worded, I'm not 100% sure > if this is supposed to happen when we construct it / init it, or if it just > happens asynchronously. Done. https://codereview.chromium.org/2092453002/diff/410001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.h:50: bool GetMutableValue(const std::string& key, base::Value** value) override; On 2016/10/31 20:11:31, dcheng wrote: > Btw, how is this supposed to work in conjunction with the preference > notifications? I've updated ReportValueChanged to notify the PreferencesManager of changes. This better reflects the usage throughout the code which has a pattern of: - GetMutableValue - ReportValueChanged In general it would be nice to see usage shift away from GetMutableValue. As right now two access points can become out of sync unless they are both taking mutable values. This will become worse with the process split.
mojo lgtm https://codereview.chromium.org/2092453002/diff/450001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.h (right): https://codereview.chromium.org/2092453002/diff/450001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.h:66: const base::Value* value); Nit: might be nice to pass this as a const ref
https://codereview.chromium.org/2092453002/diff/450001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.h (right): https://codereview.chromium.org/2092453002/diff/450001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.h:66: const base::Value* value); On 2016/11/01 18:04:38, dcheng wrote: > Nit: might be nice to pass this as a const ref Done.
The CQ bit was checked by jonross@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, ben@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2092453002/#ps470001 (title: "Const ref nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Mojom interface for Preferences This change creates a mojom interface for Preferences. This will allow chrome/browser to continue to manager preferences. While allowing clients in other processes to access them. PrefObserverStore has been created to abstract away the interface from clients. Instead this can be used by clients as a normal PrefStore, with synchronous access to preferences. TEST=PrefObserverStoreTest, also manual testing with shelf preferences (not in this review) BUG=622347 ========== to ========== Mojom interface for Preferences This change creates a mojom interface for Preferences. This will allow chrome/browser to continue to manager preferences. While allowing clients in other processes to access them. PrefObserverStore has been created to abstract away the interface from clients. Instead this can be used by clients as a normal PrefStore, with synchronous access to preferences. TEST=PrefObserverStoreTest, also manual testing with shelf preferences (not in this review) BUG=622347 ==========
Message was sent while issue was closed.
Committed patchset #22 (id:470001)
Message was sent while issue was closed.
Description was changed from ========== Mojom interface for Preferences This change creates a mojom interface for Preferences. This will allow chrome/browser to continue to manager preferences. While allowing clients in other processes to access them. PrefObserverStore has been created to abstract away the interface from clients. Instead this can be used by clients as a normal PrefStore, with synchronous access to preferences. TEST=PrefObserverStoreTest, also manual testing with shelf preferences (not in this review) BUG=622347 ========== to ========== Mojom interface for Preferences This change creates a mojom interface for Preferences. This will allow chrome/browser to continue to manager preferences. While allowing clients in other processes to access them. PrefObserverStore has been created to abstract away the interface from clients. Instead this can be used by clients as a normal PrefStore, with synchronous access to preferences. TEST=PrefObserverStoreTest, also manual testing with shelf preferences (not in this review) BUG=622347 Committed: https://crrev.com/63b021f2555ac1094961fc6f82de77b7ac31b312 Cr-Commit-Position: refs/heads/master@{#429166} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/63b021f2555ac1094961fc6f82de77b7ac31b312 Cr-Commit-Position: refs/heads/master@{#429166} |
