835880 - Implement the basic DownloadList object

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

The jsdownloads API should give access to a list of downloads that can be
populated and enumerated.

Comment 1

[:Paolo Amadini]

The aOptions argument of createDownload would have an "addToDownloadList" property
that determines whether the newly created download will be immediately added to
the appropriate global download list.

Comment 2

[:Paolo Amadini]

Attachment: The patch

The very basic version of the DownloadList object.

Comment 3

[Neil Deakin]

Comment on attachment 714974 [details] [diff] [review]
The patch

Seems ok. Not sure if I like the term 'view' for the listener, but you should at least document that three methods that can be called on it.

>+  /**
>+   * Removes a download from the list.  If the download was already removed,
>+   * this mehtod has no effect.
>+   *

mehtod -> method


>+add_task(function test_getPublicDownloadList()
>+{
>+  let downloadListOne = yield Downloads.getPublicDownloadList();
>+  let downloadListTwo = yield Downloads.getPublicDownloadList();
>+
>+  do_check_true(downloadListOne === downloadListTwo);
>+});

do_check_eq should be ok to use here.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 714974 [details] [diff] [review]
The patch

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

::: toolkit/components/jsdownloads/src/DownloadList.jsm
@@ +72,5 @@
> +   *        The Download object to add.
> +   */
> +  add: function DL_add(aDownload) {
> +    this._downloads.push(aDownload);
> +    aDownload.onchange = this._change.bind(this, aDownload);

is there the risk the external consumer may try to set onchange on a download object taken from a list?
I'm wondering if we should use defineProperty with writable: false to set onchange here.

@@ +143,5 @@
> +   *        The view object to add.
> +   */
> +  addView: function DL_addView(aView)
> +  {
> +    this._views.push(aView);

I'm thinking you could rather use a Set for the views, so it's automatically protected from multiple bogus calls to addView

::: toolkit/components/jsdownloads/src/Downloads.jsm
@@ +127,5 @@
>      });
>    },
>  
>    /**
> +   * Retrieves the global DownloadList object for downloads that were not

what does "global" is trying to say here? do we have partial lists?

@@ +137,5 @@
> +   * @return {Promise}
> +   * @resolves The DownloadList object for public downloads.
> +   * @rejects JavaScript exception.
> +   */
> +  getPublicDownloadList: function D_getPublicDownloadList(aProperties)

aProperties is not used anywhere, nor is documented

Comment 5

[:Paolo Amadini]

Attachment: Updated patch

This time with the regression tests, and comments addressed.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 717520 [details] [diff] [review]
Updated patch

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

::: toolkit/components/jsdownloads/src/DownloadList.jsm
@@ +72,5 @@
> +   *        The Download object to add.
> +   */
> +  add: function DL_add(aDownload) {
> +    this._downloads.push(aDownload);
> +    aDownload.onchange = this._change.bind(this, aDownload);

you didn't answer my concern in first comment of comment 4, is that on purpose/design to allow a consumer to detach the download from the views observing it?

Comment 7

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] (Away Mar 1) from comment #4)
> is there the risk the external consumer may try to set onchange on a
> download object taken from a list?
> I'm wondering if we should use defineProperty with writable: false to set
> onchange here.

Sorry, I forgot to answer to this in the bug. I think we may evaluate these
misuse protections later, if we determine they will actually help for real
world cases. But we're still in a state where the interface may be revised,
so I'd prefer to just keep the code as simple as possible for now.

In any case, starting a new download that is added to a global list will
most probably be done with a parameter when starting the new download, and those
consumers won't concern themselves with neither "onchange" nor the view at all.

Comment 8

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c2cf61b8f39

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2c2cf61b8f39