Policy: Re-enabled plugin still disabled

webkit/plugins/npapi/plugin_group.h

Index: webkit/plugins/npapi/plugin_group.h
diff --git a/webkit/plugins/npapi/plugin_group.h b/webkit/plugins/npapi/plugin_group.h
index 308fee79a8185e542bd1121ffa7fee34f5c23747..65b5a0314a33758a406953183ef48831fe2c857e 100644
--- a/webkit/plugins/npapi/plugin_group.h
+++ b/webkit/plugins/npapi/plugin_group.h
@@ -26,7 +26,9 @@ namespace npapi {
 
 class PluginList;
 struct WebPluginInfo;
-
+namespace plugin_test_internal {
+class PluginListWithoutFileIO;
+}
 // Hard-coded version ranges for plugin groups.
 struct VersionRangeDefinition {
   // Matcher for lowest version matched by this range (inclusive). May be empty
@@ -93,22 +95,24 @@ class PluginGroup {
   // the lookup key.
   static bool IsPluginNameDisabledByPolicy(const string16& plugin_name);
 
-  // Tests to see if a plugin is on the blacklist using its path as
-  // the lookup key.
-  static bool IsPluginPathDisabledByPolicy(const FilePath& plugin_path);
-
   // Returns true if the given plugin matches this group.
   bool Match(const WebPluginInfo& plugin) const;
 
-  // Adds the given plugin to this group. Provide the position of the
-  // plugin as given by PluginList so we can display its priority.
-  void AddPlugin(const WebPluginInfo& plugin, int position);
+  // Adds the given plugin to this group.
+  // Retuns true if plugin has been added and false if it was already contained
+  // in the group before.
+  bool AddPlugin(const WebPluginInfo& plugin, int priority);
 
-  bool IsEmpty() const;
+  // The two following functions enable/disable a plugin given its filename. The
+  // function returns true if the plugin could be enabled/disabled. Plugins
+  // might not get enabled/disabled if they are controlled by policy or are
+  // already in the wanted state.
+  bool EnablePlugin(const FilePath& filename);
+  bool DisablePlugin(const FilePath& filename);
 
   // Enables/disables this group. This enables/disables all plugins in the
   // group.
-  void Enable(bool enable);
+  bool EnableGroup(bool enable);
 
   // Returns whether the plugin group is enabled or not.
   bool Enabled() const { return enabled_; }
@@ -121,6 +125,12 @@ class PluginGroup {
   // is empty.
   string16 GetGroupName() const;
 
+  const std::vector<WebPluginInfo>& GetPlugins() const;
+
+  std::vector<WebPluginInfo>& GetPlugins() { return web_plugin_infos_; }

[jam, 2010/12/21 19:57:42]

we shouldn't be returning a non-const reference to the internal data structure.

[pastarmovj, 2010/12/21 20:31:19]

I probably don't need this version anymore. I will remove it if this is the case
or replace it with a proper manipulator function for the aim I have.

[pastarmovj, 2010/12/23 13:00:19]

Done.

+
+  bool ContainsPlugin(const FilePath& path) const;
+
   // Returns the description of the highest-priority plug-in in the group.
   const string16& description() const { return description_; }
 
@@ -137,6 +147,16 @@ class PluginGroup {
   // security problems.
   bool IsVulnerable() const;
 
+  // Check if the group has no plugins or only non-existing plugins
+  // (with priority 0).

[Bernhard Bauer, 2010/12/21 19:07:39]

Comment is outdated.

[pastarmovj, 2010/12/23 13:00:19]

Done.

+  bool IsEmpty() const;
+
+  // Set plugin placeholder status
+  void SetPluginIsPlaceholder(const WebPluginInfo& plugin, bool status);
+
+  // Set plugin placeholder status

[Bernhard Bauer, 2010/12/21 19:07:39]

Comment is wrong. Also, it doesn't really tell you anything you wouldn't know
from the method name (if it was correct, that is).

[pastarmovj, 2010/12/23 13:00:19]

Done.

+  bool IsPluginPlaceholder(const WebPluginInfo& plugin);
+
   // Disables all plugins in this group that are older than the
   // minimum version.
   void DisableOutdatedPlugins();
@@ -149,6 +169,7 @@ class PluginGroup {
   typedef std::map<std::string, PluginGroup*> PluginMap;
 
   friend class PluginList;
+  friend class plugin_test_internal::PluginListWithoutFileIO;
   friend class PluginGroupTest;
   friend class ::TableModelArrayControllerTest;
   friend class ::PluginExceptionsTableModelTest;
@@ -169,6 +190,10 @@ class PluginGroup {
   // the created PluginGroup.
   static PluginGroup* FromWebPluginInfo(const WebPluginInfo& wpi);
 
+  // Creates an empty PluginGroup from a given name. The caller takes
+  // ownership of the created PluginGroup.
+  static PluginGroup* CreateEmptyGroup(const string16& name);
+
   // Returns |true| if |version| is contained in [low, high) of |range|.
   static bool IsVersionInRange(const Version& version,
                                const VersionRange& range);
@@ -193,6 +218,9 @@ class PluginGroup {
   // enabled one, or if all plugins are disabled, simply the first one.
   void UpdateActivePlugin(const WebPluginInfo& plugin);
 
+  // Refreshes the enabled flag based on the state of its plugins.
+  void RefreshEnabledState();
+
   static std::set<string16>* policy_disabled_plugin_patterns_;
 
   std::string identifier_;
@@ -204,7 +232,11 @@ class PluginGroup {
   std::vector<VersionRange> version_ranges_;
   scoped_ptr<Version> version_;
   std::vector<WebPluginInfo> web_plugin_infos_;
-  std::vector<int> web_plugin_positions_;
+  std::vector<int> web_plugin_priority_;

[Bernhard Bauer, 2010/12/21 19:07:39]

Nit: You should call it priorities. It's confusing otherwise.

Also, we probably could get fully rid of them, but I'd say we should do that in
a separate CL when we have proper test coverage in place so that we know we're
not breaking anything. That okay with you, John?

[jam, 2010/12/21 19:57:42]

i think we can do without this.

[pastarmovj, 2010/12/23 13:00:19]

I have removed it for now but I am not sure I have cleaned all the UI traces of
that one. Should I look into some localized files for the IDS_PLUGIN_PRIORITY or
is the generated_resources the only one that I had to clean?

+
+  // Set to true for every entry in |web_plugin_infos_| that is only a
+  // placeholder.
+  std::vector<bool> is_plugin_placeholder_;

[Bernhard Bauer, 2010/12/21 19:07:39]

I'm still not exactly sure what the invariants for this are. Is there ever going
to be more than one placeholder? And what's the difference between a group
without plugins and one with a placeholder?

[pastarmovj, 2010/12/21 20:31:19]

In a group one can have more than one placeholder, but only one per distinctive
plugin (different file). From the perspective of an user there is no difference
between an empty group and one with only placeholders - both appear empty and
are not returned from the PluginList::GetPluginGroups call. The concept is not
much different than the disabledgroups/plugins lists we had before but
implemented clean in terms only of existing obejcts and reducing the chance for
these getting out of sync (as it happened before - plugins/groups couldn't get
properly disabled/enabled separately or didn't update the groups status
correctly).

 };
 
 }  // namespace npapi