Closed Bug 863773 Opened 7 years ago Closed 6 years ago

In Options | Applications, a different plugin is shown for a MIME type than the one we'd currently use

Categories

(Firefox :: Preferences, defect, P3)

defect
Points:
3

Tracking

()

RESOLVED WORKSFORME
Firefox 34
Iteration:
34.3

People

(Reporter: brian_duddy, Assigned: rittme, Mentored)

References

Details

(Whiteboard: [good second bug][lang=js])

Attachments

(1 file, 5 obsolete files)

I installed multiple plugins that handle PDFs (Adobe, Foxit, Nitro, and Nuance (i.e. "DocuCom PDF Plus")). In the about:plugins display, these plugins are displayed in order from most recent modification date to oldest. However, only the -least- recently modified enabled plugin is available to open PDFs in the Options>Applications menu. According to Bug 653116, this is the opposite of the intended behavior.
I don't think this has anything to do with plugins. I'm pretty sure that Options/Applications is generated from the list of OS handlers for a particular extension and/or MIME type.

But I also don't understand why we have an Options/Applications menu anyway... pretty sure we should just use the OS handlers. Why does this exist at all?
After a discussion with Gavin on IRC, the answer seems to be that "Options | Applications" gives users the option to view content with "a plugin". The plugin that's displayed, however, doesn't match the plugin we'd actually use in practice.

The code which does this magic is at http://hg.mozilla.org/mozilla-central/annotate/cbfe9618bc2b/browser/components/preferences/applications.js#l1076

This is horribly broken and is an incorrect duplicate of the logic of nsPluginHost::FindPluginForType. This should really be using that function (which is currently not exposed on nsIPluginHost.idl, but could be).

In any case, since this is primarily a cosmetic issue, it's not high on my priority list, but I'll happily mentor somebody who wants to try and fix it.
Component: Plug-ins → Preferences
Keywords: helpwanted
Priority: -- → P3
Product: Core → Firefox
Summary: plugin with oldest modification date is prioritized when multiple installed plugins handle same MIME type → In Options | Applications, a different plugin is shown for a MIME type than the one we'd currently use
Whiteboard: [mentor=bsmedberg][good second bug][lang=C++/JS]
Version: 20 Branch → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
Jan will take a look at this.
Assignee: nobody → jan.fuchsmann
I've managed to expose nsPluginHost::FindPluginForType. This doesn't fix the bug yet, but is a prerequisite.
Looks like Jan doesn't have time to continue this.
The above patch already exposes the functionality as getPluginTagForType(). This still needs to get hooked up in applications.js.
Assignee: jan.fuchsmann → nobody
Hi, I am interested in working on this bug. So please can you assign this to me?

Thanks in Advance,

Regards,
A. Anup
Let's look into bug 942461 first and this afterwards :)
Turns out that that bug was already closed, so assigning here.
Assignee: nobody → allamsetty.anup
Comment on attachment 746518 [details] [diff] [review]
Expose FindPluginForType in nsIPluginHost.idl

As it turns out getPluginTagForType() was already added in bug 880735 in the meantime.
Attachment #746518 - Attachment is obsolete: true
Anup currently doesn't have time.
Assignee: allamsetty.anup → nobody
OS: Windows 7 → All
Hardware: x86_64 → All
Mentor: benjamin
Whiteboard: [mentor=bsmedberg][good second bug][lang=C++/JS] → [good second bug][lang=C++/JS]
Assignee: nobody → bernardo
I used nsPluginHost method getPluginTagForType to find the actual plugin used to handle each mime type available from the navigator object.

I did manual testing with some plugins to verify it's working.
Attachment #8454110 - Flags: review?(benjamin)
Attachment #8454110 - Flags: review?(benjamin) → review?(georg.fritzsche)
Bernardo, sorry for the delay here. This is a really busy week while we try to finish some urgent things up for Firefox 33, so i'm not sure if i can get to this review this week.
I'll definitely be on it early next week though.
Comment on attachment 8454110 [details] [diff] [review]
rev1 - Plugin handlers are now loaded using pluginHost method getPluginTagForType

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

Sorry for the delay. This looks good to me, thanks!
Sadly, we currently support both in-content prefs (enabled on Nightly) and the normal ones [1] elsewhere. So, we need to fix it there as well.

[1] browser/components/preferences/applications.js
Attachment #8454110 - Flags: review?(jaws)
Attachment #8454110 - Flags: review?(georg.fritzsche)
Attachment #8454110 - Flags: feedback+
Attachment #8454110 - Flags: review?(jaws)
Mentor: georg.fritzsche
Flags: firefox-backlog+
Thank you for you review, Georg!

Sorry, I forgot about the normal preferences.
This should work.
Attachment #8454110 - Attachment is obsolete: true
Attachment #8461038 - Flags: review?(georg.fritzsche)
Comment on attachment 8461038 [details] [diff] [review]
rev 2 - Applications pane plugin handlers are now loaded using pluginHost method getPluginTagForType

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

Thanks!
Blair, can you review this? hg log suggests you're a candidate here?
Attachment #8461038 - Flags: review?(georg.fritzsche)
Attachment #8461038 - Flags: review?(bmcbride)
Attachment #8461038 - Flags: feedback+
Comment on attachment 8461038 [details] [diff] [review]
rev 2 - Applications pane plugin handlers are now loaded using pluginHost method getPluginTagForType

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

::: browser/components/preferences/applications.js
@@ +1102,3 @@
>  
> +    for(let mimeType of mimeTypes) {
> +      let plugin = pluginHost.getPluginTagForType(mimeType.type);

Actually, you don't even need getPluginTagForType() - since you're only using nsIPluginTag.name, navigator.mimeTypes already contains all the info you need. mimeType.enabledPlugin will give you the same plugin as getPluginTagForType() will give you.
Attachment #8461038 - Flags: review?(bmcbride) → review-
Hi Bernardo, do you think you can pick this up and address the minor changes from comment 16?
Flags: needinfo?(bernardo)
Whiteboard: [good second bug][lang=C++/JS] → [good second bug][lang=js]
Hi Georg, sure, I'll pick this up and finish it on this next iteration.
Flags: needinfo?(bernardo)
Iteration: --- → 34.3
Points: --- → 3
Status: NEW → ASSIGNED
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
QA Contact: bogdan.maris
Fixed the review changes.
Attachment #8461038 - Attachment is obsolete: true
Attachment #8476852 - Flags: review?(bmcbride)
Comment on attachment 8476852 [details] [diff] [review]
rev 3 - Applications pane plugin handlers are now loaded using enabledPlugin from navigator.mimeTypes.

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

::: browser/components/preferences/applications.js
@@ +1102,5 @@
> +    for(let mimeType of mimeTypes) {
> +      let handlerInfoWrapper;
> +      if (mimeType.type in this._handledTypes)
> +          handlerInfoWrapper = this._handledTypes[mimeType.type];
> +      else {

Nit: If you wrap one side of an if-else, always wrap the other too.

Seeing it like this, it's far too easy to assume the contents of the else-block is just a single line.
Attachment #8476852 - Flags: review?(bmcbride) → review+
Thank you for the review, Unfocused.
Attachment #8476852 - Attachment is obsolete: true
Attachment #8478408 - Flags: review+
Keywords: checkin-needed
Can we please run this through Try first? :)
Keywords: checkin-needed
Comment on attachment 8478408 [details] [diff] [review]
rev 4 - Applications pane plugin handlers are now loaded using enabledPlugin from navigator.mimeTypes. r=Unfocused

>+    for(let mimeType of mimeTypes) {

please add a space after "for"

>       }
>+      else {

} else {
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/55f027c764be
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
I installed Foxit, Adobe, Nitro, Gaaiho and DocuCom in this order.
Using an old Nightly (25.08.2014) I see that the newly plugin installed is added to Action field (one by one): https://db.tt/TFoNjZNZ
Using latest Nightly (01.08.2014) I see that the Action field keeps the same plugin no matter how many plugins I install: https://db.tt/MFSJcOCa
Opening 'Use other' will display the plugins available for opening PDF in the same order. This is the behavior I see before and after the fix.

Not sure If I`m testing correctly here. Comment 0 is quite confusing to me in order to identify the actual fix that was handled here.
Flags: needinfo?(bernardo)
I installed Adobe, Foxit, PDF-XChange and Sumatra.

Before this patch landed, the applications pane would display Sumatra as the alternative to the built-in viewer, while after this patch landed, the applications pane now displays PDF X-Change as the alternative.

If however I select Sumatra/PDF-XChange, I actually get Adobe.

Note that I tried manually applying rev 2 and that displays Adobe.
Depends on: 1063424
Filed bug 1063424.
Flags: needinfo?(bernardo)
Seems like the only remaining issue (bug 1063424) is no longer reproducing for Neil and Bogdan, so closing this as WFM.
Resolution: FIXED → WORKSFORME
You need to log in before you can comment on or make changes to this bug.