Heads up! This PR modifies the following files:

• @wafflespeanut: python/tidy/servo_tidy/tidy.py
• @KiChjang: components/script/dom/navigator.rs, components/script/dom/mod.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/workerglobalscope.rs, components/script/dom/webidls/Navigator.webidl, components/script/dom/serviceworker.rs, components/script/dom/webidls/ServiceWorkerGlobalScope.webidl, components/script/dom/webidls/ServiceWorker.webidl, components/script/script_thread.rs, components/script/script_runtime.rs, components/script/dom/serviceworkerregistration.rs, components/script/dom/serviceworkercontainer.rs, components/script/dom/webidls/ServiceWorkerRegistration.webidl, components/script/dom/serviceworkerglobalscope.rs, components/script/dom/webidls/ServiceWorkerContainer.webidl

Warning

• These commits include an empty title element, '<title></title>'. Consider adding appropriate metadata.
• These commits modify unsafe code. Please review it carefully!

New code was committed to pull request.

New code was committed to pull request.

@jdm I would like to get comments on this PR.

☔ The latest upstream changes (presumably #11225) made this pull request unmergeable. Please resolve the merge conflicts.

Good start! My biggest concern is the amount of duplicated code in the new worker implementation; if you'd like me to provide more specific suggestions then please let me know.
-S-awaiting-review -S-needs-rebase

Reviewed 26 of 26 files at r1, 20 of 20 files at r2, 2 of 2 files at r3, 4 of 4 files at r4.
Review status: all files reviewed at latest revision, 65 unresolved discussions.

components/script/dom/client.rs, line 20 [r2] (raw file):

#[dom_struct]
pub struct Client {
reflector_: Reflector,

nit: indentation

components/script/dom/client.rs, line 21 [r2] (raw file):

pub struct Client {
reflector_: Reflector,
    active_worker: MutNullableHeap<JS<ServiceWorker>>,

Leave this as Option<JS<ServiceWorker>> until we actually need the mutation.

components/script/dom/client.rs, line 47 [r2] (raw file):

    // https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#client-url
    fn Url(&self) -> USVString {
        USVString((*self.url).to_owned())

Why not store a USVString?

components/script/dom/client.rs, line 49 [r2] (raw file):

        USVString((*self.url).to_owned())
    }
    // https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#client-frametype

nit: add a newline above this

components/script/dom/client.rs, line 53 [r2] (raw file):

        self.frame_type
    }
    // https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#client-id

nit: add a newline above this

components/script/dom/navigator.rs, line 105 [r1] (raw file):

        false
    }
    // https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#navigator-service-worker-attribute

nit: add a newline above this.

components/script/dom/serviceworker.rs, line 1 [r1] (raw file):

/* This Source Code Form is subject to the terms of the Mozilla Public

I am generally concerned with how much code is duplicated between this file and worker.rs. We should look for ways to share it; perhaps making a AbstractWorker trait with an associated type for the TrustedWorkerAddress/TrustedServiceWorkerAddress and message types? I am hesitant to spend much time reviewing the code in this file until we've made it more maintainable.

components/script/dom/serviceworker.rs, line 169 [r1] (raw file):

        USVString(self.script_url.borrow().clone())
    }
    // https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#service-worker-container-onerror-attribute

nit: newline above the //

components/script/dom/serviceworker.rs, line 42 [r2] (raw file):

    eventtarget: EventTarget,
    script_url: DOMRefCell<String>,
    state: DOMRefCell<ServiceWorkerState>,

This can be a Cell instead.

components/script/dom/serviceworker.rs, line 96 [r2] (raw file):

    }

    pub fn transition_state(&self, state: ServiceWorkerState) {

This should be called set_transition_state.

components/script/dom/serviceworker.rs, line 155 [r2] (raw file):

        // represents a service worker client
        let sw_client = Client::new(global.as_window());
        let trusted_client = Trusted::new(sw_client.r());

&*sw_client instead of sw_client.r().

components/script/dom/serviceworker.rs, line 166 [r2] (raw file):

impl ServiceWorkerMethods for ServiceWorker {

nit: remove this newline.

components/script/dom/serviceworkercontainer.rs, line 37 [r1] (raw file):

impl ServiceWorkerContainerMethods for ServiceWorkerContainer {

nit: remote this newline

components/script/dom/serviceworkercontainer.rs, line 38 [r1] (raw file):

impl ServiceWorkerContainerMethods for ServiceWorkerContainer {

// https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#service-worker-container-controller-attribute

nit: indent to match the fn

components/script/dom/serviceworkercontainer.rs, line 43 [r1] (raw file):

    }

// https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#service-worker-container-register-method

nit: indent to match the fn

components/script/dom/serviceworkercontainer.rs, line 48 [r1] (raw file):

                options: &RegistrationOptions)
                -> Fallible<Root<ServiceWorkerRegistration>> {

nit: remove this newline.

components/script/dom/serviceworkercontainer.rs, line 53 [r1] (raw file):

        let script_url = match self.global().r().api_base_url().join(script_url) {
            Ok(url) => url,
            Err(_) => return Err(Error::Syntax),

This should be a Type error according to the spec.

components/script/dom/serviceworkercontainer.rs, line 61 [r1] (raw file):

        }
        // Step 6
        if script_url.path().to_lowercase().contains("%2f")

Let's use to_ascii_lowercase here.

components/script/dom/serviceworkercontainer.rs, line 62 [r1] (raw file):

        // Step 6
        if script_url.path().to_lowercase().contains("%2f")
                || script_url.path().to_lowercase().contains("%5c") {

nit: || on the previous line

components/script/dom/serviceworkercontainer.rs, line 63 [r1] (raw file):

        if script_url.path().to_lowercase().contains("%2f")
                || script_url.path().to_lowercase().contains("%5c") {
            return Err(Error::Type("Invalid Url Path".to_owned()));

nit: Invalid URL path

components/script/dom/serviceworkercontainer.rs, line 71 [r1] (raw file):

                match self.global().r().api_base_url().join(inner_scope) {
                    Ok(url) => url.as_str().to_owned(),
                    Err(_) => return Err(Error::Syntax),

This should be a Type error according to the spec.

components/script/dom/serviceworkercontainer.rs, line 76 [r1] (raw file):

            None => {
                script_url.as_str().to_owned()
            }

nit: no need for the {} here.

components/script/dom/serviceworkercontainer.rs, line 80 [r1] (raw file):

        let active_worker = ServiceWorker::init_service_worker(self.global().r(), script_url);
        Ok(ServiceWorkerRegistration::new(self.global().r(), Some(&(active_worker).unwrap()), scope_url))

nit: the () around active_worker is unnecessary.
The unwrap looks dangerous, though.

components/script/dom/serviceworkercontainer.rs, line 26 [r2] (raw file):

    controller: MutNullableHeap<JS<ServiceWorker>>,
    #[ignore_heap_size_of = "Defined in std"]
    registration_map: DOMRefCell<HashMap<String, JS<ServiceWorkerRegistration>>>

I think this belongs in ScriptThread, instead. Consider a service worker for facebook.com - if there are multiple facebook.com documents, we'll end up needing to duplicate the registration object for each one. If we keep the registration map in the ScriptThread, there only needs to be one of them. Use things like ScriptThread::page_load_complete as an example for how to access it from non-ScriptThread code.

components/script/dom/serviceworkercontainer.rs, line 42 [r2] (raw file):

    }

    pub fn set_registration(&self, scope_url: String, registration_ref: Root<ServiceWorkerRegistration>) {

&ServiceWorkerRegistration

components/script/dom/serviceworkercontainer.rs, line 48 [r2] (raw file):

    #[allow(unrooted_must_root)]
    pub fn match_registration(&self, client_url: &str) -> Option<Root<ServiceWorkerRegistration>> {
        for scope in self.registration_map.borrow().keys() {

Why not use the entry API instead? It's not clear to me what this code is trying to do.

components/script/dom/serviceworkercontainer.rs, line 50 [r2] (raw file):

        for scope in self.registration_map.borrow().keys() {
            match scope.cmp(&client_url.to_owned()) {
                Ordering::Equal => return self.get_registration(Url::parse(client_url).unwrap()),

Why don't we take a Url argument instead?

components/script/dom/serviceworkercontainer.rs, line 51 [r2] (raw file):

            match scope.cmp(&client_url.to_owned()) {
                Ordering::Equal => return self.get_registration(Url::parse(client_url).unwrap()),
                _ => return None

This will make us return after only comparing the first entry in the hashmap.

components/script/dom/serviceworkercontainer.rs, line 58 [r2] (raw file):

    #[allow(unrooted_must_root)]
    pub fn get_registration(&self, scope_str: Url) -> Option<Root<ServiceWorkerRegistration>> {

This could accept &Url.

components/script/dom/serviceworkercontainer.rs, line 60 [r2] (raw file):

    pub fn get_registration(&self, scope_str: Url) -> Option<Root<ServiceWorkerRegistration>> {
        let path_fragment = scope_str.path();
        match self.registration_map.borrow_mut().remove(path_fragment) {

Looks like this could be a map. Why do we want to remove it, though?

components/script/dom/serviceworkercontainer.rs, line 68 [r2] (raw file):

pub trait Controllable {
    fn set_controller(&self, active_worker: Root<ServiceWorker>);

&ServiceWorker instead of Root<ServiceWorker>.

components/script/dom/serviceworkercontainer.rs, line 71 [r2] (raw file):

}

impl Controllable for ServiceWorkerContainer {

What else is expected to implement this trait? I would expect things like Window and WorkerGlobalScope, rather than ServiceWorkerContainer...

components/script/dom/serviceworkercontainer.rs, line 84 [r2] (raw file):

    }

    #[allow(unrooted_must_root)]

Why is this necessary?

components/script/dom/serviceworkercontainer.rs, line 43 [r4] (raw file):

    pub fn set_registration(&self, scope_url: &str, registration_ref: Root<ServiceWorkerRegistration>) {
        self.registration_map.borrow_mut().insert(scope_url.to_owned(), JS::from_rooted(&registration_ref));

It's more idiomatic to take a String argument, rather than &str and then make a String out of it.

components/script/dom/serviceworkercontainer.rs, line 124 [r4] (raw file):

        }
        // Step 12
        if scope.path().to_lowercase().contains("%2f") || script_url.path().to_lowercase().contains("%5c") {

ascii_to_lowercase()

components/script/dom/serviceworkerglobalscope.rs, line 1 [r1] (raw file):

/* This Source Code Form is subject to the terms of the Mozilla Public

I have the same concern about this file as ServiceWorker.rs. It's large, and it looks like there is a lot of duplicated code. We should look for ways to share the implementations between the two. AbstractWorkerGlobalScope?

components/script/dom/serviceworkerglobalscope.rs, line 426 [r1] (raw file):

impl ServiceWorkerGlobalScopeMethods for ServiceWorkerGlobalScope {

nit: remove this newline.

components/script/dom/serviceworkerregistration.rs, line 19 [r1] (raw file):

    active: MutNullableHeap<JS<ServiceWorker>>,
    installing: MutNullableHeap<JS<ServiceWorker>>,
    waiting: MutNullableHeap<JS<ServiceWorker>>,

Since we don't mutate these, they can be Option<JS<ServiceWorker>>

components/script/dom/serviceworkerregistration.rs, line 20 [r1] (raw file):

    installing: MutNullableHeap<JS<ServiceWorker>>,
    waiting: MutNullableHeap<JS<ServiceWorker>>,
    scope: DOMRefCell<String>,

Since we don't mutate this, it doesn't need to be DOMRefCell.

components/script/dom/serviceworkerregistration.rs, line 40 [r1] (raw file):

impl ServiceWorkerRegistrationMethods for ServiceWorkerRegistration {

nit: remove this newline

components/script/dom/serviceworkerregistration.rs, line 43 [r2] (raw file):

               container: &Controllable) -> Root<ServiceWorkerRegistration> {

        let active_worker = ServiceWorker::init_service_worker(global, script_url).unwrap();

This unwrap makes me nervous.

components/script/dom/serviceworkerregistration.rs, line 50 [r2] (raw file):

// Needed to store in registration map
impl Hash for ServiceWorkerRegistration {

I'm surprised that we need to implement Hash if we're just storing it as a value, rather than a key...

components/script/dom/serviceworkerregistration.rs, line 33 [r4] (raw file):

            installing: Default::default(),
            waiting: Default::default(),
            scope: DOMRefCell::new(scope.to_owned())

More idiomatic to take a String argument.

components/script/dom/webidls/Client.webidl, line 8 [r2] (raw file):

// [Exposed=ServiceWorker]
interface Client {

Add a Pref annotation to this.

components/script/dom/webidls/Navigator.webidl, line 34 [r1] (raw file):

};

partial interface Navigator {

// https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#navigator-service-worker

components/script/dom/webidls/Navigator.webidl, line 35 [r1] (raw file):

partial interface Navigator {
  [SameObject] readonly attribute ServiceWorkerContainer serviceWorker;

Add a [Pref="dom.serviceworker.enabled"] annotation.

components/script/dom/webidls/ServiceWorker.webidl, line 4 [r1] (raw file):

 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

nit: remove this extra newline.

components/script/dom/webidls/ServiceWorker.webidl, line 8 [r1] (raw file):

// http://slightlyoff.github.io/ServiceWorker/spec/service_worker/#service-worker-obj
//[Exposed=(Window,Worker)]
interface ServiceWorker : EventTarget {

Add a [Pref="dom.serviceworker.enabled"] annotation.

components/script/dom/webidls/ServiceWorker.webidl, line 12 [r2] (raw file):

  readonly attribute ServiceWorkerState state;
  [Throws]
  void postMessage(any message/*, optional sequence<Transferable> transfer*/);

It's generally easier to make this optional sequence<any> transfer and ignore it in the rust code.

components/script/dom/webidls/ServiceWorkerContainer.webidl, line 7 [r1] (raw file):

// https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#service-worker-container
// [Exposed=(Window,Worker)]
interface ServiceWorkerContainer : EventTarget {

Add a [Pref="dom.serviceworker.enabled"] annotation.

components/script/dom/webidls/ServiceWorkerGlobalScope.webidl, line 7 [r1] (raw file):

[Global/*=(Worker,ServiceWorker), Exposed=ServiceWorker*/]
// https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#service-worker-global-scope
interface ServiceWorkerGlobalScope : WorkerGlobalScope {

Add a [Pref="dom.serviceworker.enabled"] annotation.

components/script/dom/webidls/ServiceWorkerRegistration.webidl, line 7 [r1] (raw file):

// https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#service-worker-registration-obj
// [Exposed=(Window,Worker)]
interface ServiceWorkerRegistration : EventTarget {

Add a [Pref="dom.serviceworker.enabled"] annotation.

components/script_traits/lib.rs, line 272 [r1] (raw file):

    FromWorker,
    /// The event was requested from a worker (ServiceWorkerGlobalScope).
    FromServiceWorker

Is this necessary?

resources/prefs.json, line 6 [r1] (raw file):

  "dom.mouseevent.which.enabled": false,
  "dom.mozbrowser.enabled": false,
  "dom.serviceworker.timeout": 60,

Let's call this timeout_seconds to make the units clear.

tests/wpt/mozilla/meta/mozilla/service-workers/service_worker_registration.html.ini.txt, line 1 [r1] (raw file):

This file should be removed.

tests/wpt/mozilla/tests/mozilla/interfaces.html, line 198 [r1] (raw file):

  "ServiceWorkerContainer",
  "ServiceWorkerRegistration",
  "ServiceWorkerGlobalScope",

When we hide the interfaces behind preferences this change won't be necessary.

tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html, line 1 [r1] (raw file):

<!doctype html>

Add a __dir__.ini file in service-workers that sets the dom.serviceworker.enabled preference to true (example)

tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html, line 24 [r1] (raw file):

test(function() {
  var sw_reg = register_serviceworker('resources/sw.js', {scope: ''});

We should have a test for a non-empty scope as well that can throw an exception.

tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html, line 25 [r1] (raw file):

test(function() {
  var sw_reg = register_serviceworker('resources/sw.js', {scope: ''});
  assert_equals(sw_reg.scope, "http://web-platform.test:8000/_mozilla/mozilla/service-workers/resources/sw.js");

I would use location.href.replace("service-worker-registration.html", "resources/sw.js") instead.

tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html, line 30 [r1] (raw file):

test(function () {
    assert_throws(null,

The first argument should be new TypeError().

tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html, line 33 [r1] (raw file):

                 function () { 
                  var sw_reg = register_serviceworker('./in%5Csome%5fdir/sw.js'); 
              }, "Invalid Url Path");

nit: odd indentation here.

tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html, line 39 [r1] (raw file):

test(function() {
  var sw_reg = register_serviceworker('sw.js');
  assert_class_string(sw_reg.active, "ServiceWorker");

This could be merged with the earlier test that also registers sw.js.

tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html, line 45 [r1] (raw file):

test(function() {
  var sw_reg = register_serviceworker('resources/sw.js');
  assert_equals(sw_reg.active.scriptURL, "http://web-platform.test:8000/_mozilla/mozilla/service-workers/resources/sw.js");

Use location.href like before.

tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html, line 45 [r2] (raw file):

  var sw_reg = register_sw('sw.js');
  assert_class_string(sw_reg.active, "ServiceWorker");
}, "Test:Asserts the controller attribute of ServiceWorkerContainer");

The description disagrees with the test.

tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html, line 30 [r4] (raw file):

test(function() {
  var sw_reg = register_sw('resources/sw.js', '/some/nested/cache/directory');
  assert_equals(sw_reg.scope, "http://web-platform.test:8000/some/nested/cache/directory");

Use location.protocol + '//' + location.host + ':' + location.port + '/some/nested/cache/directory'.

Comments from Reviewable

I tried extracting out some common code, from both of them. Would like to have suggestions what more can be taken out , as I am not sure, how would associated type traits look like.

Review status: all files reviewed at latest revision, 65 unresolved discussions.

components/script/dom/client.rs, line 47 [r2] (raw file):

components/script/dom/serviceworker.rs, line 1 [r1] (raw file):

components/script/dom/serviceworkercontainer.rs, line 62 [r1] (raw file):

components/script/dom/serviceworkercontainer.rs, line 26 [r2] (raw file):

components/script/dom/serviceworkercontainer.rs, line 48 [r2] (raw file):

components/script/dom/serviceworkercontainer.rs, line 51 [r2] (raw file):

components/script/dom/serviceworkercontainer.rs, line 58 [r2] (raw file):

components/script/dom/serviceworkercontainer.rs, line 60 [r2] (raw file):

components/script/dom/serviceworkercontainer.rs, line 71 [r2] (raw file):

components/script/dom/serviceworkercontainer.rs, line 84 [r2] (raw file):

components/script/dom/serviceworkerglobalscope.rs, line 1 [r1] (raw file):

components/script/dom/serviceworkerregistration.rs, line 19 [r1] (raw file):

components/script/dom/serviceworkerregistration.rs, line 43 [r2] (raw file):

components/script/dom/serviceworkerregistration.rs, line 50 [r2] (raw file):

components/script/dom/webidls/ServiceWorker.webidl, line 12 [r2] (raw file):

components/script_traits/lib.rs, line 272 [r1] (raw file):

tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html, line 24 [r1] (raw file):

tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html, line 25 [r1] (raw file):

tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html, line 39 [r1] (raw file):

tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html, line 45 [r2] (raw file):

tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html, line 30 [r4] (raw file):

Comments from Reviewable

New code was committed to pull request.

New code was committed to pull request.

Ok, having thought about it further I don't think it's worth trying to do anything too elaborate to share more code at this moment. Since all of this code is in flux, it could just make it harder to make changes in the future. We can look at reducing duplication when we have a better sense of the final design.

-S-awaiting-review +S-needs-code-changes

Reviewed 31 of 31 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 25 unresolved discussions, some commit checks failed.

components/script/script_thread.rs, line 1342 [r5] (raw file):

    }

    fn handle_serviceworker_registration(&self, scope: String, registration: &ServiceWorkerRegistration) {

This can be &mut self if we use borrow_mut earlier, and then we don't need the DOMRefCell.

components/script/dom/client.rs, line 47 [r2] (raw file):

components/script/dom/serviceworker.rs, line 88 [r5] (raw file):

                               EventBubbles::DoesNotBubble,
                               EventCancelable::Cancelable);
        event.fire(self.upcast());

This should use self.upcast::<EventTarget>().fire_simple_event("statechange"), which both uses the right event name and makes the event non-cancelable per https://html.spec.whatwg.org/multipage/webappapis.html#fire-a-simple-event .

components/script/dom/serviceworker.rs, line 138 [r5] (raw file):

        let trusted_client = Trusted::new(&*sw_client);

        let shared_rt = SharedRt { rt: unsafe { JS_GetRuntime(global.get_cx()) } };

I don't think this is correct? The shared runtime is supposed to be the one that's created for the new worker, not the existing runtime for the thread that runs the worker constructor.

components/script/dom/serviceworker.rs, line 110 [r6] (raw file):

    pub fn init_service_worker(global: GlobalRef,
                               script_url: Url,
                               skip_waiting: bool) -> Fallible<Root<ServiceWorker>> {

This only ever seems to return Ok, so this can return Root<ServiceWorker>.

components/script/dom/serviceworkercontainer.rs, line 62 [r1] (raw file):

components/script/dom/serviceworkercontainer.rs, line 62 [r5] (raw file):

        let script_url = match self.global().r().api_base_url().join(script_url) {
            Ok(url) => url,
            Err(_) => return Err(Error::Type("Invalid URL Path".to_owned()))

"Invalid script URL"

components/script/dom/serviceworkercontainer.rs, line 72 [r5] (raw file):

        if script_url.path().to_ascii_lowercase().contains("%2f")
        || script_url.path().to_ascii_lowercase().contains("%5c") {
            return Err(Error::Type("Invalid URL Path".to_owned()));

nit: "Script URL contains forbidden characters"

components/script/dom/serviceworkercontainer.rs, line 80 [r5] (raw file):

                match self.global().r().api_base_url().join(inner_scope) {
                    Ok(url) => url,
                    Err(_) => return Err(Error::Type("Invalid URL Path".to_owned()))

"Invalid scope URL"

components/script/dom/serviceworkercontainer.rs, line 92 [r5] (raw file):

        // Step 12
        if scope.path().to_ascii_lowercase().contains("%2f")
        || scope.path().to_ascii_lowercase().contains("%5c") {

nit: || on the previous line

components/script/dom/serviceworkercontainer.rs, line 93 [r5] (raw file):

        if scope.path().to_ascii_lowercase().contains("%2f")
        || scope.path().to_ascii_lowercase().contains("%5c") {
            return Err(Error::Type("Invalid Url Path".to_owned()));

"Scope URL contains forbidden characters"

components/script/dom/serviceworkercontainer.rs, line 101 [r5] (raw file):

                                                                 scope_str.clone(),
                                                                 self);
        ScriptThread::set_registration(scope_str, &*worker_registration.clone());

No need to clone this now.

components/script/dom/serviceworkerglobalscope.rs, line 190 [r5] (raw file):

            };

            let runtime = unsafe { new_rt_and_cx(parent_rt.rt()) };

We should pass ptr::mut_null() here instead. There's no guarantee that whatever provided the parent runtime will outlive the worker.

components/script/dom/serviceworkerregistration.rs, line 19 [r1] (raw file):

components/script/dom/webidls/Navigator.webidl, line 35 [r5] (raw file):

// https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#navigator-service-worker
//[Pref="dom.serviceworker.enabled"]

This belongs on the attribute instead of the interface.

components/script/dom/webidls/ServiceWorkerGlobalScope.webidl, line 9 [r5] (raw file):

[Global/*=(Worker,ServiceWorker), Exposed=ServiceWorker*/]
interface ServiceWorkerGlobalScope : WorkerGlobalScope {
[Pref="dom.serviceworker.enabled"]

This belongs on the interface; right now it's on the first property instead.

Comments from Reviewable

@jdm but these tests pass locally, when i run, ./mach test-wpt tests/wpt/mozilla/tests/mozilla/service-workers . Does the introduction of Pref, needs something to change ?

Did the ini file that sets the preference get included?

@jdm oh, i should have it in meta directory. okay; updated the changes. Should pass now.

New code was committed to pull request.

☔ The latest upstream changes (presumably #11538) made this pull request unmergeable. Please resolve the merge conflicts.

@bors-servo: r+

📌 Commit 15a2064 has been approved by jdm

⌛ Testing commit 15a2064 with merge 4bb912c...

@bors-servo clean retry

⌛ Testing commit 15a2064 with merge cc017fc...

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows