stylo: Handle namespaces during parsing

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: manishearth, Assigned: manishearth)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

4 months ago
We should parse namespaces into a namespace map and use it when parsing things like `attr()` (introduced in bug 1296477)
(Assignee)

Updated

4 months ago
Blocks: 1243581, 1296477
Priority: -- → P2
attr() is not supported yet as far as I can tell. Nevertheless I looked into adding a &stylesheet::Namespaces field to style::parser::ParserContext. However I hit two difficulties:

* When parsing a value in CSSOM (for example setPropertyValue), the namespace map of the stylesheet should be used. In the Stylo/Gecko case, this requires passing a reference to it (or to the stylesheet) across the FFI boundary

* The value of declarations that contain var() functions is parsed during cascading, 
once the var() functions have been substituted.
  - I opened https://github.com/w3c/csswg-drafts/issues/1163 about which stylesheet’s namespace map to use
  - properties::UnparsedValue cannot have lifetime parameters, so the namespace map would need to be in an Arc. 
    Which in turn would probably need something like RwLock to synchronize mutation…

I decided not to shave these yaks today.
(Assignee)

Comment 2

4 months ago
> attr() is not supported yet as far as I can tell.

Sure we do : https://hg.mozilla.org/mozilla-central/rev/fe16f2642746#l6.48 . Only for `content:` though.
Never mind that part then, I must have grepped incorrectly.
(Assignee)

Comment 4

2 months ago
This affects layout/reftests/bugs/416106-1.xhtml

We may need to pass around a pointer to the namespace map in the parser context, which may get icky.
Blocks: 1324348
(Assignee)

Comment 5

2 months ago
Ah, this is tricky.

So when parsing namespace rules, we need to register the namespace in via the namespace manager, as done in https://searchfox.org/mozilla-central/source/dom/base/nsXMLNameSpaceMap.cpp#73. We should store the resultant id in the namespace map.

When parsing, we verify that the ns is in the map and then use its id if so.

This is pretty straightforward, the problem is that the namespace map is already borrowed mutably by the TopLevelRuleParser.

I can instead shove an &RWLock into the ParserContext and try it that way. Unsure.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
These patches don't fix the (pre-existing) problem of e.g. sheet.insertRule("a|b { ... }", 0) not using the namespace map the sheet got when it was initially parsed.  If there's not a bug on file for that, can you file one?

Comment 9

2 months ago
mozreview-review
Comment on attachment 8873258 [details]
fixup! Bug 1346693 - Part 1: stylo: Move reference of namespace map to parser context;

https://reviewboard.mozilla.org/r/144716/#review148680

::: servo/components/style/parser.rs:85
(Diff revision 1)
> +    /// The list of all namespaces active in the current stylesheet
> +    pub namespaces: Option<&'a RwLock<Namespaces>>,

Is it true that we just need the namespace map at parsing time, or do we ever need to look it up during selector matching or value computation?  If it's just during parsing, should we use a RefCell instead of RwLock?
Attachment #8873258 - Flags: review?(cam) → review+
(Assignee)

Comment 10

2 months ago
Nah, we need it for matching attr selectors too IIRC.
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
(Assignee)

Comment 11

2 months ago
I'll try to fix that as well (tomorrow), if it's straightforward, else file a bug.

Comment 12

2 months ago
mozreview-review
Comment on attachment 8873259 [details]
Bug 1346693 - Part 2: stylo: Use namespace ids for content: attr(..);

https://reviewboard.mozilla.org/r/144718/#review148684

::: layout/style/ServoBindings.h:550
(Diff revision 1)
>  
>  void Gecko_AddPropertyToSet(nsCSSPropertyIDSetBorrowedMut, nsCSSPropertyID);
>  
> +// Register a namespace and get a namespace id.
> +// Returns -1 on error (OOM)
> +int32_t Gecko_RegisterNamespace(nsIAtom* aNamespace);

Nit: I guess this should be "namespace" rather than "aNamespace", as we tend to use Rust-style identifiers for arguments in ServoBindings.h.

::: servo/components/style/properties/longhand/counters.mako.rs:223
(Diff revision 1)
>                                          Token::Delim('|') => {
>                                              // must be followed by an ident
>                                              let tok2 = input.next_including_whitespace()?;
>                                              if let Token::Ident(second) = tok2 {
> -                                                return Ok(ContentItem::Attr(first, second.into_owned()))
> +                                                let first: Option<Namespace> = first.map(|i| i.into());
> +                                                let first_with_id = match (first, context.namespaces) {

The nesting in here. :o  Please consider breaking attr() parsing out into a separate function.

::: servo/components/style/stylesheets.rs:94
(Diff revision 1)
>  /// A set of namespaces applying to a given stylesheet.
> +///
> +/// The u32 is the namespace id, used in gecko
>  #[derive(Clone, Default, Debug)]
>  #[allow(missing_docs)]
>  pub struct Namespaces {
> -    pub default: Option<Namespace>,
> -    pub prefixes: FnvHashMap<Prefix , Namespace>,
> +    pub default: Option<(Namespace, u32)>,
> +    pub prefixes: FnvHashMap<Prefix, (Namespace, u32)>,
>  }

We should probably try harder not to store the u32 for Servo.  Though I'm happy for that to be done later.

(Also, namespace IDs in Gecko are nearly always stored in int32_ts, so probably this should be i32.)

::: servo/components/style/stylesheets.rs:1335
(Diff revision 1)
>  
> +
> +

Nit: Three blank lines seems extravagant!
Attachment #8873259 - Flags: review?(cam) → review+
(Assignee)

Updated

2 months ago
Depends on: 1369329
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

2 months ago
mozreview-review-reply
Comment on attachment 8873259 [details]
Bug 1346693 - Part 2: stylo: Use namespace ids for content: attr(..);

https://reviewboard.mozilla.org/r/144718/#review148684

> We should probably try harder not to store the u32 for Servo.  Though I'm happy for that to be done later.
> 
> (Also, namespace IDs in Gecko are nearly always stored in int32_ts, so probably this should be i32.)

In gecko it's an i32 because gecko uses -1 to signal failure, we don't need to. Though gecko also uses nsresult there so it doesn't really need to.

Changed to i32 to avoid casts, anyway.

Comment 19

2 months ago
mozreview-review
Comment on attachment 8873353 [details]
Bug 1346693 - Part 3: stylo: Create separate Attr type;

https://reviewboard.mozilla.org/r/144800/#review148720

::: servo/components/style/stylesheets.rs:97
(Diff revision 1)
>      User,
>  }
>  
>  /// A set of namespaces applying to a given stylesheet.
>  ///
>  /// The u32 is the namespace id, used in gecko

Please update this comment.
Attachment #8873353 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 21

2 months ago
https://github.com/servo/servo/pull/17130

Comment 22

2 months ago
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/13e2d926f0e6
Part 2: stylo: Use namespace ids for content: attr(..); r=heycam

Comment 23

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/13e2d926f0e6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.