style: Make the whole style crate independent of the implementation #9567

emilio · 2016-02-08T01:04:41Z

This allows, among other things, having different implementations for parsing pseudo{elements, classes} in both ports/geckolib and in servo.

highfive · 2016-02-08T01:04:45Z

Heads up! This PR modifies the following files:

@bholley: components/style/selector_matching.rs, components/style/selector_impl.rs, components/style/matching.rs

highfive · 2016-02-08T01:04:46Z

Warning

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

emilio · 2016-02-08T01:08:16Z

components/style/selector_impl.rs

+        if name.chars().all(|c| c.is_lowercase()) {
+            Ok(PseudoElement::from(name))
+        } else {
+            Ok(PseudoElement::from(&*name.to_ascii_lowercase()))


@bholley: Please look at the commit message, do you thing we could do well accepting only names that start with a dash, and it that case assume they are already lowercase?

That should prevent all extra allocs/traversing of the name.

emilio · 2016-02-08T01:13:18Z

Also note that if we're just planning to support the gecko anon-box pseudo-element list, we could keep PseudoElement as an enum and use this list: https://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSAnonBoxList.h

cc: @SimonSapin

emilio · 2016-02-08T02:49:17Z

Went with the second approach, and saw why both of them needed extra work.

Actually I'll need to implement a way of getting matching rules from a SelectorMap that only correspond to a single pseudo-element (or making an extra map per pseudo, which seems a no-go).

I can:

Do a post-traversal cleanup after getting all matching rules
Implement a function for that in rust-selectors (something like get_all_matching_rules_checked)
Extend the current get_all_matching_rules.

I'd go with the second, but I'd prefer to check-in with Simon in order to confirm that the current approach is correct, and also to discuss a design (in case it is needed) and prevent wasting work.

That being said, it's late, he's probably sleeping and I should too, so... enough for today! :P


        Auto merge of #141 - ecoal95:pseudo-elements, r=asajeffrey

Add "before" and "after" atoms. Needed for servo/servo#9567  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/string-cache/141)

SimonSapin · 2016-02-08T06:48:28Z

components/style/selector_impl.rs

@@ -8,6 +8,73 @@ use selectors::parser::{ParserContext, SelectorImpl};
 pub enum PseudoElement {
    Before,
    After,
+    FirstLine,
+
+    // https://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSAnonBoxList.h


Why is all thing going here rather than in a separate SelectorImpl impl in ports/geckolib? Wasn’t that the point of making the parser generic?

Yeah, forgot to mention that.

Would it be fine to have a feature-gated gecko selector implementation inside style?

I say it because I tried doing exactly that, and the code is pretty suboptimal (and way less readable).

For example, this:

let map = match pseudo_element { Some(PseudoElement::Before) => &self.before_map, Some(PseudoElement::After) => &self.after_map, Some(_) => &self.other_pseudo_elements_map, None => &self.element_map, };

Becomes this:

let map = if let Some(pseudo) = pseudo_element { if pseudo == E::Impl::parse_pseudo_element(&ParserContext::new(), "before").unwrap() { &self.before_map } else if pseudo == E::Impl::parse_pseudo_element(&ParserContext::new(), "after").unwrap() { &self.after_map } else { &self.other_pseudo_elements_map } } else { &self.element_map };

Would it be fine to have a feature-gated gecko selector implementation inside style?

I’m not opposed but I don’t see how it would help.

Becomes this: [...]

Yes, calling parsing code here is silly. It shows a mis-match between PseudoElement being generic and the before_map and after_map fields of Stylist being hard-coded. I don’t think this means me should abandon the generic parser and have a single PseudoElement type. I’d rather change the way Stylist is organized. Maybe some map of PseudoElement to PerPseudoElementSelectorMap?

Yes, having a PerPseudoElementSelectorMap per possible pseudo seemed overkill to me, but it might be doable, and it's, in fact, more ellegan than what we have right now.

I can try to do it first and get some numbers if you want.

I was talking about having two different PseudoElement types, as with two different SelectorImpl, but making a default impl feature gated, something like:

enum ServoPseudoElement { Before, After, FirstLine, } enum GeckoPseudoElement { Before, After, FirstLine, // ... } struct ServoSelectorImpl; struct GeckoSelectorImpl; impl ... for GeckoSelectorImpl { ... } impl ... for ServoSelectorImpl { ... } #[cfg(feature = "geckolib")] pub type DefaultSelectorImpl = GeckoSelectorImpl; #[cfg(not(feature = "geckolib"))] pub type DefaultSelectorImpl = ServoSelectorImpl;

And we'll ve moving again with concrete types, and would work as long as both implementations have a Before and After variants.

Re overkill, I’m also open to changing more significantly how Stylist / PerPseudoElementSelectorMap / SelectorMap are organized. (Though I don’t have a specific design in mind right now.)

Re feature-gated impls, ok, I see what you mean. I’m not necessarily opposed to this, but it doesn’t seem nice. I’d like to explore alternatives some more.

Ms2ger · 2016-02-08T13:29:30Z

I'm not happy about parsing pseudo-elements that Gecko doesn't support.

Ms2ger · 2016-02-08T13:29:44Z

Should be "... that Servo doesn't support".

SimonSapin · 2016-02-08T13:30:20Z

@Ms2ger same here. This is what I mean by a separate SelectorImpl impl in inline comments.

emilio · 2016-02-10T02:49:07Z

Could you guys take a quick look at this to see if the current approach is good enough?

Right now I've got servo building and working fine (with a rust-selectors update to add the Eq and Hash constraints to the PseudoElement associated type).

Will like to make a few more tweaks before landing it is it's good though (fnv and trying to not pre-insert elements in the ApplicableDeclarationsCache).

No gecko backend yet, I'll add it when I've got the time :P


        Add Eq and Hash bounds to PseudoElement

As discussed in IRC while discussing servo/servo#9567


        Add Eq and Hash bounds to PseudoElement

As discussed in IRC while discussing servo/servo#9567


        Add Eq and Hash bounds to PseudoElement

As discussed in IRC while discussing servo/servo#9567

emilio · 2016-02-11T02:43:44Z

I think this should be mostly ready for landing once servo/rust-selectors#69 lands...

r? @SimonSapin (for the style side)
r? @bholley (for the geckolib side)

Still missing the hability to query on-demand for anon-box pseudo-elements, but I'd prefer to do that as a followup since this is already a pretty decent patch.


        Auto merge of #69 - ecoal95:eq-and-hash, r=SimonSapin

Add Eq and Hash bounds to PseudoElement As discussed in IRC while discussing servo/servo#9567  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/rust-selectors/69)

SimonSapin · 2016-02-11T13:07:37Z

components/style/selector_impl.rs

+        let pseudo_element = match_ignore_ascii_case! { name,
+            "before" => Before,
+            "after" => After,
+            "first-line" => FirstLine,


Why add first-line here? We shouldn’t parse things we don’t support.

Sure, sorry! Just parsed it in order to ensure that the problem with the first approaches was solved.

SimonSapin · 2016-02-11T13:14:33Z

components/style/selector_matching.rs

-                            Some(PseudoElement::After) => &mut after_map,
+                        let map = if let Some(ref pseudo) = selector.pseudo_element {
+                            self.pseudos_map.entry(pseudo.clone())
+                                            .or_insert(PerPseudoElementSelectorMap::new())


Is PerPseudoElementSelectorMap::new() expensive? Maybe use .or_insert_with(PerPseudoElementSelectorMap::new) instead.

I think it should only be called on the empty case, but or_insert_with is more elegant anyways so I'll change it :P

SimonSapin · 2016-02-11T13:15:36Z

components/style/selector_matching.rs

                                              V: VecLike<DeclarationBlock> {
        assert!(!self.is_device_dirty);
        assert!(style_attribute.is_none() || pseudo_element.is_none(),
                "Style attributes do not apply to pseudo-elements");

        let map = match pseudo_element {
+            Some(ref pseudo) => self.pseudos_map.get(pseudo).unwrap(),


Can this .unwrap() really not fail?

That's why we pre-insert the eagerly cascaded pseudos map in the stylist constructor, so yes, this can't fail.

Can’t this method be called with a non-eagerly-cascaded pseudo-element?

SimonSapin · 2016-02-11T13:19:46Z

components/style/matching.rs


        applicable_declarations.normal_shareable &&
-        applicable_declarations.before.is_empty() &&
-        applicable_declarations.after.is_empty()
+        applicable_declarations.per_pseudo.values().all(|v| v.is_empty())


We have to be careful here: pseudo-elements that have UA styles with a *|* selector risk defeating style sharing completely.

What pseudo-elements are likely to be in applicable_declarations.per_pseudo at this point? Only the eagerly-cascaded ones?

Actually there's no support for querying those kind of pseudo-elements, so just the eagerly cascaded ones will be here, which are filled a few lines before.

bholley · 2016-02-12T03:59:43Z

Review status: 32 of 43 files reviewed at latest revision, 8 unresolved discussions.

components/layout/traversal.rs, line 32 [r6] (raw file):
Ok. If you have immediate plans to do a followup, feel free to leave as-is. Otherwise, I'd prefer to just operate on ServoLayoutNode and remove the generic stuff here, because it isn't actually generic given the Impl=ServoSelectorImpl.

components/style/restyle_hints.rs, line 83 [r6] (raw file):
Hm, I thought there was a way for traits to add bounds to the associated types of their bounds, but I can't seem to figure it out right now. Anyway, not important, so let's not worry about it for now.

ports/geckolib/selector_impl.rs, line 17 [r6] (raw file):
I'm saying that, instead of:

pub type Stylist = ::style::selector_matching::Stylist;

we do

pub type Stylist = style::selector_matching::Stylist;

That compiles, right?

Comments from the review on Reviewable.io

emilio · 2016-02-13T12:48:05Z

Review status: 27 of 43 files reviewed at latest revision, 7 unresolved discussions.

components/layout/traversal.rs, line 32 [r6] (raw file):
Humm... After giving it a quick try, there's a lot to change (since we should parameterize LayoutContext), including a lot of functions to parameterize due to that.

Thinking about it, most people shouldn't care about the SelectorImpl used in layout, and making layout reusable with a feature flag that defines the default SelectorImpl should be a lot more straightforward, given two style implementations won't coexist (which I don't think they should/will).

Anyways, that's not near-term, so I plan to land this, and work on querying anon-box pseudos from gecko. Removing the N parameter for now :P

ports/geckolib/selector_impl.rs, line 17 [r6] (raw file):
Yes, given we add a use style; line at the top, that was what I was saying above.

Just did that :)

Comments from the review on Reviewable.io

bors-servo · 2016-02-13T14:15:26Z

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


        Refactor style to be completely backend-independent

This commit refactors the style crate to be completely independent of the actual implementation and pseudo-elements supported. This also adds a gecko backend which introduces parsing for the anonymous box pseudo-elements[1], although there's still no way of querying them. https://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSAnonBoxList.h


        style: Use fnv hashing for pseudo-elements


        geckolib: Fix :link and :any-link pseudo-element matching

The logic was switched between them.


        style: Prevent a crash when push_applicable_declarations is called wi…

…th a non eagerly-cascaded pseudo-element This actually didn't happen, but this should be done sooner than later.


        style: Refactor the per_pseudo map from StyleData to avoid having an …

…option value type. This make the layout code way clearer.


        layout: Stop parameterizing on N for RecalcStyleAndConstructFlows

This is unfortunate, but making that useful would require parameterizing `SharedLayoutContext` and `LayoutContext` depending on the `SelectorImpl` (which is a **huge** work right now). Probably the easier way to do it, and probably the one that keeps the layout code more legible, and since there won't be multiple implementations at the same compilation unit, would be "defining" a default implementation for layout via feature flags. That should allow us to remove the components/style/servo.rs file.

bholley · 2016-02-13T20:52:24Z

Looks good to me! Thanks for working on this. :-) r=bholley

Review status: 19 of 43 files reviewed at latest revision, 5 unresolved discussions.

Comments from the review on Reviewable.io

bholley · 2016-02-13T20:53:48Z

(specifically, on the layout and geckolib stuff. I looked at most of the style stuff, but not all of it, since @SimonSapin was already happy with it).

emilio · 2016-02-13T22:29:58Z

@bors-servo: r=bholley,SimonSapin

bors-servo · 2016-02-13T22:29:58Z

📌 Commit 04d2db5 has been approved by bholley,SimonSapin

bors-servo · 2016-02-13T22:30:00Z

⌛ Testing commit 04d2db5 with merge c11844c...


        Auto merge of #9567 - ecoal95:general-pseudo-element-parsing, r=bholl…

…ey,SimonSapin style: Make the whole style crate independent of the implementation This allows, among other things, having different implementations for parsing pseudo{elements, classes} in both `ports/geckolib` and in servo.  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9567)

bors-servo · 2016-02-13T23:17:35Z

💔 Test failed - linux-rel

emilio · 2016-02-13T23:29:18Z

@bors-servo: retry

(wow, so many intermittents hit this time)

bors-servo · 2016-02-13T23:29:19Z

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

bors-servo · 2016-02-14T00:17:04Z

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

highfive added the S-awaiting-review label Feb 8, 2016

emilio mentioned this pull request Feb 8, 2016

Add "before" and "after" atoms. servo/string-cache#141

Closed

emilio reviewed Feb 8, 2016
View changes

emilio force-pushed the emilio:general-pseudo-element-parsing branch from a86125d to 754b767 Feb 8, 2016

SimonSapin reviewed Feb 8, 2016
View changes

SimonSapin mentioned this pull request Feb 9, 2016

Add details pseudo-elements. servo/rust-selectors#67

Closed

notriddle mentioned this pull request Feb 9, 2016

Details ui #9586

Merged

emilio added a commit to emilio/rust-selectors that referenced this pull request Feb 11, 2016

Add Eq and Hash bounds to PseudoElement

7c8f7be

As discussed in IRC while discussing servo/servo#9567

emilio added a commit to emilio/rust-selectors that referenced this pull request Feb 11, 2016

Add Eq and Hash bounds to PseudoElement

3cd12d1

As discussed in IRC while discussing servo/servo#9567

emilio mentioned this pull request Feb 11, 2016

Add Eq and Hash bounds to PseudoElement servo/rust-selectors#69

Merged

emilio added a commit to emilio/rust-selectors that referenced this pull request Feb 11, 2016

Add Eq and Hash bounds to PseudoElement

2622ebd

As discussed in IRC while discussing servo/servo#9567

emilio force-pushed the emilio:general-pseudo-element-parsing branch from 3a82d4b to a99959c Feb 11, 2016

emilio changed the title ~~style: Atomize pseudo-elements to allow custom pseudo-elements~~ style: Make the whole style crate independent of the implementation Feb 11, 2016

emilio force-pushed the emilio:general-pseudo-element-parsing branch from 1012b26 to 545e884 Feb 11, 2016

highfive assigned SimonSapin Feb 11, 2016

SimonSapin reviewed Feb 11, 2016
View changes

emilio force-pushed the emilio:general-pseudo-element-parsing branch 2 times, most recently from b5e564a to 1f7fc58 Feb 13, 2016

highfive added the S-needs-rebase label Feb 13, 2016

emilio force-pushed the emilio:general-pseudo-element-parsing branch from 1f7fc58 to db7dafd Feb 13, 2016

emilio added 6 commits Feb 8, 2016

style: Use fnv hashing for pseudo-elements

eddec5c

geckolib: Fix :link and :any-link pseudo-element matching

dad1738

The logic was switched between them.

style: Prevent a crash when push_applicable_declarations is called wi…

a604d60

…th a non eagerly-cascaded pseudo-element This actually didn't happen, but this should be done sooner than later.

style: Refactor the per_pseudo map from StyleData to avoid having an …

61e04df

…option value type. This make the layout code way clearer.

emilio force-pushed the emilio:general-pseudo-element-parsing branch from db7dafd to 04d2db5 Feb 13, 2016

emilio removed the S-needs-rebase label Feb 13, 2016

highfive removed the S-awaiting-review label Feb 13, 2016

highfive added the S-awaiting-merge label Feb 13, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 13, 2016

bors-servo merged commit 04d2db5 into servo:master Feb 14, 2016
2 checks passed

2 checks passed

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

homu Test successful
Details

servo / servo

style: Make the whole style crate independent of the implementation #9567

style: Make the whole style crate independent of the implementation #9567

emilio commented Feb 8, 2016

highfive commented Feb 8, 2016

highfive commented Feb 8, 2016

This comment has been minimized.

emilio commented Feb 8, 2016

emilio commented Feb 8, 2016

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.

Ms2ger commented Feb 8, 2016

Ms2ger commented Feb 8, 2016

SimonSapin commented Feb 8, 2016

emilio commented Feb 10, 2016

emilio commented Feb 11, 2016

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 Feb 12, 2016

emilio commented Feb 13, 2016

bors-servo commented Feb 13, 2016

bholley commented Feb 13, 2016

bholley commented Feb 13, 2016

emilio commented Feb 13, 2016

bors-servo commented Feb 13, 2016

bors-servo commented Feb 13, 2016

bors-servo commented Feb 13, 2016

emilio commented Feb 13, 2016

bors-servo commented Feb 13, 2016

bors-servo commented Feb 14, 2016

servo / servo

Join GitHub today

style: Make the whole style crate independent of the implementation #9567

style: Make the whole style crate independent of the implementation #9567

Conversation

emilio commented Feb 8, 2016

highfive commented Feb 8, 2016

highfive commented Feb 8, 2016

This comment has been minimized.

emilio commented Feb 8, 2016

emilio commented Feb 8, 2016

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.

Ms2ger commented Feb 8, 2016

Ms2ger commented Feb 8, 2016

SimonSapin commented Feb 8, 2016

emilio commented Feb 10, 2016

emilio commented Feb 11, 2016

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 Feb 12, 2016

emilio commented Feb 13, 2016

bors-servo commented Feb 13, 2016

bholley commented Feb 13, 2016

bholley commented Feb 13, 2016

emilio commented Feb 13, 2016

bors-servo commented Feb 13, 2016

bors-servo commented Feb 13, 2016

bors-servo commented Feb 13, 2016

emilio commented Feb 13, 2016

bors-servo commented Feb 13, 2016

bors-servo commented Feb 14, 2016