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 Parse :dir into an enum with two values (ltr/rtl) as opposed to a string for the argument #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:18:43Z

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:18: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:19:04Z

components/style/gecko/selector_parser.rs

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


emilio · 2017-11-13T00:19:38Z

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:19:53Z

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:21:20Z

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:25:06Z

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:26:33Z

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:27:23Z

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-14T12:53:31Z

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-14T12:54:56Z

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-14T12:56:52Z

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-14T12:58:04Z

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-14T12:59:51Z

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-15T11:53:03Z

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-15T11:55:53Z

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.

Fixes servo#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:39Z

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...

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...

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)

…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

…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

…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 There is new code that needs to be reviewed. label Nov 12, 2017

emilio requested changes Nov 13, 2017

View reviewed changes

wilsoniya commented Nov 14, 2017

View reviewed changes

emilio reviewed Nov 14, 2017

View reviewed changes

emilio approved these changes Nov 15, 2017

View reviewed changes

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 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

10f3ef4

Fixes servo#19195

wilsoniya force-pushed the issue-16840 branch from d0b0a6b to 10f3ef4 Compare November 16, 2017 02:13

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 Nov 16, 2017

emilio reviewed Nov 21, 2017

View reviewed changes

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 Nov 21, 2017

bors-servo merged commit 10f3ef4 into servo:master Nov 21, 2017

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

wilsoniya mentioned this pull request Nov 22, 2017

moving :dir() param serialization FIXME #19332

Merged

3 tasks

wilsoniya deleted the issue-16840 branch November 23, 2017 16:21

		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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsoniya commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsoniya commented Nov 15, 2017

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsoniya commented Nov 21, 2017

Choose a reason for hiding this comment

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