Require destinations when they are needed and do not fetch all of them in advance #5351
Conversation
timvandermeij
added
4-work-in-progress
core
viewer
performance
and removed
viewer
core
labels
timvandermeij
force-pushed
the
destinations
branch
from
0f65123 to
ae7492f
Compare
timvandermeij
force-pushed
the
destinations
branch
2 times, most recently
from
a9ba717 to
54109aa
Compare
|
@yurydelendik This PR is ready for review now that I also implemented the binary search ( |
|
Seems to work just fine, nicely done! Should we add a comment to the current Also, please include a unit test! Something like this would probably suffice (untested, basically copied from api_spec.js#L119-L125): it('gets destination', function() {
var promise = doc.getDestination('chapter1');
waitsForPromiseResolved(promise, function(data) {
expect(data).toEqual([{ gen: 0, num: 17 }, { name: 'XYZ' },
0, 841.89, null]);
});
}); |
timvandermeij
force-pushed
the
destinations
branch
2 times, most recently
from
cf3ca4a to
695b3c1
Compare
|
@Snuffleupagus Thank you. Both comments have been fixed. |
| var kids = root.get('Kids'); | ||
| var names = root.get('Names'); | ||
|
|
||
| if (isArray(kids)) { |
There was a problem hiding this comment.
you shall repeat this as a loop until 'Kids' are found (for nested kids)
There was a problem hiding this comment.
Fixed in the new commit.
|
Really good. Good to go with unneeded recursion calls removed and recursion limit set. |
| */ | ||
| getDestinations: function PDFDocumentProxy_getDestinations() { | ||
| return this.transport.getDestinations(); | ||
| }, | ||
| /** | ||
| * @return {Promise} A promise that is resolved with a all information | ||
| * for a given destination ID. |
There was a problem hiding this comment.
"... with a all ..." looks like a typo; also this sentence can probably be simplified somewhat if you add documentation for the parameter.
There was a problem hiding this comment.
Fixed in the new commit.
timvandermeij
force-pushed
the
destinations
branch
4 times, most recently
from
5904134 to
64efda5
Compare
| while (kidsOrNames.has('Kids')) { | ||
| var kids = kidsOrNames.get('Kids'); | ||
|
|
||
| if (isArray(kids)) { |
There was a problem hiding this comment.
change that to if (!isArray(kids)) { return null; }
There was a problem hiding this comment.
Fixed in the new commit.
timvandermeij
force-pushed
the
destinations
branch
from
64efda5 to
db8ba13
Compare
timvandermeij
force-pushed
the
destinations
branch
2 times, most recently
from
bb12feb to
17a4283
Compare
|
Looks good |
timvandermeij
force-pushed
the
destinations
branch
from
17a4283 to
aaa1f2c
Compare
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/8e54d03722943b5/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/8e54d03722943b5/output.txt Total script time: 0.77 mins Published
|
|
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/5dbc517e79e25eb/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://107.22.172.223:8877/0c5e21bda646117/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/0c5e21bda646117/output.txt Total script time: 19.65 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/5dbc517e79e25eb/output.txt Total script time: 22.76 mins
|
Require destinations when they are needed and do not fetch all of them in advance
|
Thank you for the patch |
|
Minor version number needs to be bumped |
Fixes #5266.