Remove nsCSSPseudoClasses and friends

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: njn, Assigned: xidorn)

Tracking

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(7 attachments)

(Reporter)

Description

a year ago
Now that the old style system is gone, CSSPseudoClassAtoms is barely used. Let's remove it.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Depends on: 1449097
(Assignee)

Comment 2

a year ago
I think we can remove more stuff than that. Stealing.
Assignee: n.nethercote → xidorn+moz
(Assignee)

Updated

a year ago
Depends on: stylo-everywhere
Summary: Remove CSSPseudoClassAtoms → Remove nsCSSPseudoClasses and friends
(Assignee)

Comment 3

a year ago
mozreview-review
Comment on attachment 8962609 [details]
Bug 1449089 - Remove CSSPseudoClassAtoms.

https://reviewboard.mozilla.org/r/231448/#review237002
Attachment #8962609 - Flags: review?(xidorn+moz)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

a year ago
mozreview-review
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 :)
Attachment #8962677 - Flags: review?(emilio) → review+

Comment 10

a year ago
mozreview-review
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
Attachment #8962678 - Flags: review?(emilio) → review+

Comment 11

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

https://reviewboard.mozilla.org/r/231528/#review237028
Attachment #8962679 - Flags: review?(emilio) → review+

Comment 12

a year ago
mozreview-review
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().
Attachment #8962680 - Flags: review?(emilio) → review+

Comment 13

a year ago
mozreview-review
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!
Attachment #8962681 - Flags: review?(emilio) → review+
(Assignee)

Comment 14

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 17

a year ago
The removal of nsCSSPseudoClasses.cpp makes my life easier elsewhere, by removing the prefs associated with pseudo-classes. Excellent!
(Assignee)

Comment 18

a year ago
Posted file Servo PR

Comment 19

a year ago
Pushed by xquan@mozilla.com:
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
Depends on: 1449900
You need to log in before you can comment on or make changes to this bug.