ronantakizawa/codereview-bench huggingface dataset:
Repository: mozilla/pdf.js
File: web/pdf_page_view.js (JavaScript)

Code before:
        "--hcm-highlight-selected-filter",
        pdfPage.filterFactory.addHighlightHCMFilter(
          "highlight_selected",
          "CanvasText",
          "Canvas",
          "HighlightText",
          "Highlight"
        )
      );
    }
    this.pdfPage = pdfPage;
    this.pdfPageRotate = pdfPage.rotate;

    const totalRotation = (this.rotation + this.pdfPageRotate) % 360;
    this.viewport = pdfPage.getViewport({
      scale: this.scale * PixelsPerInch.PDF_TO_CSS_UNITS,
      rotation: totalRotation,
    });
    this.#setDimensions();
    this.reset();
  }

  destroy() {
    this.reset();
    this.#canvasWrapper?.remove();
    this.#canvasWrapper = null;
    this.pdfPage?.cleanup();
  }

  hasEditableAnnotations() {
    return !!this.annotationLayer?.hasEditableAnnotations();
  }

  get _textHighlighter() {
    return shadow(
      this,
      "_textHighlighter",
      new TextHighlighter({
        pageIndex: this.id - 1,
        eventBus: this.eventBus,
        findController: this.#layerProperties.findController,
      })
    );
  }

  #dispatchLayerRendered(name, error) {
    this.eventBus.dispatch(name, {
      source: this,
      pageNumber: this.id,
      error,
    });

Review comment (suggestion):
Aren't these lines redundant, since calling `this.reset()` without any parameters *should* already be doing that via the following code?
https://github.com/mozilla/pdf.js/blob/7495ad0b42eebfbe83a09dab36529a7ad3eb079b/web/pdf_page_view.js#L585-L589

Code after:
        "--hcm-highlight-selected-filter",
        pdfPage.filterFactory.addHighlightHCMFilter(
          "highlight_selected",
          "CanvasText",
          "Canvas",
          "HighlightText",
          "Highlight"
        )
      );
    }
    this.pdfPage = pdfPage;
    this.pdfPageRotate = pdfPage.rotate;

    const totalRotation = (this.rotation + this.pdfPageRotate) % 360;
    this.viewport = pdfPage.getViewport({
      scale: this.scale * PixelsPerInch.PDF_TO_CSS_UNITS,
      rotation: totalRotation,
    });
    this.#setDimensions();
    this.reset();
  }

  destroy() {
    this.reset();
    this.pdfPage?.cleanup();
  }

  hasEditableAnnotations() {
    return !!this.annotationLayer?.hasEditableAnnotations();
  }

  get _textHighlighter() {
    return shadow(
      this,
      "_textHighlighter",
      new TextHighlighter({
        pageIndex: this.id - 1,
        eventBus: this.eventBus,
        findController: this.#layerProperties.findController,
      })
    );
  }

  #dispatchLayerRendered(name, error) {
    this.eventBus.dispatch(name, {
      source: this,
      pageNumber: this.id,
      error,
    });
  }
Repository: mozilla/pdf.js
File: web/pdf_find_bar.js (JavaScript)

Code before:
      [this.entireWord, "entirewordchange"],
      [this.matchDiacritics, "diacriticmatchingchange"],
    ]);

    // Add event listeners to the DOM elements.
    this.toggleButton.addEventListener("click", () => {
      this.toggle();
    });

    this.findField.addEventListener("input", () => {
      this.dispatchEvent("");
    });

    this.bar.addEventListener("keydown", ({ keyCode, shiftKey, target }) => {
      switch (keyCode) {
        case 13: // Enter
          if (target === this.findField) {
            this.dispatchEvent("again", shiftKey);
          } else {
            for (const [elem, evtName] of checkedInputs) {
              if (target === elem) {
                elem.checked = !elem.checked;
                this.dispatchEvent(evtName);
                break;
              }
            }
          }
          break;
        case 27: // Escape
          this.close();
          break;
      }
    });

    this.findPreviousButton.addEventListener("click", () => {
      this.dispatchEvent("again", true);
    });
    this.findNextButton.addEventListener("click", () => {
      this.dispatchEvent("again", false);
    });

    for (const [elem, evtName] of checkedInputs) {
      elem.addEventListener("click", () => {
        this.dispatchEvent(evtName);
      });
    }
  }

  reset() {
    this.updateUIState();
  }

Review comment (suggestion):
```suggestion
            if (checkedInputs.has(target)) {
              target.checked = !target.checked;
              this.dispatchEvent(checkedInputs.get(target));
            }
```

Given that `checkedInputs` is a map, we don't need to iterate its entries one by one.

Code after:
      [this.entireWord, "entirewordchange"],
      [this.matchDiacritics, "diacriticmatchingchange"],
    ]);

    // Add event listeners to the DOM elements.
    this.toggleButton.addEventListener("click", () => {
      this.toggle();
    });

    this.findField.addEventListener("input", () => {
      this.dispatchEvent("");
    });

    this.bar.addEventListener("keydown", ({ keyCode, shiftKey, target }) => {
      switch (keyCode) {
        case 13: // Enter
          if (target === this.findField) {
            this.dispatchEvent("again", shiftKey);
          } else if (checkedInputs.has(target)) {
            target.checked = !target.checked;
            this.dispatchEvent(/* evtName = */ checkedInputs.get(target));
          }
          break;
        case 27: // Escape
          this.close();
          break;
      }
    });

    this.findPreviousButton.addEventListener("click", () => {
      this.dispatchEvent("again", true);
    });
    this.findNextButton.addEventListener("click", () => {
      this.dispatchEvent("again", false);
    });

    for (const [elem, evtName] of checkedInputs) {
      elem.addEventListener("click", () => {
        this.dispatchEvent(evtName);
      });
    }
  }

  reset() {
    this.updateUIState();
  }

  dispatchEvent(type, findPrev = false) {
    this.eventBus.dispatch("find", {
      source: this,
      type,
Repository: mozilla/pdf.js
File: src/core/font_renderer.js (JavaScript)

Code before:
          }
          break;
      }
      if (stackClean) {
        stack.length = 0;
      }
    }
  }
  parse(charStringCode);
}

const NOOP = [];

class Commands {
  cmds = [];

  transformStack = [];

  currentTransform = [1, 0, 0, 1, 0, 0];

  add(cmd, args) {
    if (args) {
      if (!isNumberArray(args, null)) {
        warn(
          `Commands.add - "${cmd}" has at least one non-number arg: "${args}".`
        );
      }
      // "Fix" the wrong args by replacing them with 0.
      const [a, b, c, d, e, f] = this.currentTransform;
      for (let i = 0, ii = args.length; i < ii; i += 2) {
        let x = args[i];
        x = typeof x === "number" ? x : 0;
        let y = args[i + 1];
        y = typeof y === "number" ? y : 0;
        args[i] = a * x + c * y + e;
        args[i + 1] = b * x + d * y + f;
      }
      this.cmds.push(`${cmd}${args.join(" ")}`);
    } else {
      this.cmds.push(cmd);
    }
  }

  transform(transf) {
    if (!this.currentTransform) {
      this.currentTransform = transf;
      return;
    }
    const [a1, b1, c1, d1, e1, f1] = this.currentTransform;
    const [a2, b2, c2, d2, e2, f2] = transf;
    this.currentTransform = [

Review comment (refactor):
*Slightly unrelated to this patch perhaps, but something that I've been wondering:*

If a glyph outline contains non-number arguments, is it actually meaningful to replace those with zero since it seems to me that generally speaking there's no guarantee that it'd render "correctly" (whatever that even means with corrupt font data)?
Or, could we perhaps simplify things a little bit here by just ignoring the *entire* glyph in that case?
```suggestion
        throw new FormatError(
          `Commands.add - "${cmd}" has at least one non-number arg: "${args}".`
        );
```


Code after:
          }
          break;
      }
      if (stackClean) {
        stack.length = 0;
      }
    }
  }
  parse(charStringCode);
}

const NOOP = "";

class Commands {
  cmds = [];

  transformStack = [];

  currentTransform = [1, 0, 0, 1, 0, 0];

  add(cmd, args) {
    if (args) {
      const [a, b, c, d, e, f] = this.currentTransform;
      for (let i = 0, ii = args.length; i < ii; i += 2) {
        const x = args[i];
        const y = args[i + 1];
        args[i] = a * x + c * y + e;
        args[i + 1] = b * x + d * y + f;
      }
      this.cmds.push(`${cmd}${args.join(" ")}`);
    } else {
      this.cmds.push(cmd);
    }
  }

  transform(transf) {
    this.currentTransform = Util.transform(this.currentTransform, transf);
  }

  translate(x, y) {
    this.transform([1, 0, 0, 1, x, y]);
  }

  save() {
    this.transformStack.push(this.currentTransform.slice());
  }

  restore() {
    this.currentTransform = this.transformStack.pop() || [1, 0, 0, 1, 0, 0];
  }
Repository: mozilla/pdf.js
File: src/core/font_renderer.js (JavaScript)

Code before:
      if (!isNumberArray(args, null)) {
        warn(
          `Commands.add - "${cmd}" has at least one non-number arg: "${args}".`
        );
      }
      // "Fix" the wrong args by replacing them with 0.
      const [a, b, c, d, e, f] = this.currentTransform;
      for (let i = 0, ii = args.length; i < ii; i += 2) {
        let x = args[i];
        x = typeof x === "number" ? x : 0;
        let y = args[i + 1];
        y = typeof y === "number" ? y : 0;
        args[i] = a * x + c * y + e;
        args[i + 1] = b * x + d * y + f;
      }
      this.cmds.push(`${cmd}${args.join(" ")}`);
    } else {
      this.cmds.push(cmd);
    }
  }

  transform(transf) {
    if (!this.currentTransform) {
      this.currentTransform = transf;
      return;
    }
    const [a1, b1, c1, d1, e1, f1] = this.currentTransform;
    const [a2, b2, c2, d2, e2, f2] = transf;
    this.currentTransform = [
      a1 * a2 + b1 * c2,
      a1 * b2 + b1 * d2,
      c1 * a2 + d1 * c2,
      c1 * b2 + d1 * d2,
      e1 * a2 + f1 * c2 + e2,
      e1 * b2 + f1 * d2 + f2,
    ];
  }

  translate(x, y) {
    this.transform([1, 0, 0, 1, x, y]);
  }

  save() {
    this.transformStack.push(this.currentTransform.slice());
  }

  restore() {
    this.transformStack.pop();
    this.currentTransform = this.transformStack.at(-1) || [1, 0, 0, 1, 0, 0];
  }


Review comment (suggestion):
Can `this.currentTransform` actually be unset?

It's always initialized at https://github.com/mozilla/pdf.js/blob/1eb1f73d1607e37216700f86896ba43ddd6e5908/src/core/font_renderer.js#L746 and it also seems that the `restore`-method guarantees that it'll be defined given https://github.com/mozilla/pdf.js/blob/1eb1f73d1607e37216700f86896ba43ddd6e5908/src/core/font_renderer.js#L798



Code after:
      const [a, b, c, d, e, f] = this.currentTransform;
      for (let i = 0, ii = args.length; i < ii; i += 2) {
        const x = args[i];
        const y = args[i + 1];
        args[i] = a * x + c * y + e;
        args[i + 1] = b * x + d * y + f;
      }
      this.cmds.push(`${cmd}${args.join(" ")}`);
    } else {
      this.cmds.push(cmd);
    }
  }

  transform(transf) {
    this.currentTransform = Util.transform(this.currentTransform, transf);
  }

  translate(x, y) {
    this.transform([1, 0, 0, 1, x, y]);
  }

  save() {
    this.transformStack.push(this.currentTransform.slice());
  }

  restore() {
    this.currentTransform = this.transformStack.pop() || [1, 0, 0, 1, 0, 0];
  }

  getSVG() {
    return this.cmds.join("");
  }
}

class CompiledFont {
  constructor(fontMatrix) {
    if (
      (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) &&
      this.constructor === CompiledFont
    ) {
      unreachable("Cannot initialize CompiledFont.");
    }
    this.fontMatrix = fontMatrix;

    this.compiledGlyphs = Object.create(null);
    this.compiledCharCodeToGlyphId = Object.create(null);
  }

  getPathJs(unicode) {
    const { charCode, glyphId } = lookupCmap(this.cmap, unicode);
    let fn = this.compiledGlyphs[glyphId],
Repository: mozilla/pdf.js
File: src/core/font_renderer.js (JavaScript)

Code before:
        let y = args[i + 1];
        y = typeof y === "number" ? y : 0;
        args[i] = a * x + c * y + e;
        args[i + 1] = b * x + d * y + f;
      }
      this.cmds.push(`${cmd}${args.join(" ")}`);
    } else {
      this.cmds.push(cmd);
    }
  }

  transform(transf) {
    if (!this.currentTransform) {
      this.currentTransform = transf;
      return;
    }
    const [a1, b1, c1, d1, e1, f1] = this.currentTransform;
    const [a2, b2, c2, d2, e2, f2] = transf;
    this.currentTransform = [
      a1 * a2 + b1 * c2,
      a1 * b2 + b1 * d2,
      c1 * a2 + d1 * c2,
      c1 * b2 + d1 * d2,
      e1 * a2 + f1 * c2 + e2,
      e1 * b2 + f1 * d2 + f2,
    ];
  }

  translate(x, y) {
    this.transform([1, 0, 0, 1, x, y]);
  }

  save() {
    this.transformStack.push(this.currentTransform.slice());
  }

  restore() {
    this.transformStack.pop();
    this.currentTransform = this.transformStack.at(-1) || [1, 0, 0, 1, 0, 0];
  }

  getSVG() {
    return this.cmds.join("");
  }
}

class CompiledFont {
  constructor(fontMatrix) {
    if (
      (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) &&
      this.constructor === CompiledFont

Review comment (refactor):
This looks very similar to the existing `Util.transform` helper, can we use that one instead?

Code after:
    }
  }

  transform(transf) {
    this.currentTransform = Util.transform(this.currentTransform, transf);
  }

  translate(x, y) {
    this.transform([1, 0, 0, 1, x, y]);
  }

  save() {
    this.transformStack.push(this.currentTransform.slice());
  }

  restore() {
    this.currentTransform = this.transformStack.pop() || [1, 0, 0, 1, 0, 0];
  }

  getSVG() {
    return this.cmds.join("");
  }
}

class CompiledFont {
  constructor(fontMatrix) {
    if (
      (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) &&
      this.constructor === CompiledFont
    ) {
      unreachable("Cannot initialize CompiledFont.");
    }
    this.fontMatrix = fontMatrix;

    this.compiledGlyphs = Object.create(null);
    this.compiledCharCodeToGlyphId = Object.create(null);
  }

  getPathJs(unicode) {
    const { charCode, glyphId } = lookupCmap(this.cmap, unicode);
    let fn = this.compiledGlyphs[glyphId],
      compileEx;
    if (fn === undefined) {
      try {
        fn = this.compileGlyph(this.glyphs[glyphId], glyphId);
      } catch (ex) {
        fn = NOOP; // Avoid attempting to re-compile a corrupt glyph.

        compileEx = ex;
      }
      this.compiledGlyphs[glyphId] = fn;
Repository: mozilla/pdf.js
File: src/display/editor/draw.js (JavaScript)

Code before:
  }

  updateAll(options = this) {
    this.updateProperties(options);
  }

  clone() {
    unreachable("Not implemented");
  }
}

/**
 * Basic draw editor.
 */
class DrawingEditor extends AnnotationEditor {
  #drawOutlines = null;

  #mustBeCommitted;

  _drawId = null;

  static _currentDrawId = -1;

  static _currentDraw = null;

  static _currentDrawingAC = null;

  static _currentDrawingOptions = null;

  static _currentParent = null;

  static _currentPointerId = null;

  static _currentPointerType = null;

  static _currentPointerIds = null;

  static _currentMoveTimestamp = -1;

  static _INNER_MARGIN = 3;

  constructor(params) {
    super(params);
    this.#mustBeCommitted = params.mustBeCommitted || false;

    if (params.drawOutlines) {
      this.#createDrawOutlines(params);
      this.#addToDrawLayer();
    }
  }


Review comment (suggestion):
For all the new fields added in the patch, here and below, could we make them actually private rather than just semi-private?
```suggestion
  static #currentDrawingAC = null;
```

Code after:
  }

  updateAll(options = this) {
    this.updateProperties(options);
  }

  clone() {
    unreachable("Not implemented");
  }
}

/**
 * Basic draw editor.
 */
class DrawingEditor extends AnnotationEditor {
  #drawOutlines = null;

  #mustBeCommitted;

  _drawId = null;

  static _currentDrawId = -1;

  static _currentParent = null;

  static #currentDraw = null;

  static #currentDrawingAC = null;

  static #currentDrawingOptions = null;

  static #currentPointerId = NaN;

  static #currentPointerType = null;

  static #currentPointerIds = null;

  static #currentMoveTimestamp = NaN;

  static _INNER_MARGIN = 3;

  constructor(params) {
    super(params);
    this.#mustBeCommitted = params.mustBeCommitted || false;

    if (params.drawOutlines) {
      this.#createDrawOutlines(params);
      this.#addToDrawLayer();
    }
  }
Repository: mozilla/pdf.js
File: src/display/touch_manager.js (JavaScript)

Code before:

    let [touch0, touch1] = evt.touches;
    if (touch0.identifier > touch1.identifier) {
      [touch0, touch1] = [touch1, touch0];
    }
    const { screenX: screen0X, screenY: screen0Y } = touch0;
    const { screenX: screen1X, screenY: screen1Y } = touch1;
    const touchInfo = this.#touchInfo;
    const {
      touch0X: pTouch0X,
      touch0Y: pTouch0Y,
      touch1X: pTouch1X,
      touch1Y: pTouch1Y,
    } = touchInfo;

    const prevGapX = pTouch1X - pTouch0X;
    const prevGapY = pTouch1Y - pTouch0Y;
    const currGapX = screen1X - screen0X;
    const currGapY = screen1Y - screen0Y;

    const distance = Math.hypot(currGapX, currGapY) || 1;
    const pDistance = Math.hypot(prevGapX, prevGapY) || 1;
    if (
      !this.#isPinching &&
      Math.abs(pDistance - distance) <=
        TouchManager.#MIN_TOUCH_DISTANCE_TO_PINCH
    ) {
      return;
    }

    touchInfo.touch0X = screen0X;
    touchInfo.touch0Y = screen0Y;
    touchInfo.touch1X = screen1X;
    touchInfo.touch1Y = screen1Y;

    evt.preventDefault();

    if (!this.#isPinching) {
      // Start pinching.
      this.#isPinching = true;

      // We return here else the first pinch is a bit too much
      return;
    }

    const origin = [(screen0X + screen1X) / 2, (screen0Y + screen1Y) / 2];
    this.#onPinching?.(origin, pDistance, distance);
  }

  #onTouchEnd(evt) {
    this.#touchMoveAC.abort();

Review comment (suggestion):
To make use of the getter:
```suggestion
        TouchManager.MIN_TOUCH_DISTANCE_TO_PINCH
```

Code after:
    if (touch0.identifier > touch1.identifier) {
      [touch0, touch1] = [touch1, touch0];
    }
    const { screenX: screen0X, screenY: screen0Y } = touch0;
    const { screenX: screen1X, screenY: screen1Y } = touch1;
    const touchInfo = this.#touchInfo;
    const {
      touch0X: pTouch0X,
      touch0Y: pTouch0Y,
      touch1X: pTouch1X,
      touch1Y: pTouch1Y,
    } = touchInfo;

    const prevGapX = pTouch1X - pTouch0X;
    const prevGapY = pTouch1Y - pTouch0Y;
    const currGapX = screen1X - screen0X;
    const currGapY = screen1Y - screen0Y;

    const distance = Math.hypot(currGapX, currGapY) || 1;
    const pDistance = Math.hypot(prevGapX, prevGapY) || 1;
    if (
      !this.#isPinching &&
      Math.abs(pDistance - distance) <= TouchManager.MIN_TOUCH_DISTANCE_TO_PINCH
    ) {
      return;
    }

    touchInfo.touch0X = screen0X;
    touchInfo.touch0Y = screen0Y;
    touchInfo.touch1X = screen1X;
    touchInfo.touch1Y = screen1Y;

    evt.preventDefault();

    if (!this.#isPinching) {
      // Start pinching.
      this.#isPinching = true;

      // We return here else the first pinch is a bit too much
      return;
    }

    const origin = [(screen0X + screen1X) / 2, (screen0Y + screen1Y) / 2];
    this.#onPinching?.(origin, pDistance, distance);
  }

  #onTouchEnd(evt) {
    this.#touchMoveAC.abort();
    this.#touchMoveAC = null;

    if (!this.#touchInfo) {
Repository: mozilla/pdf.js
File: web/app.js (JavaScript)

Code before:
        evt => preferences.set(evt.name, evt.value),
        opts
      );
    }
  },

  bindWindowEvents() {
    if (this._windowAbortController) {
      return;
    }
    this._windowAbortController = new AbortController();

    const {
      eventBus,
      appConfig: { mainContainer },
      pdfViewer,
      _windowAbortController: { signal },
    } = this;

    this._touchManager = new TouchManager({
      container: window,
      overlayManager: this.overlayManager,
      isPinchingDisabled: () => this.pdfViewer.isInPresentationMode,
      onPinching: this.touchPinchCallback.bind(this),
      onPinchEnd: this.touchPinchEndCallback.bind(this),
      signal: this._windowAbortController.signal,
    });

    function addWindowResolutionChange(evt = null) {
      if (evt) {
        pdfViewer.refresh();
      }
      const mediaQueryList = window.matchMedia(
        `(resolution: ${window.devicePixelRatio || 1}dppx)`
      );
      mediaQueryList.addEventListener("change", addWindowResolutionChange, {
        once: true,
        signal,
      });
    }
    addWindowResolutionChange();

    window.addEventListener("wheel", onWheel.bind(this), {
      passive: false,
      signal,
    });
    window.addEventListener("click", onClick.bind(this), { signal });
    window.addEventListener("keydown", onKeyDown.bind(this), { signal });
    window.addEventListener("keyup", onKeyUp.bind(this), { signal });
    window.addEventListener(
      "resize",

Review comment (suggestion):
I believe this is possible to shorten now.
```suggestion
      signal,
```

Code after:
        evt => preferences.set(evt.name, evt.value),
        opts
      );
    }
  },

  bindWindowEvents() {
    if (this._windowAbortController) {
      return;
    }
    this._windowAbortController = new AbortController();

    const {
      eventBus,
      appConfig: { mainContainer },
      pdfViewer,
      _windowAbortController: { signal },
    } = this;

    this._touchManager = new TouchManager({
      container: window,
      isPinchingDisabled: () => this.pdfViewer.isInPresentationMode,
      isPinchingStopped: () => this.overlayManager?.active,
      onPinching: this.touchPinchCallback.bind(this),
      onPinchEnd: this.touchPinchEndCallback.bind(this),
      signal,
    });

    function addWindowResolutionChange(evt = null) {
      if (evt) {
        pdfViewer.refresh();
      }
      const mediaQueryList = window.matchMedia(
        `(resolution: ${window.devicePixelRatio || 1}dppx)`
      );
      mediaQueryList.addEventListener("change", addWindowResolutionChange, {
        once: true,
        signal,
      });
    }
    addWindowResolutionChange();

    window.addEventListener("wheel", onWheel.bind(this), {
      passive: false,
      signal,
    });
    window.addEventListener("click", onClick.bind(this), { signal });
    window.addEventListener("keydown", onKeyDown.bind(this), { signal });
    window.addEventListener("keyup", onKeyUp.bind(this), { signal });
    window.addEventListener(
      "resize",