Camino hangs when doing "Clean Up" on a big list of downloads

RESOLVED FIXED

Status

Camino Graveyard
Downloading
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Håkan Waara, Assigned: Stuart Morgan)

Tracking

({fixed1.8.1, hang})

Trunk
PowerPC
Mac OS X
fixed1.8.1, hang

Details

Attachments

(1 attachment)

6.87 KB, patch
Nick Kreeger
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
Trunk, 20061009

I had quite a big list of downloads since I almost never use "Clean Up" to remove them.

Camino hung up for a good 8-10 seconds... We should really do this on a thread (using a simple progress dialog, perhaps reusing the one we now have for importing of bookmarks?)
(Reporter)

Comment 1

11 years ago
Also, profiling what makes removing the completed download so slow would help. Maybe there's some simple optimization to be done.

Why aren't we just removing downloads.plist all at once, anyway?
(Assignee)

Comment 2

11 years ago
We definitely shouldn't make a new thread and a progress indicator, because there's no reason for it to take anything like that long.

We can't remove the whole plist because it doesn't remove in-progress downloads.  I haven't profiled, but glancing at the code making this way faster should be very easy; the cleanup is a looped call to the single-item remove, which does a complete re-layout of all the remaining views.

Taking.
Assignee: nobody → stuart.morgan
(Assignee)

Comment 3

11 years ago
Created attachment 241713 [details] [diff] [review]
suppress duplicate re-layouts

This should help quite a bit.
Attachment #241713 - Flags: review?(nick.kreeger)

Comment 4

11 years ago
Comment on attachment 241713 [details] [diff] [review]
suppress duplicate re-layouts

 // remove all inactive instances
 -(IBAction)cleanUpDownloads:(id)sender
 {
-  for (unsigned int i = 0; i < [mProgressViewControllers count]; i++)
+  for (int i = [mProgressViewControllers count] - 1; i >= 0; i--)
   {
     ProgressViewController* curProgressViewController = [mProgressViewControllers objectAtIndex:i];
     if ((![curProgressViewController isActive]) || [curProgressViewController isCanceled])
-    {
-      [self removeDownload:curProgressViewController]; // remove the download
-      i--; // leave index at the same position because the dl there got removed
-    }
+      [self removeDownload:curProgressViewController suppressRedraw:YES];

Looks like you have a tab here.

Looks good to me.
Attachment #241713 - Flags: review?(nick.kreeger) → review+
(Assignee)

Comment 5

11 years ago
Comment on attachment 241713 [details] [diff] [review]
suppress duplicate re-layouts

No tab; that's the right level of indentation for the |if| that it follows.
Attachment #241713 - Flags: superreview?(mikepinkerton)
Comment on attachment 241713 [details] [diff] [review]
suppress duplicate re-layouts

sr=pink
Attachment #241713 - Flags: superreview?(mikepinkerton) → superreview+

Comment 7

11 years ago
Checked in on 1.8branch and trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.