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

Remove combineUrl from the test files #7183

Closed
timvandermeij opened this issue Apr 11, 2016 · 8 comments
Closed

Remove combineUrl from the test files #7183

timvandermeij opened this issue Apr 11, 2016 · 8 comments

Comments

@timvandermeij
Copy link
Contributor

Take a look at https://github.com/mozilla/pdf.js/search?utf8=%E2%9C%93&q=combineUrl for where it is still used. We should replace it with the URL constructor as done in https://github.com/mozilla/pdf.js/pull/7178/files.

@yurydelendik
Copy link
Contributor

We still need to test the combineUrl function (e.g. keep it in util_spec) or remove that from our code.

@timvandermeij
Copy link
Contributor Author

Good point. Now that I come to think of it, couldn't we remove it entirely since we have a polyfill for it (at

//// Polyfill from https://github.com/Polymer/URL
)?

@yurydelendik
Copy link
Contributor

@brendandahl already wanted to remove that. Please notice that we shall properly handle if (!url) { case when it is needed.

@prakashpalanisamy
Copy link
Contributor

Hi, This will be my first contribution, is it okay if I take this issue up and work on it?

@yurydelendik
Copy link
Contributor

Hi, This will be my first contribution, is it okay

Sure, please read https://github.com/mozilla/pdf.js/wiki/Contributing

@prakashpalanisamy
Copy link
Contributor

Thank you..

@prakashpalanisamy
Copy link
Contributor

there is a source file having reference to combineUrl - api.js in display. Should I replace that refernece too? Or only the test file references?
If src refernces are to be replaced, Should the function also be removed from util.js? - assuming that it will not be used in the future.

@yurydelendik
Copy link
Contributor

there is a source file having reference to combineUrl - api.js in display. Should I replace that refernece too?

Yes, you can replace that, remove tests and from util.js.

prakashpalanisamy added a commit to prakashpalanisamy/pdf.js that referenced this issue Apr 15, 2016
timvandermeij added a commit that referenced this issue Apr 15, 2016
Remove `combineUrl` and replace it with `new URL`. Issue #7183, for reference.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants