Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
428143002:
Sync: Add a SyncTypePreferenceProvider interface. (Closed)
|
Created:
6 years, 4 months ago by Marc Treib Modified:
6 years, 4 months ago CC:
chromium-reviews, tim+watch_chromium.org, haitaol+watch_chromium.org, pam+watch_chromium.org, zea+watch_chromium.org, maniscalco+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionSync: Add a SyncTypePreferenceProvider interface to specify sync types to enable. The PSS evaluates these.
For now, this is used by the SupervisedUserService to specify data types that must always be enabled.
BUG=395105
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288951
Patch Set 1 #
Total comments: 15
Patch Set 2 : comment #
Total comments: 2
Patch Set 3 : rename and cleanup #Patch Set 4 : rename fix #
Total comments: 2
Patch Set 5 : move call #Patch Set 6 : fix #Patch Set 7 : rebase #
Total comments: 4
Patch Set 8 : Bernhard's comment, and const cleanup #Patch Set 9 : fix tests #
Messages
Total messages: 34 (0 generated)
bauerb@chromium.org: Please review changes in c/b/supervised_user. zea@chromium.org: Please review changes in c/b/sync.
https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service.cc (right): https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service.cc:2514: // TODO(treib): Resolve pref groups? It'd be easiest to expose SyncPrefs::ResolvePrefGroups and explicitly call that here. Would that be okay, or does anyone have a better idea? https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service.h (right): https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service.h:928: syncer::ModelTypeSet GetRequiredDataTypes(); Might have to make this public, so the UI can adapt accordingly (e.g. disable checkboxes in the sync config overlay).
https://codereview.chromium.org/428143002/diff/1/chrome/browser/supervised_us... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/428143002/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/supervised_user_service.cc:327: if (!ProfileIsSupervised()) If we only add ourselves as a data type provider if the profile is supervised, this method should also only be called if the profile is supervised. DCHECK it? Actually, we could always register the data type provider. That way we have less divergence between supervised and unsupervised users. https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service.cc (right): https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service.cc:2305: data_type_providers_.remove(provider); DCHECK the result? https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service.cc:2509: // because the provider might remove itself from the list in the callback. I think an easier solution here would be to require the data type provider not to remove themselves from the list while it's being called back. No need to make our lives harder than necessary :) https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service.cc:2514: // TODO(treib): Resolve pref groups? On 2014/07/30 11:42:50, Marc wrote: > It'd be easiest to expose SyncPrefs::ResolvePrefGroups and explicitly call that > here. Would that be okay, or does anyone have a better idea? Do we actually need that? Required data types could include their own dependencies.
https://codereview.chromium.org/428143002/diff/1/chrome/browser/supervised_us... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/428143002/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/supervised_user_service.cc:327: if (!ProfileIsSupervised()) On 2014/07/30 12:38:51, Bernhard Bauer wrote: > If we only add ourselves as a data type provider if the profile is supervised, > this method should also only be called if the profile is supervised. DCHECK it? > > Actually, we could always register the data type provider. That way we have less > divergence between supervised and unsupervised users. Alright, I'm registering/removing the provider in Init()/Shutdown() now. https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service.cc (right): https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service.cc:2305: data_type_providers_.remove(provider); On 2014/07/30 12:38:51, Bernhard Bauer wrote: > DCHECK the result? There is no result :P We could DCHECK(HasDataTypeProvider(..)) similar to above, if RemoveDataTypeProvider for a non-registered value should be forbidden. Here I've mirrored the behavior of ObserverLists, which do allow this. https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service.cc:2509: // because the provider might remove itself from the list in the callback. On 2014/07/30 12:38:51, Bernhard Bauer wrote: > I think an easier solution here would be to require the data type provider not > to remove themselves from the list while it's being called back. No need to make > our lives harder than necessary :) I guess we could. OTOH, this is a fairly common pattern when iterating over STL containers and I don't mind it. https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service.cc:2514: // TODO(treib): Resolve pref groups? On 2014/07/30 12:38:51, Bernhard Bauer wrote: > On 2014/07/30 11:42:50, Marc wrote: > > It'd be easiest to expose SyncPrefs::ResolvePrefGroups and explicitly call > that > > here. Would that be okay, or does anyone have a better idea? > > Do we actually need that? Required data types could include their own > dependencies. We could also specify the types explicitly, but I think that would often result in duplicating the pref groups. (e.g. APPS implies APP_SETTINGS, APP_NOTIFICATIONS and APP_LIST. Do we really want to duplicate this in our code?)
https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service.cc (right): https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service.cc:2305: data_type_providers_.remove(provider); On 2014/07/30 13:10:39, Marc Treib wrote: > On 2014/07/30 12:38:51, Bernhard Bauer wrote: > > DCHECK the result? > > There is no result :P > We could DCHECK(HasDataTypeProvider(..)) similar to above, if > RemoveDataTypeProvider for a non-registered value should be forbidden. Here I've > mirrored the behavior of ObserverLists, which do allow this. Yeah, I meant checking whether there is one registered. It just seems weird to disallow adding a data type provider twice, but allow removing it twice. https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service.cc:2509: // because the provider might remove itself from the list in the callback. On 2014/07/30 13:10:39, Marc Treib wrote: > On 2014/07/30 12:38:51, Bernhard Bauer wrote: > > I think an easier solution here would be to require the data type provider not > > to remove themselves from the list while it's being called back. No need to > make > > our lives harder than necessary :) > > I guess we could. OTOH, this is a fairly common pattern when iterating over STL > containers and I don't mind it. Not as common as a standard for loop ;-) If you keep it this way, what if the callback removes the *next* data type observer from the list? Basically, if you want to handle all the edge cases, you're going to end up reimplementing ObserverList, for a use case that doesn't even currently exist. https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service.cc:2514: // TODO(treib): Resolve pref groups? On 2014/07/30 13:10:39, Marc Treib wrote: > On 2014/07/30 12:38:51, Bernhard Bauer wrote: > > On 2014/07/30 11:42:50, Marc wrote: > > > It'd be easiest to expose SyncPrefs::ResolvePrefGroups and explicitly call > > that > > > here. Would that be okay, or does anyone have a better idea? > > > > Do we actually need that? Required data types could include their own > > dependencies. > > We could also specify the types explicitly, but I think that would often result > in duplicating the pref groups. (e.g. APPS implies APP_SETTINGS, > APP_NOTIFICATIONS and APP_LIST. Do we really want to duplicate this in our > code?) At the very least we would need to change the concept of "pref groups", as these data types here are not enabled by user choice.
https://codereview.chromium.org/428143002/diff/20001/chrome/browser/sync/prof... File chrome/browser/sync/profile_sync_service_data_type_provider.h (right): https://codereview.chromium.org/428143002/diff/20001/chrome/browser/sync/prof... chrome/browser/sync/profile_sync_service_data_type_provider.h:12: virtual syncer::ModelTypeSet GetRequiredSyncDataTypes() = 0; This is effectively encapsulating the notion of "Datatype A requires Datatype B" right? I wonder if it makes more sense to have this be part of the DataTypeController, rather than a new class just for this? In other words, DataTypeController could have a GetDependentTypes() method or something, wherein if type A is enabled (due to preferences or something else), then all of its dependent types would also be required. This kind of logic might even be able to eventually replace pref groups.
https://codereview.chromium.org/428143002/diff/20001/chrome/browser/sync/prof... File chrome/browser/sync/profile_sync_service_data_type_provider.h (right): https://codereview.chromium.org/428143002/diff/20001/chrome/browser/sync/prof... chrome/browser/sync/profile_sync_service_data_type_provider.h:12: virtual syncer::ModelTypeSet GetRequiredSyncDataTypes() = 0; On 2014/07/30 20:20:08, Nicolas Zea wrote: > This is effectively encapsulating the notion of "Datatype A requires Datatype B" > right? I wonder if it makes more sense to have this be part of the > DataTypeController, rather than a new class just for this? > > In other words, DataTypeController could have a GetDependentTypes() method or > something, wherein if type A is enabled (due to preferences or something else), > then all of its dependent types would also be required. > > This kind of logic might even be able to eventually replace pref groups. Not really, no - there's no "datatype A" here. E.g. SESSIONS should be enabled if the profile is supervised (in the future, depending on a setting configured by the custodian), independent of any other types. So this should stay separate from the DataTypeController. Moving the pref group logic into the DataTypeControllers still seems like a good idea though.
Ping? :)
Were you planning to try to move the logic to the datatype controller?
On 2014/08/04 17:12:41, Nicolas Zea wrote: > Were you planning to try to move the logic to the datatype controller? No, as I wrote above, a "GetDependentTypes" isn't what we are trying to do here. Moving the pref group logic into the DataTypeControllers is a separate issue which should go into another CL. Or maybe I'm misunderstanding what you are suggesting?
On 2014/08/04 17:22:13, Marc Treib wrote: > On 2014/08/04 17:12:41, Nicolas Zea wrote: > > Were you planning to try to move the logic to the datatype controller? > > No, as I wrote above, a "GetDependentTypes" isn't what we are trying to do here. > Moving the pref group logic into the DataTypeControllers is a separate issue > which should go into another CL. Or maybe I'm misunderstanding what you are > suggesting? Ah, yeah, I though you meant that even though GetDependentTypes might not be relevant to this CL that moving the required type state into the datatype controller still makes sense. In this case, it could be something like IsTypeRequired, so that a type could decide for itself if its required. Consuming the IsTypeRequired state could probably be done at the ProfileSyncService layer.
On 2014/08/04 17:26:15, Nicolas Zea wrote: > On 2014/08/04 17:22:13, Marc Treib wrote: > > On 2014/08/04 17:12:41, Nicolas Zea wrote: > > > Were you planning to try to move the logic to the datatype controller? > > > > No, as I wrote above, a "GetDependentTypes" isn't what we are trying to do > here. > > Moving the pref group logic into the DataTypeControllers is a separate issue > > which should go into another CL. Or maybe I'm misunderstanding what you are > > suggesting? > > Ah, yeah, I though you meant that even though GetDependentTypes might not be > relevant to this CL that moving the required type state into the datatype > controller still makes sense. In this case, it could be something like > IsTypeRequired, so that a type could decide for itself if its required. > > Consuming the IsTypeRequired state could probably be done at the > ProfileSyncService layer. That would work, but it would mean scattering the supervised user logic into the datatype controllers. I'd argue that e.g. the SESSIONS controller shouldn't have to know anything about supervised users. WDYT?
On 2014/08/05 08:37:33, Marc Treib wrote: > On 2014/08/04 17:26:15, Nicolas Zea wrote: > > On 2014/08/04 17:22:13, Marc Treib wrote: > > > On 2014/08/04 17:12:41, Nicolas Zea wrote: > > > > Were you planning to try to move the logic to the datatype controller? > > > > > > No, as I wrote above, a "GetDependentTypes" isn't what we are trying to do > > here. > > > Moving the pref group logic into the DataTypeControllers is a separate issue > > > which should go into another CL. Or maybe I'm misunderstanding what you are > > > suggesting? > > > > Ah, yeah, I though you meant that even though GetDependentTypes might not be > > relevant to this CL that moving the required type state into the datatype > > controller still makes sense. In this case, it could be something like > > IsTypeRequired, so that a type could decide for itself if its required. > > > > Consuming the IsTypeRequired state could probably be done at the > > ProfileSyncService layer. > > That would work, but it would mean scattering the supervised user logic into the > datatype controllers. I'd argue that e.g. the SESSIONS controller shouldn't have > to know anything about supervised users. WDYT? So, I think there's two separate things to accomplish here 1. A datatype controller can decide it is itself required due to policy or something 2. A datatype controller can tell sync that, if it's running, other types must also be running. This covers all of your use cases right? I.e., I envision the Supervised Users DataTypeController saying that if the user is a supervised user, then the supervised users data type must be running. In addition, depending on policy, it will depend on the sessions and extensions types, which must therefore also be running. Sessions and other types won't have any logic related to the supervised users in their controllers. Being required by something has priority over not being preferred. The PSS then just merges the set of all "dependent" types, and adds them to the normally desired types when configuring. It effectively encapsulating the GetRequiredSyncTypes API you exposed within the datatype controller.
On 2014/08/05 21:06:25, Nicolas Zea wrote: > On 2014/08/05 08:37:33, Marc Treib wrote: > > On 2014/08/04 17:26:15, Nicolas Zea wrote: > > > On 2014/08/04 17:22:13, Marc Treib wrote: > > > > On 2014/08/04 17:12:41, Nicolas Zea wrote: > > > > > Were you planning to try to move the logic to the datatype controller? > > > > > > > > No, as I wrote above, a "GetDependentTypes" isn't what we are trying to do > > > here. > > > > Moving the pref group logic into the DataTypeControllers is a separate > issue > > > > which should go into another CL. Or maybe I'm misunderstanding what you > are > > > > suggesting? > > > > > > Ah, yeah, I though you meant that even though GetDependentTypes might not be > > > relevant to this CL that moving the required type state into the datatype > > > controller still makes sense. In this case, it could be something like > > > IsTypeRequired, so that a type could decide for itself if its required. > > > > > > Consuming the IsTypeRequired state could probably be done at the > > > ProfileSyncService layer. > > > > That would work, but it would mean scattering the supervised user logic into > the > > datatype controllers. I'd argue that e.g. the SESSIONS controller shouldn't > have > > to know anything about supervised users. WDYT? > > So, I think there's two separate things to accomplish here > 1. A datatype controller can decide it is itself required due to policy or > something > 2. A datatype controller can tell sync that, if it's running, other types must > also be running. > > This covers all of your use cases right? > I.e., I envision the Supervised Users DataTypeController saying that if the user > is a supervised user, then the supervised users data type must be running. In > addition, depending on policy, it will depend on the sessions and extensions > types, which must therefore also be running. Sessions and other types won't have > any logic related to the supervised users in their controllers. Being required > by something has priority over not being preferred. > > The PSS then just merges the set of all "dependent" types, and adds them to the > normally desired types when configuring. > > It effectively encapsulating the GetRequiredSyncTypes API you exposed within the > datatype controller. Okay, that will work. It's a bit weird to put the logic whether SESSIONS or EXTENSIONS should be enabled into the controller for SUPERVISED_USER_SETTINGS, because the types really have nothing to do with each other; SUPERVISED_USER_SETTINGS doesn't "depend" on SESSIONS. But oh well, maybe it's still better than the alternatives. I'll implement it and update the CL.
On 2014/08/06 08:03:11, Marc Treib wrote: > On 2014/08/05 21:06:25, Nicolas Zea wrote: > > On 2014/08/05 08:37:33, Marc Treib wrote: > > > On 2014/08/04 17:26:15, Nicolas Zea wrote: > > > > On 2014/08/04 17:22:13, Marc Treib wrote: > > > > > On 2014/08/04 17:12:41, Nicolas Zea wrote: > > > > > > Were you planning to try to move the logic to the datatype controller? > > > > > > > > > > No, as I wrote above, a "GetDependentTypes" isn't what we are trying to > do > > > > here. > > > > > Moving the pref group logic into the DataTypeControllers is a separate > > issue > > > > > which should go into another CL. Or maybe I'm misunderstanding what you > > are > > > > > suggesting? > > > > > > > > Ah, yeah, I though you meant that even though GetDependentTypes might not > be > > > > relevant to this CL that moving the required type state into the datatype > > > > controller still makes sense. In this case, it could be something like > > > > IsTypeRequired, so that a type could decide for itself if its required. > > > > > > > > Consuming the IsTypeRequired state could probably be done at the > > > > ProfileSyncService layer. > > > > > > That would work, but it would mean scattering the supervised user logic into > > the > > > datatype controllers. I'd argue that e.g. the SESSIONS controller shouldn't > > have > > > to know anything about supervised users. WDYT? > > > > So, I think there's two separate things to accomplish here > > 1. A datatype controller can decide it is itself required due to policy or > > something > > 2. A datatype controller can tell sync that, if it's running, other types must > > also be running. > > > > This covers all of your use cases right? > > I.e., I envision the Supervised Users DataTypeController saying that if the > user > > is a supervised user, then the supervised users data type must be running. In > > addition, depending on policy, it will depend on the sessions and extensions > > types, which must therefore also be running. Sessions and other types won't > have > > any logic related to the supervised users in their controllers. Being required > > by something has priority over not being preferred. > > > > The PSS then just merges the set of all "dependent" types, and adds them to > the > > normally desired types when configuring. > > > > It effectively encapsulating the GetRequiredSyncTypes API you exposed within > the > > datatype controller. > > Okay, that will work. It's a bit weird to put the logic whether SESSIONS or > EXTENSIONS should be enabled into the controller for SUPERVISED_USER_SETTINGS, > because the types really have nothing to do with each other; > SUPERVISED_USER_SETTINGS doesn't "depend" on SESSIONS. But oh well, maybe it's > still better than the alternatives. I'll implement it and update the CL. I've given this some more thought. Actually only one of your two points is required: 1. would mean the SESSIONS controller knows about supervised users and force-enables itself when it detects a supervised profile. I don't like this because it scatters supervised user logic into lots places where it shouldn't be. 2. would mean making SESSIONS a dependent type of the SUPERVISED_USER_SETTINGS controller. I don't like this because a) it doesn't really fit the intended use, as the logic to enable SESSIONS has nothing to do with the SUPERVISED_USER_SETTINGS type, and b) it effectively duplicates pref groups. Unless the goal really is to replace pref groups, that seems like a bad idea. So, are we trying to get rid of pref groups? Otherwise I much prefer the current design with the extra interface.
I've renamed the interface as discussed in the mail thread, and also addressed Bernhard's remaining concerns. Open question: How to differentiate types enabled by user choice from those enabled by policy, so the UI can be adjusted accordingly (e.g. disable the corresponding check boxes in the sync setup overlay). But that can be done in a followup CL. Thanks for all the feedback! https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service.cc (right): https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service.cc:2305: data_type_providers_.remove(provider); On 2014/07/30 14:13:20, Bernhard Bauer wrote: > On 2014/07/30 13:10:39, Marc Treib wrote: > > On 2014/07/30 12:38:51, Bernhard Bauer wrote: > > > DCHECK the result? > > > > There is no result :P > > We could DCHECK(HasDataTypeProvider(..)) similar to above, if > > RemoveDataTypeProvider for a non-registered value should be forbidden. Here > I've > > mirrored the behavior of ObserverLists, which do allow this. > > Yeah, I meant checking whether there is one registered. It just seems weird to > disallow adding a data type provider twice, but allow removing it twice. Done. https://codereview.chromium.org/428143002/diff/1/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service.cc:2509: // because the provider might remove itself from the list in the callback. On 2014/07/30 14:13:20, Bernhard Bauer wrote: > On 2014/07/30 13:10:39, Marc Treib wrote: > > On 2014/07/30 12:38:51, Bernhard Bauer wrote: > > > I think an easier solution here would be to require the data type provider > not > > > to remove themselves from the list while it's being called back. No need to > > make > > > our lives harder than necessary :) > > > > I guess we could. OTOH, this is a fairly common pattern when iterating over > STL > > containers and I don't mind it. > > Not as common as a standard for loop ;-) > > If you keep it this way, what if the callback removes the *next* data type > observer from the list? Basically, if you want to handle all the edge cases, > you're going to end up reimplementing ObserverList, for a use case that doesn't > even currently exist. Alright. Bonus side-effect: It doesn't have to be a std::list anymore.
Thanks! LGTM
LGTM with one comment https://codereview.chromium.org/428143002/diff/60001/chrome/browser/sync/prof... File chrome/browser/sync/profile_sync_service.cc (right): https://codereview.chromium.org/428143002/diff/60001/chrome/browser/sync/prof... chrome/browser/sync/profile_sync_service.cc:1940: types = Union(GetPreferredDirectoryDataTypes(), I think it might make more sense to put the GetDataTypesFromPreferenceProviders call into the GetPreferredDirectoryDataTypes method. Everywhere that calls GetPreferredDirectoryDataTypes probably wants to include those involving policy.
Bernhard, I had to add some code in the SUService to make sure Shutdown() doesn't happen more than once, so we don't try to remove the PreferenceProvider more than once. Just in case you want to take another look :) https://codereview.chromium.org/428143002/diff/60001/chrome/browser/sync/prof... File chrome/browser/sync/profile_sync_service.cc (right): https://codereview.chromium.org/428143002/diff/60001/chrome/browser/sync/prof... chrome/browser/sync/profile_sync_service.cc:1940: types = Union(GetPreferredDirectoryDataTypes(), On 2014/08/07 17:16:33, Nicolas Zea wrote: > I think it might make more sense to put the GetDataTypesFromPreferenceProviders > call into the GetPreferredDirectoryDataTypes method. > > Everywhere that calls GetPreferredDirectoryDataTypes probably wants to include > those involving policy. Done.
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/428143002/120001
https://codereview.chromium.org/428143002/diff/120001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/428143002/diff/120001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_service.cc:155: if (!did_init_ || did_shutdown_) When would we call Shutdown() more than once?
https://codereview.chromium.org/428143002/diff/120001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/428143002/diff/120001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_service.cc:155: if (!did_init_ || did_shutdown_) On 2014/08/11 08:48:21, Bernhard Bauer wrote: > When would we call Shutdown() more than once? Ah, that was unclear - actually I'm not sure if we ever do. But a bunch of tests create an SUService which they don't Init() but do Shutdown(). See http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_.... Given the number of failing tests, it seemed best to make sure here that there are only matching Init() and Shutdown() calls.
https://codereview.chromium.org/428143002/diff/120001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/428143002/diff/120001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_service.cc:155: if (!did_init_ || did_shutdown_) On 2014/08/11 08:57:09, Marc Treib wrote: > On 2014/08/11 08:48:21, Bernhard Bauer wrote: > > When would we call Shutdown() more than once? > > Ah, that was unclear - actually I'm not sure if we ever do. But a bunch of tests > create an SUService which they don't Init() but do Shutdown(). See > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_.... > Given the number of failing tests, it seemed best to make sure here that there > are only matching Init() and Shutdown() calls. I'm not sure what you mean by that -- right now you do handle non-matching Init() and Shutdown() calls, and as you point out, making sure they always match might be a lot of work. I don't think we need to handle more than one Shutdown() though, so I would rather make the second condition into a DCHECK.
The CQ bit was unchecked by treib@chromium.org
https://codereview.chromium.org/428143002/diff/120001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/428143002/diff/120001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_service.cc:155: if (!did_init_ || did_shutdown_) On 2014/08/11 09:07:13, Bernhard Bauer wrote: > On 2014/08/11 08:57:09, Marc Treib wrote: > > On 2014/08/11 08:48:21, Bernhard Bauer wrote: > > > When would we call Shutdown() more than once? > > > > Ah, that was unclear - actually I'm not sure if we ever do. But a bunch of > tests > > create an SUService which they don't Init() but do Shutdown(). See > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_.... > > Given the number of failing tests, it seemed best to make sure here that there > > are only matching Init() and Shutdown() calls. > > I'm not sure what you mean by that -- right now you do handle non-matching > Init() and Shutdown() calls, and as you point out, making sure they always match > might be a lot of work. I don't think we need to handle more than one Shutdown() > though, so I would rather make the second condition into a DCHECK. Done.
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/428143002/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/428143002/160001
Message was sent while issue was closed.
Change committed as 288951 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
