Allow retrieving DownloadHistoryList objects that include a limited number of history results

VERIFIED FIXED in Firefox 57

Status

()

Toolkit
Downloads API
P1
normal
VERIFIED FIXED
9 months ago
6 months ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

52 Branch
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [reserve-photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)

Updated

9 months ago
Priority: P1 → --
Whiteboard: [photon-structure] → [photon-structure] [triage]

Updated

9 months ago
Summary: Allow DownloadHistory to be sorted by date - descending → Allow retrieving DownloadHistoryList objects that include a limited number of history results

Updated

9 months ago
Component: Toolbars and Customization → Downloads API
Product: Firefox → Toolkit
(Assignee)

Updated

9 months ago
Blocks: 1354532
No longer depends on: 1354532
Comment hidden (mozreview-request)

Updated

9 months ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 57.3 - Sep 19
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon-structure] [triage] → [reserve-photon-structure]

Comment 2

9 months ago
mozreview-review
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.
Attachment #8904560 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 3

9 months ago
How do you make sure that the query result doesn't contain session downloads, then?
Flags: needinfo?(paolo.mozmail)

Comment 4

9 months ago
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.
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 5

9 months ago
(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?
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 6

9 months ago
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?

Comment 7

9 months ago
(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?
Flags: needinfo?(paolo.mozmail)
Comment hidden (mozreview-request)

Comment 9

9 months ago
mozreview-review
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.
Attachment #8904560 - Flags: review?(paolo.mozmail)
Comment hidden (mozreview-request)

Comment 11

9 months ago
mozreview-review
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!
Attachment #8904560 - Flags: review?(paolo.mozmail) → review+

Comment 12

9 months ago
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

Comment 13

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c42c63940e29
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

8 months ago
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
Hi Mike,

Can you please provide us some STR in order to verify this bug?
Flags: needinfo?(mdeboer)
(Assignee)

Comment 15

7 months ago
Sure!

1. Download 43 items,
2. Restart Firefox,
3. Check the Downloads subview inside the Library - it should show no more than 42 items.
Flags: needinfo?(mdeboer)
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?
Flags: needinfo?(mdeboer)
(Assignee)

Comment 17

6 months ago
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.
Flags: needinfo?(mdeboer) → needinfo?(paolo.mozmail)

Comment 18

6 months ago
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.
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 19

6 months ago
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.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.