From a04a5d8325ff967a26f4baae3413a3d061c3f29f Mon Sep 17 00:00:00 2001
From: Jonas Jenwald <jonas.jenwald@gmail.com>
Date: Sun, 21 Jun 2020 11:29:05 +0200
Subject: [PATCH 1/3] Tweak the loop in `ChunkedStreamManager.abort` to clarify
what's being iterated (PR 11985 follow-up)
In hindsight, using the `for (let [key, value] of myMap) { ... }`-format when we don't care about the `key` probably wasn't such a great idea. Since `Map`s have explicit support for iterating either `key`s or `value`s, we should probably use that instead here.
---
src/core/chunked_stream.js | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/core/chunked_stream.js b/src/core/chunked_stream.js
index f041f8bc0..c764c7181 100644
--- a/src/core/chunked_stream.js
+++ b/src/core/chunked_stream.js
@@ -591,7 +591,7 @@ class ChunkedStreamManager {
if (this.pdfNetworkStream) {
this.pdfNetworkStream.cancelAllRequests(reason);
}
- for (const [, capability] of this._promisesByRequest) {
+ for (const capability of this._promisesByRequest.values()) {
capability.reject(reason);
}
}
From cabc2cc4fcc5c2361c693e4324419f3b49b2479b Mon Sep 17 00:00:00 2001
From: Jonas Jenwald <jonas.jenwald@gmail.com>
Date: Sun, 21 Jun 2020 15:38:09 +0200
Subject: [PATCH 2/3] Add a `InternalRenderTask.completed` getter and use it to
simplify `PDFPageProxy._destroy`
This patch aims to simplify the `PDFPageProxy._destroy` method, by:
- Replacing the unnecessary `forEach` with a "regular" `for`-loop instead.
- Use a more appropriate variable name, since `intentState.renderTasks` contain instances of `InternalRenderTask`.
- Move the "is rendering completed"-handling to a new `InternalRenderTask.completed` getter, to abstract away some (mostly) internal `InternalRenderTask` state.
---
src/display/api.js | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/display/api.js b/src/display/api.js
index 8a556c40a..784995825 100644
--- a/src/display/api.js
+++ b/src/display/api.js
@@ -1234,13 +1234,10 @@ class PDFPageProxy {
// Avoid errors below, since the renderTasks are just stubs.
return;
}
- intentState.renderTasks.forEach(function (renderTask) {
- const renderCompleted = renderTask.capability.promise.catch(
- function () {}
- ); // ignoring failures
- waitOn.push(renderCompleted);
- renderTask.cancel();
- });
+ for (const internalRenderTask of intentState.renderTasks) {
+ waitOn.push(internalRenderTask.completed);
+ internalRenderTask.cancel();
+ }
});
this.objs.clear();
this.annotationsPromise = null;
@@ -2650,6 +2647,13 @@ const InternalRenderTask = (function InternalRenderTaskClosure() {
this._canvas = params.canvasContext.canvas;
}
+ get completed() {
+ return this.capability.promise.catch(function () {
+ // Ignoring errors, since we only want to know when rendering is
+ // no longer pending.
+ });
+ }
+
initializeGraphics(transparency = false) {
if (this.cancelled) {
return;
From 4cb0c032f3753dcc82077551ced0d7f9da430187 Mon Sep 17 00:00:00 2001
From: Jonas Jenwald <jonas.jenwald@gmail.com>
Date: Sun, 21 Jun 2020 16:42:39 +0200
Subject: [PATCH 3/3] Convert the `PDFPageProxy.intentStates` property from an
`Object` to a `Map`
As can be seen in the code there's a handful of places where this structure needs to be iterated, something that becomes cumbersome when dealing with `Object`s. Hence, by changing this to a `Map` instead we can both simplify the code and avoid creating unnecessary closures.
Particularily the `PDFPageProxy._tryCleanup` method becomes a lot more readable, at least in my opinion.
Finally, since this property is intended to be "private" the name is adjusted to reflect that.
---
src/display/api.js | 60 +++++++++++++++++++++-------------------------
1 file changed, 27 insertions(+), 33 deletions(-)
diff --git a/src/display/api.js b/src/display/api.js
index 784995825..9c524809c 100644
--- a/src/display/api.js
+++ b/src/display/api.js
@@ -898,7 +898,7 @@ class PDFPageProxy {
this.cleanupAfterRender = false;
this.pendingCleanup = false;
- this.intentStates = Object.create(null);
+ this._intentStates = new Map();
this.destroyed = false;
}
@@ -1003,10 +1003,11 @@ class PDFPageProxy {
// this call to render.
this.pendingCleanup = false;
- if (!this.intentStates[renderingIntent]) {
- this.intentStates[renderingIntent] = Object.create(null);
+ let intentState = this._intentStates.get(renderingIntent);
+ if (!intentState) {
+ intentState = Object.create(null);
+ this._intentStates.set(renderingIntent, intentState);
}
- const intentState = this.intentStates[renderingIntent];
// Ensure that a pending `streamReader` cancel timeout is always aborted.
if (intentState.streamReaderCancelTimeout) {
@@ -1128,14 +1129,15 @@ class PDFPageProxy {
}
const renderingIntent = "oplist";
- if (!this.intentStates[renderingIntent]) {
- this.intentStates[renderingIntent] = Object.create(null);
+ let intentState = this._intentStates.get(renderingIntent);
+ if (!intentState) {
+ intentState = Object.create(null);
+ this._intentStates.set(renderingIntent, intentState);
}
- const intentState = this.intentStates[renderingIntent];
let opListTask;
if (!intentState.opListReadCapability) {
- opListTask = {};
+ opListTask = Object.create(null);
opListTask.operatorListChanged = operatorListChanged;
intentState.opListReadCapability = createPromiseCapability();
intentState.renderTasks = [];
@@ -1222,8 +1224,7 @@ class PDFPageProxy {
this._transport.pageCache[this._pageIndex] = null;
const waitOn = [];
- Object.keys(this.intentStates).forEach(intent => {
- const intentState = this.intentStates[intent];
+ for (const [intent, intentState] of this._intentStates) {
this._abortOperatorList({
intentState,
reason: new Error("Page was destroyed."),
@@ -1232,13 +1233,13 @@ class PDFPageProxy {
if (intent === "oplist") {
// Avoid errors below, since the renderTasks are just stubs.
- return;
+ continue;
}
for (const internalRenderTask of intentState.renderTasks) {
waitOn.push(internalRenderTask.completed);
internalRenderTask.cancel();
}
- });
+ }
this.objs.clear();
this.annotationsPromise = null;
this.pendingCleanup = false;
@@ -1261,22 +1262,16 @@ class PDFPageProxy {
* @private
*/
_tryCleanup(resetStats = false) {
- if (
- !this.pendingCleanup ||
- Object.keys(this.intentStates).some(intent => {
- const intentState = this.intentStates[intent];
- return (
- intentState.renderTasks.length !== 0 ||
- !intentState.operatorList.lastChunk
- );
- })
- ) {
+ if (!this.pendingCleanup) {
return false;
}
+ for (const { renderTasks, operatorList } of this._intentStates.values()) {
+ if (renderTasks.length !== 0 || !operatorList.lastChunk) {
+ return false;
+ }
+ }
- Object.keys(this.intentStates).forEach(intent => {
- delete this.intentStates[intent];
- });
+ this._intentStates.clear();
this.objs.clear();
this.annotationsPromise = null;
if (resetStats && this._stats) {
@@ -1290,7 +1285,7 @@ class PDFPageProxy {
* @private
*/
_startRenderPage(transparency, intent) {
- const intentState = this.intentStates[intent];
+ const intentState = this._intentStates.get(intent);
if (!intentState) {
return; // Rendering was cancelled.
}
@@ -1340,7 +1335,7 @@ class PDFPageProxy {
);
const reader = readableStream.getReader();
- const intentState = this.intentStates[args.intent];
+ const intentState = this._intentStates.get(args.intent);
intentState.streamReader = reader;
const pump = () => {
@@ -1425,13 +1420,12 @@ class PDFPageProxy {
}
// Remove the current `intentState`, since a cancelled `getOperatorList`
// call on the worker-thread cannot be re-started...
- Object.keys(this.intentStates).some(intent => {
- if (this.intentStates[intent] === intentState) {
- delete this.intentStates[intent];
- return true;
+ for (const [intent, curIntentState] of this._intentStates) {
+ if (curIntentState === intentState) {
+ this._intentStates.delete(intent);
+ break;
}
- return false;
- });
+ }
// ... and force clean-up to ensure that any old state is always removed.
this.cleanup();
}