From 91278da9dd55582401154e07f9eea34425a332c2 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Thu, 24 Apr 2014 13:03:19 -0400 Subject: [PATCH] Address review comments. --- .../script/dom/bindings/codegen/Bindings.conf | 4 +- .../dom/bindings/codegen/CodegenRust.py | 66 ++++----- .../dom/bindings/codegen/Configuration.py | 9 +- src/components/script/dom/bindings/js.rs | 140 ++++++++++-------- src/components/script/dom/bindings/utils.rs | 8 +- src/components/script/dom/browsercontext.rs | 4 +- src/components/script/dom/document.rs | 33 ++--- src/components/script/dom/documenttype.rs | 1 - src/components/script/dom/domparser.rs | 3 +- src/components/script/dom/element.rs | 42 +++--- src/components/script/dom/event.rs | 6 +- src/components/script/dom/eventdispatcher.rs | 42 +++--- src/components/script/dom/eventtarget.rs | 4 +- src/components/script/dom/formdata.rs | 4 +- src/components/script/dom/htmlbaseelement.rs | 1 - src/components/script/dom/htmlbodyelement.rs | 1 - src/components/script/dom/htmlbrelement.rs | 1 - .../script/dom/htmlbuttonelement.rs | 1 - .../script/dom/htmlcanvaselement.rs | 1 - src/components/script/dom/htmlcollection.rs | 9 +- src/components/script/dom/htmldataelement.rs | 1 - .../script/dom/htmldatalistelement.rs | 3 +- .../script/dom/htmldirectoryelement.rs | 1 - src/components/script/dom/htmldivelement.rs | 1 - src/components/script/dom/htmlelement.rs | 6 +- src/components/script/dom/htmlembedelement.rs | 1 - .../script/dom/htmlfieldsetelement.rs | 3 +- src/components/script/dom/htmlfontelement.rs | 1 - src/components/script/dom/htmlformelement.rs | 1 - src/components/script/dom/htmlframeelement.rs | 1 - .../script/dom/htmlframesetelement.rs | 1 - .../script/dom/htmlheadingelement.rs | 1 - src/components/script/dom/htmlhrelement.rs | 1 - src/components/script/dom/htmlhtmlelement.rs | 1 - .../script/dom/htmliframeelement.rs | 10 +- src/components/script/dom/htmlimageelement.rs | 12 +- src/components/script/dom/htmlinputelement.rs | 1 - src/components/script/dom/htmllabelelement.rs | 1 - .../script/dom/htmllegendelement.rs | 1 - src/components/script/dom/htmllielement.rs | 1 - src/components/script/dom/htmllinkelement.rs | 1 - src/components/script/dom/htmlmediaelement.rs | 2 - src/components/script/dom/htmlmetaelement.rs | 1 - src/components/script/dom/htmlmodelement.rs | 1 - .../script/dom/htmlobjectelement.rs | 10 +- src/components/script/dom/htmlolistelement.rs | 1 - .../script/dom/htmloptgroupelement.rs | 1 - .../script/dom/htmloptionelement.rs | 1 - .../script/dom/htmloutputelement.rs | 1 - .../script/dom/htmlparagraphelement.rs | 1 - src/components/script/dom/htmlparamelement.rs | 1 - src/components/script/dom/htmlpreelement.rs | 1 - .../script/dom/htmlprogresselement.rs | 1 - src/components/script/dom/htmlquoteelement.rs | 1 - .../script/dom/htmlscriptelement.rs | 1 - .../script/dom/htmlselectelement.rs | 1 - src/components/script/dom/htmlserializer.rs | 50 +++---- .../script/dom/htmlsourceelement.rs | 1 - src/components/script/dom/htmlstyleelement.rs | 10 +- .../script/dom/htmltablecaptionelement.rs | 1 - .../script/dom/htmltablecellelement.rs | 2 - .../script/dom/htmltablecolelement.rs | 1 - src/components/script/dom/htmltableelement.rs | 1 - .../script/dom/htmltablerowelement.rs | 1 - .../script/dom/htmltablesectionelement.rs | 1 - .../script/dom/htmltextareaelement.rs | 1 - src/components/script/dom/htmltimeelement.rs | 1 - src/components/script/dom/htmltitleelement.rs | 1 - src/components/script/dom/htmltrackelement.rs | 1 - src/components/script/dom/htmlulistelement.rs | 1 - src/components/script/dom/htmlvideoelement.rs | 1 - src/components/script/dom/location.rs | 1 - src/components/script/dom/navigator.rs | 1 - src/components/script/dom/node.rs | 112 +++++++------- .../script/dom/processinginstruction.rs | 1 - src/components/script/dom/testbinding.rs | 6 +- src/components/script/dom/text.rs | 1 - src/components/script/dom/uievent.rs | 1 - src/components/script/dom/validitystate.rs | 1 - src/components/script/dom/virtualmethods.rs | 6 +- .../script/dom/webidls/TestBinding.webidl | 2 +- .../script/html/hubbub_html_parser.rs | 9 +- src/components/script/script_task.rs | 20 ++- 83 files changed, 316 insertions(+), 374 deletions(-) diff --git a/src/components/script/dom/bindings/codegen/Bindings.conf b/src/components/script/dom/bindings/codegen/Bindings.conf index f3723fb7c9b..c41856ccf86 100644 --- a/src/components/script/dom/bindings/codegen/Bindings.conf +++ b/src/components/script/dom/bindings/codegen/Bindings.conf @@ -58,9 +58,7 @@ DOMInterfaces = { # FIXME: This should be renamed: https://github.com/mozilla/servo/issues/1625 def addHTMLElement(element): - DOMInterfaces[element] = { - 'nativeType': 'JS<%s>' % element, - } + DOMInterfaces[element] = {} addHTMLElement('Comment') addHTMLElement('DocumentFragment') diff --git a/src/components/script/dom/bindings/codegen/CodegenRust.py b/src/components/script/dom/bindings/codegen/CodegenRust.py index 8c9eb5d6f8b..55bfa9eac91 100644 --- a/src/components/script/dom/bindings/codegen/CodegenRust.py +++ b/src/components/script/dom/bindings/codegen/CodegenRust.py @@ -281,7 +281,6 @@ class CGMethodCall(CGThing): isDefinitelyObject=True), { "declName" : "arg%d" % distinguishingIndex, - "simpleDeclName" : "arg%d" % distinguishingIndex, "holderName" : ("arg%d" % distinguishingIndex) + "_holder", "val" : distinguishingArg }) @@ -395,6 +394,9 @@ def typeIsSequenceOrHasSequenceMember(type): type.flatMemberTypes) return False +def typeNeedsRooting(type, descriptorProvider): + return type.isGeckoInterface() and descriptorProvider.getDescriptor(type.name).needsRooting + def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, isDefinitelyObject=False, isMember=False, @@ -482,6 +484,8 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, if exceptionCode is None: exceptionCode = "return 0;" + needsRooting = typeNeedsRooting(type, descriptorProvider) + def handleOptional(template, declType, isOptional): if isOptional: template = "Some(%s)" % template @@ -490,7 +494,7 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, else: initialValue = None - return (template, declType, isOptional, initialValue) + return (template, declType, isOptional, initialValue, needsRooting) # Unfortunately, .capitalize() on a string will lowercase things inside the # string, which we do not want. @@ -751,7 +755,7 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, "} else {\n" " ${declName} = NULL;\n" "}" % haveCallable, - CGGeneric("JSObject*"), isOptional, None) + CGGeneric("JSObject*"), isOptional, None, needsRooting) if type.isAny(): assert not isEnforceRange and not isClamp @@ -795,7 +799,7 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, assert not isOptional # This one only happens for return values, and its easy: Just # ignore the jsval. - return ("", None, False, None) + return ("", None, False, None, False) if not type.isPrimitive(): raise TypeError("Need conversion for argument type '%s'" % str(type)) @@ -848,7 +852,7 @@ def instantiateJSToNativeConversionTemplate(templateTuple, replacements, replace ${argc} and ${index}, where ${index} is the index of this argument (0-based) and ${argc} is the total number of arguments. """ - (templateBody, declType, dealWithOptional, initialValue) = templateTuple + (templateBody, declType, dealWithOptional, initialValue, needsRooting) = templateTuple if dealWithOptional and argcAndIndex is None: raise TypeError("Have to deal with optional things, but don't know how") @@ -895,9 +899,8 @@ def instantiateJSToNativeConversionTemplate(templateTuple, replacements, # conversion. result.append(CGGeneric("")) - type = declType.define() if declType else None - if type and 'JS<' in type: - rootBody = "let ${simpleDeclName} = ${declName}.root();" + if needsRooting: + rootBody = "let ${declName} = ${declName}.root();" result.append(CGGeneric(string.Template(rootBody).substitute(replacements))) result.append(CGGeneric("")) @@ -942,7 +945,6 @@ class CGArgumentConverter(CGThing): } self.replacementVariables = { "declName" : "arg%d" % index, - "simpleDeclName" : "arg%d" % index, "holderName" : ("arg%d" % index) + "_holder" } self.replacementVariables["val"] = string.Template( @@ -1808,7 +1810,7 @@ def CreateBindingJSObject(descriptor, parent=None): if descriptor.proxy: assert not descriptor.createGlobal handler = """ - let js_info = aScope.get().page().js_info(); + let js_info = aScope.deref().page().js_info(); let handler = js_info.get_ref().dom_static.proxy_handlers.deref().get(&(PrototypeList::id::%s as uint)); """ % descriptor.name create += handler + """ let obj = NewProxyObject(aCx, *handler, @@ -2480,13 +2482,9 @@ class CGSpecializedMethod(CGAbstractExternMethod): def definition_body(self): name = self.method.identifier.name - nativeName = MakeNativeName(name) - extraPre = '' - argsPre = [] - return CGWrapper(CGMethodCall(argsPre, nativeName, self.method.isStatic(), + return CGWrapper(CGMethodCall([], MakeNativeName(name), self.method.isStatic(), self.descriptor, self.method), - pre=extraPre + - " let this = JS::from_raw(this);\n" + + pre=" let this = JS::from_raw(this);\n" + " let mut this = this.root();\n").define() class CGGenericGetter(CGAbstractBindingMethod): @@ -2530,17 +2528,14 @@ class CGSpecializedGetter(CGAbstractExternMethod): def definition_body(self): name = self.attr.identifier.name nativeName = MakeNativeName(name) - extraPre = '' - argsPre = [] infallible = ('infallible' in self.descriptor.getExtendedAttributes(self.attr, getter=True)) if self.attr.type.nullable() or not infallible: nativeName = "Get" + nativeName - return CGWrapper(CGIndenter(CGGetterCall(argsPre, self.attr.type, nativeName, + return CGWrapper(CGIndenter(CGGetterCall([], self.attr.type, nativeName, self.descriptor, self.attr)), - pre=extraPre + - " let this = JS::from_raw(this);\n" + + pre=" let this = JS::from_raw(this);\n" + " let mut this = this.root();\n").define() class CGGenericSetter(CGAbstractBindingMethod): @@ -2588,13 +2583,10 @@ class CGSpecializedSetter(CGAbstractExternMethod): def definition_body(self): name = self.attr.identifier.name - nativeName = "Set" + MakeNativeName(name) - argsPre = [] - extraPre = '' - return CGWrapper(CGIndenter(CGSetterCall(argsPre, self.attr.type, nativeName, + return CGWrapper(CGIndenter(CGSetterCall([], self.attr.type, + "Set" + MakeNativeName(name), self.descriptor, self.attr)), - pre=extraPre + - " let this = JS::from_raw(this);\n" + + pre=" let this = JS::from_raw(this);\n" + " let mut this = this.root();\n").define() @@ -2763,7 +2755,7 @@ def getUnionTypeTemplateVars(type, descriptorProvider): name = type.name typeName = "/*" + type.name + "*/" - (template, _, _, _) = getJSToNativeConversionTemplate( + (template, _, _, _, _) = getJSToNativeConversionTemplate( type, descriptorProvider, failureCode="return Ok(None);", exceptionCode='return Err(());', isDefinitelyObject=True, isOptional=False) @@ -3421,7 +3413,6 @@ class CGProxySpecialOperation(CGPerSignatureCall): treatNullAs=argument.treatNullAs) templateValues = { "declName": argument.identifier.name, - "simpleDeclName": argument.identifier.name, "holderName": argument.identifier.name + "_holder", "val": "(*desc).value", "valPtr": "&(*desc).value" @@ -3868,9 +3859,8 @@ class CGClassConstructHook(CGAbstractExternMethod): def generate_code(self): preamble = """ - let global = global_object_for_js_object(JS_CALLEE(cx, &*vp).to_object()); - let global = global.root(); - let obj = global.reflector().get_jsobject(); + let global = global_object_for_js_object(JS_CALLEE(cx, &*vp).to_object()).root(); + let obj = global.deref().reflector().get_jsobject(); """ nativeName = MakeNativeName(self._ctor.identifier.name) callGenerator = CGMethodCall(["&global.root_ref()"], nativeName, True, @@ -4148,14 +4138,14 @@ class CGDictionary(CGThing): def getMemberType(self, memberInfo): (member, (templateBody, declType, - dealWithOptional, initialValue)) = memberInfo + dealWithOptional, initialValue, _)) = memberInfo if dealWithOptional: declType = CGWrapper(declType, pre="Optional< ", post=" >") return declType.define() def getMemberConversion(self, memberInfo): (member, (templateBody, declType, - dealWithOptional, initialValue)) = memberInfo + dealWithOptional, initialValue, _)) = memberInfo replacements = { "val": "value.unwrap()" } if member.defaultValue: replacements["haveValue"] = "value.is_some()" @@ -4310,7 +4300,9 @@ class CGBindingRoot(CGThing): 'js::glue::{RUST_JS_NumberValue, RUST_JSID_IS_STRING}', 'dom::types::*', 'dom::bindings', - 'dom::bindings::js::{JS, JSRef, Root, RootedReference, Temporary, OptionalRootable, OptionalRootedRootable, ResultRootable}', + 'dom::bindings::js::{JS, JSRef, Root, RootedReference, Temporary}', + 'dom::bindings::js::{OptionalRootable, OptionalRootedRootable, ResultRootable}', + 'dom::bindings::js::{OptionalRootedReference, OptionalOptionalRootedRootable}', 'dom::bindings::utils::{CreateDOMGlobal, CreateInterfaceObjects2}', 'dom::bindings::utils::{ConstantSpec, cx_for_dom_object, Default}', 'dom::bindings::utils::{dom_object_slot, DOM_OBJECT_SLOT, DOMClass}', @@ -5311,7 +5303,7 @@ class GlobalGenRoots(): cast = [CGGeneric(string.Template('''pub trait ${castTraitName} { #[inline(always)] fn to_ref<'a, 'b, T: ${toBound}+Reflectable>(base: &'a JSRef<'b, T>) -> Option<&'a JSRef<'b, Self>> { - match base.get().${checkFn}() { + match base.deref().${checkFn}() { true => unsafe { Some(base.transmute()) }, false => None } @@ -5319,7 +5311,7 @@ class GlobalGenRoots(): #[inline(always)] fn to_mut_ref<'a, 'b, T: ${toBound}+Reflectable>(base: &'a mut JSRef<'b, T>) -> Option<&'a mut JSRef<'b, Self>> { - match base.get().${checkFn}() { + match base.deref().${checkFn}() { true => unsafe { Some(base.transmute_mut()) }, false => None } diff --git a/src/components/script/dom/bindings/codegen/Configuration.py b/src/components/script/dom/bindings/codegen/Configuration.py index dc4daa18b26..1272867f542 100644 --- a/src/components/script/dom/bindings/codegen/Configuration.py +++ b/src/components/script/dom/bindings/codegen/Configuration.py @@ -128,15 +128,18 @@ class Descriptor(DescriptorProvider): # Read the desc, and fill in the relevant defaults. ifaceName = self.interface.identifier.name + + # Callback types do not use JS smart pointers, so we should not use the + # built-in rooting mechanisms for them. if self.interface.isCallback(): - nativeTypeDefault = "nsIDOM" + ifaceName + self.needsRooting = False else: - nativeTypeDefault = 'JS<%s>' % ifaceName + self.needsRooting = True self.returnType = "Temporary<%s>" % ifaceName self.argumentType = "JSRef<%s>" % ifaceName self.memberType = "Root<'a, 'b, %s>" % ifaceName - self.nativeType = desc.get('nativeType', nativeTypeDefault) + self.nativeType = desc.get('nativeType', 'JS<%s>' % ifaceName) self.concreteType = desc.get('concreteType', ifaceName) self.createGlobal = desc.get('createGlobal', False) self.register = desc.get('register', True) diff --git a/src/components/script/dom/bindings/js.rs b/src/components/script/dom/bindings/js.rs index eee20a17e83..da84c85eeb7 100644 --- a/src/components/script/dom/bindings/js.rs +++ b/src/components/script/dom/bindings/js.rs @@ -40,8 +40,8 @@ /// - RootedReference: makes obtaining an Option> from an Option> easy use dom::bindings::utils::{Reflector, Reflectable, cx_for_dom_object}; -use dom::window::Window; -use js::jsapi::{JSObject, JSContext, JS_AddObjectRoot, JS_RemoveObjectRoot}; +use dom::node::Node; +use js::jsapi::{JSObject, JS_AddObjectRoot, JS_RemoveObjectRoot}; use layout_interface::TrustedNodeAddress; use script_task::StackRoots; @@ -90,7 +90,7 @@ impl Temporary { Temporary::new(root.unrooted()) } - /// Root this unrooted value. + /// Create a stack-bounded root for this value. pub fn root<'a, 'b>(self) -> Root<'a, 'b, T> { local_data::get(StackRoots, |opt| { let collection = opt.unwrap(); @@ -130,15 +130,17 @@ impl Clone for JS { } } -impl JS { - /// Create a new JS-reflected DOM object; returns an Temporary type because the new value - /// is not safe to use until it is rooted. - pub fn new(obj: ~T, - window: &JSRef, - wrap_fn: extern "Rust" fn(*JSContext, &JSRef, ~T) -> JS) -> Temporary { - Temporary::new(wrap_fn(window.get().get_cx(), window, obj)) +impl JS { + /// Create a new JS-owned value wrapped from an address known to be a Node pointer. + pub unsafe fn from_trusted_node_address(inner: TrustedNodeAddress) -> JS { + let TrustedNodeAddress(addr) = inner; + JS { + ptr: RefCell::new(addr as *mut Node) + } } +} +impl JS { /// Create a new JS-owned value wrapped from a raw Rust pointer. pub unsafe fn from_raw(raw: *mut T) -> JS { JS { @@ -147,14 +149,6 @@ impl JS { } - /// Create a new JS-owned value wrapped from an address known to be a Node pointer. - pub unsafe fn from_trusted_node_address(inner: TrustedNodeAddress) -> JS { - let TrustedNodeAddress(addr) = inner; - JS { - ptr: RefCell::new(addr as *mut T) - } - } - /// Root this JS-owned value to prevent its collection as garbage. pub fn root<'a, 'b>(&self) -> Root<'a, 'b, T> { local_data::get(StackRoots, |opt| { @@ -209,6 +203,8 @@ impl JS { } } + +/// Get an Option> out of an Option> pub trait RootedReference { fn root_ref<'a>(&'a self) -> Option>; } @@ -219,6 +215,17 @@ impl<'a, 'b, T: Reflectable> RootedReference for Option> { } } +/// Get an Option>> out of an Option>> +pub trait OptionalRootedReference { + fn root_ref<'a>(&'a self) -> Option>>; +} + +impl<'a, 'b, T: Reflectable> OptionalRootedReference for Option>> { + fn root_ref<'a>(&'a self) -> Option>> { + self.as_ref().map(|inner| inner.root_ref()) + } +} + /// Trait that allows extracting a JS value from a variety of rooting-related containers, /// which in general is an unsafe operation since they can outlive the rooted lifetime of the /// original value. @@ -244,6 +251,8 @@ impl Assignable for Temporary { } } +/// Assign an optional rootable value (either of JS or Temporary) to an optional +/// field of a DOM type (ie. Option>) pub trait OptionalSettable { fn assign(&mut self, val: Option); } @@ -254,6 +263,7 @@ impl, U: Reflectable> OptionalSettable for Option> { } } +/// Root a rootable Option type (used for Option>) pub trait OptionalRootable { fn root<'a, 'b>(self) -> Option>; } @@ -264,6 +274,18 @@ impl OptionalRootable for Option> { } } +/// Return an unrooted type for storing in optional DOM fields +pub trait OptionalUnrootable { + fn unrooted(&self) -> Option>; +} + +impl<'a, T: Reflectable> OptionalUnrootable for Option> { + fn unrooted(&self) -> Option> { + self.as_ref().map(|inner| inner.unrooted()) + } +} + +/// Root a rootable Option type (used for Option>) pub trait OptionalRootedRootable { fn root<'a, 'b>(&self) -> Option>; } @@ -274,6 +296,19 @@ impl OptionalRootedRootable for Option> { } } +/// Root a rootable Option