1449089 - Remove nsCSSPseudoClasses and friends

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Nicholas Nethercote [inactive]]

Now that the old style system is gone, CSSPseudoClassAtoms is barely used. Let's remove it.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1449089 - Remove CSSPseudoClassAtoms.

With the old style system gone it's no longer needed.

The patch renames nsCSSPseudoClasses::RegisterStaticAtoms() as
InitializePrefs() because that's all that function does now.

Review commit: https://reviewboard.mozilla.org/r/231448/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231448/

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

I think we can remove more stuff than that. Stealing.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8962609 [details]
Bug 1449089 - Remove CSSPseudoClassAtoms.

https://reviewboard.mozilla.org/r/231448/#review237002

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1449089 part 1 - Make :-moz-native-anonymous and :-moz-use-shadow-tree-root matches in Rust code.

:-moz-native-anonymous can just use is_in_native_anonymous_subtree()
which is an existing function duplicating IsInNativeAnonymousSubtree().

IsRootOfUseElementShadowTree() in C++ code only has a single reference
from nsCSSPseudoClasses::MatchesElement() so we can just move it to the
Rust side. And actually Rust code has existing duplicate logic for
blocks_ancestor_combinators().

Review commit: https://reviewboard.mozilla.org/r/231524/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231524/

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1449089 part 2 - Move matching logic of :-moz-table-border-nonzero and :-moz-browser-frame into individual binding functions.

Review commit: https://reviewboard.mozilla.org/r/231526/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231526/

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1449089 part 3 - Remove pref layout.css.scope-pseudo.enabled.

It was never effective in Stylo anyway.

Review commit: https://reviewboard.mozilla.org/r/231528/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231528/

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1449089 part 4 - Move logic of nsCSSPseudoClasses::LangPseudoMatches into Gecko_MatchLang.

Review commit: https://reviewboard.mozilla.org/r/231530/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231530/

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1449089 part 5 - Remove nsCSSPseudoClasses.{h,cpp} and nsCSSPseudoClassList.h.

Review commit: https://reviewboard.mozilla.org/r/231532/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231532/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8962677 [details]
Bug 1449089 part 1 - Make :-moz-native-anonymous and :-moz-use-shadow-tree-root matches in Rust code.

https://reviewboard.mozilla.org/r/231524/#review237024

::: servo/components/style/gecko/wrapper.rs:826
(Diff revision 1)
> +            return false
> +        }
> +        match self.parent_element() {
> +            Some(e) => {
> +                e.local_name() == &*local_name!("use") &&
> +                    e.namespace() == &*ns!("http://www.w3.org/2000/svg")

nit: maybe add an is_svg_element() function that checks the namespace id? Should be a bit faster.

::: servo/components/style/gecko/wrapper.rs:2259
(Diff revision 1)
>  
>      #[inline]
>      fn blocks_ancestor_combinators(&self) -> bool {
> -        if !self.is_root_of_anonymous_subtree() {
> -            return false
> -        }
> +        // If this element is the shadow root of an use-element shadow tree,
> +        // according to the spec, we should not match rules cross the shadow
> +        // DOM boundary.

We'd get this automatically if our `<svg:use>` element actually used Shadow DOM... Oh well :)

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8962678 [details]
Bug 1449089 part 2 - Move matching logic of :-moz-table-border-nonzero and :-moz-browser-frame into individual binding functions.

https://reviewboard.mozilla.org/r/231526/#review237026

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8962679 [details]
Bug 1449089 part 3 - Remove pref layout.css.scope-pseudo.enabled.

https://reviewboard.mozilla.org/r/231528/#review237028

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8962680 [details]
Bug 1449089 part 4 - Move logic of nsCSSPseudoClasses::LangPseudoMatches into Gecko_MatchLang.

https://reviewboard.mozilla.org/r/231530/#review237030

::: layout/style/ServoBindings.cpp:862
(Diff revision 1)
> +    return nsStyleUtil::DashMatchCompare(nsDependentAtomString(language),
> +                                         nsDependentString(aValue),
> +                                         nsASCIICaseInsensitiveStringComparator());
> +  }
>  
> -  return nsCSSPseudoClasses::LangPseudoMatches(
> +  nsIDocument* document = aElement->OwnerDoc();

No need to null-check OwnerDoc().

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8962681 [details]
Bug 1449089 part 5 - Remove nsCSSPseudoClasses.{h,cpp} and nsCSSPseudoClassList.h.

https://reviewboard.mozilla.org/r/231532/#review237034

LGTM, thanks for cleaning this up!

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8962677 [details]
Bug 1449089 part 1 - Make :-moz-native-anonymous and :-moz-use-shadow-tree-root matches in Rust code.

https://reviewboard.mozilla.org/r/231524/#review237024

> nit: maybe add an is_svg_element() function that checks the namespace id? Should be a bit faster.

Well, I actually didn't do this by intention.

Gecko has IsSVGElement and IsHTMLElement etc. accepting atom for the local name. It was done intentionally to avoid comparing local name and namespace separately so that there would be fewer tricky issues, especially security issues.

Servo may want something like that at some point, so I'd rather just leave it as-is for now until we really have other uses of this.

> We'd get this automatically if our `<svg:use>` element actually used Shadow DOM... Oh well :)

I guess it shouldn't be considered an issue here?

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8962680 [details]
Bug 1449089 part 4 - Move logic of nsCSSPseudoClasses::LangPseudoMatches into Gecko_MatchLang.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231530/diff/1-2/

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8962681 [details]
Bug 1449089 part 5 - Remove nsCSSPseudoClasses.{h,cpp} and nsCSSPseudoClassList.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231532/diff/1-2/

Comment 17

[Nicholas Nethercote [inactive]]

The removal of nsCSSPseudoClasses.cpp makes my life easier elsewhere, by removing the prefs associated with pseudo-classes. Excellent!

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Servo PR

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6545b04dfe5f
part 1 - Make :-moz-native-anonymous and :-moz-use-shadow-tree-root matches in Rust code. r=emilio
https://hg.mozilla.org/integration/autoland/rev/7f17b7282ac7
part 2 - Move matching logic of :-moz-table-border-nonzero and :-moz-browser-frame into individual binding functions. r=emilio
https://hg.mozilla.org/integration/autoland/rev/e7b9d8cd1d2e
part 3 - Remove pref layout.css.scope-pseudo.enabled. r=emilio
https://hg.mozilla.org/integration/autoland/rev/ff8313a58baa
part 4 - Move logic of nsCSSPseudoClasses::LangPseudoMatches into Gecko_MatchLang. r=emilio
https://hg.mozilla.org/integration/autoland/rev/16560f62edad
part 5 - Remove nsCSSPseudoClasses.{h,cpp} and nsCSSPseudoClassList.h. r=emilio

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6545b04dfe5f
https://hg.mozilla.org/mozilla-central/rev/7f17b7282ac7
https://hg.mozilla.org/mozilla-central/rev/e7b9d8cd1d2e
https://hg.mozilla.org/mozilla-central/rev/ff8313a58baa
https://hg.mozilla.org/mozilla-central/rev/16560f62edad