auto merge of #947 : jdm/servo/textcontent-setter2, r=metajack

src/components/main/layout/layout_task.rs

@@ -360,7 +360,7 @@ impl LayoutTask {
         // FIXME(pcwalton): This should probably be *one* channel, but we can't fix this without
         // either select or a filtered recv() that only looks for messages of a given type.
         data.script_join_chan.send(());
-        data.script_chan.send(ReflowCompleteMsg(self.id));
+        data.script_chan.send(ReflowCompleteMsg(self.id, data.id));
     }
 
     /// Handles a query from the script task. This is the main routine that DOM functions like

src/components/script/dom/bindings/codegen/Bindings.conf

@@ -293,7 +293,7 @@ DOMInterfaces = {
     'nativeType': 'AbstractNode<ScriptView>',
     'concreteType': 'Node<ScriptView>',
     'pointerType': '',
-    'needsAbstract': ['appendChild', 'removeChild']
+    'needsAbstract': ['appendChild', 'removeChild', 'textContent']
 },
 
 'NodeList': [

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

@@ -4020,7 +4020,8 @@ def finalizeHook(descriptor, hookName, context):
         assert descriptor.nativeIsISupports
         release = """let val = JS_GetReservedSlot(obj, 0);
 let _: @mut %s = cast::transmute(RUST_JSVAL_TO_PRIVATE(val));
-""" % descriptor.concreteType
+debug!("%s finalize: %%p", this);
+""" % (descriptor.concreteType, descriptor.concreteType)
     #return clearWrapper + release
     return release
 

src/components/script/dom/document.rs

@@ -331,10 +331,7 @@ impl Document {
                         for title_child in child.children() {
                             child.remove_child(title_child);
                         }
-                        let new_text = unsafe { 
+                        child.add_child(self.CreateTextNode(title));
-                            Node::as_abstract_node(cx, @Text::new(title.to_str())) 
-                        };
-                        child.add_child(new_text);
                         break;
                     }
                     if !has_title {
@@ -344,10 +341,7 @@ impl Document {
                         let new_title = unsafe { 
                             Node::as_abstract_node(cx, new_title) 
                         };
-                        let new_text = unsafe {
+                        new_title.add_child(self.CreateTextNode(title));
-                            Node::as_abstract_node(cx, @Text::new(title.to_str()))
-                        };
-                        new_title.add_child(new_text);
                         node.add_child(new_title);
                     }
                     break;
@@ -443,7 +437,7 @@ impl Document {
         self.createHTMLCollection(|elem|
             elem.get_attr("name").is_some() && eq_slice(elem.get_attr("name").unwrap(), name.to_str()))
     }
-    
+
     pub fn createHTMLCollection(&self, callback: &fn(elem: &Element) -> bool) -> @mut HTMLCollection {
         let mut elements = ~[];
         let _ = for child in self.root.traverse_preorder() {
@@ -464,6 +458,12 @@ impl Document {
             window.content_changed()
         }
     }
+
+    pub fn wait_until_safe_to_modify_dom(&self) {
+        for window in self.window.iter() {
+            window.wait_until_safe_to_modify_dom();
+        }
+    }
 }
 
 impl Traceable for Document {

src/components/script/dom/node.rs

@@ -562,11 +562,11 @@ impl Node<ScriptView> {
     pub fn SetNodeValue(&mut self, _val: &DOMString, _rv: &mut ErrorResult) {
     }
 
-    pub fn GetTextContent(&self) -> DOMString {
+    pub fn GetTextContent(&self, abstract_self: AbstractNode<ScriptView>) -> DOMString {
         match self.type_id {
           ElementNodeTypeId(*) => {
             let mut content = ~"";
-            for node in self.abstract.unwrap().traverse_preorder() {
+            for node in abstract_self.traverse_preorder() {
                 if node.is_text() {
                     do node.with_imm_text() |text| {
                         let s = text.parent.Data();
@@ -577,7 +577,7 @@ impl Node<ScriptView> {
             str(content)
           }
           CommentNodeTypeId | TextNodeTypeId => {
-            do self.abstract.unwrap().with_imm_characterdata() |characterdata| {
+            do abstract_self.with_imm_characterdata() |characterdata| {
                 characterdata.Data()
             }
           }
@@ -587,13 +587,73 @@ impl Node<ScriptView> {
         }
     }
 
-    pub fn SetTextContent(&mut self, _val: &DOMString, _rv: &mut ErrorResult) {
+    // http://dom.spec.whatwg.org/#concept-node-replace-all
+    pub fn replace_all(&mut self,
+                       abstract_self: AbstractNode<ScriptView>,
+                       node: Option<AbstractNode<ScriptView>>) {
+        //FIXME: We should batch document notifications that occur here
+        let mut rv = Ok(());
+        for child in abstract_self.children() {
+            self.RemoveChild(abstract_self, child, &mut rv);
+        }
+        match node {
+            None => {},
+            Some(node) => {
+                self.AppendChild(abstract_self, node, &mut rv);
+            }
+        }
+    }
+
+    pub fn SetTextContent(&mut self,
+                          abstract_self: AbstractNode<ScriptView>,
+                          value: &DOMString,
+                          _rv: &mut ErrorResult) {
+        let is_empty = match value {
+            &str(~"") | &null_string => true,
+            _ => false
+        };
+        match self.type_id {
+          ElementNodeTypeId(*) => {
+            let node = if is_empty {
+                None
+            } else {
+                let text_node = do self.owner_doc.unwrap().with_base |document| {
+                    document.CreateTextNode(value)
+                };
+                Some(text_node)
+            };
+            self.replace_all(abstract_self, node);
+          }
+          CommentNodeTypeId | TextNodeTypeId => {
+            self.wait_until_safe_to_modify_dom();
+
+            do abstract_self.with_mut_characterdata() |characterdata| {
+                characterdata.data = value.to_str();
+
+                // Notify the document that the content of this node is different
+                for doc in self.owner_doc.iter() {
+                    do doc.with_base |doc| {
+                        doc.content_changed();
+                    }
+                }
+            }
+          }
+          DoctypeNodeTypeId => {}
+        }
     }
 
     pub fn InsertBefore(&mut self, _node: AbstractNode<ScriptView>, _child: Option<AbstractNode<ScriptView>>, _rv: &mut ErrorResult) -> AbstractNode<ScriptView> {
         fail!("stub")
     }
 
+    fn wait_until_safe_to_modify_dom(&self) {
+        for doc in self.owner_doc.iter() {
+            do doc.with_base |doc| {
+                doc.wait_until_safe_to_modify_dom();
+            }
+        }
+    }
+
     pub fn AppendChild(&mut self,
                        abstract_self: AbstractNode<ScriptView>,
                        node: AbstractNode<ScriptView>,
@@ -626,6 +686,8 @@ impl Node<ScriptView> {
         // TODO: Should we handle WRONG_DOCUMENT_ERR here?
 
         if rv.is_ok() {
+            self.wait_until_safe_to_modify_dom();
+
             // If the node already exists it is removed from current parent node.
             node.parent_node().map(|parent| parent.remove_child(node));
             abstract_self.add_child(node);
@@ -659,6 +721,8 @@ impl Node<ScriptView> {
             *rv = Err(NotFound);
         }
         if rv.is_ok() {
+            self.wait_until_safe_to_modify_dom();
+
             abstract_self.remove_child(node);
             self.remove_from_doc();
         }

src/components/script/dom/window.rs

@@ -173,6 +173,12 @@ impl Window {
         self.page.reflow_all(ReflowForDisplay, self.script_chan.clone(), self.compositor);
     }
 
+    pub fn wait_until_safe_to_modify_dom(&self) {
+        // FIXME: This disables concurrent layout while we are modifying the DOM, since
+        //        our current architecture is entirely unsafe in the presence of races.
+        self.page.join_layout();
+    }
+
     #[fixed_stack_segment]
     pub fn new(cx: *JSContext,
                page: @mut Page,

src/components/script/layout_interface.rs

@@ -105,6 +105,8 @@ pub struct Reflow {
     window_size: Size2D<uint>,
     /// The channel that we send a notification to.
     script_join_chan: Chan<()>,
+    /// Unique identifier
+    id: uint
 }
 
 /// Encapsulates a channel to the layout task.

src/components/script/script_task.rs

@@ -68,7 +68,7 @@ pub enum ScriptMsg {
     /// Fires a JavaScript timeout.
     FireTimerMsg(PipelineId, ~TimerData),
     /// Notifies script that reflow is finished.
-    ReflowCompleteMsg(PipelineId),
+    ReflowCompleteMsg(PipelineId, uint),
     /// Notifies script that window has been resized but to not take immediate action.
     ResizeInactiveMsg(PipelineId, Size2D<uint>),
     /// Exits the constellation.
@@ -106,6 +106,9 @@ pub struct Page {
     /// Pipeline id associated with this page.
     id: PipelineId,
 
+    /// Unique id for last reflow request; used for confirming completion reply.
+    last_reflow_id: uint,
+
     /// The outermost frame containing the document, window, and page URL.
     frame: Option<Frame>,
 
@@ -158,6 +161,7 @@ impl PageTree {
                 url: None,
                 next_subpage_id: SubpageId(0),
                 resize_event: None,
+                last_reflow_id: 0
             },
             inner: ~[],
         }
@@ -216,7 +220,7 @@ impl Page {
 
     /// Sends a ping to layout and waits for the response. The response will arrive when the
     /// layout task has finished any pending request messages.
-    fn join_layout(&mut self) {
+    pub fn join_layout(&mut self) {
         if self.layout_join_port.is_some() {
             let join_port = replace(&mut self.layout_join_port, None);
             match join_port {
@@ -263,6 +267,8 @@ impl Page {
         let (join_port, join_chan) = comm::stream();
         self.layout_join_port = Some(join_port);
 
+        self.last_reflow_id += 1;
+
         match self.frame {
             None => fail!(~"Tried to relayout with no root frame!"),
             Some(ref frame) => {
@@ -275,6 +281,7 @@ impl Page {
                     script_chan: script_chan,
                     script_join_chan: join_chan,
                     damage: replace(&mut self.damage, None).unwrap(),
+                    id: self.last_reflow_id,
                 };
 
                 self.layout_chan.send(ReflowMsg(reflow))
@@ -498,7 +505,7 @@ impl ScriptTask {
                 SendEventMsg(id, event) => self.handle_event(id, event),
                 FireTimerMsg(id, timer_data) => self.handle_fire_timer_msg(id, timer_data),
                 NavigateMsg(direction) => self.handle_navigate_msg(direction),
-                ReflowCompleteMsg(id) => self.handle_reflow_complete_msg(id),
+                ReflowCompleteMsg(id, reflow_id) => self.handle_reflow_complete_msg(id, reflow_id),
                 ResizeInactiveMsg(id, new_size) => self.handle_resize_inactive_msg(id, new_size),
                 ExitMsg => {
                     self.handle_exit_msg();
@@ -576,11 +583,14 @@ impl ScriptTask {
     }
 
     /// Handles a notification that reflow completed.
-    fn handle_reflow_complete_msg(&mut self, pipeline_id: PipelineId) {
+    fn handle_reflow_complete_msg(&mut self, pipeline_id: PipelineId, reflow_id: uint) {
-        debug!("Script: Reflow complete for %?", pipeline_id);
+        debug!("Script: Reflow %? complete for %?", reflow_id, pipeline_id);
-        self.page_tree.find(pipeline_id).expect("ScriptTask: received a load
+        let page_tree = self.page_tree.find(pipeline_id).expect(
-            message for a layout channel that is not associated with this script task. This
+            "ScriptTask: received a load message for a layout channel that is not associated \
-            is a bug.").page.layout_join_port = None;
+             with this script task. This is a bug.");
+        if page_tree.page.last_reflow_id == reflow_id {
+            page_tree.page.layout_join_port = None;
+        }
         self.constellation_chan.send(RendererReadyMsg(pipeline_id));
         self.compositor.set_ready_state(FinishedLoading);
     }

src/test/html/content/test_textcontent.html

@@ -0,0 +1,17 @@
+<html>
+<head>
+<script src="harness.js"></script>
+<script>
+  var div = document.getElementsByTagName('div')[0];
+  is(div.textContent, "this is\n text content");
+  var newContent = "new text con\ntent";
+  div.textContent = newContent;
+  is(div.textContent, newContent);
+  finish();
+</script>
+</head>
+<body>
+<div>this is
+ text content</div>
+</body>
+</html>