Xidorn Quan [:xidorn] UTC+11 Assignee
	Description • 7 years ago

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.

	Chris Peterson [:cpeterson]
	Comment 1 • 7 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 2 • 7 years ago

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.

	Tom Tromey :tromey
	Comment 3 • 7 years ago

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

	Emilio Cobos Álvarez (:emilio)
	Comment 4 • 7 years ago

(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)

	Emilio Cobos Álvarez (:emilio)
	Comment 5 • 7 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 6 • 7 years ago

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?

	Nathan Froyd [:froydnj]
	Comment 7 • 7 years ago

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?

	Simon Sapin (:SimonSapin)
	Comment 8 • 7 years ago

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]

	Tom Tromey :tromey
	Comment 9 • 7 years ago

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

	Emilio Cobos Álvarez (:emilio)
	Comment 10 • 6 years ago

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

	Bobby Holley (:bholley)
	Comment 11 • 6 years ago

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.

	Tom Tromey :tromey
	Comment 12 • 6 years ago

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

	Bobby Holley (:bholley)
	Comment 13 • 6 years ago

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 14 • 6 years ago

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

	Bobby Holley (:bholley)
	Comment 15 • 6 years ago

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

	Emilio Cobos Álvarez (:emilio)
	Comment 16 • 6 years ago

(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=

	Bobby Holley (:bholley)
	Comment 17 • 6 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 18 • 6 years ago

(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

	Emilio Cobos Álvarez (:emilio)
	Comment 22 • 6 years ago
mozreview-review

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?

	Emilio Cobos Álvarez (:emilio)
	Comment 23 • 6 years ago
mozreview-review

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

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

	Emilio Cobos Álvarez (:emilio)
	Comment 24 • 6 years ago
mozreview-review

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`?

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 25 • 6 years ago
mozreview-review-reply

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 :(

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 26 • 6 years ago
mozreview-review-reply

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 36 • 6 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 37 • 6 years ago

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 38 • 6 years ago
mozreview-review

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 39 • 6 years ago
mozreview-review

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 40 • 6 years ago
mozreview-review

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?

	Emilio Cobos Álvarez (:emilio)
	Comment 41 • 6 years ago
mozreview-review

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 42 • 6 years ago
mozreview-review

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

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

	Emilio Cobos Álvarez (:emilio)
	Comment 43 • 6 years ago
mozreview-review

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

	Emilio Cobos Álvarez (:emilio)
	Comment 44 • 6 years ago
mozreview-review

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 45 • 6 years ago
mozreview-review

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

	Emilio Cobos Álvarez (:emilio)
	Comment 46 • 6 years ago
mozreview-review

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 47 • 6 years ago
mozreview-review-reply

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 48 • 6 years ago
mozreview-review-reply

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 64 • 6 years ago
mozreview-review

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 65 • 6 years ago
mozreview-review

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?

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 66 • 6 years ago

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 67 • 6 years ago
mozreview-review

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 68 • 6 years ago
mozreview-review

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 69 • 6 years ago
mozreview-review

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!

	Emilio Cobos Álvarez (:emilio)
	Comment 70 • 6 years ago
mozreview-review

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 71 • 6 years ago
mozreview-review

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

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 72 • 6 years ago
mozreview-review-reply

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 73 • 6 years ago
mozreview-review-reply

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 74 • 6 years ago
mozreview-review-reply

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 75 • 6 years ago
mozreview-review-reply

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 76 • 6 years ago
mozreview-review-reply

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 77 • 6 years ago
mozreview-review

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 78 • 6 years ago
mozreview-review-reply

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 94 • 6 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 97 • 6 years ago

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

	Tom Tromey :tromey
	Comment 98 • 6 years ago

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.

	Gabriel [:gl] (ΦωΦ)
	Comment 99 • 6 years ago

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.

	Pulsebot
	Comment 100 • 6 years ago

Pushed by xquan@mozilla.com:
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

	Noemi Erli[:noemi_erli]
	Comment 101 • 6 years ago
bugherder

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

	Eric Rahm [:erahm]
	Comment 102 • 6 years ago

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

	Emilio Cobos Álvarez (:emilio)
	Comment 103 • 6 years ago

(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?

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 104 • 6 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 105 • 6 years ago

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

	Nathan Froyd [:froydnj]
	Comment 106 • 6 years ago

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?

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 107 • 6 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 108 • 6 years ago

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.