Closed Bug 205938 Opened 22 years ago Closed 22 years ago

Clean up download rate in nsDownloadProgressListener.js

Categories

(SeaMonkey :: Download & File Handling, defect)

x86
All
defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: helgedt, Assigned: bugzilla)

Details

Attachments

(1 file, 1 obsolete file)

Calculation of download rate in kilobytes/sec in nsDownloadProgressListener.js presently uses unnecessary parseInt()s, divitions and modulations. Patch below replaces these with one .toFixed(1), and corrects indentation. toFixed() is as far as I can see also a better way to handle it because it can be localized (comments appreciated).
(should the use of toFixed be more obvious? like, kRate = kRate.toFixed(1);?)
Attachment #123506 - Flags: review?(jaggernaut)
Comment on attachment 123506 [details] [diff] [review] Cleans up rate-calculation + fixes indentation I think you may want to change this to: var kRate = (rate / 1024).toFixed(1); In the old code, e.g. rate = 1024 and rate = 1025, kRate for both would be 10 for the check (kRate != this.priorRate), and thus considered as no (visible) change ("Don't update too often!"). With your current patch, kRate becomes 1 and 1.0009765625 respectively, meaning it now thinks there's a visible change, even though later on in replaceInsert it turns out there isn't. That said, the code you're patching seems broken. Currently there seems to be no point to check if there's no visual change, other than to avoid some simple calculations. If however speedcol.setAttribute("label", rateMsg) were to be made dependent on (kRate != this.priorRate), the code would make a little more sense since you'd avoid doing some layout/reflow work if visibly nothing would change. E.g. var timeRemainingCol = elt.nextSibling.nextSibling.nextSibling; var statusCol = timeRemainingCol.nextSibling; var speedCol = statusCol.nextSibling; var kRate = (rate / 1024).toFixed(1); if (!kRate) kRate = "??.?"; if (kRate != this.priorRate) { rateMsg = replaceInsert(rateMsg, 1, kRate); speedCol.setAttribute("label", kRate); this.priorRate = kRate; } Leaving out rateChanges here since rateChangesLimit is always 0. Of course, the above code is only needed if we feel there are performance problems with the download manager window. Otherwise this code could be simplified to just always set rateMsg like it's doing now (no check against this.priorRate). Perhaps another bug is in order to fix that code. For now, just move the .toFixed(1) up.
Attachment #123506 - Flags: review?(jaggernaut) → review-
My first version actually used toFixed() at the top. Then I noticed it would be comparing strings instead of floats (kRate != this.priorRate). But I see your point.
Attachment #123506 - Attachment is obsolete: true
Attachment #123521 - Flags: review?(jaggernaut)
Comment on attachment 123521 [details] [diff] [review] Patch v0.2, moves .toFixed(1) up by jag's request. r/sr=jag
Attachment #123521 - Flags: review?(jaggernaut) → superreview+
Attachment #123521 - Flags: review?(blaker)
4 months, and we're still waiting on review for this bug? You've got to be kidding me.
Attachment #123521 - Flags: review?(blake) → review+
Fix checked in.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: