67214 - parseInt() used instead of Math.round() or Math.floor() in embedding/components/ui/progressDlg/nsProgressDlg.js

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Boris Zbarsky [:bzbarsky]]

parseInt() is used all over the place in
xpfe/components/ucth/resources/helperAppDldProgress.js.  This is a function that
takes a string and converts it to an integer, but it's being applied to numeric
arguments!  I am guessing this ends up doing a number->string conversion and
then a string->integer conversion.  In most cases, we are doing
parseInt(foo+0.5), which is exactly equivalent to Math.round(foo), for positive foo.

Since all the values in question are positive (times, completion percentages,
amounts downloaded, and such), it might make sense to use Math.round().

Comment 1

[Jo Hermans]

I stumpled on this bug today. This seems to be a good candidate to speed up
progress-dialogs, like the one used in downloading.

The original file doesn't exists anymore, but the code seems to be moved to
/embedding/components/ui/progressDlg/nsProgressDlg.js

Comment 2

[Steve Chapel]

Attachment: fix

Comment 3

[Sebastian Späth]

Attachment: time parseInt, round and floor

Quick test case reveals that round and floor are more than twice as fast than
parseInt. This difference might not be noticable when only done once, but when
being called repeatedly it could show up.

50000 parseInt(2) take: 1010
50000 floor(2) take: 412
50000 round(2) take: 403
50000 parseInt(2.7) take: 1010
50000 floor(2.7) take: 403
50000 round(2.7) take: 415

Comment 4

[Sebastian Späth]

Attachment: supplemental fix for hte Progess listener

This patch fixes the parseInt's in the progress listener of the download
manager. I have run mozilla it and it works fine for me.

Comment 5

[Sebastian Späth]

Attachment: v.02

This changes rounds that really need to be floors to actually be floors. Ready
fro review and checkin now

Comment 6

[Stephen Walker]

Comment on attachment 118751 [details] [diff] [review]
v.02

Switching reviewer & adding super-reviewer in hopes of getting this fixed since
bug 199655 was fixed within a month.

Comment 7

[jag (Peter Annema)]

Comment on attachment 118699 [details] [diff] [review]
fix

r=jag

Comment 8

[Steve Chapel]

The v.02 patch has a Mat.round that should be Math.round.

Comment 9

[timeless]

Comment on attachment 118699 [details] [diff] [review]
fix

checked in

Comment 10

[Steve Chapel]

This is a low risk patch that improves performance of the download dialog. I've
used the patch that was just checked into the trunk for months without any
problems or JS warnings, and it deserves to go into the 1.4 branch.

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 118699 [details] [diff] [review]
fix

moving approval request forward.

Comment 12

[Steve Chapel]

This was checked in months ago.

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

changing summary to reflect what file the patch really touched