Closed Bug 1381409 Opened 2 years ago Closed 2 years ago

Use DownloadList notifications directly in front-end download views

Categories

(Firefox :: Downloads Panel, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(3 files, 2 obsolete files)

The DownloadsData front-end object currently relays the notifications from DownloadList to the front-end views. This indirection can be avoided now that there is only one supported back-end.
Blocks: 1381411
Attachment #8886963 - Attachment is obsolete: true
Attachment #8886963 - Flags: review?(mak77)
Attachment #8886992 - Attachment is obsolete: true
Attachment #8886992 - Flags: review?(mak77)
Comment on attachment 8886992 [details]
Bug 1381409 - Part 2 - Let each view keep the state of downloads relevant to it.

Well, it seems that today MozReview won't let me push multiple changesets.
Attachment #8886992 - Attachment is obsolete: false
Attachment #8886992 - Flags: review?(mak77)
Attachment #8886992 - Attachment is obsolete: true
Attachment #8886992 - Flags: review?(mak77)
Priority: -- → P1
Comment on attachment 8886996 [details]
Bug 1381409 - Part 1 - Always add Download objects in their original order.

https://reviewboard.mozilla.org/r/157756/#review163434

We still don't have lots of test coverage over the downloads views right?
Attachment #8886996 - Flags: review?(mak77) → review+
Comment on attachment 8887022 [details]
Bug 1381409 - Part 2 - Let each view keep the state of downloads relevant to it.

https://reviewboard.mozilla.org/r/157770/#review163442

::: browser/components/downloads/DownloadsCommon.jsm:1043
(Diff revision 1)
>     * including progress properties.
>     *
>     * Note that progress notification changes are throttled at the Downloads.jsm
>     * API level, and there is no throttling mechanism in the front-end.
>     *
>     * @note Subclasses should override this.

maybe change these comments (also above onDownloadAdded) saying that they should override this but still call into it if they need the basic functionality?

::: browser/components/downloads/DownloadsCommon.jsm:1368
(Diff revision 1)
>      DownloadsViewPrototype.onDataLoadCompleted.call(this);
>      this._updateViews();
>    },
>  
>    onDownloadAdded(download) {
> +    DownloadsViewPrototype.onDownloadAdded.call(this, download);

Sounds like we may have luck converting these to actual ES6 classes (then we could use super.onDownloadAdded and avoid the __proto__ hack)

Maybe it could be a good mentored bug.
Attachment #8887022 - Flags: review?(mak77) → review+
Comment on attachment 8887023 [details]
Bug 1381409 - Part 3 - Use DownloadList notifications directly in front-end download views.

https://reviewboard.mozilla.org/r/157772/#review163476
Attachment #8887023 - Flags: review?(mak77) → review+
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bf2876b0ffd
Part 1 - Always add Download objects in their original order. r=mak
https://hg.mozilla.org/integration/mozilla-inbound/rev/a529720f06ee
Part 2 - Let each view keep the state of downloads relevant to it. r=mak
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a2fd40a1239
Part 3 - Use DownloadList notifications directly in front-end download views. r=mak
https://hg.mozilla.org/mozilla-central/rev/7bf2876b0ffd
https://hg.mozilla.org/mozilla-central/rev/a529720f06ee
https://hg.mozilla.org/mozilla-central/rev/4a2fd40a1239
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1393418
You need to log in before you can comment on or make changes to this bug.