Fix plugin grouping for PDF in about:plugins

Fix plugin grouping in about:plugins

The PDF viewer has 2 plugins associated with it. These are grouped correctly for Google Chrome but not for Chromium. This groups these plugins correctly for Chromium by updating the plugin metadata.

BUG=459113
Committed: https://crrev.com/4abcce37a073abff8760a1df21eaa667c2f074cb
Cr-Commit-Position: refs/heads/master@{#316957}

[raymes, 2015-02-17 06:25:36]

raymes@chromium.org changed reviewers:
+ bauerb@chromium.org

[Bernhard Bauer, 2015-02-17 09:36:00]

Hm... the plugin identifier is supposed to be the feature that distinguishes
different plugins or groups similar plugins, whereas the group name is just a
user-visible name. What is the identifier for the two PDF plugins? Could we just
give both plugins the same identifier?

[raymes, 2015-02-17 22:06:12]

On 2015/02/17 09:36:00, Bernhard Bauer wrote:
> Hm... the plugin identifier is supposed to be the feature that distinguishes
> different plugins or groups similar plugins, whereas the group name is just a
> user-visible name. What is the identifier for the two PDF plugins? Could we
just
> give both plugins the same identifier?

Hmm, but that doesn't seem to be the case according to the code in
plugin_finder.cc. AFAICT the grouping of plugins is sort of arbitrary in that we
should group them by whatever makes sense in the UI. I'm assuming the reason
this grouping stuff was added was to group in-process and out-of-process plugins
which users would typically want to enable/disable together.

The plugin identifier is basically the DLL name. But in the case of extensions
that are plugins (like the new PDF plugin) they don't have a DLL name and that's
why these things aren't being grouped together currently. That's not very easy
to change unfortunately.

Outside of grouping things by a separate "group name" property, I think it makes
sense to group them by their name here. wdyt?

[chromium-reviews, 2015-02-17 23:17:33]

The grouping was added for two reasons:
* Some plugins (Quicktime) are split up into multiple DLLs, so grouping
them allows to disable all of Quicktime at once.
* Some plugins exist in multiple versions (in-process vs. out-of-process,
NPAPI vs PPAPI for Flash, debug vs. release for Flash as well, etc.), so
this showing them together again allows disabling all of them, as well
as making
it easier to select which one should be used.

Both of these things are based on the notion of something identifying "the"
plugin, to find out that two WebPluginInfo structs are actually the same
plugin (like "Flash" for "Shockwave Flash Player"). For the most common
plugins, this is based on metadata we know about them (see
chrome/browser/resources/metadata/plugins_*.json). Only for cases that we
can't match there, we fall back to heuristics, like deriving the identifier
from the name, or from the filename.

We also use the identifier for per-plugin settings (this makes sure for
example that a previously disabled version of a plugin stays disabled if
the plugin is updated, or it allows exempting a certain plugin from
click-to-play completely), and that's the reason why I'm so fixated on
using the identifier :)

So, for this particular case, updating the metadata so that it knows about
all variants of the PDF plugin might be the easiest way. There is a
WebPluginInfo for the PDF viewer extension, right? We just need to make
sure the plugin_matcher field in the metadata JSON file matches the
WebPluginInfo name.

On Tue Feb 17 2015 at 10:06:13 PM <raymes@chromium.org> wrote:

> On 2015/02/17 09:36:00, Bernhard Bauer wrote:
> > Hm... the plugin identifier is supposed to be the feature that
> > distinguishes
> > different plugins or groups similar plugins, whereas the group name is
> > just a
> > user-visible name. What is the identifier for the two PDF plugins? Could
> > we
> just
> > give both plugins the same identifier?
>
> Hmm, but that doesn't seem to be the case according to the code in
> plugin_finder.cc. AFAICT the grouping of plugins is sort of arbitrary in
> that we
> should group them by whatever makes sense in the UI. I'm assuming the
> reason
> this grouping stuff was added was to group in-process and out-of-process
> plugins
> which users would typically want to enable/disable together.
>
> The plugin identifier is basically the DLL name. But in the case of
> extensions
> that are plugins (like the new PDF plugin) they don't have a DLL name and
> that's
> why these things aren't being grouped together currently. That's not very
> easy
> to change unfortunately.
>
> Outside of grouping things by a separate "group name" property, I think it
> makes
> sense to group them by their name here. wdyt?
>
> https://codereview.chromium.org/931063002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Bernhard Bauer, 2015-02-17 23:20:47]

(Oh, that's hilarious. I replied via mail from my @google.com address, and
Rietveld somehow turned it into chromium-reviews :) )

On 2015/02/17 23:17:33, chromium-reviews wrote:
> The grouping was added for two reasons:
> * Some plugins (Quicktime) are split up into multiple DLLs, so grouping
> them allows to disable all of Quicktime at once.
> * Some plugins exist in multiple versions (in-process vs. out-of-process,
> NPAPI vs PPAPI for Flash, debug vs. release for Flash as well, etc.), so
> this showing them together again allows disabling all of them, as well
> as making
> it easier to select which one should be used.
> 
> Both of these things are based on the notion of something identifying "the"
> plugin, to find out that two WebPluginInfo structs are actually the same
> plugin (like "Flash" for "Shockwave Flash Player"). For the most common
> plugins, this is based on metadata we know about them (see
> chrome/browser/resources/metadata/plugins_*.json). Only for cases that we
> can't match there, we fall back to heuristics, like deriving the identifier
> from the name, or from the filename.
> 
> We also use the identifier for per-plugin settings (this makes sure for
> example that a previously disabled version of a plugin stays disabled if
> the plugin is updated, or it allows exempting a certain plugin from
> click-to-play completely), and that's the reason why I'm so fixated on
> using the identifier :)
> 
> So, for this particular case, updating the metadata so that it knows about
> all variants of the PDF plugin might be the easiest way. There is a
> WebPluginInfo for the PDF viewer extension, right? We just need to make
> sure the plugin_matcher field in the metadata JSON file matches the
> WebPluginInfo name.
> 
> On Tue Feb 17 2015 at 10:06:13 PM <mailto:raymes@chromium.org> wrote:
> 
> > On 2015/02/17 09:36:00, Bernhard Bauer wrote:
> > > Hm... the plugin identifier is supposed to be the feature that
> > > distinguishes
> > > different plugins or groups similar plugins, whereas the group name is
> > > just a
> > > user-visible name. What is the identifier for the two PDF plugins? Could
> > > we
> > just
> > > give both plugins the same identifier?
> >
> > Hmm, but that doesn't seem to be the case according to the code in
> > plugin_finder.cc. AFAICT the grouping of plugins is sort of arbitrary in
> > that we
> > should group them by whatever makes sense in the UI. I'm assuming the
> > reason
> > this grouping stuff was added was to group in-process and out-of-process
> > plugins
> > which users would typically want to enable/disable together.
> >
> > The plugin identifier is basically the DLL name. But in the case of
> > extensions
> > that are plugins (like the new PDF plugin) they don't have a DLL name and
> > that's
> > why these things aren't being grouped together currently. That's not very
> > easy
> > to change unfortunately.
> >
> > Outside of grouping things by a separate "group name" property, I think it
> > makes
> > sense to group them by their name here. wdyt?
> >
> > https://codereview.chromium.org/931063002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[raymes, 2015-02-17 23:37:22]

Bernhard, thank you for the detailed explanation! Very helpful. I have updated
the metadata files. I just followed the pattern there but let me know if there
is a better way.

Thanks!
Raymes

[Bernhard Bauer, 2015-02-18 09:15:50]

LGTM, thank you!

[raymes, 2015-02-18 21:18:35]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2015-02-18 21:19:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/931063002/20001

[commit-bot: I haz the power, 2015-02-19 01:29:32]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-19 01:30:28]

Patchset 2 (id:??) landed as
https://crrev.com/4abcce37a073abff8760a1df21eaa667c2f074cb
Cr-Commit-Position: refs/heads/master@{#316957}