Closed
Bug 1434130
Opened 7 years ago
Closed 7 years ago
stylo: Port InspectorUtils::GetCSSValuesForProperty to be based on information from stylo rather than the old style system
Categories
(DevTools :: Inspector: Rules, defect, P2)
DevTools
Inspector: Rules
Tracking
(firefox-esr52 unaffected, firefox60 wontfix, firefox61 fixed)
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox60 | --- | wontfix |
firefox61 | --- | fixed |
People
(Reporter: xidorn, Assigned: xidorn)
References
(Depends on 1 open bug, Blocks 3 open bugs)
Details
Attachments
(15 files, 1 obsolete file)
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+
gl
:
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.
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.
status-firefox58:
--- → wontfix
status-firefox59:
--- → wontfix
status-firefox60:
--- → affected
status-firefox-esr52:
--- → unaffected
Priority: -- → P2
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.
Comment 3•7 years ago
|
||
Note there are a few other InspectorUtils methods that need a property database: inheritedProperty, getCSSPropertyNames,
getSubpropertiesForCSSProperty, cssPropertyIsShorthand, and cssPropertySupportsType.
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)
Comment 5•7 years ago
|
||
I guess we'd need to add the type information there, but that may not be a huge issue...
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?
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?
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]
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).
Comment 10•7 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.
Flags: needinfo?(xidorn+moz)
Comment 11•7 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.
Comment 12•7 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.
Comment 13•7 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.
Flags: needinfo?(dbaron)
Comment 14•7 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).
Comment 15•7 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.
Comment 16•7 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®exp=true&path=
Comment 17•7 years ago
|
||
Good points. I guess the complexity involved tilts things more in favor of a manual list.
Assignee | ||
Comment 18•7 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
Flags: needinfo?(xidorn+moz)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → xidorn+moz
Assignee | ||
Updated•7 years ago
|
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 22•7 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?
Attachment #8970823 -
Flags: review?(emilio) → review+
Comment 23•7 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
Attachment #8970824 -
Flags: review?(emilio) → review+
Comment 24•7 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`?
Attachment #8970825 -
Flags: review?(emilio) → review+
Assignee | ||
Comment 25•7 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 :(
Assignee | ||
Comment 26•7 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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8970824 -
Attachment is obsolete: true
Assignee | ||
Comment 36•7 years ago
|
||
This is still WIP. I hope I can finish it today...
Assignee | ||
Comment 37•7 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.
Attachment #8970823 -
Flags: review+ → review?(emilio)
Comment 38•7 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.
Attachment #8970823 -
Flags: review?(emilio) → review+
Comment 39•7 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.
Comment 40•7 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?
Attachment #8971140 -
Flags: review?(emilio) → review+
Comment 41•7 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.
Attachment #8971141 -
Flags: review?(emilio) → review+
Comment 42•7 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
Attachment #8971142 -
Flags: review?(emilio) → review+
Comment 43•7 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
Attachment #8971143 -
Flags: review?(emilio) → review+
Comment 44•7 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.
Attachment #8971144 -
Flags: review?(emilio) → review+
Comment 45•7 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
Attachment #8971145 -
Flags: review?(emilio) → review+
Comment 46•7 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.
Attachment #8971146 -
Flags: review?(emilio) → review+
Assignee | ||
Comment 47•7 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.
Assignee | ||
Comment 48•7 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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 64•7 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.
Attachment #8971221 -
Flags: review?(emilio) → review+
Comment 65•7 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?
Assignee | ||
Comment 66•7 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.
Comment 67•7 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.
Attachment #8971222 -
Flags: review?(emilio) → review+
Comment 68•7 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.
Attachment #8971223 -
Flags: review?(emilio) → review+
Comment 69•7 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!
Attachment #8971224 -
Flags: review?(emilio) → review+
Comment 70•7 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.
Attachment #8971225 -
Flags: review?(emilio) → review+
Comment 71•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 72•7 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.
Assignee | ||
Comment 73•7 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.
Assignee | ||
Comment 74•7 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.
Assignee | ||
Comment 75•7 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...
Assignee | ||
Comment 76•7 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.)
Assignee | ||
Comment 77•7 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 `/`.
Assignee | ||
Comment 78•7 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...
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 94•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 97•7 years ago
|
||
Comment 98•7 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.
Updated•7 years ago
|
Attachment #8971224 -
Flags: review?(ttromey) → review?(gl)
Comment 99•7 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.
Attachment #8971224 -
Flags: review?(gl) → review+
Comment 100•7 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
Comment 101•7 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
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment 102•7 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
Comment 103•7 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?
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 104•7 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
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 105•7 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)...
Comment 106•7 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?
Assignee | ||
Comment 107•7 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.
Assignee | ||
Comment 108•7 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.
Updated•6 years ago
|
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•