style: Split the invalidation processing from the invalidator step. #18847

emilio · 2017-10-12T11:34:00Z

This is the first step in reusing the invalidation machinery for other stuff,
potentially including QuerySelector / QuerySelectorAll.

This change is

highfive · 2017-10-12T11:34:05Z

Heads up! This PR modifies the following files:

@bholley: components/style/data.rs, components/style/invalidation/element/collector.rs, components/style/invalidation/element/invalidator.rs, components/style/invalidation/element/mod.rs
@canaltinova: components/style/data.rs, components/style/invalidation/element/collector.rs, components/style/invalidation/element/invalidator.rs, components/style/invalidation/element/mod.rs

highfive · 2017-10-12T11:34:06Z

Warning

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

emilio · 2017-10-12T11:34:59Z

r? @heycam or @SimonSapin

This is just moving code around, no behavior change (apart from introducing the InvalidatorCollector trait).

emilio · 2017-10-12T16:37:50Z

I ended up working more on top of this, so please review each commit individually.

Thanks a lot!

heycam

Looks good!

heycam · 2017-10-13T05:42:43Z

components/style/invalidation/element/collector.rs

@@ -49,10 +49,11 @@ where
 }

 ///  A collector for state and attribute invalidations.


Update this comment now that these things are called processors rather than collectors?

heycam · 2017-10-13T05:44:54Z

components/style/invalidation/element/invalidator.rs

@@ -33,12 +33,61 @@ pub trait InvalidationCollector {
    ) -> bool
    where
        E: TElement;
+
+    /// Returns whether a given element should process its descendants.


Nit: I guess it's the process that processes the descendants, not the element. So maybe "Returns whether the invalidation process should process the descendants of the given element."

Also, document what data is, here and on the other functions?

heycam · 2017-10-13T05:46:08Z

components/style/invalidation/element/invalidator.rs

+    /// any).
+    fn recursion_limit_exceeded<E>(
+        &self,
+        _element: E,


Nit: odd to use "_" in a trait function declaration.

heycam · 2017-10-13T06:01:41Z

components/style/invalidation/element/invalidator.rs

+        let invalidated_descendants = child_invalidator.invalidate_descendants(
+            &invalidations_for_descendants
+        );
+
        // The child may not be a flattened tree child of the current element,
        // but may be arbitrarily deep.
        //
        // Since we keep the traversal flags in terms of the flattened tree,
        // we need to propagate it as appropriate.


This comment seems to be describing what we do inside the invalidated_descendants implementation, which could be anything on some abritrary InvalidationProcessor. This comment is already duplicated in the (one) invalidated_descendants implementation so I think we should just drop this comment here.

heycam · 2017-10-13T06:05:12Z

components/style/invalidation/element/invalidator.rs

@@ -22,6 +22,11 @@ pub trait InvalidationProcessor<E>
 where
    E: TElement,
 {
+    /// Whether an invalidation that only leaves an eager pseudo-element like


(Not sure what you mean by "leaves".)

This is the first step in reusing the invalidation machinery for other stuff, potentially including QuerySelector / QuerySelectorAll.

…an InvalidationProcessor trait. Ditto, no change in behavior.

I think invalidated_descendants was buggy, and this fixes it.

…s self" bit.

This would allow querySelector / querySelectorAll to mutate the list of matched nodes as it sees fit.

emilio · 2017-10-13T10:17:23Z

@bors-servo r=heycam p=1

bors-servo · 2017-10-13T10:17:24Z

📌 Commit e447f81 has been approved by heycam

bors-servo · 2017-10-13T11:14:44Z

⌛ Testing commit e447f81 with merge a89a76e...

style: Split the invalidation processing from the invalidator step. This is the first step in reusing the invalidation machinery for other stuff, potentially including QuerySelector / QuerySelectorAll.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18847)

bors-servo · 2017-10-13T12:22:39Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: heycam
Pushing a89a76e to master...

highfive assigned KiChjang Oct 12, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 12, 2017

highfive assigned heycam and unassigned KiChjang Oct 12, 2017

emilio force-pushed the invalidation-generic branch from 9378669 to 0023ceb Compare October 12, 2017 15:23

emilio changed the title ~~style: Split the invalidation collection from the invalidator step.~~ style: Split the invalidation processing from the invalidator step. Oct 12, 2017

heycam approved these changes Oct 13, 2017

View reviewed changes

emilio added 7 commits October 13, 2017 12:16

style: Split the invalidation collection from the invalidator step.

b9b3e59

This is the first step in reusing the invalidation machinery for other stuff, potentially including QuerySelector / QuerySelectorAll.

style: Isolate all the restyling related logic in the invalidator in …

a5e2f2c

…an InvalidationProcessor trait. Ditto, no change in behavior.

style: Move the bounds up in InvalidationProcessor.

557353c

style: Unify invalidated_child with invalidated_descendants.

9e61c19

I think invalidated_descendants was buggy, and this fixes it.

Be more precise in should_process_descendants.

ecdb10e

style: Add a way to skip the "invalidation on eager pseudo invalidate…

9034e6a

…s self" bit.

style: Make InvalidationProcessor methods be &mut self.

e447f81

This would allow querySelector / querySelectorAll to mutate the list of matched nodes as it sees fit.

emilio force-pushed the invalidation-generic branch from 832b864 to e447f81 Compare October 13, 2017 10:17

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 13, 2017

bors-servo merged commit e447f81 into servo:master Oct 13, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style: Split the invalidation processing from the invalidator step. #18847

style: Split the invalidation processing from the invalidator step. #18847

emilio commented Oct 12, 2017 •

edited by SimonSapin

highfive commented Oct 12, 2017

highfive commented Oct 12, 2017

emilio commented Oct 12, 2017

emilio commented Oct 12, 2017

heycam left a comment

heycam Oct 13, 2017

heycam Oct 13, 2017

heycam Oct 13, 2017

heycam Oct 13, 2017

heycam Oct 13, 2017

emilio commented Oct 13, 2017

bors-servo commented Oct 13, 2017

bors-servo commented Oct 13, 2017

bors-servo commented Oct 13, 2017

		@@ -49,10 +49,11 @@ where
		}

		/// A collector for state and attribute invalidations.

style: Split the invalidation processing from the invalidator step. #18847

style: Split the invalidation processing from the invalidator step. #18847

Conversation

emilio commented Oct 12, 2017 • edited by SimonSapin

highfive commented Oct 12, 2017

highfive commented Oct 12, 2017

emilio commented Oct 12, 2017

emilio commented Oct 12, 2017

heycam left a comment

Choose a reason for hiding this comment

heycam Oct 13, 2017

Choose a reason for hiding this comment

heycam Oct 13, 2017

Choose a reason for hiding this comment

heycam Oct 13, 2017

Choose a reason for hiding this comment

heycam Oct 13, 2017

Choose a reason for hiding this comment

heycam Oct 13, 2017

Choose a reason for hiding this comment

emilio commented Oct 13, 2017

bors-servo commented Oct 13, 2017

bors-servo commented Oct 13, 2017

bors-servo commented Oct 13, 2017

emilio commented Oct 12, 2017 •

edited by SimonSapin