Policy: Re-enabled plugin still disabled

webkit/glue/plugins/webplugininfo.h

Index: webkit/glue/plugins/webplugininfo.h
diff --git a/webkit/glue/plugins/webplugininfo.h b/webkit/glue/plugins/webplugininfo.h
index 34eff3d5ae9d2ae6910d7104e95438547e3c4fa8..166b73bc47882b14f5b2649c56015ccece42e26f 100644
--- a/webkit/glue/plugins/webplugininfo.h
+++ b/webkit/glue/plugins/webplugininfo.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef WEBKIT_GLUE_WEBPLUGININFO_H_
-#define WEBKIT_GLUE_WEBPLUGININFO_H_
+#ifndef WEBKIT_GLUE_PLUGINS_WEBPLUGININFO_H_
+#define WEBKIT_GLUE_PLUGINS_WEBPLUGININFO_H_
 
 #include <string>
 #include <vector>
@@ -35,9 +35,34 @@ struct WebPluginInfo {
 
   // Special constructor only used during unit testing:
   WebPluginInfo(const string16& fake_name,
+                const FilePath& fake_path,
                 const string16& fake_version,
                 const string16& fake_desc);
 
+
+  // Enables the plugin if not already enabled and if policy allows it to.
+  // Returns true on success.
+  bool Enable(int reason);

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

please use an enum instead of a reason, that way you force callers to use valid
values.  the enum values should be compatible with a bitfield, i.e.

enum DisabledReason {
  NONE = 0,
  USER = 1 << 1,
  ADMINISTRATOR or GROUP_POLICY = 1 << 2,

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

Done.

+
+  // Disables the plugin if not already disabled and if policy allows it to.
+  // Returns true on success.
+  bool Disable(int reason);
+
+  bool IsEnabled() const { return enabled; }
+  static bool IsManaged(int reason) { return (reason & MANAGED) != 0; }
+  bool HasVersion() const { return version.length() != 0; }
+
+  // Returns true if the plugin supports "mime-type". |mime_type| should be all

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

Nit: Why the quotes?

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

Done.

+  // lower case.
+  bool SupportsType(const std::string& mime_type, bool allow_wildcard) const;
+
+  // Returns true if the given plugin 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

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

Mini-nit: "MIME type" is usually written in caps, we just use lower case in
variable names.

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

Done.

+  // extension is optionally returned back.
+  bool SupportsExtension(const std::string& extension,
+                         std::string* actual_mime_type) const;
+
   // The name of the plugin (i.e. Flash).
   string16 name;
 
@@ -55,6 +80,16 @@ struct WebPluginInfo {
 
   // Whether the plugin is enabled.
   bool enabled;
+
+  // Reason for the plugin being either enabled or disabled.
+  int reason;

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

If you add a new field to WebPluginInfo, you should also update
chrome/common/render_messages.{h,cc} to serialize the fields, otherwise when you
send a WebPluginInfo over IPC, hilarity will ensue.

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

Done.

+
+  // Constants definig bit fields in the reason member.

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

Nit: defining

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

Removed anyhow

+  static const int USER;
+  static const int MANAGED;

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

nit: the meaning of managed is a little cryptic.  perhaps user and group policy,
just to be very clear?

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

Managed is the name used in the policy world to identify a preference that is
controlled by a greater power than the user. I prefer to keep the name managed
so that it is consistent with the rest of the policy talk in chrome.

+
+  // Priority of the plugin (obsolete?)

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

At least it shouldn't be here. The priority is not an attribute of the plugin
itself, it's just its index in the list of plugins.

+  int priority;

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

i think the order of plugins in a plugin group should be enough to denote the
priority?

[Bernhard Bauer, 2010/12/17 19:26:40]

The order inside a group is probably not sufficient. The internal PDF viewer and
Acrobat Reader for example are in different groups, but the PDF viewer as a
bundled plug-in has higher priority.

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

We could only think of one synthetic case where order could be important - two
separate groups both having internal and external plugins and all of them
handling the same MIME types. This would lead to having the preference set to
first group's internal plugin then first group's external one and then the
internal of the other group which should be  rather internal 1, internal 2,
external 1... Moreover now I use a special value of this field - 0 to specify
that the Plugin is only a placeholder and not a real active plugin. I would like
to know if you think the situation I described above could arise and should be
treated properly or if I should remove this field and introduce a boolean flag
is_placeholder which will be enough for my needs.

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

I'm really against having this concept of order because I think it unnecessarily
complicates things.  We didn't need this previously because the order of the
plugin_list denoted the priority, since we always searched from the beginning. 
Since the plugin list is now generated from the plugin groups, it seems we
should just be able to keep plugin groups in a vector and we don't need an
ordering.

An ordering is currently needed because the internal and external flash are in
the same group.  It seems that if we don't group them, and instead treat them
separately like the internal PDF plugin and Adobe Reader are treated, then this
problem goes away.

[jam, 2010/12/20 21:01:42]

one more thing: if you add a is_placeholder, then the right place is not
WebPluginInfo, which is simply a description of a plugin.  That's a variable
used in the internal workings of PluginGroup, so that class should store this
information in another way, and not complicate WebPluginInfo just because of its
own implementation

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

The order the array of plugins that existed before in PluginList is preserved in
the fact that groups are created sequentially in the same order. As for the
internal external flash differentiation I couldn't see any code making use of
that. If I search for uses of the priority it doesn't appear anywhere so even
before my CL it has been superseded apparently.

I will move the flag to the PluginGroup class and if no other comments arise
remove the priority from here too. 

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

why is this needed in PluginGroup?  I really don't see the need.  If it's used
to put our internal flash before the external one, then PluginGroup should be
doing that with a one-off check when it creates the group

 };
 
-#endif  // WEBKIT_GLUE_WEBPLUGININFO_H_
+#endif  // WEBKIT_GLUE_PLUGINS_WEBPLUGININFO_H_