style: Expose the traversal kind to the style system. #15160

emilio · 2017-01-23T18:57:20Z

This way we'll be able to take different paths for the sequential and parallel
traversals in some concrete cases.

This is a preliminar patch to fix bug 1332525.

r? @bholley

This change is

highfive · 2017-01-23T18:57:23Z

Heads up! This PR modifies the following files:

@bholley: components/style/parallel.rs, components/style/sequential.rs, components/style/gecko/traversal.rs, components/style/traversal.rs

highfive · 2017-01-23T18:57:24Z

Warning

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

bholley · 2017-01-23T19:58:55Z

components/style/traversal.rs

@@ -62,6 +62,23 @@ impl LogBehavior {
    fn allow(&self) -> bool { matches!(*self, MayLog) }
 }

+/// The kind of traversals we could perform.
+#[derive(Debug, Copy, Clone)]
+pub enum TraversalKind {


Nit: I think it would be a little more specific to refer to this as TraversalDriver. It's a bit of a judgement call though so I won't insist.

bholley · 2017-01-23T20:01:01Z

components/style/traversal.rs

+    /// regard via the type system via a `TraversalKind` trait for this trait,
+    /// that could be one of two concrete types. It's not clear whether the
+    /// potential code size impact of that is worth it.
+    fn kind(&self) -> TraversalKind;


Yeah I agree that doing this dynamically makes the most sense.

I think the ergonomics would be a bit better if we just had is_parallel() here, rather than implementing it on TraversalKind, then exposing kind(), then having the caller check that. We can always expose more information as-needed if we ever develop new traversal variants.

bholley

@bors-servo delegate+

I'd prefer 'driver' to 'kind', but am ok with 'kind' if you prefer it. I'd like to do the other fix mentioned though.

bholley · 2017-01-23T20:02:44Z

@bors-servo delegate+

(Apparently that doesn't work in review comments)

bors-servo · 2017-01-23T20:02:45Z

✌️ @emilio can now approve this pull request

emilio · 2017-01-24T01:14:03Z

Driver works for me, and I don't feel like bike-shedding about this ;)

@bors-servo r=bholley

bors-servo · 2017-01-24T01:14:04Z

📌 Commit de53d6b has been approved by bholley

emilio · 2017-01-24T01:17:38Z

@bors-servo r-

Let's wait for #15164 to land first.

bholley · 2017-01-24T02:07:45Z

components/layout_thread/lib.rs

@@ -1173,7 +1173,13 @@ impl LayoutThread {
                                                                         data.reflow_info.goal);

        // NB: Type inference falls apart here for some reason, so we need to be very verbose. :-(
-        let traversal = RecalcStyleAndConstructFlows::new(shared_layout_context);
+        let traversal_kind = if self.parallel_flag && self.parallel_traversal.is_some() {


Nit: probably call this variable |driver|.

bholley · 2017-01-24T02:08:17Z

ports/geckolib/glue.rs

-
-    if per_doc_data.num_threads == 1 || per_doc_data.work_queue.is_none() {
-        sequential::traverse_dom(&traversal, element, token);
+    let traversal_kind = if per_doc_data.num_threads == 1 || per_doc_data.work_queue.is_none() {


emilio · 2017-01-24T07:39:05Z

@bors-servo r=bholley

bors-servo · 2017-01-24T07:39:06Z

📌 Commit c675cdd has been approved by bholley


        style: Expose the traversal kind to the style system.

This way we'll be able to take different paths for the sequential and parallel traversals in some concrete cases. This is a preliminar patch to fix bug 1332525.

emilio · 2017-01-24T07:42:57Z

@bors-servo r=bholley

You can't sed fast enough

bors-servo · 2017-01-24T07:42:58Z

📌 Commit f00b628 has been approved by bholley

bors-servo · 2017-01-24T13:43:18Z

⌛ Testing commit f00b628 with merge 55165bf...


        Auto merge of #15160 - emilio:expose-traversal-kind, r=bholley

@bholley

style: Expose the traversal kind to the style system. This way we'll be able to take different paths for the sequential and parallel traversals in some concrete cases. This is a preliminar patch to fix bug 1332525. r? @bholley  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15160)

bors-servo · 2017-01-24T14:11:38Z

💔 Test failed - windows-gnu-dev

Ms2ger · 2017-01-24T15:16:07Z

@bors-servo retry servo/saltfs#585

jdm · 2017-01-24T19:16:07Z

@bors-servo: r-

mbrubeck · 2017-01-24T22:03:34Z

@bors-servo r=bholley

bors-servo · 2017-01-24T22:03:34Z

📌 Commit f00b628 has been approved by bholley

bors-servo · 2017-01-25T01:02:42Z

⌛ Testing commit f00b628 with merge 1934a33...


        Auto merge of #15160 - emilio:expose-traversal-kind, r=bholley

@bholley

style: Expose the traversal kind to the style system. This way we'll be able to take different paths for the sequential and parallel traversals in some concrete cases. This is a preliminar patch to fix bug 1332525. r? @bholley  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15160)

bors-servo · 2017-01-25T04:06:15Z

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

highfive assigned bholley Jan 23, 2017

highfive added the S-awaiting-review label Jan 23, 2017

emilio force-pushed the emilio:expose-traversal-kind branch from 1828302 to ff88d9a Jan 23, 2017

bholley reviewed Jan 23, 2017

View changes

emilio force-pushed the emilio:expose-traversal-kind branch 2 times, most recently from 70d4034 to de53d6b Jan 24, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 24, 2017

bholley reviewed Jan 24, 2017

View changes

emilio force-pushed the emilio:expose-traversal-kind branch from de53d6b to c675cdd Jan 24, 2017

highfive added S-awaiting-review and removed S-awaiting-merge labels Jan 24, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 24, 2017

emilio force-pushed the emilio:expose-traversal-kind branch from c675cdd to f00b628 Jan 24, 2017

highfive added S-awaiting-review and removed S-awaiting-merge labels Jan 24, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 24, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 24, 2017

jdm added the S-blocked-on-closed-tree label Jan 24, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels Jan 24, 2017

mbrubeck removed the S-blocked-on-closed-tree label Jan 24, 2017

bors-servo merged commit f00b628 into servo:master Jan 25, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Jan 25, 2017

emilio deleted the emilio:expose-traversal-kind branch Jan 25, 2017

servo / servo

style: Expose the traversal kind to the style system. #15160

style: Expose the traversal kind to the style system. #15160

emilio commented Jan 23, 2017 •

edited by larsbergstrom

highfive commented Jan 23, 2017

highfive commented Jan 23, 2017

This comment has been minimized.

This comment has been minimized.

bholley left a comment

bholley commented Jan 23, 2017

bors-servo commented Jan 23, 2017

emilio commented Jan 24, 2017

bors-servo commented Jan 24, 2017

emilio commented Jan 24, 2017

This comment has been minimized.

This comment has been minimized.

emilio commented Jan 24, 2017

bors-servo commented Jan 24, 2017

emilio commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 24, 2017

Ms2ger commented Jan 24, 2017

jdm commented Jan 24, 2017

mbrubeck commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 25, 2017

bors-servo commented Jan 25, 2017

servo / servo

Join GitHub today

style: Expose the traversal kind to the style system. #15160

style: Expose the traversal kind to the style system. #15160

Conversation

emilio commented Jan 23, 2017 • edited by larsbergstrom

highfive commented Jan 23, 2017

highfive commented Jan 23, 2017

This comment has been minimized.

bholley Jan 23, 2017

This comment has been minimized.

bholley Jan 23, 2017

bholley left a comment

bholley commented Jan 23, 2017

bors-servo commented Jan 23, 2017

emilio commented Jan 24, 2017

bors-servo commented Jan 24, 2017

emilio commented Jan 24, 2017

This comment has been minimized.

bholley Jan 24, 2017

This comment has been minimized.

bholley Jan 24, 2017

emilio commented Jan 24, 2017

bors-servo commented Jan 24, 2017

emilio commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 24, 2017

Ms2ger commented Jan 24, 2017

jdm commented Jan 24, 2017

mbrubeck commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 25, 2017

bors-servo commented Jan 25, 2017

emilio commented Jan 23, 2017 •

edited by larsbergstrom