Closed
Bug 1245644
Opened 9 years ago
Closed 9 years ago
Implement chrome.downloads.removeFile()
Categories
(WebExtensions :: General, defect)
WebExtensions
General
Tracking
(firefox48 fixed)
RESOLVED
FIXED
mozilla48
| Tracking | Status | |
|---|---|---|
| firefox48 | --- | fixed |
People
(Reporter: aswan, Assigned: scolville)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, Whiteboard: [downloads][good first bug])
Attachments
(1 file)
Part of supporting chrome.downloads in WebExtensions
https://developer.chrome.com/extensions/downloads#method-removeFile
|
||
Updated•9 years ago
|
Whiteboard: [downloads]
|
Reporter | |
Updated•9 years ago
|
Whiteboard: [downloads] → [downloads][good first bug]
Review commit: https://reviewboard.mozilla.org/r/39757/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39757/
Attachment #8730219 -
Flags: review?(kmaglione+bmo)
|
|
Reporter | |
Comment 2•9 years ago
|
||
Stuart, you should run eslint over your patch, if it gets checked in with lint errors, it will quickly get backed out (but Kris will flag them before giving r+ anyway).
Just run "mach eslint toolkit/components/extensions" (you may need to run "mach eslint --setup" first)
Comment on attachment 8730219 [details]
MozReview Request: bug 1245644 add chrome.downloads.removeFile r?kmag
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39757/diff/1-2/
|
||
Updated•9 years ago
|
Attachment #8730219 -
Flags: review?(kmaglione+bmo)
|
|
||
Comment 4•9 years ago
|
||
Comment on attachment 8730219 [details]
MozReview Request: bug 1245644 add chrome.downloads.removeFile r?kmag
https://reviewboard.mozilla.org/r/39757/#review36551
::: toolkit/components/extensions/ext-downloads.js:397
(Diff revision 2)
> return item.id;
> });
> },
>
> + removeFile(downloadId) {
> + const item = DownloadMap.fromId(downloadId);
We need to handle the case where download ID is invalid, and return the expected error. We should also have a test for this.
::: toolkit/components/extensions/ext-downloads.js:398
(Diff revision 2)
> });
> },
>
> + removeFile(downloadId) {
> + const item = DownloadMap.fromId(downloadId);
> + if (!item.State === "complete") {
This is comparing `true === "complete"`, so it'll always be false.
::: toolkit/components/extensions/ext-downloads.js:399
(Diff revision 2)
> },
>
> + removeFile(downloadId) {
> + const item = DownloadMap.fromId(downloadId);
> + if (!item.State === "complete") {
> + return Promise.reject("downloadItem.State is not complete");
This needs to be an object with a "message" property. I don't think this message will mean much to a user. Maybe "Cannot remove file from an incomplete download"?
::: toolkit/components/extensions/ext-downloads.js:401
(Diff revision 2)
> + removeFile(downloadId) {
> + const item = DownloadMap.fromId(downloadId);
> + if (!item.State === "complete") {
> + return Promise.reject("downloadItem.State is not complete");
> + }
> + return OS.File.remove(item.filename);
If the file doesn't exist, or can't be removed, this promise will reject, and the extension will get an "An unexpected error occurred" error. We need to handle the rejection and return a better error. We should also test for this and the previous error cases.
::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_removefile.html:90
(Diff revision 2)
> + SimpleTest.registerCleanupFunction(() => {
> + Services.prefs.clearUserPref("browser.download.folderList");
> + Services.prefs.clearUserPref("browser.download.dir");
> + });
> +
> + SimpleTest.requestCompleteLog();
Please remove this line.
Comment on attachment 8730219 [details]
MozReview Request: bug 1245644 add chrome.downloads.removeFile r?kmag
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39757/diff/2-3/
Attachment #8730219 -
Flags: review?(kmaglione+bmo)
Comment on attachment 8730219 [details]
MozReview Request: bug 1245644 add chrome.downloads.removeFile r?kmag
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39757/diff/3-4/
|
|
||
Comment 7•9 years ago
|
||
Comment on attachment 8730219 [details]
MozReview Request: bug 1245644 add chrome.downloads.removeFile r?kmag
https://reviewboard.mozilla.org/r/39757/#review36957
::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_removefile.html:1
(Diff revision 4)
> +<!DOCTYPE HTML>
Can we just add these tests to test_chrome_ext_downloads_misc.html rather than duplicating all of this code?
::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_removefile.html:192
(Diff revision 4)
> + msg = yield runInExtension("removeFile", id);
> + is(msg.status, "success", "removeFile() succeeded");
> +
> + msg = yield runInExtension("removeFile", id);
> + is(msg.status, "error", "removeFile() fails since the file was already removed.");
> + ok(/file doesn't exist/.test(msg.errmsg), "error", "removeFile() failed on removed file.");
The "error" argument is not necessary for any of these `ok` calls.
::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_removefile.html:199
(Diff revision 4)
> + msg = yield runInExtension("removeFile", "bogus");
> + is(msg.status, "error", "removeFile() failed due to bad type");
> + ok(/Incorrect argument types for downloads.removeFile/.test(msg.errmsg), "error", "removeFile() failed");
> +
> + msg = yield runInExtension("removeFile", "bogus");
> + ok(/Incorrect argument types/.test(msg.errmsg), "error", "removeFile() failed due to bad type");
Why do we need to test this twice?
Attachment #8730219 -
Flags: review?(kmaglione+bmo)
Comment on attachment 8730219 [details]
MozReview Request: bug 1245644 add chrome.downloads.removeFile r?kmag
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39757/diff/4-5/
Attachment #8730219 -
Flags: review?(kmaglione+bmo)
|
|
||
Comment 9•9 years ago
|
||
Comment on attachment 8730219 [details]
MozReview Request: bug 1245644 add chrome.downloads.removeFile r?kmag
https://reviewboard.mozilla.org/r/39757/#review36973
Attachment #8730219 -
Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed
|
||
Comment 10•9 years ago
|
||
has problems to apply: applying 262b9ea0d3f5
patching file toolkit/components/extensions/ext-downloads.js
Hunk #1 FAILED at 405
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/extensions/ext-downloads.js.rej
patching file toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_misc.html
Hunk #1 FAILED at 459
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_misc.html.rej
Flags: needinfo?(scolville)
Keywords: checkin-needed
|
Assignee | |
Comment 11•9 years ago
|
||
Comment on attachment 8730219 [details]
MozReview Request: bug 1245644 add chrome.downloads.removeFile r?kmag
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39757/diff/5-6/
|
|
Assignee | |
Comment 12•9 years ago
|
||
Patch has been rebased
Flags: needinfo?(scolville)
Keywords: checkin-needed
|
||
Comment 13•9 years ago
|
||
Keywords: checkin-needed
|
|
||
Comment 14•9 years ago
|
||
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8096447&repo=fx-team
Flags: needinfo?(scolville)
|
|
||
Comment 15•9 years ago
|
||
|
|
Assignee | |
Comment 16•9 years ago
|
||
Comment on attachment 8730219 [details]
MozReview Request: bug 1245644 add chrome.downloads.removeFile r?kmag
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39757/diff/6-7/
|
|
Assignee | |
Comment 17•9 years ago
|
||
Test failures fixed by re-arranging test order due to expected events not being triggered in the erase tests which are unrelated to this patch. A fix for that is coming separately.
Flags: needinfo?(scolville)
Keywords: checkin-needed
|
|
||
Comment 18•9 years ago
|
||
Keywords: checkin-needed
|
||
Updated•9 years ago
|
Keywords: dev-doc-needed
|
|
||
Comment 19•9 years ago
|
||
| bugherder | ||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
|
||
Comment 20•9 years ago
|
||
Keywords: dev-doc-needed → dev-doc-complete
|
||
Updated•6 years ago
|
Product: Toolkit → WebExtensions
|
||
Updated•2 years ago
|
Component: Untriaged → General
You need to log in
before you can comment on or make changes to this bug.
Description
•