Auto merge of #24665 - saschanaz:timerhandler, r=nox

Use TimerHandler IDL union type

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #20700

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

components/script/dom/bindings/codegen/CodegenRust.py

@@ -2413,6 +2413,7 @@ def UnionTypes(descriptors, dictionaries, callbacks, typedefs, config):
         'crate::dom::bindings::conversions::root_from_handlevalue',
         'crate::dom::bindings::conversions::windowproxy_from_handlevalue',
         'std::ptr::NonNull',
+        'std::rc::Rc',
         'crate::dom::bindings::record::Record',
         'crate::dom::bindings::num::Finite',
         'crate::dom::bindings::root::DomRoot',
@@ -2427,6 +2428,7 @@ def UnionTypes(descriptors, dictionaries, callbacks, typedefs, config):
         'js::error::throw_type_error',
         'js::rust::HandleValue',
         'js::jsapi::Heap',
+        'js::jsapi::IsCallable',
         'js::jsapi::JSContext',
         'js::jsapi::JSObject',
         'js::rust::MutableHandleValue',
@@ -2442,9 +2444,10 @@ def UnionTypes(descriptors, dictionaries, callbacks, typedefs, config):
         if not t.isUnion():
             continue
         for memberType in t.flatMemberTypes:
-            if memberType.isDictionary() or memberType.isEnum():
+            if memberType.isDictionary() or memberType.isEnum() or memberType.isCallback():
                 memberModule = getModuleFromObject(memberType)
-                memberName = memberType.inner.identifier.name
+                memberName = (memberType.callback.identifier.name
+                              if memberType.isCallback() else memberType.inner.identifier.name)
                 imports.append("%s::%s" % (memberModule, memberName))
                 if memberType.isEnum():
                     imports.append("%s::%sValues" % (memberModule, memberName))
@@ -4384,6 +4387,9 @@ def getUnionTypeTemplateVars(type, descriptorProvider):
     elif is_typed_array(type):
         name = type.name
         typeName = "typedarray::Heap" + name
+    elif type.isCallback():
+        name = type.name
+        typeName = name
     else:
         raise TypeError("Can't handle %s in unions yet" % type)
 
@@ -4422,12 +4428,19 @@ class CGUnionStruct(CGThing):
         return False
 
     def define(self):
+        def getTypeWrapper(t):
+            if type_needs_tracing(t):
+                return "RootedTraceableBox"
+            if t.isCallback():
+                return "Rc"
+            return ""
+
         templateVars = map(lambda t: (getUnionTypeTemplateVars(t, self.descriptorProvider),
-                                      type_needs_tracing(t)),
+                                      getTypeWrapper(t)),
                            self.type.flatMemberTypes)
         enumValues = [
-            "    %s(%s)," % (v["name"], "RootedTraceableBox<%s>" % v["typeName"] if trace else v["typeName"])
+            "    %s(%s)," % (v["name"], "%s<%s>" % (wrapper, v["typeName"]) if wrapper else v["typeName"])
-            for (v, trace) in templateVars
+            for (v, wrapper) in templateVars
         ]
         enumConversions = [
             "            %s::%s(ref inner) => inner.to_jsval(cx, rval),"
@@ -4510,7 +4523,8 @@ class CGUnionConversionStruct(CGThing):
         callbackMemberTypes = filter(lambda t: t.isCallback() or t.isCallbackInterface(), memberTypes)
         if len(callbackMemberTypes) > 0:
             assert len(callbackMemberTypes) == 1
-            raise TypeError("Can't handle callbacks in unions.")
+            typeName = callbackMemberTypes[0].name
+            callbackObject = CGGeneric(get_match(typeName))
         else:
             callbackObject = None
 
@@ -4541,7 +4555,7 @@ class CGUnionConversionStruct(CGThing):
         else:
             mozMapObject = None
 
-        hasObjectTypes = object or interfaceObject or arrayObject or dateObject or mozMapObject
+        hasObjectTypes = object or interfaceObject or arrayObject or dateObject or callbackObject or mozMapObject
         if hasObjectTypes:
             # "object" is not distinguishable from other types
             assert not object or not (interfaceObject or arrayObject or dateObject or callbackObject or mozMapObject)
@@ -4552,6 +4566,8 @@ class CGUnionConversionStruct(CGThing):
                 templateBody.append(interfaceObject)
             if arrayObject:
                 templateBody.append(arrayObject)
+            if callbackObject:
+                templateBody.append(callbackObject)
             if mozMapObject:
                 templateBody.append(mozMapObject)
             conversions.append(CGIfWrapper("value.get().is_object()", templateBody))
@@ -4612,6 +4628,8 @@ class CGUnionConversionStruct(CGThing):
         actualType = templateVars["typeName"]
         if type_needs_tracing(t):
             actualType = "RootedTraceableBox<%s>" % actualType
+        if t.isCallback():
+            actualType = "Rc<%s>" % actualType
         returnType = "Result<Option<%s>, ()>" % actualType
         jsConversion = templateVars["jsConversion"]
 

components/script/dom/webidls/WindowOrWorkerGlobalScope.webidl

@@ -4,8 +4,7 @@
 
 // https://html.spec.whatwg.org/multipage/#windoworworkerglobalscope
 
-// FIXME(nox): https://github.com/servo/servo/issues/20700
+typedef (DOMString or Function) TimerHandler;
-// typedef (DOMString or Function) TimerHandler;
 
 [Exposed=(Window,Worker)]
 interface mixin WindowOrWorkerGlobalScope {
@@ -16,13 +15,9 @@ interface mixin WindowOrWorkerGlobalScope {
   [Throws] DOMString atob(DOMString data);
 
   // timers
-  // FIXME(nox): https://github.com/servo/servo/issues/20700
+  long setTimeout(TimerHandler handler, optional long timeout = 0, any... arguments);
-  long setTimeout(Function handler, optional long timeout = 0, any... arguments);
-  long setTimeout(DOMString handler, optional long timeout = 0, any... arguments);
   void clearTimeout(optional long handle = 0);
-  // FIXME(nox): https://github.com/servo/servo/issues/20700
+  long setInterval(TimerHandler handler, optional long timeout = 0, any... arguments);
-  long setInterval(Function handler, optional long timeout = 0, any... arguments);
-  long setInterval(DOMString handler, optional long timeout = 0, any... arguments);
   void clearInterval(optional long handle = 0);
 
   // ImageBitmap

components/script/dom/window.rs

@@ -7,7 +7,6 @@ use crate::dom::bindings::cell::DomRefCell;
 use crate::dom::bindings::codegen::Bindings::DocumentBinding::{
     DocumentMethods, DocumentReadyState,
 };
-use crate::dom::bindings::codegen::Bindings::FunctionBinding::Function;
 use crate::dom::bindings::codegen::Bindings::HistoryBinding::HistoryBinding::HistoryMethods;
 use crate::dom::bindings::codegen::Bindings::MediaQueryListBinding::MediaQueryListBinding::MediaQueryListMethods;
 use crate::dom::bindings::codegen::Bindings::PermissionStatusBinding::PermissionState;
@@ -16,7 +15,7 @@ use crate::dom::bindings::codegen::Bindings::WindowBinding::{
     self, FrameRequestCallback, WindowMethods, WindowPostMessageOptions,
 };
 use crate::dom::bindings::codegen::Bindings::WindowBinding::{ScrollBehavior, ScrollToOptions};
-use crate::dom::bindings::codegen::UnionTypes::RequestOrUSVString;
+use crate::dom::bindings::codegen::UnionTypes::{RequestOrUSVString, StringOrFunction};
 use crate::dom::bindings::error::{Error, ErrorResult, Fallible};
 use crate::dom::bindings::inheritance::Castable;
 use crate::dom::bindings::num::Finite;
@@ -817,28 +816,16 @@ impl WindowMethods for Window {
     fn SetTimeout(
         &self,
         _cx: JSContext,
-        callback: Rc<Function>,
+        callback: StringOrFunction,
         timeout: i32,
         args: Vec<HandleValue>,
     ) -> i32 {
+        let callback = match callback {
+            StringOrFunction::String(i) => TimerCallback::StringTimerCallback(i),
+            StringOrFunction::Function(i) => TimerCallback::FunctionTimerCallback(i),
+        };
         self.upcast::<GlobalScope>().set_timeout_or_interval(
-            TimerCallback::FunctionTimerCallback(callback),
+            callback,
-            args,
-            timeout,
-            IsInterval::NonInterval,
-        )
-    }
-
-    // https://html.spec.whatwg.org/multipage/#dom-windowtimers-settimeout
-    fn SetTimeout_(
-        &self,
-        _cx: JSContext,
-        callback: DOMString,
-        timeout: i32,
-        args: Vec<HandleValue>,
-    ) -> i32 {
-        self.upcast::<GlobalScope>().set_timeout_or_interval(
-            TimerCallback::StringTimerCallback(callback),
             args,
             timeout,
             IsInterval::NonInterval,
@@ -855,28 +842,16 @@ impl WindowMethods for Window {
     fn SetInterval(
         &self,
         _cx: JSContext,
-        callback: Rc<Function>,
+        callback: StringOrFunction,
         timeout: i32,
         args: Vec<HandleValue>,
     ) -> i32 {
+        let callback = match callback {
+            StringOrFunction::String(i) => TimerCallback::StringTimerCallback(i),
+            StringOrFunction::Function(i) => TimerCallback::FunctionTimerCallback(i),
+        };
         self.upcast::<GlobalScope>().set_timeout_or_interval(
-            TimerCallback::FunctionTimerCallback(callback),
+            callback,
-            args,
-            timeout,
-            IsInterval::Interval,
-        )
-    }
-
-    // https://html.spec.whatwg.org/multipage/#dom-windowtimers-setinterval
-    fn SetInterval_(
-        &self,
-        _cx: JSContext,
-        callback: DOMString,
-        timeout: i32,
-        args: Vec<HandleValue>,
-    ) -> i32 {
-        self.upcast::<GlobalScope>().set_timeout_or_interval(
-            TimerCallback::StringTimerCallback(callback),
             args,
             timeout,
             IsInterval::Interval,

components/script/dom/workerglobalscope.rs

@@ -4,11 +4,10 @@
 
 use crate::compartments::InCompartment;
 use crate::dom::bindings::cell::DomRefCell;
-use crate::dom::bindings::codegen::Bindings::FunctionBinding::Function;
 use crate::dom::bindings::codegen::Bindings::RequestBinding::RequestInit;
 use crate::dom::bindings::codegen::Bindings::WorkerBinding::WorkerType;
 use crate::dom::bindings::codegen::Bindings::WorkerGlobalScopeBinding::WorkerGlobalScopeMethods;
-use crate::dom::bindings::codegen::UnionTypes::RequestOrUSVString;
+use crate::dom::bindings::codegen::UnionTypes::{RequestOrUSVString, StringOrFunction};
 use crate::dom::bindings::error::{report_pending_exception, Error, ErrorResult, Fallible};
 use crate::dom::bindings::inheritance::Castable;
 use crate::dom::bindings::reflector::DomObject;
@@ -297,28 +296,16 @@ impl WorkerGlobalScopeMethods for WorkerGlobalScope {
     fn SetTimeout(
         &self,
         _cx: JSContext,
-        callback: Rc<Function>,
+        callback: StringOrFunction,
         timeout: i32,
         args: Vec<HandleValue>,
     ) -> i32 {
+        let callback = match callback {
+            StringOrFunction::String(i) => TimerCallback::StringTimerCallback(i),
+            StringOrFunction::Function(i) => TimerCallback::FunctionTimerCallback(i),
+        };
         self.upcast::<GlobalScope>().set_timeout_or_interval(
-            TimerCallback::FunctionTimerCallback(callback),
+            callback,
-            args,
-            timeout,
-            IsInterval::NonInterval,
-        )
-    }
-
-    // https://html.spec.whatwg.org/multipage/#dom-windowtimers-settimeout
-    fn SetTimeout_(
-        &self,
-        _cx: JSContext,
-        callback: DOMString,
-        timeout: i32,
-        args: Vec<HandleValue>,
-    ) -> i32 {
-        self.upcast::<GlobalScope>().set_timeout_or_interval(
-            TimerCallback::StringTimerCallback(callback),
             args,
             timeout,
             IsInterval::NonInterval,
@@ -335,28 +322,16 @@ impl WorkerGlobalScopeMethods for WorkerGlobalScope {
     fn SetInterval(
         &self,
         _cx: JSContext,
-        callback: Rc<Function>,
+        callback: StringOrFunction,
         timeout: i32,
         args: Vec<HandleValue>,
     ) -> i32 {
+        let callback = match callback {
+            StringOrFunction::String(i) => TimerCallback::StringTimerCallback(i),
+            StringOrFunction::Function(i) => TimerCallback::FunctionTimerCallback(i),
+        };
         self.upcast::<GlobalScope>().set_timeout_or_interval(
-            TimerCallback::FunctionTimerCallback(callback),
+            callback,
-            args,
-            timeout,
-            IsInterval::Interval,
-        )
-    }
-
-    // https://html.spec.whatwg.org/multipage/#dom-windowtimers-setinterval
-    fn SetInterval_(
-        &self,
-        _cx: JSContext,
-        callback: DOMString,
-        timeout: i32,
-        args: Vec<HandleValue>,
-    ) -> i32 {
-        self.upcast::<GlobalScope>().set_timeout_or_interval(
-            TimerCallback::StringTimerCallback(callback),
             args,
             timeout,
             IsInterval::Interval,

tests/wpt/metadata/html/webappapis/timers/evil-spec-example.html.ini

@@ -1,6 +0,0 @@
-[evil-spec-example.html]
-  type: testharness
-  expected: ERROR
-  [Interaction of setTimeout and WebIDL]
-    expected: NOTRUN
-