From c3c87e3d44d8e57ee0035ead33b575372fafc33f Mon Sep 17 00:00:00 2001 From: Keegan McAllister Date: Tue, 10 Dec 2013 13:11:13 -0800 Subject: [PATCH 1/6] Propagate up the right bits of style damage The call to propagate_up was lost in eb1b40db13a13c27269c57bad60433e0597bbbeb. --- src/components/main/layout/layout_task.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/main/layout/layout_task.rs b/src/components/main/layout/layout_task.rs index 0ccdcfd5db0..0909cbf74df 100644 --- a/src/components/main/layout/layout_task.rs +++ b/src/components/main/layout/layout_task.rs @@ -100,7 +100,7 @@ impl PostorderFlowTraversal for ComputeDamageTraversal { fn process(&mut self, flow: &mut Flow) -> bool { let mut damage = flow::base(flow).restyle_damage; for child in flow::child_iter(flow) { - damage.union_in_place(flow::base(*child).restyle_damage) + damage.union_in_place(flow::base(*child).restyle_damage.propagate_up()) } flow::mut_base(flow).restyle_damage = damage; true From dd0bb0892702f952bf7628c63c1320031828fb46 Mon Sep 17 00:00:00 2001 From: Keegan McAllister Date: Tue, 10 Dec 2013 13:18:00 -0800 Subject: [PATCH 2/6] Print restyle damage for debugging --- src/components/main/layout/layout_task.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/main/layout/layout_task.rs b/src/components/main/layout/layout_task.rs index 0909cbf74df..be46d173d0e 100644 --- a/src/components/main/layout/layout_task.rs +++ b/src/components/main/layout/layout_task.rs @@ -120,6 +120,7 @@ impl PreorderFlowTraversal for PropagateDamageTraversal { if self.all_style_damage { flow::mut_base(flow).restyle_damage.union_in_place(RestyleDamage::all()) } + debug!("restyle damage = {:?}", flow::base(flow).restyle_damage); let prop = flow::base(flow).restyle_damage.propagate_down(); if prop.is_nonempty() { From 39fc9eb86881e486f8a9eaf0cae8357c864f49b0 Mon Sep 17 00:00:00 2001 From: Keegan McAllister Date: Tue, 10 Dec 2013 13:42:30 -0800 Subject: [PATCH 3/6] Get rid of reflow_all This refactoring should not alter behavior. --- src/components/script/dom/window.rs | 5 +++-- src/components/script/script_task.rs | 28 ++++++++++------------------ 2 files changed, 13 insertions(+), 20 deletions(-) diff --git a/src/components/script/dom/window.rs b/src/components/script/dom/window.rs index aeb6b317a07..74d785784a4 100644 --- a/src/components/script/dom/window.rs +++ b/src/components/script/dom/window.rs @@ -12,7 +12,7 @@ use dom::node::{AbstractNode, ScriptView}; use dom::location::Location; use dom::navigator::Navigator; -use layout_interface::ReflowForDisplay; +use layout_interface::{ReflowForDisplay, ContentChangedDocumentDamage}; use script_task::{ExitWindowMsg, FireTimerMsg, Page, ScriptChan}; use servo_msg::compositor_msg::ScriptListener; use servo_net::image_cache_task::ImageCacheTask; @@ -189,7 +189,8 @@ impl Window { // FIXME This should probably be ReflowForQuery, not Display. All queries currently // currently rely on the display list, which means we can't destroy it by // doing a query reflow. - self.page.reflow_all(ReflowForDisplay, self.script_chan.clone(), self.compositor); + self.page.damage(ContentChangedDocumentDamage); + self.page.reflow(ReflowForDisplay, self.script_chan.clone(), self.compositor); } pub fn wait_until_safe_to_modify_dom(&self) { diff --git a/src/components/script/script_task.rs b/src/components/script/script_task.rs index 35ebeaf69ce..0c3bf255163 100755 --- a/src/components/script/script_task.rs +++ b/src/components/script/script_task.rs @@ -224,8 +224,11 @@ impl<'self> Iterator<@mut Page> for PageTreeIterator<'self> { impl Page { /// Adds the given damage. - fn damage(&mut self, level: DocumentDamageLevel) { - let root = self.frame.get_ref().document.document().GetDocumentElement(); + pub fn damage(&mut self, level: DocumentDamageLevel) { + let root = match self.frame { + None => return, + Some(ref frame) => frame.document.document().GetDocumentElement() + }; match root { None => {}, Some(root) => { @@ -282,7 +285,7 @@ impl Page { /// computation to finish. /// /// This function fails if there is no root frame. - fn reflow(&mut self, goal: ReflowGoal, script_chan: ScriptChan, compositor: @ScriptListener) { + pub fn reflow(&mut self, goal: ReflowGoal, script_chan: ScriptChan, compositor: @ScriptListener) { let root = match self.frame { None => return, Some(ref frame) => { @@ -325,19 +328,6 @@ impl Page { } } - /// Reflows the entire document. - /// - /// FIXME: This should basically never be used. - pub fn reflow_all(&mut self, goal: ReflowGoal, script_chan: ScriptChan, compositor: @ScriptListener) { - if self.frame.is_some() { - self.damage(ContentChangedDocumentDamage); - } - - //FIXME: In the case where an initial reflow is required, we should always - // ReflowForDisplay, regardless of the original goal. - self.reflow(goal, script_chan, compositor) - } - pub fn initialize_js_info(&mut self, js_context: @Cx, global: *JSObject) { // Note that the order that these variables are initialized is _not_ arbitrary. Switching them around // can -- and likely will -- lead to things breaking. @@ -599,7 +589,8 @@ impl ScriptTask { } // We don't know what the script changed, so for now we will do a total redisplay. - page.reflow_all(ReflowForDisplay, self.chan.clone(), self.compositor); + page.damage(ContentChangedDocumentDamage); + page.reflow(ReflowForDisplay, self.chan.clone(), self.compositor); } /// Handles a notification that reflow completed. @@ -683,7 +674,8 @@ impl ScriptTask { if *loaded == url { page.url = Some((loaded.clone(), false)); if needs_reflow { - page.reflow_all(ReflowForDisplay, self.chan.clone(), self.compositor); + page.damage(ContentChangedDocumentDamage); + page.reflow(ReflowForDisplay, self.chan.clone(), self.compositor); } return; } From 93e10eaf20e169853d54350d56408bb91d94e343 Mon Sep 17 00:00:00 2001 From: Keegan McAllister Date: Thu, 12 Dec 2013 11:27:30 -0800 Subject: [PATCH 4/6] Call Element::after_set_attr only for null namespace --- src/components/script/dom/element.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/components/script/dom/element.rs b/src/components/script/dom/element.rs index a1670150cf9..a928626fb20 100644 --- a/src/components/script/dom/element.rs +++ b/src/components/script/dom/element.rs @@ -252,19 +252,20 @@ impl<'self> Element { } }); - self.after_set_attr(abstract_self, &namespace, local_name, value, old_raw_value); + if namespace == namespace::Null { + self.after_set_attr(abstract_self, local_name, value, old_raw_value); + } Ok(()) } fn after_set_attr(&mut self, abstract_self: AbstractNode, - namespace: &Namespace, local_name: DOMString, value: DOMString, old_value: Option) { match local_name.as_slice() { - "style" if *namespace == namespace::Null => { + "style" => { self.style_attribute = Some(style::parse_style_attribute(value)) } "id" => { From 0238410b478c1b8af0599e6a2f2869e8fdd29439 Mon Sep 17 00:00:00 2001 From: Keegan McAllister Date: Tue, 10 Dec 2013 15:15:43 -0800 Subject: [PATCH 5/6] Allow setting id, class, style without a full reflow Instead we do selector matching again, then diff the style structs to set the "restyle damage" bits which are used to prune reflow traversals. Also don't force a reflow when timers finish, because individual DOM methods should already take care of that. --- src/components/script/dom/document.rs | 7 ++++++- src/components/script/dom/element.rs | 9 +++++++-- src/components/script/dom/window.rs | 6 +++--- src/components/script/script_task.rs | 3 --- 4 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/components/script/dom/document.rs b/src/components/script/dom/document.rs index 8e0c7676ca8..e86e7465ec3 100644 --- a/src/components/script/dom/document.rs +++ b/src/components/script/dom/document.rs @@ -23,6 +23,7 @@ use dom::htmltitleelement::HTMLTitleElement; use html::hubbub_html_parser::build_element_from_tag; use js::jsapi::{JSObject, JSContext, JSTracer}; use servo_util::tree::{TreeNodeRef, ElementLike}; +use layout_interface::{DocumentDamageLevel, ContentChangedDocumentDamage}; use std::hashmap::HashMap; @@ -319,7 +320,11 @@ impl Document { } pub fn content_changed(&self) { - self.window.content_changed(); + self.damage_and_reflow(ContentChangedDocumentDamage); + } + + pub fn damage_and_reflow(&self, damage: DocumentDamageLevel) { + self.window.damage_and_reflow(damage); } pub fn wait_until_safe_to_modify_dom(&self) { diff --git a/src/components/script/dom/element.rs b/src/components/script/dom/element.rs index a928626fb20..e8acb68f731 100644 --- a/src/components/script/dom/element.rs +++ b/src/components/script/dom/element.rs @@ -18,7 +18,8 @@ use dom::document; use dom::namespace; use dom::namespace::Namespace; use layout_interface::{ContentBoxQuery, ContentBoxResponse, ContentBoxesQuery}; -use layout_interface::{ContentBoxesResponse}; +use layout_interface::{ContentBoxesResponse, ContentChangedDocumentDamage}; +use layout_interface::{MatchSelectorsDocumentDamage}; use style; use servo_util::tree::{TreeNodeRef, ElementLike}; @@ -293,8 +294,12 @@ impl<'self> Element { } if abstract_self.is_in_doc() { + let damage = match local_name.as_slice() { + "style" | "id" | "class" => MatchSelectorsDocumentDamage, + _ => ContentChangedDocumentDamage + }; let document = self.node.owner_doc(); - document.document().content_changed(); + document.document().damage_and_reflow(damage); } } } diff --git a/src/components/script/dom/window.rs b/src/components/script/dom/window.rs index 74d785784a4..9f55e3cf3c5 100644 --- a/src/components/script/dom/window.rs +++ b/src/components/script/dom/window.rs @@ -12,7 +12,7 @@ use dom::node::{AbstractNode, ScriptView}; use dom::location::Location; use dom::navigator::Navigator; -use layout_interface::{ReflowForDisplay, ContentChangedDocumentDamage}; +use layout_interface::{ReflowForDisplay, DocumentDamageLevel}; use script_task::{ExitWindowMsg, FireTimerMsg, Page, ScriptChan}; use servo_msg::compositor_msg::ScriptListener; use servo_net::image_cache_task::ImageCacheTask; @@ -185,11 +185,11 @@ impl Window { self.active_timers.remove(&handle); } - pub fn content_changed(&self) { + pub fn damage_and_reflow(&self, damage: DocumentDamageLevel) { // FIXME This should probably be ReflowForQuery, not Display. All queries currently // currently rely on the display list, which means we can't destroy it by // doing a query reflow. - self.page.damage(ContentChangedDocumentDamage); + self.page.damage(damage); self.page.reflow(ReflowForDisplay, self.script_chan.clone(), self.compositor); } diff --git a/src/components/script/script_task.rs b/src/components/script/script_task.rs index 0c3bf255163..b77b383f4ed 100755 --- a/src/components/script/script_task.rs +++ b/src/components/script/script_task.rs @@ -588,9 +588,6 @@ impl ScriptTask { &rval); } - // We don't know what the script changed, so for now we will do a total redisplay. - page.damage(ContentChangedDocumentDamage); - page.reflow(ReflowForDisplay, self.chan.clone(), self.compositor); } /// Handles a notification that reflow completed. From 53747638a1f4b2f39cb54c453c07a307596b7982 Mon Sep 17 00:00:00 2001 From: Keegan McAllister Date: Wed, 11 Dec 2013 16:00:53 -0800 Subject: [PATCH 6/6] Disable remaining reflow traversal pruning for now We don't reuse Flow objects between reflows, so we have no old values to fall back to. I think this used to work because FlowContexts (as they were called then) were stored in a DOM node's LayoutData and reused. But it's possible that it never really worked, and my testing when I landed the restyle damage code was insufficient (I didn't understand the layout code nearly as well back then). --- src/components/main/layout/layout_task.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/components/main/layout/layout_task.rs b/src/components/main/layout/layout_task.rs index be46d173d0e..501c4fb6cad 100644 --- a/src/components/main/layout/layout_task.rs +++ b/src/components/main/layout/layout_task.rs @@ -15,7 +15,7 @@ use layout::extra::LayoutAuxMethods; use layout::flow::{Flow, ImmutableFlowUtils, MutableFlowUtils, PreorderFlowTraversal}; use layout::flow::{PostorderFlowTraversal}; use layout::flow; -use layout::incremental::{RestyleDamage, BubbleWidths}; +use layout::incremental::{RestyleDamage}; use layout::util::{LayoutData, LayoutDataAccess}; use extra::arc::{Arc, RWArc, MutexArc}; @@ -143,10 +143,13 @@ impl<'self> PostorderFlowTraversal for BubbleWidthsTraversal<'self> { true } + // FIXME: We can't prune until we start reusing flows + /* #[inline] fn should_prune(&mut self, flow: &mut Flow) -> bool { flow::mut_base(flow).restyle_damage.lacks(BubbleWidths) } + */ } /// The assign-widths traversal. In Gecko this corresponds to `Reflow`. @@ -372,10 +375,9 @@ impl LayoutTask { layout_context: &mut LayoutContext) { let _ = layout_root.traverse_postorder(&mut BubbleWidthsTraversal(layout_context)); - // FIXME(kmc): We want to do - // for flow in layout_root.traverse_preorder_prune(|f| - // f.restyle_damage().lacks(Reflow)) - // but FloatContext values can't be reused, so we need to recompute them every time. + // FIXME(kmc): We want to prune nodes without the Reflow restyle damage + // bit, but FloatContext values can't be reused, so we need to + // recompute them every time. // NOTE: this currently computes borders, so any pruning should separate that operation out. let _ = layout_root.traverse_preorder(&mut AssignWidthsTraversal(layout_context));