Closed Bug 1280044 Opened 7 years ago Closed 6 years ago

browser.downloads.download failed with subdirectories in filename

Categories

(WebExtensions :: Untriaged, defect, P3)

50 Branch
defect

Tracking

(firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: mail, Assigned: zombie, Mentored)

Details

(Whiteboard: [downloads] triaged)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.84 Safari/537.36

Steps to reproduce:

Run next code in Web Extension:

var blob = new Blob(['text'], {
    type: 'text/plain'
});
var url = URL.createObjectURL(blob);
browser.downloads.download({
    url: url,
    filename: 'one/file.txt'
});



Actual results:

Download has failed.


Expected results:

Under the default downloads folder (from browser settings) Firefox should create directory "one" and file in it "file.txt" with content from blob object. This is how extensions work in chrome: https://developer.chrome.com/extensions/downloads#method-download (see filename property description).
Mentor: aswan
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: [downloads] triaged
Assignee: nobody → tomica
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8789861 [details]
bug 1280044 - handle subdirs in browser.downloads filenames,

https://reviewboard.mozilla.org/r/77916/#review76402

Thanks for working on this!  A couple of things below and this will be ready to go.
Also please do a try run on all platforms.

::: toolkit/components/extensions/ext-downloads.js:420
(Diff revision 1)
>            // TODO
>            // if (options.saveAs) { }
>  
>            let target;
>            if (options.filename) {
> -            target = OS.Path.join(downloadsDir, options.filename);
> +            // can't use OS.Path.split(), but explicitly split on "/" instead

What's the purpose of this change?  And why wouldn't OS.Path.split() be suitable?

::: toolkit/components/extensions/ext-downloads.js:437
(Diff revision 1)
>  
> +          // handle filenames with subdirs
> +          const dir = OS.Path.dirname(target);
> +          return OS.File.exists(dir).then(dirExists => {
> +            if (!dirExists) {
> +              return OS.File.makeDir(dir, {from: downloadsDir})

lets not return early here, if somebody wants to add logic below later this will trip them up.  just chain the remaining logic into another then handler.

::: toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js:123
(Diff revision 1)
>    function testDownload(options, localFile, expectedSize, description) {
>      return download(options).then(msg => {
>        equal(msg.status, "success", `downloads.download() works with ${description}`);
>        return waitForDownloads();
>      }).then(() => {
> +      let parts = localFile.split("/");

Lets just pass the array of parts in here instead of splitting the string.  but keep localFile working as a string, i.e. something like:
```
parts = Array.isArrary(localFile) ? localFile : [localFile];
```

::: toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js:152
(Diff revision 1)
> +  // Call download() with a filename with subdirs.
> +  yield testDownload({
> +    url: FILE_URL,
> +    filename: "sub/dir/file",
> +  }, "sub/dir/file", FILE_LEN, "source and filename with subdirs");
> +

can you add a test hitting sub/dir/file2 here to make sure everything works when the target is an existing subdirectory.
To do that, you'll need to take the logic for removing the subdirectory from testDownload() and manually do `file.remove("sub")` after the second test.
Attachment #8789861 - Flags: review?(aswan)
Comment on attachment 8789861 [details]
bug 1280044 - handle subdirs in browser.downloads filenames,

https://reviewboard.mozilla.org/r/77916/#review76402

> What's the purpose of this change?  And why wouldn't OS.Path.split() be suitable?

running OS.Path.split() on Windows splits on backslash "\\", so the example doesn't work.  slashes "/" are the cross-platform way to write paths in js extensions (as well as node.js, etc), and I even thought that using a backslash wouldn't work. I just tested, and it seems to work, so I'm gonna try a different approach.
Comment on attachment 8789861 [details]
bug 1280044 - handle subdirs in browser.downloads filenames,

https://reviewboard.mozilla.org/r/77916/#review76696

::: toolkit/components/extensions/ext-downloads.js:396
(Diff revision 2)
>    let {extension} = context;
>    return {
>      downloads: {
>        download(options) {
> -        if (options.filename != null) {
> -          if (options.filename.length == 0) {
> +        let filename = options.filename;
> +        if (filename && PlatformInfo.os === "win") {

Okay, I see what you're trying to do here.  Can you verify what Chrome does in this case?  (i.e., this means that it would not be possible to create a file with a / in the name on windows, which is an otherwise valid thing to do).  Also, this code will need a test (i.e., test that both types of relative paths work on windows)

::: toolkit/components/extensions/ext-downloads.js:427
(Diff revision 2)
> -            target = OS.Path.join(downloadsDir, options.filename);
> +            const path = OS.Path.split(filename);
> +            target = OS.Path.join(downloadsDir, ...path.components);

Why is this change necessary?

::: toolkit/components/extensions/ext-downloads.js:432
(Diff revision 2)
> -            target = OS.Path.join(downloadsDir, options.filename);
> +            const path = OS.Path.split(filename);
> +            target = OS.Path.join(downloadsDir, ...path.components);
>            } else {
>              let uri = NetUtil.newURI(options.url);
>  
>              let filename;

this is now shadowing filename from the enclosing scope, I think the simplest thing is to just remove this line.

::: toolkit/components/extensions/ext-downloads.js:439
(Diff revision 2)
>                filename = uri.fileName;
>              }
>              target = OS.Path.join(downloadsDir, filename || "download");
>            }
>  
> +          // handle filenames with subdirs

Please start the comment with a capital letter and end with a period, and make this more clear (e.g., "Create any needed intermediate directories if the target is in a subdirectory")
Attachment #8789861 - Flags: review?(aswan)
Comment on attachment 8789861 [details]
bug 1280044 - handle subdirs in browser.downloads filenames,

https://reviewboard.mozilla.org/r/77916/#review76696

> Why is this change necessary?

it was in the previous patch, reverted.
Comment on attachment 8789861 [details]
bug 1280044 - handle subdirs in browser.downloads filenames,

https://reviewboard.mozilla.org/r/77916/#review76696

> this is now shadowing filename from the enclosing scope, I think the simplest thing is to just remove this line.

I dislike modifying the variable from the outher scope even more. renamed instead.
Comment on attachment 8789861 [details]
bug 1280044 - handle subdirs in browser.downloads filenames,

https://reviewboard.mozilla.org/r/77916/#review77334

Nice, thank you!
Attachment #8789861 - Flags: review?(aswan) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/c22083b56078
handle subdirs in browser.downloads filenames, r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c22083b56078
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment on attachment 8789861 [details]
bug 1280044 - handle subdirs in browser.downloads filenames,

https://reviewboard.mozilla.org/r/77916/#review167414

::: toolkit/components/extensions/ext-downloads.js:431
(Diff revision 3)
> -            let filename;
> +            let remote = "download";
>              if (uri instanceof Ci.nsIURL) {
> -              filename = uri.fileName;
> +              remote = uri.fileName;
>              }
> -            target = OS.Path.join(downloadsDir, filename || "download");
> +            target = OS.Path.join(downloadsDir, remote);

This change is wrong, filename will still be empty if the URL doesn't include one.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.