Closed Bug 536503 Opened 10 years ago Closed 9 years ago

Last downloaded-to directory should be remembered on a site-by-site basis

Categories

(Toolkit :: Downloads API, enhancement)

enhancement
Not set

Tracking

()

VERIFIED FIXED
mozilla7

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 11 obsolete files)

11.05 KB, patch
Details | Diff | Splinter Review
22.11 KB, patch
Details | Diff | Splinter Review
1.35 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
Attached patch patch v1 (obsolete) — Splinter Review
This does for downloads what bug 524408 does for uploads.
Attachment #418967 - Flags: review?(bzbarsky)
Ugh.  We are using content pref service so more file IO on the main thread?  Sadfaces.

How does this interact with private browsing?
The content pref service is aware of the private browsing mode (see <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/tests/unit/test_bug248970.js> for the behavior of the content pref service with regard to private browsing mode.

This patch modifies <http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/src/DownloadLastDir.jsm> (which is good), but it regresses the behavior implemented in bug 464795.  A correct patch should save the directory in memory inside the private browsing mode instead of trying to save it to the content prefs service (because that would fail), and delete that in-memory value when the private browsing mode is deactivated (similar to how the gDownloadLastDir.file API is working.)

Also, it should come with unit tests which test this functionality (and probably also test the functionality that the patch is itself implementing.)
Comment on attachment 418967 [details] [diff] [review]
patch v1

r- based on comment 2.

Geoff, can you please ask a review from me as well with your next version of this patch?  Thanks!
Attachment #418967 - Flags: review?(bzbarsky) → review-
Ehsan, can you look over the code that landed in bug 524408?  Does that look ok pb-wise?
(In reply to comment #4)
> Ehsan, can you look over the code that landed in bug 524408?  Does that look ok
> pb-wise?

Well, it doesn't leak anything (since the content prefs service doesn't store the pref inside the private browsing mode), but its behavior is not optimal.  I'll file a new bug on that.
(In reply to comment #5)
> (In reply to comment #4)
> > Ehsan, can you look over the code that landed in bug 524408?  Does that look ok
> > pb-wise?
> 
> Well, it doesn't leak anything (since the content prefs service doesn't store
> the pref inside the private browsing mode), but its behavior is not optimal. 
> I'll file a new bug on that.

Filed bug 536567.
Attached patch patch v2 (obsolete) — Splinter Review
I think this covers it all, except Shawn's sadfaces.
Attachment #418967 - Attachment is obsolete: true
Attachment #419223 - Flags: review?
Attachment #419223 - Flags: review? → review?(ehsan.akhgari)
Comment on attachment 419223 [details] [diff] [review]
patch v2

I think this patch is mostly good, except for the fact that I don't think there is much point in keeping both the gDownloadLastDir.file API and the new API side by side, because it would just confuse callers.  gDownloadLastDir can fall back to getting and setting a plain (non hostname specific) directory under the hoods as it does in your current patch, but we shouldn't expose both APIs to the callers.  Instead, I suggest renaming the new APIs to getFile and setFile, and remove the old API.  passing null or undefined to these APIs should be equivalent to using the current file property (and the existing tests should be modified to reflect this.)

Also, please change the |if (aURL != undefined)| tests to |if (!aURL)|, so that passing in null also works.

Oh, and while you're at it, it's better to rename those params to aURI, since those are URI objects, really, and not URLs.  :-)
Attachment #419223 - Attachment is obsolete: true
Attachment #419223 - Flags: review?(ehsan.akhgari)
Attached patch patch v3 (obsolete) — Splinter Review
I suspect this needs SR for the API changes.
Attachment #420016 - Flags: review?
Attachment #420016 - Flags: review? → review?(ehsan.akhgari)
Comment on attachment 420016 [details] [diff] [review]
patch v3

Seems good to me, thanks for the patch.  Note that you still need a review from a toolkit peer, and a superreview as you mentioned yourself.

Thanks for working on this!
Attachment #420016 - Flags: superreview?
Attachment #420016 - Flags: review?(ehsan.akhgari)
Attachment #420016 - Flags: review?
Attachment #420016 - Flags: review+
Ehsan, did you mean to flag actual requestees there?
Comment on attachment 420016 [details] [diff] [review]
patch v3

I wanted to let Geoff set them, but whatever!  There we go!
Attachment #420016 - Flags: superreview?(mconnor)
Attachment #420016 - Flags: superreview?
Attachment #420016 - Flags: review?(gavin.sharp)
Attachment #420016 - Flags: review?
Comment on attachment 420016 [details] [diff] [review]
patch v3

>diff --git a/toolkit/content/contentAreaUtils.js b/toolkit/content/contentAreaUtils.js

>-    if (!getTargetFile(fpParams, aSkipPrompt))
>+    let documentURI = aDocument ? makeURI(aDocument.location.href, null, null) :
>+                                  (documentURI = aReferrer ? aReferrer : aURL);

Is that last "=" meant to be "==" in the parenthetical? Quite confusing to read, a comment would help...

makeURI arguments can be omitted rather than passing null explicitly.

>diff --git a/toolkit/mozapps/downloads/src/DownloadLastDir.jsm b/toolkit/mozapps/downloads/src/DownloadLastDir.jsm

Can you keep the original pseudo-API (.file getter/setter) and just make getFile()/setFile() additional methods?

>diff --git a/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in b/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in

>+      let documentURI;
>+      try {
>+        documentURI = ioService.newURI(aContext.document.location.href, null, null);

Does this actually work? I thought mContext could be a chrome window...
(In reply to comment #13)
> (From update of attachment 420016 [details] [diff] [review])
> >diff --git a/toolkit/content/contentAreaUtils.js b/toolkit/content/contentAreaUtils.js
> 
> >-    if (!getTargetFile(fpParams, aSkipPrompt))
> >+    let documentURI = aDocument ? makeURI(aDocument.location.href, null, null) :
> >+                                  (documentURI = aReferrer ? aReferrer : aURL);
> 
> Is that last "=" meant to be "==" in the parenthetical? Quite confusing to
> read, a comment would help...

Hmm, it shouldn't even be there.
 
> >diff --git a/toolkit/mozapps/downloads/src/DownloadLastDir.jsm b/toolkit/mozapps/downloads/src/DownloadLastDir.jsm
> 
> Can you keep the original pseudo-API (.file getter/setter) and just make
> getFile()/setFile() additional methods?

I just took them out at Ehsan's request! But actually I think they might just as well be there. Putting them back in....

> >diff --git a/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in b/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in
> 
> >+      let documentURI;
> >+      try {
> >+        documentURI = ioService.newURI(aContext.document.location.href, null, null);
> 
> Does this actually work? I thought mContext could be a chrome window...

mContext is an QIed to nsIDOMWindowInternal a bit earlier, but I guess that doesn't mean it IS one. I'll stick in an explicit QI to nsIDomWindow.(In reply to comment #12)

> (From update of attachment 420016 [details] [diff] [review])
> I wanted to let Geoff set them, but whatever!  There we go!

You know these people much better than I do :)
Status: NEW → ASSIGNED
(In reply to comment #13)
> >diff --git a/toolkit/mozapps/downloads/src/DownloadLastDir.jsm b/toolkit/mozapps/downloads/src/DownloadLastDir.jsm
> 
> Can you keep the original pseudo-API (.file getter/setter) and just make
> getFile()/setFile() additional methods?

Why do you think that they should be kept?
(In reply to comment #14)
> mContext is an QIed to nsIDOMWindowInternal a bit earlier, but I guess that
> doesn't mean it IS one. I'll stick in an explicit QI to nsIDomWindow.

QI won't turn a chrome window into a content window (or vice versa) - if this works, then you have a content window and everything is fine, no need for QI junk. Quick look at nsExternalHelperAppService code suggests that that is the case, so just ignore my original comment.

(In reply to comment #15)
> Why do you think that they should be kept?

There seems to be little benefit to removing them, and there is a compatibility cost and increased risk introduced by having to change more code/tests.
(In reply to comment #16)
> (In reply to comment #15)
> > Why do you think that they should be kept?
> 
> There seems to be little benefit to removing them, and there is a compatibility
> cost and increased risk introduced by having to change more code/tests.

OK, I guess I'm ambivalent here, so let's keep the old API.
Comment on attachment 420016 [details] [diff] [review]
patch v3

sr- for the unnecessary pseudo-API changes, happy to take another look once gavin's comments are addressed.
Attachment #420016 - Flags: superreview?(mconnor)
Attachment #420016 - Flags: superreview-
Attachment #420016 - Flags: review?(gavin.sharp)
Attached patch patch v2.1 (obsolete) — Splinter Review
Attachment #420016 - Attachment is obsolete: true
Attachment #422053 - Flags: review?(gavin.sharp)
(In reply to comment #19)
> Created an attachment (id=422053) [details]
> patch v2.1

Oh, forget the changes I made to test_privatebrowsing_downloadLastDir.js, they weren't meant to still be there.
Attached patch patch v2.2 (obsolete) — Splinter Review
Finally figured out a sometimes-failing test.
Attachment #422053 - Attachment is obsolete: true
Attachment #422306 - Flags: superreview?
Attachment #422306 - Flags: review?(gavin.sharp)
Attachment #422053 - Flags: review?(gavin.sharp)
Attachment #422306 - Flags: superreview? → superreview?(mconnor)
Attachment #422306 - Flags: superreview?(mconnor)
Attachment #422306 - Flags: review?(gavin.sharp)
Attachment #422306 - Flags: review-
Comment on attachment 422306 [details] [diff] [review]
patch v2.2

>diff --git a/toolkit/content/contentAreaUtils.js b/toolkit/content/contentAreaUtils.js

>+    // Find the most relevant URI to remember this file selection by
>+    let documentURI = aDocument ? makeURI(aDocument.location.href) :
>+                                  (aReferrer ? aReferrer : aURL);

// Find a URI to use for determining last-downloaded-to directory
let relatedURI = (aDocument) ? aDocument.documentURIObject : (aReferrer || aURL);

>+ * @param aDocumentURI
>+ *        An nsIURI of the page the user is currently viewing. The last used
>+ *        directory of the picker is retrieved from/stored in the 
>+ *        Content Pref Service using this URI.

"currently viewing" is overly specific given that we fall back to the URI of the referrer or of the file itself. "A URI associated with the save", perhaps? Though that makes me wonder what the aDocument==null cases are.

>diff --git a/toolkit/mozapps/downloads/src/DownloadLastDir.jsm b/toolkit/mozapps/downloads/src/DownloadLastDir.jsm

>+// contentPrefSvc can't be initialized until the profile directory is available
>+let contentPrefSvc = null;

>+let hostnameGrouperSvc = Components.classes["@mozilla.org/content-pref/hostname-grouper;1"]
>+                                   .getService(Components.interfaces.nsIContentURIGrouper);

What's that comment about? Who's importing this module before the profile directory is available? I wouldn't expect that to happen, and if it is something's probably wrong. In any case, you should use XPCOMUtils.defineLazyServiceGetter() for these.

>diff --git a/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in b/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in

>+      var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>+                                .getService(Components.interfaces.nsIIOService);
>+      let documentURI;
>+      try {
>+        documentURI = ioService.newURI(aContext.document.location.href);
>+      }
>+      catch (ex) {
>+      }

Just use aContext.document.documentURIObject.
Attached patch patch v2.3 (obsolete) — Splinter Review
> >+// contentPrefSvc can't be initialized until the profile directory is available
> >+let contentPrefSvc = null;
> 
> >+let hostnameGrouperSvc = Components.classes["@mozilla.org/content-pref/hostname-grouper;1"]
> >+                                   .getService(Components.interfaces.nsIContentURIGrouper);
> 
> What's that comment about? Who's importing this module before the profile
> directory is available? I wouldn't expect that to happen, and if it is
> something's probably wrong. In any case, you should use
> XPCOMUtils.defineLazyServiceGetter() for these.

nsHelperAppDlg.js is the guilty party.
Attachment #422306 - Attachment is obsolete: true
Attachment #422423 - Flags: review?(gavin.sharp)
(In reply to comment #23)
> nsHelperAppDlg.js is the guilty party.

Can you elaborate? That file is just a normal JS component, and I wouldn't expect it to be loaded in the startup path at all (let alone before we had a profile).
My knowledge of start up isn't great, but I've figured out that:

nsHelperAppDlg.js is importing DownloadLastDir.jsm at some point before the contentPref service and the hostname grouper service are registered,

and this means Cc['@mozilla.org/helperapplauncherdialog;1'] is undefined.
Attached patch patch v4 (obsolete) — Splinter Review
I've left the defineLazyServiceGetter in, although I'm pretty sure only xpcshell tests are having problems with it. One I can fix with a call to do_get_profile, but the other (test_privatebrowsing_downloadLastDir.js) I can't. Not sure that it's worth spending time on.

Other than that I think this patch is ready.
Attachment #422423 - Attachment is obsolete: true
Attachment #426194 - Flags: review?(gavin.sharp)
Attachment #422423 - Flags: review?(gavin.sharp)
Attached patch patch v5 (obsolete) — Splinter Review
Aha! I've got DownloadLastDir.jsm out of the startup path, which fixes the tests without needing lazy service getters. Plus it'll make startup that little bit faster :D
Attachment #426194 - Attachment is obsolete: true
Attachment #426761 - Flags: review?(gavin.sharp)
Attachment #426194 - Flags: review?(gavin.sharp)
Attachment #426761 - Flags: superreview?(mconnor)
Whiteboard: [ts]
Blocks: 447581
Attachment #426761 - Flags: superreview?(mconnor) → superreview+
Comment on attachment 426761 [details] [diff] [review]
patch v5

Can we just make the .file getter and setter be shims for the new methods, and move their contents to the end of getFile/setFile?  Something like:

// compat shims
get file() { return this.getFile() }
set file(val) { this.setFile(null, val) }

It's mostly future-proofing, but it's cleaner to read as a pattern, and we can refactor a little to only need one check for PB state.  sr=me with that fixed.
Attached patch patch v6 (obsolete) — Splinter Review
Attachment #426761 - Attachment is obsolete: true
Attachment #432238 - Flags: review?(gavin.sharp)
Attachment #426761 - Flags: review?(gavin.sharp)
Gavin, can we get this reviewed soon please? :)
Attachment #432238 - Flags: approval2.0?
Comment on attachment 432238 [details] [diff] [review]
patch v6

I'm sorry that I haven't had a chance to review this in a reasonable amount of time - I'll try to get to it soon when I'm back from vacation. It needs to be reviewed before you request approval, though.
Attachment #432238 - Flags: approval2.0?
Attached patch patch v6, updated (obsolete) — Splinter Review
Just giving this patch a bit of bitrot removal, and a fix for an extremely infrequent case. I'm reasonably sure it's not affected by feature freeze, but it would still be nice to get it in the tree soon. :)
Attachment #432238 - Attachment is obsolete: true
Attachment #475302 - Flags: review?(gavin.sharp)
Attachment #432238 - Flags: review?(gavin.sharp)
Whiteboard: [ts] → [ts][not-ready]
I have some concern.
* When user clear history whether the entry saving download location 
  be removed will be removed? 
* Once implemented, Is there a about:config entry to disable 
  this feature all together?
Comment on attachment 475302 [details] [diff] [review]
patch v6, updated

Switching reviewers on this as it's so old Gavin's not the appropriate person any more.
Attachment #475302 - Flags: review?(gavin.sharp) → review?(sdwilsh)
(In reply to comment #34)
> Switching reviewers on this as it's so old Gavin's not the appropriate
> person any more.
Can you respond to comment 33 please?  Gavin might be the right person to do this still since he's already done a few cycles on this patch.  I'll talk to him about it.
I've been going through my queue lately, and this was near the top of the list. I'll get to it soon.
Attachment #475302 - Flags: review?(sdwilsh) → review?(gavin.sharp)
Comment on attachment 475302 [details] [diff] [review]
patch v6, updated

>diff --git a/toolkit/content/contentAreaUtils.js b/toolkit/content/contentAreaUtils.js

>-    if (!getTargetFile(fpParams, aSkipPrompt))
>+    // Find a URI to use for determining last-downloaded-to directory
>+    let relatedURI = aDocument ? aDocument.documentURIObject : (aReferrer || sourceURI);

internalSave is a horrible function to try to understand, but it looks like the only caller that passes in aDocument only does so when the document itself is being saved (so document.documentURIObject would be equal to sourceURI). I think that means that this would be better off as just (aReferrer || sourceURI).

>diff --git a/toolkit/mozapps/downloads/DownloadLastDir.jsm b/toolkit/mozapps/downloads/DownloadLastDir.jsm

>+let hostnameGrouperSvc = Components.classes["@mozilla.org/content-pref/hostname-grouper;1"]
>+                                   .getService(Components.interfaces.nsIContentURIGrouper);

Using this directly will cause a difference in behavior in PB mode if content prefs ever end up using a different grouper. I guess that's unlikely (it would probably break a bunch of other stuff), but you can avoid that by just using contentPrefSvc.grouper instead (and using a more generic "contentPrefsGrouper" variable name). Other places that use nsIContentURIGrouper seem to have made this same mistake. A followup on fixing them would be good!

I suppose we also run the risk of being broken by a grouper that produces group names that aren't valid JS properties (or that can't be overridden). We should avoid that by using Dict.jsm instead of a plain old object.

>+  setFile: function (aURI, aFile) {

Hmm, a bit odd that you end up setting both the site-specific pref (if applicable) and the global pref at the same time. I guess it makes sense.

>diff --git a/toolkit/mozapps/downloads/nsHelperAppDlg.js b/toolkit/mozapps/downloads/nsHelperAppDlg.js

>-Components.utils.import("resource://gre/modules/DownloadLastDir.jsm");

This change isn't worth making (perf difference is neglible, not perf-critical codepath, and hampers code clarity/simplicity).

>+      if (aContext.document && aContext.document.documentURIObject)
>+        relatedURI = aContext.document.documentURIObject;

No need for the aContext.document.documentURIObject null check. Indentation is off here (and there's some unnecessary whitespace on the blank line added above this).

>diff --git a/toolkit/mozapps/downloads/tests/unit/test_DownloadLastDirWithCPS.js b/toolkit/mozapps/downloads/tests/unit/test_DownloadLastDirWithCPS.js

>+  function clearHistory() {

nit: simulateClearHistory?

r=me with these addressed, but we might need to tweak the behavior here in response to feedback. This would probably benefit from ui-review to ensure that the behavior is desirable in general.

I'm sorry that I was so unresponsive for such a long time here Geoff - this is good work.
Attachment #475302 - Flags: review?(gavin.sharp) → review+
(In reply to comment #37)
> >-Components.utils.import("resource://gre/modules/DownloadLastDir.jsm");
> 
> This change isn't worth making (perf difference is neglible, not
> perf-critical codepath, and hampers code clarity/simplicity).

Oh, just re-read comment 27. I really don't understand why this code would be in the startup path - we should figure that out before working around it.
Attached patch patch v7 (obsolete) — Splinter Review
Comments addressed, and I've also changed the code to use Services.jsm.

(In reply to comment #38)
> Oh, just re-read comment 27. I really don't understand why this code would
> be in the startup path - we should figure that out before working around it.

I have no idea what I meant either.
Attachment #475302 - Attachment is obsolete: true
Attachment #538228 - Flags: review?(gavin.sharp)
Attached patch changes v6 -> 7Splinter Review
Comment on attachment 538228 [details] [diff] [review]
patch v7

>diff --git a/toolkit/content/contentAreaUtils.js b/toolkit/content/contentAreaUtils.js

>+ * @param aDocumentURI

nit: relatedURI? sourceURI?

>diff --git a/toolkit/mozapps/downloads/DownloadLastDir.jsm b/toolkit/mozapps/downloads/DownloadLastDir.jsm

>+  getFile: function (aURI) {

>+        if (gDownloadLastDirStore.has(group))
>+          lastDir = gDownloadLastDirStore.get(group);

You could just use get(group, null) and skip the call to has().

>diff --git a/toolkit/mozapps/downloads/tests/unit/test_DownloadLastDir.js b/toolkit/mozapps/downloads/tests/unit/test_DownloadLastDir.js

>   function clearHistory() {

Oh I didn't this was copied - should keep the consistent.
Attachment #538228 - Flags: review?(gavin.sharp) → review+
Attached patch patch v8Splinter Review
Attachment #538228 - Attachment is obsolete: true
No longer blocks: 447581
Keywords: checkin-needed
Whiteboard: [ts][not-ready]
http://hg.mozilla.org/mozilla-central/rev/8cae6781db4f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
I have the latest nightly with this bug fixed , how to make use of it and see it working ?
I tried to download a pic from one site in one folder and a vid from another to another , but when I went to the first site again to download a pic , I got the opened directory as the one for the vid site. 
Do I have to add a config entry ?
please tell.
This would've first appeared in the 2011-06-12 nightly. Also, if you're downloading a video, is the download done by Firefox, or by a plugin or addon?
Video was just an example . Everything is done by firefox. Lets say any two links on different sites . All I want is to know that this feature works or not .
Blocks: 663777
Attachment #539085 - Flags: review?(gavin.sharp)
Attachment #539085 - Flags: review?(gavin.sharp) → review+
Keywords: checkin-needed
(In reply to comment #48)
> Comment on attachment 539085 [details] [diff] [review] [review]
> fix for test mistake
> 
> http://hg.mozilla.org/mozilla-central/rev/142532172c8c

The push broke the build, so I backed all of its changesets out.  This has been landed on inbound though, and will get on the tree in the next merge.
is there a hidden pref to turn this on? its definitely not working on my mac atm...
James, using which exact channel of Firefox?
I'm am on the latest nightly build.

I've dug into this a bit further. on mac the download folder is remembered per site if you right click and 'save link as' but as soon as you just click a link and go through the 'what should nightly do with this file box' it reverts to just the most recent download folder... 

ah, it also throws a javascript error:

Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIHelperAppLauncher.saveToDisk]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource:///components/nsHelperAppDlg.js :: <TOP_LEVEL> :: line 462"  data: no]
Source File: resource:///components/nsHelperAppDlg.js
Line: 462

from my brief testing in the office it all seems to work ok on windows
James, I can't seem to reproduce that error (also on Mac and with a nightly), but maybe we're doing different things.  Would you mind filing a new bug, with clear steps to reproduce and ccing me?
James could you provide some steps on how you encountered this issue?I'm also failling on reproducing this issue.
Thanks.
i created bug 672817 with my steps to reproduce. thanks.
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0

Last downloaded-to directory is remembered.
Setting resolution to Verified Fixed.
Thanks.
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0

Last downloaded-to directory is remembered.
Setting resolution to Verified Fixed.
Thanks.
Sry about the spam.My browser seem to have timedout, and when i hit reloaded it posted my comment twice.
Status: RESOLVED → VERIFIED
Is there a pref to turn this off? It seems to cause bug 693153.
Depends on: 693153, 672817
Apparently not yet, bug 702748 was opened to create such a pref.
Blocks: 693153
No longer depends on: 693153
From FF 7 through FF 10 the default "Save as" directory has been determined by whatever directory a file from the site a user is presently on was last saved to. According to Bug 702748, this will be resolved in FF 11, but I am unsure of how it is being resolved.

   Comment 8 David E. Ross 2011-12-14 11:38:51 PST
   In which end-user release will this be fixed?

   Comment 9 Jens Hatlak 2011-12-14 12:33:46 PST
   As the target milestone says: mozilla11 (FF/TB 11 / SM 2.8

What it seems to be changing to is:

   Comment 10 :aceman 2011-12-28 23:35:24 PST

   And the most important information is missing in the comments:
   The pref is called "browser.download.lastDir.savePerSite" and is
   accessible in about:config (if not, create it as a bool).

"browser.download.lastDir.savePerSite" is the problem. It needs to be changed from savePerSite to simply lastDir. Can this become an choice in the FF Options?

Example of need for fix:

I research a lot on the Web. That means I am generally saving a group of files or Web pages from all over the Web to one directory for the project that I am researching. However FF now downloads to the last folder I downloaded to from whatever site I am saving from, meaning that almost every time I select "Save as", Firefox sends my download to another directory on another drive in another partition. This action is constantly infuriating. All I want is to have the "Save as" default folder be what ever folder I last saved a download to irregardless of what site I am on.
Tony, as I understand it you will have to create the "browser.download.lastDir.savePerSite" pref in about:config as boolean, and then set the value to FALSE. That's it.
Thanks Marcelo. That seemed like it should work. However, something's overriding that preference. In my about:config file I have the following entry —

browser.download.lastDir.savePerSite    user set    boolean    false

Do I need to delete or modify another preference? The string value for "browser.download.lastDir" has the last directory I downloaded to correctly entered, but going to sites I recently visited (such as Wikipedia and Fox News) and clicking on "Save as" still takes me to the last directories I downloaded to from those sites, not to the last directory specified in "browser.download.lastDir". Searching within about:config for any of those directory names does not find anything, so the names must be saved in some other file or in the Registry (this is a Windows OS system). Searching for those directory names within the files of my Fierfox Profile directory pulls up content-prefs.sqlite, which also contains names of many directories I downloaded to from many months back. Searching for the directory names in my Registry pulls up nothing to note.

I've closed and reopened FF several times to make sure the modified about:config file is loading. I've downloaded files to different folders to clear the old path strings, but that only produced new path string defaults for those sites (servers).
I take it that this "feature" has been hard coded into the download manager, and that there is not way to override it via about:config modifiers.
As Firefox 11 was about to come out with the problem resolved, I thought I'd wait for that release and see how it worked. It works great! I'm not sure if the reason it works is that I already had the entry

browser.download.lastDir.savePerSite    user set    boolean    false

in my about:config file or if FF 11 writes that entry into the about:config file, but I am very happy with the way Save as now works.
You need to log in before you can comment on or make changes to this bug.