Policy: Re-enabled plugin still disabled

webkit/glue/plugins/plugin_list.h

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef WEBKIT_GLUE_PLUGINS_PLUGIN_LIST_H_
 #define WEBKIT_GLUE_PLUGINS_PLUGIN_LIST_H_
 
+#include <map>
 #include <set>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
 #include "base/linked_ptr.h"
 #include "base/lock.h"
 #include "third_party/npapi/bindings/nphostapi.h"
 #include "webkit/glue/plugins/plugin_group.h"
 #include "webkit/glue/plugins/webplugininfo.h"
 
@@ skipping 163 matching lines @@
 
   // Load a specific plugin with full path.
   void LoadPlugin(const FilePath& filename,
                   std::vector<WebPluginInfo>* plugins);
 
   // Enable a specific plugin, specified by path. Returns |true| iff a plugin
   // currently in the plugin list was actually enabled as a result; regardless
   // of return value, if a plugin is found in the future with the given name, it
   // will be enabled. Note that plugins are enabled by default as far as
   // |PluginList| is concerned.
-  bool EnablePlugin(const FilePath& filename);
+  bool EnablePlugin(const FilePath& filename, const string16& name);

[Bernhard Bauer, 2010/12/17 18:50:59]

Can you explain what |name| is for? Are both required? What if they don't match?

[pastarmovj, 2010/12/20 19:57:37]

Done.

 
   // Disable a specific plugin, specified by path. Returns |true| iff a plugin
   // currently in the plugin list was actually disabled as a result; regardless
   // of return value, if a plugin is found in the future with the given name, it
-  // will be disabled.
-  bool DisablePlugin(const FilePath& filename);
+  // will be disabled. The boolean flag should be set to true if the plugin is
+  // disabled by a policy and not a user.

[Bernhard Bauer, 2010/12/17 18:50:59]

That comment doesn't seem right.

[jam, 2010/12/17 19:14:45]

nit: using boolean to specify whether it's user/policy makes it hard to read at
the caller's site, and also limiting in the future in case we want to disable
through other means (say because the plugin is out-of-date).  please use an enum
instead

[pastarmovj, 2010/12/20 19:57:37]

I fixed the comment to make clear what the return flag means. It doesn't mean
weather the plugin is policy enabled or disabled but only whether the change
requested could be performed.

[jam, 2010/12/20 20:56:59]

oops, i meant to add this comment on line 209.  so is this function only used to
disable plugins based on user action, and not on group policy?

[pastarmovj, 2010/12/20 21:47:22]

In any case these functions doesn't differentiate between policy or user call
externally. If there is a policy set it has precedence (the groups store
information about what policies exist) over the user action. Normally the UI
should prevent the user from changing the status of a plugin or group if it is
policy controlled. But in case the user tries to manipulate that it will still
be overridden inside these functions. 

A policy change can trigger calling any of these function or a user interaction
can do as well. 

+  bool DisablePlugin(const FilePath& filename, const string16& name);
 
   // Enable/disable a plugin group, specified by group_name.  Returns |true| iff
   // a plugin currently in the plugin list was actually enabled/disabled as a
   // result; regardless of return value, if a plugin is found in the future with
   // the given name, it will be enabled/disabled. Note that plugins are enabled
   // by default as far as |PluginList| is concerned.
   bool EnableGroup(bool enable, const string16& name);
 
   // Disable all plugins groups that are known to be outdated, according to
   // the information hardcoded in PluginGroup, to make sure that they can't
   // be loaded on a web page and instead show a UI to update to the latest
   // version.
   void DisableOutdatedPluginGroups();
 
-  ~PluginList();
+  virtual ~PluginList();
 
  private:
   FRIEND_TEST_ALL_PREFIXES(PluginGroupTest, PluginGroupDefinition);
 
   // Constructors are private for singletons
   PluginList();
 
   // Creates PluginGroups for the static group definitions, and adds them to
   // the PluginGroup cache of this PluginList.
   void AddHardcodedPluginGroups();
 
   // Adds the given WebPluginInfo to its corresponding group, creating it if
   // necessary, and returns the group.
   // Callers need to protect calls to this method by a lock themselves.
   PluginGroup* AddToPluginGroups(const WebPluginInfo& web_plugin_info);
 
+  // Adds the Plugin given by its path and name to its corresponding group,
+  // creating it if necessary, and returns the group.
+  // Callers need to protect calls to this method by a lock themselves.
+  PluginGroup* AddToPluginGroups(const FilePath& filename,
+                                 const string16& name);
+
   // Load all plugins from the default plugins directory
   void LoadPlugins(bool refresh);
 
   // Load all plugins from a specific directory.
   // |plugins| is updated with loaded plugin information.
   // |visited_plugins| is updated with paths to all plugins that were considered
   //   (including those we didn't load)
   void LoadPluginsFromDir(const FilePath& path,
                           std::vector<WebPluginInfo>* plugins,
                           std::set<FilePath>* visited_plugins);
 
   // Returns true if we should load the given plugin, or false otherwise.
   // plugins is the list of plugins we have crawled in the current plugin
   // loading run.
   bool ShouldLoadPlugin(const WebPluginInfo& info,
                         std::vector<WebPluginInfo>* plugins);
 
   // Return whether a plug-in group with the given name should be disabled,
   // either because it already is on the list of disabled groups, or because it
   // is blacklisted by a policy. In the latter case, add the plugin group to the
   // list of disabled groups as well.
   bool ShouldDisableGroup(const string16& group_name);
 
-  // Returns true if the given WebPluginInfo supports "mime-type".
-  // mime_type should be all lower case.
-  static bool SupportsType(const WebPluginInfo& info,
-                           const std::string &mime_type,
-                           bool allow_wildcard);
-
-  // Returns true if the given WebPluginInfo supports a given file extension.
-  // extension should be all lower case.
-  // If mime_type is not NULL, it will be set to the mime type if found.
-  // The mime type which corresponds to the extension is optionally returned
-  // back.
-  static bool SupportsExtension(const WebPluginInfo& info,
-                                const std::string &extension,
-                                std::string* actual_mime_type);
-
   //
   // Platform functions
   //
 
   // Do any initialization.
   void PlatformInit();
 
   // Get the ordered list of directories from which to load plugins
   void GetPluginDirectories(std::vector<FilePath>* plugin_dirs);
 
   //
   // Command-line switches
   //
 
 #if defined(OS_WIN)
   // true if we shouldn't load the new WMP plugin.
   bool dont_load_new_wmp_;
 
   // Loads plugins registered under HKCU\Software\MozillaPlugins and
   // HKLM\Software\MozillaPlugins.
-  void LoadPluginsFromRegistry(std::vector<WebPluginInfo>* plugins,
-                               std::set<FilePath>* visited_plugins);
+  void LoadPluginsFromRegistry(std::set<FilePath>* visited_plugins,

[jam, 2010/12/17 19:14:45]

why switch these parameters' order?

[pastarmovj, 2010/12/20 19:57:37]

Not by intention. Fixed.

+                               std::vector<WebPluginInfo>* plugins);
 #endif
 
   //
   // Internals
   //
 
   bool plugins_loaded_;
 
   // If true, we reload plugins even if they've been loaded already.
   bool plugins_need_refresh_;
 
-  // Contains information about the available plugins.
-  std::vector<WebPluginInfo> plugins_;
-
   // Extra plugin paths that we want to search when loading.
   std::vector<FilePath> extra_plugin_paths_;
 
   // Extra plugin directories that we want to search when loading.
   std::vector<FilePath> extra_plugin_dirs_;
 
   // Holds information about internal plugins.
   std::vector<PluginVersionInfo> internal_plugins_;
 
-  // Path names of plugins to disable (the default is to enable them all).
-  std::set<FilePath> disabled_plugins_;
-
-  // Group names to disable (the default is to enable them all).
-  std::set<string16> disabled_groups_;
-
   bool disable_outdated_plugins_;
 
   // Holds the currently available plugin groups.
   PluginGroup::PluginMap plugin_groups_;
 
   int next_priority_;
 
   // Need synchronization for the above members since this object can be
   // accessed on multiple threads.
-  Lock lock_;
+  mutable Lock lock_;

[jam, 2010/12/17 19:14:45]

why make this mutable?  if locking is required, seems the function don't need to
be const?  that would be a hint to the caller that some work can be required..

[pastarmovj, 2010/12/20 19:57:37]

I lock some getters to prevent writing happening while copying the output. The
getters don't change anything and could well function in a const situation. In
general I try to be as const friendly as possible but if you thing it's better
to do it otherwise I will change it.

 
   friend struct base::DefaultLazyInstanceTraits<PluginList>;
 
   DISALLOW_COPY_AND_ASSIGN(PluginList);
 };
 
 }  // namespace NPAPI
 
 #endif  // WEBKIT_GLUE_PLUGINS_PLUGIN_LIST_H_