Pseudo element build flow and box #1496

parkjaeman · 2014-01-15T10:27:11Z

Implementation flow & box construction for pseudo element :before and :after.

highfive · 2014-01-15T10:27:16Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify layout code, but no reftests are modified. Please consider adding a reftest!
@parkjaeman, please confirm that src/test/html/acid1.html and your favourite wikipedia page still render correctly!

hoppipolla-critic-bot · 2014-01-15T10:27:16Z

Critic review: https://critic.hoppipolla.co.uk/r/555

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

parkjaeman · 2014-01-16T23:52:58Z

@jdm r?

sammykim · 2014-01-17T07:53:31Z

I rebased it.

SimonSapin · 2014-01-17T10:58:06Z

build_flow_or_boxes_for_before_node and build_flow_or_boxes_for_after_node have a lot of duplicated code. Please refactor them to a single function that takes parameters for the parts that vary. If that proves difficult, as a last resort use a macro with ident parameters as in 8d8a8ad.

Same for need_before and need_after.

parkjaeman · 2014-01-18T01:34:20Z

@SimonSapin I have refactored for this PR. Please check that.

parkjaeman · 2014-01-21T04:42:35Z

@SimonSapin r?

jdm · 2014-01-24T00:11:30Z

src/components/script/dom/node.rs

@@ -1546,29 +1560,49 @@ impl Node {
        self.parent_node = new_parent_node
    }

+    pub fn set_parent_node_without_doc(&mut self, new_parent_node: Option<AbstractNode>) {
+        self.parent_node = new_parent_node
+    }


I don't think we should add these methods to Node, because they are not safe to use from the script task. Instead, let's just set the fields directly from the methods in LayoutNode.

jdm · 2014-01-24T00:11:54Z

src/components/main/layout/wrapper.rs

    /// Returns the interior of this node as a `Node`. This is highly unsafe for layout to call
    /// and as such is marked `unsafe`.
    pub unsafe fn get<'a>(&'a self) -> &'a Node {
        cast::transmute(self.node.node())
    }

+    pub unsafe fn get_mut<'a>(&'a mut self) -> &'a mut Node {
+        cast::transmute_mut(self.node.mut_node())


Why is a transmute necessary here?

jdm · 2014-01-24T00:12:56Z

src/components/main/layout/wrapper.rs

+                let p_ldw = p.borrow_layout_data();
+                match *p_ldw.get() {
+                    Some(ref layout_data_wrapper) => {
+                        match layout_data_wrapper.data.before_style {


before_style_exist = layout_data_wrapper.data.before_style.is_some();
after_style_exist = layout_data_wrapper.data.after_style.is_some();

jdm · 2014-01-24T00:13:12Z

src/components/main/layout/wrapper.rs

+    pub fn necessary_pseudo_elements(&self) -> ~[PseudoElement] {
+        let mut pseudo_elements = ~[];
+        let mut before_style_exist = false;
+        let mut after_style_exist = false;


s/exist/exists/

jdm · 2014-01-24T00:16:33Z

src/components/main/layout/wrapper.rs

+        match *ldw.get() {
+            Some(ref layout_data_wrapper) => {
+                if before_style_exist {
+                    match layout_data_wrapper.data.before_abstract_node {


if layout_data_wrapper.data.before_abstract_node.is_none() {
...
}

jdm · 2014-01-24T00:24:14Z

src/components/main/layout/construct.rs

+                let document = unsafe { node.get().owner_doc() };
+
+                // Create pseudo parent_node
+                let pseudo_parent_element = ~Element::new_inherited(HTMLUnknownElementTypeId,~"pseudo_element", HTML, document);


Let's create a new method for Element and Text called new_pseudo that does not require any arguments. Then we do not need the document variable any more.

jdm · 2014-01-24T00:41:28Z

src/components/script/dom/node.rs

@@ -331,6 +331,20 @@ impl<'a> AbstractNode {
        }
    }

+    pub unsafe fn from_element(element: ~Element) -> AbstractNode {


Call this from_layout_pseudo and make it a generic method bounded by Reflectable (ie. <T: Reflectable>(node: ~T)). We don't need from_text if we do this.

jdm · 2014-01-24T00:45:35Z

src/components/script/dom/node.rs

+    pub unsafe fn from_element(element: ~Element) -> AbstractNode {
+        let node = AbstractNode {
+            obj: transmute(element),
+        };


Right now, this will leak the memory for the Element and Text objects each time we use generated content. I think we need to add a new type LayoutPseudoNode that contains an AbstractNode, and has a destructor that frees the memory - the new fields in LayoutPrivateData should be this new type.

jdm · 2014-01-24T00:46:37Z

src/components/main/layout/wrapper.rs

+        let mut after_style_exist = false;
+
+        match self.parent_node() {
+            Some(p) => {


if self.parent_node().is_none() { return ~[]; } let p = self.parent_node().unwrap();

jdm · 2014-01-24T00:47:11Z

src/components/main/layout/wrapper.rs

        LayoutNode {
            node: node,
            chain: self.chain,
        }
    }

+    pub fn set_parent_node_without_doc(&mut self, node: LayoutNode) {


Just call this set_parent_node (and same for the other new methods)

In fact, let's replace all of these with a append_child method with sets all of the fields correctly.

Maybe we'll need append_sibling and prepend_sibling as well.

jdm · 2014-01-24T00:47:42Z

src/components/main/layout/wrapper.rs

        LayoutNode {
            node: node,
            chain: self.chain,
        }
    }

+    pub fn set_parent_node_without_doc(&mut self, node: LayoutNode) {
+        unsafe { self.get_mut().set_parent_node_without_doc(Some(node.node)) }


As discussed below, just set the field manually instead of calling a method.

jdm · 2014-01-24T00:48:10Z

src/components/main/layout/util.rs

@@ -142,6 +142,15 @@ pub struct PrivateLayoutData {

    after_style: Option<Arc<ComputedValues>>,

+    before_parent_abstract_node: Option<AbstractNode>,


Don't add the abstract_node suffix.

jdm · 2014-01-24T00:48:51Z

src/components/main/layout/construct.rs

+                let iter = &mut value.clone().move_iter().peekable();
+                match iter.next() {
+                    Some(content::StringContent(str)) => str,
+                        _ => ~"",


Indentation.

jdm · 2014-01-24T00:49:08Z

src/components/main/layout/construct.rs

+    }
+
+    fn get_content(content_list: &content::T) -> ~str{
+        let content = match *content_list {


No need for the let content =; this can just return the result of the match.

jdm · 2014-01-24T00:58:13Z

src/components/main/layout/construct.rs

+                    None => fail!("node has no layout_data"),
+                }
+            }
+            _ => fail!("Text node should has its parent"),


s/has/have/

jdm · 2014-01-24T00:59:19Z

src/components/main/layout/construct.rs

@@ -497,6 +505,175 @@ impl<'fc> FlowConstructor<'fc> {
            self.build_boxes_for_replaced_inline_content(node)
        }
    }
+
+    fn build_flow_or_boxes_for_pseudo_element(&mut self, node: LayoutNode, pseudo_element: PseudoElement) {
+        match node.parent_node() {


let p = node.parent_node().expect("Text node should have its parent");

And then we don't need this match.

jdm · 2014-01-24T01:00:36Z

src/components/main/layout/construct.rs

+                }
+
+                // Store pseudo_parent_node & pseudo_node in node
+                match *layout_data_ref.get() {


let ref mut layout_data_wrapper = (*layout_data_ref.get()).expect("node has no layout_data");

jdm · 2014-01-24T01:01:00Z

src/components/main/layout/construct.rs

+                                pseudo_parent_node.set_next_sibling_without_doc(next_sibling);
+                            }
+                            None => {
+                                match p.parent_node() {


let mut gp = p.parent_node().expect("p should have parent");

jdm · 2014-01-24T01:01:13Z

src/components/main/layout/construct.rs

+                                pseudo_parent_node.set_prev_sibling_without_doc(prev_sibling);
+                            }
+                            None => {
+                                match p.parent_node() {


let mut gp = p.parent_node().expect("p should have parent");

jdm · 2014-01-24T01:04:04Z

Ok, I've finished my review. This looks good to me, but we need to solve the memory leak I describe, and build_flow_or_boxes_for_pseudo_element is very long and has lots of unneeded matches, so it is difficult to read. I want all of the manual set_prev_sibling/set_first_child/etc. method calls to move into LayoutNode inside of descriptive methods like append_child, append_sibling, prepend_sibling, etc. That will make build_flow_or_boxes_for_pseudo_element much easier to read, and it will be harder to make mistakes when setting up pseudo DOM hierarchies manually.

parkjaeman · 2014-01-25T01:06:07Z

@jdm Thanks for your very kind comments. I modified this PR as you said.

jdm · 2014-01-25T01:36:10Z

src/components/main/layout/wrapper.rs

+            let node: ~Element = unsafe { cast::transmute(self.node) };
+        } else if self.node.is_text() {
+            let node: ~Text = unsafe { cast::transmute(self.node) };
+        }


If LayoutPseudoNode is generic over type T, this can be replaced with

fn drop(&self) { let _: ~T = unsafe { cast::transmute(self.node) }; }

@jdm
First way to solve this issue is changing LayoutPseudoNode as generic.
As long as LayoutPseudoNode is generic e.g) pub struct LayoutPseudoNode<T>
Then the struct which includes LayoutPseudoNode has to be generic.
e.g) LayoutDataWrapper has PrivateLayoutData has LayoutPseudoNode
Then all of those have to be generic. Not only I think that's not best way to do this, but also we have trouble to implement that.

Second, we tried to change priv node type to ~Reflectable and also remove all of the code relate to AbstractNode and LayoutNode. But as long as we want to build flow( e.g build_flow_for_block), we need to create abstract node also layout node.

We don't know how to make LayoutPseudoNode to be generic to drop easily and also remove from_layout_pseudo in node.rs

Anyway, I addressed another comments about function name and arguments.
If you have any opinion, let us know more detail.
Thanks for answer.

Ok, I understand. This is ok to leave this way.

jdm · 2014-02-06T22:43:40Z

src/components/main/layout/wrapper.rs

        }
+
+        let prev_sibling = unsafe {
+                               self.node.node().prev_sibling.map(|node| self.new_with_this_lifetime(node))


jdm · 2014-02-06T22:44:01Z

src/components/main/layout/wrapper.rs

+        if (self.is_element() && self.node.with_imm_element(|element| "before" == element.tag_name)) ||
+            (self.is_text() && self.parent_node().unwrap().node.with_imm_element(|element| "before" == element.tag_name)) {
+            return unsafe {
+                        self.node.node().next_sibling.map(|node| self.new_with_this_lifetime(node))


jdm · 2014-02-06T22:44:09Z

src/components/main/layout/wrapper.rs

        }
+
+        let next_sibling = unsafe{
+                                self.node.node().next_sibling.map(|node| self.new_with_this_lifetime(node))


jdm · 2014-02-06T22:45:34Z

src/components/main/layout/wrapper.rs

+        } else if self.node.is_text() {
+            let _: ~Text = unsafe { cast::transmute(self.node) };
+        }
+    }


Let's add an else block that fails.

jdm · 2014-02-06T22:47:55Z

src/components/main/layout/wrapper.rs

+
+        let ldw = self.borrow_layout_data();
+        let ldw_ref = ldw.get().get_ref();
+        if unsafe { self.parent_node().is_none() } {


Make a temporary variable for the parent node so you don't have to keep repeating the unsafe calls.

jdm · 2014-02-06T22:50:52Z

src/components/main/layout/wrapper.rs

+        }
+    }
+
+    pub unsafe fn parent_node(&self) -> Option<ThreadSafeLayoutNode<'ln>> {


@pcwalton Could you confirm that making methods like this available does not break our threadsafety guarantees?

This is not thread safe. You cannot access parent nodes.

parkjaeman · 2014-02-07T04:36:14Z

@jdm We modified this PR according to your comments except for using to_pseudo_layout_node.

jdm · 2014-02-07T22:20:57Z

Unfortunately Patrick has confirmed that we can't use parent nodes, so we're going to have to do something different. In build_flow_or_boxes_for_pseudo_element, I suggest using a new variant of initialize_layout_data that does not require a channel. After that we just need to make the parent's before and after styles available somehow; I'm not really sure, maybe @pcwalton has an idea. For methods like prev_sibling, I guess we're going to have to store a flag in the layout node indicating that the parent is a pseudo parent.

jdm · 2014-02-07T22:22:29Z

Perhaps we can add parent_before_style and parent_after_style fields to PrivateLayoutData.

parkjaeman · 2014-02-08T00:33:30Z

@jdm Do you mean that we have to remove all variables regarding pseudo parent and use new initialize_layout_data for ThreadSafeLayoutNode in build_flow_or_boxes_for_pseudo_element?
If so,how can we support pseudo element with display = block? In this case, pseudo element needs its own block.

parkjaeman · 2014-02-08T00:36:58Z

@jdm We currently insert before or after style to pseudo node's style if that is before or after text node, respectively. Why do we need parent_before_style and parent_after_style?

jdm · 2014-02-08T03:49:00Z

@parkjaeman Any new layout code that is calling parent_node() is not threadsafe; layout code can only safely access a node, its siblings, and its children, or else it cannot be parallelized. We need to find ways to store the data required from parent nodes in the children instead. #1635 is one way of solving this problem for percentage heights, but the solution we need here will be more complicated.

jdm · 2014-02-08T03:50:45Z

I believe that creating the pseudo parent in build_flow_or_boxes_for_pseudo_element is fine. The problem is when we have an already existing ThreadSafeLayoutNode and call parent_node on it.

parkjaeman · 2014-02-10T08:02:08Z

@jdm OK. We will try to transfer storage for before style and after style from target element to target text so that we don't need to call parent_node() in build_flow_or_boxes_for_pseudo_element.
And I have a question. Can you explain how calling parent_node() in layout code breaks thread safety?


        Create before_parent_node and before_node

Conflicts: src/components/main/layout/construct.rs src/components/main/layout/wrapper.rs src/components/script/dom/node.rs src/components/util/slot.rs


        Add some function for pseudo_node and reduce duplication in construct.rs


        Rebase PR


        Change after and before parent, child node to struct


        Fix indent and unefficient code


        Remove duplicate get_pseudo_node and so on


        Add initialize_layout_data without a channel


        Add debug message in MoseOver Event

jdm · 2014-02-10T12:03:20Z

@pcwalton is best able to answer that question. Are the design for parallel layout and the constraints required written down anywhere?

pcwalton · 2014-02-12T00:28:48Z

It's not thread safe because multiple children of a single parent can be laid out in parallel. So two siblings might both call parent_node() at the same time. If they happen to mutate the parent at the same time, then that could lead to a data race.

It is possible to safely call parent_node(), if all siblings are only reading properties that are not modified by this layout pass. But we would need a new type of ThreadSafeLayoutNode to enforce this safety. It is probably simpler to avoid looking at parent nodes entirely.

I'll review this PR later today and see if I can suggest an alternative strategy.

pcwalton · 2014-02-12T04:54:07Z

Here's a suggestion for how to do this. I apologize that this is significantly different from what you implemented.

Modify ThreadSafeLayoutNode so that it has an extra field: an enum describing the pseudo-element, if any. This enum would have values Normal, Before, or After. For most nodes there will be no pseudo-element, so it can just be Normal. The only time a ThreadSafeLayoutNode is created with any pseudo-element of Before or After is as the result of iterating over the children of a ThreadSafeLayoutNode, as below.
When iterating over children of a ThreadSafeLayoutNode in the ThreadSafeLayoutNodeChildrenIterator, examine if the layout node's pseudo-element is Normal, examine the layout node's style. If it has a before_style, then yield the ThreadSafeLayoutNode corresponding to its before pseudo-element as its first "child" before yielding its real children. If it has an after_style, then yield the ThreadSafeLayoutNode corresponding to its after pseudo-element as its last "child" after yielding its real children.
Get rid of the mutate_layout_data call inside construct.rs and instead expose a function in ThreadSafeLayoutNode that returns a mutable reference to the flow_construction_result inside the flow. You will want a flow_construction_result for each pseudo-element, as well as for normal nodes. (Eventually these extra flow construction results will be allocated lazily.)
The StyledNode trait needs to check the pseudo-element of the ThreadSafeLayoutNode and return before_style or after_style as appropriate.

SimonSapin · 2014-02-12T10:18:05Z

@pcwalton Nit: I’d rather Normal (as in, not a pseudo-element) be called Element instead. "Normal" a way overloaded. Other than that, I don’t see anything wrong spec-wise in your last comment.

pcwalton · 2014-02-12T16:53:30Z

Oh, one more thing: You will need to do something about the borrow flags in mutate_layout_data because it is possible for a node with :before and :after to have flows for its two pieces of generated content laid out in parallel and they will race on the borrow flags. I suggest changing to borrow_layout_data_unchecked and eliminating the mutate_layout_data method entirely for ThreadSafeLayoutNode, since it becomes unsafe.

sammykim · 2014-02-14T02:11:56Z

@pcwalton I am little bit confused. :) I want to make sure.

When iterating over children of ThreadSafeLayoutNode, Why do we need to put that enum Normal, Before, or After in ThreadSafeLayoutNode. I guess it should be used to check the node is BeforePseudoElement or AfterPseudoEment or just Element as you said Normal. But also I don't know when we can use it useful?
And what you mean yielding is we can make relation a node which is sent from script_task with pseudo_node? e.g) If before comes, we can make it(before pseudo element) as element's first child.
Do you mean iterating over children is in fn build_children_of_block_flow or build_boxes_for_replaced_inline_content that they call node.children() in construct.rs? And at the same time, If the node have before and after style, Will we call before pseudo-element as its first children, after pseudo-element as its last child in that part or another way..?
The reason I am confused is the ThreadSafeLayoutNode actually iterate in traverse_postorder_mut which doesn't use ThreadSafeLayoutNodeChildrenIterator.
We use mutate_layout_data to save pseudo_parent_node(element node), pseudo_node(text_node) in real dom node. If we don't use that function, we can't save those node. Because borrow_layout_data_unchecked returns immutable data. And also as long as we don't save pseudo_node and pseudo_parent_node , we had segmentation fault.
This is a last question that I missed. I don't know why we can't call node.parent_node()..? Event if we call it immutable, How come will it make a problem?? I understand if it's not immutable, it can make a big problem. But we only use it as immutable data. So I don't know how it can make data races.

Thanks for your kind comment. Please make me sure. :)

jdm · 2014-02-24T17:17:25Z

I'm assuming the new PR in #1725 makes this obsolete.

jdm reviewed Jan 24, 2014
View changes

src/components/main/layout/construct.rs Outdated

let iter = &mut value.clone().move_iter().peekable();

match iter.next() {

Some(content::StringContent(str)) => str,

_ => ~"",

This comment has been minimized.

Sign in to view

Copy link

jdm Jan 24, 2014
Member

Indentation.

jdm reviewed Jan 24, 2014
View changes

src/components/main/layout/construct.rs Outdated

None => fail!("node has no layout_data"),

}

}

_ => fail!("Text node should has its parent"),

This comment has been minimized.

Sign in to view

Copy link

jdm Jan 24, 2014
Member

s/has/have/

jdm reviewed Jan 24, 2014
View changes

parkjaeman mentioned this pull request Jan 25, 2014

Pseudo element ":before" #1158

Closed

jdm reviewed Jan 25, 2014
View changes

jdm reviewed Feb 6, 2014
View changes

src/components/main/layout/wrapper.rs Outdated

}

let prev_sibling = unsafe {

self.node.node().prev_sibling.map(|node| self.new_with_this_lifetime(node))

This comment has been minimized.

Sign in to view

Copy link

jdm Feb 6, 2014
Member

Unindent.

jdm reviewed Feb 6, 2014
View changes

src/components/main/layout/wrapper.rs Outdated

}

let next_sibling = unsafe{

self.node.node().next_sibling.map(|node| self.new_with_this_lifetime(node))

This comment has been minimized.

Sign in to view

Copy link

jdm Feb 6, 2014
Member

Unindent.

jdm reviewed Feb 6, 2014
View changes

src/components/main/layout/wrapper.rs Outdated

} else if self.node.is_text() {

let _: ~Text = unsafe { cast::transmute(self.node) };

}

}

This comment has been minimized.

Sign in to view

Copy link

jdm Feb 6, 2014
Member

Let's add an else block that fails.

jdm reviewed Feb 6, 2014
View changes

parkjaeman and others added 8 commits Jan 10, 2014

Create before_parent_node and before_node

66f325a

Conflicts: src/components/main/layout/construct.rs src/components/main/layout/wrapper.rs src/components/script/dom/node.rs src/components/util/slot.rs

Add some function for pseudo_node and reduce duplication in construct.rs

75aaa1d

Rebase PR

49dff11

Change after and before parent, child node to struct

21f58a2

Fix indent and unefficient code

c684327

Remove duplicate get_pseudo_node and so on

0d541f9

Add initialize_layout_data without a channel

5e14445

Add debug message in MoseOver Event

684a2e0

jdm closed this Feb 24, 2014

		@@ -142,6 +142,15 @@ pub struct PrivateLayoutData {

		after_style: Option<Arc<ComputedValues>>,

		before_parent_abstract_node: Option<AbstractNode>,

servo / servo

Join GitHub today

Pseudo element build flow and box #1496

Pseudo element build flow and box #1496

Conversation

parkjaeman commented Jan 15, 2014

highfive commented Jan 15, 2014

hoppipolla-critic-bot commented Jan 15, 2014

parkjaeman commented Jan 16, 2014

sammykim commented Jan 17, 2014

SimonSapin commented Jan 17, 2014

parkjaeman commented Jan 18, 2014

parkjaeman commented Jan 21, 2014

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Jan 24, 2014

parkjaeman commented Jan 25, 2014

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

parkjaeman commented Feb 7, 2014

jdm commented Feb 7, 2014

jdm commented Feb 7, 2014

parkjaeman commented Feb 8, 2014

parkjaeman commented Feb 8, 2014

jdm commented Feb 8, 2014

jdm commented Feb 8, 2014

parkjaeman commented Feb 10, 2014

jdm commented Feb 10, 2014

pcwalton commented Feb 12, 2014

pcwalton commented Feb 12, 2014

SimonSapin commented Feb 12, 2014

pcwalton commented Feb 12, 2014

sammykim commented Feb 14, 2014

jdm commented Feb 24, 2014