+++ This bug was initially created as a clone of Bug #498695 +++ ------- Comment #1 From Paolo Amadini ------- Possible improvements: >+ // Default to lastDir if we must prompt, and lastDir >+ // is configured and valid. Otherwise, keep the default of >+ // the user's default downloads directory; since it doesn't >+ // exist, it will be changed to the user's desktop later. >+ if (!useDownloadDir) try { Removing this "if" condition would cause the last used directory to be proposed in the file picker, instead of the user's desktop, when an attempt to automatically save in the user's default download directory without a prompt failed because the directory doesn't exist anymore. This would make the file picker's default directory logic more consistent; I don't believe the current behavior was intended by design. > var inPrivateBrowsing = false; > try { > var pbs = Components.classes["@mozilla.org/privatebrowsing;1"] > .getService(Components.interfaces.nsIPrivateBrowsingService); > inPrivateBrowsing = pbs.privateBrowsingEnabled; > } > catch (e) { > } Replacing the try block with a component check would fix a strict JavaScript warning and avoid that other exceptions get eaten: if ("@mozilla.org/privatebrowsing;1" in Components.classes) { var pbs = Components.classes["@mozilla.org/privatebrowsing;1"] .getService(Components.interfaces.nsIPrivateBrowsingService); inPrivateBrowsing = pbs.privateBrowsingEnabled; } ------- Comment #5 From Boris Zbarsky ------- >+ var prefs = getPrefsBrowserDownload("browser.download."); That function's name is sort of redundant with the argument, and the documentation doesn't match the implementation. If you want a generic getPrefBranch function here, that might be fine; just check with a toolkit peer? ------- Comment #7 From Paolo Amadini ------- >+ * @param aSkipPrompt Probably aSkipPrompt is not the best name for the parameter, see bug 484616 comment 8, 9 and 10 for an enumeration of alternatives ;-) I propose to leave the name as it is for now; we can always change it later in a follow-up patch together with the behavior changes. ------- Comment #10 From Shawn Wilsher ------- http://reviews.visophyte.org/r/383508/ on file: toolkit/content/contentAreaUtils.js line 529 > * Given the Filepicker Parameters (aFpP), show the file picker dialog, I think file picker should be two words here on file: toolkit/content/contentAreaUtils.js line 531 > * @param aFpP a structure (see definition in internalSave(...) method) > * containing all the data used within this method. nit: newline after aFpP. Also, can we use an actual phrase here please? on file: toolkit/content/contentAreaUtils.js line 577 > if (!useDownloadDir) try { I'm surprised that's valid. Add braces for the if and have the try on a newline please. on file: toolkit/content/contentAreaUtils.js line 587 > } catch(e) {} } catch (e) { } with a comment in the catch as to why we are doing this on file: toolkit/content/contentAreaUtils.js line 638 > // Since we're automatically downloading, we don't get the file picker's > // logic to check for existing files, so we need to do that here. > // > // Note - this code is identical to that in > // mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in > // If you are updating this code, update that code too! We can't share code > // here since that code is called in a js component. Can you please file a follow-up to move this stuff to DownloadUtils.jsm and then it can be shared. on file: toolkit/content/contentAreaUtils.js line 645 > function uniqueFile(aLocalFile) javadoc comment please? on file: toolkit/content/contentAreaUtils.js line 771 > // Get the preferences branch ("browser.download." for normal 'save' mode)... > function getPrefsBrowserDownload(branch) Ditto on bz's comment. Also, this should have a javadoc style comment. on file: toolkit/content/contentAreaUtils.js line 776 > return Components.classes[prefSvcContractID].getService(prefSvcIID).getBranch(branch); Don't we have Cc and Ci defined in this file? Please use them.

> on file: toolkit/content/contentAreaUtils.js line 638 > > // Since we're automatically downloading, we don't get the file picker's > > // logic to check for existing files, so we need to do that here. > > // > > // Note - this code is identical to that in > > // mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in > > // If you are updating this code, update that code too! We can't share code > > // here since that code is called in a js component. > > Can you please file a follow-up to move this stuff to DownloadUtils.jsm and > then it can be shared. Now that bug 484616 is closed, I've spent some time in studying the existing code related to the selection of the file name to be used for downloads. I'm impressed by the fact that, besides the uniqueFile/makeFileUnique functions, most of the logic in nsHelperAppDlg.js and contentAreaUtils.js is identical, just implemented with totally different code, but the results are the same even in some corner cases I tested. I'm considering whether it can be appropriate to move most of the common logic to a new JS module, say DownloadPaths.jsm (whereas DownloadUtils.jsm is currently dedicated to calculating download times). This refactoring should be done carefully, keeping the various corner cases in mind, but might help in sorting out the desired behavior on various platforms, and maybe ease the task of adding new features consistently (like the ability of saving a page using its title as the file name). Another reasons why this refactoring would be useful for me is that I'd write the functions in such a way that customizing their behavior would be easier; for example, I need to add file types for web archives to the "Save As" dialog, but today I have to override entire functions in contentAreaUtils.js. There are also extensions that want to customize the automatic download folder based on the file type, for example.

(In reply to comment #1) > I'm considering whether it can be appropriate to move most of the common > logic to a new JS module, say DownloadPaths.jsm (whereas DownloadUtils.jsm > is currently dedicated to calculating download times). This refactoring > should be done carefully, keeping the various corner cases in mind, but > might help in sorting out the desired behavior on various platforms, and > maybe ease the task of adding new features consistently (like the ability > of saving a page using its title as the file name). This would probably be a great idea!

Here is the DownloadPaths module with a few functions already defined. Shawn, can you have a look and see if this structure makes sense? The next step would consist in adding a constructor or factory function in DownloadPaths to create DownloadTarget JavaScript objects. These objects would have properties like sourceUri, contentType, contentDisposition and mimeInfo that can be set to have the object find out both the suggested file name and target download directory by itself. Overriding the factory function or constructor function to return derived objects (the "__proto__" technique) would allow extensions to customize this behavior.

(In reply to comment #3) > Shawn, can you have a look and see if this structure makes sense? It will be at least a week until I can take a look I am sorry to say.

(In reply to comment #4) > It will be at least a week until I can take a look I am sorry to say. No problem, thanks for telling me :-) I won't have much time to work on this bug this week too, but meanwhile I'll try to do some experiments with the DownloadTarget object.

If you could take a look to the work in progress and provide some feedback on the idea in comment 3, it would be helpful. I'm not asking for a review, just feedback on the approach. Or would you like to see some other code first?

There is a feedback flag on patches just for this ;)

Comment on attachment 432526 [details] [diff] [review] Work in progress - DownloadPaths module (In reply to comment #7) > There is a feedback flag on patches just for this ;) Ah, interesting...

I was really hoping to get to the feedback request today, but it doesn't look like it is going to happen. I'll prioritize it once I get back in a week. Sorry for the delay!

Comment on attachment 432526 [details] [diff] [review] Work in progress - DownloadPaths module createNiceUniqueFile should not touch aLocalFile if it throws an exception. Just clone it. In fact, I'd rather it return the new file and never touch the one passed in. Of course, you'll have tests when I do a final review :)

(In reply to comment #10) > (From update of attachment 432526 [details] [diff] [review]) > Of course, you'll have tests when I do a final review :) Whoops... where did the tests go? Ah, just forgot to "hg add" the file... mind to take a look? In my intentions, the test were an integral part of the feedback request!

I'd also like to know your opinion on the approach described in comment 3. I can also write some code to show what I mean, if required.

(In reply to comment #12) > I'd also like to know your opinion on the approach described in comment 3. > I can also write some code to show what I mean, if required. I'd appreciate some elaboration because it is not completely clear to me.

Comment on attachment 439487 [details] [diff] [review] Work in progress - DownloadPaths module Yeah, this looks good. Also, if you are OK with the licensing, you should have the test be under the creative commons public domain per http://www.mozilla.org/MPL/license-policy.html

Comment on attachment 439487 [details] [diff] [review] Work in progress - DownloadPaths module I moved the part of the patch that's not related to contentAreaUtils.js to bug 561472, so that it can land independently. The part in contentAreaUtils.js still requires attention since the new createNiceUniqueFile function creates an empty file as a placeholder while getTargetFile doesn't.

Hi, I am coming from https://bugzilla.mozilla.org/show_bug.cgi?id=429827 It seems that proposed changes to this file are finally settling down (in 561472). Now, I am interested in incorporating the changes into nsHelperAppDlg.js But I need to figure out how to create the .jsm file for it. Or can I share it with this change? (I am a newbie to javascript and am unfamiliar with how javascript modules are handled inside mozilla code at runtime although I did read basic xpcom documents.) Anyway, do let me know when I can finally work on the 429827. Quick reading made me think that my proposed changes in 429827 and changes proposed here don't step on each other (of course we need to split and introduce a new file [or not]), so the merging the changes should be OK. The only thing I need to figure out is whether I need to introduce a separate .jsm file or not. TIA step on e

I'm pretty sure you don't need to get a new jsm; you should be able to import the one that this bug creates (see the tests for how!)

(In reply to comment #17) > I'm pretty sure you don't need to get a new jsm; you should be able to import > the one that this bug creates (see the tests for how!) Thanks. I can finally test and build my latest clean up patch that is based on the checked-in patch from Paolo Amadini. I wonder if it is all right to post the "PROPERLY INDENTED" nsHelperAppDlg.js first ( see bug 559833 ) first, and then post the patch to fix the problem mentioned in bug 429827 . Well, anyway, to show how the patch looks like. I will first post the indentation patch to bug 559833. Then later, I will post the patch to fix the bug 429827, hopefully the indention is applied for bug 559833. TIA.

> I can finally test and build my latest clean up patch that is based on the > checked-in patch from Paolo Amadini. I meant to say that "I could finally test", and I DID find the newly modified patch for 429827 works with this factoring already applied. Thanks for the clean up, Paolo. I agree that the less code, the better. Usually, the less code means less bugs. So removing duplicate like this is a way to go where applicable. TIA.

I went through the list of improvements in comment 0 and noticed that most of them have already been implemented, while others don't apply anymore since the code has changed significantly in the meantime.