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

RESOLVED FIXED in Firefox 61

Status

defect
P2
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Depends on 1 bug, Blocks 3 bugs)

unspecified
Firefox 61
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox60 wontfix, firefox61 fixed)

Details

Attachments

(15 attachments, 1 obsolete attachment)

59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
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.
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.
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.
Note there are a few other InspectorUtils methods that need a property database: inheritedProperty, getCSSPropertyNames,
getSubpropertiesForCSSProperty, cssPropertyIsShorthand, and cssPropertySupportsType.
(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)
I guess we'd need to add the type information there, but that may not be a huge issue...
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?
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?
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]
(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).
(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.
Flags: needinfo?(xidorn+moz)
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.
(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.
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.
Flags: needinfo?(dbaron)
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).
(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.
(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=
Good points. I guess the complexity involved tilts things more in favor of a manual list.
Depends on: 1446763
(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
Flags: needinfo?(xidorn+moz)
Depends on: 1452534
Blocks: 1448759
Assignee: nobody → xidorn+moz
Summary: stylo: Port InspectorUtils to be based on information from stylo rather than the old style system → stylo: Port InspectorUtils::GetCSSValuesForProperty to be based on information from stylo rather than the old style system
Depends on: 1455576
Blocks: 1254949
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?
Attachment #8970823 - Flags: review?(emilio) → review+
Comment on attachment 8970824 [details]
Bug 1434130 part 2 - Derive SpecifiedValueInfo for keywords types.

https://reviewboard.mozilla.org/r/239602/#review245270
Attachment #8970824 - Flags: review?(emilio) → 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`?
Attachment #8970825 - Flags: review?(emilio) → 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

> 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 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.
Attachment #8970824 - Attachment is obsolete: true
This is still WIP. I hope I can finish it today...
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.
Attachment #8970823 - Flags: review+ → review?(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.
Attachment #8970823 - Flags: review?(emilio) → 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.
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?
Attachment #8971140 - Flags: review?(emilio) → 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.
Attachment #8971141 - Flags: review?(emilio) → review+
Comment on attachment 8971142 [details]
Bug 1434130 part 5 - Derive ToCss for values::generics::font::KeywordSize.

https://reviewboard.mozilla.org/r/239928/#review245698
Attachment #8971142 - Flags: review?(emilio) → 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
Attachment #8971143 - Flags: review?(emilio) → 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.
Attachment #8971144 - Flags: review?(emilio) → 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
Attachment #8971145 - Flags: review?(emilio) → 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.
Attachment #8971146 - Flags: review?(emilio) → 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

> 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 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 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.
Attachment #8971221 - Flags: review?(emilio) → 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?
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 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.
Attachment #8971222 - Flags: review?(emilio) → 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.
Attachment #8971223 - Flags: review?(emilio) → 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!
Attachment #8971224 - Flags: review?(emilio) → 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.
Attachment #8971225 - Flags: review?(emilio) → review+
Comment on attachment 8971265 [details]
Bug 1434130 part 15 - Remove kCSSRawPredefinedCounterStyles.

https://reviewboard.mozilla.org/r/240028/#review245832
Attachment #8971265 - Flags: review?(emilio) → 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

> 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.
Blocks: 1457331
Blocks: 1457332
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.
Blocks: 1457333
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 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 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 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 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...
Blocks: 1457343
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.
Attachment #8971224 - Flags: review?(ttromey) → review?(gl)
Blocks: 1457635
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.
Attachment #8971224 - Flags: review?(gl) → review+
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
(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?
Flags: needinfo?(xidorn+moz)
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
Flags: needinfo?(xidorn+moz)
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)...
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?
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.
Depends on: 1464742
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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.