Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6253012:
Add ContentSettingsProvider Interface. (Closed)
|
Created:
9 years, 11 months ago by markusheintz_ Modified:
9 years, 6 months ago CC:
chromium-reviews, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd ContentSettingsProvider Interface.
BUG=63656
TEST=TBA
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=73279
Patch Set 1 #
Total comments: 27
Patch Set 2 : " #
Total comments: 2
Patch Set 3 : Fix lints nits and rename *CSP to *DefaultCSP #Patch Set 4 : Removing ContentSettingIdentifier type #Patch Set 5 : Add ContentSettingsTypeIsManaged method #Patch Set 6 : Remove obsolete classes #
Total comments: 8
Patch Set 7 : Re-Add "Interface" suffix to interface class names. #Patch Set 8 : Fix nits and rename ContentSettingsProviderInterface to ProviderInterface #
Total comments: 2
Patch Set 9 : " #Patch Set 10 : " #Patch Set 11 : content_settings::ProviderInterface only #Patch Set 12 : " #
Total comments: 3
Patch Set 13 : Remove ContentSettingsTypeIsManaged method from ProviderInterface #
Total comments: 2
Patch Set 14 : " #
Total comments: 14
Patch Set 15 : Fix nits and last comments. #
Total comments: 8
Patch Set 16 : " #
Total comments: 6
Patch Set 17 : " #
Messages
Total messages: 54 (0 generated)
Please review. Thanks @Jochen: Following up our email conversation: I think you are right that having these struct like classes will result in having code like foo(NewClass(a,b,c)). But it will also allow us to have something like foo(NewClass(a)) in case b,c is not needed and to use sensible default values. It will also allow to have specialized subclasses in case we need to treat pairs special for some content_types. I also allow to bundle code to handle the parameter much better. @bauerb,jochen Concerning these little classes: I guess it might be good to make them value objects, but this would lead to a lot of copying so I started differently.
On 2011/01/26 09:45:26, markusheintz_ wrote: > Please review. Thanks > > @Jochen: Following up our email conversation: I think you are right that having > these struct like classes will result in having code like foo(NewClass(a,b,c)). > > But it will also allow us to have something like foo(NewClass(a)) in case b,c is > not needed and to use sensible default values. It will also allow to have > specialized subclasses in case we need to treat pairs special for some > content_types. I also allow to bundle code to handle the parameter much better. Overloaded constructors are not allowed in the styleguide. However, we could just as easily add fooWithA(a) convenience methods and only pass around NewClass objects internally. > @bauerb,jochen > Concerning these little classes: I guess it might be good to make them value > objects, but this would lead to a lot of copying so I started differently. What do you mean by value objects here? (For me, a value object is an immutable object with referential transparency, so it has the same semantics as a simple value type like int). http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... File chrome/browser/content_settings/content_identifier.h (right): http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_identifier.h:21: const ResourceIdentifier& resource_identifier); Are you going to implement these declared methods as well? http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... File chrome/browser/content_settings/content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_provider.h:14: #include "chrome/browser/content_settings/host_pattern.h" If these are classes, I guess you could just forward-declare them. http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_provider.h:17: using content_settings::ContentIdentifier; We don't use namespace imports in header files, only in implementation files, in order not to pollute the namespaces of every files that includes this header. You could put these classes into the content_settings namespace though. http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_provider.h:59: // Nit: remove empty comment lines. http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_provider.h:71: const Host& host, Should this take a pattern? http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... File chrome/browser/content_settings/content_settings_rule.h (right): http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_rule.h:22: const ContentSetting& setting) : Nit: colon on the following line. http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_rule.h:51: typedef std::vector<ContentSettingsRulePtr> ContentSettingsRules; Nit: I think we usually have typedef's before class definitions (you might have to forward-declare ContentSettingsRule). http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... File chrome/browser/content_settings/host_pattern.h (right): http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/host_pattern.h:17: const ContentSettingsPattern& embedding_url_pattern) : Nit: fix indentation and put the colon on the following line.
All those new small classes make the interface very hard to understand. e.g. I think of a Host as something like www.google.com. In fact it is (http://www.google.com/whatver, http://www.someothersite.org/). Also, they shouldn't be classes, but at best typedefs. And the typedefs should probably go into the ProviderInterface Anyway, I'd suggest to leave them out for now. http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... File chrome/browser/content_settings/content_identifier.h (right): http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_identifier.h:15: typedef std::string ResourceIdentifier; I think this should be part of the ProviderInterface, and in a separate namespace. http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_identifier.h:17: class ContentIdentifier { This class should be std::pair<ContentSettingsType, ResourceIdentifier> http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... File chrome/browser/content_settings/content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_provider.h:55: class ContentSettingsProvider { public: missing http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_provider.h:56: virtual ~ContentSettingsProvider() {} where is isManaged, isReadOnly, and canProvide? Probably also getProviderType and enum { USER_PREFS, POLICIES, WEBAPPS } http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... File chrome/browser/content_settings/content_settings_rule.h (right): http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_rule.h:19: public: this should be std::pair<>
http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... File chrome/browser/content_settings/content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_provider.h:56: virtual ~ContentSettingsProvider() {} On 2011/01/26 11:29:18, jochen wrote: > where is isManaged, isReadOnly, and canProvide? > > Probably also getProviderType and enum { USER_PREFS, POLICIES, WEBAPPS } We don't need it anymore with the new interface :) For example, the only writable ContentSettingProvider is the one reading from user preferences, so the HostContentSettingsMap can just keep track of it separately.
Well ... somehow this degrades more and more to copying code. We start slowly taking out step by step little parts Bernhard and I discussed. At least it seems to me that we are heading more and more in this direction. For the sake of making progress and to facilitate finding a consent on the API, I feel more and more that copying code is the right approach for now. So I'll pause on this for now and copy the code first. We can take it from there then as this will shortcut some discussions around the API. I'm still convinced that the API here is the right direction. On 2011/01/26 11:34:23, Bernhard Bauer wrote: > http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... > File chrome/browser/content_settings/content_settings_provider.h (right): > > http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... > chrome/browser/content_settings/content_settings_provider.h:56: virtual > ~ContentSettingsProvider() {} > On 2011/01/26 11:29:18, jochen wrote: > > where is isManaged, isReadOnly, and canProvide? > > > > Probably also getProviderType and enum { USER_PREFS, POLICIES, WEBAPPS } > > We don't need it anymore with the new interface :) For example, the only > writable ContentSettingProvider is the one reading from user preferences, so the > HostContentSettingsMap can just keep track of it separately.
On 2011/01/26 11:34:23, Bernhard Bauer wrote: > http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... > File chrome/browser/content_settings/content_settings_provider.h (right): > > http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... > chrome/browser/content_settings/content_settings_provider.h:56: virtual > ~ContentSettingsProvider() {} > On 2011/01/26 11:29:18, jochen wrote: > > where is isManaged, isReadOnly, and canProvide? > > > > Probably also getProviderType and enum { USER_PREFS, POLICIES, WEBAPPS } > > We don't need it anymore with the new interface :) For example, the only > writable ContentSettingProvider is the one reading from user preferences, so the > HostContentSettingsMap can just keep track of it separately. the enum is something Markus wanted for the UI the others are required imo. The map shouldn't rely on certain providers to be there, but ask the providers for their capabilities.
On Wed, Jan 26, 2011 at 15:08, <jochen@chromium.org> wrote: > On 2011/01/26 11:34:23, Bernhard Bauer wrote: > > http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... >> >> File chrome/browser/content_settings/content_settings_provider.h (right): > > > http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... >> >> chrome/browser/content_settings/content_settings_provider.h:56: virtual >> ~ContentSettingsProvider() {} >> On 2011/01/26 11:29:18, jochen wrote: >> > where is isManaged, isReadOnly, and canProvide? >> > >> > Probably also getProviderType and enum { USER_PREFS, POLICIES, WEBAPPS } > >> We don't need it anymore with the new interface :) For example, the only >> writable ContentSettingProvider is the one reading from user preferences, >> so > > the >> >> HostContentSettingsMap can just keep track of it separately. > > the enum is something Markus wanted for the UI I think for the enum we can do something similar to what we do in the PrefService -- we ask the HostContentSettingsMap which Provider provides the setting for a given host. That way, only the HostContentSettingsMap has to know about the kinds of Providers (which it needs to anyway). > the others are required imo. The map shouldn't rely on certain providers to > be > there, but ask the providers for their capabilities. If the number of writable Providers is less than one, your changes aren't going to be saved, if it's more than one, you get redundancy in the best and inconsistencies in the worst case. So we might as well make the fact that there is going to be exactly one writable Provider explicit. > > http://codereview.chromium.org/6253012/ >
Please see CL http://codereview.chromium.org/6242013/ and let me know if you are fine with adding a LegacyInterface so that I can also start copying the content_settings logic out of the HCSM. I'll address your comments for this CL now. On 2011/01/26 14:17:57, Bernhard Bauer wrote: > On Wed, Jan 26, 2011 at 15:08, <mailto:jochen@chromium.org> wrote: > > On 2011/01/26 11:34:23, Bernhard Bauer wrote: > > > > > http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... > >> > >> File chrome/browser/content_settings/content_settings_provider.h (right): > > > > > > > http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... > >> > >> chrome/browser/content_settings/content_settings_provider.h:56: virtual > >> ~ContentSettingsProvider() {} > >> On 2011/01/26 11:29:18, jochen wrote: > >> > where is isManaged, isReadOnly, and canProvide? > >> > > >> > Probably also getProviderType and enum { USER_PREFS, POLICIES, WEBAPPS } > > > >> We don't need it anymore with the new interface :) For example, the only > >> writable ContentSettingProvider is the one reading from user preferences, > >> so > > > > the > >> > >> HostContentSettingsMap can just keep track of it separately. > > > > the enum is something Markus wanted for the UI > > I think for the enum we can do something similar to what we do in the > PrefService -- we ask the HostContentSettingsMap which Provider > provides the setting for a given host. That way, only the > HostContentSettingsMap has to know about the kinds of Providers (which > it needs to anyway). > > > the others are required imo. The map shouldn't rely on certain providers to > > be > > there, but ask the providers for their capabilities. > > If the number of writable Providers is less than one, your changes > aren't going to be saved, if it's more than one, you get redundancy in > the best and inconsistencies in the worst case. So we might as well > make the fact that there is going to be exactly one writable Provider > explicit. > > > > > http://codereview.chromium.org/6253012/ > >
Please check if I used the namespace definitions according to the style guide and whether you are fine with what I put in the namespace content_settings. http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... File chrome/browser/content_settings/content_identifier.h (right): http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_identifier.h:15: typedef std::string ResourceIdentifier; On 2011/01/26 11:29:18, jochen wrote: > I think this should be part of the ProviderInterface, and in a separate > namespace. Moved into the ContentSettingsProvider class. http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_identifier.h:17: class ContentIdentifier { On 2011/01/26 11:29:18, jochen wrote: > This class should be std::pair<ContentSettingsType, ResourceIdentifier> Defined the pair in the ContentSettingsProvider Interface. http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_identifier.h:21: const ResourceIdentifier& resource_identifier); On 2011/01/26 10:25:18, Bernhard Bauer wrote: > Are you going to implement these declared methods as well? uups will do if we keep the class. otherwise obsolete. http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... File chrome/browser/content_settings/content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_provider.h:14: #include "chrome/browser/content_settings/host_pattern.h" On 2011/01/26 10:25:18, Bernhard Bauer wrote: > If these are classes, I guess you could just forward-declare them. Will fix when clear which of this classes stay. http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_provider.h:17: using content_settings::ContentIdentifier; On 2011/01/26 10:25:18, Bernhard Bauer wrote: > We don't use namespace imports in header files, only in implementation files, in > order not to pollute the namespaces of every files that includes this header. > > You could put these classes into the content_settings namespace though. I put the ContentSettingsProvider in the content_settings namespace. http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_provider.h:55: class ContentSettingsProvider { On 2011/01/26 11:29:18, jochen wrote: > public: missing Done. http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_provider.h:56: virtual ~ContentSettingsProvider() {} On 2011/01/26 11:29:18, jochen wrote: > where is isManaged, isReadOnly, and canProvide? > > Probably also getProviderType and enum { USER_PREFS, POLICIES, WEBAPPS } Don't need them. I think the enum should live in the HCSM. I guess it's enougth. Don't think the provider needs such a method as long as the HCSP knows which provider is which that is fine. http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_provider.h:59: // On 2011/01/26 10:25:18, Bernhard Bauer wrote: > Nit: remove empty comment lines. Done. http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_provider.h:71: const Host& host, On 2011/01/26 10:25:18, Bernhard Bauer wrote: > Should this take a pattern? of course! :) done http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... File chrome/browser/content_settings/content_settings_rule.h (right): http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_rule.h:19: public: On 2011/01/26 11:29:18, jochen wrote: > this should be std::pair<> Will not work if we keep the embedder URL. http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_rule.h:22: const ContentSetting& setting) : On 2011/01/26 10:25:18, Bernhard Bauer wrote: > Nit: colon on the following line. Will fix if we keep the class http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/content_settings_rule.h:51: typedef std::vector<ContentSettingsRulePtr> ContentSettingsRules; On 2011/01/26 10:25:18, Bernhard Bauer wrote: > Nit: I think we usually have typedef's before class definitions (you might have > to forward-declare ContentSettingsRule). Will fix if we keep the class http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... File chrome/browser/content_settings/host_pattern.h (right): http://codereview.chromium.org/6253012/diff/1/chrome/browser/content_settings... chrome/browser/content_settings/host_pattern.h:17: const ContentSettingsPattern& embedding_url_pattern) : On 2011/01/26 10:25:18, Bernhard Bauer wrote: > Nit: fix indentation and put the colon on the following line. Will fix if we keep the class.
Just a small nit for now, but I'm holding back review of the rest until we have a final candidate. http://codereview.chromium.org/6253012/diff/15001/chrome/browser/content_sett... File chrome/browser/content_settings/host_content_settings_map.h (right): http://codereview.chromium.org/6253012/diff/15001/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map.h:29: class DefaultContentSettingsProvider; Nit: Don't indent.
Fixed the nit some lints and renamed the {Pref,Policy}ContentSettingsProvider to
{Pref,Policy}DefaultContentSettingsProvider
http://codereview.chromium.org/6253012/diff/15001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.h (right):
http://codereview.chromium.org/6253012/diff/15001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.h:29: class
DefaultContentSettingsProvider;
On 2011/01/26 15:51:59, Bernhard Bauer wrote:
> Nit: Don't indent.
Done.
Updated the CL based on our email discussion.
On 2011/01/27 09:01:09, markusheintz_ wrote: > Updated the CL based on our email discussion. Thanks! Can't reply inline from android... Can you please remove all changes except for the new interface? In the header, do you really need utility? I don't think we need a namespace here. Can you move Rules inside the.provider interface?
On 2011/01/27 09:48:26, jochen wrote: > On 2011/01/27 09:01:09, markusheintz_ wrote: > > Updated the CL based on our email discussion. > Thanks! > > Can't reply inline from android... > > Can you please remove all changes except for the new interface? > > In the header, do you really need utility? > > I don't think we need a namespace here. The namespace is useful if it allows us to shorten the class names, e.g. a DefaultContentSettingsProviderInterface could become a content_settings::DefaultProviderInterface, and just a DefaultProviderInterface inside the namespace / when |using| the class. > Can you move Rules inside the.provider interface?
http://codereview.chromium.org/6253012/diff/33001/chrome/browser/content_sett... File chrome/browser/content_settings/content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/33001/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:56: : requesting_url_pattern_(requesting_url_pattern), Nit: The colon should be indented four spaces w/r/t the preceding level, which is two spaces. http://codereview.chromium.org/6253012/diff/33001/chrome/browser/content_sett... File chrome/browser/content_settings/policy_content_settings_provider.cc (right): http://codereview.chromium.org/6253012/diff/33001/chrome/browser/content_sett... chrome/browser/content_settings/policy_content_settings_provider.cc:45: : profile_(profile), Indent only four spaces in total here as well. http://codereview.chromium.org/6253012/diff/33001/chrome/browser/content_sett... File chrome/browser/content_settings/pref_content_settings_provider.cc (right): http://codereview.chromium.org/6253012/diff/33001/chrome/browser/content_sett... chrome/browser/content_settings/pref_content_settings_provider.cc:66: : profile_(profile), And here. http://codereview.chromium.org/6253012/diff/33001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/6253012/diff/33001/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:810: 'browser/content_settings/content_settings_pattern.cc', Nit: replace tab with spaces.
On 2011/01/27 09:54:14, Bernhard Bauer wrote: > On 2011/01/27 09:48:26, jochen wrote: > > On 2011/01/27 09:01:09, markusheintz_ wrote: > > > Updated the CL based on our email discussion. > > Thanks! > > > > Can't reply inline from android... > > > > Can you please remove all changes except for the new interface? > > > > In the header, do you really need utility? > > > > I don't think we need a namespace here. > > The namespace is useful if it allows us to shorten the class names, e.g. a > DefaultContentSettingsProviderInterface could become a > content_settings::DefaultProviderInterface, and just a DefaultProviderInterface > inside the namespace / when |using| the class. > Either remove the namespace or shorten the names then :-) > > Can you move Rules inside the.provider interface?
I shorted the names and kept the namespaces. Should I really revert renaming the PrefContentSettingsProvider to PrefDefaultContentSettingsProvider and putting them into a namespace? Shouldn't I rather also shorten the names as well? http://codereview.chromium.org/6253012/diff/33001/chrome/browser/content_sett... File chrome/browser/content_settings/content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/33001/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:56: : requesting_url_pattern_(requesting_url_pattern), On 2011/01/27 09:54:26, Bernhard Bauer wrote: > Nit: The colon should be indented four spaces w/r/t the preceding level, which > is two spaces. Done. http://codereview.chromium.org/6253012/diff/33001/chrome/browser/content_sett... File chrome/browser/content_settings/policy_content_settings_provider.cc (right): http://codereview.chromium.org/6253012/diff/33001/chrome/browser/content_sett... chrome/browser/content_settings/policy_content_settings_provider.cc:45: : profile_(profile), On 2011/01/27 09:54:26, Bernhard Bauer wrote: > Indent only four spaces in total here as well. Done. http://codereview.chromium.org/6253012/diff/33001/chrome/browser/content_sett... File chrome/browser/content_settings/pref_content_settings_provider.cc (right): http://codereview.chromium.org/6253012/diff/33001/chrome/browser/content_sett... chrome/browser/content_settings/pref_content_settings_provider.cc:66: : profile_(profile), On 2011/01/27 09:54:26, Bernhard Bauer wrote: > And here. Done. http://codereview.chromium.org/6253012/diff/33001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/6253012/diff/33001/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:810: 'browser/content_settings/content_settings_pattern.cc', On 2011/01/27 09:54:26, Bernhard Bauer wrote: > Nit: replace tab with spaces. Done.
I guess you are right separating the changes. Are you fine with me having a CL to rename the *ContentSettingsProvider to *DefaultContentSettingsProvider. On 2011/01/27 10:24:37, markusheintz_ wrote: > I shorted the names and kept the namespaces. > > Should I really revert renaming the PrefContentSettingsProvider to > PrefDefaultContentSettingsProvider and putting them into a namespace? > > Shouldn't I rather also shorten the names as well? > > http://codereview.chromium.org/6253012/diff/33001/chrome/browser/content_sett... > File chrome/browser/content_settings/content_settings_provider.h (right): > > http://codereview.chromium.org/6253012/diff/33001/chrome/browser/content_sett... > chrome/browser/content_settings/content_settings_provider.h:56: : > requesting_url_pattern_(requesting_url_pattern), > On 2011/01/27 09:54:26, Bernhard Bauer wrote: > > Nit: The colon should be indented four spaces w/r/t the preceding level, which > > is two spaces. > > Done. > > http://codereview.chromium.org/6253012/diff/33001/chrome/browser/content_sett... > File chrome/browser/content_settings/policy_content_settings_provider.cc > (right): > > http://codereview.chromium.org/6253012/diff/33001/chrome/browser/content_sett... > chrome/browser/content_settings/policy_content_settings_provider.cc:45: : > profile_(profile), > On 2011/01/27 09:54:26, Bernhard Bauer wrote: > > Indent only four spaces in total here as well. > > Done. > > http://codereview.chromium.org/6253012/diff/33001/chrome/browser/content_sett... > File chrome/browser/content_settings/pref_content_settings_provider.cc (right): > > http://codereview.chromium.org/6253012/diff/33001/chrome/browser/content_sett... > chrome/browser/content_settings/pref_content_settings_provider.cc:66: : > profile_(profile), > On 2011/01/27 09:54:26, Bernhard Bauer wrote: > > And here. > > Done. > > http://codereview.chromium.org/6253012/diff/33001/chrome/chrome_browser.gypi > File chrome/chrome_browser.gypi (right): > > http://codereview.chromium.org/6253012/diff/33001/chrome/chrome_browser.gypi#... > chrome/chrome_browser.gypi:810: > 'browser/content_settings/content_settings_pattern.cc', > On 2011/01/27 09:54:26, Bernhard Bauer wrote: > > Nit: replace tab with spaces. > > Done.
http://codereview.chromium.org/6253012/diff/10/chrome/browser/content_setting... File chrome/browser/content_settings/content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/10/chrome/browser/content_setting... chrome/browser/content_settings/content_settings_provider.h:54: struct Rule { Sorry to create conflicts with jochen's comments, but I think I'd actually prefer to have Rule outside of ProviderInterface, and even in a separate file. That way, you can forward-declare it everywhere it's only passed around, and you also don't need to include content_settings_pattern.h here. http://codereview.chromium.org/6253012/diff/10/chrome/browser/content_setting... chrome/browser/content_settings/content_settings_provider.h:59: : requesting_url_pattern_(requesting_url_pattern), Nit: Indent two more spaces :)
On 2011/01/27 11:03:48, Bernhard Bauer wrote: > http://codereview.chromium.org/6253012/diff/10/chrome/browser/content_setting... > File chrome/browser/content_settings/content_settings_provider.h (right): > > http://codereview.chromium.org/6253012/diff/10/chrome/browser/content_setting... > chrome/browser/content_settings/content_settings_provider.h:54: struct Rule { > Sorry to create conflicts with jochen's comments, but I think I'd actually > prefer to have Rule outside of ProviderInterface, and even in a separate file. > That way, you can forward-declare it everywhere it's only passed around, and you > also don't need to include content_settings_pattern.h here. I think this would make sense if this struct was supposed to be visible from the outside. However, it should only be used between the host content settings map and the providers (which have to include this header anyway). For communication with the UI, we already have HostContentSettingsMap::SettingsForOneType which should contain additional info e.g. whether the given rule can be edited or not. I don't think it's a good idea to use the same type as changing something in the backend would force us to update the UI as well. > > http://codereview.chromium.org/6253012/diff/10/chrome/browser/content_setting... > chrome/browser/content_settings/content_settings_provider.h:59: : > requesting_url_pattern_(requesting_url_pattern), > Nit: Indent two more spaces :)
I agree with Jochen here. Outside the HostContentSettingsMap you either: - The CONTENT_SETTING or - A struct like (url, url, res_id, content_settings, read_only) or similar That is different from the Rule though it includes the same info. So I'd say keep the rule inside for now and see what's next. We can alway take it out later. On Thu, Jan 27, 2011 at 1:27 PM, <jochen@chromium.org> wrote: > On 2011/01/27 11:03:48, Bernhard Bauer wrote: > > > http://codereview.chromium.org/6253012/diff/10/chrome/browser/content_setting... > >> File chrome/browser/content_settings/content_settings_provider.h (right): >> > > > > http://codereview.chromium.org/6253012/diff/10/chrome/browser/content_setting... > >> chrome/browser/content_settings/content_settings_provider.h:54: struct >> Rule { >> Sorry to create conflicts with jochen's comments, but I think I'd actually >> prefer to have Rule outside of ProviderInterface, and even in a separate >> file. >> That way, you can forward-declare it everywhere it's only passed around, >> and >> > you > >> also don't need to include content_settings_pattern.h here. >> > > I think this would make sense if this struct was supposed to be visible > from the > outside. However, it should only be used between the host content settings > map > and the providers (which have to include this header anyway). > > For communication with the UI, we already have > HostContentSettingsMap::SettingsForOneType which should contain additional > info > e.g. whether the given rule can be edited or not. I don't think it's a good > idea > to use the same type as changing something in the backend would force us to > update the UI as well. > > > > > > http://codereview.chromium.org/6253012/diff/10/chrome/browser/content_setting... > >> chrome/browser/content_settings/content_settings_provider.h:59: : >> requesting_url_pattern_(requesting_url_pattern), >> Nit: Indent two more spaces :) >> > > > > http://codereview.chromium.org/6253012/ > -- Markus
Please continue with the review. NOW: content_settings::ProviderInterface changes only.
http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... File chrome/browser/content_settings/content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:73: ContentSettingsType content_type) = 0; Do we need this? What level of strictness does managed mean here?
http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... File chrome/browser/content_settings/content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:73: ContentSettingsType content_type) = 0; On 2011/01/31 13:48:58, Bernhard Bauer wrote: > Do we need this? What level of strictness does managed mean here? Actually I think we could get rid of this.
http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... File chrome/browser/content_settings/content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:73: ContentSettingsType content_type) = 0; On 2011/01/31 14:21:19, markusheintz_ wrote: > On 2011/01/31 13:48:58, Bernhard Bauer wrote: > > Do we need this? What level of strictness does managed mean here? > > Actually I think we could get rid of this. If an provider returns true here, the user's settings are ignored (webapps are also ignored then) If all return false, the user can define exceptions.
On Mon, Jan 31, 2011 at 3:30 PM, <jochen@chromium.org> wrote: > > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > File chrome/browser/content_settings/content_settings_provider.h > (right): > > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > chrome/browser/content_settings/content_settings_provider.h:73: > ContentSettingsType content_type) = 0; > On 2011/01/31 14:21:19, markusheintz_ wrote: > >> On 2011/01/31 13:48:58, Bernhard Bauer wrote: >> > Do we need this? What level of strictness does managed mean here? >> > > Actually I think we could get rid of this. >> > > If an provider returns true here, the user's settings are ignored > (webapps are also ignored then) > > If all return false, the user can define exceptions. > > > As a long term goal I'd like to store default_content_settings as a [*] "match_all_pattern". Then we would not need such a method anymore. In the mean while the host content settings map could handle the case you described. > http://codereview.chromium.org/6253012/ > -- Markus
On 2011/01/31 14:39:34, markusheintz wrote: > On Mon, Jan 31, 2011 at 3:30 PM, <mailto:jochen@chromium.org> wrote: > > > > > > > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > > File chrome/browser/content_settings/content_settings_provider.h > > (right): > > > > > > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > > chrome/browser/content_settings/content_settings_provider.h:73: > > ContentSettingsType content_type) = 0; > > On 2011/01/31 14:21:19, markusheintz_ wrote: > > > >> On 2011/01/31 13:48:58, Bernhard Bauer wrote: > >> > Do we need this? What level of strictness does managed mean here? > >> > > > > Actually I think we could get rid of this. > >> > > > > If an provider returns true here, the user's settings are ignored > > (webapps are also ignored then) > > > > If all return false, the user can define exceptions. > > > > > > > As a long term goal I'd like to store default_content_settings as a [*] > "match_all_pattern". Then we would not need such a method anymore. > > In the mean while the host content settings map could handle the case you > described. > Where would it know from that the user must not create exceptions? > > > > http://codereview.chromium.org/6253012/ > > > > > > -- > Markus
The same source the provider would know. On Mon, Jan 31, 2011 at 3:42 PM, <jochen@chromium.org> wrote: > On 2011/01/31 14:39:34, markusheintz wrote: > > On Mon, Jan 31, 2011 at 3:30 PM, <mailto:jochen@chromium.org> wrote: >> > > > >> > >> > >> > > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > >> > File chrome/browser/content_settings/content_settings_provider.h >> > (right): >> > >> > >> > >> > > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > >> > chrome/browser/content_settings/content_settings_provider.h:73: >> > ContentSettingsType content_type) = 0; >> > On 2011/01/31 14:21:19, markusheintz_ wrote: >> > >> >> On 2011/01/31 13:48:58, Bernhard Bauer wrote: >> >> > Do we need this? What level of strictness does managed mean here? >> >> >> > >> > Actually I think we could get rid of this. >> >> >> > >> > If an provider returns true here, the user's settings are ignored >> > (webapps are also ignored then) >> > >> > If all return false, the user can define exceptions. >> > >> > >> > >> As a long term goal I'd like to store default_content_settings as a [*] >> "match_all_pattern". Then we would not need such a method anymore. >> > > In the mean while the host content settings map could handle the case you >> described. >> > > > Where would it know from that the user must not create exceptions? > > > > > http://codereview.chromium.org/6253012/ >> > >> > > > > -- >> Markus >> > > > > http://codereview.chromium.org/6253012/ > -- Markus
Ideally, only the policy provider knows how to check whether something is managed. On 2011/01/31 14:43:55, markusheintz wrote: > The same source the provider would know. > > On Mon, Jan 31, 2011 at 3:42 PM, <mailto:jochen@chromium.org> wrote: > > > On 2011/01/31 14:39:34, markusheintz wrote: > > > > On Mon, Jan 31, 2011 at 3:30 PM, <mailto:jochen@chromium.org> wrote: > >> > > > > > > >> > > >> > > >> > > > > > > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > > > >> > File chrome/browser/content_settings/content_settings_provider.h > >> > (right): > >> > > >> > > >> > > >> > > > > > > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > > > >> > chrome/browser/content_settings/content_settings_provider.h:73: > >> > ContentSettingsType content_type) = 0; > >> > On 2011/01/31 14:21:19, markusheintz_ wrote: > >> > > >> >> On 2011/01/31 13:48:58, Bernhard Bauer wrote: > >> >> > Do we need this? What level of strictness does managed mean here? > >> >> > >> > > >> > Actually I think we could get rid of this. > >> >> > >> > > >> > If an provider returns true here, the user's settings are ignored > >> > (webapps are also ignored then) > >> > > >> > If all return false, the user can define exceptions. > >> > > >> > > >> > > >> As a long term goal I'd like to store default_content_settings as a [*] > >> "match_all_pattern". Then we would not need such a method anymore. > >> > > > > In the mean while the host content settings map could handle the case you > >> described. > >> > > > > > > Where would it know from that the user must not create exceptions? > > > > > > > > > http://codereview.chromium.org/6253012/ > >> > > >> > > > > > > > > -- > >> Markus > >> > > > > > > > > http://codereview.chromium.org/6253012/ > > > > > > -- > Markus
To be exact, the host content settings map would know that the content settings are coming from a higher-precedence source than the user preferences, so it should DCHECK that the UI doesn't try to create exceptions when they wouldn't be effective. On Mon, Jan 31, 2011 at 15:43, Markus Heintz <markusheintz@google.com> wrote: > The same source the provider would know. > > On Mon, Jan 31, 2011 at 3:42 PM, <jochen@chromium.org> wrote: >> >> On 2011/01/31 14:39:34, markusheintz wrote: >>> >>> On Mon, Jan 31, 2011 at 3:30 PM, <mailto:jochen@chromium.org> wrote: >> >>> > >>> > >>> > >> >> >> http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... >>> >>> > File chrome/browser/content_settings/content_settings_provider.h >>> > (right): >>> > >>> > >>> > >> >> >> http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... >>> >>> > chrome/browser/content_settings/content_settings_provider.h:73: >>> > ContentSettingsType content_type) = 0; >>> > On 2011/01/31 14:21:19, markusheintz_ wrote: >>> > >>> >> On 2011/01/31 13:48:58, Bernhard Bauer wrote: >>> >> > Do we need this? What level of strictness does managed mean here? >>> >> >>> > >>> > Actually I think we could get rid of this. >>> >> >>> > >>> > If an provider returns true here, the user's settings are ignored >>> > (webapps are also ignored then) >>> > >>> > If all return false, the user can define exceptions. >>> > >>> > >>> > >>> As a long term goal I'd like to store default_content_settings as a [*] >>> "match_all_pattern". Then we would not need such a method anymore. >> >>> In the mean while the host content settings map could handle the case you >>> described. >> >> >> Where would it know from that the user must not create exceptions? >> >> >>> > http://codereview.chromium.org/6253012/ >>> > >> >> >> >>> -- >>> Markus >> >> >> >> http://codereview.chromium.org/6253012/ > > > > -- > Markus > >
Once we replaced the default content settings with a "match_all_pattern" the managed content settings provider will be the only on to know. On Mon, Jan 31, 2011 at 3:51 PM, <jochen@chromium.org> wrote: > Ideally, only the policy provider knows how to check whether something is > managed. > > > > > On 2011/01/31 14:43:55, markusheintz wrote: > >> The same source the provider would know. >> > > On Mon, Jan 31, 2011 at 3:42 PM, <mailto:jochen@chromium.org> wrote: >> > > > On 2011/01/31 14:39:34, markusheintz wrote: >> > >> > On Mon, Jan 31, 2011 at 3:30 PM, <mailto:jochen@chromium.org> wrote: >> >> >> > >> > > >> >> > >> >> > >> >> >> > >> > >> > >> > > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > >> > >> >> > File chrome/browser/content_settings/content_settings_provider.h >> >> > (right): >> >> > >> >> > >> >> > >> >> >> > >> > >> > >> > > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > >> > >> >> > chrome/browser/content_settings/content_settings_provider.h:73: >> >> > ContentSettingsType content_type) = 0; >> >> > On 2011/01/31 14:21:19, markusheintz_ wrote: >> >> > >> >> >> On 2011/01/31 13:48:58, Bernhard Bauer wrote: >> >> >> > Do we need this? What level of strictness does managed mean here? >> >> >> >> >> > >> >> > Actually I think we could get rid of this. >> >> >> >> >> > >> >> > If an provider returns true here, the user's settings are ignored >> >> > (webapps are also ignored then) >> >> > >> >> > If all return false, the user can define exceptions. >> >> > >> >> > >> >> > >> >> As a long term goal I'd like to store default_content_settings as a [*] >> >> "match_all_pattern". Then we would not need such a method anymore. >> >> >> > >> > In the mean while the host content settings map could handle the case >> you >> >> described. >> >> >> > >> > >> > Where would it know from that the user must not create exceptions? >> > >> > >> > >> > > http://codereview.chromium.org/6253012/ >> >> > >> >> >> > >> > >> > >> > -- >> >> Markus >> >> >> > >> > >> > >> > http://codereview.chromium.org/6253012/ >> > >> > > > > -- >> Markus >> > > > > http://codereview.chromium.org/6253012/ > -- Markus
On 2011/01/31 14:52:08, Bernhard Bauer wrote: > To be exact, the host content settings map would know that the content > settings are coming from a higher-precedence source than the user > preferences, so it should DCHECK that the UI doesn't try to create > exceptions when they wouldn't be effective. That's something different. Managed means the exceptions editor is disabled. Even if no policy controlled exceptions exist, the user can't create any > > On Mon, Jan 31, 2011 at 15:43, Markus Heintz <mailto:markusheintz@google.com> wrote: > > The same source the provider would know. > > > > On Mon, Jan 31, 2011 at 3:42 PM, <mailto:jochen@chromium.org> wrote: > >> > >> On 2011/01/31 14:39:34, markusheintz wrote: > >>> > >>> On Mon, Jan 31, 2011 at 3:30 PM, <mailto:jochen@chromium.org> wrote: > >> > >>> > > >>> > > >>> > > >> > >> > >> > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > >>> > >>> > File chrome/browser/content_settings/content_settings_provider.h > >>> > (right): > >>> > > >>> > > >>> > > >> > >> > >> > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > >>> > >>> > chrome/browser/content_settings/content_settings_provider.h:73: > >>> > ContentSettingsType content_type) = 0; > >>> > On 2011/01/31 14:21:19, markusheintz_ wrote: > >>> > > >>> >> On 2011/01/31 13:48:58, Bernhard Bauer wrote: > >>> >> > Do we need this? What level of strictness does managed mean here? > >>> >> > >>> > > >>> > Actually I think we could get rid of this. > >>> >> > >>> > > >>> > If an provider returns true here, the user's settings are ignored > >>> > (webapps are also ignored then) > >>> > > >>> > If all return false, the user can define exceptions. > >>> > > >>> > > >>> > > >>> As a long term goal I'd like to store default_content_settings as a [*] > >>> "match_all_pattern". Then we would not need such a method anymore. > >> > >>> In the mean while the host content settings map could handle the case you > >>> described. > >> > >> > >> Where would it know from that the user must not create exceptions? > >> > >> > >>> > http://codereview.chromium.org/6253012/ > >>> > > >> > >> > >> > >>> -- > >>> Markus > >> > >> > >> > >> http://codereview.chromium.org/6253012/ > > > > > > > > -- > > Markus > > > >
On Mon, Jan 31, 2011 at 16:00, <jochen@chromium.org> wrote: > On 2011/01/31 14:52:08, Bernhard Bauer wrote: >> >> To be exact, the host content settings map would know that the content >> settings are coming from a higher-precedence source than the user >> preferences, so it should DCHECK that the UI doesn't try to create >> exceptions when they wouldn't be effective. > > > That's something different. Managed means the exceptions editor is disabled. That is one way for the UI to make sure no exceptions are set. > Even if no policy controlled exceptions exist, the user can't create any It's not exceptions that would be controlled by policy in this case, it would be defaults. If exceptions are controlled by policy (meaning if there are are exceptions that are provided by policy), the user would be not allowed to create exceptions matching the same or a more specific pattern, just as the user is not allowed to create any exceptions when the defaults are policy-controlled. >> On Mon, Jan 31, 2011 at 15:43, Markus Heintz >> <mailto:markusheintz@google.com> > > wrote: >> >> > The same source the provider would know. >> > >> > On Mon, Jan 31, 2011 at 3:42 PM, <mailto:jochen@chromium.org> wrote: >> >> >> >> On 2011/01/31 14:39:34, markusheintz wrote: >> >>> >> >>> On Mon, Jan 31, 2011 at 3:30 PM, <mailto:jochen@chromium.org> wrote: >> >> >> >>> > >> >>> > >> >>> > >> >> >> >> >> >> > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... >> >> >>> >> >>> > File chrome/browser/content_settings/content_settings_provider.h >> >>> > (right): >> >>> > >> >>> > >> >>> > >> >> >> >> >> >> > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... >> >> >>> >> >>> > chrome/browser/content_settings/content_settings_provider.h:73: >> >>> > ContentSettingsType content_type) = 0; >> >>> > On 2011/01/31 14:21:19, markusheintz_ wrote: >> >>> > >> >>> >> On 2011/01/31 13:48:58, Bernhard Bauer wrote: >> >>> >> > Do we need this? What level of strictness does managed mean here? >> >>> >> >> >>> > >> >>> > Actually I think we could get rid of this. >> >>> >> >> >>> > >> >>> > If an provider returns true here, the user's settings are ignored >> >>> > (webapps are also ignored then) >> >>> > >> >>> > If all return false, the user can define exceptions. >> >>> > >> >>> > >> >>> > >> >>> As a long term goal I'd like to store default_content_settings as a >> >>> [*] >> >>> "match_all_pattern". Then we would not need such a method anymore. >> >> >> >>> In the mean while the host content settings map could handle the case >> >>> you >> >>> described. >> >> >> >> >> >> Where would it know from that the user must not create exceptions? >> >> >> >> >> >>> > http://codereview.chromium.org/6253012/ >> >>> > >> >> >> >> >> >> >> >>> -- >> >>> Markus >> >> >> >> >> >> >> >> http://codereview.chromium.org/6253012/ >> > >> > >> > >> > -- >> > Markus >> > >> > > > > > http://codereview.chromium.org/6253012/ >
We want to support the case (1) default managed, user can create exceptions in addition to policy provided ones (2) default managed, user cannot create exceptions, and (3) nothing managed On 2011/01/31 15:05:42, Bernhard Bauer wrote: > On Mon, Jan 31, 2011 at 16:00, <mailto:jochen@chromium.org> wrote: > > On 2011/01/31 14:52:08, Bernhard Bauer wrote: > >> > >> To be exact, the host content settings map would know that the content > >> settings are coming from a higher-precedence source than the user > >> preferences, so it should DCHECK that the UI doesn't try to create > >> exceptions when they wouldn't be effective. > > > > > > That's something different. Managed means the exceptions editor is disabled. > > That is one way for the UI to make sure no exceptions are set. > > > Even if no policy controlled exceptions exist, the user can't create any > > It's not exceptions that would be controlled by policy in this case, > it would be defaults. > > If exceptions are controlled by policy (meaning if there are are > exceptions that are provided by policy), the user would be not allowed > to create exceptions matching the same or a more specific pattern, > just as the user is not allowed to create any exceptions when the > defaults are policy-controlled. > > >> On Mon, Jan 31, 2011 at 15:43, Markus Heintz > >> <mailto:markusheintz@google.com> > > > > wrote: > >> > >> > The same source the provider would know. > >> > > >> > On Mon, Jan 31, 2011 at 3:42 PM, <mailto:jochen@chromium.org> wrote: > >> >> > >> >> On 2011/01/31 14:39:34, markusheintz wrote: > >> >>> > >> >>> On Mon, Jan 31, 2011 at 3:30 PM, <mailto:jochen@chromium.org> wrote: > >> >> > >> >>> > > >> >>> > > >> >>> > > >> >> > >> >> > >> >> > > > > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > >> > >> >>> > >> >>> > File chrome/browser/content_settings/content_settings_provider.h > >> >>> > (right): > >> >>> > > >> >>> > > >> >>> > > >> >> > >> >> > >> >> > > > > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > >> > >> >>> > >> >>> > chrome/browser/content_settings/content_settings_provider.h:73: > >> >>> > ContentSettingsType content_type) = 0; > >> >>> > On 2011/01/31 14:21:19, markusheintz_ wrote: > >> >>> > > >> >>> >> On 2011/01/31 13:48:58, Bernhard Bauer wrote: > >> >>> >> > Do we need this? What level of strictness does managed mean here? > >> >>> >> > >> >>> > > >> >>> > Actually I think we could get rid of this. > >> >>> >> > >> >>> > > >> >>> > If an provider returns true here, the user's settings are ignored > >> >>> > (webapps are also ignored then) > >> >>> > > >> >>> > If all return false, the user can define exceptions. > >> >>> > > >> >>> > > >> >>> > > >> >>> As a long term goal I'd like to store default_content_settings as a > >> >>> [*] > >> >>> "match_all_pattern". Then we would not need such a method anymore. > >> >> > >> >>> In the mean while the host content settings map could handle the case > >> >>> you > >> >>> described. > >> >> > >> >> > >> >> Where would it know from that the user must not create exceptions? > >> >> > >> >> > >> >>> > http://codereview.chromium.org/6253012/ > >> >>> > > >> >> > >> >> > >> >> > >> >>> -- > >> >>> Markus > >> >> > >> >> > >> >> > >> >> http://codereview.chromium.org/6253012/ > >> > > >> > > >> > > >> > -- > >> > Markus > >> > > >> > > > > > > > > > http://codereview.chromium.org/6253012/ > >
There are two use cases: 1) Admin wants to manage default and allow the user to set exceptions 2) Admin wants to manage the default but not allow the user to set exceptions On Mon, Jan 31, 2011 at 4:05 PM, Bernhard Bauer <bauerb@chromium.org> wrote: > On Mon, Jan 31, 2011 at 16:00, <jochen@chromium.org> wrote: > > On 2011/01/31 14:52:08, Bernhard Bauer wrote: > >> > >> To be exact, the host content settings map would know that the content > >> settings are coming from a higher-precedence source than the user > >> preferences, so it should DCHECK that the UI doesn't try to create > >> exceptions when they wouldn't be effective. > > > > > > That's something different. Managed means the exceptions editor is > disabled. > > That is one way for the UI to make sure no exceptions are set. > > > Even if no policy controlled exceptions exist, the user can't create any > > It's not exceptions that would be controlled by policy in this case, > it would be defaults. > > If exceptions are controlled by policy (meaning if there are are > exceptions that are provided by policy), the user would be not allowed > to create exceptions matching the same or a more specific pattern, > just as the user is not allowed to create any exceptions when the > defaults are policy-controlled. > > >> On Mon, Jan 31, 2011 at 15:43, Markus Heintz > >> <mailto:markusheintz@google.com> > > > > wrote: > >> > >> > The same source the provider would know. > >> > > >> > On Mon, Jan 31, 2011 at 3:42 PM, <mailto:jochen@chromium.org> wrote: > >> >> > >> >> On 2011/01/31 14:39:34, markusheintz wrote: > >> >>> > >> >>> On Mon, Jan 31, 2011 at 3:30 PM, <mailto:jochen@chromium.org> > wrote: > >> >> > >> >>> > > >> >>> > > >> >>> > > >> >> > >> >> > >> >> > > > > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > >> > >> >>> > >> >>> > File chrome/browser/content_settings/content_settings_provider.h > >> >>> > (right): > >> >>> > > >> >>> > > >> >>> > > >> >> > >> >> > >> >> > > > > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > >> > >> >>> > >> >>> > chrome/browser/content_settings/content_settings_provider.h:73: > >> >>> > ContentSettingsType content_type) = 0; > >> >>> > On 2011/01/31 14:21:19, markusheintz_ wrote: > >> >>> > > >> >>> >> On 2011/01/31 13:48:58, Bernhard Bauer wrote: > >> >>> >> > Do we need this? What level of strictness does managed mean > here? > >> >>> >> > >> >>> > > >> >>> > Actually I think we could get rid of this. > >> >>> >> > >> >>> > > >> >>> > If an provider returns true here, the user's settings are ignored > >> >>> > (webapps are also ignored then) > >> >>> > > >> >>> > If all return false, the user can define exceptions. > >> >>> > > >> >>> > > >> >>> > > >> >>> As a long term goal I'd like to store default_content_settings as a > >> >>> [*] > >> >>> "match_all_pattern". Then we would not need such a method anymore. > >> >> > >> >>> In the mean while the host content settings map could handle the > case > >> >>> you > >> >>> described. > >> >> > >> >> > >> >> Where would it know from that the user must not create exceptions? > >> >> > >> >> > >> >>> > http://codereview.chromium.org/6253012/ > >> >>> > > >> >> > >> >> > >> >> > >> >>> -- > >> >>> Markus > >> >> > >> >> > >> >> > >> >> http://codereview.chromium.org/6253012/ > >> > > >> > > >> > > >> > -- > >> > Markus > >> > > >> > > > > > > > > > http://codereview.chromium.org/6253012/ > > > -- Markus
I don't think we are actually in disagreement here. All I'm saying is I think we can get rid of the "ContentSettingsTypeIsManaged" flag, as we want to support more fine-grained use cases (like managed--but-overridable defaults), and we can already get all the information from the providers without an additional flag. On Mon, Jan 31, 2011 at 16:14, Markus Heintz <markusheintz@google.com> wrote: > There are two use cases: > 1) Admin wants to manage default and allow the user to set exceptions > 2) Admin wants to manage the default but not allow the user to set > exceptions > > On Mon, Jan 31, 2011 at 4:05 PM, Bernhard Bauer <bauerb@chromium.org> wrote: >> >> On Mon, Jan 31, 2011 at 16:00, <jochen@chromium.org> wrote: >> > On 2011/01/31 14:52:08, Bernhard Bauer wrote: >> >> >> >> To be exact, the host content settings map would know that the content >> >> settings are coming from a higher-precedence source than the user >> >> preferences, so it should DCHECK that the UI doesn't try to create >> >> exceptions when they wouldn't be effective. >> > >> > >> > That's something different. Managed means the exceptions editor is >> > disabled. >> >> That is one way for the UI to make sure no exceptions are set. >> >> > Even if no policy controlled exceptions exist, the user can't create any >> >> It's not exceptions that would be controlled by policy in this case, >> it would be defaults. >> >> If exceptions are controlled by policy (meaning if there are are >> exceptions that are provided by policy), the user would be not allowed >> to create exceptions matching the same or a more specific pattern, >> just as the user is not allowed to create any exceptions when the >> defaults are policy-controlled. >> >> >> On Mon, Jan 31, 2011 at 15:43, Markus Heintz >> >> <mailto:markusheintz@google.com> >> > >> > wrote: >> >> >> >> > The same source the provider would know. >> >> > >> >> > On Mon, Jan 31, 2011 at 3:42 PM, <mailto:jochen@chromium.org> wrote: >> >> >> >> >> >> On 2011/01/31 14:39:34, markusheintz wrote: >> >> >>> >> >> >>> On Mon, Jan 31, 2011 at 3:30 PM, <mailto:jochen@chromium.org> >> >> >>> wrote: >> >> >> >> >> >>> > >> >> >>> > >> >> >>> > >> >> >> >> >> >> >> >> >> >> > >> > >> > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... >> >> >> >> >>> >> >> >>> > File chrome/browser/content_settings/content_settings_provider.h >> >> >>> > (right): >> >> >>> > >> >> >>> > >> >> >>> > >> >> >> >> >> >> >> >> >> >> > >> > >> > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... >> >> >> >> >>> >> >> >>> > chrome/browser/content_settings/content_settings_provider.h:73: >> >> >>> > ContentSettingsType content_type) = 0; >> >> >>> > On 2011/01/31 14:21:19, markusheintz_ wrote: >> >> >>> > >> >> >>> >> On 2011/01/31 13:48:58, Bernhard Bauer wrote: >> >> >>> >> > Do we need this? What level of strictness does managed mean >> >> >>> >> > here? >> >> >>> >> >> >> >>> > >> >> >>> > Actually I think we could get rid of this. >> >> >>> >> >> >> >>> > >> >> >>> > If an provider returns true here, the user's settings are ignored >> >> >>> > (webapps are also ignored then) >> >> >>> > >> >> >>> > If all return false, the user can define exceptions. >> >> >>> > >> >> >>> > >> >> >>> > >> >> >>> As a long term goal I'd like to store default_content_settings as a >> >> >>> [*] >> >> >>> "match_all_pattern". Then we would not need such a method anymore. >> >> >> >> >> >>> In the mean while the host content settings map could handle the >> >> >>> case >> >> >>> you >> >> >>> described. >> >> >> >> >> >> >> >> >> Where would it know from that the user must not create exceptions? >> >> >> >> >> >> >> >> >>> > http://codereview.chromium.org/6253012/ >> >> >>> > >> >> >> >> >> >> >> >> >> >> >> >>> -- >> >> >>> Markus >> >> >> >> >> >> >> >> >> >> >> >> http://codereview.chromium.org/6253012/ >> >> > >> >> > >> >> > >> >> > -- >> >> > Markus >> >> > >> >> > >> > >> > >> > >> > http://codereview.chromium.org/6253012/ >> > > > > > -- > Markus > >
On 2011/01/31 15:26:37, Bernhard Bauer wrote: > I don't think we are actually in disagreement here. All I'm saying is > I think we can get rid of the "ContentSettingsTypeIsManaged" flag, as > we want to support more fine-grained use cases (like > managed--but-overridable defaults), and we can already get all the > information from the providers without an additional flag. How do you decide whether (a) the policy hasn't set any exceptions, manages the default, and the user is allowed to add exceptions, or (b) the policy hasn't set any exceptions, manages the default, and the user is _not_ allowed to add exceptions? > > On Mon, Jan 31, 2011 at 16:14, Markus Heintz <mailto:markusheintz@google.com> wrote: > > There are two use cases: > > 1) Admin wants to manage default and allow the user to set exceptions > > 2) Admin wants to manage the default but not allow the user to set > > exceptions > > > > On Mon, Jan 31, 2011 at 4:05 PM, Bernhard Bauer <mailto:bauerb@chromium.org> wrote: > >> > >> On Mon, Jan 31, 2011 at 16:00, mailto: <jochen@chromium.org> wrote: > >> > On 2011/01/31 14:52:08, Bernhard Bauer wrote: > >> >> > >> >> To be exact, the host content settings map would know that the content > >> >> settings are coming from a higher-precedence source than the user > >> >> preferences, so it should DCHECK that the UI doesn't try to create > >> >> exceptions when they wouldn't be effective. > >> > > >> > > >> > That's something different. Managed means the exceptions editor is > >> > disabled. > >> > >> That is one way for the UI to make sure no exceptions are set. > >> > >> > Even if no policy controlled exceptions exist, the user can't create any > >> > >> It's not exceptions that would be controlled by policy in this case, > >> it would be defaults. > >> > >> If exceptions are controlled by policy (meaning if there are are > >> exceptions that are provided by policy), the user would be not allowed > >> to create exceptions matching the same or a more specific pattern, > >> just as the user is not allowed to create any exceptions when the > >> defaults are policy-controlled. > >> > >> >> On Mon, Jan 31, 2011 at 15:43, Markus Heintz > >> >> <mailto:markusheintz@google.com> > >> > > >> > wrote: > >> >> > >> >> > The same source the provider would know. > >> >> > > >> >> > On Mon, Jan 31, 2011 at 3:42 PM, <mailto:jochen@chromium.org> wrote: > >> >> >> > >> >> >> On 2011/01/31 14:39:34, markusheintz wrote: > >> >> >>> > >> >> >>> On Mon, Jan 31, 2011 at 3:30 PM, <mailto:jochen@chromium.org> > >> >> >>> wrote: > >> >> >> > >> >> >>> > > >> >> >>> > > >> >> >>> > > >> >> >> > >> >> >> > >> >> >> > >> > > >> > > >> > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > >> >> > >> >> >>> > >> >> >>> > File chrome/browser/content_settings/content_settings_provider.h > >> >> >>> > (right): > >> >> >>> > > >> >> >>> > > >> >> >>> > > >> >> >> > >> >> >> > >> >> >> > >> > > >> > > >> > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > >> >> > >> >> >>> > >> >> >>> > chrome/browser/content_settings/content_settings_provider.h:73: > >> >> >>> > ContentSettingsType content_type) = 0; > >> >> >>> > On 2011/01/31 14:21:19, markusheintz_ wrote: > >> >> >>> > > >> >> >>> >> On 2011/01/31 13:48:58, Bernhard Bauer wrote: > >> >> >>> >> > Do we need this? What level of strictness does managed mean > >> >> >>> >> > here? > >> >> >>> >> > >> >> >>> > > >> >> >>> > Actually I think we could get rid of this. > >> >> >>> >> > >> >> >>> > > >> >> >>> > If an provider returns true here, the user's settings are ignored > >> >> >>> > (webapps are also ignored then) > >> >> >>> > > >> >> >>> > If all return false, the user can define exceptions. > >> >> >>> > > >> >> >>> > > >> >> >>> > > >> >> >>> As a long term goal I'd like to store default_content_settings as a > >> >> >>> [*] > >> >> >>> "match_all_pattern". Then we would not need such a method anymore. > >> >> >> > >> >> >>> In the mean while the host content settings map could handle the > >> >> >>> case > >> >> >>> you > >> >> >>> described. > >> >> >> > >> >> >> > >> >> >> Where would it know from that the user must not create exceptions? > >> >> >> > >> >> >> > >> >> >>> > http://codereview.chromium.org/6253012/ > >> >> >>> > > >> >> >> > >> >> >> > >> >> >> > >> >> >>> -- > >> >> >>> Markus > >> >> >> > >> >> >> > >> >> >> > >> >> >> http://codereview.chromium.org/6253012/ > >> >> > > >> >> > > >> >> > > >> >> > -- > >> >> > Markus > >> >> > > >> >> > > >> > > >> > > >> > > >> > http://codereview.chromium.org/6253012/ > >> > > > > > > > > > -- > > Markus > > > >
Bernhard answered: "For case (a) the default setting is set in the recommended content settings provider, for case (b) it's in the managed content settings provider." In case (a), the user could then also override the default setting which is not intended. (Note that we currently do not have match-all patterns, so you can't write a "default" exception).
For case (a) the default setting is set in the recommended content settings provider, for case (b) it's in the managed content settings provider. On Mon, Jan 31, 2011 at 16:48, <jochen@chromium.org> wrote: > On 2011/01/31 15:26:37, Bernhard Bauer wrote: >> >> I don't think we are actually in disagreement here. All I'm saying is >> I think we can get rid of the "ContentSettingsTypeIsManaged" flag, as >> we want to support more fine-grained use cases (like >> managed--but-overridable defaults), and we can already get all the >> information from the providers without an additional flag. > > How do you decide whether (a) the policy hasn't set any exceptions, manages > the > default, and the user is allowed to add exceptions, or (b) the policy hasn't > set > any exceptions, manages the default, and the user is _not_ allowed to add > exceptions? > > >> On Mon, Jan 31, 2011 at 16:14, Markus Heintz >> <mailto:markusheintz@google.com> > > wrote: >> >> > There are two use cases: >> > 1) Admin wants to manage default and allow the user to set exceptions >> > 2) Admin wants to manage the default but not allow the user to set >> > exceptions >> > >> > On Mon, Jan 31, 2011 at 4:05 PM, Bernhard Bauer >> > <mailto:bauerb@chromium.org> > > wrote: >> >> >> >> >> On Mon, Jan 31, 2011 at 16:00, mailto: <jochen@chromium.org> wrote: >> >> > On 2011/01/31 14:52:08, Bernhard Bauer wrote: >> >> >> >> >> >> To be exact, the host content settings map would know that the >> >> >> content >> >> >> settings are coming from a higher-precedence source than the user >> >> >> preferences, so it should DCHECK that the UI doesn't try to create >> >> >> exceptions when they wouldn't be effective. >> >> > >> >> > >> >> > That's something different. Managed means the exceptions editor is >> >> > disabled. >> >> >> >> That is one way for the UI to make sure no exceptions are set. >> >> >> >> > Even if no policy controlled exceptions exist, the user can't create >> >> > any >> >> >> >> It's not exceptions that would be controlled by policy in this case, >> >> it would be defaults. >> >> >> >> If exceptions are controlled by policy (meaning if there are are >> >> exceptions that are provided by policy), the user would be not allowed >> >> to create exceptions matching the same or a more specific pattern, >> >> just as the user is not allowed to create any exceptions when the >> >> defaults are policy-controlled. >> >> >> >> >> On Mon, Jan 31, 2011 at 15:43, Markus Heintz >> >> >> <mailto:markusheintz@google.com> >> >> > >> >> > wrote: >> >> >> >> >> >> > The same source the provider would know. >> >> >> > >> >> >> > On Mon, Jan 31, 2011 at 3:42 PM, <mailto:jochen@chromium.org> >> >> >> > wrote: >> >> >> >> >> >> >> >> On 2011/01/31 14:39:34, markusheintz wrote: >> >> >> >>> >> >> >> >>> On Mon, Jan 31, 2011 at 3:30 PM, <mailto:jochen@chromium.org> >> >> >> >>> wrote: >> >> >> >> >> >> >> >>> > >> >> >> >>> > >> >> >> >>> > >> >> >> >> >> >> >> >> >> >> >> >> >> >> > >> >> > >> >> > > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... >> >> >> >> >> >> >> >>> >> >> >> >>> > File >> >> >> >>> > chrome/browser/content_settings/content_settings_provider.h >> >> >> >>> > (right): >> >> >> >>> > >> >> >> >>> > >> >> >> >>> > >> >> >> >> >> >> >> >> >> >> >> >> >> >> > >> >> > >> >> > > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... >> >> >> >> >> >> >> >>> >> >> >> >>> > >> >> >> >>> > chrome/browser/content_settings/content_settings_provider.h:73: >> >> >> >>> > ContentSettingsType content_type) = 0; >> >> >> >>> > On 2011/01/31 14:21:19, markusheintz_ wrote: >> >> >> >>> > >> >> >> >>> >> On 2011/01/31 13:48:58, Bernhard Bauer wrote: >> >> >> >>> >> > Do we need this? What level of strictness does managed mean >> >> >> >>> >> > here? >> >> >> >>> >> >> >> >> >>> > >> >> >> >>> > Actually I think we could get rid of this. >> >> >> >>> >> >> >> >> >>> > >> >> >> >>> > If an provider returns true here, the user's settings are >> >> >> >>> > ignored >> >> >> >>> > (webapps are also ignored then) >> >> >> >>> > >> >> >> >>> > If all return false, the user can define exceptions. >> >> >> >>> > >> >> >> >>> > >> >> >> >>> > >> >> >> >>> As a long term goal I'd like to store default_content_settings >> >> >> >>> as a >> >> >> >>> [*] >> >> >> >>> "match_all_pattern". Then we would not need such a method >> >> >> >>> anymore. >> >> >> >> >> >> >> >>> In the mean while the host content settings map could handle the >> >> >> >>> case >> >> >> >>> you >> >> >> >>> described. >> >> >> >> >> >> >> >> >> >> >> >> Where would it know from that the user must not create >> >> >> >> exceptions? >> >> >> >> >> >> >> >> >> >> >> >>> > http://codereview.chromium.org/6253012/ >> >> >> >>> > >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >>> -- >> >> >> >>> Markus >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> http://codereview.chromium.org/6253012/ >> >> >> > >> >> >> > >> >> >> > >> >> >> > -- >> >> >> > Markus >> >> >> > >> >> >> > >> >> > >> >> > >> >> > >> >> > http://codereview.chromium.org/6253012/ >> >> > >> > >> > >> > >> > -- >> > Markus >> > >> > > > > > http://codereview.chromium.org/6253012/ >
I think we already had that discussion ;-) Defaults managed but exceptions allowed effectively means that you could change the defaults as well, you just have to add a bunch of [*.]com, [*.]org, etc. patterns[1]. [1] which you could even do with the upcoming content settings extension API :-D On Mon, Jan 31, 2011 at 17:13, <jochen@chromium.org> wrote: > Bernhard answered: "For case (a) the default setting is set in the > recommended > content > settings provider, for case (b) it's in the managed content settings > provider." > > In case (a), the user could then also override the default setting which is > not > intended. (Note that we currently do not have match-all patterns, so you > can't > write a "default" exception). > > http://codereview.chromium.org/6253012/ >
On 2011/01/31 16:32:51, Bernhard Bauer wrote: > I think we already had that discussion ;-) > > Defaults managed but exceptions allowed effectively means that you > could change the defaults as well, you just have to add a bunch of > [*.]com, [*.]org, etc. patterns[1]. It's not just that the pref provider could override the default, it will, as it always returns a default setting. Maybe that wasn't clear enough, but your solution to (a) using a recommended provider just won't work. > > [1] which you could even do with the upcoming content settings extension API :-D > > On Mon, Jan 31, 2011 at 17:13, <mailto:jochen@chromium.org> wrote: > > Bernhard answered: "For case (a) the default setting is set in the > > recommended > > content > > settings provider, for case (b) it's in the managed content settings > > provider." > > > > In case (a), the user could then also override the default setting which is > > not > > intended. (Note that we currently do not have match-all patterns, so you > > can't > > write a "default" exception). > > > > http://codereview.chromium.org/6253012/ > >
On Mon, Jan 31, 2011 at 17:37, <jochen@chromium.org> wrote: > On 2011/01/31 16:32:51, Bernhard Bauer wrote: >> >> I think we already had that discussion ;-) > >> Defaults managed but exceptions allowed effectively means that you >> could change the defaults as well, you just have to add a bunch of >> [*.]com, [*.]org, etc. patterns[1]. > > It's not just that the pref provider could override the default, it will, as > it > always returns a default setting. Well, it doesn't need to, necessarily. As long as the UI doesn't allow the user to change the default, there is no need to have a default setting (in the sense of all domains) defined at the user level (just like we have in the PrefService). > Maybe that wasn't clear enough, but your > solution to (a) using a recommended provider just won't work. > >> [1] which you could even do with the upcoming content settings extension >> API > > :-D > >> On Mon, Jan 31, 2011 at 17:13, <mailto:jochen@chromium.org> wrote: >> > Bernhard answered: "For case (a) the default setting is set in the >> > recommended >> > content >> > settings provider, for case (b) it's in the managed content settings >> > provider." >> > >> > In case (a), the user could then also override the default setting which >> > is >> > not >> > intended. (Note that we currently do not have match-all patterns, so you >> > can't >> > write a "default" exception). >> > >> > http://codereview.chromium.org/6253012/ >> > > > > > http://codereview.chromium.org/6253012/ >
On Mon, Jan 31, 2011 at 4:48 PM, <jochen@chromium.org> wrote: > On 2011/01/31 15:26:37, Bernhard Bauer wrote: > >> I don't think we are actually in disagreement here. All I'm saying is >> I think we can get rid of the "ContentSettingsTypeIsManaged" flag, as >> we want to support more fine-grained use cases (like >> managed--but-overridable defaults), and we can already get all the >> information from the providers without an additional flag. >> > > How do you decide whether (a) the policy hasn't set any exceptions, manages > the > default, and the user is allowed to add exceptions, or (b) the policy > hasn't set > any exceptions, manages the default, and the user is _not_ allowed to add > exceptions? > > How would the content_settings_provider decide this? managed_exceptions = { } managed_default = ALLOW user_can_set_addexceptions = {true|false} You probably need a policy/pref that set's this status right? Why not let the UI handle this? otherwise you need to have the following chain. UI -> HostContenSettingsMap -> ContentSettingsProvider -> Pref If we let the UI decide, then we only need this UI-> Pref > > On Mon, Jan 31, 2011 at 16:14, Markus Heintz <mailto: >> markusheintz@google.com> >> > wrote: > >> > There are two use cases: >> >> > 1) Admin wants to manage default and allow the user to set exceptions >> > 2) Admin wants to manage the default but not allow the user to set >> > exceptions >> > >> > On Mon, Jan 31, 2011 at 4:05 PM, Bernhard Bauer <mailto: >> bauerb@chromium.org> >> > wrote: > > >> >> >> On Mon, Jan 31, 2011 at 16:00, mailto: <jochen@chromium.org> wrote: >> >> > On 2011/01/31 14:52:08, Bernhard Bauer wrote: >> >> >> >> >> >> To be exact, the host content settings map would know that the >> content >> >> >> settings are coming from a higher-precedence source than the user >> >> >> preferences, so it should DCHECK that the UI doesn't try to create >> >> >> exceptions when they wouldn't be effective. >> >> > >> >> > >> >> > That's something different. Managed means the exceptions editor is >> >> > disabled. >> >> >> >> That is one way for the UI to make sure no exceptions are set. >> >> >> >> > Even if no policy controlled exceptions exist, the user can't create >> any >> >> >> >> It's not exceptions that would be controlled by policy in this case, >> >> it would be defaults. >> >> >> >> If exceptions are controlled by policy (meaning if there are are >> >> exceptions that are provided by policy), the user would be not allowed >> >> to create exceptions matching the same or a more specific pattern, >> >> just as the user is not allowed to create any exceptions when the >> >> defaults are policy-controlled. >> >> >> >> >> On Mon, Jan 31, 2011 at 15:43, Markus Heintz >> >> >> <mailto:markusheintz@google.com> >> >> > >> >> > wrote: >> >> >> >> >> >> > The same source the provider would know. >> >> >> > >> >> >> > On Mon, Jan 31, 2011 at 3:42 PM, <mailto:jochen@chromium.org> >> wrote: >> >> >> >> >> >> >> >> On 2011/01/31 14:39:34, markusheintz wrote: >> >> >> >>> >> >> >> >>> On Mon, Jan 31, 2011 at 3:30 PM, <mailto:jochen@chromium.org> >> >> >> >>> wrote: >> >> >> >> >> >> >> >>> > >> >> >> >>> > >> >> >> >>> > >> >> >> >> >> >> >> >> >> >> >> >> >> >> > >> >> > >> >> > >> > > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > >> >> >> >> >> >> >>> >> >> >> >>> > File >> chrome/browser/content_settings/content_settings_provider.h >> >> >> >>> > (right): >> >> >> >>> > >> >> >> >>> > >> >> >> >>> > >> >> >> >> >> >> >> >> >> >> >> >> >> >> > >> >> > >> >> > >> > > > http://codereview.chromium.org/6253012/diff/54001/chrome/browser/content_sett... > >> >> >> >> >> >> >>> >> >> >> >>> > >> chrome/browser/content_settings/content_settings_provider.h:73: >> >> >> >>> > ContentSettingsType content_type) = 0; >> >> >> >>> > On 2011/01/31 14:21:19, markusheintz_ wrote: >> >> >> >>> > >> >> >> >>> >> On 2011/01/31 13:48:58, Bernhard Bauer wrote: >> >> >> >>> >> > Do we need this? What level of strictness does managed mean >> >> >> >>> >> > here? >> >> >> >>> >> >> >> >> >>> > >> >> >> >>> > Actually I think we could get rid of this. >> >> >> >>> >> >> >> >> >>> > >> >> >> >>> > If an provider returns true here, the user's settings are >> ignored >> >> >> >>> > (webapps are also ignored then) >> >> >> >>> > >> >> >> >>> > If all return false, the user can define exceptions. >> >> >> >>> > >> >> >> >>> > >> >> >> >>> > >> >> >> >>> As a long term goal I'd like to store default_content_settings >> as a >> >> >> >>> [*] >> >> >> >>> "match_all_pattern". Then we would not need such a method >> anymore. >> >> >> >> >> >> >> >>> In the mean while the host content settings map could handle the >> >> >> >>> case >> >> >> >>> you >> >> >> >>> described. >> >> >> >> >> >> >> >> >> >> >> >> Where would it know from that the user must not create >> exceptions? >> >> >> >> >> >> >> >> >> >> >> >>> > http://codereview.chromium.org/6253012/ >> >> >> >>> > >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >>> -- >> >> >> >>> Markus >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> http://codereview.chromium.org/6253012/ >> >> >> > >> >> >> > >> >> >> > >> >> >> > -- >> >> >> > Markus >> >> >> > >> >> >> > >> >> > >> >> > >> >> > >> >> > http://codereview.chromium.org/6253012/ >> >> > >> > >> > >> > >> > -- >> > Markus >> > >> > >> > > > > http://codereview.chromium.org/6253012/ > -- Markus
Somehow my answer got lost so here I sent it again:
How would the content_settings_provider decide this?
managed_exceptions = { }
managed_default = ALLOW
user_can_set_addexceptions = {true|false}
You probably need a policy/pref that set's this status right?
Why not let the UI handle this? otherwise you need to have the following
chain.
UI -> HostContenSettingsMap -> ContentSettingsProvider -> Pref
If we let the UI decide, then we only need this
UI-> Pref
On Mon, Jan 31, 2011 at 5:57 PM, Bernhard Bauer <bauerb@chromium.org> wrote:
> On Mon, Jan 31, 2011 at 17:37, <jochen@chromium.org> wrote:
> > On 2011/01/31 16:32:51, Bernhard Bauer wrote:
> >>
> >> I think we already had that discussion ;-)
> >
> >> Defaults managed but exceptions allowed effectively means that you
> >> could change the defaults as well, you just have to add a bunch of
> >> [*.]com, [*.]org, etc. patterns[1].
> >
> > It's not just that the pref provider could override the default, it will,
> as
> > it
> > always returns a default setting.
>
> Well, it doesn't need to, necessarily. As long as the UI doesn't allow
> the user to change the default, there is no need to have a default
> setting (in the sense of all domains) defined at the user level (just
> like we have in the PrefService).
>
> > Maybe that wasn't clear enough, but your
> > solution to (a) using a recommended provider just won't work.
> >
> >> [1] which you could even do with the upcoming content settings extension
> >> API
> >
> > :-D
> >
> >> On Mon, Jan 31, 2011 at 17:13, <mailto:jochen@chromium.org> wrote:
> >> > Bernhard answered: "For case (a) the default setting is set in the
> >> > recommended
> >> > content
> >> > settings provider, for case (b) it's in the managed content settings
> >> > provider."
> >> >
> >> > In case (a), the user could then also override the default setting
> which
> >> > is
> >> > not
> >> > intended. (Note that we currently do not have match-all patterns, so
> you
> >> > can't
> >> > write a "default" exception).
> >> >
> >> > http://codereview.chromium.org/6253012/
> >> >
> >
> >
> >
> > http://codereview.chromium.org/6253012/
> >
>
--
Markus
Can we just try it out without adding ContentSettingsTypeIsManaged and see how far we get? On Mon, Jan 31, 2011 at 18:15, Markus Heintz <markusheintz@google.com> wrote: > Somehow my answer got lost so here I sent it again: > How would the content_settings_provider decide this? > managed_exceptions = { } > managed_default = ALLOW > user_can_set_addexceptions = {true|false} > You probably need a policy/pref that set's this status right? > Why not let the UI handle this? otherwise you need to have the following > chain. > UI -> HostContenSettingsMap -> ContentSettingsProvider -> Pref > If we let the UI decide, then we only need this > UI-> Pref > On Mon, Jan 31, 2011 at 5:57 PM, Bernhard Bauer <bauerb@chromium.org> wrote: >> >> On Mon, Jan 31, 2011 at 17:37, <jochen@chromium.org> wrote: >> > On 2011/01/31 16:32:51, Bernhard Bauer wrote: >> >> >> >> I think we already had that discussion ;-) >> > >> >> Defaults managed but exceptions allowed effectively means that you >> >> could change the defaults as well, you just have to add a bunch of >> >> [*.]com, [*.]org, etc. patterns[1]. >> > >> > It's not just that the pref provider could override the default, it >> > will, as >> > it >> > always returns a default setting. >> >> Well, it doesn't need to, necessarily. As long as the UI doesn't allow >> the user to change the default, there is no need to have a default >> setting (in the sense of all domains) defined at the user level (just >> like we have in the PrefService). >> >> > Maybe that wasn't clear enough, but your >> > solution to (a) using a recommended provider just won't work. >> > >> >> [1] which you could even do with the upcoming content settings >> >> extension >> >> API >> > >> > :-D >> > >> >> On Mon, Jan 31, 2011 at 17:13, <mailto:jochen@chromium.org> wrote: >> >> > Bernhard answered: "For case (a) the default setting is set in the >> >> > recommended >> >> > content >> >> > settings provider, for case (b) it's in the managed content settings >> >> > provider." >> >> > >> >> > In case (a), the user could then also override the default setting >> >> > which >> >> > is >> >> > not >> >> > intended. (Note that we currently do not have match-all patterns, so >> >> > you >> >> > can't >> >> > write a "default" exception). >> >> > >> >> > http://codereview.chromium.org/6253012/ >> >> > >> > >> > >> > >> > http://codereview.chromium.org/6253012/ >> > > > > > -- > Markus > >
+1 because: - It's easier to add than to remove from an API - We actually only need this method for the ManagedProvider and for non of the others. That a good indicator that it should not be in the abstract Interface. On Mon, Jan 31, 2011 at 6:31 PM, Bernhard Bauer <bauerb@chromium.org> wrote: > Can we just try it out without adding ContentSettingsTypeIsManaged and > see how far we get? > > On Mon, Jan 31, 2011 at 18:15, Markus Heintz <markusheintz@google.com> > wrote: > > Somehow my answer got lost so here I sent it again: > > How would the content_settings_provider decide this? > > managed_exceptions = { } > > managed_default = ALLOW > > user_can_set_addexceptions = {true|false} > > You probably need a policy/pref that set's this status right? > > Why not let the UI handle this? otherwise you need to have the following > > chain. > > UI -> HostContenSettingsMap -> ContentSettingsProvider -> Pref > > If we let the UI decide, then we only need this > > UI-> Pref > > On Mon, Jan 31, 2011 at 5:57 PM, Bernhard Bauer <bauerb@chromium.org> > wrote: > >> > >> On Mon, Jan 31, 2011 at 17:37, <jochen@chromium.org> wrote: > >> > On 2011/01/31 16:32:51, Bernhard Bauer wrote: > >> >> > >> >> I think we already had that discussion ;-) > >> > > >> >> Defaults managed but exceptions allowed effectively means that you > >> >> could change the defaults as well, you just have to add a bunch of > >> >> [*.]com, [*.]org, etc. patterns[1]. > >> > > >> > It's not just that the pref provider could override the default, it > >> > will, as > >> > it > >> > always returns a default setting. > >> > >> Well, it doesn't need to, necessarily. As long as the UI doesn't allow > >> the user to change the default, there is no need to have a default > >> setting (in the sense of all domains) defined at the user level (just > >> like we have in the PrefService). > >> > >> > Maybe that wasn't clear enough, but your > >> > solution to (a) using a recommended provider just won't work. > >> > > >> >> [1] which you could even do with the upcoming content settings > >> >> extension > >> >> API > >> > > >> > :-D > >> > > >> >> On Mon, Jan 31, 2011 at 17:13, <mailto:jochen@chromium.org> wrote: > >> >> > Bernhard answered: "For case (a) the default setting is set in the > >> >> > recommended > >> >> > content > >> >> > settings provider, for case (b) it's in the managed content > settings > >> >> > provider." > >> >> > > >> >> > In case (a), the user could then also override the default setting > >> >> > which > >> >> > is > >> >> > not > >> >> > intended. (Note that we currently do not have match-all patterns, > so > >> >> > you > >> >> > can't > >> >> > write a "default" exception). > >> >> > > >> >> > http://codereview.chromium.org/6253012/ > >> >> > > >> > > >> > > >> > > >> > http://codereview.chromium.org/6253012/ > >> > > > > > > > > > -- > > Markus > > > > > -- Markus
LGTM http://codereview.chromium.org/6253012/diff/54009/chrome/browser/content_sett... File chrome/browser/content_settings/content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/54009/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:74: const GURL requesting_url, Nit: const GURL&
Added comments to the interface methods and a dummy implementation of the ProviderInterface to the MockContentSettingsProvider. http://codereview.chromium.org/6253012/diff/54009/chrome/browser/content_sett... File chrome/browser/content_settings/content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/54009/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:74: const GURL requesting_url, On 2011/02/01 09:38:43, Bernhard Bauer wrote: > Nit: const GURL& Done.
LGTM with nits http://codereview.chromium.org/6253012/diff/13006/chrome/browser/content_sett... File chrome/browser/content_settings/content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/13006/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:63: const ContentSettingsPattern requesting_url_pattern_; no _ at the end of member variables (since this is a struct with public members) http://codereview.chromium.org/6253012/diff/13006/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:81: const ContentSettingsType& content_type, ContentSettingsType doesn't need to be const nor a reference http://codereview.chromium.org/6253012/diff/13006/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:85: // |embedding_pattern|, |content_type| tupel. For ContentSettingsTypes that nit. tuple http://codereview.chromium.org/6253012/diff/13006/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:93: const ContentSettingsType& content_type, same here http://codereview.chromium.org/6253012/diff/13006/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:99: // |content_settings_rules| must be a non-NULL outparam. If this map was must be non-NULL. (no outparam) map -> provider http://codereview.chromium.org/6253012/diff/13006/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:101: // differing from the main map. For ContentSettingsTypes that require a main map -> corresponding regular provider http://codereview.chromium.org/6253012/diff/13006/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:107: const ContentSettingsType& content_type, same here
http://codereview.chromium.org/6253012/diff/13006/chrome/browser/content_sett... File chrome/browser/content_settings/content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/13006/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:63: const ContentSettingsPattern requesting_url_pattern_; On 2011/02/01 11:22:35, jochen wrote: > no _ at the end of member variables (since this is a struct with public members) Done. http://codereview.chromium.org/6253012/diff/13006/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:81: const ContentSettingsType& content_type, On 2011/02/01 11:22:35, jochen wrote: > ContentSettingsType doesn't need to be const nor a reference Done. http://codereview.chromium.org/6253012/diff/13006/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:85: // |embedding_pattern|, |content_type| tupel. For ContentSettingsTypes that On 2011/02/01 11:22:35, jochen wrote: > nit. tuple Done. http://codereview.chromium.org/6253012/diff/13006/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:93: const ContentSettingsType& content_type, On 2011/02/01 11:22:35, jochen wrote: > same here Done. http://codereview.chromium.org/6253012/diff/13006/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:99: // |content_settings_rules| must be a non-NULL outparam. If this map was On 2011/02/01 11:22:35, jochen wrote: > must be non-NULL. (no outparam) > > map -> provider map -> user pref provider http://codereview.chromium.org/6253012/diff/13006/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:101: // differing from the main map. For ContentSettingsTypes that require a On 2011/02/01 11:22:35, jochen wrote: > main map -> corresponding regular provider Done. http://codereview.chromium.org/6253012/diff/13006/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:107: const ContentSettingsType& content_type, On 2011/02/01 11:22:35, jochen wrote: > same here Done.
http://codereview.chromium.org/6253012/diff/54010/chrome/browser/content_sett... File chrome/browser/content_settings/content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/54010/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:59: : requesting_url_pattern_(requesting_url_pattern), also remove the underscores here http://codereview.chromium.org/6253012/diff/54010/chrome/browser/content_sett... File chrome/browser/content_settings/mock_content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/54010/chrome/browser/content_sett... chrome/browser/content_settings/mock_content_settings_provider.h:38: const ContentSettingsType& content_type, also remove the const ref here http://codereview.chromium.org/6253012/diff/54010/chrome/browser/content_sett... chrome/browser/content_settings/mock_content_settings_provider.h:46: const ContentSettingsType& content_type, and here http://codereview.chromium.org/6253012/diff/54010/chrome/browser/content_sett... chrome/browser/content_settings/mock_content_settings_provider.h:51: const ContentSettingsType& content_type, and here
http://codereview.chromium.org/6253012/diff/54010/chrome/browser/content_sett... File chrome/browser/content_settings/content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/54010/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:59: : requesting_url_pattern_(requesting_url_pattern), On 2011/02/01 11:38:39, jochen wrote: > also remove the underscores here Done. http://codereview.chromium.org/6253012/diff/54010/chrome/browser/content_sett... File chrome/browser/content_settings/mock_content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/54010/chrome/browser/content_sett... chrome/browser/content_settings/mock_content_settings_provider.h:38: const ContentSettingsType& content_type, On 2011/02/01 11:38:39, jochen wrote: > also remove the const ref here Done. http://codereview.chromium.org/6253012/diff/54010/chrome/browser/content_sett... chrome/browser/content_settings/mock_content_settings_provider.h:46: const ContentSettingsType& content_type, On 2011/02/01 11:38:39, jochen wrote: > and here Done. http://codereview.chromium.org/6253012/diff/54010/chrome/browser/content_sett... chrome/browser/content_settings/mock_content_settings_provider.h:51: const ContentSettingsType& content_type, On 2011/02/01 11:38:39, jochen wrote: > and here Done.
lgtm, no need for a further cycle http://codereview.chromium.org/6253012/diff/36002/chrome/browser/content_sett... File chrome/browser/content_settings/content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/36002/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:59: : requesting_url_pattern(requesting_pattern), nit. you can use requesting_url_pattern both times (as parameter and member variable). C++ will do the right thing http://codereview.chromium.org/6253012/diff/36002/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:91: const ContentSettingsPattern& requesting_pattern, nit. requesting_url_pattern and embedding_url_pattern http://codereview.chromium.org/6253012/diff/36002/chrome/browser/content_sett... File chrome/browser/content_settings/mock_content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/36002/chrome/browser/content_sett... chrome/browser/content_settings/mock_content_settings_provider.h:44: const ContentSettingsPattern& requesting_pattern, nit. requesting_url_pattern and embedding_url_pattern
THANKS! Just an FYI reply here http://codereview.chromium.org/6253012/diff/36002/chrome/browser/content_sett... File chrome/browser/content_settings/content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/36002/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:59: : requesting_url_pattern(requesting_pattern), On 2011/02/01 12:33:11, jochen wrote: > nit. you can use requesting_url_pattern both times (as parameter and member > variable). C++ will do the right thing Ah cool! :-) THANKS. http://codereview.chromium.org/6253012/diff/36002/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_provider.h:91: const ContentSettingsPattern& requesting_pattern, On 2011/02/01 12:33:11, jochen wrote: > nit. requesting_url_pattern and embedding_url_pattern Done. http://codereview.chromium.org/6253012/diff/36002/chrome/browser/content_sett... File chrome/browser/content_settings/mock_content_settings_provider.h (right): http://codereview.chromium.org/6253012/diff/36002/chrome/browser/content_sett... chrome/browser/content_settings/mock_content_settings_provider.h:44: const ContentSettingsPattern& requesting_pattern, On 2011/02/01 12:33:11, jochen wrote: > nit. requesting_url_pattern and embedding_url_pattern Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
