incremental restyle: Hoist more stuff to Element #13951

bholley · 2016-10-28T00:47:00Z

The patch here hoists pseudo-element handling onto ThreadSafeLayoutElement. I have another patch which hoists stuff from TNode to TElement, but want to make sure this patch passes try first.

This change is

highfive · 2016-10-28T00:47:06Z

Heads up! This PR modifies the following files:

@KiChjang: components/script_layout_interface/wrapper_traits.rs, components/script/layout_wrapper.rs
@fitzgen: components/script_layout_interface/wrapper_traits.rs, components/script/layout_wrapper.rs
@emilio: components/layout/fragment.rs, components/layout/traversal.rs, components/layout/construct.rs, components/layout/query.rs, components/layout/wrapper.rs

highfive · 2016-10-28T00:47:07Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify layout and script code, but no tests are modified. Please consider adding a test!

bholley · 2016-10-28T00:47:27Z

@bors-servo try

bors-servo · 2016-10-28T00:47:30Z

⌛ Trying commit 9094b8c with merge 144440e...


        Auto merge of #13951 - bholley:more_element, r=<try>

incremental restyle: Hoist more stuff to Element The patch here hoists pseudo-element handling onto ThreadSafeLayoutElement. I have another patch which hoists stuff from TNode to TElement, but want to make sure this patch passes try first.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13951)

bors-servo · 2016-10-28T01:37:49Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

bholley · 2016-10-28T03:26:37Z

The next patch is 99% done but I may not finish it tonight. r? @emilio on this one.


        Auto merge of #13956 - bholley:more_telement, r=<try>

incremental restyle: Hoist most styling functionality from TNode to TElement This is a continuation of the work in #13951. I'm separating it out into a separate PR since the aforementioned patch has a green try run and this one doesn't yet.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13956)

emilio

r=me

emilio · 2016-10-28T08:18:34Z

components/script_layout_interface/wrapper_traits.rs

    fn debug_id(self) -> usize;

    /// Returns an iterator over this node's children.
    fn children(&self) -> LayoutIterator<Self::ChildrenIterator>;

-    /// If this is an element, accesses the element data. Fails if this is not an element node.
+    /// Returns a ThreadSafeLayoutElement this is an element, None otherwise.


"if this is an element".

emilio · 2016-10-28T08:18:34Z

components/script_layout_interface/wrapper_traits.rs

+
+    /// Returns true if this node contributes content. This is used in the implementation of
+    /// `empty_cells` per CSS 2.1 § 17.6.1.1.
+    fn is_content(&self) -> bool {


Now this can be written just as self.get_pseudo_element_type() == PseudoElementType::Normal, or self.type_id().is_some().

Not a big deal, but maybe worth doing.

emilio · 2016-10-28T08:18:34Z

components/script_layout_interface/wrapper_traits.rs

+        }
+    }
+
+    fn selected_style(&self) -> Arc<ServoComputedValues> {


I think having the context parameter here was sort of intended for consistency, but it's fine too.

Yeah I was kinda on the fence about it, but decided to opt for removal, since we can always add it back if we need it.

emilio · 2016-10-28T08:18:34Z

components/script/layout_wrapper.rs

@@ -757,46 +779,48 @@ impl<'ln> ServoThreadSafeLayoutNode<'ln> {
    }
 }

+// NB: The implementation here is a bit tricky because elements implementing
+// pseudos are supposed to return false for is_element().


Yeah, I'd like this to change. In every other browser a pseudo-element is represented as an Element. Of course, not in the scope of this PR.

Well, other browser elements put NAC in the DOM, which Servo explicitly avoids doing. But yeah, we might be able to make the intra-layout abstraction cleaner. Agreed on out of scope.

emilio · 2016-10-28T08:18:34Z

components/script/layout_wrapper.rs

-    /// The pseudo-element type, with (optionally),
-    /// an specified display value to override the stylesheet.
+    /// The pseudo-element type, with (optionally)
+    /// a specified display value to override the stylesheet.


arrg, Bobby the dictionary strikes again to correct my typos! :P

Thanks for doing this.

emilio · 2016-10-28T08:18:34Z

components/layout/query.rs

-    let layout_node = match *pseudo {
-        Some(PseudoElement::Before) => layout_node.get_before_pseudo(),
-        Some(PseudoElement::After) => layout_node.get_after_pseudo(),
+    let layout_el = requested_node.to_threadsafe().as_element().unwrap();


Probably worth making this function take an Element directly. Can be a followup if it's too much pain.

Turns out to be kinda hard to do because LayoutElement doesn't exist until #13956, and the opaque representations are all Node-y. We can do it later.

emilio · 2016-10-28T08:18:34Z

components/script/layout_wrapper.rs

+        self.pseudo == PseudoElementType::Normal
+    }
+
+    fn is_text_node(&self) -> bool {


Probably we should panic here (or similar) given we're doing a totally unnecessary check.

I think we can actually just remove this trait impl entirely.

emilio · 2016-10-28T08:18:34Z

components/script/layout_wrapper.rs

+        false
+    }
+
+    fn needs_layout(&self) -> bool { true }


emilio · 2016-10-28T08:18:34Z

components/script/layout_wrapper.rs

    }

    fn needs_layout(&self) -> bool {
-        self.pseudo != PseudoElementType::Normal ||
-        self.node.is_element() || self.node.is_text_node()
+        self.node.is_text_node() || self.node.is_element()


We shouldn't be creating ThreadSafeLayoutNodes for other kind of nodes now right? So this function can pretty much go away I think.

We still can. The iterator returned by children() operates on nodes of type Self, and then filters them using needs_layout before returning them. So if there's a comment node, we'll still create a ServoThreadSafeLayoutNode for it, just not return it to the traversal code.

So I think we need this.


        Auto merge of #13956 - bholley:more_telement, r=<try>

incremental restyle: Hoist most styling functionality from TNode to TElement This is a continuation of the work in #13951. I'm separating it out into a separate PR since the aforementioned patch has a green try run and this one doesn't yet.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13956)


        Auto merge of #13956 - bholley:more_telement, r=<try>

incremental restyle: Hoist most styling functionality from TNode to TElement This is a continuation of the work in #13951. I'm separating it out into a separate PR since the aforementioned patch has a green try run and this one doesn't yet.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13956)


        Move core pseudo-element handling to ThreadSafeLayoutElement.

bholley · 2016-10-28T17:50:24Z

@bors-servo r=emilio p=1

bors-servo · 2016-10-28T17:50:25Z

📌 Commit 46e2918 has been approved by emilio

bors-servo · 2016-10-28T18:22:15Z

⌛ Testing commit 46e2918 with merge 04ca7cd...


        Auto merge of #13951 - bholley:more_element, r=emilio

incremental restyle: Hoist more stuff to Element The patch here hoists pseudo-element handling onto ThreadSafeLayoutElement. I have another patch which hoists stuff from TNode to TElement, but want to make sure this patch passes try first.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13951)

larsbergstrom · 2016-10-28T18:30:10Z

@bors-servo force clean retry

bors-servo · 2016-10-28T18:30:27Z

⌛ Testing commit 46e2918 with merge f61f380...


        Auto merge of #13951 - bholley:more_element, r=emilio

incremental restyle: Hoist more stuff to Element The patch here hoists pseudo-element handling onto ThreadSafeLayoutElement. I have another patch which hoists stuff from TNode to TElement, but want to make sure this patch passes try first.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13951)


        Auto merge of #13956 - bholley:more_telement, r=<try>

incremental restyle: Hoist most styling functionality from TNode to TElement This is a continuation of the work in #13951. I'm separating it out into a separate PR since the aforementioned patch has a green try run and this one doesn't yet.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13956)

bors-servo · 2016-10-28T18:30:34Z

💔 Test failed - arm64

bors-servo · 2016-10-28T18:33:57Z

💔 Test failed - linux-rel-wpt

bholley · 2016-10-28T18:38:05Z

@bors-servo retry

bors-servo · 2016-10-28T18:38:08Z

⌛ Testing commit 46e2918 with merge 355935e...


        Auto merge of #13951 - bholley:more_element, r=emilio

incremental restyle: Hoist more stuff to Element The patch here hoists pseudo-element handling onto ThreadSafeLayoutElement. I have another patch which hoists stuff from TNode to TElement, but want to make sure this patch passes try first.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13951)

bors-servo · 2016-10-28T18:38:14Z

💔 Test failed - linux-rel-wpt

larsbergstrom · 2016-10-28T18:50:24Z

@bors-servo retry

infra

bors-servo · 2016-10-28T18:50:28Z

⌛ Testing commit 46e2918 with merge e7b5891...


        Auto merge of #13951 - bholley:more_element, r=emilio

incremental restyle: Hoist more stuff to Element The patch here hoists pseudo-element handling onto ThreadSafeLayoutElement. I have another patch which hoists stuff from TNode to TElement, but want to make sure this patch passes try first.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13951)


        Auto merge of #13956 - bholley:more_telement, r=<try>

incremental restyle: Hoist most styling functionality from TNode to TElement This is a continuation of the work in #13951. I'm separating it out into a separate PR since the aforementioned patch has a green try run and this one doesn't yet.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13956)

bors-servo · 2016-10-28T19:33:10Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev


        Auto merge of #13956 - bholley:more_telement, r=<try>

incremental restyle: Hoist most styling functionality from TNode to TElement This is a continuation of the work in #13951. I'm separating it out into a separate PR since the aforementioned patch has a green try run and this one doesn't yet.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13956)


        Auto merge of #13956 - bholley:more_telement, r=<try>

incremental restyle: Hoist most styling functionality from TNode to TElement This is a continuation of the work in #13951. I'm separating it out into a separate PR since the aforementioned patch has a green try run and this one doesn't yet.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13956)


        Auto merge of #13956 - bholley:more_telement, r=<try>

incremental restyle: Hoist most styling functionality from TNode to TElement This is a continuation of the work in #13951. I'm separating it out into a separate PR since the aforementioned patch has a green try run and this one doesn't yet.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13956)


        Auto merge of #13956 - bholley:more_telement, r=<try>

incremental restyle: Hoist most styling functionality from TNode to TElement This is a continuation of the work in #13951. I'm separating it out into a separate PR since the aforementioned patch has a green try run and this one doesn't yet.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13956)


        Auto merge of #13956 - bholley:more_telement, r=<try>

incremental restyle: Hoist most styling functionality from TNode to TElement This is a continuation of the work in #13951. I'm separating it out into a separate PR since the aforementioned patch has a green try run and this one doesn't yet.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13956)


        Auto merge of #13956 - bholley:more_telement, r=<try>

incremental restyle: Hoist most styling functionality from TNode to TElement This is a continuation of the work in #13951. I'm separating it out into a separate PR since the aforementioned patch has a green try run and this one doesn't yet.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13956)


        Auto merge of #13956 - bholley:more_telement, r=emilio

incremental restyle: Hoist most styling functionality from TNode to TElement This is a continuation of the work in #13951. I'm separating it out into a separate PR since the aforementioned patch has a green try run and this one doesn't yet.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13956)


        Auto merge of #13956 - bholley:more_telement, r=emilio

incremental restyle: Hoist most styling functionality from TNode to TElement This is a continuation of the work in #13951. I'm separating it out into a separate PR since the aforementioned patch has a green try run and this one doesn't yet.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13956)

highfive assigned cbrewster Oct 28, 2016

highfive added the S-awaiting-review label Oct 28, 2016

highfive assigned emilio and unassigned cbrewster Oct 28, 2016

bholley mentioned this pull request Oct 28, 2016

incremental restyle: Hoist most styling functionality from TNode to TElement #13956

Merged

emilio approved these changes Oct 28, 2016

View changes

Move core pseudo-element handling to ThreadSafeLayoutElement.

Loading status checks…

46e2918

bholley force-pushed the bholley:more_element branch from 9094b8c to 46e2918 Oct 28, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 28, 2016

highfive removed the S-awaiting-merge label Oct 28, 2016

highfive added the S-awaiting-merge label Oct 28, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 28, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Oct 28, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 28, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Oct 28, 2016

bors-servo merged commit 46e2918 into servo:master Oct 28, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Oct 28, 2016

bholley deleted the bholley:more_element branch Oct 30, 2016

servo / servo

Join GitHub today

incremental restyle: Hoist more stuff to Element #13951

incremental restyle: Hoist more stuff to Element #13951

Conversation

bholley commented Oct 28, 2016 • edited by larsbergstrom

highfive commented Oct 28, 2016

highfive commented Oct 28, 2016

bholley commented Oct 28, 2016

bors-servo commented Oct 28, 2016

bors-servo commented Oct 28, 2016

bholley commented Oct 28, 2016

emilio left a comment

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.

bholley commented Oct 28, 2016

bors-servo commented Oct 28, 2016

bors-servo commented Oct 28, 2016

larsbergstrom commented Oct 28, 2016

bors-servo commented Oct 28, 2016

bors-servo commented Oct 28, 2016

bors-servo commented Oct 28, 2016

bholley commented Oct 28, 2016

bors-servo commented Oct 28, 2016

bors-servo commented Oct 28, 2016

larsbergstrom commented Oct 28, 2016

bors-servo commented Oct 28, 2016

bors-servo commented Oct 28, 2016

bholley commented Oct 28, 2016 •

edited by larsbergstrom