Closed Bug 1245644 Opened 4 years ago Closed 4 years ago

Implement chrome.downloads.removeFile()

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

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
Blocks: 1213445
Whiteboard: [downloads]
Whiteboard: [downloads] → [downloads][good first bug]
QA Contact: scolville
Assignee: nobody → scolville
QA Contact: scolville
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/
Attachment #8730219 - Flags: review?(kmaglione+bmo)
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 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 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+
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
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/
Patch has been rebased
Flags: needinfo?(scolville)
Keywords: checkin-needed
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8096447&repo=fx-team
Flags: needinfo?(scolville)
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/
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)
https://hg.mozilla.org/mozilla-central/rev/73977dc040ff
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.