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

Code before:
// https://w3c.github.io/battery/#the-getbattery-method
GC::Ref<WebIDL::Promise> Navigator::get_battery()
{
    auto& realm = this->realm();

    // 1. If this.[[BatteryPromise]] is null, then set it to a new promise in this's relevant realm.
    if (!m_battery_promise) {
        // FIXME: Don't reject promise here; let steps 2-3 determine resolution
        WebIDL::SimpleException exception {
            WebIDL::SimpleExceptionType::TypeError,
            "Battery Status API is not yet implemented"sv
        };
        m_battery_promise = WebIDL::create_rejected_promise_from_exception(realm, move(exception));
    }

    // 2. If this's relevant global object's associated Document is not allowed to use the "battery"
    //    policy-controlled feature, then reject this.[[BatteryPromise]] with a "NotAllowedError" DOMException.
    // FIXME: Check if Document is allowed to use "battery" policy-controlled feature

    // 3. Otherwise:
    //    1. If this.[[BatteryManager]] is null, then set it to the result of creating a new BatteryManager
    //       in this's relevant realm.
    // FIXME: Create new BatteryManager in this's relevant realm

    //    2. Resolve this.[[BatteryPromise]] with this.[[BatteryManager]].
    // FIXME

    // 4. Return this.[[BatteryPromise]].
    return *m_battery_promise;
}

}


Review comment (suggestion):
Instead of adding new `FIXME` lines, you can just `FIXME` the spec steps themselves. E.g.:

```cpp
    // FIXME: 1. If this.[[BatteryManager]] is null, then set it to the result of creating a new BatteryManager
    //    in this's relevant realm.
```

Code after:
// https://w3c.github.io/battery/#the-getbattery-method
GC::Ref<WebIDL::Promise> Navigator::get_battery()
{
    auto& realm = this->realm();

    // FIXME: 1. If this.[[BatteryPromise]] is null, then set it to a new promise in this's relevant realm.
    if (!m_battery_promise) {
        WebIDL::SimpleException exception {
            WebIDL::SimpleExceptionType::TypeError,
            "Battery Status API is not yet implemented"sv
        };
        m_battery_promise = WebIDL::create_rejected_promise_from_exception(realm, move(exception));
    }

    // FIXME: 2. If this's relevant global object's associated Document is not allowed to use the "battery"
    // policy-controlled feature, then reject this.[[BatteryPromise]] with a "NotAllowedError" DOMException.

    // FIXME: 3. Otherwise:
    //    1. If this.[[BatteryManager]] is null, then set it to the result of creating a new BatteryManager
    //       in this's relevant realm.

    //    2. Resolve this.[[BatteryPromise]] with this.[[BatteryManager]].

    // 4. Return this.[[BatteryPromise]].
    return *m_battery_promise;
}

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibWeb/WebAudio/ControlMessageQueue.cpp (C++)

Code before:
            return;
        if (m_messages.is_empty())
            return;
        m_is_processing = true;
    }

    while (true) {
        Vector<GC::Ref<GC::Function<void()>>> messages_to_process;

        {
            Threading::MutexLocker locker(m_mutex);
            if (m_messages.is_empty()) {
                m_is_processing = false;
                return;
            }
            // Snapshot prevents deadlock from reentrant enqueue
            messages_to_process = move(m_messages);
        }

        // FIXME: Should be called from the rendering thread

        for (auto& message : messages_to_process) {
            message->function()();
        }
    }
}

void ControlMessageQueue::visit_edges(Cell::Visitor& visitor)
{
    Base::visit_edges(visitor);

    Threading::MutexLocker locker(m_mutex);
    visitor.visit(m_messages);
}

}


Review comment (suggestion):
It's stated in the spec that the control thread employs a "traditional event loop, as described in [HTML]" - we should probably implement that?

Since this isn't called yet, what's your plan on implementing the control thread? Seems like the spec calls for two different concepts; the control thread whose job is to execute the messages on the control message queue, and the control message queue whose job it is to maintain the list of messages.

Code after:
/*
 * Copyright (c) 2025, Ben Eidson <b.e.eidson@gmail.com>
 *
 * SPDX-License-Identifier: BSD-2-Clause
 */

#include <LibWeb/WebAudio/ControlMessageQueue.h>
namespace Web::WebAudio {

void ControlMessageQueue::enqueue(ControlMessage message)
{
    Threading::MutexLocker locker(m_mutex);
    m_messages.append(move(message));
}

Vector<ControlMessage> ControlMessageQueue::drain()
{
    Threading::MutexLocker locker(m_mutex);
    return move(m_messages);
}

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibWeb/WebAudio/ControlMessageQueue.h (C/C++)

Code before:
/*
 * Copyright (c) 2025, Trey Shaffer <trey@trsh.dev>
 *
 * SPDX-License-Identifier: BSD-2-Clause
 */

#pragma once

#include <AK/Vector.h>
#include <LibGC/Function.h>
#include <LibGC/Ptr.h>
#include <LibJS/Forward.h>
#include <LibThreading/Mutex.h>
#include <LibWeb/Bindings/PlatformObject.h>
#include <LibWeb/Forward.h>

namespace Web::WebAudio {

// https://webaudio.github.io/web-audio-api/#control-message-queue
class ControlMessageQueue : public Bindings::PlatformObject {
    WEB_PLATFORM_OBJECT(ControlMessageQueue, Bindings::PlatformObject);
    GC_DECLARE_ALLOCATOR(ControlMessageQueue);

public:
    static GC::Ref<ControlMessageQueue> create(JS::Realm&);
    virtual ~ControlMessageQueue() override;

    void enqueue(GC::Ref<GC::Function<void()>>);
    void process_messages();

    bool has_messages() const;

private:
    explicit ControlMessageQueue(JS::Realm&);
    virtual void initialize(JS::Realm&) override;

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

    mutable Threading::Mutex m_mutex;
    Vector<GC::Ref<GC::Function<void()>>> m_messages;
    bool m_is_processing { false };
};

}


Review comment (refactor):
`ControlMessageQueue` is not a platform object but rather a helper object that will be used by the control thread to perform calls eventually picked up by the rendering thread. You can still make it GC-allocated though.

Code after:
/*
 * Copyright (c) 2025, Ben Eidson <b.e.eidson@gmail.com>
 *
 * SPDX-License-Identifier: BSD-2-Clause
 */

#pragma once

#include <AK/Vector.h>
#include <LibThreading/Mutex.h>
#include <LibWeb/Export.h>
#include <LibWeb/WebAudio/ControlMessage.h>

namespace Web::WebAudio {

// https://webaudio.github.io/web-audio-api/#control-message-queue
class WEB_API ControlMessageQueue {

public:
    void enqueue(ControlMessage);   // Called by the control thread.
    Vector<ControlMessage> drain(); // Called by the rendering thread.

private:
    mutable Threading::Mutex m_mutex;
    Vector<ControlMessage> m_messages;
};

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibWeb/WebGL/WebGLBuffer.h (C/C++)

Code before:
#pragma once

#include <LibWeb/WebGL/Types.h>
#include <LibWeb/WebGL/WebGLObject.h>

namespace Web::WebGL {

class WebGLBuffer final : public WebGLObject {
    WEB_PLATFORM_OBJECT(WebGLBuffer, WebGLObject);
    GC_DECLARE_ALLOCATOR(WebGLBuffer);

public:
    static GC::Ref<WebGLBuffer> create(JS::Realm& realm, WebGLRenderingContextBase& context, GLuint handle);

    virtual ~WebGLBuffer();

    void mark_as_index_buffer(bool value) { m_is_index_buffer = value ? TriState::True : TriState::False; }
    TriState is_index_buffer() const { return m_is_index_buffer; }

protected:
    explicit WebGLBuffer(JS::Realm&, WebGLRenderingContextBase&, GLuint handle);

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

private:
    TriState m_is_index_buffer { TriState::Unknown };
};

}


Review comment (bug):
Instead of using this tristate, maybe we could add `Optional<GLenum> m_target` and compare that? It's a useful value to have for future debugging exercises as well.

Code after:
#pragma once

#include <LibWeb/WebGL/Types.h>
#include <LibWeb/WebGL/WebGLObject.h>

namespace Web::WebGL {

class WebGLBuffer final : public WebGLObject {
    WEB_PLATFORM_OBJECT(WebGLBuffer, WebGLObject);
    GC_DECLARE_ALLOCATOR(WebGLBuffer);

public:
    static GC::Ref<WebGLBuffer> create(JS::Realm& realm, WebGLRenderingContextBase& context, GLuint handle);

    virtual ~WebGLBuffer();

    bool is_compatible_with(GLenum target);

protected:
    explicit WebGLBuffer(JS::Realm&, WebGLRenderingContextBase&, GLuint handle);

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

private:
    Optional<GLenum> m_target;
};

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibWeb/WebGL/WebGLBuffer.cpp (C++)

Code before:
{
    return realm.create<WebGLBuffer>(realm, context, handle);
}

WebGLBuffer::WebGLBuffer(JS::Realm& realm, WebGLRenderingContextBase& context, GLuint handle)
    : WebGLObject(realm, context, handle)
{
}

WebGLBuffer::~WebGLBuffer() = default;

void WebGLBuffer::initialize(JS::Realm& realm)
{
    WEB_SET_PROTOTYPE_FOR_INTERFACE(WebGLBuffer);
    Base::initialize(realm);
}

bool WebGLBuffer::is_compatible_with(GLenum target)
{
    // https://registry.khronos.org/webgl/specs/latest/2.0/#5.1
    if (!m_target.has_value()) {
        m_target = target;
        return true;
    }

    return (target == GL_ELEMENT_ARRAY_BUFFER && m_target.value() == GL_ELEMENT_ARRAY_BUFFER) || (target != GL_ELEMENT_ARRAY_BUFFER && m_target.value() != GL_ELEMENT_ARRAY_BUFFER);
}

}


Review comment (suggestion):
I feel this would be easier to read as:

```cpp
if (target === GL_ELEMENT_ARRAY_BUFFER)
    return m_target.value() == GL_ELEMENT_ARRAY_BUFFER;

return m_target.value() != GL_ELEMENT_ARRAY_BUFFER;
```

Code after:
{
    return realm.create<WebGLBuffer>(realm, context, handle);
}

WebGLBuffer::WebGLBuffer(JS::Realm& realm, WebGLRenderingContextBase& context, GLuint handle)
    : WebGLObject(realm, context, handle)
{
}

WebGLBuffer::~WebGLBuffer() = default;

void WebGLBuffer::initialize(JS::Realm& realm)
{
    WEB_SET_PROTOTYPE_FOR_INTERFACE(WebGLBuffer);
    Base::initialize(realm);
}

bool WebGLBuffer::is_compatible_with(GLenum target)
{
    // https://registry.khronos.org/webgl/specs/latest/2.0/#5.1
    if (!m_target.has_value()) {
        m_target = target;
        return true;
    }

    if (target == GL_ELEMENT_ARRAY_BUFFER)
        return m_target.value() == GL_ELEMENT_ARRAY_BUFFER;

    return m_target.value() != GL_ELEMENT_ARRAY_BUFFER;
}

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

Code before:
/*
 * Copyright (c) 2021, the SerenityOS developers.
 * Copyright (c) 2021-2025, Sam Atkins <sam@ladybird.org>
 * Copyright (c) 2022, Andreas Kling <andreas@ladybird.org>
 *
 * SPDX-License-Identifier: BSD-2-Clause
 */

#pragma once

#include <LibWeb/CSS/CSSRule.h>
#include <LibWeb/CSS/CSSStyleSheet.h>
#include <LibWeb/CSS/Supports.h>
#include <LibWeb/CSS/URL.h>
#include <LibWeb/DOM/DocumentLoadEventDelayer.h>
#include <LibWeb/Export.h>

namespace Web::CSS {

class WEB_API CSSImportRule final
    : public CSSRule {
    WEB_PLATFORM_OBJECT(CSSImportRule, CSSRule);
    GC_DECLARE_ALLOCATOR(CSSImportRule);

public:
    [[nodiscard]] static GC::Ref<CSSImportRule> create(JS::Realm&, URL, GC::Ptr<DOM::Document>, RefPtr<Supports>, Vector<NonnullRefPtr<MediaQuery>>);

    virtual ~CSSImportRule() = default;

    URL const& url() const { return m_url; }
    String href() const { return m_url.url(); }

    CSSStyleSheet* loaded_style_sheet() { return m_style_sheet; }
    CSSStyleSheet const* loaded_style_sheet() const { return m_style_sheet; }
    GC::Ptr<MediaList> media() const;
    CSSStyleSheet* style_sheet_for_bindings() { return m_style_sheet; }

    Optional<String> supports_text() const;

Review comment (suggestion):
I think `Supports` should be fine to use as a forward declaration in this header if we outline the destructor? Like you did with `ImageBitmap`

Code after:
/*
 * Copyright (c) 2021, the SerenityOS developers.
 * Copyright (c) 2021-2025, Sam Atkins <sam@ladybird.org>
 * Copyright (c) 2022, Andreas Kling <andreas@ladybird.org>
 *
 * SPDX-License-Identifier: BSD-2-Clause
 */

#pragma once

#include <LibWeb/CSS/CSSRule.h>
#include <LibWeb/CSS/CSSStyleSheet.h>
#include <LibWeb/CSS/URL.h>
#include <LibWeb/DOM/DocumentLoadEventDelayer.h>
#include <LibWeb/Export.h>
#include <LibWeb/Forward.h>

namespace Web::CSS {

class WEB_API CSSImportRule final
    : public CSSRule {
    WEB_PLATFORM_OBJECT(CSSImportRule, CSSRule);
    GC_DECLARE_ALLOCATOR(CSSImportRule);

public:
    [[nodiscard]] static GC::Ref<CSSImportRule> create(JS::Realm&, URL, GC::Ptr<DOM::Document>, RefPtr<Supports>, Vector<NonnullRefPtr<MediaQuery>>);

    virtual ~CSSImportRule();

    URL const& url() const { return m_url; }
    String href() const { return m_url.url(); }

    CSSStyleSheet* loaded_style_sheet() { return m_style_sheet; }
    CSSStyleSheet const* loaded_style_sheet() const { return m_style_sheet; }
    GC::Ptr<MediaList> media() const;
    CSSStyleSheet* style_sheet_for_bindings() { return m_style_sheet; }

    Optional<String> supports_text() const;
Repository: LadybirdBrowser/ladybird
File: Libraries/LibWeb/HTML/Timer.h (C/C++)

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

#pragma once

#include <AK/Forward.h>
#include <AK/Function.h>
#include <AK/WeakPtr.h>
#include <LibCore/Forward.h>
#include <LibGC/Function.h>
#include <LibGC/Ptr.h>
#include <LibJS/Heap/Cell.h>
#include <LibWeb/Forward.h>

namespace Web::HTML {

class Timer final : public JS::Cell {
    GC_CELL(Timer, JS::Cell);
    GC_DECLARE_ALLOCATOR(Timer);

public:
    static GC::Ref<Timer> create(JS::Object&, i32 milliseconds, Function<void()> callback, i32 id);
    virtual ~Timer() override;

    void start();
    void stop();

private:
    Timer(JS::Object& window, i32 milliseconds, GC::Ref<GC::Function<void()>> callback, i32 id);

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

    RefPtr<Core::Timer> m_timer;
    GC::Ref<JS::Object> m_window_or_worker_global_scope;
    GC::Ref<GC::Function<void()>> m_callback;
    i32 m_id { 0 };
};

}


Review comment (suggestion):
On windows the default enum is an int. 
```suggestion
    enum class Repeating : u8 {
```

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

#pragma once

#include <AK/Forward.h>
#include <AK/Function.h>
#include <AK/WeakPtr.h>
#include <LibCore/Forward.h>
#include <LibGC/Function.h>
#include <LibGC/Ptr.h>
#include <LibJS/Heap/Cell.h>
#include <LibWeb/Forward.h>

namespace Web::HTML {

class Timer final : public JS::Cell {
    GC_CELL(Timer, JS::Cell);
    GC_DECLARE_ALLOCATOR(Timer);

public:
    enum class Repeating {
        No,
        Yes,
    };

    static GC::Ref<Timer> create(JS::Object&, i32 milliseconds, Function<void()> callback, i32 id, Repeating);
    virtual ~Timer() override;

    void start();
    void stop();

    void set_callback(Function<void()>);

private:
    Timer(JS::Object& window, i32 milliseconds, Function<void()> callback, i32 id, Repeating);

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

    RefPtr<Core::Timer> m_timer;
    GC::Ref<JS::Object> m_window_or_worker_global_scope;
    i32 m_id { 0 };
};

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

Code before:
    EventLoopManager::the().unregister_notifier(notifier);
}

void EventLoop::wake()
{
    m_impl->wake();
}

void EventLoop::deferred_invoke(Function<void()> invokee)
{
    m_impl->deferred_invoke(move(invokee));
}

void deferred_invoke(Function<void()> invokee)
{
    EventLoop::current().deferred_invoke(move(invokee));
}

EventLoopWeak::EventLoopWeak(EventLoop& event_loop)
    : m_event_loop(&event_loop)
{
}

void EventLoopWeak::revoke()
{
    m_lock.lock_write();
    m_event_loop = nullptr;
    m_lock.unlock();
}

EventLoopLockedReference EventLoopWeak::take()
{
    return EventLoopLockedReference(*this);
}

EventLoopLockedReference::EventLoopLockedReference(EventLoopWeak& event_loop_weak)
{
    event_loop_weak.m_lock.lock_read();
    m_event_loop_weak = &event_loop_weak;
}

EventLoopLockedReference::~EventLoopLockedReference()
{
    m_event_loop_weak->m_lock.unlock();
}

bool EventLoopLockedReference::is_alive() const
{
    return m_event_loop_weak->m_event_loop != nullptr;
}


Review comment (suggestion):
use RWLockLocker instead:
```cpp
Threading::RWLockLocker<Threading::LockMode::Write> lock(m_lock);
```

Code after:
    EventLoopManager::the().unregister_notifier(notifier);
}

void EventLoop::wake()
{
    m_impl->wake();
}

void EventLoop::deferred_invoke(Function<void()> invokee)
{
    m_impl->deferred_invoke(move(invokee));
}

void deferred_invoke(Function<void()> invokee)
{
    EventLoop::current().deferred_invoke(move(invokee));
}

WeakEventLoopReference::WeakEventLoopReference(EventLoop& event_loop)
    : m_event_loop(&event_loop)
{
}

void WeakEventLoopReference::revoke()
{
    Threading::RWLockLocker<Threading::LockMode::Read> locker { m_lock };
    m_event_loop = nullptr;
}

StrongEventLoopReference WeakEventLoopReference::take()
{
    return StrongEventLoopReference(*this);
}

StrongEventLoopReference::StrongEventLoopReference(WeakEventLoopReference& event_loop_weak)
{
    event_loop_weak.m_lock.lock_read();
    m_event_loop_weak = &event_loop_weak;
}

StrongEventLoopReference::~StrongEventLoopReference()
{
    m_event_loop_weak->m_lock.unlock();
}

bool StrongEventLoopReference::is_alive() const
{
    return m_event_loop_weak->m_event_loop != nullptr;
}

StrongEventLoopReference::operator bool() const
Repository: LadybirdBrowser/ladybird
File: Libraries/LibCore/EventLoopImplementation.cpp (C++)

Code before:
 */

#include <AK/NonnullOwnPtr.h>
#include <LibCore/Event.h>
#include <LibCore/EventLoopImplementation.h>
#include <LibCore/ThreadEventQueue.h>
#ifdef AK_OS_WINDOWS
#    include <LibCore/EventLoopImplementationWindows.h>
#else
#    include <LibCore/EventLoopImplementationUnix.h>
#endif

namespace Core {

EventLoopImplementation::EventLoopImplementation()
    : m_thread_event_queue(ThreadEventQueue::current())
{
}

EventLoopImplementation::~EventLoopImplementation() = default;

void EventLoopImplementation::deferred_invoke(Function<void()>&& invokee)
{
    m_thread_event_queue.deferred_invoke(move(invokee));
    wake();
}

static EventLoopManager* s_event_loop_manager = nullptr;
EventLoopManager& EventLoopManager::the()
{
    if (!s_event_loop_manager)
        s_event_loop_manager = new EventLoopManagerPlatform;
    return *s_event_loop_manager;
}

void EventLoopManager::install(Core::EventLoopManager& manager)
{
    VERIFY(!s_event_loop_manager);
    s_event_loop_manager = &manager;
}

EventLoopManager::EventLoopManager() = default;

EventLoopManager::~EventLoopManager() = default;

}


Review comment (bug):
I have not looked deeply in the event loop before, but is it guaranteed that we don't hit poll again before running any of the deferred_invoke tasks?

Since otherwise I suppose we might also deadlock no other fd's in the poll loop have any event on them before we hit the poll loop, and the deferred tasks are not run by that point we get stuck at this point: https://github.com/LadybirdBrowser/ladybird/blob/89dbdd3411a3037ec31dcd96c15a47f062c7f48f/Libraries/LibCore/EventLoopImplementationUnix.cpp#L353 not ever running the deferred tasks

Seems like we might hit that if you deferred_invoke and then immediately pump: https://github.com/LadybirdBrowser/ladybird/blob/89dbdd3411a3037ec31dcd96c15a47f062c7f48f/Libraries/LibCore/EventLoopImplementationUnix.cpp#L307

Code after:
 */

#include <AK/NonnullOwnPtr.h>
#include <LibCore/Event.h>
#include <LibCore/EventLoopImplementation.h>
#include <LibCore/ThreadEventQueue.h>
#ifdef AK_OS_WINDOWS
#    include <LibCore/EventLoopImplementationWindows.h>
#else
#    include <LibCore/EventLoopImplementationUnix.h>
#endif

namespace Core {

EventLoopImplementation::EventLoopImplementation()
    : m_thread_event_queue(ThreadEventQueue::current())
{
}

EventLoopImplementation::~EventLoopImplementation() = default;

void EventLoopImplementation::deferred_invoke(Function<void()>&& invokee)
{
    m_thread_event_queue.deferred_invoke(move(invokee));
    if (&m_thread_event_queue != ThreadEventQueue::current_or_null())
        wake();
}

static EventLoopManager* s_event_loop_manager = nullptr;
EventLoopManager& EventLoopManager::the()
{
    if (!s_event_loop_manager)
        s_event_loop_manager = new EventLoopManagerPlatform;
    return *s_event_loop_manager;
}

void EventLoopManager::install(Core::EventLoopManager& manager)
{
    VERIFY(!s_event_loop_manager);
    s_event_loop_manager = &manager;
}

EventLoopManager::EventLoopManager() = default;

EventLoopManager::~EventLoopManager() = default;

}
Repository: LadybirdBrowser/ladybird
File: UI/AppKit/Interface/TabController.mm (Objective-C++)

Code before:

    if (selector == @selector(moveUp:)) {
        if ([self.autocomplete selectPreviousSuggestion])
            return YES;
    }

    if (selector != @selector(insertNewline:)) {
        return NO;
    }

    auto location = [self.autocomplete selectedSuggestion].value_or_lazy_evaluated([&]() {
        return Ladybird::ns_string_to_string([[text_view textStorage] string]);
    });

    [self navigateToLocation:move(location)];
    return YES;
}

- (void)controlTextDidEndEditing:(NSNotification*)notification
{
    // Check if editing ended due to pressing Enter (navigation)
    auto* user_info = [notification userInfo];
    auto text_movement = [[user_info objectForKey:@"NSTextMovement"] intValue];

    // If the user pressed Enter, navigation is handled by doCommandBySelector:
    // For all other cases (Escape, clicking outside, etc.), restore the URL
    if (text_movement != NSTextMovementReturn) {
        auto const& url = [[[self tab] web_view] view].url();
        [self setLocationFieldText:url.serialize()];
    }
}

- (void)controlTextDidChange:(NSNotification*)notification
{
    auto* location_search_field = (LocationSearchField*)[self.location_toolbar_item view];

    auto url_string = Ladybird::ns_string_to_string([location_search_field stringValue]);
    m_autocomplete->query_autocomplete_engine(move(url_string));
}

#pragma mark - AutocompleteObserver

- (void)onSelectedSuggestion:(String)suggestion
{
    [self navigateToLocation:move(suggestion)];
}

@end


Review comment (suggestion):
I think we can leave this function as it was? `setLocationFieldText` just attempts to highlight individual parts of URL. If the user entered a valid URL, we might as well highlight it? Firefox and Chrome seem to behave that way at least.

Code after:
        if ([self.autocomplete selectPreviousSuggestion])
            return YES;
    }

    if (selector != @selector(insertNewline:)) {
        return NO;
    }

    auto location = [self.autocomplete selectedSuggestion].value_or_lazy_evaluated([&]() {
        return Ladybird::ns_string_to_string([[text_view textStorage] string]);
    });

    [self navigateToLocation:move(location)];
    return YES;
}

- (void)controlTextDidEndEditing:(NSNotification*)notification
{
    auto* location_search_field = (LocationSearchField*)[self.location_toolbar_item view];

    auto url_string = Ladybird::ns_string_to_string([location_search_field stringValue]);
    [self setLocationFieldText:url_string];
}

- (void)controlTextDidChange:(NSNotification*)notification
{
    auto* location_search_field = (LocationSearchField*)[self.location_toolbar_item view];

    auto url_string = Ladybird::ns_string_to_string([location_search_field stringValue]);
    m_autocomplete->query_autocomplete_engine(move(url_string));
}

#pragma mark - AutocompleteObserver

- (void)onSelectedSuggestion:(String)suggestion
{
    [self navigateToLocation:move(suggestion)];
}

@end