Closed
Bug 536503
Opened 14 years ago
Closed 13 years ago
Last downloaded-to directory should be remembered on a site-by-site basis
Categories
(Toolkit :: Downloads API, enhancement)
Toolkit
Downloads API
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 |
This does for downloads what bug 524408 does for uploads.
Attachment #418967 -
Flags: review?(bzbarsky)
Comment 1•14 years ago
|
||
Ugh. We are using content pref service so more file IO on the main thread? Sadfaces. How does this interact with private browsing?
Comment 2•14 years ago
|
||
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 3•14 years ago
|
||
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-
Comment 4•14 years ago
|
||
Ehsan, can you look over the code that landed in bug 524408? Does that look ok pb-wise?
Comment 5•14 years ago
|
||
(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.
Comment 6•14 years ago
|
||
(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.
Assignee | ||
Comment 7•14 years ago
|
||
I think this covers it all, except Shawn's sadfaces.
Attachment #418967 -
Attachment is obsolete: true
Attachment #419223 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #419223 -
Flags: review? → review?(ehsan.akhgari)
Comment 8•14 years ago
|
||
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)
Assignee | ||
Comment 9•14 years ago
|
||
I suspect this needs SR for the API changes.
Attachment #420016 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #420016 -
Flags: review? → review?(ehsan.akhgari)
Comment 10•14 years ago
|
||
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+
Comment 11•14 years ago
|
||
Ehsan, did you mean to flag actual requestees there?
Comment 12•14 years ago
|
||
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 13•14 years ago
|
||
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...
Assignee | ||
Comment 14•14 years ago
|
||
(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
Comment 15•14 years ago
|
||
(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?
Comment 16•14 years ago
|
||
(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.
Comment 17•14 years ago
|
||
(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 18•14 years ago
|
||
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)
Assignee | ||
Comment 19•14 years ago
|
||
Attachment #420016 -
Attachment is obsolete: true
Attachment #422053 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 20•14 years ago
|
||
(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.
Assignee | ||
Comment 21•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #422306 -
Flags: superreview? → superreview?(mconnor)
Updated•14 years ago
|
Attachment #422306 -
Flags: superreview?(mconnor)
Attachment #422306 -
Flags: review?(gavin.sharp)
Attachment #422306 -
Flags: review-
Comment 22•14 years ago
|
||
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.
Assignee | ||
Comment 23•14 years ago
|
||
> >+// 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)
Comment 24•14 years ago
|
||
(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).
Assignee | ||
Comment 25•14 years ago
|
||
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.
Assignee | ||
Comment 26•14 years ago
|
||
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)
Assignee | ||
Comment 27•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #426761 -
Flags: superreview?(mconnor)
Updated•14 years ago
|
Whiteboard: [ts]
Updated•14 years ago
|
Attachment #426761 -
Flags: superreview?(mconnor) → superreview+
Comment 28•14 years ago
|
||
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.
Assignee | ||
Comment 29•14 years ago
|
||
Attachment #426761 -
Attachment is obsolete: true
Attachment #432238 -
Flags: review?(gavin.sharp)
Attachment #426761 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 30•14 years ago
|
||
Gavin, can we get this reviewed soon please? :)
Assignee | ||
Updated•14 years ago
|
Attachment #432238 -
Flags: approval2.0?
Comment 31•14 years ago
|
||
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?
Assignee | ||
Comment 32•14 years ago
|
||
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)
Updated•13 years ago
|
Whiteboard: [ts] → [ts][not-ready]
Comment 33•13 years ago
|
||
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?
Assignee | ||
Comment 34•13 years ago
|
||
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)
Comment 35•13 years ago
|
||
(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.
Comment 36•13 years ago
|
||
I've been going through my queue lately, and this was near the top of the list. I'll get to it soon.
Updated•13 years ago
|
Attachment #475302 -
Flags: review?(sdwilsh) → review?(gavin.sharp)
Comment 37•13 years ago
|
||
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+
Comment 38•13 years ago
|
||
(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.
Assignee | ||
Comment 39•13 years ago
|
||
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)
Assignee | ||
Comment 40•13 years ago
|
||
Comment 41•13 years ago
|
||
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+
Assignee | ||
Comment 42•13 years ago
|
||
Attachment #538228 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Comment 43•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/8cae6781db4f
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Assignee | ||
Updated•13 years ago
|
Keywords: dev-doc-complete
Comment 44•13 years ago
|
||
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.
Assignee | ||
Comment 45•13 years ago
|
||
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?
Comment 46•13 years ago
|
||
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 .
Assignee | ||
Comment 47•13 years ago
|
||
Attachment #539085 -
Flags: review?(gavin.sharp)
Updated•13 years ago
|
Attachment #539085 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Updated•13 years ago
|
Keywords: checkin-needed
Comment 48•13 years ago
|
||
Comment on attachment 539085 [details] [diff] [review] fix for test mistake http://hg.mozilla.org/mozilla-central/rev/142532172c8c
Comment 49•13 years ago
|
||
(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.
Comment 50•13 years ago
|
||
Comment on attachment 539085 [details] [diff] [review] fix for test mistake relanded: http://hg.mozilla.org/mozilla-central/rev/cdb2ccff9107
Comment 51•13 years ago
|
||
is there a hidden pref to turn this on? its definitely not working on my mac atm...
Comment 52•13 years ago
|
||
James, using which exact channel of Firefox?
Comment 53•13 years ago
|
||
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
Comment 54•13 years ago
|
||
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?
Comment 55•13 years ago
|
||
James could you provide some steps on how you encountered this issue?I'm also failling on reproducing this issue. Thanks.
Comment 56•13 years ago
|
||
i created bug 672817 with my steps to reproduce. thanks.
Comment 57•13 years ago
|
||
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.
Comment 58•13 years ago
|
||
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.
Comment 59•13 years ago
|
||
Sry about the spam.My browser seem to have timedout, and when i hit reloaded it posted my comment twice.
Status: RESOLVED → VERIFIED
Comment 60•13 years ago
|
||
Is there a pref to turn this off? It seems to cause bug 693153.
Comment 61•13 years ago
|
||
Apparently not yet, bug 702748 was opened to create such a pref.
Comment 62•12 years ago
|
||
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.
Comment 63•12 years ago
|
||
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.
Comment 64•12 years ago
|
||
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).
Comment 65•12 years ago
|
||
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.
Comment 66•12 years ago
|
||
See bug 702748 for the pref.
Comment 67•12 years ago
|
||
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.
Description
•