Address review comments.

src/components/script/dom/attrlist.rs

@@ -17,16 +17,16 @@ pub struct AttrList {
 }
 
 impl AttrList {
-    pub fn new_inherited(window: JS<Window>, elem: JS<Element>) -> AttrList {
+    pub fn new_inherited(window: &JSRef<Window>, elem: &JSRef<Element>) -> AttrList {
         AttrList {
             reflector_: Reflector::new(),
-            window: window,
-            owner: elem
+            window: window.unrooted(),
+            owner: elem.unrooted(),
         }
     }
 
     pub fn new(window: &JSRef<Window>, elem: &JSRef<Element>) -> Temporary<AttrList> {
-        reflect_dom_object(~AttrList::new_inherited(window.unrooted(), elem.unrooted()),
+        reflect_dom_object(~AttrList::new_inherited(window, elem),
                            window, AttrListBinding::Wrap)
     }
 }
@@ -37,7 +37,7 @@ pub trait AttrListMethods {
     fn IndexedGetter(&self, index: u32, found: &mut bool) -> Option<Temporary<Attr>>;
 }
 
-impl<'a> AttrListMethods for JSRef<'a, AttrList> { 
+impl<'a> AttrListMethods for JSRef<'a, AttrList> {
     fn Length(&self) -> u32 {
         self.owner.root().attrs.len() as u32
     }

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

@@ -558,15 +558,6 @@ def wrapObjectTemplate(templateBody, isDefinitelyObject, type,
             else:
                 assert(defaultValue is None)
 
-            #if type.isGeckoInterface() and not type.unroll().inner.isCallback():
-            #    if type.nullable() or isOptional:
-            #        
-            #    else:
-            #        
-            #    templateBody = CGList([CGGeneric(templateBody),
-            #                           CGGeneric("\n"),
-            #                           CGGeneric(rootBody)]).define()
-
         return templateBody
 
     assert not (isEnforceRange and isClamp) # These are mutually exclusive
@@ -901,12 +892,7 @@ def instantiateJSToNativeConversionTemplate(templateTuple, replacements,
 
     type = declType.define() if declType else None
     if type and 'JS<' in type:
-        if dealWithOptional or 'Option<' in type:
-            rootBody = """let ${simpleDeclName} = ${declName}.as_ref().map(|inner| {
-  inner.root() //second root code
-});"""
-        else:
-            rootBody = "let ${simpleDeclName} = ${declName}.root(); //third root code"
+        rootBody = "let ${simpleDeclName} = ${declName}.root();"
         result.append(CGGeneric(string.Template(rootBody).substitute(replacements)))
         result.append(CGGeneric(""))
 
@@ -1725,8 +1711,6 @@ class Argument():
     A class for outputting the type and name of an argument
     """
     def __init__(self, argType, name, default=None, mutable=False):
-        if argType and 'JS<' in argType:
-            argType = argType.replace('JS<', 'JSRef<')
         self.argType = argType
         self.name = name
         self.default = default
@@ -4321,7 +4305,7 @@ def __init__(self, config, prefix, webIDLFile):
             'js::glue::{RUST_JS_NumberValue, RUST_JSID_IS_STRING}',
             'dom::types::*',
             'dom::bindings',
-            'dom::bindings::js::{JS, JSRef, RootedReference, Temporary, OptionalRootable}',
+            'dom::bindings::js::{JS, JSRef, RootedReference, Temporary, OptionalRootable, OptionalRootedRootable}',
             'dom::bindings::utils::{CreateDOMGlobal, CreateInterfaceObjects2}',
             'dom::bindings::utils::{ConstantSpec, cx_for_dom_object, Default}',
             'dom::bindings::utils::{dom_object_slot, DOM_OBJECT_SLOT, DOMClass}',
@@ -4603,7 +4587,7 @@ def doGetArgType(self, type, optional, isMember):
                 else:
                     typeDecl = "%s"
             descriptor = self.descriptorProvider.getDescriptor(iface.identifier.name)
-            return (typeDecl % descriptor.nativeType,
+            return (typeDecl % descriptor.argumentType,
                     False, False)
 
         if type.isSpiderMonkeyInterface():

src/components/script/dom/bindings/codegen/Configuration.py

@@ -134,6 +134,7 @@ def __init__(self, config, interface, desc):
             nativeTypeDefault = 'JS<%s>' % ifaceName
 
         self.returnType = "Temporary<%s>" % ifaceName
+        self.argumentType = "JSRef<%s>" % ifaceName
         self.nativeType = desc.get('nativeType', nativeTypeDefault)
         self.concreteType = desc.get('concreteType', ifaceName)
         self.createGlobal = desc.get('createGlobal', False)

src/components/script/dom/bindings/js.rs

@@ -2,6 +2,43 @@
  * 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/. */
 
+/// The DOM is made up of Rust types whose lifetime is entirely controlled by the whims of
+/// the SpiderMonkey garbage collector. The types in this module are designed to ensure
+/// that any interactions with said Rust types only occur on values that will remain alive
+/// the entire time.
+///
+/// Here is a brief overview of the important types:
+/// - JSRef<T>: a freely-copyable reference to a rooted value.
+/// - JS<T>: a pointer to JS-owned memory that can automatically be traced by the GC when
+///          encountered as a field of a Rust structure.
+/// - Temporary<T>: a value that will remain rooted for the duration of its lifetime.
+///
+/// The rule of thumb is as follows:
+/// - All methods return Temporary<T>, to ensure the value remains alive until it is stored
+///   somewhere that is reachable by the GC.
+/// - All functions take &JSRef<T> arguments, to ensure that they will remain uncollected for
+///   the duration of their usage.
+/// - All types contain JS<T> fields and derive the Encodable trait, to ensure that they are
+///   transitively marked as reachable by the GC if the enclosing value is reachable.
+/// - All methods for type T are implemented for JSRef<T>, to ensure that the self value
+///   will not be collected for the duration of the method call.
+///
+/// Both Temporary<T> and JS<T> do not allow access to their inner value without explicitly
+/// creating a stack-based root via the `root` method. This returns a Root<T>, which causes
+/// the JS-owned value to be uncollectable for the duration of the Root type's lifetime.
+/// A JSRef<T> can be obtained from a Root<T> either by dereferencing the Root<T> (`*rooted`)
+/// or explicitly calling the `root_ref` method. These JSRef<T> values are not allowed to
+/// outlive their originating Root<T>, to ensure that all interactions with the enclosed value
+/// only occur when said value is uncollectable, and will cause static lifetime errors if
+/// misused.
+///
+/// Other miscellaneous helper traits:
+/// - OptionalRootable and OptionalRootedRootable: make rooting Option values easy via a `root` method
+/// - ResultRootable: make rooting successful Result values easy
+/// - TemporaryPushable: allows mutating vectors of JS<T> with new elements of JSRef/Temporary
+/// - OptionalSettable: allows assigning Option values of JSRef/Temporary to fields of Option<JS<T>>
+/// - RootedReference: makes obtaining an Option<JSRef<T>> from an Option<Root<T>> easy
+
 use dom::bindings::utils::{Reflector, Reflectable, cx_for_dom_object};
 use dom::window::Window;
 use js::jsapi::{JSObject, JSContext, JS_AddObjectRoot, JS_RemoveObjectRoot};
@@ -15,7 +52,7 @@ use std::local_data;
 /// A type that represents a JS-owned value that is rooted for the lifetime of this value.
 /// Importantly, it requires explicit rooting in order to interact with the inner value.
 /// Can be assigned into JS-owned member fields (ie. JS<T> types) safely via the
-/// `JS<T>::assign` method or `OptionalAssignable::assign` (for Option<JS<T>> fields).
+/// `JS<T>::assign` method or `OptionalSettable::assign` (for Option<JS<T>> fields).
 pub struct Temporary<T> {
     inner: JS<T>,
 }
@@ -49,7 +86,7 @@ impl<T: Reflectable> Temporary<T> {
     }
 
     /// Create a new Temporary value from a rooted value.
-    pub fn new_rooted<'a>(root: &JSRef<'a, T>) -> Temporary<T> {
+    pub fn from_rooted<'a>(root: &JSRef<'a, T>) -> Temporary<T> {
         Temporary::new(root.unrooted())
     }
 
@@ -182,39 +219,38 @@ impl<'a, 'b, T: Reflectable> RootedReference<T> for Option<Root<'a, 'b, T>> {
     }
 }
 
-// This trait should never be public; it allows access to unrooted values, and is thus
-// easy to misuse.
+/// Trait that allows extracting a JS<T> 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.
 /*definitely not public*/ trait Assignable<T> {
-    fn get_js(&self) -> JS<T>;
+    unsafe fn get_js(&self) -> JS<T>;
 }
 
 impl<T> Assignable<T> for JS<T> {
-    fn get_js(&self) -> JS<T> {
+    unsafe fn get_js(&self) -> JS<T> {
         self.clone()
     }
 }
 
 impl<'a, T> Assignable<T> for JSRef<'a, T> {
-    fn get_js(&self) -> JS<T> {
+    unsafe fn get_js(&self) -> JS<T> {
         self.unrooted()
     }
 }
 
-// Assignable should not be exposed publically, since it's used to
-// extract unrooted values in a safe way WHEN USED CORRECTLY.
 impl<T: Reflectable> Assignable<T> for Temporary<T> {
-    fn get_js(&self) -> JS<T> {
-        unsafe { self.inner() }
+    unsafe fn get_js(&self) -> JS<T> {
+        self.inner()
     }
 }
 
-pub trait OptionalAssignable<T> {
+pub trait OptionalSettable<T> {
     fn assign(&mut self, val: Option<T>);
 }
 
-impl<T: Assignable<U>, U: Reflectable> OptionalAssignable<T> for Option<JS<U>> {
+impl<T: Assignable<U>, U: Reflectable> OptionalSettable<T> for Option<JS<U>> {
     fn assign(&mut self, val: Option<T>) {
-        *self = val.map(|val| val.get_js());
+        *self = val.map(|val| unsafe { val.get_js() });
     }
 }
 
@@ -252,12 +288,17 @@ impl<T: Reflectable, U> ResultRootable<T, U> for Result<Temporary<T>, U> {
 /// under the assumption that said lists are reachable via the GC graph, and therefore the
 /// new values are transitively rooted for the lifetime of their new owner.
 pub trait TemporaryPushable<T> {
-    fn push_unrooted(&mut self, val: Temporary<T>);
+    fn push_unrooted(&mut self, val: &T);
+    fn insert_unrooted(&mut self, index: uint, val: &T);
 }
 
-impl<T: Reflectable> TemporaryPushable<T> for Vec<JS<T>> {
-    fn push_unrooted(&mut self, val: Temporary<T>) {
-        unsafe { self.push(val.inner()) };
+impl<T: Assignable<U>, U: Reflectable> TemporaryPushable<T> for Vec<JS<U>> {
+    fn push_unrooted(&mut self, val: &T) {
+        self.push(unsafe { val.get_js() });
+    }
+
+    fn insert_unrooted(&mut self, index: uint, val: &T) {
+        self.insert(index, unsafe { val.get_js() });
     }
 }
 
@@ -277,26 +318,16 @@ impl RootCollection {
         Root::new(self, unrooted)
     }
 
-    fn root<'a, 'b, T: Reflectable>(&self, unrooted: &Root<'a, 'b, T>) {
-        self.root_raw(unrooted.js_ptr);
-    }
-
-    /// Root a raw JS pointer.
-    pub fn root_raw(&self, unrooted: *JSObject) {
+    fn root<'a, 'b, T: Reflectable>(&self, untracked: &Root<'a, 'b, T>) {
         let mut roots = self.roots.borrow_mut();
-        roots.push(unrooted);
-        debug!("  rooting {:?}", unrooted);
+        roots.push(untracked.js_ptr);
+        debug!("  rooting {:?}", untracked.js_ptr);
     }
 
     fn unroot<'a, 'b, T: Reflectable>(&self, rooted: &Root<'a, 'b, T>) {
-        self.unroot_raw(rooted.js_ptr);
-    }
-
-    /// Unroot a raw JS pointer. Must occur in reverse order to its rooting.
-    pub fn unroot_raw(&self, rooted: *JSObject) {
         let mut roots = self.roots.borrow_mut();
-        debug!("unrooting {:?} (expecting {:?}", roots.last().unwrap(), rooted);
-        assert!(*roots.last().unwrap() == rooted);
+        debug!("unrooting {:?} (expecting {:?}", roots.last().unwrap(), rooted.js_ptr);
+        assert!(*roots.last().unwrap() == rooted.js_ptr);
         roots.pop().unwrap();
     }
 }

src/components/script/dom/bindings/utils.rs

@@ -412,6 +412,9 @@ impl Reflector {
         self.object = object;
     }
 
+    /// Return a pointer to the memory location at which the JS reflector object is stored.
+    /// Used by Temporary values to root the reflector, as required by the JSAPI rooting
+    /// APIs.
     pub fn rootable(&self) -> **JSObject {
         &self.object as **JSObject
     }

src/components/script/dom/blob.rs

@@ -16,15 +16,15 @@ pub struct Blob {
 }
 
 impl Blob {
-    pub fn new_inherited(window: JS<Window>) -> Blob {
+    pub fn new_inherited(window: &JSRef<Window>) -> Blob {
         Blob {
             reflector_: Reflector::new(),
-            window: window
+            window: window.unrooted()
         }
     }
 
     pub fn new(window: &JSRef<Window>) -> Temporary<Blob> {
-        reflect_dom_object(~Blob::new_inherited(window.unrooted()),
+        reflect_dom_object(~Blob::new_inherited(window),
                            window,
                            BlobBinding::Wrap)
     }

src/components/script/dom/characterdata.rs

@@ -5,7 +5,7 @@
 //! DOM bindings for `CharacterData`.
 
 use dom::bindings::codegen::InheritTypes::CharacterDataDerived;
-use dom::bindings::js::{JS, JSRef};
+use dom::bindings::js::JSRef;
 use dom::bindings::error::{Fallible, ErrorResult, IndexSize};
 use dom::bindings::utils::{Reflectable, Reflector};
 use dom::document::Document;
@@ -31,7 +31,7 @@ impl CharacterDataDerived for EventTarget {
 }
 
 impl CharacterData {
-    pub fn new_inherited(id: NodeTypeId, data: DOMString, document: JS<Document>) -> CharacterData {
+    pub fn new_inherited(id: NodeTypeId, data: DOMString, document: &JSRef<Document>) -> CharacterData {
         CharacterData {
             node: Node::new_inherited(id, document),
             data: data

src/components/script/dom/clientrect.rs

@@ -19,7 +19,7 @@ pub struct ClientRect {
 }
 
 impl ClientRect {
-    pub fn new_inherited(window: JS<Window>,
+    pub fn new_inherited(window: &JSRef<Window>,
                          top: Au, bottom: Au,
                          left: Au, right: Au) -> ClientRect {
         ClientRect {
@@ -28,14 +28,14 @@ impl ClientRect {
             left: left.to_nearest_px() as f32,
             right: right.to_nearest_px() as f32,
             reflector_: Reflector::new(),
-            window: window,
+            window: window.unrooted(),
         }
     }
 
     pub fn new(window: &JSRef<Window>,
                top: Au, bottom: Au,
                left: Au, right: Au) -> Temporary<ClientRect> {
-        let rect = ClientRect::new_inherited(window.unrooted(), top, bottom, left, right);
+        let rect = ClientRect::new_inherited(window, top, bottom, left, right);
         reflect_dom_object(~rect, window, ClientRectBinding::Wrap)
     }
 }

src/components/script/dom/clientrectlist.rs

@@ -16,18 +16,18 @@ pub struct ClientRectList {
 }
 
 impl ClientRectList {
-    pub fn new_inherited(window: JS<Window>,
+    pub fn new_inherited(window: &JSRef<Window>,
                          rects: Vec<JSRef<ClientRect>>) -> ClientRectList {
         ClientRectList {
             reflector_: Reflector::new(),
             rects: rects.iter().map(|rect| rect.unrooted()).collect(),
-            window: window,
+            window: window.unrooted(),
         }
     }
 
     pub fn new(window: &JSRef<Window>,
                rects: Vec<JSRef<ClientRect>>) -> Temporary<ClientRectList> {
-        reflect_dom_object(~ClientRectList::new_inherited(window.unrooted(), rects),
+        reflect_dom_object(~ClientRectList::new_inherited(window, rects),
                            window, ClientRectListBinding::Wrap)
     }
 }

src/components/script/dom/comment.rs

@@ -4,7 +4,7 @@
 
 use dom::bindings::codegen::InheritTypes::CommentDerived;
 use dom::bindings::codegen::BindingDeclarations::CommentBinding;
-use dom::bindings::js::{JS, JSRef, Temporary};
+use dom::bindings::js::{JSRef, Temporary};
 use dom::bindings::error::Fallible;
 use dom::characterdata::CharacterData;
 use dom::document::Document;
@@ -29,14 +29,14 @@ impl CommentDerived for EventTarget {
 }
 
 impl Comment {
-    pub fn new_inherited(text: DOMString, document: JS<Document>) -> Comment {
+    pub fn new_inherited(text: DOMString, document: &JSRef<Document>) -> Comment {
         Comment {
             characterdata: CharacterData::new_inherited(CommentNodeTypeId, text, document)
         }
     }
 
     pub fn new(text: DOMString, document: &JSRef<Document>) -> Temporary<Comment> {
-        let node = Comment::new_inherited(text, document.unrooted());
+        let node = Comment::new_inherited(text, document);
         Node::reflect_node(~node, document, CommentBinding::Wrap)
     }