308073 - Change default downloading folder in Windows Vista from Desktop to Downloads

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Jeremy Visser]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

In Windows Vista (Beta 1), there is a dedicated Downloads folder in the
%USERPROFILE% folder. It would probably be a good idea to change the default
from Desktop to Downloads. I'm not sure if there is an environment variable for
Downloads...if there is, it would probably be a good idea to use it.

Reproducible: Always

Steps to Reproduce:
1. Go to any website
2. Download something
3. Watch as it downloads onto the desktop.
4. Wish it would download into the Downloads folder.

Actual Results:  
It downloaded to the Desktop.

Expected Results:  
Download into the Downloads folder.

Comment 1

[Mike Connor [:mconnor]]

The problem we're trying to solve in choosing the Desktop is the fact that we auto-download by default and this makes sure people (at least eventually) find their files without having to think about it, much.

Comment 2

[Mike Connor [:mconnor]]

Not going to worry about Vista-specific issues now, bigger fish to fry for one.

Comment 3

[hammy]

In all fairness IE7 doesn't even download there now. I'm not sure why that is though. I assume downloads from IE would go there but perhaps I misunderstand its intent.

Comment 4

[Isriya Paireepairit]

I agree with the reporter. Firefox on Vista should be Vista-ish as much as possible. Since IE7 is default browser on Vista, so users who install Firefox as well will expect the same download destination folder.

Comment 5

[Doug Turner (:dougt)]

In RC1, other apps are continuing to default to desktop.  

Comment 6

[Robert Strong (they/them - no direct email)]

I checked with a clean install of RC1 and IE defaulted to the downloads folder for me.

Comment 7

[Doug Turner (:dougt)]

right, on a clean install i see the same thing.


we need to:

1) add a new location to the directory service in xpcom that corresponds to the downloads location.  I am not sure of the CSLID that needs to be used.

2) fix up this code:

http://lxr.mozilla.org/mozilla/source/mail/components/preferences/downloads.js#114

3) add code in startup to toggle the browser.download.folderList pref to 1 at start if running Vista.

The js to check for vista is something like:

    function isVistaOrGreater()
    {
      var si = Components.classes["@mozilla.org/system-info;1"].getService(Components.interfaces.nsIPropertyBag2);
      var version = parseInt(si.getProperty("version"));  // only care about the major version.
      var os = si.getProperty("name").toLowerCase();

      if (os.indexOf("win") != -1 && version >= 5)  // vista or above!
        return true;
      return false;
    }

4) change up any other places that might look in the "Pers" key.

Comment 8

[Doug Turner (:dougt)]

Attachment: XPCOM Patch

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 242785 [details] [diff] [review]
XPCOM Patch

we can free with |    CoTaskMemFree(path);|

I made this change locally.

Comment 10

[timeless]

Comment on attachment 242785 [details] [diff] [review]
XPCOM Patch

+    gGetKnownFolderPath(*guid, 0, NULL, &path);

this can technically fail.

don't you want to allow access to -1 too? :)

+    // Not sure how to free |path|.  I have sent an email to Microsoft;

normally it's the shell one which is the same as another.

I don't appreciate holding review blame for a leak.

Comment 11

[Doug Turner (:dougt)]

Checking in io/SpecialSystemDirectory.cpp;
/cvsroot/mozilla/xpcom/io/SpecialSystemDirectory.cpp,v  <--  SpecialSystemDirectory.cpp
new revision: 1.27; previous revision: 1.26
done
Checking in io/SpecialSystemDirectory.h;
/cvsroot/mozilla/xpcom/io/SpecialSystemDirectory.h,v  <--  SpecialSystemDirectory.h
new revision: 1.8; previous revision: 1.7
done
Checking in io/nsDirectoryService.cpp;
/cvsroot/mozilla/xpcom/io/nsDirectoryService.cpp,v  <--  nsDirectoryService.cpp
new revision: 1.90; previous revision: 1.89
done
Checking in io/nsDirectoryService.h;
/cvsroot/mozilla/xpcom/io/nsDirectoryService.h,v  <--  nsDirectoryService.h
new revision: 1.34; previous revision: 1.33
done
Checking in io/nsDirectoryServiceDefs.h;
/cvsroot/mozilla/xpcom/io/nsDirectoryServiceDefs.h,v  <--  nsDirectoryServiceDefs.h
new revision: 1.30; previous revision: 1.29
done

Microsoft got back to me and you free with CoTaskMemFree(path);

Comment 12

[Daniel Veditz [:dveditz]]

This is part of the Vista stuff we want in 1.8.1.2, right?

Comment 13

[Daniel Veditz [:dveditz]]

(In reply to comment #11)
> Microsoft got back to me and you free with CoTaskMemFree(path);

Can we get a branch patch that reflects this change?

Comment 14

[Doug Turner (:dougt)]

The xpcom patch is the first thing to do in order to get the correct path value.  There is a bunch of work to keep this up for all platforms (like OSX who also shares the same problem).  This work was partially done in 358039.

I am not sure if this needs to be blocking any FF2 release.

Comment 15

[Jay Patel [:jay]]

Removing blocking flag, dougt says this isn't gonna happen in time for the next release.  Leaving wanted+, so hopefully we can deal with this and bug 358039 sometime in the near future.

Comment 16

[Doug Turner (:dougt)]

1) Bug 245467 needs to get reviewed, then land.

2) The XPCOM patch needs to also land.

3) Then we should change getDownloadsFolder() in the patch for 245467 so that it picks the download directory, instead of the Desktop for all platforms.

4) We should also identify any other places were we download files and change them also.

Comment 17

[Shawn Wilsher :sdwilsh]

Doug - is that patch ready to commit since Bug 245467 is done?

Comment 18

[Jim Mathies [:jimm]]

The one anomaly here is that right-click, save as on an embedded image will land in Pictures, not Downloads. I'll see if I can fit that in too.

Comment 19

[Shawn Wilsher :sdwilsh]

I think Rob Arnold has been playing with this as well, so you might want to talk to him ;)

Comment 20

[Jim Mathies [:jimm]]

Comment on attachment 242785 [details] [diff] [review]
XPCOM Patch

This patch is obsolete - the functionality already landed at some point, maybe in another bug.

Comment 21

[Jim Mathies [:jimm]]

note to self - read the comments. Doug checked it in here 2006-10-24. :/

Comment 22

[Jim Mathies [:jimm]]

Side note - while working on this, came across something funny in 
/browser/components/preferences/main.js#330:

dir.append("My Downloads"); // XXX l12y!

Not sure if that's the way we're supposed to do that :/ but I thought I'd make a note of it so it doesn't get missed.

Comment 23

[Jim Mathies [:jimm]]

Attachment: vista downloads patch v.1

first rev, also fixed the string I mentioned previously.

Comment 24

[Jim Mathies [:jimm]]

Attachment: vista downloads patch v.2

Some cleanup, improvements, and additional testing on xp.

Comment 25

[Rob Arnold [:robarnold]]

Jim, there are a few things in your patch that I don't really like. I think we disagree on what the semantics of the pref ought to be. Under vista, folderList=1 => downloads go to Downloads. Under XP/2K, I think we should go to the desktop. I strongly dislike creating the 'My Downloads' folder for the user; there is no way to disable that behavior and if the user really wants a 'My Downloads' folder, they can make it themselves. I also do not like the fact that we add a pref solely for the purpose of migrating the download folder from the desktop to the downloads folder; I think it would be better to change the default value of the pref on windows to the downloads folder. This way, existing vista users won't have all their downloads move when they upgrade, but new installs will point to the 'right' spot.

From a purely code style point of view, it seems that your changes are the same set of changes made in three different places. It would be far better to consolidate all that copy-and-pasted code into one location, preferably the download manager. This would make the code far easier to maintain. OS 10.5 has a separate download folder, so we should make this easy whoever has to make that change.

Comment 26

[Shawn Wilsher :sdwilsh]

An nsI[Local]File attribute on the download manager that points to the default download directory would be a nice abstraction here...

Comment 27

[Jim Mathies [:jimm]]

>Jim, there are a few things in your patch that I don't really like. I think we
>disagree on what the semantics of the pref ought to be. Under vista,
>folderList=1 => downloads go to Downloads. Under XP/2K, I think we should go to
>the desktop. 

This is the way it works, the default is to go to the Desktop through the default pref on folderList of 0.
 
>I strongly dislike creating the 'My Downloads' folder for the
>user; there is no way to disable that behavior and if the user really wants a
>'My Downloads' folder, they can make it themselves. 

I agree, however, this wasn't my design, it's been there (at least) since 2.0. It's never been exposed through a UI but setting folderList to 1 on XP in 2.0 and trunk will kick this in. I actually fixed a bug related to this in prefs while working on it where canceling the browse-for-folder dialog blew away the user's setting for it. (There is still a minor issue where if the "My Downloads" folder is not present and you browse-for-folder and folderList is set to 1 it will not create the folder.) But I don't think anybody uses the option on XP, so it really makes no difference. If you're curious, try a fresh install of 2.0 on XP, set folderList to 1 and open prefs, you'll see a nonexistent "My Downloads" folder selected. Open download manager, drop something into it and "My Downloads" will be created. Open prefs back up and you'll have it. Note, there are only two folders supported with custom labels, Desktop, and "My Downloads", everything else will display the full path. I've added one more, "Downloads" that only applies to Vista. Overall I was simply trying to leverage something that was already there to do something new on Vista. 

>I also do not like the fact
>that we add a pref solely for the purpose of migrating the download folder from
>the desktop to the downloads folder; I think it would be better to change the
>default value of the pref on windows to the downloads folder. 

Ok, but the bug request was to set the default on Vista to Downloads, and leave the default on XP to Desktop. If you want me to change XP I can, that would simplify things a bit although I'm not sure XP users will like it.

On the one-off pref, I agree. However, I wasn't aware of any way to do something like this once and never do it again without it. I checked the prefs a found at least one other migration pref like it, which is why I went this route.

>This way,
>existing vista users won't have all their downloads move when they upgrade, but
>new installs will point to the 'right' spot.

You know I looked for a way to set the default folderList pref to 1 on Vista on fresh installs and 0 on XP, but didn't find a way to do it. Any ideas? Even with that, the one-off switch will still be needed for existing profiles. 

>From a purely code style point of view, it seems that your changes are the same
>set of changes made in three different places. It would be far better to
>consolidate all that copy-and-pasted code into one location, preferably the
>download manager. 

Considered that, although I'm not sure how I can access that from main.js in prefs, or the unknown content dialog? A possible solution would be to use contentAreaUtils.js, although I'm a little hesitant to source something like that into prefs this late in the ball game. Suggestions welcome. :) Note this code was already distributed in, by my count, three places prior to my working on it. (Not that that's an excuse not to clean it up if people think changes like that are ok at this point.)

Comment 28

[Jim Mathies [:jimm]]

>An nsI[Local]File attribute on the download manager that points to the default
>download directory would be a nice abstraction here...

Ahh, through @mozilla.org/download-manager;1 and create an instance of that. I'm always a a little leery of modifying p;ublic interfaces, maybe I should get over that.

Comment 29

[Jim Mathies [:jimm]]

I think the reason we have My Downloads is due to OSX compatibility which I believe has always had a real downloads folder. When we added support for that, we probably added something to XP to make the code seamless. So now we have Vista finally with a real download folder. The problem of course is that if there are users on XP using MD, and we do away with it, we totally hose their downloads up.

So overall I'm curious what people think - 

Change the default pref on Windows from Desktop to Downloads. 

- New XP users would go to My Downloads in the user's personal folders
- Existing XP users would be left alone
- New Vista users would go to Downloads
- Existing Vista users would remain where ever they are, probably Desktop. 

vs. 

Leave the default to Desktop for Windows, and keep the one-off for flipping Vista users programmatically to Downloads. 

- New XP users would go to Desktop
- Existing XP users would be left alone
- New Vista users would go to Downloads 
- Existing Vista users would have their prefs changed from Desktop to Downloads. 

I don’t like the first because of new XP users, but we could one-off that in the code similar to the second approach for Vista.

Comment 30

[Shawn Wilsher :sdwilsh]

(In reply to comment #28)
> Ahh, through @mozilla.org/download-manager;1 and create an instance of that.
> I'm always a a little leery of modifying p;ublic interfaces, maybe I should get
> over that.
s/create an instance/get the service/

It's a non-frozen interface, and I've already changed it a lot from 1.8.1 to 1.9.  Also, adding things isn't a big deal generally.

Comment 31

[Doug Turner (:dougt)]

I am in favor of leaving the default to Desktop for Windows and make Vista default to Downloads.  However, to some degree I am happy with keeping the default as Desktop to be consistent cross platform.  mconnor, thoughts?

Comment 32

[Rob Arnold [:robarnold]]

Jim, I was thinking that we just switch the special folder name for all Windows versions from "Pers" to "DfltDwnld" and change the implementation of GetSpecialSystemDirectory in xpcom/io/SpecialSystemDirectory.cpp to fall back to the desktop if the call to get the download folder fails. This should fix some of the issues you came up with. I.e:

 case Win_Downloads:
        {
            // Defined in KnownFolders.h.
            GUID folderid_downloads = {0x374de290, 0x123f, 0x4565, {0x91, 0x64, 0x39, 0xc4, 0x92, 0x5e, 0x46, 0x7b}};
            nsresult rv = GetKnownFolder(&folderid_downloads, aFile);
            // On WinXP and 2k, there is no downloads folder
            if(NS_ERROR_FAILURE == rv)
            {
              rv = GetWindowsFolder(CSIDL_DESKTOP, aFile);
            }
            return rv;
        }

And I think we should drop the idea of a "My Downloads" folder. The functionality was never exposed (in the gui) in fx2, so I doubt that there are many people who have such a thing. As far as I know, OSX is getting a Downloads folder in 10.5; it has not had one before (and of course Linux does not have one).

As for the migration pref: I think that on a fresh install, browser.download.lastDir is empty; that can be our indicator. Set it to non-empty after we've done the migration.

Comment 33

[Jim Mathies [:jimm]]

>I was thinking that we just switch the special folder name for all Windows
>versions from "Pers" to "DfltDwnld"...
..
>And I think we should drop the idea of a "My Downloads" folder.

Oh yeah I like that a lot. Will do.

>browser.download.lastDir is empty; that can be our indicator.

Hmm, in testing I don't remember seeing that, I do remember:

* browser.download.dir
*   the last directory to which a download was saved

I'll look at it. Although, there's nothing preventing this from getting cleared out mid-stream after an install.  There are a lot of pages out there that relate to busted downloads that tell users to muck with these settings. I'll test and see what happens. I grep what your suggesting though as an alternative to the migration flag.

Comment 34

[Jim Mathies [:jimm]]

> As far as I know, OSX is getting a Downloads
> folder in 10.5; it has not had one before (and 
> of course Linux does not have one).

Which now begs the question, why the heck did we add the My Downloads folder in the first place?? :/

Comment 35

[Jim Mathies [:jimm]]

Of interest:

"I'm not sure what happened here, but I just tried to do a new install on a new
machine and the first thing I do is set my download location to my downloads. 
This is something I demonstrate in my classes.  People love this...."

bug 306912

Not that I think this changes anything but people are using this option.

Comment 36

[Jim Mathies [:jimm]]

Attachment: vista downloads patch v.3

Notes:

Going into this there were two prefs used in storing the user specified download folder path: browser.download.dir & browser.download.downloadDir. While I'm sure everybody prefers browser.download.downloadDir because of the name, I believe it was recently introduced with the 2.0 prefs ui overhaul, and is not widely used. There's also a lot of confusion as to what it does on the net. browser.download.dir on the other hand has been around for a while, is well documented on the net, and is used heavily. So I've standardized on it and depreciated browser.download.downloadDir. With this patch firefox shouldn't be using it for anything. Note though Thunderbird and Minimo still use it in a few of places.

http://lxr.mozilla.org/mozilla/search?string=downloadDir

I've also updated the comments in prefs main.js making it clear (I hope) what all of these download related prefs do. It's actually not that bad once you get rid of downloadDir or dir.

The My Downloads folder functionality and special label are now depreciated, I've removed all references to it and removed the strings. On XP, we still needed a download folder for folderList = 2, this now points to My Documents. OSX 10.4 apparently has a downloads folder although it must be configured by the user. Directory service was already handling looking for it and falling back on desktop if it didn't exist. Note though, prior to this patch OSX defaulted to user docs/my downloads, that's now changed and I'm using the ds downloads folder.
 
I've tested this pretty heavily on vista, today I'll test heavily on XP. It'd be nice if we could find someone to run it through its paces on OSX as well.

Some Mozilazine articles that are related and could probably stand an update if this gets approved:

http://kb.mozillazine.org/About:config_Entries
http://kb.mozillazine.org/Unable_to_save_or_download_files

Comments welcome.

Comment 37

[Shawn Wilsher :sdwilsh]

Comment on attachment 275147 [details] [diff] [review]
vista downloads patch v.3

>+  /**
>+   * Returns the platform default downloads directory
>+   */
>+  readonly attribute nsILocalFile defaultDownloadsDirectory;
So I was thinking more like the actual downloads directory - the default one isn't so useful.

>+  return S_OK;
NS_OK ;)

Comment 38

[Jim Mathies [:jimm]]

Well, taking up to a higher level presents some problems in places and removes code in others. In prefs, if defaultDownloadsDirectory returns "a directory", you're going to have to figure out what it is and if it's valid to set the label. In downloads.js, a higher level call that returns the user's downloads directory would lessen the amount of code.
 
I'm open to either, although think this is a nice abstraction in that it hides all the ugliness of the ns tokens, while not going so high up that the script has to jump through hoops to figure out what was handed back.

Comment 39

[Jim Mathies [:jimm]]

How about two? defaultDownloadsDirectory & usersDownloadsDirectory?

Comment 40

[Rob Arnold [:robarnold]]

I don't like having downloads go to the "My Documents"; what's wrong with the desktop?

Comment 41

[Jim Mathies [:jimm]]

well, on XP and OSX -

folderList = 0 -> Desktop
folderList = 1 -> ??

That's the best directory I could come up with, and it fits with what we've been doing in the past minus the My Downloads sub dir.

Comment 42

[Jim Mathies [:jimm]]

Attachment: vista downloads patch v.4

Comment 43

[Jim Mathies [:jimm]]

Just came across yet another "default download folder" - 

browser. download. defaultFolder

Doesn't look like it's in much use either. I'll do some more research and try to remove it. 

http://lxr.mozilla.org/mozilla/search?string=defaultFolder

Comment 44

[Jim Mathies [:jimm]]

Attachment: vista downloads patch v.5

purged browser.download.defaultFolder.

Comment 45

[Jim Mathies [:jimm]]

In that last patch, I believe this is a memory leak within GetUserDownloadsDirectory:

nsCOMPtr<nsILocalFile> aFile; 
rv = GetDefaultDownloadsDirectory(getter_AddRefs(aFile)); 
NS_ENSURE_SUCCESS(rv, rv); 
NS_ADDREF(*aResult = aFile); 
return NS_OK; 

Since GetDefaultDownloadsDirectory also calls NS_ADDREF on aFile? I'll debug it tomorrow - I was just going through things and noticed this.

Comment 46

[Shawn Wilsher :sdwilsh]

(In reply to comment #45)
> Since GetDefaultDownloadsDirectory also calls NS_ADDREF on aFile? I'll debug it
> tomorrow - I was just going through things and noticed this.
Probably not a leak - that's what nsCOMPtr's take care of.  There is, however, probably a better way to do that (I haven't looked at the code yet to be sure).

Comment 47

[Jim Mathies [:jimm]]

Attachment: vista downloads patch v.6

Notes:

- removed the double ref ups in download manager
- after testing on xp, removed support for mapping the Downloads label to My Documents. (It was confusing.) XP/2k now only support the Desktop label.
- improved commenting.

Comment 48

[Doug Turner (:dougt)]

Comment on attachment 275276 [details] [diff] [review]
vista downloads patch v.6

i am not sure we want do default Win_Downloads to "My Documents" on XP.  I think the layers above XPCOM should deal with that.  Personally, i think the DESKTOP is a better fallback, but again outside of xpcom.

I am going to defer to rob on the rest of this.  I don't have much time to look at it right now, and I think we should really try to get this in ASAP.

Comment 49

[Jim Mathies [:jimm]]

If folks want to swap CSIDL_PERSONAL for CSIDL_DESKTOP that's fine by me.

Comment 50

[Jim Mathies [:jimm]]

Attachment: vista downloads patch v.7

I went ahead and made that switch, and updated prefs to support it. 

Changes:
- swapped personal folder for desktop on XP/2k and updated prefs to support it
- fixed a minor issue with the download folder icon in prefs with a custom download folder path.
- touched up download manager, remove some unneeded code.
- added a bit more commenting in contentareautils.js.

Comment 51

[Mike Connor [:mconnor]]

So, I remember commenting about this somewhere, but clearly not in this bug...

For XP/2k, where there is no downloads folder, I think we want to move away from filling the desktop up with downloads.  there's some security UI issues around automatically putting things like .exes on the desktop, i.e. putting something on the desktop with the Firefox icon might lead users to click on it and install malware, etc.  Same goes for putting stuff in $HOME on *nix, really...

We'd need to localize this, clearly, but I'm thinking in the fallback case we'd create a Firefox Downloads folder on the Desktop, in the default case.  This would create a little bit of UI weirdness in the prefpane if we haven't created the folder yet, but we can work around that.  This would avoid the oft-quoted clutter issue, and the concern around social engineering a malware install...

Thoughts?

Comment 52

[Jim Mathies [:jimm]]

Currently:

OSX:
0 -> Desktop (default)
1 -> Safari configured download folder or Desktop if this does not exist
Vista:
0 -> Desktop 
1 -> Downloads (default)
XP/2K:
0 -> Desktop (default)
1 -> Desktop
Linux:
0 -> Home (default)
1 -> Home

Through the new attributes in download manager, we can pretty much move these anyplace we want and handle creating folders on the fly as needed through the same code.

I think there are user's out there who want stuff to land on their desktop, and if they like that, they should have the ability to easily set it. Seems to me what's more important is the default folder you choose on new profiles. I don't think these should be set to Desktop, but I have no problem letting the user choose that option if they prefer it. 

For reference, FF2.0 - 

OSX:
0 -> Desktop (default)
1 -> download folder|desktop/My Downloads
Vista:
0 -> Desktop (default)
1 -> Documents/My Downloads
XP/2K:
0 -> Desktop (default)
1 -> My Documents/My Downloads
Linux:
0 -> Home (default)
1 -> Home/My Downloads

(however, we weren't doing a good job of creating the My Downloads folders.)

Comment 53

[Rob Arnold [:robarnold]]

Comment on attachment 275361 [details] [diff] [review]
vista downloads patch v.7

>+   * browser.download.showWhenStarting - bool
>+   *   True if the Download Manager should be opened when a download is
>+   *   started, false if it shouldn't be opened.
>+   * browser.download.closeWhenDone - bool
>+   *   True if the Download Manager should be closed when all downloads
>+   *   complete, false if it should be left open.
>+   * browser.download.useDownloadDir - bool
>+   *   True if downloads are saved with no save-as UI shown, false if 
>+   *   the user should always be asked where to save a file.
>+   * browser.download.dir - str path
>+   *   A local path the user may have selected for downloaded files to be 
>+   *   saved. Migration of other browser settings may also set this path.
>+   *   This path is enabled when folderList is equals 2.
>+   * browser.download.lastDir - str path
>+   *   May contain the last folder path accessed when the user browsed 
>+   *   via the file save-as dialog. (see contentAreaUtils.js)
>+   * browser.download.folderList - int
>+   *   Indicates the location users wish to save downloaded files too. 
>+   *   It is also used to display special file labels when the default 
>+   *   download location is either the Desktop or the Downloads folder.
>+   *   Values:
>+   *     0 - The desktop is the default download location.
>+   *     1 - The system's downloads folder is the default download location.
>+   *     2 - The default download location is elsewhere as specified in 
>+   *         browser.download.dir.

I like the type/semantic annotations.

>+    var folderListPref = document.getElementById("browser.download.folderList");
>+    var currentDirPref = this._indexToFolder(folderListPref.value); // file
>+    var defDownloads = this._indexToFolder(1); // file
>+
>+    // First try to open what's currently configured
>+    if (currentDirPref && currentDirPref.exists()) {
>+      fp.displayDirectory = currentDirPref;
>+    } // Try the system's download dir
>+    else if (defDownloads && defDownloads.exists()) {
>+      fp.displayDirectory = defDownloads;
>+    } // Fall back to Desktop
>+    else {
>+      fp.displayDirectory = this._indexToFolder(0);
>+    }
>+

This code is now much clearer. Finally, some comments :)

>+#ifdef XP_WIN
>+    var supportDownloadLabel = false;
>+    try {
>+      var sysInfo = Components.classes["@mozilla.org/system-info;1"].
>+                    getService(Components.interfaces.nsIPropertyBag2);
>+      supportDownloadLabel = // XP = 5.1, Vista = 6
>+        parseFloat(sysInfo.getProperty("version")) >= 6 ? true:false;
>+    } catch (ex) {
>+    }
>+#else
>+    var supportDownloadLabel = true;
>+#endif

Do we really want to support it on all other platforms unconditionally? Should we check on OSX if there is a user configured download directory?
>+    try {
>+      var urlspec = fph.getURLSpecFromFile(currentDirPref.value);
>+      downloadFolder.image = "moz-icon://" + urlspec + "?size=16";
>+    } catch (ex) {
>+    }

This can throw? I thought currentDirPref always pointed to a valid current 

>   _getDownloadsFolder: function (aFolder)directory.

Can this be rolled up into _indexToFolder?
>   writeFolderList: function ()

This function has such a misleading name. Can we change that, or would that be outside the scope of this patch?

>+  /*
>+   * Preferences:
>+   * 
>+   * browser.download.showWhenStarting - bool
>+   *   True if the Download Manager should be opened when a download is
>+   *   started, false if it shouldn't be opened.
>+   * browser.download.closeWhenDone - bool
>+   *   True if the Download Manager should be closed when all downloads
>+   *   complete, false if it should be left open.
>+   * browser.download.useDownloadDir - bool
>+   *   True if downloads are saved with no save-as UI shown, false if
>+   *   the user should always be asked where to save a file.
>+   * browser.download.dir - str path
>+   *   A local path the user may have selected for downloaded files to be
>+   *   saved. Migration of other browser settings may also set this path.
>+   *   This path is enabled when folderList is equals 2.
>+   * browser.download.lastDir - str path
>+   *   May contain the last folder path accessed when the user browsed
>+   *   via the file save-as dialog.
>+   * browser.download.folderList - int
>+   *   Indicates the location users wish to save downloaded files too.
>+   *   It is also used to display special file labels when the default
>+   *   download location is either the Desktop or the Downloads folder.
>+   *   Values:
>+   *     0 - The desktop is the default download location.
>+   *     1 - The system's downloads folder is the default download location.
>+   *     2 - The default download location is elsewhere as specified in
>+   *         browser.download.dir.
>+   * browser.download.downloadDir
>+   *   depreciated.
>+   * browser.download.defaultFolder
>+   *   depreciated.
>+   */

I dislike having such a big comment in two places; how about just a reference to preferences.js?

>+      // Get the default download directory from download manager
>+      var dnldMgr = Components.classes["@mozilla.org/download-manager;1"]
>+                              .getService(Components.interfaces.nsIDownloadManager);
>+      try {
>+        var startDir = dnldMgr.defaultDownloadsDirectory;
>+        if (startDir.exists()) {
>+          picker.displayDirectory = startDir;
>         }
>+      } 
>+      catch(exception) { }

Why the exception handling and why the check if the directory exists? Can we make/assume as an invariant that the download directory from the download manager always exists?

I like Mike Connor's suggestion of creating a Firefox Downloads (I dislike the 'My ...' convention) on the desktop as a fallback on XP/2k. I think that some users will still want to have their files download to the desktop.

Comment 54

[Jim Mathies [:jimm]]

> Do we really want to support it on all other platforms 
> unconditionally? Should we check on OSX if there is a 
> user configured download directory?

Sure, it'll require a comparison of the returned userDownloadFolder and a value retreived from directory service. A lot of work but I don't see that as an issue.

> This can throw? I thought currentDirPref always pointed to a valid current 

Not sure what would happen with the abstraction in xul. if the user miss configured browser.download.dir by setting it to say, 'false'? I'll test and see.

> Can this be rolled up into _indexToFolder?
>   writeFolderList: function ()
> This function has such a misleading name. Can we change that

will do.

> I dislike having such a big comment in two places; how about 
> just a reference to preferences.js?

good point, will do.

> Why the exception handling and why the check if the directory exists?

Currently the download manager code doesn't check in to make sure the path exists. There's a reason for that, it's meant to reflect what the user has configured, which may be an invalid path. I'll have to look at exists() to see if it actually throws, probably not. This I think was a left over from editing older code.

Comment 55

[Jim Mathies [:jimm]]

> > Why the exception handling and why the check if the directory exists?
> Currently the download manager code doesn't check 

Actually, not true. I do check. I think we can assume all paths returned are valid. The fallback stuff for both OSX and XP in dir service makes sure of that. I'll update it.

Comment 56

[Jim Mathies [:jimm]]

Attachment: vista downloads patch v.8

Rob, pretty much every you seggested is in here except the _getDownloadsFolder thing which was being used in two places, so I left it alone.

Comment 57

[Rob Arnold [:robarnold]]

Comment on attachment 275638 [details] [diff] [review]
vista downloads patch v.8

Looks good!

Comment 58

[Shawn Wilsher :sdwilsh]

Comment on attachment 275638 [details] [diff] [review]
vista downloads patch v.8

toolkit nor browser need sr, but you do need r from a peer, which mconnor is

Comment 59

[Jim Mathies [:jimm]]

Ugh - 

> With 3.0, XP's default download directory points to My Documents,...

I need a comment update. :/ I'll repost tomorrow.

Comment 60

[Robert Strong (they/them - no direct email)]

Comment on attachment 275638 [details] [diff] [review]
vista downloads patch v.8

r=me with the comment updated.

Comment 61

[Jim Mathies [:jimm]]

Attachment: vista downloads patch v.9

commenting fixed

Comment 62

[Jim Mathies [:jimm]]

Comment on attachment 275805 [details] [diff] [review]
vista downloads patch v.9

Hey Doug, can you give me a quick sr on the directory services stuff?

Comment 63

[Doug Turner (:dougt)]

Comment on attachment 275805 [details] [diff] [review]
vista downloads patch v.9

this is fine (the change under xpcom/).

Comment 64

[Mike Connor [:mconnor]]

Comment on attachment 275805 [details] [diff] [review]
vista downloads patch v.9

mostly front end, gecko piece has the right reviews, looks safe, a=mconnor on behalf of drivers

Comment 65

[Shawn Wilsher :sdwilsh]

Jim, if you could unbitrot this, I can go ahead and land it for you.

Comment 66

[Jim Mathies [:jimm]]

Attachment: vista downloads patch v.10

No major changes made - just had to remove the default download folder code in downloads.js, which wasn't required anymore.

Comment 67

[Shawn Wilsher :sdwilsh]

We should look into getting some automated tests on this with xpcshell and/or browser tests.  Jim, you up for doing that?

Checking in browser/base/content/browser.js;
new revision: 1.824; previous revision: 1.823
Checking in browser/components/migration/src/nsOperaProfileMigrator.cpp;
new revision: 1.69; previous revision: 1.68
Checking in browser/components/migration/src/nsSafariProfileMigrator.cpp;
new revision: 1.44; previous revision: 1.43
Checking in browser/components/preferences/main.js;
new revision: 1.10; previous revision: 1.9
Checking in browser/components/preferences/main.xul;
new revision: 1.12; previous revision: 1.11
Checking in browser/locales/en-US/chrome/browser/preferences/preferences.properties;
new revision: 1.13; previous revision: 1.12
Checking in toolkit/components/downloads/public/nsIDownloadManager.idl;
new revision: 1.17; previous revision: 1.16
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
new revision: 1.104; previous revision: 1.103
Checking in toolkit/content/contentAreaUtils.js;
new revision: 1.93; previous revision: 1.92
Checking in toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties;
new revision: 1.9; previous revision: 1.8
Checking in toolkit/locales/en-US/chrome/mozapps/downloads/unknownContentType.properties;
new revision: 1.7; previous revision: 1.6
Checking in toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in;
new revision: 1.50; previous revision: 1.49
Checking in xpcom/io/SpecialSystemDirectory.cpp;
new revision: 1.31; previous revision: 1.30

Comment 68

[Jim Mathies [:jimm]]

> Jim, you up for doing that?

Not sure how much time I'll have for it in the next week or so, but I'll try to get something together.

Comment 69

[Jim Jeffery not reading bug-mail 1/2/11]

This caused https://bugzilla.mozilla.org/show_bug.cgi?id=393247 , last download directory no longer respected in Vista systems.

Comment 70

[[e-mail]]

Comment on attachment 277418 [details] [diff] [review]
vista downloads patch v.10

>+        case Win_Downloads:
>+        {
>+            // Defined in KnownFolders.h.
>+            GUID folderid_downloads = {0x374de290, 0x123f, 0x4565, {0x91, 0x64,
>+                                       0x39, 0xc4, 0x92, 0x5e, 0x46, 0x7b}};
>+            nsresult rv = GetKnownFolder(&folderid_downloads, aFile);
>+            // On WinXP and 2k, there is no downloads folder, default
>+            // to 'Desktop'.
>+            if(NS_ERROR_FAILURE == rv)
>+            {
>+              rv = GetWindowsFolder(CSIDL_DESKTOP, aFile);
>+            }
>+            return rv;
>+        }
This makes things harder for callers. What's wrong with erroring out if there's no downloads folder? Also, the GUID should be static const.

>+  rv = dirService->Get(NS_WIN_DEFAULT_DOWNLOAD_DIR,
>+                       NS_GET_IID(nsILocalFile),
>+                       getter_AddRefs(downloadDir));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // Check the os version
>+  #define NS_SYSTEMINFO_CONTRACTID "@mozilla.org/system-info;1"
>+  nsCOMPtr<nsIPropertyBag2> infoService =
>+     do_GetService(NS_SYSTEMINFO_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRInt32 version;
>+  NS_NAMED_LITERAL_STRING(osVersion, "version");
>+  rv = infoService->GetPropertyAsInt32(osVersion, &version);
>+  if (version < 6) { // XP/2K
>+    rv = downloadDir->Append(folderName);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
Eww. Why not just get the download dir, and if that fails, get the desktop dir and append the folder name? (Assuming the above change)

Comment 71

[Jim Mathies [:jimm]]

>This makes things harder for callers. What's wrong with erroring out if there's
>no downloads folder? Also, the GUID should be static const.

The idea here was that this call would return the download folder. On some systems that's the desktop, on others it's a different folder. Personally I prefer a directory service call that succeeds unless the system is totally messed up. However, we have bug 393342 that's been posted where it appears this code is failing on the Mac, so it looks like handling failures in ds will be needed. In which case we can make your suggested change.

In general, it comes down to a question of whether or not ds calls are expected to succeed, or can fail. I'm not aware of what that precedent is. Maybe some of the folks cc'd on the bug can comment on that.

Comment 72

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

http://litmus.mozilla.org/show_test.cgi?id=4658 (new profile case)

http://litmus.mozilla.org/show_test.cgi?id=4659 (migrated profile case)

in-litmus+

Comment 73

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

Both Litmus cases (new profile/migrated) are verified as FIXED using:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007101905 Minefield/3.0a9pre

Comment 74

[Shawn Wilsher :sdwilsh]

There's really no good way to test this.