From fa69d9a3bce242bc4ee3eed44a0c6eed5ce17d74 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 24 Apr 2024 21:39:10 +0200 Subject: [PATCH 1/3] Inline the helper method in `PDFLinkService.goToDestination` We no longer need the helper method to *potentially* call itself once data is available, and can instead take full advantage of async/await by inlining the code. --- web/pdf_link_service.js | 82 ++++++++++++++++++----------------------- 1 file changed, 35 insertions(+), 47 deletions(-) diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js index d13c939543a07..f5be0a37ecc01 100644 --- a/web/pdf_link_service.js +++ b/web/pdf_link_service.js @@ -131,10 +131,31 @@ class PDFLinkService { return this.pdfDocument ? this.pdfViewer.isInPresentationMode : false; } - #goToDestinationHelper(rawDest, namedDest = null, explicitDest) { + /** + * This method will, when available, also update the browser history. + * + * @param {string|Array} dest - The named, or explicit, PDF destination. + */ + async goToDestination(dest) { + if (!this.pdfDocument) { + return; + } + let namedDest, explicitDest, pageNumber; + if (typeof dest === "string") { + namedDest = dest; + explicitDest = await this.pdfDocument.getDestination(dest); + } else { + namedDest = null; + explicitDest = await dest; + } + if (!Array.isArray(explicitDest)) { + console.error( + `goToDestination: "${explicitDest}" is not a valid destination array, for dest="${dest}".` + ); + return; + } // Dest array looks like that: - const destRef = explicitDest[0]; - let pageNumber; + const [destRef] = explicitDest; if (typeof destRef === "object" && destRef !== null) { pageNumber = this._cachedPageNumber(destRef); @@ -142,33 +163,27 @@ class PDFLinkService { if (!pageNumber) { // Fetch the page reference if it's not yet available. This could // only occur during loading, before all pages have been resolved. - this.pdfDocument - .getPageIndex(destRef) - .then(pageIndex => { - this.cachePageRef(pageIndex + 1, destRef); - this.#goToDestinationHelper(rawDest, namedDest, explicitDest); - }) - .catch(() => { - console.error( - `PDFLinkService.#goToDestinationHelper: "${destRef}" is not ` + - `a valid page reference, for dest="${rawDest}".` - ); - }); - return; + try { + pageNumber = (await this.pdfDocument.getPageIndex(destRef)) + 1; + this.cachePageRef(pageNumber, destRef); + } catch { + console.error( + `goToDestination: "${destRef}" is not a valid page reference, for dest="${dest}".` + ); + return; + } } } else if (Number.isInteger(destRef)) { pageNumber = destRef + 1; } else { console.error( - `PDFLinkService.#goToDestinationHelper: "${destRef}" is not ` + - `a valid destination reference, for dest="${rawDest}".` + `goToDestination: "${destRef}" is not a valid destination reference, for dest="${dest}".` ); return; } if (!pageNumber || pageNumber < 1 || pageNumber > this.pagesCount) { console.error( - `PDFLinkService.#goToDestinationHelper: "${pageNumber}" is not ` + - `a valid page number, for dest="${rawDest}".` + `goToDestination: "${pageNumber}" is not a valid page number, for dest="${dest}".` ); return; } @@ -187,33 +202,6 @@ class PDFLinkService { }); } - /** - * This method will, when available, also update the browser history. - * - * @param {string|Array} dest - The named, or explicit, PDF destination. - */ - async goToDestination(dest) { - if (!this.pdfDocument) { - return; - } - let namedDest, explicitDest; - if (typeof dest === "string") { - namedDest = dest; - explicitDest = await this.pdfDocument.getDestination(dest); - } else { - namedDest = null; - explicitDest = await dest; - } - if (!Array.isArray(explicitDest)) { - console.error( - `PDFLinkService.goToDestination: "${explicitDest}" is not ` + - `a valid destination array, for dest="${dest}".` - ); - return; - } - this.#goToDestinationHelper(dest, namedDest, explicitDest); - } - /** * This method will, when available, also update the browser history. * From f6cd03955b921c6cd5187f8f5e76d37f471560d1 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 24 Apr 2024 21:55:50 +0200 Subject: [PATCH 2/3] [api-minor] Move the page reference/number caching into the API Rather than having to handle this *manually* throughout the viewer, this functionality can instead be moved into the API which simplifies the code slightly. --- src/core/worker.js | 1 + src/display/api.js | 41 ++++++++++++++++++++++++++++++++------- web/interfaces.js | 6 ------ web/pdf_link_service.js | 39 ++----------------------------------- web/pdf_outline_viewer.js | 19 ++++-------------- web/pdf_viewer.js | 10 +--------- 6 files changed, 42 insertions(+), 74 deletions(-) diff --git a/src/core/worker.js b/src/core/worker.js index 631bb52df9f26..804936229bcc9 100644 --- a/src/core/worker.js +++ b/src/core/worker.js @@ -419,6 +419,7 @@ class WorkerMessageHandler { return { rotate, ref, + refStr: ref?.toString() ?? null, userUnit, view, }; diff --git a/src/display/api.js b/src/display/api.js index 3644e26e09c65..9c70a7c22cb2f 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -560,6 +560,16 @@ function getDataProp(val) { ); } +function isRefProxy(ref) { + return ( + typeof ref === "object" && + Number.isInteger(ref?.num) && + ref.num >= 0 && + Number.isInteger(ref?.gen) && + ref.gen >= 0 + ); +} + /** * @typedef {Object} OnProgressParameters * @property {number} loaded - Currently loaded number of bytes. @@ -1066,6 +1076,14 @@ class PDFDocumentProxy { return this.loadingTask.destroy(); } + /** + * @param {RefProxy} ref - The page reference. + * @returns {number | null} The page number, if it's cached. + */ + cachedPageNumber(ref) { + return this._transport.cachedPageNumber(ref); + } + /** * @type {DocumentInitParameters} A subset of the current * {DocumentInitParameters}, which are needed in the viewer. @@ -2340,6 +2358,8 @@ class WorkerTransport { #pagePromises = new Map(); + #pageRefCache = new Map(); + #passwordCapability = null; constructor(messageHandler, loadingTask, networkStream, params, factory) { @@ -2483,6 +2503,7 @@ class WorkerTransport { } this.#pageCache.clear(); this.#pagePromises.clear(); + this.#pageRefCache.clear(); // Allow `AnnotationStorage`-related clean-up when destroying the document. if (this.hasOwnProperty("annotationStorage")) { this.annotationStorage.resetModified(); @@ -2915,6 +2936,10 @@ class WorkerTransport { if (this.destroyed) { throw new Error("Transport destroyed"); } + if (pageInfo.refStr) { + this.#pageRefCache.set(pageInfo.refStr, pageNumber); + } + const page = new PDFPageProxy( pageIndex, pageInfo, @@ -2929,13 +2954,7 @@ class WorkerTransport { } getPageIndex(ref) { - if ( - typeof ref !== "object" || - !Number.isInteger(ref?.num) || - ref.num < 0 || - !Number.isInteger(ref?.gen) || - ref.gen < 0 - ) { + if (!isRefProxy(ref)) { return Promise.reject(new Error("Invalid pageIndex request.")); } return this.messageHandler.sendWithPromise("GetPageIndex", { @@ -3076,6 +3095,14 @@ class WorkerTransport { cleanupTextLayer(); } + cachedPageNumber(ref) { + if (!isRefProxy(ref)) { + return null; + } + const refStr = ref.gen === 0 ? `${ref.num}R` : `${ref.num}R${ref.gen}`; + return this.#pageRefCache.get(refStr) ?? null; + } + get loadingParams() { const { disableAutoFetch, enableXfa } = this._params; return shadow(this, "loadingParams", { diff --git a/web/interfaces.js b/web/interfaces.js index 4c44031285844..31976c40f92f6 100644 --- a/web/interfaces.js +++ b/web/interfaces.js @@ -106,12 +106,6 @@ class IPDFLinkService { * @param {Object} action */ executeSetOCGState(action) {} - - /** - * @param {number} pageNum - page number. - * @param {Object} pageRef - reference to the page. - */ - cachePageRef(pageNum, pageRef) {} } /** diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js index f5be0a37ecc01..4f013ef26eede 100644 --- a/web/pdf_link_service.js +++ b/web/pdf_link_service.js @@ -49,8 +49,6 @@ const LinkTarget = { class PDFLinkService { externalLinkEnabled = true; - #pagesRefCache = new Map(); - /** * @param {PDFLinkServiceOptions} options */ @@ -74,7 +72,6 @@ class PDFLinkService { setDocument(pdfDocument, baseUrl = null) { this.baseUrl = baseUrl; this.pdfDocument = pdfDocument; - this.#pagesRefCache.clear(); } setViewer(pdfViewer) { @@ -157,15 +154,14 @@ class PDFLinkService { // Dest array looks like that: const [destRef] = explicitDest; - if (typeof destRef === "object" && destRef !== null) { - pageNumber = this._cachedPageNumber(destRef); + if (destRef && typeof destRef === "object") { + pageNumber = this.pdfDocument.cachedPageNumber(destRef); if (!pageNumber) { // Fetch the page reference if it's not yet available. This could // only occur during loading, before all pages have been resolved. try { pageNumber = (await this.pdfDocument.getPageIndex(destRef)) + 1; - this.cachePageRef(pageNumber, destRef); } catch { console.error( `goToDestination: "${destRef}" is not a valid page reference, for dest="${dest}".` @@ -496,31 +492,6 @@ class PDFLinkService { ); } - /** - * @param {number} pageNum - page number. - * @param {Object} pageRef - reference to the page. - */ - cachePageRef(pageNum, pageRef) { - if (!pageRef) { - return; - } - const refStr = - pageRef.gen === 0 ? `${pageRef.num}R` : `${pageRef.num}R${pageRef.gen}`; - this.#pagesRefCache.set(refStr, pageNum); - } - - /** - * @ignore - */ - _cachedPageNumber(pageRef) { - if (!pageRef) { - return null; - } - const refStr = - pageRef.gen === 0 ? `${pageRef.num}R` : `${pageRef.num}R${pageRef.gen}`; - return this.#pagesRefCache.get(refStr) || null; - } - static #isValidExplicitDest(dest) { if (!Array.isArray(dest) || dest.length < 2) { return false; @@ -580,12 +551,6 @@ class PDFLinkService { */ class SimpleLinkService extends PDFLinkService { setDocument(pdfDocument, baseUrl = null) {} - - /** - * @param {number} pageNum - page number. - * @param {Object} pageRef - reference to the page. - */ - cachePageRef(pageNum, pageRef) {} } export { LinkTarget, PDFLinkService, SimpleLinkService }; diff --git a/web/pdf_outline_viewer.js b/web/pdf_outline_viewer.js index a198767b4e871..efb7e5ec13424 100644 --- a/web/pdf_outline_viewer.js +++ b/web/pdf_outline_viewer.js @@ -325,21 +325,10 @@ class PDFOutlineViewer extends BaseTreeViewer { if (Array.isArray(explicitDest)) { const [destRef] = explicitDest; - if (typeof destRef === "object" && destRef !== null) { - pageNumber = this.linkService._cachedPageNumber(destRef); - - if (!pageNumber) { - try { - pageNumber = (await pdfDocument.getPageIndex(destRef)) + 1; - - if (pdfDocument !== this._pdfDocument) { - return null; // The document was closed while the data resolved. - } - this.linkService.cachePageRef(pageNumber, destRef); - } catch { - // Invalid page reference, ignore it and continue parsing. - } - } + if (destRef && typeof destRef === "object") { + // The page reference must be available, since the current method + // won't be invoked until all pages have been loaded. + pageNumber = pdfDocument.cachedPageNumber(destRef); } else if (Number.isInteger(destRef)) { pageNumber = destRef + 1; } diff --git a/web/pdf_viewer.js b/web/pdf_viewer.js index fd9ff4d8f4913..1a1eca3ae123a 100644 --- a/web/pdf_viewer.js +++ b/web/pdf_viewer.js @@ -935,11 +935,7 @@ class PDFViewer { // Set the first `pdfPage` immediately, since it's already loaded, // rather than having to repeat the `PDFDocumentProxy.getPage` call in // the `this.#ensurePdfPageLoaded` method before rendering can start. - const firstPageView = this._pages[0]; - if (firstPageView) { - firstPageView.setPdfPage(firstPdfPage); - this.linkService.cachePageRef(1, firstPdfPage.ref); - } + this._pages[0]?.setPdfPage(firstPdfPage); if (this._scrollMode === ScrollMode.PAGE) { // Ensure that the current page becomes visible on document load. @@ -994,7 +990,6 @@ class PDFViewer { if (!pageView.pdfPage) { pageView.setPdfPage(pdfPage); } - this.linkService.cachePageRef(pageNum, pdfPage.ref); if (--getPagesLeft === 0) { this._pagesCapability.resolve(); } @@ -1718,9 +1713,6 @@ class PDFViewer { if (!pageView.pdfPage) { pageView.setPdfPage(pdfPage); } - if (!this.linkService._cachedPageNumber?.(pdfPage.ref)) { - this.linkService.cachePageRef(pageView.id, pdfPage.ref); - } return pdfPage; } catch (reason) { console.error("Unable to get page for page view", reason); From 150964dd6d448ce9eb3f32689e576b33f3f86f3f Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 25 Apr 2024 10:10:00 +0200 Subject: [PATCH 3/3] Remove unnecessary check from `PDFLinkService.goToDestination` (PR 17984 follow-up) After the changes in PR 17984 this code can no longer be reached, since the destination is now validated on the worker-thread. --- web/pdf_link_service.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js index 4f013ef26eede..7dd8398053ff3 100644 --- a/web/pdf_link_service.js +++ b/web/pdf_link_service.js @@ -171,11 +171,6 @@ class PDFLinkService { } } else if (Number.isInteger(destRef)) { pageNumber = destRef + 1; - } else { - console.error( - `goToDestination: "${destRef}" is not a valid destination reference, for dest="${dest}".` - ); - return; } if (!pageNumber || pageNumber < 1 || pageNumber > this.pagesCount) { console.error(