ronantakizawa/codereview-bench huggingface dataset:
Repository: electron/electron
File: shell/browser/native_window_views_win.cc (C++)

Code before:
// Must be included after other Windows headers.
#include <UIAutomationClient.h>
#include <UIAutomationCoreApi.h>

namespace electron {

namespace {

void SetWindowBorderAndCaptionColor(HWND hwnd, COLORREF color) {
  if (base::win::GetVersion() < base::win::Version::WIN11)
    return;

  HRESULT result =
      DwmSetWindowAttribute(hwnd, DWMWA_CAPTION_COLOR, &color, sizeof(color));

  if (FAILED(result))
    LOG(WARNING) << "Failed to set caption color";

  result =
      DwmSetWindowAttribute(hwnd, DWMWA_BORDER_COLOR, &color, sizeof(color));

  if (FAILED(result))
    LOG(WARNING) << "Failed to set border color";
}

DWORD GetAccentColor() {
  DWORD accent_color = 0;
  DWORD color_size = sizeof(accent_color);

  HKEY key;
  if (RegOpenKeyEx(HKEY_CURRENT_USER, L"SOFTWARE\\Microsoft\\Windows\\DWM", 0,
                   KEY_READ, &key) == ERROR_SUCCESS) {
    RegQueryValueEx(key, L"AccentColor", NULL, NULL, (LPBYTE)&accent_color,
                    &color_size);
    RegCloseKey(key);
  }

  return accent_color;
}

bool IsAccentColorOnTitleBarsEnabled() {
  DWORD enabled = 0;
  DWORD size = sizeof(enabled);

  HKEY key;
  if (RegOpenKeyEx(HKEY_CURRENT_USER, L"SOFTWARE\\Microsoft\\Windows\\DWM", 0,
                   KEY_READ, &key) == ERROR_SUCCESS) {
    RegQueryValueEx(key, L"ColorPrevalence", NULL, NULL, (LPBYTE)&enabled,
                    &size);
    RegCloseKey(key);
  }

Review comment (bug):
```suggestion
std::optional<DWORD> GetAccentColor() {
```

If the `RegOpenKeyEx` call fails, currently this would return `0` which would then most likely result in a border color of black? So seems best to guard against that case and ensure the returned value from `GetAccentColor` is valid before applying it.

Code after:

// Must be included after other Windows headers.
#include <UIAutomationClient.h>
#include <UIAutomationCoreApi.h>

namespace electron {

namespace {

void SetWindowBorderAndCaptionColor(HWND hwnd, COLORREF color) {
  if (base::win::GetVersion() < base::win::Version::WIN11)
    return;

  HRESULT result =
      DwmSetWindowAttribute(hwnd, DWMWA_CAPTION_COLOR, &color, sizeof(color));

  if (FAILED(result))
    LOG(WARNING) << "Failed to set caption color";

  result =
      DwmSetWindowAttribute(hwnd, DWMWA_BORDER_COLOR, &color, sizeof(color));

  if (FAILED(result))
    LOG(WARNING) << "Failed to set border color";
}

std::optional<DWORD> GetAccentColor() {
  base::win::RegKey key;
  if (key.Open(HKEY_CURRENT_USER, L"SOFTWARE\\Microsoft\\Windows\\DWM",
               KEY_READ) != ERROR_SUCCESS) {
    return std::nullopt;
  }

  DWORD accent_color = 0;
  if (key.ReadValueDW(L"AccentColor", &accent_color) != ERROR_SUCCESS) {
    return std::nullopt;
  }

  return accent_color;
}

bool IsAccentColorOnTitleBarsEnabled() {
  base::win::RegKey key;
  if (key.Open(HKEY_CURRENT_USER, L"SOFTWARE\\Microsoft\\Windows\\DWM",
               KEY_READ) != ERROR_SUCCESS) {
    return false;
  }

  DWORD enabled = 0;
  if (key.ReadValueDW(L"ColorPrevalence", &enabled) != ERROR_SUCCESS) {
    return false;
Repository: electron/electron
File: shell/browser/api/electron_api_tray.h (C/C++)

Code before:
// Copyright (c) 2014 GitHub, Inc.
// Use of this source code is governed by the MIT license that can be
// found in the LICENSE file.

#ifndef ELECTRON_SHELL_BROWSER_API_ELECTRON_API_TRAY_H_
#define ELECTRON_SHELL_BROWSER_API_ELECTRON_API_TRAY_H_

#include <memory>
#include <optional>
#include <string>
#include <vector>

#include "gin/wrappable.h"
#include "shell/browser/event_emitter_mixin.h"
#include "shell/browser/ui/tray_icon.h"
#include "shell/browser/ui/tray_icon_observer.h"
#include "shell/common/gin_converters/guid_converter.h"
#include "shell/common/gin_helper/cleaned_up_at_exit.h"
#include "shell/common/gin_helper/constructible.h"
#include "shell/common/gin_helper/pinnable.h"

namespace base {
class Uuid;
}  // namespace base

namespace gfx {
class Image;
class Image;
}  // namespace gfx

namespace gin {
template <typename T>
class Handle;
}  // namespace gin

namespace gin_helper {
class Dictionary;
class ErrorThrower;
}  // namespace gin_helper

namespace electron::api {

class Menu;

class Tray final : public gin::DeprecatedWrappable<Tray>,
                   public gin_helper::EventEmitterMixin<Tray>,

Review comment (suggestion):
I don't think this forward declare is in effect here, `std::optional` needs a complete type I'm fairly sure.

Code after:
// Copyright (c) 2014 GitHub, Inc.
// Use of this source code is governed by the MIT license that can be
// found in the LICENSE file.

#ifndef ELECTRON_SHELL_BROWSER_API_ELECTRON_API_TRAY_H_
#define ELECTRON_SHELL_BROWSER_API_ELECTRON_API_TRAY_H_

#include <memory>
#include <optional>
#include <string>
#include <vector>

#include "shell/browser/event_emitter_mixin.h"
#include "shell/browser/ui/tray_icon.h"
#include "shell/browser/ui/tray_icon_observer.h"
#include "shell/common/gin_converters/guid_converter.h"
#include "shell/common/gin_helper/cleaned_up_at_exit.h"
#include "shell/common/gin_helper/constructible.h"
#include "shell/common/gin_helper/pinnable.h"
#include "shell/common/gin_helper/wrappable.h"

namespace gfx {
class Image;
class Image;
}  // namespace gfx

namespace gin_helper {
class Dictionary;
class ErrorThrower;
template <typename T>
class Handle;
}  // namespace gin_helper

namespace electron::api {

class Menu;

class Tray final : public gin_helper::DeprecatedWrappable<Tray>,
                   public gin_helper::EventEmitterMixin<Tray>,
                   public gin_helper::Constructible<Tray>,
                   public gin_helper::CleanedUpAtExit,
                   public gin_helper::Pinnable<Tray>,
                   private TrayIconObserver {
 public:
  // gin_helper::Constructible
  static gin_helper::Handle<Tray> New(gin_helper::ErrorThrower thrower,
Repository: electron/electron
File: shell/common/gin_helper/event.h (C/C++)

Code before:
// Copyright (c) 2023 Salesforce, Inc.
// Use of this source code is governed by the MIT license that can be
// found in the LICENSE file.

#ifndef ELECTRON_SHELL_COMMON_GIN_HELPER_EVENT_H_
#define ELECTRON_SHELL_COMMON_GIN_HELPER_EVENT_H_

#include "shell/common/gin_helper/constructible.h"
#include "shell/common/gin_helper/wrappable.h"

namespace gin_helper {
template <typename T>
class Handle;
}  // namespace gin_helper

namespace v8 {
class Isolate;
template <typename T>
class Local;
class Object;
class ObjectTemplate;
}  // namespace v8

namespace gin_helper::internal {

class Event final : public gin_helper::DeprecatedWrappable<Event>,
                    public gin_helper::Constructible<Event> {
 public:
  // gin_helper::Constructible
  static gin_helper::Handle<Event> New(v8::Isolate* isolate);
  static v8::Local<v8::ObjectTemplate> FillObjectTemplate(
      v8::Isolate* isolate,
      v8::Local<v8::ObjectTemplate> prototype);
  static const char* GetClassName() { return "Event"; }

  // gin_helper::Wrappable
  static gin::DeprecatedWrapperInfo kWrapperInfo;
  const char* GetTypeName() override;

  ~Event() override;

  void PreventDefault() { default_prevented_ = true; }

Review comment (refactor):
Not a blocker, but we have a lot of instances of this boilerplate code:

```c++
    internal::Event* event = internal::Event::New(isolate);
    v8::Local<v8::Object> event_object =
        event->GetWrapper(isolate).ToLocalChecked();
```

Since the caller no longer has to hold onto a `gin_helper::Handle<>`, maybe we could also add a helper `v8::Local<v8::Object> NewChecked(v8::Isolate* isolate);` to wrap these two steps together?

Code after:
// Copyright (c) 2023 Salesforce, Inc.
// Use of this source code is governed by the MIT license that can be
// found in the LICENSE file.

#ifndef ELECTRON_SHELL_COMMON_GIN_HELPER_EVENT_H_
#define ELECTRON_SHELL_COMMON_GIN_HELPER_EVENT_H_

#include "gin/wrappable.h"
#include "shell/common/gin_helper/constructible.h"

namespace gin_helper::internal {

class Event final : public gin::Wrappable<Event>,
                    public gin_helper::Constructible<Event> {
 public:
  // gin_helper::Constructible
  static Event* New(v8::Isolate* isolate);
  static v8::Local<v8::ObjectTemplate> FillObjectTemplate(
      v8::Isolate* isolate,
      v8::Local<v8::ObjectTemplate> prototype);
  static const char* GetClassName() { return "Event"; }

  // gin::Wrappable
  static gin::WrapperInfo kWrapperInfo;
  const gin::WrapperInfo* wrapper_info() const override;
  const char* GetHumanReadableName() const override;

  Event();
  ~Event() override;

  void PreventDefault() { default_prevented_ = true; }

  bool GetDefaultPrevented() { return default_prevented_; }

 private:
  bool default_prevented_ = false;
};

}  // namespace gin_helper::internal

#endif  // ELECTRON_SHELL_COMMON_GIN_HELPER_EVENT_H_
Repository: electron/electron
File: shell/browser/notifications/win/notification_presenter_win.cc (C++)

Code before:
bool NotificationPresenterWin::Init() {
  ScopedAllowBlockingForElectron allow_blocking;
  return temp_dir_.CreateUniqueTempDir();
}

std::wstring NotificationPresenterWin::SaveIconToFilesystem(
    const SkBitmap& icon,
    const GURL& origin) {
  if (icon.drawsNothing() || icon.width() <= 0 || icon.height() <= 0)
    return L"";

  std::string filename;
  if (origin.is_valid()) {
    filename = base::MD5String(origin.spec()) + ".png";
  } else {
    const int64_t now_usec = base::Time::Now().since_origin().InMicroseconds();
    filename = base::NumberToString(now_usec) + ".png";
  }

  ScopedAllowBlockingForElectron allow_blocking;
  base::FilePath path = temp_dir_.GetPath().Append(base::UTF8ToWide(filename));

  if (!base::PathExists(path)) {
    if (!SaveIconToPath(icon, path))
      return L"";
  }

  std::optional<int64_t> size = base::GetFileSize(path);
  if (!size.has_value())
    return L"";

  return path.value();
}

Notification* NotificationPresenterWin::CreateNotificationObject(
    NotificationDelegate* delegate) {
  return new WindowsToastNotification(delegate, this);
}

}  // namespace electron


Review comment (suggestion):
Can we move `base::PathExists` inside `SaveIconToPath` which would become an implementation detail. We can then just check for `!SaveIconToPath(icon, path)`

Code after:
  ScopedAllowBlockingForElectron allow_blocking;
  return temp_dir_.CreateUniqueTempDir();
}

std::wstring NotificationPresenterWin::SaveIconToFilesystem(
    const SkBitmap& icon,
    const GURL& origin) {
  if (icon.drawsNothing())
    return L"";

  std::string filename;
  if (origin.is_valid()) {
    filename = base::MD5String(origin.spec()) + ".png";
  } else {
    const int64_t now_usec = base::Time::Now().since_origin().InMicroseconds();
    filename = base::NumberToString(now_usec) + ".png";
  }

  ScopedAllowBlockingForElectron allow_blocking;
  base::FilePath path = temp_dir_.GetPath().Append(base::UTF8ToWide(filename));

  if (!SaveIconToPath(icon, path))
    return L"";

  return path.value();
}

Notification* NotificationPresenterWin::CreateNotificationObject(
    NotificationDelegate* delegate) {
  return new WindowsToastNotification(delegate, this);
}

}  // namespace electron
Repository: electron/electron
File: shell/browser/notifications/win/notification_presenter_win.cc (C++)

Code before:

std::wstring NotificationPresenterWin::SaveIconToFilesystem(
    const SkBitmap& icon,
    const GURL& origin) {
  if (icon.drawsNothing() || icon.width() <= 0 || icon.height() <= 0)
    return L"";

  std::string filename;
  if (origin.is_valid()) {
    filename = base::MD5String(origin.spec()) + ".png";
  } else {
    const int64_t now_usec = base::Time::Now().since_origin().InMicroseconds();
    filename = base::NumberToString(now_usec) + ".png";
  }

  ScopedAllowBlockingForElectron allow_blocking;
  base::FilePath path = temp_dir_.GetPath().Append(base::UTF8ToWide(filename));

  if (!base::PathExists(path)) {
    if (!SaveIconToPath(icon, path))
      return L"";
  }

  std::optional<int64_t> size = base::GetFileSize(path);
  if (!size.has_value())
    return L"";

  return path.value();
}

Notification* NotificationPresenterWin::CreateNotificationObject(
    NotificationDelegate* delegate) {
  return new WindowsToastNotification(delegate, this);
}

}  // namespace electron


Review comment (suggestion):
I would prefer this to be moved inside `SaveIconToPath` and check the size of input `png_data` given the implementation of  [base::WriteFile](https://source.chromium.org/chromium/chromium/src/+/main:base/files/file_util_win.cc;l=1060-1065), it avoids an extra I/O

Code after:
std::wstring NotificationPresenterWin::SaveIconToFilesystem(
    const SkBitmap& icon,
    const GURL& origin) {
  if (icon.drawsNothing())
    return L"";

  std::string filename;
  if (origin.is_valid()) {
    filename = base::MD5String(origin.spec()) + ".png";
  } else {
    const int64_t now_usec = base::Time::Now().since_origin().InMicroseconds();
    filename = base::NumberToString(now_usec) + ".png";
  }

  ScopedAllowBlockingForElectron allow_blocking;
  base::FilePath path = temp_dir_.GetPath().Append(base::UTF8ToWide(filename));

  if (!SaveIconToPath(icon, path))
    return L"";

  return path.value();
}

Notification* NotificationPresenterWin::CreateNotificationObject(
    NotificationDelegate* delegate) {
  return new WindowsToastNotification(delegate, this);
}

}  // namespace electron
Repository: electron/electron
File: shell/browser/ui/views/frameless_view.cc (C++)

Code before:

  return HTCLIENT;
}

views::View* FramelessView::TargetForRect(views::View* root,
                                          const gfx::Rect& rect) {
  CHECK_EQ(root, this);

  if (NonClientHitTest(rect.origin()) != HTCLIENT)
    return this;

  return FrameView::TargetForRect(root, rect);
}

gfx::Size FramelessView::CalculatePreferredSize(
    const views::SizeBounds& available_size) const {
  return frame_->non_client_view()
      ->GetWindowBoundsForClientBounds(gfx::Rect(
          frame_->client_view()->CalculatePreferredSize(available_size)))
      .size();
}

gfx::Size FramelessView::GetMinimumSize() const {
  return window_->GetMinimumSize();
}

gfx::Size FramelessView::GetMaximumSize() const {
  gfx::Size size = window_->GetMaximumSize();
  // Electron public APIs returns (0, 0) when maximum size is not set, but it
  // would break internal window APIs like HWNDMessageHandler::SetAspectRatio.
  return size.IsEmpty() ? gfx::Size(INT_MAX, INT_MAX) : size;
}
BEGIN_METADATA(FramelessView)
END_METADATA

}  // namespace electron


Review comment (suggestion):
The min/max methods on the base FramelessView class had Windows-specific implementations to use content size constraints and make adjustments for Windows APIs, so I moved that code to overrides in `WinFrameView`. This helped get rid of lots of conversion code in the other frameless view classes used on Linux.

Code after:

  return HTCLIENT;
}

views::View* FramelessView::TargetForRect(views::View* root,
                                          const gfx::Rect& rect) {
  CHECK_EQ(root, this);

  if (NonClientHitTest(rect.origin()) != HTCLIENT)
    return this;

  return FrameView::TargetForRect(root, rect);
}

gfx::Size FramelessView::CalculatePreferredSize(
    const views::SizeBounds& available_size) const {
  return frame_->non_client_view()
      ->GetWindowBoundsForClientBounds(gfx::Rect(
          frame_->client_view()->CalculatePreferredSize(available_size)))
      .size();
}

gfx::Size FramelessView::GetMinimumSize() const {
  if (!window_)
    return gfx::Size();
  return window_->GetMinimumSize();
}

gfx::Size FramelessView::GetMaximumSize() const {
  if (!window_)
    return gfx::Size();
  return window_->GetMaximumSize();
}
BEGIN_METADATA(FramelessView)
END_METADATA

}  // namespace electron
Repository: electron/electron
File: spec/fixtures/api/singleton-userdata/main.js (JavaScript)

Code before:
const { app } = require('electron');

const fs = require('node:fs');
const path = require('node:path');

const userDataFolder = path.join(app.getPath('temp'), 'electron-test-singleton-userdata')
app.setPath('userData', userDataFolder);

// non-existent user data folder should not break requestSingleInstanceLock()
// ref: https://github.com/electron/electron/issues/33547
fs.rmSync(userDataFolder, { recursive: true, force: true });

const gotTheLock = app.requestSingleInstanceLock();
app.exit(gotTheLock ? 0 : 1);

fs.rmSync(userDataFolder, { recursive: true, force: true });

Review comment (suggestion):
Nit: honestly it might make sense now to move the `app.setPath` line back down to after the first `rmSync`. But I think it's a matter of preference.

Code after:
const { app } = require('electron');

const fs = require('node:fs');
const path = require('node:path');

const userDataFolder = path.join(app.getPath('temp'), 'electron-test-singleton-userdata');

// non-existent user data folder should not break requestSingleInstanceLock()
// ref: https://github.com/electron/electron/issues/33547
fs.rmSync(userDataFolder, { recursive: true, force: true });

// set the user data path after clearing out old state and right before we use it
app.setPath('userData', userDataFolder);
const gotTheLock = app.requestSingleInstanceLock();

app.releaseSingleInstanceLock();
fs.rmSync(userDataFolder, { recursive: true, force: true });

app.exit(gotTheLock ? 0 : 1);
Repository: electron/electron
File: spec/fixtures/api/singleton-userdata/main.js (JavaScript)

Code before:
const { app } = require('electron');

const fs = require('node:fs');
const path = require('node:path');

const userDataFolder = path.join(app.getPath('temp'), 'electron-test-singleton-userdata');
app.setPath('userData', userDataFolder);

// non-existent user data folder should not break requestSingleInstanceLock()
// ref: https://github.com/electron/electron/issues/33547
fs.rmSync(userDataFolder, { recursive: true, force: true });
const gotTheLock = app.requestSingleInstanceLock();
fs.rmSync(userDataFolder, { recursive: true, force: true });

app.exit(gotTheLock ? 0 : 1);


Review comment (suggestion):
I think we should call `fs.rmSync` before `app.setPath()`. It's thinkable that Electron would start using the folder as soon as it's set as the `userData` path, so we should clear it before that happens, not after.

Code after:
const { app } = require('electron');

const fs = require('node:fs');
const path = require('node:path');

const userDataFolder = path.join(app.getPath('temp'), 'electron-test-singleton-userdata');

// non-existent user data folder should not break requestSingleInstanceLock()
// ref: https://github.com/electron/electron/issues/33547
fs.rmSync(userDataFolder, { recursive: true, force: true });

// set the user data path after clearing out old state and right before we use it
app.setPath('userData', userDataFolder);
const gotTheLock = app.requestSingleInstanceLock();

app.releaseSingleInstanceLock();
fs.rmSync(userDataFolder, { recursive: true, force: true });

app.exit(gotTheLock ? 0 : 1);
Repository: emcie-co/parlant
File: server/tests/test_loading_configuration.py (Python)

Code before:
import copy
import json
from pathlib import Path
from pydoc import importfile
import tempfile
from typing import Callable

from pytest import fixture

from emcie.server.configuration_validator import ConfigurationFileValidator
from emcie.server.core.common import JSONSerializable


@fixture(scope="function")
def new_file() -> Callable[[], Path]:
    def create_temp_file() -> Path:
        with tempfile.NamedTemporaryFile() as file:
            return Path(file.name)

    return create_temp_file


@fixture
def valid_config(
    new_file: Callable[[], Path],
) -> JSONSerializable:
    tool_file = new_file()

    def create_valid_config() -> JSONSerializable:
        return {
            "agents": [{"name": "Default Agent"}],
            "guidelines": {
                "Default Agent": [
                    {
                        "when": "Ask to multiply two numbers",
                        "then": "use the multiply tool to provide the result",
                        "enabled_tools": ["multiply"],
                    }
                ]
            },
            "tools": {
                "multiply": {
                    "description": "Multiply two numbers",

Review comment (bug):
There's a bug here. You return out of the `with` statement, which ends up calling the `__exit__()` function of `NamedTemporaryFile`. What you want here is to yield the value, and you don't need to return a callable, but instead implement the whole thing with a generator.

See how I implemented this in `persistence.py`



Code after:
from contextlib import asynccontextmanager
import copy
import json
from pathlib import Path
from pydoc import importfile
import tempfile
from typing import AsyncIterator, Callable

from pytest import fixture

from emcie.server.configuration_validator import ConfigurationFileValidator
from emcie.server.core.common import JSONSerializable


@asynccontextmanager
async def new_file_path() -> AsyncIterator[Path]:
    with tempfile.NamedTemporaryFile() as new_file:
        yield Path(new_file.name)


@fixture
async def valid_config() -> JSONSerializable:
    def create_valid_config() -> JSONSerializable:
        return {
            "agents": [{"name": "Default Agent"}],
            "guidelines": {
                "Default Agent": [
                    {
                        "when": "Ask to multiply two numbers",
                        "then": "use the multiply tool to provide the result",
                        "enabled_tools": ["multiply"],
                    }
                ]
            },
            "tools": {
                "multiply": {
                    "description": "Multiply two numbers",
                    "function_name": "multiply",
                    "module_path": importfile(str(tool_file)).__name__,
                    "parameters": {
                        "a": {"description": "first number", "type": "number"},
                        "b": {"description": "second number", "type": "number"},
                    },