Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure downloaded files have a ".pdf" extension #2515

Closed
wants to merge 2 commits into from
Closed

Make sure downloaded files have a ".pdf" extension #2515

wants to merge 2 commits into from

Conversation

waddlesplash
Copy link
Contributor

Partially addresses #2407. I couldn't find a way to get the HTTP headers without sending an XMLHttpRequest and reading the headers sent back, but there are plenty of reasons to NOT do that.

This patch simply ensures there is a .pdf extension on the file (e.g if you are at a URL that ends in .php because a PHP script generated the PDF, this adds a .pdf to the file, so it is .php.pdf)

@waddlesplash
Copy link
Contributor Author

Can someone preview & lint?
There shouldn't be any errors, I already linted locally.

@jviereck
Copy link
Contributor

/botio-windows preview

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_preview from @jviereck received. Current queue size: 0

Live output at: http://107.22.172.223:8877/c1493e4aba918e1/output.txt

@jviereck
Copy link
Contributor

/botio-windows lint

@pdfjsbot
Copy link

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_lint from @jviereck received. Current queue size: 0

Live output at: http://107.22.172.223:8877/4bf6f3e5646b8f9/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/4bf6f3e5646b8f9/output.txt

Total script time: 1.20 mins

  • Lint: Passed

@waddlesplash
Copy link
Contributor Author

Any changes I should make?

@@ -1024,8 +1024,16 @@ var PDFView = {
// function getDataSuccess(data) {
// var blob = PDFJS.createBlob(data.buffer, 'application/pdf');
// var blobUrl = window.URL.createObjectURL(blob);
// var endOk = (url.toLowerCase().indexOf('.pdf', url.length - 4) !== -1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the more concise:

// Append .pdf to the filename if it isn't already there
url += (url.toLowerCase().indexOf('.pdf', url.length - 4) !== -1) ? '.pdf' : ''

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now why didn't I think of that! 👍

@waddlesplash
Copy link
Contributor Author

OK, added the change @saebekassebil suggested and squashed.

@jviereck
Copy link
Contributor

/botio-windows preview
/botio-windows lint

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_preview from @jviereck received. Current queue size: 0

Live output at: http://107.22.172.223:8877/dabaad54c30b696/output.txt

@pdfjsbot
Copy link

@waddlesplash
Copy link
Contributor Author

@yurydelendik I appear to have gotten into something over my head... 😕. I just commited my WIP changes, which completely breaks downloading and nothing appears in the error log... What am I doing wrong here?

@yurydelendik
Copy link
Contributor

The proper place to get content disposition will be near https://github.com/waddlesplash/pdf.js/blob/5eb7f171f66b2ccad144f7451f08f2660a144c06/extensions/firefox/components/PdfStreamConverter.js#L598
You can use the contentDispositionFilename property of aRequest instead of parsing yourself:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIChannel.idl#294 . Notice that exception will be thrown if contentDispositionFilename is not available.

@yurydelendik
Copy link
Contributor

You can use log() function to place message to error console (see web developer->error console)

@yurydelendik
Copy link
Contributor

@waddlesplash, thank you for looking into this issue. We will be releasing #2635 to fix the issue #2407.

@waddlesplash
Copy link
Contributor Author

Thanks, @brendandahl and @yurydelendik. It would've taken me weeks to figure all that out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants