1346693 - stylo: Handle namespaces during parsing

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P2)

Description

[Manish Goregaokar [:manishearth]]

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

Comment 1

[Simon Sapin (:SimonSapin)]

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.

Comment 2

[Manish Goregaokar [:manishearth]]

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

Comment 3

[Simon Sapin (:SimonSapin)]

Never mind that part then, I must have grepped incorrectly.

Comment 4

[Manish Goregaokar [:manishearth]]

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.

Comment 5

[Manish Goregaokar [:manishearth]]

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 6

[Manish Goregaokar [:manishearth]]

Attachment: fixup! Bug 1346693 - Part 1: stylo: Move reference of namespace map to parser context;

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

Comment 7

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1346693 - Part 2: stylo: Use namespace ids for content: attr(..);

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

Comment 8

[Cameron McCormack (:heycam)]

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

[Cameron McCormack (:heycam)]

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?

Comment 10

[Manish Goregaokar [:manishearth]]

Nah, we need it for matching attr selectors too IIRC.

Comment 11

[Manish Goregaokar [:manishearth]]

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

Comment 12

[Cameron McCormack (:heycam)]

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!

Comment 13

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1346693 - Part 3: stylo: Create separate Attr type;

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

Comment 14

[Manish Goregaokar [:manishearth]]

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

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

Comment 15

[Manish Goregaokar [:manishearth]]

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

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

Comment 16

[Manish Goregaokar [:manishearth]]

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

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

Comment 17

[Manish Goregaokar [:manishearth]]

Comment on attachment 8873353 [details]
Bug 1346693 - Part 3: stylo: Create separate Attr type;

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

Comment 18

[Manish Goregaokar [:manishearth]]

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

[Cameron McCormack (:heycam)]

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.

Comment 20

[Manish Goregaokar [:manishearth]]

Comment on attachment 8873353 [details]
Bug 1346693 - Part 3: stylo: Create separate Attr type;

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

Comment 21

[Manish Goregaokar [:manishearth]]

https://github.com/servo/servo/pull/17130

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13e2d926f0e6
Part 2: stylo: Use namespace ids for content: attr(..); r=heycam

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/13e2d926f0e6