[api-minor] Implement a permissions API #10033
Conversation
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/58015d08e6b16dc/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/3d7a20169b2726a/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/58015d08e6b16dc/output.txt Total script time: 19.44 mins
|
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/3d7a20169b2726a/output.txt Total script time: 24.59 mins
|
|
/botio-linux fonttest |
From: Bot.io (Linux m4)ReceivedCommand cmd_fonttest from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/29d10d6e619a0ff/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/29d10d6e619a0ff/output.txt Total script time: 1.76 mins
|
|
/botio-windows fonttest |
From: Bot.io (Windows)ReceivedCommand cmd_fonttest from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/e307900554adc4f/output.txt |
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/e307900554adc4f/output.txt Total script time: 3.23 mins
|
src/display/api.js
Outdated
| this._fullReader.cancel(reason); | ||
| setupMessageHandler() { | ||
| const messageHandler = this.messageHandler; | ||
| const loadingTask = this.loadingTask; |
There was a problem hiding this comment.
Optional change, but this could be shortened to const { messageHandler, loadingTask, } = this; instead.
src/display/api.js
Outdated
| if (this.destroyed) { | ||
| return Promise.reject(new Error('Worker was destroyed')); | ||
| } | ||
| if (this.loadingTask.onProgress) { |
There was a problem hiding this comment.
Can't you use loadingTask as defined in the surrounding scope, both here and on the next line?
| // PDF integer objects are represented internally in signed 2's complement | ||
| // form. Therefore, convert the signed decimal integer to a signed 2's | ||
| // complement binary integer so we can use regular bitwise operations on it. | ||
| flags += 2 ** 32; |
There was a problem hiding this comment.
I assuming that you've already checked, but otherwise please make sure that Webpack/Babel is able to convert this into code compatible with older browsers. (Note that I'm expecting this to "just work" given this option).
There was a problem hiding this comment.
Good point. I found that babel-present-env fortunately includes the exponentiation operator plugin (see https://github.com/babel/babel/blob/master/packages/babel-preset-env/package.json#L36), so that should be fine.
|
|
||
| const permissions = []; | ||
| for (const key in PermissionFlag) { | ||
| const value = PermissionFlag[key]; |
There was a problem hiding this comment.
If you include something along these lines e59f952, then you ought to be able to use for...of instead here:
for (const flag of PermissionFlag) {
if (flags & flag) {
...
}There was a problem hiding this comment.
I tried this and unfortunately it doesn't work. Objects, unlike other data structures such as Maps, are not iterable in JavaScript. According to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/is_not_iterable there are some other ways to do this but I don't think it's much more readable.
There was a problem hiding this comment.
I totally overlooked that it's an Object, sorry about wasting your time here.
src/display/api.js
Outdated
| } | ||
|
|
||
| /** | ||
| * Cleans up resources allocated by the document, e.g., created `@font-face`. |
There was a problem hiding this comment.
Nit: Remove the comma placed after e.g.
src/display/api.js
Outdated
| @@ -691,6 +691,14 @@ class PDFDocumentProxy { | |||
| return this._transport.getOutline(); | |||
| } | |||
|
|
|||
| /** | |||
| * @return {Promise} A promise that is resolved with an {Array} that contains | |||
| * the permission flags for the PDF document. | |||
There was a problem hiding this comment.
Similar to the getPageLabel JSDocs, please mention the null case as well.
test/unit/api_spec.js
Outdated
| expect(permissions).toEqual(null); | ||
|
|
||
| done(); | ||
| }).catch(function(reason) { |
There was a problem hiding this comment.
Here, and elsewhere below, the slightly less verbose }).catch(done.fail); ought to work.
… syntax Moreover, indicate that a member are private and improve the comments to be more consistent.
timvandermeij
force-pushed
the
permissions-api
branch
from
b4401d9
to
e812c6e
Compare
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/b438b966042ab6c/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/b438b966042ab6c/output.txt Total script time: 2.99 mins Published |
|
/botio unittest |
From: Bot.io (Linux m4)ReceivedCommand cmd_unittest from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/49ae4a9a7ea1de2/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/cb3e726cc626952/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/49ae4a9a7ea1de2/output.txt Total script time: 3.72 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/cb3e726cc626952/output.txt Total script time: 5.28 mins
|
Fixes #9972.
@Snuffleupagus Would you be willing to review this if you have time? I recommend to review this per commit and with the
?w=1flag. For some reason the?w=1flag doesn't seem to work on GitHub for the second commit, but it does locally if you usegit show HEAD~1 --ignore-all-spacewith this branch checked out.