-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
refactor src/core/pdf_manager.js: rename pdfModel to pdfDocument #4481
refactor src/core/pdf_manager.js: rename pdfModel to pdfDocument #4481
Conversation
This looks a bit strange, since most of these changes are already present in: https://github.com/mozilla/pdf.js/blob/master/src/core/pdf_manager.js. Did you by any chance base this PR on an older version of the repo? |
It's a fixup to an already merged version and should replace that commit. The only other option is to treat it as something completely new, which it isn't (at least in my eyes). I comes down on how you want to organise things. |
After squash (as done per wiki) the commit shall look like 8175753 |
Yes, lets add a test https://github.com/mozilla/pdf.js/blob/master/test/unit/api_spec.js |
I will see what I can do tomorrow morning |
Rename pdfModel to pdfDocument to let the name closer resemble what the variable actually holds
Added a test that correctly fails without this patch, I like jasmine |
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/e56b51525b4570b/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/0c15db400e5a551/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/e56b51525b4570b/output.txt Total script time: 25.83 mins
|
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/0c15db400e5a551/output.txt Total script time: 37.25 mins
|
refactor src/core/pdf_manager.js: rename pdfModel to pdfDocument
Thank you |
Fix #4480, only changes in src/core/worker.js below line 296 are new. It's amazing that all tests didn't find this error?!