[Policy Experimental] Add "recommended" policies for URL exceptions.

[Policy Experimental] Add "recommended" policies for URL exceptions.

We want to allow various Content Settings items to be recommendations that
can be overridden by user. This is an proof-of-concept CL to experiment
with this new feature.

BUG=568031

[huangs, 2016-03-23 01:35:16]

Patch Set 1: Basic support, where admin can edit items (for {Cookies, Image,
Popup}) under "Googe -> Google Chrome - Default Settings -> Content Settings"
for Local Group Policy Editor (gpedit.msc on Windows). For chrome://settings UI,
recommended settings are highlighted, and appear below user-added exceptions.

Limitations:
- If both "mandatory" and "recommended" settings are set, the latter disappears.
 Note that "Allow..." and "Block..." are considered different.
- The experimental approach instantiates 2 PolicyProvider (for mandatory /
recommended). But this means all downstream observers will be called twice!
- Not considering interaction with default settings.
- The text "You are following the administrator's recommendation for this
setting" is reused, and might not be a good fit.

[huangs, 2016-03-23 01:40:45]

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

[commit-bot: I haz the power, 2016-03-23 01:40:59]

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

[huangs, 2016-03-23 01:42:29]

huangs@chromium.org changed reviewers:
+ pastarmovj@google.com, saswat@chromium.org

[huangs, 2016-03-23 01:42:30]

Extensive changes are needed, but any early feedback is appreciated. thanks!

[commit-bot: I haz the power, 2016-03-23 01:56:49]

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

[commit-bot: I haz the power, 2016-03-23 01:56:51]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[pastarmovj, 2016-03-23 13:24:48]

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

[pastarmovj, 2016-03-23 13:24:49]

Just a comment on your FIXME. :)

https://codereview.chromium.org/1825173003/diff/1/components/content_settings...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/1825173003/diff/1/components/content_settings...
components/content_settings/core/browser/host_content_settings_map.cc:150: //
FIXME(huangs): We have 2 instances of PolicyProvider, so policy changes
I guess you can avoid this issue by using the "want_recommended" flag in the
OnPolicyChanged callback as well to filter the propagation only in the
appropriate case.

[huangs, 2016-03-23 14:56:06]

Thanks! Replied.

[huangs, 2016-03-23 15:45:08]

Oops needed to send the actual reply...

https://codereview.chromium.org/1825173003/diff/1/components/content_settings...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/1825173003/diff/1/components/content_settings...
components/content_settings/core/browser/host_content_settings_map.cc:150: //
FIXME(huangs): We have 2 instances of PolicyProvider, so policy changes
The NotifyObserver() call passes dummy parameters, which
ObservableProvider::NotifyObservers() just forwards without adding anything new.
 So I presume this is just a "kick" rather than passing info?  If so then
perhaps we can just attach observer to |recommended_policy_provider| (and remove
line 155)?

In the long term I think the cleaner fix is to split PolicyProvider(): the
|pref_change_registrar_| and OnPreferenceChanged() can go to a separate class
that has 3 listeners: the 2 instances of PolicyProvider, and
HostContentSettingsMap.  I'd rather not do this right now though.  :)

[huangs, 2016-03-23 20:01:21]

It looks like PrefsService uses PrefRegistry prioritize the multi-value nature
of PrefsValueStore?  This is fine for independent scalar values, but for lists
and for interaction among variables (e.g., default vs. exceptions) it looks like
the abstraction's will leak through...

[huangs, 2016-03-23 22:15:53]

I think V2 is ready for more in-depth review, since it exposes some data that
were previously hidden.  There are some fine-tuning problems to be addressed
(below).

Page Set 2: Solved some problems from Patch Set 1.  Status (matching points in
comment #1):
- Not fixed: Exception list: "mandatory" still completely displaces
"recommended" items; not coexisting.
- Kludge-fixed: Double-call of observers: Just letting the "recommended"
observer propagate to downstream. Real fix will require refactoring, not
worrying now.
- Fixed: Supporting default settings: Now default settings appears to work
(testing with Images settings): Recommended image settings can be overridden by
user exceptions. This is also indicated in UI.
- Not fixed: Text.

In addition, for "recommended" default settings, the radio buttons cannot be
changed.  (As example) for images, currently the English texts are:

* Show all images (recommended)
* Do not show any images

And if policy exists then the city icon appears.
I think the UI should move the text "(recommended)" around.  Thoughts?

[huangs, 2016-03-23 22:16:43]

Description was changed from

==========
[Policy Experimetal] Add "suggested" policies for URL exceptions

We want to allow various Content Settings items to be suggestions that
can be overridden by user. This is an proof-of-concept CL to experiment
with this new feature.
==========

to

==========
[Policy Experimetal] Add "suggested" policies for URL exceptions

We want to allow various Content Settings items to be suggestions that
can be overridden by user. This is an proof-of-concept CL to experiment
with this new feature.

BUG=568031
==========

[huangs, 2016-03-24 02:36:43]

Description was changed from

==========
[Policy Experimetal] Add "suggested" policies for URL exceptions

We want to allow various Content Settings items to be suggestions that
can be overridden by user. This is an proof-of-concept CL to experiment
with this new feature.

BUG=568031
==========

to

==========
[Policy Experimetal] Add "recommended" policies for URL exceptions.

We want to allow various Content Settings items to be recommendations that
can be overridden by user. This is an proof-of-concept CL to experiment
with this new feature.

BUG=568031
==========

[huangs, 2016-03-24 23:42:48]

Feature won't be done this week; spent lots of time understanding flow.  :( 
Hopefully we'll get review going next week (back on Tuesday).

V3: Made code uglier, but we're prototyping.  Following the behavior of "On
startup"
- Fix: Setting default settings to "Not Configured" does not cause UI refresh.
- Decision: For defaults, recommended & user are on equal footing.
- Fixed pop-up text for default settings, reconciling recommended settings and
user settings, using existing text:
  - Matching => "You are following the administrator's recommendation for this
setting."
  - Different => "Your administrator recommends a specific value for this
setting."
- Works in Debug build, so crbug.com/106682 can probably be set to Fixed after
we commit.

TODO:
- Exception list issue: "mandatory" displaces "recommended".
- For "Matching" pop-up: Would be nice to have "Follow recommendation" link.  To
fix this in JS we need to add "resetHandler" function.
- Apply to all items in content settings, and test.
- Test on Incognito mode.

[Bernhard Bauer, 2016-03-30 08:49:39]

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

[Bernhard Bauer, 2016-03-30 08:49:39]

Drive-by comment! 😃

I think a good chunk of the complexity in this CL (in particular the need for
changes to PrefService) is coming from the fact that
content_settings::PolicyProvider goes through the PrefService, which abstracts
away the source of a pref value, so it has to undo that abstraction first before
abstracting again for most customers of the HostContentSettingsMap. An easier
way to do this might be to have content_settings::PolicyProvider not go through
prefs, but instead directly observe the PolicyService, which will directly
expose whether a policy is recommended or mandatory. (Really, the only reason
why PolicyProvider goes through prefs right now is because it predated the
PolicyService.)

[huangs, 2016-03-30 19:30:13]

Thanks for the comment!  Indeed I'm bothered by mandatory/recommended data
vanishing along the observer flow.  But it seems that eventually

const PrefService::Preference* prefs_->FindPreference(pref_name);

is still able to get recommended/mandatory data, via prefs_ -> pref_service_ ->
pref_value_store_.  We can simplify things after the prototype is done, but
currently downstream does not appear to block me.

The problem I'm encountering now (mandatory masking recommended) seem to
originate near the top of the flow: CreatePreferencesFromPolicies() returns
PrefValueMap, which maps each key (e.g.
"profile.managed_images_blocked_for_urls") to a single value?  I'm still
investigating, but any tip will be helpful.  Thanks!

[huangs, 2016-03-30 20:45:14]

Confirmed: For Windows, PolicyLoaderWin::Load() merges recommended policy into
mandatory policy:

    LoadChromePolicy(&gpo_dict, POLICY_LEVEL_MANDATORY, scope, chrome_policy);
    LoadChromePolicy(recommended_dict.get(), POLICY_LEVEL_RECOMMENDED, scope,
                     chrome_policy);

Options:
1. Simplest: Live with the limitation (mandatory policies shadows the matching
recommended), worry about this in v2!
2. Update policy loader to store multiple policies, add extra functions to allow
resolution for callers that want it.
3. Add special prefix for keys of recommended settings, resolve this downstream.
4. ?

Note that the code is platform dependent.

[huangs, 2016-03-31 23:29:51]

Went with (2) and have hacky prototype that's feature-complete!  PTAL and
comment on the approach; if it's a go then I'll start writing tests, breaking
into smaller CLs, write docs, etc.

Main idea: changing PolicyMap from {name => value} to {[name, level] => value}
so policy values with different levels (mandatory vs. recommended) can coexist. 
Added comparator to order higher priority elements first; these are the
"dominant" elements. To preserve old behavior, added next_dominant() to traverse
over the dominant values only.

Another idea is to keep PolicyMap same as before, but make its values store a
linked-list: Then we can leave a lot of existing code alone, and only traverse
the linked-list when we extract level-specific settings
(ConfigurationPolicyPrefStore::CreatePreferencesFromPolicies()?).

The work is incomplete!  Concerns & TODO:
- This can affect other parts of Chrome that I didn't test (chromeos).
- Does this affect extensions?  PolicyValueStore::SetCurrentPolicy() loop
assumes mandatory; is this relevant??
- The change in policy_ui_handler.cc did not require JS change (just worked?!)
-- need more investigation.
- Incognito mode.
- Persistence and migration?

[huangs, 2016-04-01 18:20:12]

Unless there are objections, I'm going to proceed with this general plan and
trying to get commit the code!  Steps (roughly corresponding to CLs):

1a. PrefService & PrefValueStore: Add new accessors.
1b. Pure refactor: Make PolicyMap::const_iterator a new class, with interfaces
next_dominant() and get_name(), update all callers to use these.
1c. The JS / CSS code, which does not affect original functionality.

2. Change PolicyMap to store "recessive" recommended policies.  Now
GetDifferingKeyNames() and FilterLevel() behave differently!
  - Need to test: no weirdness from existing recommended policies.

There's some murkiness beyond this: need to investigate incongnito, Android,
extensions and other users of PolicyMap.  Also need to figure out what's going
on with PolicyUIHandler.

3. Downstream code: HostContentSettingsMap & PolicyProvider changes, adding
DEFAULT_PROVIDER.  Unit tests.
4. Update policy_templates.json to, enable mandatory settings!

[huangs, 2016-04-01 20:03:59]

Discussed with grt@: Looks like this might not be the way to go re. what we want
to do currently, i.e., focus on Popups and third-party cookies.  Will produce
new (and much simpler) CL!

[huangs, 2016-04-06 15:20:45]

Description was changed from

==========
[Policy Experimetal] Add "recommended" policies for URL exceptions.

We want to allow various Content Settings items to be recommendations that
can be overridden by user. This is an proof-of-concept CL to experiment
with this new feature.

BUG=568031
==========

to

==========
[Policy Experimental] Add "recommended" policies for URL exceptions.

We want to allow various Content Settings items to be recommendations that
can be overridden by user. This is an proof-of-concept CL to experiment
with this new feature.

BUG=568031
==========