Closed Bug 1381411 Opened 7 years ago Closed 7 years ago

Implement the DownloadHistoryList object

Categories

(Toolkit :: Downloads API, enhancement, P1)

Product:
Toolkit
Component:
Downloads API
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1381411 - Implement the DownloadHistoryList object.
59 bytes, text/x-review-board-request
mak
: review+
Details 
The history of past downloads is currently visible only in the Downloads View in the Library window, but there are plans to show it in a new Downloads Subview in the Library panel and also make it accessible to WebExtensions.

In order to support this, we can implement a new DownloadHistoryList object that handles public session downloads as well as past downloads, hiding the fact that history downloads are stored using Places interfaces.

Instead of Download objects, this list contains DownloadSlot objects that at various points in time can map to a Download object or a HistoryDownload object. For simplicity, properties and methods of the Download interface are not forwarded to the underlying object.

The DownloadHistoryList will be ordered, and this can be achieved by adding an insertBefore option to the onDownloadAdded notification. This allows showing session downloads at the top in the Library window.
Priority: -- → P1
Comment on attachment 8889078 [details]
Bug 1381411 - Implement the DownloadHistoryList object.

Asking for feedback since this already looks quite close to the final architecture I have in mind. Before the final review I will split the patch in smaller parts and do some more thorough manual testing. I plan to add regression tests for the DownloadHistoryList object, but not for the front-end view at the moment.

I didn't use "hg copy" for "DownloadHistory.jsm" even if some of the code is originally from "allDownloadsViewOverlay.js", because I edited most of it quite heavily to provide a simpler solution. The HistoryDownload object is the only part that didn't change much.

The only user-visible change in the Library window should be with how the selection behaves when retrying downloads. This is because previously a history download could become a session download when retrying, and the other way around when clearing session downloads. Now, instead, the old download type is removed and the new download type is added at the correct position.

This allows the DownloadHistoryList to pass Download and HistoryDownload objects to onDownloadAdded and other notifications. This makes DownloadList and DownloadHistoryList interchangeable. If the type of the underlying download could change dynamically, then we would need to pass the DownloadSlot to the notifications, and either proxy all the Download methods or make the views aware of the DownloadSlot object. We would also need a method to change the position of an existing download in the list.

One thing to keep in mind is that order of downloads in DownloadHistoryList is currently reversed compared to DownloadList. This inversion is visible when displaying downloads in private windows, that are powered by the private global DownloadList. I'll fix this issue with an additional changeset on top of the current patch, because the current state makes it easier to follow how the insertBefore logic was moved to DownloadHistoryList from "allDownloadsViewOverlay.js".
Attachment #8889078 - Flags: feedback?(mak77)
Depends on: 830415
(In reply to :Paolo Amadini from comment #2)
> I didn't use "hg copy" for "DownloadHistory.jsm" even if some of the code is
> originally from "allDownloadsViewOverlay.js", because I edited most of it

That's fine.

> The only user-visible change in the Library window should be with how the
> selection behaves when retrying downloads. This is because previously a
> history download could become a session download when retrying, and the
> other way around when clearing session downloads. Now, instead, the old
> download type is removed and the new download type is added at the correct
> position.

This is fine, provided we restore the selection properly when we do that.
I don't expect users to do batch changes of hundreds of downloads, differently from common History.
Comment on attachment 8889078 [details]
Bug 1381411 - Implement the DownloadHistoryList object.

https://reviewboard.mozilla.org/r/160110/#review166258

In general it sounds OK. Though, I'm a bit worried of functional and performance regressions (especially on large lists of history downloads), so we should take care of checking those constantly.

::: browser/components/downloads/content/allDownloadsViewOverlay.js
(Diff revision 1)
> -      if (aValue) {
> -        this.sessionDownloadState = DownloadsCommon.stateOfDownload(aValue);
> -      }
> -
> -      this.ensureActive();
> -      this._updateUI();

I'm a little bit worried by the removal of all the calls to ensureActive and _updateUI.

In particular, the .active trick IIRC was used to manage updates in very large lists so that never shown elements didn't get pointless updates, and started getting them after becoming visible.

How do we achieve that now?

::: browser/components/downloads/content/allDownloadsViewOverlay.js:239
(Diff revision 1)
>        return;
>      }
>  
>      // Start checking for existence.  This may be done twice if onSelect is
>      // called again before the information is collected.
>      if (!this._targetFileChecked) {

also, _updateUI was unsetting _targetFileChecked when an element was updated, so that the file size could be checked again. How is that now?

::: browser/components/downloads/content/allDownloadsViewOverlay.js
(Diff revision 1)
>    set place(val) {
> -    // Don't reload everything if we don't have to.
>      if (this._place == val) {
>        // XXXmano: places.js relies on this behavior (see Bug 822203).
>        this.searchTerm = "";
> -      return val;

Historically setters are expected to return the set value. I'm not sure if that still matters.

::: browser/components/downloads/content/allDownloadsViewOverlay.js:385
(Diff revision 1)
> -    return val;
>    },
>  
>    get selectedNodes() {
>        return Array.filter(this._richlistbox.selectedItems,
> -                          element => element._placesNode);
> +                          element => element._shell.download.placesNode);

I think the reason (not strict) for having DOMelement._placesNode is that it's coherent across all the views. It's not critical, just wanted to point that out.

::: toolkit/components/jsdownloads/src/DownloadHistory.jsm:75
(Diff revision 1)
> +   *
> +   * The targetFileSpec property is the value of "downloads/destinationFileURI",
> +   * while the other properties are taken from "downloads/metaData". Any of the
> +   * properties may be missing from the object.
> +   */
> +  get cachedPlacesMetaData() {

may probably become a defineLazyGetter now

::: toolkit/components/jsdownloads/src/Downloads.jsm:46
(Diff revision 1)
>              "resource://gre/modules/DownloadIntegration.jsm");
>  
> +// Places query used to retrieve all history downloads for the related list.
> +const HISTORY_PLACES_QUERY =
> +      "place:transition=" + Ci.nsINavHistoryService.TRANSITION_DOWNLOAD +
> +      "&sort=" + Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_DESCENDING;

This adds a little bit of Places pollution into code that may be used by clients not using Places.
Maybe for now it should be abstracted away in DownloadHistory (so put the url there and allow another consumer to define a different DownloadsHistory implementation)?
Attachment #8889078 - Flags: feedback?(mak77) → feedback+
(In reply to Marco Bonardo [::mak] from comment #4)
> In general it sounds OK. Though, I'm a bit worried of functional and
> performance regressions (especially on large lists of history downloads), so
> we should take care of checking those constantly.

I'm actually thinking of splitting the back-end and the front-end patches, and landing the latter at the beginning of next cycle, to get more testing in Nightly.

> ::: browser/components/downloads/content/allDownloadsViewOverlay.js
> (Diff revision 1)
> > -      if (aValue) {
> > -        this.sessionDownloadState = DownloadsCommon.stateOfDownload(aValue);
> > -      }
> > -
> > -      this.ensureActive();
> > -      this._updateUI();
> 
> I'm a little bit worried by the removal of all the calls to ensureActive and
> _updateUI.
> 
> In particular, the .active trick IIRC was used to manage updates in very
> large lists so that never shown elements didn't get pointless updates, and
> started getting them after becoming visible.
> 
> How do we achieve that now?

Setting elements as active is handled by the _ensureVisibleElementsAreActive call, and this part still works fine, but there is a regression because onChanged was previously onSessionDownloadChanged, and it assumed the download was active because all session downloads were active. I'll refactor the code to fix the issue. Thanks for catching it!

> ::: browser/components/downloads/content/allDownloadsViewOverlay.js:239
> also, _updateUI was unsetting _targetFileChecked when an element was
> updated, so that the file size could be checked again. How is that now?

Same here.

> ::: browser/components/downloads/content/allDownloadsViewOverlay.js
> Historically setters are expected to return the set value. I'm not sure if
> that still matters.

I don't think it matters anymore. I was never sure why that was the case, anyways.

> ::: browser/components/downloads/content/allDownloadsViewOverlay.js:385
> I think the reason (not strict) for having DOMelement._placesNode is that
> it's coherent across all the views. It's not critical, just wanted to point
> that out.

Well, if that's not part of the interface visible from the outside, I'd rather not duplicate the data and avoid the additional expando property.

> ::: toolkit/components/jsdownloads/src/Downloads.jsm:46
> (Diff revision 1)
> >              "resource://gre/modules/DownloadIntegration.jsm");
> >  
> > +// Places query used to retrieve all history downloads for the related list.
> > +const HISTORY_PLACES_QUERY =
> > +      "place:transition=" + Ci.nsINavHistoryService.TRANSITION_DOWNLOAD +
> > +      "&sort=" + Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_DESCENDING;
> 
> This adds a little bit of Places pollution into code that may be used by
> clients not using Places.
> Maybe for now it should be abstracted away in DownloadHistory (so put the
> url there and allow another consumer to define a different DownloadsHistory
> implementation)?

That's a good point, thanks for the suggestion! Given that we'd like to keep things separate, it sounds like we want a DownloadHistory.getList() method, instead of having a Downloads.HISTORY type, and import the module directly in the front-end.

This will create a nice separation. There will still be some Places references in DownloadIntegration.jsm for the history observer, but eventually they can move to DownloadHistory and be integrated through an Integration.jsm override.
Comment on attachment 8889078 [details]
Bug 1381411 - Implement the DownloadHistoryList object.

This version use the same order as ordinary download lists, with the most recent downloads at the top. The front-end code around this is now simpler as it uses modern DOM methods.

I've removed the call that scrolls the list to the top when a history download is retried or a new session download is added. The selection in the list does not move with a retried download, and I think this new behavior is as good as the previous one. When retrying multiple downloads in a sequence, for example, the next download to retry is still visible and selected.

I'll add tests for DownloadHistoryList and split the front-end portion to a separate bug, then I'll ask for review.
Attachment #8889078 - Flags: feedback?(mak77)
I've tested the performance with this new system, and it has not regressed. Both the separate fragment and removing the richlistbox from the document while appending elements are still required to get a 4x performance improvement. Using just one of the two techniques without the other only gets a 2x improvement.
Comment on attachment 8889078 [details]
Bug 1381411 - Implement the DownloadHistoryList object.

Looks good so far, now the separation of concerns is nicer.
Attachment #8889078 - Flags: feedback?(mak77) → feedback+
Comment on attachment 8889078 [details]
Bug 1381411 - Implement the DownloadHistoryList object.

https://reviewboard.mozilla.org/r/160110/#review169750

There are a few things that I'm sure are untested in automation, and would be nice to do a quick check before landing:
1. multiple selection in the Library/Downloads view and Remove From History or Delete
2. Clear all history while the Downloads view in the Library is open and check downloads are updated properly
3. Delete related history from the History Sidebar while the Downloads view in the Library is open and check downloads are updated properly
Maybe a few could be added to the test?

I'm sure there's more things, but we're also at the right time to land a scary refactoring since we just merged, there's time to handle eventual downsides.
Also, when actually handling the view, we may discover more, for example many of the Places views may expect DOMNode._placesNode... But let's not get mad before the time.

::: browser/components/downloads/content/allDownloadsViewOverlay.js:163
(Diff revision 4)
> +
> +    let browserWin = RecentWindow.getMostRecentBrowserWindow();
> +    let initiatingDoc = browserWin ? browserWin.document : document;
> +
> +    // Do not suggest a file name if we don't know the original target.
> +    let leafName = this.download.target.path ?

leafName sounds like the wrong name for a path? maybe this comes from a previous refactoring.

::: toolkit/components/jsdownloads/src/DownloadHistory.jsm:135
(Diff revision 4)
> +   *
> +   * The targetFileSpec property is the value of "downloads/destinationFileURI",
> +   * while the other properties are taken from "downloads/metaData". Any of the
> +   * properties may be missing from the object.
> +   */
> +  getPlacesMetaDataFor(spec) {

How hard would it be to make this fake-async? (yes, you're free to fix bug 1382963 if you wish).
It's currently synchronous just because annotations are synchronous, but it can't be forever.

::: toolkit/components/jsdownloads/src/DownloadHistory.jsm:180
(Diff revision 4)
> + */
> +XPCOMUtils.defineLazyGetter(this, "gCachedPlacesMetaData", function() {
> +  let placesMetaData = new Map();
> +
> +  // Read the metadata annotations first, but ignore invalid JSON.
> +  for (let result of PlacesUtils.annotations.getAnnotationsWithName(

this cahe will also likely become async... I don't want us to fix everything here though, just pointing out possible future problem.

::: toolkit/components/jsdownloads/src/DownloadHistory.jsm:523
(Diff revision 4)
> +    // If there are no existing slots for this URL, we have to create a new one.
> +    // Since the history download is visible in the slot, we also have to update
> +    // the object using the Places metadata.
> +    let historyDownload = new HistoryDownload(placesNode);
> +    historyDownload.updateFromMetaData(
> +                    gCachedPlacesMetaData.get(placesNode.uri) ||

nit: indent normally (just 2 spaces)

::: toolkit/components/jsdownloads/test/unit/test_DownloadHistory.js:72
(Diff revision 4)
> + * by the DownloadHistoryList object, and that the order of results is correct.
> + */
> +add_task(async function test_DownloadHistory() {
> +  // Clean up at the beginning and at the end of the test.
> +  async function cleanup() {
> +    await PlacesTestUtils.clearHistory();

nit: you can directly use await PlacesUtils.history.clear() now. The PlacesTestUtils util is deprecated.

::: toolkit/components/jsdownloads/test/unit/test_DownloadHistory.js:105
(Diff revision 4)
> +    }
> +
> +    // Add the download to history using the XPCOM service, then use the
> +    // DownloadHistory module to save the associated metadata.
> +    let promiseAnnotations = waitForAnnotations(properties.source.url);
> +    let promiseVisit = promiseWaitForVisit(properties.source.url);

fwiw promiseWaitForVisit may probably be replaced by PlacesTestUtils.waitForNotification("onVisit", check_callback, "history");
Attachment #8889078 - Flags: review+
Thanks! I'll now handle the issues or file follow-up bugs for them. I've already tested most of the mentioned cases manually already, I'll repeat on the latest version. Adding an automated test for clear history is also a good idea.
I also have some test failures to figure out, I didn't think we had some automated testing of the front-end.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=238a56fc2f83be68712cf9847401d34d70205342
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56294c966822
Implement the DownloadHistoryList object. r=mak
Blocks: 1387446
We'll need to verify that the behavior of the Library window has not been regressed, although we could also verify this after the rest of the refactoring blocking bug 1354532 has been done.
Flags: qe-verify+
https://hg.mozilla.org/mozilla-central/rev/56294c966822
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 1396581
Verified fixed using the latest Nightly (2017-10-08) and Firefox Beta 57.0b6 on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12

While verifying this bug I encountered bug 1406906.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
status-firefox58: --- → verified
Flags: qe-verify+
Depends on: 1424469
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: