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

@@ -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
     }
 }
-

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 {
+            return (*self).clone();
-                NoConstructionResult =>
-                    return NoConstructionResult,
-                FlowConstructionResult(ref flow_ref, ref abs_descendants) =>
-                    return FlowConstructionResult((*flow_ref).clone(), (*abs_descendants).clone()),
-                ConstructionItemConstructionResult(_) => {},
-            }
         }
 
         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<InlineBlockSplit>,
@@ -147,6 +152,7 @@ pub struct InlineFragmentsConstructionResult {
 ///             C
 ///         ])
 /// ```
+#[deriving(Clone)]
 pub struct InlineBlockSplit {
     /// The inline fragments that precede the flow.
     pub predecessors: InlineFragments,

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<E>> Encodable<S, E> for &'a Flow + 'a {
@@ -854,8 +858,9 @@ impl BaseFlow {
         &self.ref_count
     }
 
-    pub fn debug_id(&self) -> String {
+    pub fn debug_id(&self) -> uint {
-        format!("{:p}", self as *const _)
+        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)
         }

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<Au>, specific: SpecificFragmentInfo) -> Fragment {

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<Fragment>,
@@ -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.

components/layout/layout_task.rs

@@ -159,6 +159,7 @@ impl ImageResponder<UntrustedNodeAddress> 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))))

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 {
         }
     }
 }
-

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");
 

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() {

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)
     }
 }
-

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");
 

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

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<T: TLayoutNode>(this: &T) -> Option<T> {
@@ -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<LayoutNode<'ln>> {
+    pub fn layout_parent_node(self, shared: &SharedLayoutContext) -> Option<LayoutNode<'ln>> {
-        let opaque_node: OpaqueNode = OpaqueNodeMethods::from_layout_node(self);
+        let opaque_node: OpaqueNode = OpaqueNodeMethods::from_layout_node(&self);
         if opaque_node == shared.reflow_root {
             None
         } else {
@@ -425,12 +433,12 @@ impl<'a> Iterator<LayoutNode<'a>> for LayoutTreeIterator<'a> {
 }
 
 /// FIXME(pcwalton): This is super inefficient.
-fn gather_layout_nodes<'a>(cur: &LayoutNode<'a>, refs: &mut Vec<LayoutNode<'a>>, postorder: bool) {
+fn gather_layout_nodes<'a>(cur: LayoutNode<'a>, refs: &mut Vec<LayoutNode<'a>>, 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<ThreadSafeLayoutNode<'ln>> {
         if self.pseudo.is_before() {

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

tests/ref/img_simple.html

@@ -0,0 +1,3 @@
+<!doctype html>
+Hello
+<img src=400x400_green.png>

tests/ref/img_simple_ref.html

@@ -0,0 +1,2 @@
+<!doctype html>
+<img src=400x400_green.png>