Add the current loading percentage to the onPassword callback

The percentage calculation is currently "spread out" across various viewer functionality, which we can avoid by having the API handle that instead.

Also, remove the `this.#lastProgress` special-case[1] and just register a "normal" `fullReader.onProgress` callback unconditionally. Once `headersReady` is resolved the callback can simply be removed when not needed, since the "worst" thing that could theoretically happen is that the loadingBar (in the viewer) updates sooner this way. In practice though, since `fullReader.read` cannot return data until `headersReady` is resolved, this change is not actually observable in the API.

---

[1] This was added in PR 8617, close to a decade ago, but it's not obvious to me that it was ever necessary to implement it that way.

Author: Snuffleupagus

examples/mobile-viewer/viewer.mjs

@@ -46,19 +46,13 @@ const PDFViewerApplication = {
    * @returns {Promise} - Returns the promise, which is resolved when document
    *                      is opened.
    */
-  open(params) {
+  async open(params) {
     if (this.pdfLoadingTask) {
-      // We need to destroy already opened document
-      return this.close().then(
-        function () {
-          // ... and repeat the open() call.
-          return this.open(params);
-        }.bind(this)
-      );
+      // We need to destroy already opened document.
+      await this.close();
     }
 
-    const url = params.url;
-    const self = this;
+    const { url } = params;
     this.setTitleUsingUrl(url);
 
     // Loading document.
@@ -70,24 +64,22 @@ const PDFViewerApplication = {
     });
     this.pdfLoadingTask = loadingTask;
 
-    loadingTask.onProgress = function (progressData) {
-      self.progress(progressData.loaded / progressData.total);
-    };
+    loadingTask.onProgress = evt => this.progress(evt.percent);
 
     return loadingTask.promise.then(
-      function (pdfDocument) {
+      pdfDocument => {
         // Document loaded, specifying document for the viewer.
-        self.pdfDocument = pdfDocument;
-        self.pdfViewer.setDocument(pdfDocument);
-        self.pdfLinkService.setDocument(pdfDocument);
-        self.pdfHistory.initialize({
+        this.pdfDocument = pdfDocument;
+        this.pdfViewer.setDocument(pdfDocument);
+        this.pdfLinkService.setDocument(pdfDocument);
+        this.pdfHistory.initialize({
           fingerprint: pdfDocument.fingerprints[0],
         });
 
-        self.loadingBar.hide();
-        self.setTitleUsingMetadata(pdfDocument);
+        this.loadingBar.hide();
+        this.setTitleUsingMetadata(pdfDocument);
       },
-      function (reason) {
+      reason => {
         let key = "pdfjs-loading-error";
         if (reason instanceof pdfjsLib.InvalidPDFException) {
           key = "pdfjs-invalid-file-error";
@@ -96,10 +88,10 @@ const PDFViewerApplication = {
             ? "pdfjs-missing-file-error"
             : "pdfjs-unexpected-response-error";
         }
-        self.l10n.get(key).then(msg => {
-          self.error(msg, { message: reason?.message });
+        this.l10n.get(key).then(msg => {
+          this.error(msg, { message: reason.message });
         });
-        self.loadingBar.hide();
+        this.loadingBar.hide();
       }
     );
   },
@@ -109,9 +101,9 @@ const PDFViewerApplication = {
    * @returns {Promise} - Returns the promise, which is resolved when all
    *                      destruction is completed.
    */
-  close() {
+  async close() {
     if (!this.pdfLoadingTask) {
-      return Promise.resolve();
+      return;
     }
 
     const promise = this.pdfLoadingTask.destroy();
@@ -128,7 +120,7 @@ const PDFViewerApplication = {
       }
     }
 
-    return promise;
+    await promise;
   },
 
   get loadingBar() {
@@ -152,48 +144,36 @@ const PDFViewerApplication = {
     this.setTitle(title);
   },
 
-  setTitleUsingMetadata(pdfDocument) {
-    const self = this;
-    pdfDocument.getMetadata().then(function (data) {
-      const info = data.info,
-        metadata = data.metadata;
-      self.documentInfo = info;
-      self.metadata = metadata;
-
-      // Provides some basic debug information
-      console.log(
-        "PDF " +
-          pdfDocument.fingerprints[0] +
-          " [" +
-          info.PDFFormatVersion +
-          " " +
-          (info.Producer || "-").trim() +
-          " / " +
-          (info.Creator || "-").trim() +
-          "]" +
-          " (PDF.js: " +
-          (pdfjsLib.version || "-") +
-          ")"
-      );
-
-      let pdfTitle;
-      if (metadata && metadata.has("dc:title")) {
-        const title = metadata.get("dc:title");
-        // Ghostscript sometimes returns 'Untitled', so prevent setting the
-        // title to 'Untitled.
-        if (title !== "Untitled") {
-          pdfTitle = title;
-        }
-      }
+  async setTitleUsingMetadata(pdfDocument) {
+    const { info, metadata } = await pdfDocument.getMetadata();
+    this.documentInfo = info;
+    this.metadata = metadata;
+
+    // Provides some basic debug information
+    console.log(
+      `PDF ${pdfDocument.fingerprints[0]} [${info.PDFFormatVersion} ` +
+        `${(metadata?.get("pdf:producer") || info.Producer || "-").trim()} / ` +
+        `${(metadata?.get("xmp:creatortool") || info.Creator || "-").trim()}` +
+        `] (PDF.js: ${pdfjsLib.version || "?"} [${pdfjsLib.build || "?"}])`
+    );
 
-      if (!pdfTitle && info && info.Title) {
-        pdfTitle = info.Title;
+    let pdfTitle;
+    if (metadata && metadata.has("dc:title")) {
+      const title = metadata.get("dc:title");
+      // Ghostscript sometimes returns 'Untitled', so prevent setting the
+      // title to 'Untitled.
+      if (title !== "Untitled") {
+        pdfTitle = title;
       }
+    }
 
-      if (pdfTitle) {
-        self.setTitle(pdfTitle + " - " + document.title);
-      }
-    });
+    if (!pdfTitle && info && info.Title) {
+      pdfTitle = info.Title;
+    }
+
+    if (pdfTitle) {
+      this.setTitle(pdfTitle + " - " + document.title);
+    }
   },
 
   setTitle: function pdfViewSetTitle(title) {
@@ -223,8 +203,7 @@ const PDFViewerApplication = {
     console.error(`${message}\n\n${moreInfoText.join("\n")}`);
   },
 
-  progress: function pdfViewProgress(level) {
-    const percent = Math.round(level * 100);
+  progress(percent) {
     // Updating the bar if value increases.
     if (percent > this.loadingBar.percent || isNaN(percent)) {
       this.loadingBar.percent = percent;

src/display/api.js

@@ -25,6 +25,7 @@ import {
   getVerbosityLevel,
   info,
   isNodeJS,
+  MathClamp,
   RenderingIntentFlag,
   setVerbosityLevel,
   shadow,
@@ -525,6 +526,8 @@ function getDocument(src = {}) {
  * @typedef {Object} OnProgressParameters
  * @property {number} loaded - Currently loaded number of bytes.
  * @property {number} total - Total number of bytes in the PDF file.
+ * @property {number} percent - Currently loaded percentage, as an integer value
+ *   in the [0, 100] range. If `total` is undefined, the percentage is `NaN`.
  */
 
 /**
@@ -2396,8 +2399,6 @@ class WorkerTransport {
 
   #fullReader = null;
 
-  #lastProgress = null;
-
   #methodPromises = new Map();
 
   #networkStream = null;
@@ -2492,6 +2493,14 @@ class WorkerTransport {
     return promise;
   }
 
+  #onProgress({ loaded, total }) {
+    this.loadingTask.onProgress?.({
+      loaded,
+      total,
+      percent: MathClamp(Math.round((loaded / total) * 100), 0, 100),
+    });
+  }
+
   get annotationStorage() {
     return shadow(this, "annotationStorage", new AnnotationStorage());
   }
@@ -2624,12 +2633,10 @@ class WorkerTransport {
         "GetReader - no `BasePDFStream` instance available."
       );
       this.#fullReader = this.#networkStream.getFullReader();
-      this.#fullReader.onProgress = evt => {
-        this.#lastProgress = {
-          loaded: evt.loaded,
-          total: evt.total,
-        };
-      };
+      // If stream or range turn out to be disabled, once `headersReady` is
+      // resolved, this is our only way to report loading progress.
+      this.#fullReader.onProgress = evt => this.#onProgress(evt);
+
       sink.onPull = () => {
         this.#fullReader
           .read()
@@ -2669,20 +2676,9 @@ class WorkerTransport {
       const { isStreamingSupported, isRangeSupported, contentLength } =
         this.#fullReader;
 
-      // If stream or range are disabled, it's our only way to report
-      // loading progress.
-      if (!isStreamingSupported || !isRangeSupported) {
-        if (this.#lastProgress) {
-          loadingTask.onProgress?.(this.#lastProgress);
-        }
-        this.#fullReader.onProgress = evt => {
-          loadingTask.onProgress?.({
-            loaded: evt.loaded,
-            total: evt.total,
-          });
-        };
+      if (isStreamingSupported && isRangeSupported) {
+        this.#fullReader.onProgress = null; // See comment in "GetReader" above.
       }
-
       return { isStreamingSupported, isRangeSupported, contentLength };
     });
 
@@ -2779,10 +2775,7 @@ class WorkerTransport {
     messageHandler.on("DataLoaded", data => {
       // For consistency: Ensure that progress is always reported when the
       // entire PDF file has been loaded, regardless of how it was fetched.
-      loadingTask.onProgress?.({
-        loaded: data.length,
-        total: data.length,
-      });
+      this.#onProgress({ loaded: data.length, total: data.length });
 
       this.downloadInfoCapability.resolve(data);
     });
@@ -2905,10 +2898,7 @@ class WorkerTransport {
       if (this.destroyed) {
         return; // Ignore any pending requests if the worker was terminated.
       }
-      loadingTask.onProgress?.({
-        loaded: data.loaded,
-        total: data.total,
-      });
+      this.#onProgress(data);
     });
 
     messageHandler.on("FetchBinaryData", async data => {

test/unit/api_spec.js

@@ -160,14 +160,18 @@ describe("api", function () {
         progressReportedCapability.resolve(progressData);
       };
 
-      const data = await Promise.all([
-        progressReportedCapability.promise,
+      const [pdfDoc, progress] = await Promise.all([
         loadingTask.promise,
+        progressReportedCapability.promise,
       ]);
 
-      expect(data[0].loaded / data[0].total >= 0).toEqual(true);
-      expect(data[1] instanceof PDFDocumentProxy).toEqual(true);
-      expect(loadingTask).toEqual(data[1].loadingTask);
+      expect(pdfDoc instanceof PDFDocumentProxy).toEqual(true);
+      expect(pdfDoc.loadingTask).toBe(loadingTask);
+
+      expect(progress.loaded).toBeGreaterThanOrEqual(0);
+      expect(progress.total).toEqual(basicApiFileLength);
+      expect(progress.percent).toBeGreaterThanOrEqual(0);
+      expect(progress.percent).toBeLessThanOrEqual(100);
 
       await loadingTask.destroy();
     });
@@ -218,12 +222,17 @@ describe("api", function () {
         progressReportedCapability.resolve(data);
       };
 
-      const data = await Promise.all([
+      const [pdfDoc, progress] = await Promise.all([
         loadingTask.promise,
         progressReportedCapability.promise,
       ]);
-      expect(data[0] instanceof PDFDocumentProxy).toEqual(true);
-      expect(data[1].loaded / data[1].total).toEqual(1);
+
+      expect(pdfDoc instanceof PDFDocumentProxy).toEqual(true);
+      expect(pdfDoc.loadingTask).toBe(loadingTask);
+
+      expect(progress.loaded).toEqual(basicApiFileLength);
+      expect(progress.total).toEqual(basicApiFileLength);
+      expect(progress.percent).toEqual(100);
 
       // Check that the TypedArray was transferred.
       expect(typedArrayPdf.length).toEqual(0);

web/app.js

@@ -1236,9 +1236,7 @@ const PDFViewerApplication = {
       this.passwordPrompt.open();
     };
 
-    loadingTask.onProgress = ({ loaded, total }) => {
-      this.progress(loaded / total);
-    };
+    loadingTask.onProgress = evt => this.progress(evt.percent);
 
     return loadingTask.promise.then(
       pdfDocument => {
@@ -1374,8 +1372,7 @@ const PDFViewerApplication = {
     return message;
   },
 
-  progress(level) {
-    const percent = Math.round(level * 100);
+  progress(percent) {
     // When we transition from full request to range requests, it's possible
     // that we discard some of the loaded data. This can cause the loading
     // bar to move backwards. So prevent this by only updating the bar if it

web/firefoxcom.js

@@ -13,7 +13,7 @@
  * limitations under the License.
  */
 
-import { isPdfFile, PDFDataRangeTransport } from "pdfjs-lib";
+import { isPdfFile, MathClamp, PDFDataRangeTransport } from "pdfjs-lib";
 import { AppOptions } from "./app_options.js";
 import { BaseExternalServices } from "./external_services.js";
 import { BasePreferences } from "./preferences.js";
@@ -627,7 +627,13 @@ class ExternalServices extends BaseExternalServices {
           pdfDataRangeTransport?.onDataProgressiveDone();
           break;
         case "progress":
-          viewerApp.progress(args.loaded / args.total);
+          const percent = MathClamp(
+            Math.round((args.loaded / args.total) * 100),
+            0,
+            100
+          );
+
+          viewerApp.progress(percent);
           break;
         case "complete":
           if (!args.data) {

web/ui_utils.js

@@ -709,7 +709,7 @@ class ProgressBar {
   }
 
   set percent(val) {
-    this.#percent = MathClamp(val, 0, 100);
+    this.#percent = val;
 
     if (isNaN(val)) {
       this.#classList.add("indeterminate");