only showing 7 days of download history, until I search, then I see more items steps to reproduce: 1) open the download manager, i think you only see the downloads from the past 7 days 2) type in a search term, like "e" and I see a whole lot more. this seems problematic, because if I am trying to cover my download tracks, on first glance, what I want to remove appears gone.

I think the last item in the completed section should be "show all download history," which loads the Places Organizer.

I filed bug 392362 so we can at least test in the meantime...

I think the more important question is: why are we only showing the last 7 days of downloads? Historically there were perf issues, but I thought that gets fixed with the new DM?

There still perf issues because you have to create lots of DOM nodes for each download. It isn't as bad as it once was mind you, but part of the perf win was the fact that we don't show everything by default.

Adding madhava and blocking on a UI decision, there's been some ux team discussion about how useful it is to always show all download history, and he's been tasked with coming up with some recommendations.

until we have a decision from the UX team, we can't do anything. Targeting M10.

This bug appears to become invalid/wontfix as per bug 397655.

Bug 399783 will remove the search box, so this bug would become invalid.

In any case of informing the user of "# downloads not displayed" or "show all # downloads" button.. What action would we provide to the user? Showing all the downloads could potentially be a huge list with performance/display issues -- probably why the 'advanced search' or places button/link was suggested.

This is resolved by a revised design - mockup posted in bug 397655.

(In reply to bug 397655comment #23) > One design detail that is not visible from the mockup is that, given that the > DM is again the only place to view download history, we should list all the > download as far back as we have history -- ie., no cut-off at the beginning of > the session or 7 days So, I'm not sure we want to do that. Creating a large number of download entries really sucks perf wise...

I was thinking about lazily creating download items or lazily querying with a unified query that grabs both active and done downloads in one pass. I'm assuming the slowdown is from updating the UI, so we could also keep around an array/dictionary of downloads and cherry pick the items we want to show..

!!! Major heart surgery !!! ... :p - Unified query that replaces 1) active list 2) done list 3) time query 4) search query - Incremental adding of download items with timeouts - Prepend new downloads instead of rebuilding the whole list - Rework evenOddCellAttribution -> stripeifyList to only color things necessary - Various searchbox refactoring Unified query grabs active and searched items at the same time and leverages '%%' to select all. This removes many code paths to clean up some inconsistencies and makes it easier to incrementally build the list. Incremental just grabs the next item in the statement and adds it and sets up the next builder. Potentially we could display in groups instead of one by one, even on a debug build, this looks fine.. just takes a while to populate. (User sees a shrinking scrollbar.) [I'm using appendChild, but it looks like the overhead is dominated by the setTimeout. So we could definitely make it go faster by showing say.. 10 items per "step".] New downloads now get tacked on the front instead of killing the whole list. This reintroduces the other createDownloadItem path... but oh well. (Double and triple checked that target is file, name is target, source is uri, etc :() stripeifyList(aItem) only colors from the item to the end of the list instead of recoloring the whollleee list. So appending items and coloring only causes that one item to be colored. A new gSearchBox and a gSearchTerms which we could use to keep just-finished downloads that /do/ match the search term instead of just removing them.

This version first adds 1 item then adds them 5 at a time. On the debug build it went from ~45 seconds (1 by 1) to ~30 seconds (5s) to show all 275 downloads. Using a non-debug build of v1 (adds 1 by 1).. https://build.mozilla.org/tryserver-builds/2007-12-03_23:46-edward.lee@engineering.uiuc.edu-fastdlmgr/ It took about 30 seconds to populate all items on the first open. The second open has the window appearing much faster instead of pausing 5 seconds or so. And took perhaps 20-25 seconds to fill in the list. Because there's an initial delay of opening the first time, I'm pretty sure it's related to initializing the backend download manager service and not so much to showing the window and adding items. Potentially we could address bug 399838 by setTimeout(start dlmgr service, 10000). The chunked adding from v1.1 would definitely give benefits as well.. choosing a good number is tricky on just debug builds though. 5? 10? 50?

There's a build going with patch v1.1 but chunk of 50, so it should show up on the tryserver soon as "chunk50dlmgr".

Wow. My iBook is slow :( :p hehe https://build.mozilla.org/tryserver-builds/2007-12-04_01:00-edward.lee@engineering.uiuc.edu-chunk50dlmgr/ Chunking by 50, initially showing 1 has the window appearing immediately.. at least for successive loads. There is some UI pause as it adds 50 items, but that's on my iBook. But even then, displaying 278 downloads takes 15 seconds with this method vs ~30 from before. I've tested the fastdlmgr (1 by 1) on windows now that the build finished and for 233 downloads, it takes 5 seconds, so the chunking by 50 might get down to 2-3 seconds. iBook being 1.33GHz powerpc G4 1.5GB and.. windows machine 2.6GHz pentium 4 hyperthread 1GB (wow.. I never realized I had more memory on my ibook..) Download manager opens instantly with 1by1 on windows as well.

Chunk by 10: https://build.mozilla.org/tryserver-builds/2007-12-04_09:40-edward.lee@engineering.uiuc.edu-chunk10dlmgr/ Again takes about 15 seconds for 275 items on my iBook similar to chunking by 50, but there's less of a delay to fill up the initial window.

Initially show 1 and chunk by 10. Quick enough for the window to appear and fill up the whitespace while getting the performance of multiple appendChilds. Also, don't bother setting the referrer because it'll always be null per bug 406923.

Whoops. We'll need to back out bug 392670 for thunderbird or unpatch it because we're removing browser.download.manager.displayedHistoryDays. And s/gSearchbox/gSearchBox/ Yay tryserver dogfooding :p https://build.mozilla.org/tryserver-builds/2007-12-04_22:14-edward.lee@engineering.uiuc.edu-aw.bar.dl.mgr/ has this latest patch (1.3) for people to compare the download manager window loading speed.. with some other changes like download manager ui and adaptive (Bug 392097 Bug 405884 Bug 406230 Bug 394516 Bug 406857 Bug 405893 Bug 405888 Bug 405886 Bug 406923 Bug 395735 Bug 406359 Bug 406358 Bug 395739 Bug 406422 Bug 406425 Bug 406427)

This fixes bug 393190 with the unified code path always selecting a download when building the list. (Original code didn't select a value after searching, so it remained on a removed item.)

Comment on attachment 291621 [details] [diff] [review] v1.3 >-var gUserInterfered = false; no again :p >+// The statement to query for downloads that are active or match the search >+let gStmt = gDownloadManager.DBConnection.createStatement( >+ "SELECT id, target, name, source, state, startTime, endTime, referrer, " + >+ "currBytes, maxBytes, state IN (?1, ?2, ?3, ?4, ?5) isActive " + >+ "FROM moz_downloads " + >+ "WHERE isActive OR name LIKE ?6 ESCAPE '/' " + >+ "ORDER BY isActive DESC, endTime DESC, startTime DESC"); What's the point of using gStmt for both searching *and* normal use? LIKE in SQL is pretty darn slow, so using it should be avoided if possible. >+function buildDownloadList() >+ // Clear the list before adding items >+ while (gDownloadsView.hasChildNodes()) >+ gDownloadsView.removeChild(gDownloadsView.firstChild); So, I strongly suspect it'd be faster if we do a cloneNode(false) and then replace the existing one with the clone. >+/** >+ * Incrementally build the download list by adding one item at a time one isn't accurate since it's an argument :p >+ let id = gStmt.getInt64(0); >+ let file = gStmt.getString(1); >+ let target = gStmt.getString(2); >+ let uri = gStmt.getString(3); >+ let state = gStmt.getInt32(4); >+ let start = Math.round(gStmt.getInt64(5) / 1000); >+ let end = Math.round(gStmt.getInt64(6) / 1000); >+ let referrer = gStmt.getString(7); >+ let currBytes = gStmt.getInt64(8); >+ let maxBytes = gStmt.getInt64(9); >+ let isActive = gStmt.getInt32(10); >+ >+ let progress = 100; >+ // Grab the real progress only if the download is active. >+ if (isActive) >+ progress = gDownloadManager.getDownload(id).percentComplete; >+ >+ let item = createDownloadItem(id, file, target, uri, state, progress, >+ start, end, referrer, currBytes, maxBytes); >+ if (item) { >+ // Add item to the end and color just that one item >+ gDownloadsView.appendChild(item); >+ stripeifyList(item); >+ } >+ >+ // Add another item to the list if we should; otherwise, let the UI update >+ // and continue later >+ if (aNumItems > 1) >+ stepListBuilder(aNumItems - 1); >+ else >+ gBuilder = setTimeout(stepListBuilder, 0, 10); This should all be in a try-catch block. Pretty much getting any kind of results from storage should be, and shame on me for not doing it right in the first place. I need to get docs up on how to do storage stuff right still :/ I'd also like two constants added and used: gListBuildDelay - the time in milliseconds before building more of the list gListBuildChunk - the size of the chunk to build between each delay >+} >+ >+/** >+ * Add a download to the front of the download list >+ * >+ * @param aDownload >+ * The download manager download to make into a richlist item nit: the nsIDownload - download manager download is pretty vague >+function stripeifyList(aItem) ah, this was hard to get my head around - nice though :) I'm starting to think that maybe createDownloadItem should just take an object that has all the properties we care about on it to make the function call to it a little bit more sane :/ Otherwise, this patch looks pretty good.

(In reply to comment #21) > Whoops. We'll need to back out bug 392670 for thunderbird or unpatch it because > we're removing browser.download.manager.displayedHistoryDays. Please file a follow-up.

(In reply to comment #23) > >+// The statement to query for downloads that are active or match the search > >+let gStmt = gDownloadManager.DBConnection.createStatement( > What's the point of using gStmt for both searching *and* normal use? LIKE in > SQL is pretty darn slow, so using it should be avoided if possible. This goes pretty fast even on my iBook and it's doing a LIKE %%, so it probably short circuits. The bottleneck isn't even here anyway -- it's displaying the results. It simplifies the code and makes sure we don't forget to do something for the other path. Combining it actually fixed at least one bug (bug 393190) > >+function buildDownloadList() > >+ // Clear the list before adding items > >+ while (gDownloadsView.hasChildNodes()) > >+ gDownloadsView.removeChild(gDownloadsView.firstChild); > So, I strongly suspect it'd be faster if we do a cloneNode(false) and then > replace the existing one with the clone. Instead of creating a node? Or are you talking about removing items? I did some performance tests with removeChild(first) and removeChild(last) and some reason the latter ran /much/ slower.

I mean replace the while() loop with this: let elm = gDownloadsView.cloneNode(false); gDownloadsView.parentNode.replaceChild(elm, gDownloadsView); gDownloadsView = elm;

(In reply to comment #25) > it's doing a LIKE %%, so it probably short circuits For the record.. http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/storage/src/mozStorageUnicodeFunctions.cpp&rev=1.3&mark=111,117-135#111 :) The pattern skips the leading %s and ends up with nothing else to match, so it returns 1.

(In reply to comment #23) > (From update of attachment 291621 [details] [diff] [review]) > >-var gUserInterfered = false; > no again :p What is this used for? http://mxr.mozilla.org/mozilla/ident?i=gUserInterfered There's only one def of it ever in the whole codebase. We don't even use it to do stuff.. ?? In the case of gUserInteracted which was set when clicking the info button.. http://mxr.mozilla.org/mozilla/ident?i=gUserInteracted When I get rid of the info button, there will be no more defs of gUserInteracted outside of the initial def of false, so the single use of it !gUserInteracted is always true, so just remove it... ??

(In reply to comment #23) > (From update of attachment 291621 [details] [diff] [review]) > >-var gUserInterfered = false; > no again :p Leaving in for now... > >+function buildDownloadList() > >+ // Clear the list before adding items > >+ while (gDownloadsView.hasChildNodes()) > >+ gDownloadsView.removeChild(gDownloadsView.firstChild); > So, I strongly suspect it'd be faster if we do a cloneNode(false) and then > replace the existing one with the clone. Replaced with the shallow copy. This will be much faster than iterating and removing. > >+ * Incrementally build the download list by adding one item at a time > one isn't accurate since it's an argument :p Expanded the comment. > >+ let id = gStmt.getInt64(0); ... > This should all be in a try-catch block. Into a try-catch. The recursive calls are still outside. > >+ gBuilder = setTimeout(stepListBuilder, 0, 10); > I'd also like two constants added and used: > gListBuildDelay - the time in milliseconds before building more of the list > gListBuildChunk - the size of the chunk to build between each delay Added them to the top and commented. > >+ * @param aDownload > >+ * The download manager download to make into a richlist item > nit: the nsIDownload - download manager download is pretty vague Fixed. > >+function stripeifyList(aItem) > ah, this was hard to get my head around - nice though :) :) > I'm starting to think that maybe createDownloadItem should just take an object > that has all the properties we care about on it to make the function call to it > a little bit more sane :/ Fixed createDownloadItem and converted both call sites of stepListBuilder and prependList.

(In reply to comment #28) > There's only one def of it ever in the whole codebase. We don't even use it to > do stuff.. ?? Yeah, there's a bug about getting it to work right. I just need the time to fix it... I'm not sure if the try-catch as you described it is right either, but I'll comment on that when I review.

Also, instead of just calling the function with n-1, it might provide a more responsive UI to do a setTimeout call of 0 (testing needed).

The reason for not setTimeout is so that items are batched together with 0 delay. setTimeout of 0 still counts as 10ms, so 100 items will delay around 1000ms. I talked with mconnor and gavin, and we'll delay based Math.min(numElements, 100) where I suppose 100 would be in gListBuildMaxDelay.

Variable delay for the first 100 downloads (first 10 chunks)

Comment on attachment 293317 [details] [diff] [review] v2.1 >+let gListBuildDelay = 100; >+let gListBuildChunk = 10; s/let/const/ >+ // If we aren't displaying search results, move the download to after the >+ // active ones >+ if (gSearchTerms.length == 0) { >+ // Iterate down until we find a non-active download >+ let next = dl.nextSibling; >+ while (next && next.inProgress) >+ next = next.nextSibling; >+ >+ // Move the item and color everything after where it moved from >+ let fixup = dl.nextSibling; >+ gDownloadsView.insertBefore(dl, next); >+ stripeifyList(fixup); >+ } else { > removeFromView(dl); >+ } OK, this is an edge case, but what if the download in question would be displayed in the search results? I'd be OK with a follow-up bug for this. >+ clearTimeout(gBuilder); >+ gStmt.finalize(); You may want to reset the statement here as well before finalizing it. >+function stepListBuilder(aNumItems) { >+ // Don't do work if there's nothing to add >+ if (!gStmt.executeStep()) >+ return; > >+ try { >+ // Try to get the attribute values from the statement >+ let attrs = { >+ dlid: gStmt.getInt64(0), >+ file: gStmt.getString(1), >+ target: gStmt.getString(2), >+ uri: gStmt.getString(3), >+ state: gStmt.getInt32(4), >+ startTime: Math.round(gStmt.getInt64(5) / 1000), >+ endTime: Math.round(gStmt.getInt64(6) / 1000), >+ currBytes: gStmt.getInt64(8), >+ maxBytes: gStmt.getInt64(9) >+ }; > >+ // Only add the referrer if it's not null >+ let (referrer = gStmt.getString(7)) { >+ if (referrer) >+ attrs.referrer = referrer; >+ } >+ >+ // If the download is active, grab the real progress, otherwise default 100 >+ attrs.progress = gStmt.getInt32(10) ? >+ gDownloadManager.getDownload(attrs.dlid).percentComplete : 100; >+ >+ // Make the item and add it to the end >+ let item = createDownloadItem(attrs); >+ if (item) { >+ // Add item to the end and color just that one item >+ gDownloadsView.appendChild(item); >+ stripeifyList(item); >+ } >+ } catch (e) { } >+ >+ // Add another item to the list if we should; otherwise, let the UI update >+ // and continue later >+ if (aNumItems > 1) { >+ stepListBuilder(aNumItems - 1); >+ } else { >+ // Use a shorter delay for earlier downloads to display them faster >+ let delay = Math.min(gDownloadsView.childNodes.length, gListBuildDelay); >+ gBuilder = setTimeout(stepListBuilder, delay, gListBuildChunk); > } > } So, the try-catch needs to also get executeStep, and if you catch anything, you should reset the statement. >- try { >- stmt.bindInt32Parameter(0, nsIDM.DOWNLOAD_NOTSTARTED); >- stmt.bindInt32Parameter(1, nsIDM.DOWNLOAD_DOWNLOADING); >- stmt.bindInt32Parameter(2, nsIDM.DOWNLOAD_PAUSED); >- stmt.bindInt32Parameter(3, nsIDM.DOWNLOAD_QUEUED); >- stmt.bindInt32Parameter(4, nsIDM.DOWNLOAD_SCANNING); >- buildDownloadList(stmt, gDownloadsActiveArea); >- } finally { >- stmt.reset(); > } You need to wrap your code like this earlier. I forgot to make the comment, and I've already deleted it from this comment box... r=sdwilsh with comments fixed.

Rerequesting r? to double check the try/reset stuff (In reply to comment #34) > (From update of attachment 293317 [details] [diff] [review]) > >+let gListBuildDelay = 100; > >+let gListBuildChunk = 10; > s/let/const/ Const-ified. > > removeFromView(dl); > OK, this is an edge case, but what if the download in question would be > displayed in the search results? I was just keeping par with the existing implementation. Filed bug 408503. > >+ clearTimeout(gBuilder); > >+ gStmt.finalize(); > You may want to reset the statement here as well before finalizing it. Added reset. > >+function stepListBuilder(aNumItems) { > >+ // Don't do work if there's nothing to add > >+ if (!gStmt.executeStep()) > >+ return; > So, the try-catch needs to also get executeStep, and if you catch anything, you > should reset the statement. 1) Added reset for having nothing left to do. 2) Added try/catch+return reset if failing to execute step. 3) Added return reset if failing to get. > >- try { > >- stmt.bindInt32Parameter(0, nsIDM.DOWNLOAD_NOTSTARTED); .. > >- } finally { > >- stmt.reset(); > > } > You need to wrap your code like this earlier. Added a try/catch+return reset if failing to bind. Can't use finally because killing the statement when doing incremental would be bad. The purpose of the original finally is to reset the statement when we're done with it as I've fixed above (1). Also, I noticed the richlist has itemCount, so no need to use childNodes.length.

Split return gStmt.reset() into 2 lines.

Comment on attachment 293334 [details] [diff] [review] v2.3 >+function stepListBuilder(aNumItems) { >+ try { >+ // If we're done adding all items, we can clear and quit >+ if (!gStmt.executeStep()) { >+ gStmt.reset(); Don't need to reset if executeStep returns false because it automatically does that for you. >+ return; >+ } >+ } catch (e) { >+ // Something bad happened when stepping, so clear and quit >+ gStmt.reset(); >+ return; >+ } > >+ try { >+ // Try to get the attribute values from the statement >+ let attrs = { >+ dlid: gStmt.getInt64(0), >+ file: gStmt.getString(1), >+ target: gStmt.getString(2), >+ uri: gStmt.getString(3), >+ state: gStmt.getInt32(4), >+ startTime: Math.round(gStmt.getInt64(5) / 1000), >+ endTime: Math.round(gStmt.getInt64(6) / 1000), >+ currBytes: gStmt.getInt64(8), >+ maxBytes: gStmt.getInt64(9) >+ }; > >+ // Only add the referrer if it's not null >+ let (referrer = gStmt.getString(7)) { >+ if (referrer) >+ attrs.referrer = referrer; >+ } >+ >+ // If the download is active, grab the real progress, otherwise default 100 >+ attrs.progress = gStmt.getInt32(10) ? >+ gDownloadManager.getDownload(attrs.dlid).percentComplete : 100; >+ >+ // Make the item and add it to the end >+ let item = createDownloadItem(attrs); >+ if (item) { >+ // Add item to the end and color just that one item >+ gDownloadsView.appendChild(item); >+ stripeifyList(item); >+ } >+ } catch (e) { >+ // Something must have gone wrong when getting values, so clear and quit >+ gStmt.reset(); >+ return; >+ } Why not just one try-catch block here? r=sdwilsh with these fixed.

(In reply to comment #38) > >+ if (!gStmt.executeStep()) { > >+ gStmt.reset(); > Don't need to reset if executeStep returns false because it automatically does > that for you. Neato. Removed reset and fixed comments. > >+ } catch (e) { > >+ // Something bad happened when stepping, so clear and quit .. > >+ } catch (e) { > >+ // Something must have gone wrong when getting values, so clear and quit > Why not just one try-catch block here? Combined and updated comments.

Checking in browser/app/profile/firefox.js; /cvsroot/mozilla/browser/app/profile/firefox.js,v <-- firefox.js new revision: 1.241; previous revision: 1.240 done Checking in toolkit/mozapps/downloads/content/DownloadProgressListener.js; /cvsroot/mozilla/toolkit/mozapps/downloads/content/DownloadProgressListener.js,v <-- DownloadProgressListener.js new revision: 1.33; previous revision: 1.32 done Checking in toolkit/mozapps/downloads/content/download.xml; /cvsroot/mozilla/toolkit/mozapps/downloads/content/download.xml,v <-- download.xml new revision: 1.46; previous revision: 1.45 done Checking in toolkit/mozapps/downloads/content/downloads.js; /cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v <-- downloads.js new revision: 1.113; previous revision: 1.112 done Checking in toolkit/mozapps/downloads/content/downloads.xul; /cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.xul,v <-- downloads.xul new revision: 1.39; previous revision: 1.38 done

VERIFIED on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre