Closed Bug 1117141 Opened 5 years ago Closed 5 years ago

Remove DownloadsDataItem

Categories

(Firefox :: Downloads Panel, defect)

defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

The DownloadsDataItem object can be removed in favor of using Download or HistoryDownload objects directly.
Attachment #8543306 - Flags: review?(mak77)
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #8543307 - Flags: review?(mak77)
Blocks: 1117145
Flags: qe-verify-
Flags: firefox-backlog+
Points: --- → 3
Iteration: --- → 38.2 - 9 Feb
Comment on attachment 8543306 [details] [diff] [review]
Part 1 of 2 - Bypass all the DataItem properties

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

Since you promised to kill maxBytes with fire, I'm not going to complain about it, yet!

::: browser/components/downloads/DownloadsCommon.jsm
@@ +691,5 @@
>    get canRemoveFinished() {
>      for (let dataItem of this.dataItems) {
> +      let download = dataItem.download;
> +      // Stopped, paused, and failed downloads with partial data are removed.
> +      if (download.stopped && !(download.canceled && download.hasPartialData)) {

more readable:
let canceledWithPartialData = download.canceled && download.hasPartialData;
if (download.stopped && !canceledWithPartialData)

also, looks like this could be rewritten as return this.dataItems.some...

@@ +725,4 @@
>    },
>  
>    onDownloadChanged(aDownload) {
> +    let aDataItem = this._downloadToDataItemMap.get(aDownload);

why the rename from dataItem to aDataItem considered this is not an argument?

@@ +783,5 @@
> +      }
> +    }
> +
> +    if (!aDownload.newDownloadNotified) {
> +      aDownload.newDownloadNotified = true;

Wouldn't be better to use a Set rather than adding stuff to aDownload?

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +30,2 @@
>   */
> +function HistoryDownload(aPlacesNode) {

hm, nope, looks like the params documentation is broken.

@@ +418,5 @@
>        }
>        return "";
>      }
> +    let command = getDefaultCommandForState(
> +                            DownloadsCommon.stateOfDownload(this.download));

temp var :)

@@ +1172,5 @@
>      // session downloads, check from bottom to top.
>      for (let elt = this._richlistbox.lastChild; elt; elt = elt.previousSibling) {
> +      // Stopped, paused, and failed downloads with partial data are removed.
> +      let download = elt._shell.download;
> +      if (download.stopped && !(download.canceled && download.hasPartialData)) {

ditto (! || !)
Attachment #8543306 - Flags: review?(mak77) → feedback+
Comment on attachment 8543307 [details] [diff] [review]
Part 2 of 2 - Refactor notifications and remove the DataItem object

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

::: browser/components/downloads/DownloadsCommon.jsm
@@ +684,4 @@
>     * True if there are finished downloads that can be removed from the list.
>     */
>    get canRemoveFinished() {
> +    for (let download of this.oldDownloadStates.keys()) {

why not using the .downloads getter?

@@ +832,5 @@
>      aView.onDataLoadStarting();
>  
>      // Sort backwards by start time, ensuring that the most recent
>      // downloads are added first regardless of their state.
> +    let downloadsArray = [...this.oldDownloadStates.keys()];

why not using the .downloads getter?

@@ +1018,5 @@
>     *
>     * @note Subclasses should override this.
>     */
> +  onDownloadAdded(download, newest) {
> +    throw Components.results.NS_ERROR_NOT_IMPLEMENTED;

nit: throwing Components.Exception should give better stacks than plain Cr, iirc

@@ +1433,3 @@
>     * which was set when constructing this DownloadsSummaryData instance.
>     */
> +  _downloadsForSummary() {

shorthand generator (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions)

* _downloadsForSummary() {
Attachment #8543307 - Flags: review?(mak77) → feedback+
Blocks: 1129896
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb
Attachment #8543306 - Flags: feedback?(smacleod)
Attachment #8543306 - Flags: feedback?(jaws)
Attachment #8543307 - Flags: feedback?(smacleod)
Attachment #8543307 - Flags: feedback?(jaws)
Comment on attachment 8543306 [details] [diff] [review]
Part 1 of 2 - Bypass all the DataItem properties

Review comments addressed in bug 1129896.
Attachment #8543306 - Flags: review+
Comment on attachment 8543307 [details] [diff] [review]
Part 2 of 2 - Refactor notifications and remove the DataItem object

Review comments addressed in bug 1129896.
Attachment #8543307 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2b8d8208e2c5
https://hg.mozilla.org/mozilla-central/rev/448f00fe77e1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Depends on: 1137996
Attachment #8543306 - Flags: feedback?(smacleod) → feedback+
Attachment #8543307 - Flags: feedback?(smacleod) → feedback+
You need to log in before you can comment on or make changes to this bug.