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

Code before:

  get _highlightColors() {
    return shadow(
      this,
      "_highlightColors",
      this.#highlightColors
        ? new Map(
            this.#highlightColors.split(",").map(pair => {
              pair = pair.split("=").map(x => x.trim());
              pair[1] = pair[1].toUpperCase();
              return pair;
            })
          )
        : null
    );
  }

  get highlightColors() {
    const { _highlightColors } = this;
    if (!_highlightColors) {
      return shadow(this, "highlightColors", null);
    }
    const map = new Map();
    const hasHCM = !!this.#pageColors;
    for (const [name, color] of _highlightColors) {
      if (hasHCM === name.endsWith("_HCM")) {
        // We only want to use the colors that match the current HCM state.
        map.set(name.replace("_HCM", ""), color);
      }
    }
    return shadow(this, "highlightColors", map);
  }

  get highlightColorNames() {
    return shadow(
      this,
      "highlightColorNames",
      this.highlightColors
        ? new Map(Array.from(this.highlightColors, e => e.reverse()))
        : null
    );
  }

  getNonHCMColor(color) {
    if (!this._highlightColors) {
      return color;
    }
    const colorName = this.highlightColorNames.get(color);
    return this._highlightColors.get(colorName) || color;
  }


Review comment (suggestion):
It seems easier to me to write this as `if (hasHCM && name.endsWith("_HCM")) {` to make it more clear that we check two conditions here.

Code after:
      this,
      "direction",
      getComputedStyle(this.#container).direction
    );
  }

  get _highlightColors() {
    return shadow(
      this,
      "_highlightColors",
      this.#highlightColors
        ? new Map(
            this.#highlightColors.split(",").map(pair => {
              pair = pair.split("=").map(x => x.trim());
              pair[1] = pair[1].toUpperCase();
              return pair;
            })
          )
        : null
    );
  }

  get highlightColors() {
    const { _highlightColors } = this;
    if (!_highlightColors) {
      return shadow(this, "highlightColors", null);
    }
    const map = new Map();
    const hasHCM = !!this.#pageColors;
    for (const [name, color] of _highlightColors) {
      const isNameForHCM = name.endsWith("_HCM");
      if (hasHCM && isNameForHCM) {
        map.set(name.replace("_HCM", ""), color);
        continue;
      }
      if (!hasHCM && !isNameForHCM) {
        map.set(name, color);
      }
    }
    return shadow(this, "highlightColors", map);
  }

  get highlightColorNames() {
    return shadow(
      this,
      "highlightColorNames",
      this.highlightColors
        ? new Map(Array.from(this.highlightColors, e => e.reverse()))
        : null
    );
  }
Repository: mozilla/pdf.js
File: src/display/editor/tools.js (JavaScript)

Code before:
            this.#highlightColors.split(",").map(pair => {
              pair = pair.split("=").map(x => x.trim());
              pair[1] = pair[1].toUpperCase();
              return pair;
            })
          )
        : null
    );
  }

  get highlightColors() {
    const { _highlightColors } = this;
    if (!_highlightColors) {
      return shadow(this, "highlightColors", null);
    }
    const map = new Map();
    const hasHCM = !!this.#pageColors;
    for (const [name, color] of _highlightColors) {
      if (hasHCM === name.endsWith("_HCM")) {
        // We only want to use the colors that match the current HCM state.
        map.set(name.replace("_HCM", ""), color);
      }
    }
    return shadow(this, "highlightColors", map);
  }

  get highlightColorNames() {
    return shadow(
      this,
      "highlightColorNames",
      this.highlightColors
        ? new Map(Array.from(this.highlightColors, e => e.reverse()))
        : null
    );
  }

  getNonHCMColor(color) {
    if (!this._highlightColors) {
      return color;
    }
    const colorName = this.highlightColorNames.get(color);
    return this._highlightColors.get(colorName) || color;
  }

  getNonHCMColorName(color) {
    return this.highlightColorNames.get(color) || color;
  }

  /**
   * Set the current drawing session.
   * @param {AnnotationEditorLayer} layer

Review comment (suggestion):
I'm probably missing something here, but the map only seems to be filled for HCM because there is no other place where the map is being edited, but it's being returned below so won't `highlightColors` always contain HCM-only colors or nothing if HCM is disabled? I'm mainly confused how this works for non-HCM now.

Code after:
    return shadow(
      this,
      "_highlightColors",
      this.#highlightColors
        ? new Map(
            this.#highlightColors.split(",").map(pair => {
              pair = pair.split("=").map(x => x.trim());
              pair[1] = pair[1].toUpperCase();
              return pair;
            })
          )
        : null
    );
  }

  get highlightColors() {
    const { _highlightColors } = this;
    if (!_highlightColors) {
      return shadow(this, "highlightColors", null);
    }
    const map = new Map();
    const hasHCM = !!this.#pageColors;
    for (const [name, color] of _highlightColors) {
      const isNameForHCM = name.endsWith("_HCM");
      if (hasHCM && isNameForHCM) {
        map.set(name.replace("_HCM", ""), color);
        continue;
      }
      if (!hasHCM && !isNameForHCM) {
        map.set(name, color);
      }
    }
    return shadow(this, "highlightColors", map);
  }

  get highlightColorNames() {
    return shadow(
      this,
      "highlightColorNames",
      this.highlightColors
        ? new Map(Array.from(this.highlightColors, e => e.reverse()))
        : null
    );
  }

  getNonHCMColor(color) {
    if (!this._highlightColors) {
      return color;
    }
    const colorName = this.highlightColorNames.get(color);
    return this._highlightColors.get(colorName) || color;
Repository: mozilla/pdf.js
File: web/app.js (JavaScript)

Code before:
          return;
        }
        stopEvent(evt);
        eventBus.dispatch("fileinputchange", {
          source: this,
          fileInput: evt.dataTransfer,
        });
      });
    }

    if (!AppOptions.get("supportsDocumentFonts")) {
      AppOptions.set("disableFontFace", true);
      this.l10n.get("pdfjs-web-fonts-disabled").then(msg => {
        console.warn(msg);
      });
    }

    const hidePrintingButtons = visible => {
      appConfig.toolbar?.print?.classList.toggle("hidden", !visible);
      appConfig.secondaryToolbar?.printButton.classList.toggle(
        "hidden",
        !visible
      );
    };
    if (!this.supportsPrinting) {
      hidePrintingButtons();
    } else {
      eventBus.on("printingallowed", ({ isAllowed }) =>
        hidePrintingButtons(isAllowed)
      );
    }

    if (!this.supportsFullscreen) {
      appConfig.secondaryToolbar?.presentationModeButton.classList.add(
        "hidden"
      );
    }

    if (this.supportsIntegratedFind) {
      appConfig.findBar?.toggleButton?.classList.add("hidden");
    }

    if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) {
      if (file) {
        this.open({ url: file });
      } else {
        this._hideViewBookmark();
      }
    } else if (PDFJSDev.test("MOZCENTRAL || CHROME")) {
      this.setTitleUsingUrl(file, /* downloadUrl = */ file);


Review comment (suggestion):
Let's provide the parameter value as a boolean explicitly here, which together with the rename should make this code easier to understand: `togglePrintingButtons(false)`

Code after:
          return;
        }
        stopEvent(evt);
        eventBus.dispatch("fileinputchange", {
          source: this,
          fileInput: evt.dataTransfer,
        });
      });
    }

    if (!AppOptions.get("supportsDocumentFonts")) {
      AppOptions.set("disableFontFace", true);
      this.l10n.get("pdfjs-web-fonts-disabled").then(msg => {
        console.warn(msg);
      });
    }

    const togglePrintingButtons = visible => {
      appConfig.toolbar?.print?.classList.toggle("hidden", !visible);
      appConfig.secondaryToolbar?.printButton.classList.toggle(
        "hidden",
        !visible
      );
    };
    if (!this.supportsPrinting) {
      togglePrintingButtons(false);
    } else {
      eventBus.on("printingallowed", ({ isAllowed }) =>
        togglePrintingButtons(isAllowed)
      );
    }

    if (!this.supportsFullscreen) {
      appConfig.secondaryToolbar?.presentationModeButton.classList.add(
        "hidden"
      );
    }

    if (this.supportsIntegratedFind) {
      appConfig.findBar?.toggleButton?.classList.add("hidden");
    }

    if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) {
      if (file) {
        this.open({ url: file });
      } else {
        this._hideViewBookmark();
      }
    } else if (PDFJSDev.test("MOZCENTRAL || CHROME")) {
      this.setTitleUsingUrl(file, /* downloadUrl = */ file);
Repository: mozilla/pdf.js
File: test/unit/to_unicode_map_spec.js (JavaScript)

Code before:
/* Copyright 2022 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 { ToUnicodeMap } from "../../src/core/to_unicode_map.js";

describe("ToUnicodeMap", () => {
  it("should correctly map Extension B characters using codePointAt", () => {
    const cmap = { 0x20: "\uD840\uDC00" }; // Example Extension B character
    const toUnicodeMap = new ToUnicodeMap(cmap);

    const expected = 0x20000; // Unicode code point for the character
    let actual;
    toUnicodeMap.forEach((charCode, unicode) => {
      if (charCode === "32") {
        // 0x20 in decimal
        actual = unicode;
      }
    });

    expect(actual).toBe(expected);
  });
});


Review comment (suggestion):
We could also use `0x20.toString()` here to make it a little bit clearer where the value comes from and that it matches the key in the `cmap` variable. That'd also allow for removing the comment below.

Code after:
/* Copyright 2025 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 { ToUnicodeMap } from "../../src/core/to_unicode_map.js";

describe("ToUnicodeMap", () => {
  it("should correctly map Extension B characters using codePointAt", () => {
    const cmap = { 0x20: "\uD840\uDC00" }; // Example Extension B character
    const toUnicodeMap = new ToUnicodeMap(cmap);

    const expected = 0x20000; // Unicode code point for the character
    let actual;
    toUnicodeMap.forEach((charCode, unicode) => {
      if (charCode === (0x20).toString()) {
        actual = unicode;
      }
    });

    expect(actual).toBe(expected);
  });
});
Repository: mozilla/pdf.js
File: src/core/primitives.js (JavaScript)

Code before:
      }
      if (subDict.size > 0) {
        mergedDict._map.set(name, subDict);
      }
    }
    properties.clear();

    return mergedDict.size > 0 ? mergedDict : Dict.empty;
  }

  clone() {
    const dict = new Dict(this.xref);
    for (const key of this.getKeys()) {
      dict.set(key, this.getRaw(key));
    }
    return dict;
  }

  delete(key) {
    delete this._map[key];
  }
}

class Ref {
  constructor(num, gen) {
    this.num = parseInt(num);
    this.gen = gen;
  }

  toString() {
    // This function is hot, so we make the string as compact as possible.
    // |this.gen| is almost always zero, so we treat that case specially.
    if (this.gen === 0) {
      return `${this.num}R`;
    }
    return `${this.num}R${this.gen}`;
  }

  static fromString(str) {
    const ref = RefCache[str];
    if (ref) {
      return ref;
    }
    const m = /^(\d+)R(\d*)$/.exec(str);
    if (!m || m[1] === "0") {
      return null;
    }

    // eslint-disable-next-line no-restricted-syntax
    return (RefCache[str] = new Ref(
      parseInt(m[1]),

Review comment (performance):
If we do it here we should have a unit test in https://github.com/mozilla/pdf.js/blob/master/test/unit/primitives_spec.js#L429 to assert that the integer conversion happens if a string is provided.

Moreover, do you think this might have a performance impact? This seems like an object we create quite often, and in the general case this will already be an integer so the function call is obsolete. It seems like in the `src/core/xref.js` file where we used to have this it's in a fallback path, so we have to make sure that this change doesn't create a performance penalty for the general case.

Code after:
      }
      if (subDict.size > 0) {
        mergedDict._map.set(name, subDict);
      }
    }
    properties.clear();

    return mergedDict.size > 0 ? mergedDict : Dict.empty;
  }

  clone() {
    const dict = new Dict(this.xref);
    for (const key of this.getKeys()) {
      dict.set(key, this.getRaw(key));
    }
    return dict;
  }

  delete(key) {
    delete this._map[key];
  }
}

class Ref {
  constructor(num, gen) {
    this.num = num;
    this.gen = gen;
  }

  toString() {
    // This function is hot, so we make the string as compact as possible.
    // |this.gen| is almost always zero, so we treat that case specially.
    if (this.gen === 0) {
      return `${this.num}R`;
    }
    return `${this.num}R${this.gen}`;
  }

  static fromString(str) {
    const ref = RefCache[str];
    if (ref) {
      return ref;
    }
    const m = /^(\d+)R(\d*)$/.exec(str);
    if (!m || m[1] === "0") {
      return null;
    }

    // eslint-disable-next-line no-restricted-syntax
    return (RefCache[str] = new Ref(
      parseInt(m[1]),
Repository: mozilla/pdf.js
File: src/display/annotation_layer.js (JavaScript)

Code before:
        }
      }
      return [maxX, maxY];
    }
    if (rect) {
      return [rect[2], rect[3]];
    }
    return null;
  }

  get commentButtonColor() {
    if (!this.data.color) {
      return null;
    }
    const [r, g, b] = this.data.color;
    const opacity = this.data.opacity ?? 1;
    const oppositeOpacity = 255 * (1 - opacity);

    return this.changeLightness(
      Math.min(r + oppositeOpacity, 255),
      Math.min(g + oppositeOpacity, 255),
      Math.min(b + oppositeOpacity, 255)
    );
  }

  changeLightness(r, g, b) {
    r /= 255;
    g /= 255;
    b /= 255;

    const max = Math.max(r, g, b);
    const min = Math.min(r, g, b);
    const l = (max + min) / 2;
    const newL = (((1 + Math.sqrt(l)) / 2) * 100).toFixed(2);

    if (max === min) {
      // gray
      return `hsl(0, 0%, ${newL}%)`;
    }

    const d = max - min;

    // hue (branch on max only; avoids mod)
    let h;
    if (max === r) {
      h = (g - b) / d + (g < b ? 6 : 0);
    } else if (max === g) {
      h = (b - r) / d + 2;
    } else {
      // max === b
      h = (r - g) / d + 4;

Review comment (suggestion):
Let's make this private because it's not use outside of the class: `#changeLightness`.

Code after:
        }
      }
      return [maxX, maxY];
    }
    if (rect) {
      return [rect[2], rect[3]];
    }
    return null;
  }

  get commentButtonColor() {
    if (!this.data.color) {
      return null;
    }
    const [r, g, b] = this.data.color;
    const opacity = this.data.opacity ?? 1;
    const oppositeOpacity = 255 * (1 - opacity);

    return this.#changeLightness(
      Math.min(r + oppositeOpacity, 255),
      Math.min(g + oppositeOpacity, 255),
      Math.min(b + oppositeOpacity, 255)
    );
  }

  #changeLightness(r, g, b) {
    r /= 255;
    g /= 255;
    b /= 255;

    const max = Math.max(r, g, b);
    const min = Math.min(r, g, b);
    const l = (max + min) / 2;
    const newL = (((1 + Math.sqrt(l)) / 2) * 100).toFixed(2);

    if (max === min) {
      // gray
      return `hsl(0, 0%, ${newL}%)`;
    }

    const d = max - min;

    // hue (branch on max only; avoids mod)
    let h;
    if (max === r) {
      h = (g - b) / d + (g < b ? 6 : 0);
    } else if (max === g) {
      h = (b - r) / d + 2;
    } else {
      // max === b
      h = (r - g) / d + 4;
Repository: mozilla/pdf.js
File: src/display/display_utils.js (JavaScript)

Code before:
          ? window.innerWidth * window.innerHeight
          : window.screen.availWidth * window.screen.availHeight) *
          this.pixelRatio ** 2 *
          (1 + capAreaFactor / 100)
      );
      return maxPixels > 0 ? Math.min(maxPixels, winPixels) : winPixels;
    }
    return maxPixels;
  }
}

// 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",
];

function changeLightness(r, g, b, lumCallback) {
  r /= 255;
  g /= 255;
  b /= 255;

  const max = Math.max(r, g, b);
  const min = Math.min(r, g, b);
  const l = (max + min) / 2;
  const newL = (lumCallback(l) * 100).toFixed(2);

  if (max === min) {
    // gray
    return `hsl(0, 0%, ${newL}%)`;
  }

  const d = max - min;

  // hue (branch on max only; avoids mod)
  let h;
  if (max === r) {
    h = (g - b) / d + (g < b ? 6 : 0);
  } else if (max === g) {
    h = (b - r) / d + 2;
  } else {
    // max === b
    h = (r - g) / d + 4;

Review comment (refactor):
The `lumCallback` value given in the callsites seems to be equal everywhere, so can we perhaps make that value a default argument to simplify the callers?

Code after:
          ? window.innerWidth * window.innerHeight
          : window.screen.availWidth * window.screen.availHeight) *
          this.pixelRatio ** 2 *
          (1 + capAreaFactor / 100)
      );
      return maxPixels > 0 ? Math.min(maxPixels, winPixels) : winPixels;
    }
    return maxPixels;
  }
}

// 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",
];

function changeLightness(r, g, b, lumCallback = l => (1 + Math.sqrt(l)) / 2) {
  r /= 255;
  g /= 255;
  b /= 255;

  const max = Math.max(r, g, b);
  const min = Math.min(r, g, b);
  const l = (max + min) / 2;
  const newL = (lumCallback(l) * 100).toFixed(2);

  if (max === min) {
    // gray
    return `hsl(0, 0%, ${newL}%)`;
  }

  const d = max - min;

  // hue (branch on max only; avoids mod)
  let h;
  if (max === r) {
    h = (g - b) / d + (g < b ? 6 : 0);
  } else if (max === g) {
    h = (b - r) / d + 2;
  } else {
    // max === b
    h = (r - g) / d + 4;
Repository: mozilla/pdf.js
File: src/display/annotation_layer.js (JavaScript)

Code before:
        }
      }
      return [maxX, maxY];
    }
    if (rect) {
      return [rect[2], rect[3]];
    }
    return null;
  }

  get commentButtonColor() {
    if (!this.data.color) {
      return null;
    }
    const [r, g, b] = this.data.color;
    const opacity = this.data.opacity ?? 1;
    const oppositeOpacity = 255 * (1 - opacity);

    return this.#changeLightness(
      Math.min(r + oppositeOpacity, 255),
      Math.min(g + oppositeOpacity, 255),
      Math.min(b + oppositeOpacity, 255)
    );
  }

  #changeLightness(r, g, b) {
    return changeLightness(r, g, b, l => (1 + Math.sqrt(l)) / 2);
  }

  _normalizePoint(point) {
    const {
      page: { view },
      viewport: {
        rawDims: { pageWidth, pageHeight, pageX, pageY },
      },
    } = this.parent;
    point[1] = view[3] - point[1] + view[1];
    point[0] = (100 * (point[0] - pageX)) / pageWidth;
    point[1] = (100 * (point[1] - pageY)) / pageHeight;
    return point;
  }

  updateEdited(params) {
    if (!this.container) {
      return;
    }

    if (params.rect) {
      this.#updates ||= {
        rect: this.data.rect.slice(0),
      };

Review comment (suggestion):
This private method can now be removed because it's only called in one place in this file, so let's just inline the call to `changeLightness` there to reduce indirection.

Code after:
        }
      }
      return [maxX, maxY];
    }
    if (rect) {
      return [rect[2], rect[3]];
    }
    return null;
  }

  get commentButtonColor() {
    if (!this.data.color) {
      return null;
    }
    const [r, g, b] = this.data.color;
    const opacity = this.data.opacity ?? 1;
    const oppositeOpacity = 255 * (1 - opacity);

    return changeLightness(
      Math.min(r + oppositeOpacity, 255),
      Math.min(g + oppositeOpacity, 255),
      Math.min(b + oppositeOpacity, 255)
    );
  }

  _normalizePoint(point) {
    const {
      page: { view },
      viewport: {
        rawDims: { pageWidth, pageHeight, pageX, pageY },
      },
    } = this.parent;
    point[1] = view[3] - point[1] + view[1];
    point[0] = (100 * (point[0] - pageX)) / pageWidth;
    point[1] = (100 * (point[1] - pageY)) / pageHeight;
    return point;
  }

  updateEdited(params) {
    if (!this.container) {
      return;
    }

    if (params.rect) {
      this.#updates ||= {
        rect: this.data.rect.slice(0),
      };
    }

    const { rect, popup: newPopup } = params;