+++ This bug was initially created as a clone of Bug #506987 +++ Add the new DownloadPaths JavaScript module.

Comment on attachment 441173 [details] [diff] [review] New DownloadPaths module, used in nsHelperAppDlg.js >+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- >+ * ***** BEGIN LICENSE BLOCK ***** Should follow https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Mode_Line, plus the filetype bit (http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#2) so js highlighting works in vim Also, make sure you've copied the license from http://www.mozilla.org/MPL/boilerplate-1.1/ >+var EXPORTED_SYMBOLS = [ "DownloadPaths" ]; nit: format like this var EXPORTED_SYMBOLS = [ "DownloadPaths", ]; >+var DownloadPaths = { Probably want this to be a const too. >+ * @return A new instance of an nsILocalFile object pointing to the newly nit: @returns >+ for (var i = 1; i < 10000 && curFile.exists(); i++) { >+ curFile.leafName = base + "(" + i + ")" + ext; >+ } nit: use let in the for loop. >+ curFile.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0600); Use Ci here >+++ b/toolkit/mozapps/downloads/tests/unit/test_DownloadPaths.js >+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- nit: missing vim modeline >+ * ***** BEGIN LICENSE BLOCK ***** nit: this should be "/*" instead of " ". Basically, it should be its own comment block. >+function createTemporarySaveDirectory() { nit: brace on new line >+ if (!saveDir.exists()) >+ saveDir.create(Ci.nsIFile.DIRECTORY_TYPE, 0755); nit: brace one line ifs please (in new code. In old code, follow existing style) r=sdwilsh

(In reply to comment #2) > https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Mode_Line Aren't the two mode lines in the style guide inconsistent at present? /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ /* vim: set ts=2 et sw=2 tw=80: */ ...instead of... /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ /* vim: set ts=8 et sw=2 tw=80: */ Also, is it fine to use Mode: Java for JS files? > plus the filetype bit so js highlighting works in vim I guess this is required only for .jsm files, right? > nit: brace one line ifs please (in new code. In old code, follow existing > style) Ah, good, that's the style I like ;-)

Following up to bug 506987 comment 3, the next step for this module would be to create a somewhat customizable system for assigning the correct default file name and location to a web page or binary file being downloaded. The default file name might depend on the properties of the source, like the "Content-Disposition" header in an HTTP download. The destination would depend on the user's download preferences under normal conditions, but extensions might want to customize this behavior, for example to allow the user to configure different download folders based on the file type. Here is my proposed way of implementing this: // Not exported funtion DownloadTarget() { } DownloadTarget.prototype = { sourceUri: null, contentDisposition: "", contentType: "", setPropertiesFromDocument: function(aDocument) { ... }, getTargetFolder: function() do_processing(userPreferences), getTargetLeafName: function() do_processing(this.sourceUri, ...), getTargetFile: function() do_concat(getTargetFolder(), getTargetLeafName()), createUniqueTargetFile: function() { return DownloadPaths.createNiceUniqueFile(this.getTargetFile()); } } const DownloadPaths = { // Overridable constructor for DownloadTarget DownloadTarget: DownloadTarget, newDownloadTargetFromDocument: function(aDocument) { var target = new this.DownloadTarget(); target.setPropertiesFromDocument(aDocument); return target; }, newDownloadTarget: function() { return new this.DownloadTarget(); } } Using the object: var target = DownloadPaths.newDownloadTargetFromDocument(doc); target.otherProperties = ...; return target.getTargetFile(); Customizing (in extensions): function MyDownloadTargetImpl() { }; MyDownloadTargetImpl.prototype = { __proto__: DownloadPaths.DownloadTarget.prototype, // Override the default implementation of getTargetFolder getTargetFolder: function() do_processing(this.contentType, ...) } DownloadPaths.DownloadTarget = MyDownloadTargetImpl; The key to easy customizability is to split the logic for file name and location selection into small functions that do only part of the task. This would also help in clarifying the steps that are currently done to determine the final name, which at present are defined in various places and slightly different between "Save As" and the helper apps dialog.

(In reply to comment #3) > Aren't the two mode lines in the style guide inconsistent at present? > /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > /* vim: set ts=8 et sw=2 tw=80: */ The problem is that et (expand tab) is set, so when you press the tab key, 8 spaces are created. If we didn't set et, then I'd agree with you. > Also, is it fine to use Mode: Java for JS files? You'd have to find an emacs user and ask. > I guess this is required only for .jsm files, right? Yup, vim knows nothing about .jsm but does know that .js is a JavaScript file.

(In reply to comment #5) > > /* vim: set ts=8 et sw=2 tw=80: */ > The problem is that et (expand tab) is set, so when you press the tab key, 8 > spaces are created. If we didn't set et, then I'd agree with you. Ah, ok, so shiftwidth is not considered when pressing the tab key.

This patch may be subject to the crash in bug 551658.