454242 - The location of files used by helper app locations is no longer consistent with the download location

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect, P2)

Description

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

We used to be good and dumped all files that were to be opened with helper applications in the same place as the download location.  Then, the browser got fancy and allowed for more customizations of this, and exthandler is now out of date.  This is really from bug 311292, but I've come to the realization that this is not what those folks actually want.

Comment 1

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

Comment on attachment 337478 [details] [diff] [review]
v1.0

ui-yarrrr=beltzner

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 337478 [details] [diff] [review]
v1.0

Why is the pref called folderList when it's just an integer identifying a single directory?

+#define NS_PREF_BD_DIR        "browser.download.dir"

please put those defines near the top of the file (i.e. before any function definitions)

I think using DOWNLOAD instead of BD would make it clearer what the defines stand for.

+#define NS_FOLDER_VALUE_DESKTOP 0

can you make that an enum instead?

+static inline
+nsresult
+GetDownloadDirectory(nsIFile **_directory)

style of this file to have this all on a single line

also, why the underscore prefix on the argument?

+  (void)prefs->GetIntPref(NS_PREF_BD_FOLDERLIST, &folderValue);

I'm not a fan of the (void) casting. I'd prefer that you don't do it. (but at least you should be consistent within the file - the other two uses in here have a space after the cast)

You aren't even consistently using the (void) where you are ignoring the return value...

+        nsCOMPtr<nsIProperties> dirService =
+          do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID);
+        if (!dirService) break;
+
+        (void)dirService->Get(NS_OS_DESKTOP_DIR,
+                              NS_GET_IID(nsILocalFile),
+                              getter_AddRefs(dir));

Why not use NS_GetSpecialDirectory like in the other places in this function?

+  NS_ASSERTION(dir, "Somehow we didn't get a download directory!");

You shouldn't assert that here. NS_GetSpecialDirectory might have failed (non-OS X case). If you want to keep the assertion add an NS_ENSURE_SUCCESS for that.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

also, this function seems too complex to be marked inline. just make it static nsresult GetDownloadDirectory.

Comment 4

[Mike Connor [:mconnor]]

Comment on attachment 337478 [details] [diff] [review]
v1.0

r=me with biesi's comments addressed

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> (From update of attachment 337478 [details] [diff] [review])
> Why is the pref called folderList when it's just an integer identifying a
> single directory?
It was one of those bad decisions made long ago that we are stuck with :/

> also, why the underscore prefix on the argument?
It's an outparam, which we sometimes use an underscore, and sometimes don't...

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Addresses review comments

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

It turns out that doing a qrefresh before generating a diff is a good idea...

Comment 8

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/7ca065e506eb

Comment 9

[Boris Zbarsky [:bzbarsky]]

Wouldn't it make more sense to teach the standard file/directory provider about this stuff so that a simple get can be used both here and in the UI or something?  Or does that not work for some reason?

Comment 10

[Shawn Wilsher :sdwilsh]

It might - I'll admit to not even thinking about that approach.

Comment 11

[Stephen Donner [:stephend] Not actively reading bugmail]

What's the testcase for this?

I have |browser.download.folderList| set to 2, and have /Users/stephend/Documents set as "Save files to".

When I open a RealPlayer clip via its .RAM file on Mac OS X, I don't see it in either download manager or in my Documents folder.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Shawn, this bug has the in-limus flag set. So before I'm able to add this I have to make sure everything works. I've tested your patch with the latest trunk build and have problems with the custom download location. It's the same what Stephen already mentioned.

* The custom location which should be "browser.download.dir" isn't used. It always uses the systems download location which is set on my location to ~/Library/Cache/TemporaryItems.

* Further shouldn't we set NS_FOLDER_VALUE_DOWNLOADS as default on OS X 10.5? That will lower the frustration for a lot of users for those the temporary files are stored to desktop now.

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> When I open a RealPlayer clip via its .RAM file on Mac OS X, I don't see it in
> either download manager or in my Documents folder.
That must mean that some plugin is handling it.  If it doesn't show up in the download manager, we don't have any control over it.

(In reply to comment #12)
> Shawn, this bug has the in-limus flag set. So before I'm able to add this I
> have to make sure everything works. I've tested your patch with the latest
> trunk build and have problems with the custom download location. It's the same
> what Stephen already mentioned.
As in it doesn't even show up in the download manager?  That's not the behavior I get.

> * The custom location which should be "browser.download.dir" isn't used. It
> always uses the systems download location which is set on my location to
> ~/Library/Cache/TemporaryItems.
I don't think that setting a string pref there is right.  The code get's a complex preference, so you have to set it in the preferences UI.

> * Further shouldn't we set NS_FOLDER_VALUE_DOWNLOADS as default on OS X 10.5?
> That will lower the frustration for a lot of users for those the temporary
> files are stored to desktop now.
I had thought we did that globally for Firefox 3, but I can see clearly that it is not set that way.  I'm fairly certain I reviewed a patch of jimm's that did this.  We should probably file a new bug to get it set that way.

What's really weird is that I swear this was working when I landed it, as I did a bazillion local tests to make sure everything was working as expected :(

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #13)
> > When I open a RealPlayer clip via its .RAM file on Mac OS X, I don't see it in
> > either download manager or in my Documents folder.
> That must mean that some plugin is handling it.  If it doesn't show up in the
> download manager, we don't have any control over it.

I think, that Steven has moved his download folder too (changed the location within Safari?). 

> > * The custom location which should be "browser.download.dir" isn't used. It
> > always uses the systems download location which is set on my location to
> > ~/Library/Cache/TemporaryItems.
> I don't think that setting a string pref there is right.  The code get's a
> complex preference, so you have to set it in the preferences UI.

I've updated the path via preferences | download folder. So the correct value should have been set. But in any way it downloads the file to the above given directory, which is the system wide download folder (set via Safari).

> > * Further shouldn't we set NS_FOLDER_VALUE_DOWNLOADS as default on OS X 10.5?
>
> What's really weird is that I swear this was working when I landed it, as I did
> a bazillion local tests to make sure everything was working as expected :(

I'll file that bug and make it depends on that one.

Comment 15

[Shawn Wilsher :sdwilsh]

So it seems like this isn't fixed.  Can you do me a favor and give me exact steps on how you are doing this download so I can make sure I test exactly that please?

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #15)
> So it seems like this isn't fixed.  Can you do me a favor and give me exact
> steps on how you are doing this download so I can make sure I test exactly that
> please?

I've already started to file a new bug for that part. But ok, let's do it here. It's mostly simple to reproduce:

1. Create a fresh profile
2. Open about:config and set "browser.download.folderList" to '1'
3. Goto http://www.google.com/search?q=pdf and click any pdf
4. Check your system download folder the file is stored correctly
5. Goto preferences and select another download folder
6. Check "browser.download.folderList" set it is set to '2' now
7. Do step 3 again

With step 7 you will see that the systems download folder is used again, instead of the formerly selected user defined download folder.

Comment 17

[Jim Mathies [:jimm]]

(In reply to comment #16)
> (In reply to comment #15)
> > So it seems like this isn't fixed.  Can you do me a favor and give me exact
> > steps on how you are doing this download so I can make sure I test exactly that
> > please?
> 
> I've already started to file a new bug for that part. But ok, let's do it here.
> It's mostly simple to reproduce:
> 
> 1. Create a fresh profile
> 2. Open about:config and set "browser.download.folderList" to '1'
> 3. Goto http://www.google.com/search?q=pdf and click any pdf
> 4. Check your system download folder the file is stored correctly
> 5. Goto preferences and select another download folder
> 6. Check "browser.download.folderList" set it is set to '2' now
> 7. Do step 3 again
> 
> With step 7 you will see that the systems download folder is used again,
> instead of the formerly selected user defined download folder.

Isn't content that's displayed within the content window via a plugin going to end up in the cache? Maybe pdf is a bad example, can you reproduce this for other file types like zips? I checked on windows and for other files like zips, this isn't reproducible. For pdfs, the file is in the cache folder and is not displayed in the downloads window. That seems correct to me.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Isn't content that's displayed within the content window via a plugin going to
> end up in the cache?

Jim, on Mac there is no PDF plug-in installed by default, and PDFs are handled in an external app (typically Preview).

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Jim, this bug is for OS X only where the file handling is a bit different then on Windows. At least what the majority of users say. ;)

Comment 20

[David Ascher (:davida)]

adding status whiteboard indicating that this affects TB3 as well.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

sdwilsh: we would seem to have missed the 1.9.1beta1 target milestone, can you reset that field to something appropriate?

Comment 22

[Shawn Wilsher :sdwilsh]

I'll talk to drivers about this tomorrow to get it blocking (P2)

Comment 23

[Shawn Wilsher :sdwilsh]

Attachment: follow-up v1.0

So it's pretty clear that I didn't test this code path, but the fix is utterly trivial.  I just wish there was a way to make an automated test case for this :(

Comment 24

[Mike Connor [:mconnor]]

Comment on attachment 359077 [details] [diff] [review]
follow-up v1.0

Yeah, oy.

I hate that these are so similar, and yet so different.

Comment 25

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/8caadf42408c

Comment 26

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d805ea008384

Comment 27

[Marcia Knous [:marcia]]

verified fixed on the 1.9.1 branch using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090128 Shiretoko/3.1b3pre following the steps in Comment 16.

Comment 28

[Oakwine]

I find this new behavior worse than the previous (also bad) behavior.  Anything opened by a helper app is (for me) disposable.  Before this patch landed, I knew that anything left on my desktop could be thrown away.  Now, all of my temporarily opened PDFs are mixed in with the items I wanted to download and keep in my Downloads folder.

Comment 29

[Shawn Wilsher :sdwilsh]

(In reply to comment #28)
> I find this new behavior worse than the previous (also bad) behavior.  Anything
> opened by a helper app is (for me) disposable.  Before this patch landed, I
> knew that anything left on my desktop could be thrown away.  Now, all of my
> temporarily opened PDFs are mixed in with the items I wanted to download and
> keep in my Downloads folder.
You are looking for bug 311292.

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Shawn, thanks! That looks great. Verified too with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090129 Minefield/3.2a1pre

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Shawn, wouldn't it be possible to have an automated test here instead of a manually run litmus test? This bug has a higher priority and it would be important to catch a regression as soon as possible.

Comment 33

[Shawn Wilsher :sdwilsh]

(In reply to comment #31)
> Shawn, wouldn't it be possible to have an automated test here instead of a
> manually run litmus test? This bug has a higher priority and it would be
> important to catch a regression as soon as possible.
It's not possible since we'd have to run another application to verify it.

Comment 34

[Stephen Donner [:stephend] Not actively reading bugmail]

This isn't fixed for me on 10.4, with a new profile, using the STR in comment 16.

Build identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b3pre) Gecko/20090130 Shiretoko/3.1b3pre

Temp files from RealPlayer/Preview (PDFs) are still being thrown to the Desktop, rather than being stored in the "Downloads" folder, which is the default now on new profiles.

Comment 35

[Shawn Wilsher :sdwilsh]

Stephen - I'm betting you are seeing bug 476174, especially since it's on 10.4.  I'm assuming custom locations work correctly now?

Comment 36

[Stephen Donner [:stephend] Not actively reading bugmail]

Well, I changed Safari's download location to a custom folder, then created a new profile, and set Shiretoko to its own custom-download directory, rather than the "Downloads" folder.

I tried it first with browser.download.folderList set to 1, and I got temporary files saved to the desktop; then I tried browser.download.folderList set to 2, and the "Saves files to" textfield just goes blank, but temp files still go to the desktop.

Comment 37

[Shawn Wilsher :sdwilsh]

(In reply to comment #36)
> Well, I changed Safari's download location to a custom folder, then created a
> new profile, and set Shiretoko to its own custom-download directory, rather
> than the "Downloads" folder.
Which version of Safari?  Newer versions don't update the preference that we check.  You can use Camino to set the pref though.

> I tried it first with browser.download.folderList set to 1, and I got temporary
> files saved to the desktop; then I tried browser.download.folderList set to 2,
> and the "Saves files to" textfield just goes blank, but temp files still go to
> the desktop.
If you set it to two and didn't set a path in the UI, it's going to revert to the system download location, which I'd suspect is going to be the Desktop.

Comment 38

[Stephen Donner [:stephend] Not actively reading bugmail]

The tryserver build over in bug 476174 has fixed the issue I was seeing; reclosing this bug, thanks!

Comment 39

[Jesse Ruderman]

I think this change was a step in the wrong direction.  See bug 369462 comment 5.