LibWeb/HTML: Update worker construction spec steps

This is largely editorial. One behaviour change is that events are now
sent from a global task on the DOMManipulation task source.

Somewhat awkwardly, the spec refers to `this` before the Worker exists.
As it's for getting the relevant global object / settings object, I've
had to work around that.

Corresponds to:
whatwg/html@917c2f6

Author: AtkinsSJ

Libraries/LibWeb/HTML/SharedWorker.cpp

@@ -27,7 +27,7 @@ GC_DEFINE_ALLOCATOR(SharedWorker);
 // https://html.spec.whatwg.org/multipage/workers.html#dom-sharedworker
 WebIDL::ExceptionOr<GC::Ref<SharedWorker>> SharedWorker::construct_impl(JS::Realm& realm, TrustedTypes::TrustedScriptURLOrString const& script_url, Variant<String, WorkerOptions>& options_value)
 {
-    // 1. Let compliantScriptURL be the result of invoking the Get Trusted Type compliant string algorithm with
+    // 1. Let compliantScriptURL be the result of invoking the get trusted type compliant string algorithm with
     //    TrustedScriptURL, this's relevant global object, scriptURL, "SharedWorker constructor", and "script".
     auto const compliant_script_url = TRY(get_trusted_type_compliant_string(
         TrustedTypes::TrustedTypeName::TrustedScriptURL,
@@ -36,8 +36,8 @@ WebIDL::ExceptionOr<GC::Ref<SharedWorker>> SharedWorker::construct_impl(JS::Real
         TrustedTypes::InjectionSink::SharedWorker_constructor,
         TrustedTypes::Script.to_string()));
 
-    // 2. If options is a DOMString, set options to a new WorkerOptions dictionary whose name member is set to the value
-    //    of options and whose other members are set to their default values.
+    // 2. If options is a DOMString, set options to a new WorkerOptions dictionary whose name member is set to the
+    //    value of options and whose other members are set to their default values.
     auto options = options_value.visit(
         [&](String& options) {
             return WorkerOptions { .name = move(options) };
@@ -46,122 +46,125 @@ WebIDL::ExceptionOr<GC::Ref<SharedWorker>> SharedWorker::construct_impl(JS::Real
             return move(options);
         });
 
-    // 3. Let outside settings be the current settings object.
+    // 3. Let outside settings be this's relevant settings object.
+    // FIXME: We don't have a `this` yet, so use the current principal settings object, as the previous spec did.
     auto& outside_settings = current_principal_settings_object();
 
-    // 4. Let urlRecord be the result of encoding-parsing a URL given compliantScriptURL, relative to outside settings.
+    // 4. Let urlRecord be the result of encoding-parsing a URL given compliantScriptURL, relative to outsideSettings.
     auto url = outside_settings.encoding_parse_url(compliant_script_url.to_utf8_but_should_be_ported_to_utf16());
 
     // 5. If urlRecord is failure, then throw a "SyntaxError" DOMException.
     if (!url.has_value())
         return WebIDL::SyntaxError::create(realm, "SharedWorker constructed with invalid URL"_utf16);
 
-    // 7. Let outside port be a new MessagePort in outside settings's realm.
-    // NOTE: We do this first so that we can store the port as a GC::Ref.
+    // 6. Let outsidePort be a new MessagePort in outsideSettings's realm.
     auto outside_port = MessagePort::create(outside_settings.realm());
 
-    // 6. Let worker be a new SharedWorker object.
-    // 8. Assign outside port to the port attribute of worker.
+    // 10. Let worker be this.
+    // AD-HOC: We do this first so that we can use `this`.
+
+    // 7. Set this's port to outsidePort.
     auto worker = realm.create<SharedWorker>(realm, url.release_value(), options, outside_port);
 
-    // 9. Let callerIsSecureContext be true if outside settings is a secure context; otherwise, false.
+    // 8. Let callerIsSecureContext be true if outside settings is a secure context; otherwise, false.
     auto caller_is_secure_context = HTML::is_secure_context(outside_settings);
 
-    // 10. Let outside storage key be the result of running obtain a storage key for non-storage purposes given outside settings.
+    // 9. Let outsideStorageKey be the result of running obtain a storage key for non-storage purposes given outsideSettings.
     auto outside_storage_key = StorageAPI::obtain_a_storage_key_for_non_storage_purposes(outside_settings);
 
+    // 10. Let worker be this.
+    // NB: This is done earlier.
+
     // 11. Enqueue the following steps to the shared worker manager:
     // FIXME: "A user agent has an associated shared worker manager which is the result of starting a new parallel queue."
     //        We just use the singular global event loop for now.
     Platform::EventLoopPlugin::the().deferred_invoke(GC::create_function(realm.heap(), [worker, outside_port, &outside_settings, caller_is_secure_context, outside_storage_key = move(outside_storage_key)]() mutable {
-        // 1. Let worker global scope be null.
+        // 1. Let workerGlobalScope be null.
         GC::Ptr<SharedWorkerGlobalScope> worker_global_scope;
 
         // 2. For each scope in the list of all SharedWorkerGlobalScope objects:
         for (auto& scope : all_shared_worker_global_scopes()) {
-            // 1. Let worker storage key be the result of running obtain a storage key for non-storage purposes given
+            // 1. Let workerStorageKey be the result of running obtain a storage key for non-storage purposes given
             //    scope's relevant settings object.
             auto worker_storage_key = StorageAPI::obtain_a_storage_key_for_non_storage_purposes(HTML::relevant_settings_object(scope));
 
             // 2. If all of the following are true:
             if (
-                // * worker storage key equals outside storage key;
+                // * workerStorageKey equals outsideStorageKey;
                 worker_storage_key == outside_storage_key
 
                 // * scope's closing flag is false;
                 && !scope->is_closing()
 
-                // * scope's constructor url equals urlRecord; and
+                // * scope's constructor URL equals urlRecord; and
                 && scope->url() == worker->m_script_url
 
-                // * scope's name equals the value of options's name member,
+                // * scope's name equals options["name"],
                 && scope->name() == worker->m_options.name)
             // then:
             {
-                // 1. Set worker global scope to scope.
+                // 1. Set workerGlobalScope to scope.
                 worker_global_scope = scope;
 
                 // 2. Break.
                 break;
             }
         }
 
-        // FIXME: 3. If worker global scope is not null, but the user agent has been configured to disallow communication
-        //           between the worker represented by the worker global scope and the scripts whose settings object is outside
-        //           settings, then set worker global scope to null.
-        // FIXME: 4. If worker global scope is not null, then check if worker global scope's type and credentials match the
-        //           options values. If not, queue a task to fire an event named error and abort these steps.
+        // FIXME: 3. If workerGlobalScope is not null, but the user agent has been configured to disallow communication between the worker represented by the workerGlobalScope and the scripts whose settings object is outsideSettings, then set workerGlobalScope to null.
+        // FIXME: 4. If workerGlobalScope is not null, and any of the following are true: ...
 
-        // 5. If worker global scope is not null, then run these subsubsteps:
+        // 5. If workerGlobalScope is not null:
         if (worker_global_scope) {
-            // 1. Let settings object be the relevant settings object for worker global scope.
-            auto& settings_object = HTML::relevant_settings_object(*worker_global_scope);
+            // 1. Let insideSettings be workerGlobalScope's relevant settings object.
+            auto& inside_settings = relevant_settings_object(*worker_global_scope);
 
-            // 2. Let workerIsSecureContext be true if settings object is a secure context; otherwise, false.
-            auto worker_is_secure_context = HTML::is_secure_context(settings_object);
+            // 2. Let workerIsSecureContext be true if insideSettings is a secure context; otherwise, false.
+            auto worker_is_secure_context = is_secure_context(inside_settings);
 
-            // 3. If workerIsSecureContext is not callerIsSecureContext, then queue a task to fire an event named error
-            //    at worker and abort these steps. [SECURE-CONTEXTS]
+            // 3. If workerIsSecureContext is not callerIsSecureContext:
             if (worker_is_secure_context != caller_is_secure_context) {
-                queue_a_task(HTML::Task::Source::Unspecified, nullptr, nullptr, GC::create_function(worker->heap(), [worker]() {
-                    worker->dispatch_event(DOM::Event::create(worker->realm(), HTML::EventNames::error));
+                // 1. Queue a global task on the DOM manipulation task source given worker's relevant global object to fire an event named error at worker.
+                queue_global_task(Task::Source::DOMManipulation, relevant_global_object(worker), GC::create_function(worker->heap(), [worker]() {
+                    worker->dispatch_event(DOM::Event::create(worker->realm(), EventNames::error));
                 }));
 
+                // 2. Abort these steps.
                 return;
             }
 
-            // FIXME: 4. Associate worker with worker global scope.
+            // FIXME: 4. Associate worker with workerGlobalScope.
 
-            // 5. Let inside port be a new MessagePort in settings object's realm.
-            auto inside_port = HTML::MessagePort::create(settings_object.realm());
+            // 5. Let insidePort be a new MessagePort in insideSettings's realm.
+            auto inside_port = HTML::MessagePort::create(inside_settings.realm());
 
-            // 6. Entangle outside port and inside port.
+            // 6. Entangle outsidePort and insidePort.
             outside_port->entangle_with(inside_port);
 
-            // 7. Queue a task, using the DOM manipulation task source, to fire an event named connect at worker global
-            //    scope, using MessageEvent, with the data attribute initialized to the empty string, the ports attribute
-            //    initialized to a new frozen array containing only inside port, and the source attribute initialized to
-            //    inside port.
-            queue_a_task(HTML::Task::Source::DOMManipulation, nullptr, nullptr, GC::create_function(worker->heap(), [worker_global_scope, inside_port]() {
+            // 7. Queue a global task on the DOM manipulation task source given workerGlobalScope to fire an event
+            //   named connect at workerGlobalScope, using MessageEvent, with the data attribute initialized to the
+            //   empty string, the ports attribute initialized to a new frozen array containing only insidePort, and
+            //   the source attribute initialized to insidePort.
+            queue_global_task(Task::Source::DOMManipulation, *worker_global_scope, GC::create_function(worker->heap(), [worker_global_scope, inside_port]() {
                 auto& realm = worker_global_scope->realm();
 
                 MessageEventInit init;
                 init.data = JS::PrimitiveString::create(realm.vm(), String {});
                 init.ports.append(inside_port);
                 init.source = inside_port;
 
-                worker_global_scope->dispatch_event(MessageEvent::create(realm, HTML::EventNames::connect, init));
+                worker_global_scope->dispatch_event(MessageEvent::create(realm, EventNames::connect, init));
             }));
 
-            // FIXME: 8. Append the relevant owner to add given outside settings to worker global scope's owner set.
+            // FIXME: 8. Append the relevant owner to add given outsideSettings to workerGlobalScope's owner set.
+
         }
-        // 6. Otherwise, in parallel, run a worker given worker, urlRecord, outside settings, outside port, and options.
+        // 6. Otherwise, in parallel, run a worker given worker, urlRecord, outsideSettings, outsidePort, and options.
         else {
             run_a_worker(worker, worker->m_script_url, outside_settings, outside_port, worker->m_options);
         }
     }));
 
-    // 12. Return worker.
     return worker;
 }
 

Libraries/LibWeb/HTML/SharedWorker.h

@@ -27,7 +27,12 @@ class SharedWorker final
 
     virtual ~SharedWorker();
 
-    GC::Ref<MessagePort> port() { return m_port; }
+    // https://html.spec.whatwg.org/multipage/workers.html#dom-sharedworker-port
+    GC::Ref<MessagePort> port()
+    {
+        // The port getter steps are to return this's port.
+        return m_port;
+    }
 
     void set_agent(WorkerAgentParent& agent) { m_agent = agent; }
 
@@ -42,7 +47,11 @@ class SharedWorker final
 
     URL::URL m_script_url;
     WorkerOptions m_options;
+
+    // Each SharedWorker has a port, a MessagePort set when the object is created.
+    // https://html.spec.whatwg.org/multipage/workers.html#concept-sharedworker-port
     GC::Ref<MessagePort> m_port;
+
     GC::Ptr<WorkerAgentParent> m_agent;
 };
 

Libraries/LibWeb/HTML/Worker.cpp

@@ -55,6 +55,7 @@ WebIDL::ExceptionOr<GC::Ref<Worker>> Worker::create(TrustedTypes::TrustedScriptU
 
     // 1. Let compliantScriptURL be the result of invoking the Get Trusted Type compliant string algorithm with
     //    TrustedScriptURL, this's relevant global object, scriptURL, "Worker constructor", and "script".
+    // FIXME: We don't have a `this` yet, so use the document.
     auto const compliant_script_url = TRY(TrustedTypes::get_trusted_type_compliant_string(
         TrustedTypes::TrustedTypeName::TrustedScriptURL,
         HTML::relevant_global_object(document),
@@ -64,35 +65,39 @@ WebIDL::ExceptionOr<GC::Ref<Worker>> Worker::create(TrustedTypes::TrustedScriptU
 
     dbgln_if(WEB_WORKER_DEBUG, "WebWorker: Creating worker with compliant_script_url = {}", compliant_script_url);
 
-    // 2. Let outside settings be the current principal settings object.
-    auto& outside_settings = current_principal_settings_object();
+    // 2. Let outsideSettings be this's relevant settings object.
+    // FIXME: We don't have a `this` yet, so use the document.
+    auto& outside_settings = relevant_settings_object(document);
 
-    // 3. Parse the scriptURL argument relative to outside settings.
-    auto url = outside_settings.parse_url(compliant_script_url.to_utf8_but_should_be_ported_to_utf16());
+    // 3. Let workerURL be the result of encoding-parsing a URL given compliantScriptURL, relative to outsideSettings.
+    auto worker_url = outside_settings.encoding_parse_url(compliant_script_url.to_utf8_but_should_be_ported_to_utf16());
 
-    // 4. If this fails, throw a "SyntaxError" DOMException.
-    if (!url.has_value()) {
+    // 4. If workerURL is failure, then throw a "SyntaxError" DOMException.
+    if (!worker_url.has_value()) {
         dbgln_if(WEB_WORKER_DEBUG, "WebWorker: Invalid URL loaded '{}'.", compliant_script_url);
         return WebIDL::SyntaxError::create(document.realm(), "url is not valid"_utf16);
     }
 
-    // 5. Let worker URL be the resulting URL record.
+    // 5. Let outsidePort be a new MessagePort in outsideSettings's realm.
+    auto outside_port = MessagePort::create(outside_settings.realm());
 
-    // 6. Let worker be a new Worker object.
+    // 8. Let worker be this.
+    // AD-HOC: AD-HOC: We do this first so that we can use `this`.
     auto worker = document.realm().create<Worker>(compliant_script_url.to_utf8_but_should_be_ported_to_utf16(), options, document);
 
-    // 7. Let outside port be a new MessagePort in outside settings's Realm.
-    auto outside_port = MessagePort::create(outside_settings.realm());
+    // 6. Set outsidePort's message event target to this.
+    outside_port->set_worker_event_target(worker);
 
-    // 8. Associate the outside port with worker
+    // 7. Set this's outside port to outsidePort.
     worker->m_outside_port = outside_port;
-    worker->m_outside_port->set_worker_event_target(worker);
+
+    // 8. Let worker be this.
+    // NB: This is done earlier.
 
     // 9. Run this step in parallel:
-    //    1. Run a worker given worker, worker URL, outside settings, outside port, and options.
-    run_a_worker(worker, url.value(), outside_settings, *outside_port, options);
+    // 1. Run a worker given worker, workerURL, outsideSettings, outsidePort, and options.
+    run_a_worker(worker, worker_url.value(), outside_settings, *outside_port, options);
 
-    // 10. Return worker
     return worker;
 }
 
@@ -107,15 +112,10 @@ void run_a_worker(Variant<GC::Ref<Worker>, GC::Ref<SharedWorker>> worker, URL::U
     if (!is<HTML::WindowEnvironmentSettingsObject>(outside_settings))
         TODO();
 
-    // 3. Let parent worker global scope be null.
-    // 4. If owner is a WorkerGlobalScope object (i.e., we are creating a nested dedicated worker),
-    //    then set parent worker global scope to owner.
-    // FIXME: Support for nested workers.
-
-    // 5. Let unsafeWorkerCreationTime be the unsafe shared current time.
+    // 3. Let unsafeWorkerCreationTime be the unsafe shared current time.
 
-    // 6. Let agent be the result of obtaining a dedicated/shared worker agent given outside settings
-    //    and is shared. Run the rest of these steps in that agent.
+    // 4. Let agent be the result of obtaining a dedicated/shared worker agent given outside settings and is shared.
+    //    Run the rest of these steps in that agent.
 
     // Note: This spawns a new process to act as the 'agent' for the worker.
     auto agent = outside_settings.realm().create<WorkerAgentParent>(url, options, port, outside_settings, agent_type);
@@ -127,6 +127,7 @@ WebIDL::ExceptionOr<void> Worker::terminate()
 {
     dbgln_if(WEB_WORKER_DEBUG, "WebWorker: Terminate");
 
+    // FIXME: The terminate() method steps are to terminate a worker given this's worker.
     return {};
 }
 

Libraries/LibWeb/HTML/Worker.idl

@@ -19,9 +19,9 @@ interface Worker : EventTarget {
 };
 
 dictionary WorkerOptions {
+    DOMString name = "";
     WorkerType type = "classic";
     RequestCredentials credentials = "same-origin";
-    DOMString name = "";
 };
 
 enum WorkerType { "classic", "module" };

Libraries/LibWeb/HTML/WorkerAgentParent.h

@@ -15,9 +15,9 @@
 namespace Web::HTML {
 
 struct WorkerOptions {
+    String name { String {} };
     Bindings::WorkerType type { Bindings::WorkerType::Classic };
     Bindings::RequestCredentials credentials { Bindings::RequestCredentials::SameOrigin };
-    String name { String {} };
 };
 
 // FIXME: Figure out a better naming convention for this type of parent/child process pattern.