ronantakizawa/codereview-bench huggingface dataset:
Repository: LadybirdBrowser/ladybird
File: Libraries/LibWeb/DOM/Event.cpp (C++)

Code before:
            // 1. Decrease currentHiddenLevel by 1.
            --current_hidden_level;

            // 2. If currentHiddenLevel is less than maxHiddenLevel, then set maxHiddenLevel to currentHiddenLevel.
            if (current_hidden_level < max_hidden_level)
                max_hidden_level = current_hidden_level;
        }

        // 4. Increase index by 1.
    }

    // 17. Return composedPath.
    return composed_path;
}

GC::Ptr<JS::Object> Event::current_target() const
{

    if (!m_current_target) {
        return m_current_target;
    }
    if (is<HTML::Window>(*m_current_target)) {
        auto& window = as<HTML::Window>(*m_current_target);
        return window.window();
    }
    return current_target_internal();
}

}


Review comment (suggestion):
This method can be simplified to:

```cpp
    if (auto* window = as_if<HTML::Window>(m_current_target.ptr()))
        return window->window();
    return m_current_target;
```

Code after:
            // 1. Decrease currentHiddenLevel by 1.
            --current_hidden_level;

            // 2. If currentHiddenLevel is less than maxHiddenLevel, then set maxHiddenLevel to currentHiddenLevel.
            if (current_hidden_level < max_hidden_level)
                max_hidden_level = current_hidden_level;
        }

        // 4. Increase index by 1.
    }

    // 17. Return composedPath.
    return composed_path;
}

// AD-HOC: Needed to return a WindowProxy when the current target is a Window,
// ensuring that JavaScript sees the correct global object instead of the internal Window.
GC::Ptr<EventTarget> Event::current_target_for_bindings() const
{
    if (auto* window = as_if<HTML::Window>(m_current_target.ptr()))
        return window->window();
    return m_current_target;
}

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibCore/Forward.h (C/C++)

Code before:
class Event;
class EventLoop;
class EventReceiver;
class File;
class LocalServer;
class LocalSocket;
class MappedFile;
class MimeData;
class NetworkJob;
class NetworkResponse;
class Notifier;
class Process;
class ProcessStatisticsReader;
class Resource;
class ResourceImplementation;
class Socket;
template<typename Result, typename TError = AK::Error>
class Promise;
template<typename Result, typename TError = AK::Error>
class ThreadedPromise;
class SocketAddress;
class TCPServer;
class TCPSocket;
class Timer;
class TimerEvent;
class UDPServer;
class UDPSocket;

struct ProxyData;

#ifdef AK_OS_MACH
class MachPort;
#endif

}


Review comment (bug):
TimeZoneWatcher feels like something that belongs in LibCore though. The fact that one of its implementations require a WebView context feels like we're doing something in the wrong order or are depending on the wrong things?

I'd rather not have the requirement of setting up a web view just to watch for platform timezone changes.

Couldn't we expose a public API for the Windows TimeZoneWatcher implementation that allows the GUI part to inform it of transpired events such as WM_TIMECHANGE?

Code after:
class Event;
class EventLoop;
class EventReceiver;
class File;
class LocalServer;
class LocalSocket;
class MappedFile;
class MimeData;
class NetworkJob;
class NetworkResponse;
class Notifier;
class Process;
class ProcessStatisticsReader;
class Resource;
class ResourceImplementation;
class Socket;
template<typename Result, typename TError = AK::Error>
class Promise;
template<typename Result, typename TError = AK::Error>
class ThreadedPromise;
class SocketAddress;
class TCPServer;
class TCPSocket;
class Timer;
class TimerEvent;
class TimeZoneWatcher;
class UDPServer;
class UDPSocket;

struct ProxyData;

#ifdef AK_OS_MACH
class MachPort;
#endif

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibUnicode/TimeZone.cpp (C++)

Code before:
    icu::UnicodeString time_zone_name;
    time_zone->getCanonicalID(time_zone_id, time_zone_name, status);

    if (icu_failure(status))
        return "UTC"_string;

    return icu_string_to_string(time_zone_name);
}

static String current_host_time_zone()
{
    return current_time_zone_impl(adopt_own_if_nonnull(icu::TimeZone::detectHostTimeZone()));
}

static String current_default_time_zone()
{
    return current_time_zone_impl(adopt_own_if_nonnull(icu::TimeZone::createDefault()));
}

String current_time_zone()
{
    if (cached_system_time_zone.has_value())
        return *cached_system_time_zone;

    cached_system_time_zone = current_host_time_zone();
    return *cached_system_time_zone;
}

void clear_system_time_zone_cache()
{
    cached_system_time_zone.clear();
}

ErrorOr<void> set_current_time_zone(StringView time_zone)
{
    auto time_zone_data = TimeZoneData::for_time_zone(time_zone);
    if (!time_zone_data.has_value())
        return Error::from_string_literal("Unable to find the provided time zone");

    icu::TimeZone::setDefault(time_zone_data->time_zone());
    cached_system_time_zone = current_default_time_zone();

    return {};
}

// https://github.com/unicode-org/icu/blob/main/icu4c/source/tools/tzcode/icuzones
static constexpr bool is_legacy_non_iana_time_zone(StringView time_zone)
{
    constexpr auto legacy_zones = to_array({
        "ACT"sv,
        "AET"sv,

Review comment (performance):
```cpp
return cached_system_time_zone.ensure([] { return current_host_time_zone(); });
```

Code after:
    icu::UnicodeString time_zone_name;
    time_zone->getCanonicalID(time_zone_id, time_zone_name, status);

    if (icu_failure(status))
        return "UTC"_string;

    return icu_string_to_string(time_zone_name);
}

static String current_host_time_zone()
{
    return current_time_zone_impl(adopt_own_if_nonnull(icu::TimeZone::detectHostTimeZone()));
}

static String current_default_time_zone()
{
    return current_time_zone_impl(adopt_own_if_nonnull(icu::TimeZone::createDefault()));
}

String current_time_zone()
{
    return cached_system_time_zone.ensure([] { return current_host_time_zone(); });
}

void clear_system_time_zone_cache()
{
    cached_system_time_zone.clear();
}

ErrorOr<void> set_current_time_zone(StringView time_zone)
{
    auto time_zone_data = TimeZoneData::for_time_zone(time_zone);
    if (!time_zone_data.has_value())
        return Error::from_string_literal("Unable to find the provided time zone");

    icu::TimeZone::setDefault(time_zone_data->time_zone());
    cached_system_time_zone = current_default_time_zone();

    return {};
}

// https://github.com/unicode-org/icu/blob/main/icu4c/source/tools/tzcode/icuzones
static constexpr bool is_legacy_non_iana_time_zone(StringView time_zone)
{
    constexpr auto legacy_zones = to_array({
        "ACT"sv,
        "AET"sv,
        "AGT"sv,
        "ART"sv,
        "AST"sv,
        "BET"sv,
Repository: LadybirdBrowser/ladybird
File: Libraries/LibWeb/NotificationsAPI/Notification.h (C/C++)

Code before:
public:
    [[nodiscard]] static WebIDL::ExceptionOr<GC::Ref<Notification>> construct_impl(
        JS::Realm& realm,
        String title,
        Optional<NotificationOptions> options);

private:
    Notification(JS::Realm&);

    virtual void initialize(JS::Realm&) override;
};

}

namespace Web {
namespace Bindings {

constexpr std::nullptr_t null = nullptr;

}

using NotificationsAPI::Notification;
using NotificationsAPI::NotificationAction;
using NotificationsAPI::NotificationOptions;

}


Review comment (refactor):
I was very confused why you needed this... and you actually don't, but you need to add a fix elsewhere! This function sticks a `using namespace` into the generated code for every `Web::Foo` namespace:

https://github.com/LadybirdBrowser/ladybird/blob/a54215c07d161a98bc8e07d68e2e32803eb3f5da/Meta/Lagom/Tools/CodeGenerators/LibWeb/BindingsGenerator/IDLGenerators.cpp#L4909

You added a new namespace, so you also need to add it to the list here (and correct it in `Forward.h`). Then the generated code will be able to see your types and you can remove this code.

It's a hack because the code generator isn't smart enough to put namespaces before type names. But it works so nobody's fixed it. :^)

Also, needs the fix in #6225 so we stop trying to assign `null` to `NotificationOptions::silent`.

Code after:
/*
 * Copyright (c) 2025, Niccolo Antonelli-Dziri <niccolo.antonelli-dziri@protonmail.com>
 *
 * SPDX-License-Identifier: BSD-2-Clause
 */

#pragma once

#include <AK/Optional.h>
#include <AK/String.h>
#include <AK/Vector.h>
#include <LibJS/Runtime/Value.h>
#include <LibWeb/DOM/EventTarget.h>
#include <LibWeb/HighResolutionTime/EpochTimeStamp.h>

namespace Web::NotificationsAPI {

struct NotificationAction {
    String action;
    String title;
    Optional<String> navigate;
    Optional<String> icon;
};

struct NotificationOptions {
    Bindings::NotificationDirection dir = Bindings::NotificationDirection::Auto;
    String lang = ""_string;
    String body = ""_string;
    Optional<String> navigate;
    String tag = ""_string;
    Optional<String> image;
    Optional<String> icon;
    Optional<String> badge;
    // VibratePattern vibrate;  // FIXME: properly implement vibrate pattern
    Optional<HighResolutionTime::EpochTimeStamp> timestamp;
    bool renotify = false;
    Optional<bool> silent;
    bool require_interaction = false;
    JS::Value data;
    Vector<NotificationAction> actions;
};

// https://notifications.spec.whatwg.org/#notifications
class WEB_API Notification final : public DOM::EventTarget {
    WEB_PLATFORM_OBJECT(Notification, DOM::EventTarget);
    GC_DECLARE_ALLOCATOR(Notification);

public:
    [[nodiscard]] static WebIDL::ExceptionOr<GC::Ref<Notification>> construct_impl(
        JS::Realm& realm,
        String title,
        Optional<NotificationOptions> options);

private:
    Notification(JS::Realm&);

    virtual void initialize(JS::Realm&) override;
};

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibWeb/CSS/CSSRule.h (C/C++)

Code before:
    String css_text() const;
    void set_css_text(StringView);

    CSSRule* parent_rule() { return m_parent_rule.ptr(); }
    CSSRule const* parent_rule() const { return m_parent_rule.ptr(); }
    void set_parent_rule(CSSRule*);

    CSSStyleSheet* parent_style_sheet() { return m_parent_style_sheet.ptr(); }
    virtual void set_parent_style_sheet(CSSStyleSheet*);

    template<typename T>
    bool fast_is() const = delete;

    // https://drafts.csswg.org/cssom-1/#serialize-a-css-rule
    virtual String serialized() const = 0;

    virtual void clear_caches();

protected:
    CSSRule(JS::Realm&, Type);

    virtual void visit_edges(Cell::Visitor&) override;

    [[nodiscard]] FlyString const& parent_layer_internal_qualified_name() const
    {
        return m_cached_layer_name.ensure([&] { return parent_layer_internal_qualified_name_slow_case(); });
    }

    [[nodiscard]] FlyString const& parent_layer_internal_qualified_name_slow_case() const;

    Type m_type;
    GC::Ptr<CSSRule> m_parent_rule;
    GC::Ptr<CSSStyleSheet> m_parent_style_sheet;

    mutable Optional<FlyString> m_cached_layer_name;
};

}


Review comment (performance):
Hmm this one's a little weird because `parent_layer_internal_qualified_name_slow_case` internally sets `m_cached_layer_name`. So we should probably remove that and have it return the generated `FlyString` directly.

Code after:
    String css_text() const;
    void set_css_text(StringView);

    CSSRule* parent_rule() { return m_parent_rule.ptr(); }
    CSSRule const* parent_rule() const { return m_parent_rule.ptr(); }
    void set_parent_rule(CSSRule*);

    CSSStyleSheet* parent_style_sheet() { return m_parent_style_sheet.ptr(); }
    virtual void set_parent_style_sheet(CSSStyleSheet*);

    template<typename T>
    bool fast_is() const = delete;

    // https://drafts.csswg.org/cssom-1/#serialize-a-css-rule
    virtual String serialized() const = 0;

    virtual void clear_caches();

protected:
    CSSRule(JS::Realm&, Type);

    virtual void visit_edges(Cell::Visitor&) override;

    [[nodiscard]] FlyString const& parent_layer_internal_qualified_name() const
    {
        return m_cached_layer_name.ensure([&] { return parent_layer_internal_qualified_name_slow_case(); });
    }

    [[nodiscard]] FlyString parent_layer_internal_qualified_name_slow_case() const;

    Type m_type;
    GC::Ptr<CSSRule> m_parent_rule;
    GC::Ptr<CSSStyleSheet> m_parent_style_sheet;

    mutable Optional<FlyString> m_cached_layer_name;
};

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibWeb/HTML/HTMLHeadingElement.h (C/C++)

Code before:
 * SPDX-License-Identifier: BSD-2-Clause
 */

#pragma once

#include <LibWeb/ARIA/Roles.h>
#include <LibWeb/HTML/HTMLElement.h>

namespace Web::HTML {

class HTMLHeadingElement final : public HTMLElement {
    WEB_PLATFORM_OBJECT(HTMLHeadingElement, HTMLElement);
    GC_DECLARE_ALLOCATOR(HTMLHeadingElement);

public:
    virtual ~HTMLHeadingElement() override;

    virtual bool is_presentational_hint(FlyString const&) const override;
    virtual void apply_presentational_hints(GC::Ref<CSS::CascadedProperties>) const override;

    // https://www.w3.org/TR/html-aria/#el-h1-h6
    virtual Optional<ARIA::Role> default_role() const override { return ARIA::Role::heading; }

    virtual Optional<String> aria_level() const override
    {
        if (auto const attr = get_attribute("aria-level"_fly_string); attr.has_value())
            return attr;

        // Implicit defaults to the number in the element's tag name.
        auto const digit_view = local_name().bytes_as_string_view().substring_view(1, 1);
        return String::from_utf8_without_validation(digit_view.bytes());
    }

private:
    HTMLHeadingElement(DOM::Document&, DOM::QualifiedName);

    virtual void initialize(JS::Realm&) override;
};

}


Review comment (suggestion):
We create FlyStrings for standard attribute names already. For this, it'd be `ARIA::AttributeNames::aria_level`, in `LibWeb/ARIA/AttributeNames.h`.

Code after:
 * SPDX-License-Identifier: BSD-2-Clause
 */

#pragma once

#include <LibWeb/ARIA/Roles.h>
#include <LibWeb/HTML/HTMLElement.h>

namespace Web::HTML {

class HTMLHeadingElement final : public HTMLElement {
    WEB_PLATFORM_OBJECT(HTMLHeadingElement, HTMLElement);
    GC_DECLARE_ALLOCATOR(HTMLHeadingElement);

public:
    virtual ~HTMLHeadingElement() override;

    virtual bool is_presentational_hint(FlyString const&) const override;
    virtual void apply_presentational_hints(GC::Ref<CSS::CascadedProperties>) const override;

    // https://www.w3.org/TR/html-aria/#el-h1-h6
    virtual Optional<ARIA::Role> default_role() const override { return ARIA::Role::heading; }

    virtual Optional<String> aria_level() const override
    {
        if (auto const attr = get_attribute(ARIA::AttributeNames::aria_level); attr.has_value())
            return attr;

        // Implicit defaults to the number in the element's tag name.
        return MUST(local_name().to_string().substring_from_byte_offset(1));
    }

private:
    HTMLHeadingElement(DOM::Document&, DOM::QualifiedName);

    virtual void initialize(JS::Realm&) override;
};

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibWeb/HTML/HTMLHeadingElement.h (C/C++)

Code before:
#include <LibWeb/ARIA/Roles.h>
#include <LibWeb/HTML/HTMLElement.h>

namespace Web::HTML {

class HTMLHeadingElement final : public HTMLElement {
    WEB_PLATFORM_OBJECT(HTMLHeadingElement, HTMLElement);
    GC_DECLARE_ALLOCATOR(HTMLHeadingElement);

public:
    virtual ~HTMLHeadingElement() override;

    virtual bool is_presentational_hint(FlyString const&) const override;
    virtual void apply_presentational_hints(GC::Ref<CSS::CascadedProperties>) const override;

    // https://www.w3.org/TR/html-aria/#el-h1-h6
    virtual Optional<ARIA::Role> default_role() const override { return ARIA::Role::heading; }

    virtual Optional<String> aria_level() const override
    {
        if (auto const attr = get_attribute("aria-level"_fly_string); attr.has_value())
            return attr;

        // Implicit defaults to the number in the element's tag name.
        auto const digit_view = local_name().bytes_as_string_view().substring_view(1, 1);
        return String::from_utf8_without_validation(digit_view.bytes());
    }

private:
    HTMLHeadingElement(DOM::Document&, DOM::QualifiedName);

    virtual void initialize(JS::Realm&) override;
};

}


Review comment (suggestion):
You don't need to limit the substring to being 1 byte long - just skipping the first byte is enough, and would make it a bit more future proof in case HTML decides to add further h levels.

Also, you can write this a bit more directly. Something like:
```c++
MUST(local_name().to_string().substring_from_byte_offset(1));
```
(Working with byte counts is a bit sketchy in general, but should be fine here as we know the first character is `h` so it's a single byte.)

(You might be able to use `substring_from_byte_offset_with_shared_superstring()` but I'm not 100% confident on the FlyString->String conversion still pointing to the same data.)

Code after:
#include <LibWeb/ARIA/Roles.h>
#include <LibWeb/HTML/HTMLElement.h>

namespace Web::HTML {

class HTMLHeadingElement final : public HTMLElement {
    WEB_PLATFORM_OBJECT(HTMLHeadingElement, HTMLElement);
    GC_DECLARE_ALLOCATOR(HTMLHeadingElement);

public:
    virtual ~HTMLHeadingElement() override;

    virtual bool is_presentational_hint(FlyString const&) const override;
    virtual void apply_presentational_hints(GC::Ref<CSS::CascadedProperties>) const override;

    // https://www.w3.org/TR/html-aria/#el-h1-h6
    virtual Optional<ARIA::Role> default_role() const override { return ARIA::Role::heading; }

    virtual Optional<String> aria_level() const override
    {
        if (auto const attr = get_attribute(ARIA::AttributeNames::aria_level); attr.has_value())
            return attr;

        // Implicit defaults to the number in the element's tag name.
        return MUST(local_name().to_string().substring_from_byte_offset(1));
    }

private:
    HTMLHeadingElement(DOM::Document&, DOM::QualifiedName);

    virtual void initialize(JS::Realm&) override;
};

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibJS/Tests/functions/arrow-functions.js (JavaScript)

Code before:
    expect(sum).toBe(6);

    const product = ((x, y, z) => {
        let res = x * y * z;
        return res;
    })(5, 4, 2);
    expect(product).toBe(40);

    const half = (x => {
        return x / 2;
    })(10);
    expect(half).toBe(5);
});

test("currying", () => {
    let add = a => b => a + b;
    expect(typeof add(1)).toBe("function");
    expect(typeof add(1, 2)).toBe("function");
    expect(add(1)(2)).toBe(3);
});

test("with comma operator", () => {
    let foo, bar;
    // prettier-ignore
    // Ignored so the expressions arent wrapped in parens
    (foo = bar), baz => {};
    expect(foo).toBe(undefined);
    expect(bar).toBe(undefined);
});

test("arrow functions in objects", () => {
    function FooBar() {
        this.x = {
            y: () => this,
            z: function () {
                return (() => this)();
            },
        };
    }

    const foobar = new FooBar();
    expect(foobar.x.y()).toBe(foobar);
    expect(foobar.x.z()).toBe(foobar.x);
});

test("strict mode propagation", () => {
    (() => {
        "use strict";
        expect(isStrictMode()).toBeTrue();

        (() => {

Review comment (suggestion):
We prefer our commits to be atomic. So if the purpose of this commit is to remove files from .prettierignore, then the changes to handle differences from prettier 3.6 should be in another commit. Personally, I don't mind if prettier adds parentheses here so long as older prettier versions (namely, the version used in CI) don't complain. We also have the option to update the version of prettier used in CI.

Code after:
    expect(sum).toBe(6);

    const product = ((x, y, z) => {
        let res = x * y * z;
        return res;
    })(5, 4, 2);
    expect(product).toBe(40);

    const half = (x => {
        return x / 2;
    })(10);
    expect(half).toBe(5);
});

test("currying", () => {
    let add = a => b => a + b;
    expect(typeof add(1)).toBe("function");
    expect(typeof add(1, 2)).toBe("function");
    expect(add(1)(2)).toBe(3);
});

test("with comma operator", () => {
    let foo, bar;
    (foo = bar), baz => {};
    expect(foo).toBe(undefined);
    expect(bar).toBe(undefined);
});

test("arrow functions in objects", () => {
    function FooBar() {
        this.x = {
            y: () => this,
            z: function () {
                return (() => this)();
            },
        };
    }

    const foobar = new FooBar();
    expect(foobar.x.y()).toBe(foobar);
    expect(foobar.x.z()).toBe(foobar.x);
});

test("strict mode propagation", () => {
    (() => {
        "use strict";
        expect(isStrictMode()).toBeTrue();

        (() => {
            expect(isStrictMode()).toBeTrue();
        })();
Repository: LadybirdBrowser/ladybird
File: Meta/check-flatpak.py (Python)

Code before:
import sys

from enum import Enum

VCPKG = "vcpkg.json"
VCPKG_OVERLAYS_PORTS = "Meta/CMake/vcpkg/overlay-ports/*"
FLATPAK_MANIFEST = "Meta/CMake/flatpak/org.ladybird.Ladybird.json"
SELF = "Ladybird"

# List of build tools that are not provided by the Flatpak SDK and therefor in the manifest
# For a vcpkg build these are installed on the host system
flatpak_build_tools = [
    "gn",
]

# List of libraries that are dependencies of dependencies
# These are in the manifest to fetch before the build but not explicitely versioned in vcpkg
flatpak_deps_of_deps = [
    "brotli",
    "dav1d",
    "highway",
    "libdwarf",
    "libxml2",
    "libyuv",
    "vulkan-memory-allocator",
    "zstd",
]

# List of libraries that are in the Flatpak runtime and therefor not in the manifest
# See: https://docs.flatpak.org/en/latest/available-runtimes.html#check-software-available-in-runtimes
flatpak_runtime_libs = [
    "curl",
    "dbus",
    "ffmpeg",
    "fontconfig",
    "harfbuzz",
    "libjpeg-turbo",
    "libproxy",
    "openssl",
    "qtbase",
    "qtmultimedia",
    "sqlite3",
    "zlib",
]

# List of libraries that are in vcpkg but not needed for the Linux platform
vcpkg_not_linux = [
    "dirent",
    "mman",
]


Review comment (suggestion):
Apart from `vulkan-memory-allocator`, seems like we pull all of these in as part of a vcpkg build. Shouldn't we pin these versions in vcpkg.json as well, even though they're transitive dependencies? 🤔 

Code after:
import os
import subprocess
import sys

from enum import Enum

VCPKG = "vcpkg.json"
VCPKG_OVERLAYS_PORTS = "Meta/CMake/vcpkg/overlay-ports/*"
VCPKG_URL = "https://github.com/microsoft/vcpkg.git"
VCPKG_REPO = "Build/vcpkg"
FLATPAK_MANIFEST = "Meta/CMake/flatpak/org.ladybird.Ladybird.json"
SELF = "Ladybird"

# List of build tools that are not provided by the Flatpak SDK and therefore in the manifest
# For a vcpkg build these are installed on the host system
flatpak_build_tools = [
    "gn",
]

# List of libraries that are transitive dependencies
# These are in the manifest to fetch before building, but not explicitely versioned
flatpak_transitive_deps = [
    "libyuv",
]

# List of libraries that are in the Flatpak runtime and therefore not in the manifest
# See: https://docs.flatpak.org/en/latest/available-runtimes.html#check-software-available-in-runtimes
flatpak_runtime_libs = [
    "curl",
    "dbus",
    "fontconfig",
    "harfbuzz",
    "libjpeg-turbo",
    "libproxy",
    "qtbase",
    "qtmultimedia",
    "sqlite3",
    "zlib",
]

# List of libraries that are in vcpkg but not needed for the Linux platform
vcpkg_not_linux = [
    "dirent",
    "mman",
]


class DepMatch(Enum):
    Match = (0,)
    NoMatch = (1,)
    VersionMismatch = (2,)