Centralize event states in rust-selectors #8162

bholley · 2015-10-22T20:37:34Z

This still needs a rev bump on rust-selectors once servo/rust-selectors#55 gets merged.

highfive · 2015-10-22T20:37:37Z

Warning

These commits modify layout code, but no reftests are modified. Please consider adding a reftest!

bholley · 2015-10-22T20:37:45Z

r? @Manishearth

bholley · 2015-10-22T22:16:48Z

Added rust-selectors rev bump.

Manishearth · 2015-10-22T22:17:54Z

components/script/dom/element.rs

-}
+// NB: We can't [derive(..)] this because it's defined in rust-selectors, which
+// doesn't know about JS<T>.
+impl JSTraceable for EventState { fn trace(&self, _trc: *mut JSTracer) {} }


no_jsmanaged_fields! is what we should use here (grep the source for it)

Manishearth · 2015-10-22T22:46:35Z

Reviewed 15 of 15 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

^{components/script/dom/htmlinputelement.rs, line 245 [r2] (raw file):}
There's some extra logic in update_checked_state, let's move the set_state there

Comments from the review on Reviewable.io

bholley · 2015-10-22T22:56:24Z

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

^{components/script/dom/htmlinputelement.rs, line 245 [r2] (raw file):}
Whoops, good catch!

Comments from the review on Reviewable.io

bholley · 2015-10-22T22:56:38Z

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

^{components/script/dom/htmlinputelement.rs, line 245 [r2] (raw file):}
Done.

Comments from the review on Reviewable.io

SimonSapin · 2015-10-23T01:09:37Z

r-

This change is unjustified, seems unnecessary to me (is there a benefit I don’t see?), and the corresponding rust-selectors change (servo/rust-selectors#55) forces users to either keep all state in a single u16 of bit flags like Servo does, or do mork work than necessary gathering all state when only one bit is used.

Please explain motivation for this change and the rust-selectors one.

bholley · 2015-10-23T01:12:18Z

@SimonSapin:
See servo/rust-selectors#55 (comment) , and the email I just sent.

SimonSapin · 2015-10-23T01:14:53Z

Can we discuss in public, where people other than me can find it later?

bholley · 2015-10-23T01:16:33Z

@SimonSapin I need to run, and there's not an easy way to convert an email thread to a github issue thread. But feel free to post/summarize, and I can respond in a few hours when I get back.

bholley · 2015-10-23T06:57:45Z

A bit more context here now that I have a few minutes (sorry for the delay):

My high-level goal here is to fix #6942 by implementing restyle hints in Servo. This logic needs to live at the intersection of layout, style, and rust-selectors, and therefore will understandably involve changes to rust-selectors.

Restyle hints work by batching up a series of EventState changes for a given Element and then asking the style system for an upper-bound of the categories of elements (self, subtree, siblings, etc) that may need restyling. In order to ask this question, rust-selectors and layout need a common bitfield type by which to represent a set of event states. Given the dependency tree, this type needs to live in rust-selectors, hence this change.

Once we have this common representation, it seems like an obvious simplification to remove the per-state methods from the trait definition and pass this information with a single method. This removes a lot of boilerplate code (duplicated across tree.rs, element.rs, and wrapper.rs), and makes it possible to access the entire set of state values with a single virtual call. More importantly, it eliminates the chicken-and-egg dependency problem that causes Servo to break when rust-selectors adds support for a new EventState. This bit me today with #8142 , and I wanted to prevent it from biting other people.

The one downside is the one mentioned in [1], whereby other theoretical consumers without a centralized EventState representation will pay a performance cost when aggregating their state. However, optimizing performance for multiple consumer designs is pretty hard. Any consumer that wants maximal performance is going to want to centralize EventState to take advantage of restyle hints (and subsequent optimizations beyond it), so I'm not convinced that this should block what we do here.

[1] #8162 (comment)

bholley · 2015-10-23T07:12:29Z

@Ms2ger explained to me that our use of generics allows us to bypass virtual calls on the trait impls (which is super neat!). I think the rest of the points are still valid though.

SimonSapin · 2015-10-23T07:48:46Z

https://github.com/SimonSapin/kuchiki is not theoretical. It doesn’t have many users yet (as far as I know?), but there was demand for it repetitively.

I’m not buying the preformance thing. As you mentioned @Ms2ger explained, the compiler generates separate code for generic functions/methods for each concrete type arguments. Moving the bitfield.contains() call to rust-selectors might not even affect the generated code (assuming sufficiently advanced inlining). Dynamic dispatch (and type ereasure / heterogenous collections) can be opted into by using trait objects instead of generics.

Repetitive code probably wouldn’t be hard to generate with macro_rules!.

Please explain motivation in pull request messages. Restyle hints does make servo/rust-selectors#55 seem less gratuitous than it first did in isolation.

Still, it’s not clear to me: why does rust-selectors need to know about this bitfield?

bholley · 2015-10-23T17:12:24Z

Please explain motivation in pull request messages.

Sorry. The motivation is explained in commit messages and comments of other dependent commits, but I was trying to get this piece landed separately, since it requires me to update Servo's rust-selectors dependency, and I wanted to isolate any issues that might be caused by that step.

The larger context here is that @pcwalton and I are investigating what it would take to plug Servo's style system into Gecko. In order to familiarize myself with Servo's style system (and Rust in general), I'm implementing restyle hints.

Still, it’s not clear to me: why does rust-selectors need to know about this bitfield?

When testing restyle hints, we need to match against selectors while ignoring the subset of pseudo-selectors corresponding to the EventStates that changed. Suppose a selector S of the form a b:invalid:focus:hover c exists in a stylesheet, and we change the focus and hover state of an element foo. The restyle hint algorithm would then want to match against the effective selector T of the form a b:invalid in order to determine whether the event changes in foo might affect the set of elements in the document matching S.

However, T may not exist in any stylesheet, and it cannot be computed before match time because it depends on the set of EventStates that changed. So we need to pass an optional bitfield to selector matching instructing rust-selectors which pseudo-classes to ignore. This is why rust-selectors needs EventState (or something isomorphic), and my primary motivation for moving it.

Another side-benefit is that it consolidates the two existing enumerates of states (EventState and SimpleSelector) into the same library, making it easier to change both of them together (see my complaint about :target). Landing coordinated mutually-dependent changes to multiple libraries is a barrier to contribution that I'd like to remove.

Repetitive code probably wouldn’t be hard to generate with macro_rules!

Potentially yes, but we still need to enumerate those states in all of the places I mentioned, making us susceptible to cross-library breakage.

All that being said, coalescing the API was an attempt to simplify the API and improve the cross-library ergonomic footguns that I was encountering. We could revert that change if you like without scuttling restyle hints, though I do think that doing so would make our lives harder on the whole.

SimonSapin · 2015-10-24T16:31:11Z

Please see my proposed changes in servo/rust-selectors#56 and discuss them there. Let’s block this in the mean time.

bors-servo · 2015-10-31T03:38:25Z

🔒 Merge conflict

bors-servo · 2015-10-31T03:39:23Z

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

Manishearth · 2015-10-31T03:39:57Z

Probably should start using p=1 for PRs which have a tendency to bitrot often. (@bors-servo p=1 works)

This is necessary for those selectors to take advantage of restyle hints.

bholley · 2015-10-31T04:03:05Z

@Manishearth r?

bholley · 2015-10-31T04:04:57Z

@bors-servo p=1

mbrubeck · 2015-10-31T04:35:43Z

@bors-servo r=pcwalton p=1

bors-servo · 2015-10-31T04:35:44Z

📌 Commit 79ac365 has been approved by pcwalton

bors-servo · 2015-10-31T04:35:47Z

⌛ Testing commit 79ac365 with merge 521a871...

Centralize event states in rust-selectors This still needs a rev bump on rust-selectors once servo/rust-selectors#55 gets merged.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8162)

bors-servo · 2015-10-31T05:14:05Z

💔 Test failed - linux-rel

mbrubeck · 2015-10-31T05:23:48Z

@bors-servo retry

#8157

bors-servo · 2015-10-31T05:23:49Z

⚡ Previous build results for android, gonk, linux-dev, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only linux-rel...

bors-servo · 2015-10-31T06:02:11Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

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

highfive assigned Manishearth Oct 22, 2015

bholley force-pushed the centralize_event_states branch from b3a6f8a to eabcb38 Compare October 22, 2015 22:15

Manishearth reviewed Oct 22, 2015
View reviewed changes

bholley force-pushed the centralize_event_states branch from eabcb38 to a162226 Compare October 22, 2015 22:56

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 22, 2015

SimonSapin added the S-awaiting-answer Someone asked a question that requires an answer. label Oct 23, 2015

bholley mentioned this pull request Oct 23, 2015

Move EventState from servo to rust-selectors servo/rust-selectors#55

Merged

SimonSapin added S-needs-decision A decision is required and has not been made. and removed S-awaiting-answer Someone asked a question that requires an answer. labels Oct 24, 2015

bholley force-pushed the centralize_event_states branch from a162226 to 1e16a66 Compare October 26, 2015 22:57

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Oct 26, 2015

highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 31, 2015

bholley added 2 commits October 30, 2015 21:01

Hoist checked and indeterminate state into EventState.

f7f27a8

This is necessary for those selectors to take advantage of restyle hints.

Move EventState to rust-selectors.

79ac365

bholley force-pushed the centralize_event_states branch from 37fb636 to 79ac365 Compare October 31, 2015 04:02

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Oct 31, 2015

bholley removed the S-needs-rebase There are merge conflict errors. label Oct 31, 2015

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 31, 2015

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 31, 2015

bors-servo mentioned this pull request Oct 31, 2015

Implement 'labels' attribute on 'labelable elements' #7965

Merged

bors-servo merged commit 79ac365 into servo:master Oct 31, 2015

bors-servo mentioned this pull request Oct 31, 2015

M1504: Implement support for missing XMLHttpRequest APIs #8182

Merged

SimonSapin removed the S-tests-failed The changes caused existing tests to fail. label Jan 27, 2016

bholley deleted the centralize_event_states branch October 30, 2016 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Centralize event states in rust-selectors #8162

Centralize event states in rust-selectors #8162

bholley commented Oct 22, 2015

highfive commented Oct 22, 2015

bholley commented Oct 22, 2015

bholley commented Oct 22, 2015

Manishearth Oct 22, 2015

Manishearth commented Oct 22, 2015

bholley commented Oct 22, 2015

bholley commented Oct 22, 2015

SimonSapin commented Oct 23, 2015

bholley commented Oct 23, 2015

SimonSapin commented Oct 23, 2015

bholley commented Oct 23, 2015

bholley commented Oct 23, 2015

bholley commented Oct 23, 2015

SimonSapin commented Oct 23, 2015

bholley commented Oct 23, 2015

SimonSapin commented Oct 24, 2015

bors-servo commented Oct 31, 2015

bors-servo commented Oct 31, 2015

Manishearth commented Oct 31, 2015

bholley commented Oct 31, 2015

bholley commented Oct 31, 2015

mbrubeck commented Oct 31, 2015

bors-servo commented Oct 31, 2015

bors-servo commented Oct 31, 2015

bors-servo commented Oct 31, 2015

mbrubeck commented Oct 31, 2015

bors-servo commented Oct 31, 2015

bors-servo commented Oct 31, 2015

Centralize event states in rust-selectors #8162

Centralize event states in rust-selectors #8162

Conversation

bholley commented Oct 22, 2015

highfive commented Oct 22, 2015

bholley commented Oct 22, 2015

bholley commented Oct 22, 2015

Manishearth Oct 22, 2015

Choose a reason for hiding this comment

Manishearth commented Oct 22, 2015

bholley commented Oct 22, 2015

bholley commented Oct 22, 2015

SimonSapin commented Oct 23, 2015

bholley commented Oct 23, 2015

SimonSapin commented Oct 23, 2015

bholley commented Oct 23, 2015

bholley commented Oct 23, 2015

bholley commented Oct 23, 2015

SimonSapin commented Oct 23, 2015

bholley commented Oct 23, 2015

SimonSapin commented Oct 24, 2015

bors-servo commented Oct 31, 2015

bors-servo commented Oct 31, 2015

Manishearth commented Oct 31, 2015

bholley commented Oct 31, 2015

bholley commented Oct 31, 2015

mbrubeck commented Oct 31, 2015

bors-servo commented Oct 31, 2015

bors-servo commented Oct 31, 2015

bors-servo commented Oct 31, 2015

mbrubeck commented Oct 31, 2015

bors-servo commented Oct 31, 2015

bors-servo commented Oct 31, 2015