544597 - Remap plugin names to a more readable form for plugin-crashed UI

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Justin Dolske [:Dolske]]

The plugin-crashed UI from bug displays the name of the plugin that crashed. Unfortunately, the names provided by the plugins are not always ideal, because they've typically been buried in about:plugins or the EM UI, where users don't encounter them often.

We'd like to remap these names to nicer versions. EG, "Shockwave Flash" --> "Adobe Flash", "QuickTime Plug-in 7.6.3" --> "Quicktime", etc. I'd like at least clean up the names for the Top-N plugins, but I think the folks who did the plugin update page may have a more exhaustive list so I might just steal that.

Comment 1

[Justin Dolske [:Dolske]]

Looks like we should be able to steal some of the work done for PFS2...

http://svn.mozilla.org/projects/pfs2/trunk/plugins-info/

Comment 2

[Axel Hecht [:Pike]]

I'm not sure, should we expose the readable names to l10n?

Comment 3

[Justin Dolske [:Dolske]]

I don't think the names need to be exposed to l10n, afaik plugins already don't localize these strings. And if a few did, things would be no worse than the current UI.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Comment 5

[Justin Dolske [:Dolske]]

Comment on attachment 433473 [details] [diff] [review]
Patch v.1

jst for the rs to the /content parts (which is basically just cut'n'paste of what's already there).

Comment 6

[Justin Dolske [:Dolske]]

To build up the list of names, I basically looked at the plugins that were installed on the systems I had at hand, and then Googled around for text and screenshots related to about:plugins.

If we want to make this list more exhaustive (and I think we probably *don't*), http://plugindoc.mozdev.org/ would be a source for a whole pile. Would also be nice if the Plugin Check page (http://www.mozilla.com/en-US/plugincheck/) could log this data.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 433473 [details] [diff] [review]
Patch v.1

Content parts look good. r=jst

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 433473 [details] [diff] [review]
Patch v.1

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+    { niceName: "Sun Java",                 regexs: [/^Java Embedding Plugin/ , /^Java.* Platform/, /^Java.* Plug/] },

I don't think these are all guaranteed to be *Sun* Java, right? There are other cases here where it seems like we might blame the wrong company, which makes me a bit nervous...

I wonder whether we need to care about ® and ™ and such...

>+  // Map the plugin's name to a filtered version more suitable for user UI.
>+  makeNicePluginName : function (aName, aFilename) {
>+    // Check for a cached mapping, so we don't do a ton of regexs every crash.
>+    if (aName in this.nicePluginNameCache)
>+      return this.nicePluginNameCache[aName];

Should probably cache based on nameToTest rather than just name.

>+    // Glue the strings together for easier testing.

I think I would prefer testing against both strings separately just to avoid potential weirdness.

>+    let nameToTest = aName + aFilename.toLowerCase();
>+    for each (let entry in this.nicePluginNameLookups) {

>+      for each (let regex in entry.regexs) {
>+        if (regex.test(nameToTest)) {

how about:

if (entry.regexs.some(function (r) r.test(aName) || r.test(aFileName))

>+          let result = entry.niceName;
>+          this.nicePluginNameCache[aName] = result;
>+          return result;

>+    let result = newName;
>+    this.nicePluginNameCache[aName] = result;
>+    return result;

could merge these into one line.

>+    // We have no data for this plugin, so just clean up the name a bit.
>+    
>+    // Strip off trailing version numbers (eg 1.6_06-2)

I think this is better accomplished using something like:

newName = aName.replace(/[\s\d\.\-\_\(\)]+$/, "");
newName = newName.replace(/plug-?in\s*$/i, "");


>     let doPrompt        = true; // XXX followup for .getData("doPrompt");
>     let submitReports   = true; // XXX followup for .getData("submitReports");

(are these bugs filed?)

Comment 9

[Dave Townsend [:mossop]]

I'd really like to have the name mapping somewhere more central (like in the PluginProvider.jsm I am adding as part of the EM rewrite perhaps!) so we can use the same names in the EM UI and anywhere else that needs it.

Comment 10

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Comment 11

[Justin Dolske [:Dolske]]

(In reply to comment #8)

> I don't think these are all guaranteed to be *Sun* Java, right?

The examples I found were all Sun. I don't think there are too many Java plugins out there...

> I wonder whether we need to care about ® and ™ and such...

I'm pretty sure not.

> >     let doPrompt        = true; // XXX followup for .getData("doPrompt");
> >     let submitReports   = true; // XXX followup for .getData("submitReports");
> 
> (are these bugs filed?)

Sortof, it's part of bug 550303 and related work that's on my plate right now.


(In reply to comment #9)
> I'd really like to have the name mapping somewhere more central (like in the
> PluginProvider.jsm I am adding as part of the EM rewrite perhaps!) so we can
> use the same names in the EM UI and anywhere else that needs it.

It's easy to move, and will be a great followup bug. :-) Leaving here, since this code will also land on 1.9.2 for Lorentz.

Comment 12

[timeless]

technically the java plugin for os x is Apple's not Sun's, so no, you should not blame Sun, users should be contacting Apple for support.

there's also IBM Java, IcedTea Java and a couple of others floating around, I'm not sure if your regexp would actually match those (I'd hope not).

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 433506 [details] [diff] [review]
Patch v.2

We discussed a more limited approach that just covers flash+stripping as a first step, on IRC.

Comment 14

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 437747 [details] [diff] [review]
Patch v.3

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+  makeNicePluginName : function (aName, aFilename) {

>+    // Check for a cached mapping, so we don't do a ton of regexs every crash.
>+    if (cacheKey in this.nicePluginNameCache)
>+      return this.nicePluginNameCache[cacheKey];

Seems like we should just avoid adding the cache now, and add it when we actually implement the more elaborate fix.

>+    // Clean up the plugin name by stripping off any trailing version numbers
>+    // or "plugin". EG, "Foo Bar Plugin 1.23_02" --> "Foo Bar"
>+    let newName = aName.replace(/[\s\d\.\-\_\(\)(plugin)(plug-in)]+$/i, "");

This isn't quite doing what you would expect, see e.g. what it does to "CoolWin 4.3 Plugin".

How about:
.replace(/\bplug-?in\b/i, "").replace(/[\s\d\.\-\_\(\)]+$/, ""); ?

Comment 16

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Comment 17

[Justin Dolske [:Dolske]]

Comment on attachment 437777 [details] [diff] [review]
Patch v.4

Ha, jst already reviewed the patch v.1, so no need to look at it again.

Comment 18

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/4a5cbe606e2a

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Does this need l10n equivalent opt-in patches?

Comment 20

[Justin Dolske [:Dolske]]

Attachment: Patch v.4 (branch)

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 437981 [details] [diff] [review]
Patch v.4 (branch)

a1.9.2.4 = beltzner

Comment 22

[Justin Dolske [:Dolske]]

Filed bug 558207 for getting some real data so we can to additional plugin name remapping.

Comment 23

[Justin Dolske [:Dolske]]

Pushed to 192: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b2b5137c9473