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

changed decodeURI to decodeURIComponent: Fixes #8987 #9364

Conversation

shikhar-scs
Copy link
Contributor

pdf.js node extension now opens file containing the character # in the file name or the path of the file.

Read the following snippet from here

The main difference between `decodeURI` and `decodeURIComponent` is that:
     -decodeURI is intended for use on the full URI.
     -decodeURIComponent is intended to be used on URI components, that is, any part that lies between separators (; / ? : @ & = + $ , #).
So, in decodeURIComponent these separators are decoded also because they are regarded as text and not special characters.

Solves #8987

Thank you @mukulmishra18 for your help.

Please review.

Copy link
Contributor

@mukulmishra18 mukulmishra18 left a comment

Choose a reason for hiding this comment

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

Looks good to me. @timvandermeij can you please have a look and run the tests to see if we are good to go.

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/c72690391793dbb/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.215.176.217:8877/47644de70f0b947/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/47644de70f0b947/output.txt

Total script time: 23.30 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/47644de70f0b947/reftest-analyzer.html#web=eq.log

@shikhar-scs
Copy link
Contributor Author

shikhar-scs commented Jan 15, 2018

@timvandermeij you have any idea why are these tests failing on windows??

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/c72690391793dbb/output.txt

Total script time: 38.15 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@timvandermeij timvandermeij merged commit 1ad33c4 into mozilla:master Jan 15, 2018
@timvandermeij
Copy link
Contributor

Thank you for fixing this! Note that the reference test failure is a known intermittent problem and can be ignored for this patch.

@shikhar-scs
Copy link
Contributor Author

oh thanx a lot @timvandermeij @mukulmishra18 😄

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
…decodeURIcomponent

changed decodeURI to decodeURIComponent: Fixes mozilla#8987
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants