ronantakizawa/codereview-bench huggingface dataset:
Repository: mozilla/pdf.js
File: test/integration/autolinker_spec.mjs (JavaScript)

Code before:
            annotations => annotations.map(a => a.href)
          );
          expect(url.length).withContext(`In ${browserName}`).toEqual(1);
        })
      );
    });
  });

  describe("when a detected links overlaps with a search result", function () {
    let pages;

    beforeAll(async () => {
      pages = await loadAndWait(
        "issue3115r.pdf",
        ".annotationLayer",
        null,
        null,
        { enableAutoLinking: true }
      );
    });

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

    it("shuold work", async () => {
      await Promise.all(
        pages.map(async ([browserName, page]) => {
          await awaitPromise(await waitForLinkAnnotations(page));

          const linkAnnotationsPromise = await waitForLinkAnnotations(page);

          // Search for "rich.edu"
          await page.click("#viewFindButton");
          await page.waitForSelector("#viewFindButton", { hidden: false });
          await page.type("#findInput", "rich.edu");
          await page.waitForSelector(".textLayer .highlight");

          await awaitPromise(linkAnnotationsPromise);

          const urls = await page.$$eval(
            ".page[data-page-number='36'] > .annotationLayer > .linkAnnotation > a",
            annotations => annotations.map(a => a.href)
          );

          expect(urls)
            .withContext(`In ${browserName}`)
            .toContain(jasmine.stringContaining("rich.edu"));
        })
      );
    });

Review comment (suggestion):
Spelling-nit :-)
```suggestion
    it("should work", async () => {
```

Code after:
        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);
        })
      );
    });
  });

  describe("when highlighting search results", function () {
    let pages;

    beforeAll(async () => {
      pages = await loadAndWait(
        "issue3115r.pdf",
        ".annotationLayer",
        null,
        { eventBusSetup: recordInitialLinkAnnotationsEvent },
        { enableAutoLinking: true }
      );
    });
Repository: mozilla/pdf.js
File: src/core/colorspace.js (JavaScript)

Code before:
    localColorSpaceCache
  ) {
    if (!globalColorSpaceCache || !localColorSpaceCache) {
      throw new Error(
        'ColorSpace.getCached - expected "globalColorSpaceCache"/"localColorSpaceCache" argument.'
      );
    }
    if (cacheKey instanceof Ref) {
      const cachedCS =
        globalColorSpaceCache.getByRef(cacheKey) ||
        localColorSpaceCache.getByRef(cacheKey);
      if (cachedCS) {
        return cachedCS;
      }

      try {
        cacheKey = xref.fetch(cacheKey);
      } catch (ex) {
        if (ex instanceof MissingDataException) {
          throw ex;
        }
        // Any errors should be handled during parsing, rather than here.
      }
    }
    if (cacheKey instanceof Name) {
      const cachedCS = localColorSpaceCache.getByName(cacheKey.name);
      if (cachedCS) {
        return cachedCS;
      }
    }
    return null;
  }

  static async parseAsync({
    cs,
    xref,
    resources = null,
    pdfFunctionFactory,
    globalColorSpaceCache,
    localColorSpaceCache,
  }) {
    if (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) {
      assert(
        !this.getCached(cs, xref, globalColorSpaceCache, localColorSpaceCache),
        "Expected `ColorSpace.getCached` to have been manually checked " +
          "before calling `ColorSpace.parseAsync`."
      );
    }
    const parsedCS = this.#parse(cs, xref, resources, pdfFunctionFactory);

    // Attempt to cache the parsed ColorSpace, by name and/or reference.

Review comment (performance):
nit: `return localColorSpaceCache.getByName(cacheKey.name) || null`;

Code after:
    localColorSpaceCache
  ) {
    if (!globalColorSpaceCache || !localColorSpaceCache) {
      throw new Error(
        'ColorSpace.getCached - expected "globalColorSpaceCache"/"localColorSpaceCache" argument.'
      );
    }
    if (cacheKey instanceof Ref) {
      const cachedCS =
        globalColorSpaceCache.getByRef(cacheKey) ||
        localColorSpaceCache.getByRef(cacheKey);
      if (cachedCS) {
        return cachedCS;
      }

      try {
        cacheKey = xref.fetch(cacheKey);
      } catch (ex) {
        if (ex instanceof MissingDataException) {
          throw ex;
        }
        // Any errors should be handled during parsing, rather than here.
      }
    }
    if (cacheKey instanceof Name) {
      return localColorSpaceCache.getByName(cacheKey.name) || null;
    }
    return null;
  }

  static async parseAsync({
    cs,
    xref,
    resources = null,
    pdfFunctionFactory,
    globalColorSpaceCache,
    localColorSpaceCache,
  }) {
    if (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) {
      assert(
        !this.getCached(cs, xref, globalColorSpaceCache, localColorSpaceCache),
        "Expected `ColorSpace.getCached` to have been manually checked " +
          "before calling `ColorSpace.parseAsync`."
      );
    }
    const parsedCS = this.#parse(cs, xref, resources, pdfFunctionFactory);

    // Attempt to cache the parsed ColorSpace, by name and/or reference.
    this.#cache(
      cs,
      xref,
Repository: mozilla/pdf.js
File: src/core/colorspace_utils.js (JavaScript)

Code before:
          warn(`Unimplemented ColorSpace object: ${mode}`);
          return this.gray;
      }
    }
    // Fallback to the default gray color space.
    warn(`Unrecognized ColorSpace object: ${cs}`);
    return this.gray;
  }

  static get gray() {
    return shadow(this, "gray", new DeviceGrayCS());
  }

  static get rgb() {
    return shadow(this, "rgb", new DeviceRgbCS());
  }

  static get rgba() {
    return shadow(this, "rgba", new DeviceRgbaCS());
  }

  static get cmyk() {
    try {
      return shadow(this, "cmyk", new CmykICCBasedCS());
    } catch {
      warn("CMYK fallback: DeviceCmykCS");
    }
    return shadow(this, "cmyk", new DeviceCmykCS());
  }
}

export { ColorSpaceUtils };


Review comment (suggestion):
Nit: Let's match the actual spec-name.
```suggestion
      warn("CMYK fallback: DeviceCMYK");
```

Code after:
          const range = params.getArray("Range");
          return new LabCS(whitePoint, blackPoint, range);
        default:
          // Fallback to the default gray color space.
          warn(`Unimplemented ColorSpace object: ${mode}`);
          return this.gray;
      }
    }
    // Fallback to the default gray color space.
    warn(`Unrecognized ColorSpace object: ${cs}`);
    return this.gray;
  }

  static get gray() {
    return shadow(this, "gray", new DeviceGrayCS());
  }

  static get rgb() {
    return shadow(this, "rgb", new DeviceRgbCS());
  }

  static get rgba() {
    return shadow(this, "rgba", new DeviceRgbaCS());
  }

  static get cmyk() {
    if (IccColorSpace.isUsable) {
      try {
        return shadow(this, "cmyk", new CmykICCBasedCS());
      } catch {
        warn("CMYK fallback: DeviceCMYK");
      }
    }
    return shadow(this, "cmyk", new DeviceCmykCS());
  }
}

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

Code before:
      typeof PDFJSDev !== "undefined" && !PDFJSDev.test("TESTING")
        ? PDFJSDev.eval("BUNDLE_VERSION")
        : null,
    data,
    password,
    disableAutoFetch,
    rangeChunkSize,
    length,
    docBaseUrl,
    enableXfa,
    evaluatorOptions: {
      maxImageSize,
      disableFontFace,
      ignoreErrors,
      isEvalSupported,
      isOffscreenCanvasSupported,
      isImageDecoderSupported,
      canvasMaxAreaInBytes,
      fontExtraProperties,
      useSystemFonts,
      useWasm,
      useWorkerFetch,
      cMapUrl,
      iccUrl,
      standardFontDataUrl,
      wasmUrl,
    },
  };
  const transportParams = {
    ownerDocument,
    pdfBug,
    styleElement,
    loadingParams: {
      disableAutoFetch,
      enableXfa,
    },
  };

  worker.promise
    .then(function () {
      if (task.destroyed) {
        throw new Error("Loading aborted");
      }
      if (worker.destroyed) {
        throw new Error("Worker was destroyed");
      }

      const workerIdPromise = worker.messageHandler.sendWithPromise(
        "GetDocRequest",
        docParams,
        data ? [data.buffer] : null

Review comment (suggestion):
This property should have a default entry in the following structure as well.
https://github.com/mozilla/pdf.js/blob/af89e77124bf9ccf271d27e3d3f619ce5878c7c4/src/core/evaluator.js#L84-L99

Code after:
    docId,
    apiVersion:
      typeof PDFJSDev !== "undefined" && !PDFJSDev.test("TESTING")
        ? PDFJSDev.eval("BUNDLE_VERSION")
        : null,
    data,
    password,
    disableAutoFetch,
    rangeChunkSize,
    length,
    docBaseUrl,
    enableXfa,
    evaluatorOptions: {
      maxImageSize,
      disableFontFace,
      ignoreErrors,
      isEvalSupported,
      isOffscreenCanvasSupported,
      isImageDecoderSupported,
      canvasMaxAreaInBytes,
      fontExtraProperties,
      useSystemFonts,
      useWasm,
      useWorkerFetch,
      cMapUrl,
      iccUrl,
      standardFontDataUrl,
      wasmUrl,
    },
  };
  const transportParams = {
    ownerDocument,
    pdfBug,
    styleElement,
    loadingParams: {
      disableAutoFetch,
      enableXfa,
    },
  };

  worker.promise
    .then(function () {
      if (task.destroyed) {
        throw new Error("Loading aborted");
      }
      if (worker.destroyed) {
        throw new Error("Worker was destroyed");
      }

      const workerIdPromise = worker.messageHandler.sendWithPromise(
        "GetDocRequest",
Repository: mozilla/pdf.js
File: src/core/icc_colorspace.js (JavaScript)

Code before:
    return shadow(this, "isUsable", isUsable);
  }

  static #load() {
    // Parsing and using color spaces is still synchronous,
    // so we must load the wasm module synchronously.
    // TODO: Make the color space stuff asynchronous and use fetch.
    const filename = "qcms_bg.wasm";
    const xhr = new XMLHttpRequest();
    xhr.open("GET", `${this.#wasmUrl}${filename}`, false);
    xhr.responseType = "arraybuffer";
    xhr.send(null);
    return initSync({ module: xhr.response });
  }
}

class CmykICCBasedCS extends IccColorSpace {
  static #iccUrl;

  constructor() {
    const filename = "CGATS001Compat-v2-micro.icc";
    const xhr = new XMLHttpRequest();
    xhr.open("GET", `${CmykICCBasedCS.#iccUrl}${filename}`, false);
    xhr.responseType = "arraybuffer";
    xhr.send(null);
    super(new Uint8Array(xhr.response), 4);
  }

  static setOptions({ iccUrl }) {
    this.#iccUrl = iccUrl;
  }
}

export { CmykICCBasedCS, IccColorSpace };


Review comment (bug):
This class will currently have `this.name === "ICCBased"` set, which seems *potentially strange* and might cause bugs since we in various places (e.g. in the `src/core/image.js` file) use the `name`-field to check what type of ColorSpace something is.

Do we want something like the following instead to ensure that this class will still be treated like CMYK, or is that not appropriate?
```suggestion
    xhr.send(null);
    
    super(new Uint8Array(xhr.response), 4);
    this.name = "DeviceCMYK";
```

Code after:
    }

    return shadow(this, "isUsable", isUsable);
  }

  static #load() {
    // Parsing and using color spaces is still synchronous,
    // so we must load the wasm module synchronously.
    // TODO: Make the color space stuff asynchronous and use fetch.
    const filename = "qcms_bg.wasm";
    const xhr = new XMLHttpRequest();
    xhr.open("GET", `${this.#wasmUrl}${filename}`, false);
    xhr.responseType = "arraybuffer";
    xhr.send(null);
    return initSync({ module: xhr.response });
  }
}

class CmykICCBasedCS extends IccColorSpace {
  static #iccUrl;

  constructor() {
    const filename = "CGATS001Compat-v2-micro.icc";
    const xhr = new XMLHttpRequest();
    xhr.open("GET", `${CmykICCBasedCS.#iccUrl}${filename}`, false);
    xhr.responseType = "arraybuffer";
    xhr.send(null);
    super(new Uint8Array(xhr.response), "DeviceCMYK", 4);
  }

  static setOptions({ iccUrl }) {
    this.#iccUrl = iccUrl;
  }
}

export { CmykICCBasedCS, IccColorSpace };
Repository: mozilla/pdf.js
File: src/core/jpg.js (JavaScript)

Code before:
    this._convertYcckToCmyk(data);
    return this._convertCmykToRgb(data);
  }

  _convertYcckToRgba(data) {
    this._convertYcckToCmyk(data);
    return this._convertCmykToRgba(data);
  }

  _convertYcckToCmyk(data) {
    let Y, Cb, Cr;
    for (let i = 0, length = data.length; i < length; i += 4) {
      Y = data[i];
      Cb = data[i + 1];
      Cr = data[i + 2];
      data[i] = 434.456 - Y - 1.402 * Cr;
      data[i + 1] = 119.541 - Y + 0.344 * Cb + 0.714 * Cr;
      data[i + 2] = 481.816 - Y - 1.772 * Cb;
      // K in data[i + 3] is unchanged
    }
    return data;
  }

  _convertCmykToRgb(data) {
    ColorSpaceUtils.cmyk.getRgbBuffer(data, 0, data.length / 4, data, 0, 8, 0);
    return data.subarray(0, (data.length / 4) * 3);
  }

  _convertCmykToRgba(data) {
    ColorSpaceUtils.cmyk.getRgbBuffer(data, 0, data.length / 4, data, 0, 8, 1);
    return data;
  }

  getData({
    width,
    height,
    forceRGBA = false,
    forceRGB = false,
    isSourcePDF = false,
  }) {
    if (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) {
      assert(
        isSourcePDF === true,
        'JpegImage.getData: Unexpected "isSourcePDF" value for PDF files.'
      );
    }
    if (this.numComponents > 4) {
      throw new JpegError("Unsupported color mode");
    }
    // Type of data: Uint8ClampedArray(width * height * numComponents)
    const data = this._getLinearizedBlockData(width, height, isSourcePDF);

Review comment (suggestion):
Optional nit:
```suggestion
    const count = data.length / 4;
    ColorSpaceUtils.cmyk.getRgbBuffer(data, 0, count, data, 0, 8, 0);
    return data.subarray(0, count * 3);
```

Code after:
    this._convertYcckToCmyk(data);
    return this._convertCmykToRgb(data);
  }

  _convertYcckToRgba(data) {
    this._convertYcckToCmyk(data);
    return this._convertCmykToRgba(data);
  }

  _convertYcckToCmyk(data) {
    let Y, Cb, Cr;
    for (let i = 0, length = data.length; i < length; i += 4) {
      Y = data[i];
      Cb = data[i + 1];
      Cr = data[i + 2];
      data[i] = 434.456 - Y - 1.402 * Cr;
      data[i + 1] = 119.541 - Y + 0.344 * Cb + 0.714 * Cr;
      data[i + 2] = 481.816 - Y - 1.772 * Cb;
      // K in data[i + 3] is unchanged
    }
    return data;
  }

  _convertCmykToRgb(data) {
    const count = data.length / 4;
    ColorSpaceUtils.cmyk.getRgbBuffer(data, 0, count, data, 0, 8, 0);
    return data.subarray(0, count * 3);
  }

  _convertCmykToRgba(data) {
    ColorSpaceUtils.cmyk.getRgbBuffer(data, 0, data.length / 4, data, 0, 8, 1);
    return data;
  }

  getData({
    width,
    height,
    forceRGBA = false,
    forceRGB = false,
    isSourcePDF = false,
  }) {
    if (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) {
      assert(
        isSourcePDF === true,
        'JpegImage.getData: Unexpected "isSourcePDF" value for PDF files.'
      );
    }
    if (this.numComponents > 4) {
      throw new JpegError("Unsupported color mode");
    }
    // Type of data: Uint8ClampedArray(width * height * numComponents)
Repository: mozilla/pdf.js
File: src/core/evaluator.js (JavaScript)

Code before:
    }
    throw reason;
  }

  async setGState({
    resources,
    gState,
    operatorList,
    cacheKey,
    task,
    stateManager,
    localGStateCache,
    localColorSpaceCache,
  }) {
    const gStateRef = gState.objId;
    let isSimpleGState = true;
    // This array holds the converted/processed state data.
    const gStateObj = [];
    let promise = Promise.resolve();
    for (const [key, value] of gState) {
      switch (key) {
        case "Type":
          break;
        case "LW":
          if (typeof value !== "number") {
            warn("Line width is not a number");
            break;
          }
          gStateObj.push([key, Math.abs(value)]);
          break;
        case "LC":
        case "LJ":
        case "ML":
        case "D":
        case "RI":
        case "FL":
        case "CA":
        case "ca":
          gStateObj.push([key, value]);
          break;
        case "Font":
          isSimpleGState = false;

          promise = promise.then(() =>
            this.handleSetFont(
              resources,
              null,
              value[0],
              operatorList,
              task,
              stateManager.state

Review comment (suggestion):
Please update this one a little bit, to have a more consistent warning format.
```suggestion
            warn(`Invalid LW (line width): ${value}`);
```

Code after:
    }
    throw reason;
  }

  async setGState({
    resources,
    gState,
    operatorList,
    cacheKey,
    task,
    stateManager,
    localGStateCache,
    localColorSpaceCache,
  }) {
    const gStateRef = gState.objId;
    let isSimpleGState = true;
    // This array holds the converted/processed state data.
    const gStateObj = [];
    let promise = Promise.resolve();
    for (const [key, value] of gState) {
      switch (key) {
        case "Type":
          break;
        case "LW":
          if (typeof value !== "number") {
            warn(`Invalid LW (line width): ${value}`);
            break;
          }
          gStateObj.push([key, Math.abs(value)]);
          break;
        case "LC":
        case "LJ":
        case "ML":
        case "D":
        case "RI":
        case "FL":
        case "CA":
        case "ca":
          gStateObj.push([key, value]);
          break;
        case "Font":
          isSimpleGState = false;

          promise = promise.then(() =>
            this.handleSetFont(
              resources,
              null,
              value[0],
              operatorList,
              task,
              stateManager.state
Repository: mozilla/pdf.js
File: src/display/canvas.js (JavaScript)

Code before:
    unreachable("Should not call beginInlineImage");
  }

  beginImageData() {
    unreachable("Should not call beginImageData");
  }

  paintFormXObjectBegin(matrix, bbox) {
    if (!this.contentVisible) {
      return;
    }
    this.save();
    this.baseTransformStack.push(this.baseTransform);

    if (matrix) {
      this.transform(...matrix);
    }
    this.baseTransform = getCurrentTransform(this.ctx);

    if (bbox) {
      const [x0, y0, x1, y1] = bbox;
      const width = x1 - x0;
      const height = y1 - y0;
      this.current.updateRectMinMax(getCurrentTransform(this.ctx), bbox);
      const clip = new Path2D();
      clip.rect(x0, y0, width, height);
      this.ctx.clip(clip);
      this.endPath();
    }
  }

  paintFormXObjectEnd() {
    if (!this.contentVisible) {
      return;
    }
    this.restore();
    this.baseTransform = this.baseTransformStack.pop();
  }

  beginGroup(group) {
    if (!this.contentVisible) {
      return;
    }

    this.save();
    // If there's an active soft mask we don't want it enabled for the group, so
    // clear it out. The mask and suspended canvas will be restored in endGroup.
    if (this.inSMaskMode) {
      this.endSMaskMode();
      this.current.activeSMask = null;
    }

Review comment (suggestion):
Optional nit: We could perhaps just skip the width/height variables above?
```suggestion
      clip.rect(x0, y0, x1 - x0, y1 - y0);
```

Code after:
  paintFormXObjectBegin(matrix, bbox) {
    if (!this.contentVisible) {
      return;
    }
    this.save();
    this.baseTransformStack.push(this.baseTransform);

    if (matrix) {
      this.transform(...matrix);
    }
    this.baseTransform = getCurrentTransform(this.ctx);

    if (bbox) {
      this.current.updateRectMinMax(getCurrentTransform(this.ctx), bbox);
      const [x0, y0, x1, y1] = bbox;
      const clip = new Path2D();
      clip.rect(x0, y0, x1 - x0, y1 - y0);
      this.ctx.clip(clip);
      this.endPath();
    }
  }

  paintFormXObjectEnd() {
    if (!this.contentVisible) {
      return;
    }
    this.restore();
    this.baseTransform = this.baseTransformStack.pop();
  }

  beginGroup(group) {
    if (!this.contentVisible) {
      return;
    }

    this.save();
    // If there's an active soft mask we don't want it enabled for the group, so
    // clear it out. The mask and suspended canvas will be restored in endGroup.
    if (this.inSMaskMode) {
      this.endSMaskMode();
      this.current.activeSMask = null;
    }

    const currentCtx = this.ctx;
    // TODO non-isolated groups - according to Rik at adobe non-isolated
    // group results aren't usually that different and they even have tools
    // that ignore this setting. Notes from Rik on implementing:
    // - When you encounter an transparency group, create a new canvas with
    // the dimensions of the bbox
    // - copy the content from the previous canvas to the new canvas
    // - draw as usual