293995 - Download status repaints every second even when downloads are done

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[L. H.]

Doing a download with flash repaint on shows dowloads repaint every second even
when none of the downloads are active. The update should only happen for
downloads that are still going.

Comment 1

[Wevah (Nate Weaver)]

The downloads window isn't very efficient. It also seems to suck up unecessary
amounts of CPU when it's in the background.

Comment 2

[Wevah (Nate Weaver)]

Erm, I should say "when it's open".

Comment 3

[Torben]

Attachment: Completly untested patch

IIRC the icon for the downloads will be updated infintely until it's removed
from the download manager. The easy solution would probably be to add a check
for wheter the download is done and stop updating the icon then. I'll try to
get time to test how this works later today.

Comment 4

[Torben]

Attachment: Still untested, but at least it's the right file

Oops, attached the wrong file.

Comment 5

[Torben]

Attachment: Working patch

> The easy solution would probably be to add a check for wheter the
> download is done and stop updating the icon then.

That didn't work very well. Made changes to ProgressDlgController so it only
refreshes the download info for active downloads. This seems to work well.

Comment 6

[Josh Aas]

Attachment: Josh's patch, v1.0

I made this patch on an airplane, did't see the other guy's.

Mine sets a flag whenever a download never needs to be repainted again. That
solves the problem that this bug is about. Furthermore, my patch kills the
timer when no downloads are active, even if some are in the list.

Comment 7

[Josh Aas]

Comment on attachment 185843 [details] [diff] [review]
Josh's patch, v1.0

There is a typo in the patch I posted that will do bad things with the timer. I
probably should have said:

if ([self numDownloadsInProgress] == 0)

Comment 8

[Mike Pinkerton (not reading bugmail)]

yeah, was gonna say that about the == 0 issue. you going to post a new patch?

which patch do we want to use here? torben? how does yours compare to the one
josh put up?

Comment 9

[Josh Aas]

Here is what I think should happen after looking at both patches...

- use Torben's approach, but modify it to only examine visible items - no need
up update things in the list that aren't visible.
- also my fix for not running the timer all the time.

Comment 10

[Josh Aas]

Attachment: combination of the two patches, v3.0

This combines Torben's patch with mine for the best solution.

Comment 11

[Torben]

FWIW, I agree with Josh that the combined patch is the best.

Comment 12

[Mike Pinkerton (not reading bugmail)]

-  [mProgressViewControllers
makeObjectsPerformSelector:@selector(refreshDownloadInfo)];
+  for (unsigned i = 0; i < [mProgressViewControllers count]; i++) {
+    if ([[mProgressViewControllers objectAtIndex:i] isActive])
+      [[mProgressViewControllers objectAtIndex:i] refreshDownloadInfo];
+  }

why not just have |-refreshDownloadInfo| check if it's active before it redraws?
Then you can leave this code out. Seems like a normal thing for a refresh
routine to check regardless.

Comment 13

[Torben]

(In reply to comment #12)

> why not just have |-refreshDownloadInfo| check if it's active before it redraws?

AFAIK we need to do the last update after the download is finnished (and thus
not active):

+  // force final redraw on recently completed download
+  // then kill the timer if there are no active downloads
+  [(ProgressViewController*)progressDisplay refreshDownloadInfo];
+  if ([self numDownloadsInProgress] == 0)
+    [self killDownloadTimer];

Comment 14

[Simon Fraser [no longer active]]

You have to keep the timer alive even when there are no active downloads,
because we keep the file's icon (and maybe other stuff) up-to-date.

Comment 15

[Simon Fraser [no longer active]]

Comment on attachment 185861 [details] [diff] [review]
combination of the two patches, v3.0

This breaks updating of icons of downloaded files.

Comment 16

[Torben]

> This breaks updating of icons of downloaded files.

Why do we care about this?

(IIRC, we don't track moving of downloaded files, nor will deleted files be
removed (or grayed out) from the window.)

Comment 17

[Simon Fraser [no longer active]]

(In reply to comment #16)
> > This breaks updating of icons of downloaded files.
> 
> (IIRC, we don't track moving of downloaded files, nor will deleted files be
> removed (or grayed out) from the window.)

We do change the icon of a file that has disappeared to a generic icon, and
ideally we'd then disable the Show button (we don't now, which is a bug).

Comment 18

[Simon Fraser [no longer active]]

I made the text strings not redraw if they don't have to. It's harder to tell if
the image changed, so that still redraws (and we hit the disk every time to see
if the icon changed).

To be more performance here, we could maybe use file system notifications.

Comment 19

[Nick Kreeger]

Taking

Comment 20

[Nick Kreeger]

I have a patch that is part of a fix for bug 187619 (saving download instances).
We know only set the icon on start up, and when a download starts and ends. Upon
adding 150 downloads in the manager, Camino idles where its supposed to (near 0%
cpu load) Also when downloading mulitple downloads (3-4) while having those 150
in the manager, Camino's cpu load holds below 50%.

https://bugzilla.mozilla.org/show_bug.cgi?id=187619

Comment 21

[Mark Mentovai]

Fixed with bug 187619's checkin.

Comment 22

[Mark Mentovai]

Although the painting issue should be cleaned up, the timer might be running
when it's no longer needed.