1116176 - Create DownloadsHistoryDataItem and HistoryDownload objects

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[:Paolo Amadini]

This simplifies the Library code as mentioned in bug 1115983.

Comment 1

[:Paolo Amadini]

I'm also thinking that, again in the spirit of simplifying the code, we may want to remove the check for the file size on disk for very old downloads without Places metadata, and just assume they succeeded.

Comment 2

[:Paolo Amadini]

Attachment: Initial patch

I didn't test this extensively yet, but it seems to work fine with recent downloads. Might require a little bit of cleanup.

Comment 3

[:Paolo Amadini]

Attachment: On latest trunk

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8542583 [details] [diff] [review]
Initial patch

Review of attachment 8542583 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed over IRC, with the scope of unblocking this work, I'm going through these patches with feedback, someone else will do the deep line by line review.

once all of the queue of patches has been reviewed, I'll be available for a last pass on the coalesced patch (with so many patches removing changes done by the previous one, it's very hard to figure what survives and what doesn't)

::: browser/components/downloads/DownloadsCommon.jsm
@@ +7,5 @@
>  "use strict";
>  
>  this.EXPORTED_SYMBOLS = [
>    "DownloadsCommon",
> +  "DownloadsDataItem",

I don't like having modules exposing more than one object, they kill the defineLazyModuleGetter functionality and are more confusing for global scope pollution (I import DownloadsCommon and I get a DownloadsDataItem symbol)

We could either move it to a separate module, or make DownloadsCommon expose it through a property, like DownloadsCommon.newDataItem() or new DownloadsCommon.DataItem()

@@ +27,5 @@
>   *
>   * DownloadsDataItem
> + * Represents a single item in the list of downloads.  This object wraps the
> + * Download object from the JavaScript API for downloads.  A specialized version
> + * of this object is implemented in the Places front-end view.

would be nice to point out where that view code lives.

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +59,5 @@
> +   */
> +  start() {
> +    // In future we may try to download into the same original target uri, when
> +    // we have it.  Though that requires verifying the path is still valid and
> +    // may surprise the user if he wants to be requested every time.

the last phrase could likely be clarified (wants to be requested what? it's the path, but it's not well expressed)

@@ +78,5 @@
> + *
> + * @param aPlacesNode
> + *        The Places node for the history download.
> + */
> +function DownloadsHistoryDataItem(aPlacesNode) {

Maybe just HistoryDataItem (would cope better with HistoryDownload)

@@ +139,4 @@
>   *
> + * The shell may contain a session download, a history download, or both.  When
> + * both a history and a current download are present, the current download gets
> + * priority and its information is displayed.

please unify the notation between "session download" and "current download", I think the former is better.

@@ +220,5 @@
> +  _historyDataItem: null,
> +  get historyDataItem() this._historyDataItem,
> +  set historyDataItem(aValue) {
> +    if (this._historyDataItem != aValue) {
> +      if (!aValue && !this._sessionDataItem) {

you are sometimes using ._sessionDataItem and sometimes .sessionDataItem... please unify notation.
The same for _historyDataItem.

@@ +247,1 @@
>    },

hm, looks like some of the lazy getters could use defineLazyGetter?

@@ +465,3 @@
>        case "downloadsCmd_retry":
> +        console.log(this.dataItem);
> +        return this.dataItem.canRetry;

debug spew

@@ +595,5 @@
> +  },
> +
> +  _checkTargetFileOnSelect: Task.async(function* () {
> +    try {
> +      this._targetFileExists = yield OS.File.exists(this.download.target.path);

could pass the path as argument or merge with onSelect (the only call point)

::: browser/components/downloads/content/downloads.js
@@ +1060,5 @@
>      // is reloaded, we must change the URI used by the XUL image element, for
>      // example by adding a query parameter.  Since this URI has a "moz-icon"
>      // scheme, this only works if we add one of the parameters explicitly
>      // supported by the nsIMozIconURI interface.
> +    if (this.dataItem.state == Ci.nsIDownloadManager.DOWNLOAD_FINISHED) {

hm, I don't remember why we needed to filter on aOldState. maybe onStateChanged was notifying progress changes when actually the state didn't change at all?

btw, you should probably remove it as an argument if unused.

Comment 5

[Marco Mucci [:MarcoM]]

Hi Paolo, can you provide a point value.

Comment 6

[:Paolo Amadini]

Comment on attachment 8542583 [details] [diff] [review]
Initial patch

Review comments addressed in bug 1129896.

Comment 7

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/44c72cf73a97

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/44c72cf73a97