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

Code before:
      length: this.length,
    };
  }

  get _transfers() {
    const transfers = [];
    const { fnArray, argsArray, length } = this;
    for (let i = 0; i < length; i++) {
      switch (fnArray[i]) {
        case OPS.paintInlineImageXObject:
        case OPS.paintInlineImageXObjectGroup:
        case OPS.paintImageMaskXObject:
          const arg = argsArray[i][0]; // First parameter in imgData.
          if (arg.data?.buffer instanceof ArrayBuffer) {
            transfers.push(arg.data.buffer);
          }
          break;
        case OPS.constructPath:
          const [, [data], minMax] = argsArray[i];
          if (data) {
            transfers.push(data.buffer, minMax.buffer);
          }
          break;
        case OPS.paintFormXObjectBegin:
          const [matrix, bbox] = argsArray[i];
          transfers.push(matrix.buffer);
          if (bbox) {
            transfers.push(bbox.buffer);
          }
          break;
      }
    }
    return transfers;
  }

  flush(lastChunk = false, separateAnnots = null) {
    this.optimizer.flush();
    const length = this.length;
    this._totalLength += length;

    this._streamSink.enqueue(
      {
        fnArray: this.fnArray,
        argsArray: this.argsArray,
        lastChunk,
        separateAnnots,
        length,
      },
      1,
      this._transfers
    );

Review comment (suggestion):
Given the previous comment.
```suggestion
          if (matrix) {
            transfers.push(matrix.buffer);
          }
```

Code after:
      length: this.length,
    };
  }

  get _transfers() {
    const transfers = [];
    const { fnArray, argsArray, length } = this;
    for (let i = 0; i < length; i++) {
      switch (fnArray[i]) {
        case OPS.paintInlineImageXObject:
        case OPS.paintInlineImageXObjectGroup:
        case OPS.paintImageMaskXObject:
          const arg = argsArray[i][0]; // First parameter in imgData.
          if (arg.data?.buffer instanceof ArrayBuffer) {
            transfers.push(arg.data.buffer);
          }
          break;
        case OPS.constructPath:
          const [, [data], minMax] = argsArray[i];
          if (data) {
            transfers.push(data.buffer, minMax.buffer);
          }
          break;
        case OPS.paintFormXObjectBegin:
          const [matrix, bbox] = argsArray[i];
          if (matrix) {
            transfers.push(matrix.buffer);
          }
          if (bbox) {
            transfers.push(bbox.buffer);
          }
          break;
      }
    }
    return transfers;
  }

  flush(lastChunk = false, separateAnnots = null) {
    this.optimizer.flush();
    const length = this.length;
    this._totalLength += length;

    this._streamSink.enqueue(
      {
        fnArray: this.fnArray,
        argsArray: this.argsArray,
        lastChunk,
        separateAnnots,
        length,
      },
      1,
Repository: mozilla/pdf.js
File: src/display/display_utils.js (JavaScript)

Code before:
    /**
     * @type {number} Vertical scale.
     */
    this.sy = pixelRatio;
  }

  /**
   * @type {boolean} Returns `true` when scaling is required, `false` otherwise.
   */
  get scaled() {
    return this.sx !== 1 || this.sy !== 1;
  }

  /**
   * @type {boolean} Returns `true` when scaling is symmetric,
   *   `false` otherwise.
   */
  get symmetric() {
    return this.sx === this.sy;
  }

  /**
   * @returns {boolean} Returns `true` if scaling was limited,
   *   `false` otherwise.
   */
  limitCanvas(width, height, maxPixels, maxDim, capCanvasAreaFactor) {
    let maxAreaScale = Infinity,
      maxWidthScale = Infinity,
      maxHeightScale = Infinity;

    if (capCanvasAreaFactor >= 0) {
      const { innerWidth, innerHeight } = window;
      maxPixels = Math.min(
        maxPixels,
        innerWidth *
          innerHeight *
          (1 + capCanvasAreaFactor / 100) *
          OutputScale.pixelRatio ** 2
      );
    }
    if (maxPixels > 0) {
      maxAreaScale = Math.sqrt(maxPixels / (width * height));
    }
    if (maxDim !== -1) {
      maxWidthScale = maxDim / width;
      maxHeightScale = maxDim / height;
    }
    const maxScale = Math.min(maxAreaScale, maxWidthScale, maxHeightScale);

    if (this.sx > maxScale || this.sy > maxScale) {
      this.sx = maxScale;

Review comment (suggestion):
Nit: The other parameters don't contain the word "Canvas" to shorten the method signature a little bit.

Also, since the thumbnail rendering also uses this method (where the cap-parameter won't be used) let's give the new parameter a default value.
```suggestion
  limitCanvas(width, height, maxPixels, maxDim, capAreaFactor = -1) {
```

Code after:
    this.sx = pixelRatio;

    /**
     * @type {number} Vertical scale.
     */
    this.sy = pixelRatio;
  }

  /**
   * @type {boolean} Returns `true` when scaling is required, `false` otherwise.
   */
  get scaled() {
    return this.sx !== 1 || this.sy !== 1;
  }

  /**
   * @type {boolean} Returns `true` when scaling is symmetric,
   *   `false` otherwise.
   */
  get symmetric() {
    return this.sx === this.sy;
  }

  /**
   * @returns {boolean} Returns `true` if scaling was limited,
   *   `false` otherwise.
   */
  limitCanvas(width, height, maxPixels, maxDim, capAreaFactor = -1) {
    let maxAreaScale = Infinity,
      maxWidthScale = Infinity,
      maxHeightScale = Infinity;

    if (capAreaFactor >= 0) {
      const cappedWindowArea = OutputScale.getCappedWindowArea(capAreaFactor);
      maxPixels =
        maxPixels > 0
          ? Math.min(maxPixels, cappedWindowArea)
          : cappedWindowArea;
    }
    if (maxPixels > 0) {
      maxAreaScale = Math.sqrt(maxPixels / (width * height));
    }
    if (maxDim !== -1) {
      maxWidthScale = maxDim / width;
      maxHeightScale = maxDim / height;
    }
    const maxScale = Math.min(maxAreaScale, maxWidthScale, maxHeightScale);

    if (this.sx > maxScale || this.sy > maxScale) {
      this.sx = maxScale;
      this.sy = maxScale;
Repository: mozilla/pdf.js
File: src/display/display_utils.js (JavaScript)

Code before:
      this.sx = maxScale;
      this.sy = maxScale;
      return true;
    }
    return false;
  }

  static get pixelRatio() {
    return globalThis.devicePixelRatio || 1;
  }

  static getCappedWindowArea(capAreaFactor) {
    if (typeof PDFJSDev !== "undefined" && PDFJSDev.test("TESTING")) {
      return (
        window.innerWidth *
        window.innerHeight *
        this.pixelRatio ** 2 *
        (1 + capAreaFactor / 100)
      );
    }
    return (
      window.screen.availWidth *
      window.screen.availHeight *
      this.pixelRatio ** 2 *
      (1 + capAreaFactor / 100)
    );
  }
}

// See https://developer.mozilla.org/en-US/docs/Web/Media/Formats/Image_types
// to know which types are supported by the browser.
const SupportedImageMimeTypes = [
  "image/apng",
  "image/avif",
  "image/bmp",
  "image/gif",
  "image/jpeg",
  "image/png",
  "image/svg+xml",
  "image/webp",
  "image/x-icon",
];

export {
  deprecated,
  fetchData,
  getColorValues,
  getCurrentTransform,
  getCurrentTransformInverse,
  getFilenameFromUrl,
  getPdfFilenameFromUrl,

Review comment (suggestion):
It just occurred to me that the pixelRatio can be a decimal number and that the capAreaFactor could be any integer (e.g. `123`), hence the resulting value could also become a decimal number which seems a bit strange.
While that probably won't cause any issues, it still seems nice to ensure that the result is always an integer.

```suggestion
      return Math.ceil(
        window.innerWidth *
        window.innerHeight *
        this.pixelRatio ** 2 *
        (1 + capAreaFactor / 100)
      );
    }
    return Math.ceil(
      window.screen.availWidth *
      window.screen.availHeight *
      this.pixelRatio ** 2 *
      (1 + capAreaFactor / 100)
    );
```

Code after:
      this.sx = maxScale;
      this.sy = maxScale;
      return true;
    }
    return false;
  }

  static get pixelRatio() {
    return globalThis.devicePixelRatio || 1;
  }

  static getCappedWindowArea(capAreaFactor) {
    if (typeof PDFJSDev !== "undefined" && PDFJSDev.test("TESTING")) {
      return Math.ceil(
        window.innerWidth *
          window.innerHeight *
          this.pixelRatio ** 2 *
          (1 + capAreaFactor / 100)
      );
    }
    return Math.ceil(
      window.screen.availWidth *
        window.screen.availHeight *
        this.pixelRatio ** 2 *
        (1 + capAreaFactor / 100)
    );
  }
}

// See https://developer.mozilla.org/en-US/docs/Web/Media/Formats/Image_types
// to know which types are supported by the browser.
const SupportedImageMimeTypes = [
  "image/apng",
  "image/avif",
  "image/bmp",
  "image/gif",
  "image/jpeg",
  "image/png",
  "image/svg+xml",
  "image/webp",
  "image/x-icon",
];

export {
  deprecated,
  fetchData,
  getColorValues,
  getCurrentTransform,
  getCurrentTransformInverse,
  getFilenameFromUrl,
  getPdfFilenameFromUrl,
Repository: mozilla/pdf.js
File: src/shared/util.js (JavaScript)

Code before:
  static applyTransform(p, m) {
    const [p0, p1] = p;
    p[0] = p0 * m[0] + p1 * m[2] + m[4];
    p[1] = p0 * m[1] + p1 * m[3] + m[5];
  }

  // For 2d affine transforms
  static applyTransformToBezier(p, [m0, m1, m2, m3, m4, m5]) {
    for (let i = 0; i < 6; i += 2) {
      const pI = p[i];
      const pI1 = p[i + 1];
      p[i] = pI * m0 + pI1 * m2 + m4;
      p[i + 1] = pI * m1 + pI1 * m3 + m5;
    }
  }

  static applyInverseTransform(p, m) {
    const [p0, p1] = p;
    const d = m[0] * m[3] - m[1] * m[2];
    p[0] = (p0 * m[3] - p1 * m[2] + m[2] * m[5] - m[4] * m[3]) / d;
    p[1] = (-p0 * m[1] + p1 * m[0] + m[4] * m[1] - m[5] * m[0]) / d;
  }

  // Applies the transform to the rectangle and finds the minimum axially
  // aligned bounding box.
  static getAxialAlignedBoundingBox(rect, transform, output) {
    const m0 = transform[0];
    const m1 = transform[1];
    const m2 = transform[2];
    const m3 = transform[3];
    const m4 = transform[4];
    const m5 = transform[5];
    const r0 = rect[0];
    const r1 = rect[1];
    const r2 = rect[2];
    const r3 = rect[3];

    let a0 = m0 * r0 + m4;
    let a2 = a0;
    let a1 = m0 * r2 + m4;
    let a3 = a1;
    let b0 = m3 * r1 + m5;
    let b2 = b0;
    let b1 = m3 * r3 + m5;
    let b3 = b1;

    if (m1 !== 0 || m2 !== 0) {
      // Non-scaling matrix: shouldn't be frequent.
      const m1r0 = m1 * r0;
      const m1r2 = m1 * r2;
      const m2r1 = m2 * r1;

Review comment (suggestion):
Given that the method no longer returns anything, how about renaming it slightly?
```suggestion
  static axialAlignedBoundingBox(rect, transform, output) {
```

Code after:
  static applyTransform(p, m) {
    const p0 = p[0];
    const p1 = p[1];
    p[0] = p0 * m[0] + p1 * m[2] + m[4];
    p[1] = p0 * m[1] + p1 * m[3] + m[5];
  }

  // For 2d affine transforms
  static applyTransformToBezier(p, transform) {
    const m0 = transform[0];
    const m1 = transform[1];
    const m2 = transform[2];
    const m3 = transform[3];
    const m4 = transform[4];
    const m5 = transform[5];
    for (let i = 0; i < 6; i += 2) {
      const pI = p[i];
      const pI1 = p[i + 1];
      p[i] = pI * m0 + pI1 * m2 + m4;
      p[i + 1] = pI * m1 + pI1 * m3 + m5;
    }
  }

  static applyInverseTransform(p, m) {
    const p0 = p[0];
    const p1 = p[1];
    const d = m[0] * m[3] - m[1] * m[2];
    p[0] = (p0 * m[3] - p1 * m[2] + m[2] * m[5] - m[4] * m[3]) / d;
    p[1] = (-p0 * m[1] + p1 * m[0] + m[4] * m[1] - m[5] * m[0]) / d;
  }

  // Applies the transform to the rectangle and finds the minimum axially
  // aligned bounding box.
  static axialAlignedBoundingBox(rect, transform, output) {
    const m0 = transform[0];
    const m1 = transform[1];
    const m2 = transform[2];
    const m3 = transform[3];
    const m4 = transform[4];
    const m5 = transform[5];
    const r0 = rect[0];
    const r1 = rect[1];
    const r2 = rect[2];
    const r3 = rect[3];

    let a0 = m0 * r0 + m4;
    let a2 = a0;
    let a1 = m0 * r2 + m4;
    let a3 = a1;
    let b0 = m3 * r1 + m5;
    let b2 = b0;
Repository: mozilla/pdf.js
File: src/display/annotation_storage.js (JavaScript)

Code before:
 */
class AnnotationStorage {
  #modified = false;

  #modifiedIds = null;

  #storage = new Map();

  constructor() {
    // Callbacks to signal when the modification state is set or reset.
    // This is used by the viewer to only bind on `beforeunload` if forms
    // are actually edited to prevent doing so unconditionally since that
    // can have undesirable effects.
    this.onSetModified = null;
    this.onResetModified = null;
    this.onAnnotationEditor = null;
  }

  /**
   * Get the value for a given key if it exists, or return the default value.
   * @param {string} key
   * @param {Object} defaultValue
   * @returns {Object}
   */
  getValue(key, defaultValue) {
    const value = this.#storage.get(key);
    if (value === undefined) {
      return defaultValue;
    }

    return Object.assign(defaultValue, value);
  }

  /**
   * Get the value for a given key.
   * @param {string} key
   * @returns {Object}
   */
  getRawValue(key) {
    return this.#storage.get(key);
  }

  /**
   * Remove a value from the storage.
   * @param {string} key
   */
  remove(key) {
    this.#storage.delete(key);

    if (this.#storage.size === 0) {
      this.resetModified();

Review comment (suggestion):
Given that this function doesn't capture any private state from the constructor it doesn't really need to be defined in this file, behind the "if testing" check. Can it be moved to `test/driver.js` (the only place where it's used), as
```js
function setAnnotationsStorage(storage, obj) {
  for (const [key, val] of Object.entries(obj)) {
    storage.setValue(key, val);
  }
}
```

Code after:
 */
class AnnotationStorage {
  #modified = false;

  #modifiedIds = null;

  #storage = new Map();

  constructor() {
    // Callbacks to signal when the modification state is set or reset.
    // This is used by the viewer to only bind on `beforeunload` if forms
    // are actually edited to prevent doing so unconditionally since that
    // can have undesirable effects.
    this.onSetModified = null;
    this.onResetModified = null;
    this.onAnnotationEditor = null;

    if (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) {
      // For testing purposes.
      Object.defineProperty(this, "_setValues", {
        value: obj => {
          for (const [key, val] of Object.entries(obj)) {
            this.setValue(key, val);
          }
        },
      });
    }
  }

  /**
   * Get the value for a given key if it exists, or return the default value.
   * @param {string} key
   * @param {Object} defaultValue
   * @returns {Object}
   */
  getValue(key, defaultValue) {
    const value = this.#storage.get(key);
    if (value === undefined) {
      return defaultValue;
    }

    return Object.assign(defaultValue, value);
  }

  /**
   * Get the value for a given key.
   * @param {string} key
   * @returns {Object}
   */
  getRawValue(key) {
    return this.#storage.get(key);
Repository: mozilla/pdf.js
File: src/display/touch_manager.js (JavaScript)

Code before:

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

    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) {
    if (evt.touches.length >= 2) {
      return;
    }
    // #touchMoveAC shouldn't be null be it seems that irl it can (see #19793).
    this.#touchMoveAC?.abort();
    this.#touchMoveAC = null;
    this.#onPinchEnd?.();

    if (!this.#touchInfo) {
      return;
    }
    stopEvent(evt);
    this.#touchInfo = null;
    this.#isPinching = false;
  }

  destroy() {
    this.#touchManagerAC?.abort();
    this.#touchManagerAC = null;
    this.#pointerDownAC?.abort();
    this.#pointerDownAC = null;
  }
}

export { TouchManager };


Review comment (suggestion):
That error is really odd, since it'd suggest (at least to me) that the "touchend" listener fires *multiple* times!?

If that's what happens, doesn't it also mean that we invoke `#onPinchEnd` multiple times in that case?
Could that be a problem, since it's not clear to me if those callbacks really support being invoked back-to-back?

Should we perhaps do the following, or is that not correct/necessary?
```suggestion
    // #touchMoveAC shouldn't be null but it seems that irl it can (see #19793).
    if (this.#touchMoveAC) {
      this.#touchMoveAC.abort();
      this.#touchMoveAC = null;
      this.#onPinchEnd?.();
    }
```

Code after:

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

    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) {
    if (evt.touches.length >= 2) {
      return;
    }
    // #touchMoveAC shouldn't be null but it seems that irl it can (see #19793).
    if (this.#touchMoveAC) {
      this.#touchMoveAC.abort();
      this.#touchMoveAC = null;
      this.#onPinchEnd?.();
    }

    if (!this.#touchInfo) {
      return;
    }
    stopEvent(evt);
    this.#touchInfo = null;
    this.#isPinching = false;
  }

  destroy() {
    this.#touchManagerAC?.abort();
    this.#touchManagerAC = null;
    this.#pointerDownAC?.abort();
    this.#pointerDownAC = null;
  }
}

export { TouchManager };
Repository: mozilla/pdf.js
File: src/display/touch_manager.js (JavaScript)

Code before:
    ) {
      return;
    }

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

    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) {
    if (evt.touches.length >= 2) {
      return;
    }
    // #touchMoveAC shouldn't be null be it seems that irl it can (see #19793).
    if (this.#touchMoveAC) {
      this.#touchMoveAC.abort();
      this.#touchMoveAC = null;
      this.#onPinchEnd?.();
    }

    if (!this.#touchInfo) {
      return;
    }
    stopEvent(evt);
    this.#touchInfo = null;
    this.#isPinching = false;
  }

  destroy() {
    this.#touchManagerAC?.abort();
    this.#touchManagerAC = null;
    this.#pointerDownAC?.abort();
    this.#pointerDownAC = null;
  }
}

export { TouchManager };


Review comment (suggestion):
Nit:
```suggestion
    // #touchMoveAC shouldn't be null but it seems that irl it can (see #19793).
```

Code after:
    ) {
      return;
    }

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

    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) {
    if (evt.touches.length >= 2) {
      return;
    }
    // #touchMoveAC shouldn't be null but it seems that irl it can (see #19793).
    if (this.#touchMoveAC) {
      this.#touchMoveAC.abort();
      this.#touchMoveAC = null;
      this.#onPinchEnd?.();
    }

    if (!this.#touchInfo) {
      return;
    }
    stopEvent(evt);
    this.#touchInfo = null;
    this.#isPinching = false;
  }

  destroy() {
    this.#touchManagerAC?.abort();
    this.#touchManagerAC = null;
    this.#pointerDownAC?.abort();
    this.#pointerDownAC = null;
  }
}

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

Code before:
/* Copyright 2012 Mozilla Foundation
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

import { RenderingCancelledException } from "pdfjs-lib";
import { RenderingStates } from "./ui_utils.js";

// We draw on the canvas at 60fps (in using `requestAnimationFrame`), so if the
// canvas is large, updating it at 60fps can be a way too much and can cause
// some serious performance issues.
// To avoid that we only update the canvas every 500ms.
const MIN_DURATION_TO_UPDATE_CANVAS = 500; // ms

class BasePDFPageView {
  #enableHWA = false;

  #loadingId = null;

  #renderError = null;

  #renderingState = RenderingStates.INITIAL;

  #showCanvas = null;

  #startTime = 0;

  #tempCanvas = null;

  canvas = null;

  /** @type {null | HTMLDivElement} */
  div = null;

  eventBus = null;

  id = null;


Review comment (suggestion):
We could even increase this (e.g. to 1 sec), so users see that something is being drawn, but we don't spend too much resources to draw too often.

Code after:
/* Copyright 2012 Mozilla Foundation
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

import { RenderingCancelledException } from "pdfjs-lib";
import { RenderingStates } from "./ui_utils.js";

class BasePDFPageView {
  #enableHWA = false;

  #loadingId = null;

  #minDurationToUpdateCanvas = 0;

  #renderError = null;

  #renderingState = RenderingStates.INITIAL;

  #showCanvas = null;

  #startTime = 0;

  #tempCanvas = null;

  canvas = null;

  /** @type {null | HTMLDivElement} */
  div = null;

  eventBus = null;

  id = null;

  pageColors = null;

  renderingQueue = null;