Closed Bug 915036 Opened 6 years ago Closed 3 years ago

The JS Downloads API should allow sending an upload stream (POST data) with the download request

Categories

(Toolkit :: Downloads API, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: mao, Assigned: Tomislav)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Many web sites require some POST data to be sent along with the request which initiates a file download.
In order to have feature parity with the old download API the caller must be able to specify at least the upload stream to be sent, or even better to initialize its own channel / have a chance to grab the channel for further customization.
Blocks: jsdownloads
The first thing that comes to my mind is to set callbacks on the DownloadSource object, like:

Promise<nsIInputStream> getPostDataStream()
Promise adjustChannel(nsIChannel aChannel)
Promise<nsIRequestObserver> getRequestObserver();

However, these would need special handling during serialization across different sessions, either making the Download object non-serializable, or requiring special handling during deserialization by the code that set the callback originally, which I'm not sure how to obtain (might be a custom DownloadSaver type). We should definitely put some more thought on this to get it right.

In any case we should not allow the POST data to be set as a string in the JSON file, for performance reasons, as the upload stream can be quite long. Maybe there is an alternative storage that could be used externally, like the session store or the cache.
Taking as part of bug 1247793.
Assignee: nobody → tomica
Status: NEW → ASSIGNED
I decided to test the whole thing here (custom headers + POST), since this bug was originally exactly about that: uploading POST data.
Attachment #8803342 - Flags: review?(paolo.mozmail)
Looks great, I'll read through it one more time before r+ but I don't expect any major changes.

I don't remember right now what would be the naming convention for callbacks, at present we have "onchange" for the Download object but I'm not sure if it's the best naming convention to use here.
Comment on attachment 8803342 [details]
bug 915036 - Implement DownloadSource.adjustChannel callback to support POST requests

https://reviewboard.mozilla.org/r/87658/#review86706

I think adjustChannel as a name is fine, it's like a method on the DownloadSaver object. One day it might be implemented by subclassing. A possible alternative could be beforeChannelOpened, to be more specific in case we need to intercept the request at different times in the future, but I can't think of a case right now where the adjustChannel name could become unclear.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm:1280
(Diff revision 1)
> +   * @callback  adjustCallback
> +   *            Handler to be called to adjust the channel after creation,
> +   *            but before opening it.  Optional, null if not required.
> +   *
> +   *            @param  aChannel
> +   *                    The channel to be ajdusted.
> +   *
> +   *            @return {Promise}
> +   *                    @resolves when done with adjustments.
> +   */
> +   adjustChannel: null,

I've rewritten the comment with more information and the conventions used in this file. Also removed the extra space before the method name.

  /**
   * For downloads handled by DownloadCopySaver, this function can adjust the
   * network channel before it is opened, for example to change the HTTP headers
   * or to upload a stream as POST data.
   *
   * @note If this is defined this object will not be serializable, thus the
   *       Download object will not be persisted across sessions.
   *
   * @param aChannel
   *        The nsIChannel to be adjusted.
   *
   * @return {Promise}
   * @resolves When the channel has been adjusted and can be opened.
   * @rejects JavaScript exception that will cause the download to fail.
   */
  adjustChannel: null,

::: toolkit/components/jsdownloads/src/DownloadCore.jsm:1336
(Diff revision 1)
> + *          adjustChannel: {adjustCallback} handler can be used to set custom
> + *                         headers on the channel, change the HTTP method or
> + *                         send the upload stream as POST data. Optional.

Keep the same phrasing as the comment on the object:

 *          adjustChannel: For downloads handled by DownloadCopySaver, this
 *                         function can adjust the network channel before it is
 *                         opened, for example to change the HTTP headers or to
 *                         upload a stream as POST data.  Optional.
Attachment #8803342 - Flags: review?(paolo.mozmail) → review+
By the way, really good job with updating all the comments and places that needed updating :-)
Blocks: 1247793
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/656db2159f13
Implement DownloadSource.adjustChannel callback to support POST requests r=Paolo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/656db2159f13
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.