style: :dir() pseudo class now represented by enum #19195

wilsoniya · 2017-11-12T23:34:27Z

:dir() pseudo class param now represented as enum variants.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #16840
There are tests for these changes

This change is

highfive · 2017-11-12T23:34:34Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @mbrubeck (or someone else) soon.

highfive · 2017-11-12T23:34:34Z

Heads up! This PR modifies the following files:

@bholley: components/selectors/size_of_tests.rs, components/style/invalidation/element/invalidation_map.rs, components/style/gecko/selector_parser.rs, components/style/invalidation/element/element_wrapper.rs, components/style/gecko/wrapper.rs and 2 more
@canaltinova: components/style/invalidation/element/invalidation_map.rs, components/style/gecko/selector_parser.rs, components/style/invalidation/element/element_wrapper.rs, components/style/gecko/wrapper.rs, components/style/gecko/non_ts_pseudo_class_list.rs
@emilio: components/style/invalidation/element/invalidation_map.rs, components/style/gecko/selector_parser.rs, components/style/invalidation/element/element_wrapper.rs, components/style/gecko/wrapper.rs, components/style/gecko/non_ts_pseudo_class_list.rs

highfive · 2017-11-12T23:34:36Z

Warning

These commits modify unsafe code. Please review it carefully!

emilio

Good start!

I have a bunch of comments, let me know if any of those is unclear and I'm happy to clarify or help out.

Thanks for working on this!

emilio · 2017-11-13T00:28:50Z

components/selectors/gecko_like_types.rs

+pub enum Direction {
+    Rtl,
+    Ltr,
+    Other(String),


This should still be Box<[u16]>, or Box<str> at least. That may avoid Component growing.

emilio · 2017-11-13T00:28:50Z

components/selectors/size_of_tests.rs

@@ -14,8 +14,8 @@ use visitor::SelectorVisitor;
 size_of_test!(size_of_selector, Selector<Impl>, 8);
 size_of_test!(size_of_pseudo_element, gecko_like_types::PseudoElement, 24);

-size_of_test!(size_of_component, Component<Impl>, 32);
-size_of_test!(size_of_pseudo_class, PseudoClass, 24);
+size_of_test!(size_of_component, Component<Impl>, 48);


Growing Component is not really acceptable I think :(

emilio · 2017-11-13T00:28:50Z

components/style/gecko/selector_parser.rs

+    /// left-to-right semantic directionality
+    Ltr,
+    /// Some other provided directionality value
+    Other(String),


emilio · 2017-11-13T00:28:50Z

components/style/gecko/selector_parser.rs

+    Other(String),
+}
+
+impl <'a> From<&'a str> for Direction {


No need for From<&'a str>, let's use the Parse trait.

emilio · 2017-11-13T00:28:50Z

components/style/gecko/selector_parser.rs

+
+impl <'a> From<&'a str> for Direction {
+    fn from(string: &'a str) -> Direction {
+        match string.to_lowercase().as_str() {


No need to allocate here, let's use match_ignore_ascii_case.

emilio · 2017-11-13T00:28:50Z

components/style/gecko/selector_parser.rs

@@ -360,6 +417,10 @@ impl<'a, 'i> ::selectors::Parser<'i> for SelectorParser<'a> {
                            .chain(Some(0u16)).collect();
                        NonTSPseudoClass::$k_name(utf16.into_boxed_slice())
                    }, )*
+                    "dir" => {
+                        let name: &str = parser.expect_ident()?;


Let's add the match here instead of the From impl.

emilio · 2017-11-13T00:28:50Z

components/style/gecko/wrapper.rs

@@ -2066,6 +2065,15 @@ impl<'le> ::selectors::Element for GeckoElement<'le> {
                    )
                }
            }
+            NonTSPseudoClass::Dir(ref dir) => {
+                unsafe {
+                    Gecko_MatchStringArgPseudo(


The point of converting to an enum is not having to go through Gecko_MatchStringArgPseudo.

Instead of that, we can just do:

match *dir { Direction::Ltr => self.get_state().intersects(element_state::IN_HAS_DIR_ATTR_LTR_STATE), Direction::Rtl => self.get_state().intersects(element_state::IN_HAS_DIR_ATTR_RTL_STATE), Direction::Other(..) => false, }

emilio · 2017-11-13T00:28:50Z

components/style/invalidation/element/element_wrapper.rs

                use invalidation::element::invalidation_map::dir_selector_to_state;
-                let selector_flag = dir_selector_to_state(s);


Similarly, we can simplify dir_selector_to_state to take &Direction, instead of comparing the string.

That should make the Box<[u16]> impl unnecessary.

emilio · 2017-11-13T00:28:50Z

tests/unit/stylo/dir_repr.rs

@@ -0,0 +1,31 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public


No need for this, we have tests in mozilla-central for this :)

wilsoniya · 2017-11-14T05:41:45Z

components/selectors/size_of_tests.rs

@@ -14,8 +14,8 @@ use visitor::SelectorVisitor;
 size_of_test!(size_of_selector, Selector<Impl>, 8);
 size_of_test!(size_of_pseudo_element, gecko_like_types::PseudoElement, 24);

-size_of_test!(size_of_component, Component<Impl>, 32);
-size_of_test!(size_of_pseudo_class, PseudoClass, 24);
+size_of_test!(size_of_component, Component<Impl>, 40);


Even using the Other<Box[u16]> enum variant type, the size of these structs is increased :(

Hmm, that's pretty unfortunate, let's box the Direction then:

Dir(Box<Direction>)

Mentioning that it allows not growing Component, even though it's slightly unfortunate.

Oh, the other thing we can do instead, and it'll fix the serialization bug I just noted below too, I think, is using CustomIdent, which should just be one word and allow us not to box it:

enum Direction { Rtl, Ltr, Other(CustomIdent), }

Then you don't even need to manually write the serialization code, and you can just #[derive(ToCss)] on it.

wilsoniya · 2017-11-14T05:44:38Z

Hey, @emilio, thanks for the review! I think I've addressed your comments, with the exception of the change in size of Component.

Do you have ideas regarding how to eliminate the size increase on that struct?

emilio · 2017-11-14T13:01:26Z

components/style/selector_parser.rs

+    Other(Box<[u16]>),
+}
+
+impl<'a> From<&'a Direction> for String {


Given we don't use the utf16 impl anyway, this could be Box<str>, and there'd be no need to allocate a String for serialization.

emilio · 2017-11-14T13:01:26Z

components/selectors/size_of_tests.rs

@@ -14,8 +14,8 @@ use visitor::SelectorVisitor;
 size_of_test!(size_of_selector, Selector<Impl>, 8);
 size_of_test!(size_of_pseudo_element, gecko_like_types::PseudoElement, 24);

-size_of_test!(size_of_component, Component<Impl>, 32);
-size_of_test!(size_of_pseudo_class, PseudoClass, 24);
+size_of_test!(size_of_component, Component<Impl>, 40);


Hmm, that's pretty unfortunate, let's box the Direction then:

Dir(Box<Direction>)

Mentioning that it allows not growing Component, even though it's slightly unfortunate.

emilio · 2017-11-14T13:01:26Z

components/style/gecko/selector_parser.rs

@@ -92,6 +94,12 @@ impl ToCss for NonTSPseudoClass {
                        dest.write_str(&value)?;
                        return dest.write_char(')')
                    }, )*
+                    NonTSPseudoClass::Dir(ref dir) => {


Let's impl ToCss for Direction, and use that here, doing just:

dest.write_str(":dir(")?; dir.to_css(dest)?; dest.write_char(')')

emilio · 2017-11-14T13:01:26Z

components/style/gecko/selector_parser.rs

+                    NonTSPseudoClass::Dir(ref dir) => {
+                        let value: String = dir.into();
+                        dest.write_str(concat!(":dir("))?;
+                        dest.write_str(&value)?;


Also, I believe this is not correct, and should do escaping for the identifier... Though it's an existing bug.

emilio · 2017-11-14T13:01:26Z

components/selectors/size_of_tests.rs

@@ -14,8 +14,8 @@ use visitor::SelectorVisitor;
 size_of_test!(size_of_selector, Selector<Impl>, 8);
 size_of_test!(size_of_pseudo_element, gecko_like_types::PseudoElement, 24);

-size_of_test!(size_of_component, Component<Impl>, 32);
-size_of_test!(size_of_pseudo_class, PseudoClass, 24);
+size_of_test!(size_of_component, Component<Impl>, 40);


Oh, the other thing we can do instead, and it'll fix the serialization bug I just noted below too, I think, is using CustomIdent, which should just be one word and allow us not to box it:

enum Direction { Rtl, Ltr, Other(CustomIdent), }

Then you don't even need to manually write the serialization code, and you can just #[derive(ToCss)] on it.

wilsoniya · 2017-11-15T07:11:43Z

@emilio Thanks for the follow-up. Regarding your comment regarding escaping: do you have a link to an issue about existing escaping shortcomings and/or thoughts on how to proceed?

Your attention and patience on this PR is much appreciated! At this point it probably would have been faster for you to implement a fix yourself than to explain all these things to me :)

emilio

Looks great!

r=me with these two nits and the commits squashed. Thanks a lot for working on this!

emilio · 2017-11-15T12:02:05Z

components/style/gecko/selector_parser.rs

@@ -92,6 +94,11 @@ impl ToCss for NonTSPseudoClass {
                        dest.write_str(&value)?;
                        return dest.write_char(')')
                    }, )*
+                    NonTSPseudoClass::Dir(ref dir) => {
+                        dest.write_str(concat!(":dir("))?;


Why is there a need for concat! here?

emilio · 2017-11-15T12:02:05Z

components/style/selector_parser.rs

+        let dir_str = match *self {
+            Direction::Rtl => "rtl",
+            Direction::Ltr => "ltr",
+            Direction::Other(ref other) => other,


Please add a FIXME here saying:

FIXME: This should be escaped as an identifier.

That can be a followup if you want, since that's a bugfix, requires tests and all that.

The short version is that it should use serialize_identifier, in order to properly handle escapes. I believe we have tests that test this where you could grab an example... But looking at what serialize_identifier handles, something like this:

:dir(\-) { .. }

Should be a valid rule, but serializes back to:

:dir(-) { .. }

Which isn't an identifier token IIRC.

Created #19231 which I can pick up as a followup.


        style: :dir() pseudo class now represented by enum

Fixes #19195

wilsoniya · 2017-11-21T01:38:15Z

@emilio are there any other changes you'd like to see before this is mergeable?

emilio · 2017-11-21T08:33:59Z

components/style/gecko/selector_parser.rs

@@ -92,6 +94,12 @@ impl ToCss for NonTSPseudoClass {
                        dest.write_str(&value)?;
                        return dest.write_char(')')
                    }, )*
+                    NonTSPseudoClass::Dir(ref dir) => {
+                        dest.write_str(":dir(")?;
+                        // FIXME: This should be escaped as an identifier; see #19231


This comment should probably go in the to_css impl, but that can be done in a followup PR.

emilio · 2017-11-21T08:34:04Z

Nope, sorry! I had missed the latest update. Feel free to ping me sooner if this happens again.

@bors-servo r+

bors-servo · 2017-11-21T08:34:05Z

📌 Commit 10f3ef4 has been approved by emilio

emilio · 2017-11-21T08:34:13Z

Looks great, thanks a lot!

bors-servo · 2017-11-21T08:56:50Z

⌛ Testing commit 10f3ef4 with merge 0062027...


        Auto merge of #19195 - wilsoniya:issue-16840, r=emilio

style: :dir() pseudo class now represented by enum `:dir()` pseudo class param now represented as enum variants. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #16840 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19195)

bors-servo · 2017-11-21T10:17:41Z

☀️ 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: emilio
Pushing 0062027 to master...


        Auto merge of #19332 - wilsoniya:moving-fixme, r=emilio

moving :dir() param serialization FIXME  Per emilio's comment, #19195 (comment), a `FIXME` regarding `:dir()` parameter serialization has been moved. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes do not require tests because they only modify comment lines    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19332)


        servo: Merge #19332 - moving :dir() param serialization FIXME (from w…

…ilsoniya:moving-fixme); r=emilio  Per emilio's comment, servo/servo#19195 (comment), a `FIXME` regarding `:dir()` parameter serialization has been moved. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes do not require tests because they only modify comment lines   Source-Repo: https://github.com/servo/servo Source-Revision: 3ecd0174cc7817cdd2007850c29b8b069a845b46 --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : bd40b06fc49360f0a8ac206558734ec557c741cc


        servo: Merge #19332 - moving :dir() param serialization FIXME (from w…

…ilsoniya:moving-fixme); r=emilio  Per emilio's comment, servo/servo#19195 (comment), a `FIXME` regarding `:dir()` parameter serialization has been moved. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes do not require tests because they only modify comment lines   Source-Repo: https://github.com/servo/servo Source-Revision: 3ecd0174cc7817cdd2007850c29b8b069a845b46


        servo: Merge #19332 - moving :dir() param serialization FIXME (from w…

…ilsoniya:moving-fixme); r=emilio  Per emilio's comment, servo/servo#19195 (comment), a `FIXME` regarding `:dir()` parameter serialization has been moved. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes do not require tests because they only modify comment lines   Source-Repo: https://github.com/servo/servo Source-Revision: 3ecd0174cc7817cdd2007850c29b8b069a845b46


        servo: Merge #19332 - moving :dir() param serialization FIXME (from w…

…ilsoniya:moving-fixme); r=emilio  Per emilio's comment, servo/servo#19195 (comment), a `FIXME` regarding `:dir()` parameter serialization has been moved. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes do not require tests because they only modify comment lines   Source-Repo: https://github.com/servo/servo Source-Revision: 3ecd0174cc7817cdd2007850c29b8b069a845b46


        servo: Merge #19332 - moving :dir() param serialization FIXME (from w…

…ilsoniya:moving-fixme); r=emilio  Per emilio's comment, servo/servo#19195 (comment), a `FIXME` regarding `:dir()` parameter serialization has been moved. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes do not require tests because they only modify comment lines   Source-Repo: https://github.com/servo/servo Source-Revision: 3ecd0174cc7817cdd2007850c29b8b069a845b46 UltraBlame original commit: 84b90d871c2c14c24356f2840b5bbe9413a5e729


        servo: Merge #19332 - moving :dir() param serialization FIXME (from w…

…ilsoniya:moving-fixme); r=emilio  Per emilio's comment, servo/servo#19195 (comment), a `FIXME` regarding `:dir()` parameter serialization has been moved. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes do not require tests because they only modify comment lines   Source-Repo: https://github.com/servo/servo Source-Revision: 3ecd0174cc7817cdd2007850c29b8b069a845b46 UltraBlame original commit: 84b90d871c2c14c24356f2840b5bbe9413a5e729


        servo: Merge #19332 - moving :dir() param serialization FIXME (from w…

…ilsoniya:moving-fixme); r=emilio  Per emilio's comment, servo/servo#19195 (comment), a `FIXME` regarding `:dir()` parameter serialization has been moved. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes do not require tests because they only modify comment lines   Source-Repo: https://github.com/servo/servo Source-Revision: 3ecd0174cc7817cdd2007850c29b8b069a845b46 UltraBlame original commit: 84b90d871c2c14c24356f2840b5bbe9413a5e729

highfive assigned mbrubeck Nov 12, 2017

highfive added the S-awaiting-review label Nov 12, 2017

emilio requested changes Nov 13, 2017

View changes

wilsoniya reviewed Nov 14, 2017

View changes

emilio reviewed Nov 14, 2017

View changes

emilio approved these changes Nov 15, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Nov 15, 2017

jdm assigned emilio and unassigned mbrubeck Nov 15, 2017

wilsoniya mentioned this pull request Nov 15, 2017

style: parameter to :dir() css pseudo class need escaping on serialization #19231

Open

style: :dir() pseudo class now represented by enum

Loading status checks…

10f3ef4

Fixes #19195

wilsoniya force-pushed the wilsoniya:issue-16840 branch from d0b0a6b to 10f3ef4 Nov 16, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 16, 2017

emilio reviewed Nov 21, 2017

View changes

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 21, 2017

bors-servo merged commit 10f3ef4 into servo:master Nov 21, 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 Nov 21, 2017

wilsoniya mentioned this pull request Nov 22, 2017

moving :dir() param serialization FIXME #19332

Merged

3 of 3 tasks complete

wilsoniya deleted the wilsoniya:issue-16840 branch Nov 23, 2017

servo / servo

style: :dir() pseudo class now represented by enum #19195

style: :dir() pseudo class now represented by enum #19195

wilsoniya commented Nov 12, 2017 •

edited by SimonSapin

highfive commented Nov 12, 2017

highfive commented Nov 12, 2017

highfive commented Nov 12, 2017

emilio left a comment

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

wilsoniya commented Nov 14, 2017

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.

wilsoniya commented Nov 15, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

wilsoniya commented Nov 21, 2017

This comment has been minimized.

emilio commented Nov 21, 2017

bors-servo commented Nov 21, 2017

emilio commented Nov 21, 2017

bors-servo commented Nov 21, 2017

bors-servo commented Nov 21, 2017

		use invalidation::element::invalidation_map::dir_selector_to_state;
		let selector_flag = dir_selector_to_state(s);

		@@ -0,0 +1,31 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

servo / servo

Join GitHub today

style: :dir() pseudo class now represented by enum #19195

style: :dir() pseudo class now represented by enum #19195

Conversation

wilsoniya commented Nov 12, 2017 • edited by SimonSapin

highfive commented Nov 12, 2017

highfive commented Nov 12, 2017

highfive commented Nov 12, 2017

emilio left a comment

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

wilsoniya commented Nov 14, 2017

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.

wilsoniya commented Nov 15, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

wilsoniya commented Nov 21, 2017

This comment has been minimized.

emilio commented Nov 21, 2017

bors-servo commented Nov 21, 2017

emilio commented Nov 21, 2017

bors-servo commented Nov 21, 2017

bors-servo commented Nov 21, 2017

wilsoniya commented Nov 12, 2017 •

edited by SimonSapin