Enable NPAPI if policy has plugin policies.

Enable NPAPI if policy has plugin policies.

BUG=472648
TEST=manual - verify that NPAPI plugins appear in chrome://plugins when either a machine or user plugin policy is set.

Committed: https://crrev.com/26d09db19381c2236a2890c045761465c78bc958
Cr-Commit-Position: refs/heads/master@{#324754}

[Will Harris, 2015-04-09 01:07:48]

wfh@chromium.org changed reviewers:
+ atwilson@chromium.org

[Will Harris, 2015-04-09 01:08:16]

atwilson - am I wiring the profile policy parts into the right place here?

[Andrew T Wilson (Slow), 2015-04-09 08:44:41]

atwilson@chromium.org changed reviewers:
+ mnissler@chromium.org

[Andrew T Wilson (Slow), 2015-04-09 08:44:42]

+mnissler for a second set of eyes.

[Mattias Nissler (ping if slow), 2015-04-09 17:15:02]

https://codereview.chromium.org/1071713004/diff/20001/chrome/browser/policy/p...
File chrome/browser/policy/profile_policy_connector.cc (right):

https://codereview.chromium.org/1071713004/diff/20001/chrome/browser/policy/p...
chrome/browser/policy/profile_policy_connector.cc:143: void
ProfilePolicyConnector::EnableNPAPIIfRequired() {
The canonical way to do this within the policy/prefs framework: You'd have a
policy handler that looks at the policy keys you care about (i.e. the ones
below). You introduce a boolean pref named prefs::kEnableNpapi, which the policy
handler sets to true if any of the policies are set. Then, have some code
register a pref observer on the Profile's PrefService that takes care of making
the appropriate call into content to change the value if appropriate.

[Will Harris, 2015-04-09 20:28:52]

wfh@chromium.org changed reviewers:
+ bauerb@chromium.org, jam@chromium.org

[Will Harris, 2015-04-09 20:32:37]

Done - PTAL.

mnissler -> chrome/browser/policy
bauerb -> chrome/browser/plugins
jam -> content/

[jam, 2015-04-10 00:19:39]

lgtm

[Bernhard Bauer, 2015-04-10 08:07:03]

lgtm

[Mattias Nissler (ping if slow), 2015-04-10 08:47:15]

https://codereview.chromium.org/1071713004/diff/60001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_policy_handler.cc (right):

https://codereview.chromium.org/1071713004/diff/60001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_policy_handler.cc:33:
prefs->SetBoolean(prefs::kEnableNpapi, true);
You have multiple instances of this policy handler, and they all write to the
same pref key. This is  frowned upon, as the result will generally depend on the
order in which the handlers run. Admittedly, that's not the case here as setting
the pref to true multiple times will give the same result regardless of order.

For consistency, I'd still prefer a solution that just keeps the
SimplePolicyHandlers for mapping the existing plugin policy settings to prefs,
and then an additional handler that is solely responsible for mapping policy to
prefs::kEnableNpapi.

https://codereview.chromium.org/1071713004/diff/60001/chrome/browser/policy/c...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/1071713004/diff/60001/chrome/browser/policy/c...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:485: #if
defined(ENABLE_PLUGINS)
Nice!

https://codereview.chromium.org/1071713004/diff/60001/chrome/browser/policy/c...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:487: // NPAPI
to be re-enabled.
This makes it sound like these policy keys are just used for re-enabling NPAPI,
which is a bit misleading.

[Will Harris, 2015-04-10 18:51:23]

mnissler PTAL

https://codereview.chromium.org/1071713004/diff/60001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_policy_handler.cc (right):

https://codereview.chromium.org/1071713004/diff/60001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_policy_handler.cc:33:
prefs->SetBoolean(prefs::kEnableNpapi, true);
On 2015/04/10 08:47:15, Mattias Nissler wrote:
> You have multiple instances of this policy handler, and they all write to the
> same pref key. This is  frowned upon, as the result will generally depend on
the
> order in which the handlers run. Admittedly, that's not the case here as
setting
> the pref to true multiple times will give the same result regardless of order.
> 
> For consistency, I'd still prefer a solution that just keeps the
> SimplePolicyHandlers for mapping the existing plugin policy settings to prefs,
> and then an additional handler that is solely responsible for mapping policy
to
> prefs::kEnableNpapi.

Done.

https://codereview.chromium.org/1071713004/diff/60001/chrome/browser/policy/c...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/1071713004/diff/60001/chrome/browser/policy/c...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:485: #if
defined(ENABLE_PLUGINS)
On 2015/04/10 08:47:15, Mattias Nissler wrote:
> Nice!

Acknowledged.

https://codereview.chromium.org/1071713004/diff/60001/chrome/browser/policy/c...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:487: // NPAPI
to be re-enabled.
On 2015/04/10 08:47:15, Mattias Nissler wrote:
> This makes it sound like these policy keys are just used for re-enabling
NPAPI,
> which is a bit misleading.

Acknowledged.

[Mattias Nissler (ping if slow), 2015-04-10 19:22:45]

LGTM w/ nits.

https://codereview.chromium.org/1071713004/diff/80001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_policy_handler.h (right):

https://codereview.chromium.org/1071713004/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_policy_handler.h:18: class PluginPolicyHandler :
public ConfigurationPolicyHandler {
It probably makes sense to rename PluginPolicyHandler to
NPApiEnabledPolicyHandler (or similar) to make the name reflect the behavior it
implements.

https://codereview.chromium.org/1071713004/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_policy_handler.h:31:
DISALLOW_COPY_AND_ASSIGN(PluginPolicyHandler);
nit: #include "base/macros.h"

https://codereview.chromium.org/1071713004/diff/80001/content/public/browser/...
File content/public/browser/plugin_service.h (right):

https://codereview.chromium.org/1071713004/diff/80001/content/public/browser/...
content/public/browser/plugin_service.h:146: // This is equivalent to specifying
kEnableNpapi.
nit: I assume you mean switches::kEnableNpapi here?

[Will Harris, 2015-04-10 19:52:36]

Can't reply inline as rietveld appears to be having issues, but all comments
have been addressed.

[Will Harris, 2015-04-10 22:44:07]

The CQ bit was checked by wfh@chromium.org

[Will Harris, 2015-04-10 22:44:08]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
jam@chromium.org, mnissler@chromium.org
Link to the patchset: https://codereview.chromium.org/1071713004/#ps120001
(title: "fix GN")

[commit-bot: I haz the power, 2015-04-10 22:44:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1071713004/120001

[commit-bot: I haz the power, 2015-04-11 02:24:13]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-04-11 02:25:10]

Patchset 7 (id:??) landed as
https://crrev.com/26d09db19381c2236a2890c045761465c78bc958
Cr-Commit-Position: refs/heads/master@{#324754}

[Andrew T Wilson (Slow), 2015-04-13 13:55:38]

On 2015/04/11 02:25:10, I haz the power (commit-bot) wrote:
> Patchset 7 (id:??) landed as
> https://crrev.com/26d09db19381c2236a2890c045761465c78bc958
> Cr-Commit-Position: refs/heads/master@{#324754}

Retroactive LGTM :)