395713 - "Save to Disk" option not shown for entries in Applications prefpane

Status: VERIFIED FIXED

(Firefox :: File Handling, defect, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

The "Save to Disk" option is not getting shown in the action dropdown menu for entries in the Applications prefpane.  It should be shown for entries representing MIME types, including those handled by plugins.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: fixes bug

The problem is that we're checking if the wrapper instead of the wrapped object to see if it's an instanceof Ci.nsIMIMEInfo.  This patch fixes that.  Also, I noticed that nothing is using the wrapper's QueryInterface method, so I remove that method in this patch.

Ideally we'd make the wrapper's prototype be the wrapped object, inheriting all its non-overridden properties and methods, and somehow have instanceof work correctly when used with the wrapper object (and never have to have external callers access the wrapped object), but I don't think we can do that.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 280417 [details] [diff] [review]
patch v1: fixes bug

Of all regressions from the new Applications prefpane, this is the one most likely to be painful, so it'd be good to get it in for M8, if there's time to do so.

The fix is simple, well-understood, and low-risk.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Checking in browser/components/preferences/applications.js;
/cvsroot/mozilla/browser/components/preferences/applications.js,v  <--  applications.js
new revision: 1.7; previous revision: 1.6
done

Comment 4

[Steffen Wilberg]

Doesn't work with 2007091804 trunk nightly on Windows. For example, I can't set pdf documents to save, only choose between the plugin and the application.

Don't know if this is related, but I get this error a couple of dozen times when opening the Applications pane:
Error: [Exception... "'Component is not available' when calling method: [nsIHandlerService::fillHandlerInfo]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: chrome://browser/content/preferences/applications.js :: anonymous :: line 807"  data: no]
Source File: chrome://browser/content/preferences/applications.js
Line: 807
According to view source, that line is this:
            this._mimeSvc.getFromTypeAndExtension(type, null);

Comment 5

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #4)
> Doesn't work with 2007091804 trunk nightly on Windows. For example, I can't set
> pdf documents to save, only choose between the plugin and the application.

Hmm, indeed.  Strangely, I see the option to "Save to Disk" on Linux and Mac, but I don't see it on Windows.  Looking into the problem...

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: fixes additional case of problem

From applications.js:

    // Create a menu item for saving to disk.
    // Note: this option isn't available to protocol types, since we don't know
    // what it means to save a URL having a certain scheme to disk, nor is it
    // available to feeds, since the feed code doesn't implement the capability.
    // And it's not available to types handled only by plugins either, although
    // I would think we'd want to give users the ability to redirect that stuff
    // to disk (so maybe we should revisit that decision).

The last three lines of that comment explain why it isn't showing up, I think.  In this context "handled only by plugin" just means that Firefox doesn't have any configuration information about the type in its datastore of handlers.  It doesn't necessarily mean there's no other handler, since there could be an OS default handler (like the Adobe Reader desktop app).

I hid "save to disk" in these situations because it was hidden in the old Download Actions dialog, but it didn't make sense to me why that was the case, and it still doesn't.  It seems like we should remove this special case.

Here's a patch that does so.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: updates comment to reflect change

Hmm, I should update the comment now that I've changed the functionality.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Requesting blocking-firefox3 for this Applications prefpane regression fix.

Comment 9

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

Hrm, doesn't this make "handledOnlyByPlugin" a lie? Now these types can be set to "save to disk", and that means some of the other UI decisions we make might be wrong. What happens if I set a type that can be handled by a plugin to "save to disk" and then disable the plugin? Seems to me there'd be no way to modify that decision afterwards. 

Comment 10

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #9)
> Hrm, doesn't this make "handledOnlyByPlugin" a lie? Now these types can be set
> to "save to disk", and that means some of the other UI decisions we make might
> be wrong.

"handledOnlyByPlugin" may be a bit confusing.  It doesn't mean "there are no other options for handling this type," it just means "there is no user-configured handler for this type."

So it's not incorrect to provide additional options for handling such a type, including the OS default handler (if any), a "save to disk" option (with this bug fix), and an "always ask" option (with the fix for bug 395138).

Besides the functionality being changed in this bug, there are two places where handledOnlyByPlugin affects UI:

1. If "browser.download.show_plugins_in_list" is false, we don't show types handledOnlyByPlugin.
2. We don't show types handledOnlyByPlugin where the plugin is disabled.

We're removing the latter case in bug 395136, so that's not a concern.  And as for the former case, I think it's reasonable to continue hiding types handledOnlyByPlugin when the pref is false and revealing them when it's true, since that pref is designed to hide the large numbers of types that plugins register on Mac (and, I think, Windows) which aren't useful to configure, and we still want to do that instead of showing these types now that we have an extra option for handling them.


> What happens if I set a type that can be handled by a plugin to "save
> to disk" and then disable the plugin? Seems to me there'd be no way to modify
> that decision afterwards. 

Setting the type to "save to disk" automatically disables the plugin for that type.  But it also creates a user configuration record for it, so even without the fix for bug 395136, it will thenceforth appear in the list, and you will be able to modify the decision by setting the type to "always ask" (once bug 395138 is fixed), the OS default handler (if any), or the plugin (which will reenable it).

Comment 11

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

Comment on attachment 281585 [details] [diff] [review]
patch v3: updates comment to reflect change

OK, thanks for the explanation.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: unrotted

Resolves trivial conflicts.  This is the version of the patch I'll check in.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 280417 [details] [diff] [review]
patch v1: fixes bug

Err, this patch is actually still relevant.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Checking in browser/components/preferences/applications.js;
/cvsroot/mozilla/browser/components/preferences/applications.js,v  <--  applications.js
new revision: 1.18; previous revision: 1.17
done

Comment 15

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b3pre) Gecko/2008010805 Minefield/3.0b3pre ID:2008010805 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre)Gecko/2008010804 Minefield/3.0b3pre

note: its now called safe file because of bug 395731