From c93d13b0da1bc423ccff2520da01a558c6b3d8ec Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sat, 5 Oct 2013 21:42:13 +0200 Subject: [PATCH 1/4] Don't require passing a root element to Document::new (needed for issue #888). --- src/components/script/dom/document.rs | 158 +++++++++++++--------- src/components/script/dom/domparser.rs | 38 +++--- src/components/script/dom/htmldocument.rs | 25 ++-- src/components/script/script_task.rs | 89 +++++++----- 4 files changed, 183 insertions(+), 127 deletions(-) diff --git a/src/components/script/dom/document.rs b/src/components/script/dom/document.rs index 293ed9cb7e5..c780def9400 100644 --- a/src/components/script/dom/document.rs +++ b/src/components/script/dom/document.rs @@ -76,6 +76,12 @@ impl AbstractDocument { _ => fail!("attempt to downcast a non-HTMLDocument to HTMLDocument") } } + + pub fn set_root(&mut self, root: AbstractNode) { + self.with_mut_base(|document| { + document.set_root(root); + }); + } } pub enum DocumentType { @@ -85,7 +91,7 @@ pub enum DocumentType { } pub struct Document { - root: AbstractNode, + root: Option>, wrapper: WrapperCache, window: Option<@mut Window>, doctype: DocumentType, @@ -94,9 +100,9 @@ pub struct Document { impl Document { #[fixed_stack_segment] - pub fn new(root: AbstractNode, window: Option<@mut Window>, doctype: DocumentType) -> Document { + pub fn new(window: Option<@mut Window>, doctype: DocumentType) -> Document { Document { - root: root, + root: None, wrapper: WrapperCache::new(), window: window, doctype: doctype, @@ -104,14 +110,22 @@ impl Document { } } + pub fn set_root(&mut self, root: AbstractNode) { + self.root = Some(root); + } + pub fn Constructor(owner: @mut Window) -> Fallible { + let cx = owner.page.js_info.get_ref().js_compartment.cx.ptr; + + let mut document = AbstractDocument::as_abstract(cx, @mut Document::new(None, XML)); + let root = @HTMLHtmlElement { htmlelement: HTMLElement::new(HTMLHtmlElementTypeId, ~"html") }; - - let cx = owner.page.js_info.get_ref().js_compartment.cx.ptr; let root = unsafe { Node::as_abstract_node(cx, root) }; - Ok(AbstractDocument::as_abstract(cx, @mut Document::new(root, None, XML))) + document.set_root(root); + + Ok(document) } } @@ -208,7 +222,7 @@ impl Document { } pub fn GetDocumentElement(&self) -> Option> { - Some(self.root) + self.root } fn get_cx(&self) -> *JSContext { @@ -288,20 +302,25 @@ impl Document { fail!("no SVG document yet") }, _ => { - let _ = for node in self.root.traverse_preorder() { - if node.type_id() != ElementNodeTypeId(HTMLTitleElementTypeId) { - loop; - } - for child in node.children() { - if child.is_text() { - do child.with_imm_text() |text| { - let s = text.element.Data(); - title = title + null_str_as_empty(&s); + match self.root { + None => {}, + Some(root) => { + let _ = for node in root.traverse_preorder() { + if node.type_id() != ElementNodeTypeId(HTMLTitleElementTypeId) { + loop; } - } + for child in node.children() { + if child.is_text() { + do child.with_imm_text() |text| { + let s = text.element.Data(); + title = title + null_str_as_empty(&s); + } + } + } + break; + }; } - break; - }; + } } } let v: ~[&str] = title.word_iter().collect(); @@ -317,34 +336,39 @@ impl Document { }, _ => { let (_scope, cx) = self.get_scope_and_cx(); - let _ = for node in self.root.traverse_preorder() { - if node.type_id() != ElementNodeTypeId(HTMLHeadElementTypeId) { - loop; - } - let mut has_title = false; - for child in node.children() { - if child.type_id() != ElementNodeTypeId(HTMLTitleElementTypeId) { - loop; - } - has_title = true; - for title_child in child.children() { - child.remove_child(title_child); - } - child.add_child(self.CreateTextNode(title)); - break; - } - if !has_title { - let new_title = @HTMLTitleElement { - htmlelement: HTMLElement::new(HTMLTitleElementTypeId, ~"title") + match self.root { + None => {}, + Some(root) => { + let _ = for node in root.traverse_preorder() { + if node.type_id() != ElementNodeTypeId(HTMLHeadElementTypeId) { + loop; + } + let mut has_title = false; + for child in node.children() { + if child.type_id() != ElementNodeTypeId(HTMLTitleElementTypeId) { + loop; + } + has_title = true; + for title_child in child.children() { + child.remove_child(title_child); + } + child.add_child(self.CreateTextNode(title)); + break; + } + if !has_title { + let new_title = @HTMLTitleElement { + htmlelement: HTMLElement::new(HTMLTitleElementTypeId, ~"title") + }; + let new_title = unsafe { + Node::as_abstract_node(cx, new_title) + }; + new_title.add_child(self.CreateTextNode(title)); + node.add_child(new_title); + } + break; }; - let new_title = unsafe { - Node::as_abstract_node(cx, new_title) - }; - new_title.add_child(self.CreateTextNode(title)); - node.add_child(new_title); } - break; - }; + } } } Ok(()) @@ -440,15 +464,20 @@ impl Document { pub fn createHTMLCollection(&self, callback: &fn(elem: &Element) -> bool) -> @mut HTMLCollection { let mut elements = ~[]; - let _ = for child in self.root.traverse_preorder() { - if child.is_element() { - do child.with_imm_element |elem| { - if callback(elem) { - elements.push(child); + match self.root { + None => {}, + Some(root) => { + let _ = for child in root.traverse_preorder() { + if child.is_element() { + do child.with_imm_element |elem| { + if callback(elem) { + elements.push(child); + } + } } - } + }; } - }; + } let (scope, cx) = self.get_scope_and_cx(); HTMLCollection::new(elements, cx, scope) } @@ -469,16 +498,21 @@ impl Document { impl Traceable for Document { #[fixed_stack_segment] fn trace(&self, tracer: *mut JSTracer) { - unsafe { - (*tracer).debugPrinter = ptr::null(); - (*tracer).debugPrintIndex = -1; - do "root".to_c_str().with_ref |name| { - (*tracer).debugPrintArg = name as *libc::c_void; - debug!("tracing root node"); - do self.root.with_base |node| { - JS_CallTracer(tracer as *JSTracer, - node.wrapper.wrapper, - JSTRACE_OBJECT as u32); + match self.root { + None => {}, + Some(root) => { + unsafe { + (*tracer).debugPrinter = ptr::null(); + (*tracer).debugPrintIndex = -1; + do "root".to_c_str().with_ref |name| { + (*tracer).debugPrintArg = name as *libc::c_void; + debug!("tracing root node"); + do root.with_base |node| { + JS_CallTracer(tracer as *JSTracer, + node.wrapper.wrapper, + JSTRACE_OBJECT as u32); + } + } } } } diff --git a/src/components/script/dom/domparser.rs b/src/components/script/dom/domparser.rs index 82f94262ea8..685019da52a 100644 --- a/src/components/script/dom/domparser.rs +++ b/src/components/script/dom/domparser.rs @@ -41,26 +41,26 @@ impl DOMParser { _s: &DOMString, ty: DOMParserBinding::SupportedType) -> Fallible { - unsafe { - let root = @HTMLHtmlElement { - htmlelement: HTMLElement::new(HTMLHtmlElementTypeId, ~"html") - }; - - let root = Node::as_abstract_node((*self.owner.page).js_info.get_ref().js_compartment.cx.ptr, root); - let cx = (*self.owner.page).js_info.get_ref().js_compartment.cx.ptr; - - match ty { - Text_html => { - Ok(HTMLDocument::new(root, None)) - } - Text_xml => { - Ok(AbstractDocument::as_abstract(cx, @mut Document::new(root, None, XML))) - } - _ => { - fail!("unsupported document type") - } + let cx = (*self.owner.page).js_info.get_ref().js_compartment.cx.ptr; + let mut document = match ty { + Text_html => { + HTMLDocument::new(None) } - } + Text_xml => { + AbstractDocument::as_abstract(cx, @mut Document::new(None, XML)) + } + _ => { + fail!("unsupported document type") + } + }; + + let root = @HTMLHtmlElement { + htmlelement: HTMLElement::new(HTMLHtmlElementTypeId, ~"html") + }; + let root = unsafe { Node::as_abstract_node(cx, root) }; + document.set_root(root); + + Ok(document) } } diff --git a/src/components/script/dom/htmldocument.rs b/src/components/script/dom/htmldocument.rs index 6e70ad236a5..c96785bdc99 100644 --- a/src/components/script/dom/htmldocument.rs +++ b/src/components/script/dom/htmldocument.rs @@ -24,9 +24,9 @@ pub struct HTMLDocument { } impl HTMLDocument { - pub fn new(root: AbstractNode, window: Option<@mut Window>) -> AbstractDocument { + pub fn new(window: Option<@mut Window>) -> AbstractDocument { let doc = @mut HTMLDocument { - parent: Document::new(root, window, HTML) + parent: Document::new(window, HTML) }; let compartment = window.get_ref().page.js_info.get_ref().js_compartment; @@ -70,14 +70,19 @@ impl HTMLDocument { } pub fn GetHead(&self) -> Option> { - let mut headNode: Option> = None; - let _ = for child in self.parent.root.traverse_preorder() { - if child.type_id() == ElementNodeTypeId(HTMLHeadElementTypeId) { - headNode = Some(child); - break; + match self.parent.root { + None => None, + Some(root) => { + let mut headNode: Option> = None; + let _ = for child in root.traverse_preorder() { + if child.type_id() == ElementNodeTypeId(HTMLHeadElementTypeId) { + headNode = Some(child); + break; + } + }; + headNode } - }; - headNode + } } pub fn Images(&self) -> @mut HTMLCollection { @@ -216,4 +221,4 @@ impl Traceable for HTMLDocument { fn trace(&self, tracer: *mut JSTracer) { self.parent.trace(tracer); } -} \ No newline at end of file +} diff --git a/src/components/script/script_task.rs b/src/components/script/script_task.rs index 8ca1eb4a5bf..1ccdefabf31 100644 --- a/src/components/script/script_task.rs +++ b/src/components/script/script_task.rs @@ -217,20 +217,26 @@ impl<'self> Iterator<@mut Page> for PageTreeIterator<'self> { impl Page { /// Adds the given damage. fn damage(&mut self, level: DocumentDamageLevel) { - match self.damage { - None => {} - Some(ref mut damage) => { - // FIXME(pcwalton): This is wrong. We should trace up to the nearest ancestor. - damage.root = do self.frame.get_ref().document.with_base |doc| { doc.root }; - damage.level.add(level); - return - } - } + let root = do self.frame.get_ref().document.with_base |doc| { doc.root }; + match root { + None => {}, + Some(root) => { + match self.damage { + None => {} + Some(ref mut damage) => { + // FIXME(pcwalton): This is wrong. We should trace up to the nearest ancestor. + damage.root = root; + damage.level.add(level); + return + } + } - self.damage = Some(DocumentDamage { - root: do self.frame.get_ref().document.with_base |doc| { doc.root }, - level: level, - }) + self.damage = Some(DocumentDamage { + root: root, + level: level, + }) + } + }; } /// Sends a ping to layout and waits for the response. The response will arrive when the @@ -269,27 +275,34 @@ impl Page { /// /// This function fails if there is no root frame. fn reflow(&mut self, goal: ReflowGoal, script_chan: ScriptChan, compositor: @ScriptListener) { - - debug!("script: performing reflow for goal %?", goal); - - // Now, join the layout so that they will see the latest changes we have made. - self.join_layout(); - - // Tell the user that we're performing layout. - compositor.set_ready_state(PerformingLayout); - - // Layout will let us know when it's done. - let (join_port, join_chan) = comm::stream(); - self.layout_join_port = Some(join_port); - - self.last_reflow_id += 1; - - match self.frame { + let root = match self.frame { None => fail!(~"Tried to relayout with no root frame!"), Some(ref frame) => { + do frame.document.with_base |doc| { + doc.root + } + } + }; + match root { + None => {}, + Some(root) => { + debug!("script: performing reflow for goal %?", goal); + + // Now, join the layout so that they will see the latest changes we have made. + self.join_layout(); + + // Tell the user that we're performing layout. + compositor.set_ready_state(PerformingLayout); + + // Layout will let us know when it's done. + let (join_port, join_chan) = comm::stream(); + self.layout_join_port = Some(join_port); + + self.last_reflow_id += 1; + // Send new document and relevant styles to layout. let reflow = ~Reflow { - document_root: do frame.document.with_base |doc| { doc.root }, + document_root: root, url: self.url.get_ref().first().clone(), goal: goal, window_size: self.window_size.get(), @@ -299,11 +312,11 @@ impl Page { id: self.last_reflow_id, }; - self.layout_chan.send(ReflowMsg(reflow)) + self.layout_chan.send(ReflowMsg(reflow)); + + debug!("script: layout forked") } } - - debug!("script: layout forked") } /// Reflows the entire document. @@ -709,9 +722,10 @@ impl ScriptTask { page.next_subpage_id.clone(), self.constellation_chan.clone()); - let HtmlParserResult {root, discovery_port, url: final_url} = html_parsing_result; + let mut document = HTMLDocument::new(Some(window)); - let document = HTMLDocument::new(root, Some(window)); + let HtmlParserResult {root, discovery_port, url: final_url} = html_parsing_result; + document.set_root(root); // Create the root frame. page.frame = Some(Frame { @@ -817,8 +831,11 @@ impl ScriptTask { let root = do page.frame.expect("root frame is None").document.with_base |doc| { doc.root }; + if root.is_none() { + return; + } let (port, chan) = comm::stream(); - match page.query_layout(HitTestQuery(root, point, chan), port) { + match page.query_layout(HitTestQuery(root.unwrap(), point, chan), port) { Ok(node) => match node { HitTestResponse(node) => { debug!("clicked on %s", node.debug_str()); From 17796725f40d2e7d6ffa6523c8dfcf4823784bc5 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sun, 6 Oct 2013 09:02:12 +0200 Subject: [PATCH 2/4] Address review comments. --- src/components/script/dom/document.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/components/script/dom/document.rs b/src/components/script/dom/document.rs index c780def9400..c7204965e35 100644 --- a/src/components/script/dom/document.rs +++ b/src/components/script/dom/document.rs @@ -77,7 +77,7 @@ impl AbstractDocument { } } - pub fn set_root(&mut self, root: AbstractNode) { + pub fn set_root(&self, root: AbstractNode) { self.with_mut_base(|document| { document.set_root(root); }); @@ -305,7 +305,7 @@ impl Document { match self.root { None => {}, Some(root) => { - let _ = for node in root.traverse_preorder() { + for node in root.traverse_preorder() { if node.type_id() != ElementNodeTypeId(HTMLTitleElementTypeId) { loop; } @@ -339,7 +339,7 @@ impl Document { match self.root { None => {}, Some(root) => { - let _ = for node in root.traverse_preorder() { + for node in root.traverse_preorder() { if node.type_id() != ElementNodeTypeId(HTMLHeadElementTypeId) { loop; } @@ -467,7 +467,7 @@ impl Document { match self.root { None => {}, Some(root) => { - let _ = for child in root.traverse_preorder() { + for child in root.traverse_preorder() { if child.is_element() { do child.with_imm_element |elem| { if callback(elem) { From 179582d939e4ee1797ed5669bf048bcaa6c3c442 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sun, 6 Oct 2013 09:15:38 +0200 Subject: [PATCH 3/4] Remove unnecessary mutability. --- src/components/script/dom/document.rs | 2 +- src/components/script/dom/domparser.rs | 2 +- src/components/script/script_task.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/script/dom/document.rs b/src/components/script/dom/document.rs index c7204965e35..98260344bca 100644 --- a/src/components/script/dom/document.rs +++ b/src/components/script/dom/document.rs @@ -117,7 +117,7 @@ impl Document { pub fn Constructor(owner: @mut Window) -> Fallible { let cx = owner.page.js_info.get_ref().js_compartment.cx.ptr; - let mut document = AbstractDocument::as_abstract(cx, @mut Document::new(None, XML)); + let document = AbstractDocument::as_abstract(cx, @mut Document::new(None, XML)); let root = @HTMLHtmlElement { htmlelement: HTMLElement::new(HTMLHtmlElementTypeId, ~"html") diff --git a/src/components/script/dom/domparser.rs b/src/components/script/dom/domparser.rs index 685019da52a..ddfa8d5280f 100644 --- a/src/components/script/dom/domparser.rs +++ b/src/components/script/dom/domparser.rs @@ -42,7 +42,7 @@ impl DOMParser { ty: DOMParserBinding::SupportedType) -> Fallible { let cx = (*self.owner.page).js_info.get_ref().js_compartment.cx.ptr; - let mut document = match ty { + let document = match ty { Text_html => { HTMLDocument::new(None) } diff --git a/src/components/script/script_task.rs b/src/components/script/script_task.rs index 1ccdefabf31..cf711c1fc68 100644 --- a/src/components/script/script_task.rs +++ b/src/components/script/script_task.rs @@ -722,7 +722,7 @@ impl ScriptTask { page.next_subpage_id.clone(), self.constellation_chan.clone()); - let mut document = HTMLDocument::new(Some(window)); + let document = HTMLDocument::new(Some(window)); let HtmlParserResult {root, discovery_port, url: final_url} = html_parsing_result; document.set_root(root); From e43505d6413c66e2ffa1d5a08210e77d2516057c Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Mon, 7 Oct 2013 15:17:47 +0200 Subject: [PATCH 4/4] Remove Document::set_root. All callers have an AbstractDocument, and this makes my life easier later. --- src/components/script/dom/document.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/components/script/dom/document.rs b/src/components/script/dom/document.rs index 98260344bca..853e7e4a080 100644 --- a/src/components/script/dom/document.rs +++ b/src/components/script/dom/document.rs @@ -79,7 +79,7 @@ impl AbstractDocument { pub fn set_root(&self, root: AbstractNode) { self.with_mut_base(|document| { - document.set_root(root); + document.root = Some(root); }); } } @@ -110,10 +110,6 @@ impl Document { } } - pub fn set_root(&mut self, root: AbstractNode) { - self.root = Some(root); - } - pub fn Constructor(owner: @mut Window) -> Fallible { let cx = owner.page.js_info.get_ref().js_compartment.cx.ptr;