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

Status: RESOLVED WORKSFORME

(Firefox :: Settings UI, defect, P3)

Description

[Brian Duddy]

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.

Comment 1

[Benjamin Smedberg]

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?

Comment 2

[Benjamin Smedberg]

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.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Jan will take a look at this.

Comment 4

[Jan Fuchsmann]

Attachment: Expose FindPluginForType in nsIPluginHost.idl

I've managed to expose nsPluginHost::FindPluginForType. This doesn't fix the bug yet, but is a prerequisite.

Comment 5

[Georg Fritzsche [:gfritzsche]]

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.

Comment 6

[Anup Kumar [:reznord]]

Hi, I am interested in working on this bug. So please can you assign this to me?

Thanks in Advance,

Regards,
A. Anup

Comment 7

[Georg Fritzsche [:gfritzsche]]

Let's look into bug 942461 first and this afterwards :)

Comment 8

[Georg Fritzsche [:gfritzsche]]

Turns out that that bug was already closed, so assigning here.

Comment 9

[Georg Fritzsche [:gfritzsche]]

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.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Anup currently doesn't have time.

Comment 11

[Bernardo Rittmeyer [:rittme]]

Attachment: rev1 - Plugin handlers are now loaded using pluginHost method getPluginTagForType

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.

Comment 12

[Georg Fritzsche [:gfritzsche]]

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 13

[Georg Fritzsche [:gfritzsche]]

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

Comment 14

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 2 - Applications pane plugin handlers are now loaded using pluginHost method getPluginTagForType

Thank you for you review, Georg!

Sorry, I forgot about the normal preferences.
This should work.

Comment 15

[Georg Fritzsche [:gfritzsche]]

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?

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 17

[Georg Fritzsche [:gfritzsche]]

Hi Bernardo, do you think you can pick this up and address the minor changes from comment 16?

Comment 18

[Bernardo Rittmeyer [:rittme]]

Hi Georg, sure, I'll pick this up and finish it on this next iteration.

Comment 19

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 3 - Applications pane plugin handlers are now loaded using enabledPlugin from navigator.mimeTypes.

Fixed the review changes.

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 21

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 4 - Applications pane plugin handlers are now loaded using enabledPlugin from navigator.mimeTypes. r=Unfocused

Thank you for the review, Unfocused.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Can we please run this through Try first? :)

Comment 23

[Bernardo Rittmeyer [:rittme]]

https://tbpl.mozilla.org/?tree=Try&rev=895843ff601b

Comment 24

[Dão Gottwald [:dao]]

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 {

Comment 25

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 5 - Applications pane plugin handlers are now loaded using enabledPlugin from navigator.mimeTypes. r=Unfocused

Thank you dao. 
Here is the fixed patch.

Comment 26

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/55f027c764be

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/55f027c764be

Comment 28

[Bogdan Maris, Desktop QA]

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.

Comment 29

[[e-mail]]

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.

Comment 30

[Georg Fritzsche [:gfritzsche]]

Filed bug 1063424.

Comment 31

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Seems like the only remaining issue (bug 1063424) is no longer reproducing for Neil and Bogdan, so closing this as WFM.