stylo: avoid traversing non element/text nodes in style and layout #13172

bholley · 2016-09-04T09:34:16Z

r? @emilio

This change is

highfive · 2016-09-04T09:34:19Z

Warning

These commits modify style code, but no tests are modified. Please consider adding a test!

emilio · 2016-09-04T09:35:51Z

@bors-servo: r+

bors-servo · 2016-09-04T09:35:52Z

📌 Commit 9f8b2b1 has been approved by emilio

bors-servo · 2016-09-04T09:42:02Z

⌛ Testing commit 9f8b2b1 with merge f6d8fa1...


        Auto merge of #13172 - bholley:display_enum, r=emilio

@emilio

stylo: Update bindings and avoid traversing non-stylable nodes r? @emilio  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13172)

bors-servo · 2016-09-04T10:41:06Z

💔 Test failed - mac-rel-wpt

emilio · 2016-09-04T11:10:46Z

Well, the stack traces in the builder are, er... not great, but basically, a bit of code in Servo's layout expects all the children layout data to be initialized, let me find a link to the actual problematic line...

emilio · 2016-09-04T11:18:44Z

Yeah, basically, this is the problematic line: https://github.com/servo/servo/blob/master/components/layout/construct.rs#L1393. (The unwrap() in flags() is what fails).

IMO the solution should be splitting the layout data and the style data properly, but that's probably too much for this patch, so probably you want different implementations for Servo and Gecko, and the assertion inside an if cfg!(not(feature = "servo")) block.

Also, please put a // FIXME: Split layout and style data so we can stop styling comments and such in the Servo impl.

bors-servo · 2016-09-09T07:59:21Z

☔ The latest upstream changes (presumably #13200) made this pull request unmergeable. Please resolve the merge conflicts.


        Split out is_element and is_text_node into a helper trait.


        Introduce the LayoutIterator newtype and return it for all children()…

… methods in style and layout.

bholley · 2016-09-21T00:48:48Z

@bors-servo try

bors-servo · 2016-09-21T00:48:51Z

⌛ Trying commit fa7214b with merge 570d97e...


        Auto merge of #13172 - bholley:display_enum, r=<try>

@emilio

stylo: avoid traversing non element/text nodes in style and layout r? @emilio  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13172)

bors-servo · 2016-09-21T01:39:02Z

💔 Test failed - mac-rel-css

bors-servo · 2016-09-21T18:13:52Z

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

bholley · 2016-09-21T18:28:21Z

r? @emilio

emilio

neat, r=me with the nits :)

Also, a comment in the appropriate places indicating to this iterator would be awesome, since it might be tricky to find otherwise.

emilio · 2016-09-21T18:35:07Z

components/script/layout_wrapper.rs

+impl<'ln> NodeInfo for ServoThreadSafeLayoutNode<'ln> {
+    fn is_element(&self) -> bool {
+        if let Some(LayoutNodeType::Element(_)) = self.type_id() { true } else { false }
+    }


nit: missing newline

emilio · 2016-09-21T18:35:07Z

components/style/dom.rs

+    fn next(&mut self) -> Option<I> {
+        loop {
+            // Filter out nodes that layout should ignore.
+            let n = self.0.next();


nit: maybe this would look clearer as:

match self.0.next() { None => return None, Some(node) if node.needs_layout() => return Some(node), _ => {}, }

Though I don't have a strong preference, so feel free to leave as-is.


        Filter non-element / non-text nodes in LayoutIterator.

bholley · 2016-09-21T18:48:24Z

@bors-servo r=emilio

bors-servo · 2016-09-21T18:48:25Z

📌 Commit 63124ba has been approved by emilio

bors-servo · 2016-09-21T19:11:52Z

⌛ Testing commit 63124ba with merge f773cb8...


        Auto merge of #13172 - bholley:display_enum, r=emilio

@emilio

stylo: avoid traversing non element/text nodes in style and layout r? @emilio  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13172)

bors-servo · 2016-09-21T21:05:29Z

💔 Test failed - mac-rel-wpt

highfive · 2016-09-21T21:05:34Z

  ▶ FAIL [expected PASS] /_mozilla/css/inline_block_opacity_change.html
  └   → /_mozilla/css/inline_block_opacity_change.html 154f410f8d7d73334aa7e14a37628b88a672d994
/_mozilla/css/inline_block_opacity_change_ref.html 8db155b7d5bde8852308b23a66bdf5ef0f5a4a00
Testing 154f410f8d7d73334aa7e14a37628b88a672d994 == 8db155b7d5bde8852308b23a66bdf5ef0f5a4a00

emilio · 2016-09-21T21:25:53Z

@bors-servo: retry #13360

bors-servo · 2016-09-21T21:42:16Z

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, windows-dev are reusable. Rebuilding only mac-rel-wpt...

bors-servo · 2016-09-21T22:46:00Z

💔 Test failed - mac-rel-wpt

bholley · 2016-09-21T23:00:18Z

@bors-servo retry #12654

bors-servo · 2016-09-22T00:59:53Z

⌛ Testing commit 63124ba with merge 614e9ca...


        Auto merge of #13172 - bholley:display_enum, r=emilio

@emilio

stylo: avoid traversing non element/text nodes in style and layout r? @emilio  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13172)

bors-servo · 2016-09-22T02:10:27Z

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

highfive assigned emilio Sep 4, 2016

highfive added the S-awaiting-review label Sep 4, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 4, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 4, 2016

highfive added the S-needs-rebase label Sep 9, 2016

Split out is_element and is_text_node into a helper trait.

e6bbff1

bholley changed the title ~~stylo: Update bindings and avoid traversing non-stylable nodes~~ stylo: avoid traversing non element/text nodes in style and layout Sep 21, 2016

bholley force-pushed the bholley:display_enum branch from 9f8b2b1 to efd7623 Sep 21, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Sep 21, 2016

Introduce the LayoutIterator newtype and return it for all children()…

4aa3e58

… methods in style and layout.

bholley force-pushed the bholley:display_enum branch from efd7623 to fa7214b Sep 21, 2016

highfive added the S-tests-failed label Sep 21, 2016

bholley force-pushed the bholley:display_enum branch from fa7214b to c5d6c2b Sep 21, 2016

highfive removed the S-tests-failed label Sep 21, 2016

bholley removed the S-needs-rebase label Sep 21, 2016

emilio reviewed Sep 21, 2016

View changes

Filter non-element / non-text nodes in LayoutIterator.

Loading status checks…

63124ba

bholley force-pushed the bholley:display_enum branch from c5d6c2b to 63124ba Sep 21, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 21, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 21, 2016

highfive added the S-tests-failed label Sep 21, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Sep 22, 2016

bors-servo mentioned this pull request Sep 22, 2016

Fix WebGL tests & Implement WebGLRenderingContext::{validateProgram, getProgramInfoLog, disableVertexAttribArray}. #13208

Merged

4 of 5 tasks complete

bors-servo merged commit 63124ba into servo:master Sep 22, 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 Sep 22, 2016

bholley deleted the bholley:display_enum branch Oct 9, 2016

servo / servo

stylo: avoid traversing non element/text nodes in style and layout #13172

stylo: avoid traversing non element/text nodes in style and layout #13172

bholley commented Sep 4, 2016 •

edited by larsbergstrom

highfive commented Sep 4, 2016

emilio commented Sep 4, 2016

bors-servo commented Sep 4, 2016

bors-servo commented Sep 4, 2016

bors-servo commented Sep 4, 2016

emilio commented Sep 4, 2016

emilio commented Sep 4, 2016

bors-servo commented Sep 9, 2016

bholley commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bholley commented Sep 21, 2016

emilio left a comment

This comment has been minimized.

This comment has been minimized.

bholley commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bors-servo commented Sep 21, 2016

highfive commented Sep 21, 2016

emilio commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bholley commented Sep 21, 2016

bors-servo commented Sep 22, 2016

bors-servo commented Sep 22, 2016

servo / servo

Join GitHub today

stylo: avoid traversing non element/text nodes in style and layout #13172

stylo: avoid traversing non element/text nodes in style and layout #13172

Conversation

bholley commented Sep 4, 2016 • edited by larsbergstrom

highfive commented Sep 4, 2016

emilio commented Sep 4, 2016

bors-servo commented Sep 4, 2016

bors-servo commented Sep 4, 2016

bors-servo commented Sep 4, 2016

emilio commented Sep 4, 2016

emilio commented Sep 4, 2016

bors-servo commented Sep 9, 2016

bholley commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bholley commented Sep 21, 2016

emilio left a comment

This comment has been minimized.

emilio Sep 21, 2016 • edited

This comment has been minimized.

emilio Sep 21, 2016 • edited

bholley commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bors-servo commented Sep 21, 2016

highfive commented Sep 21, 2016

emilio commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bholley commented Sep 21, 2016

bors-servo commented Sep 22, 2016

bors-servo commented Sep 22, 2016

bholley commented Sep 4, 2016 •

edited by larsbergstrom

emilio Sep 21, 2016 •

edited

emilio Sep 21, 2016 •

edited