In bug 1354876, I did give up implement all sub properties of font-variant.

This causes significant number of failures in test_value_storage.html.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d18cf978656247160e37df8144eead283324516

Comment on attachment 8874431 [details] Bug 1356134 - stylo: support font-variant shorthand. https://reviewboard.mozilla.org/r/145796/#review149974 ::: servo/components/style/properties/shorthand/font.mako.rs:245 (Diff revision 1) > + % if product == "gecko" or data.testing: > + let mut alternates = None; > + let mut east_asian = None; > + let mut ligatures = None; > + let mut numeric = None; > + let mut position = None; > + % endif This, and code below looks very redundant. Probably you can do something like: ```rust <% subprops = ["caps"] if product == "gecko" or data.testing: subprops += "alternates east_asian ligatures numeric position".split() %> % for prop in subprops: let mut ${prop} = None; % endfor // ... % for prop in subprops: if ${prop}.is_none() { if let Ok(value) = input.try(|i| font_${prop}::parse(context, i)) { ${prop} = Some(value); continue } } % endfor ``` oh, actually, you can reuse `gecko_sub_properties` somehow. Probably rename it to `sub_properties` and use it directly everywhere. ::: servo/components/style/properties/shorthand/font.mako.rs:303 (Diff revision 1) > + let nb_non_normals = > + count(&caps) + count(&alternates) + count(&east_asian) + > + count(&ligatures) + count(&numeric) + count(&position); > + let parsed_count = nb_non_normals + nb_normals; > + if parsed_count == 0 || parsed_count > 6 { > + return Err(()) > + } > + // 'normal' can not be used with any non-normal values. > + // The only exception would be 'none', since this would set > + // font_variant_ligatures to 'none', and reset rest of the > + // font_variant_* longhands to 'normal'. > + if (nb_normals == 1 && nb_non_normals > 0) || > + (nb_normals > 1 && nb_normals < 5) || > + (nb_normals == 5 && count(&ligatures) == 0) { > + return Err(()) > + } This seems to be unnecessarily complicated. Given the grammar, I suggest you just try parsing `normal` and `none` at the beginning of the function, and if that fails, reject the value whenever you see it again at the end of the loop. ::: servo/components/style/properties/shorthand/font.mako.rs:350 (Diff revision 1) > return Ok(()); > } > % endfor > % endif > > self.font_variant_caps.to_css(dest)?; You need to implement the serialization here. The basic logic is: * if all subprops are normal, then normal, otherwise * if lingatures is none, and others are normal, then none, otherwise * if any of them is system font or lingatures is none, then empty * otherwise serialize each non-normal value.

Comment on attachment 8874434 [details] Bug 1356134 - stylo: update test expectations for font-variant shorthand. https://reviewboard.mozilla.org/r/145808/#review149994 ::: layout/style/test/stylo-failures.md:68 (Diff revision 1) > * test_initial_computation.html `grid` [4] > * test_property_syntax_errors.html `grid`: actually there are issues with this [8] > * test_value_storage.html `'grid` [195] > * Unimplemented CSS properties: > * font-variant shorthand bug 1356134 > - * test_value_storage.html `'font-variant'` [65] > + * test_value_storage.html `'font-variant'` [37] I would expect this item to be completely removed, probably with some failures fall into `font-variant-alternates`'s bucket. Please remove this and see what's remaining.

Comment on attachment 8874431 [details] Bug 1356134 - stylo: support font-variant shorthand. https://reviewboard.mozilla.org/r/145796/#review149974 > This, and code below looks very redundant. Probably you can do something like: > ```rust > <% > subprops = ["caps"] > if product == "gecko" or data.testing: > subprops += "alternates east_asian ligatures numeric position".split() > %> > % for prop in subprops: > let mut ${prop} = None; > % endfor > // ... > % for prop in subprops: > if ${prop}.is_none() { > if let Ok(value) = input.try(|i| font_${prop}::parse(context, i)) { > ${prop} = Some(value); > continue > } > } > % endfor > ``` > > oh, actually, you can reuse `gecko_sub_properties` somehow. Probably rename it to `sub_properties` and use it directly everywhere. Yes, will do. > This seems to be unnecessarily complicated. > > Given the grammar, I suggest you just try parsing `normal` and `none` at the beginning of the function, and if that fails, reject the value whenever you see it again at the end of the loop. Will update the implementation in the next version. > You need to implement the serialization here. The basic logic is: > * if all subprops are normal, then normal, otherwise > * if lingatures is none, and others are normal, then none, otherwise > * if any of them is system font or lingatures is none, then empty > * otherwise serialize each non-normal value. Will do.

Comment on attachment 8874434 [details] Bug 1356134 - stylo: update test expectations for font-variant shorthand. https://reviewboard.mozilla.org/r/145808/#review149994 > I would expect this item to be completely removed, probably with some failures fall into `font-variant-alternates`'s bucket. > > Please remove this and see what's remaining. Okay, will check if we can remove all of these, or move the rest to `font-variant-alternates`'s bucket.

Hmm... just figure that we need a way to tell whether we're parsing an invalid value for the current longhand, or a valid value for another longhand. For example, "font-variant: traditional oldstyle-nums;" should be valid since 'traditional' is valid for font-variant-east-asian property and 'oldstyle-nums' is valid for font-variant-numeric property. However, while parsing 'traditional' for font-variant-east-asian longhand, we might treat 'oldstyle-nums' as an invalid second value, which in fact is an valid value for font-variant-numeric longhand. I guess we might need a way to recover from the Err(()) state returned from each longhand parser, so we could keep parsing the rest of the longhands for font-variant shorthand.

I think, in general, parse function of property value should just parse as much as it can. And as far as it parses something, it should return Ok(()) and leave the Parser stop before the next thing it cannot parse. It's caller is responsible to handle the rest of return error if it requires parsing entirely. That said, I think the parse function of font-variant-east-asian is wrong. You should just fix that function to break from the loop instead of returning Err(()) when hitting an invalid value.

It seems many of the font-variant subprops have the same problem.

> It's caller is responsible to handle the rest of return error if it requires parsing entirely. It is caller's responsiblity to handle the rest of input, and return error if it requires parsing entirely.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #14) > I think, in general, parse function of property value should just parse as > much as it can. And as far as it parses something, it should return Ok(()) > and leave the Parser stop before the next thing it cannot parse. It's caller > is responsible to handle the rest of return error if it requires parsing > entirely. > > That said, I think the parse function of font-variant-east-asian is wrong. > You should just fix that function to break from the loop instead of > returning Err(()) when hitting an invalid value. Agree! Thanks for the feedback!!! (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #15) > It seems many of the font-variant subprops have the same problem. Yes, I'll fix them as well.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=161221ca02a5 I updated the test expectations based on my local, so we may or may not need some tweaks. We may need to confirm that the fix for font-variant subprops parser doesn't break anything. Let's see how it goes on try first.

try looks fine except for 3 wierd reftest fails: https://treeherder.mozilla.org/#/jobs?repo=try&revision=928edc29ee9e34fbfb3568580564f542bc04ceab All 3 of them do not have font-variant related css properties, and I can't reproduce any of them on my mac. :/ Maybe I shall push a more clean try and see if they still remain. At the meantime, I think these patches are ready for review.

No idea why I can't re-request review on ReviewBoard.... set from bugzilla then...

Comment on attachment 8875981 [details] Bug 1356134 - stylo: make font-variant-* longhands parsing more tolerant. https://reviewboard.mozilla.org/r/147378/#review152096 ::: servo/components/style/properties/longhand/font.mako.rs:1329 (Diff revision 3) > > if input.try(|input| input.expect_ident_matching("normal")).is_ok() { > return Ok(SpecifiedValue::Value(result)) > } > > + let mut pos = input.position(); This is not necessary. `input.try` is supposed to restore the position when parsing fails inside.

Comment on attachment 8874431 [details] Bug 1356134 - stylo: support font-variant shorthand. https://reviewboard.mozilla.org/r/145796/#review152098 ::: servo/components/style/properties/shorthand/font.mako.rs:288 (Diff revision 6) > + > break > } > - #[inline] > - fn count<T>(opt: &Option<T>) -> u8 { > - if opt.is_some() { 1 } else { 0 } > + > + % if product == "gecko" or data.testing: > + let count = nb_normals + nb_nones + nb_non_normals; This is still wrong. The syntax of `font-variant` is `normal | none | [ ... ]` [1] which means `font-variant: normal` is valid, while `font-variant: normal ruby` or `font-variant: ruby normal` is invalid. My suggestion in comment 5 was parsing `normal` and `none` at the beginning of the **function** and reject it in the loop. See CSS Values spec [2] for the meaning of the syntax grammar. [1] https://drafts.csswg.org/css-fonts-3/#font-variant-prop [2] https://drafts.csswg.org/css-values-3/#component-combinators

Comment on attachment 8874431 [details] Bug 1356134 - stylo: support font-variant shorthand. https://reviewboard.mozilla.org/r/145796/#review152100 ::: servo/components/style/properties/shorthand/font.mako.rs:253 (Diff revision 6) > if input.try(|input| input.expect_ident_matching("normal")).is_ok() { > + if nb_non_normals + nb_normals + nb_nones > 0 { > + return Err(()) > + } > + > nb_normals += 1; > - continue; > + continue > } > - if caps.is_none() { > - if let Ok(value) = input.try(|input| font_variant_caps::parse(context, input)) { > - caps = Some(value); > + if input.try(|input| input.expect_ident_matching("none")).is_ok() { > + if nb_non_normals + nb_normals + nb_nones > 0 { > + return Err(()) > + } > + > + nb_nones += 1; > - continue > + continue > - } > + } Oh, okay, so you check it here... which is unnecessarily complicated and confusing. If `normal` and `none` can only appear once, please make them `bool` rather than number. Also the first time when you see `normal` or `none`, you should be able to break from the loop, because that should immediately lead to either a success parsing which sets all subprops to a normal/none value, or a failure that normal/none value is combined with other values before. The caller of this function is responsible to return Err if there is any leftovers after we return.

Comment on attachment 8874434 [details] Bug 1356134 - stylo: update test expectations for font-variant shorthand. https://reviewboard.mozilla.org/r/145808/#review152102 ::: layout/style/test/stylo-failures.md:67 (Diff revision 6) > * test_initial_computation.html `grid` [4] > * test_property_syntax_errors.html `grid`: actually there are issues with this [8] > * test_value_storage.html `'grid` [195] > * Unimplemented CSS properties: > * font-variant shorthand bug 1356134 > - * test_value_storage.html `'font-variant'` [65] > + * test_value_storage.html `'font-variant'` [14] If you simply remove this line, would the remaining 14 failures fall into the one for `font-variant-alternates` directly? If yes, please do so. Otherwise, what is remaining?

Comment on attachment 8875981 [details] Bug 1356134 - stylo: make font-variant-* longhands parsing more tolerant. https://reviewboard.mozilla.org/r/147378/#review152096 > This is not necessary. `input.try` is supposed to restore the position when parsing fails inside. Well, but we're changing the `Err(())` with `break`, which won't let input.try to restore to the right position. The code here is try to make what is invalid for the current longhand can still have a chance to be parsed for other longhands.

Comment on attachment 8874434 [details] Bug 1356134 - stylo: update test expectations for font-variant shorthand. https://reviewboard.mozilla.org/r/145808/#review152102 > If you simply remove this line, would the remaining 14 failures fall into the one for `font-variant-alternates` directly? If yes, please do so. Otherwise, what is remaining? The rest 14 failures are introduced by setting `font-variant-alternates` in `fon-variant`. We have two cases for this, "traditional historical-forms styleset(ok-alt-a, ok-alt-b)" and "styleset(potato)". The parsing error caused by `font-variant-alternates` sub property would lead to an empty serialization result for `font-variant`, and causes 7 fails: 1 for font-variant shorthand, and 6 for each of the sub properties. http://searchfox.org/mozilla-central/rev/12afa8cc9cea4a141494f767541b411845eef7a0/layout/style/test/property_database.js#3072-3074 Looks like we have to keep all 14 of them in this section, maybe just point to the same bug as `font-variant-alternates` support?

Comment on attachment 8875981 [details] Bug 1356134 - stylo: make font-variant-* longhands parsing more tolerant. https://reviewboard.mozilla.org/r/147378/#review152096 > Well, but we're changing the `Err(())` with `break`, which won't let input.try to restore to the right position. The code here is try to make what is invalid for the current longhand can still have a chance to be parsed for other longhands. Hmmm, you're right. Then it is probably better to wrap everything inside a `input.try`, like ```rust while let Ok(flag) = input.try(|input| { match_ignore_ascii_case! { &input.expect_ident()?, "stylistic" => Ok(STYLISTIC), // ... _ => Err(()), } }) { // do something with flag } ``` or ```rust while let Ok(flag) = input.try(|input| { Ok(match_ignore_ascii_case! { &input.expect_ident()?, "stylistic" => STYLISTIC, // ... _ => return Err(()), }) }) { // do something with flag } ```

Comment on attachment 8874431 [details] Bug 1356134 - stylo: support font-variant shorthand. https://reviewboard.mozilla.org/r/145796/#review152100 > Oh, okay, so you check it here... which is unnecessarily complicated and confusing. > > If `normal` and `none` can only appear once, please make them `bool` rather than number. > > Also the first time when you see `normal` or `none`, you should be able to break from the loop, because that should immediately lead to either a success parsing which sets all subprops to a normal/none value, or a failure that normal/none value is combined with other values before. > > The caller of this function is responsible to return Err if there is any leftovers after we return. Aha, you're totally right. We do have something like `input.expect_exhausted()?` in the caller to check if there's any leftovers. I'll fix this part in the next version.

Comment on attachment 8874434 [details] Bug 1356134 - stylo: update test expectations for font-variant shorthand. https://reviewboard.mozilla.org/r/145808/#review152102 > The rest 14 failures are introduced by setting `font-variant-alternates` in `fon-variant`. We have two cases for this, "traditional historical-forms styleset(ok-alt-a, ok-alt-b)" and "styleset(potato)". The parsing error caused by `font-variant-alternates` sub property would lead to an empty serialization result for `font-variant`, and causes 7 fails: 1 for font-variant shorthand, and 6 for each of the sub properties. > > http://searchfox.org/mozilla-central/rev/12afa8cc9cea4a141494f767541b411845eef7a0/layout/style/test/property_database.js#3072-3074 > > Looks like we have to keep all 14 of them in this section, maybe just point to the same bug as `font-variant-alternates` support? I mean, do the failure messages also mention `font-variant-alternates`? If so, when you remove this line, those failues should be captured by the rule for `font-variant-alternates` below, so you don't need a separate line for it. (Just increase the number of that line by 14.)

Comment on attachment 8874434 [details] Bug 1356134 - stylo: update test expectations for font-variant shorthand. https://reviewboard.mozilla.org/r/145808/#review152102 > I mean, do the failure messages also mention `font-variant-alternates`? If so, when you remove this line, those failues should be captured by the rule for `font-variant-alternates` below, so you don't need a separate line for it. (Just increase the number of that line by 14.) The 14 failures are listed in below. I suppose we do the failure-pattern matching from the syntax of setting something on `[PROPERTY_NAME]`, so they all seem to be categorized into `font-variant`. ``` 149131 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'traditional historical-forms styleset(ok-alt-a, ok-alt-b)' on 'font-variant' - didn't expect "", but got it 149132 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'traditional historical-forms styleset(ok-alt-a, ok-alt-b)' on 'font-variant' (for 'font-variant-alternates') - didn't expect "", but got it 149133 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'traditional historical-forms styleset(ok-alt-a, ok-alt-b)' on 'font-variant' (for 'font-variant-caps') - didn't expect "", but got it 149134 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'traditional historical-forms styleset(ok-alt-a, ok-alt-b)' on 'font-variant' (for 'font-variant-east-asian') - didn't expect "", but got it 149135 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'traditional historical-forms styleset(ok-alt-a, ok-alt-b)' on 'font-variant' (for 'font-variant-ligatures') - didn't expect "", but got it 149136 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'traditional historical-forms styleset(ok-alt-a, ok-alt-b)' on 'font-variant' (for 'font-variant-numeric') - didn't expect "", but got it 149137 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'traditional historical-forms styleset(ok-alt-a, ok-alt-b)' on 'font-variant' (for 'font-variant-position') - didn't expect "", but got it 149138 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'styleset(potato)' on 'font-variant' - didn't expect "", but got it 149139 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'styleset(potato)' on 'font-variant' (for 'font-variant-alternates') - didn't expect "", but got it 149140 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'styleset(potato)' on 'font-variant' (for 'font-variant-caps') - didn't expect "", but got it 149141 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'styleset(potato)' on 'font-variant' (for 'font-variant-east-asian') - didn't expect "", but got it 149142 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'styleset(potato)' on 'font-variant' (for 'font-variant-ligatures') - didn't expect "", but got it 149143 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'styleset(potato)' on 'font-variant' (for 'font-variant-numeric') - didn't expect "", but got it 149144 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'styleset(potato)' on 'font-variant' (for 'font-variant-position') - didn't expect "", but got it ```

Comment on attachment 8874434 [details] Bug 1356134 - stylo: update test expectations for font-variant shorthand. https://reviewboard.mozilla.org/r/145808/#review152102 > The 14 failures are listed in below. I suppose we do the failure-pattern matching from the syntax of setting something on `[PROPERTY_NAME]`, so they all seem to be categorized into `font-variant`. > > ``` > 149131 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'traditional historical-forms styleset(ok-alt-a, ok-alt-b)' on 'font-variant' - didn't expect "", but got it > > 149132 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'traditional historical-forms styleset(ok-alt-a, ok-alt-b)' on 'font-variant' (for 'font-variant-alternates') - didn't expect "", but got it > > 149133 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'traditional historical-forms styleset(ok-alt-a, ok-alt-b)' on 'font-variant' (for 'font-variant-caps') - didn't expect "", but got it > > 149134 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'traditional historical-forms styleset(ok-alt-a, ok-alt-b)' on 'font-variant' (for 'font-variant-east-asian') - didn't expect "", but got it > > 149135 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'traditional historical-forms styleset(ok-alt-a, ok-alt-b)' on 'font-variant' (for 'font-variant-ligatures') - didn't expect "", but got it > > 149136 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'traditional historical-forms styleset(ok-alt-a, ok-alt-b)' on 'font-variant' (for 'font-variant-numeric') - didn't expect "", but got it > > 149137 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'traditional historical-forms styleset(ok-alt-a, ok-alt-b)' on 'font-variant' (for 'font-variant-position') - didn't expect "", but got it > > 149138 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'styleset(potato)' on 'font-variant' - didn't expect "", but got it > > 149139 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'styleset(potato)' on 'font-variant' (for 'font-variant-alternates') - didn't expect "", but got it > > 149140 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'styleset(potato)' on 'font-variant' (for 'font-variant-caps') - didn't expect "", but got it > > 149141 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'styleset(potato)' on 'font-variant' (for 'font-variant-east-asian') - didn't expect "", but got it > > 149142 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'styleset(potato)' on 'font-variant' (for 'font-variant-ligatures') - didn't expect "", but got it > > 149143 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'styleset(potato)' on 'font-variant' (for 'font-variant-numeric') - didn't expect "", but got it > > 149144 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'styleset(potato)' on 'font-variant' (for 'font-variant-position') - didn't expect "", but got it > ``` Hmmm, okay, then probably move this line down to the section of "font-variant-alternates".

Comment on attachment 8874434 [details] Bug 1356134 - stylo: update test expectations for font-variant shorthand. https://reviewboard.mozilla.org/r/145808/#review152130

try looks fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=204198dec3ce444ba79ac7231db37ec1d7301d53

Comment on attachment 8875981 [details] Bug 1356134 - stylo: make font-variant-* longhands parsing more tolerant. https://reviewboard.mozilla.org/r/147378/#review152192

Comment on attachment 8874431 [details] Bug 1356134 - stylo: support font-variant shorthand. https://reviewboard.mozilla.org/r/145796/#review152194 ::: servo/components/style/properties/longhand/font.mako.rs:1653 (Diff revision 7) > + } > /// normal | none | Probably consider adding an empty line here to separate the two lines. ::: servo/components/style/properties/shorthand/font.mako.rs:250 (Diff revision 7) > - // all sub properties. > + // normal > - // Leaves the values to None, 'normal' is the initial value for each of them. > - if input.try(|input| input.expect_ident_matching("normal")).is_ok() { > + if input.try(|input| input.expect_ident_matching("normal")).is_ok() { > - nb_normals += 1; > - continue; > + return Ok(expanded! { > + % for prop in sub_properties: > + font_variant_${prop}: unwrap_or_initial!(font_variant_${prop}, ${prop}), > + % endfor > + }) > - } > + } > - if caps.is_none() { > - if let Ok(value) = input.try(|input| font_variant_caps::parse(context, input)) { > - caps = Some(value); > + > + // none > + if input.try(|input| input.expect_ident_matching("none")).is_ok() { > + ligatures = Some(font_variant_ligatures::get_none_specified_value()); > + return Ok(expanded! { > + % for prop in sub_properties: > + font_variant_${prop}: unwrap_or_initial!(font_variant_${prop}, ${prop}), > + % endfor > + }) > + } One suggestion: given you are always using code like ```rust return Ok(expanded! { // ... }) ``` probably you can reuse this part by doing ```rust if input.try(...).is_ok() { // ... } else if input.try(...).is_ok() { // ... } else { loop { // ... } } Ok(expanded! { // ... }) ``` instead. ::: servo/components/style/properties/shorthand/font.mako.rs:288 (Diff revision 7) > - fn count<T>(opt: &Option<T>) -> u8 { > - if opt.is_some() { 1 } else { 0 } > + % if product == "gecko" or data.testing: > + if nb_non_normals == 0 || nb_non_normals > 6 { > + return Err(()) > - } > + } > - let count = count(&caps) + nb_normals; > - if count == 0 || count > 1 { > + % else: > + if nb_non_normals == 0 || nb_non_normals > 1 { > - return Err(()) > + return Err(()) > - } > + } > + % endif Where are the magic number "6" and "1" from? Are they the number of subproperties? Is it really possible that we parse more subproperties then that number? It seems to me that we check `is_none()` before we try parsing a subproperties, and we set the corresponding variable to `Some(...)` when it succeeds, so no variable can be double-assigned, and thus the number would naturally limited to the number of subproperties. So I don't see any reason we need to check that. Actually, using an additional magic number check here may make the code more fragile, since people would need to update these numbers when they are trying to add new subproperties, but nothing enforces such update. What we really need to check here is just that, we did parse something in the loop. For checking that, a bool should be enough. And with that, you would not need to differentiate gecko and servo either. ::: servo/components/style/properties/shorthand/font.mako.rs:309 (Diff revision 7) > + let mut has_none_ligatures = false; > - % if product == "gecko" or data.testing: > + % if product == "gecko" or data.testing: > - % for name in gecko_sub_properties: > - // FIXME: Bug 1356134 - handle all sub properties. > + has_none_ligatures = self.font_variant_ligatures == &font_variant_ligatures::get_none_specified_value(); > + % endif What about just ```rust let has_none_ligatures = % if produce == "gecko" or data.testing: // ... % else: false; % endif ``` ::: servo/components/style/properties/shorthand/font.mako.rs:315 (Diff revision 7) > - % if product == "gecko" or data.testing: > + % if product == "gecko" or data.testing: > - % for name in gecko_sub_properties: > - // FIXME: Bug 1356134 - handle all sub properties. > - if self.font_variant_${name} != &font_variant_${name}::get_initial_specified_value() { > - return Ok(()); > + has_none_ligatures = self.font_variant_ligatures == &font_variant_ligatures::get_none_specified_value(); > + % endif > + > + let mut nb_normals = 0; > + let mut nb_total = 0; `nb_total` isn't strictly necessary... that should just equal to `${len(sub_properties)}`. Probably you can do ```rust const TOTAL_SUBPROPS: usize = ${len(sub_properties)}; ``` instead. I'm fine either way, though.

Comment on attachment 8874431 [details] Bug 1356134 - stylo: support font-variant shorthand. https://reviewboard.mozilla.org/r/145796/#review152194 > One suggestion: given you are always using code like > ```rust > return Ok(expanded! { > // ... > }) > ``` > probably you can reuse this part by doing > ```rust > if input.try(...).is_ok() { > // ... > } else if input.try(...).is_ok() { > // ... > } else { > loop { > // ... > } > } > Ok(expanded! { > // ... > }) > ``` > instead. Will do. > Where are the magic number "6" and "1" from? Are they the number of subproperties? Is it really possible that we parse more subproperties then that number? > > It seems to me that we check `is_none()` before we try parsing a subproperties, and we set the corresponding variable to `Some(...)` when it succeeds, so no variable can be double-assigned, and thus the number would naturally limited to the number of subproperties. So I don't see any reason we need to check that. > > Actually, using an additional magic number check here may make the code more fragile, since people would need to update these numbers when they are trying to add new subproperties, but nothing enforces such update. > > What we really need to check here is just that, we did parse something in the loop. For checking that, a bool should be enough. And with that, you would not need to differentiate gecko and servo either. Indeed, this check could be simplified with the latest patch. I'll fix it in the next version. > What about just > ```rust > let has_none_ligatures = > % if produce == "gecko" or data.testing: > // ... > % else: > false; > % endif > ``` Will do. > `nb_total` isn't strictly necessary... that should just equal to `${len(sub_properties)}`. Probably you can do > ```rust > const TOTAL_SUBPROPS: usize = ${len(sub_properties)}; > ``` > instead. > > I'm fine either way, though. Cool!!! I guess I am still not that familiar with cross using python with rust...

Comment on attachment 8874431 [details] Bug 1356134 - stylo: support font-variant shorthand. https://reviewboard.mozilla.org/r/145796/#review152344 There are just some nits left :) ::: servo/components/style/properties/shorthand/font.mako.rs:255 (Diff revision 8) > - loop { > - // Special-case 'normal' because it is valid in each of > - // all sub properties. > - // Leaves the values to None, 'normal' is the initial value for each of them. > + % endfor > + > + let mut is_normal_or_none: bool = false; > + let mut has_custom_value: bool = false; > - if input.try(|input| input.expect_ident_matching("normal")).is_ok() { > + if input.try(|input| input.expect_ident_matching("normal")).is_ok() { > - nb_normals += 1; > + // Leave the values to None, 'normal' is the initial value for each of the sub properties. Probably better: > 'normal' is the initial value for **all** the sub properties. ::: servo/components/style/properties/shorthand/font.mako.rs:258 (Diff revision 8) > - // Leaves the values to None, 'normal' is the initial value for each of them. > + let mut has_custom_value: bool = false; > - if input.try(|input| input.expect_ident_matching("normal")).is_ok() { > + if input.try(|input| input.expect_ident_matching("normal")).is_ok() { > - nb_normals += 1; > - continue; > + // Leave the values to None, 'normal' is the initial value for each of the sub properties. > + is_normal_or_none = true; > + } else if input.try(|input| input.expect_ident_matching("none")).is_ok() { > + // The ‘none’ value sets ‘font-variant-ligatures’ to ‘none’ and resets all other sub properties Please just use ASCII quote instead of U+2018 and U+2019... ::: servo/components/style/properties/shorthand/font.mako.rs:266 (Diff revision 8) > + is_normal_or_none = true; > + } else { > + loop { > + if input.try(|input| input.expect_ident_matching("normal")).is_ok() || > + input.try(|input| input.expect_ident_matching("none")).is_ok() { > + return Err(StyleParseError::UnspecifiedError.into()) This seems to be a new thing... I suppose this should really be `PropertyDeclarationParseError::InvalidValue` rather than unspecified value. ::: servo/components/style/properties/shorthand/font.mako.rs:282 (Diff revision 8) > - if count == 0 || count > 1 { > + if !is_normal_or_none && !has_custom_value { > return Err(StyleParseError::UnspecifiedError.into()) > } Why not just check this in the last branch? I believe you would not need `is_normal_or_none` anymore and you can move `has_custom_value` into the last branch that way. ::: servo/components/style/properties/shorthand/font.mako.rs:283 (Diff revision 8) > - fn count<T>(opt: &Option<T>) -> u8 { > - if opt.is_some() { 1 } else { 0 } > } > - let count = count(&caps) + nb_normals; > - if count == 0 || count > 1 { > + > + if !is_normal_or_none && !has_custom_value { > return Err(StyleParseError::UnspecifiedError.into()) This should probably be `InvalidValue` as well.

Comment on attachment 8875981 [details] Bug 1356134 - stylo: make font-variant-* longhands parsing more tolerant. https://reviewboard.mozilla.org/r/147378/#review152346 ::: servo/components/style/properties/longhand/font.mako.rs:1392 (Diff revision 5) > - "annotation" => Some(ANNOTATION), > - _ => None, > - }; > - let flag = match flag { > - Some(flag) if !result.intersects(flag) => flag, > - _ => return Err(SelectorParseError::UnexpectedIdent(ident).into()), > + "annotation" => ANNOTATION, > + _ => return Err(()), > + }) > + }) { > + if result.intersects(flag) { > + return Err(StyleParseError::UnspecifiedError.into()) This should probably be `InvalidValue` as well. ::: servo/components/style/properties/longhand/font.mako.rs:1400 (Diff revision 5) > } > > if !result.is_empty() { > Ok(SpecifiedValue::Value(result)) > } else { > Err(StyleParseError::UnspecifiedError.into()) I would actually suggest this and those in other functions below should be `InvalidValue` as well, but given this is not part of your commit, I'm fine leaving them as is. Probably worth checking with jdm what does he think about error here.

Comment on attachment 8875981 [details] Bug 1356134 - stylo: make font-variant-* longhands parsing more tolerant. https://reviewboard.mozilla.org/r/147378/#review152346 > I would actually suggest this and those in other functions below should be `InvalidValue` as well, but given this is not part of your commit, I'm fine leaving them as is. > > Probably worth checking with jdm what does he think about error here. Ok, I'm going to leave them as is in this bug, and try to land the shorthand support first. Filed Bug 1372266 for the further discussion.

Comment on attachment 8874431 [details] Bug 1356134 - stylo: support font-variant shorthand. https://reviewboard.mozilla.org/r/145796/#review152344 > Probably better: > > 'normal' is the initial value for **all** the sub properties. Fixed. > Please just use ASCII quote instead of U+2018 and U+2019... Fixed. > Why not just check this in the last branch? I believe you would not need `is_normal_or_none` anymore and you can move `has_custom_value` into the last branch that way. Yes, you're right! Will do.

Oops, just noticed that I pushed the latest version to try, but forgot to push them to reviewboard....:(

Comment on attachment 8874431 [details] Bug 1356134 - stylo: support font-variant shorthand. https://reviewboard.mozilla.org/r/145796/#review153262

servo PR: https://github.com/servo/servo/pull/17310

servo PR merged!!! https://hg.mozilla.org/integration/autoland/rev/23bf1cea14d7

Pushed by jichen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/edcb0dad14b6 stylo: update test expectations for font-variant shorthand. r=xidorn