493168 - Wasting CPU cycles with refreshing download info for non-active downloads

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[Ilya Sherman]

Attachment: Patch

AFAICT, the timer we use to refresh download info in |ProgressDlgController| is really just meant to be used to animate downloads-in-progress.  Currently, the timer keeps calling |refreshDownloadInfo| on every single download, even when there are no active downloads remaining.  With a bunch of downloads in progress, this can have a noticeable impact on CPU use.

I'm not sure if this is best place to add |killTimer|, but given what I know about how the timer works, it seems likely to be an o.k. place to do it.  The other reasonable option I can think of is to check for how many active downloads remain each time |didEndDownload| is called.

Comment 2

[Stuart Morgan]

Not a blocker for b3, but marking 2.0

Comment 3

[Stuart Morgan]

Comment on attachment 377629 [details] [diff] [review]
Patch

Makes sense, and all the cases other than progress that I can think of (completion, file is moved away, pause/resume) already have explicit calls to refreshDownloadInfo. I agree this is a reasonable place to kill the timer.

One thing though: I believe that a paused download is considered "active", which means if there's a paused download in the list it'll keep getting refreshed and the timer won't go away. I believe that you also want to check ![curController isPaused] in your look, and then have a call in resume: to setupDownloadTimer. That would need testing though; I'm just going by code here.

r=me, with or without that change depending on what you find.

Comment 4

[Ilya Sherman]

Attachment: Patch v2

Also attends to paused downloads

Comment 5

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 378978 [details] [diff] [review]
Patch v2

sr=pink

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on cvs trunk.