1434130 - stylo: Port InspectorUtils::GetCSSValuesForProperty to be based on information from stylo rather than the old style system

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P2)

Description

[Xidorn Quan [:xidorn] UTC+10]

Many functions in InspectorUtils still relies on data of the old style system, e.g. InspectorUtils::GetCSSValuesForProperty uses the lookup tables and parsing flags in nsCSSProps.

We should generate those information from stylo rather than keeping them using Gecko stuff, that way we should be able to remove the lookup tables in nsCSSProps and probably also the CSS keywords. That should both shrink the code size, and help reduce the maintenance burden.

Comment 1

[Chris Peterson [:cpeterson]]

We will need to implement a new Servo API to provide the property information and then fix Gecko's devtools to use the new API.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

I thought a bit about this, and I think we probably just want to reimplement the logic of InspectorUtils::GetCSSValuesForProperty in Servo side, and the Gecko side change should be trivial that it just needs to invoke the binding function properly.

Comment 3

[Tom Tromey :tromey]

Note there are a few other InspectorUtils methods that need a property database: inheritedProperty, getCSSPropertyNames,
getSubpropertiesForCSSProperty, cssPropertyIsShorthand, and cssPropertySupportsType.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to Tom Tromey :tromey from comment #3)
> Note there are a few other InspectorUtils methods that need a property
> database: inheritedProperty, getCSSPropertyNames,
> getSubpropertiesForCSSProperty, cssPropertyIsShorthand, and
> cssPropertySupportsType.

Can't we use the property database we use for testing somehow? (https://searchfox.org/mozilla-central/source/layout/style/test/property_database.js)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

I guess we'd need to add the type information there, but that may not be a huge issue...

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

I think bholley concerns about code size for this issue, and thus he suggested thia may be tricky to fix. Using property_databaae definitely wouldn't help on code size. But I suppose we only ship inspector on desktop, where code size may not be such a large concern?

Comment 7

[Nathan Froyd [:froydnj]]

If we could manage to ship the property database alongside devtools in the hypothetical future where devtools isn't shipped with the browser, would we care about the codesize of the property database?

Comment 8

[Simon Sapin (:SimonSapin)]

So of this data is already available through existing APIs in the style crate:

* PropertyId::parse(&str) -> Result<Self, ()>
* PropertyId is an enum that can contain LonghandId or ShorthandId
* LonghandId::inherited(&self) -> bool
* ShorthandId::longhands(&self) -> &'static [LonghandId]

Comment 9

[Tom Tromey :tromey]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #6)
> But I suppose we only ship inspector on desktop,
> where code size may not be such a large concern?

Ideally for devtools the data would live on the server side.
The reason for this is that when remote debugging, it's preferable to get
information from the server, so that what the inspector shows reflects
the browser one is inspecting -- not the browser running the inspector.

I say "ideally" because during de-chrome-ification we tried to make this separation
and ran into some issues in the inspector.  In practice we do some queries
client-side, and also ship over a giant database.

Anyway I don't think the exact form of the database matters a whole lot to devtools.
Also I wonder if InspectorUtils::cssPropertySupportsType is removable (perhaps with
a bit of work).

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #6)
> I think bholley concerns about code size for this issue, and thus he
> suggested thia may be tricky to fix. Using property_databaae definitely
> wouldn't help on code size. But I suppose we only ship inspector on desktop,
> where code size may not be such a large concern?

Given GetCSSValuesForProperty is only used to generate the properties database, would this be a codesize issue? We don't need to include that code anyway.

Comment 11

[Bobby Holley (:bholley)]

Maybe we could split up property_database.js into two parts - a small file with the allowable keywords, and a larger file with all the rest of info we want during testing? We could then share that first file with devtools.

Comment 12

[Tom Tromey :tromey]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)

> Given GetCSSValuesForProperty is only used to generate the properties
> database, would this be a codesize issue? We don't need to include that code
> anyway.

I think for devtools it would be better to get rid of the property database.
The idea would be for the inspector to always ask the server about validity
of properties, etc; but due to problems in the inspector we couldn't make this
change during the de-chromeing project, and so we had to settle for the database
instead.  However, doing things this way means that the client's view of what
is valid may disagree with the server's, which is confusing for users.

Comment 13

[Bobby Holley (:bholley)]

There are three distinct issues here:
(1) Storing the data in a file vs embedded in the binary.
(2) Manually curating vs auto-generating.
(3) Devtools using the server's view of the world, rather than the client's.

For (1), a file is preferable because it only needs to get read into memory when devtools are being used, which improves memory usage and startup time. This is not a hard requirement, but worth shooting for if we can.

For (2), auto-generation has obvious advantages. The one counter-point is that we may (or may not) want to maintain an explicit list of supported keywords so that our tests can verify those expectations. If we don't need such a manual list, then teaching Mako to spit out the appropriate file at build time would make sense. If we do need such a list, then we might as well just use it for devtools as well to avoid the build system hackery.

(3) is orthogonal to everything else. Devtools can just decide which side of the fence the question should be asked.

So we basically need to decide whether we want a manual list anyway. If we do, then we should just refactor the properties database to be usable from devtools, and drop the C++ API. If we don't need it, we should investigate how much work it would be to auto-generate the list from Mako. If it's doable, we should do it. If it seems like a ton of work, we should measure the codesize impact of having Mako generate a hardcoded Rust function to answer the question instead.

NI dbaron to make a decision about the manual list.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Note that it's not so easy to do with mako since it doesn't have the rust type info.

Though a rust proc macro would do (probably very similar to how ToCss works, maybe they could share most of the code).

Comment 15

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> Note that it's not so easy to do with mako since it doesn't have the rust
> type info.

Why do we need type info? Mako already knows all the keyword properties (which is all this API needs), so it just needs to spit out a big table.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) from comment #15)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> > Note that it's not so easy to do with mako since it doesn't have the rust
> > type info.
> 
> Why do we need type info? Mako already knows all the keyword properties
> (which is all this API needs), so it just needs to spit out a big table.

It does not. It doesn't know, let's say, overscroll-behavior[1], which is a keyword property. We have tons of them like that, see all the derive(Parse) stuff[2]. This is because they're shared across two properties, for example.

Similarly, it wouldn't know all the "either <keyword> or an <integer>", or stuff like the align-* justify-* properties that are represented as bitflags.

Also, inspector needs to know whether something quacks like a color (see GetColorsForProperty for example), or like other kind of function (see the variant stuff right below that).

[1]: https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/servo/components/style/values/specified/box.rs#348
[2]: https://searchfox.org/mozilla-central/search?q=derive.%2BParse&case=true&regexp=true&path=

Comment 17

[Bobby Holley (:bholley)]

Good points. I guess the complexity involved tilts things more in favor of a manual list.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #6)
> > I think bholley concerns about code size for this issue, and thus he
> > suggested thia may be tricky to fix. Using property_databaae definitely
> > wouldn't help on code size. But I suppose we only ship inspector on desktop,
> > where code size may not be such a large concern?
> 
> Given GetCSSValuesForProperty is only used to generate the properties
> database, would this be a codesize issue? We don't need to include that code
> anyway.

It's true that it is only used to generate the database, but devtools also seems to generate it at runtime, see initCssProperties[1].

Using property_database may not be that bad, if we can remove the existing database we ship, and we can compress it and only decompress when necessary, I guess...


[1] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/devtools/shared/fronts/css-properties.js#241

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1434130 part 1 - Skip system font variant for ToCss in font subproperties.

System font keywords are not a valid value for those properties.

The newly-added #[css(skip)] would be reused by deriving algorithm of
SpecifiedValueInfo to skip them as well.

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

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1434130 part 2 - Derive SpecifiedValueInfo for keywords types.

I don't know why I didn't do that, but it seems it's easy to just have
them derived...

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

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1434130 part 2 - Add collect_values function to SpecifiedValueInfo trait for collecting possible values.

This is the basic structure of the stuff. Following patches will fill
the gap between Gecko and Servo on value generating, and finally hook
it into InspectorUtils.

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

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8970823 [details]
Bug 1434130 part 1 - Skip system font variant for ToCss in font subproperties.

https://reviewboard.mozilla.org/r/239600/#review245268

We should probably tag the `-x-system-font` stuff, etc too, right?

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8970824 [details]
Bug 1434130 part 2 - Derive SpecifiedValueInfo for keywords types.

https://reviewboard.mozilla.org/r/239602/#review245270

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8970825 [details]
Bug 1434130 part 2 - Add collect_values function to SpecifiedValueInfo trait for collecting possible values.

https://reviewboard.mozilla.org/r/239604/#review245266

Looks good, thanks for doing all this work :)

::: servo/components/style/properties/properties.mako.rs:546
(Diff revision 1)
>          }
>  
>          false
>      }
>  
> +    <% len_longhand_shorthand = len(data.longhands) + len(data.shorthands) %>

I'd just inline this in place, even if it duplicates it a bit. It's easier to figure out because the next lines are just `for prop in data.shorthands:` and `for prop in data.longhands:`.

::: servo/components/style/properties/properties.mako.rs:566
(Diff revision 1)
>              % endfor
>          ];
>          SUPPORTED_TYPES[self.0]
>      }
> +
> +    fn collect_property_values(&self, f: &mut FnMut(&[&'static str])) {

Please add docs for what it's used and such.

::: servo/components/style/properties/properties.mako.rs:567
(Diff revision 1)
>          ];
>          SUPPORTED_TYPES[self.0]
>      }
> +
> +    fn collect_property_values(&self, f: &mut FnMut(&[&'static str])) {
> +        const COLLECT_FUNCTIONS: [&Fn(&mut FnMut(&[&'static str])); ${len_longhand_shorthand}] = [

Somewhat surprised this works without `&'static Fn`, nice :).

::: servo/components/style/properties/properties.mako.rs:765
(Diff revision 1)
>      }
>  }
>  
>  /// An enum to represent a CSS Wide keyword.
> -#[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq, ToCss)]
> +#[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq, SpecifiedValueInfo,
> +         ToCss)]

Probably this should derive `Parse` too... Though we'd need to audit callers because of the `expect_exhausted` stuff. Maybe file a bug? I can do it later today if you agree :)

::: servo/components/style/properties/properties.mako.rs:1733
(Diff revision 1)
>              PropertyId::ShorthandAlias(_, alias_id) => alias_id.into(),
>              PropertyId::LonghandAlias(_, alias_id) => alias_id.into(),
>          })
>      }
>  
> +    fn non_custom_non_alias_id(&self) -> Option<NonCustomPropertyId> {

Document? Though probably we could just make entries for aliases too, and have less special-cases...

Not a big deal in any case.

::: servo/components/style_derive/cg.rs:273
(Diff revision 1)
>      let result = &camel_case[..end_position];
>      *camel_case = &camel_case[end_position..];
>      Some(result)
>  }
> +
> +pub fn tokens_all<T, I>(iter: I) -> Tokens

Not super-convinced these are super-useful, but your call :)

::: servo/components/style_derive/specified_value_info.rs:80
(Diff revision 1)
> +            }
> +        }));
> +        (types_value, nested_collects)
> +    };
> +
> +    let append_values = if values.is_empty() {

There's no need to special-case this right? You'd call it with an empty slice but...

Anyway, it doesn't hurt I suppose.

::: servo/components/style_traits/specified_value_info.rs:41
(Diff revision 1)
> +    /// beginning of a value of this type.
> +    ///
> +    /// Caller should pass in a callback function to accept the list of
> +    /// values. The callback function can be called multiple times, and
> +    /// some values passed to the callback may be duplicate.
> +    fn collect_values(_f: &mut FnMut(&[&'static str])) {}

What about just making the function take a `&'static str`? I think it'd be a bit less convoluted.

If you want, you could add a helper here which would just iterate over it, and use it for the mako code or wherever it's more useful.

Also I don't really love the name, since it implies that the strings are values, but they really aren't valid values all the time, just starting words.

Maybe `collect_completion_keywords`? Or `collect_keywords`?

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8970825 [details]
Bug 1434130 part 2 - Add collect_values function to SpecifiedValueInfo trait for collecting possible values.

https://reviewboard.mozilla.org/r/239604/#review245266

> What about just making the function take a `&'static str`? I think it'd be a bit less convoluted.
> 
> If you want, you could add a helper here which would just iterate over it, and use it for the mako code or wherever it's more useful.
> 
> Also I don't really love the name, since it implies that the strings are values, but they really aren't valid values all the time, just starting words.
> 
> Maybe `collect_completion_keywords`? Or `collect_keywords`?

I don't think making it taking a `&'static str` rather than a slice makes much difference... The most annoying bit here IMO is the `'static` part...

I guess `collect_completion_keywords` is probably better, but I don't really like long names :(

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8970825 [details]
Bug 1434130 part 2 - Add collect_values function to SpecifiedValueInfo trait for collecting possible values.

https://reviewboard.mozilla.org/r/239604/#review245266

> Probably this should derive `Parse` too... Though we'd need to audit callers because of the `expect_exhausted` stuff. Maybe file a bug? I can do it later today if you agree :)

Hmmm, the `expect_exhausted` stuff does seem tricky. I would suggest we don't try to derive `Parse` here, I suspect that would complicate more stuff at its callsites.

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1434130 part 3 - Add SequenceWriter::item_str for writing str items.

This will be used in the next patch for font-variant bitflag types.

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

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1434130 part 4 - Use unified lists to impl several bitflag font-variant properties.

This also changes their ToCss impl to use SequenceWriter instead of
checking has_value manually.

SpecifiedValueInfo for those types are also implemented in this patch.

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

Comment 29

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1434130 part 5 - Derive ToCss for values::generics::font::KeywordSize.

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

Comment 30

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1434130 part 6 - Allow shorthands to specify their own impl of SpecifiedValueInfo and manual impl it for font and border.

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

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1434130 part 7 - Have Parse derive respect #[css(skip)] on variant as well and derive Parse for KeywordSize.

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

Comment 32

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1434130 part 8 - Have TextAlign derive ToCss and unship char value from it.

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

Comment 33

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1434130 part 9 - Use unified list for TextDecorationLine and implement its SpecifiedValueInfo.

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

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8970823 [details]
Bug 1434130 part 1 - Skip system font variant for ToCss in font subproperties.

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

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8970825 [details]
Bug 1434130 part 2 - Add collect_values function to SpecifiedValueInfo trait for collecting possible values.

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

Comment 36

[Xidorn Quan [:xidorn] UTC+10]

This is still WIP. I hope I can finish it today...

Comment 37

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8970823 [details]
Bug 1434130 part 1 - Skip system font variant for ToCss in font subproperties.

This adds a change to the test to skip subproperty tests when font shorthand is specified with a system font, so it may need another review.

Comment 38

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8970823 [details]
Bug 1434130 part 1 - Skip system font variant for ToCss in font subproperties.

https://reviewboard.mozilla.org/r/239600/#review245686

::: layout/style/test/test_value_storage.html:225
(Diff revision 2)
>      if ("subproperties" in info &&
>          // Using setProperty over subproperties is not sufficient for
>          // system fonts, since the shorthand does more than its parts.
> -        (property != "font" || !(value in gSystemFont)) &&
> +        !is_system_font &&
>          // Likewise for special compatibility values of transform
>          (property != "-moz-transform" || !value.match(/^matrix.*(px|em|%)/)) &&

nit: You can remove the -moz-transform special-case while here.

Comment 39

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8970823 [details]
Bug 1434130 part 1 - Skip system font variant for ToCss in font subproperties.

https://reviewboard.mozilla.org/r/239600/#review245690

::: servo/components/style/properties/helpers.mako.rs:414
(Diff revision 2)
>  
>          #[cfg_attr(feature = "gecko", derive(MallocSizeOf))]
>          #[derive(Clone, Copy, Debug, Eq, PartialEq, SpecifiedValueInfo, ToCss)]
>          pub enum SpecifiedValue {
>              Keyword(computed_value::T),
> +            #[css(skip)]

Per IRC discussion, let's add a comment somewhere in the speecified font types file about the situation, and that we serialize font as-if it contained variable references, pointing to https://github.com/w3c/csswg-drafts/issues/1586.

Comment 40

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8971140 [details]
Bug 1434130 part 3 - Add SequenceWriter::item_str for writing str items.

https://reviewboard.mozilla.org/r/239924/#review245692

::: servo/components/style_traits/values.rs:178
(Diff revision 1)
> -    /// and is written in subsequent calls only if the `item` produces some
> -    /// output on its own again. This lets us handle `Option<T>` fields by
> -    /// just not printing anything on `None`.
>      #[inline]
> -    pub fn item<T>(&mut self, item: &T) -> fmt::Result
> -    where
> +    fn write_item<F>(&mut self, f: F) -> fmt::Result
> +    where F: FnOnce(&mut CssWriter<'b, W>) -> fmt::Result

nit: F to the next line.

::: servo/components/style_traits/values.rs:224
(Diff revision 1)
> +        T: ToCss,
> +    {
> +        self.write_item(|inner| item.to_css(inner))
> +    }
> +
> +    /// Writes a string, with any separator as necessary.

Please note that the string is not escaped or anything, and that people should not use this unless they know what they're doing :)

::: servo/components/style_traits/values.rs:228
(Diff revision 1)
> +
> +    /// Writes a string, with any separator as necessary.
> +    ///
> +    /// See SequenceWriter::item.
> +    #[inline]
> +    pub fn item_str(&mut self, item: &str) -> fmt::Result {

raw_item maybe or something like that?

Comment 41

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8971141 [details]
Bug 1434130 part 4 - Use unified lists to impl several bitflag font-variant properties.

https://reviewboard.mozilla.org/r/239926/#review245696

::: servo/components/style/values/specified/font.rs:1756
(Diff revision 1)
> -pub fn assert_variant_numeric_matches() {
> +        pub fn assert_variant_numeric_matches() {
> -    use gecko_bindings::structs;
> +            use gecko_bindings::structs;
> -
> -    macro_rules! check_variant_numeric {
> -        ( $( $a:ident => $b:path),*, ) => {
>              if cfg!(debug_assertions) {

This could remove the cfg!() and use debug_assert_eq! while here.

Comment 42

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8971142 [details]
Bug 1434130 part 5 - Derive ToCss for values::generics::font::KeywordSize.

https://reviewboard.mozilla.org/r/239928/#review245698

Comment 43

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8971143 [details]
Bug 1434130 part 6 - Allow shorthands to specify their own impl of SpecifiedValueInfo and manual impl it for font and border.

https://reviewboard.mozilla.org/r/239930/#review245702

Comment 44

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8971144 [details]
Bug 1434130 part 7 - Have Parse derive respect #[css(skip)] on variant as well and derive Parse for KeywordSize.

https://reviewboard.mozilla.org/r/239932/#review245704

::: servo/components/style_derive/parse.rs:23
(Diff revision 1)
>              bindings.is_empty(),
>              "Parse is only supported for single-variant enums for now"
>          );
>  
>          let variant_attrs = cg::parse_variant_attrs_from_ast::<CssVariantAttrs>(&variant.ast());
> +        if variant_attrs.skip {

I think you can derive parse for KeywordInfo with this patch.

Comment 45

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8971145 [details]
Bug 1434130 part 8 - Have TextAlign derive ToCss and unship char value from it.

https://reviewboard.mozilla.org/r/239934/#review245708

Comment 46

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8971146 [details]
Bug 1434130 part 9 - Use unified list for TextDecorationLine and implement its SpecifiedValueInfo.

https://reviewboard.mozilla.org/r/239936/#review245712

I wonder if we should teach the derive code about bitfields... Maybe a bit hard?

::: servo/components/style/values/specified/text.rs:357
(Diff revision 1)
> +    }
> +}
> +
> +impl_text_decoration_line! {
> +    /// Underline
> +    UNDERLINE / "underline" => 0x01,

While you're here, you may consider changing 0x01 for 1 << 0 and so on.

Comment 47

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8970823 [details]
Bug 1434130 part 1 - Skip system font variant for ToCss in font subproperties.

https://reviewboard.mozilla.org/r/239600/#review245686

> nit: You can remove the -moz-transform special-case while here.

This has grown to a big patchset, so I'd rather leave this as-is for now.

Comment 48

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8970823 [details]
Bug 1434130 part 1 - Skip system font variant for ToCss in font subproperties.

https://reviewboard.mozilla.org/r/239600/#review245690

> Per IRC discussion, let's add a comment somewhere in the speecified font types file about the situation, and that we serialize font as-if it contained variable references, pointing to https://github.com/w3c/csswg-drafts/issues/1586.

I'm adding this comment to the `system_font` mod.

Comment 49

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1434130 part 10 - Handle keywords for color values.

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

Comment 50

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1434130 part 11 - Add some attributes for SpecifiedValueInfo to help deriving more from types.

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

Comment 51

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1434130 part 12 - Manually implement collect_completion_keywords for some types.

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

Comment 52

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1434130 part 13 - Use Servo code to back GetCSSValuesForProperty.

This causes changes to properties-db.js. Most of them are making it
reflect the reality more closely:

* `{-webkit-,}align-{content,items,self}` get `first baseline`, `safe`,
  `unsafe`, and lose `left` and `right`.

* `{-moz-,-webkit-,}{animation,transition}{,-timing-function}` has a
  new `frame` keyword which is a function value in `<timing-function>`.

* `{background,{-webkit-,}mask}-position-x` lose `top` and `bottom`, and
  correspondingly `{background,{-webkit-,}mask}-position-y` lose `left`
  and `right`.  They don't deserve those values.

* `{background,{-webkit-,}mask}{,-size}` get `auto`.

* `border` shorthand loses `<image>` values as well as other keyword
  values for `border-image-*` subproperties, because they aren't parsed
  on the shorthand.

* `{-moz-,}border-image{,-width}` get `auto`.

* `-moz-context-properties` gets `none`.

* `cursor` get some -moz-prefixed values as well as `url`.

* `fill` and `stroke` get the color keywords.

* `{-webkit-,}filter` get the keywords and function names.

* `font` shorthand loses values from many of `font-variant-*` properties
  because they are not parsed there.

* `font-variant` and `font-variant-alternates` get function values of
  the longhand.

* `font-variant-{east-asian,ligatures,numeric}` get `normal`, and
  `font-variant-ligatures` in addition gets `none`.
  `font-{feature,variation}-settings` also get `normal`.

* `grid` and `grid-template-{areas,columns,rows}` get `none`.

* `grid`, `grid-template`, and `grid-template-{columns,rows}` get
  `auto`, `fit-content`, `minmax`, and `repeat`.

* `grid-auto-{columns,rows}` get `auto`, `fit-content` and `minmax`.

* `-moz-image-region` gets `auto` and `rect`.

* `{-webkit-,}justify-content` lose `baseline`, `last baseline`, and
  get `safe` and `unsafe`.

* `{justify,place}-items` get `first baseline`, `legacy`, `safe`,
  `unsafe` and lose `auto`.

* `{justify,place}-self` and `place-content` get `first baseline`,
  `safe`, and `unsafe`.

* `outline{,-style}` get `hidden`.

* `scroll-snap-coordinate` gets `none`, and `scroll-snap-points-{x,y}`
  gets `none` and `repeat`.

* `shape-outside`, `text-emphasis{,-style}` get all the keyword values
  and function names they deserve.

* `stroke-dasharray` gets `none`.

* `text-combine-upright` drops `digits` which we never implemented.

* `{-moz-,-webkit-,}transform` and `-moz-window-transform` get their
  transform function list. `accumulatematrix` and `interpolatematrix`
  aren't real CSS value but they have `#[css(function)]` specified.
  We should probably remove them at some point.

* `will-change` gets `auto`.

* All properties accept `<image>` value now gets -webkit-prefixed
  gradient function names, including
  * `background{,-image}`,
  * `{-moz-,-webkit-,}border-image{,-source}`, and
  * `{-webkit-,}mask{,-image}`.

But there are also some unfortunate changes:

* `{min-,max-,}block-size` get -moz-prefixed intrinsic size keywords.
  It's probably wrong, but nothing seems to prevent them from being
  parse currently.

* `calc` are removed from many properties. We never have it listed in
  every properties deserve it, and doing so without much false positive
  (i.e. properties don't deserve it but get it) can be pretty tricky.
  So they are just get removed for now.

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

Comment 53

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1434130 part 14 - Remove nsCSSProps::kParseVariantTable.

The parsevariant field is not removed from nsCSSPropList.h since that
file is going away soon anyway.

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

Comment 54

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8970823 [details]
Bug 1434130 part 1 - Skip system font variant for ToCss in font subproperties.

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

Comment 55

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8970825 [details]
Bug 1434130 part 2 - Add collect_values function to SpecifiedValueInfo trait for collecting possible values.

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

Comment 56

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971140 [details]
Bug 1434130 part 3 - Add SequenceWriter::item_str for writing str items.

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

Comment 57

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971141 [details]
Bug 1434130 part 4 - Use unified lists to impl several bitflag font-variant properties.

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

Comment 58

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971142 [details]
Bug 1434130 part 5 - Derive ToCss for values::generics::font::KeywordSize.

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

Comment 59

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971143 [details]
Bug 1434130 part 6 - Allow shorthands to specify their own impl of SpecifiedValueInfo and manual impl it for font and border.

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

Comment 60

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971144 [details]
Bug 1434130 part 7 - Have Parse derive respect #[css(skip)] on variant as well and derive Parse for KeywordSize.

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

Comment 61

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971145 [details]
Bug 1434130 part 8 - Have TextAlign derive ToCss and unship char value from it.

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

Comment 62

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971146 [details]
Bug 1434130 part 9 - Use unified list for TextDecorationLine and implement its SpecifiedValueInfo.

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

Comment 63

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1434130 part 15 - Remove kCSSRawPredefinedCounterStyles.

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

Comment 64

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8971221 [details]
Bug 1434130 part 10 - Handle keywords for color values.

https://reviewboard.mozilla.org/r/240014/#review245796

::: servo/ports/geckolib/glue.rs:1026
(Diff revision 1)
>      // Use B-tree set for unique and sorted result.
>      let mut values = BTreeSet::<&'static str>::new();
>      prop_id.collect_property_completion_keywords(&mut |list| values.extend(list.iter()));
>  
> +    let mut extras = vec![];
> +    if values.contains("transparent") {

Would checking `supports_type(COLOR)` be better? Specially if we move transparent and re-case currentColor.

Comment 65

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8971222 [details]
Bug 1434130 part 11 - Add some attributes for SpecifiedValueInfo to help deriving more from types.

https://reviewboard.mozilla.org/r/240016/#review245798

Any reason represents_keyword shouldn't live in `css`? It would allow us to derive serialization for more stuff.

I had talked with nox about adding it a while ago actually.

Even if you don't implement the to_css functionality here, probably keeping it as a `css(..)` attribute makes sense, wdyt?

Comment 66

[Xidorn Quan [:xidorn] UTC+10]

I'd prefer not adding stuff not currently working in ToCss into css attribute. I think it wouldn't be too hard to move in the future if we do want that.

Comment 67

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8971222 [details]
Bug 1434130 part 11 - Add some attributes for SpecifiedValueInfo to help deriving more from types.

https://reviewboard.mozilla.org/r/240016/#review245806

Hmm, ok. Can you file followups for that and ni? me? I'll get to them or mentor them otherwise. Thank you!

::: servo/components/style/values/generics/counters.rs:62
(Diff revision 1)
>  /// A generic value for lists of counters.
>  ///
>  /// Keyword `none` is represented by an empty vector.
>  #[derive(Clone, Debug, MallocSizeOf, PartialEq, SpecifiedValueInfo,
>           ToComputedValue)]
> -pub struct Counters<I>(Box<[(CustomIdent, I)]>);
> +pub struct Counters<I>(#[css(if_empty = "none")] Box<[(CustomIdent, I)]>);

derive ToCss while at it, could do with `iterable`?

::: servo/components/style/values/generics/grid.rs:202
(Diff revision 1)
>      /// A `minmax` function for a range over an inflexible `<track-breadth>`
>      /// and a flexible `<track-breadth>`
>      ///
>      /// <https://drafts.csswg.org/css-grid/#valdef-grid-template-columns-minmax>
> +    #[css(function)]
>      Minmax(TrackBreadth<L>, TrackBreadth<L>),

We should just get rid of the special-case of `Minmax` in `ToCss` and `derive` it too. Chrome doesn't have that special-case at all.

Comment 68

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8971223 [details]
Bug 1434130 part 12 - Manually implement collect_completion_keywords for some types.

https://reviewboard.mozilla.org/r/240018/#review245816

::: servo/components/style/values/generics/mod.rs:143
(Diff revision 1)
>      }
>  }
>  
> +impl SpecifiedValueInfo for CounterStyleOrNone {
> +    fn collect_completion_keywords(f: KeywordsCollectFn) {
> +        // XXX The best approach for implementing this is probably

nit: I usually leave my name in the XXX comments so that it's easier without looking at blame to reach out to someone.

Also, in servo code there's usually much more instances of `TODO(foo)` than `XXX` or `XXXfoo`.

If you don't feel too strongly about this, mind changing them to `TODO(xidorn):` or `FIXME(xidorn):`. (the ones in previous patches too, but can be a followup DONTBUILD commit or what not)

::: servo/components/style/values/specified/align.rs:231
(Diff revision 1)
>          Ok(ContentDistribution::new(
>              content_position | overflow_position,
>          ))
>      }
> +
> +    fn list_keywords(f: KeywordsCollectFn, axis: AxisDirection) {

nit: `KeywordsCollectFn` is a somewhat weird name, wdyt about `KeywordCollectionFn` or something like that? Though maybe it's subjective... It definitely sounds a bit weird to me when reading it, but by all means don't change it if you think it's not worth it.

::: servo/components/style/values/specified/align.rs:368
(Diff revision 1)
>              .unwrap_or(AlignFlags::empty());
>          let self_position = parse_self_position(input, axis)?;
>          Ok(SelfAlignment(overflow_position | self_position))
>      }
> +
> +    fn list_keywords(f: KeywordsCollectFn, axis: AxisDirection) {

Please mention in the `parse` methods that these should be updated as well when changing parsing?

::: servo/components/style/values/specified/box.rs:498
(Diff revision 1)
>              },
>          }
>      }
>  }
>  
> -impl SpecifiedValueInfo for TouchAction {}
> +impl SpecifiedValueInfo for TouchAction {

Can we derive it and use `other_values` as you've done before? It sounds a bit more compact, and also a bit more close to the type definition.

::: servo/components/style/values/specified/image.rs:65
(Diff revision 1)
> +
> +    fn collect_completion_keywords(f: KeywordsCollectFn) {
> +        // This list here should keep sync with that in Gradient::parse.
> +        f(&[
> +          "linear-gradient",
> +          "-webkit-linear-gradient",

Do we really want to list prefixed stuff that we just alias? I don't think it's a good idea. I'd rip the -moz- and -webkit- prefixed versions. Specially since we want to get rid of -moz- gradients.

Comment 69

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8971224 [details]
Bug 1434130 part 13 - Use Servo code to back GetCSSValuesForProperty.

https://reviewboard.mozilla.org/r/240020/#review245826

Hoorray for autogenerated code!

Comment 70

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8971225 [details]
Bug 1434130 part 14 - Remove nsCSSProps::kParseVariantTable.

https://reviewboard.mozilla.org/r/240022/#review245828

Can we remove the variant keywords from nsCSSProps too? Now they're unused.

Comment 71

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8971265 [details]
Bug 1434130 part 15 - Remove kCSSRawPredefinedCounterStyles.

https://reviewboard.mozilla.org/r/240028/#review245832

Comment 72

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971223 [details]
Bug 1434130 part 12 - Manually implement collect_completion_keywords for some types.

https://reviewboard.mozilla.org/r/240018/#review245816

> nit: `KeywordsCollectFn` is a somewhat weird name, wdyt about `KeywordCollectionFn` or something like that? Though maybe it's subjective... It definitely sounds a bit weird to me when reading it, but by all means don't change it if you think it's not worth it.

I... don't really think `KeywordCollectionFn` is a better name. I agree that `KeywordsCollectFn` is somewhat weird, but I don't really have better idea. If there is one, we can do that in a followup.

> Can we derive it and use `other_values` as you've done before? It sounds a bit more compact, and also a bit more close to the type definition.

Hmmm, I did that for `Contain`... I'd rather not use `other_values` if there is nothing else derivable at all from the type, but I guess it's fine...

> Do we really want to list prefixed stuff that we just alias? I don't think it's a good idea. I'd rip the -moz- and -webkit- prefixed versions. Specially since we want to get rid of -moz- gradients.

That's a good point. We can probably remove them. Actually I'm considering purging all `-moz-` and probably also `-webkit-` prefx values from the value list so that they aren't present in devtools. That would come in a followup.

Comment 73

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971222 [details]
Bug 1434130 part 11 - Add some attributes for SpecifiedValueInfo to help deriving more from types.

https://reviewboard.mozilla.org/r/240016/#review245806

> derive ToCss while at it, could do with `iterable`?

I'm... not very happy with doing this kind of change in a unrelated patch. Filed bug 1457332 as followup.

> We should just get rid of the special-case of `Minmax` in `ToCss` and `derive` it too. Chrome doesn't have that special-case at all.

Filed bug 1457331 as followup.

Comment 74

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971223 [details]
Bug 1434130 part 12 - Manually implement collect_completion_keywords for some types.

https://reviewboard.mozilla.org/r/240018/#review245816

> That's a good point. We can probably remove them. Actually I'm considering purging all `-moz-` and probably also `-webkit-` prefx values from the value list so that they aren't present in devtools. That would come in a followup.

I'd rather not regress the property-db more by doing this in this bug. I'm going to keep it as is, and do the removal of prefixed values in followup bug 1457333.

Comment 75

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971223 [details]
Bug 1434130 part 12 - Manually implement collect_completion_keywords for some types.

https://reviewboard.mozilla.org/r/240018/#review245816

> nit: I usually leave my name in the XXX comments so that it's easier without looking at blame to reach out to someone.
> 
> Also, in servo code there's usually much more instances of `TODO(foo)` than `XXX` or `XXXfoo`.
> 
> If you don't feel too strongly about this, mind changing them to `TODO(xidorn):` or `FIXME(xidorn):`. (the ones in previous patches too, but can be a followup DONTBUILD commit or what not)

I don't really like to leave names in code, and that's why I keep avoiding doing that...

Comment 76

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971221 [details]
Bug 1434130 part 10 - Handle keywords for color values.

https://reviewboard.mozilla.org/r/240014/#review245796

> Would checking `supports_type(COLOR)` be better? Specially if we move transparent and re-case currentColor.

I would change it to `currentcolor` after re-casing it. (Actually I picked `currentcolor` at the start, and when I revert the casing of that word back to `currentColor`, I decided to use `transparent` for now.)

Comment 77

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971224 [details]
Bug 1434130 part 13 - Use Servo code to back GetCSSValuesForProperty.

https://reviewboard.mozilla.org/r/240020/#review246002

::: commit-message-6cdd5:16
(Diff revision 1)
> +
> +* `{background,{-webkit-,}mask}-position-x` lose `top` and `bottom`, and
> +  correspondingly `{background,{-webkit-,}mask}-position-y` lose `left`
> +  and `right`.  They don't deserve those values.
> +
> +* `{background,{-webkit-,}mask}{,-size}` get `auto`.

I just realized that `background` and `mask` shorthand shouldn't get `auto` actually, because size always comes after a `/`.

Comment 78

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971224 [details]
Bug 1434130 part 13 - Use Servo code to back GetCSSValuesForProperty.

https://reviewboard.mozilla.org/r/240020/#review246002

> I just realized that `background` and `mask` shorthand shouldn't get `auto` actually, because size always comes after a `/`.

Well... properties-db listed `contain` and `cover` so I guess it should also have `auto` then...

Comment 79

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8970823 [details]
Bug 1434130 part 1 - Skip system font variant for ToCss in font subproperties.

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

Comment 80

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8970825 [details]
Bug 1434130 part 2 - Add collect_values function to SpecifiedValueInfo trait for collecting possible values.

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

Comment 81

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971140 [details]
Bug 1434130 part 3 - Add SequenceWriter::item_str for writing str items.

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

Comment 82

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971141 [details]
Bug 1434130 part 4 - Use unified lists to impl several bitflag font-variant properties.

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

Comment 83

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971142 [details]
Bug 1434130 part 5 - Derive ToCss for values::generics::font::KeywordSize.

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

Comment 84

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971143 [details]
Bug 1434130 part 6 - Allow shorthands to specify their own impl of SpecifiedValueInfo and manual impl it for font and border.

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

Comment 85

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971144 [details]
Bug 1434130 part 7 - Have Parse derive respect #[css(skip)] on variant as well and derive Parse for KeywordSize.

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

Comment 86

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971145 [details]
Bug 1434130 part 8 - Have TextAlign derive ToCss and unship char value from it.

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

Comment 87

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971146 [details]
Bug 1434130 part 9 - Use unified list for TextDecorationLine and implement its SpecifiedValueInfo.

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

Comment 88

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971221 [details]
Bug 1434130 part 10 - Handle keywords for color values.

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

Comment 89

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971222 [details]
Bug 1434130 part 11 - Add some attributes for SpecifiedValueInfo to help deriving more from types.

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

Comment 90

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971223 [details]
Bug 1434130 part 12 - Manually implement collect_completion_keywords for some types.

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

Comment 91

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971224 [details]
Bug 1434130 part 13 - Use Servo code to back GetCSSValuesForProperty.

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

Comment 92

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971225 [details]
Bug 1434130 part 14 - Remove nsCSSProps::kParseVariantTable.

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

Comment 93

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971265 [details]
Bug 1434130 part 15 - Remove kCSSRawPredefinedCounterStyles.

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

Comment 94

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e4fc95dac2fde1508151e38cbee586665d24711

Comment 95

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971225 [details]
Bug 1434130 part 14 - Remove nsCSSProps::kParseVariantTable.

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

Comment 96

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971265 [details]
Bug 1434130 part 15 - Remove kCSSRawPredefinedCounterStyles.

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

Comment 97

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 98

[Tom Tromey :tromey]

I think that, since this involves a possible regression, someone more active in the
inspector should be involved, so I'm kicking it over to :gl.

Comment 99

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8971224 [details]
Bug 1434130 part 13 - Use Servo code to back GetCSSValuesForProperty.

I am assuming we will reimplement getting back the color values judging by the XXXX. 

This mainly regresses having the color values and we lose autocomplete for calc which I think we are ok with.

Comment 100

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44c33a20df27
part 1 - Skip system font variant for ToCss in font subproperties. r=emilio
https://hg.mozilla.org/integration/autoland/rev/5edb30a5ac7b
part 2 - Add collect_values function to SpecifiedValueInfo trait for collecting possible values. r=emilio
https://hg.mozilla.org/integration/autoland/rev/cebd5132edfc
part 3 - Add SequenceWriter::item_str for writing str items. r=emilio
https://hg.mozilla.org/integration/autoland/rev/543c9e49bfe5
part 4 - Use unified lists to impl several bitflag font-variant properties. r=emilio
https://hg.mozilla.org/integration/autoland/rev/10da0873e0a5
part 5 - Derive ToCss for values::generics::font::KeywordSize. r=emilio
https://hg.mozilla.org/integration/autoland/rev/30d22ed91462
part 6 - Allow shorthands to specify their own impl of SpecifiedValueInfo and manual impl it for font and border. r=emilio
https://hg.mozilla.org/integration/autoland/rev/3575c9c52285
part 7 - Have Parse derive respect #[css(skip)] on variant as well and derive Parse for KeywordSize. r=emilio
https://hg.mozilla.org/integration/autoland/rev/1fc2da3cfe61
part 8 - Have TextAlign derive ToCss and unship char value from it. r=emilio
https://hg.mozilla.org/integration/autoland/rev/e615f3f0029a
part 9 - Use unified list for TextDecorationLine and implement its SpecifiedValueInfo. r=emilio
https://hg.mozilla.org/integration/autoland/rev/83df94ad2416
part 10 - Handle keywords for color values. r=emilio
https://hg.mozilla.org/integration/autoland/rev/b9e4fda9a50f
part 11 - Add some attributes for SpecifiedValueInfo to help deriving more from types. r=emilio
https://hg.mozilla.org/integration/autoland/rev/1baf95995178
part 12 - Manually implement collect_completion_keywords for some types. r=emilio
https://hg.mozilla.org/integration/autoland/rev/f31672f0c57a
part 13 - Use Servo code to back GetCSSValuesForProperty. r=emilio,gl
https://hg.mozilla.org/integration/autoland/rev/2265aba376c0
part 14 - Remove nsCSSProps::kParseVariantTable. r=emilio
https://hg.mozilla.org/integration/autoland/rev/ee0ac8a8f46e
part 15 - Remove kCSSRawPredefinedCounterStyles. r=emilio

Comment 101

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/44c33a20df27
https://hg.mozilla.org/mozilla-central/rev/5edb30a5ac7b
https://hg.mozilla.org/mozilla-central/rev/cebd5132edfc
https://hg.mozilla.org/mozilla-central/rev/543c9e49bfe5
https://hg.mozilla.org/mozilla-central/rev/10da0873e0a5
https://hg.mozilla.org/mozilla-central/rev/30d22ed91462
https://hg.mozilla.org/mozilla-central/rev/3575c9c52285
https://hg.mozilla.org/mozilla-central/rev/1fc2da3cfe61
https://hg.mozilla.org/mozilla-central/rev/e615f3f0029a
https://hg.mozilla.org/mozilla-central/rev/83df94ad2416
https://hg.mozilla.org/mozilla-central/rev/b9e4fda9a50f
https://hg.mozilla.org/mozilla-central/rev/1baf95995178
https://hg.mozilla.org/mozilla-central/rev/f31672f0c57a
https://hg.mozilla.org/mozilla-central/rev/2265aba376c0
https://hg.mozilla.org/mozilla-central/rev/ee0ac8a8f46e

Comment 102

[Eric Rahm [:erahm]]

It looks like this regressed libxul size by 212k.

https://treeherder.mozilla.org/perf.html#/graphs?timerange=2592000&series=autoland,1338582,1,2&series=mozilla-inbound,1299711,0,2&zoom=1524878415904.5642,1524988460670.1245,130197303.34335499,130511910.08492802&selected=autoland,1338582,331976,460069505,2

Comment 103

[Emilio Cobos Álvarez (:emilio)]

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #102)
> It looks like this regressed libxul size by 212k.
> 
> https://treeherder.mozilla.org/perf.html#/
> graphs?timerange=2592000&series=autoland,1338582,1,2&series=mozilla-inbound,
> 1299711,0,2&zoom=1524878415904.5642,1524988460670.1245,130197303.34335499,
> 130511910.08492802&selected=autoland,1338582,331976,460069505,2

Hmm, I'm afraid that was kind of expected given the different approach here...

Xidorn, we don't run this code other than to generate the devtools database atm, right? Do you know if it would be an option to compile this code away somehow?

Comment 104

[Xidorn Quan [:xidorn] UTC+10]

DevTools invoke this function at runtime to get the database, and use the generated database as a fallback when this function is not availabe. See initCssProperties[1].

It's not very clear to me why it increases such a bit of codesize, though, because I would expect Rust compiler to be able to merge all the strings so that we should not be producing any new string. Is it purely the arrays themselves taking this large code size? It would probably be helpful if we can do some analysis on those functions.


[1] https://searchfox.org/mozilla-central/rev/bf4def01bf8f6ff0d18f02f2d7e9efc73e12c63f/devtools/shared/fronts/css-properties.js#241

Comment 105

[Xidorn Quan [:xidorn] UTC+10]

If the code size increase is mainly from the functions which invoking the callback function, or the arrays... Maybe we can probably think about someway to generate a unified list for each type (rather than requiring nestedly invoking stuff), but that doesn't seem to be easy (actually I'm not sure whether it's possible at all)...

Comment 106

[Nathan Froyd [:froydnj]]

size(1) says:

   text	   data	    bss	    dec	    hex	filename
97784128	4082224	 636281	102502633	61c10e9	firefox-before/libxul.so
97879056	4116096	 636281	102631433	61e0809	firefox-after/libxul.so

So that's about ~90k text (.text, .rodata, and .eh_frame* being the major contributors here), 30k data.  .rodata doesn't play a big part in this particular change.  .symtab and .strtab aren't represented in these numbers, but they get counted in the treeherder numbers:

Before:

  [14] .text             PROGBITS        0000000000825720 0f1720 3f7fedf 00  AX  0   0 32
  [18] .eh_frame         PROGBITS        0000000005bfcbb8 54c8bb8 878b8c 00   A  0   0  8
  [34] .symtab           SYMTAB          0000000000000000 6127330 71cc10 18     35 307230  8
  [35] .strtab           STRTAB          0000000000000000 6843f40 13f32e3 00      0   0  1

After:

  [14] .text             PROGBITS        00000000008357c0 0f37c0 3f8aadf 00  AX  0   0 32
  [18] .eh_frame         PROGBITS        0000000005c18f78 54d6f78 881454 00   A  0   0  8
  [34] .symtab           SYMTAB          0000000000000000 6146330 723360 18     35 308332  8
  [35] .strtab           STRTAB          0000000000000000 6869690 140172f 00      0   0  1

text size increase = 0x3f8aadf - 0x3f7fedf   = 44032
eh_frame size increase = 0x881454 - 0x878b8c = 35016
symtab size increase = 0x723360 - 0x71cc10   = 26448
strtab size increase = 0x140172f - 0x13f32e3 = 58444

So those four things are ~160k, plus the 30k data, adds up to 190, which is at least in the right ballpark for the quoted 212k size increase.  Maybe we need fewer functions somehow?

Comment 107

[Xidorn Quan [:xidorn] UTC+10]

Majority of the function calls are static dispatch (with a function pointer passed down), so I would expect llvm or rustc to inline most of them... But maybe that isn't very effective, or invoking via a function trait takes lots of code size.

Comment 108

[Xidorn Quan [:xidorn] UTC+10]

Filed bug 1464742 for that. From the graph it doesn't seem to be a very significant regression, and also I don't have a great idea on how it can be fixed, so I gave it a P3 for now. We can continue discussion there.