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

In display/api.js ensure that we always reject with an Error in JpegDecode, and adjust a couple of other rejection sites as well #7595

Merged
merged 1 commit into from
Sep 5, 2016

Conversation

Snuffleupagus
Copy link
Collaborator

In the case where the document was destroyed, we were rejecting the Promise in JpegDecode with a string instead of an Error. The patch also brings the wording more inline with other such rejections.

Use the isInt utility function when validating the pageNumber parameter in WorkerTransport_getPage, to make it more obvious what's actually happening. There's also a couple more unit-tests added, to ensure that we always fail in the expected way.

Finally, we can simplify the rejection handling in WorkerTransport_getPageIndexByRef somewhat. (Note that the only reason for using catch here is that since the promise is rejected on the worker side, the reason becomes a string instead of an Error which is why we "re-reject" on the display side.)

@Snuffleupagus
Copy link
Collaborator Author

/botio unittest

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2016

From: Bot.io (Linux)


Received

Command cmd_unittest from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.21.233.14:8877/73fd2f137b27250/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2016

From: Bot.io (Windows)


Received

Command cmd_unittest from @Snuffleupagus received. Current queue size: 0

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

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2016

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/73fd2f137b27250/output.txt

Total script time: 2.10 mins

  • Unit Tests: Passed

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2016

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/eaad3be9db5f424/output.txt

Total script time: 2.12 mins

  • Unit Tests: Passed

@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2016

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/55c05b9a3c5865e/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2016

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/55c05b9a3c5865e/output.txt

Total script time: 1.25 mins

Published

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2016

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/ac57678d81e0e69/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2016

From: Bot.io (Windows)


Received

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

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

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2016

From: Bot.io (Windows)


Failed

Full output at http://107.22.172.223:8877/ffca5119a131d44/output.txt

Total script time: 23.89 mins

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

Image differences available at: http://107.22.172.223:8877/ffca5119a131d44/reftest-analyzer.html#web=eq.log

done();
var outOfRangePromise = doc.getPage(100);
var nonIntegerPromise = doc.getPage(2.5);
var nonNumberPromise = doc.getPage('pageNumber');
Copy link
Collaborator Author

@Snuffleupagus Snuffleupagus Sep 5, 2016

Choose a reason for hiding this comment

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

Whoops, this should have been a number given as a string, e.g. var nonNumberPromise = doc.getPage('1');, I've pushed a new version that fixes this.

@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Sep 5, 2016

I've pushed an updated patch addressing a test-only issue, as described in #7595 (comment); no functional changes were made!

…JpegDecode`, and adjust a couple of other rejection sites as well

In the case where the document was destroyed, we were rejecting the `Promise` in `JpegDecode` with a string instead of an `Error`. The patch also brings the wording more inline with other such rejections.

Use the `isInt` utility function when validating the `pageNumber` parameter in `WorkerTransport_getPage`, to make it more obvious what's actually happening. There's also a couple more unit-tests added, to ensure that we always fail in the expected way.

Finally, we can simplify the rejection handling in `WorkerTransport_getPageIndexByRef` somewhat. (Note that the only reason for using `catch` here is that since the promise is rejected on the worker side, the `reason` becomes a string instead of an `Error` which is why we "re-reject" on the display side.)
@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2016

From: Bot.io (Linux)


Failed

Full output at http://107.21.233.14:8877/ac57678d81e0e69/output.txt

Total script time: 39.55 mins

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

Image differences available at: http://107.21.233.14:8877/ac57678d81e0e69/reftest-analyzer.html#web=eq.log

@Snuffleupagus
Copy link
Collaborator Author

/botio unittest

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2016

From: Bot.io (Windows)


Received

Command cmd_unittest from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.22.172.223:8877/66e311e9cd9adcb/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2016

From: Bot.io (Linux)


Received

Command cmd_unittest from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.21.233.14:8877/496d44512067aaf/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2016

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/66e311e9cd9adcb/output.txt

Total script time: 2.02 mins

  • Unit Tests: Passed

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2016

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/496d44512067aaf/output.txt

Total script time: 2.31 mins

  • Unit Tests: Passed

@timvandermeij timvandermeij merged commit b26af7e into mozilla:master Sep 5, 2016
@timvandermeij
Copy link
Contributor

Looks good, thank you!

@Snuffleupagus Snuffleupagus deleted the api-reject-with-Error branch September 5, 2016 15:45
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.

3 participants