Closed
Bug 561472
Opened 15 years ago
Closed 14 years ago
Add a new DownloadPaths JavaScript module
Categories
(Toolkit :: Downloads API, defect)
Toolkit
Downloads API
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a5
People
(Reporter: Paolo, Assigned: Paolo)
References
Details
Attachments
(1 file, 2 obsolete files)
16.45 KB,
patch
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #506987 +++
Add the new DownloadPaths JavaScript module.
Assignee | ||
Comment 1•15 years ago
|
||
Assignee: nobody → paolo.02.prg
Attachment #441173 -
Flags: review?(sdwilsh)
Comment 2•14 years ago
|
||
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
Attachment #441173 -
Flags: review?(sdwilsh) → review+
Assignee | ||
Comment 3•14 years ago
|
||
(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 ;-)
Attachment #441173 -
Attachment is obsolete: true
Assignee | ||
Comment 4•14 years ago
|
||
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.
Comment 5•14 years ago
|
||
(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.
Assignee | ||
Comment 6•14 years ago
|
||
(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.
Assignee | ||
Comment 7•14 years ago
|
||
Attachment #441734 -
Attachment is obsolete: true
Assignee | ||
Comment 8•14 years ago
|
||
This patch may be subject to the crash in bug 551658.
Updated•14 years ago
|
Whiteboard: [c-n: after bug 551658 please!]
Comment 9•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
You need to log in
before you can comment on or make changes to this bug.
Description
•