Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pseudo element build flow and box #1496

Closed
wants to merge 8 commits into from
+469 −12
Closed

Pseudo element build flow and box #1496

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
66f325a
Create before_parent_node and before_node
parkjaeman Jan 10, 2014
75aaa1d
Add some function for pseudo_node and reduce duplication in construct.rs
hyunjunekim Feb 4, 2014
49dff11
Rebase PR
parkjaeman Feb 6, 2014
21f58a2
Change after and before parent, child node to struct
sammykim Feb 6, 2014
c684327
Fix indent and unefficient code
sammykim Feb 6, 2014
0d541f9
Remove duplicate get_pseudo_node and so on
parkjaeman Feb 7, 2014
5e14445
Add initialize_layout_data without a channel
sammykim Feb 10, 2014
684a2e0
Add debug message in MoseOver Event
sammykim Feb 10, 2014
File filter...
Filter file types
Clear filters
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Prev Next

Fix indent and unefficient code

  • Loading branch information
@sammykim
sammykim committed Feb 10, 2014
commit c68432701bfcc1dadccc55261ea3361449d6e5a5
9 src/components/main/layout/util.rs
View file
Open in desktop
@@ -130,7 +130,6 @@ impl ElementMapping {
}
}

#[deriving(Clone)]
pub struct PseudoNode {
parent: LayoutPseudoNode,
element: LayoutPseudoNode
@@ -209,11 +208,13 @@ impl PrivateLayoutData {
self.after_applicable_declarations = SmallVec0::new();
}

pub fn get_pseudo_element(&self, pseudo_element: PseudoElement) -> Option<PseudoNode> {
pub fn get_pseudo_element<'a>(&'a self, pseudo_element: PseudoElement) -> Option<&'a PseudoNode> {
if pseudo_element == Before {

This comment has been minimized.

Sign in to view
@jdm

jdm Feb 6, 2014

Member

Use a match here, perhaps?
self.before.clone()
self.before.as_ref()
} else if pseudo_element == After{
self.after.as_ref()
} else {
self.after.clone()
None
}
}
}
147 src/components/main/layout/wrapper.rs
View file
Open in desktop
@@ -188,11 +188,11 @@ impl<'ln> TLayoutNode for LayoutNode<'ln> {
self.node
}

fn get_pseudo_node(&self, pseudo_type: PseudoElement) -> Option<LayoutNode<'ln>> {
if unsafe { self.get_abstract().is_text() } {
fn get_pseudo_node(&self, pseudo_element: PseudoElement) -> Option<LayoutNode<'ln>> {

This comment has been minimized.

Sign in to view
@jdm

jdm Feb 6, 2014

Member

I just noticed that the two get_pseudo_node methods are exact duplicates. We need to refactor it somehow to avoid this; it's ok for the one-liners, but this is a lot of nontrivial logic being copied.
if self.is_text() {
let layout_data_ref = self.borrow_layout_data();
return layout_data_ref.get().as_ref().and_then(|ldw|{
let pseudo_element = ldw.data.get_pseudo_element(pseudo_type);
let pseudo_element = ldw.data.get_pseudo_element(pseudo_element);
pseudo_element.and_then(|pseudo_node|{
if pseudo_node.parent.get_display() == display::inline {
unsafe{

This comment has been minimized.

Sign in to view
@jdm

jdm Feb 6, 2014

Member

Still space before {. Please search for this through the changes and fix all instances.
@@ -204,11 +204,10 @@ impl<'ln> TLayoutNode for LayoutNode<'ln> {
})
});
} else if self.is_element() {
match self.first_child() {
Some(first_child) => {
let layout_data_ref = first_child.borrow_layout_data();
return layout_data_ref.get().as_ref().and_then(|ldw|{
let pseudo_element = ldw.data.get_pseudo_element(pseudo_type);
self.first_child().map(|first_child|{
let layout_data_ref = first_child.borrow_layout_data();
return layout_data_ref.get().as_ref().and_then(|ldw|{
let pseudo_element = ldw.data.get_pseudo_element(pseudo_element);

This comment has been minimized.

Sign in to view
@jdm

jdm Feb 6, 2014

Member

Unindent.
pseudo_element.and_then(|pseudo_node|{
if pseudo_node.parent.get_display() == display::block {
unsafe{

This comment has been minimized.

Sign in to view
@jdm

jdm Feb 6, 2014

Member

Unindent.
@@ -219,67 +218,11 @@ impl<'ln> TLayoutNode for LayoutNode<'ln> {
}
})
});
}
None => {
return None
}
}
} else {
});
return None

This comment has been minimized.

Sign in to view
@jdm

jdm Feb 6, 2014

Member

This will cause the return value of the map call to be ignored. Are there tests that would have caught this?
}
/*
macro_rules! get_pseudo_node(
($pseudo_parent_node: ident, $pseudo_node: ident) => {
if unsafe { self.get_abstract().is_text() } {
let layout_data_ref = self.borrow_layout_data();
return layout_data_ref.get().as_ref().and_then(|ldw|{
ldw.data.$pseudo_parent_node.as_ref().and_then(|$pseudo_parent_node|{
if $pseudo_parent_node.get_display() == display::inline {
ldw.data.$pseudo_node.as_ref().and_then(|$pseudo_node|{
unsafe{
Some(self.new_with_this_lifetime($pseudo_node.node))
}
})
} else {
None
}
})
});
} else if self.is_element() {
match self.first_child() {
Some(first_child) => {
let layout_data_ref = first_child.borrow_layout_data();
return layout_data_ref.get().as_ref().and_then(|ldw|{
ldw.data.$pseudo_parent_node.as_ref().and_then(|$pseudo_parent_node|{
if $pseudo_parent_node.get_display() == display::block {
ldw.data.$pseudo_parent_node.as_ref().and_then(|$pseudo_parent_node|{
unsafe{
Some(self.new_with_this_lifetime($pseudo_parent_node.node))
}
})
} else {
None
}
})
});
}
None => {
return None
}
}
} else {
return None
}
}
)
if pseudo_element == Before {
return get_pseudo_node!(before_parent_node, before_node)
} else if pseudo_element == After {
return get_pseudo_node!(after_parent_node, after_node)
} else {
return None
}
*/
}
}

@@ -336,7 +279,7 @@ impl<'ln> TNode<LayoutElement<'ln>> for LayoutNode<'ln> {
return before_layout_node
}

let prev_sibling = unsafe{
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
@jdm

jdm Feb 5, 2014

Member

Unindent.

This comment has been minimized.

Sign in to view
@jdm

jdm Feb 6, 2014

Member

Unindent.
};

@@ -563,12 +506,11 @@ impl<'ln> TLayoutNode for ThreadSafeLayoutNode<'ln> {
self.node
}

fn get_pseudo_node(&self, pseudo_type: PseudoElement) -> Option<ThreadSafeLayoutNode<'ln>> {

if unsafe { self.get_abstract().is_text() } {
fn get_pseudo_node(&self, pseudo_element: PseudoElement) -> Option<ThreadSafeLayoutNode<'ln>> {
if self.is_text() {
let layout_data_ref = self.borrow_layout_data();
return layout_data_ref.get().as_ref().and_then(|ldw|{
let pseudo_element = ldw.data.get_pseudo_element(pseudo_type);
let pseudo_element = ldw.data.get_pseudo_element(pseudo_element);
pseudo_element.and_then(|pseudo_node|{
if pseudo_node.parent.get_display() == display::inline {
unsafe{
@@ -579,12 +521,12 @@ impl<'ln> TLayoutNode for ThreadSafeLayoutNode<'ln> {
}
})
});
} else if unsafe { self.get_abstract().is_element() } {
} else if self.is_element() {
match self.first_child() {
Some(first_child) => {
let layout_data_ref = first_child.borrow_layout_data();
return layout_data_ref.get().as_ref().and_then(|ldw|{
let pseudo_element = ldw.data.get_pseudo_element(pseudo_type);
let pseudo_element = ldw.data.get_pseudo_element(pseudo_element);
pseudo_element.and_then(|pseudo_node|{
if pseudo_node.parent.get_display() == display::block {
unsafe{
@@ -603,61 +545,9 @@ impl<'ln> TLayoutNode for ThreadSafeLayoutNode<'ln> {
} else {
return None
}
}

/*
macro_rules! get_pseudo_node(
($pseudo_parent_node: ident, $pseudo_node: ident) => {
if unsafe { self.get_abstract().is_text() } {
let layout_data_ref = self.borrow_layout_data();
return layout_data_ref.get().as_ref().and_then(|ldw|{
ldw.data.$pseudo_parent_node.as_ref().and_then(|$pseudo_parent_node|{
if $pseudo_parent_node.get_display() == display::inline {
ldw.data.$pseudo_node.as_ref().and_then(|$pseudo_node|{
unsafe{
Some(self.new_with_this_lifetime($pseudo_node.node))
}
})
} else {
None
}
})
});
} else if unsafe { self.get_abstract().is_element() } {
match self.first_child() {
Some(first_child) => {
let layout_data_ref = first_child.borrow_layout_data();
return layout_data_ref.get().as_ref().and_then(|ldw|{
ldw.data.$pseudo_parent_node.as_ref().and_then(|$pseudo_parent_node|{
if $pseudo_parent_node.get_display() == display::block {
ldw.data.$pseudo_parent_node.as_ref().and_then(|$pseudo_parent_node|{
unsafe{
Some(self.new_with_this_lifetime($pseudo_parent_node.node))
}
})
} else {
None
}
})
});
}
None => {
return None
}
}
} else {
return None
}
}
)
if pseudo_element == Before {
return get_pseudo_node!(before_parent_node, before_node)
} else if pseudo_element == After {
return get_pseudo_node!(after_parent_node, after_node)
} else {
return None
}*/
}
}

impl<'ln> ThreadSafeLayoutNode<'ln> {
/// Creates a new `ThreadSafeLayoutNode` from the given `LayoutNode`.
@@ -680,7 +570,7 @@ impl<'ln> ThreadSafeLayoutNode<'ln> {
}

unsafe fn next_sibling(&self) -> Option<ThreadSafeLayoutNode<'ln>> {
if (self.get_abstract().is_element() && self.node.with_imm_element(|element| "before" == element.tag_name))
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 self.node.node().next_sibling.map(|node| self.new_with_this_lifetime(node))
}
@@ -805,7 +695,10 @@ impl<'ln> ThreadSafeLayoutNode<'ln> {
self.node.is_text()
}


fn is_element(&self) -> bool{
self.node.is_element()
}

}

pub struct ThreadSafeLayoutNodeChildrenIterator<'a> {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.