Port |Bug 195702 - attachment size should be visible in compose window| (and bug 634620)

RESOLVED FIXED in seamonkey2.1b3

Status

enhancement
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)

Tracking

Trunk
seamonkey2.1b3
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Bug 26517 added the attachment size to the attachments list of existing messages. This is for adding the same for new messages (composition).

Note: This does not port |Bug 623346 - Show Total File Size of all attachments|, which I'd like to keep for another porting bug. Bug 623346 is already listed as a TB2SM dependency.

NB: It was hard to resist the temptation of reformatting everything. Too bad I'm not allowed to do that.
Attachment #514531 - Flags: review?(mnyromyr)
Comment on attachment 514531 [details] [diff] [review]
patch [Checkin: comment 2]

>       for (let i = 0; i < dataListLength; i++)
>       {
>         var item = dataList[i].first;
>         var prettyName;
>+        var size;
>         var rawData = item.data;

This is a bit weird (not your fault) - 'var' variables are always function-global in JS…
I'd prefer sub-scope definitions to use 'let' instead of 'var'.

>+            if (pieces.length > 2)
>+              size = parseInt(pieces[2]);

If - whyever - pieces[2] contains junk that begins with a number, parseInt will take for real and extract that number, which will usually be wrong. 

>+              if (size != undefined)
>+                attachment.size = size;

Comparing against undefined is plain ugly.

How about:
- initializing size to NaN
- using Number() instead parseInt(), because it'll give NaN for invalid numbers
- checking 'if (!isNaN(size))' finally before using it 

r/moa=me with that
Attachment #514531 - Flags: superreview+
Attachment #514531 - Flags: review?(mnyromyr)
Attachment #514531 - Flags: review+
Assignee

Comment 2

8 years ago
Comment on attachment 514531 [details] [diff] [review]
patch [Checkin: comment 2]

http://hg.mozilla.org/comm-central/rev/34dc4195f5fd
with comment 1 addressed (complete in-for-loop s/var/let/ as per IRC)
Attachment #514531 - Attachment description: patch → patch [Checkin: comment 2]
Assignee

Updated

8 years ago
No longer blocks: TB2SM
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b3
Assignee: nobody → jh
Whiteboard: [TB2SM]

Updated

7 years ago
Duplicate of this bug: 636185
You need to log in before you can comment on or make changes to this bug.