Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1873343002:
[Policy] HostContentSettingsMap: Add AreUserExceptionsAllowedForType() and tests. (Closed)
|
Created:
4 years, 8 months ago by huangs Modified:
4 years, 8 months ago CC:
chromium-reviews, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Policy] HostContentSettingsMap: Add AreUserExceptionsAllowedForType() and tests.
Simple changes, with some refactoring:
- Add AreUserExceptionsAllowedForType(). This interface will shortly
be used by front end.
- Store PrefProvider as typed member functions for ease of access.
- Add 2 tests to HostContentSettingsMapTest (with supporting routines).
- AreUserExceptionsAllowedForType: testing new interface.
- PrefExceptionsOperation: testing how policy defaults can override
user exceptions.
Committed: https://crrev.com/3973014f0fbf36ec4a961e97273516b2ed1953d6
Cr-Commit-Position: refs/heads/master@{#388885}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Sync. #Patch Set 3 : Deferring GetDefaultContentSetting() change to another CL; removing TestUtils addition. #
Total comments: 2
Patch Set 4 : Fix comments. #
Total comments: 8
Patch Set 5 : Cleanups. #
Total comments: 8
Patch Set 6 : Make AreUserExceptionsAllowedForType() more general and use ProviderType directly. #
Total comments: 2
Patch Set 7 : Add DCHECK in AreUserExceptionsAllowedForType(). #
Messages
Total messages: 56 (15 generated)
huangs@chromium.org changed reviewers: + bartfab@chromium.org, bauerb@chromium.org, grt@chromium.org
This CL extracts some content from https://codereview.chromium.org/1865803002/, and enables a number of relatively independent follow-ups: - UI: Update JS code and fix http://crbug.com/106682 - UI: Update ContentSettingSingleRadioGroup - Refactor: Create POLICY_DEFAULT_PROVIDER. - Refactor: Add provider selection layer in HostContentSettingsMap (need design). - Refactor: Simplify callers of GetDefaultContentSetting(). PTAL. Thanks! https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:53: // Helper that wrap test boilerplates, and maintains a "current content type" Adding this layer of abstraction makes test much smaller, but also makes them less "hands on". If you'd prefer that I don't do this, can I use the lambda approach as seen in https://codereview.chromium.org/1865803002/diff/140001/chrome/browser/content... ?
Description was changed from
==========
[Policy] Refactor HostContentSettingsMap and adding tests.
Relative mechanical changes to HostContentSettingsMap:
- Store PolicyProvider and PrefProvider as typed member functions for
ease of access.
- Add alternatve interface to GetDefaultContentSetting() that returns
ProviderType in a struct, without needing string pointer param.
- Add AreUserExceptionsAllowedForType(). This interface will shortly
be used by front end.
- Add 2 tests to HostContentSettingsMapTest (with supporting routines).
- AreUserExceptionsAllowedForType: testing new interface.
- PrefExceptionsOperation: testing how policy defaults can override
user exceptions.
==========
to
==========
[Policy] Refactor HostContentSettingsMap and add tests.
Relative mechanical changes to HostContentSettingsMap:
- Store PolicyProvider and PrefProvider as typed member functions for
ease of access.
- Add alternatve interface to GetDefaultContentSetting() that returns
ProviderType in a struct, without needing string pointer param.
- Add AreUserExceptionsAllowedForType(). This interface will shortly
be used by front end.
- Add 2 tests to HostContentSettingsMapTest (with supporting routines).
- AreUserExceptionsAllowedForType: testing new interface.
- PrefExceptionsOperation: testing how policy defaults can override
user exceptions.
==========
The CQ bit was checked by huangs@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1873343002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1873343002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
raymes@chromium.org changed reviewers: + raymes@chromium.org
Thanks! https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:55: class HostContentSettingsMapTester { Since these are specific to your tests, how about calling this PolicyDefaultTester (or something similar)? You could also make this a test class that your tests inherit from, similar to HostContentSettingsMapTest above. https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:65: HostContentSettingsMapTester& SelectType(ContentSettingsType content_type) { nit: How about removing this function (and not tracking the "cur" state in this class). Instead make ClearPolicyDefault and SetPolicyDefault take a ContentSettingsType and lookup the pref based on that type. https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:102: content_settings::SettingSource SourceMatch( nit: GetSettingSourceForURL? https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:870: // Not calling tester.ClearPolicyDefault(); let it linger. Should we clean up the state? https://codereview.chromium.org/1873343002/diff/1/components/content_settings... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1873343002/diff/1/components/content_settings... components/content_settings/core/browser/host_content_settings_map.cc:268: bool HostContentSettingsMap::AreUserExceptionsAllowedForType( I'm guessing this function is going to become more complicated later? (Similar to in the other CL?) https://codereview.chromium.org/1873343002/diff/1/components/content_settings... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1873343002/diff/1/components/content_settings... components/content_settings/core/browser/host_content_settings_map.h:88: GetDefaultContentSetting(ContentSettingsType content_type) const; I don't think we should have two functions for GetDefaultContentSetting. It adds complexity to the interface and we're currently in the process of trying to make the interface more simple :). I would suggest either 1) refactoring all the callsites to use the new interface (in a separate CL) or 2) just use the old version of GetDefaultContentSetting and compare the string returned to "policy". If you don't want to do 1) we could just do 2) for now and come back and refactoring later. Would that work? https://codereview.chromium.org/1873343002/diff/1/components/content_settings... File components/content_settings/core/test/content_settings_test_utils.cc (right): https://codereview.chromium.org/1873343002/diff/1/components/content_settings... components/content_settings/core/test/content_settings_test_utils.cc:54: bool TestUtils::AddUserException(const std::string& pattern_string, Could you instead just call HostContentSettingsMap::SetContentSetting? It will always end up setting the user's preference.
Before we go much further with this CL, I would like to consider the overall design for a bit, to avoid doing unnecessary changes here. Leaving the issue of wildcard user exceptions aside, I'm fairly certain we can get the desired behavior by adding a recommended policy content settings provider below the user pref content settings provider. The provider stack would (simplified) look like this: (highest precedence) mandatory policy user prefs recommended policy hardcoded defaults (lowest precedence) The two policy providers could probably be the same class, with just a flag that determines their position in the stack. That is, the mandatory policy provider would not set any content settings if the policy is set to allow user exceptions, and vice-versa the recommended policy provider would not set anything if the policy is set not to allow user exceptions (not that it would matter). With that approach, we could keep the invariant that content setting providers are always consulted in order of their precedence, and we could remove some of the current special-casing for the policy provider in HCSM in this CL. WDYT? https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:104: GURL host(url_str); Should this be |url|? https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:855: HostContentSettingsMapTester tester; Move this into the loop to limit its scope? https://codereview.chromium.org/1873343002/diff/1/components/content_settings... File components/content_settings/core/test/content_settings_test_utils.cc (right): https://codereview.chromium.org/1873343002/diff/1/components/content_settings... components/content_settings/core/test/content_settings_test_utils.cc:54: bool TestUtils::AddUserException(const std::string& pattern_string, On 2016/04/12 04:52:47, raymes wrote: > Could you instead just call HostContentSettingsMap::SetContentSetting? It will > always end up setting the user's preference. +1 (That's what I meant in my earlier comments).
On 2016/04/12 09:30:05, Bernhard Bauer wrote: > Before we go much further with this CL, I would like to consider the overall > design for a bit, to avoid doing unnecessary changes here. Leaving the issue of > wildcard user exceptions aside, I'm fairly certain we can get the desired > behavior by adding a recommended policy content settings provider below the user > pref content settings provider. The provider stack would (simplified) look like > this: > > (highest precedence) > mandatory policy > user prefs > recommended policy > hardcoded defaults > (lowest precedence) > > The two policy providers could probably be the same class, with just a flag that > determines their position in the stack. That is, the mandatory policy provider > would not set any content settings if the policy is set to allow user > exceptions, and vice-versa the recommended policy provider would not set > anything if the policy is set not to allow user exceptions (not that it would > matter). > > With that approach, we could keep the invariant that content setting providers > are always consulted in order of their precedence, and we could remove some of > the current special-casing for the policy provider in HCSM in this CL. WDYT? What you describe is how regular prefs work. Content settings were always a weird exception that did not support such layering. A definite +1 to supporting it. However, I am not sure this alone will cover the feature request at hand: The layering happens on a per-setting basis. The default behavior and a URL-specific exception will appear as different entities, so even if mandatory policy locks one of them (e.g. the default behavior), users can still specify their own value for the other (e.g. a URL-specific exception). We will still need a mechanism to tell us whether we should honor exceptions and if so, whether only policy-mandated ones or all should be honored. > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > File chrome/browser/content_settings/host_content_settings_map_unittest.cc > (right): > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > chrome/browser/content_settings/host_content_settings_map_unittest.cc:104: GURL > host(url_str); > Should this be |url|? > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > chrome/browser/content_settings/host_content_settings_map_unittest.cc:855: > HostContentSettingsMapTester tester; > Move this into the loop to limit its scope? > > https://codereview.chromium.org/1873343002/diff/1/components/content_settings... > File components/content_settings/core/test/content_settings_test_utils.cc > (right): > > https://codereview.chromium.org/1873343002/diff/1/components/content_settings... > components/content_settings/core/test/content_settings_test_utils.cc:54: bool > TestUtils::AddUserException(const std::string& pattern_string, > On 2016/04/12 04:52:47, raymes wrote: > > Could you instead just call HostContentSettingsMap::SetContentSetting? It will > > always end up setting the user's preference. > > +1 (That's what I meant in my earlier comments).
On 2016/04/12 12:33:07, bartfab (slow) wrote: > On 2016/04/12 09:30:05, Bernhard Bauer wrote: > > Before we go much further with this CL, I would like to consider the overall > > design for a bit, to avoid doing unnecessary changes here. Leaving the issue > of > > wildcard user exceptions aside, I'm fairly certain we can get the desired > > behavior by adding a recommended policy content settings provider below the > user > > pref content settings provider. The provider stack would (simplified) look > like > > this: > > > > (highest precedence) > > mandatory policy > > user prefs > > recommended policy > > hardcoded defaults > > (lowest precedence) > > > > The two policy providers could probably be the same class, with just a flag > that > > determines their position in the stack. That is, the mandatory policy provider > > would not set any content settings if the policy is set to allow user > > exceptions, and vice-versa the recommended policy provider would not set > > anything if the policy is set not to allow user exceptions (not that it would > > matter). > > > > With that approach, we could keep the invariant that content setting providers > > are always consulted in order of their precedence, and we could remove some of > > the current special-casing for the policy provider in HCSM in this CL. WDYT? > > What you describe is how regular prefs work. Content settings were always a > weird exception that did not support such layering. Well, recommended prefs came _way_ after the content settings refactoring that introduced the layering. 😃 > A definite +1 to supporting > it. However, I am not sure this alone will cover the feature request at hand: > The layering happens on a per-setting basis. The default behavior and a > URL-specific exception will appear as different entities, so even if mandatory > policy locks one of them (e.g. the default behavior), users can still specify > their own value for the other (e.g. a URL-specific exception). We will still > need a mechanism to tell us whether we should honor exceptions and if so, > whether only policy-mandated ones or all should be honored. Sorry, can you give a specific example? > > > > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > > File chrome/browser/content_settings/host_content_settings_map_unittest.cc > > (right): > > > > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > > chrome/browser/content_settings/host_content_settings_map_unittest.cc:104: > GURL > > host(url_str); > > Should this be |url|? > > > > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > > chrome/browser/content_settings/host_content_settings_map_unittest.cc:855: > > HostContentSettingsMapTester tester; > > Move this into the loop to limit its scope? > > > > > https://codereview.chromium.org/1873343002/diff/1/components/content_settings... > > File components/content_settings/core/test/content_settings_test_utils.cc > > (right): > > > > > https://codereview.chromium.org/1873343002/diff/1/components/content_settings... > > components/content_settings/core/test/content_settings_test_utils.cc:54: bool > > TestUtils::AddUserException(const std::string& pattern_string, > > On 2016/04/12 04:52:47, raymes wrote: > > > Could you instead just call HostContentSettingsMap::SetContentSetting? It > will > > > always end up setting the user's preference. > > > > +1 (That's what I meant in my earlier comments).
On 2016/04/12 12:41:26, Bernhard Bauer wrote: > On 2016/04/12 12:33:07, bartfab (slow) wrote: > > On 2016/04/12 09:30:05, Bernhard Bauer wrote: > > > Before we go much further with this CL, I would like to consider the overall > > > design for a bit, to avoid doing unnecessary changes here. Leaving the issue > > of > > > wildcard user exceptions aside, I'm fairly certain we can get the desired > > > behavior by adding a recommended policy content settings provider below the > > user > > > pref content settings provider. The provider stack would (simplified) look > > like > > > this: > > > > > > (highest precedence) > > > mandatory policy > > > user prefs > > > recommended policy > > > hardcoded defaults > > > (lowest precedence) > > > > > > The two policy providers could probably be the same class, with just a flag > > that > > > determines their position in the stack. That is, the mandatory policy > provider > > > would not set any content settings if the policy is set to allow user > > > exceptions, and vice-versa the recommended policy provider would not set > > > anything if the policy is set not to allow user exceptions (not that it > would > > > matter). > > > > > > With that approach, we could keep the invariant that content setting > providers > > > are always consulted in order of their precedence, and we could remove some > of > > > the current special-casing for the policy provider in HCSM in this CL. WDYT? > > > > What you describe is how regular prefs work. Content settings were always a > > weird exception that did not support such layering. > > Well, recommended prefs came _way_ after the content settings refactoring that > introduced the layering. 😃 > > > A definite +1 to supporting > > it. However, I am not sure this alone will cover the feature request at hand: > > The layering happens on a per-setting basis. The default behavior and a > > URL-specific exception will appear as different entities, so even if mandatory > > policy locks one of them (e.g. the default behavior), users can still specify > > their own value for the other (e.g. a URL-specific exception). We will still > > need a mechanism to tell us whether we should honor exceptions and if so, > > whether only policy-mandated ones or all should be honored. > > Sorry, can you give a specific example? Sure. Say cookies: * There is one content setting "default cookie behavior" * There is another content setting "cookie behavior for example.com" An admin who wants to disable cookies will set the "default cookie behavior" to "blocked" via mandatory policy. Since this is the highest-priority source, every read of the "default cookie behavior" setting will return "blocked." However, this does not affect any other settings, including "cookie behavior for example.com." The user can still set that to "allow," trumping the admin policy for a specific URL. We still need a policy that will tell us how to interpret exceptions relative to the default policy. > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > > > File chrome/browser/content_settings/host_content_settings_map_unittest.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > > > chrome/browser/content_settings/host_content_settings_map_unittest.cc:104: > > GURL > > > host(url_str); > > > Should this be |url|? > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > > > chrome/browser/content_settings/host_content_settings_map_unittest.cc:855: > > > HostContentSettingsMapTester tester; > > > Move this into the loop to limit its scope? > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/components/content_settings... > > > File components/content_settings/core/test/content_settings_test_utils.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/components/content_settings... > > > components/content_settings/core/test/content_settings_test_utils.cc:54: > bool > > > TestUtils::AddUserException(const std::string& pattern_string, > > > On 2016/04/12 04:52:47, raymes wrote: > > > > Could you instead just call HostContentSettingsMap::SetContentSetting? It > > will > > > > always end up setting the user's preference. > > > > > > +1 (That's what I meant in my earlier comments).
On 2016/04/12 12:45:20, bartfab (slow) wrote: > On 2016/04/12 12:41:26, Bernhard Bauer wrote: > > On 2016/04/12 12:33:07, bartfab (slow) wrote: > > > On 2016/04/12 09:30:05, Bernhard Bauer wrote: > > > > Before we go much further with this CL, I would like to consider the > overall > > > > design for a bit, to avoid doing unnecessary changes here. Leaving the > issue > > > of > > > > wildcard user exceptions aside, I'm fairly certain we can get the desired > > > > behavior by adding a recommended policy content settings provider below > the > > > user > > > > pref content settings provider. The provider stack would (simplified) look > > > like > > > > this: > > > > > > > > (highest precedence) > > > > mandatory policy > > > > user prefs > > > > recommended policy > > > > hardcoded defaults > > > > (lowest precedence) > > > > > > > > The two policy providers could probably be the same class, with just a > flag > > > that > > > > determines their position in the stack. That is, the mandatory policy > > provider > > > > would not set any content settings if the policy is set to allow user > > > > exceptions, and vice-versa the recommended policy provider would not set > > > > anything if the policy is set not to allow user exceptions (not that it > > would > > > > matter). > > > > > > > > With that approach, we could keep the invariant that content setting > > providers > > > > are always consulted in order of their precedence, and we could remove > some > > of > > > > the current special-casing for the policy provider in HCSM in this CL. > WDYT? > > > > > > What you describe is how regular prefs work. Content settings were always a > > > weird exception that did not support such layering. > > > > Well, recommended prefs came _way_ after the content settings refactoring that > > introduced the layering. 😃 > > > > > A definite +1 to supporting > > > it. However, I am not sure this alone will cover the feature request at > hand: > > > The layering happens on a per-setting basis. The default behavior and a > > > URL-specific exception will appear as different entities, so even if > mandatory > > > policy locks one of them (e.g. the default behavior), users can still > specify > > > their own value for the other (e.g. a URL-specific exception). We will still > > > need a mechanism to tell us whether we should honor exceptions and if so, > > > whether only policy-mandated ones or all should be honored. > > > > Sorry, can you give a specific example? > > Sure. Say cookies: > > * There is one content setting "default cookie behavior" > * There is another content setting "cookie behavior for example.com" > > An admin who wants to disable cookies will set the "default cookie behavior" to > "blocked" via mandatory policy. Since this is the highest-priority source, every > read of the "default cookie behavior" setting will return "blocked." However, > this does not affect any other settings, including "cookie behavior for > example.com." The user can still set that to "allow," trumping the admin policy > for a specific URL. We still need a policy that will tell us how to interpret > exceptions relative to the default policy. > We query providers in the order of priority; the first provider that stores a pattern matching example.com answers. Within a provider, we take the rule with the most specific pattern. Policy provider has a higher priority than pref provider, so it doesn't matter that "*" is a less specific pattern than "https://example.com". Policy provider will win. The user-chosen site specific content setting will not override the policy-set default setting. So if I'm not mistaken, this ordering of providers would work: Mandatory policy (defaults & exceptions) User prefs (exceptions) Recommended (exceptions) User prefs (defaults) Recommended (defaults) Hardcoded (defaults) > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > > > > File chrome/browser/content_settings/host_content_settings_map_unittest.cc > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > > > > chrome/browser/content_settings/host_content_settings_map_unittest.cc:104: > > > GURL > > > > host(url_str); > > > > Should this be |url|? > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > > > > chrome/browser/content_settings/host_content_settings_map_unittest.cc:855: > > > > HostContentSettingsMapTester tester; > > > > Move this into the loop to limit its scope? > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/components/content_settings... > > > > File components/content_settings/core/test/content_settings_test_utils.cc > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/components/content_settings... > > > > components/content_settings/core/test/content_settings_test_utils.cc:54: > > bool > > > > TestUtils::AddUserException(const std::string& pattern_string, > > > > On 2016/04/12 04:52:47, raymes wrote: > > > > > Could you instead just call HostContentSettingsMap::SetContentSetting? > It > > > will > > > > > always end up setting the user's preference. > > > > > > > > +1 (That's what I meant in my earlier comments).
On 2016/04/12 12:45:20, bartfab (slow) wrote: > On 2016/04/12 12:41:26, Bernhard Bauer wrote: > > On 2016/04/12 12:33:07, bartfab (slow) wrote: > > > On 2016/04/12 09:30:05, Bernhard Bauer wrote: > > > > Before we go much further with this CL, I would like to consider the > overall > > > > design for a bit, to avoid doing unnecessary changes here. Leaving the > issue > > > of > > > > wildcard user exceptions aside, I'm fairly certain we can get the desired > > > > behavior by adding a recommended policy content settings provider below > the > > > user > > > > pref content settings provider. The provider stack would (simplified) look > > > like > > > > this: > > > > > > > > (highest precedence) > > > > mandatory policy > > > > user prefs > > > > recommended policy > > > > hardcoded defaults > > > > (lowest precedence) > > > > > > > > The two policy providers could probably be the same class, with just a > flag > > > that > > > > determines their position in the stack. That is, the mandatory policy > > provider > > > > would not set any content settings if the policy is set to allow user > > > > exceptions, and vice-versa the recommended policy provider would not set > > > > anything if the policy is set not to allow user exceptions (not that it > > would > > > > matter). > > > > > > > > With that approach, we could keep the invariant that content setting > > providers > > > > are always consulted in order of their precedence, and we could remove > some > > of > > > > the current special-casing for the policy provider in HCSM in this CL. > WDYT? > > > > > > What you describe is how regular prefs work. Content settings were always a > > > weird exception that did not support such layering. > > > > Well, recommended prefs came _way_ after the content settings refactoring that > > introduced the layering. 😃 > > > > > A definite +1 to supporting > > > it. However, I am not sure this alone will cover the feature request at > hand: > > > The layering happens on a per-setting basis. The default behavior and a > > > URL-specific exception will appear as different entities, so even if > mandatory > > > policy locks one of them (e.g. the default behavior), users can still > specify > > > their own value for the other (e.g. a URL-specific exception). We will still > > > need a mechanism to tell us whether we should honor exceptions and if so, > > > whether only policy-mandated ones or all should be honored. > > > > Sorry, can you give a specific example? > > Sure. Say cookies: > > * There is one content setting "default cookie behavior" > * There is another content setting "cookie behavior for example.com" > > An admin who wants to disable cookies will set the "default cookie behavior" to > "blocked" via mandatory policy. Since this is the highest-priority source, every > read of the "default cookie behavior" setting will return "blocked." However, > this does not affect any other settings, including "cookie behavior for > example.com." The user can still set that to "allow," trumping the admin policy > for a specific URL. We still need a policy that will tell us how to interpret > exceptions relative to the default policy. Thanks! I think this will still work, because the policy provider reads from a different pref, one that is exclusively used for policy (kManagedDefaultCookiesSetting). I do see a problem with implementing recommended content settings though in the current architecture, which is that the hardcoded defaults and the user-defined defaults are coming from the same provider. That means the stack actually looks like this: policy user exceptions user defaults & hardcoded defaults And this means that we can't put another content setting provider between user settings and hardcoded defaults, because it would override any user defaults. That could be fixed though by moving the user defaults to the current content_settings_pref_provider (so that it would contain all user-defined content settings, wildcard or exception).
On 2016/04/12 12:58:06, msramek wrote: > On 2016/04/12 12:45:20, bartfab (slow) wrote: > > On 2016/04/12 12:41:26, Bernhard Bauer wrote: > > > On 2016/04/12 12:33:07, bartfab (slow) wrote: > > > > On 2016/04/12 09:30:05, Bernhard Bauer wrote: > > > > > Before we go much further with this CL, I would like to consider the > > overall > > > > > design for a bit, to avoid doing unnecessary changes here. Leaving the > > issue > > > > of > > > > > wildcard user exceptions aside, I'm fairly certain we can get the > desired > > > > > behavior by adding a recommended policy content settings provider below > > the > > > > user > > > > > pref content settings provider. The provider stack would (simplified) > look > > > > like > > > > > this: > > > > > > > > > > (highest precedence) > > > > > mandatory policy > > > > > user prefs > > > > > recommended policy > > > > > hardcoded defaults > > > > > (lowest precedence) > > > > > > > > > > The two policy providers could probably be the same class, with just a > > flag > > > > that > > > > > determines their position in the stack. That is, the mandatory policy > > > provider > > > > > would not set any content settings if the policy is set to allow user > > > > > exceptions, and vice-versa the recommended policy provider would not set > > > > > anything if the policy is set not to allow user exceptions (not that it > > > would > > > > > matter). > > > > > > > > > > With that approach, we could keep the invariant that content setting > > > providers > > > > > are always consulted in order of their precedence, and we could remove > > some > > > of > > > > > the current special-casing for the policy provider in HCSM in this CL. > > WDYT? > > > > > > > > What you describe is how regular prefs work. Content settings were always > a > > > > weird exception that did not support such layering. > > > > > > Well, recommended prefs came _way_ after the content settings refactoring > that > > > introduced the layering. 😃 > > > > > > > A definite +1 to supporting > > > > it. However, I am not sure this alone will cover the feature request at > > hand: > > > > The layering happens on a per-setting basis. The default behavior and a > > > > URL-specific exception will appear as different entities, so even if > > mandatory > > > > policy locks one of them (e.g. the default behavior), users can still > > specify > > > > their own value for the other (e.g. a URL-specific exception). We will > still > > > > need a mechanism to tell us whether we should honor exceptions and if so, > > > > whether only policy-mandated ones or all should be honored. > > > > > > Sorry, can you give a specific example? > > > > Sure. Say cookies: > > > > * There is one content setting "default cookie behavior" > > * There is another content setting "cookie behavior for example.com" > > > > An admin who wants to disable cookies will set the "default cookie behavior" > to > > "blocked" via mandatory policy. Since this is the highest-priority source, > every > > read of the "default cookie behavior" setting will return "blocked." However, > > this does not affect any other settings, including "cookie behavior for > > example.com." The user can still set that to "allow," trumping the admin > policy > > for a specific URL. We still need a policy that will tell us how to interpret > > exceptions relative to the default policy. > > > > We query providers in the order of priority; the first provider that stores a > pattern matching http://example.com answers. Within a provider, we take the rule with > the most specific pattern. > > Policy provider has a higher priority than pref provider, so it doesn't matter > that "*" is a less specific pattern than "https://example.com". Policy provider > will win. The user-chosen site specific content setting will not override the > policy-set default setting. > > So if I'm not mistaken, this ordering of providers would work: > Mandatory policy (defaults & exceptions) > User prefs (exceptions) > Recommended (exceptions) > User prefs (defaults) > Recommended (defaults) > Hardcoded (defaults) Thanks for clarifying. I was not aware that we allow the providers to process the patterns and come up with a matching value. In that case, layering will work but we need more than the four standard layers (default, recommended, user, mandatory), as you also point out. Even with the six layers you listed, there still are some use cases we cannot support though: In current parlance, setting a pref to a mandatory value means that we do not want the user to change that particular pref. Translating this into content settings lingo, I would say that setting the default behavior to a particular value via mandatory policy only means that we want the *default* to be uncheangeable. It does not say anything about whether we want to allow exceptions from this default or not. To be fully flexible, we should support both possibilities - lock down the default and do not allow any exceptions, as well as lock down default but do allow exceptions. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > > > > > File > chrome/browser/content_settings/host_content_settings_map_unittest.cc > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > > > > > > chrome/browser/content_settings/host_content_settings_map_unittest.cc:104: > > > > GURL > > > > > host(url_str); > > > > > Should this be |url|? > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > > > > > > chrome/browser/content_settings/host_content_settings_map_unittest.cc:855: > > > > > HostContentSettingsMapTester tester; > > > > > Move this into the loop to limit its scope? > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/components/content_settings... > > > > > File > components/content_settings/core/test/content_settings_test_utils.cc > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/components/content_settings... > > > > > components/content_settings/core/test/content_settings_test_utils.cc:54: > > > bool > > > > > TestUtils::AddUserException(const std::string& pattern_string, > > > > > On 2016/04/12 04:52:47, raymes wrote: > > > > > > Could you instead just call HostContentSettingsMap::SetContentSetting? > > It > > > > will > > > > > > always end up setting the user's preference. > > > > > > > > > > +1 (That's what I meant in my earlier comments).
On 2016/04/12 13:00:52, Bernhard Bauer wrote: > On 2016/04/12 12:45:20, bartfab (slow) wrote: > > On 2016/04/12 12:41:26, Bernhard Bauer wrote: > > > On 2016/04/12 12:33:07, bartfab (slow) wrote: > > > > On 2016/04/12 09:30:05, Bernhard Bauer wrote: > > > > > Before we go much further with this CL, I would like to consider the > > overall > > > > > design for a bit, to avoid doing unnecessary changes here. Leaving the > > issue > > > > of > > > > > wildcard user exceptions aside, I'm fairly certain we can get the > desired > > > > > behavior by adding a recommended policy content settings provider below > > the > > > > user > > > > > pref content settings provider. The provider stack would (simplified) > look > > > > like > > > > > this: > > > > > > > > > > (highest precedence) > > > > > mandatory policy > > > > > user prefs > > > > > recommended policy > > > > > hardcoded defaults > > > > > (lowest precedence) > > > > > > > > > > The two policy providers could probably be the same class, with just a > > flag > > > > that > > > > > determines their position in the stack. That is, the mandatory policy > > > provider > > > > > would not set any content settings if the policy is set to allow user > > > > > exceptions, and vice-versa the recommended policy provider would not set > > > > > anything if the policy is set not to allow user exceptions (not that it > > > would > > > > > matter). > > > > > > > > > > With that approach, we could keep the invariant that content setting > > > providers > > > > > are always consulted in order of their precedence, and we could remove > > some > > > of > > > > > the current special-casing for the policy provider in HCSM in this CL. > > WDYT? > > > > > > > > What you describe is how regular prefs work. Content settings were always > a > > > > weird exception that did not support such layering. > > > > > > Well, recommended prefs came _way_ after the content settings refactoring > that > > > introduced the layering. 😃 > > > > > > > A definite +1 to supporting > > > > it. However, I am not sure this alone will cover the feature request at > > hand: > > > > The layering happens on a per-setting basis. The default behavior and a > > > > URL-specific exception will appear as different entities, so even if > > mandatory > > > > policy locks one of them (e.g. the default behavior), users can still > > specify > > > > their own value for the other (e.g. a URL-specific exception). We will > still > > > > need a mechanism to tell us whether we should honor exceptions and if so, > > > > whether only policy-mandated ones or all should be honored. > > > > > > Sorry, can you give a specific example? > > > > Sure. Say cookies: > > > > * There is one content setting "default cookie behavior" > > * There is another content setting "cookie behavior for example.com" > > > > An admin who wants to disable cookies will set the "default cookie behavior" > to > > "blocked" via mandatory policy. Since this is the highest-priority source, > every > > read of the "default cookie behavior" setting will return "blocked." However, > > this does not affect any other settings, including "cookie behavior for > > example.com." The user can still set that to "allow," trumping the admin > policy > > for a specific URL. We still need a policy that will tell us how to interpret > > exceptions relative to the default policy. > > Thanks! I think this will still work, because the policy provider reads from a > different pref, one that is exclusively used for policy > (kManagedDefaultCookiesSetting). > > I do see a problem with implementing recommended content settings though in the > current architecture, which is that the hardcoded defaults and the user-defined > defaults are coming from the same provider. That means the stack actually looks > like this: > > policy > user exceptions > user defaults & hardcoded defaults > > And this means that we can't put another content setting provider between user > settings and hardcoded defaults, because it would override any user defaults. > That could be fixed though by moving the user defaults to the current > content_settings_pref_provider (so that it would contain all user-defined > content settings, wildcard or exception). Just to avoid confusion: I believe Bernhard and I are proposing the same thing. I started with the assumption that DefaultProvider would be split into hardcoded and user defaults; if we don't need the recommended exceptions provider, then we can collapse the two user pref providers in my stack and get the same thing that Bernhard proposes. However! There is one unfortunate side effect. Default provider currently stores "user choice == hardcoded default" as "user choice == NULL". If we insert the recommended default provider between user default and hardcoded default, all users that explicitly chose the hardcoded default value will be switched to the recommended default value. This is probably not such a big deal though, as it only affects policy users, where the administrator is *supposed* to be in control of the setting anyway.
On 2016/04/12 13:12:41, bartfab (slow) wrote: > On 2016/04/12 12:58:06, msramek wrote: > > On 2016/04/12 12:45:20, bartfab (slow) wrote: > > > On 2016/04/12 12:41:26, Bernhard Bauer wrote: > > > > On 2016/04/12 12:33:07, bartfab (slow) wrote: > > > > > On 2016/04/12 09:30:05, Bernhard Bauer wrote: > > > > > > Before we go much further with this CL, I would like to consider the > > > overall > > > > > > design for a bit, to avoid doing unnecessary changes here. Leaving the > > > issue > > > > > of > > > > > > wildcard user exceptions aside, I'm fairly certain we can get the > > desired > > > > > > behavior by adding a recommended policy content settings provider > below > > > the > > > > > user > > > > > > pref content settings provider. The provider stack would (simplified) > > look > > > > > like > > > > > > this: > > > > > > > > > > > > (highest precedence) > > > > > > mandatory policy > > > > > > user prefs > > > > > > recommended policy > > > > > > hardcoded defaults > > > > > > (lowest precedence) > > > > > > > > > > > > The two policy providers could probably be the same class, with just a > > > flag > > > > > that > > > > > > determines their position in the stack. That is, the mandatory policy > > > > provider > > > > > > would not set any content settings if the policy is set to allow user > > > > > > exceptions, and vice-versa the recommended policy provider would not > set > > > > > > anything if the policy is set not to allow user exceptions (not that > it > > > > would > > > > > > matter). > > > > > > > > > > > > With that approach, we could keep the invariant that content setting > > > > providers > > > > > > are always consulted in order of their precedence, and we could remove > > > some > > > > of > > > > > > the current special-casing for the policy provider in HCSM in this CL. > > > WDYT? > > > > > > > > > > What you describe is how regular prefs work. Content settings were > always > > a > > > > > weird exception that did not support such layering. > > > > > > > > Well, recommended prefs came _way_ after the content settings refactoring > > that > > > > introduced the layering. 😃 > > > > > > > > > A definite +1 to supporting > > > > > it. However, I am not sure this alone will cover the feature request at > > > hand: > > > > > The layering happens on a per-setting basis. The default behavior and a > > > > > URL-specific exception will appear as different entities, so even if > > > mandatory > > > > > policy locks one of them (e.g. the default behavior), users can still > > > specify > > > > > their own value for the other (e.g. a URL-specific exception). We will > > still > > > > > need a mechanism to tell us whether we should honor exceptions and if > so, > > > > > whether only policy-mandated ones or all should be honored. > > > > > > > > Sorry, can you give a specific example? > > > > > > Sure. Say cookies: > > > > > > * There is one content setting "default cookie behavior" > > > * There is another content setting "cookie behavior for example.com" > > > > > > An admin who wants to disable cookies will set the "default cookie behavior" > > to > > > "blocked" via mandatory policy. Since this is the highest-priority source, > > every > > > read of the "default cookie behavior" setting will return "blocked." > However, > > > this does not affect any other settings, including "cookie behavior for > > > example.com." The user can still set that to "allow," trumping the admin > > policy > > > for a specific URL. We still need a policy that will tell us how to > interpret > > > exceptions relative to the default policy. > > > > > > > We query providers in the order of priority; the first provider that stores a > > pattern matching http://example.com answers. Within a provider, we take the > rule with > > the most specific pattern. > > > > Policy provider has a higher priority than pref provider, so it doesn't matter > > that "*" is a less specific pattern than "https://example.com". Policy > provider > > will win. The user-chosen site specific content setting will not override the > > policy-set default setting. > > > > So if I'm not mistaken, this ordering of providers would work: > > Mandatory policy (defaults & exceptions) > > User prefs (exceptions) > > Recommended (exceptions) > > User prefs (defaults) > > Recommended (defaults) > > Hardcoded (defaults) > > Thanks for clarifying. I was not aware that we allow the providers to process > the patterns and come up with a matching value. In that case, layering will work > but we need more than the four standard layers (default, recommended, user, > mandatory), as you also point out. By the way, there is also a UI bug filed for this: https://bugs.chromium.org/p/chromium/issues/detail?id=155516. You are actually the reporter :-) > > Even with the six layers you listed, there still are some use cases we cannot > support though: In current parlance, setting a pref to a mandatory value means > that we do not want the user to change that particular pref. Translating this > into content settings lingo, I would say that setting the default behavior to a > particular value via mandatory policy only means that we want the *default* to > be uncheangeable. It does not say anything about whether we want to allow > exceptions from this default or not. To be fully flexible, we should support > both possibilities - lock down the default and do not allow any exceptions, as > well as lock down default but do allow exceptions. Indeed. This is a bit more difficult to capture within the framework of content settings. Although arguably, if the admin sets a mandatory default policy and allows me to override it by exceptions - semantically, isn't that exactly a *recommendation*? I can change admin's decision on any site that I visit, or if the content setting allows custom exceptions, simply define two exceptions for "https://*" and "http://*" (yes, that's possible). (but I guess this is a bit off topic now, so we can continue the discussion elsewhere) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > > > > > > File > > chrome/browser/content_settings/host_content_settings_map_unittest.cc > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > > > > > > > > chrome/browser/content_settings/host_content_settings_map_unittest.cc:104: > > > > > GURL > > > > > > host(url_str); > > > > > > Should this be |url|? > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > > > > > > > > chrome/browser/content_settings/host_content_settings_map_unittest.cc:855: > > > > > > HostContentSettingsMapTester tester; > > > > > > Move this into the loop to limit its scope? > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/components/content_settings... > > > > > > File > > components/content_settings/core/test/content_settings_test_utils.cc > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/components/content_settings... > > > > > > > components/content_settings/core/test/content_settings_test_utils.cc:54: > > > > bool > > > > > > TestUtils::AddUserException(const std::string& pattern_string, > > > > > > On 2016/04/12 04:52:47, raymes wrote: > > > > > > > Could you instead just call > HostContentSettingsMap::SetContentSetting? > > > It > > > > > will > > > > > > > always end up setting the user's preference. > > > > > > > > > > > > +1 (That's what I meant in my earlier comments).
On 2016/04/12 13:36:12, msramek wrote: > On 2016/04/12 13:12:41, bartfab (slow) wrote: > > On 2016/04/12 12:58:06, msramek wrote: > > > On 2016/04/12 12:45:20, bartfab (slow) wrote: > > > > On 2016/04/12 12:41:26, Bernhard Bauer wrote: > > > > > On 2016/04/12 12:33:07, bartfab (slow) wrote: > > > > > > On 2016/04/12 09:30:05, Bernhard Bauer wrote: > > > > > > > Before we go much further with this CL, I would like to consider the > > > > overall > > > > > > > design for a bit, to avoid doing unnecessary changes here. Leaving > the > > > > issue > > > > > > of > > > > > > > wildcard user exceptions aside, I'm fairly certain we can get the > > > desired > > > > > > > behavior by adding a recommended policy content settings provider > > below > > > > the > > > > > > user > > > > > > > pref content settings provider. The provider stack would > (simplified) > > > look > > > > > > like > > > > > > > this: > > > > > > > > > > > > > > (highest precedence) > > > > > > > mandatory policy > > > > > > > user prefs > > > > > > > recommended policy > > > > > > > hardcoded defaults > > > > > > > (lowest precedence) > > > > > > > > > > > > > > The two policy providers could probably be the same class, with just > a > > > > flag > > > > > > that > > > > > > > determines their position in the stack. That is, the mandatory > policy > > > > > provider > > > > > > > would not set any content settings if the policy is set to allow > user > > > > > > > exceptions, and vice-versa the recommended policy provider would not > > set > > > > > > > anything if the policy is set not to allow user exceptions (not that > > it > > > > > would > > > > > > > matter). > > > > > > > > > > > > > > With that approach, we could keep the invariant that content setting > > > > > providers > > > > > > > are always consulted in order of their precedence, and we could > remove > > > > some > > > > > of > > > > > > > the current special-casing for the policy provider in HCSM in this > CL. > > > > WDYT? > > > > > > > > > > > > What you describe is how regular prefs work. Content settings were > > always > > > a > > > > > > weird exception that did not support such layering. > > > > > > > > > > Well, recommended prefs came _way_ after the content settings > refactoring > > > that > > > > > introduced the layering. 😃 > > > > > > > > > > > A definite +1 to supporting > > > > > > it. However, I am not sure this alone will cover the feature request > at > > > > hand: > > > > > > The layering happens on a per-setting basis. The default behavior and > a > > > > > > URL-specific exception will appear as different entities, so even if > > > > mandatory > > > > > > policy locks one of them (e.g. the default behavior), users can still > > > > specify > > > > > > their own value for the other (e.g. a URL-specific exception). We will > > > still > > > > > > need a mechanism to tell us whether we should honor exceptions and if > > so, > > > > > > whether only policy-mandated ones or all should be honored. > > > > > > > > > > Sorry, can you give a specific example? > > > > > > > > Sure. Say cookies: > > > > > > > > * There is one content setting "default cookie behavior" > > > > * There is another content setting "cookie behavior for example.com" > > > > > > > > An admin who wants to disable cookies will set the "default cookie > behavior" > > > to > > > > "blocked" via mandatory policy. Since this is the highest-priority source, > > > every > > > > read of the "default cookie behavior" setting will return "blocked." > > However, > > > > this does not affect any other settings, including "cookie behavior for > > > > example.com." The user can still set that to "allow," trumping the admin > > > policy > > > > for a specific URL. We still need a policy that will tell us how to > > interpret > > > > exceptions relative to the default policy. > > > > > > > > > > We query providers in the order of priority; the first provider that stores > a > > > pattern matching http://example.com answers. Within a provider, we take the > > rule with > > > the most specific pattern. > > > > > > Policy provider has a higher priority than pref provider, so it doesn't > matter > > > that "*" is a less specific pattern than "https://example.com". Policy > > provider > > > will win. The user-chosen site specific content setting will not override > the > > > policy-set default setting. > > > > > > So if I'm not mistaken, this ordering of providers would work: > > > Mandatory policy (defaults & exceptions) > > > User prefs (exceptions) > > > Recommended (exceptions) > > > User prefs (defaults) > > > Recommended (defaults) > > > Hardcoded (defaults) > > > > Thanks for clarifying. I was not aware that we allow the providers to process > > the patterns and come up with a matching value. In that case, layering will > work > > but we need more than the four standard layers (default, recommended, user, > > mandatory), as you also point out. > > By the way, there is also a UI bug filed for this: > https://bugs.chromium.org/p/chromium/issues/detail?id=155516. You are actually > the reporter :-) > > > > > Even with the six layers you listed, there still are some use cases we cannot > > support though: In current parlance, setting a pref to a mandatory value means > > that we do not want the user to change that particular pref. Translating this > > into content settings lingo, I would say that setting the default behavior to > a > > particular value via mandatory policy only means that we want the *default* to > > be uncheangeable. It does not say anything about whether we want to allow > > exceptions from this default or not. To be fully flexible, we should support > > both possibilities - lock down the default and do not allow any exceptions, as > > well as lock down default but do allow exceptions. > > Indeed. This is a bit more difficult to capture within the framework of content > settings. > > Although arguably, if the admin sets a mandatory default policy and allows me to > override it by exceptions - semantically, isn't that exactly a *recommendation*? > I can change admin's decision on any site that I visit, or if the content > setting allows custom exceptions, simply define two exceptions for "https://*" > and "http://*" (yes, that's possible). > > (but I guess this is a bit off topic now, so we can continue the discussion > elsewhere) I think this is on topic. What granularity of control do we want to provide? Only after we have settled that will we know what the right implementation is. Yes, a default value that you can override could be interpreted as a recommendation. In the prefs system today, the semantics would be slightly different - recommended would mean that you can change the default value itself. Pattern-based exceptions aside, forcing a default value and allowing exceptions can actually be useful: The admin could make sure that by default, e.g. access to your camera is off (to avoid any accidental data leaks) but still allow you to selectively turn it on for sites of your choosing. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > > > > > > > File > > > chrome/browser/content_settings/host_content_settings_map_unittest.cc > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > > > > > > > > > > chrome/browser/content_settings/host_content_settings_map_unittest.cc:104: > > > > > > GURL > > > > > > > host(url_str); > > > > > > > Should this be |url|? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/chrome/browser/content_sett... > > > > > > > > > > chrome/browser/content_settings/host_content_settings_map_unittest.cc:855: > > > > > > > HostContentSettingsMapTester tester; > > > > > > > Move this into the loop to limit its scope? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/components/content_settings... > > > > > > > File > > > components/content_settings/core/test/content_settings_test_utils.cc > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1873343002/diff/1/components/content_settings... > > > > > > > > > components/content_settings/core/test/content_settings_test_utils.cc:54: > > > > > bool > > > > > > > TestUtils::AddUserException(const std::string& pattern_string, > > > > > > > On 2016/04/12 04:52:47, raymes wrote: > > > > > > > > Could you instead just call > > HostContentSettingsMap::SetContentSetting? > > > > It > > > > > > will > > > > > > > > always end up setting the user's preference. > > > > > > > > > > > > > > +1 (That's what I meant in my earlier comments).
Replying some comments for discussion, before changing code. Regarding recommended policy: this seems like the first proposed solution to the main problem, and I experiment with it in https://chromiumcodereview.appspot.com/1825173003/ . This looks like a much bigger change, in terms of UI (creating many more fields in gpedit.msc) and in code (upstream data does not support this). Meanwhile, the feature at hand would break the assumption of having one fixed priority stack: Old behavior: Policy default > User exceptions New option: User exceptions > policy default However, this CL only refactors, and does not touch this. Perhaps I should hurry up on a design doc. Since we're still settling on key architecture changes, it will be more of a "discussion" doc rather than "intent to implement" doc. Thanks! https://chromiumcodereview.appspot.com/1873343002/diff/1/chrome/browser/conte... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://chromiumcodereview.appspot.com/1873343002/diff/1/chrome/browser/conte... chrome/browser/content_settings/host_content_settings_map_unittest.cc:65: HostContentSettingsMapTester& SelectType(ContentSettingsType content_type) { The ability to select "cur" state once to establish context, and avoid repetition was the main purpose of this helper class. But if the preferred style is to explicitly state |content_type| then I can remove this (need feedback). https://chromiumcodereview.appspot.com/1873343002/diff/1/components/content_s... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://chromiumcodereview.appspot.com/1873343002/diff/1/components/content_s... components/content_settings/core/browser/host_content_settings_map.cc:268: bool HostContentSettingsMap::AreUserExceptionsAllowedForType( Yes. The advantage of adding this function now is that we can immediately proceed on the JS work. https://chromiumcodereview.appspot.com/1873343002/diff/1/components/content_s... File components/content_settings/core/browser/host_content_settings_map.h (right): https://chromiumcodereview.appspot.com/1873343002/diff/1/components/content_s... components/content_settings/core/browser/host_content_settings_map.h:88: GetDefaultContentSetting(ContentSettingsType content_type) const; I'm fine with changing all callers (see FIXME(huangs) in the .cc file), but that will bloat this CL. Probably better to omit this from this CL and do all this in a separate CL, with callers and all. https://chromiumcodereview.appspot.com/1873343002/diff/1/components/content_s... File components/content_settings/core/test/content_settings_test_utils.cc (right): https://chromiumcodereview.appspot.com/1873343002/diff/1/components/content_s... components/content_settings/core/test/content_settings_test_utils.cc:54: bool TestUtils::AddUserException(const std::string& pattern_string, But it looks like SetContentSettingCustomScope() -> SetWebsiteSettingCustomScope() loops over all providers? Even if ultimately only user pref gets set, there appears to be mismatch in intent?
Description was changed from
==========
[Policy] Refactor HostContentSettingsMap and add tests.
Relative mechanical changes to HostContentSettingsMap:
- Store PolicyProvider and PrefProvider as typed member functions for
ease of access.
- Add alternatve interface to GetDefaultContentSetting() that returns
ProviderType in a struct, without needing string pointer param.
- Add AreUserExceptionsAllowedForType(). This interface will shortly
be used by front end.
- Add 2 tests to HostContentSettingsMapTest (with supporting routines).
- AreUserExceptionsAllowedForType: testing new interface.
- PrefExceptionsOperation: testing how policy defaults can override
user exceptions.
==========
to
==========
[Policy] HostContentSettingsMap: Add AreUserExceptionsAllowedForType() and
tests.
Simple changes, with some refactoring:
- Add AreUserExceptionsAllowedForType(). This interface will shortly
be used by front end.
- Store PolicyProvider and PrefProvider as typed member functions for
ease of access.
- Add 2 tests to HostContentSettingsMapTest (with supporting routines).
- AreUserExceptionsAllowedForType: testing new interface.
- PrefExceptionsOperation: testing how policy defaults can override
user exceptions.
==========
Updated and read for next round. PTAL. https://chromiumcodereview.appspot.com/1873343002/diff/1/chrome/browser/conte... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://chromiumcodereview.appspot.com/1873343002/diff/1/chrome/browser/conte... chrome/browser/content_settings/host_content_settings_map_unittest.cc:55: class HostContentSettingsMapTester { Changing this to just a wrapper: - Renaming to TesterForType. - TestingProfile will be owned by caller. - Type is injected at construction. https://chromiumcodereview.appspot.com/1873343002/diff/1/chrome/browser/conte... chrome/browser/content_settings/host_content_settings_map_unittest.cc:102: content_settings::SettingSource SourceMatch( On 2016/04/12 04:52:47, raymes wrote: > nit: GetSettingSourceForURL? Done. https://chromiumcodereview.appspot.com/1873343002/diff/1/chrome/browser/conte... chrome/browser/content_settings/host_content_settings_map_unittest.cc:104: GURL host(url_str); On 2016/04/12 09:30:05, Bernhard Bauer wrote: > Should this be |url|? Done. https://chromiumcodereview.appspot.com/1873343002/diff/1/chrome/browser/conte... chrome/browser/content_settings/host_content_settings_map_unittest.cc:855: HostContentSettingsMapTester tester; Done. Note that I moved TestingProfile outside, and now the tester is just a wrapper. https://chromiumcodereview.appspot.com/1873343002/diff/1/chrome/browser/conte... chrome/browser/content_settings/host_content_settings_map_unittest.cc:870: // Not calling tester.ClearPolicyDefault(); let it linger. On 2016/04/12 04:52:47, raymes wrote: > Should we clean up the state? Done. https://chromiumcodereview.appspot.com/1873343002/diff/1/components/content_s... File components/content_settings/core/browser/host_content_settings_map.h (right): https://chromiumcodereview.appspot.com/1873343002/diff/1/components/content_s... components/content_settings/core/browser/host_content_settings_map.h:88: GetDefaultContentSetting(ContentSettingsType content_type) const; Removed from this CL. https://chromiumcodereview.appspot.com/1873343002/diff/1/components/content_s... File components/content_settings/core/test/content_settings_test_utils.cc (right): https://chromiumcodereview.appspot.com/1873343002/diff/1/components/content_s... components/content_settings/core/test/content_settings_test_utils.cc:54: bool TestUtils::AddUserException(const std::string& pattern_string, Anyway, if that's the convention then I'll use it. Removing the new routines in TestUtils, but keeping minor cleanup.
Updated and ready for next round. PTAL.
Ping for review. Thanks!
Overall this looks ok to me but I know Bernhard wanted to settle on the overall design first, so I'll wait for him to comment on that :) Thanks! https://chromiumcodereview.appspot.com/1873343002/diff/40001/components/conte... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://chromiumcodereview.appspot.com/1873343002/diff/40001/components/conte... components/content_settings/core/browser/host_content_settings_map.cc:256: // Allowed iff default content setting does not came from policy provider. // Allowed if default content setting does not come from policy provider.
Updated, PTAL. Please note that this change is rather agnostic of whichever alternative we take. https://chromiumcodereview.appspot.com/1873343002/diff/40001/components/conte... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://chromiumcodereview.appspot.com/1873343002/diff/40001/components/conte... components/content_settings/core/browser/host_content_settings_map.cc:256: // Allowed iff default content setting does not came from policy provider. Here iff means "if and only if", but that's rather obscure... Rephrased.
looks good https://codereview.chromium.org/1873343002/diff/60001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1873343002/diff/60001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:156: pref_provider_ = nit: i find the double assignment to be "clever" rather than clean. i think it's more readable as it was before: pref_provider_ = new content_settings::PrefProvider(...); pref_provider_->AddObserver(this); content_settings_providers_[PREF_PROVIDER] = pref_provider_; https://codereview.chromium.org/1873343002/diff/60001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1873343002/diff/60001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.h:135: // Returns whether user exceptions are allowed for |content_type|. nit: "whether" -> "if" https://codereview.chromium.org/1873343002/diff/60001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.h:380: content_settings::PolicyProvider* policy_provider_ = nullptr; this member is unused, please remove. https://codereview.chromium.org/1873343002/diff/60001/components/content_sett... File components/content_settings/core/test/content_settings_test_utils.cc (right): https://codereview.chromium.org/1873343002/diff/60001/components/content_sett... components/content_settings/core/test/content_settings_test_utils.cc:8: #include "components/content_settings/core/common/content_settings_pattern.h" unused, no? a forward decl of "class ContentSettingsPattern;" should suffice.
Updated. OWNER review for bauerb@, PTAL. https://chromiumcodereview.appspot.com/1873343002/diff/60001/components/conte... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://chromiumcodereview.appspot.com/1873343002/diff/60001/components/conte... components/content_settings/core/browser/host_content_settings_map.cc:156: pref_provider_ = Done. I did that to avoid having a naked pointers in one line, and using scoped_ptr / unique_ptr seemed excessive. https://chromiumcodereview.appspot.com/1873343002/diff/60001/components/conte... File components/content_settings/core/browser/host_content_settings_map.h (right): https://chromiumcodereview.appspot.com/1873343002/diff/60001/components/conte... components/content_settings/core/browser/host_content_settings_map.h:135: // Returns whether user exceptions are allowed for |content_type|. Changed to "true if"; "Return if" sounds like it won't return otherwise. https://chromiumcodereview.appspot.com/1873343002/diff/60001/components/conte... components/content_settings/core/browser/host_content_settings_map.h:380: content_settings::PolicyProvider* policy_provider_ = nullptr; On 2016/04/19 01:57:34, grt wrote: > this member is unused, please remove. Done. https://chromiumcodereview.appspot.com/1873343002/diff/60001/components/conte... File components/content_settings/core/test/content_settings_test_utils.cc (right): https://chromiumcodereview.appspot.com/1873343002/diff/60001/components/conte... components/content_settings/core/test/content_settings_test_utils.cc:8: #include "components/content_settings/core/common/content_settings_pattern.h" Ah, didn't realize we can do this for .cc files as well. Removed.
https://codereview.chromium.org/1873343002/diff/1/components/content_settings... File components/content_settings/core/test/content_settings_test_utils.cc (right): https://codereview.chromium.org/1873343002/diff/1/components/content_settings... components/content_settings/core/test/content_settings_test_utils.cc:54: bool TestUtils::AddUserException(const std::string& pattern_string, On 2016/04/12 14:19:14, huangs wrote: > But it looks like > > SetContentSettingCustomScope() > -> SetWebsiteSettingCustomScope() > > loops over all providers? Even if ultimately only user pref gets set, there > appears to be mismatch in intent? No, this was written to not treat the pref provider differently from the others -- it just happens to be the only one that accepts a write. https://codereview.chromium.org/1873343002/diff/80001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1873343002/diff/80001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:261: kProviderNamesSourceMap[POLICY_PROVIDER].provider_name; Do we also want to take supervised user settings or extension-defined content settings into account here? I could imagine getting the provider source (i.e. the enum value), and numerically comparing it to PREF_PROVIDER.
https://codereview.chromium.org/1873343002/diff/1/components/content_settings... File components/content_settings/core/test/content_settings_test_utils.cc (right): https://codereview.chromium.org/1873343002/diff/1/components/content_settings... components/content_settings/core/test/content_settings_test_utils.cc:54: bool TestUtils::AddUserException(const std::string& pattern_string, On 2016/04/12 14:19:14, huangs wrote: > But it looks like > > SetContentSettingCustomScope() > -> SetWebsiteSettingCustomScope() > > loops over all providers? Even if ultimately only user pref gets set, there > appears to be mismatch in intent? No, this was written to not treat the pref provider differently from the others -- it just happens to be the only one that accepts a write. https://codereview.chromium.org/1873343002/diff/80001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1873343002/diff/80001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:261: kProviderNamesSourceMap[POLICY_PROVIDER].provider_name; Do we also want to take supervised user settings or extension-defined content settings into account here? I could imagine getting the provider source (i.e. the enum value), and numerically comparing it to PREF_PROVIDER.
Replied to comment, update to follow later today. https://chromiumcodereview.appspot.com/1873343002/diff/80001/components/conte... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://chromiumcodereview.appspot.com/1873343002/diff/80001/components/conte... components/content_settings/core/browser/host_content_settings_map.cc:261: kProviderNamesSourceMap[POLICY_PROVIDER].provider_name; Supvervised user settings https://support.google.com/chrome/answer/3463947?hl=en appears to block/allow websites entirely without going into content setting details. Content settings https://developer.chrome.com/extensions/contentSettings seem to have default changes, so indeed we should add this. Re. comparing enum value instead of string: That was why I added the extra GetDefaultContentSetting() interface in revision #1. Will bring it back under a different name, and make it private.
https://codereview.chromium.org/1873343002/diff/80001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1873343002/diff/80001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:261: kProviderNamesSourceMap[POLICY_PROVIDER].provider_name; On 2016/04/20 15:20:32, huangs wrote: > Supvervised user settings > https://support.google.com/chrome/answer/3463947?hl=en > appears to block/allow websites entirely without going into content setting > details. Supervised user settings can also affect content settings: There is a supervised user setting that can disable usage of camera/mic and geolocation, by setting wildcard rules that block these content setting types.
https://chromiumcodereview.appspot.com/1873343002/diff/80001/components/conte... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://chromiumcodereview.appspot.com/1873343002/diff/80001/components/conte... components/content_settings/core/browser/host_content_settings_map.cc:261: kProviderNamesSourceMap[POLICY_PROVIDER].provider_name; It might be easier if we just let GetDefaultContentSetting() do the work, and check whether default content setting is controlled by DEFAULT_PROVIDER?
https://codereview.chromium.org/1873343002/diff/80001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1873343002/diff/80001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:261: kProviderNamesSourceMap[POLICY_PROVIDER].provider_name; On 2016/04/20 15:40:41, huangs wrote: > It might be easier if we just let GetDefaultContentSetting() do the work, and > check whether default content setting is controlled by DEFAULT_PROVIDER? Well, we'd want something that will also work once we add recommended content settings, right? :) Hence my suggestion to check whether it's coming from anything above user prefs.
https://chromiumcodereview.appspot.com/1873343002/diff/80001/components/conte... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://chromiumcodereview.appspot.com/1873343002/diff/80001/components/conte... components/content_settings/core/browser/host_content_settings_map.cc:261: kProviderNamesSourceMap[POLICY_PROVIDER].provider_name; Or we can just get the numerical index, and do a comparison for priority? Instead of simple "<", maybe wrap this in a helper function to (a) make intention clear, (b) accommodate future changes?
https://codereview.chromium.org/1873343002/diff/80001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1873343002/diff/80001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:261: kProviderNamesSourceMap[POLICY_PROVIDER].provider_name; On 2016/04/20 16:07:48, huangs wrote: > Or we can just get the numerical index, and do a comparison for priority? Yeah, that is how I would have suggested to implement it :) > Instead of simple "<", maybe wrap this in a helper function to (a) make > intention clear, (b) accommodate future changes? Sure (although I think it would probably also be fine without one, as usage of it is rather limited.)
Description was changed from
==========
[Policy] HostContentSettingsMap: Add AreUserExceptionsAllowedForType() and
tests.
Simple changes, with some refactoring:
- Add AreUserExceptionsAllowedForType(). This interface will shortly
be used by front end.
- Store PolicyProvider and PrefProvider as typed member functions for
ease of access.
- Add 2 tests to HostContentSettingsMapTest (with supporting routines).
- AreUserExceptionsAllowedForType: testing new interface.
- PrefExceptionsOperation: testing how policy defaults can override
user exceptions.
==========
to
==========
[Policy] HostContentSettingsMap: Add AreUserExceptionsAllowedForType() and
tests.
Simple changes, with some refactoring:
- Add AreUserExceptionsAllowedForType(). This interface will shortly
be used by front end.
- Store PrefProvider as typed member functions for ease of access.
- Add 2 tests to HostContentSettingsMapTest (with supporting routines).
- AreUserExceptionsAllowedForType: testing new interface.
- PrefExceptionsOperation: testing how policy defaults can override
user exceptions.
==========
Updated, PTAL. https://chromiumcodereview.appspot.com/1873343002/diff/80001/components/conte... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://chromiumcodereview.appspot.com/1873343002/diff/80001/components/conte... components/content_settings/core/browser/host_content_settings_map.cc:261: kProviderNamesSourceMap[POLICY_PROVIDER].provider_name; Okay, did this using just numerical comparison.
The CQ bit was checked by huangs@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1873343002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1873343002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by huangs@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1873343002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1873343002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping for review. Thanks!
LGTM https://codereview.chromium.org/1873343002/diff/100001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1873343002/diff/100001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:271: if (content_setting == CONTENT_SETTING_DEFAULT) I think we could even DCHECK that this does not happen -- a content setting should always have a default (and yeah, getting a value of CONTENT_SETTING_DEFAULT means we _don't_ have a default. Heh 😃).
Thanks! Committing. https://chromiumcodereview.appspot.com/1873343002/diff/100001/components/cont... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://chromiumcodereview.appspot.com/1873343002/diff/100001/components/cont... components/content_settings/core/browser/host_content_settings_map.cc:271: if (content_setting == CONTENT_SETTING_DEFAULT) Indeed, heh. Adding DCHECK().
The CQ bit was checked by huangs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1873343002/#ps120001 (title: "Add DCHECK in AreUserExceptionsAllowedForType().")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1873343002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1873343002/120001
Message was sent while issue was closed.
Description was changed from
==========
[Policy] HostContentSettingsMap: Add AreUserExceptionsAllowedForType() and
tests.
Simple changes, with some refactoring:
- Add AreUserExceptionsAllowedForType(). This interface will shortly
be used by front end.
- Store PrefProvider as typed member functions for ease of access.
- Add 2 tests to HostContentSettingsMapTest (with supporting routines).
- AreUserExceptionsAllowedForType: testing new interface.
- PrefExceptionsOperation: testing how policy defaults can override
user exceptions.
==========
to
==========
[Policy] HostContentSettingsMap: Add AreUserExceptionsAllowedForType() and
tests.
Simple changes, with some refactoring:
- Add AreUserExceptionsAllowedForType(). This interface will shortly
be used by front end.
- Store PrefProvider as typed member functions for ease of access.
- Add 2 tests to HostContentSettingsMapTest (with supporting routines).
- AreUserExceptionsAllowedForType: testing new interface.
- PrefExceptionsOperation: testing how policy defaults can override
user exceptions.
==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from
==========
[Policy] HostContentSettingsMap: Add AreUserExceptionsAllowedForType() and
tests.
Simple changes, with some refactoring:
- Add AreUserExceptionsAllowedForType(). This interface will shortly
be used by front end.
- Store PrefProvider as typed member functions for ease of access.
- Add 2 tests to HostContentSettingsMapTest (with supporting routines).
- AreUserExceptionsAllowedForType: testing new interface.
- PrefExceptionsOperation: testing how policy defaults can override
user exceptions.
==========
to
==========
[Policy] HostContentSettingsMap: Add AreUserExceptionsAllowedForType() and
tests.
Simple changes, with some refactoring:
- Add AreUserExceptionsAllowedForType(). This interface will shortly
be used by front end.
- Store PrefProvider as typed member functions for ease of access.
- Add 2 tests to HostContentSettingsMapTest (with supporting routines).
- AreUserExceptionsAllowedForType: testing new interface.
- PrefExceptionsOperation: testing how policy defaults can override
user exceptions.
Committed: https://crrev.com/3973014f0fbf36ec4a961e97273516b2ed1953d6
Cr-Commit-Position: refs/heads/master@{#388885}
==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/3973014f0fbf36ec4a961e97273516b2ed1953d6 Cr-Commit-Position: refs/heads/master@{#388885} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
