Fix syncing of NPAPI plugins.

Fix syncing of NPAPI plugins.

This fix adds a check for |plugin| permission
while syncing NPAPI plugins.

BUG=252034
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207830

[jln (very slow on Chromium), 2013-06-20 02:28:51]

https://codereview.chromium.org/16816024/diff/1/chrome/common/extensions/sync...
File chrome/common/extensions/sync_helper.cc (right):

https://codereview.chromium.org/16816024/diff/1/chrome/common/extensions/sync...
chrome/common/extensions/sync_helper.cc:44: if
(PluginInfo::HasPlugins(extension) ||
Shouldn't we patch HasPlugins() instead?

It would change the semantics of HasPlugins a bit, but it would be harder to
make a mistake.

Alternatively create a new HasSomethingToDoWithPlugins() ? :)

[Jeffrey Yasskin, 2013-06-20 03:08:42]

Ah, I see. It's not an inconsistency between an empty plugins list and a
non-empty one; it's an assumption that the only way "plugin" winds up on the
permissions list is through the addition at
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext....
LGTM.

I suspect the eventual desired state is to only check HasAPIPermission, since
the manifest handlers won't be able to load a list of plugins without setting
that, but we may not be able to block setting the permission explicitly before
manifest v3.

https://codereview.chromium.org/16816024/diff/1/chrome/common/extensions/sync...
File chrome/common/extensions/sync_helper.cc (right):

https://codereview.chromium.org/16816024/diff/1/chrome/common/extensions/sync...
chrome/common/extensions/sync_helper.cc:44: if
(PluginInfo::HasPlugins(extension) ||
On 2013/06/20 02:28:52, Julien Tinnes wrote:
> Shouldn't we patch HasPlugins() instead?
> 
> It would change the semantics of HasPlugins a bit, but it would be harder to
> make a mistake.
> 
> Alternatively create a new HasSomethingToDoWithPlugins() ? :)

HasPlugins is used to control content settings
(https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/co...)
(???), so maybe it's not such a good idea to change it.

[meacer, 2013-06-20 18:37:41]

On 2013/06/20 03:08:42, Jeffrey Yasskin wrote:
> Ah, I see. It's not an inconsistency between an empty plugins list and a
> non-empty one; it's an assumption that the only way "plugin" winds up on the
> permissions list is through the addition at
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext....
> LGTM.
> 
> I suspect the eventual desired state is to only check HasAPIPermission, since
> the manifest handlers won't be able to load a list of plugins without setting
> that, but we may not be able to block setting the permission explicitly before
> manifest v3.
> 

Actually, does it make sense to just remove "plugin" permission?
Is that permission even useful without also having the "plugins" list?


>
https://codereview.chromium.org/16816024/diff/1/chrome/common/extensions/sync...
> File chrome/common/extensions/sync_helper.cc (right):
> 
>
https://codereview.chromium.org/16816024/diff/1/chrome/common/extensions/sync...
> chrome/common/extensions/sync_helper.cc:44: if
> (PluginInfo::HasPlugins(extension) ||
> On 2013/06/20 02:28:52, Julien Tinnes wrote:
> > Shouldn't we patch HasPlugins() instead?
> > 
> > It would change the semantics of HasPlugins a bit, but it would be harder to
> > make a mistake.
> > 
> > Alternatively create a new HasSomethingToDoWithPlugins() ? :)
> 
> HasPlugins is used to control content settings
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/co...)
> (???), so maybe it's not such a good idea to change it.

[meacer, 2013-06-20 18:52:04]

> Actually, does it make sense to just remove "plugin" permission?
> Is that permission even useful without also having the "plugins" list?

Never mind, Jeffrey's comment makes it clear it may not be possible right now.
I'll commit this as is.

[commit-bot: I haz the power, 2013-06-20 19:02:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/meacer@chromium.org/16816024/7001

[commit-bot: I haz the power, 2013-06-20 19:02:30]

Failed to apply patch for chrome/common/extensions/sync_type_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/common/extensions/sync_type_unittest.cc
  Hunk #2 FAILED at 25.
  Hunk #5 succeeded at 257 with fuzz 2.
  1 out of 5 hunks FAILED -- saving rejects to file
chrome/common/extensions/sync_type_unittest.cc.rej

Patch:       chrome/common/extensions/sync_type_unittest.cc
Index: chrome/common/extensions/sync_type_unittest.cc
diff --git a/chrome/common/extensions/sync_type_unittest.cc
b/chrome/common/extensions/sync_type_unittest.cc
index
c64d8b838a1a90a6f35bd1c2193e395db3606d68..a74ed865e33e35f8bab8b132458b192d04c8ba10
100644
--- a/chrome/common/extensions/sync_type_unittest.cc
+++ b/chrome/common/extensions/sync_type_unittest.cc
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "base/files/file_path.h"
+#include "chrome/common/extensions/api/plugins/plugins_handler.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_manifest_constants.h"
 #include "chrome/common/extensions/manifest.h"
@@ -24,14 +25,15 @@ class ExtensionSyncTypeTest : public testing::Test {
     THEME
   };
 
-  static scoped_refptr<Extension> MakeSyncTestExtension(
+  static scoped_refptr<Extension> MakeSyncTestExtensionWithPluginPermission(
       SyncTestExtensionType type,
       const GURL& update_url,
       const GURL& launch_url,
       Manifest::Location location,
       int num_plugins,
       const base::FilePath& extension_path,
-      int creation_flags) {
+      int creation_flags,
+      bool has_plugin_permission) {
     DictionaryValue source;
     source.SetString(keys::kName, "PossiblySyncableExtension");
     source.SetString(keys::kVersion, "0.0.0.0");
@@ -55,6 +57,11 @@ class ExtensionSyncTypeTest : public testing::Test {
       }
       source.Set(keys::kPlugins, plugins);
     }
+    if (has_plugin_permission) {
+      ListValue* plugins = new ListValue();
+      plugins->Set(0, new StringValue("plugin"));
+      source.Set(keys::kPermissions, plugins);
+    }
 
     std::string error;
     scoped_refptr<Extension> extension = Extension::Create(
@@ -64,6 +71,19 @@ class ExtensionSyncTypeTest : public testing::Test {
     return extension;
   }
 
+  static scoped_refptr<Extension> MakeSyncTestExtension(
+      SyncTestExtensionType type,
+      const GURL& update_url,
+      const GURL& launch_url,
+      Manifest::Location location,
+      int num_plugins,
+      const base::FilePath& extension_path,
+      int creation_flags) {
+    return MakeSyncTestExtensionWithPluginPermission(
+        type, update_url, launch_url, location, num_plugins, extension_path,
+        creation_flags, false);
+  }
+
   static const char kValidUpdateUrl1[];
   static const char kValidUpdateUrl2[];
 };
@@ -238,6 +258,16 @@ TEST_F(ExtensionSyncTypeTest, ExtensionWithTwoPlugins) {
   if (extension.get())
     EXPECT_FALSE(sync_helper::IsSyncableExtension(extension));
 }
+
+TEST_F(ExtensionSyncTypeTest, ExtensionWithPluginPermission) {
+  scoped_refptr<Extension> extension(
+      MakeSyncTestExtensionWithPluginPermission(EXTENSION, GURL(), GURL(),
+                                                Manifest::INTERNAL,
+                                                0, base::FilePath(),
+                                                Extension::NO_FLAGS, true));
+  if (extension.get())
+    EXPECT_FALSE(sync_helper::IsSyncableExtension(extension));
+}
 #endif // !defined(OS_CHROMEOS)
 
 }  // namespace extensions

[Jeffrey Yasskin, 2013-06-20 19:21:08]

On Thu, Jun 20, 2013 at 11:37 AM,  <meacer@chromium.org> wrote:
> Reviewers: Julien Tinnes, Jeffrey Yasskin,
>
> Message:
>
> On 2013/06/20 03:08:42, Jeffrey Yasskin wrote:
>>
>> Ah, I see. It's not an inconsistency between an empty plugins list and a
>> non-empty one; it's an assumption that the only way "plugin" winds up on
>> the
>> permissions list is through the addition at
>
>
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext....
>>
>> LGTM.
>
>
>> I suspect the eventual desired state is to only check HasAPIPermission,
>> since
>> the manifest handlers won't be able to load a list of plugins without
>> setting
>> that, but we may not be able to block setting the permission explicitly
>> before
>> manifest v3.
>
>
>
> Actually, does it make sense to just remove "plugin" permission?
> Is that permission even useful without also having the "plugins" list?

It would make sense to remove the ability to put "plugin" in the
permissions list, but I'm not sure we can do that without breaking any
extensions that have done it. I believe it doesn't make sense to
remove the permission itself, since that's how we trigger the dialog.

ext_plorer actually says we might be able to give an error for
"plugin" in the permissions list:

/google/data/ro/projects/ext_plorer/ext_plorer
>>> manifests = Manifests()
79565 manifests fetched (10/10 shards)
79395 manifests loaded (170 failures)
>>> with_plugins = manifests.Filter(lambda m: 'plugin' in GetPermissions(m))
2 manifests match filter
>>> for id, m in with_plugins.items():
...   print id, GetNumUsers(m)
...
lankeacickinbpamhdnipeiphdoomjol 3
gkogkdnapiipmhoodhkgplmgohgbeddi 260


But I'd want Yoyo or Matt to chime in before we decide to block that
permission string. I don't think we do it for any other string, which
might be intentional.

>
https://codereview.chromium.org/16816024/diff/1/chrome/common/extensions/sync...
>>
>> File chrome/common/extensions/sync_helper.cc (right):
>
>
>
>
https://codereview.chromium.org/16816024/diff/1/chrome/common/extensions/sync...
>>
>> chrome/common/extensions/sync_helper.cc:44: if
>> (PluginInfo::HasPlugins(extension) ||
>> On 2013/06/20 02:28:52, Julien Tinnes wrote:
>> > Shouldn't we patch HasPlugins() instead?
>> >
>> > It would change the semantics of HasPlugins a bit, but it would be
>> > harder to
>> > make a mistake.
>> >
>> > Alternatively create a new HasSomethingToDoWithPlugins() ? :)
>
>
>> HasPlugins is used to control content settings
>
>
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/co...)
>>
>> (???), so maybe it's not such a good idea to change it.
>
>
>
>
> Description:
> Fix syncing of NPAPI plugins.
>
> This fix adds a check for |plugin| permission
> while syncing NPAPI plugins.
>
> BUG=252034
>
> Please review this at https://codereview.chromium.org/16816024/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files:
>   M chrome/common/extensions/sync_helper.cc
>   M chrome/common/extensions/sync_type_unittest.cc
>
>
> Index: chrome/common/extensions/sync_helper.cc
> diff --git a/chrome/common/extensions/sync_helper.cc
> b/chrome/common/extensions/sync_helper.cc
> index
>
3cffcca357f1813b17dbe5756784133c71933d9b..e4c88f235e9e9ffc53fc6136942cf5b005bb8ae9
> 100644
> --- a/chrome/common/extensions/sync_helper.cc
> +++ b/chrome/common/extensions/sync_helper.cc
> @@ -41,7 +41,8 @@ SyncType GetSyncType(const Extension* extension) {
>    //
>    // TODO(akalin): Relax this restriction once we've put in UI to
>    // approve synced extensions.
> -  if (PluginInfo::HasPlugins(extension))
> +  if (PluginInfo::HasPlugins(extension) ||
> +      extension->HasAPIPermission(APIPermission::kPlugin))
>      return SYNC_TYPE_NONE;
>
>    switch (extension->GetType()) {
> Index: chrome/common/extensions/sync_type_unittest.cc
> diff --git a/chrome/common/extensions/sync_type_unittest.cc
> b/chrome/common/extensions/sync_type_unittest.cc
> index
>
c64d8b838a1a90a6f35bd1c2193e395db3606d68..a74ed865e33e35f8bab8b132458b192d04c8ba10
> 100644
> --- a/chrome/common/extensions/sync_type_unittest.cc
> +++ b/chrome/common/extensions/sync_type_unittest.cc
> @@ -3,6 +3,7 @@
>  // found in the LICENSE file.
>
>  #include "base/files/file_path.h"
> +#include "chrome/common/extensions/api/plugins/plugins_handler.h"
>  #include "chrome/common/extensions/extension.h"
>  #include "chrome/common/extensions/extension_manifest_constants.h"
>  #include "chrome/common/extensions/manifest.h"
> @@ -24,14 +25,15 @@ class ExtensionSyncTypeTest : public testing::Test {
>      THEME
>    };
>
> -  static scoped_refptr<Extension> MakeSyncTestExtension(
> +  static scoped_refptr<Extension>
> MakeSyncTestExtensionWithPluginPermission(
>        SyncTestExtensionType type,
>        const GURL& update_url,
>        const GURL& launch_url,
>        Manifest::Location location,
>        int num_plugins,
>        const base::FilePath& extension_path,
> -      int creation_flags) {
> +      int creation_flags,
> +      bool has_plugin_permission) {
>      DictionaryValue source;
>      source.SetString(keys::kName, "PossiblySyncableExtension");
>      source.SetString(keys::kVersion, "0.0.0.0");
> @@ -55,6 +57,11 @@ class ExtensionSyncTypeTest : public testing::Test {
>        }
>        source.Set(keys::kPlugins, plugins);
>      }
> +    if (has_plugin_permission) {
> +      ListValue* plugins = new ListValue();
> +      plugins->Set(0, new StringValue("plugin"));
> +      source.Set(keys::kPermissions, plugins);
> +    }
>
>      std::string error;
>      scoped_refptr<Extension> extension = Extension::Create(
> @@ -64,6 +71,19 @@ class ExtensionSyncTypeTest : public testing::Test {
>      return extension;
>    }
>
> +  static scoped_refptr<Extension> MakeSyncTestExtension(
> +      SyncTestExtensionType type,
> +      const GURL& update_url,
> +      const GURL& launch_url,
> +      Manifest::Location location,
> +      int num_plugins,
> +      const base::FilePath& extension_path,
> +      int creation_flags) {
> +    return MakeSyncTestExtensionWithPluginPermission(
> +        type, update_url, launch_url, location, num_plugins,
> extension_path,
> +        creation_flags, false);
> +  }
> +
>    static const char kValidUpdateUrl1[];
>    static const char kValidUpdateUrl2[];
>  };
> @@ -238,6 +258,16 @@ TEST_F(ExtensionSyncTypeTest, ExtensionWithTwoPlugins)
> {
>    if (extension.get())
>      EXPECT_FALSE(sync_helper::IsSyncableExtension(extension));
>  }
> +
> +TEST_F(ExtensionSyncTypeTest, ExtensionWithPluginPermission) {
> +  scoped_refptr<Extension> extension(
> +      MakeSyncTestExtensionWithPluginPermission(EXTENSION, GURL(), GURL(),
> +                                                Manifest::INTERNAL,
> +                                                0, base::FilePath(),
> +                                                Extension::NO_FLAGS,
> true));
> +  if (extension.get())
> +    EXPECT_FALSE(sync_helper::IsSyncableExtension(extension));
> +}
>  #endif // !defined(OS_CHROMEOS)
>
>  }  // namespace extensions
>
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Extensions reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to extensions-reviews+unsubscribe@chromium.org.
> For more options, visit
> https://groups.google.com/a/chromium.org/groups/opt_out.
>
>

[Matt Perry, 2013-06-20 19:25:09]

On 2013/06/20 19:21:08, Jeffrey Yasskin wrote:
> On Thu, Jun 20, 2013 at 11:37 AM,  <mailto:meacer@chromium.org> wrote:
> > Reviewers: Julien Tinnes, Jeffrey Yasskin,
> >
> > Message:
> >
> > On 2013/06/20 03:08:42, Jeffrey Yasskin wrote:
> >>
> >> Ah, I see. It's not an inconsistency between an empty plugins list and a
> >> non-empty one; it's an assumption that the only way "plugin" winds up on
> >> the
> >> permissions list is through the addition at
> >
> >
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext....
> >>
> >> LGTM.
> >
> >
> >> I suspect the eventual desired state is to only check HasAPIPermission,
> >> since
> >> the manifest handlers won't be able to load a list of plugins without
> >> setting
> >> that, but we may not be able to block setting the permission explicitly
> >> before
> >> manifest v3.
> >
> >
> >
> > Actually, does it make sense to just remove "plugin" permission?
> > Is that permission even useful without also having the "plugins" list?
> 
> It would make sense to remove the ability to put "plugin" in the
> permissions list, but I'm not sure we can do that without breaking any
> extensions that have done it. I believe it doesn't make sense to
> remove the permission itself, since that's how we trigger the dialog.
> 
> ext_plorer actually says we might be able to give an error for
> "plugin" in the permissions list:
> 
> /google/data/ro/projects/ext_plorer/ext_plorer
> >>> manifests = Manifests()
> 79565 manifests fetched (10/10 shards)
> 79395 manifests loaded (170 failures)
> >>> with_plugins = manifests.Filter(lambda m: 'plugin' in GetPermissions(m))
> 2 manifests match filter
> >>> for id, m in with_plugins.items():
> ...   print id, GetNumUsers(m)
> ...
> lankeacickinbpamhdnipeiphdoomjol 3
> gkogkdnapiipmhoodhkgplmgohgbeddi 260
> 
> 
> But I'd want Yoyo or Matt to chime in before we decide to block that
> permission string. I don't think we do it for any other string, which
> might be intentional.

I didn't realize we even had a "plugins" permission. It's redundant to have one,
and we don't do it for other APIs with implicit permissions (like
browser_actions), so we should remove it.

[Jeffrey Yasskin, 2013-06-20 20:53:25]

On Thu, Jun 20, 2013 at 12:25 PM,  <mpcomplete@chromium.org> wrote:
> On 2013/06/20 19:21:08, Jeffrey Yasskin wrote:
>
>> On Thu, Jun 20, 2013 at 11:37 AM,  <mailto:meacer@chromium.org> wrote:
>> > Reviewers: Julien Tinnes, Jeffrey Yasskin,
>> >
>> > Message:
>> >
>> > On 2013/06/20 03:08:42, Jeffrey Yasskin wrote:
>> >>
>> >> Ah, I see. It's not an inconsistency between an empty plugins list and
>> >> a
>> >> non-empty one; it's an assumption that the only way "plugin" winds up
>> >> on
>> >> the
>> >> permissions list is through the addition at
>> >
>> >
>> >
>
>
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext....
>>
>> >>
>> >> LGTM.
>> >
>> >
>> >> I suspect the eventual desired state is to only check HasAPIPermission,
>> >> since
>> >> the manifest handlers won't be able to load a list of plugins without
>> >> setting
>> >> that, but we may not be able to block setting the permission explicitly
>> >> before
>> >> manifest v3.
>> >
>> >
>> >
>> > Actually, does it make sense to just remove "plugin" permission?
>> > Is that permission even useful without also having the "plugins" list?
>
>
>> It would make sense to remove the ability to put "plugin" in the
>> permissions list, but I'm not sure we can do that without breaking any
>> extensions that have done it. I believe it doesn't make sense to
>> remove the permission itself, since that's how we trigger the dialog.
>
>
>> ext_plorer actually says we might be able to give an error for
>> "plugin" in the permissions list:
>
>
>> /google/data/ro/projects/ext_plorer/ext_plorer
>> >>> manifests = Manifests()
>> 79565 manifests fetched (10/10 shards)
>> 79395 manifests loaded (170 failures)
>> >>> with_plugins = manifests.Filter(lambda m: 'plugin' in
>> >>> GetPermissions(m))
>> 2 manifests match filter
>> >>> for id, m in with_plugins.items():
>> ...   print id, GetNumUsers(m)
>> ...
>> lankeacickinbpamhdnipeiphdoomjol 3
>> gkogkdnapiipmhoodhkgplmgohgbeddi 260
>
>
>
>> But I'd want Yoyo or Matt to chime in before we decide to block that
>> permission string. I don't think we do it for any other string, which
>> might be intentional.
>
>
> I didn't realize we even had a "plugins" permission. It's redundant to have
> one,
> and we don't do it for other APIs with implicit permissions (like
> browser_actions), so we should remove it.
>

Summarizing my impression of the followup discussion for this comment:
The "plugin" string is defined at
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext....
Because of the extra flags on that permission, it's not clear that
deleting that one line will improve the situation. There's a new
"features" system that should replace this eventually, but certainly
not in time to replace this patch.

That all sound right?
Jeffrey

[meacer, 2013-06-20 22:24:45]

On 2013/06/20 20:53:25, Jeffrey Yasskin wrote:
> On Thu, Jun 20, 2013 at 12:25 PM,  <mailto:mpcomplete@chromium.org> wrote:
> > On 2013/06/20 19:21:08, Jeffrey Yasskin wrote:
> >
> >> On Thu, Jun 20, 2013 at 11:37 AM,  <mailto:meacer@chromium.org> wrote:
> >> > Reviewers: Julien Tinnes, Jeffrey Yasskin,
> >> >
> >> > Message:
> >> >
> >> > On 2013/06/20 03:08:42, Jeffrey Yasskin wrote:
> >> >>
> >> >> Ah, I see. It's not an inconsistency between an empty plugins list and
> >> >> a
> >> >> non-empty one; it's an assumption that the only way "plugin" winds up
> >> >> on
> >> >> the
> >> >> permissions list is through the addition at
> >> >
> >> >
> >> >
> >
> >
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext....
> >>
> >> >>
> >> >> LGTM.
> >> >
> >> >
> >> >> I suspect the eventual desired state is to only check HasAPIPermission,
> >> >> since
> >> >> the manifest handlers won't be able to load a list of plugins without
> >> >> setting
> >> >> that, but we may not be able to block setting the permission explicitly
> >> >> before
> >> >> manifest v3.
> >> >
> >> >
> >> >
> >> > Actually, does it make sense to just remove "plugin" permission?
> >> > Is that permission even useful without also having the "plugins" list?
> >
> >
> >> It would make sense to remove the ability to put "plugin" in the
> >> permissions list, but I'm not sure we can do that without breaking any
> >> extensions that have done it. I believe it doesn't make sense to
> >> remove the permission itself, since that's how we trigger the dialog.
> >
> >
> >> ext_plorer actually says we might be able to give an error for
> >> "plugin" in the permissions list:
> >
> >
> >> /google/data/ro/projects/ext_plorer/ext_plorer
> >> >>> manifests = Manifests()
> >> 79565 manifests fetched (10/10 shards)
> >> 79395 manifests loaded (170 failures)
> >> >>> with_plugins = manifests.Filter(lambda m: 'plugin' in
> >> >>> GetPermissions(m))
> >> 2 manifests match filter
> >> >>> for id, m in with_plugins.items():
> >> ...   print id, GetNumUsers(m)
> >> ...
> >> lankeacickinbpamhdnipeiphdoomjol 3
> >> gkogkdnapiipmhoodhkgplmgohgbeddi 260
> >
> >
> >
> >> But I'd want Yoyo or Matt to chime in before we decide to block that
> >> permission string. I don't think we do it for any other string, which
> >> might be intentional.
> >
> >
> > I didn't realize we even had a "plugins" permission. It's redundant to have
> > one,
> > and we don't do it for other APIs with implicit permissions (like
> > browser_actions), so we should remove it.
> >
> 
> Summarizing my impression of the followup discussion for this comment:
> The "plugin" string is defined at
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext....
> Because of the extra flags on that permission, it's not clear that
> deleting that one line will improve the situation. There's a new
> "features" system that should replace this eventually, but certainly
> not in time to replace this patch.
> 
> That all sound right?
> Jeffrey

"plugin" permission is used in a few places such as preference migration:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

I looked at removing it but it looks complicated to do in a short time.
I also suggest going through with this patch, and then filing another bug
to remove the "plugin" permission. I'll commit this one if there are no
objections :)
My suggestion is go through with this patch and

[commit-bot: I haz the power, 2013-06-21 01:50:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/meacer@chromium.org/16816024/16001

[commit-bot: I haz the power, 2013-06-21 15:33:37]

Change committed as 207830