ronantakizawa/codereview-bench huggingface dataset:
Repository: mozilla/pdf.js
File: src/display/text_layer.js (JavaScript)

Code before:
      return cachedAscent;
    }
    const ctx = this.#getCtx(lang);

    ctx.canvas.width = ctx.canvas.height = DEFAULT_FONT_SIZE;
    this.#ensureCtxFont(ctx, DEFAULT_FONT_SIZE, fontFamily);
    const metrics = ctx.measureText("");

    // Both properties aren't available by default in Firefox.
    let ascent = metrics.fontBoundingBoxAscent;
    let descent = Math.abs(metrics.fontBoundingBoxDescent);
    if (ascent) {
      const ratio = ascent / (ascent + descent);
      this.#ascentCache.set(fontFamily, ratio);

      ctx.canvas.width = ctx.canvas.height = 0;
      return ratio;
    }

    // Try basic heuristic to guess ascent/descent.
    // Draw a g with baseline at 0,0 and then get the line
    // number where a pixel has non-null red component (starting
    // from bottom).
    ctx.strokeStyle = "red";
    ctx.clearRect(0, 0, DEFAULT_FONT_SIZE, DEFAULT_FONT_SIZE);
    ctx.strokeText("g", 0, 0);
    let pixels = ctx.getImageData(
      0,
      0,
      DEFAULT_FONT_SIZE,
      DEFAULT_FONT_SIZE
    ).data;
    descent = 0;
    for (let i = pixels.length - 1 - 3; i >= 0; i -= 4) {
      if (pixels[i] > 0) {
        descent = Math.ceil(i / 4 / DEFAULT_FONT_SIZE);
        break;
      }
    }

    // Draw an A with baseline at 0,DEFAULT_FONT_SIZE and then get the line
    // number where a pixel has non-null red component (starting
    // from top).
    ctx.clearRect(0, 0, DEFAULT_FONT_SIZE, DEFAULT_FONT_SIZE);
    ctx.strokeText("A", 0, DEFAULT_FONT_SIZE);
    pixels = ctx.getImageData(0, 0, DEFAULT_FONT_SIZE, DEFAULT_FONT_SIZE).data;
    ascent = 0;
    for (let i = 0, ii = pixels.length; i < ii; i += 4) {
      if (pixels[i] > 0) {
        ascent = DEFAULT_FONT_SIZE - Math.floor(i / 4 / DEFAULT_FONT_SIZE);
        break;

Review comment (refactor):
@Snuffleupagus Why not use `style.ascent` first, which is the real value extracted from the font file?

Code after:
    }
    const ctx = this.#getCtx(lang);

    ctx.canvas.width = ctx.canvas.height = DEFAULT_FONT_SIZE;
    this.#ensureCtxFont(ctx, DEFAULT_FONT_SIZE, fontFamily);
    const metrics = ctx.measureText("");

    const ascent = metrics.fontBoundingBoxAscent;
    const descent = Math.abs(metrics.fontBoundingBoxDescent);

    ctx.canvas.width = ctx.canvas.height = 0;
    let ratio = 0.8; // DEFAULT_FONT_ASCENT

    if (ascent) {
      ratio = ascent / (ascent + descent);
    } else {
      if (
        (typeof PDFJSDev !== "undefined" && PDFJSDev.test("MOZCENTRAL")) ||
        FeatureTest.platform.isFirefox
      ) {
        warn(
          "Enable the `dom.textMetrics.fontBoundingBox.enabled` preference " +
            "in `about:config` to improve TextLayer rendering."
        );
      }
      if (style.ascent) {
        ratio = style.ascent;
      } else if (style.descent) {
        ratio = 1 + style.descent;
      }
    }

    this.#ascentCache.set(fontFamily, ratio);
    return ratio;
  }
}

export { TextLayer };
Repository: mozilla/pdf.js
File: web/genericcom.js (JavaScript)

Code before:
  // We could use the Web Crypto API for this (see https://bradyjoslin.com/blog/encryption-webcrypto/
  // for an example).

  #signatures = null;

  #save() {
    localStorage.setItem("pdfjs.signature", JSON.stringify(this.#signatures));
  }

  async getAll() {
    if (!this.#signatures) {
      const data = localStorage.getItem("pdfjs.signature");
      this.#signatures = data ? JSON.parse(data) : Object.create(null);
    }
    return this.#signatures;
  }

  async isFull() {
    return Object.keys(await this.getAll()).length === 5;
  }

  async create(data) {
    if (await this.isFull()) {
      return null;
    }
    const uuid = crypto.randomUUID();
    this.#signatures[uuid] = data;
    this.#save();

    return uuid;
  }

  async delete(uuid) {
    const signatures = await this.getAll();
    if (!signatures[uuid]) {
      return false;
    }
    delete signatures[uuid];
    this.#save();

    return true;
  }

  async update(uuid, data) {
    const signatures = await this.getAll();
    const oldData = signatures[uuid];
    if (!oldData) {
      return false;
    }
    Object.assign(oldData, data);
    this.#save();

Review comment (refactor):
This method is limited to secure context, and thus won't be available everywhere; see https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID

Here, and elsewhere in this patch, please use our helper function instead:
https://github.com/mozilla/pdf.js/blob/e608daed9f25d22e35afad91190462ace06162f5/src/shared/util.js#L1076-L1086

Code after:
  }
}

class MLManager {
  async isEnabledFor(_name) {
    return false;
  }

  async deleteModel(_service) {
    return null;
  }

  isReady(_name) {
    return false;
  }

  guess(_data) {}

  toggleService(_name, _enabled) {}

  static getFakeMLManager(options) {
    return new FakeMLManager(options);
  }
}

class FakeMLManager {
  eventBus = null;

  hasProgress = false;

  constructor({ enableGuessAltText, enableAltTextModelDownload }) {
    this.enableGuessAltText = enableGuessAltText;
    this.enableAltTextModelDownload = enableAltTextModelDownload;
  }

  setEventBus(eventBus, abortSignal) {
    this.eventBus = eventBus;
  }

  async isEnabledFor(_name) {
    return this.enableGuessAltText;
  }

  async deleteModel(_name) {
    this.enableAltTextModelDownload = false;
    return null;
  }

  async loadModel(_name) {}

  async downloadModel(_name) {
Repository: mozilla/pdf.js
File: web/signature_manager.js (JavaScript)

Code before:
    let data;
    switch (this.#currentTab) {
      case "type":
        data = this.#getOutlineForType();
        break;
      case "draw":
        data = this.#getOutlineForDraw();
        break;
      case "image":
        data = this.#extractedSignatureData;
        break;
    }
    this.#currentEditor.addSignature(
      data.outline,
      /* heightInPage */ 40,
      this.#description.value
    );
    if (this.#saveCheckbox.checked) {
      const description = this.#description.value;
      const signatureData = await this.#currentEditor.compress({
        outlines: data.newCurves,
        areContours: this.#currentTab === "draw",
        thickness: this.#currentTab === "draw" ? this.#drawThickness.value : 0,
      });
      const uuid = (this.#currentEditor._signatureUUID =
        await this.#signatureStorage.create({
          description,
          signatureData,
        }));
      if (!uuid) {
        console.warn("SignatureManager.add: cannot save the signature.");
      }
    }
    this.#finish();
  }

  destroy() {
    this.#uiManager = null;
    this.#finish();
  }
}

export { SignatureManager };


Review comment (suggestion):
Nit:
```suggestion
      const areContours = this.#currentTab === "draw",
        description = this.#description.value;
      const signatureData = await this.#currentEditor.compress({
        outlines: data.newCurves,
        areContours,
        thickness: areContours ? this.#drawThickness.value : 0,
      });
```

Code after:
    switch (this.#currentTab) {
      case "type":
        data = this.#getOutlineForType();
        break;
      case "draw":
        data = this.#getOutlineForDraw();
        break;
      case "image":
        data = this.#extractedSignatureData;
        break;
    }
    this.#currentEditor.addSignature(
      data.outline,
      /* heightInPage */ 40,
      this.#description.value
    );
    if (this.#saveCheckbox.checked) {
      const description = this.#description.value;
      const { newCurves, areContours, thickness, width, height } = data;
      const signatureData = await SignatureExtractor.compressSignature({
        outlines: newCurves,
        areContours,
        thickness,
        width,
        height,
      });
      const uuid = (this.#currentEditor._signatureUUID =
        await this.#signatureStorage.create({
          description,
          signatureData,
        }));
      if (!uuid) {
        console.warn("SignatureManager.add: cannot save the signature.");
      }
    }
    this.#finish();
  }

  destroy() {
    this.#uiManager = null;
    this.#finish();
  }
}

export { SignatureManager };
Repository: mozilla/pdf.js
File: web/annotation_layer_builder.js (JavaScript)

Code before:
      );
    }
    if (this._cancelled || this.#linksInjected) {
      return;
    }

    const newLinks = this.#annotations.length
      ? this.#checkInferredLinks(inferredLinks)
      : inferredLinks;

    if (!newLinks.length) {
      return;
    }

    if (!this.annotationLayer) {
      this.#initAnnotationLayer(viewport, structTreeLayer);
      setLayerDimensions(this.div, viewport);
    }

    await this.annotationLayer.addLinkAnnotations(newLinks, this.linkService);
    // Don't show the annotation layer if it was explicitly hidden previously.
    if (!this.#externalHide) {
      this.div.hidden = false;
    }

    this.#linksInjected = true;
  }

  #updatePresentationModeState(state) {
    if (!this.div) {
      return;
    }
    let disableFormElements = false;

    switch (state) {
      case PresentationModeState.FULLSCREEN:
        disableFormElements = true;
        break;
      case PresentationModeState.NORMAL:
        break;
      default:
        return;
    }
    for (const section of this.div.childNodes) {
      if (section.hasAttribute("data-internal-link")) {
        continue;
      }
      section.inert = disableFormElements;
    }
  }


Review comment (suggestion):
The same line should also be added to the early-return branch above https://github.com/mozilla/pdf.js/blob/e3cca6d513d4c24393cf5d8d4af25d69b399b46e/web/annotation_layer_builder.js#L246-L248 since otherwise there'd still be unnecessary re-parsing happening (even if nothing renders in that case).

Code after:
      );
    }
    if (this._cancelled || this.#linksInjected) {
      return;
    }
    this.#linksInjected = true;

    const newLinks = this.#annotations.length
      ? this.#checkInferredLinks(inferredLinks)
      : inferredLinks;

    if (!newLinks.length) {
      return;
    }

    if (!this.annotationLayer) {
      this.#initAnnotationLayer(viewport, structTreeLayer);
      setLayerDimensions(this.div, viewport);
    }

    await this.annotationLayer.addLinkAnnotations(newLinks, this.linkService);
    // Don't show the annotation layer if it was explicitly hidden previously.
    if (!this.#externalHide) {
      this.div.hidden = false;
    }
  }

  #updatePresentationModeState(state) {
    if (!this.div) {
      return;
    }
    let disableFormElements = false;

    switch (state) {
      case PresentationModeState.FULLSCREEN:
        disableFormElements = true;
        break;
      case PresentationModeState.NORMAL:
        break;
      default:
        return;
    }
    for (const section of this.div.childNodes) {
      if (section.hasAttribute("data-internal-link")) {
        continue;
      }
      section.inert = disableFormElements;
    }
  }

  #checkInferredLinks(inferredLinks) {
Repository: mozilla/pdf.js
File: test/integration/autolinker_spec.mjs (JavaScript)

Code before:
    afterAll(async () => {
      await closePages(pages);
    });

    it("must not repeatedly add link annotations redundantly", async () => {
      await Promise.all(
        pages.map(async ([browserName, page]) => {
          let url = await page.$$eval(
            ".annotationLayer > .linkAnnotation > a",
            annotations => annotations.map(a => a.href)
          );
          expect(url.length).withContext(`In ${browserName}`).toEqual(1);
          await page.evaluate(() =>
            window.PDFViewerApplication.pdfViewer.updateScale({
              drawingDelay: -1,
              scaleFactor: 2,
            })
          );
          url = await page.$$eval(
            ".annotationLayer > .linkAnnotation > a",
            annotations => annotations.map(a => a.href)
          );
          expect(url.length).withContext(`In ${browserName}`).toEqual(1);
        })
      );
    });
  });
});


Review comment (suggestion):
```suggestion

          await page.evaluate(() =>
```
(https://github.com/mozilla/pdf.js/pull/19470#discussion_r1952272752)

Code after:
      pages = await loadAndWait(
        "bug1019475_2.pdf",
        ".annotationLayer",
        null,
        null,
        {
          enableAutoLinking: true,
        }
      );
    });

    afterAll(async () => {
      await closePages(pages);
    });

    it("must not repeatedly add link annotations redundantly", async () => {
      await Promise.all(
        pages.map(async ([browserName, page]) => {
          await waitForLinkAnnotations(page);
          let url = await page.$$eval(
            ".annotationLayer > .linkAnnotation > a",
            annotations => annotations.map(a => a.href)
          );
          expect(url.length).withContext(`In ${browserName}`).toEqual(1);

          await page.evaluate(() =>
            window.PDFViewerApplication.pdfViewer.updateScale({
              drawingDelay: -1,
              scaleFactor: 2,
            })
          );
          await waitForLinkAnnotations(page);
          url = await page.$$eval(
            ".annotationLayer > .linkAnnotation > a",
            annotations => annotations.map(a => a.href)
          );
          expect(url.length).withContext(`In ${browserName}`).toEqual(1);
        })
      );
    });
  });
});
Repository: mozilla/pdf.js
File: web/pdf_page_view.js (JavaScript)

Code before:
    const items = [];
    for (const item of text.items) {
      items.push(item.str);
    }
    this._textHighlighter.setTextMapping(textDivs, items);
    this._textHighlighter.enable();
  }

  async #injectLinkAnnotations(textLayerPromise) {
    let error = null;
    try {
      await textLayerPromise;

      if (!this.annotationLayer) {
        return; // Rendering was cancelled while the textLayerPromise resolved.
      }
      await this.annotationLayer.injectLinkAnnotations({
        inferredLinks: Autolinker.processLinks(this),
        viewport: this.viewport,
        structTreeLayer: this.structTreeLayer,
      });
    } catch (ex) {
      console.error("#injectLinkAnnotations:", ex);
      error = ex;
    }
    this.#dispatchLayerRendered("linkannotationsadded", error);
  }

  #resetCanvas() {
    const { canvas } = this;
    if (!canvas) {
      return;
    }
    canvas.remove();
    canvas.width = canvas.height = 0;
    this.canvas = null;
    this.#originalViewport = null;
  }

  reset({
    keepAnnotationLayer = false,
    keepAnnotationEditorLayer = false,
    keepXfaLayer = false,
    keepTextLayer = false,
    keepCanvasWrapper = false,
  } = {}) {
    this.cancelRendering({
      keepAnnotationLayer,
      keepAnnotationEditorLayer,
      keepXfaLayer,
      keepTextLayer,

Review comment (suggestion):
If we use this event only for testing purposes, I think we should avoid to dispatch it in other contexts, at least it should be removed from the mozcentral build.

Code after:
    const items = [];
    for (const item of text.items) {
      items.push(item.str);
    }
    this._textHighlighter.setTextMapping(textDivs, items);
    this._textHighlighter.enable();
  }

  async #injectLinkAnnotations(textLayerPromise) {
    let error = null;
    try {
      await textLayerPromise;

      if (!this.annotationLayer) {
        return; // Rendering was cancelled while the textLayerPromise resolved.
      }
      await this.annotationLayer.injectLinkAnnotations({
        inferredLinks: Autolinker.processLinks(this),
        viewport: this.viewport,
        structTreeLayer: this.structTreeLayer,
      });
    } catch (ex) {
      console.error("#injectLinkAnnotations:", ex);
      error = ex;
    }
    if (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) {
      this.#dispatchLayerRendered("linkannotationsadded", error);
    }
  }

  #resetCanvas() {
    const { canvas } = this;
    if (!canvas) {
      return;
    }
    canvas.remove();
    canvas.width = canvas.height = 0;
    this.canvas = null;
    this.#originalViewport = null;
  }

  reset({
    keepAnnotationLayer = false,
    keepAnnotationEditorLayer = false,
    keepXfaLayer = false,
    keepTextLayer = false,
    keepCanvasWrapper = false,
  } = {}) {
    this.cancelRendering({
      keepAnnotationLayer,
      keepAnnotationEditorLayer,
Repository: mozilla/pdf.js
File: web/app_options.js (JavaScript)

Code before:
    // don't use the system fonts in this case (bug 1882613).
    /** @type {boolean|undefined} */
    value: (
      typeof PDFJSDev === "undefined"
        ? window.isGECKOVIEW
        : PDFJSDev.test("GECKOVIEW")
    )
      ? false
      : undefined,
    kind: OptionKind.API,
    type: Type.BOOLEAN + Type.UNDEFINED,
  },
  verbosity: {
    /** @type {number} */
    value: 1,
    kind: OptionKind.API,
  },
  viewerCssTheme: {
    /** @type {number} */
    value: typeof PDFJSDev !== "undefined" && PDFJSDev.test("CHROME") ? 2 : 0,
    kind: OptionKind.VIEWER + OptionKind.PREFERENCE,
  },
  wasmUrl: {
    /** @type {string} */
    value:
      typeof PDFJSDev !== "undefined" && PDFJSDev.test("MOZCENTRAL")
        ? "resource://pdf.js/web/wasm/"
        : "../web/wasm/",
    kind: OptionKind.API,
  },
  workerPort: {
    /** @type {Object} */
    value: null,
    kind: OptionKind.WORKER,
  },
  workerSrc: {
    /** @type {string} */
    value:
      // eslint-disable-next-line no-nested-ternary
      typeof PDFJSDev === "undefined"
        ? "../src/pdf.worker.js"
        : PDFJSDev.test("MOZCENTRAL")
          ? "resource://pdf.js/build/pdf.worker.mjs"
          : "../build/pdf.worker.mjs",
    kind: OptionKind.WORKER,
  },
};
if (typeof PDFJSDev === "undefined" || !PDFJSDev.test("MOZCENTRAL")) {
  defaultOptions.defaultUrl = {
    /** @type {string} */
    value:

Review comment (suggestion):
This line-break should not be removed, since it helps readability by separating API and WORKER options.

Code after:
          : "../web/standard_fonts/",
    kind: OptionKind.API,
  },
  useSystemFonts: {
    // On Android, there is almost no chance to have the font we want so we
    // don't use the system fonts in this case (bug 1882613).
    /** @type {boolean|undefined} */
    value: (
      typeof PDFJSDev === "undefined"
        ? window.isGECKOVIEW
        : PDFJSDev.test("GECKOVIEW")
    )
      ? false
      : undefined,
    kind: OptionKind.API,
    type: Type.BOOLEAN + Type.UNDEFINED,
  },
  verbosity: {
    /** @type {number} */
    value: 1,
    kind: OptionKind.API,
  },
  wasmUrl: {
    /** @type {string} */
    value:
      typeof PDFJSDev !== "undefined" && PDFJSDev.test("MOZCENTRAL")
        ? "resource://pdf.js/web/wasm/"
        : "../web/wasm/",
    kind: OptionKind.API,
  },

  workerPort: {
    /** @type {Object} */
    value: null,
    kind: OptionKind.WORKER,
  },
  workerSrc: {
    /** @type {string} */
    value:
      // eslint-disable-next-line no-nested-ternary
      typeof PDFJSDev === "undefined"
        ? "../src/pdf.worker.js"
        : PDFJSDev.test("MOZCENTRAL")
          ? "resource://pdf.js/build/pdf.worker.mjs"
          : "../build/pdf.worker.mjs",
    kind: OptionKind.WORKER,
  },
};
if (typeof PDFJSDev === "undefined" || !PDFJSDev.test("MOZCENTRAL")) {
  defaultOptions.defaultUrl = {
    /** @type {string} */
Repository: mozilla/pdf.js
File: web/signature_manager.js (JavaScript)

Code before:
          button.setAttribute("tabindex", -1);
        }
      }
    };

    const buttonsArray = Array.from(buttons.values());
    for (let i = 0, ii = buttonsArray.length; i < ii; i++) {
      const button = buttonsArray[i];
      button.addEventListener("click", tabCallback, { passive: true });
      button.addEventListener(
        "keydown",
        ({ key }) => {
          if (key !== "ArrowLeft" && key !== "ArrowRight") {
            return;
          }
          buttonsArray[i + (key === "ArrowLeft" ? -1 : 1)]?.focus();
        },
        { passive: true }
      );
    }
  }

  #resetCommon() {
    this.#hasDescriptionChanged = false;
    this.#description.value = "";
    this.#tabsToAltText.get(this.#currentTab).value = "";
  }

  #resetTab(name) {
    switch (name) {
      case "type":
        this.#typeInput.value = "";
        break;
      case "draw":
        this.#drawCurves = null;
        this.#drawPoints = null;
        this.#drawPathString = "";
        this.#drawPath?.remove();
        this.#drawPath = null;
        this.#drawPlaceholder.hidden = false;
        this.#drawThickness.value = 1;
        break;
      case "image":
        this.#imagePlaceholder.hidden = false;
        this.#imagePath?.remove();
        this.#imagePath = null;
        break;
    }
  }

  #initTab(name) {

Review comment (suggestion):
Is `this.#currentTab` always guaranteed to be set here, or would the following be safer?
```suggestion
    if (this.#currentTab) {
      this.#tabsToAltText.get(this.#currentTab).value = "";
    }
```

Code after:
          button.setAttribute("tabindex", -1);
        }
      }
    };

    const buttonsArray = Array.from(buttons.values());
    for (let i = 0, ii = buttonsArray.length; i < ii; i++) {
      const button = buttonsArray[i];
      button.addEventListener("click", tabCallback, { passive: true });
      button.addEventListener(
        "keydown",
        ({ key }) => {
          if (key !== "ArrowLeft" && key !== "ArrowRight") {
            return;
          }
          buttonsArray[i + (key === "ArrowLeft" ? -1 : 1)]?.focus();
        },
        { passive: true }
      );
    }
  }

  #resetCommon() {
    this.#hasDescriptionChanged = false;
    this.#description.value = "";
    if (this.#currentTab) {
      this.#tabsToAltText.get(this.#currentTab).value = "";
    }
  }

  #resetTab(name) {
    switch (name) {
      case "type":
        this.#typeInput.value = "";
        break;
      case "draw":
        this.#drawCurves = null;
        this.#drawPoints = null;
        this.#drawPathString = "";
        this.#drawPath?.remove();
        this.#drawPath = null;
        this.#drawPlaceholder.hidden = false;
        this.#drawThickness.value = 1;
        break;
      case "image":
        this.#imagePlaceholder.hidden = false;
        this.#imagePath?.remove();
        this.#imagePath = null;
        break;
    }
  }