Closed
Bug 468335
Opened 16 years ago
Closed 15 years ago
AddFileAttachment and AddUrlAttachment functions
Categories
(Thunderbird :: Message Compose Window, defect)
Thunderbird
Message Compose Window
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b2
People
(Reporter: mkmelin, Assigned: mkmelin)
References
Details
Attachments
(1 file, 3 obsolete files)
13.34 KB,
patch
|
Details | Diff | Splinter Review |
Follow up to bug 331387 comment 9. Create AddFileAttachment and AddUrlAttachment functions... and some minor cleanup.
Attachment #351785 -
Flags: review?(jminta)
Assignee | ||
Comment 1•16 years ago
|
||
Assignee | ||
Comment 2•16 years ago
|
||
jminta: r? ping
Updated•16 years ago
|
Attachment #351785 -
Flags: review?(jminta) → review-
Comment 3•16 years ago
|
||
Comment on attachment 351785 [details] [diff] [review] proposed fix + let haveSetAttachDirectory = false; + while (attachments.hasMoreElements()) + { + let currentFile = attachments.getNext().QueryInterface(Components.interfaces.nsILocalFile); + if (!haveSetAttachDirectory) + { + SetLastAttachDirectory(currentFile); + haveSetAttachDirectory = true; + } + AddFileAttachment(currentFile); This loop seems weird/inefficient. How about saving a variable/loop time with: let attachment = attachments.getNext().QueryInterface(Components.interfaces.nsILocalFile); SetLastAttachDirectory(attachment); while (true) { AddFileAttachment(attachment); if (!attachments.hasMoreElements()) break; attachment = attachments.getNext().QueryInterface(Components.interfaces.nsILocalFile); } +/** Add a file object as attachment. */ +function AddFileAttachment(file) +{ This needs more complete documentation. I had to look for a bit to figure out what |file| actually was. You should also mention that this is mostly just a helper to wrap files into urls. + if (!DuplicateFileCheck(currentAttachmentURL)) + { + let attachment = Components.classes["@mozilla.org/messengercompose/attachment;1"] + .createInstance(Components.interfaces.nsIMsgAttachment); Return early if the duplicate check fails, so that you don't need to nest all this stuff in a block. +/** Add an attachment object as attachment. The attachment URL must be set. */ +function AddUrlAttachment(attachment) Again, you should further spell out that |attachment| is a nsIMsgAttachment object (since AddUrlAttachment makes me expect an nsIURL). Also, why aren't we doing duplicate checking/bucket visibility here? At least note that in the documentation, but it might be better to move the duplicate checking out of AddFileAttachment and into AddUrlAttachment. + item.attachment = attachment;// Full attachment object stored here. Space between ; and / + var url = gIOService.newURI(attachment.url, null, null); + try { + url = url.QueryInterface(Components.interfaces.nsIURL); + } + catch (ex) { + url = null; + } This should use instanceof, and an if block. Something like if (url instanceof Ci.nsIURL) { // do icon stuff } Sorry for the delay here. Would be happy to review another patch with additional documentation and the duplicate/visibility stuff sorted out.
Assignee | ||
Comment 4•16 years ago
|
||
Review comments addressed.
Attachment #351785 -
Attachment is obsolete: true
Attachment #351786 -
Attachment is obsolete: true
Attachment #354211 -
Flags: review?(jminta)
Comment 5•15 years ago
|
||
Comment on attachment 354211 [details] [diff] [review] proposed fix, v2 + ChangeAttachmentBucketVisibility(false); + gContentChanged = true; I'm pretty sure these need to move into AddURLAttachment too. (Adding a duplicate attachment shouldn't result in the content-changed flag getting set.) + else + { + // For security reasons, don't allow mail protocol uris to leak out. + // We don't want to reveal the .slt path (for mailbox://), or the username + // or hostname. + if (/^file:|^mailbox:|^imap:|^s?news:/i.test(attachment.name)) + attachment.name = bundle.getString("partAttachmentSafeName"); + } You can combine this into an else-if. - AddAttachment(attachment); + AddUrlAttachment(attachment); ChangeAttachmentBucketVisibility(false); If you make the above change about moving the visibility and content-changed stuff, these calls can drop-out. + * Check if the given fileURL already exists in the attachment bucket. + * @param the URL of the file to check You need to specify the param-name, and also please note that the url here is a string (not a nsIURL). + if (DuplicateFileAlreadyAttached(rawData)) Trailing whitespace. r=jminta with that. Bonus points for a mozmill test exercising some of these functions.
Attachment #354211 -
Flags: review?(jminta) → review+
Assignee | ||
Comment 6•15 years ago
|
||
Attachment #354211 -
Attachment is obsolete: true
Assignee | ||
Comment 7•15 years ago
|
||
changeset: 1527:ce561724ed95 http://hg.mozilla.org/comm-central/rev/ce561724ed95
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b2
You need to log in
before you can comment on or make changes to this bug.
Description
•