Bug 1273706 (css-properties-values-api)

[META] implement CSS Properties and Values API

REOPENED
Assigned to

Status

()

Core
CSS Parsing and Computation
REOPENED
a year ago
16 days ago

People

(Reporter: heycam, Assigned: jyc)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs, {dev-doc-needed, meta})

Firefox Tracking Flags

(Not tracked)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(36 attachments)

11.43 KB, patch
Details | Diff | Splinter Review
35.02 KB, patch
Details | Diff | Splinter Review
9.10 KB, patch
Details | Diff | Splinter Review
25.51 KB, patch
Details | Diff | Splinter Review
66.32 KB, application/zip
Details
26.61 KB, text/plain
Details
58 bytes, text/x-review-board-request
Details | Review
58 bytes, text/x-review-board-request
Details | Review
58 bytes, text/x-review-board-request
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
dbaron
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
dbaron
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
dbaron
: review-
Details | Review
58 bytes, text/x-review-board-request
dbaron
: review-
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review-
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
dbaron
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
dbaron
: review-
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review-
Details | Review
A few months ago I started looking at implementing support for typed custom properties.  I'm attaching my WIP patches here in case someone gets to finish it off before I do.
Created attachment 8753599 [details] [diff] [review]
Part 1: Track a separate list of exposed property order on Declarations. (WIP)

This is needed because we want to store any invalid typed custom property declarations on a Declaration, but we don't want to expose them to script.  We need to store them so that if someone calls registerProperty later, we don't have to re-parse the style sheet.
Created attachment 8753602 [details] [diff] [review]
Part 2: Store normal and important variable declarations in the one object. (WIP)

I can't recall why I needed to have a single object storing !important and non-!important custom properties.

This patch also changes how custom properties are stored in a CSSVariableDeclarations object.  I think this was because we can have multiple custom property declarations with the same name that must all be preserved in case we call registerProperty.
Created attachment 8753603 [details] [diff] [review]
Part 3: Add CSSVariableSyntax class to represent the types of syntax CSS Custom Properties can be defined to take. (WIP)
Created attachment 8753605 [details] [diff] [review]
Part 4: (WIP)

This is just the remainder of the work I had in my tree, which includes:

* initial implementations of registerProperty/unregisterProperty
* CSS parser support for parsing a custom property with a given registered type
Attachment #8753602 - Attachment description: Part 2: Store normal and important variable declarations in the one object. → Part 2: Store normal and important variable declarations in the one object. (WIP)
Hi Cameron! Jonathan Chan's internship project (CSS Paint & several supporting houdini specs) will likely build on top of this work.  Do you have plans to finish this bug off in the near term?  And/or would it be useful for him to take your patches here & run with them, with your guidance?
Flags: needinfo?(cam)
(Assignee)

Updated

a year ago
See Also: → bug 1278697
(Assignee)

Comment 6

a year ago
Hi Cameron! Just to clarify the intent of the code re: invalid typed custom properties, is it so that we can have something like in Example 2 of the spec [1]? That is, supposing we have a bunch of custom property declarations for the same custom property but with values of different types, using register/unregisterProperty to "change" the type of the custom property should cause us to switch between declarations? E.g.:

{
  ...
  --something: url(...); // A
  --something: rgb(...); // B
  --something: ... s;    // C
  ...
}

// Switch to B
registerProperty({ name: "--something", syntax: "<color>" });
unregisterProperty("--something");

// Switch to A
registerProperty({ name: "--something", syntax: "<url>" });
unregisterProperty("--something");

Thanks!

[1]: https://drafts.css-houdini.org/css-properties-values-api/#the-registerproperty-function
I don't think I'll get to it within the next couple of months, so I'm happy if Jonathan can take over!

> Just to clarify the intent of the code re: invalid typed custom properties, is it so that we can
> have something like in Example 2 of the spec [1]? That is, supposing we have a bunch of custom
> property declarations for the same custom property but with values of different types, using
> register/unregisterProperty to "change" the type of the custom property should cause us to switch
> between declarations?

Yes, that's right.

An alternative approach would be to re-parse the style sheet after a register/unregisterProperty call, but this would result in a fair amount of work that's not necessary (the actual parsing of the style sheet, rebuilding the cascade, rerunning selector matching for the entire document).
Flags: needinfo?(cam)
Keywords: dev-doc-needed
(Assignee)

Comment 8

a year ago
Attaching my work in progress patches here as backup before leaving for the airport! They start from Cameron's patches and only add a little code. p12 adds a new style struct which I later decided not to do. Able to render this properly:

<!doctype html>
<html>
<head>
  <title>variables test</title>
  <script>
CSS.registerProperty({name: "--a", syntax: "<color>", inherits: true, initialValue: "green"});
CSS.registerProperty({name: "--b", syntax: "<color>", inherits: false, initialValue: "red"});
  </script>
  <style>
:root {
}
div {
  --a: 10px;
  --a: 20px;
  --a: 20px;
  --my-width: calc(var(--a) + 10px);
  width: calc(var(--my-width) + 10px);
  background-color: var(--a);
  --b: blue;
}
span {
  color: white;
  background-color: var(--b);
}
  </style>
</head>
<body>
  <div>
    Hi there!
    <span>
      I should be red, not blue!
    </span>
  </div>
  <form action="#">
    <input type="button" onclick="javascript:CSS.unregisterProperty('--a')" value="Unregister" />
  </form>
</body>
</html>

Made a lot of assumptions and there are some hacky things throughout, so my next step will be verifying everything works as expected and then cleaning up.
(Assignee)

Comment 9

a year ago
Created attachment 8762435 [details]
jyc-wip-patches.zip
(Assignee)

Comment 10

a year ago
Created attachment 8762757 [details]
p15-idempotence-and-supports

Messy test patch for implementing computational idempotence checking, resolution & transform function parsing, and supports. Absolutely not final and not very tested! Uploaded as backup.
(Assignee)

Comment 11

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd8735ad6e9b
(Assignee)

Comment 12

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0b72557ded2
(Assignee)

Comment 13

a year ago
Oops, looks like |./mach try| automatically posted stuff here. Not sure what's going on with every variables test failing in those try runs, I've been trying to replicate the failures on my Linux laptop without success.
(Assignee)

Updated

a year ago
Depends on: 1285365
Blocks: 1287171
(Assignee)

Comment 14

11 months ago
(Hopefully) final try run before review including some preliminary tests -- there are a few failures on Linux x64 that seem to be unrelated. Doing final once-over of patches (the same as on the try run) before pushing to MozReview!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da9e264c1724&selectedJob=23960462
Assignee: nobody → jchan
(Assignee)

Comment 15

11 months ago
Created attachment 8773488 [details]
Bug 1273706 - Part 1: Add missing includes exposed by unification.

Review commit: https://reviewboard.mozilla.org/r/66136/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66136/
Attachment #8773488 - Flags: review?(cam)
Attachment #8773489 - Flags: review?(cam)
Attachment #8773490 - Flags: review?(cam)
Attachment #8773491 - Flags: review?(cam)
Attachment #8773492 - Flags: review?(cam)
Attachment #8773493 - Flags: review?(cam)
Attachment #8773494 - Flags: review?(cam)
Attachment #8773495 - Flags: review?(cam)
Attachment #8773496 - Flags: review?(cam)
Attachment #8773497 - Flags: review?(dbaron)
Attachment #8773498 - Flags: review?(cam)
Attachment #8773499 - Flags: review?(cam)
Attachment #8773500 - Flags: review?(dbaron)
Attachment #8773501 - Flags: review?(dbaron)
Attachment #8773502 - Flags: review?(cam)
Attachment #8773503 - Flags: review?(cam)
Attachment #8773504 - Flags: review?(cam)
Attachment #8773505 - Flags: review?(dbaron)
Attachment #8773506 - Flags: review?(cam)
Attachment #8773507 - Flags: review?(dbaron)
Attachment #8773508 - Flags: review?(cam)
Attachment #8773509 - Flags: review?(cam)
Attachment #8773510 - Flags: review?(cam)
Attachment #8773511 - Flags: review?(cam)
Attachment #8773512 - Flags: review?(cam)
Attachment #8773513 - Flags: review?(cam)
(Assignee)

Comment 16

11 months ago
Created attachment 8773489 [details]
Bug 1273706 - Part 2: Add missing namespace names that the Windows build complained about.

Review commit: https://reviewboard.mozilla.org/r/66138/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66138/
(Assignee)

Comment 17

11 months ago
Created attachment 8773490 [details]
Bug 1273706 - Part 3: Add new pref to enable the Properties & Values API.

This controls whether or not CSS.registerProperty and CSS.unregisterProperty are
available. Behavior with existing CSS variables should be the same (we currently
pass all tests).

Review commit: https://reviewboard.mozilla.org/r/66140/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66140/
(Assignee)

Comment 18

11 months ago
Created attachment 8773491 [details]
Bug 1273706 - Part 4: Add new nsCSSValue units for resolutions.

Before, eCSSUnit_Inch, eCSSUnit_Pixel, and eCSSUnit_Centimeter were used for
DPI, DPPX, and DPCM, respectively.

Now custom properties can have type <resolution>, and it'd be nice to associate
proper units with the resulting nsCSSValues.

The next patch in this series edits nsCSSParser to parse resolutions using the
new units.

Review commit: https://reviewboard.mozilla.org/r/66142/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66142/
(Assignee)

Comment 19

11 months ago
Created attachment 8773492 [details]
Bug 1273706 - Part 5: Factor out code for parsing resolutions in nsCSSParser.

Move it into a new ParseResolution method on CSSParserImpl. This is used by a
later patch in this series which parses custom property values.

Review commit: https://reviewboard.mozilla.org/r/66144/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66144/
(Assignee)

Comment 20

11 months ago
Created attachment 8773493 [details]
Bug 1273706 - Part 6: Add CSSProperty type for custom properties.

The current animation code only deals with shorthand properties, who are given
values in the nsCSSProperty enum. With the implementation of the Properties &
Values API, we need to be able to animate custom properties whose types support
it as well.

This patch makes the backing type of nsCSSProperty intptr_t, meaning that a
variable for holding a nsCSSProperty will have room for holding a nsIAtom*.
I also update relevant helper functions (those called by code which will be
exposed to nsIAtom* nsCSSProperties in later patches in the series) to
recognize a nsCSSProperty argument representing a custom property accordingly.

The nsIAtoms are not static atoms, but they aren't deallocated because they are
owned by the registrations for their respective custom properties (and atoms
are only needed for the custom properties, because they are the only other
properties that are animatable).

There should not be any collisions between nsCSSProperties representing
pointers to atoms and regular nsCSSProperties because modern operating systems
mark page zero as inaccessble. To be even more sure, we could make regular
properties all have their least-significant bit set (pointers shouldn't collide
they will be word aligned, cf tagged integers).

I thought that doing this would mean modifying the least code, because these
custom properties need to be passed around like this *only* for animations (see
later patches in this series). We don't use these for other things, like
fetching custom properties from data blocks, because they aren't stored in data
blocks. Some alternatives:

* making animations store things in another representation
* modifying nsCSSProperty to be a struct with a separate tag field

Review commit: https://reviewboard.mozilla.org/r/66146/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66146/
(Assignee)

Comment 21

11 months ago
Created attachment 8773494 [details]
Bug 1273706 - Part 7: Add support to StyleAnimationValues for storing lists of values.

The lists are homogeneous lists of style animation values. We also implement
computing distances & interpolating between them.

When calculating the distance or interpolating between two lists, the lists are
repeated to the length of the result list, whose length is the least common
multiple of the lengths of the input lists, as specified in the CSS Transitions
spec [1].

This is for support of custom properties with syntax like <number>+, which
indicates a list of <number>s with length at least one. Currently the syntax
only supports homogeneous non-empty list.

[1]: https://drafts.csswg.org/css-transitions-1/#animtype-repeatable-list.

Review commit: https://reviewboard.mozilla.org/r/66148/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66148/
(Assignee)

Comment 22

11 months ago
Created attachment 8773495 [details]
Bug 1273706 - Part 8: Separate nsStyleImage from nsStyleStruct.

Previously this was not necessary because computed values for various
properties were simply stored with the appropriate type in the corresponding
style struct. Now we can have typed custom properties whose names and types are
not known until run-time.

We already have StyleAnimationValues, which represent computed animatable
values, and nsCSSValues, which represent specified values. Why do we need a new
type?

Some custom property types are not animatable, for example, <custom-ident>s. At
the same time, its convenient to store computed information, e.g. for <length>s,
both so that we can extract computed values for animatable properties and so
that we can expose typed values through the future Typed OM API.

CSSComputedValues has simple internal representations for types that makes
conversion to StyleAnimationValues (and in the future, Typed OM values)
straightforward.

Computed values are stored in a hashtable on a new ComputedVars style struct
that is added in a future patch in this series.

Review commit: https://reviewboard.mozilla.org/r/66150/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66150/
(Assignee)

Comment 23

11 months ago
Created attachment 8773496 [details]
Bug 1273706 - Part 12: Add a new type for computed CSS values.

CSSComputedValues provides a key-value map from custom property names (without
the preceding --) to their computed values, like CSSVariableValues. The primary
reason we write this small wrapper class instead of using a ns*Hashtable
directly is so that we can expose operator== for computing differences.

Review commit: https://reviewboard.mozilla.org/r/66152/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66152/
(Assignee)

Comment 24

11 months ago
Created attachment 8773497 [details]
Bug 1273706 - Part 10: Expose StyleAnimationValue::StyleCoordToValue and StyleCoordToCSSValue.

A custom property has a syntax that specifies the set of legal values (see the
Properties & Values API specification).

CSSVariableSyntax::SetSyntax parses a specified syntax string into a
CSSVariableSyntax object.
A future patch in this series adds functions to nsCSSParser to parse and type
check a declaration with a CSSVariableSyntax.

heycam wrote this code. I only made small modifications for parsing resolutions
and transform-functions and a fix to the syntax parser.

Review commit: https://reviewboard.mozilla.org/r/66154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66154/
(Assignee)

Comment 25

11 months ago
Created attachment 8773498 [details]
Bug 1273706 - Part 11: Have StyleAnimationValue::UncomputeValue(..., nsCSSValue) uncompute UnparsedStrings to token stream nsCSSValues.

Add a CSSVariableRegistration struct to hold registration information. Also add
some utility methods for getting access to the registrations hashtable from
available objects (used in nsCSSParser, nsRuleNode, etc.) that are used in
future patches in this series.

Add a VariableExprContext for storing the context of a variable expression. This
is important for when the computed value of a custom property might depend on
the context its declared in -- for example, relative URLs need to be resolved
based on the stylesheet they were declared in.

Some of these type and function names are absurdly long, though I'm not sure how
best to shorten them.

Review commit: https://reviewboard.mozilla.org/r/66156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66156/
(Assignee)

Comment 26

11 months ago
Created attachment 8773499 [details]
Bug 1273706 - Part 14: Store custom property registrations on the document.

When the parser is bound to a sheet, we can get custom property registrations
from the sheet's document's inner window.
But sometimes we use the parser and need registrations, but are not associated
with a sheet. In particular, when we are handling custom property values that
come through JS API calls rather than through parsing actual sheets.
So we expose a method for setting a custom property registrations object on the
parser that overrides getting custom property registrations from the associated
sheet.

Review commit: https://reviewboard.mozilla.org/r/66158/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66158/
(Assignee)

Comment 27

11 months ago
Created attachment 8773500 [details]
Bug 1273706 - Part 13: Add CSSVariableSyntax for representing the syntax of a custom property.

We allow variable references when parsing variable declarations in support
conditions or to load into a |Declaration| object.
However, we expect resolved variable declarations (with variable values
substituted) to not contain variable references, so we disallow variable
references there. In particular, a future patch in this series adds a
ParseTypedValue method.
This expects fully resolved values (otherwise they couldn't be typed), so it
disallows variable references.

Review commit: https://reviewboard.mozilla.org/r/66160/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66160/
(Assignee)

Comment 28

11 months ago
Created attachment 8773501 [details]
Bug 1273706 - Part 9: Expose nsComputedDOMStyle::SetValueToStyleImage as a static method so that we can compute serialized computed values for gradients.

This is what we use for checking the types of custom property values in future
patches in this series.

Review commit: https://reviewboard.mozilla.org/r/66162/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66162/
(Assignee)

Comment 29

11 months ago
Created attachment 8773502 [details]
Bug 1273706 - Part 15: Add nsCSSProps::LookupCustomProperty for looking up a CSSProperty given a custom property name and registrations.

For conditions like |(--a: 5px)|, when |--a| is a registetred custom property,
we verify that the value types correctly.

Review commit: https://reviewboard.mozilla.org/r/66164/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66164/
(Assignee)

Comment 30

11 months ago
Created attachment 8773503 [details]
Bug 1273706 - Part 16: Store custom property registrations on nsCSSParser.

Inherited from heycam's patches.

This means we have to store more information for variables (in particular, if
the variable is important).
We now also store the VariableExprContext, which I added in the previous patch,
on variable declarations, although in this patch we leave it empty (it's used by
a future patch in the series).

The eTokenStream, etc. enum values are changed to an enum class, which
necessitates some renaming in nsCSSParser.

Most importantly, we store invalid variable declarations on a |Declaration|'s
|CSSVariableDeclaration|. This allows us to map the declaration that becomes
correct after registering properties into the rule data without reparsing
everything.

We lazily determine which is the correct declaration when someone asks us for
information on declarations (e.g. which declaration is at some index in the
list of declarations, or when someone asks us to |MapRuleInfoInto|).

Because changing registrations might cause which declaration is correct to
change, we keep track of which version of the |CSSVariableRegistrations|
we last computed the 'used declaration' based on (the 'used declaration' is the
most recent declaration with the correct type, considering importance and
overriding importance).

This necessitates a couple changes:
* The constructor for |Declaration| now takes a |RefPtr| to a
  |CSSVariableRegistrations|, which is used for computing the used
  declaration.
* |Declaration| presents an 'exposed order' that is computed from the actual
  order. Suppose |--a| is registered with syntax <number>. Then in the
  declaration:
    {
      --a: red;
      color: black;
      --a: 50px;
      display: none;
      --a: 4.7;
    }
  ... the internal order needs to keep track of the positions of *all* of the
  declarations of |--a|, so that if the registration for |--a| is removed and
  replaced with a new registration, indexed getters can return |--a| at the
  correct position, which is expressed in the exposed order.
  The 'exposed order' system on |Declaration|s comes from heycam's patches.

Sorry this is such a long patch. I couldn't find a good way to split it up.

Review commit: https://reviewboard.mozilla.org/r/66166/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66166/
(Assignee)

Comment 31

11 months ago
Created attachment 8773504 [details]
Bug 1273706 - Part 17: Add an aAllowVariableReferences parameter to ParseVariableDeclaration.

Namely, the parsed CSS value (for now, a placeholder -- in a later patch in the
series, we put all unregistered properties into token stream values and typed
properties into specified values of the appropriate type) as well as the type
(provided by ParseTypedValue) and the context of the declaration.

Review commit: https://reviewboard.mozilla.org/r/66168/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66168/
(Assignee)

Comment 32

11 months ago
Created attachment 8773505 [details]
Bug 1273706 - Part 18: Add the |ParseTypedValue| method for parsing typed CSS values.

Review commit: https://reviewboard.mozilla.org/r/66170/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66170/
(Assignee)

Comment 33

11 months ago
Created attachment 8773506 [details]
Bug 1273706 - Part 19: Add support for custom properties in supports conditions.

We also add a new |mHasUninherited| field to the |nsStyleVariables| style
struct. Some custom properties are uninherited, but we assume that most will be
inherited (all unregistered custom properties). Thus we keep the Variables
style struct as an inherited style struct but we set a flag when we set an
uninherited variable.

The implementation of this is in the next patch in this series.

Review commit: https://reviewboard.mozilla.org/r/66172/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66172/
(Assignee)

Comment 34

11 months ago
Created attachment 8773507 [details]
Bug 1273706 - Part 20: Store important and unimportant declarations in one object.

Note that although this patch series modifies a lot of code related to CSS
variables in order to support custom properties, behavior if a variable is
unregistered is exactly the same as before. This is why merely hiding
(un)?registerProperty behind a pref is sufficient to disable the Properties &
Values API functionality (hopefully!).

Review commit: https://reviewboard.mozilla.org/r/66174/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66174/
(Assignee)

Comment 35

11 months ago
Created attachment 8773508 [details]
Bug 1273706 - Part 21: Have CSSVariableValues store more information.

This is in preparation for adding a new ComputedVars style struct, for holding
the computed values of CSS custom properties (regular variables are just token
streams, but custom properties can have types that need computation. For
example, a variable can be set to have syntax <length> and value 5em.)

Originally I thought that I could just make them uint32_t's, because other
layout people found an unused extra bit. Unfortunately, although there is an
unused bit, it turns out that adding an extra bit to NS_STYLE_INHERIT_MASK
causes it to overlap with the additional bits for nsStyleContext's mBits, which
are pushed as far to the left as they can go.

Consequently, we need to make mDependentBits and mNoneBits uint64_t's, shift
the bits for nsStyleContext's mBits (which coincidentally is already a
uint64_t) four bits to the left, and shift the additional bits for nsRuleNode's
mDependentBits one bit to the left.

Review commit: https://reviewboard.mozilla.org/r/66176/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66176/
(Assignee)

Comment 36

11 months ago
Created attachment 8773509 [details]
Bug 1273706 - Part 22: Add WebIDL bindings for the Properties & Values API.

Custom properties that have been registered using the Properties & Values API
with the appropriate types can contain values that need to be computed and/or
that can be animated.

Because there may be multiple interfaces through which computed CSS custom
property values are accessed and in order to make use of the existing
infrastructure for sharing, we implement this as a new style struct.
The new style struct contains only a hashtable mapping from variable names to
|CSSComputedValue|s.

Review commit: https://reviewboard.mozilla.org/r/66178/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66178/
(Assignee)

Comment 37

11 months ago
Created attachment 8773510 [details]
Bug 1273706 - Part 23: Update CSSVariableResolver for custom properties.

Needed because animation code uses |nsCSSPropertySet|s to store sets of
properties to be animated, and registered custom properties are animatable.

Review commit: https://reviewboard.mozilla.org/r/66180/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66180/
(Assignee)

Comment 38

11 months ago
Created attachment 8773511 [details]
Bug 1273706 - Part 24: Set initial values for custom properties on the root.

Now that we can register custom properties that have computed values and are
animatable, we should support getting their computed value through
getComputedStyle and getPropertyValue the same way we do other properties.

Only registered properties have computed values in the ComputedVars style
struct, so we try that first. If there is no entry, then either no such
variable was declared, or the property is unregistered. In that case we fall
back to reading from the Variables style struct as before.

Review commit: https://reviewboard.mozilla.org/r/66182/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66182/
(Assignee)

Comment 39

11 months ago
Created attachment 8773512 [details]
Bug 1273706 - Part 25: Implement CSS.registerProperty and CSS.unregisterProperty.

Review commit: https://reviewboard.mozilla.org/r/66184/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66184/
(Assignee)

Comment 40

11 months ago
Created attachment 8773513 [details]
Bug 1273706 - Part 29: Implement animations for custom properties.

Review commit: https://reviewboard.mozilla.org/r/66186/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66186/
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
(Assignee)

Comment 62

11 months ago
Updated patch 6 to account for very high pointers with MSB set (those would become negative intptr_ts, so cast them to uintptr_ts before comparing).
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
I'm going to steal reviews on the first few trivial patches here, to lighten heycam's load a little.
Comment on attachment 8773488 [details]
Bug 1273706 - Part 1: Add missing includes exposed by unification.

https://reviewboard.mozilla.org/r/66136/#review63276

r=me on part 1
Attachment #8773488 - Flags: review+
Attachment #8773489 - Flags: review+
Comment on attachment 8773489 [details]
Bug 1273706 - Part 2: Add missing namespace names that the Windows build complained about.

https://reviewboard.mozilla.org/r/66138/#review63278

r=me with nits addressed on part 2

First nit -- commit message:
 "Part 2: Patch to add missing namespace names that the Windows build complained about."
Please drop "Patch to", and just begin the text with "Add ..."

::: layout/style/nsTransitionManager.h:210
(Diff revision 1)
>    // because the animation on the compositor may be running ahead while
>    // main-thread is busy.
>    static Nullable<TimeDuration> GetCurrentTimeAt(
>        const DocumentTimeline& aTimeline,
> -      const TimeStamp& aBaseTime,
> -      const TimeDuration& aStartTime,
> +      const ::mozilla::TimeStamp& aBaseTime,
> +      const ::mozilla::TimeDuration& aStartTime,

Please drop the leading "::" on "::mozilla" in both lines here -- just use "mozilla::TimeStamp" / "mozilla::TimeDuration."

(This is slightly less strict, but it shouldn't matter in practice, and it saves us a few characters & is consistent with how we use these types everywhere else.)

::: layout/style/nsTransitionManager.cpp
(Diff revision 1)
>  #include "mozilla/RestyleManagerHandle.h"
>  #include "mozilla/RestyleManagerHandleInlines.h"
>  #include "nsDOMMutationObserver.h"
>  
> -using mozilla::TimeStamp;
> -using mozilla::TimeDuration;

I suspect you should be able to revert all the changes to this .cpp file, and just add "using mozilla::dom::DocumentTimeline;", at the right sorted place in this "using" list.

(The compile errors you were getting for GetCurrentTimeAt are likely all about the fact that "DocumentTimeline" is not correctly provided by any of this file's "using" declarations right now. I'll bet that's all this is.)
Attachment #8773488 - Flags: review?(cam)
Attachment #8773489 - Flags: review?(cam)
Attachment #8773490 - Flags: review?(cam) → review?(dholbert)
Comment on attachment 8773490 [details]
Bug 1273706 - Part 3: Add new pref to enable the Properties & Values API.

https://reviewboard.mozilla.org/r/66140/#review63284

r=me on part 3, with nits addressed.

First, two nits on the extended commit message:
> This controls whether or not CSS.registerProperty and CSS.unregisterProperty
> are available. Behavior with existing CSS variables should be the same
> (we currently pass all tests).

* First sentence: s/This controls/This will control/  (since technically this pref doesn't control anything at all, at this point in the commit history. It only controls stuff as of a much later patch in the series.)

* Second sentence: Perhaps clarify this sentence as "This shouldn't impact legacy CSS Variable behavior; tests for that behavior should still pass, regardless of this pref's state."

("we currently pass all tests" is trivially true, for the most immediate definition of "currently", given that the pref has no effect at this point in the commit history. :) Really you're wanting to state an expectation/intention, not an assertion about the current state of things)

::: modules/libpref/init/all.js:5476
(Diff revision 1)
>  #if !defined(RELEASE_BUILD)
>  pref("osfile.reset_worker_delay", 30000);
>  #endif
> +
> +// Whether the Properties & Values API is enabled
> +pref("layout.css.properties_and_values.enabled", false);

Please move this up to be adjacent to other layout.css.XYZ prefs.

r=me with that
Attachment #8773490 - Flags: review?(dholbert) → review+
(Assignee)

Comment 88

11 months ago
https://reviewboard.mozilla.org/r/66140/#review63284

Thanks! Will fix this.

> Please move this up to be adjacent to other layout.css.XYZ prefs.
> 
> r=me with that

Will do.
(Assignee)

Comment 89

11 months ago
I have a patch for dholbert's review requests and a patch to replace the second union in CSSComputedValue with a MFBT Variant (which also makes the destructor, copy and move constructors, and copy and move assignment operators unnecessary. I am holding off on pushing them until I need to push other review changes to avoid excessive bug comments.
(Assignee)

Comment 90

11 months ago
I ported the Mochitest in patch 26 to a Web Platform Test that does the same thing.
Comment on attachment 8773491 [details]
Bug 1273706 - Part 4: Add new nsCSSValue units for resolutions.

https://reviewboard.mozilla.org/r/66142/#review64418

::: layout/style/nsCSSValue.h:418
(Diff revision 1)
>    eCSSUnit_Seconds      = 3000,    // (float) Standard time
>    eCSSUnit_Milliseconds = 3001,    // (float) 1/1000 second
>  
>    // Flexible fraction (CSS Grid)
> -  eCSSUnit_FlexFraction = 4000     // (float) Fraction of free space
> +  eCSSUnit_FlexFraction = 4000,    // (float) Fraction of free space
> +  

Nit: trailing white space.

::: layout/style/nsCSSValue.h:420
(Diff revision 1)
> +  eCSSUnit_DPI          = 5000,    // (float) dots per inch
> +  eCSSUnit_DPPX         = 5001,    // (float) dots per pixel
> +  eCSSUnit_DPCM         = 5002,    // (float) dots per centimeter

All of the other unit names here tend to spell out their words rather than use abbreviations (with the exception of RGB/HSL).  So I think I'd prefer to see

eCSSUnit_DotsPerInch,
eCSSUnit_DotsPerPixel,
eCSSUnit_DotsPerCentimeter,

here.  (They're no longer than the longest existing eCSSUnit_* name)
Attachment #8773491 - Flags: review?(cam) → review+
(Reporter)

Updated

11 months ago
Attachment #8773492 - Flags: review?(cam) → review+
Comment on attachment 8773492 [details]
Bug 1273706 - Part 5: Factor out code for parsing resolutions in nsCSSParser.

https://reviewboard.mozilla.org/r/66144/#review64420

::: layout/style/nsCSSParser.cpp:350
(Diff revision 1)
> +  bool ParseResolution(nsCSSValue& aValue);
> +

Do we need to call this from outside CSSParserImpl?  If not, let's move it down into the protected section.
(Assignee)

Comment 93

11 months ago
https://reviewboard.mozilla.org/r/66144/#review64420

> Do we need to call this from outside CSSParserImpl?  If not, let's move it down into the protected section.

We do not need to call it from the outside. Thanks.
(Assignee)

Comment 94

11 months ago
https://reviewboard.mozilla.org/r/66142/#review64418

> Nit: trailing white space.

Cool, fixed.

> All of the other unit names here tend to spell out their words rather than use abbreviations (with the exception of RGB/HSL).  So I think I'd prefer to see
> 
> eCSSUnit_DotsPerInch,
> eCSSUnit_DotsPerPixel,
> eCSSUnit_DotsPerCentimeter,
> 
> here.  (They're no longer than the longest existing eCSSUnit_* name)

Makes sense. Done!
(Assignee)

Comment 95

11 months ago
https://reviewboard.mozilla.org/r/66138/#review63278

> Please drop the leading "::" on "::mozilla" in both lines here -- just use "mozilla::TimeStamp" / "mozilla::TimeDuration."
> 
> (This is slightly less strict, but it shouldn't matter in practice, and it saves us a few characters & is consistent with how we use these types everywhere else.)

OK

> I suspect you should be able to revert all the changes to this .cpp file, and just add "using mozilla::dom::DocumentTimeline;", at the right sorted place in this "using" list.
> 
> (The compile errors you were getting for GetCurrentTimeAt are likely all about the fact that "DocumentTimeline" is not correctly provided by any of this file's "using" declarations right now. I'll bet that's all this is.)

That indeed works!
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Attachment #8773488 - Flags: review?(cam)
Attachment #8773489 - Flags: review?(cam)
Attachment #8773490 - Flags: review?(cam)
(In reply to Jonathan Chan [:jyc] from comment #20)
> Bug 1273706 - Part 6: Make nsCSSProperty castable to/from nsIAtom*.
...
> The nsIAtoms are not static atoms, but they aren't deallocated because they are
> owned by the registrations for their respective custom properties (and atoms
> are only needed for the custom properties, because they are the only other
> properties that are animatable).

Per one of your alternative suggestions, I wonder if it might be better to have a separate type to represent "built-in or custom property", rather than using nsCSSProperty itself.  This would make it clear which parts of the system need to deal with custom properties specifically.  And if this is a class we can stick the "grab the atom" method on it.

I also have a slight concern that we will end up holding on to nsCSSProperty values that have an nsIAtom inside them after we deregister a custom property.  Do you think it's feasible to hold a strong reference to the nsIAtom?  I think at least if we change from nsCSSProperty to a wrapper class then it will be clear where we need to handle storing nsCSSProperty values, and thus where it might be best to hold a strong reference to their underlying atom.

WDYT?

(I think I have a small preference to using the lower-bit tagging system, rather than relying on pointers never being in page zero, although you are probably right that none of the OSes we care about will allocate page zero.  We use that tagging system elsewhere.  But it would be easier to do this, or even just separate the storage of the nsCSSProperty and the nsIAtom, if we had a class to represent "built-in or custom property".)
Flags: needinfo?(jchan)
(In reply to Jonathan Chan [:jyc] from comment #21)
> When calculating the distance or interpolating between two lists, the lists
> are repeated to the length of the result list, whose length is the least common
> multiple of the lengths of the input lists, as specified in the CSS Transitions
> spec [1].

I can't see anything in the Properties and Values API spec that says to interpolate these lists as repeatable lists rather than simple lists.  Is this something that needs an issue being raised?
(In reply to Jonathan Chan [:jyc] from comment #22)
> Bug 1273706 - Part 8: Add a new type for computed CSS values.
...
> CSSComputedValues has simple internal representations for types that makes
> conversion to StyleAnimationValues (and in the future, Typed OM values)
> straightforward.

It would be nice if we can avoid defining another way to store most of these values.  For lengths, percentages, and LP-calc() values, we could use nsStyleCoord.  For the types where the spec says the computed value is the same as the specified value, we can store them as nsCSSValues.  CSSComputedValue would be a union of these.  Is there any disadvantage to doing this?
(Assignee)

Comment 125

11 months ago
(In reply to Cameron McCormack (:heycam) from comment #123)
> (In reply to Jonathan Chan [:jyc] from comment #21)
> > When calculating the distance or interpolating between two lists, the lists
> > are repeated to the length of the result list, whose length is the least common
> > multiple of the lengths of the input lists, as specified in the CSS Transitions
> > spec [1].
> 
> I can't see anything in the Properties and Values API spec that says to
> interpolate these lists as repeatable lists rather than simple lists.  Is
> this something that needs an issue being raised?

You're right, I misread the Transitions & Animations spec. Do you think treating them as repeatable lists would make sense? My thought was that it made sense because if the syntax was the same we should try to animate, and it seemed like someone might find it useful.

Should be easy to revert, otherwise, I would be down for filing an issue.
(Assignee)

Comment 126

11 months ago
(In reply to Cameron McCormack (:heycam) from comment #124)
> (In reply to Jonathan Chan [:jyc] from comment #22)
> > Bug 1273706 - Part 8: Add a new type for computed CSS values.
> ...
> > CSSComputedValues has simple internal representations for types that makes
> > conversion to StyleAnimationValues (and in the future, Typed OM values)
> > straightforward.
> 
> It would be nice if we can avoid defining another way to store most of these
> values.  For lengths, percentages, and LP-calc() values, we could use
> nsStyleCoord.  For the types where the spec says the computed value is the
> same as the specified value, we can store them as nsCSSValues. 
> CSSComputedValue would be a union of these.  Is there any disadvantage to
> doing this?

This sounds good to me!
(In reply to Jonathan Chan [:jyc] from comment #125)
> You're right, I misread the Transitions & Animations spec. Do you think
> treating them as repeatable lists would make sense? My thought was that it
> made sense because if the syntax was the same we should try to animate, and
> it seemed like someone might find it useful.
> 
> Should be easy to revert, otherwise, I would be down for filing an issue.

I think it's worth filing an issue so that it is clear which of the two list interpolation types should be used.  As for which is more useful, I think it really depends on kind of thing the custom property is used for, so I'm not sure.  To avoid unnecessary churn, how about we leave it like this for now, add a comment in the code saying it's not clear which type of list interpolation should be used, and change the code if/when the spec issue is resolved.
(Assignee)

Comment 128

11 months ago
> I think it's worth filing an issue so that it is clear which of the two list
> interpolation types should be used.

Done: https://github.com/w3c/css-houdini-drafts/issues/273

> As for which is more useful, I think it
> really depends on kind of thing the custom property is used for, so I'm not
> sure.  To avoid unnecessary churn, how about we leave it like this for now,
> add a comment in the code saying it's not clear which type of list
> interpolation should be used, and change the code if/when the spec issue is
> resolved.

OK
Flags: needinfo?(jchan)
(Assignee)

Comment 129

11 months ago
(In reply to Cameron McCormack (:heycam) from comment #127)
> I think it's worth filing an issue so that it is clear which of the two list
> interpolation types should be used.  As for which is more useful, I think it
> really depends on kind of thing the custom property is used for, so I'm not
> sure.  To avoid unnecessary churn, how about we leave it like this for now,
> add a comment in the code saying it's not clear which type of list
> interpolation should be used, and change the code if/when the spec issue is
> resolved.

We just got clarification that they should be simple lists: https://github.com/w3c/css-houdini-drafts/issues/273 . I'll implement that.

(In reply to Cameron McCormack (:heycam) from comment #122)
> Per one of your alternative suggestions, I wonder if it might be better to
> have a separate type to represent "built-in or custom property", rather than
> using nsCSSProperty itself.  This would make it clear which parts of the
> system need to deal with custom properties specifically.  And if this is a
> class we can stick the "grab the atom" method on it.

I have been working on this yesterday and today, and it looks so far like it will require a lot more changes, but it is more explicit. Some existing tests are failing now so I'm working on fixing that.
(Assignee)

Comment 130

11 months ago
After some clarification, it appears that 'identical to the specified value' is actually supposed to read 'as specified', and so the computed values should be regular CSSOM values, serialized the regular CSSOM way ( https://drafts.csswg.org/cssom/#serialize-a-css-value ). Re-editing to reflect that.
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → INVALID
(Assignee)

Comment 131

11 months ago
Whoa, not sure how the status got changed to RESOLVED INVALID.

Does anyone have the requisite permissions that could fix this? Sorry for the inconvenience.
(Reporter)

Updated

11 months ago
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Assignee)

Comment 132

11 months ago
Created attachment 8777231 [details]
Bug 1273706 - Part 27: Add CSSPropertySet.

Review commit: https://reviewboard.mozilla.org/r/68828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68828/
Attachment #8773493 - Attachment description: Bug 1273706 - Part 6: Make nsCSSProperty castable to/from nsIAtom*. → Bug 1273706 - Part 6: Add MaybeCustomProperty type for custom properties.
Attachment #8773495 - Attachment description: Bug 1273706 - Part 8: Add a new type for computed CSS values. → Bug 1273706 - Part 7.5: Expose nsComputedDOMStyle::SetValueToStyleImage as a static method so that we
Attachment #8773496 - Attachment description: Bug 1273706 - Part 9: Add CSSComputedValues, like CSSVariableValues but for computed variable values. → Bug 1273706 - Part 8: Add a new type for computed CSS values.
Attachment #8773497 - Attachment description: Bug 1273706 - Part 10: Add CSSVariableSyntax for representing the syntax of a custom property. → Bug 1273706 - Part 9: Add CSSComputedValues, like CSSVariableValues but for computed variable values.
Attachment #8773498 - Attachment description: Bug 1273706 - Part 11: Store custom property registrations on the inner window (in nsGlobalWindow). → Bug 1273706 - Part 10: Add CSSVariableSyntax for representing the syntax of a custom property.
Attachment #8773499 - Attachment description: Bug 1273706 - Part 12: Store custom property registrations on nsCSSParser. → Bug 1273706 - Part 11: Store custom property registrations on the inner window (in nsGlobalWindow).
Attachment #8773500 - Attachment description: Bug 1273706 - Part 13: Add an aAllowVariableReferences parameter to ParseVariableDeclaration. → Bug 1273706 - Part 12: Store custom property registrations on nsCSSParser.
Attachment #8773501 - Attachment description: Bug 1273706 - Part 14: Add the |ParseTypedValue| method for parsing typed CSS values. → Bug 1273706 - Part 13: Add an aAllowVariableReferences parameter to ParseVariableDeclaration.
Attachment #8773502 - Attachment description: Bug 1273706 - Part 15: Add support for custom properties in supports conditions. → Bug 1273706 - Part 14: Add the |ParseTypedValue| method for parsing typed CSS values.
Attachment #8773503 - Attachment description: Bug 1273706 - Part 16: Store important and unimportant declarations in one object. → Bug 1273706 - Part 15: Add support for custom properties in supports conditions.
Attachment #8773504 - Attachment description: Bug 1273706 - Part 17: Have CSSVariableValues store more information. → Bug 1273706 - Part 16: Store important and unimportant declarations in one object.
Attachment #8773505 - Attachment description: Bug 1273706 - Part 18: Add WebIDL bindings for the Properties & Values API. → Bug 1273706 - Part 17: Have CSSVariableValues store more information.
Attachment #8773506 - Attachment description: Bug 1273706 - Part 19: Update CSSVariableResolver for custom properties. → Bug 1273706 - Part 18: Add WebIDL bindings for the Properties & Values API.
Attachment #8773507 - Attachment description: Bug 1273706 - Part 20: Implement CSS.registerProperty and CSS.unregisterProperty. → Bug 1273706 - Part 19: Update CSSVariableResolver for custom properties.
Attachment #8773508 - Attachment description: Bug 1273706 - Part 21: Make nsRuleNode's mNoneBits and mDependentBits uint64_t's (were uint32_t's). → Bug 1273706 - Part 20: Implement CSS.registerProperty and CSS.unregisterProperty.
Attachment #8773509 - Attachment description: Bug 1273706 - Part 22: Add new ComputedVars style struct. → Bug 1273706 - Part 21: Make nsRuleNode's mNoneBits and mDependentBits uint64_t's (were uint32_t's).
Attachment #8773510 - Attachment description: Bug 1273706 - Part 23: Add support for storing custom properties in nsCSSPropertySet. → Bug 1273706 - Part 22: Add new ComputedVars style struct.
Attachment #8773511 - Attachment description: Bug 1273706 - Part 24: Have nsComputedDOMStyle try to get a computed value first for variables. → Bug 1273706 - Part 23: Add MaybeCustomPropertySet.
Attachment #8773512 - Attachment description: Bug 1273706 - Part 25: Make registered custom properties animatable. → Bug 1273706 - Part 24: Have nsComputedDOMStyle try to get a computed value first for variables.
Attachment #8773513 - Attachment description: Bug 1273706 - Part 26: Add preliminary tests. → Bug 1273706 - Part 25: Implement animations for custom properties.
Attachment #8777231 - Flags: review?(cam)
Attachment #8773497 - Flags: review?(dbaron) → review?(cam)
Attachment #8773498 - Flags: review?(cam) → review?(dbaron)
Attachment #8773500 - Flags: review?(dbaron) → review?(cam)
Attachment #8773502 - Flags: review?(cam) → review?(dbaron)
Attachment #8773505 - Flags: review?(dbaron) → review?(cam)
Attachment #8773506 - Flags: review?(cam) → review?(dbaron)
Attachment #8773507 - Flags: review?(dbaron) → review?(cam)
Attachment #8773508 - Flags: review?(cam) → review?(dbaron)
(Assignee)

Comment 133

11 months ago
Comment on attachment 8773488 [details]
Bug 1273706 - Part 1: Add missing includes exposed by unification.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66136/diff/2-3/
(Assignee)

Comment 134

11 months ago
Comment on attachment 8773489 [details]
Bug 1273706 - Part 2: Add missing namespace names that the Windows build complained about.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66138/diff/2-3/
(Assignee)

Comment 135

11 months ago
Comment on attachment 8773490 [details]
Bug 1273706 - Part 3: Add new pref to enable the Properties & Values API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66140/diff/2-3/
(Assignee)

Comment 136

11 months ago
Comment on attachment 8773491 [details]
Bug 1273706 - Part 4: Add new nsCSSValue units for resolutions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66142/diff/2-3/
(Assignee)

Comment 137

11 months ago
Comment on attachment 8773492 [details]
Bug 1273706 - Part 5: Factor out code for parsing resolutions in nsCSSParser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66144/diff/2-3/
(Assignee)

Comment 138

11 months ago
Comment on attachment 8773493 [details]
Bug 1273706 - Part 6: Add CSSProperty type for custom properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66146/diff/4-5/
(Assignee)

Comment 139

11 months ago
Comment on attachment 8773494 [details]
Bug 1273706 - Part 7: Add support to StyleAnimationValues for storing lists of values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66148/diff/4-5/
(Assignee)

Comment 140

11 months ago
Comment on attachment 8773495 [details]
Bug 1273706 - Part 8: Separate nsStyleImage from nsStyleStruct.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66150/diff/4-5/
(Assignee)

Comment 141

11 months ago
Comment on attachment 8773496 [details]
Bug 1273706 - Part 12: Add a new type for computed CSS values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66152/diff/4-5/
(Assignee)

Comment 142

11 months ago
Comment on attachment 8773497 [details]
Bug 1273706 - Part 10: Expose StyleAnimationValue::StyleCoordToValue and StyleCoordToCSSValue.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66154/diff/4-5/
(Assignee)

Comment 143

11 months ago
Comment on attachment 8773498 [details]
Bug 1273706 - Part 11: Have StyleAnimationValue::UncomputeValue(..., nsCSSValue) uncompute UnparsedStrings to token stream nsCSSValues.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66156/diff/4-5/
(Assignee)

Comment 144

11 months ago
Comment on attachment 8773499 [details]
Bug 1273706 - Part 14: Store custom property registrations on the document.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66158/diff/4-5/
(Assignee)

Comment 145

11 months ago
Comment on attachment 8773500 [details]
Bug 1273706 - Part 13: Add CSSVariableSyntax for representing the syntax of a custom property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66160/diff/4-5/
(Assignee)

Comment 146

11 months ago
Comment on attachment 8773501 [details]
Bug 1273706 - Part 9: Expose nsComputedDOMStyle::SetValueToStyleImage as a static method so that we can compute serialized computed values for gradients.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66162/diff/4-5/
(Assignee)

Comment 147

11 months ago
Comment on attachment 8773502 [details]
Bug 1273706 - Part 15: Add nsCSSProps::LookupCustomProperty for looking up a CSSProperty given a custom property name and registrations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66164/diff/4-5/
(Assignee)

Comment 148

11 months ago
Comment on attachment 8773503 [details]
Bug 1273706 - Part 16: Store custom property registrations on nsCSSParser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66166/diff/4-5/
(Assignee)

Comment 149

11 months ago
Comment on attachment 8773504 [details]
Bug 1273706 - Part 17: Add an aAllowVariableReferences parameter to ParseVariableDeclaration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66168/diff/4-5/
(Assignee)

Comment 150

11 months ago
Comment on attachment 8773505 [details]
Bug 1273706 - Part 18: Add the |ParseTypedValue| method for parsing typed CSS values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66170/diff/4-5/
(Assignee)

Comment 151

11 months ago
Comment on attachment 8773506 [details]
Bug 1273706 - Part 19: Add support for custom properties in supports conditions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66172/diff/4-5/
(Assignee)

Comment 152

11 months ago
Comment on attachment 8773507 [details]
Bug 1273706 - Part 20: Store important and unimportant declarations in one object.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66174/diff/4-5/
(Assignee)

Comment 153

11 months ago
Comment on attachment 8773508 [details]
Bug 1273706 - Part 21: Have CSSVariableValues store more information.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66176/diff/4-5/
(Assignee)

Comment 154

11 months ago
Comment on attachment 8773509 [details]
Bug 1273706 - Part 22: Add WebIDL bindings for the Properties & Values API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66178/diff/4-5/
(Assignee)

Comment 155

11 months ago
Comment on attachment 8773510 [details]
Bug 1273706 - Part 23: Update CSSVariableResolver for custom properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66180/diff/4-5/
(Assignee)

Comment 156

11 months ago
Comment on attachment 8773511 [details]
Bug 1273706 - Part 24: Set initial values for custom properties on the root.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66182/diff/4-5/
(Assignee)

Comment 157

11 months ago
Comment on attachment 8773512 [details]
Bug 1273706 - Part 25: Implement CSS.registerProperty and CSS.unregisterProperty.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66184/diff/4-5/
(Assignee)

Comment 158

11 months ago
Comment on attachment 8773513 [details]
Bug 1273706 - Part 29: Implement animations for custom properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66186/diff/4-5/
Comment on attachment 8773493 [details]
Bug 1273706 - Part 6: Add CSSProperty type for custom properties.

https://reviewboard.mozilla.org/r/66146/#review66236

A lot of the methods on the new class are pretty simple, so they can probably just go inline in nsCSSProperty.h.

::: layout/style/nsCSSProperty.h:13
(Diff revision 5)
>  #include <nsHashKeys.h>
> +#include <nsIAtom.h>
> +#include <nsCOMPtr.h>

I think all these should be using ""s, not <>s.

::: layout/style/nsCSSProperty.h:83
(Diff revision 5)
>  Hash<nsCSSProperty>(const nsCSSProperty& aValue)
>  {
>    return uint32_t(aValue);
>  }
>  
> +class MaybeCustomProperty

Please add a comment describing what this class is used for.


I'm not sure I like the name -- "Maybe" in the type name makes me think of mozilla::Maybe, and that we are storing an optional custom property here, rather than storing a property and maybe it's a custom property.

What do you think about these ideas:

* PossiblyCustomProperty -- clearer, but still a bit wordy (also I think having "CSS" in the name would be good)

* CSSExtendedProperty -- we have extended nsCSSProperty with information to identify custom properties (whereas nsCSSProperty merges all custom properties into a single value)

* CSSNamedProperty or CSSPropertyName -- we have enough information in the object to know the full name of the property (although "named property" sounds like the Web IDL thing)

* CSSPropertyID -- we have enough information to uniquely identify a property (though we have some existing uses of "PropertyID" as variable names of type nsCSSProperty)

* CSSAvailableProperty -- it's a property that's available for use in the document

None really stands out to me as the best name.  I'll leave it to you to decide (or if you have a better idea).

::: layout/style/nsCSSProperty.h:109
(Diff revision 5)
> +  bool operator==(nsIAtom* const& aOther) const;
> +  bool operator!=(nsIAtom* const& aOther) const

Just |nsIAtom* aOther| is OK.

::: layout/style/nsCSSProperty.cpp:12
(Diff revision 5)
> +#include "nsCSSProperty.h"
> +
> +namespace mozilla {
> +
> +MaybeCustomProperty::MaybeCustomProperty()
> +  : mState(MaybeCustomProperty::State::Invalid)

All "MaybeCustomProperty::State"s can be just "State" in this file.

::: layout/style/nsCSSProperty.cpp:19
(Diff revision 5)
> +}
> +
> +MaybeCustomProperty::MaybeCustomProperty(nsCSSProperty aProperty)
> +  : mState(MaybeCustomProperty::State::Fixed)
> +  , mFixed(aProperty)
> +{ }

Nit: "}" on new line, for consistency with the other empty-body methods in this file.

::: layout/style/nsCSSProperty.cpp:144
(Diff revision 5)
> +  mCustom->AddRef();
> +  return already_AddRefed<nsIAtom>(mCustom);

More idiomatic:

  return do_AddRef(mCustom);
Attachment #8773493 - Flags: review?(cam) → review+
(Reporter)

Updated

11 months ago
Attachment #8773494 - Flags: review?(cam) → review+
Comment on attachment 8773494 [details]
Bug 1273706 - Part 7: Add support to StyleAnimationValues for storing lists of values.

https://reviewboard.mozilla.org/r/66148/#review66250

::: layout/style/StyleAnimationValue.h:292
(Diff revision 5)
>      eUnit_Filter, // nsCSSValueList* (may be null)
>      eUnit_Transform, // nsCSSValueList* (never null)
>      eUnit_BackgroundPositionCoord, // nsCSSValueList* (never null)
>      eUnit_CSSValuePairList, // nsCSSValuePairList* (never null)
> -    eUnit_UnparsedString // nsStringBuffer* (never null)
> +    eUnit_UnparsedString, // nsStringBuffer* (never null)
> +    eUnit_List, // nsTArray<StyleAnimationValue*> (never null)

"*" after the ">"?

::: layout/style/StyleAnimationValue.h:385
(Diff revision 5)
>    }
>    const char16_t* GetStringBufferValue() const {
>      NS_ASSERTION(IsStringUnit(mUnit), "unit mismatch");
>      return GetBufferValue(mValue.mString);
>    }
> +  nsTArray<StyleAnimationValue> GetListValue() const {

Should we return a pointer to the nsTArray, rather than a copy of it?  The other accessors on StyleAnimationValue for non-primitive types return pointers.

::: layout/style/StyleAnimationValue.cpp:2778
(Diff revision 5)
> +        new nsTArray<StyleAnimationValue>(start->Length());
> +      for (size_t i = 0; i < start->Length(); i++) {
> +        StyleAnimationValue val;
> +        if (!AddWeighted(aProperty, aCoeff1, (*start)[i], aCoeff2, (*end)[i],
> +                         val)) {
> +          return false;

This will leak |result|.  Make |result| a UniquePtr and while we're at it, make SetAndAdoptListValue take a UniquePtr too.
(Reporter)

Updated

11 months ago
Attachment #8773495 - Flags: review?(cam) → review+
Comment on attachment 8773495 [details]
Bug 1273706 - Part 8: Separate nsStyleImage from nsStyleStruct.

https://reviewboard.mozilla.org/r/66150/#review66276

In the commit message:
&gt; SetValueToCoord is modified to take an class object pointer argument and calls
&gt; are updated appropriately to pass either nullptr or this.

s/an class/a class/

::: layout/style/nsComputedDOMStyle.cpp:5269
(Diff revision 5)
>                                      PercentageBaseGetter aPercentageBaseGetter,
>                                      const KTableEntry aTable[],
>                                      nscoord aMinAppUnits,
>                                      nscoord aMaxAppUnits)
>  {
>    NS_PRECONDITION(aValue, "Must have a value to work with");

Can you assert in here that we don't have a null aThis and a non-null aPercentageBaseGetter?
Comment on attachment 8773496 [details]
Bug 1273706 - Part 12: Add a new type for computed CSS values.

https://reviewboard.mozilla.org/r/66152/#review66284

::: layout/style/CSSComputedValue.h:22
(Diff revision 5)
> +
> +namespace mozilla {
> +
> +class StyleAnimationValue;
> +
> +class CSSComputedValue

Can you add a comment describing what this class is for, and when we use it (i.e., only for representing typed CSS custom property values)?

::: layout/style/CSSComputedValue.h:30
(Diff revision 5)
> +  class SingleTerm
> +  {
> +  public:
> +    CSSValueType GetType() const;
> +
> +    bool SetAnimationValue(StyleAnimationValue& aValue) const;

To me, "SetAnimationValue" sounds like it is going to set this object with the contents of the StyleAnimationValue.  Compare with the naming of the SetXXX methods on nsCSSValue, for example.  I know the |const| there indicates it won't, but how about we call it "GetAnimationValue" or "GetAsAnimationValue" or something like that?

::: layout/style/CSSComputedValue.h:33
(Diff revision 5)
> +    CSSValueType GetType() const;
> +
> +    bool SetAnimationValue(StyleAnimationValue& aValue) const;
> +    void AppendToString(nsAString& aString) const;
> +
> +    /* void SetOMValue(OMValue& aValue) const; */

Do we need to keep this here?  If it's a hint to the reader that later we'll want to use CSSComputedValue as part of our CSS Typed OM implementation, then better just to mention this in the comment above the class, rather than mention types like "OMValue" which may or may not exist once we come to implement it.

::: layout/style/CSSComputedValue.h:58
(Diff revision 5)
> +      // <length>
> +      nscoord,
> +      // <percentage>
> +      float,
> +      // <length-percentage>
> +      LengthPercentage,

Per comment 124, is it possible to use nsStyleCoord to represent these types?  Then we won't need the LengthPercentage struct.  (It might mean we need to distinguish between a <length-percentage> length and a <length-percentage> percentage and a <length-percentage> calc value in the CSSValueType.)

::: layout/style/CSSComputedValue.h:117
(Diff revision 5)
> +    nsTArray<SingleTerm> mTerms;
> +  };
> +
> +  // Note: the type of all the SingleTerms must be the same!
> +  // See + syntax in Properties & Values API.
> +  static ListTerm List(nsTArray<SingleTerm> aTerms);

Either make this take a |const nsTArray<SingleTerm>&|, so we don't copy the array twice, or (preferably) make it |const nsTArray<SingleTerm>&&| so that we don't have to copy it at all.

::: layout/style/CSSComputedValue.cpp:16
(Diff revision 5)
> +class nsComputedDOMStyle
> +{
> +public:
> +  static void SetValueToStyleImage(const nsStyleImage&, nsROCSSPrimitiveValue*);
> +};

I think this might cause a problem if we happened to include nsComputedDOMStyle.h in the same translation unit (which, even if we don't happen to include it through the includes above, we might do due to unified compilation).  If you can't just #include "nsComputedDOMStyle.h", you might need to make them functions rather than static methods n nsComputedDOMStyle, so that you can declare them in here (or have them in a nsComputedDOMStyleUtils.h or something).

::: layout/style/CSSComputedValue.cpp:118
(Diff revision 5)
> +                              eCSSUnit_Function);
> +    list->mNext = nullptr;
> +    // <transform-function> is a single transform function, while normally
> +    // StyleAnimationValues has lists of transforms. This is why we have the
> +    // weird single-element list.
> +    RefPtr<nsCSSValueSharedList> sharedList = new nsCSSValueSharedList { list };

Nit: I don't think we tend to use uniform initialization syntax unless there's a reason we need to.  Also feel free to just write:

  aValue.SetTransformValue(new nsCSSValueSharedList(list));

like we do elsewhere in StyleAnimationValue.cpp, which avoids an AddRef/Release pair.

::: layout/style/CSSComputedValue.cpp:134
(Diff revision 5)
> +    }
> +    // const nsStyleGradient* gradient = image.GetGradientData();
> +    // XXXjyc Implement support for CSS Gradients.
> +    // (They can be faked using references to animated variables, and it doesn't
> +    // seem like any other property needs them right now -- no support in
> +    // StyleAnimtaionValue)

*StyleAnimationValue

::: layout/style/CSSComputedValue.cpp:137
(Diff revision 5)
> +    // (They can be faked using references to animated variables, and it doesn't
> +    // seem like any other property needs them right now -- no support in
> +    // StyleAnimtaionValue)
> +    return false;
> +  }
> +  case CSSValueType::URL:

StyleAnimationValue supports URL values.  Why don't we support setting it here?

Also what's the reason for just returning false for these remaining types?  Can you add a comment stating why?

::: layout/style/CSSComputedValue.cpp:199
(Diff revision 5)
> +      // gradients, so we compute arguments.
> +      // AppendToString gives us the specified URL value, but we should
> +      // absolutize it.
> +      nsIURI* uri = mData.as<nsCSSValue>().GetURLValue();
> +      nsAutoCString spec;
> +      MOZ_ALWAYS_FALSE(NS_FAILED(uri->GetSpec(spec)));

Let's be more positive. :-)

$ git grep 'MOZ_ALWAYS_FALSE.NS_FAILED' | wc -l
0
$ git grep 'MOZ_ALWAYS_TRUE.NS_SUCCEEDED' | wc -l
5

Actually, we could just use:

  MOZ_ALWAYS_SUCCEEDS(uri->GetSpec(...));

But why can't we just use nsCSSValue::AppendToString here?

::: layout/style/CSSComputedValue.cpp:215
(Diff revision 5)
> +  case CSSValueType::URL: {
> +    nsIURI* uri = mData.as<nsCSSValue>().GetURLValue();
> +    nsAutoCString spec;
> +    MOZ_ALWAYS_FALSE(NS_FAILED(uri->GetSpec(spec)));
> +    aString.AppendLiteral("url(\"");
> +    aString.Append(NS_ConvertUTF8toUTF16(spec));
> +    aString.AppendLiteral("\")");
> +    break;
> +  }

Same here, can we just use nsCSSValue::AppendToString?

::: layout/style/CSSComputedValue.cpp:257
(Diff revision 5)
> +/* static */ CSSComputedValue::SingleTerm
> +CSSComputedValue::Length(nscoord aLength)
> +{
> +  SingleTerm term;
> +  term.mType = CSSValueType::Length;
> +  term.mData = CSSComputedValue::SingleTerm::Data(aLength);

We can leave off the "CSSComputedValue::" prefix here and in the rest of this file.

::: layout/style/CSSComputedValue.cpp:261
(Diff revision 5)
> +/* static */ CSSComputedValue::SingleTerm
> +CSSComputedValue::Number(nsCSSValue aValue)
> +{
> +  SingleTerm term;

For the methods that take nsCSSValues, can we assert that their units have expected values?  Also, we should probably prefer to use |const nsCSSValue&| for the argument (though perhaps the compiler would optimize one of the copies away).

::: layout/style/CSSComputedValue.cpp:302
(Diff revision 5)
> +  term.mData = CSSComputedValue::SingleTerm::Data(aValue);
> +  return term;
> +}
> +
> +/* static */ CSSComputedValue::SingleTerm
> +CSSComputedValue::Image(nsStyleImage aImage)

const nsStyleImage&

::: layout/style/CSSComputedValue.cpp:409
(Diff revision 5)
> +{
> +  // <syntax>+ guarantees at least one item.
> +  // If an empty string was used:
> +  //   --property: ;
> +  // ... then that would have to have been a token stream.
> +  MOZ_ASSERT(aTerms.Length() > 0);

!aTerms.IsEmpty()

::: layout/style/CSSComputedValue.cpp:415
(Diff revision 5)
> +    if (aTerms[i].GetType() != type) {
> +      MOZ_ASSERT_UNREACHABLE("All list term types must be the same.");

Maybe just:

  MOZ_ASSERT(aTerms[i].GetType() == type,
             "All list term types ...");

::: layout/style/CSSComputedValue.cpp:442
(Diff revision 5)
> +  nsTArray<StyleAnimationValue>* list =
> +    new nsTArray<StyleAnimationValue>(mTerms.Length());

Use UniquePtr to avoid leaking when we return false.

::: layout/style/CSSComputedValue.cpp:459
(Diff revision 5)
> +  // nsCSSParser has a much fancier thing based on the adjoining tokens.
> +  // Is that needed here?

I think for the types we allow in lists, just writing a space between each item is fine.

::: layout/style/CSSComputedValue.cpp:476
(Diff revision 5)
> +// CSSComputedValue
> +CSSComputedValue::CSSComputedValue(CSSComputedValue::SingleTerm aTerm)
> +  : mData(aTerm)
> +{ }
> +
> +CSSComputedValue::CSSComputedValue(ListTerm aTerms)
> +  : mData(aTerms)
> +{ }

Should these take move references, to avoid unnecessary copying?

::: layout/style/CSSComputedValue.cpp:491
(Diff revision 5)
> +CSSComputedValue::IsList() const
> +{
> +  return mData.is<ListTerm>();
> +}
> +
> +const CSSComputedValue::SingleTerm

Should this (and GetList) return a const reference?

::: layout/style/CSSValueType.h:12
(Diff revision 5)
> +#ifndef mozilla_CSSValueType_h
> +#define mozilla_CSSValueType_h
> +
> +namespace mozilla {
> +
> +enum class CSSValueType

Please add a comment describing what this represents.
Attachment #8773496 - Flags: review?(cam) → review-
Comment on attachment 8773497 [details]
Bug 1273706 - Part 10: Expose StyleAnimationValue::StyleCoordToValue and StyleCoordToCSSValue.

https://reviewboard.mozilla.org/r/66154/#review66306

::: layout/style/CSSComputedValues.h:35
(Diff revision 5)
> +  nsDataHashtable<nsStringHashKey, size_t> mValueIDs;
> +  nsTArray<CSSComputedValue> mValues;

It looks like you aren't exposing typed custom properties on nsComputedDOMStyle objects, which is what the integer IDs are used for in CSSVariableValues, and which you aren't using here.  So probably you will need to add some methods like CSSVariableValues' to get typed custom property values at a particular index.

::: layout/style/CSSComputedValues.cpp:6
(Diff revision 5)
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/CSSComputedValues.h"

Nit: the style guide says to keep the main header (CSSComputedValues.h here) separated from the rest.

::: layout/style/CSSVariableValues.cpp
(Diff revision 5)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /* computed CSS Variable values */
>  
>  #include "CSSVariableValues.h"
> -

Don't drop this blank line.
Attachment #8773497 - Flags: review?(cam) → review-
(Assignee)

Comment 164

11 months ago
https://reviewboard.mozilla.org/r/66152/#review66284

> Per comment 124, is it possible to use nsStyleCoord to represent these types?  Then we won't need the LengthPercentage struct.  (It might mean we need to distinguish between a <length-percentage> length and a <length-percentage> percentage and a <length-percentage> calc value in the CSSValueType.)

I tried doing that, but nsStyleCoord can represent a much larger variety of data than we need here. Using nsCSSValue for values equal to their specified values makes sense to me because then we don't have to rewrite AppendToString for them, but if we used nsStyleCoord here, we wouldn't get any benefit like that -- I'd still have to write AppendTostring. Plus, nsStyleCoord doesn't store whether or not we had any lengths in the initial expression, only mHasPercent, and we need to keep track of that according to the computation rule in the spec. We don't even get to use it to save work when setting a StyleAnimationValue, beacuse the internal calc object is in a different format. It seems like it would be making things a lot more complicated for little benefit.

If you are strongly in favor of it, though, it's not that much difference in the end, so I can do that.

> Let's be more positive. :-)
> 
> $ git grep 'MOZ_ALWAYS_FALSE.NS_FAILED' | wc -l
> 0
> $ git grep 'MOZ_ALWAYS_TRUE.NS_SUCCEEDED' | wc -l
> 5
> 
> Actually, we could just use:
> 
>   MOZ_ALWAYS_SUCCEEDS(uri->GetSpec(...));
> 
> But why can't we just use nsCSSValue::AppendToString here?

As the comment says, AppendToString gives us exactly the specified string, see:

- CSSParserImpl::SetValueToURL (called by ParseVariant, ParseImageRect, ParseFontSrc, with exactly mToken.mIdent)
- URLValue constructor (passes the string to URLValueData)
- URLValueData constructor (sets mString to that)

And AppendToString calls GetOriginalURLValue(), which gives that mString.

uri->GetSpec() (which doesn't have a comment :() seemed the best way to get the absolutized URL value, including the base URI etc. that nsCSSParser sets.

Originally I just used the specified value, then after talking with Tab in #houdini, he said:

12:00:30 [TabAtkins] jyc: That's just referring to the same concept in other CSS specs, where the computed value is just the specified value.
12:00:54 [TabAtkins] (Tho that's wrong here; units and urls need to be absolutized during computed-value.)

So that is what I am working off for the time being.

Do you think this seems ok?
https://reviewboard.mozilla.org/r/66152/#review66284

> I tried doing that, but nsStyleCoord can represent a much larger variety of data than we need here. Using nsCSSValue for values equal to their specified values makes sense to me because then we don't have to rewrite AppendToString for them, but if we used nsStyleCoord here, we wouldn't get any benefit like that -- I'd still have to write AppendTostring. Plus, nsStyleCoord doesn't store whether or not we had any lengths in the initial expression, only mHasPercent, and we need to keep track of that according to the computation rule in the spec. We don't even get to use it to save work when setting a StyleAnimationValue, beacuse the internal calc object is in a different format. It seems like it would be making things a lot more complicated for little benefit.
> 
> If you are strongly in favor of it, though, it's not that much difference in the end, so I can do that.

I feel like we can get more code re-use benefit here than might otherwise be apparent, by storing calc(1px) and calc(1%) values as CSSValueType::Length and CSSValueType::Percentage, rather than as CSSValueType::LengthPercentage and then checking whether we need to serialize as a calc() or as a single value.  Then we wouldn't need a new type to represent calc() values, and would allow us to re-use the serialization code that is in nsComputeDOMStyle::SetValueToCoord (which would need to be extracted and exposed).  If we store more value types as nsStyleCoords (Integer, too), then in CSSComputedValue::SingleTerm::SetAnimationValue we could have a new setter on StyleAnimationValue that takes an nsStyleCoord and passes it to StyleCoordToValue, and we could handle them all with one case in SetAnimationValue.

Is there any problem with the LengthPercentage constructor being able to create CSSValueType::Length and CSSValueType::Percentage values too, depending on what was in the calc()?  WDYT?

> As the comment says, AppendToString gives us exactly the specified string, see:
> 
> - CSSParserImpl::SetValueToURL (called by ParseVariant, ParseImageRect, ParseFontSrc, with exactly mToken.mIdent)
> - URLValue constructor (passes the string to URLValueData)
> - URLValueData constructor (sets mString to that)
> 
> And AppendToString calls GetOriginalURLValue(), which gives that mString.
> 
> uri->GetSpec() (which doesn't have a comment :() seemed the best way to get the absolutized URL value, including the base URI etc. that nsCSSParser sets.
> 
> Originally I just used the specified value, then after talking with Tab in #houdini, he said:
> 
> 12:00:30 [TabAtkins] jyc: That's just referring to the same concept in other CSS specs, where the computed value is just the specified value.
> 12:00:54 [TabAtkins] (Tho that's wrong here; units and urls need to be absolutized during computed-value.)
> 
> So that is what I am working off for the time being.
> 
> Do you think this seems ok?

Ah, you are right that we should be serializing the absolute URI here.  But maybe we should already be storing the absolute URI in ComputeCSSValueTerm, by calling GetURLValue() on the URLValue to get an nsIURI?  And then store that in the Variant as an nsIURI* (or, now that bug 652991 has landed, a FragmentOrURL value).  Does that make sense?
(Assignee)

Comment 166

11 months ago
https://reviewboard.mozilla.org/r/66154/#review66306

> It looks like you aren't exposing typed custom properties on nsComputedDOMStyle objects, which is what the integer IDs are used for in CSSVariableValues, and which you aren't using here.  So probably you will need to add some methods like CSSVariableValues' to get typed custom property values at a particular index.

You are right!
(Assignee)

Comment 167

11 months ago
https://reviewboard.mozilla.org/r/66152/#review66284

> Should this (and GetList) return a const reference?

I don't see any reason why not! Changing it.
(Assignee)

Comment 168

11 months ago
https://reviewboard.mozilla.org/r/66154/#review66306

> You are right!

Actually, it looks like nsComputedDOMStyle::IndexedGetter is just so that we can get the property name at a given index (https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-item). Because the variables we will store on the CSSComputedValues are a subset of those we store on CSSVariableValues and because the order isn't otherwise exposed, I think we should be OK? Users can just use item() to get the name from the CSSVariableValues and then use getPropertyValue to read from CSSComputedValues.

What do you think?
(Assignee)

Comment 169

11 months ago
(In reply to Cameron McCormack (:heycam) from comment #165)

> I feel like we can get more code re-use benefit here than might otherwise be
> apparent, by storing calc(1px) and calc(1%) values as CSSValueType::Length
> and CSSValueType::Percentage, rather than as CSSValueType::LengthPercentage
> and then checking whether we need to serialize as a calc() or as a single
> value.  Then we wouldn't need a new type to represent calc() values, and
> would allow us to re-use the serialization code that is in
> nsComputeDOMStyle::SetValueToCoord (which would need to be extracted and
> exposed).  If we store more value types as nsStyleCoords (Integer, too),
> then in CSSComputedValue::SingleTerm::SetAnimationValue we could have a new
> setter on StyleAnimationValue that takes an nsStyleCoord and passes it to
> StyleCoordToValue, and we could handle them all with one case in
> SetAnimationValue.

I think nsDOMComputedStyle::SetValueToCoord creates a nsROCSSPrimitiveValue. Wouldn't this actually lead to more code, because then we would have to add code to extract values from nsROCSSPrimitiveValues and convert between CSS_* units and StyleAnimationValue::Units? (In addition to adding the new setter to StyleAnimationValue).

It also looks like we would have to change how SetValueToCalc in nsComputedDOMStyle to only print the length component if there was a length component in the specified value, as Properties & Values says. But it looks like the current behavior doesn't follow CSS Values' specification for serialization of computed calc() values (which is apparently 'still under discussion', but I can't see the discussion) anyways.

If you are OK with this, then I am OK with using nsStyleCoord.

> Is there any problem with the LengthPercentage constructor being able to
> create CSSValueType::Length and CSSValueType::Percentage values too,
> depending on what was in the calc()?  WDYT?

I don't have a problem with this!

> Ah, you are right that we should be serializing the absolute URI here.  But
> maybe we should already be storing the absolute URI in ComputeCSSValueTerm,
> by calling GetURLValue() on the URLValue to get an nsIURI?  And then store
> that in the Variant as an nsIURI* (or, now that bug 652991 has landed, a
> FragmentOrURL value).  Does that make sense?

OK, that sounds good.
(Assignee)

Comment 170

11 months ago
> ::: layout/style/CSSComputedValue.cpp:137
> (Diff revision 5)
> > +    // (They can be faked using references to animated variables, and it doesn't
> > +    // seem like any other property needs them right now -- no support in
> > +    // StyleAnimtaionValue)
> > +    return false;
> > +  }
> > +  case CSSValueType::URL:
> 
> StyleAnimationValue supports URL values.  Why don't we support setting it here?
> 
> Also what's the reason for just returning false for these remaining types?  Can you add a comment stating why?

I did some more tests and it looks like this is actually incorrect -- it doesn't cause the 50% flip for all registered custom property values, even if they are uninterpolable, as the spec requires. [1] Not sure why I thought that, sorry. Will rework this. Should just have StyleAnimationValue use URL values for URL values and unparsed string for other non-interpolable values.

[1]: https://drafts.css-houdini.org/css-properties-values-api/#animation-behavior-of-custom-properties

Also just realized a problem with missing keyframe values and custom properties with no initial values. It looks like this is a special case, because with regular animatable properties we can always fill a missing value using a style context without the animation? (CSSAnimationBuilder::GetComputedValue). Will update FillInMissingKeyframeValues to reflect this for custom properties.
(In reply to Jonathan Chan [:jyc] from comment #170)
> I did some more tests and it looks like this is actually incorrect -- it
> doesn't cause the 50% flip for all registered custom property values, even
> if they are uninterpolable, as the spec requires. [1] Not sure why I thought
> that, sorry. Will rework this. Should just have StyleAnimationValue use URL
> values for URL values and unparsed string for other non-interpolable values.

Bug 1277433 is adding support for animations flipping at 50%, so you might have to wait until that is done before getting this to work.  Then it should just be a matter of setting the non-interpolable value as an nsCSSValue on the StyleAnimationValue.
https://reviewboard.mozilla.org/r/66154/#review66306

> Actually, it looks like nsComputedDOMStyle::IndexedGetter is just so that we can get the property name at a given index (https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-item). Because the variables we will store on the CSSComputedValues are a subset of those we store on CSSVariableValues and because the order isn't otherwise exposed, I think we should be OK? Users can just use item() to get the name from the CSSVariableValues and then use getPropertyValue to read from CSSComputedValues.
> 
> What do you think?

Ah, I didn't realise that we have all variables stored on CSSVariableValues, and that CSSComputedValues is just a subset of those (with types).  In that case, just using the CSSVariableValues to expose things on the computed DOM style sounds fine.
(Reporter)

Updated

11 months ago
Attachment #8773497 - Flags: review- → review+
Comment on attachment 8773497 [details]
Bug 1273706 - Part 10: Expose StyleAnimationValue::StyleCoordToValue and StyleCoordToCSSValue.

https://reviewboard.mozilla.org/r/66154/#review66618

r=me with the other comments addressed.
(In reply to Jonathan Chan [:jyc] from comment #169)
> I think nsDOMComputedStyle::SetValueToCoord creates a nsROCSSPrimitiveValue.
> Wouldn't this actually lead to more code, because then we would have to add
> code to extract values from nsROCSSPrimitiveValues and convert between CSS_*
> units and StyleAnimationValue::Units? (In addition to adding the new setter
> to StyleAnimationValue).

Ah, yes, my mistake.  For calc() values we do generate the nsROCSSPrimitiveValue as a string, so that at least could be re-used, but for the other types it's setting the values directly on the nsROCSSPrimitiveValue.

Having to create an actual nsROCSSPrimitiveValue object isn't great, I agree, though it is what nsComputedDOMStyle does normally when exposing strings to script.  (It does have the GetCssText() method, so we still wouldn't need to add a new serialization method.)

> It also looks like we would have to change how SetValueToCalc in nsComputedDOMStyle to only print
> the length component if there was a length component in the specified value, as Properties & Values
> says. But it looks like the current behavior doesn't follow CSS Values' specification for
> serialization of computed calc() values (which is apparently 'still under discussion', but I can't
> see the discussion) anyways.

We wouldn't have to do that if we store calc(1px) and calc(1%) values as CSSValueType::Length and CSSValueType::Percentage, would we?


So how about this:

* Make calc(1px) and calc(1%) values use CSSValueType::Length and CSSValueType::Percentage, and leave CSSValueType::LengthPercentage (possibly renamed to something else) just for calc() values that have both specified

* Use nsStyleCoord to store Length, Percentage, LengthPercentage, Integer

* Serialize LengthPercentage by factoring out the serialization part of SetValueToCalc in nsComputedDOMStyle.cpp into a function we can call

* Serialize Length, Percentage and Integer as we are doing currently, by grabbing the value off the nsStyleCoord (happily these serializations are fairly simple)

* Handle URL values as discussed above

* Leave the current nsCSSValue storage/serialization for all the other types

* Handle all of the nsStyleCoord-stored values in SetAnimationValue by calling a new method that uses StyleCoordToValue internally

* Handle all the remaining types in SetAnimationValue as you are doing currently

Let me know if you think this is still a net win.
(Assignee)

Comment 175

11 months ago
(In reply to Cameron McCormack (:heycam) from comment #174)
> (In reply to Jonathan Chan [:jyc] from comment #169)
> We wouldn't have to do that if we store calc(1px) and calc(1%) values as
> CSSValueType::Length and CSSValueType::Percentage, would we?

OK, I see what you mean. You're right.

> So how about this:
> 
> ...
> 
> Let me know if you think this is still a net win.

Sounds good. I will try this tomorrow and see how it goes.
(Reporter)

Comment 176

11 months ago
mozreview-review
Comment on attachment 8773503 [details]
Bug 1273706 - Part 16: Store custom property registrations on nsCSSParser.

https://reviewboard.mozilla.org/r/66166/#review67124

How do we ensure that @supports conditions get re-evaluated when registrations change?  I think all current @supports conditions do not depend on anything in the document, and unlike @media queries, we don't have a mechanism for restyling when conditions change.

Might it make sense to record somewhere which custom properties we evaluated in @supports conditions (on the style sheet maybe?) and just restyle the entire document if any of those change?  That's a bit of a big hammer, but it's probably the easiest thing to do.
(Reporter)

Comment 177

11 months ago
mozreview-review
Comment on attachment 8773499 [details]
Bug 1273706 - Part 14: Store custom property registrations on the document.

https://reviewboard.mozilla.org/r/66158/#review67134

Mostly OK, some comments below.

::: dom/base/nsGlobalWindow.h:1928
(Diff revision 5)
> +  // CSS Properties & Values API support.
> +  // Tentatively the custom property registrations here, so that they are
> +  // window-scoped.
> +  RefPtr<mozilla::CSSVariableRegistrations> mCSSVariableRegistrations;

Can we put this on nsIDocument instead?  Having this on nsGlobalWindow means some slightly odd things (I believe), such as being able to have variable registrations persist when you start with an <iframe> and its initial about:blank document, do the registrations, then navigate the document.

Also, not public would be good. :-)

::: layout/style/CSSVariableRegistration.h:22
(Diff revision 5)
> +#include "nsISupportsImpl.h"
> +
> +class nsIAtom;
> +class nsStyleContext;
> +
> +struct VariableExprContext

Put this inside the |namespace mozilla|.

::: layout/style/CSSVariableRegistration.h:35
(Diff revision 5)
> +  nsIURI* mSheetURL;
> +  nsIURI* mBaseURL;
> +  nsIPrincipal* mSheetPrincipal;

These should be nsCOMPtrs.  I'm not confident that we are guaranteed to hang on to them all.

::: layout/style/CSSVariableRegistration.h:52
(Diff revision 5)
> +  //  value used [sic]. This initial value must be considered parseable by
> +  //  registerProperty() but invalid at computed value time."
> +  // See: https://drafts.css-houdini.org/css-properties-values-api/#the-registerproperty-function
> +  // We use the empty string.
> +  // See also:
> +  // - CSSVariableResolver::VisitNod

*VisitNode

::: layout/style/CSSVariableRegistration.h:54
(Diff revision 5)
> +  nsString mInitialExpr;
> +  nsCSSValue mInitialValue;

Why do we need to store the initial value as a string (in addition to the nsCSSValue)?

::: layout/style/CSSVariableRegistration.h:56
(Diff revision 5)
> +  // See also:
> +  // - CSSVariableResolver::VisitNod
> +  // - nsCSSParser::ResolveValueWithVariableReferencesRec
> +  nsString mInitialExpr;
> +  nsCSSValue mInitialValue;
> +  CSSValueType mInitialType;

What if the initial value is a list?

::: layout/style/CSSVariableRegistration.h:63
(Diff revision 5)
> +  VariableExprContext mContext;
> +
> +  // These end up being used inside of CSSParserImpl::ResolveVariableValues.
> +  // It calls AppendTokens to construct the resulting expanded value, and needs
> +  // to know whether to insert /**/ between adjacent token.
> +  nsCSSTokenSerializationType mFirstToken;
> +  nsCSSTokenSerializationType mLastToken;

Are these variables only for the initial value?  If so, it is probably worth putting "Initial" in their names.

::: layout/style/CSSVariableRegistration.h:93
(Diff revision 5)
> +{
> +  typedef nsClassHashtable<nsStringHashKey, CSSVariableRegistration> Data;
> +
> +  NS_INLINE_DECL_REFCOUNTING(CSSVariableRegistrations);
> +
> +  unsigned int mVersion = 0;

Similar variables in other places are referred to as "generations", so let's call this mGeneration.  I think it's uncommon to use platform specific sized integers like this in Gecko code, so just make it uint32_t.

::: layout/style/CSSVariableRegistration.h:100
(Diff revision 5)
> +RefPtr<CSSVariableRegistrations>
> +CSSVariableRegistrationsOfDocument(const nsIDocument* aDoc);
> +
> +RefPtr<CSSVariableRegistrations>

Make these return raw pointers; if the caller needs to hold a strong reference it can assign it to a RefPtr.
Attachment #8773499 - Flags: review?(cam) → review-
(Reporter)

Comment 178

11 months ago
mozreview-review
Comment on attachment 8773500 [details]
Bug 1273706 - Part 13: Add CSSVariableSyntax for representing the syntax of a custom property.

https://reviewboard.mozilla.org/r/66160/#review67470

::: layout/style/nsCSSParser.h:21
(Diff revision 5)
> +#include "mozilla/CSSVariableRegistration.h"
> +

Nit: Maybe put this up with the other #include "mozilla/..." lines.

::: layout/style/nsCSSParser.h:331
(Diff revision 5)
> +  void SetVariableRegistrations(
> +      RefPtr<const mozilla::CSSVariableRegistrations>
> +      aRegistrations);

Make this take a raw pointer (so we can avoid an extra AddRef/Release pair).  Or make it RefPtr<...>&& or already_AddRefed<...>,, since you are passing Move(...) at the call sites.  But a raw pointer is fine, especially if you use a raw pointer for ParsingInfo::mRegistrations, which should be fine for the same reasons that it's fine to use raw pointers for the URIs/principal there.

::: layout/style/nsCSSParser.cpp:142
(Diff revision 5)
>    ~CSSParserImpl();
>  
>    nsresult SetStyleSheet(CSSStyleSheet* aSheet);
>  
>    nsIDocument* GetDocument();
> +  RefPtr<const CSSVariableRegistrations> GetCustomPropRegs();

Just return a raw pointer.
Attachment #8773500 - Flags: review?(cam) → review+
(Reporter)

Comment 179

11 months ago
mozreview-review
Comment on attachment 8773511 [details]
Bug 1273706 - Part 24: Set initial values for custom properties on the root.

https://reviewboard.mozilla.org/r/66182/#review67474

::: layout/style/MaybeCustomPropertySet.h:5
(Diff revision 5)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* bit vectors for sets of CSS properties */

Update this file comment (which is shown in DXR directory listings) to mention custom properties.

::: layout/style/MaybeCustomPropertySet.h:10
(Diff revision 5)
> +#include <limits.h> // for CHAR_BIT
> +
> +#include "mozilla/ArrayUtils.h"

I don't think we need these two.  (We don't use them directly in this header.)

::: layout/style/MaybeCustomPropertySet.h:24
(Diff revision 5)
> +namespace mozilla {
> +
> +/**
> + * The same as nsCSSPropertySet, but also capable of holding custom properties.
> + */
> +class MaybeCustomPropertySet {

This might get renamed according to the discussion we had the other day about MaybeCustomProperty?

::: layout/style/MaybeCustomPropertySet.h:26
(Diff revision 5)
> +/**
> + * The same as nsCSSPropertySet, but also capable of holding custom properties.
> + */
> +class MaybeCustomPropertySet {
> +public:
> +    MaybeCustomPropertySet() = default;

Nit: two space indent.

::: layout/style/MaybeCustomPropertySet.h:37
(Diff revision 5)
> +    typedef void (*IterFunc)(const RefPtr<nsIAtom>&, void*);
> +    void IterCustomProps(IterFunc aFunc, void* aData);

Maybe simpler just to return mCustomProps.ConstIter()?

::: layout/style/MaybeCustomPropertySet.h:42
(Diff revision 5)
> +    typedef void (*IterFunc)(const RefPtr<nsIAtom>&, void*);
> +    void IterCustomProps(IterFunc aFunc, void* aData);
> +
> +private:
> +    nsCSSPropertySet mFixedProps;
> +    nsDataHashtable<nsUint64HashKey, RefPtr<nsIAtom>> mCustomProps;

Use nsPtrHashKey<nsIAtom> instead of nsUint64HashKey.  Then we can avoid the casts in AddProperty etc., and avoid storing more data than needed in 32 bit builds.

Use nsCOMPtr for strong references to interfaces, and RefPtr for strong references to concrete classes.

::: layout/style/MaybeCustomPropertySet.cpp:13
(Diff revision 5)
> +
> +namespace mozilla {
> +
> +MaybeCustomPropertySet::MaybeCustomPropertySet(const MaybeCustomPropertySet& aOther)
> +{
> +  mFixedProps = aOther.mFixedProps;

Put this in an initializer to avoid the memset() from the nsCSSPropertySet default constructor.

::: layout/style/MaybeCustomPropertySet.cpp:20
(Diff revision 5)
> +    mCustomProps.Put(iter.Key(), iter.UserData());
> +  }
> +}
> +
> +void
> +MaybeCustomPropertySet::AddProperty(MaybeCustomProperty aProperty) {

Nit: "{" on new line (and in the other methods in this file).
Attachment #8773511 - Flags: review?(cam) → review+
(Reporter)

Comment 180

11 months ago
mozreview-review
Comment on attachment 8773503 [details]
Bug 1273706 - Part 16: Store custom property registrations on nsCSSParser.

https://reviewboard.mozilla.org/r/66166/#review67504

r=me but please add support for re-evaluating @supports rules when property registrations change.  (Either in this patch, and re-request review, or in a separate patch.)

::: layout/style/nsCSSParser.cpp:2398
(Diff revision 5)
>      MOZ_ASSERT(Substring(aProperty, 0,
>                           CSS_CUSTOM_NAME_PREFIX_LENGTH).EqualsLiteral("--"));
>      const nsDependentSubstring varName =
>        Substring(aProperty, CSS_CUSTOM_NAME_PREFIX_LENGTH);  // remove '--'
> +    // remove '--'
> +    RefPtr<const CSSVariableRegistrations> registrations = GetCustomPropRegs();
> +    CSSVariableRegistration* registration;
> +    if (registrations && registrations->mData.Get(varName, &registration)) {
> +      nsCSSValue value;
> +      mozilla::CSSValueType type;
> +      // Allow variable references.
> +      // layout/reftests/w3c-css/submitted/variables/variable-supports-33.html
> +      parsedOK = ParseTypedValue(registration->mSyntax,
> +                                 /* aAllowVariableReferences = */ true,
> +                                 value, type);
> +    } else {
> -    CSSVariableDeclarations::Type variableType;
> +      CSSVariableDeclarations::Type variableType;
> -    nsString variableValue;
> +      nsString variableValue;
> -    parsedOK = ParseVariableDeclaration(&variableType, true, variableValue) &&
> +      parsedOK = ParseVariableDeclaration(&variableType, true, variableValue) &&
> -               !GetToken(true);
> +                 !GetToken(true);
> +    }

This block is pretty much the same as the one in ParseSupportsConditionInParensInsideParens.  Can you factor it out into a function?

::: layout/style/nsCSSParser.cpp:2402
(Diff revision 5)
>    if (propID == eCSSPropertyExtra_variable) {
>      MOZ_ASSERT(Substring(aProperty, 0,
>                           CSS_CUSTOM_NAME_PREFIX_LENGTH).EqualsLiteral("--"));
>      const nsDependentSubstring varName =
>        Substring(aProperty, CSS_CUSTOM_NAME_PREFIX_LENGTH);  // remove '--'
> +    // remove '--'

Duplicate comment?

::: layout/style/nsCSSParser.cpp:2409
(Diff revision 5)
> +    CSSVariableRegistration* registration;
> +    if (registrations && registrations->mData.Get(varName, &registration)) {
> +      nsCSSValue value;
> +      mozilla::CSSValueType type;
> +      // Allow variable references.
> +      // layout/reftests/w3c-css/submitted/variables/variable-supports-33.html

Probably don't need the reference to the specific test here.  (That test really applies to other branch of the if statement, for non-registered custom properties, anyway.)
Attachment #8773503 - Flags: review?(cam) → review+
(Reporter)

Comment 181

11 months ago
mozreview-review
Comment on attachment 8773513 [details]
Bug 1273706 - Part 29: Implement animations for custom properties.

https://reviewboard.mozilla.org/r/66186/#review67478

I've given you lots of comments, sorry, with a few questions in here.  But the approach looks OK.

::: dom/animation/AnimValuesStyleRule.h:42
(Diff revision 5)
> +    if (aProperty.IsCustom()) {
> +      mStyleBits |= nsCachedStyleData::GetBitForSID(eStyleStruct_Variables);
> +    } else {
> -    mStyleBits |=
> +      mStyleBits |=
> -      nsCachedStyleData::GetBitForSID(nsCSSProps::kSIDTable[aProperty]);
> +        nsCachedStyleData::GetBitForSID(
> +          nsCSSProps::kSIDTable[aProperty.AsFixed()]);
> +    }

Maybe add a method on MaybeCustomProperty that can return its appropriate nsStyleStructID.  Then we can avoid having two branches in these two AddValue methods.

::: dom/animation/AnimValuesStyleRule.cpp:55
(Diff revision 5)
> +      if (!(aRuleData->mSIDs & NS_STYLE_INHERIT_BIT(Variables))) {
> +        return;
> +      }

Then this aRuleData->mSIDs check can be done outside of the if statement too.

::: dom/animation/KeyframeEffect.cpp:500
(Diff revision 5)
> -    if (aProperty == mProperties[propIdx].mProperty) {
> +    if (mProperties[propIdx].mProperty.IsFixed() &&
> +        aProperty == mProperties[propIdx].mProperty.AsFixed()) {

Since you added an operator== to MaybeCustomProperty, this can just be:

  if (mProperties[propIdx].mProperty == aProperty)

::: dom/animation/KeyframeEffect.cpp:581
(Diff revision 5)
>      return;
>    }
>  
>    // Preserve the state of mWinsInCascade and mIsRunningOnCompositor flags.
> -  nsCSSPropertySet winningInCascadeProperties;
> -  nsCSSPropertySet runningOnCompositorProperties;
> +  MaybeCustomPropertySet winningInCascadeProperties;
> +  MaybeCustomPropertySet runningOnCompositorProperties;

Since custom property animations can't be run on the compositor, we can keep this as nsCSSPropertySet.

::: dom/animation/KeyframeEffect.cpp:1051
(Diff revision 5)
> +    if (property.mProperty.IsFixed()) {
> -    propertyDetails.mProperty =
> +      propertyDetails.mProperty =
> -      NS_ConvertASCIItoUTF16(nsCSSProps::GetStringValue(property.mProperty));
> +        NS_ConvertASCIItoUTF16(
> +          nsCSSProps::GetStringValue(property.mProperty.AsFixed()));
> +    } else {
> +      RefPtr<nsIAtom> custom = property.mProperty.AsCustom();
> +      propertyDetails.mProperty.AppendLiteral("--");
> +      nsAutoString name;
> +      custom->ToString(name);
> +      propertyDetails.mProperty.Append(name);
> +    }

It seems we get a property name like this (either as a UTF-8 or as a UTF-16 string) in a few different places.  Can we add methods (and maybe the UTF-8 one can be #ifdef DEBUG) to MaybeCustomProperty to return either the nsString / nsCString for the property's name?

::: dom/animation/KeyframeEffect.cpp:1163
(Diff revision 5)
> +        nsAutoString fullName, unprefixedName;
> +        RefPtr<nsIAtom> custom = propertyValue.mProperty.AsCustom();
> +        custom->ToString(unprefixedName);
> +        fullName.AppendLiteral("--");
> +        fullName.Append(unprefixedName);
> +        name = ToNewUTF8String(fullName);

Instead of allocating a new string, you could have an nsAutoCString declared next to |name|, and then do |name = thatNewVariable.get();| after appending the UTF-8 converted string to thatNewVariable.  Then we wouldn't have to manage memory manually.

::: dom/animation/KeyframeEffect.cpp:1172
(Diff revision 5)
>        // nsCSSValue::AppendToString does not accept shorthands properties but
>        // works with token stream values if we pass eCSSProperty_UNKNOWN as
>        // the property.
>        nsCSSProperty propertyForSerializing =
> -        nsCSSProps::IsShorthand(propertyValue.mProperty)
> +        propertyValue.mProperty.IsCustom()
> +        ? eCSSProperty_UNKNOWN

I think it's fine to use eCSSProperty_UNKNOWN here for custom properties, since it looks like for all the types we might set the nsCSSValue to for typed custom properties, we won't run into a case in nsCSSValue::AppendToString where we need to know the specific property.  However, can you update the comment above to say it's safe for this reason?

::: dom/animation/KeyframeEffectParams.cpp:146
(Diff revision 5)
> -      aPacedProperty == eCSSPropertyExtra_variable ||
> -      !KeyframeUtils::IsAnimatableProperty(aPacedProperty)) {
> -    aPacedProperty = eCSSProperty_UNKNOWN;
> +      CSSVariableRegistration* reg = nullptr;
> +      if (aRegistrations &&
> +          aRegistrations->mData.Get(Substring(identToken,
> +                                              CSS_CUSTOM_NAME_PREFIX_LENGTH),
> +                                    &reg)) {
> +        aPacedProperty = MaybeCustomProperty(reg->mAtom);
> +      } else {
> +        aPacedProperty = MaybeCustomProperty(eCSSProperty_UNKNOWN);

We do this turning a property name string into a MaybeCustomProperty in a couple of places -- at least here, and in nsRuleNode::ComputeDisplayData.

Can we add a method to nsCSSProps that takes the registrations as an argument (and the string and enabled state, like nsCSSProps::LookupProperty), and returns a MaybeCustomProperty?

::: dom/animation/KeyframeUtils.cpp:76
(Diff revision 5)
>    }
>  
> -  bool LessThan(nsCSSProperty aLhs,
> -                nsCSSProperty aRhs) const
> +  bool LessThan(MaybeCustomProperty aLhs,
> +                MaybeCustomProperty aRhs) const
>    {
> -    bool isShorthandLhs = nsCSSProps::IsShorthand(aLhs);
> +    if (aLhs.IsCustom() && aRhs.IsCustom()) {

Please add a comment saying that first we ensure that custom properties sort last, and that between two custom properties we just sort by their names.  Then in the comment below replace "First" with "Next".

::: dom/animation/KeyframeUtils.cpp:77
(Diff revision 5)
> -    bool isShorthandRhs = nsCSSProps::IsShorthand(aRhs);
> +      RefPtr<nsIAtom> left = aLhs.AsCustom();
> +      RefPtr<nsIAtom> right = aRhs.AsCustom();
> +      nsAutoString lefts, rights;
> +      left->ToString(lefts);
> +      right->ToString(rights);

No need to store these in strong references and AddRef/Release them, since we know the MaybeCustomProperty objects will hold on to the nsIAtoms for us.  So just write this a bit shorter as:

  nsString left, right;
  aLhs.AsCustom()->ToString(left);
  aRhs.AsCustom()->ToString(right);

I think nsAutoStrings support sharing string buffers (and so won't cause the characters to be copied in nsIAtom::ToString), but it won't use (and we don't want it to use) the auto-buffer.  So we can just use nsString here.

::: dom/animation/KeyframeUtils.cpp:274
(Diff revision 5)
> -             nsCSSProps::PropertyIDLNameSortPosition(aRhs.mProperty);
> +        RefPtr<nsIAtom> left = aLhs.mProperty.AsCustom();
> +        RefPtr<nsIAtom> right = aRhs.mProperty.AsCustom();
> +        nsAutoString lefts, rights;
> +        left->ToString(lefts);
> +        right->ToString(rights);
> +        return lefts < rights;

Hmm, might it make sense to pull this out into a static helper function?

::: dom/animation/KeyframeUtils.cpp:316
(Diff revision 5)
>               aLhs.mOffset == aRhs.mOffset;
>      }
>      static bool LessThan(const KeyframeValueEntry& aLhs,
>                           const KeyframeValueEntry& aRhs)
>      {
> +      if (aLhs.mProperty.IsCustom() && aRhs.mProperty.IsCustom()) {

Add a comment in here explaining our custom property order, and in the comment below replace "First" with "Then".

::: dom/animation/KeyframeUtils.cpp:375
(Diff revision 5)
> -  return !nsCSSProps::IsShorthand(aPair.mProperty) &&
> +  return aPair.mProperty.IsFixed() &&
> +         !nsCSSProps::IsShorthand(aPair.mProperty.AsFixed()) &&
>           aPair.mValue.GetUnit() == eCSSUnit_TokenStream &&
>           aPair.mValue.GetTokenStreamValue()->mPropertyID
>             == eCSSProperty_UNKNOWN;

Should we instead be doing:

  return (aPair.mProperty.IsCustom() ||
          !nsCSSProps::IsShorthand(...)) &&
         aPair.mValue.GetUnit() == ...

so that we will return true for custom properties with invalid values?  Is it still the case that we can use the mPropertyID == eCSSProperty_UNKNOWN to recognise invalid values?

::: dom/animation/KeyframeUtils.cpp:407
(Diff revision 5)
>  static bool
>  GetPropertyValuesPairs(JSContext* aCx,
>                         JS::Handle<JSObject*> aObject,
>                         ListAllowance aAllowLists,
> -                       nsTArray<PropertyValuesPair>& aResult);
> +                       nsTArray<PropertyValuesPair>& aResult,
> +                       RefPtr<const CSSVariableRegistrations> aRegistrations);

Raw pointer.

::: dom/animation/KeyframeUtils.cpp:924
(Diff revision 5)
> +    // XXXjyc Update this pending decision on mapping in
> +    // https://github.com/w3c/web-animations/issues/163

Looks like this is resolved now.

::: dom/animation/KeyframeUtils.cpp:927
(Diff revision 5)
>        return false;
>      }
> +    // XXXjyc Update this pending decision on mapping in
> +    // https://github.com/w3c/web-animations/issues/163
> +    if (nsCSSProps::IsCustomPropertyName(propName)) {
> +      CSSVariableRegistration* reg = nullptr;

Can you add a comment in here (or whereever you think is appropriate) pointing out that we will want to make unregistered custom properties discretely animatable, once bug 1277433 lands?

::: dom/animation/KeyframeUtils.cpp:1045
(Diff revision 5)
> -  if (!nsCSSProps::IsShorthand(aProperty)) {
> -    aParser.ParseLonghandProperty(aProperty,
> +  nsCSSProperty cssprop =
> +    aProperty.IsFixed() ? aProperty.AsFixed() : eCSSProperty_UNKNOWN;
> +  if (!nsCSSProps::IsShorthand(cssprop)) {

I think IsShorthand will assert with eCSSProperty_UNKNOWN.  Check IsFixed() in the if statement condition instead?

Also, I guess calling ParseLonghandProperty will fail and report an error to the console, so it would be better to skip doing that for custom properties.

Should we be trying to parse the custom property properly, though?  Or do we want to store a token stream even for typed registered custom properties?

::: dom/base/nsDOMWindowUtils.cpp:2674
(Diff revision 5)
>  
> -  nsCSSProperty property =
> +  MaybeCustomProperty property;
> +  nsCSSProperty fixed =
>      nsCSSProps::LookupProperty(aProperty, CSSEnabledState::eIgnoreEnabledState);
> -  if (property != eCSSProperty_UNKNOWN && nsCSSProps::IsShorthand(property)) {
> -    property = eCSSProperty_UNKNOWN;
> +  if (fixed == eCSSPropertyExtra_variable) {
> +    nsIDocument* doc = content->GetUncomposedDoc();

Should we use OwnerDoc() instead?  I don't think anything calls computeAnimationDistance() with an element that's not in the document, but it's easy to make it work if anyone does.

::: dom/base/nsDOMWindowUtils.cpp:2693
(Diff revision 5)
> +  } else {
> +    property = MaybeCustomProperty(fixed);
>    }
>  
>    MOZ_ASSERT(property == eCSSProperty_UNKNOWN ||
> -             !nsCSSProps::IsShorthand(property),
> +             !nsCSSProps::IsShorthand(property.AsFixed()),

Do we need to check property.IsFixed() too?

::: layout/base/nsDisplayList.cpp:430
(Diff revision 5)
>    animation->iterations() = computedTiming.mIterations;
>    animation->iterationStart() = computedTiming.mIterationStart;
>    animation->direction() = static_cast<uint32_t>(timing.mDirection);
> -  animation->property() = aProperty.mProperty;
> +  // Only certain properties can be animated on the compositor, and it doesn't
> +  // make sense to animate custom properties.
> +  MOZ_ASSERT(aProperty.mProperty.IsFixed());

I guess this same assertion will be checked by the AsFixed() call on the next line, so you could leave this one out.

::: layout/style/StyleAnimationValue.cpp:86
(Diff revision 5)
> -GetCommonUnit(nsCSSProperty aProperty,
> +GetCommonUnit(MaybeCustomProperty aProperty,
>                nsCSSUnit aFirstUnit,
>                nsCSSUnit aSecondUnit)
>  {
>    if (aFirstUnit != aSecondUnit) {
> -    if (nsCSSProps::PropHasFlags(aProperty, CSS_PROPERTY_STORES_CALC) &&
> +    if (aProperty.IsCustom() ||

Is this right?  I don't think all custom properties can be represented as a calc() value.  Do the parens need to go elsewhere?

Or is it that we can never get in here with say aFirstUnit == eCSSUnit_Pixel and aSecondUnit == eCSSUnit_EnumColor?  (If so some assertions are needed in here.)

::: layout/style/StyleAnimationValue.cpp:2347
(Diff revision 5)
> -      if (aProperty == eCSSProperty_font_weight) {
> +      if (aProperty.IsFixed() &&
> +          aProperty.AsFixed() == eCSSProperty_font_weight) {

Just |aProperty == eCSSProperty_font_weight|?

::: layout/style/StyleAnimationValue.cpp:2852
(Diff revision 5)
> +  } else {
> +    RefPtr<nsIAtom> custom = aProperty.AsCustom();
> +    nsString name;
> +    custom->ToString(name);
> +    parser.ParseVariable(name, aSpecifiedValue, doc->GetDocumentURI(),
> +                         baseURI, aTargetElement->NodePrincipal(), declaration,
> +                         &changed, false);
> +  }

The fixed property branch above checks to see if the property parsing succeded or not.  Do we need to do that here?  (I'm not sure when ParseProperty will fail here, since we should have valid values here already.)  If we expect ParseVariable to have succeeded, can you assert it?

::: layout/style/StyleAnimationValue.cpp:2873
(Diff revision 5)
> -  MOZ_ASSERT(!nsCSSProps::IsShorthand(aProperty),
> +  MOZ_ASSERT(aProperty.IsCustom() ||
> +             !nsCSSProps::IsShorthand(aProperty.AsFixed()),

(Since we have a lot of this code to check whether a MaybeCustomProperty is a shorthand, it might make sense to add an IsShorthand() method on MaybeCustomProperty to save the IsCustom/IsFixed check everwhere.)

::: layout/style/StyleAnimationValue.cpp:2902
(Diff revision 5)
> +    declaration
> +      ->AddVariableDeclaration(name, CSSVariableDeclarations::Type::TokenStream,

This is related to my comments about merging the two style structs -- but is there a reason why we can't / don't store specified typed custom property values using a non-token-stream nsCSSValue?  Is there just no advantage in doing that?

::: layout/style/StyleAnimationValue.cpp:2906
(Diff revision 5)
> +                               // OK for this to be null because no animated
> +                               // property needs the context.
> +                               VariableExprContext());

Will we need for discrete animation of url() values?

::: layout/style/StyleAnimationValue.cpp:2926
(Diff revision 5)
>                             nsTArray<PropertyStyleAnimationValuePair>& aValues,
>                             bool* aIsContextSensitive)
>  {
>    MOZ_ASSERT(aStyleContext);
> -  if (!nsCSSProps::IsEnabled(aProperty, aEnabledState)) {
> +  if ((aProperty.IsCustom() &&
> +       !nsLayoutUtils::CSSVariablesEnabled()) ||

I wonder if it's possible to get in here with a custom aProperty if variables are not enabled.  (The same if we were to check the layout.css.properties_and_values.enabled pref...)

::: layout/style/StyleAnimationValue.cpp:3711
(Diff revision 5)
>                                 eCSSUnit_Enumerated);
>    return true;
>  }
>  
> +static bool
> +ExtractComputedCustomValue(RefPtr<nsIAtom> aProperty,

Raw pointer is fine here too.

::: layout/style/StyleAnimationValue.cpp:3713
(Diff revision 5)
>  }
>  
> +static bool
> +ExtractComputedCustomValue(RefPtr<nsIAtom> aProperty,
> +                           nsStyleContext* aStyleContext,
> +                           StyleAnimationValue& aComputedValue) {

Nit: "{" on new line.

::: layout/style/nsAnimationManager.cpp:924
(Diff revision 5)
> -      StyleAnimationValue::UncomputeValue(prop, Move(computedValue),
> -                                          pair.mValue);
> +  aKeyframeRule->Declaration()
> +    ->IterVariables([](const nsAString& aName, void* aData) {

Having IterVariables() just return the iterator object might make this clearer, since we wouldn't have to use the |void* aData|.  Or you could just capture the customProps by reference so you could touch it from within the anonymous function?

Or can we just do the TryAddProperty() call inside the anonymous function, and avoid accumulating them in an array?

::: layout/style/nsAnimationManager.cpp:1039
(Diff revision 5)
> +  AutoTArray<nsIAtom*, 8> customProps;
> +  aAnimatedProperties
> +    .IterCustomProps([](const RefPtr<nsIAtom>& aProp, void* aData) {
> +      auto customProps = static_cast<AutoTArray<nsIAtom*, 8>*>(aData);
> +      customProps->AppendElement(aProp.get());
> +    }, &customProps);
> +  for (nsIAtom* customProp : customProps) {
> +    MaybeCustomProperty prop(customProp);
> +    if (startKeyframe && !aPropertiesSetAtStart.HasProperty(prop)) {
> +      AppendProperty(aPresContext, prop, startKeyframe->mPropertyValues);
> +    }
> +    if (endKeyframe && !aPropertiesSetAtEnd.HasProperty(prop)) {
> +      AppendProperty(aPresContext, prop, endKeyframe->mPropertyValues);
> +    }
> +  }

Similar comment here: can we make IterCustomProps() return the iterator, or do the AppendProperty work inside the anonymous function instead of building up an array?

::: layout/style/nsComputedDOMStyle.cpp:6320
(Diff revision 5)
> -    if (cssprop == eCSSPropertyExtra_all_properties)
> +      if (cssprop == eCSSPropertyExtra_all_properties)
> -      property->SetIdent(eCSSKeyword_all);
> +        property->SetIdent(eCSSKeyword_all);
> -    else if (cssprop == eCSSPropertyExtra_no_properties)
> +      else if (cssprop == eCSSPropertyExtra_no_properties)
> -      property->SetIdent(eCSSKeyword_none);
> +        property->SetIdent(eCSSKeyword_none);
> -    else if (cssprop == eCSSProperty_UNKNOWN ||
> +      else if (cssprop == eCSSProperty_UNKNOWN ||
> -             cssprop == eCSSPropertyExtra_variable)
> +               cssprop == eCSSPropertyExtra_variable)

I'm a bit confused when we use eCSSPropertyExtra_variable now.  Should we assert that we never set that value in a MaybeCustomProperty?  Or do we need that in places?

::: layout/style/nsComputedDOMStyle.cpp:6333
(Diff revision 5)
> -      property->SetString(nsCSSProps::GetStringValue(cssprop));
> +        property->SetString(nsCSSProps::GetStringValue(cssprop));
> +    } else {
> +      RefPtr<nsIAtom> custom = prop.AsCustom();
> +      nsString name;
> +      custom->ToString(name);
> +      property->SetString(Move(name));

I don't think SetString takes an rvalue-reference.

::: layout/style/nsRuleNode.cpp:5536
(Diff revision 5)
> -        if (prop == eCSSProperty_UNKNOWN ||
> +        if (prop == eCSSProperty_UNKNOWN) {
> -            prop == eCSSPropertyExtra_variable) {
>            transition->SetUnknownProperty(prop, propertyStr);
> +        } else if (prop == eCSSPropertyExtra_variable) {
> +          RefPtr<const CSSVariableRegistrations> registrations =
> +            CSSVariableRegistrationsOfStyleContext(aContext);
> +          CSSVariableRegistration* reg = nullptr;
> +          if (registrations &&
> +              registrations->mData.Get(Substring(propertyStr,
> +                                                 CSS_CUSTOM_NAME_PREFIX_LENGTH),
> +                                       &reg)) {
> +            transition->SetProperty(MaybeCustomProperty(reg->mAtom));
> +          } else {
> +            transition->SetUnknownProperty(prop, propertyStr);
> +          }

We can replace this section with the nsCSSProps::LookupProperty-like method I mentioned earlier.

::: layout/style/nsStyleStruct.h:2394
(Diff revision 5)
> -      NS_ASSERTION(aProperty != eCSSProperty_UNKNOWN &&
> -                   aProperty != eCSSPropertyExtra_variable,
> +#ifdef DEBUG
> +      if (aProperty.IsFixed()) {
> +        nsCSSProperty fixed = aProperty.AsFixed();
> +        NS_ASSERTION(fixed != eCSSProperty_UNKNOWN &&
> +                     fixed != eCSSPropertyExtra_variable,
> -                   "invalid property");
> +                     "invalid property");
> +      }
> +#endif

How about just:

  MOZ_ASSERT(aProperty.IsCustom() ||
             aProperty.AsFixed() != eCSSProperty_UNKNOWN, "invalid property");

if that's the right condition.

::: layout/style/nsTransitionManager.h:20
(Diff revision 5)
> +using mozilla::MaybeCustomProperty;
> +using mozilla::MaybeCustomPropertySet;
> +

I think we try to avoid importing names into the global namespace in header files like this.  Happily I think we only need to prefix the three "MaybeCustomProperty" instances in nsTransitionManager; the other classes in this header are already in the right namespace.

::: layout/style/nsTransitionManager.h:293
(Diff revision 5)
> +    if (aProperty.IsFixed()) {
> -    mEvent.mPropertyName =
> +      mEvent.mPropertyName =
> -      NS_ConvertUTF8toUTF16(nsCSSProps::GetStringValue(aProperty));
> +        NS_ConvertUTF8toUTF16(nsCSSProps::GetStringValue(aProperty.AsFixed()));
> +    } else {
> +      nsAutoString name, unprefixedName;
> +      RefPtr<nsIAtom> custom = aProperty.AsCustom();
> +      custom->ToString(unprefixedName);
> +      name.AppendLiteral("--");
> +      name.Append(unprefixedName);
> +      custom->ToString(name);
> +    }

This again would be good to simplify with a common function for getting the property name.

::: layout/style/nsTransitionManager.cpp:87
(Diff revision 5)
>    }
> -  MOZ_ASSERT(nsCSSProps::PropHasFlags(TransitionProperty(),
> +  MOZ_ASSERT(TransitionProperty().IsFixed() &&
> +             nsCSSProps::PropHasFlags(TransitionProperty().AsFixed(),
>                                        CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR),
>               "The transition property should be able to be run on the "
> -             "compositor");
> +             "compositor (custom properties can't yet)");

Nit: This parenthetical to me sounds like it is the only the reason for this assertion to fail, but it's just one of the cases (the flag being missing being the other).

::: layout/style/nsTransitionManager.cpp:147
(Diff revision 5)
> -  nsCSSProperty prop = mEffect->AsTransition()->TransitionProperty();
> -  aRetVal = NS_ConvertUTF8toUTF16(nsCSSProps::GetStringValue(prop));
> +  MaybeCustomProperty prop = mEffect->AsTransition()->TransitionProperty();
> +  if (prop.IsFixed()) {
> +    aRetVal = NS_ConvertUTF8toUTF16(nsCSSProps::GetStringValue(prop.AsFixed()));
> +  } else {
> +    nsAutoString unprefixedName;
> +    RefPtr<nsIAtom> custom = prop.AsCustom();
> +    custom->ToString(unprefixedName);
> +    aRetVal.Truncate(0);
> +    aRetVal.AppendLiteral("--");
> +    aRetVal.Append(unprefixedName);
> +  }

Here too.

::: layout/style/nsTransitionManager.cpp:269
(Diff revision 5)
> +  if (TransitionProperty().IsCustom() &&
> +      aOther.TransitionProperty().IsCustom()) {
> +    RefPtr<nsIAtom> left = TransitionProperty().AsCustom();
> +    RefPtr<nsIAtom> right = aOther.TransitionProperty().AsCustom();
> +    nsAutoString lefts, rights;
> +    left->ToString(lefts);
> +    right->ToString(rights);
> +    return lefts < rights;

This also looks like the code in KeyframeUtils.cpp, so hopefully we can share that code here.

::: layout/style/nsTransitionManager.cpp:635
(Diff revision 5)
>    nsStyleContext* aNewStyleContext,
>    bool* aStartedAny,
> -  nsCSSPropertySet* aWhichStarted)
> +  MaybeCustomPropertySet* aWhichStarted)
>  {
>    // IsShorthand itself will assert if aProperty is not a property.
> -  MOZ_ASSERT(!nsCSSProps::IsShorthand(aProperty),
> +  MOZ_ASSERT(aProperty.IsFixed() && !nsCSSProps::IsShorthand(aProperty.AsFixed()),

Don't we want |aPropertyIsCustom() || !nsCSSProps::IsShorthand(...)|?  Otherwise we'll assert when trying to start transitions of custom properties.

::: layout/style/nsTransitionManager.cpp:646
(Diff revision 5)
>    // is 'all' and the disabled property has a default value which derives value
>    // from another property, e.g. color.
> -  if (!nsCSSProps::IsEnabled(aProperty, CSSEnabledState::eForAllContent)) {
> +  if ((aProperty.IsFixed() &&
> +       !nsCSSProps::IsEnabled(aProperty.AsFixed(), CSSEnabledState::eForAllContent)) ||
> +      (aProperty.IsCustom() &&
> +       !nsLayoutUtils::CSSVariablesEnabled())) {

Same comment as earlier -- we probably don't need to check CSSVariablesEnabled() since we won't be able to get any variables set if it's disabled.
Attachment #8773513 - Flags: review?(cam) → review-
Also, I think we need to do something to handle changes to property registrations when we have animations.  Discussing with Brian on IRC earlier, we might be able to handle this in KeyframeEffect::UpdateProperties, since that'll get called when we restyle the document (which I assume will happen when registrations change) and that point we can re-parse the custom property values, assuming they're stored as strings.
(Assignee)

Updated

11 months ago
Depends on: 1293739
(Assignee)

Updated

11 months ago
Depends on: 1293743
(Assignee)

Comment 183

11 months ago
mozreview-review-reply
Comment on attachment 8773496 [details]
Bug 1273706 - Part 12: Add a new type for computed CSS values.

https://reviewboard.mozilla.org/r/66152/#review66284

> I feel like we can get more code re-use benefit here than might otherwise be apparent, by storing calc(1px) and calc(1%) values as CSSValueType::Length and CSSValueType::Percentage, rather than as CSSValueType::LengthPercentage and then checking whether we need to serialize as a calc() or as a single value.  Then we wouldn't need a new type to represent calc() values, and would allow us to re-use the serialization code that is in nsComputeDOMStyle::SetValueToCoord (which would need to be extracted and exposed).  If we store more value types as nsStyleCoords (Integer, too), then in CSSComputedValue::SingleTerm::SetAnimationValue we could have a new setter on StyleAnimationValue that takes an nsStyleCoord and passes it to StyleCoordToValue, and we could handle them all with one case in SetAnimationValue.
> 
> Is there any problem with the LengthPercentage constructor being able to create CSSValueType::Length and CSSValueType::Percentage values too, depending on what was in the calc()?  WDYT?

You are right. Managed to obtain additional code re-use in CSSComputedValue by storing as style coords, converting to nsCSSValues, and then just using nsCSSValue's AppendToString. StyleCordToValue and StyleCoordToCSSValue are pulled out from StyleAnimationValue.

> Ah, you are right that we should be serializing the absolute URI here.  But maybe we should already be storing the absolute URI in ComputeCSSValueTerm, by calling GetURLValue() on the URLValue to get an nsIURI?  And then store that in the Variant as an nsIURI* (or, now that bug 652991 has landed, a FragmentOrURL value).  Does that make sense?

After trying to store it as an nsIURI, I ended up storing it as a nsCSSValue instead, beacuse
1. That lets us keep the URLValue, which gives us the origin principal, which the parser wants when parsing URL values. We re-parse URL values because we add variable declarations in AnimValuesStyleRule analogously to the way standard properties are added. Unfortunately this means we have to re-parse. This could be fixed in the future by adding another code path through CSSVariableDeclarations -> CSSVariableResolver. But I don't think it's worth it.
2. More importantly, StyleAnimationValue's eUnit_URL expects an nsCSSValue containing a URLValue, not an nsIURI. I think this is for the same reason (keeping additional info, incl. the principal).

What do you think?

> For the methods that take nsCSSValues, can we assert that their units have expected values?  Also, we should probably prefer to use |const nsCSSValue&| for the argument (though perhaps the compiler would optimize one of the copies away).

That sounds goood.
(Assignee)

Comment 184

11 months ago
mozreview-review-reply
Comment on attachment 8773496 [details]
Bug 1273706 - Part 12: Add a new type for computed CSS values.

https://reviewboard.mozilla.org/r/66152/#review66284

> StyleAnimationValue supports URL values.  Why don't we support setting it here?
> 
> Also what's the reason for just returning false for these remaining types?  Can you add a comment stating why?

You are correct that this was a mistake. It has been fixed, but you will definitely want to review the fix. Uploading soon.
(Reporter)

Comment 185

11 months ago
mozreview-review-reply
Comment on attachment 8773496 [details]
Bug 1273706 - Part 12: Add a new type for computed CSS values.

https://reviewboard.mozilla.org/r/66152/#review66284

> After trying to store it as an nsIURI, I ended up storing it as a nsCSSValue instead, beacuse
> 1. That lets us keep the URLValue, which gives us the origin principal, which the parser wants when parsing URL values. We re-parse URL values because we add variable declarations in AnimValuesStyleRule analogously to the way standard properties are added. Unfortunately this means we have to re-parse. This could be fixed in the future by adding another code path through CSSVariableDeclarations -> CSSVariableResolver. But I don't think it's worth it.
> 2. More importantly, StyleAnimationValue's eUnit_URL expects an nsCSSValue containing a URLValue, not an nsIURI. I think this is for the same reason (keeping additional info, incl. the principal).
> 
> What do you think?

OK, sounds like it does make sense to have it as an nsCSSValue.  Thanks for testing!
(Assignee)

Updated

10 months ago
Blocks: 1294863
(Assignee)

Comment 186

10 months ago
mozreview-review-reply
Comment on attachment 8773503 [details]
Bug 1273706 - Part 16: Store custom property registrations on nsCSSParser.

https://reviewboard.mozilla.org/r/66166/#review67124

For the record (though I'm guessing you already got the notification!) I've split this off into a new bug at https://bugzilla.mozilla.org/show_bug.cgi?id=1294863 .
(Assignee)

Comment 187

10 months ago
mozreview-review-reply
Comment on attachment 8773513 [details]
Bug 1273706 - Part 29: Implement animations for custom properties.

https://reviewboard.mozilla.org/r/66186/#review67478

> Instead of allocating a new string, you could have an nsAutoCString declared next to |name|, and then do |name = thatNewVariable.get();| after appending the UTF-8 converted string to thatNewVariable.  Then we wouldn't have to manage memory manually.

That's really clever! Thanks for that.
(Assignee)

Comment 188

10 months ago
mozreview-review-reply
Comment on attachment 8773513 [details]
Bug 1273706 - Part 29: Implement animations for custom properties.

https://reviewboard.mozilla.org/r/66186/#review67478

> I think it's fine to use eCSSProperty_UNKNOWN here for custom properties, since it looks like for all the types we might set the nsCSSValue to for typed custom properties, we won't run into a case in nsCSSValue::AppendToString where we need to know the specific property.  However, can you update the comment above to say it's safe for this reason?

Yes, I checked this when using AppendToString. Will add a comment, thanks!

> We do this turning a property name string into a MaybeCustomProperty in a couple of places -- at least here, and in nsRuleNode::ComputeDisplayData.
> 
> Can we add a method to nsCSSProps that takes the registrations as an argument (and the string and enabled state, like nsCSSProps::LookupProperty), and returns a MaybeCustomProperty?

You are right. That was a lot of unnecessary duplication. Thanks!

> Hmm, might it make sense to pull this out into a static helper function?

You're right.

> Should we instead be doing:
> 
>   return (aPair.mProperty.IsCustom() ||
>           !nsCSSProps::IsShorthand(...)) &&
>          aPair.mValue.GetUnit() == ...
> 
> so that we will return true for custom properties with invalid values?  Is it still the case that we can use the mPropertyID == eCSSProperty_UNKNOWN to recognise invalid values?

I think you are correct, according to the comments at https://dxr.mozilla.org/mozilla-central/source/dom/animation/KeyframeUtils.cpp?q=MakePropertyValuePair&redirect_type=direct#982 .

> Can you add a comment in here (or whereever you think is appropriate) pointing out that we will want to make unregistered custom properties discretely animatable, once bug 1277433 lands?

Do you think it is OK if I add it to CSSComputedValue::GetAsAnimationValue?

> I think IsShorthand will assert with eCSSProperty_UNKNOWN.  Check IsFixed() in the if statement condition instead?
> 
> Also, I guess calling ParseLonghandProperty will fail and report an error to the console, so it would be better to skip doing that for custom properties.
> 
> Should we be trying to parse the custom property properly, though?  Or do we want to store a token stream even for typed registered custom properties?

I changed it to use ParseTypedValue for custom properties to try to be consistent. Do you think this is OK?

> Should we use OwnerDoc() instead?  I don't think anything calls computeAnimationDistance() with an element that's not in the document, but it's easy to make it work if anyone does.

Didn't know about that. Thanks! Is CSSEnabledState::eIgnoreEnabledState what we want here? That's what we use for fixed properties a couple lines above.
(Assignee)

Comment 189

10 months ago
mozreview-review-reply
Comment on attachment 8773513 [details]
Bug 1273706 - Part 29: Implement animations for custom properties.

https://reviewboard.mozilla.org/r/66186/#review67478

> Is this right?  I don't think all custom properties can be represented as a calc() value.  Do the parens need to go elsewhere?
> 
> Or is it that we can never get in here with say aFirstUnit == eCSSUnit_Pixel and aSecondUnit == eCSSUnit_EnumColor?  (If so some assertions are needed in here.)

Sorry, you're right. The parentheses are in the wrong place. My reasoning was that CSS_PROPERTY_STORE_CALC only matters if the first and second units are in {pixel, percent, calc}. All of the custom properties with these types can store calc()s can accept calc()s. If a custom property doesn't have one of these types, then this branch doesn't matter anyway. What do you think about that? Fixing the parens.

> The fixed property branch above checks to see if the property parsing succeded or not.  Do we need to do that here?  (I'm not sure when ParseProperty will fail here, since we should have valid values here already.)  If we expect ParseVariable to have succeeded, can you assert it?

Looks like if ParseVariable fails, it just outputs to the error reporter. Looks like it would require some modification of ParseVariable to report failures. Shouldn't be hard to do, but do you think it's worth it for the assert?

> (Since we have a lot of this code to check whether a MaybeCustomProperty is a shorthand, it might make sense to add an IsShorthand() method on MaybeCustomProperty to save the IsCustom/IsFixed check everwhere.)

Good idea.

> This is related to my comments about merging the two style structs -- but is there a reason why we can't / don't store specified typed custom property values using a non-token-stream nsCSSValue?  Is there just no advantage in doing that?

I think we discussed this in email after you wrote this comment. Will close for now.

> Will we need for discrete animation of url() values?

You are very very right.

> I wonder if it's possible to get in here with a custom aProperty if variables are not enabled.  (The same if we were to check the layout.css.properties_and_values.enabled pref...)

Shouldn't be possible. Figured it was safe. Should I remove the check?

> Having IterVariables() just return the iterator object might make this clearer, since we wouldn't have to use the |void* aData|.  Or you could just capture the customProps by reference so you could touch it from within the anonymous function?
> 
> Or can we just do the TryAddProperty() call inside the anonymous function, and avoid accumulating them in an array?

You are right, Iter is better. Didn't think of that.

> Similar comment here: can we make IterCustomProps() return the iterator, or do the AppendProperty work inside the anonymous function instead of building up an array?

Yep, that works better. Thanks!

> I'm a bit confused when we use eCSSPropertyExtra_variable now.  Should we assert that we never set that value in a MaybeCustomProperty?  Or do we need that in places?

Yeah, me too. But here is a side by side of what do with the patches vs. previously in ComputeDisplayData:

       if (val.GetUnit() == eCSSUnit_Ident) {
         nsDependentString
           propertyStr(property.list->mValue.GetStringBufferValue());
-        nsCSSProperty prop =
+        nsCSSPropertyID prop =
           nsCSSProps::LookupProperty(propertyStr,
                                      CSSEnabledState::eForAllContent);
-        if (prop == eCSSProperty_UNKNOWN ||
-            prop == eCSSPropertyExtra_variable) {
+        if (prop == eCSSProperty_UNKNOWN) {
           transition->SetUnknownProperty(prop, propertyStr);
+        } else if (prop == eCSSPropertyExtra_variable) {
+          const CSSVariableRegistrations* registrations =
+            CSSVariableRegistrationsOfStyleContext(aContext);
+          CSSProperty property =
+            nsCSSProps::LookupCustomProperty(registrations, propertyStr,
+                                             CSSEnabledState::eForAllContent);
+          if (property == eCSSProperty_UNKNOWN) {
+            transition->SetUnknownProperty(prop, propertyStr);
+          } else {
+            transition->SetProperty(Move(property));
+          }
         } else {
-          transition->SetProperty(prop);
+          transition->SetProperty(CSSProperty(prop));
         }
       } else {

So before, if prop == eCSSPropertyExtra_variable, we would always call transition->SetUnknownProperty(eCSSPropertyExtra_variable).
Now, if prop == eCSSPropertyExtra_variable, then if this is a typed registered property, we call transition->SetProperty, but otherwise, we do what we did before and call transition->SetUnknownProperty(eCSSPropertyExtra_variable).

So I believe the system used eCSSPropertyExtra_variable before to keep track of variables, which are not animated. Now we use it to represent unregistered custom properties, which act just as they did before.

What do you think?

> Don't we want |aPropertyIsCustom() || !nsCSSProps::IsShorthand(...)|?  Otherwise we'll assert when trying to start transitions of custom properties.

You're right, sorry. Not sure how that came in there between the original tests. Need to figure out how to test transitions with Web Platform Tests.
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 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 220

10 months ago
mozreview-review
Comment on attachment 8773498 [details]
Bug 1273706 - Part 11: Have StyleAnimationValue::UncomputeValue(..., nsCSSValue) uncompute UnparsedStrings to token stream nsCSSValues.

https://reviewboard.mozilla.org/r/66156/#review68914

Here was the pending comment I had on revision 5 of the patch, which has been replaced by revision 6.  I can't really tell if this still applies to some other patch in the series.

Revision 6 appears to be a different patch entirely.  Did you use a different MozReview-Commit-ID or somehow keep the old one incorrectly?

I can't actually tell since MozReview refuses to show me the old description.

::: layout/style/CSSVariableSyntax.cpp:24
(Diff revision 5)
> +};
> +
> +// Mapping from some CSSValueTypes to <syntax> and variants for ParseVariant.
> +static const TermTableEntry kTerms[] = {
> +  { CSSValueType::Length, "length", VARIANT_LENGTH | VARIANT_CALC },
> +  { CSSValueType::Number, "number", VARIANT_NUMBER },

should <number> and maybe also <integer> (I saw some patches go by recently) also support calc()?

Or, going the other way, if you follow the spec literally, only <length-percentage> should support calc(), but <length> and <percentage> should not.  Though I think it might make more sense to change the spec...

Comment 221

10 months ago
mozreview-review
Comment on attachment 8773498 [details]
Bug 1273706 - Part 11: Have StyleAnimationValue::UncomputeValue(..., nsCSSValue) uncompute UnparsedStrings to token stream nsCSSValues.

https://reviewboard.mozilla.org/r/66156/#review69234

r=dbaron if you can explain the question about mPropertyID, although I suspect it might require revision to the patch

::: layout/style/StyleAnimationValue.cpp:3183
(Diff revision 6)
>          }
>        }
>        break;
>      }
> +    case eUnit_UnparsedString: {
> +      nsCSSValueTokenStream* value = new nsCSSValueTokenStream;

This should probably be a RefPtr<nsCSSValueTokenStream>.

::: layout/style/StyleAnimationValue.cpp:3184
(Diff revision 6)
>        }
>        break;
>      }
> +    case eUnit_UnparsedString: {
> +      nsCSSValueTokenStream* value = new nsCSSValueTokenStream;
> +      value->mPropertyID = aProperty;

What guarantees that this is always the right property ID?

In particular, what the property ID is used for is that when you have something like:

  border: var(foo)
  
then as the value of border-left-color, we store a token stream that has the string "var(foo)" and the property ID for border.  (This allows a later declaration of border-right to override border-right-color but not border-left-color.)  Thus we can later parse the token stream as a value for 'border' and then take the resulting 'border-right-color' value.
Attachment #8773498 - Flags: review?(dbaron) → review+

Comment 222

10 months ago
mozreview-review
Comment on attachment 8773500 [details]
Bug 1273706 - Part 13: Add CSSVariableSyntax for representing the syntax of a custom property.

https://reviewboard.mozilla.org/r/66160/#review69236

::: layout/style/CSSVariableSyntax.cpp:22
(Diff revision 6)
> +static const TermTableEntry kTerms[] = {
> +  { CSSValueType::Length, "length", VARIANT_LENGTH | VARIANT_CALC },
> +  { CSSValueType::Number, "number", VARIANT_NUMBER | VARIANT_CALC },

OK, comment 220 goes here:

should <number> and maybe also <integer> (I saw some patches go by recently) also support calc()?

Or, going the other way, if you follow the spec literally, only <length-percentage> should support calc(), but <length> and <percentage> should not.  Though I think it might make more sense to change the spec...


And it seems like whatever you're using to post the patches is regenerating the MozReview-Commit-IDs frequently, which makes mozreview pretty much unusable for reviewing your code.
(Assignee)

Comment 223

10 months ago
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #220)
> Comment on attachment 8773498 [details]
> Bug 1273706 - Part 11: Have StyleAnimationValue::UncomputeValue(...,
> nsCSSValue) uncompute UnparsedStrings to token stream nsCSSValues.
> 
> https://reviewboard.mozilla.org/r/66156/#review68914
> 
> Here was the pending comment I had on revision 5 of the patch, which has
> been replaced by revision 6.  I can't really tell if this still applies to
> some other patch in the series.

Thanks for the review. Weird, I see what you're describing with revision 5 & revision 6 being entirely different too. Not sure how that happened... In any case, part 11, which adds CSSVariableSyntax is now part 13.

I noticed in some cases the r+'s got mixed up too. In particular, part 24 is marked as having r+ from heycam, but it didn't used to exist. I am not sure how to remove that.

Sorry about that.

In terms of your review comment:

1. I think you are right that <number> and <integer> should also support calc().

I think it was you who pointed out to me that ParseVariant already supports VARIANT_NUMBER | VARIANT_CALC, and Bug 1293743 adds support for VARIANT_INTEGER | VARIANT_CALC.

These calcs (for numbers, integers, lengths, percentages, etc.) are then computed in part 26.

2. I think that CSS Values says that "The calc() function ... can be used wherever <length>, <frequency>, <angle>, <time>, <percentage>, <number>, or <integer> values are allowed." ( https://drafts.csswg.org/css-values-3/#funcdef-calc ). But it is weird that the spec only mentions calc() for <length-percentage>.

I realize also now that that spec says we should add support for calcs in <frequency>, <angle>, and <time>... but I remember you mentioning discussion about adding division by units. Given that, do you think it's worth implementing these right now as well the same way we implement support for integer calcs in Bug 1293743?
Given the mess that MozReview has made of this, I think this probably needs to be split into separate bugs for different parts.  I'm not going to be able to do re-reviews in this bug if I have to mark any patches review-.
Flags: needinfo?(jyc)
(In reply to Jonathan Chan [:jyc] from comment #223)
> 2. I think that CSS Values says that "The calc() function ... can be used
> wherever <length>, <frequency>, <angle>, <time>, <percentage>, <number>, or
> <integer> values are allowed." (
> https://drafts.csswg.org/css-values-3/#funcdef-calc ). But it is weird that
> the spec only mentions calc() for <length-percentage>.
> 
> I realize also now that that spec says we should add support for calcs in
> <frequency>, <angle>, and <time>... but I remember you mentioning discussion
> about adding division by units. Given that, do you think it's worth
> implementing these right now as well the same way we implement support for
> integer calcs in Bug 1293743?

Maybe, but not in this bug.  And this can land without that.
(Assignee)

Comment 226

10 months ago
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #221)
> Comment on attachment 8773498 [details]
> Bug 1273706 - Part 11: Have StyleAnimationValue::UncomputeValue(...,
> nsCSSValue) uncompute UnparsedStrings to token stream nsCSSValues.
> 
> https://reviewboard.mozilla.org/r/66156/#review69234
> 
> r=dbaron if you can explain the question about mPropertyID, although I
> suspect it might require revision to the patch
> 
> ::: layout/style/StyleAnimationValue.cpp:3183
> (Diff revision 6)
> >          }
> >        }
> >        break;
> >      }
> > +    case eUnit_UnparsedString: {
> > +      nsCSSValueTokenStream* value = new nsCSSValueTokenStream;
> 
> This should probably be a RefPtr<nsCSSValueTokenStream>.

Alright, thanks. I will change it to that.

> ::: layout/style/StyleAnimationValue.cpp:3184
> (Diff revision 6)
> >        }
> >        break;
> >      }
> > +    case eUnit_UnparsedString: {
> > +      nsCSSValueTokenStream* value = new nsCSSValueTokenStream;
> > +      value->mPropertyID = aProperty;
> 
> What guarantees that this is always the right property ID?
> In particular, what the property ID is used for is that when you have
> something like:
> 
>   border: var(foo)
>   
> then as the value of border-left-color, we store a token stream that has the
> string "var(foo)" and the property ID for border.  (This allows a later
> declaration of border-right to override border-right-color but not
> border-left-color.)  Thus we can later parse the token stream as a value for
> 'border' and then take the resulting 'border-right-color' value.

It looks like the one place we create UnparsedString StyleAnimationValues is in StyleAnimationValue::ComputeValue.
There we are storing shorthands as UnparsedString values.
The motivation for adding uncomputing of UnparsedStrings to nsCSSValues (they already uncompute to strings) was so that non-interpolable custom property values could still be animated (doing a 50% flip, because Interpolate returns false).
A later patch replaces the nsCSSPropertyID aProperty with a CSSProperty aProperty, which can represent custom values.

As far as I could tell, the only reason I needed to set mPropertyID was so that IsInvalidValuePair in KeyframeUtils didn't think the custom property value pairs represented invalid values.
Part 29 has this:

+      // Can't use eCSSPropertyExtra_UNKNOWN because that would cause
+      // KeyframeUtils::IsInvalidValuePair to think these values were invalid.
+      value->mPropertyID = aProperty.IsFixed() ? aProperty.AsFixed()
+                                               : eCSSPropertyExtra_variable;

In that sense I guess there is no guarantee right now that this is the right property ID.

What do you think about making it so that this always returns false unless we have a custom property (so we don't change existing behavior), and for custom properties setting mPropertyID to eCSSPropertyExtra_variable? I think in that case it would be cleanest to move this patch to after Part 29, although that would mean that the animation support for non-interpolable properties only implemented by Part 29 would not work until this patch was applied.
(Assignee)

Comment 227

10 months ago
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #224)
> Given the mess that MozReview has made of this, I think this probably needs
> to be split into separate bugs for different parts.  I'm not going to be
> able to do re-reviews in this bug if I have to mark any patches review-.

gps just stopped by and |hg qref| didn't seem to cause the problem to occur again.
We also updated my version of version-control-tools.

Do you think it would be OK if we kept it in this patch for now? Sorry for the inconvenience.
Flags: needinfo?(jyc) → needinfo?(dbaron)
If the IDs don't change again then it should probably be ok.
Flags: needinfo?(dbaron)
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 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 259

10 months ago
Tried to fix MozReview-IDs, added some more tests, refactored the CSSVariableSyntax parsing state machine to have fewer states (AfterBar & Start were pretty much the same, we just don't want to allow * after a bar but want to allow them at the start), and fixed a bug with empty declarations & lists.
(Reporter)

Comment 260

10 months ago
mozreview-review
Comment on attachment 8773501 [details]
Bug 1273706 - Part 9: Expose nsComputedDOMStyle::SetValueToStyleImage as a static method so that we can compute serialized computed values for gradients.

https://reviewboard.mozilla.org/r/66162/#review70148

::: layout/style/nsComputedDOMStyle.h:216
(Diff revision 7)
> -  void GetCSSGradientString(const nsStyleGradient* aGradient,
> +  // SetValueToStyleImage uses GetCSSGradientString and GetImageRectString. We
> +  // would make it a private member function except that it's used by
> +  // CSSComputedValue, which has a member function called by this class, and we
> +  // can't forward-declare member functions.
> +  friend void mozilla::SetValueToStyleImage(const nsStyleImage& aStyleImage,
> +                                            nsROCSSPrimitiveValue* aValue);

I take it you can't include nsComputedDOMStyle.h in CSSComputedValue.h.

::: layout/style/nsComputedDOMStyle.h:630
(Diff revision 7)
>     * can be expected or something?
>     */
> -  void SetValueToCoord(nsROCSSPrimitiveValue* aValue,
> +  static void SetValueToCoord(nsROCSSPrimitiveValue* aValue,
> -                       const nsStyleCoord& aCoord,
> +                              const nsStyleCoord& aCoord,
> -                       bool aClampNegativeCalc,
> +                              bool aClampNegativeCalc,
> +                              nsComputedDOMStyle* aThis = nullptr,

Can you document in the comment when aThis must be provided?  Maybe call it something like "aPercentageBaseGetterObject" or "aPercentageBaseGetterThis" to make it clear?
Attachment #8773501 - Flags: review?(cam) → review+
(Reporter)

Comment 261

10 months ago
mozreview-review
Comment on attachment 8773499 [details]
Bug 1273706 - Part 14: Store custom property registrations on the document.

https://reviewboard.mozilla.org/r/66158/#review70156

r=me with these changes.

::: dom/base/nsDocument.h:1798
(Diff revision 7)
>  #ifdef DEBUG
>  public:
>    bool mWillReparent;
>  #endif
> +
> +  mozilla::CSSVariableRegistrations*
> +  GetCSSVariableRegistrations() const override;

This accessor method isn't going to be public in non-DEBUG builds given the #ifdef here.

If there's nothing stopping you, just put this on nsIDocument rather than nsDocument, so we don't need the virtual method with the single overload.  (I don't think we have any nsIDocument implementations which aren't nsDocument.  Maybe we used to a long time ago.)

::: layout/style/CSSVariableRegistration.h:54
(Diff revision 7)
> +  nsString mInitialExpr;
> +  nsCSSValue mInitialValue;
> +  CSSValueType mInitialType;

I think it's not super clear what the difference is between these things representing the initial value.  Can you add short comments describing what they are?

::: layout/style/CSSVariableRegistration.h:67
(Diff revision 7)
> +  // nsCSSParser that use mSheetURI, etc.)
> +  CSSVariableExprContext mInitialContext;
> +
> +  // These end up being used inside of CSSParserImpl::ResolveVariableValues.
> +  // It calls AppendTokens to construct the resulting expanded value, and needs
> +  // to know whether to insert /**/ between adjacent token.

*tokens

::: layout/style/CSSVariableRegistrations.h:26
(Diff revision 7)
> +namespace mozilla {
> +
> +/**
> + * CSSVariableRegistrations, a mapping from registered custom property names to
> + * their registrations. Whenever a registration in |mData| is modified,
> + * |mGeneration| *must* be incremented. (This happens in the bindings for

"bindings" sounds like it is talking about the generated objdir/dom/webidl/CSSBinding.cpp file.  So maybe s/the bindings for//.

::: layout/style/CSSVariableRegistrations.cpp:27
(Diff revision 7)
> +  return doc ? CSSVariableRegistrationsOfDocument(doc)
> +             : nullptr;

CSSVariableRegistrationsOfDocument already handles null, so I think we don't need to check that again here.
Attachment #8773499 - Flags: review?(cam) → review+
(Reporter)

Comment 262

10 months ago
mozreview-review
Comment on attachment 8773502 [details]
Bug 1273706 - Part 15: Add nsCSSProps::LookupCustomProperty for looking up a CSSProperty given a custom property name and registrations.

https://reviewboard.mozilla.org/r/66164/#review70176

::: layout/style/nsCSSProps.h:405
(Diff revision 7)
> +  // Look up a custom property as a CSSProperty by its full name (with the '--'
> +  // prefix). If no such property exists, or if custom properties are not
> +  // enabled, we return CSSProperty(eCSSProperty_UNKNOWN).
> +  static mozilla::CSSProperty
> +  LookupCustomProperty(const mozilla::CSSVariableRegistrations* aRegistrations,
> +                       const nsAString& aName,
> +                       EnabledState aEnabled);

Can you make it clear that this looks up registered custom properties?  Maybe that is obvious, but I think it would be clearer if you say "Look up a registered custom property" and then, instead of "If no such property exists", "If the given custom property has not been registered".  Or something along those lines.

::: layout/style/nsCSSProps.cpp:614
(Diff revision 7)
>    return LookupPropertyByIDLName(NS_ConvertUTF16toUTF8(aPropertyIDLName),
>                                   aEnabled);
>  }
>  
> +/* static */ mozilla::CSSProperty
> +nsCSSProps::LookupCustomProperty(const mozilla::CSSVariableRegistrations*

No need for any "mozilla::" prefixes in this function, since we have |using namespace mozilla| at the top of the file.

::: layout/style/nsCSSProps.cpp:629
(Diff revision 7)
> +  }
> +
> +  CSSVariableRegistration* registration;
> +  if (!aRegistrations->mData.Get(Substring(aName,
> +                                           CSS_CUSTOM_NAME_PREFIX_LENGTH),
> +                                &registration)) {

Nit: indent one more space.
Attachment #8773502 - Flags: review?(cam) → review+
(Reporter)

Comment 263

10 months ago
mozreview-review
Comment on attachment 8777231 [details]
Bug 1273706 - Part 27: Add CSSPropertySet.

https://reviewboard.mozilla.org/r/68828/#review70182

r=me with that.

::: layout/style/CSSPropertySet.h:7
(Diff revision 3)
> +#ifndef mozilla_MaybeCustomPropertySet_h
> +#define mozilla_MaybeCustomPropertySet_h

Update the name of the guard macro now we've renamed the class/file.

::: layout/style/CSSPropertySet.h:24
(Diff revision 3)
> +/**
> + * The same as nsCSSPropertyIDSet, but also capable of holding custom properties.
> + */
> +class CSSPropertySet {
> +public:
> +  typedef nsDataHashtable<nsPtrHashKey<nsIAtom>, nsCOMPtr<nsIAtom>> Table;

If you just want a set, and so don't need a value that the key maps to, you can use:

  nsTHashtable<nsPtrHashKey<nsIAtom>>

and then use nsTHashtable's PutEntry/Contains/RemoveEntry API.
Attachment #8777231 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #260)
> I take it you can't include nsComputedDOMStyle.h in CSSComputedValue.h.

Sorry, I forgot we already discussed this in the previous patch revision.
(Reporter)

Comment 265

10 months ago
mozreview-review
Comment on attachment 8773496 [details]
Bug 1273706 - Part 12: Add a new type for computed CSS values.

https://reviewboard.mozilla.org/r/66152/#review70138

Looks good; I'm glad the nsStyleCoord storage worked out.  r=me with the below addressed.

::: layout/style/CSSValueType.h:14
(Diff revisions 5 - 7)
>  
>  namespace mozilla {
>  
> +/**
> + * The type of a registered custom property value.
> + * Note that in multiple CSSValueTypes are valid for a given value -- for

"Note that in multiple" doesn't sound right...

::: layout/style/CSSComputedValue.h:29
(Diff revision 7)
> +
> +/**
> + * A CSSComputedValue contains the computed value of a registered custom
> + * property. It implements a tagged union, as well as methods for conversion to
> + * StyleAnimationValues (for interpolable values) and to strings (for CSSOM).
> + * 

Nit: trailing space.

::: layout/style/CSSComputedValue.h:31
(Diff revision 7)
> + * A CSSComputedValue contains the computed value of a registered custom
> + * property. It implements a tagged union, as well as methods for conversion to
> + * StyleAnimationValues (for interpolable values) and to strings (for CSSOM).
> + * 
> + * A CSSComputedValue can be either a SingleTerm or a ListTerm, corresponding to
> + * the syntaxes "X" and "X+", respectively. A ListTerm is a homogenous list of

homogeneous?

::: layout/style/CSSComputedValue.cpp:17
(Diff revision 7)
> +namespace mozilla
> +{
> +  void SetValueToStyleImage(const nsStyleImage&, nsROCSSPrimitiveValue*);

Nit: "{" on previous line.  Also I think we don't tend to indent the contents of namespaces.

::: layout/style/CSSComputedValue.cpp:69
(Diff revision 7)
> +  case CSSValueType::TransformFunction: {
> +    // The list is a list of transform functions.
> +    // Each individual transform is an array.
> +    nsCSSValueList* list = new nsCSSValueList;
> +    // Copy the nsCSSValue, which AddRefs the underlying array.
> +    list->mValue = nsCSSValue(mData.as<nsCSSValue>());

Is the nsCSSValue() needed?  I think we can just directly assign the result of as() to list->mValue.

::: layout/style/CSSComputedValue.cpp:199
(Diff revision 7)
> +CSSComputedValue::SingleTerm::operator==(
> +                                const SingleTerm&
> +                                aOther) const

This can all fit on one line now.

::: layout/style/CSSComputedValue.cpp:209
(Diff revision 7)
> +}
> +
> +// SingleTerm builders
> +
> +/* static */ CSSComputedValue::SingleTerm
> +CSSComputedValue::Length(const nsStyleCoord& aLength)

I think it would have been fine to keep this taking nscoord, etc., and for it to construct the nsStyleCoord itself.  But this is OK too.

::: layout/style/CSSValueType.h:59
(Diff revision 7)
> + * serialization. For example, a <length-percentage>-valued property declared
> + * with value 'calc(0 + 50%)' needs to serialize as 'calc(0 + 50%)', not '50%'.
> + * The motivation is a consistent appearance during interpolation.
> + * CSSLPCalcType is used in CSSComputedValue.
> + */
> +enum class CSSLPCalcType

I think CSSLPCalcType is unused and can be removed now.
Attachment #8773496 - Flags: review?(cam) → review+
(Assignee)

Comment 266

10 months ago
mozreview-review-reply
Comment on attachment 8777231 [details]
Bug 1273706 - Part 27: Add CSSPropertySet.

https://reviewboard.mozilla.org/r/68828/#review70182

> Update the name of the guard macro now we've renamed the class/file.

Sorry! Will do.

> If you just want a set, and so don't need a value that the key maps to, you can use:
> 
>   nsTHashtable<nsPtrHashKey<nsIAtom>>
> 
> and then use nsTHashtable's PutEntry/Contains/RemoveEntry API.

Thanks! Didn't know that.

I was thinking we needed the value here to preserve the refcount on the nsIAtom. Ordinarily I think we could just get rid of it, because the person can only query if they have the pointer anyways, but the IterCustomProps method which we use for animations requires that we give them the nsIAtom.

Given that, is there some way to make a set that keeps the refcount? (Don't know if nsPtrHashKey does).
(Assignee)

Comment 267

10 months ago
mozreview-review-reply
Comment on attachment 8773502 [details]
Bug 1273706 - Part 15: Add nsCSSProps::LookupCustomProperty for looking up a CSSProperty given a custom property name and registrations.

https://reviewboard.mozilla.org/r/66164/#review70176

> Can you make it clear that this looks up registered custom properties?  Maybe that is obvious, but I think it would be clearer if you say "Look up a registered custom property" and then, instead of "If no such property exists", "If the given custom property has not been registered".  Or something along those lines.

Sounds good, sorry. Thanks!

> No need for any "mozilla::" prefixes in this function, since we have |using namespace mozilla| at the top of the file.

Wait, do we actually? This causes it to not compile on my computer.
(Assignee)

Comment 268

10 months ago
mozreview-review-reply
Comment on attachment 8773502 [details]
Bug 1273706 - Part 15: Add nsCSSProps::LookupCustomProperty for looking up a CSSProperty given a custom property name and registrations.

https://reviewboard.mozilla.org/r/66164/#review70176

> Wait, do we actually? This causes it to not compile on my computer.

Oops! Sorry. Thought it was the .h file, not the .cpp file. You were right!
(Assignee)

Comment 269

10 months ago
mozreview-review-reply
Comment on attachment 8773499 [details]
Bug 1273706 - Part 14: Store custom property registrations on the document.

https://reviewboard.mozilla.org/r/66158/#review70156

> This accessor method isn't going to be public in non-DEBUG builds given the #ifdef here.
> 
> If there's nothing stopping you, just put this on nsIDocument rather than nsDocument, so we don't need the virtual method with the single overload.  (I don't think we have any nsIDocument implementations which aren't nsDocument.  Maybe we used to a long time ago.)

Sorry, should have clarified.
The reason I put it on nsDocument instead of nsIDocument is that nsIDocument is that nsIDocument is included in some external-linkage code.
This is a problem because having CSSVariableRegistrations as a member requires we have the implementation of CSSVariableRegistration, which contains a string.
It appears that external-linkage code uses different strings (from nsStringAPI.h) than internal-linkage code (from nsString.h).
We can't just add an ifdef of our own to switch between the headers because we need to access those same registrations from internal-linkage code (e.g. all of the other code), and I think that would make the data incompatible.
dholbert and I decided that making it a virtual method on the nsIDocument interface and then implementing it on nsDocument, which is not included in any external-linkage code, was the cleanest solution, beacuse by returning the CSSVariableRegistrations pointer we don't need access to the implementation, and hence don't need to worry about the different strings.

Do you think this seems OK?

Also, you are rgiht about the #ifdef. Didn't notice that.

> I think it's not super clear what the difference is between these things representing the initial value.  Can you add short comments describing what they are?

Sorry. Adding comments.
(Assignee)

Comment 270

10 months ago
mozreview-review-reply
Comment on attachment 8773496 [details]
Bug 1273706 - Part 12: Add a new type for computed CSS values.

https://reviewboard.mozilla.org/r/66152/#review70138

> "Note that in multiple" doesn't sound right...

Right. Accidentally a word, thanks!

> homogeneous?

Thanks!

> Nit: "{" on previous line.  Also I think we don't tend to indent the contents of namespaces.

Thanks. Will try to fix this in the other patches too.

> Is the nsCSSValue() needed?  I think we can just directly assign the result of as() to list->mValue.

Oh, you are right. It gets copy assigned anyways. Thanks!

> This can all fit on one line now.

Thanks! It looks really ugly.

> I think CSSLPCalcType is unused and can be removed now.

Oops, you are right. Thanks.
(Assignee)

Comment 271

10 months ago
mozreview-review-reply
Comment on attachment 8773498 [details]
Bug 1273706 - Part 11: Have StyleAnimationValue::UncomputeValue(..., nsCSSValue) uncompute UnparsedStrings to token stream nsCSSValues.

https://reviewboard.mozilla.org/r/66156/#review69234

> This should probably be a RefPtr<nsCSSValueTokenStream>.

(Moving comments here, didn't realize Reviewboard didn't take comments from Bugzilla):


Alright, thanks. I will change it to that.

> What guarantees that this is always the right property ID?
> 
> In particular, what the property ID is used for is that when you have something like:
> 
>   border: var(foo)
>   
> then as the value of border-left-color, we store a token stream that has the string "var(foo)" and the property ID for border.  (This allows a later declaration of border-right to override border-right-color but not border-left-color.)  Thus we can later parse the token stream as a value for 'border' and then take the resulting 'border-right-color' value.

(Moving comments here, didn't realize Reviewboard didn't take comments from Bugzilla):

It looks like the one place we create UnparsedString StyleAnimationValues is in StyleAnimationValue::ComputeValue.
There we are storing shorthands as UnparsedString values.
The motivation for adding uncomputing of UnparsedStrings to nsCSSValues (they already uncompute to strings) was so that non-interpolable custom property values could still be animated (doing a 50% flip, because Interpolate returns false).
A later patch replaces the nsCSSPropertyID aProperty with a CSSProperty aProperty, which can represent custom values.

As far as I could tell, the only reason I needed to set mPropertyID was so that IsInvalidValuePair in KeyframeUtils didn't think the custom property value pairs represented invalid values.
Part 29 has this:

+      // Can't use eCSSPropertyExtra_UNKNOWN because that would cause
+      // KeyframeUtils::IsInvalidValuePair to think these values were invalid.
+      value->mPropertyID = aProperty.IsFixed() ? aProperty.AsFixed()
+                                               : eCSSPropertyExtra_variable;

In that sense I guess there is no guarantee right now that this is the right property ID.

What do you think about making it so that this always returns false unless we have a custom property (so we don't change existing behavior), and for custom properties setting mPropertyID to eCSSPropertyExtra_variable? I think in that case it would be cleanest to move this patch to after Part 29, although that would mean that the animation support for non-interpolable properties only implemented by Part 29 would not work until this patch was applied.
(Assignee)

Comment 272

10 months ago
mozreview-review-reply
Comment on attachment 8773501 [details]
Bug 1273706 - Part 9: Expose nsComputedDOMStyle::SetValueToStyleImage as a static method so that we can compute serialized computed values for gradients.

https://reviewboard.mozilla.org/r/66162/#review70148

> Can you document in the comment when aThis must be provided?  Maybe call it something like "aPercentageBaseGetterObject" or "aPercentageBaseGetterThis" to make it clear?

Yes, that is a good idea.
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 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 303

10 months ago
- Renamed mBaseURL, mSheetURL in CSSVariableExprContext to mBaseURI, mSheetURI.
I remember changing them from mBaseURI to mBaseURL earlier at some point, but I can't remember why anymore, and it looks like every place we create / use them the other variables are called URIs.
- Fixed indentation in places where we constructed CSSVariableExprContext (was previously just called VariableExprContext).
- Replaced instances of |RefPtr<nsIAtom> custom; custom = p.AsCustom(); custom->ToString| with just |p.AsCustom()->ToString|.
Was trying to replace RefPtrs with nsCOMPtrs but realized I could do this instead.
- Fixed indentation in headers after |namespace mozilla| in another place (should not have indentation).
- Fixes corresponding to review comments (marked as Fixed).
(Reporter)

Comment 304

10 months ago
mozreview-review-reply
Comment on attachment 8777231 [details]
Bug 1273706 - Part 27: Add CSSPropertySet.

https://reviewboard.mozilla.org/r/68828/#review70182

> Thanks! Didn't know that.
> 
> I was thinking we needed the value here to preserve the refcount on the nsIAtom. Ordinarily I think we could just get rid of it, because the person can only query if they have the pointer anyways, but the IterCustomProps method which we use for animations requires that we give them the nsIAtom.
> 
> Given that, is there some way to make a set that keeps the refcount? (Don't know if nsPtrHashKey does).

Oh, sorry, I should have suggested to use nsRefPtrHashKey, rather than nsPtrHashKey.  The former will hold a strong reference to the object.
(Reporter)

Comment 305

10 months ago
mozreview-review-reply
Comment on attachment 8773499 [details]
Bug 1273706 - Part 14: Store custom property registrations on the document.

https://reviewboard.mozilla.org/r/66158/#review70156

> Sorry, should have clarified.
> The reason I put it on nsDocument instead of nsIDocument is that nsIDocument is that nsIDocument is included in some external-linkage code.
> This is a problem because having CSSVariableRegistrations as a member requires we have the implementation of CSSVariableRegistration, which contains a string.
> It appears that external-linkage code uses different strings (from nsStringAPI.h) than internal-linkage code (from nsString.h).
> We can't just add an ifdef of our own to switch between the headers because we need to access those same registrations from internal-linkage code (e.g. all of the other code), and I think that would make the data incompatible.
> dholbert and I decided that making it a virtual method on the nsIDocument interface and then implementing it on nsDocument, which is not included in any external-linkage code, was the cleanest solution, beacuse by returning the CSSVariableRegistrations pointer we don't need access to the implementation, and hence don't need to worry about the different strings.
> 
> Do you think this seems OK?
> 
> Also, you are rgiht about the #ifdef. Didn't notice that.

OK, thanks for the explanation.  That sounds reasonable!
(Reporter)

Comment 306

10 months ago
mozreview-review
Comment on attachment 8773508 [details]
Bug 1273706 - Part 21: Have CSSVariableValues store more information.

https://reviewboard.mozilla.org/r/66176/#review70616

::: layout/style/CSSVariableResolver.cpp:161
(Diff revision 8)
>      }
>      mOutput->Put(mVariables[aID].mVariableName, resolvedValue,
> +                 // Replaced by a later patch in the series.
> +                 nsCSSValue(), CSSValueType(), CSSVariableExprContext(),
>                   firstToken, lastToken);
> +                 

Trailing space.

::: layout/style/CSSVariableValues.h:69
(Diff revision 8)
>     * @param aFirstToken The type of token at the start of the variable value.
>     * @param aLastToken The type of token at the en of the variable value.
>     * @return Whether a variable with the given name was found.  When false
>     *