Closed Bug 1247793 Opened 8 years ago Closed 8 years ago

Implement POST for chrome.downloads.download()

Categories

(WebExtensions :: Request Handling, defect, P2)

defect

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: aswan, Assigned: zombie)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [downloads] triaged)

Attachments

(1 file, 1 obsolete file)

Handle options.method, options.headers, and options.body in chrome.downloads.download():

https://developer.chrome.com/extensions/downloads#method-download
Blocks: 1213445
Whiteboard: [downloads]
Priority: -- → P2
Whiteboard: [downloads] → [downloads] triaged
Component: WebExtensions: Untriaged → WebExtensions: Request Handling
Assignee: nobody → mixedpuppy
Assignee: mixedpuppy → tomica
Attachment #8802425 - Flags: review?(aswan)
It's a bit weird to write the very similar tests twice, but I'm not sure if there is any way to avoid doing that?
Status: NEW → ASSIGNED
Nice, thanks for doing this.  But can you split this into two patches, one for the DownloadCore changes and then one for the webextensions changes that use the DownloadCore changes?  And ask :paolo to review the first patch.
Incidentally, I think some of the logic for what headers a client may override may belong in DownloadCore itself, but I defer to Paolo on that.
Flags: needinfo?(tomica)
Hey Paolo, if you think this would need *much* more work (testing or other), I would prefer scoping it down to just a boolean `post` flag, rather than exploding the code for features that are unlikely to ever get used by anyone (we just need `POST` for web extensions).


(In reply to Andrew Swan [:aswan] from comment #4)
> Incidentally, I think some of the logic for what headers a client may
> override may belong in DownloadCore itself, but I defer to Paolo on that.

I think those header restrictions are meant for web/extension developers, but I'd be glad to move them if Paolo thinks it would be useful elsewhere.
Flags: needinfo?(tomica)
Comment on attachment 8802425 [details]
bug 1247793 - Implement download() options: method, headers and body,

(In reply to Tomislav Jovanovic :zombie from comment #7)
> Hey Paolo, if you think this would need *much* more work (testing or other),
> I would prefer scoping it down to just a boolean `post` flag, rather than
> exploding the code for features that are unlikely to ever get used by anyone
> (we just need `POST` for web extensions).

Everything from bug 915036 comment 1 still holds. The callbacks approach is fine and should be much less work than implementing a DownloadChannelSaver. The signatures suggested there may be adjusted to match what you need here more closely. If a Download object has one of the callbacks set, it should be considered transient and not be serializable.

Ideally we would also set a flag in download history so these downloads cannot be retried as GET requests in other sessions, but that is an existing bug and we don't need to solve that problem now.

You can use bug 915036 to implement this functionality.

> I think those header restrictions are meant for web/extension developers,
> but I'd be glad to move them if Paolo thinks it would be useful elsewhere.

Yes, they belong to the WebExtension layer and not the internal Downloads API.
Attachment #8802425 - Flags: review?(paolo.mozmail) → review-
By the way, it seems you just need one callback to modify the channel, I didn't suggest that in bug 915036 because maybe it wasn't possible at the time or I didn't know about explicitSetUploadStream. This means that in bug 915036 you only need to test one case, for example setting a header. You also need a test that checks that serialization is disabled.

In this bug you can keep all the tests including those for POST data.
Attachment #8802786 - Flags: review?(aswan)
Depends on: 915036
Attachment #8802786 - Attachment is obsolete: true
Hey Andrew, I got r+ from Paolo in bug 915036. This patch includes that code, so asking r? only on extensions parts -- will remove jsdownloads code before landing.

(sorry for the slight mess, I blame mozreview, DynDNS, DDoS, closed trees.. in that order)
> Hey Andrew, I got r+ from Paolo in bug 915036. 

Pretend I didn't say anything after that.. ;)
Comment on attachment 8802425 [details]
bug 1247793 - Implement download() options: method, headers and body,

https://reviewboard.mozilla.org/r/86816/#review87098

Looking good, thanks.  Just a few little things to iron out.

::: toolkit/components/extensions/ext-downloads.js:428
(Diff revision 5)
>            return Promise.reject({message: "conflictAction prompt not yet implemented"});
>          }
>  
> +        // Handle method, headers and body options.
> +        function adjustChannel(channel) {
> +          if (channel instanceof Ci.nsIHttpChannel) {

Under what circumstances do we get here and have this test fail?

::: toolkit/components/extensions/ext-downloads.js:434
(Diff revision 5)
> +            const method = options.method || "GET";
> +            channel.requestMethod = method;
> +
> +            if (options.headers) {
> +              for (let {name, value} of options.headers) {
> +                // From https://fetch.spec.whatwg.org/#forbidden-header-name

Please move this comment up to where the list is defined

::: toolkit/components/extensions/ext-downloads.js:435
(Diff revision 5)
> +                if (!FORBIDDEN_HEADERS.includes(name.toUpperCase()) && !name.match(/^Proxy-|^Sec-/i)) {
> +                  channel.setRequestHeader(name, value, false);
> +                }

Please move this test to the beginning of download() and have it reject with an appropriate message if an illegal header is supplied.

::: toolkit/components/extensions/ext-downloads.js:441
(Diff revision 5)
> +                  channel.setRequestHeader(name, value, false);
> +                }
> +              }
> +            }
> +
> +            if (typeof options.body === "string") {

How about just `if (options.body != null)`?  The schema will enforce that if present, it much be a string.

::: toolkit/components/extensions/test/xpcshell/test_ext_downloads_post.js:52
(Diff revision 5)
> +    extension.sendMessage(options);
> +    return extension.awaitMessage("done");
> +  }
> +
> +  // Confirm received vs. expected values.
> +  function confirm(method, headers, body) {

This function goes along with the server setup code above, please put them together.

::: toolkit/components/extensions/test/xpcshell/test_ext_downloads_post.js:55
(Diff revision 5)
> +
> +  // Confirm received vs. expected values.
> +  function confirm(method, headers, body) {
> +    equal(received.method, method, "method is correct");
> +
> +    for (let name in (headers || {})) {

just give headers a default value in the function declaration

::: toolkit/components/extensions/test/xpcshell/test_ext_downloads_post.js:77
(Diff revision 5)
> +  let done = yield download({url});
> +  ok(done.ok, "download works without the method option, defaults to GET");
> +  confirm("GET");
> +
> +  done = yield download({url, method: "PUT"});
> +  ok(!done.ok, `download rejected with PUT method - ${done.err}`);

Please don't put the error message into the detail string, create a separate test to confirm that the error message is reasonable.
Likewise with similar tests below.

::: toolkit/components/extensions/test/xpcshell/xpcshell.ini:32
(Diff revision 5)
>  [test_ext_downloads.js]
>  [test_ext_downloads_download.js]
>  skip-if = os == "android"
>  [test_ext_downloads_misc.js]
>  skip-if = os == "android"
> +[test_ext_downloads_post.js]

Why is this a separate file?  Can you just add the code to the existing test_ext_downloads_download.js file
Attachment #8802425 - Flags: review?(aswan)
Comment on attachment 8802425 [details]
bug 1247793 - Implement download() options: method, headers and body,

https://reviewboard.mozilla.org/r/86816/#review87098

> Under what circumstances do we get here and have this test fail?

The `instanceof` is mainly used to `QuertyInterface` (for `requestMethod` and `setRequestHeader`), but without throwing.

Examples of other types of channels include `data:`, `file:`, `ftp:`, `blob:` or `moz-extension:` protocols.  I believe some of those are technically still `nsIHttpChannel`s, but that could be implementation detail.
Comment on attachment 8802425 [details]
bug 1247793 - Implement download() options: method, headers and body,

https://reviewboard.mozilla.org/r/86816/#review87098

> The `instanceof` is mainly used to `QuertyInterface` (for `requestMethod` and `setRequestHeader`), but without throwing.
> 
> Examples of other types of channels include `data:`, `file:`, `ftp:`, `blob:` or `moz-extension:` protocols.  I believe some of those are technically still `nsIHttpChannel`s, but that could be implementation detail.

Right so what should happen if I do `browser.downloads.download({url: "data:xyz", method: "POST", headers: {...} });`?
The code currently just silently ignores method, body, and headers for non http(s) urls, I think it would be better to trigger an error.
Comment on attachment 8802425 [details]
bug 1247793 - Implement download() options: method, headers and body,

https://reviewboard.mozilla.org/r/86816/#review87398

Nice, thanks!  Good to go after addressing the previous comment about method/headers/body with non-http(s) urls

::: toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js:310
(Diff revision 7)
> +    extension.sendMessage(options);
> +    return extension.awaitMessage("done");
> +  }
> +
> +  // Test method option.
> +  let done = yield download({});

nit: can you call this result instead of done?
Attachment #8802425 - Flags: review?(aswan) → review+
Comment on attachment 8802425 [details]
bug 1247793 - Implement download() options: method, headers and body,

https://reviewboard.mozilla.org/r/86816/#review87098

> Right so what should happen if I do `browser.downloads.download({url: "data:xyz", method: "POST", headers: {...} });`?
> The code currently just silently ignores method, body, and headers for non http(s) urls, I think it would be better to trigger an error.

Chrome silently ignores it.
Keywords: checkin-needed
could you fix the issue on mozreview so that we can use autoland to land this changes ? Thanks!
Flags: needinfo?(tomica)
Keywords: checkin-needed
There were no real open issues, but bug 1292371.  Steven MacLeod rejigged the count.
Flags: needinfo?(tomica)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2209fa8861c4
Implement download() options: method, headers and body, r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2209fa8861c4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
I've updated the compat data:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/downloads/download#Browser_compatibility

Let me know if we need anything else though.
Flags: needinfo?(tomica)
Thanks Will, though I noticed saveAs is also listed as not supported, but we fixed that in bug 1247791.
Flags: needinfo?(tomica) → needinfo?(wbamberg)
Oh thanks :zombie, good catch!
Flags: needinfo?(wbamberg)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: