Closed
Bug 1247793
Opened 8 years ago
Closed 8 years ago
Implement POST for chrome.downloads.download()
Categories
(WebExtensions :: Request Handling, defect, P2)
WebExtensions
Request Handling
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
Updated•8 years ago
|
Priority: -- → P2
Whiteboard: [downloads] → [downloads] triaged
Updated•8 years ago
|
Component: WebExtensions: Untriaged → WebExtensions: Request Handling
Updated•8 years ago
|
Assignee: nobody → mixedpuppy
Updated•8 years ago
|
Assignee: mixedpuppy → tomica
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8802425 -
Flags: review?(aswan)
Assignee | ||
Comment 3•8 years ago
|
||
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
Reporter | ||
Comment 4•8 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•8 years ago
|
||
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 8•8 years ago
|
||
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-
Comment 9•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Attachment #8802786 -
Flags: review?(aswan)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8802786 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•8 years ago
|
||
> Hey Andrew, I got r+ from Paolo in bug 915036.
Pretend I didn't say anything after that.. ;)
Reporter | ||
Comment 14•8 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 16•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Reporter | ||
Comment 18•8 years ago
|
||
mozreview-review-reply |
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.
Reporter | ||
Comment 19•8 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 20•8 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 22•8 years ago
|
||
could you fix the issue on mozreview so that we can use autoland to land this changes ? Thanks!
Flags: needinfo?(tomica)
Keywords: checkin-needed
Assignee | ||
Comment 23•8 years ago
|
||
There were no real open issues, but bug 1292371. Steven MacLeod rejigged the count.
Flags: needinfo?(tomica)
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 24•8 years ago
|
||
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
Comment 25•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2209fa8861c4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 26•7 years ago
|
||
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)
Assignee | ||
Comment 27•7 years ago
|
||
Thanks Will, though I noticed saveAs is also listed as not supported, but we fixed that in bug 1247791.
Flags: needinfo?(tomica) → needinfo?(wbamberg)
Comment 28•7 years ago
|
||
Oh thanks :zombie, good catch!
Flags: needinfo?(wbamberg)
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•