Policy: Re-enabled plugin still disabled

webkit/glue/plugins/webplugininfo.h

 // Copyright (c) 2006-2008 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_WEBPLUGININFO_H_
-#define WEBKIT_GLUE_WEBPLUGININFO_H_
+#ifndef WEBKIT_GLUE_PLUGINS_WEBPLUGININFO_H_
+#define WEBKIT_GLUE_PLUGINS_WEBPLUGININFO_H_
 
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
 
 // Describes a mime type entry for a plugin.
 struct WebPluginMimeType {
   WebPluginMimeType();
   ~WebPluginMimeType();
 
   // The name of the mime type (e.g., "application/x-shockwave-flash").
   std::string mime_type;
 
   // A list of all the file extensions for this mime type.
   std::vector<std::string> file_extensions;
 
   // Description of the mime type.
   string16 description;
 };
 
 // Describes an available NPAPI plugin.
 struct WebPluginInfo {

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

note that per the style guide, if this has methods, it should be a class instead
of a struct.  this will mean we'd have to add getters/setters and make the
members private, and add underscores at the end.

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

I don't think that we should do this (at least not in this CL) - it is going to
have huge impact on various files and make reviewing of this anyhow overgrown CL
harder. I'd suggest if this CL is accepted without much further refactoring to
file this as a bug and have it fixed as soon as possible. The few methods are
really only there to provide simple means of working with the fields and to
prevent code duplication. I could extract them as functions in a namespace and
provide them with a parameter being the object to modify but this is not going
to make the code more readable.

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

The issue is that the coding guidelines are pretty strict about structs only
carrying data, and classes having functions to modify it etc.  We shouldn't give
up the consistency of the guidelines unless we have a very compelling reason,
and I don't think it exists here.  If you want to make it a class in a future
change, then that change should make these functions be member functions.  But
until that is done, then it seems that the functions have to stay in
PluginList/PluginGroup and take a WebPluginInfo to operate on.

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

Would you agree on having these as global functions in a namespace or as a set
of static functions in a helper class say WebPluginInfoUtils ?

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

Is the only place that uses them in PluginGroup?  If so then please keep the
functions there.  We have to many classes for plugins as is, so avoiding adding
a new one is good.

   WebPluginInfo();
   WebPluginInfo(const WebPluginInfo& rhs);
   ~WebPluginInfo();
   WebPluginInfo& operator=(const WebPluginInfo& rhs);
 
   // 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;
 
   // The path to the plugin file (DLL/bundle/library).
   FilePath path;
 
   // The version number of the plugin file (may be OS-specific)
   string16 version;
 
   // A description of the plugin that we get from its version info.
   string16 desc;
 
   // A list of all the mime types that this plugin supports.
   std::vector<WebPluginMimeType> mime_types;
 
   // 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_