Separate the parsing part to here.

Comment on attachment 8868976 [details] Bug 1365900 - (wip) add initial style system support for @font-feature-falues rule. The most different part of @font-feature-values rule is that it may contain nested @rules blocks, such like: @font-feature-values font1 { @swash { ident1: 2; ident2: 4; } } I tried to reuse the existing parsing codes for 1-level @rule parser and wrote this patch. Since I'm not very familiar with Rust and macro_rules stuff, so I think it would be better to gather some feedback before going further. Xidorn, could you take a look and give me some feedback? Do you think this patch is toward the right direction? If you're not comfortable with giving feedback on this pure Rust patch, please feel free to pass the request to Emilio or Simon, thank you.

Comment on attachment 8868976 [details] Bug 1365900 - (wip) add initial style system support for @font-feature-falues rule. I myself don't write lots of parser stuff. Most of parser bits of my previous work were written by Simon, so I redirect f? to him.

Comment on attachment 8868976 [details] Bug 1365900 - (wip) add initial style system support for @font-feature-falues rule. @font-feature-values is different enough from @font-face that I don’t think it makes sense to try to re-use the macros. I recommend doing this without macros at first, even if that means duplicating some code. Once it is clear where duplication exist consider reducing it first with (possibly generic) functions, and then (if necessary) with macros. Now, a good starting point for new code is data structures. Once you have the correct data structures, the code around it tends to be easier to shape. For CSS stuff data structures are dictated by two things: what input syntax has to be accepted/rejected by the parser, and how it is accessed through CSSOM. Syntax of CSS things is often specified with a grammar. In this case it is https://drafts.csswg.org/css-fonts/#basic-syntax, but feature_value_definition in particular is more general than what is really accepted: it defines one or more integers, but then https://drafts.csswg.org/css-fonts/#multi-valued-feature-value-definitions specified that depending on the feature type the value may be limited to one integers or a pair of integers. The WebIDL interface at https://drafts.csswg.org/css-fonts/#om-fontfeaturevalues show that CSSOM always has a (string -> sequence of integers) map for each feature type, even if that map is empty. Now, you have: pub struct FontFeatureValuesRuleData { pub family_name: Option<FamilyName>, pub swash: Option<FontFeatureRuleData>, // TODO: [ @stylistic, @styleset, @character-variant, @ornaments, @annotation ] } pub struct FontFeatureRuleData { name: String, features: Vec<FeatureValuePair>, } pub struct FeatureValuePair { pub name: Option<String>, pub value: Option<i32>, } This doesn’t look right. Option<FamilyName> encodes zero or one family name, but font_family_name_list in the grammar is `font_family_name [ S* ',' S* font_family_name ]*` which is is one or more family names. So Option<FamilyName> should be replaced Vec<FamilyName>. (Vec is "zero or more" rather than "one or more", but it’s often not worth enforcing the "at least one" part with the type system.) CSSOM always has a "swash" attribute in CSSFontFeatureValuesRule, which is a map, even it is empty. So Option<FontFeatureRuleData> should not be optional. FontFeatureRuleData contains a name string, which seems redundant with the name of the "swash" field which is know statically. It seems like that name doesn’t need to be stored in memory at all. This leaves only a Vec inside FontFeatureRuleData, so we probably can remove FontFeatureRuleData entirely and store the Vec directly. ("FontFeatureRuleData" isn’t a great name anyway.) I assume that FeatureValuePair is named after "'name: value' pair". In other CSS contexts we call these declarations, so this type could be FontFeatureValueDeclaration. Its name is optional, which doesn’t match the feature_value_definition grammar or the CSSFontFeatureValuesMap WebIDL interface. Atom should be used instead of String for faster string comparison. This makes the field 'pub name: Atom,' The spec says "Values associated with a given identifier are limited to integer values 0 or greater.", so i32 should be u32. Option encodes "zero or one" value but the feature_value_definition grammar defines "one or more". But as mentioned before the number of integers actually depends on the feature type. I think it would be nice encode that in the type system. All in all, here is a data structure that I think matches the spec better: pub struct FontFeatureValuesRuleData { pub family_names: Vec<FamilyName>, pub swash: Vec<(Atom, u32)>, pub stylistic Vec<(Atom, u32)>, pub ornaments: Vec<(Atom, u32)>, pub annotation: Vec<(Atom, u32)>, pub character_variant: Vec<(Atom, u32, Option<u32>)>, // One or two integers pub styleset: Vec<(Atom, Vec<u32>)>, // One or more integers } (Tuples can be replaced with structs with named fields, if desired.)

(In reply to Simon Sapin (:SimonSapin) from comment #9) > > All in all, here is a data structure that I think matches the spec better: > > pub struct FontFeatureValuesRuleData { > pub family_names: Vec<FamilyName>, > pub swash: Vec<(Atom, u32)>, > pub stylistic Vec<(Atom, u32)>, > pub ornaments: Vec<(Atom, u32)>, > pub annotation: Vec<(Atom, u32)>, > pub character_variant: Vec<(Atom, u32, Option<u32>)>, // One or two > integers > pub styleset: Vec<(Atom, Vec<u32>)>, // One or more integers > } > > (Tuples can be replaced with structs with named fields, if desired.) Simon, very appreciate your kind feedback and guidance!! I'll rework based on this data structure.

By the way, instead of Vec<(Atom, …)> we’ll probably eventually want some kind of hash map to make lookup by name faster in CSSOM and layout. But this might be in the C++ side depending on how Gecko integration is done, so it’s fine to use Vec for now and revisit later.

Per discussed with Nazım offline, he might be able to take a look at this while taking Bug 1356124. They're not highly related though, but the parsing/data storage might be similar I think. Un-assign myself, and cc Nazım.

Comment on attachment 8868976 [details] Bug 1365900 - (wip) add initial style system support for @font-feature-falues rule. I think this patch is not helpful for going forward, obsolete this.

Yeah, I'll work on this next.

Comment on attachment 8890235 [details] Bug 1365900 - Implement parsing/serialization for @font-feature-values rule https://reviewboard.mozilla.org/r/161348/#review166748 ::: servo/components/style/stylesheets/font_feature_values_rule.rs:26 (Diff revision 2) > +/// A @font-feature-values block declaration. > +/// It is `<custom-ident>: <integer>+`. > +/// This struct can take 3 value types. > +/// - `SingleValue` is to keep just one unsigned integer value. > +/// - `PairValues` is to keep one or two unsigned integer values. > +/// - `VectorValues` is to keep a list of unsigned integer vaslues. typo: vaslues ::: servo/components/style/stylesheets/font_feature_values_rule.rs:170 (Diff revision 2) > + > + fn parse_value<'t>(&mut self, name: CompactCowStr<'i>, input: &mut Parser<'i, 't>) > + -> Result<(), ParseError<'i>> { > + let value = input.parse_entirely(|i| T::parse(self.context, i))?; > + self.declarations.push(FFVDeclaration { > + name: CustomIdent::from_ident(name, &[""])?, I think using CustomIdent is not appropriate here. The spec does not refer to `<custom-ident>`. Please change the the field type to Atom and use `Atom::from(&*name)` here. ::: servo/components/style/stylesheets/font_feature_values_rule.rs:339 (Diff revision 2) > + let error = ContextualParseError::UnsupportedKeyframePropertyDeclaration( > + iter.input.slice(err.span), err.error); > + log_css_error(iter.input, pos, error, &context); > + } > + } > + self.rule.$ident.append(&mut iter.parser.declarations); This should deduplicate potential multiple declarations for the same identifier: https://drafts.csswg.org/css-fonts/#basic-syntax > If the same identifier is defined mulitple times for a given feature type and font family, the last defined value is used.

Thanks for the review Simon! Addressed your comments.

Comment on attachment 8890235 [details] Bug 1365900 - Implement parsing/serialization for @font-feature-values rule https://reviewboard.mozilla.org/r/161348/#review166976 ::: servo/components/style/stylesheets/font_feature_values_rule.rs:339 (Diff revisions 1 - 3) > let error = ContextualParseError::UnsupportedKeyframePropertyDeclaration( > iter.input.slice(err.span), err.error); > log_css_error(iter.input, pos, error, &context); > } > } > - self.rule.$ident.append(&mut iter.parser.declarations); > + let _ = mem::replace(&mut self.rule.$ident, iter.parser.declarations); Individual declarations should be de-duplicated, not entire `Vec`s. For example, this: @swash { foo: 1; bar: 2; foo: 3; } @swash { bar: 4; } Needs to parse into: @swash { foo: 3; bar: 4; }

Comment on attachment 8890235 [details] Bug 1365900 - Implement parsing/serialization for @font-feature-values rule https://reviewboard.mozilla.org/r/161348/#review166976 > Individual declarations should be de-duplicated, not entire `Vec`s. For example, this: > > @swash { > foo: 1; > bar: 2; > foo: 3; > } > @swash { > bar: 4; > } > > Needs to parse into: > > @swash { > foo: 3; > bar: 4; > } Oh, thanks for clarification. I thought we had to overwrite all @swash declaration list. Converted Vec into a HasMap instead and added a test for it. That should work.

Comment on attachment 8890235 [details] Bug 1365900 - Implement parsing/serialization for @font-feature-values rule https://reviewboard.mozilla.org/r/161348/#review167050 ::: servo/components/style/stylesheets/font_feature_values_rule.rs:24 (Diff revisions 1 - 4) > use stylesheets::CssRuleType; > -use values::CustomIdent; > > -/// A @font-feature-values block declaration. > -/// It is `<custom-ident>: <integer>+`. > -/// This struct can take 3 value types. > +/// A list of @font-feature-values block declarations. > +/// One declaration is `<custom-ident>: <integer>+`. > +/// Key is a `<custom-ident>` for declaration name. This comment still refers to `<custom-ident>`. ::: servo/components/style/stylesheets/font_feature_values_rule.rs:330 (Diff revisions 1 - 4) > match prelude { > $( > BlockType::$ident_camel => { > - let parser = FFVDeclarationParser { > + let parser = FFVDeclarationsParser { > context: &context, > - declarations: vec![] as Vec<FFVDeclaration<$ty>>, > + declarations: FFVDeclarations(HashMap::new() as HashMap<Atom, $ty>), Are these intermediate hash maps necessary? Couldn’t FFVDeclarationsParser contains a mutable reference to the rule’s hash map instead? (This would also make the `append` method unnecessary.) ::: servo/components/style/stylesheets/font_feature_values_rule.rs:330 (Diff revisions 1 - 4) > match prelude { > $( > BlockType::$ident_camel => { > - let parser = FFVDeclarationParser { > + let parser = FFVDeclarationsParser { > context: &context, > - declarations: vec![] as Vec<FFVDeclaration<$ty>>, > + declarations: FFVDeclarations(HashMap::new() as HashMap<Atom, $ty>), `as` looks like a conversion. If specifying the type is necessary, consider `HashMap::<Atom, $ty>::new()`

Comment on attachment 8890235 [details] Bug 1365900 - Implement parsing/serialization for @font-feature-values rule https://reviewboard.mozilla.org/r/161348/#review167072

Comment on attachment 8890236 [details] Bug 1365900 - Extract CSSFontFeatureValuesRule base class https://reviewboard.mozilla.org/r/161350/#review167084 ::: layout/style/PostTraversalTask.h:10 (Diff revision 4) > +#include "nsError.h" > + Why is this?

Comment on attachment 8890236 [details] Bug 1365900 - Extract CSSFontFeatureValuesRule base class https://reviewboard.mozilla.org/r/161350/#review167086 ::: layout/style/CSSFontFeatureValuesRule.h:25 (Diff revision 5) > + , public nsIDOMCSSFontFeatureValuesRule > +{ > +public: > + NS_DECL_ISUPPORTS_INHERITED > + > + virtual bool IsCCLeaf() const override = 0; I believe you can move this method from `nsFontFeatureValuesRule` to this class as well.

Comment on attachment 8890236 [details] Bug 1365900 - Extract CSSFontFeatureValuesRule base class https://reviewboard.mozilla.org/r/161350/#review167084 > Why is this? Oh, after adding new files to UNIFIED_SOURCES, this file started to give error about `nsresult`. I guess this file started to unify with other files during compilation and `nsError.h` was not imported there.

Comment on attachment 8890237 [details] Bug 1365900 - Create ServoFontFeatureValuesRule and bind servo data https://reviewboard.mozilla.org/r/161352/#review167090

Comment on attachment 8890235 [details] Bug 1365900 - Implement parsing/serialization for @font-feature-values rule I had to change back to the Vec from HashMap since it doesn't hold the order and makes tests fail :( Also I had to add a new function named `update_or_push` to overwrite old value. Could you review again Simon?

Comment on attachment 8890235 [details] Bug 1365900 - Implement parsing/serialization for @font-feature-values rule https://reviewboard.mozilla.org/r/161348/#review167232

Servo side is https://github.com/servo/servo/pull/17894

Pushed by canaltinova@gmail.com: https://hg.mozilla.org/integration/autoland/rev/b5451544fbee Extract CSSFontFeatureValuesRule base class r=xidorn https://hg.mozilla.org/integration/autoland/rev/63810d7a059c Create ServoFontFeatureValuesRule and bind servo data r=xidorn

Pushed by canaltinova@gmail.com: https://hg.mozilla.org/integration/autoland/rev/43b15ca14bb5 stylo: Update test expectations for @font-feature-values r=me

Pushed by canaltinova@gmail.com: https://hg.mozilla.org/integration/autoland/rev/8331e94d228a stylo: Update more test expectations for @font-feature-values r=me

Nazım, thank you for working on this. :)

No problem! :)

https://hg.mozilla.org/mozilla-central/rev/b5451544fbee https://hg.mozilla.org/mozilla-central/rev/63810d7a059c https://hg.mozilla.org/mozilla-central/rev/43b15ca14bb5 https://hg.mozilla.org/mozilla-central/rev/8331e94d228a