Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2474653003:
PreferencesManager (Closed)
DescriptionChrome implementation of prefs::mojom::PreferencesManager, which connects to the current Profile and subscribes to preference changes when requested to by a prefs::mojom::PreferencesObserver.
Also includes a connection manager to handle mapping incoming requests to their own preferences manager.
TEST=manual testing with example code, PreferencesManagerTest
BUG=622347
Committed: https://crrev.com/3b1b258d4e59022e8a931f965e800427618894c4
Cr-Commit-Position: refs/heads/master@{#440118}
Patch Set 1 #Patch Set 2 : Ash example #
Total comments: 1
Patch Set 3 : Update example and chrome notifies ash #Patch Set 4 : PreferencesConnectionManager #
Total comments: 2
Patch Set 5 : Connection Manager and Tests #Patch Set 6 : Rebase #
Total comments: 24
Patch Set 7 : Update Manager - example update to come #Patch Set 8 : WmShell Owns PrefObserverStore #
Total comments: 2
Patch Set 9 : Cleanup Init notifications #
Total comments: 2
Patch Set 10 : Update mojom to support separate subscriptions #
Total comments: 23
Patch Set 11 : Rebase #Patch Set 12 : Address Review Comments #
Total comments: 12
Patch Set 13 : Namespace #Patch Set 14 : Rebase #Patch Set 15 : Enforce Observer Removal #Patch Set 16 : Revert Example #
Total comments: 18
Patch Set 17 : Review Updates #Patch Set 18 : DLOG error states #
Total comments: 18
Patch Set 19 : Delete bindings on connection error #Patch Set 20 : Updates #
Total comments: 4
Patch Set 21 : Update Binding Check and ConnectionError Handling #
Total comments: 2
Patch Set 22 : Rebase #Patch Set 23 : Switch to Embedded Service #Patch Set 24 : Rebase #Patch Set 25 : Rebase missed a changed method #Patch Set 26 : Unrelated test static breaking PrefConnectionM #Patch Set 27 : Rebase #Patch Set 28 : Update PreferencesManager to account for base::Value API change #Messages
Total messages: 120 (44 generated)
https://codereview.chromium.org/2474653003/diff/20001/ash/common/shelf/shelf_... File ash/common/shelf/shelf_controller.cc (right): https://codereview.chromium.org/2474653003/diff/20001/ash/common/shelf/shelf_... ash/common/shelf/shelf_controller.cc:188: store_->Init(keys); Once this is connected it can be used as a normal PrefStore for accessing prefs. I'm considering modifying PrefService to perform this init behind the scenes. On the Ash side you can implement PrefStore::Observer to be notified of changes to prefs. The PrefObserverStore above will trigger notifications to these observers once chrome has sent the initial values.
jamescook@chromium.org changed reviewers: + jamescook@chromium.org
https://codereview.chromium.org/2474653003/diff/60001/ash/common/shelf/shelf_... File ash/common/shelf/shelf_controller.cc (right): https://codereview.chromium.org/2474653003/diff/60001/ash/common/shelf/shelf_... ash/common/shelf/shelf_controller.cc:191: keys.insert(key); Drive-by: I wonder if we should have a single pref_manager or pref_service controller hanging off of WmShell. In particular, it might be nice to have a single place where we build the list of all prefs that ash cares about, so we can subscribe to all of them in one IPC. We could also do something where each individual controller class (like this one) in its constructor registers the prefs it cares about with WmShell/pref_manager, then WmShell subscribes to them all at the end of initialization. Thanks for working on this, btw, it'll simplify some of the stuff I have to do for SystemTrayDelegate.
https://codereview.chromium.org/2474653003/diff/60001/ash/common/shelf/shelf_... File ash/common/shelf/shelf_controller.cc (right): https://codereview.chromium.org/2474653003/diff/60001/ash/common/shelf/shelf_... ash/common/shelf/shelf_controller.cc:191: keys.insert(key); On 2016/11/15 20:27:17, James Cook wrote: > Drive-by: I wonder if we should have a single pref_manager or pref_service > controller hanging off of WmShell. In particular, it might be nice to have a > single place where we build the list of all prefs that ash cares about, so we > can subscribe to all of them in one IPC. > > We could also do something where each individual controller class (like this > one) in its constructor registers the prefs it cares about with > WmShell/pref_manager, then WmShell subscribes to them all at the end of > initialization. > > Thanks for working on this, btw, it'll simplify some of the stuff I have to do > for SystemTrayDelegate. We could make a single exit point from Ash. To reduce the size of the subscription IPC. As is right now each component in ash could add itself as a PrefStore::Observer, but that would become quite noisy in process as the current observer model sends notifications for any change in the store. We could do: 1) Create a prefs::mojom::PreferenceObserver that manages the connection. Which would create small PrefStores for each component in Ash that wants to listen. 2) Update PrefObserverStore::AddObserver to require a set of keys, and do the filtering of notifications there.
Description was changed from ========== WIP PreferencesManager Not for review, building implementation of prefs::mojom::PreferencesManager BUG=622347 ========== to ========== Chrome implementation of prefs::mojom::PreferencesManager, which connects to the current Profile and subscribes to preference changes when requested to by a prefs::mojom::PreferencesObserver. Also includes a connection manager to handle mapping incoming requests to their own preferences manager. TEST=manual testing with example code, PreferencesManagerTest BUG=622347 ==========
jonross@chromium.org changed reviewers: + sadrul@chromium.org
Hey sadrul@ Could you provide a review of this change? It's the chrome implementation of prefs::mojom::PreferencesManager which you previously reviewed. The example usage code in ash/ will be reverted before submission. Thanks, Jon
The CQ bit was checked by jonross@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by jonross@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
On 2016/11/16 22:32:22, jonross wrote: > Hey sadrul@ > > Could you provide a review of this change? It's the chrome implementation of > prefs::mojom::PreferencesManager which you previously reviewed. > > The example usage code in ash/ will be reverted before submission. > > Thanks, > Jon Current trybot failure is due do testing example code I added to the shelf controller. It will be reverted before submission.
msw@chromium.org changed reviewers: + msw@chromium.org
Nice! I'm excited about this work; I didn't do a full review, but I have some tangential comments for the example usage in this patch set, and one other question. Thanks! https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... File ash/common/shelf/shelf_controller.cc (right): https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... ash/common/shelf/shelf_controller.cc:179: // Sample of connecting to the PreferencesManager aside: reducing some boilerplate might be nice. https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... ash/common/shelf/shelf_controller.cc:189: // chrome::common::pref_names::kShelfAutoHideBehavior aside: I guess we'll move ash-relevant pref names to ash, right? (we'll probably have to figure this out on a case-by-case basis) https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... ash/common/shelf/shelf_controller.cc:223: switch (behavior) { aside: would it make sense to store the behavior enum value? (perhaps this is how the current pref works, I didn't check) https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... ash/common/shelf/shelf_controller.cc:337: void ShelfController::OnInitializationCompleted(bool succeeded) { aside: I wonder if it'd make sense to just call OnPrefValueChanged for each of the initialized prefs. That would probably work in this case, but might not make sense in other cases... luckily, it seems easy for this impl to do that itself (manually call OnPrefValueChanged(key1); OnPrefValueChanged(key2);) https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.cc (right): https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:55: mojo::MakeStrongBinding(base::MakeUnique<PreferencesManager>(profile), I wonder if a single PreferencesManager instance could just manage a BindingSet instead?
https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... File ash/common/shelf/shelf_controller.cc (right): https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... ash/common/shelf/shelf_controller.cc:179: // Sample of connecting to the PreferencesManager On 2016/11/18 00:32:57, msw wrote: > aside: reducing some boilerplate might be nice. Moving this construction to WmShell would centralize the mojom boilerplate. Then various ash components could add themselves as observers of the single PrefObserverStore https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... ash/common/shelf/shelf_controller.cc:189: // chrome::common::pref_names::kShelfAutoHideBehavior On 2016/11/18 00:32:57, msw wrote: > aside: I guess we'll move ash-relevant pref names to ash, right? > (we'll probably have to figure this out on a case-by-case basis) An issue with this is that the consts need to be in both locations. Chrome has strict restrictions on registering preferences at startup. I'm not sure the best place to put them to have them shared between ash/chrome https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... ash/common/shelf/shelf_controller.cc:223: switch (behavior) { On 2016/11/18 00:32:57, msw wrote: > aside: would it make sense to store the behavior enum value? > (perhaps this is how the current pref works, I didn't check) Yeah in most cases it would make sense to just have an enum define behaviour, and send that across. For this particular case the chrome-side is storing the behaviour as string-prefs. Then converting to enums and sending them over mojo. As we switch to using the prefs::mojom it wouls be a good idea to clean this up. https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... ash/common/shelf/shelf_controller.cc:337: void ShelfController::OnInitializationCompleted(bool succeeded) { On 2016/11/18 00:32:57, msw wrote: > aside: I wonder if it'd make sense to just call OnPrefValueChanged for each of > the initialized prefs. That would probably work in this case, but might not make > sense in other cases... luckily, it seems easy for this impl to do that itself > (manually call OnPrefValueChanged(key1); OnPrefValueChanged(key2);) This init/changed paradigm is just brought over from PrefStore to match existing behaviour. Was trying to reduce the api changes. There's also a weird creation order bug in ValueMapPrefStore which causes crashes when asking for prefs before init is done. However this is something that could be handled within the new PrefObserverStore quite easily. Especially if it made it so that ash only needs to listen to one of these. https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.cc (right): https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:55: mojo::MakeStrongBinding(base::MakeUnique<PreferencesManager>(profile), On 2016/11/18 00:32:57, msw wrote: > I wonder if a single PreferencesManager instance could just manage a BindingSet > instead? I originally had that, but wanted to handle different services listening to the same pref. So that one could set a new value, receive no notification, while others who didn't request the change receive an update. The StrongBindingPtr also nicely handles onConnectionError automatically.
Just some drive-by comments. I looked mostly at ash, not at chrome. https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... File ash/common/shelf/shelf_controller.cc (right): https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... ash/common/shelf/shelf_controller.cc:189: // chrome::common::pref_names::kShelfAutoHideBehavior On 2016/11/18 00:32:57, msw wrote: > aside: I guess we'll move ash-relevant pref names to ash, right? > (we'll probably have to figure this out on a case-by-case basis) +1. Most components have a pref_names.cc file, we could do the same. https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... File ash/common/shelf/shelf_controller.h (right): https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... ash/common/shelf/shelf_controller.h:74: scoped_refptr<PrefObserverStore> store_; Like we discussed in person, I think a single centralized store would be nice. (That might also reduce the #include burden. Mojo headers are really heavy, and pref_observer_store.h includes some mojo stuff.) https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.cc (right): https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:61: for (auto it : bindings_) { auto& ? auto* ? Otherwise the copying (or not) isn't clear. https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.h (right): https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.h:11: #include "components/keyed_service/core/keyed_service_shutdown_notifier.h" Forward declare this? https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.cc (right): https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:34: for (auto it : preferences) { ditto https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.h (right): https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.h:18: // class PreferencesManagerTest; remove
https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... File ash/common/shelf/shelf_controller.cc (right): https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... ash/common/shelf/shelf_controller.cc:189: // chrome::common::pref_names::kShelfAutoHideBehavior On 2016/11/18 00:47:58, jonross wrote: > On 2016/11/18 00:32:57, msw wrote: > > aside: I guess we'll move ash-relevant pref names to ash, right? > > (we'll probably have to figure this out on a case-by-case basis) > > An issue with this is that the consts need to be in both locations. Chrome has > strict restrictions on registering preferences at startup. > > I'm not sure the best place to put them to have them shared between ash/chrome Yeah, ash/public/cpp/pref_names.h could declare these for ash and chrome.
https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... File ash/common/shelf/shelf_controller.cc (right): https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... ash/common/shelf/shelf_controller.cc:189: // chrome::common::pref_names::kShelfAutoHideBehavior On 2016/11/18 00:58:13, msw wrote: > On 2016/11/18 00:47:58, jonross wrote: > > On 2016/11/18 00:32:57, msw wrote: > > > aside: I guess we'll move ash-relevant pref names to ash, right? > > > (we'll probably have to figure this out on a case-by-case basis) > > > > An issue with this is that the consts need to be in both locations. Chrome has > > strict restrictions on registering preferences at startup. > > > > I'm not sure the best place to put them to have them shared between ash/chrome > > Yeah, ash/public/cpp/pref_names.h could declare these for ash and chrome. That SGTM
https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... File ash/common/shelf/shelf_controller.h (right): https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... ash/common/shelf/shelf_controller.h:74: scoped_refptr<PrefObserverStore> store_; On 2016/11/18 00:50:34, James Cook wrote: > Like we discussed in person, I think a single centralized store would be nice. > > (That might also reduce the #include burden. Mojo headers are really heavy, and > pref_observer_store.h includes some mojo stuff.) Yeah I agree. I will update this example in a bit with a unified one in WmShell https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.cc (right): https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:61: for (auto it : bindings_) { On 2016/11/18 00:50:34, James Cook wrote: > auto& ? auto* ? > > Otherwise the copying (or not) isn't clear. Done. https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.h (right): https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.h:11: #include "components/keyed_service/core/keyed_service_shutdown_notifier.h" On 2016/11/18 00:50:34, James Cook wrote: > Forward declare this? I'm using the a nested class from it :( https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.cc (right): https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:34: for (auto it : preferences) { On 2016/11/18 00:50:34, James Cook wrote: > ditto Done. https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.h (right): https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.h:18: // class PreferencesManagerTest; On 2016/11/18 00:50:34, James Cook wrote: > remove Done.
The CQ bit was checked by jonross@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
On 2016/11/18 20:31:24, jonross wrote: > https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... > File ash/common/shelf/shelf_controller.h (right): > > https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... > ash/common/shelf/shelf_controller.h:74: scoped_refptr<PrefObserverStore> store_; > On 2016/11/18 00:50:34, James Cook wrote: > > Like we discussed in person, I think a single centralized store would be nice. > > > > (That might also reduce the #include burden. Mojo headers are really heavy, > and > > pref_observer_store.h includes some mojo stuff.) > > Yeah I agree. I will update this example in a bit with a unified one in WmShell > > https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... > File chrome/browser/prefs/preferences_connection_manager.cc (right): > > https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... > chrome/browser/prefs/preferences_connection_manager.cc:61: for (auto it : > bindings_) { > On 2016/11/18 00:50:34, James Cook wrote: > > auto& ? auto* ? > > > > Otherwise the copying (or not) isn't clear. > > Done. > > https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... > File chrome/browser/prefs/preferences_connection_manager.h (right): > > https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... > chrome/browser/prefs/preferences_connection_manager.h:11: #include > "components/keyed_service/core/keyed_service_shutdown_notifier.h" > On 2016/11/18 00:50:34, James Cook wrote: > > Forward declare this? > > I'm using the a nested class from it :( > > https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... > File chrome/browser/prefs/preferences_manager.cc (right): > > https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... > chrome/browser/prefs/preferences_manager.cc:34: for (auto it : preferences) { > On 2016/11/18 00:50:34, James Cook wrote: > > ditto > > Done. > > https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... > File chrome/browser/prefs/preferences_manager.h (right): > > https://codereview.chromium.org/2474653003/diff/100001/chrome/browser/prefs/p... > chrome/browser/prefs/preferences_manager.h:18: // class PreferencesManagerTest; > On 2016/11/18 00:50:34, James Cook wrote: > > remove > > Done. I've updated the example usage code to include WmShell instantiating the PrefObserverStore and taking over the connection handling. This makes me want to update a bit of the PrefObserverStore naming. Since instead of "init" components in ash would be adding subscriptions.
On 2016/11/18 23:24:24, jonross wrote: > I've updated the example usage code to include WmShell instantiating the > PrefObserverStore and taking over the connection handling. This makes me want to > update a bit of the PrefObserverStore naming. Since instead of "init" components > in ash would be adding subscriptions. Is it possible now to address my comment about having a single PreferencesManager instance that just manages a BindingSet (and since now there's just one connection, it would really just have one binding)?
On 2016/11/18 23:31:13, msw wrote: > On 2016/11/18 23:24:24, jonross wrote: > > I've updated the example usage code to include WmShell instantiating the > > PrefObserverStore and taking over the connection handling. This makes me want > to > > update a bit of the PrefObserverStore naming. Since instead of "init" > components > > in ash would be adding subscriptions. > > Is it possible now to address my comment about having a single > PreferencesManager instance that just manages a BindingSet (and since now > there's just one connection, it would really just have one binding)? This gives us a single chrome-ash connection, which could use a BindingSet. I know in the future if we were to pull out prefs into a standalone service that we would be facing multiple connections: ash; app_shell; chrome. In that case having strong bindings would allow us to track who requested what. In the interim do we foresee any other service needing to connect to chrome for prefs? If not I'll update this back to just being a binding set.
https://codereview.chromium.org/2474653003/diff/140001/ash/common/wm_shell.h File ash/common/wm_shell.h (right): https://codereview.chromium.org/2474653003/diff/140001/ash/common/wm_shell.h#... ash/common/wm_shell.h:157: PrefObserverStore* pref_observer_store() { optional naming thought: Since most of ash is going to use this as a PrefStore, what about calling it pref_store()? It's not clear to me if the usage semantics are similar enough to PrefStore that we can call it pref_store (which would be nice) or if the name should emphasize that it works differently. Also, depending on how you need use it, you could store it as a std::unique_ptr<PrefStore>.
On 2016/11/18 23:45:12, jonross wrote: > On 2016/11/18 23:31:13, msw wrote: > > On 2016/11/18 23:24:24, jonross wrote: > > > I've updated the example usage code to include WmShell instantiating the > > > PrefObserverStore and taking over the connection handling. This makes me > want > > to > > > update a bit of the PrefObserverStore naming. Since instead of "init" > > components > > > in ash would be adding subscriptions. > > > > Is it possible now to address my comment about having a single > > PreferencesManager instance that just manages a BindingSet (and since now > > there's just one connection, it would really just have one binding)? > > This gives us a single chrome-ash connection, which could use a BindingSet. > I know in the future if we were to pull out prefs into a standalone service that > we would be facing multiple connections: ash; app_shell; chrome. In that case > having strong bindings would allow us to track who requested what. > In the interim do we foresee any other service needing to connect to chrome for > prefs? > > If not I'll update this back to just being a binding set. Ah, we'll definitely have multiple clients connecting to chrome (and eventually a prefs service) for prefs, and they'll each want to be notified if another service tweaks a pref, so your earlier reply is still valid. Sorry for the distraction.
@msw: not a distraction at all, I'm all for keeping the interface as clean/simple as required. The feedback is appreciated. Updated PrefObserverStore init notifications. To come: renaming PrefObserverStore::Init to Subscribe, to better reflect that there is now a 1-* relationship, and we need to subscribe multiple times. https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... File ash/common/shelf/shelf_controller.cc (right): https://codereview.chromium.org/2474653003/diff/100001/ash/common/shelf/shelf... ash/common/shelf/shelf_controller.cc:337: void ShelfController::OnInitializationCompleted(bool succeeded) { On 2016/11/18 00:47:58, jonross wrote: > On 2016/11/18 00:32:57, msw wrote: > > aside: I wonder if it'd make sense to just call OnPrefValueChanged for each of > > the initialized prefs. That would probably work in this case, but might not > make > > sense in other cases... luckily, it seems easy for this impl to do that itself > > (manually call OnPrefValueChanged(key1); OnPrefValueChanged(key2);) > > This init/changed paradigm is just brought over from PrefStore to match existing > behaviour. Was trying to reduce the api changes. There's also a weird creation > order bug in ValueMapPrefStore which causes crashes when asking for prefs before > init is done. > > However this is something that could be handled within the new PrefObserverStore > quite easily. Especially if it made it so that ash only needs to listen to one > of these. I've updated this based on your suggestion. Further digging also showed that most OnInitCompleted was just used to handle error states. PrefObserverStore will not fire off the OnInitCompleted, then will fire OnPrefValueChanged per key. https://codereview.chromium.org/2474653003/diff/140001/ash/common/wm_shell.h File ash/common/wm_shell.h (right): https://codereview.chromium.org/2474653003/diff/140001/ash/common/wm_shell.h#... ash/common/wm_shell.h:157: PrefObserverStore* pref_observer_store() { On 2016/11/18 23:56:05, James Cook wrote: > optional naming thought: Since most of ash is going to use this as a PrefStore, > what about calling it pref_store()? > > It's not clear to me if the usage semantics are similar enough to PrefStore that > we can call it pref_store (which would be nice) or if the name should emphasize > that it works differently. > > Also, depending on how you need use it, you could store it as a > std::unique_ptr<PrefStore>. I like the idea of renaming this as pref_store. One of the goals was being able to have areas of the code which use prefstores to be able to initialize one of these. Then everything can use it as if it was just another store. Sadly PrefStores are ref-counted, so I can't use unique_ptr :(
https://codereview.chromium.org/2474653003/diff/160001/ash/common/wm_shell.h File ash/common/wm_shell.h (right): https://codereview.chromium.org/2474653003/diff/160001/ash/common/wm_shell.h#... ash/common/wm_shell.h:494: scoped_refptr<PrefObserverStore> pref_store_; Sorry, I meant, store this as a scoped_refptr<PrefStore>. But that may not work if you need to call Init() in client code.
https://codereview.chromium.org/2474653003/diff/160001/ash/common/wm_shell.h File ash/common/wm_shell.h (right): https://codereview.chromium.org/2474653003/diff/160001/ash/common/wm_shell.h#... ash/common/wm_shell.h:494: scoped_refptr<PrefObserverStore> pref_store_; On 2016/11/19 00:33:19, James Cook wrote: > Sorry, I meant, store this as a scoped_refptr<PrefStore>. But that may not work > if you need to call Init() in client code. Ah, yeah. I had planned for client code to call Init. That way each component can register on demand. The chrome side has a few monolithic spots where it registers all prefs. We could do something similar, however I am hesitant as that has lead to issues with bringing up unit tests.
Updated the preferences mojom to separate becoming an observer of a PreferencesManager from subscribing to sets of preferences. This will allow services to bind once to the manager (eg ash-to-chrome) while the components within ash can each subscribe separately as they are needed.
https://codereview.chromium.org/2474653003/diff/180001/ash/common/wm_shell.cc File ash/common/wm_shell.cc (right): https://codereview.chromium.org/2474653003/diff/180001/ash/common/wm_shell.cc... ash/common/wm_shell.cc:64: connector->ConnectToInterface("content_browser", &pref_manager_ptr); I know that you will be removing this change from this CL. But ... I think the better way of doing this would be to go through the catalog service (see https://cs.chromium.org/chromium/src/services/ui/ime/ime_server_impl.cc?type=...), instead of directly connecting to content_browser, if possible. https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.cc (right): https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:47: base::Unretained(this)))) {} Since you are using Unretained(this) to create the callback, do you not need to unsubscribe from the dtor? Or does |profile_shutdown_notification_| take care of that? https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:65: (static_cast<PreferencesManager*>(it->impl()))->OnProfileDestroyed(); Not a huge fan of the static_cast<>ing. Can PreferencesManager call back into PCM when it gets destroyed, so it updates |bindings_| instead? https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.cc (right): https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:46: setting_preferences_ = true; Move this out of the for loop, and use base::AutoReset<>? Maybe a DCHECK() that |setting_preferences_| already isn't true when this function is called? https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:60: // will outlive |preferences_change_registrar_| which it owns. This comment is slightly wrong: PCM does not actually manage the lifetime of this PM, right? https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:78: return; Should |pref| ever be null? https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:81: client_->OnPreferencesChanged(dictionary); If a lot of preferences are updating at the same time, it'd be nice to bundle up all those updates in the same IPC, instead of a separate message for each pref change. Maybe things like that don't happen [very often]? https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.h (right): https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.h:27: // the requested preferences, notifying the client of all changes. If we have a case where multiple clients are observing the same set of preferences, then it may make sense to rearrange how this works. For example, instead of having a PreferencesManager instance for each client, we could have a single PreferencesManager instance that services all clients, and maintains a {pref => [clients]} map to decide when to notify which clients. But for now, this should be OK. https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.h:45: void PreferenceChanged(const std::string& preference_name); Non-override before override. (I seem to remember we used to have a style-guide entry for this, but can't find it anymore ... so dunno) https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.h:59: } // namespace chrome Do we usually use chrome namespace? https://codereview.chromium.org/2474653003/diff/180001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.cc (right): https://codereview.chromium.org/2474653003/diff/180001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:76: ClearObservers(); Why is this needed?
https://codereview.chromium.org/2474653003/diff/180001/ash/common/wm_shell.cc File ash/common/wm_shell.cc (right): https://codereview.chromium.org/2474653003/diff/180001/ash/common/wm_shell.cc... ash/common/wm_shell.cc:64: connector->ConnectToInterface("content_browser", &pref_manager_ptr); On 2016/11/29 17:25:49, sadrul wrote: > I know that you will be removing this change from this CL. But ... > > I think the better way of doing this would be to go through the catalog service > (see > https://cs.chromium.org/chromium/src/services/ui/ime/ime_server_impl.cc?type=...), > instead of directly connecting to content_browser, if possible. I've updated this to follow the catalog model. Based on the feedback from jamescook@ & msw@ I'm considering landing the WmShell portion of the code with this change. I would still revert the ShelfController example. This would then allow followup patches to independently start using the chrome-ash connection established here. Thoughts? https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.cc (right): https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:47: base::Unretained(this)))) {} On 2016/11/29 17:25:49, sadrul wrote: > Since you are using Unretained(this) to create the callback, do you not need to > unsubscribe from the dtor? Or does |profile_shutdown_notification_| take care of > that? |profile_shutdown_notification_| does that deep within its dtor. The unique_ptr + Unretained is the pattern used everywhere https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:65: (static_cast<PreferencesManager*>(it->impl()))->OnProfileDestroyed(); On 2016/11/29 17:25:49, sadrul wrote: > Not a huge fan of the static_cast<>ing. Can PreferencesManager call back into > PCM when it gets destroyed, so it updates |bindings_| instead? This isn't for clearing out the bindings_, the StrongBindingPtr takes care of that for us This is for when shutdown has begun, as PrefChangeRegistrar and other parts of the Profile code can't handle PrefService and the browser process being gone. So I use this to clear out the listeners now. Ideally the Profile code could handle alternative shutdown timings. https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.cc (right): https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:46: setting_preferences_ = true; On 2016/11/29 17:25:49, sadrul wrote: > Move this out of the for loop, and use base::AutoReset<>? Maybe a DCHECK() that > |setting_preferences_| already isn't true when this function is called? Done. https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:60: // will outlive |preferences_change_registrar_| which it owns. On 2016/11/29 17:25:49, sadrul wrote: > This comment is slightly wrong: PCM does not actually manage the lifetime of > this PM, right? Half true now with the wrapping mojo::StrongBindingPtr. Either the pipe dies, cleaning up PM. Or chrome shuts down, and PCM clears the bindings. Updated the comment, as either way PM outlives the PCR. https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:78: return; On 2016/11/29 17:25:49, sadrul wrote: > Should |pref| ever be null? Actually no. We don't subscribe if the pref doesn't exist. https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:81: client_->OnPreferencesChanged(dictionary); On 2016/11/29 17:25:49, sadrul wrote: > If a lot of preferences are updating at the same time, it'd be nice to bundle up > all those updates in the same IPC, instead of a separate message for each pref > change. Maybe things like that don't happen [very often]? It would be nice to reduce the number of IPCs. I don't currently have any metrics on how often they are updated. The Profile code has no batching, but we could do that here. With some post task mechanism. https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.h (right): https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.h:27: // the requested preferences, notifying the client of all changes. On 2016/11/29 17:25:49, sadrul wrote: > If we have a case where multiple clients are observing the same set of > preferences, then it may make sense to rearrange how this works. For example, > instead of having a PreferencesManager instance for each client, we could have a > single PreferencesManager instance that services all clients, and maintains a > {pref => [clients]} map to decide when to notify which clients. But for now, > this should be OK. I went this this 1:1 model so that clients setting a pref wouldn't be notified of that pref. While other clients listening for changes would. We could come up with a model where the client needs to identify itself with setting a pref, but it felt wrong to send the mojom ptr across on each call. https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.h:45: void PreferenceChanged(const std::string& preference_name); On 2016/11/29 17:25:49, sadrul wrote: > Non-override before override. (I seem to remember we used to have a style-guide > entry for this, but can't find it anymore ... so dunno) Done. https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.h:59: } // namespace chrome On 2016/11/29 17:25:49, sadrul wrote: > Do we usually use chrome namespace? So chrome/browser/prefs has one use of chrome, and one of chrome_prefs, then a bunch with no namespace. In general almost 400 examples of it being used. However I don't feel strongly, can remove if desired. https://codereview.chromium.org/2474653003/diff/180001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.cc (right): https://codereview.chromium.org/2474653003/diff/180001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.cc:76: ClearObservers(); On 2016/11/29 17:25:49, sadrul wrote: > Why is this needed? It was a workaround for ValueMapPrefStore enforcing that all observers be removed at shutdown. We could change that. Or require that every observer remove itself. I wanted to avoid that, as it requires carefully planning shutdown order in WmShell.
Sorry about the late review; I have been out for the last few days. lgtm https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.h (right): https://codereview.chromium.org/2474653003/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.h:59: } // namespace chrome On 2016/11/30 01:01:31, jonross wrote: > On 2016/11/29 17:25:49, sadrul wrote: > > Do we usually use chrome namespace? > > So chrome/browser/prefs has one use of chrome, and one of chrome_prefs, then a > bunch with no namespace. > > In general almost 400 examples of it being used. However I don't feel strongly, > can remove if desired. No strong preference either way. Whatever the code in the same place is doing is fine. https://codereview.chromium.org/2474653003/diff/220001/ash/common/wm_shell.cc File ash/common/wm_shell.cc (right): https://codereview.chromium.org/2474653003/diff/220001/ash/common/wm_shell.cc... ash/common/wm_shell.cc:424: DCHECK(!entries.empty()); Maybe actually DCHECK that size() == 1? https://codereview.chromium.org/2474653003/diff/220001/ash/common/wm_shell.h File ash/common/wm_shell.h (right): https://codereview.chromium.org/2474653003/diff/220001/ash/common/wm_shell.h#... ash/common/wm_shell.h:25: class PrefObserverStore; Shouldn't this be in some namespace? https://codereview.chromium.org/2474653003/diff/220001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.h (right): https://codereview.chromium.org/2474653003/diff/220001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.h:32: class PrefObserverStore : public ValueMapPrefStore, Sorry that I missed this earlier, but these should all be in their own namespace ('preferences' probably).
https://codereview.chromium.org/2474653003/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_interface_factory.cc (right): https://codereview.chromium.org/2474653003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_interface_factory.cc:201: registry, main_thread_task_runner_); Oh btw: see if moving this to the IO thread would make anything simpler. If a large part of the prefs code hops into the IO thread for doing actual work, it may make sense to bind this in the IO thread as well.
https://codereview.chromium.org/2474653003/diff/220001/ash/common/wm_shell.cc File ash/common/wm_shell.cc (right): https://codereview.chromium.org/2474653003/diff/220001/ash/common/wm_shell.cc... ash/common/wm_shell.cc:424: DCHECK(!entries.empty()); On 2016/12/06 18:34:29, sadrul (OOO) wrote: > Maybe actually DCHECK that size() == 1? From discussions about a full standalone prefs service, there may end up being more than one entry. https://codereview.chromium.org/2474653003/diff/220001/ash/common/wm_shell.h File ash/common/wm_shell.h (right): https://codereview.chromium.org/2474653003/diff/220001/ash/common/wm_shell.h#... ash/common/wm_shell.h:25: class PrefObserverStore; On 2016/12/06 18:34:29, sadrul (OOO) wrote: > Shouldn't this be in some namespace? Whoops, yeah missed this when moving from components/prefs/ into its own service. Done. https://codereview.chromium.org/2474653003/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_interface_factory.cc (right): https://codereview.chromium.org/2474653003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_interface_factory.cc:201: registry, main_thread_task_runner_); On 2016/12/06 18:35:55, sadrul (OOO) wrote: > Oh btw: see if moving this to the IO thread would make anything simpler. If a > large part of the prefs code hops into the IO thread for doing actual work, it > may make sense to bind this in the IO thread as well. Investigating. https://codereview.chromium.org/2474653003/diff/220001/services/preferences/p... File services/preferences/public/cpp/pref_observer_store.h (right): https://codereview.chromium.org/2474653003/diff/220001/services/preferences/p... services/preferences/public/cpp/pref_observer_store.h:32: class PrefObserverStore : public ValueMapPrefStore, On 2016/12/06 18:34:29, sadrul (OOO) wrote: > Sorry that I missed this earlier, but these should all be in their own namespace > ('preferences' probably). Whoops, yeah missed this when moving from components/prefs/ into its own service. Done.
jonross@chromium.org changed reviewers: + sky@chromium.org
Hey sky@ Could you provide an owners review for the changes in chrome/browser? This change implements the PreferencesManager interface within chrome. It also contains WmShell initializing and connecting to the manager. Along with updates to the preferences mojom based on the review. Thanks, Jon
https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_interface_factory.cc (right): https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_interface_factory.cc:133: preferences_connection_manager_->ProcessRequest(std::move(request)); file-bug-later: It would be nice if naming was consistent here. https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.cc (right): https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:57: bindings_.push_back(binding); I'm surprised you don't need std::move here. https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:65: (static_cast<PreferencesManager*>(it->impl()))->OnProfileDestroyed(); Is there a reason to keep the binding around? Won't doing so be error prone? By that I mean if the profile is destroyed then won't something like SetPreference fail? https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.h (right): https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.h:15: namespace chrome { I wouldn't put this in any namespace (see threads in chromium-dev on namespaces). https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.h:24: class PreferencesConnectionManager { Is the plan that this class is used from places other than chromeos? If not, or if there aren't any near term plans I suggest moving to chromeos. https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.cc (right): https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:46: const base::DictionaryValue& preferences) { Do callers not care about error conditions such as no preference? https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.h (right): https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.h:21: namespace chrome { Same comment about namespaces.
https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.h (right): https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.h:24: class PreferencesConnectionManager { On 2016/12/06 23:37:37, sky wrote: > Is the plan that this class is used from places other than chromeos? If not, or > if there aren't any near term plans I suggest moving to chromeos. This would come down to the timing of a few things - mus enabled on non-chromeos - the full preferences services being its own process - non-ash processes interested in preferences from chrome If we only need to support ash and chromeos until the preferences service is separated, then this can be moved into the chromeos dir https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.cc (right): https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:46: const base::DictionaryValue& preferences) { On 2016/12/06 23:37:37, sky wrote: > Do callers not care about error conditions such as no preference? Current pattern involves chrome enforcing all preferences be registered at startup. Then failing CHECKs if prefs are written to that aren't registered. As for missing preferences, that currently relies on the PrefStore returning false when missing values are accessed. I'd be open to changing this with more explicit error notifications if desired.
https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_interface_factory.cc (right): https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_interface_factory.cc:133: preferences_connection_manager_->ProcessRequest(std::move(request)); On 2016/12/06 23:37:37, sky wrote: > file-bug-later: It would be nice if naming was consistent here. As in unify all naming within this file?
https://codereview.chromium.org/2474653003/diff/220001/ash/common/wm_shell.cc File ash/common/wm_shell.cc (right): https://codereview.chromium.org/2474653003/diff/220001/ash/common/wm_shell.cc... ash/common/wm_shell.cc:424: DCHECK(!entries.empty()); On 2016/12/06 21:09:32, jonross wrote: > On 2016/12/06 18:34:29, sadrul (OOO) wrote: > > Maybe actually DCHECK that size() == 1? > > From discussions about a full standalone prefs service, there may end up being > more than one entry. Indeed. That's why I want this to be a DCHECK(), so that when that does happen, we add the correct logic here to select the right entry, instead of always picking the first one.
https://codereview.chromium.org/2474653003/diff/220001/ash/common/wm_shell.cc File ash/common/wm_shell.cc (right): https://codereview.chromium.org/2474653003/diff/220001/ash/common/wm_shell.cc... ash/common/wm_shell.cc:424: DCHECK(!entries.empty()); On 2016/12/07 04:11:58, sadrul wrote: > On 2016/12/06 21:09:32, jonross wrote: > > On 2016/12/06 18:34:29, sadrul (OOO) wrote: > > > Maybe actually DCHECK that size() == 1? > > > > From discussions about a full standalone prefs service, there may end up being > > more than one entry. > > Indeed. That's why I want this to be a DCHECK(), so that when that does happen, > we add the correct logic here to select the right entry, instead of always > picking the first one. Oh... I follow now, sorry. Done. https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.cc (right): https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:57: bindings_.push_back(binding); On 2016/12/06 23:37:37, sky wrote: > I'm surprised you don't need std::move here. mojo::StrongBindingPtr is actually masking a base::WeakPtr<mojo::StrongBinding<Interface> so I was hitting WeakPtr's copy constructor. I've switched this over to the explicit move. https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:65: (static_cast<PreferencesManager*>(it->impl()))->OnProfileDestroyed(); On 2016/12/06 23:37:37, sky wrote: > Is there a reason to keep the binding around? Won't doing so be error prone? By > that I mean if the profile is destroyed then won't something like SetPreference > fail? I've switched this to just explicitly close the StrongBinding, which will clear out the PreferenceManager. It's cleaner and will prevent any errors like you describe, with SetPreferences arriving mid shutdown. https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.h (right): https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.h:15: namespace chrome { On 2016/12/06 23:37:37, sky wrote: > I wouldn't put this in any namespace (see threads in chromium-dev on > namespaces). I had missed those threads. Removed the namespace. https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.h (right): https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.h:21: namespace chrome { On 2016/12/06 23:37:37, sky wrote: > Same comment about namespaces. Done. https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager_unittest.cc (right): https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager_unittest.cc:306: TEST_F(PreferencesManagerTest, NoNotificationsDuringShutdown) { Redundant as we now explicitly destroy the manager during shutdown.
https://codereview.chromium.org/2474653003/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_interface_factory.cc (right): https://codereview.chromium.org/2474653003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_interface_factory.cc:201: registry, main_thread_task_runner_); On 2016/12/06 21:09:32, jonross wrote: > On 2016/12/06 18:35:55, sadrul (OOO) wrote: > > Oh btw: see if moving this to the IO thread would make anything simpler. If a > > large part of the prefs code hops into the IO thread for doing actual work, it > > may make sense to bind this in the IO thread as well. > > Investigating. The Prefs code saves jumps to the IO thread for the final actual file work. It does it async with posted tasks to a dedicated SequencedTaskRunner within components/prefs/json_pref_store. Since the final PrefStore isn't transparent to code calling into prefs, I wouldn't recommend doing all PreferencesManager work on the IO thread.
https://codereview.chromium.org/2474653003/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_interface_factory.cc (right): https://codereview.chromium.org/2474653003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_interface_factory.cc:201: registry, main_thread_task_runner_); On 2016/12/07 16:08:53, jonross wrote: > On 2016/12/06 21:09:32, jonross wrote: > > On 2016/12/06 18:35:55, sadrul (OOO) wrote: > > > Oh btw: see if moving this to the IO thread would make anything simpler. If > a > > > large part of the prefs code hops into the IO thread for doing actual work, > it > > > may make sense to bind this in the IO thread as well. > > > > Investigating. > > The Prefs code saves jumps to the IO thread for the final actual file work. It > does it async with posted tasks to a dedicated SequencedTaskRunner within > components/prefs/json_pref_store. > > Since the final PrefStore isn't transparent to code calling into prefs, I > wouldn't recommend doing all PreferencesManager work on the IO thread. Sounds good. Thanks for investigating.
https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.h (right): https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.h:24: class PreferencesConnectionManager { On 2016/12/07 00:31:28, jonross wrote: > On 2016/12/06 23:37:37, sky wrote: > > Is the plan that this class is used from places other than chromeos? If not, > or > > if there aren't any near term plans I suggest moving to chromeos. > > This would come down to the timing of a few things > - mus enabled on non-chromeos > - the full preferences services being its own process > - non-ash processes interested in preferences from chrome > > If we only need to support ash and chromeos until the preferences service is > separated, then this can be moved into the chromeos dir If long term we're going to go with this, then leave it here. https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.cc (right): https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:46: const base::DictionaryValue& preferences) { On 2016/12/07 00:31:28, jonross wrote: > On 2016/12/06 23:37:37, sky wrote: > > Do callers not care about error conditions such as no preference? > > Current pattern involves chrome enforcing all preferences be registered at > startup. Then failing CHECKs if prefs are written to that aren't registered. > > As for missing preferences, that currently relies on the PrefStore returning > false when missing values are accessed. > > I'd be open to changing this with more explicit error notifications if desired. My concern is making it easy to diagnose doing the wrong thing vs failing in weird ways later on. A success callback is one way, LOGs are another. LOGs are likely easier as they don't involve callbacks and tracking when the call was made.
https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.cc (right): https://codereview.chromium.org/2474653003/diff/300001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:46: const base::DictionaryValue& preferences) { On 2016/12/07 18:05:16, sky wrote: > On 2016/12/07 00:31:28, jonross wrote: > > On 2016/12/06 23:37:37, sky wrote: > > > Do callers not care about error conditions such as no preference? > > > > Current pattern involves chrome enforcing all preferences be registered at > > startup. Then failing CHECKs if prefs are written to that aren't registered. > > > > As for missing preferences, that currently relies on the PrefStore returning > > false when missing values are accessed. > > > > I'd be open to changing this with more explicit error notifications if > desired. > > My concern is making it easy to diagnose doing the wrong thing vs failing in > weird ways later on. A success callback is one way, LOGs are another. LOGs are > likely easier as they don't involve callbacks and tracking when the call was > made. Went with DLOGs as discussed offline.
LGTM https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.cc (right): https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:60: // Check that the PreferencesManager was not cleared from a connection This description doesn't really match what happens here. Remove it?
https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.cc (right): https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:60: // Check that the PreferencesManager was not cleared from a connection On 2016/12/07 23:54:25, sky wrote: > This description doesn't really match what happens here. Remove it? Maybe tweak the wording? As its possible that arriving here that the PreferencesManager has been cleared. As StrongBindingPtr kills itself upon onConnectionError, so our weakptr here may be invalid.
jonross@chromium.org changed reviewers: + ben@chromium.org
Hey Ben, Could you provide an owners review to services/preferences/ ? Thanks, Jon
https://codereview.chromium.org/2474653003/diff/340001/services/preferences/p... File services/preferences/public/interfaces/preferences.mojom (right): https://codereview.chromium.org/2474653003/diff/340001/services/preferences/p... services/preferences/public/interfaces/preferences.mojom:9: const string kServiceName = "prefs:manager"; This is kCapabilityName, right?
jonross@chromium.org changed reviewers: + dcheng@chromium.org
Hey dcheng@ Could you provide a Security Owners review of: chrome/browser/chrome_content_browser_manifest_overlay.json content/public/app/mojo/content_browser_manifest.json services/preferences/public/interfaces/preferences.mojom I've made an update to the Preferences.mojom API. Then updated the json so that chrome begins providing the PreferencesManager implementation. Thanks, Jon
https://codereview.chromium.org/2474653003/diff/340001/services/preferences/p... File services/preferences/public/interfaces/preferences.mojom (right): https://codereview.chromium.org/2474653003/diff/340001/services/preferences/p... services/preferences/public/interfaces/preferences.mojom:9: const string kServiceName = "prefs:manager"; On 2016/12/08 01:07:22, sadrul wrote: > This is kCapabilityName, right? I'd only seen kServiceName used in mojoms for declaring services/capabilities
https://codereview.chromium.org/2474653003/diff/340001/services/preferences/p... File services/preferences/public/interfaces/preferences.mojom (right): https://codereview.chromium.org/2474653003/diff/340001/services/preferences/p... services/preferences/public/interfaces/preferences.mojom:9: const string kServiceName = "prefs:manager"; On 2016/12/08 14:36:40, jonross wrote: > On 2016/12/08 01:07:22, sadrul wrote: > > This is kCapabilityName, right? > > I'd only seen kServiceName used in mojoms for declaring services/capabilities service name is the actual name of the service you use to connect to the thing, e.g. "ui" or "catalog". In this case, 'prefs:manager' is not the name of the service, it's the name of the capability you use in your manifest, right? I think kCapabilityName would be a better name to clarify that. Are there other places where we use 'service name' to refer to capability?
https://codereview.chromium.org/2474653003/diff/340001/ash/common/wm_shell.cc File ash/common/wm_shell.cc (right): https://codereview.chromium.org/2474653003/diff/340001/ash/common/wm_shell.cc... ash/common/wm_shell.cc:428: // If there are zero entries, this is failing to connect to the service. I'm not sure what this comment means: if the connection failed, do we actually still dispatch the callback? I wouldn't expect that, since Mojo has to make up random values in that case. https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.cc (right): https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:60: // Check that the PreferencesManager was not cleared from a connection On 2016/12/08 01:03:19, jonross wrote: > On 2016/12/07 23:54:25, sky wrote: > > This description doesn't really match what happens here. Remove it? > > Maybe tweak the wording? As its possible that arriving here that the > PreferencesManager has been cleared. As StrongBindingPtr kills itself upon > onConnectionError, so our weakptr here may be invalid. Maybe just "shut down any preference managers that are still live". (It does feel like that the PreferencesManager should just have a pointer back to the Manager, and unregister itself on deletion. It's a bit unusual that |bindings_| can grow without bound) https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.cc (right): https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:36: client_ = std::move(client); Why just a single observer? That feels a bit surprising to me. Is it the case that we'll only ever want one observer?
https://codereview.chromium.org/2474653003/diff/340001/ash/common/wm_shell.cc File ash/common/wm_shell.cc (right): https://codereview.chromium.org/2474653003/diff/340001/ash/common/wm_shell.cc... ash/common/wm_shell.cc:428: // If there are zero entries, this is failing to connect to the service. On 2016/12/08 23:13:01, dcheng wrote: > I'm not sure what this comment means: if the connection failed, do we actually > still dispatch the callback? I wouldn't expect that, since Mojo has to make up > random values in that case. The Catalog implementation does call this if there are 0 services found which match the request we sent. https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.cc (right): https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:60: // Check that the PreferencesManager was not cleared from a connection On 2016/12/08 23:13:01, dcheng wrote: > On 2016/12/08 01:03:19, jonross wrote: > > On 2016/12/07 23:54:25, sky wrote: > > > This description doesn't really match what happens here. Remove it? > > > > Maybe tweak the wording? As its possible that arriving here that the > > PreferencesManager has been cleared. As StrongBindingPtr kills itself upon > > onConnectionError, so our weakptr here may be invalid. > > Maybe just "shut down any preference managers that are still live". > > (It does feel like that the PreferencesManager should just have a pointer back > to the Manager, and unregister itself on deletion. It's a bit unusual that > |bindings_| can grow without bound) I'll switch to the cleaner doc you suggest. I went with this pattern based on the mojo::InterfacePtrSet. Where we hold onto WeakPtrs, which delete themselves on connection errors. I haven't added a clearing of |bindings_| yet. I expected the size of bindings_ to remain small. One per connecting client process. With rare connection errors, and eventual shutdowns. With most connections being like Ash, for the lifetime of the process. I could add a clearing of |bindings_| similar to mojo::PtrSet::ClearNullPtrs, which runs after each action on the class. https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.cc (right): https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:36: client_ = std::move(client); On 2016/12/08 23:13:01, dcheng wrote: > Why just a single observer? That feels a bit surprising to me. Is it the case > that we'll only ever want one observer? These will end up pairing 1:1 with connecting clients. - We do not want to notify a client of changes which it just requested. - In the cases where the multiple clients want the same pref we still want to notify the others. So a PreferencesManager will handle the connection to a single client, cleaning itself up when the connection is terminated. PreferencesConnectionManager will coordinate the requests from various clients wanting prefs. For example Ash will only connect to Chrome once for preferences. Other processes would also each connect once.
https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.cc (right): https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:60: // Check that the PreferencesManager was not cleared from a connection On 2016/12/08 23:32:55, jonross wrote: > On 2016/12/08 23:13:01, dcheng wrote: > > On 2016/12/08 01:03:19, jonross wrote: > > > On 2016/12/07 23:54:25, sky wrote: > > > > This description doesn't really match what happens here. Remove it? > > > > > > Maybe tweak the wording? As its possible that arriving here that the > > > PreferencesManager has been cleared. As StrongBindingPtr kills itself upon > > > onConnectionError, so our weakptr here may be invalid. > > > > Maybe just "shut down any preference managers that are still live". > > > > (It does feel like that the PreferencesManager should just have a pointer back > > to the Manager, and unregister itself on deletion. It's a bit unusual that > > |bindings_| can grow without bound) > > I'll switch to the cleaner doc you suggest. > > I went with this pattern based on the mojo::InterfacePtrSet. Where we hold onto > WeakPtrs, which delete themselves on connection errors. > > I haven't added a clearing of |bindings_| yet. I expected the size of bindings_ > to remain small. One per connecting client process. With rare connection errors, > and eventual shutdowns. With most connections being like Ash, for the lifetime > of the process. > > I could add a clearing of |bindings_| similar to mojo::PtrSet::ClearNullPtrs, > which runs after each action on the class. It feels a bit inelegant, since dead weak pointers will build up in this vector over time. I agree that it's useful to have an object be strongly bound whenever possible; I just wish there was a less awkward way to write this. https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.cc (right): https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:36: client_ = std::move(client); On 2016/12/08 23:32:55, jonross wrote: > On 2016/12/08 23:13:01, dcheng wrote: > > Why just a single observer? That feels a bit surprising to me. Is it the case > > that we'll only ever want one observer? > > These will end up pairing 1:1 with connecting clients. > - We do not want to notify a client of changes which it just requested. > - In the cases where the multiple clients want the same pref we still want > to notify the others. > > So a PreferencesManager will handle the connection to a single client, cleaning > itself up when the connection is terminated. > PreferencesConnectionManager will coordinate the requests from various clients > wanting prefs. > > For example Ash will only connect to Chrome once for preferences. Other > processes would also each connect once. If that's the case, should this always have exactly 1 client? Things like PreferenceChanged() don't check that client_ is bound. If there's only ever one client, I think it would be better if we could vend this interface out of a factory, and ensure the client is always bound: that way, we don't have to trust the other end, e.g. something like: interface PreferencesManagerFactory { Create(PreferencesObserverPtr) => (PreferencesManager); } (Also as a minor nit, I would then call it a client or delegate; it's custom for it to be possible to be multiple observers, while a client / delegate should have a 1:1 relationship)
https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.cc (right): https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:60: // Check that the PreferencesManager was not cleared from a connection On 2016/12/09 05:26:41, dcheng wrote: > On 2016/12/08 23:32:55, jonross wrote: > > On 2016/12/08 23:13:01, dcheng wrote: > > > On 2016/12/08 01:03:19, jonross wrote: > > > > On 2016/12/07 23:54:25, sky wrote: > > > > > This description doesn't really match what happens here. Remove it? > > > > > > > > Maybe tweak the wording? As its possible that arriving here that the > > > > PreferencesManager has been cleared. As StrongBindingPtr kills itself > upon > > > > onConnectionError, so our weakptr here may be invalid. > > > > > > Maybe just "shut down any preference managers that are still live". > > > > > > (It does feel like that the PreferencesManager should just have a pointer > back > > > to the Manager, and unregister itself on deletion. It's a bit unusual that > > > |bindings_| can grow without bound) > > > > I'll switch to the cleaner doc you suggest. > > > > I went with this pattern based on the mojo::InterfacePtrSet. Where we hold > onto > > WeakPtrs, which delete themselves on connection errors. > > > > I haven't added a clearing of |bindings_| yet. I expected the size of > bindings_ > > to remain small. One per connecting client process. With rare connection > errors, > > and eventual shutdowns. With most connections being like Ash, for the lifetime > > of the process. > > > > I could add a clearing of |bindings_| similar to mojo::PtrSet::ClearNullPtrs, > > which runs after each action on the class. > > It feels a bit inelegant, since dead weak pointers will build up in this vector > over time. I agree that it's useful to have an object be strongly bound whenever > possible; I just wish there was a less awkward way to write this. Yeah I agree if that. I've filed crbug.com/672881 to track a full solution to this. I would like future usage to avoid this problem. I'll update this review by setting a callback on the mojo::StrongBindingPtrs which will then clear them from the container.
Patchset #19 (id:360001) has been deleted
https://codereview.chromium.org/2474653003/diff/340001/services/preferences/p... File services/preferences/public/interfaces/preferences.mojom (right): https://codereview.chromium.org/2474653003/diff/340001/services/preferences/p... services/preferences/public/interfaces/preferences.mojom:9: const string kServiceName = "prefs:manager"; On 2016/12/08 15:33:31, sadrul wrote: > On 2016/12/08 14:36:40, jonross wrote: > > On 2016/12/08 01:07:22, sadrul wrote: > > > This is kCapabilityName, right? > > > > I'd only seen kServiceName used in mojoms for declaring services/capabilities > > service name is the actual name of the service you use to connect to the thing, > e.g. "ui" or "catalog". In this case, 'prefs:manager' is not the name of the > service, it's the name of the capability you use in your manifest, right? I > think kCapabilityName would be a better name to clarify that. Are there other > places where we use 'service name' to refer to capability? There currently is 0 usage of kCapabilityName. Though it does make sense to differentiate between the two styles of connection.
https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.cc (right): https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:36: client_ = std::move(client); On 2016/12/09 05:26:41, dcheng wrote: > On 2016/12/08 23:32:55, jonross wrote: > > On 2016/12/08 23:13:01, dcheng wrote: > > > Why just a single observer? That feels a bit surprising to me. Is it the > case > > > that we'll only ever want one observer? > > > > These will end up pairing 1:1 with connecting clients. > > - We do not want to notify a client of changes which it just requested. > > - In the cases where the multiple clients want the same pref we still want > > to notify the others. > > > > So a PreferencesManager will handle the connection to a single client, > cleaning > > itself up when the connection is terminated. > > PreferencesConnectionManager will coordinate the requests from various clients > > wanting prefs. > > > > For example Ash will only connect to Chrome once for preferences. Other > > processes would also each connect once. > > If that's the case, should this always have exactly 1 client? Things like > PreferenceChanged() don't check that client_ is bound. > > If there's only ever one client, I think it would be better if we could vend > this interface out of a factory, and ensure the client is always bound: that > way, we don't have to trust the other end, e.g. something like: > > interface PreferencesManagerFactory { > Create(PreferencesObserverPtr) => (PreferencesManager); > } > > (Also as a minor nit, I would then call it a client or delegate; it's custom for > it to be possible to be multiple observers, while a client / delegate should > have a 1:1 relationship) I'll look into the factory style
https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.cc (right): https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:36: client_ = std::move(client); On 2016/12/09 20:49:04, jonross wrote: > On 2016/12/09 05:26:41, dcheng wrote: > > On 2016/12/08 23:32:55, jonross wrote: > > > On 2016/12/08 23:13:01, dcheng wrote: > > > > Why just a single observer? That feels a bit surprising to me. Is it the > > case > > > > that we'll only ever want one observer? > > > > > > These will end up pairing 1:1 with connecting clients. > > > - We do not want to notify a client of changes which it just requested. > > > - In the cases where the multiple clients want the same pref we still > want > > > to notify the others. > > > > > > So a PreferencesManager will handle the connection to a single client, > > cleaning > > > itself up when the connection is terminated. > > > PreferencesConnectionManager will coordinate the requests from various > clients > > > wanting prefs. > > > > > > For example Ash will only connect to Chrome once for preferences. Other > > > processes would also each connect once. > > > > If that's the case, should this always have exactly 1 client? Things like > > PreferenceChanged() don't check that client_ is bound. > > > > If there's only ever one client, I think it would be better if we could vend > > this interface out of a factory, and ensure the client is always bound: that > > way, we don't have to trust the other end, e.g. something like: > > > > interface PreferencesManagerFactory { > > Create(PreferencesObserverPtr) => (PreferencesManager); > > } > > > > (Also as a minor nit, I would then call it a client or delegate; it's custom > for > > it to be possible to be multiple observers, while a client / delegate should > > have a 1:1 relationship) > > I'll look into the factory style Similar to the factory pattern (eg media/mojo/interfaces/remoting.mojom) the binding between PreferencesManager and PreferencesObserver is already enforced by being in a StrongBindingPtr. The addition of the onConnectionError handler verifies that. I could further add DCHECKs in this class if desired. However I'm not sure I see any additional advantage of a factory indirection at the mojom level. If the 1:1 ratio of Manager:Observer is more representative of a client/delegate I'd prefer to pursue that as a follow on refactor. That way we can unblock chrome-ash preference communications, and the mojom interface can be refined underneath subsequent work.
On 2016/12/13 01:30:18, jonross wrote: > https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... > File chrome/browser/prefs/preferences_manager.cc (right): > > https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... > chrome/browser/prefs/preferences_manager.cc:36: client_ = std::move(client); > On 2016/12/09 20:49:04, jonross wrote: > > On 2016/12/09 05:26:41, dcheng wrote: > > > On 2016/12/08 23:32:55, jonross wrote: > > > > On 2016/12/08 23:13:01, dcheng wrote: > > > > > Why just a single observer? That feels a bit surprising to me. Is it the > > > case > > > > > that we'll only ever want one observer? > > > > > > > > These will end up pairing 1:1 with connecting clients. > > > > - We do not want to notify a client of changes which it just > requested. > > > > - In the cases where the multiple clients want the same pref we still > > want > > > > to notify the others. > > > > > > > > So a PreferencesManager will handle the connection to a single client, > > > cleaning > > > > itself up when the connection is terminated. > > > > PreferencesConnectionManager will coordinate the requests from various > > clients > > > > wanting prefs. > > > > > > > > For example Ash will only connect to Chrome once for preferences. Other > > > > processes would also each connect once. > > > > > > If that's the case, should this always have exactly 1 client? Things like > > > PreferenceChanged() don't check that client_ is bound. > > > > > > If there's only ever one client, I think it would be better if we could vend > > > this interface out of a factory, and ensure the client is always bound: that > > > way, we don't have to trust the other end, e.g. something like: > > > > > > interface PreferencesManagerFactory { > > > Create(PreferencesObserverPtr) => (PreferencesManager); > > > } > > > > > > (Also as a minor nit, I would then call it a client or delegate; it's custom > > for > > > it to be possible to be multiple observers, while a client / delegate should > > > have a 1:1 relationship) > > > > I'll look into the factory style > > Similar to the factory pattern (eg media/mojo/interfaces/remoting.mojom) the > binding between PreferencesManager and PreferencesObserver is already enforced > by being in a StrongBindingPtr. The addition of the onConnectionError handler > verifies that. I could further add DCHECKs in this class if desired. > > However I'm not sure I see any additional advantage of a factory indirection at > the mojom level. > > If the 1:1 ratio of Manager:Observer is more representative of a client/delegate > I'd prefer to pursue that as a follow on refactor. That way we can unblock > chrome-ash preference communications, and the mojom interface can be refined > underneath subsequent work. The advantage of having a factory is that we can assume certain invariants about the interfaces. We don't have to assume the (untrusted in this case) caller will always set an observer/client. The current code calls through |client_| without checking its bound. While we could certainly check everywhere that |client_| is bound, it'd be better if we simply enforced this binding through a factory interface. This is a bit of an anti-pattern that I'm trying to get rid of.
On 2016/12/13 08:00:54, dcheng wrote: > On 2016/12/13 01:30:18, jonross wrote: > > > https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... > > File chrome/browser/prefs/preferences_manager.cc (right): > > > > > https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... > > chrome/browser/prefs/preferences_manager.cc:36: client_ = std::move(client); > > On 2016/12/09 20:49:04, jonross wrote: > > > On 2016/12/09 05:26:41, dcheng wrote: > > > > On 2016/12/08 23:32:55, jonross wrote: > > > > > On 2016/12/08 23:13:01, dcheng wrote: > > > > > > Why just a single observer? That feels a bit surprising to me. Is it > the > > > > case > > > > > > that we'll only ever want one observer? > > > > > > > > > > These will end up pairing 1:1 with connecting clients. > > > > > - We do not want to notify a client of changes which it just > > requested. > > > > > - In the cases where the multiple clients want the same pref we > still > > > want > > > > > to notify the others. > > > > > > > > > > So a PreferencesManager will handle the connection to a single client, > > > > cleaning > > > > > itself up when the connection is terminated. > > > > > PreferencesConnectionManager will coordinate the requests from various > > > clients > > > > > wanting prefs. > > > > > > > > > > For example Ash will only connect to Chrome once for preferences. Other > > > > > processes would also each connect once. > > > > > > > > If that's the case, should this always have exactly 1 client? Things like > > > > PreferenceChanged() don't check that client_ is bound. > > > > > > > > If there's only ever one client, I think it would be better if we could > vend > > > > this interface out of a factory, and ensure the client is always bound: > that > > > > way, we don't have to trust the other end, e.g. something like: > > > > > > > > interface PreferencesManagerFactory { > > > > Create(PreferencesObserverPtr) => (PreferencesManager); > > > > } > > > > > > > > (Also as a minor nit, I would then call it a client or delegate; it's > custom > > > for > > > > it to be possible to be multiple observers, while a client / delegate > should > > > > have a 1:1 relationship) > > > > > > I'll look into the factory style > > > > Similar to the factory pattern (eg media/mojo/interfaces/remoting.mojom) the > > binding between PreferencesManager and PreferencesObserver is already enforced > > by being in a StrongBindingPtr. The addition of the onConnectionError handler > > verifies that. I could further add DCHECKs in this class if desired. > > > > However I'm not sure I see any additional advantage of a factory indirection > at > > the mojom level. > > > > If the 1:1 ratio of Manager:Observer is more representative of a > client/delegate > > I'd prefer to pursue that as a follow on refactor. That way we can unblock > > chrome-ash preference communications, and the mojom interface can be refined > > underneath subsequent work. > > The advantage of having a factory is that we can assume certain invariants about > the interfaces. We don't have to assume the (untrusted in this case) caller will > always set an observer/client. The current code calls through |client_| without > checking its bound. > > While we could certainly check everywhere that |client_| is bound, it'd be > better if we simply enforced this binding through a factory interface. This is a > bit of an anti-pattern that I'm trying to get rid of. In the current implementation it is not possible for the caller to perform other actions until after set as an observer/client. The client impl enforces this. However I can see a desire to enforce this at an interface level in the general case. As there could be different untrusted impls. However the manager could equally just reject such calls. Looking into the interface Factory pattern, this comes across as too much of a burden for each client. As now each client has to establish multiple mojo connections for a single capability. This comes across as something that should be abstracted away at the Connector api level. Eg: ConnectToInterface(const std::String& name, mojo::InterfacePtr<T> source, mojo::InterfacePtr<Interface>* ptr); Since there are going to be a significant number of chrome-ash two-way communication, I would be concerned if each mojom has to fully establish this pattern. If the desire is to eliminate the current anti-pattern I'd prefer to see this addressed at the service_manager level.
On 2016/12/13 15:06:24, jonross wrote: > On 2016/12/13 08:00:54, dcheng wrote: > > On 2016/12/13 01:30:18, jonross wrote: > > > > > > https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... > > > File chrome/browser/prefs/preferences_manager.cc (right): > > > > > > > > > https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... > > > chrome/browser/prefs/preferences_manager.cc:36: client_ = std::move(client); > > > On 2016/12/09 20:49:04, jonross wrote: > > > > On 2016/12/09 05:26:41, dcheng wrote: > > > > > On 2016/12/08 23:32:55, jonross wrote: > > > > > > On 2016/12/08 23:13:01, dcheng wrote: > > > > > > > Why just a single observer? That feels a bit surprising to me. Is it > > the > > > > > case > > > > > > > that we'll only ever want one observer? > > > > > > > > > > > > These will end up pairing 1:1 with connecting clients. > > > > > > - We do not want to notify a client of changes which it just > > > requested. > > > > > > - In the cases where the multiple clients want the same pref we > > still > > > > want > > > > > > to notify the others. > > > > > > > > > > > > So a PreferencesManager will handle the connection to a single client, > > > > > cleaning > > > > > > itself up when the connection is terminated. > > > > > > PreferencesConnectionManager will coordinate the requests from various > > > > clients > > > > > > wanting prefs. > > > > > > > > > > > > For example Ash will only connect to Chrome once for preferences. > Other > > > > > > processes would also each connect once. > > > > > > > > > > If that's the case, should this always have exactly 1 client? Things > like > > > > > PreferenceChanged() don't check that client_ is bound. > > > > > > > > > > If there's only ever one client, I think it would be better if we could > > vend > > > > > this interface out of a factory, and ensure the client is always bound: > > that > > > > > way, we don't have to trust the other end, e.g. something like: > > > > > > > > > > interface PreferencesManagerFactory { > > > > > Create(PreferencesObserverPtr) => (PreferencesManager); > > > > > } > > > > > > > > > > (Also as a minor nit, I would then call it a client or delegate; it's > > custom > > > > for > > > > > it to be possible to be multiple observers, while a client / delegate > > should > > > > > have a 1:1 relationship) > > > > > > > > I'll look into the factory style > > > > > > Similar to the factory pattern (eg media/mojo/interfaces/remoting.mojom) the > > > binding between PreferencesManager and PreferencesObserver is already > enforced > > > by being in a StrongBindingPtr. The addition of the onConnectionError > handler > > > verifies that. I could further add DCHECKs in this class if desired. > > > > > > However I'm not sure I see any additional advantage of a factory indirection > > at > > > the mojom level. > > > > > > If the 1:1 ratio of Manager:Observer is more representative of a > > client/delegate > > > I'd prefer to pursue that as a follow on refactor. That way we can unblock > > > chrome-ash preference communications, and the mojom interface can be refined > > > underneath subsequent work. > > > > The advantage of having a factory is that we can assume certain invariants > about > > the interfaces. We don't have to assume the (untrusted in this case) caller > will > > always set an observer/client. The current code calls through |client_| > without > > checking its bound. > > > > While we could certainly check everywhere that |client_| is bound, it'd be > > better if we simply enforced this binding through a factory interface. This is > a > > bit of an anti-pattern that I'm trying to get rid of. > > In the current implementation it is not possible for the caller to perform other > actions until after set as an observer/client. The client impl enforces this. Right, the client impl shouldn't have to enforce this. it leads to more complex and harder to audit code. I'm also not sure how this is enforced: what prevents PreferenceChanged() from being called when client_ is not yet bound? > > However I can see a desire to enforce this at an interface level in the general > case. As there could be different untrusted impls. However the manager could > equally just reject such calls. > > Looking into the interface Factory pattern, this comes across as too much of a > burden for each client. As now each client has to establish multiple mojo > connections for a single capability. This comes across as something that should > be abstracted away at the Connector api level. > > Eg: ConnectToInterface(const std::String& name, mojo::InterfacePtr<T> source, > mojo::InterfacePtr<Interface>* ptr); > > Since there are going to be a significant number of chrome-ash two-way > communication, I would be concerned if each mojom has to fully establish this > pattern. If the desire is to eliminate the current anti-pattern I'd prefer to > see this addressed at the service_manager level. This is a fair concern, and I'll start a chromium-mojo thread about this (webnfc feels similarly): I assume the concern here is that: 1) each mojo interface that requires this pattern needs a factory interface somewhere 2) it makes connecting to the interface more complex, because you have to connect to the factory interface first 3) therefore, we should just have this handled by service manager, somehow Is that an accurate summary?
On 2016/12/13 18:32:01, dcheng wrote: > On 2016/12/13 15:06:24, jonross wrote: > > On 2016/12/13 08:00:54, dcheng wrote: > > > On 2016/12/13 01:30:18, jonross wrote: > > > > > > > > > > https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... > > > > File chrome/browser/prefs/preferences_manager.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... > > > > chrome/browser/prefs/preferences_manager.cc:36: client_ = > std::move(client); > > > > On 2016/12/09 20:49:04, jonross wrote: > > > > > On 2016/12/09 05:26:41, dcheng wrote: > > > > > > On 2016/12/08 23:32:55, jonross wrote: > > > > > > > On 2016/12/08 23:13:01, dcheng wrote: > > > > > > > > Why just a single observer? That feels a bit surprising to me. Is > it > > > the > > > > > > case > > > > > > > > that we'll only ever want one observer? > > > > > > > > > > > > > > These will end up pairing 1:1 with connecting clients. > > > > > > > - We do not want to notify a client of changes which it just > > > > requested. > > > > > > > - In the cases where the multiple clients want the same pref we > > > still > > > > > want > > > > > > > to notify the others. > > > > > > > > > > > > > > So a PreferencesManager will handle the connection to a single > client, > > > > > > cleaning > > > > > > > itself up when the connection is terminated. > > > > > > > PreferencesConnectionManager will coordinate the requests from > various > > > > > clients > > > > > > > wanting prefs. > > > > > > > > > > > > > > For example Ash will only connect to Chrome once for preferences. > > Other > > > > > > > processes would also each connect once. > > > > > > > > > > > > If that's the case, should this always have exactly 1 client? Things > > like > > > > > > PreferenceChanged() don't check that client_ is bound. > > > > > > > > > > > > If there's only ever one client, I think it would be better if we > could > > > vend > > > > > > this interface out of a factory, and ensure the client is always > bound: > > > that > > > > > > way, we don't have to trust the other end, e.g. something like: > > > > > > > > > > > > interface PreferencesManagerFactory { > > > > > > Create(PreferencesObserverPtr) => (PreferencesManager); > > > > > > } > > > > > > > > > > > > (Also as a minor nit, I would then call it a client or delegate; it's > > > custom > > > > > for > > > > > > it to be possible to be multiple observers, while a client / delegate > > > should > > > > > > have a 1:1 relationship) > > > > > > > > > > I'll look into the factory style > > > > > > > > Similar to the factory pattern (eg media/mojo/interfaces/remoting.mojom) > the > > > > binding between PreferencesManager and PreferencesObserver is already > > enforced > > > > by being in a StrongBindingPtr. The addition of the onConnectionError > > handler > > > > verifies that. I could further add DCHECKs in this class if desired. > > > > > > > > However I'm not sure I see any additional advantage of a factory > indirection > > > at > > > > the mojom level. > > > > > > > > If the 1:1 ratio of Manager:Observer is more representative of a > > > client/delegate > > > > I'd prefer to pursue that as a follow on refactor. That way we can unblock > > > > chrome-ash preference communications, and the mojom interface can be > refined > > > > underneath subsequent work. > > > > > > The advantage of having a factory is that we can assume certain invariants > > about > > > the interfaces. We don't have to assume the (untrusted in this case) caller > > will > > > always set an observer/client. The current code calls through |client_| > > without > > > checking its bound. > > > > > > While we could certainly check everywhere that |client_| is bound, it'd be > > > better if we simply enforced this binding through a factory interface. This > is > > a > > > bit of an anti-pattern that I'm trying to get rid of. > > > > In the current implementation it is not possible for the caller to perform > other > > actions until after set as an observer/client. The client impl enforces this. > > Right, the client impl shouldn't have to enforce this. it leads to more complex > and harder to audit code. > I'm also not sure how this is enforced: what prevents PreferenceChanged() from > being called when client_ is not yet bound? In the Manager impl, PreferencesChanged is a private callback method. It is never registered as a callback to PrefChangeRegistrar until the client requests to subscribe. The PrefStoreObserver adds itself as an observer before triggering the subscription process. So the binding will occur before the manager ever tries to call anything on the client. On the client side, other calls into the Manager are rejected until the subscription is setup, and the initial values are received. So as it stand right now, until the binding is complete, the two sides prevent requests which would act on an unbound client. > > > > > However I can see a desire to enforce this at an interface level in the > general > > case. As there could be different untrusted impls. However the manager could > > equally just reject such calls. > > > > Looking into the interface Factory pattern, this comes across as too much of a > > burden for each client. As now each client has to establish multiple mojo > > connections for a single capability. This comes across as something that > should > > be abstracted away at the Connector api level. > > > > Eg: ConnectToInterface(const std::String& name, mojo::InterfacePtr<T> source, > > mojo::InterfacePtr<Interface>* ptr); > > > > Since there are going to be a significant number of chrome-ash two-way > > communication, I would be concerned if each mojom has to fully establish this > > pattern. If the desire is to eliminate the current anti-pattern I'd prefer to > > see this addressed at the service_manager level. > > This is a fair concern, and I'll start a chromium-mojo thread about this (webnfc > feels similarly): I assume the concern here is that: > 1) each mojo interface that requires this pattern needs a factory interface > somewhere > 2) it makes connecting to the interface more complex, because you have to > connect to the factory interface first > 3) therefore, we should just have this handled by service manager, somehow > > Is that an accurate summary? That summary is accurate. Could you CC me on that thread?
On 2016/12/13 18:42:55, jonross wrote: > On 2016/12/13 18:32:01, dcheng wrote: > > On 2016/12/13 15:06:24, jonross wrote: > > > On 2016/12/13 08:00:54, dcheng wrote: > > > > On 2016/12/13 01:30:18, jonross wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... > > > > > File chrome/browser/prefs/preferences_manager.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2474653003/diff/340001/chrome/browser/prefs/p... > > > > > chrome/browser/prefs/preferences_manager.cc:36: client_ = > > std::move(client); > > > > > On 2016/12/09 20:49:04, jonross wrote: > > > > > > On 2016/12/09 05:26:41, dcheng wrote: > > > > > > > On 2016/12/08 23:32:55, jonross wrote: > > > > > > > > On 2016/12/08 23:13:01, dcheng wrote: > > > > > > > > > Why just a single observer? That feels a bit surprising to me. > Is > > it > > > > the > > > > > > > case > > > > > > > > > that we'll only ever want one observer? > > > > > > > > > > > > > > > > These will end up pairing 1:1 with connecting clients. > > > > > > > > - We do not want to notify a client of changes which it just > > > > > requested. > > > > > > > > - In the cases where the multiple clients want the same pref > we > > > > still > > > > > > want > > > > > > > > to notify the others. > > > > > > > > > > > > > > > > So a PreferencesManager will handle the connection to a single > > client, > > > > > > > cleaning > > > > > > > > itself up when the connection is terminated. > > > > > > > > PreferencesConnectionManager will coordinate the requests from > > various > > > > > > clients > > > > > > > > wanting prefs. > > > > > > > > > > > > > > > > For example Ash will only connect to Chrome once for preferences. > > > Other > > > > > > > > processes would also each connect once. > > > > > > > > > > > > > > If that's the case, should this always have exactly 1 client? Things > > > like > > > > > > > PreferenceChanged() don't check that client_ is bound. > > > > > > > > > > > > > > If there's only ever one client, I think it would be better if we > > could > > > > vend > > > > > > > this interface out of a factory, and ensure the client is always > > bound: > > > > that > > > > > > > way, we don't have to trust the other end, e.g. something like: > > > > > > > > > > > > > > interface PreferencesManagerFactory { > > > > > > > Create(PreferencesObserverPtr) => (PreferencesManager); > > > > > > > } > > > > > > > > > > > > > > (Also as a minor nit, I would then call it a client or delegate; > it's > > > > custom > > > > > > for > > > > > > > it to be possible to be multiple observers, while a client / > delegate > > > > should > > > > > > > have a 1:1 relationship) > > > > > > > > > > > > I'll look into the factory style > > > > > > > > > > Similar to the factory pattern (eg media/mojo/interfaces/remoting.mojom) > > the > > > > > binding between PreferencesManager and PreferencesObserver is already > > > enforced > > > > > by being in a StrongBindingPtr. The addition of the onConnectionError > > > handler > > > > > verifies that. I could further add DCHECKs in this class if desired. > > > > > > > > > > However I'm not sure I see any additional advantage of a factory > > indirection > > > > at > > > > > the mojom level. > > > > > > > > > > If the 1:1 ratio of Manager:Observer is more representative of a > > > > client/delegate > > > > > I'd prefer to pursue that as a follow on refactor. That way we can > unblock > > > > > chrome-ash preference communications, and the mojom interface can be > > refined > > > > > underneath subsequent work. > > > > > > > > The advantage of having a factory is that we can assume certain invariants > > > about > > > > the interfaces. We don't have to assume the (untrusted in this case) > caller > > > will > > > > always set an observer/client. The current code calls through |client_| > > > without > > > > checking its bound. > > > > > > > > While we could certainly check everywhere that |client_| is bound, it'd be > > > > better if we simply enforced this binding through a factory interface. > This > > is > > > a > > > > bit of an anti-pattern that I'm trying to get rid of. > > > > > > In the current implementation it is not possible for the caller to perform > > other > > > actions until after set as an observer/client. The client impl enforces > this. > > > > Right, the client impl shouldn't have to enforce this. it leads to more > complex > > and harder to audit code. > > I'm also not sure how this is enforced: what prevents PreferenceChanged() from > > being called when client_ is not yet bound? > > In the Manager impl, PreferencesChanged is a private callback method. It is > never registered as a callback to PrefChangeRegistrar until the client requests > to subscribe. > The PrefStoreObserver adds itself as an observer before triggering the > subscription process. So the binding will occur before the manager ever tries to > call anything on the client. > > On the client side, other calls into the Manager are rejected until the > subscription is setup, and the initial values are received. We can't depend on a client to do this though. A client running in a compromised process can simply ignore all these preconditions. > > So as it stand right now, until the binding is complete, the two sides prevent > requests which would act on an unbound client. > > > > > > > > > However I can see a desire to enforce this at an interface level in the > > general > > > case. As there could be different untrusted impls. However the manager could > > > equally just reject such calls. > > > > > > Looking into the interface Factory pattern, this comes across as too much of > a > > > burden for each client. As now each client has to establish multiple mojo > > > connections for a single capability. This comes across as something that > > should > > > be abstracted away at the Connector api level. > > > > > > Eg: ConnectToInterface(const std::String& name, mojo::InterfacePtr<T> > source, > > > mojo::InterfacePtr<Interface>* ptr); > > > > > > Since there are going to be a significant number of chrome-ash two-way > > > communication, I would be concerned if each mojom has to fully establish > this > > > pattern. If the desire is to eliminate the current anti-pattern I'd prefer > to > > > see this addressed at the service_manager level. > > > > This is a fair concern, and I'll start a chromium-mojo thread about this > (webnfc > > feels similarly): I assume the concern here is that: > > 1) each mojo interface that requires this pattern needs a factory interface > > somewhere > > 2) it makes connecting to the interface more complex, because you have to > > connect to the factory interface first > > 3) therefore, we should just have this handled by service manager, somehow > > > > Is that an accurate summary? > > That summary is accurate. Could you CC me on that thread?
I've added checks in PreferencesManager to check that client is bound. I've filed crbug.com/674140 to track updating PreferencesManager to enforce a two-way binding at creation time. Once there is an available mechanism to do this. Such as an update to Connector. I've filed crbug.com/674138 to track renaming observer to client in a followup. https://codereview.chromium.org/2474653003/diff/340001/services/preferences/p... File services/preferences/public/interfaces/preferences.mojom (right): https://codereview.chromium.org/2474653003/diff/340001/services/preferences/p... services/preferences/public/interfaces/preferences.mojom:9: const string kServiceName = "prefs:manager"; On 2016/12/09 20:48:03, jonross wrote: > On 2016/12/08 15:33:31, sadrul wrote: > > On 2016/12/08 14:36:40, jonross wrote: > > > On 2016/12/08 01:07:22, sadrul wrote: > > > > This is kCapabilityName, right? > > > > > > I'd only seen kServiceName used in mojoms for declaring > services/capabilities > > > > service name is the actual name of the service you use to connect to the > thing, > > e.g. "ui" or "catalog". In this case, 'prefs:manager' is not the name of the > > service, it's the name of the capability you use in your manifest, right? I > > think kCapabilityName would be a better name to clarify that. Are there other > > places where we use 'service name' to refer to capability? > > There currently is 0 usage of kCapabilityName. Though it does make sense to > differentiate between the two styles of connection. Done.
On 2016/12/14 15:01:41, jonross wrote: > I've added checks in PreferencesManager to check that client is bound. > > I've filed crbug.com/674140 to track updating PreferencesManager to enforce a > two-way binding at creation time. Once there is an available mechanism to do > this. Such as an update to Connector. > > I've filed crbug.com/674138 to track renaming observer to client in a followup. > > https://codereview.chromium.org/2474653003/diff/340001/services/preferences/p... > File services/preferences/public/interfaces/preferences.mojom (right): > > https://codereview.chromium.org/2474653003/diff/340001/services/preferences/p... > services/preferences/public/interfaces/preferences.mojom:9: const string > kServiceName = "prefs:manager"; > On 2016/12/09 20:48:03, jonross wrote: > > On 2016/12/08 15:33:31, sadrul wrote: > > > On 2016/12/08 14:36:40, jonross wrote: > > > > On 2016/12/08 01:07:22, sadrul wrote: > > > > > This is kCapabilityName, right? > > > > > > > > I'd only seen kServiceName used in mojoms for declaring > > services/capabilities > > > > > > service name is the actual name of the service you use to connect to the > > thing, > > > e.g. "ui" or "catalog". In this case, 'prefs:manager' is not the name of the > > > service, it's the name of the capability you use in your manifest, right? I > > > think kCapabilityName would be a better name to clarify that. Are there > other > > > places where we use 'service name' to refer to capability? > > > > There currently is 0 usage of kCapabilityName. Though it does make sense to > > differentiate between the two styles of connection. > > Done. I have started a chromium-mojo thread about two-way communication and factory interfaces. https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/j-kiK17V-Qc In order to unblock other work, I would like to de-scope that resolution from this code review. I have filed bugs to track the needed future updates to this mojom interface. dcheng@ could you PTAL?
> In order to unblock other work, I would like to de-scope that resolution from > this code review. I have filed bugs to track the needed future updates to this > mojom interface. Thanks for sending the email. Just two small comments. https://codereview.chromium.org/2474653003/diff/400001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.cc (right): https://codereview.chromium.org/2474653003/diff/400001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:67: while (it != std::end(bindings_)) { I don't think the iterator is advanced here (it's probably more common to write this as a for loop, but I don't feel strongly about that) https://codereview.chromium.org/2474653003/diff/400001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.cc (right): https://codereview.chromium.org/2474653003/diff/400001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:44: DCHECK(client_.is_bound()); This should be an if (...) with an early return.
Patchset #21 (id:420001) has been deleted
https://codereview.chromium.org/2474653003/diff/400001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_connection_manager.cc (right): https://codereview.chromium.org/2474653003/diff/400001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_connection_manager.cc:67: while (it != std::end(bindings_)) { On 2016/12/15 16:17:08, dcheng wrote: > I don't think the iterator is advanced here (it's probably more common to write > this as a for loop, but I don't feel strongly about that) Done. https://codereview.chromium.org/2474653003/diff/400001/chrome/browser/prefs/p... File chrome/browser/prefs/preferences_manager.cc (right): https://codereview.chromium.org/2474653003/diff/400001/chrome/browser/prefs/p... chrome/browser/prefs/preferences_manager.cc:44: DCHECK(client_.is_bound()); On 2016/12/15 16:17:08, dcheng wrote: > This should be an if (...) with an early return. Done.
mojo lgtm, thanks.
https://codereview.chromium.org/2474653003/diff/440001/ash/common/wm_shell.cc File ash/common/wm_shell.cc (right): https://codereview.chromium.org/2474653003/diff/440001/ash/common/wm_shell.cc... ash/common/wm_shell.cc:281: prefs::mojom::kCapabilityName, I understand how this mechanism may have been useful for IME, but it feels wrong for preferences (*). Is it possible to treat the preferences code in //chrome as an embedded service? See https://chromium.googlesource.com/chromium/src/+/master/content/public/browse... which can be implemented from chrome. That means this code can just do: delegate_->GetShellConnector()->ConnectToInterface(prefs::mojom::kServiceName, &prefs_); which feels more directionally correct. (* reasoning: there's only ever likely to be one preferences subsystem. I can't say the same for IME.)
Patchset #23 (id:480001) has been deleted
https://codereview.chromium.org/2474653003/diff/440001/ash/common/wm_shell.cc File ash/common/wm_shell.cc (right): https://codereview.chromium.org/2474653003/diff/440001/ash/common/wm_shell.cc... ash/common/wm_shell.cc:281: prefs::mojom::kCapabilityName, On 2016/12/19 20:41:55, Ben Goodger (Google) wrote: > I understand how this mechanism may have been useful for IME, but it feels wrong > for preferences (*). Is it possible to treat the preferences code in //chrome as > an embedded service? See > https://chromium.googlesource.com/chromium/src/+/master/content/public/browse... > which can be implemented from chrome. That means this code can just do: > > delegate_->GetShellConnector()->ConnectToInterface(prefs::mojom::kServiceName, > &prefs_); > > which feels more directionally correct. > > > (* reasoning: there's only ever likely to be one preferences subsystem. I can't > say the same for IME.) Done.
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, sky@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2474653003/#ps500001 (title: "Switch to Embedded Service")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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, sky@chromium.org, dcheng@chromium.org, ben@chromium.org Link to the patchset: https://codereview.chromium.org/2474653003/#ps520001 (title: "Rebase")
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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, sky@chromium.org, dcheng@chromium.org, ben@chromium.org Link to the patchset: https://codereview.chromium.org/2474653003/#ps540001 (title: "Rebase missed a changed method")
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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, sky@chromium.org, dcheng@chromium.org, ben@chromium.org Link to the patchset: https://codereview.chromium.org/2474653003/#ps560001 (title: "Unrelated test static breaking PrefConnectionM")
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jonross@chromium.org
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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, sky@chromium.org, dcheng@chromium.org, ben@chromium.org Link to the patchset: https://codereview.chromium.org/2474653003/#ps580001 (title: "Rebase")
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 jonross@chromium.org
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, sky@chromium.org, dcheng@chromium.org, ben@chromium.org Link to the patchset: https://codereview.chromium.org/2474653003/#ps600001 (title: "Update PreferencesManager to account for base::Value API change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 600001, "attempt_start_ts": 1482333288687860,
"parent_rev": "e5baff695d7475b2cb9c57cbaad70c90242c67a8", "commit_rev":
"eb326659320841ef487790922c4f4f462fc9559e"}
Message was sent while issue was closed.
Description was changed from ========== Chrome implementation of prefs::mojom::PreferencesManager, which connects to the current Profile and subscribes to preference changes when requested to by a prefs::mojom::PreferencesObserver. Also includes a connection manager to handle mapping incoming requests to their own preferences manager. TEST=manual testing with example code, PreferencesManagerTest BUG=622347 ========== to ========== Chrome implementation of prefs::mojom::PreferencesManager, which connects to the current Profile and subscribes to preference changes when requested to by a prefs::mojom::PreferencesObserver. Also includes a connection manager to handle mapping incoming requests to their own preferences manager. TEST=manual testing with example code, PreferencesManagerTest BUG=622347 Review-Url: https://codereview.chromium.org/2474653003 ==========
Message was sent while issue was closed.
Committed patchset #28 (id:600001)
Message was sent while issue was closed.
Description was changed from ========== Chrome implementation of prefs::mojom::PreferencesManager, which connects to the current Profile and subscribes to preference changes when requested to by a prefs::mojom::PreferencesObserver. Also includes a connection manager to handle mapping incoming requests to their own preferences manager. TEST=manual testing with example code, PreferencesManagerTest BUG=622347 Review-Url: https://codereview.chromium.org/2474653003 ========== to ========== Chrome implementation of prefs::mojom::PreferencesManager, which connects to the current Profile and subscribes to preference changes when requested to by a prefs::mojom::PreferencesObserver. Also includes a connection manager to handle mapping incoming requests to their own preferences manager. TEST=manual testing with example code, PreferencesManagerTest BUG=622347 Committed: https://crrev.com/3b1b258d4e59022e8a931f965e800427618894c4 Cr-Commit-Position: refs/heads/master@{#440118} ==========
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/3b1b258d4e59022e8a931f965e800427618894c4 Cr-Commit-Position: refs/heads/master@{#440118} |
