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

Code before:
      off: parseOnOff(config.get("OFF")),
      order: parseOrder(config.get("Order")),
      groups: [...groupRefCache],
    };
  }

  setActualNumPages(num = null) {
    this.#actualNumPages = num;
  }

  get hasActualNumPages() {
    return this.#actualNumPages !== null;
  }

  get _pagesCount() {
    const obj = this.toplevelPagesDict.get("Count");
    if (!Number.isInteger(obj)) {
      throw new FormatError(
        "Page count in top-level pages dictionary is not an integer."
      );
    }
    return shadow(this, "_pagesCount", obj);
  }

  get numPages() {
    return this.hasActualNumPages ? this.#actualNumPages : this._pagesCount;
  }

  get destinations() {
    const rawDests = this.#readDests(),
      dests = Object.create(null);
    for (const obj of rawDests) {
      if (obj instanceof NameTree) {
        for (const [key, value] of obj.getAll()) {
          const dest = fetchDest(value);
          if (dest) {
            dests[stringToPDFString(key, /* keepEscapeSequence = */ true)] =
              dest;
          }
        }
      } else if (obj instanceof Dict) {
        for (const [key, value] of obj) {
          const dest = fetchDest(value);
          if (dest) {
            // Always let the NameTree take precedence.
            dests[stringToPDFString(key, /* keepEscapeSequence = */ true)] ||=
              dest;
          }
        }
      }
    }

Review comment (suggestion):
Not super related to the changes made by this PR, but we can avoid unnecessarily calling the `hasActualNumPages` getter:
```suggestion
    return this.#actualNumPages ?? this._pagesCount;
```

Code after:
      off: parseOnOff(config.get("OFF")),
      order: parseOrder(config.get("Order")),
      groups: [...groupRefCache],
    };
  }

  setActualNumPages(num = null) {
    this.#actualNumPages = num;
  }

  get hasActualNumPages() {
    return this.#actualNumPages !== null;
  }

  get _pagesCount() {
    const obj = this.toplevelPagesDict.get("Count");
    if (!Number.isInteger(obj)) {
      throw new FormatError(
        "Page count in top-level pages dictionary is not an integer."
      );
    }
    return shadow(this, "_pagesCount", obj);
  }

  get numPages() {
    return this.#actualNumPages ?? this._pagesCount;
  }

  get destinations() {
    const rawDests = this.#readDests(),
      dests = Object.create(null);
    for (const obj of rawDests) {
      if (obj instanceof NameTree) {
        for (const [key, value] of obj.getAll()) {
          const dest = fetchDest(value);
          if (dest) {
            dests[stringToPDFString(key, /* keepEscapeSequence = */ true)] =
              dest;
          }
        }
      } else if (obj instanceof Dict) {
        for (const [key, value] of obj) {
          const dest = fetchDest(value);
          if (dest) {
            // Always let the NameTree take precedence.
            dests[stringToPDFString(key, /* keepEscapeSequence = */ true)] ||=
              dest;
          }
        }
      }
    }
Repository: mozilla/pdf.js
File: src/display/editor/stamp.js (JavaScript)

Code before:
  static _editorType = AnnotationEditorType.STAMP;

  constructor(params) {
    super({ ...params, name: "stampEditor" });
    this.#bitmapUrl = params.bitmapUrl;
    this.#bitmapFile = params.bitmapFile;
    this.defaultL10nId = "pdfjs-editor-stamp-editor";
  }

  /** @inheritdoc */
  static initialize(l10n, uiManager) {
    AnnotationEditor.initialize(l10n, uiManager);
  }

  /** @inheritdoc */
  static isHandlingMimeForPasting(mime) {
    return SupportedImageMimeTypes.includes(mime);
  }

  /** @inheritdoc */
  static paste(item, parent) {
    parent.pasteEditor(
      { mode: AnnotationEditorType.STAMP },
      {
        bitmapFile: item.getAsFile(),
      }
    );
  }

  /** @inheritdoc */
  altTextFinish() {
    if (this._uiManager.useNewAltTextFlow) {
      this.div.hidden = false;
    }
    super.altTextFinish();
  }

  /** @inheritdoc */
  get telemetryFinalData() {
    return {
      type: "stamp",
      hasAltText: !!this.altTextData?.altText,
    };
  }

  static computeTelemetryFinalData(data) {
    const hasAltTextStats = data.get("hasAltText");
    return {
      hasAltText: hasAltTextStats.get(true) ?? 0,
      hasNoAltText: hasAltTextStats.get(false) ?? 0,
    };

Review comment (suggestion):
Nit:
```suggestion
      { bitmapFile: item.getAsFile() }
```

Code after:
  static _editorType = AnnotationEditorType.STAMP;

  constructor(params) {
    super({ ...params, name: "stampEditor" });
    this.#bitmapUrl = params.bitmapUrl;
    this.#bitmapFile = params.bitmapFile;
    this.defaultL10nId = "pdfjs-editor-stamp-editor";
  }

  /** @inheritdoc */
  static initialize(l10n, uiManager) {
    AnnotationEditor.initialize(l10n, uiManager);
  }

  /** @inheritdoc */
  static isHandlingMimeForPasting(mime) {
    return SupportedImageMimeTypes.includes(mime);
  }

  /** @inheritdoc */
  static paste(item, parent) {
    parent.pasteEditor(
      { mode: AnnotationEditorType.STAMP },
      { bitmapFile: item.getAsFile() }
    );
  }

  /** @inheritdoc */
  altTextFinish() {
    if (this._uiManager.useNewAltTextFlow) {
      this.div.hidden = false;
    }
    super.altTextFinish();
  }

  /** @inheritdoc */
  get telemetryFinalData() {
    return {
      type: "stamp",
      hasAltText: !!this.altTextData?.altText,
    };
  }

  static computeTelemetryFinalData(data) {
    const hasAltTextStats = data.get("hasAltText");
    return {
      hasAltText: hasAltTextStats.get(true) ?? 0,
      hasNoAltText: hasAltTextStats.get(false) ?? 0,
    };
  }
Repository: mozilla/pdf.js
File: src/core/primitives.js (JavaScript)

Code before:

  setIfNotExists(key, value) {
    if (!this.has(key)) {
      this.set(key, value);
    }
  }

  setNumber(key, value) {
    if (typeof value === "number") {
      this.set(key, value);
    }
  }

  setArray(key, value) {
    if (Array.isArray(value) || ArrayBuffer.isView(value)) {
      this.set(key, value);
    }
  }

  setSomething(key, value) {
    if (value) {
      this.set(key, value);
    }
  }

  setName(key, value) {
    if (typeof value === "string") {
      this.set(key, Name.get(value));
    } else if (value instanceof Name) {
      this.set(key, value);
    }
  }

  has(key) {
    return this._map.has(key);
  }

  *[Symbol.iterator]() {
    for (const [key, value] of this._map) {
      yield [
        key,
        value instanceof Ref && this.xref
          ? this.xref.fetch(value, this.suppressEncryption)
          : value,
      ];
    }
  }

  static get empty() {
    const emptyDict = new Dict(null);


Review comment (suggestion):
Could we perhaps use slightly more "consistent" names for these new methods?
```suggestion
  setIfNumber(key, value) {
    if (typeof value === "number") {
      this.set(key, value);
    }
  }

  setIfArray(key, value) {
    if (Array.isArray(value) || ArrayBuffer.isView(value)) {
      this.set(key, value);
    }
  }

  setIfDefined(key, value) {
    if (value) {
      this.set(key, value);
    }
  }

  setIfName(key, value) {
```

Code after:

  setIfNotExists(key, value) {
    if (!this.has(key)) {
      this.set(key, value);
    }
  }

  setIfNumber(key, value) {
    if (typeof value === "number") {
      this.set(key, value);
    }
  }

  setIfArray(key, value) {
    if (Array.isArray(value) || ArrayBuffer.isView(value)) {
      this.set(key, value);
    }
  }

  setIfDefined(key, value) {
    if (value !== undefined && value !== null) {
      this.set(key, value);
    }
  }

  setIfName(key, value) {
    if (typeof value === "string") {
      this.set(key, Name.get(value));
    } else if (value instanceof Name) {
      this.set(key, value);
    }
  }

  has(key) {
    return this._map.has(key);
  }

  *[Symbol.iterator]() {
    for (const [key, value] of this._map) {
      yield [
        key,
        value instanceof Ref && this.xref
          ? this.xref.fetch(value, this.suppressEncryption)
          : value,
      ];
    }
  }

  static get empty() {
    const emptyDict = new Dict(null);
Repository: mozilla/pdf.js
File: src/core/core_utils.js (JavaScript)

Code before:
  if (!annotationStorage) {
    return null;
  }
  const newAnnotationsByPage = new Map();
  // The concept of page in a XFA is very different, so
  // editing is just not implemented.
  for (const [key, value] of annotationStorage) {
    if (!key.startsWith(AnnotationEditorPrefix)) {
      continue;
    }
    let annotations = newAnnotationsByPage.get(value.pageIndex);
    if (!annotations) {
      annotations = [];
      newAnnotationsByPage.set(value.pageIndex, annotations);
    }
    annotations.push(value);
  }
  return newAnnotationsByPage.size > 0 ? newAnnotationsByPage : null;
}

function stringToAsciiOrUTF16BE(str) {
  return isAscii(str) ? str : stringToUTF16String(str, /* bigEndian = */ true);
}

function isAscii(str) {
  return !str || /^[\x00-\x7F]*$/.test(str);
}

function stringToUTF16HexString(str) {
  const buf = [];
  for (let i = 0, ii = str.length; i < ii; i++) {
    const char = str.charCodeAt(i);
    buf.push(hexNumbers[(char >> 8) & 0xff], hexNumbers[char & 0xff]);
  }
  return buf.join("");
}

function stringToUTF16String(str, bigEndian = false) {
  const buf = [];
  if (bigEndian) {
    buf.push("\xFE\xFF");
  }
  for (let i = 0, ii = str.length; i < ii; i++) {
    const char = str.charCodeAt(i);
    buf.push(
      String.fromCharCode((char >> 8) & 0xff),
      String.fromCharCode(char & 0xff)
    );
  }
  return buf.join("");
}

Review comment (suggestion):
Please add a new test to https://github.com/mozilla/pdf.js/blob/master/test/unit/core_utils_spec.js#L427 to cover this change.

Code after:
  if (!annotationStorage) {
    return null;
  }
  const newAnnotationsByPage = new Map();
  // The concept of page in a XFA is very different, so
  // editing is just not implemented.
  for (const [key, value] of annotationStorage) {
    if (!key.startsWith(AnnotationEditorPrefix)) {
      continue;
    }
    let annotations = newAnnotationsByPage.get(value.pageIndex);
    if (!annotations) {
      annotations = [];
      newAnnotationsByPage.set(value.pageIndex, annotations);
    }
    annotations.push(value);
  }
  return newAnnotationsByPage.size > 0 ? newAnnotationsByPage : null;
}

// If the string is null or undefined then it is returned as is.
function stringToAsciiOrUTF16BE(str) {
  if (str === null || str === undefined) {
    return str;
  }
  return isAscii(str) ? str : stringToUTF16String(str, /* bigEndian = */ true);
}

function isAscii(str) {
  if (typeof str !== "string") {
    return false;
  }
  return !str || /^[\x00-\x7F]*$/.test(str);
}

function stringToUTF16HexString(str) {
  const buf = [];
  for (let i = 0, ii = str.length; i < ii; i++) {
    const char = str.charCodeAt(i);
    buf.push(hexNumbers[(char >> 8) & 0xff], hexNumbers[char & 0xff]);
  }
  return buf.join("");
}

function stringToUTF16String(str, bigEndian = false) {
  const buf = [];
  if (bigEndian) {
    buf.push("\xFE\xFF");
  }
  for (let i = 0, ii = str.length; i < ii; i++) {
    const char = str.charCodeAt(i);
Repository: mozilla/pdf.js
File: test/integration/thumbnail_view_spec.mjs (JavaScript)

Code before:
import { closePages, loadAndWait, waitForTimeout } from "./test_utils.mjs";

describe("PDF Thumbnail View", () => {
  describe("Works without errors", () => {
    let pages;

    beforeEach(async () => {
      pages = await loadAndWait("tracemonkey.pdf", "#sidebarToggleButton");
    });

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

    it("should render thumbnails without errors", async () => {
      await Promise.all(
        pages.map(async ([browserName, page]) => {
          await page.click("#sidebarToggleButton");

          const thumbSelector = "#thumbnailView .thumbnailImage";
          await page.waitForSelector(thumbSelector, { visible: true });

          await page.evaluate(() => {
            window.PDFViewerApplication.pdfViewer.currentScaleValue = "50";
          });
          await waitForTimeout(500); // Give time for rendering.

          const src = await page.$eval(thumbSelector, el => el.src);
          expect(src)
            .withContext(`In ${browserName}`)
            .toMatch(/^data:image\//);
        })
      );
    });
  });
});


Review comment (bug):
Linting currently fails on this because using `waitForTimeout` is discouraged as it often introduced intermittent failures, so deterministic methods should be used instead of fixed timeouts.

In this case it would be better to, for example, wait for the `data-loaded="true"` attribute to be present on the `thumbnail` class instead because that is an indicator that loading finished.

Code after:
import { closePages, loadAndWait } from "./test_utils.mjs";

describe("PDF Thumbnail View", () => {
  describe("Works without errors", () => {
    let pages;

    beforeEach(async () => {
      pages = await loadAndWait("tracemonkey.pdf", "#sidebarToggleButton");
    });

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

    it("should render thumbnails without errors", async () => {
      await Promise.all(
        pages.map(async ([browserName, page]) => {
          await page.click("#sidebarToggleButton");

          const thumbSelector = "#thumbnailView .thumbnailImage";
          await page.waitForSelector(thumbSelector, { visible: true });

          await page.waitForSelector(
            '#thumbnailView .thumbnail[data-loaded="true"]'
          );

          const src = await page.$eval(thumbSelector, el => el.src);
          expect(src)
            .withContext(`In ${browserName}`)
            .toMatch(/^data:image\//);
        })
      );
    });
  });
});
Repository: mozilla/pdf.js
File: src/core/annotation.js (JavaScript)

Code before:
      transform,
      matrix,
      isUsingOwnCanvas,
    ]);

    await evaluator.getOperatorList({
      stream: appearance,
      task,
      resources,
      operatorList: opList,
      fallbackFontDict: this._fallbackFontDict,
    });
    opList.addOp(OPS.endAnnotation, []);

    if (optionalContent !== undefined) {
      opList.addOp(OPS.endMarkedContent, []);
    }
    this.reset();
    return { opList, separateForm: false, separateCanvas: isUsingOwnCanvas };
  }

  async save(evaluator, task, annotationStorage, changes) {
    return null;
  }

  get hasHighlightedContent() {
    return false;
  }

  get hasTextContent() {
    return false;
  }

  async extractTextContent(evaluator, task, viewBox) {
    if (!this.appearance) {
      return;
    }

    const resources = await this.loadResources(
      RESOURCES_KEYS_TEXT_CONTENT,
      this.appearance
    );

    const text = [];
    const buffer = [];
    let firstPosition = null;
    const sink = {
      desiredSize: Math.Infinity,
      ready: true,

      enqueue(chunk, size) {

Review comment (suggestion):
Give that "highlight" is also a specific type of annotations, I think we should choose another term here to disambiguate between the highlight annotation type and the text under the annotations that you're indicating here. How about `get overlaysTextContent` (also aligns with `hasTextContent`) or something similar, to indicate that the annotation type is applied over text content? (This applies to the similarly named variables or members in this patch.)

Code after:
      rect,
      transform,
      matrix,
      isUsingOwnCanvas,
    ]);

    await evaluator.getOperatorList({
      stream: appearance,
      task,
      resources,
      operatorList: opList,
      fallbackFontDict: this._fallbackFontDict,
    });
    opList.addOp(OPS.endAnnotation, []);

    if (optionalContent !== undefined) {
      opList.addOp(OPS.endMarkedContent, []);
    }
    this.reset();
    return { opList, separateForm: false, separateCanvas: isUsingOwnCanvas };
  }

  async save(evaluator, task, annotationStorage, changes) {
    return null;
  }

  get overlaysTextContent() {
    return false;
  }

  get hasTextContent() {
    return false;
  }

  async extractTextContent(evaluator, task, viewBox) {
    if (!this.appearance) {
      return;
    }

    const resources = await this.loadResources(
      RESOURCES_KEYS_TEXT_CONTENT,
      this.appearance
    );

    const text = [];
    const buffer = [];
    let firstPosition = null;
    const sink = {
      desiredSize: Math.Infinity,
      ready: true,
Repository: mozilla/pdf.js
File: src/core/document.js (JavaScript)

Code before:
  isWhiteSpace,
  lookupNormalRect,
  MissingDataException,
  PDF_VERSION_REGEXP,
  RESOURCES_KEYS_OPERATOR_LIST,
  RESOURCES_KEYS_TEXT_CONTENT,
  validateCSSFont,
  XRefEntryException,
  XRefParseException,
} from "./core_utils.js";
import {
  Dict,
  isName,
  isRefsEqual,
  Name,
  Ref,
  RefSet,
  RefSetCache,
} from "./primitives.js";
import { getXfaFontDict, getXfaFontName } from "./xfa_fonts.js";
import { BaseStream } from "./base_stream.js";
import { calculateMD5 } from "./calculate_md5.js";
import { Catalog } from "./catalog.js";
import { clearGlobalCaches } from "./cleanup_helper.js";
import { DatasetReader } from "./dataset_reader.js";
import { Intersector } from "./highlighted_text.js";
import { Linearization } from "./parser.js";
import { NullStream } from "./stream.js";
import { ObjectLoader } from "./object_loader.js";
import { OperatorList } from "./operator_list.js";
import { PartialEvaluator } from "./evaluator.js";
import { StreamsSequenceStream } from "./decode_stream.js";
import { StructTreePage } from "./struct_tree.js";
import { XFAFactory } from "./xfa/factory.js";
import { XRef } from "./xref.js";

const LETTER_SIZE_MEDIABOX = [0, 0, 612, 792];

class Page {
  #resourcesPromise = null;

  constructor({
    pdfManager,
    xref,
    pageIndex,
    pageDict,
    ref,
    globalIdFactory,
    fontCache,
    builtInCMapCache,
    standardFontDataCache,

Review comment (suggestion):
Usually we name the file after the main content/class, so since it only contains the intersector logic let's call it `intersector.js`. This would also remove the ambiguity about the "highlight" term from the previous comment.

Code after:
  isWhiteSpace,
  lookupNormalRect,
  MissingDataException,
  PDF_VERSION_REGEXP,
  RESOURCES_KEYS_OPERATOR_LIST,
  RESOURCES_KEYS_TEXT_CONTENT,
  validateCSSFont,
  XRefEntryException,
  XRefParseException,
} from "./core_utils.js";
import {
  Dict,
  isName,
  isRefsEqual,
  Name,
  Ref,
  RefSet,
  RefSetCache,
} from "./primitives.js";
import { getXfaFontDict, getXfaFontName } from "./xfa_fonts.js";
import { BaseStream } from "./base_stream.js";
import { calculateMD5 } from "./calculate_md5.js";
import { Catalog } from "./catalog.js";
import { clearGlobalCaches } from "./cleanup_helper.js";
import { DatasetReader } from "./dataset_reader.js";
import { Intersector } from "./intersector.js";
import { Linearization } from "./parser.js";
import { NullStream } from "./stream.js";
import { ObjectLoader } from "./object_loader.js";
import { OperatorList } from "./operator_list.js";
import { PartialEvaluator } from "./evaluator.js";
import { StreamsSequenceStream } from "./decode_stream.js";
import { StructTreePage } from "./struct_tree.js";
import { XFAFactory } from "./xfa/factory.js";
import { XRef } from "./xref.js";

const LETTER_SIZE_MEDIABOX = [0, 0, 612, 792];

class Page {
  #resourcesPromise = null;

  constructor({
    pdfManager,
    xref,
    pageIndex,
    pageDict,
    ref,
    globalIdFactory,
    fontCache,
    builtInCMapCache,
    standardFontDataCache,
Repository: mozilla/pdf.js
File: src/display/annotation_layer.js (JavaScript)

Code before:
  }

  getElementsToTriggerPopup() {
    return this.#polylines;
  }

  addHighlightArea() {
    this.container.classList.add("highlightArea");
  }
}

class HighlightAnnotationElement extends AnnotationElement {
  constructor(parameters) {
    super(parameters, {
      isRenderable: true,
      ignoreBorder: true,
      createQuadrilaterals: true,
    });
    this.annotationEditorType = AnnotationEditorType.HIGHLIGHT;
  }

  render() {
    const {
      data: { highlightedText, popupRef },
    } = this;

    if (!popupRef && this.hasPopupData) {
      this._createPopup();
    }

    this.container.classList.add("highlightAnnotation");
    this._editOnDoubleClick();

    if (highlightedText) {
      const mark = document.createElement("mark");
      mark.classList.add("highlightedText");
      mark.textContent = highlightedText;
      this.container.append(mark);
    }

    return this.container;
  }
}

class UnderlineAnnotationElement extends AnnotationElement {
  constructor(parameters) {
    super(parameters, {
      isRenderable: true,
      ignoreBorder: true,
      createQuadrilaterals: true,
    });

Review comment (refactor):
Let's remove this newline given that the other three classes below don't have it as well (alternatively, add it there too).

Code after:
      root.setAttribute("viewBox", `0 0 ${width} ${height}`);
      g.setAttribute("transform", transform);
    }
  }

  getElementsToTriggerPopup() {
    return this.#polylines;
  }

  addHighlightArea() {
    this.container.classList.add("highlightArea");
  }
}

class HighlightAnnotationElement extends AnnotationElement {
  constructor(parameters) {
    super(parameters, {
      isRenderable: true,
      ignoreBorder: true,
      createQuadrilaterals: true,
    });
    this.annotationEditorType = AnnotationEditorType.HIGHLIGHT;
  }

  render() {
    const {
      data: { overlaidText, popupRef },
    } = this;
    if (!popupRef && this.hasPopupData) {
      this._createPopup();
    }

    this.container.classList.add("highlightAnnotation");
    this._editOnDoubleClick();

    if (overlaidText) {
      const mark = document.createElement("mark");
      mark.classList.add("overlaidText");
      mark.textContent = overlaidText;
      this.container.append(mark);
    }

    return this.container;
  }
}

class UnderlineAnnotationElement extends AnnotationElement {
  constructor(parameters) {
    super(parameters, {
      isRenderable: true,
      ignoreBorder: true,