Move the transfers computation into the AnnotationStorage class

Rather than having to *manually* determine the potential `transfers` at various spots in the API, we can let the `AnnotationStorage.serializable` getter include this. To further simplify things, we can also let the `serializable` getter compute and include the `hash`-string as well.
mozilla · Jun 29, 2023 · 9c5e297 · 9c5e297
1 parent 88c7c8b
commit 9c5e297
Show file tree

Hide file tree

Showing 5 changed files with 71 additions and 74 deletions.
diff --git a/src/display/annotation_storage.js b/src/display/annotation_storage.js
@@ -17,6 +17,25 @@ import { objectFromMap, unreachable } from "../shared/util.js";
 import { AnnotationEditor } from "./editor/editor.js";
 import { MurmurHash3_64 } from "../shared/murmurhash3.js";
 
+const SerializableEmpty = Object.freeze({
+  map: null,
+  hash: "",
+  transfers: undefined,
+});
+
+function getHashAndTransfers(map) {
+  const hash = new MurmurHash3_64(),
+    transfers = [];
+  for (const [key, val] of map) {
+    hash.update(`${key}:${JSON.stringify(val)}`);
+
+    if (val.bitmap) {
+      transfers.push(val.bitmap);
+    }
+  }
+  return { hash: hash.hexdigest(), transfers };
+}
+
 /**
  * Key/value storage for annotation data in forms.
  */
@@ -171,34 +190,20 @@ class AnnotationStorage {
    */
   get serializable() {
     if (this.#storage.size === 0) {
-      return null;
+      return SerializableEmpty;
     }
-    const clone = new Map();
+    const map = new Map();
 
     for (const [key, val] of this.#storage) {
       const serialized =
         val instanceof AnnotationEditor ? val.serialize() : val;
       if (serialized) {
-        clone.set(key, serialized);
+        map.set(key, serialized);
       }
     }
-    return clone;
-  }
-
-  /**
-   * PLEASE NOTE: Only intended for usage within the API itself.
-   * @ignore
-   */
-  static getHash(map) {
-    if (!map) {
-      return "";
-    }
-    const hash = new MurmurHash3_64();
-
-    for (const [key, val] of map) {
-      hash.update(`${key}:${JSON.stringify(val)}`);
-    }
-    return hash.hexdigest();
+    return map.size > 0
+      ? { map, ...getHashAndTransfers(map) }
+      : SerializableEmpty;
   }
 }
 
@@ -208,12 +213,21 @@ class AnnotationStorage {
  * contents. (Necessary since printing is triggered synchronously in browsers.)
  */
 class PrintAnnotationStorage extends AnnotationStorage {
-  #serializable = null;
+  #serializable;
 
   constructor(parent) {
     super();
+    const { map, hash, transfers } = parent.serializable;
     // Create a *copy* of the data, since Objects are passed by reference in JS.
-    this.#serializable = structuredClone(parent.serializable);
+    const clone = structuredClone(
+      map,
+      (typeof PDFJSDev === "undefined" ||
+        PDFJSDev.test("SKIP_BABEL || TESTING")) &&
+        transfers
+        ? { transfer: transfers }
+        : null
+    );
+    this.#serializable = { map: clone, hash, transfers };
   }
 
   /**
@@ -233,4 +247,4 @@ class PrintAnnotationStorage extends AnnotationStorage {
   }
 }
 
-export { AnnotationStorage, PrintAnnotationStorage };
+export { AnnotationStorage, PrintAnnotationStorage, SerializableEmpty };
diff --git a/src/display/api.js b/src/display/api.js
@@ -41,6 +41,7 @@ import {
 import {
   AnnotationStorage,
   PrintAnnotationStorage,
+  SerializableEmpty,
 } from "./annotation_storage.js";
 import {
   deprecated,
@@ -1811,30 +1812,26 @@ class PDFPageProxy {
   /**
    * @private
    */
-  _pumpOperatorList({ renderingIntent, cacheKey, annotationStorageMap }) {
+  _pumpOperatorList({
+    renderingIntent,
+    cacheKey,
+    annotationStorageSerializable,
+  }) {
     if (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) {
       assert(
         Number.isInteger(renderingIntent) && renderingIntent > 0,
         '_pumpOperatorList: Expected valid "renderingIntent" argument.'
       );
     }
-
-    const transfers = [];
-    if (annotationStorageMap) {
-      for (const annotation of annotationStorageMap.values()) {
-        if (annotation.bitmap) {
-          transfers.push(annotation.bitmap);
-        }
-      }
-    }
+    const { map, transfers } = annotationStorageSerializable;
 
     const readableStream = this._transport.messageHandler.sendWithStream(
       "GetOperatorList",
       {
         pageIndex: this._pageIndex,
         intent: renderingIntent,
         cacheKey,
-        annotationStorage: annotationStorageMap,
+        annotationStorage: map,
       },
       transfers
     );
@@ -2459,7 +2456,7 @@ class WorkerTransport {
     isOpList = false
   ) {
     let renderingIntent = RenderingIntentFlag.DISPLAY; // Default value.
-    let annotationMap = null;
+    let annotationStorageSerializable = SerializableEmpty;
 
     switch (intent) {
       case "any":
@@ -2492,7 +2489,7 @@ class WorkerTransport {
             ? printAnnotationStorage
             : this.annotationStorage;
 
-        annotationMap = annotationStorage.serializable;
+        annotationStorageSerializable = annotationStorage.serializable;
         break;
       default:
         warn(`getRenderingIntent - invalid annotationMode: ${annotationMode}`);
@@ -2504,10 +2501,8 @@ class WorkerTransport {
 
     return {
       renderingIntent,
-      cacheKey: `${renderingIntent}_${AnnotationStorage.getHash(
-        annotationMap
-      )}`,
-      annotationStorageMap: annotationMap,
+      cacheKey: `${renderingIntent}_${annotationStorageSerializable.hash}`,
+      annotationStorageSerializable,
     };
   }
 
@@ -2915,22 +2910,15 @@ class WorkerTransport {
           "please use the getData-method instead."
       );
     }
-    const annotationStorage = this.annotationStorage.serializable;
-    const transfers = [];
-    if (annotationStorage) {
-      for (const annotation of annotationStorage.values()) {
-        if (annotation.bitmap) {
-          transfers.push(annotation.bitmap);
-        }
-      }
-    }
+    const { map, transfers } = this.annotationStorage.serializable;
+
     return this.messageHandler
       .sendWithPromise(
         "SaveDocument",
         {
           isPureXfa: !!this._htmlForXfa,
           numPages: this._numPages,
-          annotationStorage,
+          annotationStorage: map,
           filename: this._fullReader?.filename ?? null,
         },
         transfers

diff --git a/test/integration/freetext_editor_spec.js b/test/integration/freetext_editor_spec.js
@@ -688,13 +688,12 @@ describe("FreeText Editor", () => {
           }
 
           const serialize = proprName =>
-            page.evaluate(
-              name =>
-                [
-                  ...window.PDFViewerApplication.pdfDocument.annotationStorage.serializable.values(),
-                ].map(x => x[name]),
-              proprName
-            );
+            page.evaluate(name => {
+              const { map } =
+                window.PDFViewerApplication.pdfDocument.annotationStorage
+                  .serializable;
+              return map ? Array.from(map.values(), x => x[name]) : [];
+            }, proprName);
 
           expect(await serialize("value"))
             .withContext(`In ${browserName}`)
@@ -805,13 +804,12 @@ describe("FreeText Editor", () => {
           }
 
           const serialize = proprName =>
-            page.evaluate(
-              name =>
-                [
-                  ...window.PDFViewerApplication.pdfDocument.annotationStorage.serializable.values(),
-                ].map(x => x[name]),
-              proprName
-            );
+            page.evaluate(name => {
+              const { map } =
+                window.PDFViewerApplication.pdfDocument.annotationStorage
+                  .serializable;
+              return map ? Array.from(map.values(), x => x[name]) : [];
+            }, proprName);
 
           const rects = (await serialize("rect")).map(rect =>
             rect.slice(0, 2).map(x => Math.floor(x))

diff --git a/test/integration/test_utils.js b/test/integration/test_utils.js
@@ -136,9 +136,11 @@ const mockClipboard = async pages => {
 exports.mockClipboard = mockClipboard;
 
 const getSerialized = page =>
-  page.evaluate(() => [
-    ...window.PDFViewerApplication.pdfDocument.annotationStorage.serializable.values(),
-  ]);
+  page.evaluate(() => {
+    const { map } =
+      window.PDFViewerApplication.pdfDocument.annotationStorage.serializable;
+    return map ? [...map.values()] : [];
+  });
 exports.getSerialized = getSerialized;
 
 function getEditors(page, kind) {

diff --git a/test/unit/api_spec.js b/test/unit/api_spec.js
@@ -50,7 +50,6 @@ import {
   RenderingCancelledException,
   StatTimer,
 } from "../../src/display/display_utils.js";
-import { AnnotationStorage } from "../../src/display/annotation_storage.js";
 import { AutoPrintRegExp } from "../../web/ui_utils.js";
 import { GlobalImageCache } from "../../src/core/image_utils.js";
 import { GlobalWorkerOptions } from "../../src/display/worker_options.js";
@@ -3525,12 +3524,8 @@ Caron Broadcasting, Inc., an Ohio corporation (“Lessee”).`)
       // Update the contents of the form-field again.
       annotationStorage.setValue("22R", { value: "Printing again..." });
 
-      const annotationHash = AnnotationStorage.getHash(
-        annotationStorage.serializable
-      );
-      const printAnnotationHash = AnnotationStorage.getHash(
-        printAnnotationStorage.serializable
-      );
+      const { hash: annotationHash } = annotationStorage.serializable;
+      const { hash: printAnnotationHash } = printAnnotationStorage.serializable;
       // Sanity check to ensure that the print-storage didn't change,
       // after the form-field was updated.
       expect(printAnnotationHash).not.toEqual(annotationHash);