auto merge of #1410 : pcwalton/servo/opaque-node, r=pcwalton

This shuts off all layout access to the DOM after flow tree construction, which will allow us to run script concurrently with layout during reflow and display list construction—even without the COW DOM. It also gets us closer to making `AbstractNode` noncopyable, which is important for safety because the JS GC will not trace layout.

r? @kmcallister

src/components/main/layout/block.rs

@@ -585,11 +585,13 @@ impl Flow for BlockFlow {
     /// Dual boxes consume some width first, and the remainder is assigned to all child (block)
     /// contexts.
     fn assign_widths(&mut self, ctx: &mut LayoutContext) {
-        if self.is_float() {
+        debug!("assign_widths({}): assigning width for flow {}",
-            debug!("assign_widths_float: assigning width for flow {}",  self.base.id);
+               if self.is_float() {
-        } else {
+                   "float"
-            debug!("assign_widths_block: assigning width for flow {}",  self.base.id);
+               } else {
-        }
+                   "block"
+               },
+               self.base.id);
 
         if self.is_root {
             debug!("Setting root position");
@@ -613,6 +615,9 @@ impl Flow for BlockFlow {
         for box in self.box.iter() {
             let style = box.style();
 
+            // The text alignment of a block flow is the text alignment of its box's style.
+            self.base.flags.set_text_align(style.Text.text_align);
+
             // Can compute padding here since we know containing block width.
             box.compute_padding(style, remaining_width);
 
@@ -672,7 +677,9 @@ impl Flow for BlockFlow {
             // Per CSS 2.1 § 16.3.1, text decoration propagates to all children in flow.
             //
             // TODO(pcwalton): When we have out-of-flow children, don't unconditionally propagate.
-            child_base.flags.propagate_text_decoration_from_parent(self.base.flags)
+            child_base.flags.propagate_text_decoration_from_parent(self.base.flags);
+
+            child_base.flags.propagate_text_alignment_from_parent(self.base.flags)
         }
     }
 

src/components/main/layout/box.rs

@@ -40,6 +40,7 @@ use layout::float_context::{ClearType, ClearLeft, ClearRight, ClearBoth};
 use layout::flow::Flow;
 use layout::flow;
 use layout::model::{MaybeAuto, specified};
+use layout::util::OpaqueNode;
 
 /// Boxes (`struct Box`) are the leaves of the layout tree. They cannot position themselves. In
 /// general, boxes do not have a simple correspondence with CSS boxes in the specification:
@@ -63,8 +64,8 @@ use layout::model::{MaybeAuto, specified};
 /// FIXME(pcwalton): This can be slimmed down quite a bit.
 #[deriving(Clone)]
 pub struct Box {
-    /// The DOM node that this `Box` originates from.
+    /// An opaque reference to the DOM node that this `Box` originates from.
-    node: AbstractNode<LayoutView>,
+    node: OpaqueNode,
 
     /// The CSS style of this box.
     style: Arc<ComputedValues>,
@@ -250,7 +251,7 @@ impl Box {
         }
 
         Box {
-            node: node,
+            node: OpaqueNode::from_node(&node),
             style: (*nearest_ancestor_element.style()).clone(),
             position: Slot::init(Au::zero_rect()),
             border: Slot::init(Zero::zero()),
@@ -284,9 +285,12 @@ impl Box {
         }
     }
 
+    /// Returns the shared part of the width for computation of minimum and preferred width per
+    /// CSS 2.1.
     fn guess_width(&self) -> Au {
-        if !self.node.is_element() {
+        match self.specific {
-            return Au(0)
+            GenericBox | IframeBox(_) | ImageBox(_) => {}
+            ScannedTextBox(_) | UnscannedTextBox(_) => return Au(0),
         }
 
         let style = self.style();
@@ -921,9 +925,8 @@ impl Box {
                 let left_box = if left_range.length() > 0 {
                     let new_text_box_info = ScannedTextBoxInfo::new(text_box_info.run.clone(), left_range);
                     let new_metrics = new_text_box_info.run.get().metrics_for_range(&left_range);
-                    let new_text_box = Box::new(self.node, ScannedTextBox(new_text_box_info));
+                    Some(self.transform(new_metrics.bounding_box.size,
-                    new_text_box.set_size(new_metrics.bounding_box.size);
+                                        ScannedTextBox(new_text_box_info)))
-                    Some(new_text_box)
                 } else {
                     None
                 };
@@ -931,9 +934,8 @@ impl Box {
                 let right_box = right_range.map_default(None, |range: Range| {
                     let new_text_box_info = ScannedTextBoxInfo::new(text_box_info.run.clone(), range);
                     let new_metrics = new_text_box_info.run.get().metrics_for_range(&range);
-                    let new_text_box = Box::new(self.node, ScannedTextBox(new_text_box_info));
+                    Some(self.transform(new_metrics.bounding_box.size,
-                    new_text_box.set_size(new_metrics.bounding_box.size);
+                                        ScannedTextBox(new_text_box_info)))
-                    Some(new_text_box)
                 });
 
                 if pieces_processed_count == 1 || left_box.is_none() {

src/components/main/layout/display_list_builder.rs

@@ -6,26 +6,20 @@
 
 use layout::box::Box;
 use layout::context::LayoutContext;
-use std::cast::transmute;
+use layout::util::OpaqueNode;
-use script::dom::node::AbstractNode;
 
 use gfx;
 use style;
 
-/// Display list data is usually an AbstractNode with view () to indicate
-/// that nodes in this view shoud not really be touched. The idea is to
-/// store the nodes in the display list and have layout transmute them.
 pub trait ExtraDisplayListData {
     fn new(box: &Box) -> Self;
 }
 
 pub type Nothing = ();
 
-impl ExtraDisplayListData for AbstractNode<()> {
+impl ExtraDisplayListData for OpaqueNode {
-    fn new(box: &Box) -> AbstractNode<()> {
+    fn new(box: &Box) -> OpaqueNode {
-        unsafe {
+        box.node
-            transmute(box.node)
-        }
     }
 }
 

src/components/main/layout/flow.rs

@@ -44,6 +44,7 @@ use servo_util::geometry::Au;
 use std::cast;
 use std::cell::Cell;
 use style::ComputedValues;
+use style::computed_values::text_align;
 
 /// Virtual methods that make up a float context.
 ///
@@ -313,6 +314,16 @@ pub struct FlowFlags(u8);
 /// NB: If you update this field, you must update the bitfields below.
 static TEXT_DECORATION_OVERRIDE_BITMASK: u8 = 0b00001110;
 
+/// The bitmask of flags that represent the text alignment field.
+///
+/// NB: If you update this field, you must update the bitfields below.
+static TEXT_ALIGN_BITMASK: u8 = 0b00110000;
+
+/// The number of bits we must shift off to handle the text alignment field.
+///
+/// NB: If you update this field, you must update the bitfields below.
+static TEXT_ALIGN_SHIFT: u8 = 4;
+
 impl FlowFlags {
     /// Creates a new set of flow flags from the given style.
     fn new(style: &ComputedValues) -> FlowFlags {
@@ -328,6 +339,11 @@ impl FlowFlags {
     pub fn propagate_text_decoration_from_parent(&mut self, parent: FlowFlags) {
         *self = FlowFlags(**self | (*parent & TEXT_DECORATION_OVERRIDE_BITMASK))
     }
+
+    /// Propagates text alignment flags from an appropriate parent flow per CSS 2.1.
+    pub fn propagate_text_alignment_from_parent(&mut self, parent: FlowFlags) {
+        *self = FlowFlags(**self | (*parent & TEXT_ALIGN_BITMASK))
+    }
 }
 
 // Whether we need an in-order traversal.
@@ -348,6 +364,19 @@ bitfield!(FlowFlags, override_overline, set_override_overline, 0x04)
 // NB: If you update this, you need to update TEXT_DECORATION_OVERRIDE_BITMASK.
 bitfield!(FlowFlags, override_line_through, set_override_line_through, 0x08)
 
+// The text alignment for this flow.
+impl FlowFlags {
+    #[inline]
+    pub fn text_align(self) -> text_align::T {
+        FromPrimitive::from_u8((*self & TEXT_ALIGN_BITMASK) >> TEXT_ALIGN_SHIFT).unwrap()
+    }
+
+    #[inline]
+    pub fn set_text_align(&mut self, value: text_align::T) {
+        *self = FlowFlags((**self & !TEXT_ALIGN_BITMASK) | ((value as u8) << TEXT_ALIGN_SHIFT))
+    }
+}
+
 /// Data common to all flows.
 ///
 /// FIXME: We need a naming convention for pseudo-inheritance like this. How about

src/components/main/layout/inline.rs

@@ -17,13 +17,13 @@ use extra::container::Deque;
 use extra::ringbuf::RingBuf;
 use geom::{Point2D, Rect, Size2D};
 use gfx::display_list::DisplayList;
-use style::computed_values::text_align;
-use style::computed_values::vertical_align;
 use servo_util::geometry::Au;
 use servo_util::range::Range;
 use std::cell::Cell;
 use std::u16;
 use std::util;
+use style::computed_values::text_align;
+use style::computed_values::vertical_align;
 
 /// Lineboxes are represented as offsets into the child list, rather than
 /// as an object that "owns" boxes. Choosing a different set of line
@@ -644,7 +644,8 @@ impl Flow for InlineFlow {
         //
         // TODO: Combine this with `LineboxScanner`'s walk in the box list, or put this into `Box`.
 
-        debug!("assign_widths_inline: floats_in: {:?}", self.base.floats_in);
+        debug!("InlineFlow::assign_widths: floats_in: {:?}", self.base.floats_in);
+
         {
             let this = &mut *self;
             for box in this.boxes.iter() {
@@ -656,6 +657,7 @@ impl Flow for InlineFlow {
             let child_base = flow::mut_base(*kid);
             child_base.position.size.width = self.base.position.size.width;
             child_base.flags.set_inorder(self.base.flags.inorder());
+            child_base.flags.propagate_text_alignment_from_parent(self.base.flags)
         }
         // There are no child contexts, so stop here.
 
@@ -695,8 +697,8 @@ impl Flow for InlineFlow {
 
         let mut line_height_offset = Au::new(0);
 
-        // All lines use text alignment from base (non-inline) node
+        // All lines use text alignment of the flow.
-        let text_align = self.base.node.style().get().Text.text_align;
+        let text_align = self.base.flags.text_align();
 
         // Now, go through each line and lay out the boxes inside.
         for line in self.lines.mut_iter() {
@@ -783,18 +785,16 @@ impl Flow for InlineFlow {
                 // parent's content area.
 
                 // We should calculate the distance from baseline to the top of parent's content
-                // area. But for now we assume it's the parent's font size.
+                // area. But for now we assume it's the font size.
-                let mut parent_text_top;
+                //
+                // The spec does not state which font to use. Previous versions of the code used
+                // the parent's font; this code uses the current font.
+                let parent_text_top = cur_box.style().Font.font_size;
 
                 // We should calculate the distance from baseline to the bottom of the parent's
                 // content area. But for now we assume it's zero.
                 let parent_text_bottom = Au::new(0);
 
-                let parent = cur_box.node.parent_node().unwrap();
-                let parent_style = parent.style();
-                let font_size = parent_style.get().Font.font_size;
-                parent_text_top = font_size;
-
                 // Calculate a relative offset from the baseline.
                 //
                 // The no-update flag decides whether `biggest_top` and `biggest_bottom` are

src/components/main/layout/layout_task.rs

@@ -16,7 +16,7 @@ use layout::flow::{Flow, ImmutableFlowUtils, MutableFlowUtils, PreorderFlowTrave
 use layout::flow::{PostorderFlowTraversal};
 use layout::flow;
 use layout::incremental::{RestyleDamage};
-use layout::util::{LayoutData, LayoutDataAccess};
+use layout::util::{LayoutData, LayoutDataAccess, OpaqueNode};
 
 use extra::arc::{Arc, RWArc, MutexArc};
 use geom::point::Point2D;
@@ -69,7 +69,7 @@ struct LayoutTask {
     script_chan: ScriptChan,
 
     /// The channel on which messages can be sent to the painting task.
-    render_chan: RenderChan<AbstractNode<()>>,
+    render_chan: RenderChan<OpaqueNode>,
 
     /// The channel on which messages can be sent to the image cache.
     image_cache_task: ImageCacheTask,
@@ -81,7 +81,7 @@ struct LayoutTask {
     screen_size: Option<Size2D<Au>>,
 
     /// A cached display list.
-    display_list: Option<Arc<DisplayList<AbstractNode<()>>>>,
+    display_list: Option<Arc<DisplayList<OpaqueNode>>>,
 
     stylist: RWArc<Stylist>,
 
@@ -204,7 +204,7 @@ impl LayoutTask {
                   port: Port<Msg>,
                   constellation_chan: ConstellationChan,
                   script_chan: ScriptChan,
-                  render_chan: RenderChan<AbstractNode<()>>,
+                  render_chan: RenderChan<OpaqueNode>,
                   img_cache_task: ImageCacheTask,
                   opts: Opts,
                   profiler_chan: ProfilerChan,
@@ -232,7 +232,7 @@ impl LayoutTask {
            port: Port<Msg>,
            constellation_chan: ConstellationChan,
            script_chan: ScriptChan,
-           render_chan: RenderChan<AbstractNode<()>>, 
+           render_chan: RenderChan<OpaqueNode>, 
            image_cache_task: ImageCacheTask,
            opts: &Opts,
            profiler_chan: ProfilerChan)
@@ -478,34 +478,36 @@ impl LayoutTask {
         if data.goal == ReflowForDisplay {
             do profile(time::LayoutDispListBuildCategory, self.profiler_chan.clone()) {
                 let root_size = flow::base(layout_root).position.size;
-                let display_list= ~Cell::new(DisplayList::<AbstractNode<()>>::new());
+                let display_list = ~Cell::new(DisplayList::<OpaqueNode>::new());
                 let dirty = flow::base(layout_root).position.clone();
-                layout_root.build_display_list(
+                let display_list_builder = DisplayListBuilder {
-                    &DisplayListBuilder {
+                    ctx: &layout_ctx,
-                        ctx: &layout_ctx,
+                };
-                    },
+                layout_root.build_display_list(&display_list_builder, &dirty, display_list);
-                    &dirty,
-                    display_list);
 
                 let display_list = Arc::new(display_list.take());
 
-                    let mut color = color::rgba(255.0, 255.0, 255.0, 255.0);
+                let mut color = color::rgba(255.0, 255.0, 255.0, 255.0);
 
-                    for child in node.traverse_preorder() {
+                for child in node.traverse_preorder() {
-                      if child.type_id() == ElementNodeTypeId(HTMLHtmlElementTypeId) || 
+                    if child.type_id() == ElementNodeTypeId(HTMLHtmlElementTypeId) || 
-                         child.type_id() == ElementNodeTypeId(HTMLBodyElementTypeId) {
+                            child.type_id() == ElementNodeTypeId(HTMLBodyElementTypeId) {
-                             let element_bg_color = child.style().get().resolve_color(
+                        let element_bg_color = child.style()
-                                 child.style().get().Background.background_color
+                                                    .get()
-                             ).to_gfx_color();
+                                                    .resolve_color(child.style()
-                             match element_bg_color {
+                                                                        .get()
-                                 color::rgba(0., 0., 0., 0.) => {}
+                                                                        .Background
-                                 _ => {
+                                                                        .background_color)
-                                   color = element_bg_color;
+                                                    .to_gfx_color();
-                                   break;
+                        match element_bg_color {
-                               }
+                            color::rgba(0., 0., 0., 0.) => {}
-                             }
+                            _ => {
-                      }
+                                color = element_bg_color;
+                                break;
+                           }
+                        }
                     }
+                }
 
                 let render_layer = RenderLayer {
                     display_list: display_list.clone(),
@@ -532,16 +534,15 @@ impl LayoutTask {
     /// `getClientRects()` or `getBoundingClientRect()` ultimately invoke.
     fn handle_query(&self, query: LayoutQuery) {
         match query {
+            // The neat thing here is that in order to answer the following two queries we only
+            // need to compare nodes for equality. Thus we can safely work only with `OpaqueNode`.
             ContentBoxQuery(node, reply_chan) => {
-                // FIXME: Isolate this transmutation into a single "bridge" module.
+                let node = OpaqueNode::from_node(&node);
-                let node: AbstractNode<()> = unsafe {
-                    transmute(node)
-                };
 
                 fn union_boxes_for_node<'a>(
                                         accumulator: &mut Option<Rect<Au>>,
-                                        mut iter: DisplayItemIterator<'a,AbstractNode<()>>,
+                                        mut iter: DisplayItemIterator<'a,OpaqueNode>,
-                                        node: AbstractNode<()>) {
+                                        node: OpaqueNode) {
                     for item in iter {
                         union_boxes_for_node(accumulator, item.children(), node);
                         if item.base().extra == node {
@@ -559,15 +560,12 @@ impl LayoutTask {
                 reply_chan.send(ContentBoxResponse(rect.unwrap_or(Au::zero_rect())))
             }
             ContentBoxesQuery(node, reply_chan) => {
-                // FIXME: Isolate this transmutation into a single "bridge" module.
+                let node = OpaqueNode::from_node(&node);
-                let node: AbstractNode<()> = unsafe {
-                    transmute(node)
-                };
 
                 fn add_boxes_for_node<'a>(
                                       accumulator: &mut ~[Rect<Au>],
-                                      mut iter: DisplayItemIterator<'a,AbstractNode<()>>,
+                                      mut iter: DisplayItemIterator<'a,OpaqueNode>,
-                                      node: AbstractNode<()>) {
+                                      node: OpaqueNode) {
                     for item in iter {
                         add_boxes_for_node(accumulator, item.children(), node);
                         if item.base().extra == node {
@@ -582,7 +580,7 @@ impl LayoutTask {
                 reply_chan.send(ContentBoxesResponse(boxes))
             }
             HitTestQuery(_, point, reply_chan) => {
-                fn hit_test(x: Au, y: Au, list: &[DisplayItem<AbstractNode<()>>])
+                fn hit_test(x: Au, y: Au, list: &[DisplayItem<OpaqueNode>])
                             -> Option<HitTestResponse> {
                     for item in list.rev_iter() {
                         match *item {
@@ -602,13 +600,19 @@ impl LayoutTask {
                             _ => {}
                         }
                         let bounds = item.bounds();
-                        // TODO this check should really be performed by a method of DisplayItem
+
+                        // TODO(tikue): This check should really be performed by a method of
+                        // DisplayItem.
                         if x < bounds.origin.x + bounds.size.width &&
-                            bounds.origin.x <= x &&
+                                bounds.origin.x <= x &&
-                            y < bounds.origin.y + bounds.size.height &&
+                                y < bounds.origin.y + bounds.size.height &&
-                            bounds.origin.y <= y {
+                                bounds.origin.y <= y {
+                            // FIXME(pcwalton): This `unsafe` block is too unsafe, since incorrect
+                            // incremental flow construction could create this. Paranoid validation
+                            // against the set of valid nodes should occur in the script task to
+                            // ensure that this is a valid address instead of transmuting here.
                             let node: AbstractNode<LayoutView> = unsafe {
-                                transmute(item.base().extra)
+                                item.base().extra.to_node()
                             };
                             let resp = Some(HitTestResponse(node));
                             return resp;

src/components/main/layout/util.rs

@@ -12,6 +12,7 @@ use servo_util::slot::{MutSlotRef, SlotRef};
 use servo_util::tree::TreeNodeRef;
 use std::cast;
 use std::iter::Enumerate;
+use std::libc::uintptr_t;
 use std::vec::VecIterator;
 use style::{ComputedValues, PropertyDeclaration};
 
@@ -182,3 +183,35 @@ impl LayoutDataAccess for AbstractNode<LayoutView> {
     }
 }
 
+/// An opaque handle to a node. The only safe operation that can be performed on this node is to
+/// compare it to another opaque handle or to another node.
+///
+/// Because the script task's GC does not trace layout, node data cannot be safely stored in layout
+/// data structures. Also, layout code tends to be faster when the DOM is not being accessed, for
+/// locality reasons. Using `OpaqueNode` enforces this invariant.
+#[deriving(Clone, Eq)]
+pub struct OpaqueNode(uintptr_t);
+
+impl<T> Equiv<AbstractNode<T>> for OpaqueNode {
+    fn equiv(&self, node: &AbstractNode<T>) -> bool {
+        unsafe {
+            **self == cast::transmute_copy::<AbstractNode<T>,uintptr_t>(node)
+        }
+    }
+}
+
+impl OpaqueNode {
+    /// Converts a DOM node to an `OpaqueNode`.
+    pub fn from_node<T>(node: &AbstractNode<T>) -> OpaqueNode {
+        unsafe {
+            OpaqueNode(cast::transmute_copy(node))
+        }
+    }
+
+    /// Unsafely converts an `OpaqueNode` to a DOM node. Use this only if you absolutely know what
+    /// you're doing.
+    pub unsafe fn to_node<T>(&self) -> AbstractNode<T> {
+        cast::transmute(**self)
+    }
+}
+

src/components/main/pipeline.rs

@@ -9,7 +9,7 @@ use extra::url::Url;
 use gfx::opts::Opts;
 use gfx::render_task::{PaintPermissionGranted, PaintPermissionRevoked};
 use gfx::render_task::{RenderChan, RenderTask};
-use script::dom::node::AbstractNode;
+use layout::util::OpaqueNode;
 use script::layout_interface::LayoutChan;
 use script::script_task::LoadMsg;
 use script::script_task::{AttachLayoutMsg, NewLayoutInfo, ScriptTask, ScriptChan};
@@ -27,19 +27,19 @@ pub struct Pipeline {
     subpage_id: Option<SubpageId>,
     script_chan: ScriptChan,
     layout_chan: LayoutChan,
-    render_chan: RenderChan<AbstractNode<()>>,
+    render_chan: RenderChan<OpaqueNode>,
     layout_shutdown_port: Port<()>,
     render_shutdown_port: Port<()>,
     /// The most recently loaded url
     url: Option<Url>,
 }
 
-/// A subset of the Pipeline nthat is eeded for layer composition
+/// The subset of the pipeline that is needed for layer composition.
 #[deriving(Clone)]
 pub struct CompositionPipeline {
     id: PipelineId,
     script_chan: ScriptChan,
-    render_chan: RenderChan<AbstractNode<()>>,
+    render_chan: RenderChan<OpaqueNode>,
 }
 
 impl Pipeline {
@@ -182,7 +182,7 @@ impl Pipeline {
                subpage_id: Option<SubpageId>,
                script_chan: ScriptChan,
                layout_chan: LayoutChan,
-               render_chan: RenderChan<AbstractNode<()>>,
+               render_chan: RenderChan<OpaqueNode>,
                layout_shutdown_port: Port<()>,
                render_shutdown_port: Port<()>)
                -> Pipeline {

src/components/style/properties.rs.mako

@@ -106,7 +106,7 @@ pub mod longhands {
         <%self:single_component_value name="${name}" inherited="${inherited}">
             ${caller.body()}
             pub mod computed_value {
-                #[deriving(Eq, Clone)]
+                #[deriving(Eq, Clone, FromPrimitive)]
                 pub enum T {
                     % for value in values.split():
                         ${to_rust_ident(value)},