Comment on attachment 8904560 [details] Bug 1395663 - Allow retrieving DownloadHistoryList objects that include a limited number of history results. https://reviewboard.mozilla.org/r/176414/#review181346 We shouldn't query all results if we don't need them, and this may turn out to be simpler than I thought. Since in invalidateContainer all the results have been already loaded, we may just need to reverse the sort in all cases, and then reverse the order in which we iterate over the results. We can then apply the limit on the query correctly.

How do you make sure that the query result doesn't contain session downloads, then?

I think your assumption is that we would enforce a limit on the total number of downloads we display. I think this is not required, and it would add unnecessary complexity. The reason for the limit is mostly about the performance of the Places query and the responsiveness of the user interface when there are many history downloads. The number of session downloads tends to be quite low and not cause significant issues.

(In reply to :Paolo Amadini from comment #2) > We shouldn't query all results if we don't need them, and this may turn out > to be simpler than I thought. Since in invalidateContainer all the results > have been already loaded, we may just need to reverse the sort in all cases, > and then reverse the order in which we iterate over the results. We can then > apply the limit on the query correctly. So this was the first thing I tried, but stumbled upon the following: - Imagine a list of 10 downloads, of which are 5 session downloads and 5 older downloads in history. - When doing the query with the sorting by date descending, it'll return 10 items in the result where it's most likely that the 5 session downloads will be among the first five results. - When applying a 'maxResults=5' query param, whilst keeping the order by date descending, it'll return 5 items in the result where it's most likely that they'll only be session downloads. - When applying a 'maxResults=3' query param and changing the order back by date ascending, it'll return 7 items in the result where only a few of the oldest downloads that are not session downloads are in there. I think what the solution here could be is to construct a query that is indeed ordered by date descending and with a 'maxResults' query parameter, but explicitly excluding the current set of items that are session downloads. Can we pass in a set of URIs or perhaps better ID-like attributes as a 'WHERE ...' clause?

I think the solution for this would be to simply increase the amount passed to 'maxResults' with the amount of session downloads we've currently got in our list. That would effectively circumvent the issue I described in comment 5. What do you think?

(In reply to Mike de Boer [:mikedeboer] from comment #5) > - When applying a 'maxResults=5' query param, whilst keeping the order by > date descending, it'll return 5 items in the result where it's most likely > that they'll only be session downloads. This is fine, isn't it? > - When applying a 'maxResults=3' query param and changing the order back by > date ascending, it'll return 7 items in the result where only a few of the > oldest downloads that are not session downloads are in there. You mean the query will return 3 items? This is not the ordering that we want anyways, and it was the original issue, right?

Comment on attachment 8904560 [details] Bug 1395663 - Allow retrieving DownloadHistoryList objects that include a limited number of history results. https://reviewboard.mozilla.org/r/176414/#review182660 ::: toolkit/components/jsdownloads/src/DownloadHistory.jsm:571 (Diff revision 2) > this._removeSlot({ slot, slotsForUrl }); > } > } > > // Add new slots or reuse existing ones for history downloads. > for (let index = 0; index < container.childCount; index++) { Wait, isn't it enough to iterate in reverse? ::: toolkit/components/jsdownloads/test/unit/test_DownloadHistory.js:253 (Diff revision 2) > await addTestDownload(privateDownloadToAdd); > await view.waitForExpected(); > await allView.waitForExpected(); > > + // Now test the maxHistoryResults parameter. > + let allHistoryList2 = await DownloadHistory.getList({ type: Downloads.ALL, maxHistoryResults: 3 }); nit: wrap at 80 characters ::: toolkit/components/jsdownloads/test/unit/test_DownloadHistory.js:254 (Diff revision 2) > await view.waitForExpected(); > await allView.waitForExpected(); > > + // Now test the maxHistoryResults parameter. > + let allHistoryList2 = await DownloadHistory.getList({ type: Downloads.ALL, maxHistoryResults: 3 }); > + // Prepare the set of downloads to contain fewer history downloads. nit: Specify in the comment that we are removing the oldest downloads. Also, looks like the number could be calculated, but given that the test data is known and updating this number is simple, hardcoding is also fine.

Comment on attachment 8904560 [details] Bug 1395663 - Allow retrieving DownloadHistoryList objects that include a limited number of history results. https://reviewboard.mozilla.org/r/176414/#review182734 Thanks!

Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c42c63940e29 Allow retrieving DownloadHistoryList objects that include a limited number of history results. r=Paolo

Hi Mike, Can you please provide us some STR in order to verify this bug?

Sure! 1. Download 43 items, 2. Restart Firefox, 3. Check the Downloads subview inside the Library - it should show no more than 42 items.

Thanks Mike for the STR. I tested this bug on Windows 10 x64 with 57 Beta 12 (20171026211016). It seems that after 43 items are downloaded (.html pages in my case) and Firefox is restarted, I can only see the last item (the 43rd one) displayed in Downloads - Library. Please see the following screencast: https://imgur.com/a/WPrKx Any thoughts about this?

No, I don't have thoughts about this... since this is also the case in the full Library window, I'd like to ask Paolo to explain what might be happening here.

In the Places database we can only store one entry for each source URL, regardless of the target file name, so when the same page is saved multiple times, only the last file name is preserved in history. Downloads from the current session are kept in a separate data store, and there can be multiple entries for the same source URL.

Ah, today I learned! Thanks, Paolo.

Great, I can confirm this fix on 57.0 (20171109183137) on Win 10 x64, macOS 10.13 and Ubuntu 16.04 x64. After 43 items were downloaded in FF from different URL sources, in Library - Downloads subview, there's no more than 42 items displayed after restart.