462064 - Download manager shows decimal digit for bytes when file below 100 bytes

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Laurence "GreenReaper" Parry]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081027 Minefield/3.1b2pre (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081027 Minefield/3.1b2pre (.NET CLR 3.5.30729)

If a file is very small, the Download Manager will show a decimal point - e.g. "14.0 bytes" (as opposed to, say, "345 bytes"). This is not correct, as there is no such thing as a point of a byte - it is indivisible (as far as the Download Manager is concerned, anyway).

Reproducible: Always

Steps to Reproduce:
1. Download a 14 byte file with the Download Manager enabled
Actual Results:  
Download Manager shows "14.0 bytes"

Expected Results:  
Download Manager shows "14 bytes"

Comment 1

[Matthias Versen [:Matti]]

14.5 would be 14 Bytes and 4 Bits :-)

Comment 2

[Laurence "GreenReaper" Parry]

If you get 14.5 bytes from an integer counter, there are bigger fish to fry . . .

Comment 3

[Marius Hudea]

confirmed here also, it shows 4.0 bytes for a 4 byte file (Firefox 3.0.5).

Comment 4

[Jim Porter (:squib)]

For someone who wants to fix this, the relevant code is (I think) here: http://mxr.mozilla.org/mozilla/source/toolkit/mozapps/downloads/src/DownloadUtils.jsm#382

I think you just need to add (unitIndex != 0) to line 395. I'd do it, but I don't have FF source builds set up to test it out. See also bug 516787 which creates a C++ version of this function for Thunderbird.

Comment 5

[Florian J. [:FeuerFliege]]

You’re right. It’s just a little change to DownloadUtils.jsm:
I’ve just added “&& (unitIndex != 0)“ and it works as you predicted.

Change:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/DownloadUtils.jsm#407
to 
aBytes = aBytes.toFixed((aBytes > 0) && (aBytes < 100) && (unitIndex != 0) ? 1 : 0);

Comment 6

[Abhishek]

I am able to reproduce the bug on windows7 ff4b9. 

But the question is, should this be considered a bug. For a file of size 1118 bytes, in windows7 it shows as 1.09KB and download manager of ff shows 1.1KB. Or should I also file a bug in windows7? :)

Comment 7

[Marius Hudea]

Windows 7 is correct.

Hard disk sizes use the notation 1 KB = 1000 bytes. 

Software however uses multiples of 2, which is because the minimum space allocation possible on disk are usually 512 bytes (disk sectors), 4KB (clusters)  therefore 1 KB = 1024 bytes, so 1118 bytes / 1024 = 1.09 KB.

This difference in units of measures is also why 1 TB drives actually have approximately 931 GB when formatted. Disk manufacturers say 1 TB = 1000 MB x 1000 x 1000 while software says 1 TB = 1024MB x 1024 x 1024.

If it shows 1.1 KB in Firefox I'd argue it's incorrect, because division by 1000 would just confuse people and it doesn't seem to be rounding up error (result is 1.091 so it couldn't be rounded to 1.1)

Comment 8

[Jim Porter (:squib)]

(In reply to comment #7)
> (result is 1.091 so it couldn't be rounded to 1.1)

It absolutely can be rounded from 1.091 to 1.1. Example: http://codepad.org/I7C0lr46

Comment 9

[Florian J. [:FeuerFliege]]

This bug is not about rounding, but about showing a decimal point for files smaller than 100 bytes.
This bug can be solved by the change Jim suggested in comment #4.

Furthermore I can not find a rounding bug. Firefox uses the factor 1024 and shows just one position after the decimal point. 1118byte is both 1.09 KB (with two positions after the decimal point) and 1.1 KB (with just one position after the decimal point)

Comment 10

[Anas Husseini [:linostar]]

Attachment: Patch including the change described in comment 5

The patch was tested on ff4b9 and had given the desired result (no decimal digit appears when the file size is in bytes).

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 505042 [details] [diff] [review]
Patch including the change described in comment 5

Edward can do a first pass here.

Comment 12

[Ed Lee :Mardak]

Comment on attachment 505042 [details] [diff] [review]
Patch including the change described in comment 5

You'll need to update the tests here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/unit/test_DownloadUtils.js#101

Notice that it's explicitly checking for a decimal point for sub 100 bytes.

102   testConvertByteUnits(1, _("1.0"), "bytes");
103   testConvertByteUnits(42, _("42.0"), "bytes");
104   testConvertByteUnits(123, _("123"), "bytes");

Comment 13

[Ed Lee :Mardak]

Also, would probably be good to update the comment for the extra logic.

Comment 14

[Anas Husseini [:linostar]]

Attachment: patch v2 including the changes made to the tests

Comment 15

[Mounir Lamouri (:mounir)]

Sent to try, see:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=339b93d96e2a

There is still one failing test:
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/mozapps/downloads/tests/unit/test_DownloadUtils.js | 25 seconds remaining  22.1 of 22.1 MB (58 bytes/sec) == 25 seconds remaining  22.1 of 22.1 MB (58.0 bytes/sec)

Comment 16

[Ed Lee :Mardak]

Comment on attachment 505384 [details] [diff] [review]
patch v2 including the changes made to the tests

sdwilsh can do the second pass! ;) The style-master!

Turns out there's 6 instances of "58.0 bytes/sec" in that test, so those need to be fixed as well.

Comment 17

[Shawn Wilsher :sdwilsh]

Comment on attachment 505384 [details] [diff] [review]
patch v2 including the changes made to the tests

I'll be happy to do the review once the tests pass!

Comment 18

[Anas Husseini [:linostar]]

Attachment: patch v3 (fixing all the concerned decimal points in the test)

Comment 19

[Shawn Wilsher :sdwilsh]

Comment on attachment 508390 [details] [diff] [review]
patch v3 (fixing all the concerned decimal points in the test)

r=sdwilsh

Comment 20

[Mounir Lamouri (:mounir)]

Just sent this to the try server:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=5ab3ae478c2d

Comment 21

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/025ec6dfef03

Thank you for this patch Anas :)