Implemented policy to disable plugin finder.

Implemented policy to disable plugin finder.

BUG=49597
TEST=Set the policy to true and the default plugin should not offer to install missing plugins.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=76043

[pastarmovj, 2011-02-15 13:15:39]

I decided to wait for your CL to land and keep this one as simple as possible.
This way I can devote my spare cycles towards some more urgent policy work. 
Please have a look at it whenever you got time so that I can have it ready for
landing once it is safe to uncomment the policy fetching line.
Of course I'll ask for LGTM only after it is finished.

[Bernhard Bauer, 2011-02-15 13:25:12]

http://codereview.chromium.org/6475011/diff/2002/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6475011/diff/2002/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:744:
local_state_->RegisterBooleanPref(prefs::kDisablePluginFinder, false);
I'm not sure when this trend of registering preferences in BrowserProcessImpl
started, but there's a method Browser::RegisterPrefs where it actually should
happen. The other cases here need to do something with the preference value.

http://codereview.chromium.org/6475011/diff/2002/chrome/default_plugin/plugin...
File chrome/default_plugin/plugin_impl_win.h (right):

http://codereview.chromium.org/6475011/diff/2002/chrome/default_plugin/plugin...
chrome/default_plugin/plugin_impl_win.h:274: // Set to true of plugin finder has
been disabled.
Nit: "if"

http://codereview.chromium.org/6475011/diff/2002/webkit/glue/webkit_strings.grd
File webkit/glue/webkit_strings.grd (right):

http://codereview.chromium.org/6475011/diff/2002/webkit/glue/webkit_strings.g...
webkit/glue/webkit_strings.grd:419: Plugin finder has been disabled.
I'm not sure a user will really know what "plugin finder" is; it's never
mentioned elsewhere. Maybe "Installing plug-ins has been disabled."?

[pastarmovj, 2011-02-23 16:36:37]

Finished the implementation based on the awesome thread wandering MemberPrefs :)

[Bernhard Bauer, 2011-02-24 10:37:07]

http://codereview.chromium.org/6475011/diff/10001/chrome/browser/plugin_proce...
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/6475011/diff/10001/chrome/browser/plugin_proce...
chrome/browser/plugin_process_host.cc:465: if
(!g_browser_process->disable_plugin_finder_pref()) {
Why is this no longer in the newest version of the CL?

http://codereview.chromium.org/6475011/diff/16001/chrome/browser/browser_proc...
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/6475011/diff/16001/chrome/browser/browser_proc...
chrome/browser/browser_process.h:174: virtual bool disable_plugin_finder_pref()
const = 0;
Nit: accessors that start with a verb sound like they change something. Maybe
"plugin_finder_disabled"?

http://codereview.chromium.org/6475011/diff/16001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6475011/diff/16001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:793: // Initilialize the preference for
the plugin finder policy.
Nit: Initialize

http://codereview.chromium.org/6475011/diff/16001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/6475011/diff/16001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.h:261: // Monitors the disable plugin finder
policy state.
Nit: this comment is a bit hard to parse. Maybe "Monitors the state of the
'disable plugin finder' policy."?

http://codereview.chromium.org/6475011/diff/16001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/6475011/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:887: {
kPolicyDisablePluginFinder, Value::TYPE_BOOLEAN,
I'm not sure if we should call this policy "disable plugin finder", as plugin
finder is an internal term.

http://codereview.chromium.org/6475011/diff/16001/chrome/default_plugin/plugi...
File chrome/default_plugin/plugin_impl_win.cc (right):

http://codereview.chromium.org/6475011/diff/16001/chrome/default_plugin/plugi...
chrome/default_plugin/plugin_impl_win.cc:84: DLOG(ERROR) << "Failed to
initialize plugin install job";
You can just |DLOG(FATAL)| here, or even simpler just |NOTREACHED() << "..."|.

[pastarmovj, 2011-02-24 16:02:20]

Addressed comments and started some trybots.

http://codereview.chromium.org/6475011/diff/10001/chrome/browser/plugin_proce...
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/6475011/diff/10001/chrome/browser/plugin_proce...
chrome/browser/plugin_process_host.cc:465: if
(!g_browser_process->disable_plugin_finder_pref()) {
On 2011/02/24 10:37:07, Bernhard Bauer wrote:
> Why is this no longer in the newest version of the CL?

Sorry when rebasing to ToT this file got lost as it was moved in the trunk to
contents from chrome.

http://codereview.chromium.org/6475011/diff/16001/chrome/browser/browser_proc...
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/6475011/diff/16001/chrome/browser/browser_proc...
chrome/browser/browser_process.h:174: virtual bool disable_plugin_finder_pref()
const = 0;
On 2011/02/24 10:37:07, Bernhard Bauer wrote:
> Nit: accessors that start with a verb sound like they change something. Maybe
> "plugin_finder_disabled"?

Done.

http://codereview.chromium.org/6475011/diff/16001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6475011/diff/16001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:793: // Initilialize the preference for
the plugin finder policy.
On 2011/02/24 10:37:07, Bernhard Bauer wrote:
> Nit: Initialize

Done.

http://codereview.chromium.org/6475011/diff/16001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/6475011/diff/16001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.h:261: // Monitors the disable plugin finder
policy state.
On 2011/02/24 10:37:07, Bernhard Bauer wrote:
> Nit: this comment is a bit hard to parse. Maybe "Monitors the state of the
> 'disable plugin finder' policy."?

Done.

http://codereview.chromium.org/6475011/diff/16001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/6475011/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:887: {
kPolicyDisablePluginFinder, Value::TYPE_BOOLEAN,
On 2011/02/24 10:37:07, Bernhard Bauer wrote:
> I'm not sure if we should call this policy "disable plugin finder", as plugin
> finder is an internal term. 

As we agreed offline - I will keep the name of this policy DisablePluginFinder
and extend the description with accordingly.

http://codereview.chromium.org/6475011/diff/16001/chrome/default_plugin/plugi...
File chrome/default_plugin/plugin_impl_win.cc (right):

http://codereview.chromium.org/6475011/diff/16001/chrome/default_plugin/plugi...
chrome/default_plugin/plugin_impl_win.cc:84: DLOG(ERROR) << "Failed to
initialize plugin install job";
On 2011/02/24 10:37:07, Bernhard Bauer wrote:
> You can just |DLOG(FATAL)| here, or even simpler just |NOTREACHED() << "..."|.

Actually I only moved this piece of code but i will anyhow tune it as well as
all same calls in  this function too.

[Bernhard Bauer, 2011-02-24 16:25:18]

LGTM with a friendly request:

http://codereview.chromium.org/6475011/diff/16001/chrome/default_plugin/plugi...
File chrome/default_plugin/plugin_impl_win.cc (right):

http://codereview.chromium.org/6475011/diff/16001/chrome/default_plugin/plugi...
chrome/default_plugin/plugin_impl_win.cc:84: DLOG(ERROR) << "Failed to
initialize plugin install job";
On 2011/02/24 16:02:20, pastarmovj wrote:
> On 2011/02/24 10:37:07, Bernhard Bauer wrote:
> > You can just |DLOG(FATAL)| here, or even simpler just |NOTREACHED() <<
"..."|.
> 
> Actually I only moved this piece of code but i will anyhow tune it as well as
> all same calls in  this function too.

Please do. Even if you're not the original author of the code, it's encouraged
to do minor cleanups as you come across them.

http://codereview.chromium.org/6475011/diff/21001/content/browser/plugin_proc...
File content/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/6475011/diff/21001/content/browser/plugin_proc...
content/browser/plugin_process_host.cc:458: void
PluginProcessHost::OnGetPluginFinderUrl(std::string* plugin_finder_url) {
We should move this into a MessageFilter some time. Could you add a TODO(bauerb)
for that?

[pastarmovj, 2011-02-25 09:56:37]

Addressed the friendly advice :)
Will land if try bots are happy.

http://codereview.chromium.org/6475011/diff/16001/chrome/default_plugin/plugi...
File chrome/default_plugin/plugin_impl_win.cc (right):

http://codereview.chromium.org/6475011/diff/16001/chrome/default_plugin/plugi...
chrome/default_plugin/plugin_impl_win.cc:84: DLOG(ERROR) << "Failed to
initialize plugin install job";
On 2011/02/24 16:25:18, Bernhard Bauer wrote:
> On 2011/02/24 16:02:20, pastarmovj wrote:
> > On 2011/02/24 10:37:07, Bernhard Bauer wrote:
> > > You can just |DLOG(FATAL)| here, or even simpler just |NOTREACHED() <<
> "..."|.
> > 
> > Actually I only moved this piece of code but i will anyhow tune it as well
as
> > all same calls in  this function too.
> 
> Please do. Even if you're not the original author of the code, it's encouraged
> to do minor cleanups as you come across them.

Yep sure. :)

http://codereview.chromium.org/6475011/diff/21001/content/browser/plugin_proc...
File content/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/6475011/diff/21001/content/browser/plugin_proc...
content/browser/plugin_process_host.cc:458: void
PluginProcessHost::OnGetPluginFinderUrl(std::string* plugin_finder_url) {
On 2011/02/24 16:25:19, Bernhard Bauer wrote:
> We should move this into a MessageFilter some time. Could you add a
TODO(bauerb)
> for that?

Done.