ronantakizawa/codereview-bench huggingface dataset:
Repository: LadybirdBrowser/ladybird
File: Libraries/LibWebView/Settings.h (C/C++)

Code before:
    void set_autoplay_enabled_globally(bool);
    void add_autoplay_site_filter(String const&);
    void remove_autoplay_site_filter(String const&);
    void remove_all_autoplay_site_filters();

    DoNotTrack do_not_track() const { return m_do_not_track; }
    void set_do_not_track(DoNotTrack);

    static DNSSettings parse_dns_settings(JsonValue const&);
    DNSSettings const& dns_settings() const { return m_dns_settings; }
    void set_dns_settings(DNSSettings const&, bool override_by_command_line = false);

    static void add_observer(Badge<SettingsObserver>, SettingsObserver&);
    static void remove_observer(Badge<SettingsObserver>, SettingsObserver&);

private:
    explicit Settings(ByteString settings_path);

    void persist_settings();

    Optional<SearchEngine> find_search_engine_by_name(StringView name);

    ByteString m_settings_path;

    URL::URL m_new_tab_page_url;
    double m_default_zoom_level_factor;
    Vector<String> m_languages;
    Optional<SearchEngine> m_search_engine;
    Vector<SearchEngine> m_custom_search_engines;
    Optional<AutocompleteEngine> m_autocomplete_engine;
    SiteSetting m_autoplay;
    DoNotTrack m_do_not_track { DoNotTrack::No };
    DNSSettings m_dns_settings { SystemDNS() };
    bool m_dns_override_by_command_line { false };

    Vector<SettingsObserver&> m_observers;
};

}


Review comment (suggestion):
We prefer to always initialize plain in the member list:
```c++
double m_default_zoom_level_factor { 0 };
```
even if the value is set in the constructor.

Code after:
    void set_autoplay_enabled_globally(bool);
    void add_autoplay_site_filter(String const&);
    void remove_autoplay_site_filter(String const&);
    void remove_all_autoplay_site_filters();

    DoNotTrack do_not_track() const { return m_do_not_track; }
    void set_do_not_track(DoNotTrack);

    static DNSSettings parse_dns_settings(JsonValue const&);
    DNSSettings const& dns_settings() const { return m_dns_settings; }
    void set_dns_settings(DNSSettings const&, bool override_by_command_line = false);

    static void add_observer(Badge<SettingsObserver>, SettingsObserver&);
    static void remove_observer(Badge<SettingsObserver>, SettingsObserver&);

private:
    explicit Settings(ByteString settings_path);

    void persist_settings();

    Optional<SearchEngine> find_search_engine_by_name(StringView name);

    ByteString m_settings_path;

    URL::URL m_new_tab_page_url;
    double m_default_zoom_level_factor { 0 };
    Vector<String> m_languages;
    Optional<SearchEngine> m_search_engine;
    Vector<SearchEngine> m_custom_search_engines;
    Optional<AutocompleteEngine> m_autocomplete_engine;
    SiteSetting m_autoplay;
    DoNotTrack m_do_not_track { DoNotTrack::No };
    DNSSettings m_dns_settings { SystemDNS() };
    bool m_dns_override_by_command_line { false };

    Vector<SettingsObserver&> m_observers;
};

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibCore/SystemWindows.cpp (C++)

Code before:
    if (rc != 0)
        return Error::from_windows_error(rc);

    Vector<struct addrinfo> addresses;

    for (auto* result = results; result != nullptr; result = result->ai_next)
        TRY(addresses.try_append(*result));

    return AddressInfoVector { move(addresses), results };
}

ErrorOr<void> connect(int socket, struct sockaddr const* address, socklen_t address_length)
{
    if (::connect(socket, address, address_length) == SOCKET_ERROR)
        return Error::from_windows_error();
    return {};
}


ErrorOr<void> kill(pid_t pid, int signal)
{
    if (signal == SIGTERM) {
        HANDLE process_handle = OpenProcess(PROCESS_TERMINATE, FALSE, pid);
        if (process_handle == nullptr)
            return Error::from_windows_error();
        if (!TerminateProcess(process_handle, 0))
            return Error::from_windows_error();
        if (!CloseHandle(process_handle))
            return Error::from_windows_error();
    }
    else
        return Error::from_string_literal("Unsupported signal value");
    return {};
}

}


Review comment (suggestion):
Ah, this is actually more akin to `SIGKILL` behaviour, should probably use a more graceful shutdown along the lines to the strategy described in https://danielkaes.wordpress.com/2009/06/04/how-to-catch-kill-events-with-python/

Code after:
    if (rc != 0)
        return Error::from_windows_error(rc);

    Vector<struct addrinfo> addresses;

    for (auto* result = results; result != nullptr; result = result->ai_next)
        TRY(addresses.try_append(*result));

    return AddressInfoVector { move(addresses), results };
}

ErrorOr<void> connect(int socket, struct sockaddr const* address, socklen_t address_length)
{
    if (::connect(socket, address, address_length) == SOCKET_ERROR)
        return Error::from_windows_error();
    return {};
}

ErrorOr<void> kill(pid_t pid, int signal)
{
    if (signal == SIGTERM) {
        if (!EnumWindows([](HWND hwnd, LPARAM l_param) -> BOOL {
                DWORD window_pid = 0;
                GetWindowThreadProcessId(hwnd, &window_pid);
                if (window_pid == static_cast<DWORD>(l_param)) {
                    PostMessage(hwnd, WM_CLOSE, 0, 0);
                }
                return TRUE;
            },
                pid))
            return Error::from_windows_error();
    } else {
        return Error::from_string_literal("Unsupported signal value");
    }
    return {};
}

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibCore/SystemWindows.cpp (C++)

Code before:

#include <AK/ByteString.h>
#include <AK/ScopeGuard.h>
#include <LibCore/Process.h>
#include <LibCore/System.h>
#include <direct.h>
#include <sys/mman.h>

#include <AK/Windows.h>

namespace Core::System {

int windows_socketpair(SOCKET socks[2], int make_overlapped);

static void invalid_parameter_handler(wchar_t const*, wchar_t const*, wchar_t const*, unsigned int, uintptr_t)
{
}

static int init_crt()
{
    // Make _get_osfhandle return -1 instead of crashing on invalid fd in release (debug still __debugbreak's)
    _set_invalid_parameter_handler(invalid_parameter_handler);
    return 0;
}

static auto dummy = init_crt();

ErrorOr<int> open(StringView path, int options, mode_t mode)
{
    ByteString str = path;
    int fd = _open(str.characters(), options | O_BINARY | _O_OBTAIN_DIR, mode);
    if (fd < 0)
        return Error::from_syscall("open"sv, errno);
    ScopeGuard guard = [&] { _close(fd); };
    return dup(_get_osfhandle(fd));
}

ErrorOr<void> close(int handle)
{
    if (is_socket(handle)) {
        if (closesocket(handle))
            return Error::from_windows_error();
    } else {
        if (!CloseHandle(to_handle(handle)))
            return Error::from_windows_error();
    }
    return {};
}

ErrorOr<ssize_t> read(int handle, Bytes buffer)
{

Review comment (suggestion):
Are you sure this is fine? Calling CRT functions during during CRT init and DllMain is generally considered unsafe. To be sure can we move it to where WSA init is now? Testing on my machine I just moved the whole function to libmain and called it there.

Code after:

#include <AK/ByteString.h>
#include <AK/ScopeGuard.h>
#include <LibCore/Process.h>
#include <LibCore/System.h>
#include <direct.h>
#include <sys/mman.h>

#include <AK/Windows.h>

namespace Core::System {

int windows_socketpair(SOCKET socks[2], int make_overlapped);

ErrorOr<int> open(StringView path, int options, mode_t mode)
{
    ByteString str = path;
    int fd = _open(str.characters(), options | O_BINARY | _O_OBTAIN_DIR, mode);
    if (fd < 0)
        return Error::from_syscall("open"sv, errno);
    ScopeGuard guard = [&] { _close(fd); };
    return dup(_get_osfhandle(fd));
}

ErrorOr<void> close(int handle)
{
    if (is_socket(handle)) {
        if (closesocket(handle))
            return Error::from_windows_error();
    } else {
        if (!CloseHandle(to_handle(handle)))
            return Error::from_windows_error();
    }
    return {};
}

ErrorOr<ssize_t> read(int handle, Bytes buffer)
{
    DWORD n_read = 0;
    if (!ReadFile(to_handle(handle), buffer.data(), buffer.size(), &n_read, NULL))
        return Error::from_windows_error();
    return n_read;
}

ErrorOr<ssize_t> write(int handle, ReadonlyBytes buffer)
{
    DWORD n_written = 0;
    if (!WriteFile(to_handle(handle), buffer.data(), buffer.size(), &n_written, NULL))
        return Error::from_windows_error();
    return n_written;
}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibWeb/CSS/ComputedProperties.h (C/C++)

Code before:
    friend class StyleComputer;

    ComputedProperties();

    virtual void visit_edges(Visitor&) override;

    Overflow overflow(PropertyID) const;
    Vector<ShadowData> shadow(PropertyID, Layout::Node const&) const;

    GC::Ptr<CSSStyleDeclaration const> m_animation_name_source;
    GC::Ptr<CSSStyleDeclaration const> m_transition_property_source;

    Array<RefPtr<StyleValue const>, number_of_properties> m_property_values;
    Array<u8, ceil_div(number_of_properties, 8uz)> m_property_important {};
    Array<u8, ceil_div(number_of_properties, 8uz)> m_property_inherited {};
    Array<u8, ceil_div(number_of_properties, 8uz)> m_animated_property_inherited {};

    HashMap<PropertyID, NonnullRefPtr<StyleValue const>> m_animated_property_values;

    int m_math_depth { InitialValues::math_depth() };
    RefPtr<Gfx::FontCascadeList const> m_font_list;
    RefPtr<Gfx::Font const> m_first_available_computed_font;

    Optional<CSSPixels> m_line_height;

    PseudoClassBitmap m_attempted_pseudo_class_matches;
};

}


Review comment (performance):
We originally added this because `font_size()` was getting called all over the place in layout (due to units like `em` resolving against the font size). It seems likely that changing this to a `property()` lookup every time would cause a performance regression :)

Code after:
    virtual void visit_edges(Visitor&) override;

    Overflow overflow(PropertyID) const;
    Vector<ShadowData> shadow(PropertyID, Layout::Node const&) const;

    GC::Ptr<CSSStyleDeclaration const> m_animation_name_source;
    GC::Ptr<CSSStyleDeclaration const> m_transition_property_source;

    Array<RefPtr<StyleValue const>, number_of_longhand_properties> m_property_values;
    Array<u8, ceil_div(number_of_longhand_properties, 8uz)> m_property_important {};
    Array<u8, ceil_div(number_of_longhand_properties, 8uz)> m_property_inherited {};
    Array<u8, ceil_div(number_of_longhand_properties, 8uz)> m_animated_property_inherited {};

    HashMap<PropertyID, NonnullRefPtr<StyleValue const>> m_animated_property_values;

    int m_math_depth { InitialValues::math_depth() };
    RefPtr<Gfx::FontCascadeList const> m_font_list;
    RefPtr<Gfx::Font const> m_first_available_computed_font;

    Optional<CSSPixels> m_line_height;

    PseudoClassBitmap m_attempted_pseudo_class_matches;
};

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

Code before:
    bool has_in_list_item_scope(FlyString const& tag_name) const;
    bool has_in_select_scope(FlyString const& tag_name) const;

    bool has_in_scope(DOM::Element const&) const;

    bool contains(DOM::Element const&) const;
    [[nodiscard]] bool contains_template_element() const;

    auto const& elements() const { return m_elements; }
    auto& elements() { return m_elements; }

    void pop_until_an_element_with_tag_name_has_been_popped(FlyString const& local_name);

    GC::Ptr<DOM::Element> topmost_special_node_below(DOM::Element const&);

    struct LastElementResult {
        GC::Ptr<DOM::Element> element;
        ssize_t index;
    };
    LastElementResult last_element_with_tag_name(FlyString const&);
    GC::Ptr<DOM::Element> element_immediately_above(DOM::Element const&);

    void visit_edges(JS::Cell::Visitor&);

private:
    bool has_in_scope_impl(FlyString const& tag_name, Vector<FlyString> const&, bool checkMathAndSvg = false) const;
    bool has_in_scope_impl(DOM::Element const& target_node, Vector<FlyString> const&) const;

    Vector<GC::Ref<DOM::Element>> m_elements;
};

}


Review comment (suggestion):
A couple of code-style things:
- We use `snake_case` for variable names.
- We like to use named enum classes instead of bools for function parameters, as it makes it easier to see what they do.

So something like:

```c++
enum class CheckMathAndSVG : u8 {
    No,
    Yes,
};
bool has_in_scope_impl(FlyString const& tag_name, Vector<FlyString> const&, CheckMathAndSVG = CheckMathAndSVG::No) const;
```

Code after:
    bool has_in_list_item_scope(FlyString const& tag_name) const;
    bool has_in_select_scope(FlyString const& tag_name) const;

    bool has_in_scope(DOM::Element const&) const;

    bool contains(DOM::Element const&) const;
    [[nodiscard]] bool contains_template_element() const;

    auto const& elements() const { return m_elements; }
    auto& elements() { return m_elements; }

    void pop_until_an_element_with_tag_name_has_been_popped(FlyString const& local_name);

    GC::Ptr<DOM::Element> topmost_special_node_below(DOM::Element const&);

    struct LastElementResult {
        GC::Ptr<DOM::Element> element;
        ssize_t index;
    };
    LastElementResult last_element_with_tag_name(FlyString const&);
    GC::Ptr<DOM::Element> element_immediately_above(DOM::Element const&);

    void visit_edges(JS::Cell::Visitor&);

private:
    enum class CheckMathAndSVG : u8 {
        No,
        Yes,
    };

    bool has_in_scope_impl(FlyString const& tag_name, Vector<FlyString> const&, CheckMathAndSVG) const;
    bool has_in_scope_impl(DOM::Element const& target_node, Vector<FlyString> const&) const;

    Vector<GC::Ref<DOM::Element>> m_elements;
};

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

Code before:
    bool contains(DOM::Element const&) const;
    [[nodiscard]] bool contains_template_element() const;

    auto const& elements() const { return m_elements; }
    auto& elements() { return m_elements; }

    void pop_until_an_element_with_tag_name_has_been_popped(FlyString const& local_name);

    GC::Ptr<DOM::Element> topmost_special_node_below(DOM::Element const&);

    struct LastElementResult {
        GC::Ptr<DOM::Element> element;
        ssize_t index;
    };
    LastElementResult last_element_with_tag_name(FlyString const&);
    GC::Ptr<DOM::Element> element_immediately_above(DOM::Element const&);

    void visit_edges(JS::Cell::Visitor&);

private:
    enum class CheckMathAndSVG : u8 {
        No,
        Yes,
    };

    bool has_in_scope_impl(FlyString const& tag_name, Vector<FlyString> const&, CheckMathAndSVG = CheckMathAndSVG::No) const;
    bool has_in_scope_impl(DOM::Element const& target_node, Vector<FlyString> const&) const;

    Vector<GC::Ref<DOM::Element>> m_elements;
};

}


Review comment (suggestion):
I'd rather not have the default `CheckMathAndSVG::No` value - it makes it clearer in the code what we're checking against.

Code after:
    bool contains(DOM::Element const&) const;
    [[nodiscard]] bool contains_template_element() const;

    auto const& elements() const { return m_elements; }
    auto& elements() { return m_elements; }

    void pop_until_an_element_with_tag_name_has_been_popped(FlyString const& local_name);

    GC::Ptr<DOM::Element> topmost_special_node_below(DOM::Element const&);

    struct LastElementResult {
        GC::Ptr<DOM::Element> element;
        ssize_t index;
    };
    LastElementResult last_element_with_tag_name(FlyString const&);
    GC::Ptr<DOM::Element> element_immediately_above(DOM::Element const&);

    void visit_edges(JS::Cell::Visitor&);

private:
    enum class CheckMathAndSVG : u8 {
        No,
        Yes,
    };

    bool has_in_scope_impl(FlyString const& tag_name, Vector<FlyString> const&, CheckMathAndSVG) const;
    bool has_in_scope_impl(DOM::Element const& target_node, Vector<FlyString> const&) const;

    Vector<GC::Ref<DOM::Element>> m_elements;
};

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibWeb/DOM/DocumentOrShadowRoot.h (C/C++)

Code before:
#include <LibWeb/DOM/Utils.h>

namespace Web::DOM {

template<typename T>
concept DocumentOrShadowRoot = OneOf<T, Document, ShadowRoot>;

// https://html.spec.whatwg.org/multipage/interaction.html#dom-documentorshadowroot-activeelement
template<DocumentOrShadowRoot T>
Element* calculate_active_element(T& self)
{
    // 1. Let candidate be this's node document's focused area's DOM anchor.
    Node* candidate = self.document().focused_area();

    // 2. Set candidate to the result of retargeting candidate against this.
    candidate = as<Node>(retarget(candidate, &self));

    // 3. If candidate's root is not this, then return null.
    if (&candidate->root() != &self)
        return nullptr;

    // 4. If candidate is not a Document object, then return candidate.
    if (!is<Document>(candidate))
        return as<Element>(candidate);

    auto* candidate_document = static_cast<Document*>(candidate);

    // 5. If candidate has a body element, then return that body element.
    if (candidate_document->body())
        return candidate_document->body();

    // 6. If candidate's document element is non-null, then return that document element.
    if (candidate_document->document_element())
        return candidate_document->document_element();

    // 7. Return null.
    return nullptr;
}

}


Review comment (suggestion):
`as<T>()`/`as_if<T>()` should be used where possible instead of `static_cast`

```suggestion
    auto* candidate_document = as_if<Document>(candidate);
    if (!candidate_document)
        return as<Element>(candidate);
```

Code after:
#include <LibWeb/DOM/Utils.h>

namespace Web::DOM {

template<typename T>
concept DocumentOrShadowRoot = OneOf<T, Document, ShadowRoot>;

// https://html.spec.whatwg.org/multipage/interaction.html#dom-documentorshadowroot-activeelement
template<DocumentOrShadowRoot T>
GC::Ptr<Element> calculate_active_element(T& self)
{
    // 1. Let candidate be this's node document's focused area's DOM anchor.
    Node* candidate = self.document().focused_area();

    // 2. Set candidate to the result of retargeting candidate against this.
    candidate = as<Node>(retarget(candidate, &self));

    // 3. If candidate's root is not this, then return null.
    if (&candidate->root() != &self)
        return nullptr;

    // 4. If candidate is not a Document object, then return candidate.
    if (!is<Document>(candidate))
        return as<Element>(candidate);

    auto* candidate_document = as_if<Document>(candidate);

    // 5. If candidate has a body element, then return that body element.
    if (auto* body = candidate_document->body())
        return body;

    // 6. If candidate's document element is non-null, then return that document element.
    if (auto* document_element = candidate_document->document_element())
        return document_element;

    // 7. Return null.
    return nullptr;
}

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibWeb/DOM/DocumentOrShadowRoot.h (C/C++)

Code before:
template<DocumentOrShadowRoot T>
Element* calculate_active_element(T& self)
{
    // 1. Let candidate be this's node document's focused area's DOM anchor.
    Node* candidate = self.document().focused_area();

    // 2. Set candidate to the result of retargeting candidate against this.
    candidate = as<Node>(retarget(candidate, &self));

    // 3. If candidate's root is not this, then return null.
    if (&candidate->root() != &self)
        return nullptr;

    // 4. If candidate is not a Document object, then return candidate.
    if (!is<Document>(candidate))
        return as<Element>(candidate);

    auto* candidate_document = static_cast<Document*>(candidate);

    // 5. If candidate has a body element, then return that body element.
    if (candidate_document->body())
        return candidate_document->body();

    // 6. If candidate's document element is non-null, then return that document element.
    if (candidate_document->document_element())
        return candidate_document->document_element();

    // 7. Return null.
    return nullptr;
}

}


Review comment (suggestion):
If-init statements can be used here to avoid repetition:
```suggestion
    if (auto* body = candidate_document->body())
        return body;

    // 6. If candidate's document element is non-null, then return that document element.
    if (auto* document_element = candidate_document->document_element())
        return document_element;
```

Code after:
template<DocumentOrShadowRoot T>
GC::Ptr<Element> calculate_active_element(T& self)
{
    // 1. Let candidate be this's node document's focused area's DOM anchor.
    Node* candidate = self.document().focused_area();

    // 2. Set candidate to the result of retargeting candidate against this.
    candidate = as<Node>(retarget(candidate, &self));

    // 3. If candidate's root is not this, then return null.
    if (&candidate->root() != &self)
        return nullptr;

    // 4. If candidate is not a Document object, then return candidate.
    if (!is<Document>(candidate))
        return as<Element>(candidate);

    auto* candidate_document = as_if<Document>(candidate);

    // 5. If candidate has a body element, then return that body element.
    if (auto* body = candidate_document->body())
        return body;

    // 6. If candidate's document element is non-null, then return that document element.
    if (auto* document_element = candidate_document->document_element())
        return document_element;

    // 7. Return null.
    return nullptr;
}

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibJS/Runtime/ValueInlines.h (C/C++)

Code before:
/*
 * Copyright (c) 2023, Andreas Kling <andreas@ladybird.org>
 *
 * SPDX-License-Identifier: BSD-2-Clause
 */

#pragma once

#include <LibJS/Runtime/Completion.h>
#include <LibJS/Runtime/Value.h>

namespace JS {

inline bool Value::to_boolean() const
{
    // OPTIMIZATION: Fast path for when this value is already a boolean.
    if (is_boolean())
        return as_bool();

    if (is_int32())
        return as_i32() != 0;

    return to_boolean_slow_case();
}

inline ThrowCompletionOr<Value> Value::to_number(VM& vm) const
{
    // OPTIMIZATION: Fast path for when this value is already a number.
    if (is_number())
        return *this;

    return to_number_slow_case(vm);
}

inline ThrowCompletionOr<Value> Value::to_numeric(VM& vm) const
{
    // OPTIMIZATION: Fast path for when this value is already a number.
    if (is_number())
        return *this;

    return to_numeric_slow_case(vm);
}

inline ThrowCompletionOr<Value> Value::to_primitive(VM& vm, PreferredType preferred_type) const
{
    if (!is_object())
        return *this;
    return to_primitive_slow_case(vm, preferred_type);
}

}


Review comment (performance):
for the record: you have `*_slow_case` in other places. shall other places also use this fast path? (slow case seem to be internal only for other types).

there's also a number check in the slow case; maybe it is enough to comibne these two instead and request `realm` in branches it is needed to achieve the same? 🤔

Code after:
        return *this;

    return to_number_slow_case(vm);
}

inline ThrowCompletionOr<Value> Value::to_numeric(VM& vm) const
{
    // OPTIMIZATION: Fast path for when this value is already a number.
    if (is_number())
        return *this;

    return to_numeric_slow_case(vm);
}

inline ThrowCompletionOr<Value> Value::to_primitive(VM& vm, PreferredType preferred_type) const
{
    if (!is_object())
        return *this;
    return to_primitive_slow_case(vm, preferred_type);
}

inline ThrowCompletionOr<GC::Ref<Object>> Value::to_object(VM& vm) const
{
    if (is_object())
        return const_cast<Object&>(as_object());
    return to_object_slow(vm);
}

}