From 2faf5b270fac715ee8b84c85c8d6ed493303865f Mon Sep 17 00:00:00 2001 From: Clark Gaebel Date: Wed, 15 Oct 2014 16:27:40 -0700 Subject: [PATCH] Fix image_dynamic_remove reftest with incremental layout turned out This also adds some extra debugging infrastructure which I found useful tracking this bug down. A regression in the br reftests is also uncovered by this patch, which I'll work on fixing next. r? @pcwalton --- components/layout/block.rs | 9 ++++---- components/layout/construct.rs | 20 +++++++++++------ components/layout/flow.rs | 11 +++++++--- components/layout/fragment.rs | 4 ++++ components/layout/inline.rs | 34 ++++++++++++++++++++++++----- components/layout/layout_task.rs | 1 + components/layout/table.rs | 5 ++--- components/layout/table_cell.rs | 4 ++-- components/layout/table_colgroup.rs | 2 +- components/layout/table_row.rs | 5 ++--- components/layout/table_rowgroup.rs | 4 ++-- components/layout/traversal.rs | 2 +- components/layout/wrapper.rs | 30 +++++++++++++++++-------- tests/ref/basic.list | 1 + tests/ref/img_simple.html | 3 +++ tests/ref/img_simple_ref.html | 2 ++ 16 files changed, 96 insertions(+), 41 deletions(-) create mode 100644 tests/ref/img_simple.html create mode 100644 tests/ref/img_simple_ref.html diff --git a/components/layout/block.rs b/components/layout/block.rs index 62c40da0672..5f89cf7f825 100644 --- a/components/layout/block.rs +++ b/components/layout/block.rs @@ -811,7 +811,7 @@ impl BlockFlow { pub fn assign_block_size_block_base<'a>(&mut self, layout_context: &'a LayoutContext<'a>, margins_may_collapse: MarginsMayCollapseFlag) { - let _scope = layout_debug_scope!("assign_block_size_block_base {:s}", self.base.debug_id()); + let _scope = layout_debug_scope!("assign_block_size_block_base {:x}", self.base.debug_id()); // Our current border-box position. let mut cur_b = Au(0); @@ -1513,7 +1513,7 @@ impl Flow for BlockFlow { /// This function must decide minimum/preferred inline-sizes based on its children's /// inline-sizes and the dimensions of any fragments it is responsible for flowing. fn bubble_inline_sizes(&mut self) { - let _scope = layout_debug_scope!("block::bubble_inline_sizes {:s}", self.base.debug_id()); + let _scope = layout_debug_scope!("block::bubble_inline_sizes {:x}", self.base.debug_id()); let mut flags = self.base.flags; flags.set_has_left_floated_descendants(false); @@ -1583,7 +1583,7 @@ impl Flow for BlockFlow { /// Dual fragments consume some inline-size first, and the remainder is assigned to all child /// (block) contexts. fn assign_inline_sizes(&mut self, layout_context: &LayoutContext) { - let _scope = layout_debug_scope!("block::assign_inline_sizes {:s}", self.base.debug_id()); + let _scope = layout_debug_scope!("block::assign_inline_sizes {:x}", self.base.debug_id()); debug!("assign_inline_sizes({}): assigning inline_size for flow", if self.is_float() { @@ -1683,7 +1683,7 @@ impl Flow for BlockFlow { fn assign_block_size<'a>(&mut self, ctx: &'a LayoutContext<'a>) { if self.is_replaced_content() { - let _scope = layout_debug_scope!("assign_replaced_block_size_if_necessary {:s}", + let _scope = layout_debug_scope!("assign_replaced_block_size_if_necessary {:x}", self.base.debug_id()); // Assign block-size for fragment if it is an image fragment. @@ -2567,4 +2567,3 @@ fn propagate_column_inline_sizes_to_child(kid: &mut Flow, *inline_start_margin_edge = *inline_start_margin_edge + inline_size } } - diff --git a/components/layout/construct.rs b/components/layout/construct.rs index c795f5025f4..fb550b6cf2a 100644 --- a/components/layout/construct.rs +++ b/components/layout/construct.rs @@ -66,6 +66,7 @@ use sync::Arc; use url::Url; /// The results of flow construction for a DOM node. +#[deriving(Clone)] pub enum ConstructionResult { /// This node contributes nothing at all (`display: none`). Alternately, this is what newly /// created nodes have their `ConstructionResult` set to. @@ -84,22 +85,25 @@ pub enum ConstructionResult { impl ConstructionResult { pub fn swap_out(&mut self, layout_context: &LayoutContext) -> ConstructionResult { if layout_context.shared.opts.incremental_layout { - match *self { - NoConstructionResult => - return NoConstructionResult, - FlowConstructionResult(ref flow_ref, ref abs_descendants) => - return FlowConstructionResult((*flow_ref).clone(), (*abs_descendants).clone()), - ConstructionItemConstructionResult(_) => {}, - } + return (*self).clone(); } mem::replace(self, NoConstructionResult) } + + pub fn debug_id(&self) -> uint { + match self { + &NoConstructionResult => 0u, + &ConstructionItemConstructionResult(_) => 0u, + &FlowConstructionResult(ref flow_ref, _) => flow::base(flow_ref.deref()).debug_id(), + } + } } /// Represents the output of flow construction for a DOM node that has not yet resulted in a /// complete flow. Construction items bubble up the tree until they find a `Flow` to be attached /// to. +#[deriving(Clone)] pub enum ConstructionItem { /// Inline fragments and associated {ib} splits that have not yet found flows. InlineFragmentsConstructionItem(InlineFragmentsConstructionResult), @@ -110,6 +114,7 @@ pub enum ConstructionItem { } /// Represents inline fragments and {ib} splits that are bubbling up from an inline. +#[deriving(Clone)] pub struct InlineFragmentsConstructionResult { /// Any {ib} splits that we're bubbling up. pub splits: Vec, @@ -147,6 +152,7 @@ pub struct InlineFragmentsConstructionResult { /// C /// ]) /// ``` +#[deriving(Clone)] pub struct InlineBlockSplit { /// The inline fragments that precede the flow. pub predecessors: InlineFragments, diff --git a/components/layout/flow.rs b/components/layout/flow.rs index 9231e448ba7..3698fe65974 100644 --- a/components/layout/flow.rs +++ b/components/layout/flow.rs @@ -299,6 +299,10 @@ pub trait Flow: fmt::Show + ToString + Sync { LayerId(pointer, fragment_id) } } + + fn debug_print(&self) -> String { + format!("{} - {:x}", self.class(), base(self).debug_id()) + } } impl<'a, E, S: Encoder> Encodable for &'a Flow + 'a { @@ -854,8 +858,9 @@ impl BaseFlow { &self.ref_count } - pub fn debug_id(&self) -> String { - format!("{:p}", self as *const _) + pub fn debug_id(&self) -> uint { + let p = self as *const _; + p as uint } } @@ -1020,7 +1025,7 @@ impl<'a> ImmutableFlowUtils for &'a Flow + 'a { for _ in range(0, level) { indent.push_str("| ") } - debug!("{}+ {}", indent, self.to_string()); + debug!("{}+ {}", indent, self.debug_print()); for kid in imm_child_iter(self) { kid.dump_with_level(level + 1) } diff --git a/components/layout/fragment.rs b/components/layout/fragment.rs index 63992e37413..e3d63e348e6 100644 --- a/components/layout/fragment.rs +++ b/components/layout/fragment.rs @@ -584,6 +584,10 @@ impl Fragment { self.debug_id } + pub fn debug_show(&self) -> String { + format!("{:x} {}", self.debug_id(), self.specific.get_type()) + } + /// Transforms this fragment into another fragment of the given type, with the given size, preserving all /// the other data. pub fn transform(&self, size: LogicalSize, specific: SpecificFragmentInfo) -> Fragment { diff --git a/components/layout/inline.rs b/components/layout/inline.rs index 3fd918502ed..4625a0dd9df 100644 --- a/components/layout/inline.rs +++ b/components/layout/inline.rs @@ -562,7 +562,7 @@ impl LineBreaker { } /// Represents a list of inline fragments, including element ranges. -#[deriving(Encodable)] +#[deriving(Encodable, Clone)] pub struct InlineFragments { /// The fragments themselves. pub fragments: Vec, @@ -576,6 +576,26 @@ impl InlineFragments { } } + pub fn debug_show(&self) -> String { + let mut ret = String::new(); + ret.push_str("[ "); + for (i, fragment) in self.fragments.iter().enumerate() { + if i != 0 { + ret.push_str(", "); + } + + ret.push_str(fragment.debug_show().as_slice()); + } + + if self.is_empty() { + ret.push_str("]"); + } else { + ret.push_str(" ]"); + } + + ret + } + /// Returns the number of inline fragments. pub fn len(&self) -> uint { self.fragments.len() @@ -584,7 +604,7 @@ impl InlineFragments { /// Returns true if this list contains no fragments and false if it contains at least one /// fragment. pub fn is_empty(&self) -> bool { - self.len() == 0 + self.fragments.is_empty() } /// Pushes a new inline fragment. @@ -978,6 +998,10 @@ impl Flow for InlineFlow { InlineFlowClass } + fn debug_print(&self) -> String { + format!("{} - {:x} - {}", self.class(), self.base.debug_id(), self.fragments.debug_show()) + } + fn as_immutable_inline<'a>(&'a self) -> &'a InlineFlow { self } @@ -989,7 +1013,7 @@ impl Flow for InlineFlow { fn bubble_inline_sizes(&mut self) { self.update_restyle_damage(); - let _scope = layout_debug_scope!("inline::bubble_inline_sizes {:s}", self.base.debug_id()); + let _scope = layout_debug_scope!("inline::bubble_inline_sizes {:x}", self.base.debug_id()); let writing_mode = self.base.writing_mode; for kid in self.base.child_iter() { @@ -1007,7 +1031,7 @@ impl Flow for InlineFlow { /// Recursively (top-down) determines the actual inline-size of child contexts and fragments. /// When called on this context, the context has had its inline-size set by the parent context. fn assign_inline_sizes(&mut self, _: &LayoutContext) { - let _scope = layout_debug_scope!("inline::assign_inline_sizes {:s}", self.base.debug_id()); + let _scope = layout_debug_scope!("inline::assign_inline_sizes {:x}", self.base.debug_id()); // Initialize content fragment inline-sizes if they haven't been initialized already. // @@ -1039,7 +1063,7 @@ impl Flow for InlineFlow { /// Calculate and set the block-size of this flow. See CSS 2.1 ยง 10.6.1. fn assign_block_size(&mut self, layout_context: &LayoutContext) { - let _scope = layout_debug_scope!("inline::assign_block_size {:s}", self.base.debug_id()); + let _scope = layout_debug_scope!("inline::assign_block_size {:x}", self.base.debug_id()); // Collect various offsets needed by absolutely positioned inline-block or hypothetical // absolute descendants. diff --git a/components/layout/layout_task.rs b/components/layout/layout_task.rs index c36ad0433f6..31c3b99094e 100644 --- a/components/layout/layout_task.rs +++ b/components/layout/layout_task.rs @@ -159,6 +159,7 @@ impl ImageResponder for LayoutImageResponder { let f: proc(ImageResponseMsg, UntrustedNodeAddress):Send = proc(_, node_address) { let ScriptControlChan(chan) = script_chan; + debug!("Dirtying {:x}", node_address as uint); let mut nodes = SmallVec1::new(); nodes.vec_push(node_address); drop(chan.send_opt(SendEventMsg(id.clone(), ReflowEvent(nodes)))) diff --git a/components/layout/table.rs b/components/layout/table.rs index e066e7adc6a..55312d97267 100644 --- a/components/layout/table.rs +++ b/components/layout/table.rs @@ -159,7 +159,7 @@ impl Flow for TableFlow { /// The maximum min/pref inline-sizes of each column are set from the rows for the automatic /// table layout calculation. fn bubble_inline_sizes(&mut self) { - let _scope = layout_debug_scope!("table::bubble_inline_sizes {:s}", + let _scope = layout_debug_scope!("table::bubble_inline_sizes {:x}", self.block_flow.base.debug_id()); let mut computation = IntrinsicISizesContribution::new(); @@ -233,7 +233,7 @@ impl Flow for TableFlow { /// Recursively (top-down) determines the actual inline-size of child contexts and fragments. /// When called on this context, the context has had its inline-size set by the parent context. fn assign_inline_sizes(&mut self, layout_context: &LayoutContext) { - let _scope = layout_debug_scope!("table::assign_inline_sizes {:s}", + let _scope = layout_debug_scope!("table::assign_inline_sizes {:x}", self.block_flow.base.debug_id()); debug!("assign_inline_sizes({}): assigning inline_size for flow", "table"); @@ -396,4 +396,3 @@ impl ColumnInlineSize { } } } - diff --git a/components/layout/table_cell.rs b/components/layout/table_cell.rs index 0cda4fb922a..d79c4d532b0 100644 --- a/components/layout/table_cell.rs +++ b/components/layout/table_cell.rs @@ -78,7 +78,7 @@ impl Flow for TableCellFlow { /// Minimum/preferred inline-sizes set by this function are used in automatic table layout /// calculation. fn bubble_inline_sizes(&mut self) { - let _scope = layout_debug_scope!("table_cell::bubble_inline_sizes {:s}", + let _scope = layout_debug_scope!("table_cell::bubble_inline_sizes {:x}", self.block_flow.base.debug_id()); self.block_flow.bubble_inline_sizes(); @@ -102,7 +102,7 @@ impl Flow for TableCellFlow { /// When called on this context, the context has had its inline-size set by the parent table /// row. fn assign_inline_sizes(&mut self, ctx: &LayoutContext) { - let _scope = layout_debug_scope!("table_cell::assign_inline_sizes {:s}", + let _scope = layout_debug_scope!("table_cell::assign_inline_sizes {:x}", self.block_flow.base.debug_id()); debug!("assign_inline_sizes({}): assigning inline_size for flow", "table_cell"); diff --git a/components/layout/table_colgroup.rs b/components/layout/table_colgroup.rs index 664d28e8100..5db1dd3ab2a 100644 --- a/components/layout/table_colgroup.rs +++ b/components/layout/table_colgroup.rs @@ -57,7 +57,7 @@ impl Flow for TableColGroupFlow { } fn bubble_inline_sizes(&mut self) { - let _scope = layout_debug_scope!("table_colgroup::bubble_inline_sizes {:s}", + let _scope = layout_debug_scope!("table_colgroup::bubble_inline_sizes {:x}", self.base.debug_id()); for fragment in self.cols.iter() { diff --git a/components/layout/table_row.rs b/components/layout/table_row.rs index 0ee0441e3ca..ea3395704d4 100644 --- a/components/layout/table_row.rs +++ b/components/layout/table_row.rs @@ -162,7 +162,7 @@ impl Flow for TableRowFlow { /// The specified column inline-sizes of children cells are used in fixed table layout /// calculation. fn bubble_inline_sizes(&mut self) { - let _scope = layout_debug_scope!("table_row::bubble_inline_sizes {:s}", + let _scope = layout_debug_scope!("table_row::bubble_inline_sizes {:x}", self.block_flow.base.debug_id()); // Bubble up the specified inline-sizes from child table cells. @@ -209,7 +209,7 @@ impl Flow for TableRowFlow { /// Recursively (top-down) determines the actual inline-size of child contexts and fragments. /// When called on this context, the context has had its inline-size set by the parent context. fn assign_inline_sizes(&mut self, ctx: &LayoutContext) { - let _scope = layout_debug_scope!("table_row::assign_inline_sizes {:s}", + let _scope = layout_debug_scope!("table_row::assign_inline_sizes {:x}", self.block_flow.base.debug_id()); debug!("assign_inline_sizes({}): assigning inline_size for flow", "table_row"); @@ -253,4 +253,3 @@ impl fmt::Show for TableRowFlow { write!(f, "TableRowFlow: {}", self.block_flow.fragment) } } - diff --git a/components/layout/table_rowgroup.rs b/components/layout/table_rowgroup.rs index 1a4b1ce374a..f0a6483515e 100644 --- a/components/layout/table_rowgroup.rs +++ b/components/layout/table_rowgroup.rs @@ -124,7 +124,7 @@ impl Flow for TableRowGroupFlow { /// Also, this function finds the specified column inline-sizes from the first row. These are /// used in fixed table layout calculation. fn bubble_inline_sizes(&mut self) { - let _scope = layout_debug_scope!("table_rowgroup::bubble_inline_sizes {:s}", + let _scope = layout_debug_scope!("table_rowgroup::bubble_inline_sizes {:x}", self.block_flow.base.debug_id()); let mut computation = IntrinsicISizesContribution::new(); @@ -167,7 +167,7 @@ impl Flow for TableRowGroupFlow { /// Recursively (top-down) determines the actual inline-size of child contexts and fragments. /// When called on this context, the context has had its inline-size set by the parent context. fn assign_inline_sizes(&mut self, ctx: &LayoutContext) { - let _scope = layout_debug_scope!("table_rowgroup::assign_inline_sizes {:s}", + let _scope = layout_debug_scope!("table_rowgroup::assign_inline_sizes {:x}", self.block_flow.base.debug_id()); debug!("assign_inline_sizes({}): assigning inline_size for flow", "table_rowgroup"); diff --git a/components/layout/traversal.rs b/components/layout/traversal.rs index c05e67b4dc0..cb2f4a245fa 100644 --- a/components/layout/traversal.rs +++ b/components/layout/traversal.rs @@ -212,9 +212,9 @@ impl<'a> PostorderDomTraversal for ConstructFlows<'a> { if node.has_dirty_descendants() { tnode.set_restyle_damage(RestyleDamage::all()); - debug!("Constructing flow for {}", tnode.debug_id()); let mut flow_constructor = FlowConstructor::new(self.layout_context); flow_constructor.process(&tnode); + debug!("Constructed flow for {:x}: {:x}", tnode.debug_id(), tnode.flow_debug_id()); } // Reset the layout damage in this node. It's been propagated to the diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index 0b78a2f97ac..c4085522692 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -205,17 +205,25 @@ impl<'ln> LayoutNode<'ln> { }) } + pub fn flow_debug_id(self) -> uint { + let layout_data_ref = self.borrow_layout_data(); + match *layout_data_ref { + None => 0u, + Some(ref layout_data) => layout_data.data.flow_construction_result.debug_id() + } + } + /// Iterates over this node and all its descendants, in preorder. /// /// FIXME(pcwalton): Terribly inefficient. We should use parallelism. - pub fn traverse_preorder(&self) -> LayoutTreeIterator<'ln> { + pub fn traverse_preorder(self) -> LayoutTreeIterator<'ln> { let mut nodes = vec!(); gather_layout_nodes(self, &mut nodes, false); LayoutTreeIterator::new(nodes) } /// Returns an iterator over this node's children. - pub fn children(&self) -> LayoutNodeChildrenIterator<'ln> { + pub fn children(self) -> LayoutNodeChildrenIterator<'ln> { // FIXME(zwarich): Remove this when UFCS lands and there is a better way // of disambiguating methods. fn first_child(this: &T) -> Option { @@ -223,7 +231,7 @@ impl<'ln> LayoutNode<'ln> { } LayoutNodeChildrenIterator { - current_node: first_child(self), + current_node: first_child(&self), } } @@ -234,7 +242,7 @@ impl<'ln> LayoutNode<'ln> { /// Resets layout data and styles for the node. /// /// FIXME(pcwalton): Do this as part of fragment building instead of in a traversal. - pub fn initialize_layout_data(&self, chan: LayoutChan) { + pub fn initialize_layout_data(self, chan: LayoutChan) { let mut layout_data_ref = self.mutate_layout_data(); match *layout_data_ref { None => { @@ -248,14 +256,14 @@ impl<'ln> LayoutNode<'ln> { } } - pub fn has_children(&self) -> bool { + pub fn has_children(self) -> bool { self.first_child().is_some() } /// While doing a reflow, the node at the root has no parent, as far as we're /// concerned. This method returns `None` at the reflow root. - pub fn layout_parent_node(&self, shared: &SharedLayoutContext) -> Option> { - let opaque_node: OpaqueNode = OpaqueNodeMethods::from_layout_node(self); + pub fn layout_parent_node(self, shared: &SharedLayoutContext) -> Option> { + let opaque_node: OpaqueNode = OpaqueNodeMethods::from_layout_node(&self); if opaque_node == shared.reflow_root { None } else { @@ -425,12 +433,12 @@ impl<'a> Iterator> for LayoutTreeIterator<'a> { } /// FIXME(pcwalton): This is super inefficient. -fn gather_layout_nodes<'a>(cur: &LayoutNode<'a>, refs: &mut Vec>, postorder: bool) { +fn gather_layout_nodes<'a>(cur: LayoutNode<'a>, refs: &mut Vec>, postorder: bool) { if !postorder { refs.push(cur.clone()); } for kid in cur.children() { - gather_layout_nodes(&kid, refs, postorder) + gather_layout_nodes(kid, refs, postorder) } if postorder { refs.push(cur.clone()); @@ -690,6 +698,10 @@ impl<'ln> ThreadSafeLayoutNode<'ln> { self.node.debug_id() } + pub fn flow_debug_id(self) -> uint { + self.node.flow_debug_id() + } + /// Returns the next sibling of this node. Unsafe and private because this can lead to races. unsafe fn next_sibling(&self) -> Option> { if self.pseudo.is_before() { diff --git a/tests/ref/basic.list b/tests/ref/basic.list index 015d379e01e..d35bc28c0b9 100644 --- a/tests/ref/basic.list +++ b/tests/ref/basic.list @@ -1,3 +1,4 @@ +!= img_simple.html img_simple_ref.html == basic_width_px.html basic_width_em.html == br.html br-ref.html # `?` and `#` in the name is a test for https://github.com/servo/servo/issues/3340 diff --git a/tests/ref/img_simple.html b/tests/ref/img_simple.html new file mode 100644 index 00000000000..056cef562c9 --- /dev/null +++ b/tests/ref/img_simple.html @@ -0,0 +1,3 @@ + +Hello + diff --git a/tests/ref/img_simple_ref.html b/tests/ref/img_simple_ref.html new file mode 100644 index 00000000000..c1ad2a4335d --- /dev/null +++ b/tests/ref/img_simple_ref.html @@ -0,0 +1,2 @@ + +