Create DownloadsHistoryDataItem and HistoryDownload objects

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks 1 bug)

Trunk
Firefox 38
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(2 attachments)

This simplifies the Library code as mentioned in bug 1115983.
Blocks: 830675
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.
No longer blocks: 830675
Posted patch Initial patchSplinter Review
I didn't test this extensively yet, but it seems to work fine with recent downloads. Might require a little bit of cleanup.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #8542583 - Flags: review?(mak77)
Flags: qe-verify-
Flags: firefox-backlog+
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.
Attachment #8542583 - Flags: review?(mak77) → feedback+
Hi Paolo, can you provide a point value.
Iteration: --- → 38.2 - 9 Feb
Flags: needinfo?(paolo.mozmail)
Points: --- → 3
Flags: needinfo?(paolo.mozmail)
Blocks: 1129896
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb
Attachment #8542583 - Flags: feedback?(smacleod)
Attachment #8542583 - Flags: feedback?(jaws)
Comment on attachment 8542583 [details] [diff] [review]
Initial patch

Review comments addressed in bug 1129896.
Attachment #8542583 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/44c72cf73a97
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Depends on: 1135348
No longer depends on: 1135348
Attachment #8542583 - Flags: feedback?(jaws) → feedback+
Attachment #8542583 - Flags: feedback?(smacleod) → feedback+
You need to log in before you can comment on or make changes to this bug.