Closed Bug 825591 Opened 11 years ago Closed 11 years ago

Skeleton of asynchronous JavaScript API for downloads

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 2 obsolete files)

This defines an initial, minimal skeleton of the asynchronous JavaScript API
for downloads, upon which various features can be added in parallel.
Attached patch Initial version for feedback (obsolete) — Splinter Review
This is the initial skeleton of the JavaScript API for downloads, for feedback.

While it does not do anything particularly useful, it shows the proposed
module structure and defines the infrastructure for regressions tests.

There are already two working regression tests, showing a simple call for a
direct download (but the code is not the final one, eventually we'll use the
background file saver component). Since this is a back-end module, it should
be possible to unit-test it using only xpcshell tests.

I expect that it will be very simple to add features one by one, with their
tests, over this initial skeleton. This way, reviews should also be easy.

The proposed working strategy is to land this in mozilla-central and work
there, so that people can contribute more easily. A branch is not needed
because we're not touching other areas of the code. We're always free to
change the API or module structure if we realize that something should be
done differently.

I've implemented a build flag, so that the component is only built when the
line "export MOZ_JSDOWNLOADS=1" is specified in .mozconfig. This may even not
be needed, however it makes sure the API is not used while it's in the early
experimental stages. The downside is that we won't get regression tests on
Nightly builds, so we'll be responsible for adapting the code in case some
other work breaks it, but this could be appropriate at this stage.
Attachment #696711 - Flags: feedback?(mak77)
Attachment #696711 - Flags: feedback?(enndeakin)
+  yield Downloads.createDownload({
+    source: { uri: NetUtil.newURI("about:blank") },
+    target: { file: getTempFile(TEST_TARGET_FILE_NAME) },
+    saver: { type: "copy" },
+  });

Why is an object of objects passed to createDownload, instead of a single object with three properties?
(In reply to Neil Deakin from comment #2)
> Why is an object of objects passed to createDownload, instead of a single
> object with three properties?

I expect "source" and "target" could be objects returned by other functions, with
a structure similar to a DownloadSource or DownloadTarget object. You may not
know in advance whether additional properties of the object are populated. For
example, in the future, there could be a file picker function that is able to
return a DownloadTarget object populated with an URI instead of a target file,
to support downloads to GIO network locations similarly to what's being done in
bug 682838.

Grouping could also be clearer than flat properties when the objects are
complex, like "source: { document: ..., uri: ..., postData: ... }" rather
than "sourceDocument: ..., sourceUri: ..., sourcePostData: ...".

"saver" is polymorphic and depends on the saver type, it may require additional
properties that are passed to the specific DownloadSaver constructor based on
the saver type.

For simple downloads (like saving an URI to a local file) I think we should
create a different function, like Downloads.simpleDownload(aSourceURIorURISpec,
aTargetFileOrFilePath, aOptions).then(onSuccess, onFailure), following the
model of OS.File.copy.

Does it make sense, or do you think we should start flat and adapt as needed?
(In reply to Paolo Amadini [:paolo] from comment #3)

> For simple downloads (like saving an URI to a local file) I think we should
> create a different function, like
> Downloads.simpleDownload(aSourceURIorURISpec,
> aTargetFileOrFilePath, aOptions).then(onSuccess, onFailure), following the
> model of OS.File.copy.

This is what I was thinking. aOptions should also be optional.
In this version, I also implemented a simpleDownload function, along the
lines of what was discussed here, though I didn't add the aOptions parameter
yet because it would be unused. I also fixed a couple of undetected typos.

This module is still excluded from the build by default as per comment 1,
it requires its mozconfig option to be set in order to be included. This
draft module structure should allow us to work on adding basic features,
but can later be changed, extended or corrected at any time.
Attachment #696711 - Attachment is obsolete: true
Attachment #696711 - Flags: feedback?(mak77)
Attachment #696711 - Flags: feedback?(enndeakin)
Attachment #703260 - Flags: superreview?(mak77)
Attachment #703260 - Flags: review?(enndeakin)
Attachment #703260 - Flags: superreview?(mak77) → review?(mak77)
Comment on attachment 703260 [details] [diff] [review]
Fixed and updated with simpleDownload

+  start: function D_start()
+  {
+    this._deferFinish.resolve(Task.spawn(function task_D_start() {
+      yield this.saver.execute();
+    }.bind(this)));
+
+    return this.finish();
+  },

I'm not sure I understand what _deferFinish is for, but I assume it works.

+
+/**
+ * Provides functions to integrate with the host application, handling for
+ * example the global prompts on shutdown.
+ */
+this.DownloadIntegration = {
+};

You write this (and DownloadUIHelper) differently than DownloadList and DownloadStore where you write:

  function DownloadList() { }

  DownloadList.prototype = {
  };

Also, you duplicate the comment in each of these files, both here and near the top.


+  createDownload: function D_createDownload(aProperties)
+  {
+    return Task.spawn(function task_D_createDownload() {
+      let download = new Download();
+
+      download.source = new DownloadSource();
+      download.source.uri = aProperties.source.uri;
+      download.target = new DownloadTarget();
+      download.target.file = aProperties.target.file;
+      download.saver = new DownloadCopySaver();
+      download.saver.download = download;
+

This doesn't use aProperties.saver. If this is because it isn't implemented yet, I'd add a comment saying this.


+add_task(function test_simpleDownload_object_arguments()
+{
+  let targetFile = getTempFile(TEST_TARGET_FILE_NAME);
+  yield Downloads.simpleDownload({ uri: TEST_SOURCE_URI },
+                                 { file: targetFile });
+  yield promiseVerifyContents(targetFile, TEST_DATA_SHORT);
+  targetFile.remove(false);

getTempFile marks the file for removal automatically, so this last remove() isn't needed right?
Attachment #703260 - Flags: review?(enndeakin) → review+
Comment on attachment 703260 [details] [diff] [review]
Fixed and updated with simpleDownload

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

So, it's really hard to evaluate this in its current status, I suspect final shape may differ quite a bit when this hits actual use-cases...
It surely looks well thought, but that also means it brings with it some complication than in the end we may found not being totally needed for our use-cases.
Though, since this is a prototype and it's disabled, I think it's ok to proceed with it and evaluate issues when they come.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +65,5 @@
> +{
> +  this._deferFinish = Promise.defer();
> +}
> +
> +Download.prototype = {

I suggest to Object.seal the prototypes

@@ +96,5 @@
> +   * @rejects JavaScript exception if the download failed.
> +   */
> +  start: function D_start()
> +  {
> +    this._deferFinish.resolve(Task.spawn(function task_D_start() {

I suspect this oneliner would be more readable as a task spawn that resolves the promise internally...

@@ +97,5 @@
> +   */
> +  start: function D_start()
> +  {
> +    this._deferFinish.resolve(Task.spawn(function task_D_start() {
> +      yield this.saver.execute();

is start() expected to do any check on _deferFinish and saver validity?
Or I suppose being just a prototype implementation error checking will come later... Indeed I don't see much error management around.

@@ +110,5 @@
> +   * @return {Promise}
> +   * @resolves When the download has finished successfully.
> +   * @rejects JavaScript exception if the download failed.
> +   */
> +  finish: function D_finish() {

it's confusing to read Download.finish().then(), since finish() sounds line an action, not just an handler.
We have start() that effectively executes the saver, and finish() that sounds like would cancel or stop the action, but instead is just an handler, like onFinish, promiseFinish...

::: toolkit/components/jsdownloads/test/unit/tail.js
@@ +18,5 @@
> +  // xpcshell testing framework supports asynchronous termination functions.
> +  let deferred = Promise.defer();
> +  gHttpServer.stop(deferred.resolve);
> +  yield deferred.promise;
> +});

I think alternatively you may just do_register_cleanup(function() gHttpServer.stop())) in the head and avoid a tail file.
Attachment #703260 - Flags: review?(mak77) → review+
(In reply to Neil Deakin from comment #6)
> You write this (and DownloadUIHelper) differently than DownloadList and
> DownloadStore where you write:
> 
>   function DownloadList() { }
> 
>   DownloadList.prototype = {
>   };

Some objects may have multiple instances, while others are singletons.

> Also, you duplicate the comment in each of these files, both here and near
> the top.

The first comment is the "file comment", it appears in MXR when you browse the
folder, but must be the first comment in the file. See for example:

http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/src/

The second is the "object comment", I expect it to appear in code completion if
you have a development environment that supports that for JavaScript. I've
usually seen it placed near the object's constructor function, so I assume
it should be there in order for code completion to show it.

Given the two conflicting requirements, I couldn't find other solutions than
duplicating the comments.

> getTempFile marks the file for removal automatically, so this last remove()
> isn't needed right?

This was done for robustness between individual tests, I've now made it so that
getTempFile ensures that the file doesn't exist, that should be equivalent.
Thanks!

(In reply to Marco Bonardo [:mak] from comment #7)
> > +    this._deferFinish.resolve(Task.spawn(function task_D_start() {
> 
> I suspect this oneliner would be more readable as a task spawn that resolves
> the promise internally...

So, we discussed on IRC and we're going with two types of notifications:
* whenDone() returns a promise that is resolved if the download succeeds
  without interruptions, and is rejected if the download fails or is
  canceled during this attempt.
* whenSucceeded() returns a promise that is resolved when the download
  succeeds, even if it fails and then is retried from the user interface.
  It is never rejected.

So, _deferDone will be resolved with the task's result, meaning that it
will be rejected if an exception occurs. _deferSucceeded will be resolved
as the last line of the task, if it is ever reached.

> Or I suppose being just a prototype implementation error checking will come
> later... Indeed I don't see much error management around.

As discussed, checking for misuse will come later if and when needed. (We're
checking normal errors and handling exceptions, of course.)

> it's confusing to read Download.finish().then(), since finish() sounds line
> an action, not just an handler.

We'll be generally using these conventions:
* Nouns, or past verbs, for status properties: progress, done, succeeded
* Present verbs for actions: start(), open()
* "When" + past verbs for methods without side-effects: whenDone()
* "On" + present verb (lowercase, DOM style) for settable callbacks: onchange
Attached patch Final patchSplinter Review
Attachment #703260 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/580dbef53b91
Target Milestone: --- → mozilla21
https://hg.mozilla.org/mozilla-central/rev/580dbef53b91
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
How can I test already committed patches? I don't see the changes in mozilla-central (for example changes in configure.in).
(In reply to jhorak from comment #12)
> How can I test already committed patches? I don't see the changes in
> mozilla-central (for example changes in configure.in).

Some of these have been modified since the feature was first implemented.
If you are interested in which parts may be tested in Nightly, you can take a
look at the current state of the files, or at the dependencies of bug 825588.
You need to log in before you can comment on or make changes to this bug.