From c62bcb55ace11ce578080a2794563163ec2a7050 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 6 Nov 2021 10:09:51 +0100 Subject: [PATCH 1/4] Adjust the "handles `resize` correctly, with `idsToKeep` provided" unit-test (PR 14238 follow-up) This small change will help validate an important part of the upcoming re-factoring, regarding the *correct* iteration of the `Set` in the `PDFPageViewBuffer.resize` method in particular. --- test/unit/base_viewer_spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/base_viewer_spec.js b/test/unit/base_viewer_spec.js index c66a1a5fc..8131f9a2d 100644 --- a/test/unit/base_viewer_spec.js +++ b/test/unit/base_viewer_spec.js @@ -136,12 +136,12 @@ describe("BaseViewer", function () { // Ensure that decreasing the size will evict the correct views, // while re-ordering the remaining ones correctly. - buffer.resize(3, new Set([1, 2, 3])); + buffer.resize(3, new Set([1, 2, 5])); expect(buffer._buffer).toEqual([ viewsMap.get(1), viewsMap.get(2), - viewsMap.get(3), + viewsMap.get(5), ]); }); From 0eba15b43ae7394a2807c17cac3fccaa47292d7d Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 6 Nov 2021 10:09:59 +0100 Subject: [PATCH 2/4] Convert `PDFPageViewBuffer` to a standard class This patch makes use of private `class` fields, to ensure that the previously "private" properties remain as such. --- web/base_viewer.js | 66 +++++++++++++++++++++++++++------------------- 1 file changed, 39 insertions(+), 27 deletions(-) diff --git a/web/base_viewer.js b/web/base_viewer.js index bab737122..39409c4ce 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -92,18 +92,37 @@ const DEFAULT_CACHE_SIZE = 10; * @property {IL10n} l10n - Localization service. */ -function PDFPageViewBuffer(size) { - const data = []; - this.push = function (view) { - const i = data.indexOf(view); +class PDFPageViewBuffer { + #data = []; + + #size = 0; + + constructor(size) { + this.#size = size; + + if ( + typeof PDFJSDev === "undefined" || + PDFJSDev.test("!PRODUCTION || TESTING") + ) { + Object.defineProperty(this, "_buffer", { + get() { + return this.#data.slice(); + }, + }); + } + } + + push(view) { + const data = this.#data, + i = data.indexOf(view); if (i >= 0) { data.splice(i, 1); } data.push(view); - if (data.length > size) { + if (data.length > this.#size) { data.shift().destroy(); } - }; + } /** * After calling resize, the size of the buffer will be `newSize`. @@ -112,31 +131,22 @@ function PDFPageViewBuffer(size) { * `idsToKeep` has no impact on the final size of the buffer; if `idsToKeep` * is larger than `newSize`, some of those pages will be destroyed anyway. */ - this.resize = function (newSize, idsToKeep = null) { - size = newSize; + resize(newSize, idsToKeep = null) { + this.#size = newSize; + + const data = this.#data; if (idsToKeep) { moveToEndOfArray(data, function (page) { return idsToKeep.has(page.id); }); } - while (data.length > size) { + while (data.length > this.#size) { data.shift().destroy(); } - }; + } - this.has = function (view) { - return data.includes(view); - }; - - if ( - typeof PDFJSDev === "undefined" || - PDFJSDev.test("!PRODUCTION || TESTING") - ) { - Object.defineProperty(this, "_buffer", { - get() { - return data.slice(); - }, - }); + has(view) { + return this.#data.includes(view); } } @@ -156,6 +166,8 @@ function isSameScale(oldScale, newScale) { * Simple viewer control to display PDF content/pages. */ class BaseViewer { + #buffer = null; + #scrollModePageState = null; /** @@ -518,7 +530,7 @@ class BaseViewer { } // Add the page to the buffer at the start of drawing. That way it can be // evicted from the buffer and destroyed even if we pause its rendering. - this._buffer.push(pageView); + this.#buffer.push(pageView); }; this.eventBus._on("pagerender", this._onBeforeDraw); @@ -684,7 +696,7 @@ class BaseViewer { this._currentScale = UNKNOWN_SCALE; this._currentScaleValue = null; this._pageLabels = null; - this._buffer = new PDFPageViewBuffer(DEFAULT_CACHE_SIZE); + this.#buffer = new PDFPageViewBuffer(DEFAULT_CACHE_SIZE); this._location = null; this._pagesRotation = 0; this._optionalContentConfigPromise = null; @@ -1153,7 +1165,7 @@ class BaseViewer { return; } const newCacheSize = Math.max(DEFAULT_CACHE_SIZE, 2 * numVisiblePages + 1); - this._buffer.resize(newCacheSize, visible.ids); + this.#buffer.resize(newCacheSize, visible.ids); this.renderingQueue.renderHighestPriority(visible); @@ -1304,7 +1316,7 @@ class BaseViewer { return false; } const pageView = this._pages[pageNumber - 1]; - return this._buffer.has(pageView); + return this.#buffer.has(pageView); } cleanup() { From f55bf4239843c89a609c6d680ecad75fad3af288 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 6 Nov 2021 10:10:05 +0100 Subject: [PATCH 3/4] Convert `PDFPageViewBuffer` to use a `Set` internally This relies on the fact that `Set`s preserve the insertion order[1], which means that we can utilize an iterator to access the *first* stored view. Note that in the `resize`-method, we can now move the visible pages to the back of the buffer using a single loop (hence we don't need to use the `moveToEndOfArray` helper function any more). --- [1] This applies to `Map`s as well, although that's not entirely relevant here. --- web/base_viewer.js | 50 ++++++++++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/web/base_viewer.js b/web/base_viewer.js index 39409c4ce..5f9f67747 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -31,7 +31,6 @@ import { MAX_AUTO_SCALE, MAX_SCALE, MIN_SCALE, - moveToEndOfArray, PresentationModeState, RendererType, SCROLLBAR_PADDING, @@ -93,7 +92,8 @@ const DEFAULT_CACHE_SIZE = 10; */ class PDFPageViewBuffer { - #data = []; + // Here we rely on the fact that `Set`s preserve the insertion order. + #buf = new Set(); #size = 0; @@ -106,21 +106,21 @@ class PDFPageViewBuffer { ) { Object.defineProperty(this, "_buffer", { get() { - return this.#data.slice(); + return [...this.#buf]; }, }); } } push(view) { - const data = this.#data, - i = data.indexOf(view); - if (i >= 0) { - data.splice(i, 1); + const buf = this.#buf; + if (buf.has(view)) { + buf.delete(view); // Move the view to the "end" of the buffer. } - data.push(view); - if (data.length > this.#size) { - data.shift().destroy(); + buf.add(view); + + if (buf.size > this.#size) { + this.#destroyFirstView(); } } @@ -134,19 +134,35 @@ class PDFPageViewBuffer { resize(newSize, idsToKeep = null) { this.#size = newSize; - const data = this.#data; + const buf = this.#buf; if (idsToKeep) { - moveToEndOfArray(data, function (page) { - return idsToKeep.has(page.id); - }); + const ii = buf.size; + let i = 1; + for (const view of buf) { + if (idsToKeep.has(view.id)) { + buf.delete(view); // Move the view to the "end" of the buffer. + buf.add(view); + } + if (++i > ii) { + break; + } + } } - while (data.length > this.#size) { - data.shift().destroy(); + + while (buf.size > this.#size) { + this.#destroyFirstView(); } } has(view) { - return this.#data.includes(view); + return this.#buf.has(view); + } + + #destroyFirstView() { + const firstView = this.#buf.keys().next().value; + + firstView?.destroy(); + this.#buf.delete(firstView); } } From a774707e312a01ff6892a5d7fdc2df5281f1603c Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 6 Nov 2021 10:10:12 +0100 Subject: [PATCH 4/4] Remove the `moveToEndOfArray` helper function, since it's unused With the previous patch, this helper function is no longer used and keeping it around will simply increase the size of the builds. This removal is purposely done *separately*, to make it easy to revert the patch in the future if this helper function would become useful again. --- test/unit/ui_utils_spec.js | 41 -------------------------------------- web/ui_utils.js | 22 -------------------- 2 files changed, 63 deletions(-) diff --git a/test/unit/ui_utils_spec.js b/test/unit/ui_utils_spec.js index 5eb21330c..4b7bf147d 100644 --- a/test/unit/ui_utils_spec.js +++ b/test/unit/ui_utils_spec.js @@ -21,7 +21,6 @@ import { getVisibleElements, isPortraitOrientation, isValidRotation, - moveToEndOfArray, parseQueryString, waitOnEventOrTimeout, WaitOnType, @@ -864,44 +863,4 @@ describe("ui_utils", function () { }); }); }); - - describe("moveToEndOfArray", function () { - it("works on empty arrays", function () { - const data = []; - moveToEndOfArray(data, function () {}); - expect(data).toEqual([]); - }); - - it("works when moving everything", function () { - const data = [1, 2, 3, 4, 5]; - moveToEndOfArray(data, function () { - return true; - }); - expect(data).toEqual([1, 2, 3, 4, 5]); - }); - - it("works when moving some things", function () { - const data = [1, 2, 3, 4, 5]; - moveToEndOfArray(data, function (x) { - return x % 2 === 0; - }); - expect(data).toEqual([1, 3, 5, 2, 4]); - }); - - it("works when moving one thing", function () { - const data = [1, 2, 3, 4, 5]; - moveToEndOfArray(data, function (x) { - return x === 1; - }); - expect(data).toEqual([2, 3, 4, 5, 1]); - }); - - it("works when moving nothing", function () { - const data = [1, 2, 3, 4, 5]; - moveToEndOfArray(data, function (x) { - return x === 0; - }); - expect(data).toEqual([1, 2, 3, 4, 5]); - }); - }); }); diff --git a/web/ui_utils.js b/web/ui_utils.js index c769bf3f0..72ad3097b 100644 --- a/web/ui_utils.js +++ b/web/ui_utils.js @@ -906,27 +906,6 @@ class ProgressBar { } } -/** - * Moves all elements of an array that satisfy condition to the end of the - * array, preserving the order of the rest. - */ -function moveToEndOfArray(arr, condition) { - const moved = [], - len = arr.length; - let write = 0; - for (let read = 0; read < len; ++read) { - if (condition(arr[read])) { - moved.push(arr[read]); - } else { - arr[write] = arr[read]; - ++write; - } - } - for (let read = 0; write < len; ++read, ++write) { - arr[write] = moved[read]; - } -} - /** * Get the active or focused element in current DOM. * @@ -1031,7 +1010,6 @@ export { MAX_AUTO_SCALE, MAX_SCALE, MIN_SCALE, - moveToEndOfArray, noContextMenuHandler, normalizeWheelEventDelta, normalizeWheelEventDirection,