Closed Bug 847072 Opened 11 years ago Closed 10 years ago

Plugin deprecation warning cuts off Plugin name

Categories

(Core Graveyard :: Plug-ins, defect, P3)

19 Branch
defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla35

People

(Reporter: b.eckenfels, Assigned: bruteforks, Mentored)

References

Details

(Whiteboard: [lang=JS][good first bug])

Attachments

(2 files, 3 obsolete files)

Attached image screenshot.jpg
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130215130331

Steps to reproduce:

Opened a webn site which has a Java Applet. This is on Windows 7, with a fairly standard resolution/font ratio.


Actual results:

Firefox told me that the Java Version is vulnerable and asked me if I want to activate it. The warning message had cut off the name of the plugin right before the actual interesting Version number.


Expected results:

Show the whole Plugin name, at least for most common deprecation warning for the Java Plugin.
The "longest" Plugin Name I have on my System is "HP Virtual Room Client Launcher Plugin" and the Java Plugins are named:

Java(TM) Platform SE 7 U15
    File: npjp2.dll
    Version: 10.15.2.3
    Next Generation Java Plug-in 10.15.2 for Mozilla browsers
Component: Untriaged → Plug-ins
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
The click-to-play UI is currently getting a redesign / new concept, so as this is not a pressing issue we should see whether this is obsoleted by UI changes.

If it isn't i think we should do a proper "..." cutoff and maybe show the full string as a tooltip or something.
OS: Windows 7 → All
Priority: -- → P3
Hardware: x86_64 → All
As it was originally designed, that UI intentionally removes (or at least tries to remove) version numbers from the plugin name (see https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-plugins.js#79 ). In the case of plugins that have a secure version to update to, there would be a link to the Mozilla plugin check/update page, so the version number isn't important as long as you replace the one you have with a secure version. However, in this case there isn't a secure version to update to. I'm not actually sure exposing the version number in this UI is very helpful, either, though, because there's really nothing you can do about it (and as an aside, you can see what version you have in about:addons -> Plugins or about:plugins, but I suppose you know that already by your second comment).

There may still be a bug with long plugin names, though. Bernd - what happens if you set plugins.click_to_play to true in about:config and then visit a page that uses the plugin with the long name? (or is it not an in-content plugin?)
Flags: needinfo?(b.eckenfels)
(In reply to David Keeler (:keeler) from comment #3)
> what
> happens if you set plugins.click_to_play to true in about:config and then
> visit a page that uses the plugin with the long name?
Same behavior
David, so this is just the version number trimming cutting off the 15?
Couldn't this just simply special-case Java then?
Flags: needinfo?(b.eckenfels)
(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> David, so this is just the version number trimming cutting off the 15?

As far as I can tell, yes (and like I said, this is the intended behavior).

> Couldn't this just simply special-case Java then?

Special-case to not trim the version number for Java? Or special-case to take off the "U" as well, since it's confusing that it gets left behind?
I guess it does not make sense to argue about this "hide all usefull technical information" policy in Firefox. However I tell you anyway why I think it is important to know what Java Version is blocked: Oracle releases a new Version now every week and Mozilla now deprecvates and blocks versions every week. The dialog should tell me (without any further actions) if the problem was my Plugin Version or Oracle. (in this case it was Oracle). 

How should I make an informed decision of allowing this Plugin, if I dont know what Version I will allow?
(In reply to David Keeler (:keeler) from comment #6)
> Or special-case to
> take off the "U" as well, since it's confusing that it gets left behind?

That, sounds simple and should also apply regardless of redesigns. Or, what do you think?
With the reimplemented doorhanger (bug 880735 and dependencies) we just show "Java Applet".
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WORKSFORME
(In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> With the reimplemented doorhanger (bug 880735 and dependencies) we just show
> "Java Applet".

I disagree, on Firefox 26 I still see a "enable plugin" Warning which says "Activate Java(TM) Platform  SE 7 U.". Even on the "FAQ" pages screenshots the broken version number is visible: https://support.mozilla.org/en-US/kb/why-do-i-have-click-activate-plugins?as=u&utm_source=inproduct
Thanks for the heads-up - it's currently fine on OS X ("Java Applet Plug-in"), but not on Windows ("Java(TM) Plaftform SE 7 UNN").

This is due to how makeNicePluginName() [1] works. We really should just special-case for Java here.

[1] http://hg.mozilla.org/mozilla-central/annotate/1ad9af3a2ab8/browser/base/content/browser-plugins.js#l71
Assignee: nobody → georg.fritzsche
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
For this bug we need to change makeNicePluginName().
I think we should be fine if we return "Java" for |aName| starting with "Java" and followed by a non-letter (to cover "Java(TM)" etc.).
Assignee: georg.fritzsche → nobody
Whiteboard: [mentor=gfritzsche][lang=JS][good first bug]
Hi, I am interested in working on this bug. So please can u assign this bug to me?

Thanks in Advance,

Regards,
A. Anup
Sure, but lets check this out after bug 863773.
Mentor: georg.fritzsche
Whiteboard: [mentor=gfritzsche][lang=JS][good first bug] → [lang=JS][good first bug]
Note that makeNicePluginName() moved to PluginContent.jsm due to the recent e10s work:
http://hg.mozilla.org/mozilla-central/annotate/5e704397529b/browser/modules/PluginContent.jsm#l141
Assignee: nobody → bruteforks
Attached patch bug847072.patch (obsolete) — Splinter Review
(In reply to Georg Fritzsche [:gfritzsche] from comment #12)
> For this bug we need to change makeNicePluginName().
> I think we should be fine if we return "Java" for |aName| starting with
> "Java" and followed by a non-letter (to cover "Java(TM)" etc.).

I added a regex to check for "Java" at the beginning of the string, followed by a non-letter character. It seems to work. Is this the correct approach?
Comment on attachment 8497383 [details] [diff] [review]
bug847072.patch

Review of attachment 8497383 [details] [diff] [review]:
-----------------------------------------------------------------

Note that you can request feedback on a patch from specific people by setting the feedback flag to ? and picking the requestee.

::: browser/modules/PluginContent.jsm
@@ +140,5 @@
>    // Map the plugin's name to a filtered version more suitable for user UI.
>    makeNicePluginName : function (aName) {
>      if (aName == "Shockwave Flash")
>        return "Adobe Flash";
> +    else if (/^Java[^\w]/.exec(aName))

We don't need an |else| here, we already |return| for the Flash case, otherwise this looks fine.
Attached patch bug847072v1.1.patch (obsolete) — Splinter Review
Ok, updated.

(In reply to Georg Fritzsche [:gfritzsche] from comment #17)
> Note that you can request feedback on a patch from specific people by
> setting the feedback flag to ? and picking the requestee.

Ah, thanks for the tip.
Attachment #8497383 - Attachment is obsolete: true
Attachment #8497468 - Flags: feedback?(georg.fritzsche)
Comment on attachment 8497468 [details] [diff] [review]
bug847072v1.1.patch

Review of attachment 8497468 [details] [diff] [review]:
-----------------------------------------------------------------

This still looks good, i think we should look into test coverage next.
Attachment #8497468 - Flags: feedback?(georg.fritzsche) → feedback+
(In reply to Georg Fritzsche [:gfritzsche] from comment #19)
> This still looks good, i think we should look into test coverage next.

Actually, test-coverage for the string-building for the notification gets a little involved. We don't have coverage for other notification strings and - sadly - no nice setup to unit-test things like makeNicePluginName().
I think we should skip it in this specific case.
Attachment #8497468 - Flags: review?(mconley)
Just for the record, it does not look good to me. It does not make sense to ask a user if he wants to activate a plugin and not tell him what version it is :-/
This didn't change here though, this just fixes the broken name handling for Java on Windows.
The additional version information is probably a discussion for firefox-dev or maybe an enhancement bug.
Comment on attachment 8497468 [details] [diff] [review]
bug847072v1.1.patch

Review of attachment 8497468 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! Just two suggestions.

::: browser/modules/PluginContent.jsm
@@ +140,5 @@
>    // Map the plugin's name to a filtered version more suitable for user UI.
>    makeNicePluginName : function (aName) {
>      if (aName == "Shockwave Flash")
>        return "Adobe Flash";
> +    if (/^Java[^\w]/.exec(aName))

I believe [^\w] is equivalent to \W.

Also, a comment about what this regex is doing would be good for future readers.
Attachment #8497468 - Flags: review?(mconley)
(In reply to Georg Fritzsche [:gfritzsche] from comment #22)
> This didn't change here though, this just fixes the broken name handling for
> Java on Windows.
> The additional version information is probably a discussion for firefox-dev
> or maybe an enhancement bug.

yes maybe, however the Comment#0 (original bug report) even had a story to explain why the version is needed and the result was it was removed :/
Attached patch bug847072v1.2.patch (obsolete) — Splinter Review
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #23)
> I believe [^\w] is equivalent to \W.
> 
> Also, a comment about what this regex is doing would be good for future
> readers.

Thanks, I made the suggested changes.
Attachment #8497468 - Attachment is obsolete: true
Comment on attachment 8497674 [details] [diff] [review]
bug847072v1.2.patch

Review of attachment 8497674 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good - thanks!
Attachment #8497674 - Flags: review+
Trivial change and fine locally, a try run is not needed here.
Keywords: checkin-needed
Just added mconley to reviewer field of commit message
Attachment #8497674 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ca5765dddaed
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
A quick check on Windows 7 x64 using Firefox 35 Beta 3 (BuildID=20141211142524) confirms that "Java" is now displayed in the doorhanger instead of "Java Platform SE 7 U" (which displays in Firefox 34.0.5). This was the intended fix so marking this as Verified.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: