831708 - Add basic download states and progress

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

This is the first feature patch on top of the Downloads API skeleton. We are
implementing off-main-thread download functionality in the final way, using
the BackgroundFileSaver object.

Comment 1

[:Paolo Amadini]

Attachment: The patch

Comment 2

[Josh Matthews [:jdm]]

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

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

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +299,5 @@
> +      // Set the target file, that will be deleted if the download fails.
> +      backgroundFileSaver.setTarget(download.target.file, false);
> +  
> +      // Create a channel from the source, and listen to progress notifications.
> +      let channel = NetUtil.newChannel(download.source.uri);

Will this initiate a network request? If so, this will expose private downloads to the permanent disk cache, etc.

Comment 3

[:Paolo Amadini]

(In reply to Josh Matthews [:jdm] from comment #2)
> Will this initiate a network request? If so, this will expose private
> downloads to the permanent disk cache, etc.

Yes, thanks for the reminder! I was thinking of adding more features related to
the source of the download later, but we might actually easily add a
download.source.isPrivate property from the start. Private downloads (those
from a private source) will be also added to their separate DownloadList,
when the DownloadList objects are implemented.

Comment 4

[Neil Deakin]

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

+  let viewNotified = false;
+  let lastNotifiedIsActive;
+  let lastNotifiedProgress;
+  download.view = {
+    onChange: function () {
+      viewNotified = true;
+      lastNotifiedIsActive = download.isActive;
+      lastNotifiedProgress = download.progress;
+    },
+  };
+
+  // Starts the download and waits for completion.
+  yield download.start();
+
+  // The view should have been notified before the download completes.
+  do_check_false(lastNotifiedIsActive);
+  do_check_eq(lastNotifiedProgress, 100);

I'm guessing from the comment that you wanted to check viewNotified here as well. Currently, it is unused.



+  download.view = {
+    onChange: function () {
+      if (download.progress == 50) {
+        // Continue after the first chunk of data is fully received.
+        deferUntilHalfProgress.resolve();

Is it possible to verify hasProgress, currBytes and maxBytes here?

Comment 5

[Marco Bonardo [:mak]]

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

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

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +104,5 @@
> +   * Progress percent, from 0 to 100.  Intermediate values are reported only if
> +   * hasProgress is true.  A value of 100 does not necessarily indicate that the
> +   * download is ready.
> +   */
> +  progress: 0,

the "ready" concept is a bit generic, what does it mean, that it may not be complete, that we have to wait for an event? I think needs better definition.

@@ +110,5 @@
> +  /**
> +   * Total number of bytes to be transferred before the download finishes.  This
> +   * value is 0 when hasProgress is false.
> +   */
> +  maxBytes: 0,

so, maybe we could avoid setting both and just have get hasProgress() !!this._maxBytes;
Is there any case where maxBytes is defined but there's no progress? If so I think it's worth documenting them in the hasProgress javadoc.

I think previously we were using progress -1 instead of having an explicit hasProgress, again I wonder if progress may have setter and getter so there's no risk they go out-of-sync (the getter would return -1 if maxBytes is 0)

Apart this, I always found the maxBytes name confusing/misleading, I think we may come with a better name for the final size of the download. totalBytes or finalBytes would already be clearer...

@@ +122,5 @@
> +
> +  /**
> +   * View object receiving onChange notifications.
> +   */
> +  view: null,

looks like you decided to go a single observer way, in some cases multiple observers may want to observe the same dataset (for example in the panel we may have both the list and the summary observe a single result, that is less expensive than having each of them own their own result). Will this be supported by the list or do we always need separate results?

@@ +298,4 @@
>  
> +      // Set the target file, that will be deleted if the download fails.
> +      backgroundFileSaver.setTarget(download.target.file, false);
> +  

trailing spaces

@@ +306,5 @@
> +        getInterface: XPCOMUtils.generateQI([Ci.nsIProgressEventSink]),
> +        onProgress: function DCSE_onProgress(aRequest, aContext, aProgress,
> +                                             aProgressMax)
> +        {
> +          download._setBytes(aProgress, aProgressMax);

this is unclean, we are invoking something marked as a private method in download... either it is an actual method, or we need an "observing" API in download that receives messages.

@@ +310,5 @@
> +          download._setBytes(aProgress, aProgressMax);
> +        },
> +        onStatus: function () { },
> +      };
> +  

trailing spaces

@@ +328,5 @@
> +            // If the data transfer completed successfully, indicate to the
> +            // background file saver that the operation can finish.
> +            if (Components.isSuccessCode(aStatusCode)) {
> +              backgroundFileSaver.finish(Cr.NS_OK);
> +            }

shouldn't be finish() invoked in both cases to free resources? The interface seems to point at the need to call it when done.

::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js
@@ +11,5 @@
>  
>  ////////////////////////////////////////////////////////////////////////////////
> +//// Globals
> +
> +function promiseSimpleDownload() {

why is this called promise when it's just a synchronous helper returning a download? Looks confusing

Comment 6

[:Paolo Amadini]

Attachment: Final patch

Comment 7

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12e12524ce9d

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/12e12524ce9d