LibWebView: Create a dedicated settings observer for the Application

We had a bit of an awkward setup where we want the Application to be a
SettingsObserver, but neither the Settings object nor the Application
itself was fully initialized by the time the observer was created. So
we invented a deferred observer initializer specifically for the
Application.

Instead, let's just create a dedicated observer subclass that is owned
by the Application. We can then create it once we have the singleton
Application appropriately set up.

Libraries/LibWebView/Application.cpp

@@ -26,13 +26,32 @@ namespace WebView {
 
 Application* Application::s_the = nullptr;
 
+struct ApplicationSettingsObserver : public SettingsObserver {
+    virtual void dns_settings_changed() override
+    {
+        Application::settings().dns_settings().visit(
+            [](SystemDNS) {
+                Application::request_server_client().async_set_use_system_dns();
+            },
+            [](DNSOverTLS const& dns_over_tls) {
+                dbgln("Setting DNS server to {}:{} with TLS", dns_over_tls.server_address, dns_over_tls.port);
+                Application::request_server_client().async_set_dns_server(dns_over_tls.server_address, dns_over_tls.port, true);
+            },
+            [](DNSOverUDP const& dns_over_udp) {
+                dbgln("Setting DNS server to {}:{}", dns_over_udp.server_address, dns_over_udp.port);
+                Application::request_server_client().async_set_dns_server(dns_over_udp.server_address, dns_over_udp.port, false);
+            });
+    }
+};
+
 Application::Application()
-    : SettingsObserver(AddToObservers::No) // Application::the() is not set yet.
-    , m_settings(Settings::create({}))
+    : m_settings(Settings::create({}))
 {
     VERIFY(!s_the);
     s_the = this;
 
+    m_settings_observer = make<ApplicationSettingsObserver>();
+
     // No need to monitor the system time zone if the TZ environment variable is set, as it overrides system preferences.
     if (!Core::Environment::has("TZ"sv)) {
         if (auto time_zone_watcher = Core::TimeZoneWatcher::create(); time_zone_watcher.is_error()) {
@@ -52,13 +71,13 @@ Application::Application()
     m_process_manager.on_process_exited = [this](Process&& process) {
         process_did_exit(move(process));
     };
-
-    SettingsObserver::complete_delayed_registration();
 }
 
 Application::~Application()
 {
-    SettingsObserver::complete_delayed_unregistration();
+    // Explicitly delete the settings observer first, as the observer destructor will refer to Application::the().
+    m_settings_observer.clear();
+
     s_the = nullptr;
 }
 
@@ -175,9 +194,6 @@ void Application::initialize(Main::Arguments const& arguments)
         .devtools_port = devtools_port,
     };
 
-    if (m_browser_options.dns_settings.has_value())
-        m_settings.set_dns_settings(m_browser_options.dns_settings.value(), true);
-
     if (webdriver_content_ipc_path.has_value())
         m_browser_options.webdriver_content_ipc_path = *webdriver_content_ipc_path;
 
@@ -268,6 +284,10 @@ ErrorOr<void> Application::launch_request_server()
 {
     // FIXME: Create an abstraction to re-spawn the RequestServer and re-hook up its client hooks to each tab on crash
     m_request_server_client = TRY(launch_request_server_process());
+
+    if (m_browser_options.dns_settings.has_value())
+        m_settings.set_dns_settings(m_browser_options.dns_settings.value(), true);
+
     return {};
 }
 
@@ -717,23 +737,4 @@ void Application::request_console_messages(DevTools::TabDescription const& descr
     view->js_console_request_messages(start_index);
 }
 
-void Application::dns_settings_changed()
-{
-    if (!m_request_server_client)
-        return;
-    auto dns_settings = settings().dns_settings();
-    auto& rs_client = *m_request_server_client;
-    dns_settings.visit(
-        [&](SystemDNS) {
-            rs_client.async_set_use_system_dns();
-        },
-        [&](DNSOverTLS const& dns_over_tls) {
-            dbgln("Setting DNS server to {}:{} with TLS", dns_over_tls.server_address, dns_over_tls.port);
-            rs_client.async_set_dns_server(dns_over_tls.server_address, dns_over_tls.port, true);
-        },
-        [&](DNSOverUDP const& dns_over_udp) {
-            dbgln("Setting DNS server to {}:{}", dns_over_udp.server_address, dns_over_udp.port);
-            rs_client.async_set_dns_server(dns_over_udp.server_address, dns_over_udp.port, false);
-        });
-}
 }

Libraries/LibWebView/Application.h

@@ -26,8 +26,9 @@
 
 namespace WebView {
 
-class Application : public DevTools::DevToolsDelegate
-    , public SettingsObserver {
+struct ApplicationSettingsObserver;
+
+class Application : public DevTools::DevToolsDelegate {
     AK_MAKE_NONCOPYABLE(Application);
 
 public:
@@ -129,12 +130,10 @@ private:
     virtual void stop_listening_for_console_messages(DevTools::TabDescription const&) const override;
     virtual void request_console_messages(DevTools::TabDescription const&, i32) const override;
 
-    // ^SettingsObserver
-    virtual void dns_settings_changed() override;
-
     static Application* s_the;
 
     Settings m_settings;
+    OwnPtr<ApplicationSettingsObserver> m_settings_observer;
 
     BrowserOptions m_browser_options;
     WebContentOptions m_web_content_options;
@@ -156,6 +155,7 @@ private:
 
     OwnPtr<DevTools::DevToolsServer> m_devtools;
 } SWIFT_IMMORTAL_REFERENCE;
+
 }
 
 #define WEB_VIEW_APPLICATION(ApplicationType)                                \

Libraries/LibWebView/Settings.cpp

@@ -142,34 +142,6 @@ Settings Settings::create(Badge<Application>)
     return settings;
 }
 
-DNSSettings Settings::parse_dns_settings(JsonValue const& dns_settings)
-{
-    if (!dns_settings.is_object())
-        return SystemDNS {};
-
-    auto& dns_settings_object = dns_settings.as_object();
-    auto mode = dns_settings_object.get_string("mode"sv);
-    if (mode.has_value()) {
-        if (*mode == "system"sv)
-            return SystemDNS {};
-        if (*mode == "custom"sv) {
-            auto server = dns_settings_object.get_string("server"sv);
-            auto port = dns_settings_object.get_u16("port"sv);
-            auto type = dns_settings_object.get_string("type"sv);
-
-            if (server.has_value() && port.has_value() && type.has_value()) {
-                if (*type == "tls"sv)
-                    return DNSOverTLS { .server_address = server->to_byte_string(), .port = *port };
-                if (*type == "udp"sv)
-                    return DNSOverUDP { .server_address = server->to_byte_string(), .port = *port };
-            }
-        }
-    }
-
-    dbgln("Invalid DNS settings in parse_dns_settings, falling back to system DNS");
-    return SystemDNS {};
-}
-
 Settings::Settings(ByteString settings_path)
     : m_settings_path(move(settings_path))
     , m_new_tab_page_url(URL::about_newtab())
@@ -452,6 +424,35 @@ void Settings::set_do_not_track(DoNotTrack do_not_track)
         observer.do_not_track_changed();
 }
 
+DNSSettings Settings::parse_dns_settings(JsonValue const& dns_settings)
+{
+    if (!dns_settings.is_object())
+        return SystemDNS {};
+
+    auto const& dns_settings_object = dns_settings.as_object();
+
+    if (auto mode = dns_settings_object.get_string("mode"sv); mode.has_value()) {
+        if (*mode == "system"sv)
+            return SystemDNS {};
+
+        if (*mode == "custom"sv) {
+            auto server = dns_settings_object.get_string("server"sv);
+            auto port = dns_settings_object.get_u16("port"sv);
+            auto type = dns_settings_object.get_string("type"sv);
+
+            if (server.has_value() && port.has_value() && type.has_value()) {
+                if (*type == "tls"sv)
+                    return DNSOverTLS { .server_address = server->to_byte_string(), .port = *port };
+                if (*type == "udp"sv)
+                    return DNSOverUDP { .server_address = server->to_byte_string(), .port = *port };
+            }
+        }
+    }
+
+    dbgln("Invalid DNS settings in parse_dns_settings, falling back to system DNS");
+    return SystemDNS {};
+}
+
 void Settings::set_dns_settings(DNSSettings const& dns_settings, bool override_by_command_line)
 {
     m_dns_settings = dns_settings;
@@ -485,29 +486,13 @@ void Settings::remove_observer(Badge<SettingsObserver>, SettingsObserver& observ
     VERIFY(was_removed);
 }
 
-SettingsObserver::SettingsObserver(AddToObservers add_to_observers)
+SettingsObserver::SettingsObserver()
 {
-    if (add_to_observers == AddToObservers::Yes)
-        Settings::add_observer({}, *this);
-    else
-        m_registration_was_delayed = true;
+    Settings::add_observer({}, *this);
 }
 
 SettingsObserver::~SettingsObserver()
 {
-    if (!m_registration_was_delayed)
-        Settings::remove_observer({}, *this);
-}
-
-void SettingsObserver::complete_delayed_registration()
-{
-    VERIFY(m_registration_was_delayed);
-    Settings::add_observer({}, *this);
-}
-
-void SettingsObserver::complete_delayed_unregistration()
-{
-    VERIFY(m_registration_was_delayed);
     Settings::remove_observer({}, *this);
 }
 

Libraries/LibWebView/Settings.h

@@ -32,11 +32,7 @@ enum class DoNotTrack {
 
 class SettingsObserver {
 public:
-    enum class AddToObservers {
-        No,
-        Yes,
-    };
-    explicit SettingsObserver(AddToObservers = AddToObservers::Yes);
+    explicit SettingsObserver();
     virtual ~SettingsObserver();
 
     virtual void new_tab_page_url_changed() { }
@@ -46,13 +42,6 @@ public:
     virtual void autoplay_settings_changed() { }
     virtual void do_not_track_changed() { }
     virtual void dns_settings_changed() { }
-
-protected:
-    void complete_delayed_registration();
-    void complete_delayed_unregistration();
-
-private:
-    bool m_registration_was_delayed { false };
 };
 
 class Settings {