Policy: Re-enabled plugin still disabled

webkit/plugins/npapi/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_PLUGINS_NPAPI_WEBPLUGININFO_H_
 #define WEBKIT_PLUGINS_NPAPI_WEBPLUGININFO_H_
 
 #include <string>
 #include <vector>
 
@@ skipping 13 matching lines @@
 
   // 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 {
+  enum Reason { USER = 1 << 1, MANAGED = 1 << 2 };

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

nit: we usually put each enum in a separate line

+
   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);
 
   // 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;

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

we don't need both an enabled boolean and a reason int.  these can be combined
into:

int disabled_reason;

enum DisabledReason {
  USER = 1 << 1,
  MANAGED = 1 << 2,
}

and then if disabled_reason is 0, we know it's enabled.

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

Indeed we do. The reason why this enum is called "Reason" and not
"DisabledReason" is that there is a reason for both being enabled and being
disabled. For now we only have disabled by policy but this soon will extend to
being enabled by policy - basically forcing a plugin being on regardless of user
wish. In that case you will have enabled == true and reason == MANAGED. reason
== 0 have no meaning and thus is not defined as a constant at all. Per default
this fiend initializes to USER which was the state before - all plugins not
explicitly managed are user controlled.

+};
+
+class WebPluginInfoUtils {

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

Maybe move this to a separate class?

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

we already have too many plugin related classes, I really prefer we don't add
more.  especially in this case, a class isn't really needed since there are no
member variables.  what you need is just namespace.  since this file is already
in webkit::npapi namespace, you can leave the functions in this header outside
of the class.

even better though, I'm still not sure these classes need to be grouped here? 
SupportsExtension and SupportsType are only used in PluginList, so they can
remain in that file.  IsEnabled is just checking one member variable, so it can
be done directly in the different callers.  Enable and Disable are only used in
PluginGroup, so they can remain there.

These functions used to be in different files, so I think we should either leave
them there if WebPluginInfo remains a struct, or they should only be moved to
one place (inside WebPluginInfo) if it's a class.  but moving them here, outside
WebPluginInfo, even though each function is only needed in one place, makes for
harder readability.

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

Separate file you mean?

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

Alright, I will move (possibly duplicate) any of these functions to where I need
them and remove the whole class.

[Bernhard Bauer, 2010/12/21 21:36:11]

Uh, yes, but like John said, a namespace would probably be better. I just don't
like having a class defined in a file which doesn't correspond to its name; it
makes finding the definition for a class kinda hard.

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

Moved to the respective user classes. Though I find this couter-OOP. We are
detaching data from manipulations on that data. Moving the responsibility to the
users of the data.

+ public:
+  // Enables the plugin if not already enabled and if policy allows it to.
+  // Returns true on success.
+  static bool Enable(WebPluginInfo* plugin, WebPluginInfo::Reason reason);
+
+  // Disables the plugin if not already disabled and if policy allows it to.
+  // Returns true on success.
+  static bool Disable(WebPluginInfo* plugin, WebPluginInfo::Reason reason);
+
+  static bool IsEnabled(const WebPluginInfo& plugin);
+  static bool IsManaged(const WebPluginInfo& plugin);
+
+  // Returns true if the plugin supports |mime_type|. |mime_type| should be all
+  // lower case.
+  static bool SupportsType(const WebPluginInfo& plugin,
+                    const std::string& mime_type,

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

nit: spacing is off

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

Done.

+                    bool allow_wildcard);
+
+  // 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
+  // extension is optionally returned back.
+  static bool SupportsExtension(const WebPluginInfo& plugin,
+                         const std::string& extension,

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

nit: spacing

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

Done.

+                         std::string* actual_mime_type);
 };
 
 }  // namespace npapi
 }  // namespace webkit
 
 #endif  // WEBKIT_PLUGINS_NPAPI_WEBPLUGININFO_H_