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

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.

The very basic version of the DownloadList object.

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 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

This time with the regression tests, and comments addressed.

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?

(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.