[Policy Experimental] Add policies to allow Cookies and Pop-ups exceptions.

[Policy Experimental] Add policies to allow Cookies and Pop-ups exceptions.

Previously users can always set preference exceptions in chrome://settings
to allow or block contents. If policy default settings are set, then
user exceptions become ineffective, but remain editable. This CL aims to:
- Make user exceptions take precedence over policy default settings, if
  allowed by policy.
- Fix related UI issues in chrome://settings

We added new policies (for cookies and pop-ups) to allow admin to
control whether the user preference exceptions:
- Are active iff policy default settings are set (current behavior).
- Can take precidence over policy default settings.

BUG=601057

[huangs, 2016-04-06 03:21:40]

huangs@chromium.org changed reviewers:
+ bauerb@chromium.org, dskaram@chromium.org, grt@chromium.org,
pastarmovj@chromium.org, saswat@chromium.org

[huangs, 2016-04-06 03:21:41]

Experimental CL without test yet.  May split and/or write design doc if
requested, PTAL.  Thanks!

https://codereview.chromium.org/1865803002/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/1865803002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:45:
const char* ChromiumVersion() {
This is not part of the CL; just fix for compile error.  May disappear on next
sync.

[huangs, 2016-04-06 03:47:28]

Also, histograms.xml and policy_test_cases.json are TODO.

[huangs, 2016-04-06 17:28:10]

Description was changed from

==========
[Policy Experimental] Add policies to enable/disable Cookies and Pop-ups
exceptions.

Previously users can always set preference exceptions in chrome://settings
to allow or block contents. If policy default settings are set, then
these preference exceptions become ineffective, but remain editable.
This CL aims to:
- If allowed by policy, make user preference exceptions take precidence
  over policy default settings (focusing on cookies and pop-ups).
- Fix related UI issues in chrome://settings

We added new policies (for cookies and pop-ups) to allow admin to
control whether the user preference exceptions:
- Are active iff policy default settings are set (current behavior).
- Can take precidence over policy default settings.
- Can never be set, in spite of default settings.
==========

to

==========
[Policy Experimental] Add policies to enable/disable Cookies and Pop-ups
exceptions.

Previously users can always set preference exceptions in chrome://settings
to allow or block contents. If policy default settings are set, then
these preference exceptions become ineffective, but remain editable.
This CL aims to:
- If allowed by policy, make user preference exceptions take precidence
  over policy default settings (focusing on cookies and pop-ups).
- Fix related UI issues in chrome://settings

We added new policies (for cookies and pop-ups) to allow admin to
control whether the user preference exceptions:
- Are active iff policy default settings are set (current behavior).
- Can take precidence over policy default settings.
- Can never be set, in spite of default settings.

BUG=601057
==========

[huangs, 2016-04-06 17:47:58]

huangs@chromium.org changed reviewers:
+ bartfab@chromium.org

[huangs, 2016-04-06 17:47:59]

+bartfab@ for OWNER review of \component\policy files, PTAL.

[huangs, 2016-04-06 19:07:54]

Sliced JS piece to https://codereview.chromium.org/1855393006/ .

[raymes, 2016-04-07 03:37:53]

raymes@chromium.org changed reviewers:
+ raymes@chromium.org

[raymes, 2016-04-07 03:37:54]

Some drive-by comments :) You may want to wait for Bernhard's thoughts in case
he disagrees with mine. Thanks!

https://codereview.chromium.org/1865803002/diff/80001/components/content_sett...
File components/content_settings/core/browser/content_settings_policy_provider.h
(right):

https://codereview.chromium.org/1865803002/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_policy_provider.h:51:
ContentSetting GetPrefExceptionsEnabledForType(
Naming suggestions below: AllowUserExceptionsForType

https://codereview.chromium.org/1865803002/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_policy_provider.h:57:
using PrefExceptionsEnabledMap =
UserExceptionsAllowedMap

https://codereview.chromium.org/1865803002/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_policy_provider.h:66:
void ReadManagedExceptionsSettings();
ReadUserExceptionsAllowed

https://codereview.chromium.org/1865803002/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_policy_provider.h:87:
PrefExceptionsEnabledMap pref_exceptions_enabled_map_;
user_exceptions_allowed_map_

https://codereview.chromium.org/1865803002/diff/80001/components/content_sett...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/1865803002/diff/80001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.cc:255:
ContentSetting HostContentSettingsMap::GetDefaultContentSetting(
Can we avoid adding another function here and just check the provider name?

https://codereview.chromium.org/1865803002/diff/80001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.cc:267: bool
HostContentSettingsMap::GetMaybeEnablePrefExceptionsForType(
I don't think this function fits well into this class. I'm not exactly sure
where we would put it, Bernhard might have thoughts. We may, for example, want
to expose the policy provider (or part of it) publicly.

https://codereview.chromium.org/1865803002/diff/80001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.cc:798: if
(type_found == POLICY_PROVIDER) {
I kind of like the idea of having a DEFAULT_POLICY_PROVIDER rather than adding
this logic here. It would mean less special-casing in HostContentSettingsMap. I
think you might have explored this earlier. What is the downside to that
approach?

[huangs, 2016-04-07 04:36:20]

huangs@chromium.org changed reviewers:
+ estade@chromium.org

[huangs, 2016-04-07 04:36:21]

+estade@ for JS file reviews.

Updated and added big unittest, PTAL.  Thanks!

https://codereview.chromium.org/1865803002/diff/80001/components/content_sett...
File components/content_settings/core/browser/content_settings_policy_provider.h
(right):

https://codereview.chromium.org/1865803002/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_policy_provider.h:51:
ContentSetting GetPrefExceptionsEnabledForType(
On 2016/04/07 03:37:54, raymes wrote:
> Naming suggestions below: AllowUserExceptionsForType

Done.

https://codereview.chromium.org/1865803002/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_policy_provider.h:57:
using PrefExceptionsEnabledMap =
On 2016/04/07 03:37:54, raymes wrote:
> UserExceptionsAllowedMap

Done.

https://codereview.chromium.org/1865803002/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_policy_provider.h:66:
void ReadManagedExceptionsSettings();
On 2016/04/07 03:37:54, raymes wrote:
> ReadUserExceptionsAllowed

Done.

https://codereview.chromium.org/1865803002/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_policy_provider.h:87:
PrefExceptionsEnabledMap pref_exceptions_enabled_map_;
On 2016/04/07 03:37:54, raymes wrote:
> user_exceptions_allowed_map_

Done.

https://codereview.chromium.org/1865803002/diff/80001/components/content_sett...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/1865803002/diff/80001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.cc:255:
ContentSetting HostContentSettingsMap::GetDefaultContentSetting(
This is the old interface.  Changing it will cause lots of changes elsewhere
(bunch of functions passing NULL as second parameter).  Since we're pressed for
time it's better to put a stub.  Adding FIXME for this.

https://codereview.chromium.org/1865803002/diff/80001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.cc:267: bool
HostContentSettingsMap::GetMaybeEnablePrefExceptionsForType(
This is a matter of practicality: content_setting_bubble_model.cc also needs
this, so it should not live down stream.  Perhaps refactor later, unless there's
somewhere concrete to put it?

https://codereview.chromium.org/1865803002/diff/80001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.cc:798: if
(type_found == POLICY_PROVIDER) {
Yes, I think this would be a nicer solution.  We should also specify a "priority
list" to iterate over, and select the right one depending on the new setting.

Downside is that we're pressed for time (haven't gotten answer on whether we
want to push this to M51, which I'm assuming yes just in case), hence the
monstrosity here.

[huangs, 2016-04-07 04:41:00]

The CQ bit was checked by huangs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-07 04:41:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1865803002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1865803002/100001

[commit-bot: I haz the power, 2016-04-07 04:45:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-07 04:45:52]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Bernhard Bauer, 2016-04-07 09:11:09]

On 2016/04/06 03:21:41, huangs wrote:
> Experimental CL without test yet.  May split and/or write design doc if
> requested, PTAL.  Thanks!

I know I'm cc'd on the discussion thread, but I have to admit I lost track there
a while ago. A design doc would be very helpful here to put all of this in
order.

In general, this seems like it's punching a bunch of holes into existing
abstractions. Why wouldn't it work to add a second policy content settings
provider that takes lower precedence than user prefs? Then we could have one of
the two policy providers return the policy value -- if setting user exceptions
is allowed, it would be the one with lower precedence than user prefs, if
exceptions are not allowed, it would be the one with higher precedence.

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
File components/content_settings/core/browser/content_settings_policy_provider.h
(right):

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_policy_provider.h:51:
ContentSetting AllowUserExceptionsForType(
What does the return value here mean?

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.h
(right):

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.h:99: bool
GetMaybeEnablePrefExceptionsForType(ContentSettingsType content_type)
Why GetMaybe...? If this returns a boolean, the "maybe" isn't necessary.
GetEnablePrefExceptionsForType (or maybe GetPrefExceptionsForTypeEnabled to make
it sound a bit more natural).

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
File components/content_settings/core/test/content_settings_test_utils.cc
(right):

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
components/content_settings/core/test/content_settings_test_utils.cc:61: return
settings_map->pref_provider_->SetWebsiteSetting(
Use an accessor. Also, why does this directly access the pref provider instead
of going through the HCSM method?

[msramek, 2016-04-07 09:27:27]

> I know I'm cc'd on the discussion thread, but I have to admit I lost track
there
> a while ago. A design doc would be very helpful here to put all of this in
> order.
> 
> In general, this seems like it's punching a bunch of holes into existing
> abstractions. Why wouldn't it work to add a second policy content settings
> provider that takes lower precedence than user prefs? Then we could have one
of
> the two policy providers return the policy value -- if setting user exceptions
> is allowed, it would be the one with lower precedence than user prefs, if
> exceptions are not allowed, it would be the one with higher precedence.
> 

To support Bernhard's point, the solution with two providers is what we do for
PrefStore, see the enum PrefStoreType and the MANAGED_STORE and
RECOMMENDED_STORE entries:
https://code.google.com/p/chromium/codesearch#chromium/src/components/prefs/p...

Content settings are really just a special case of prefs, so in an ideal world,
HostContentSettingsMap::ProviderType should be identical to PrefStoreType (plus
minus the difference between defaults and exceptions).

[huangs, 2016-04-07 13:52:50]

As mentioned in the thread, I think this is the best eventual solution. 
However, this is a feature requested by Chrome Enterprise that's under time
pressure (pulled 3 12-hour days this week).  And I still need to hear feedback
on whether we really want this in by M51.  The hole-punching approach arose
because (1) it seemed faster to get working, (2) I want to reduce risk from
changing the way things already work.

If you insist that refactoring the existing framework is important enough to
delay the feature until M52, then I'm fine with that -- but please chime in the
thread to argue this case.  Alternatively we make this a Beta-blocker, but I'll
also need support for that.

I'll write the design doc, but please not let this block review.  Thanks!

[bartfab (slow), 2016-04-07 14:39:29]

Policy looks reasonable overall, but this will need some end-to-end tests.

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
components/policy/resources/policy_templates.json:3147: 'name':
'PrefExceptionsUsageCookies',
Here and for the other policy: This name is very cryptic. How about something
like "AllowCookiesSettingExceptions"?

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
components/policy/resources/policy_templates.json:3148: 'type': 'int-enum',
Here and for the other policy: Do we expect to ever have more than these two
options? If not, a Boolean will do.

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
components/policy/resources/policy_templates.json:3155: 'name':
'AllowCookiesPrefExceptions',
Here and for the other policy: Nit: Content policies use the term "Setting," not
"Pref."

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
components/policy/resources/policy_templates.json:3179: If this policy is left
not set, user's ability to set exceptions depends on the default cookies
setting.''',
Here and for the other policy: What are you referring to here? I cannot find
anything in the documentation of the default cookies setting that would tell me
when exceptions are/are not allowed.

https://codereview.chromium.org/1865803002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1865803002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:5962: +    The policy setting to
explicitly enable or disable pref exceptions usage TO
Nit: s/TO/to/

https://codereview.chromium.org/1865803002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:5971: +    The policy setting to
explicitly enable or disable pref exceptions usage TO
Nit: s/TO/to/

[Bernhard Bauer, 2016-04-07 15:30:13]

On 2016/04/07 13:52:50, huangs wrote:
> As mentioned in the thread, I think this is the best eventual solution. 
> However, this is a feature requested by Chrome Enterprise that's under time
> pressure (pulled 3 12-hour days this week).  And I still need to hear feedback
> on whether we really want this in by M51.  The hole-punching approach arose
> because (1) it seemed faster to get working, (2) I want to reduce risk from
> changing the way things already work.

I'm sorry, but this is not how we develop software in Chromium. Time pressure
has never been an excuse to compromise on software quality -- this is why we
have a six-week release cycle. Generally speaking, it is a bad idea to show up
with a big CL that makes architectural changes in many layers and ask reviewers
to wave it through because there is a deadline.

I really don't want to create additional work for you, but I think an approach
that adds an additional provider would actually be simpler and require less
invasive changes, which would mean a smaller, more easily understandable CL,
which would in turn get landed faster. As it is, I have no idea whether your CL
is breaking anything in content settings, because it's way too complicated. A
design doc would also help with that, as I mentioned (I'm not asking for it to
put things in your way, but really to let me and others understand this better!)

> If you insist that refactoring the existing framework is important enough to
> delay the feature until M52, then I'm fine with that -- but please chime in
the
> thread to argue this case.  Alternatively we make this a Beta-blocker, but
I'll
> also need support for that.

Again, this is not about refactoring the existing architecture -- what you are
doing is the more invasive change. I do have doubts though that even with me not
blocking the overall design, this CL will be in a shape to land by the M51
branch point.

> I'll write the design doc, but please not let this block review.  Thanks!

https://codereview.chromium.org/1865803002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/1865803002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/content_settings_handler.cc:880: // The special
cases covered agive to get |provider_id| are not handled in
"agive" seems like a typo, and I can't make out what this sentence is trying to
say.

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
File
components/content_settings/core/browser/content_settings_policy_provider.cc
(right):

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_policy_provider.cc:69:
KPrefsForManagedExceptionSettingsSMap[] = {
kPrefsForManagedExceptionSettingsMap

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_policy_provider.cc:368:
if (!prefs_->HasPrefPath(entry.pref_name)) {
This is unnecessary -- We know that the preference can't be set by the user, and
if it's not set by policy, it will have the default value, which we handle
below.

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_policy_provider.cc:381:
content_settings::ValueToContentSetting(pref->GetValue());
Just use prefs_->GetInteger(entry.pref_name).

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_policy_provider.cc:482:
ReadManagedContentSettings(true);
Why do we read managed content settings again in this case?

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:798:
value_found.reset(value.release());
value_found = std::move(value);

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
components/policy/resources/policy_templates.json:3162: 'caption': '''Do not
allow users to specify cookies exceptions''',
So, this would apply even if no policy sets the default cookies value? Because
otherwise, this would be covered by the not-set case anyway.

[bartfab (slow), 2016-04-07 15:32:08]

PM has since chimed in and clarified that this can (and should :) wait for M52.

[huangs, 2016-04-07 15:40:59]

On 2016/04/07 15:30:13, Bernhard Bauer wrote:
> On 2016/04/07 13:52:50, huangs wrote:
> > As mentioned in the thread, I think this is the best eventual solution. 
> > However, this is a feature requested by Chrome Enterprise that's under time
> > pressure (pulled 3 12-hour days this week).  And I still need to hear
feedback
> > on whether we really want this in by M51.  The hole-punching approach arose
> > because (1) it seemed faster to get working, (2) I want to reduce risk from
> > changing the way things already work.
> 
> I'm sorry, but this is not how we develop software in Chromium. Time pressure
> has never been an excuse to compromise on software quality -- this is why we
> have a six-week release cycle. Generally speaking, it is a bad idea to show up
> with a big CL that makes architectural changes in many layers and ask
reviewers
> to wave it through because there is a deadline.

Yup I agree with this.  Again, it would helpful if you voice on the email
thread.

> I really don't want to create additional work for you, but I think an approach
> that adds an additional provider would actually be simpler and require less
> invasive changes, which would mean a smaller, more easily understandable CL,
> which would in turn get landed faster. As it is, I have no idea whether your
CL
> is breaking anything in content settings, because it's way too complicated. A
> design doc would also help with that, as I mentioned (I'm not asking for it to
> put things in your way, but really to let me and others understand this
better!)
> 
> > If you insist that refactoring the existing framework is important enough to
> > delay the feature until M52, then I'm fine with that -- but please chime in
> the
> > thread to argue this case.  Alternatively we make this a Beta-blocker, but
> I'll
> > also need support for that.
> 
> Again, this is not about refactoring the existing architecture -- what you are
> doing is the more invasive change. I do have doubts though that even with me
not
> blocking the overall design, this CL will be in a shape to land by the M51
> branch point.

By "what I'm doing"do you mean "what I want to do in the end", rather than "the
current implementation"?  If so then yes, this is invasive because it changes
the basic behavior of having fixed list of providers, sorted in priority.

[huangs, 2016-04-07 16:27:07]

Great to hear re. M52!  So here's the tentative plan now:

- Get this CL in order by processing all code reviews (esp. renames).
- Write the design doc.  There are essentially 2 parts of this now:
1. Refactor (reflow?) HostContentSettingsMap by:
  - Extracting POLICY_DEFAULT_PROVIDER (maybe use DefaultProvider?).
  - Specify extra layer of priority list.
  - Might need more refactoring to improve testing.
2. Update this CL to use the new framework.
- Write the code!

Might have to deal with new requests (e.g., re. wildcards).

Thanks!

[Evan Stade, 2016-04-07 16:42:29]

On 2016/04/07 16:27:07, huangs wrote:
> Great to hear re. M52!  So here's the tentative plan now:
> 
> - Get this CL in order by processing all code reviews (esp. renames).
> - Write the design doc.  There are essentially 2 parts of this now:
> 1. Refactor (reflow?) HostContentSettingsMap by:
>   - Extracting POLICY_DEFAULT_PROVIDER (maybe use DefaultProvider?).
>   - Specify extra layer of priority list.
>   - Might need more refactoring to improve testing.
> 2. Update this CL to use the new framework.
> - Write the code!
> 
> Might have to deal with new requests (e.g., re. wildcards).
> 
> Thanks!

so you'll ping me when it's ready for me to look at?

[huangs, 2016-04-07 17:33:36]

> so you'll ping me when it's ready for me to look at?

Yup!  Should I CC you on the design doc?

[Evan Stade, 2016-04-07 18:08:21]

On 2016/04/07 17:33:36, huangs wrote:
> > so you'll ping me when it's ready for me to look at?
> 
> Yup!  Should I CC you on the design doc?

if you feel like your webui changes are sufficiently dramatic to warrant design
approval from a webui owner, you probably want dbeam@ rather than myself to look
at them as he is actively working on webui and settings.

[huangs, 2016-04-07 21:41:14]

Sounds good, I'll chime him in on that next week.

[huangs, 2016-04-07 21:44:07]

Updated!  I'll have the design doc next week.  Reviews of this CL would still be
quite helpful, but I'll start new (and smaller) CLs so we can implement a less
hacky solution.

https://codereview.chromium.org/1865803002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/1865803002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/content_settings_handler.cc:880: // The special
cases covered agive to get |provider_id| are not handled in
Fixed ("agive" => "above").

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
File
components/content_settings/core/browser/content_settings_policy_provider.cc
(right):

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_policy_provider.cc:69:
KPrefsForManagedExceptionSettingsSMap[] = {
Done.

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_policy_provider.cc:368:
if (!prefs_->HasPrefPath(entry.pref_name)) {
Did this to allow incremental changes to |user_exceptions_allowed_map_| -- but
to follow what's done for |value_map_|, I should just clear
|user_exceptions_allowed_map_| beforehand.  Done.

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_policy_provider.cc:381:
content_settings::ValueToContentSetting(pref->GetValue());
Also need content_settings::IntToContentSetting(). Done.

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_policy_provider.cc:482:
ReadManagedContentSettings(true);
Ah yes; we're not modifying them.  Removed.

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
File components/content_settings/core/browser/content_settings_policy_provider.h
(right):

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_policy_provider.h:51:
ContentSetting AllowUserExceptionsForType(
Updated comments and function name.

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:798:
value_found.reset(value.release());
On 2016/04/07 15:30:13, Bernhard Bauer wrote:
> value_found = std::move(value);

Done.

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.h
(right):

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.h:99: bool
GetMaybeEnablePrefExceptionsForType(ContentSettingsType content_type)
Changing to "AreUserExceptionsAllowedForType()", following
"IsDefaultSettingAllowedForType()" below.

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
File components/content_settings/core/test/content_settings_test_utils.cc
(right):

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
components/content_settings/core/test/content_settings_test_utils.cc:61: return
settings_map->pref_provider_->SetWebsiteSetting(
An accessor invites usage, but currently direct access is only needed for test,
TestUtils is a friend to HCSM.  I'll add a private accessor.

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
components/policy/resources/policy_templates.json:3147: 'name':
'PrefExceptionsUsageCookies',
"Pref" (or some synonym; "user"?) is essential because this switch controls the
role of PREF_PROVIDER exceptions.  "Allow" would be nice to use, but this can
get entangled with CONTENT_SETTING_ALLOW.  I put "Cookies" at an inconsistent
place, so it should be moved.  "Settings" is too generic, since exceptions list
is also settings.  I think "Usage" would be descriptive of what this is.

So perhaps "UserCookiesExceptionsUsage"?  I'm going to change throughout, but
still open to suggestions.

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
components/policy/resources/policy_templates.json:3148: 'type': 'int-enum',
This makes it easier to extend in future, e.g., allow exceptions, but not
wildcards.

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
components/policy/resources/policy_templates.json:3155: 'name':
'AllowCookiesPrefExceptions',
Please see comment above?

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
components/policy/resources/policy_templates.json:3162: 'caption': '''Do not
allow users to specify cookies exceptions''',
In this case user defaults (DEFAULT_PROVIDER) will be a catch-all; admin cannot
disable that.

Stepping back, BlockCookiesPrefExceptions might be a bizarre feature that nobody
wants (I added it for symmetry when copying existing patterns).  We can sort
this out in design, now that we have more time.  :)

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
components/policy/resources/policy_templates.json:3179: If this policy is left
not set, user's ability to set exceptions depends on the default cookies
setting.''',
Depends on DefaultCookiesSetting.  For exceptions, there are "policy exceptions"
and "user exceptions" (AKA "pref exceptions", owing to PREF_PROVIDER).  We're
want to choose to (1) keep old behavior, (2) elevate "user exceptions" over
"policy default", (3) disable "user exceptions".

https://codereview.chromium.org/1865803002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1865803002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:5962: +    The policy setting to
explicitly enable or disable pref exceptions usage TO
On 2016/04/07 14:39:29, bartfab (slow) wrote:
> Nit: s/TO/to/

Done.

https://codereview.chromium.org/1865803002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:5971: +    The policy setting to
explicitly enable or disable pref exceptions usage TO
On 2016/04/07 14:39:29, bartfab (slow) wrote:
> Nit: s/TO/to/

Done.

[bartfab (slow), 2016-04-08 10:17:30]

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
components/policy/resources/policy_templates.json:3147: 'name':
'PrefExceptionsUsageCookies',
On 2016/04/07 21:44:07, huangs wrote:
> "Pref" (or some synonym; "user"?) is essential because this switch controls
the
> role of PREF_PROVIDER exceptions.  "Allow" would be nice to use, but this can
> get entangled with CONTENT_SETTING_ALLOW.  I put "Cookies" at an inconsistent
> place, so it should be moved.  "Settings" is too generic, since exceptions
list
> is also settings.  I think "Usage" would be descriptive of what this is.
> 
> So perhaps "UserCookiesExceptionsUsage"?  I'm going to change throughout, but
> still open to suggestions.

You can use "Permit" or "Honor" -> "HonorCookiesSettingExceptions"

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
components/policy/resources/policy_templates.json:3179: If this policy is left
not set, user's ability to set exceptions depends on the default cookies
setting.''',
On 2016/04/07 21:44:07, huangs wrote:
> Depends on DefaultCookiesSetting.  For exceptions, there are "policy
exceptions"
> and "user exceptions" (AKA "pref exceptions", owing to PREF_PROVIDER).  We're
> want to choose to (1) keep old behavior, (2) elevate "user exceptions" over
> "policy default", (3) disable "user exceptions".

But those policy exceptions come from a different policy, not from the "default
cookies setting."

[huangs, 2016-04-08 13:39:13]

Replying to comments first.

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
components/policy/resources/policy_templates.json:3147: 'name':
'PrefExceptionsUsageCookies',
I like "Permit"; will do that in the next update.  "Not Honor" sounds too
negative (and Worf-esque :).

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
components/policy/resources/policy_templates.json:3179: If this policy is left
not set, user's ability to set exceptions depends on the default cookies
setting.''',
Yes, this is a "meta"-setting that makes Policy settings aware of User settings
(and the layered nature of the system) hence the extra complexity.  Also, this
is a knob that's Policy-only and not controllable by User.  The consequence is
(unless we do the hole-poking approach) need to abstract the current
ProviderType-ordered approach.  Are there other concerns that might arise from
this?

[Bernhard Bauer, 2016-04-08 14:44:35]

https://codereview.chromium.org/1865803002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/1865803002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/content_settings_handler.cc:880: // The special
cases covered agive to get |provider_id| are not handled in
On 2016/04/07 21:44:06, huangs wrote:
> Fixed ("agive" => "above").

Oh, that makes sense (and it's not that far apart on the keyboard -- I should
have figured that one out myself 😉)!

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
File components/content_settings/core/test/content_settings_test_utils.cc
(right):

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
components/content_settings/core/test/content_settings_test_utils.cc:61: return
settings_map->pref_provider_->SetWebsiteSetting(
On 2016/04/07 21:44:07, huangs wrote:
> An accessor invites usage, but currently direct access is only needed for
test,
> TestUtils is a friend to HCSM.  I'll add a private accessor.

I still think you could go through HCSM, no?

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
components/policy/resources/policy_templates.json:3162: 'caption': '''Do not
allow users to specify cookies exceptions''',
On 2016/04/07 21:44:07, huangs wrote:
> In this case user defaults (DEFAULT_PROVIDER) will be a catch-all; admin
cannot
> disable that.
> 
> Stepping back, BlockCookiesPrefExceptions might be a bizarre feature that
nobody
> wants (I added it for symmetry when copying existing patterns).  We can sort
> this out in design, now that we have more time.  :)

Yes... I think this isn't very useful, because if the admin wants to apply the
default setting with no way for the user to override it, they could just
explicitly set the default setting and don't set this policy.

I also think removing this option would allow more simplifications in the rest
of the CL -- for example, we wouldn't need the tri-state return value in
GetUserExceptionsUsageSettingForType() anymore.

https://codereview.chromium.org/1865803002/diff/120001/components/content_set...
File
components/content_settings/core/browser/content_settings_policy_provider.cc
(right):

https://codereview.chromium.org/1865803002/diff/120001/components/content_set...
components/content_settings/core/browser/content_settings_policy_provider.cc:371:
continue;
My previous comment still applies here.

https://codereview.chromium.org/1865803002/diff/120001/components/content_set...
components/content_settings/core/browser/content_settings_policy_provider.cc:380:
user_exceptions_usage_settings_map_.erase(entry.content_type);
This shouldn't be necessary if we clear the map in advance, no?

https://codereview.chromium.org/1865803002/diff/120001/components/content_set...
File components/content_settings/core/browser/content_settings_policy_provider.h
(right):

https://codereview.chromium.org/1865803002/diff/120001/components/content_set...
components/content_settings/core/browser/content_settings_policy_provider.h:52:
ContentSetting GetUserExceptionsUsageSettingForType(
This still doesn't explain what the individual values (ALLOW, BLOCK, or DEFAULT)
mean. In general, I would try to avoid using ContentSetting as a tri-state
boolean, among other things because it's not actually tri-state! A dedicated
enum would make the semantics more obvious.

[bartfab (slow), 2016-04-11 10:27:31]

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
components/policy/resources/policy_templates.json:3179: If this policy is left
not set, user's ability to set exceptions depends on the default cookies
setting.''',
On 2016/04/08 13:39:12, huangs wrote:
> Yes, this is a "meta"-setting that makes Policy settings aware of User
settings
> (and the layered nature of the system) hence the extra complexity.  Also, this
> is a knob that's Policy-only and not controllable by User.  The consequence is
> (unless we do the hole-poking approach) need to abstract the current
> ProviderType-ordered approach.  Are there other concerns that might arise from
> this?

My concern is that I do not understand "user's ability to set exceptions depends
on policy default cookies setting." Assume this policy is unset. The
DefaultCookiesSetting policy could take on four different values (unset,
AllowCookies, BlockCookies, SessionOnly). Which of those permit exceptions?
Which do not? How would I know? There does not seem to be any documentation
describing this case.

https://codereview.chromium.org/1865803002/diff/120001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1865803002/diff/120001/components/policy/reso...
components/policy/resources/policy_templates.json:3147: 'name':
'UserCookiesExceptionsUsage',
This is still a weird name. "Usage" sounds extremely generic and vague. Maybe
"Handling" instead, even though it, too, is very generic.

[huangs, 2016-04-11 19:01:03]

Updated.

https://codereview.chromium.org/1865803002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/1865803002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/content_settings_handler.cc:880: // The special
cases covered agive to get |provider_id| are not handled in
On 2016/04/08 14:44:35, Bernhard Bauer wrote:
> On 2016/04/07 21:44:06, huangs wrote:
> > Fixed ("agive" => "above").
> 
> Oh, that makes sense (and it's not that far apart on the keyboard -- I should
> have figured that one out myself 😉)!

Acknowledged.  :)

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
File components/content_settings/core/test/content_settings_test_utils.cc
(right):

https://codereview.chromium.org/1865803002/diff/100001/components/content_set...
components/content_settings/core/test/content_settings_test_utils.cc:61: return
settings_map->pref_provider_->SetWebsiteSetting(
Don't know what you mean; in the latest version I updated it to
settings_map->GetPrefProvider()->...
where |settings_map| is HCSM?

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
components/policy/resources/policy_templates.json:3162: 'caption': '''Do not
allow users to specify cookies exceptions''',
Sounds good, I'll remove the option.

We still have to hear back whether wildcards is a concern.  If so then we would
add "Allow non-wildcard only option".  If not then we can change type from
"int-enum" to "boolean" (assuming we won't need to handle "different types of
permission" in the future and would need to migrate).

https://codereview.chromium.org/1865803002/diff/100001/components/policy/reso...
components/policy/resources/policy_templates.json:3179: If this policy is left
not set, user's ability to set exceptions depends on the default cookies
setting.''',
unset => Permit user exceptions by default.
AllowCookies => Block user exceptions by default. New switch allows user
exceptions.
BlockCookies => Block user exceptions by default. New switch allows user
exceptions.
SessionOnly => Block user exceptions by default. New switch allows user
exceptions.

Will write the design doc.  Part of this project also involves updating online
documentation, but let's worry about that after the feature is in.

https://codereview.chromium.org/1865803002/diff/120001/components/content_set...
File
components/content_settings/core/browser/content_settings_policy_provider.cc
(right):

https://codereview.chromium.org/1865803002/diff/120001/components/content_set...
components/content_settings/core/browser/content_settings_policy_provider.cc:371:
continue;
Oh I see; I missed "if it's not set by policy, it will have the default value". 
Removed.

https://codereview.chromium.org/1865803002/diff/120001/components/content_set...
components/content_settings/core/browser/content_settings_policy_provider.cc:380:
user_exceptions_usage_settings_map_.erase(entry.content_type);
Removed.  The code is in flux: If we don't need to worry about wildcard then we
can make |user_exceptions_usage_settings_map_| an std::set instead (|..._set_|);
and if we do then we'd see more cases.

https://codereview.chromium.org/1865803002/diff/120001/components/content_set...
File components/content_settings/core/browser/content_settings_policy_provider.h
(right):

https://codereview.chromium.org/1865803002/diff/120001/components/content_set...
components/content_settings/core/browser/content_settings_policy_provider.h:52:
ContentSetting GetUserExceptionsUsageSettingForType(
Added new ContentSettingsUserExceptionsUsage.  Now it only has 2 states:
{USER_EXCEPTIONS_DEFAULT, USER_EXCEPTIONS_ALLOW}.  Still don't know if there
will be another one (USER_EXCEPTIONS_ALLOW_NO_WILDCARD).

https://codereview.chromium.org/1865803002/diff/120001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1865803002/diff/120001/components/policy/reso...
components/policy/resources/policy_templates.json:3147: 'name':
'UserCookiesExceptionsUsage',
How about "UserCookiesExceptionsOverride" ?

[huangs, 2016-04-11 21:38:04]

Split off some content to https://codereview.chromium.org/1873343002/ .

[huangs, 2016-04-11 21:45:05]

Description was changed from

==========
[Policy Experimental] Add policies to enable/disable Cookies and Pop-ups
exceptions.

Previously users can always set preference exceptions in chrome://settings
to allow or block contents. If policy default settings are set, then
these preference exceptions become ineffective, but remain editable.
This CL aims to:
- If allowed by policy, make user preference exceptions take precidence
  over policy default settings (focusing on cookies and pop-ups).
- Fix related UI issues in chrome://settings

We added new policies (for cookies and pop-ups) to allow admin to
control whether the user preference exceptions:
- Are active iff policy default settings are set (current behavior).
- Can take precidence over policy default settings.
- Can never be set, in spite of default settings.

BUG=601057
==========

to

==========
[Policy Experimental] Add policies to allow Cookies and Pop-ups exceptions.

Previously users can always set preference exceptions in chrome://settings
to allow or block contents. If policy default settings are set, then
user exceptions become ineffective, but remain editable. This CL aims to:
- Make user exceptions take precedence over policy default settings, if
  allowed by policy.
- Fix related UI issues in chrome://settings

We added new policies (for cookies and pop-ups) to allow admin to
control whether the user preference exceptions:
- Are active iff policy default settings are set (current behavior).
- Can take precidence over policy default settings.

BUG=601057
==========

[bartfab (slow), 2016-04-12 12:55:16]

https://codereview.chromium.org/1865803002/diff/120001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1865803002/diff/120001/components/policy/reso...
components/policy/resources/policy_templates.json:3147: 'name':
'UserCookiesExceptionsUsage',
On 2016/04/11 19:01:03, huangs wrote:
> How about "UserCookiesExceptionsOverride" ?

I am looking for something that is unambiguous. "UserCookiesExceptionsOverride"
can be read in two ways, either as "user cookies exceptions do override
something" or "here is a user cookies exceptions override." Policy names should
be self-explanatory whenever possible.

I think we can rest this discussion for now though. Let us wait for your design
doc and update CL first.

[huangs, 2016-04-25 19:30:59]

huangs@chromium.org changed reviewers:
+ georgesak@chromium.org

[huangs, 2016-04-25 19:38:58]

The UI piece is spun into http://crrev.com/1855393006/, which prevents users
from seeing and adding exceptions if policy defaults are active.  Note that the
"Manage Exceptions..." button is not disabled (a 1-line change, if we settle on
doing that).

General status: We're waiting on client/PM to see if specs can be relaxed, so
that we can avoid doing major changes.  Immediate smaller changes are:
- Spin out content_setting_bubble_model.cc change.
- Update chrome://md-settings

[dskaram, 2016-04-25 19:43:29]

dskaram@chromium.org changed reviewers:
- dskaram@chromium.org, saswat@chromium.org