Open Bug 1273706 (css-properties-values-api) Opened 8 years ago Updated 11 months ago

[META] implement Houdini CSS Properties and Values API

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
REOPENED

People

(Reporter: heycam, Unassigned)

References

(Depends on 11 open bugs, Blocks 7 open bugs,
URL
)

Details

(Keywords: dev-doc-needed, meta, Whiteboard: [layout:backlog:2020])

Attachments

(36 files)

Part 1: Track a separate list of exposed property order on Declarations. (WIP)
(deleted), patch
Details | Diff | Splinter Review
Part 2: Store normal and important variable declarations in the one object. (WIP)
(deleted), patch
Details | Diff | Splinter Review
Part 3: Add CSSVariableSyntax class to represent the types of syntax CSS Custom Properties can be defined to take. (WIP)
(deleted), patch
Details | Diff | Splinter Review
Part 4: (WIP)
(deleted), patch
Details | Diff | Splinter Review
jyc-wip-patches.zip
(deleted), application/zip
Details
p15-idempotence-and-supports
(deleted), text/plain
Details
Bug 1273706 - Part 1: Add missing includes exposed by unification.
(deleted), text/x-review-board-request
Details
Bug 1273706 - Part 2: Add missing namespace names that the Windows build complained about.
(deleted), text/x-review-board-request
Details
Bug 1273706 - Part 3: Add new pref to enable the Properties & Values API.
(deleted), text/x-review-board-request
Details
Bug 1273706 - Part 4: Add new nsCSSValue units for resolutions.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 5: Factor out code for parsing resolutions in nsCSSParser.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 6: Add CSSProperty type for custom properties.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 7: Add support to StyleAnimationValues for storing lists of values.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 8: Separate nsStyleImage from nsStyleStruct.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 12: Add a new type for computed CSS values.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 10: Expose StyleAnimationValue::StyleCoordToValue and StyleCoordToCSSValue.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 11: Have StyleAnimationValue::UncomputeValue(..., nsCSSValue) uncompute UnparsedStrings to token stream nsCSSValues.
(deleted), text/x-review-board-request
dbaron
: review+
Details
Bug 1273706 - Part 14: Store custom property registrations on the document.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 13: Add CSSVariableSyntax for representing the syntax of a custom property.
(deleted), text/x-review-board-request
dbaron
: review+
Details
Bug 1273706 - Part 9: Expose nsComputedDOMStyle::SetValueToStyleImage as a static method so that we can compute serialized computed values for gradients.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 15: Add nsCSSProps::LookupCustomProperty for looking up a CSSProperty given a custom property name and registrations.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 16: Store custom property registrations on nsCSSParser.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 17: Add an aAllowVariableReferences parameter to ParseVariableDeclaration.
(deleted), text/x-review-board-request
dbaron
: review-
Details
Bug 1273706 - Part 18: Add the |ParseTypedValue| method for parsing typed CSS values.
(deleted), text/x-review-board-request
dbaron
: review-
Details
Bug 1273706 - Part 19: Add support for custom properties in supports conditions.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 20: Store important and unimportant declarations in one object.
(deleted), text/x-review-board-request
heycam
: review-
Details
Bug 1273706 - Part 21: Have CSSVariableValues store more information.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 22: Add WebIDL bindings for the Properties & Values API.
(deleted), text/x-review-board-request
dbaron
: review+
Details
Bug 1273706 - Part 23: Update CSSVariableResolver for custom properties.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 24: Set initial values for custom properties on the root.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 25: Implement CSS.registerProperty and CSS.unregisterProperty.
(deleted), text/x-review-board-request
dbaron
: review-
Details
Bug 1273706 - Part 29: Implement animations for custom properties.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 27: Add CSSPropertySet.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 28: Have nsComputedDOMStyle try to get a computed value first for variables.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 26: Implement lazy computation of computed CSS registered property values on CSSVariableValues.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1273706 - Part 30: Add preliminary tests for the Properties & Values API implementation.
(deleted), text/x-review-board-request
heycam
: review-
Details 
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.
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.
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.
Attached patch Part 4: (WIP) (deleted) — — Splinter Review 
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)
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)
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.
Attached file jyc-wip-patches.zip (deleted) — 

    
    

    
  

      
      
      
      

        Attached file
          
        p15-idempotence-and-supports (deleted)
        — 
      

    


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

    
    

    
  
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.
Depends on: 1285365

    
    

    
  
(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

    
    

    
  


  
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)

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
  


  
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/

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

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

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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+

    
    

    
  
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+

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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+

    
  

        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.

    
    

    
  
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.

    
    

    
  
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!

    
    

    
  
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!

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

        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?

    
    

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

    
    

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

    
    

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

    
    

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

    
    

    
  
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
Closed: 8 years ago
Resolution: --- → INVALID

    
    

    
  
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.

    
  
Status: RESOLVED → REOPENED
Resolution: INVALID → ---

    
    

    
  


  
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)

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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/

    
    

    
  
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+

    
  

        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.

    
  

        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-

    
    

    
  
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?

    
    

    
  
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!

    
    

    
  
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.

    
    

    
  
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?

    
    

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

    
    

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

    
  

        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.

    
    

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

    
    

    
  
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.

    
    

    
  
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-

    
    

    
  
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+

    
    

    
  
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+

    
    

    
  
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+

    
    

    
  
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.
Depends on: 1293739
Depends on: 1293743

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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!
Blocks: 1294863

    
    

    
  
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 .

    
    

    
  
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.

    
    

    
  
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.

    
    

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

    
    

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

    
    

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

    
    

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

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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.

    
    

    
  
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+

    
    

    
  
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+

    
    

    
  
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+

    
    

    
  
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.

    
    

    
  
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+

    
    

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

    
    

    
  
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.

    
    

    
  
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!

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

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

    
    

    
  
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.

    
    

    
  
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!

    
    

    
  
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
>     *   is returned, aValue, aFirstToken and aLastToken will not be modified.
>     */
> -  bool Get(const nsAString& aName,
> +  bool Get(const nsAString& aName, nsString& aExpr, nsCSSValue& aValue,

If changing the other Get to take an nsAString&, may as well do this one too.

::: layout/style/CSSVariableValues.h:122
(Diff revision 8)
> -    Variable()
> -      : mFirstToken(eCSSTokenSerialization_Nothing)
> +    explicit Variable(CSSVariableExprContext aContext)
> +      : mContext(aContext)
> +      , mFirstToken(eCSSTokenSerialization_Nothing)
>        , mLastToken(eCSSTokenSerialization_Nothing)
>      {}

It looks like we don't use this constructor.  Can we remove it?

::: layout/style/CSSVariableValues.h:132
(Diff revision 8)
>  
>      Variable(const nsAString& aVariableName,
> -             nsString aValue,
> +             nsString aExpr,
> +             nsCSSValue aValue,
> +             CSSValueType aType,
> +             CSSVariableExprContext aContext,

Make this |const CSSVariableExprContext&| just in case the compiler decides not to skip one of the copy constructions?

        Attachment #8773508 -
        Flags: review?(cam) → review+

    
    

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

https://reviewboard.mozilla.org/r/66172/#review70626

        Attachment #8773506 -
        Flags: review?(cam) → 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/#review70852

r=dbaron with the following addressed

::: layout/style/CSSVariableSyntax.cpp:42
(Diff revision 8)
> +};
> +
> +bool
> +CSSVariableSyntax::SetSyntax(const nsAString& aSyntax)
> +{
> +  MOZ_ASSERT(mType == Type::Invalid, "SetSyntax already called");

probably also assert that mTerms.IsEmpty()?

::: layout/style/CSSVariableSyntax.cpp:54
(Diff revision 8)
> +
> +  CSSValueType termType = CSSValueType::Length;
> +  uint32_t termVariant = 0;
> +  nsString termSpecificIdent;
> +  bool termList = false;
> +  bool termPending = false;

I think you can just remove this variable, since it's equivalent to whether state is AfterCloseAngleOrIdent.  You can just test state.  (Right?)

(You could even consider renaming the state AfterCloseAngleOrIdent to TermPending!)

::: layout/style/CSSVariableSyntax.cpp:59
(Diff revision 8)
> +  bool termPending = false;
> +  // afterBar is true if we have already seen '|'.
> +  // This means we can't allow '*'.
> +  bool afterBar = false;
> +
> +  Type type;

I think it's probably worth initializing this to Type::Invalid, and then asserting below in all the cases that set type that type is either Invalid or the thing it's about to be set to (or at least something roughly like that; that might not quite be right).

I think that's also useful given the assertion about type at the bottom of the loop.

::: layout/style/CSSVariableSyntax.cpp:64
(Diff revision 8)
> +
> +  while (scanner.Next(token, eCSSScannerExclude_None)) {

I think it's worth adding a FIXME comment above this parsing algorithm that the spec details aren't fully worked out, and we need to follow future developments in https://github.com/w3c/css-houdini-drafts/issues/112

::: layout/style/CSSVariableSyntax.cpp:102
(Diff revision 8)
> +      case State::AfterOpenAngle:
> +        if (token.mType == eCSSToken_Ident) {
> +          bool foundType = false;
> +
> +          for (auto& entry : kTerms) {
> +            if (token.mIdent.Equals(NS_ConvertASCIItoUTF16(entry.mString))) {

please use EqualsASCII(entry.mString) instead of Equals(NS_ConvertASCIItoUTF16(entry.mString))

::: layout/style/CSSVariableSyntax.cpp:112
(Diff revision 8)
> +            }
> +          }
> +
> +          // Two special cases for the syntaxes that don't have VARIANT_* tags.
> +
> +          if (token.mIdent.Equals(NS_ConvertASCIItoUTF16("resolution"))) {

here, and 4 lines below, please use EqualsLiteral("...") instead of Equals(NS_ConvertASCIItoUTF16("..."))

::: layout/style/CSSVariableSyntax.cpp:112
(Diff revision 8)
> +          if (token.mIdent.Equals(NS_ConvertASCIItoUTF16("resolution"))) {
> +            termType = CSSValueType::Resolution;
> +            foundType = true;
> +          }
> +          if (token.mIdent.Equals(NS_ConvertASCIItoUTF16("transform-function"))) {
> +            termType = CSSValueType::TransformFunction;
> +            foundType = true;
> +          }
> +
> +          if (!foundType) {
> +            return false;
> +          }

Seems more efficient to make this an if-else chain:

if (foundType) {
  // do nothing
} else if (token.mIdent.EqualsLiteral("resolution") {
  ... // no need to set foundType in here
} else if (token.mIdent.EqualsLiteral("transform-function") {
  ... // no need to set foundType in here
} else {
  return false;
}

::: layout/style/CSSVariableSyntax.cpp:147
(Diff revision 8)
> +          break;
> +        } else if (token.IsSymbol('+')) {
> +          termList = true;
> +          state = State::AfterPlus;
> +        } else if (token.IsSymbol('|')) {
> +          // will handle later

Why?  Those 2 lines look like they could move here without any problem.

        Attachment #8773500 -
        Flags: review?(dbaron) → 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/#review70858

::: layout/style/CSSVariableSyntax.cpp:112
(Diff revision 8)
> +          if (token.mIdent.Equals(NS_ConvertASCIItoUTF16("resolution"))) {
> +            termType = CSSValueType::Resolution;
> +            foundType = true;
> +          }
> +          if (token.mIdent.Equals(NS_ConvertASCIItoUTF16("transform-function"))) {
> +            termType = CSSValueType::TransformFunction;
> +            foundType = true;
> +          }
> +
> +          if (!foundType) {
> +            return false;
> +          }

Actually, one more comment here.  You should just ditch having this as code, and make these be entries in the table with the variant field as 0, since that is equivalent to what this code does (Right?).

    
    

    
  
Also, if heycam wrote the patch, he should be the author of the mercurial commit.  IF you both did, then you should both be listed, separated by ", ".

    
    

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

https://reviewboard.mozilla.org/r/66168/#review70860

I'd like to take a look at the revised patch, but I think it should be pretty straightforward.

(I still don't know what this is going to be used for, but I'll find out soon...)

::: layout/style/nsCSSParser.cpp:17282
(Diff revision 8)
>    if (!ParseValueWithVariables(&type, &dropBackslash, impliedCharacters,
> -                               nullptr, nullptr)) {
> +                               aAllowVariableReferences ?
> +                                 nullptr : &RecordVariableReference,
> +                               &gotDisallowedVariableReference) ||
> +      gotDisallowedVariableReference) {

This is awfully ugly, for three reasons:
 1. it looks ugly
 2. it uses a function called ParseValueWithVariables in a way that variables aren't allowed
 3. it doesn't give the developer a useful error message

Instead, maybe:
 1. rename ParseValueWithVariables to ParseTokenStreamValue
 2. give it a boolean aAllowVariableReferences parameter
 3. handle aAllowVariableReferences being false right before the GetToken that's a little before the if (aFunc) test by doing the same thing that's in the REPORT_UNEXPECTED_TOKEN() branch between them
 4. add an actual error message to css.properties so that you can use a REPORT_UNEXPECTED_TOKEN with an appropriate error message

        Attachment #8773504 -
        Flags: review?(dbaron) → review-

    
    

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

https://reviewboard.mozilla.org/r/66168/#review70860

> This is awfully ugly, for three reasons:
>  1. it looks ugly
>  2. it uses a function called ParseValueWithVariables in a way that variables aren't allowed
>  3. it doesn't give the developer a useful error message
> 
> Instead, maybe:
>  1. rename ParseValueWithVariables to ParseTokenStreamValue
>  2. give it a boolean aAllowVariableReferences parameter
>  3. handle aAllowVariableReferences being false right before the GetToken that's a little before the if (aFunc) test by doing the same thing that's in the REPORT_UNEXPECTED_TOKEN() branch between them
>  4. add an actual error message to css.properties so that you can use a REPORT_UNEXPECTED_TOKEN with an appropriate error message

when I said "the same thing", I meant without the UngetToken, since it will be before the GetToken

    
    

    
  
(In reply to Cameron McCormack (:heycam) from comment #304)
> Comment on attachment 8777231 [details]
> Bug 1273706 - Part 27: Add CSSPropertySet.
> 
> https://reviewboard.mozilla.org/r/68828/#review70182
> > 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.

Thanks! Didn't know about that.

    
    

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

https://reviewboard.mozilla.org/r/66168/#review70860

> when I said "the same thing", I meant without the UngetToken, since it will be before the GetToken

You are right -- will do that. Thanks!

    
    

    
  
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #310)
> Also, if heycam wrote the patch, he should be the author of the mercurial
> commit.  IF you both did, then you should both be listed, separated by ", ".

Thanks, didn't realize that. Will add heycam.

    
    

    
  
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/#review70852

> I think you can just remove this variable, since it's equivalent to whether state is AfterCloseAngleOrIdent.  You can just test state.  (Right?)
> 
> (You could even consider renaming the state AfterCloseAngleOrIdent to TermPending!)

Yes, you are right. Thanks for noticing this!

> I think it's probably worth initializing this to Type::Invalid, and then asserting below in all the cases that set type that type is either Invalid or the thing it's about to be set to (or at least something roughly like that; that might not quite be right).
> 
> I think that's also useful given the assertion about type at the bottom of the loop.

That sounds like a good idea.

> I think it's worth adding a FIXME comment above this parsing algorithm that the spec details aren't fully worked out, and we need to follow future developments in https://github.com/w3c/css-houdini-drafts/issues/112

Neat, thanks.

> Why?  Those 2 lines look like they could move here without any problem.

I think you are correct.

    
    

    
  
Comment on attachment 8781031 [details]
Bug 1273706 - Part 28: Have nsComputedDOMStyle try to get a computed value first for variables.

https://reviewboard.mozilla.org/r/71530/#review70892

        Attachment #8781031 -
        Flags: review?(cam) → review+

    
    

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

https://reviewboard.mozilla.org/r/66170/#review71202

Given the comments, I'd like to look at a revised version of this patch.

::: layout/style/nsCSSParser.h:325
(Diff revision 8)
> +  bool ParseTypedValue(const mozilla::CSSVariableSyntax& aSyntax,
> +                       const nsAString& aBuffer,
> +                       nsIURI* aSheetURL,
> +                       nsIURI* aBaseURL,
> +                       nsIPrincipal* aSheetPrincipal,
> +                       nsCSSValue& aValue,
> +                       mozilla::CSSValueType& aType);

There should be a brief comment saying what this does.

A comment should point out that both aValue and Type are "out" (outputs).

::: layout/style/nsCSSParser.cpp:310
(Diff revision 8)
>     * Parses aPropertyValue as a CSS token stream value and resolves any
>     * variable references using the variables in aVariables.
>     *
>     * @param aPropertyValue The CSS token stream value.
>     * @param aVariables The set of variable values to use when resolving variable
> -   *   references.
> +   *   references. Can be null, in which case no variables will be resolved.

Can -> May

::: layout/style/nsCSSParser.cpp:1377
(Diff revision 8)
> +  bool ParseTypedValue(const CSSVariableSyntax& aSyntax,
> +                       bool aAllowVariableReferences,
> +                       nsCSSValue& aValue,
> +                       mozilla::CSSValueType& aType);

Again, comment about aValue and aType being outputs with "/* out */"

::: layout/style/nsCSSParser.cpp:3143
(Diff revision 8)
> +static bool
> +TryParseInherit(nsCSSToken aToken, nsCSSValue& aValue)
> +{
> +  // TODO(jyc) Add support for the 'revert' keyword here and in the resolver.
> +  if (aToken.mType == eCSSToken_Ident) {
> +    if (aToken.mIdent.Equals(NS_LITERAL_STRING("initial"))) {
> +      aValue.SetInitialValue();
> +      return true;
> +    }
> +    if (aToken.mIdent.Equals(NS_LITERAL_STRING("inherit"))) {
> +      aValue.SetInheritValue();
> +      return true;
> +    }
> +    if (aToken.mIdent.Equals(NS_LITERAL_STRING("unset"))) {
> +      aValue.SetUnsetValue();
> +      return true;
> +    }
> +  }
> +  return false;
> +}

This duplicates a bunch of code that exists elsewhere.

I think you should remove this function, and also remove all calls to it, and instead, at the *beginning* of ParseSingleTypedValue (before any type checks or GetToken calls, do:

if (ParseSingleTokenVariant(aValue, VARIANT_INHERIT, nullptr)) {
  return true;
}

::: layout/style/nsCSSParser.cpp:3178
(Diff revision 8)
> +    if (mToken.mType == eCSSToken_Ident &&
> +        mToken.mIdent.Equals(aTerm.GetIdentifier())) {
> +      aValue.SetStringValue(mToken.mIdent, eCSSUnit_Ident);
> +      return true;
> +    }
> +    return TryParseInherit(mToken, aValue);

You need an UngetToken() call here if this fails, since parsing functions shouldn't consume tokens that they don't actually parse.  (Otherwise they'll accept errors that they shouldn't in some particular cases.)

::: layout/style/nsCSSParser.cpp:3179
(Diff revision 8)
> +        mToken.mIdent.Equals(aTerm.GetIdentifier())) {
> +      aValue.SetStringValue(mToken.mIdent, eCSSUnit_Ident);
> +      return true;
> +    }
> +    return TryParseInherit(mToken, aValue);
> +  } else if (aTerm.GetType() == CSSValueType::CustomIdent) {

All of the "else if"s in this function should just be "if"s since they're preceded by returns (which per my earlier comment should just be "return false" rather than "return TryParseInherit".

::: layout/style/nsCSSParser.cpp:3187
(Diff revision 8)
> +    }
> +    if (mToken.mType == eCSSToken_Ident) {
> +      aValue.SetStringValue(mToken.mIdent, eCSSUnit_Ident);
> +      return true;
> +    }
> +    return TryParseInherit(mToken, aValue);

also need an UngetToken here before returning false

::: layout/style/nsCSSParser.cpp:3189
(Diff revision 8)
> +      aValue.SetStringValue(mToken.mIdent, eCSSUnit_Ident);
> +      return true;
> +    }
> +    return TryParseInherit(mToken, aValue);
> +  } else if (aTerm.GetType() == CSSValueType::Resolution) {
> +    return ParseResolution(aValue) || TryParseInherit(mToken, aValue);

You *don't* need an UngetToken here, since your existing code was incorrect, and the TryParseInherit wouldn't have consumed the token it was testing!

::: layout/style/nsCSSParser.cpp:3191
(Diff revision 8)
> +    // Try to parse with aIsPrefixed = false then true.
> +    return ParseSingleTransform(false, false, aValue) ||
> +           ParseSingleTransform(true, false, aValue) ||
> +           TryParseInherit(mToken, aValue);

You shouldn't try with aIsPrefixed true at all.  Just the single return with (false, false, aValue).  And no UngetToken(), since this TryParseInherit was also not consuming the token it was testing.

::: layout/style/nsCSSParser.cpp:3197
(Diff revision 8)
> +  uint32_t variant = aTerm.GetVariant();
> +  MOZ_ASSERT(variant);
> +
> +  // VARIANT_INHERIT to allow for inherit, initial, and unset.
> +  // In the future, need to allow revert too.
> +  return ParseVariant(aValue, variant | VARIANT_INHERIT, nullptr) == CSSParseResult::Ok;

You should add a comment pointing out that converting the CSSParseResult to a boolean is OK since the caller restores the old input state on failure.  (And really you should probably say the same thing above the ParseSingleTransform call.)

::: layout/style/nsCSSParser.cpp:3219
(Diff revision 8)
> +    if (type != CSSVariableDeclarations::Type::eTokenStream) {
> +      return false;
> +    }

This will cause rejection of initial, inherit, and unset.  That seems wrong.

::: layout/style/nsCSSParser.cpp:3254
(Diff revision 8)
> +          if (item) {
> +            item->mNext = nullptr;
> +          }

It's already null; no need to set this.

::: layout/style/nsCSSParser.cpp:3260
(Diff revision 8)
> +        if (!ParseSingleTypedValue(term, value)) {
> +          ok = false;
> +          break;
> +        }

This means that you allow lists to be terminated only by EOF.  That seems wrong; it means they can't occur within a style sheet.  (It's OK for the ParseTypedValue API introduced in that patch -- but is that really going to be the only path?  I suppose maybe it is since it's all reparsing of old token streams?)

Fixing this probably means making ParseSingleTypedValue return CSSParseResult rather than boolean.

Or if it is what is intended, it requires very clear comments on the limits of the function.

        Attachment #8773505 -
        Flags: review?(dbaron) → review-

    
    

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

https://reviewboard.mozilla.org/r/66178/#review71220

::: dom/webidl/PropertyDescriptorDict.webidl:13
(Diff revision 8)
> +// Renamed from PropertyDescriptor to avoid conflicting with a JS class of the
> +// same name.

Does this need to be spec feedback?  Or is the name of dictionaries not exposed to the Web?

(Maybe this is a question for heycam.)

::: dom/webidl/PropertyDescriptorDict.webidl:18
(Diff revision 8)
> +           DOMString syntax;
> +           boolean inherits;

Do we put the initial values in the spec's WebIDL here?  Or is there a reason we're not supposed to?

        Attachment #8773509 -
        Flags: review?(dbaron) → review+

    
    

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

https://reviewboard.mozilla.org/r/66184/#review71254

I'd like to see the revisions that address these comments.

::: layout/style/CSS.cpp:64
(Diff revision 8)
> +{
> +  // The inner window (the script compilation context)
> +  nsGlobalWindow* win = xpc::WindowOrNull(aGlobal.Get());
> +  if (!win) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCOMPtr<nsIDocument> doc = win->GetDoc();
> +  if (!doc) {
> +    return NS_ERROR_FAILURE;
> +  }

Instead of doing this all again, could you just have an nsCOMPtr<nsIDocument> member variable of the ParsingInfo, so you can call this function on the document.

::: layout/style/CSS.cpp:79
(Diff revision 8)
> +  }
> +
> +  nsIPresShell* shell;
> +  nsPresContext* pcx;
> +  if ((shell = doc->GetShell()) && (pcx = shell->GetPresContext())) {
> +    pcx->RebuildAllStyleData(NS_STYLE_HINT_REFLOW, eRestyle_Subtree);

Passing these parameters as nonzero requires justification.  I think the use of eRestyle_Subtree is probably correct since changing the registrations causes changes in the computed values of custom properties.  However, NS_STYLE_HINT_REFLOW should almost certainly be nsChangeHint(0) instead.

And the name of the function should be RebuildStyleDataForRegisteredProperties or something more specific, since it is passing particular parameters to RebuildAllStyleData.



Second, and even more importantly, this call is unnecessarily synchronous, which means this will do multiple rebuilds for multiple registrations.  You should instead call PostRebuildAllStyleDataEvent.

::: layout/style/CSS.cpp:83
(Diff revision 8)
> +  if ((shell = doc->GetShell()) && (pcx = shell->GetPresContext())) {
> +    pcx->RebuildAllStyleData(NS_STYLE_HINT_REFLOW, eRestyle_Subtree);
> +    return NS_OK;
> +  }
> +
> +  return NS_ERROR_FAILURE;

I don't think having a null pres shell justifies throwing an exception here.  Not currently having a presentation means there's no style to rebuild.  This function should just return void.

::: layout/style/CSS.cpp:90
(Diff revision 8)
> +
>  /* static */ bool
>  CSS::Supports(const GlobalObject& aGlobal,
>                const nsAString& aProperty,
>                const nsAString& aValue,
> -              ErrorResult& aRv)
> +              mozilla::ErrorResult& aRv)

Leave this as-is.

::: layout/style/CSS.cpp:147
(Diff revision 8)
> +static bool
> +IsIdempotentValue(const nsCSSValue& aValue)

I don't know what it means for a value to be idempotent; idempotence is a characteristic of functions.  This could use a comment and perhaps a better name.

The spec uses "computationally idempotent", which is still incorrect terminology.  (I filed https://github.com/w3c/css-houdini-drafts/issues/282 .)

::: layout/style/CSS.cpp:150
(Diff revision 8)
> +  if (aValue.IsRelativeLengthUnit()) {
> +    // Initial values need to be computationally idempotent.
> +    // NB: Variables are not computationally idempotent!
> +    // That's caught by ParseTypedValue, which will parse them as a var
> +    // function (which doesn't exist). Variables would have been resolved by
> +    // ResolveVariableValue (see CSSVariableResolver).
> +    return false;
> +  }

This comment doesn't appear to be germane to the code that it's commenting.  I think it should be above the if at the top of the function.

::: layout/style/CSS.cpp:188
(Diff revision 8)
> +      const nsCSSValue& arg = func->Item(1 + argID);
> +      if (!IsIdempotentValue(arg)) {
> +        return false;
> +      }
> +    }
> +    return true;

This seems like a strong statement to make for all functions.  I think you should have assertions that this is only operating on a limited set of known functions that are allowed in the values of typed properties.

::: layout/style/CSS.cpp:201
(Diff revision 8)
> +      }
> +      list = list->mNext;
> +    }
> +  }
> +
> +  return true;

I think you need much stronger assertions for me to believe this.

::: layout/style/CSS.cpp:227
(Diff revision 8)
> +  nsCSSToken token;
> +  if (!scanner.Next(token, eCSSScannerExclude_None) ||
> +      token.mType != eCSSToken_Ident) {

You also need to check that the scanner has consumed all of its input!

::: layout/style/CSS.cpp:234
(Diff revision 8)
> +      token.mType != eCSSToken_Ident) {
> +    aRv.ThrowDOMException(NS_ERROR_DOM_SYNTAX_ERR);
> +    return;
> +  }
> +
> +  if (info.mRegistrations->mData.Contains(name)) {

I think you actually need to use the identifier from the token so that you process CSS escaping correctly (and same for unregister!), but please see what happens on https://github.com/w3c/css-houdini-drafts/issues/283 which I just filed.

::: layout/style/CSS.cpp:253
(Diff revision 8)
> +  registration->mInherited = (aDescriptor.mInherits.WasPassed() &&
> +                              aDescriptor.mInherits.Value()) ||
> +                             false;

drop the "|| false" at the end, and the extra () needed for it

::: layout/style/CSS.cpp:258
(Diff revision 8)
> +  // If no initialValue is provided and the syntax is *, then a special initial
> +  // value used [sic]. This initial value must be considered parseable by
> +  // registerProperty() but invalid at computed value time.

Perhaps this comment should go in the else?

::: layout/style/CSS.cpp:259
(Diff revision 8)
> +                              aDescriptor.mInherits.Value()) ||
> +                             false;
> +  registration->mAtom = NS_Atomize(name);
> +
> +  // If no initialValue is provided and the syntax is *, then a special initial
> +  // value used [sic]. This initial value must be considered parseable by

I added the missing "is" to the spec, so you can add that and remove the "[sic]".

::: layout/style/CSS.cpp:284
(Diff revision 8)
> +
> +    registration->mInitialExpr = aDescriptor.mInitialValue.Value();
> +    registration->mInitialValue = value;
> +    registration->mInitialType = type;
> +
> +    nsAutoString _result;

Don't use a name starting with _.

::: layout/style/CSS.cpp:285
(Diff revision 8)
> +    MOZ_ASSERT(parser.ResolveVariableValue(aDescriptor.mInitialValue.Value(),
> +                                           // There should be no variables!
> +                                           // Caught by ParseTypedValue.
> +                                           nullptr, _result,
> +                                           registration->mInitialFirstToken,
> +                                           registration->mInitialLastToken));

This will fail to run in non-DEBUG builds.  You want a DebugOnly<bool> resolveResult = ...
followed by MOZ_ASSERT(resolveResult).

::: layout/style/CSS.cpp:323
(Diff revision 8)
> +  if (!nsCSSProps::IsCustomPropertyName(aName)) {
> +    aRv.ThrowDOMException(NS_ERROR_DOM_SYNTAX_ERR);
> +    return;
> +  }
> +
> +  // First two characters are guaranteed to be -- by the previous guard.
> +  const nsDependentSubstring name = Substring(aName, CSS_CUSTOM_NAME_PREFIX_LENGTH);
> +
> +  // Verify that the part after -- is a valid ident, as required.
> +  nsCSSScanner scanner(name, /* aLineNumber = */ 0);
> +  nsCSSToken token;
> +  if (!scanner.Next(token, eCSSScannerExclude_None) ||
> +      token.mType != eCSSToken_Ident) {
> +    aRv.ThrowDOMException(NS_ERROR_DOM_SYNTAX_ERR);
> +    return;
> +  }

Is all of this really needed?  Why not just look up the name in the existing registrations?

        Attachment #8773512 -
        Flags: review?(dbaron) → review-

    
    

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

https://reviewboard.mozilla.org/r/66174/#review71280

This is looking good, but I'd like to see the resulting patch after addressing these comments.  Thanks!


The commit message looks a bit jumbled.  Maybe needs some re-ordering?

::: layout/style/CSSVariableDeclarations.h:55
(Diff revision 8)
> -  /**
> -   * Gets the value of a variable in this set of variable declarations.
> +  bool Get(const nsAString& aName, Type& aType, nsAString& aExpr,
> +           bool& aImportant, CSSVariableExprContext& aContext) const;
> -   *
> -   * @param aName The variable name (not including any "--" prefix that would
> -   *   be part of the custom property name).
> -   * @param aType Out parameter into which the type of the variable value will
> -   *   be stored.
> -   * @param aValue Out parameter into which the value of the variable will
> -   *   be stored.  If the variable is 'initial', 'inherit' or 'unset', this will
> -   *   be the empty string.
> -   * @return Whether a variable with the given name was found.  When false
> -   *   is returned, aType and aValue will not be modified.
> -   */

Can we keep/add comments on these methods that get and set the variable declarations?  (Among other things, I think it's important to keep pointing out, for example, that aName does not have the "--" prefix.)

::: layout/style/CSSVariableDeclarations.h:58
(Diff revision 8)
> -  /**
> -   * Adds or modifies an existing entry in this set of variable declarations
> +  bool IsUsedDecl(size_t aID) const;
> +  void GetDeclName(size_t aID, nsAString& aName) const;

Please document these to explain how IDs are used, now that they are part of CSSVariableDeclaration's public interface.

::: layout/style/CSSVariableDeclarations.h:108
(Diff revision 8)
> +  typedef nsDataHashtable<nsStringHashKey, size_t>::Iterator Iterator;
> +  Iterator Iter() const;
> +
>    size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;
>  
> +  void SetImmutable() const { mImmutable = true; }

Can you explain why we need to record immutability on the Declaration?  I feel like we might not need this.  Is it just as an optimization, to skip the RecomputeValidities work if we know that the Declaration can't have tried to modify our contents?

On Declaration we have IsImmutable / EnsureMutable that we use to implement copy-on-write semantics, but we don't have similar methods on CSSVariableDeclarations.  Since all modifications of CSSVariableDeclarations should come from methods on Declaration, I think we can just rely on its handling of mImmutable for us.

::: layout/style/CSSVariableDeclarations.h:134
(Diff revision 8)
> +
> +    size_t mVarID;
> +    Type mType;
> +    nsString mExpr;
> +    bool mImportant;
> +    bool mOverrideImportant;

Can you add a comment explaining why we need to track mOverrideImportant, and when it gets used (for CSSOM setProperty() isn't it?)?

::: layout/style/CSSVariableDeclarations.h:149
(Diff revision 8)
> +    mutable nsCSSTokenSerializationType mFirstToken;
> +    mutable nsCSSTokenSerializationType mLastToken;
> +    mutable bool mParsed;
> +  };
> +
> +  RefPtr<const CSSVariableRegistrations> mRegistrations;

mRegistrations is only used within AddVariableDeclaration, to pass it to the CSSVariableDeclarations constructor.

Instead of storing the CSSVariableRegistrations pointer on every Declaration, can we instead just pass it in to AddVariableDeclarations?  There are only a few callers of AddVariableDeclarations: two in nsCSSParser, where we can just call GetVariableRegistrations; and BuildStyleRule in StyleAnimationValue.cpp, where we're currently creating a Declaration with no CSSVariableRegistrations anyway.

::: layout/style/CSSVariableDeclarations.h:174
(Diff revision 8)
> +  // |mVariableIDs| but doesn't touch anything else (in particular,
> +  // |mVariableNames| still holds the old variable name).
> +  // That means that mVariableIDs is not injective if a specific variable name
> +  // is removed and then added back.
> +  // This seems to be the simplest thing to do for now, even though it might
> +  // waste space (as declarations and other info relevant to the variable rior

s/rior/prior/

::: layout/style/CSSVariableDeclarations.cpp:16
(Diff revision 8)
> -#include "CSSVariableResolver.h"
> -#include "nsCSSScanner.h"
> +#include "mozilla/CSSVariableRegistration.h"
> +#include "mozilla/CSSVariableResolver.h"
> +#include "nsCSSParser.h"
>  #include "nsRuleData.h"
>  
> -// These three special string values are used to represent specified values of
> +#ifdef CSSVAR_DEBUG

Doesn't matter too much, but it seems to be the pattern to use |#ifdef DEBUG_BLAH| rather than |#ifdef BLAH_DEBUG| for component-specific debug code.

::: layout/style/CSSVariableDeclarations.cpp:47
(Diff revision 8)
> +  , mHasImportant(aOther.mHasImportant)
> +  , mImmutable(false)
> +  , mNeverComputed(true)
>  {
>    MOZ_COUNT_CTOR(CSSVariableDeclarations);
> -  CopyVariablesFrom(aOther);
> +  mVariableIDs.Clear();

mVariableIDs should already be empty.

::: layout/style/CSSVariableDeclarations.cpp:118
(Diff revision 8)
>  
> -void
> -CSSVariableDeclarations::PutTokenStream(const nsAString& aName,
> +bool
> +CSSVariableDeclarations::IsUsedDecl(size_t aID) const
> -                                        const nsString& aTokenStream)
>  {
> -  MOZ_ASSERT(!aTokenStream.EqualsLiteral(INITIAL_VALUE) &&
> +  RecomputeValidities();

Can we call RecomputeSingleValidity here, since we only need to ensure that we've updated validities for variable declarations aID corresponds to?

::: layout/style/CSSVariableDeclarations.cpp:169
(Diff revision 8)
> +      // TODO(jyc) Should we even keep it around at this point? Actually, can
> +      // this even happen? I think a parse error would be caught earlier.

If this is true (and I think it is), let's assert |parsed| and return false.

::: layout/style/CSSVariableDeclarations.cpp:185
(Diff revision 8)
> -  mVariables.Put(aName, NS_LITERAL_STRING(INHERIT_VALUE));
> +      // It might seem excessive parsing three times here. But unless
> +      // nsCSSParser is restructured, we certainly need the first two, and
> +      // using three here prevents us from using five total for properly typed
> +      // declarations (2 to enumerate then parse here and 3 in
> +      // CSSVariableResolver to enumerate, resolve, then parse).

Hmm, yes that does seem unfortunate that we have to parse the string three times. :(  Please file a follow-up bug to reduce the amount of parsing we do here.

::: layout/style/CSSVariableDeclarations.cpp:191
(Diff revision 8)
> +      // nsCSSParser is restructured, we certainly need the first two, and
> +      // using three here prevents us from using five total for properly typed
> +      // declarations (2 to enumerate then parse here and 3 in
> +      // CSSVariableResolver to enumerate, resolve, then parse).
> +      nsAutoString result;
> +      DebugOnly<bool> ok = 

Nit: trailing space.

::: layout/style/CSSVariableDeclarations.cpp:247
(Diff revision 8)
> +  // TODO(jyc) We don't clean up the garbage declarations. Should we? Doesn't
> +  // seem like removing properties completely is a very common thing.
> +  // We also don't remove mHasImportant, but we didn't remove it before either.

I think it's probably fine to leave them.  Good to leave a comment pointing out that we do, though.

::: layout/style/CSSVariableDeclarations.cpp:270
(Diff revision 8)
> +    if (aRuleData->mVariables->Has(name)) {
> +      // MapRuleInfoInto is called on all the applicable rules (ours is a parent
> +      // Declaration) in order of *decreasing* specificity.
> +      // If there's already an entry, it was put by a more specific applicable
> +      // rule.

Since we are careful to only add a single variable declaration (the winning one from this CSSVariableDeclarations, and never overwriting an existing one) to aRuleData->mVariables, I wonder if we should not call Has(), which will perform validity updates, but instead poke at mVariableIDs and mUsedDeclarations on aRuleData->mVariables directly?  Actually I think we can just do aRuleData->mVariables->mVariableIDs->Contains(name), since we know that aRuleData->mVariables->mUsedDeclarations would (if we updated validity) have a value other than -1 at every element.

::: layout/style/CSSVariableDeclarations.cpp:310
(Diff revision 8)
> +      CSSVAR_PRINTF("Not adding because no usable declaration.\n");
> +      continue;
> +    }
> +    const Declaration& decl = mDeclarations[declID];
> +    switch (decl.mType) {
> +    case Type::Initial:

Nit which you can ignore since this will change in a later patch: switch cases should be indented a level.

::: layout/style/CSSVariableDeclarations.cpp:348
(Diff revision 8)
>  {
> +  // TODO(jyc) verify
>    size_t n = aMallocSizeOf(this);
> -  n += mVariables.ShallowSizeOfExcludingThis(aMallocSizeOf);
> -  for (auto iter = mVariables.ConstIter(); !iter.Done(); iter.Next()) {
> +  n += mDeclarations.ShallowSizeOfExcludingThis(aMallocSizeOf);
> +  for (const auto& decl : mDeclarations) {
> +    n += aMallocSizeOf(&decl);

The size of the Declaration objects themselves should already be counted in the mDeclarations.ShallowSizeOfExcludingThis() call, as that includes the array buffer allocation.

::: layout/style/CSSVariableDeclarations.cpp:352
(Diff revision 8)
> -  for (auto iter = mVariables.ConstIter(); !iter.Done(); iter.Next()) {
> +  for (const auto& decl : mDeclarations) {
> +    n += aMallocSizeOf(&decl);
> +    n += decl.mExpr.SizeOfExcludingThisIfUnshared(aMallocSizeOf);
> +  }
> +  n += mUsedDeclarations.ShallowSizeOfExcludingThis(aMallocSizeOf);
> +  n += sizeof(size_t) * mUsedDeclarations.Length();

We should drop this too.

::: layout/style/CSSVariableDeclarations.cpp:354
(Diff revision 8)
> +    n += decl.mExpr.SizeOfExcludingThisIfUnshared(aMallocSizeOf);
> +  }
> +  n += mUsedDeclarations.ShallowSizeOfExcludingThis(aMallocSizeOf);
> +  n += sizeof(size_t) * mUsedDeclarations.Length();
> +  n += mVariableGenerations.ShallowSizeOfExcludingThis(aMallocSizeOf);
> +  n += sizeof(uint32_t) * mVariableGenerations.Length();

And this.

::: layout/style/CSSVariableDeclarations.cpp:357
(Diff revision 8)
> +  n += sizeof(size_t) * mUsedDeclarations.Length();
> +  n += mVariableGenerations.ShallowSizeOfExcludingThis(aMallocSizeOf);
> +  n += sizeof(uint32_t) * mVariableGenerations.Length();
> +  for (auto iter = mVariableIDs.ConstIter(); !iter.Done(); iter.Next()) {
>      n += iter.Key().SizeOfExcludingThisIfUnshared(aMallocSizeOf);
> -    n += iter.Data().SizeOfExcludingThisIfUnshared(aMallocSizeOf);
> +    n += sizeof(size_t);

And this.

::: layout/style/CSSVariableDeclarations.cpp:359
(Diff revision 8)
> +  if (mRegistrations) {
> +    n += aMallocSizeOf(mRegistrations);
>    }

I don't think we want to measure mRegistrations on every CSSVariableDeclarations object, as it's shared between them.  Instead, we would just measure it somewhere in nsDocument::DocAddSizeOfExcludingThis, maybe accounting it against &aWindowSizes->mStyleSheetsSize or mDOMOtherSize.

::: layout/style/CSSVariableDeclarations.cpp:368
(Diff revision 8)
> +  if (aSecond == (size_t) -1) {
> +    return true;
> +  }

Assert in here that aFirst is not -1?  (We will get an assertion from the mDeclarations access, true, though as it's a special value it's probably useful to call this assertion out.)

::: layout/style/CSSVariableDeclarations.cpp:400
(Diff revision 8)
> +{
> +  if (!mRegistrations || (mImmutable && !mNeverComputed)) {
> +    return;
> +  }
> +  mNeverComputed = false;
> +  size_t varID = mVariableIDs.Get(aName);

Assert mVariableIDs.Contains(aName)?

::: layout/style/CSSVariableDeclarations.cpp:413
(Diff revision 8)
> +    }
> +    bool valid = ComputeValidity(mRegistrations->mData, aName, decl.mType,
> +                                 decl.mExpr, decl.mValue, decl.mValueType,
> +                                 decl.mContext, decl.mFirstToken,
> +                                 decl.mLastToken, decl.mParsed);
> +    if (valid && DeclPriorityGte(declID, mUsedDeclarations[varID])) {

DeclPriorityGte is cheaper than the ComputeValidity call.  Should we do it first?  Also, should we iterate mDeclarations in reverse, so that we don't have to call ComputeValidity for all variable declarations, and instead at most have to call it twice (once we encounter a first non-important declaration, then second when we encounter an important declaration)?

::: layout/style/CSSVariableDeclarations.cpp:438
(Diff revision 8)
> +                                 decl.mType, decl.mExpr, decl.mValue,
> +                                 decl.mValueType, decl.mContext,
> +                                 decl.mFirstToken, decl.mLastToken,
> +                                 decl.mParsed);
> +    CSSVAR_PRINTF("declID=%zu, varID=%zu, valid=%d\n", declID, varID, valid);
> +    if (valid && DeclPriorityGte(declID, mUsedDeclarations[varID])) {

Similarly here.

::: layout/style/Declaration.h:399
(Diff revision 8)
>    /**
>     * Returns the property at the given index in the ordered list of
>     * declarations.  For custom properties, eCSSPropertyExtra_variable
>     * is returned.
>     */
>    nsCSSPropertyID GetPropertyAt(uint32_t aIndex) const {

Looks like GetPropertyAt and GetPropertyNameAt are both only used inside Declaration.  Can you make them private?

::: layout/style/Declaration.h:441
(Diff revision 8)
>    // Subtracting eCSSProperty_COUNT from those values that represent custom
>    // properties results in an index into mVariableOrder, which identifies the
>    // specific variable the custom property declaration is for.

This comment should be updated now that we have removed mVariableOrder.

::: layout/style/Declaration.h:477
(Diff revision 8)
> +  // Custom property registrations of the owning window.
> +  // TODO(jyc) Do we change this to the owning document?
> +  // We do not own this. Only used by |mVariables|.
> +  RefPtr<const CSSVariableRegistrations> mRegistrations;

Per above hopefully we can remove this.

::: layout/style/Declaration.cpp:108
(Diff revision 8)
>  /* virtual */ bool
>  Declaration::MightMapInheritedStyleData()
>  {
>    MOZ_ASSERT(mData, "must call only while compressed");
>    if (mVariables && mVariables->Count() != 0) {
> +    // Some custom properties might be inherited. All variables are.

Not sure what the distinction you are drawing between custom properties and variables is here.  Is this comment out of date now that we are storing non-registered and registered custom properties in the same style struct?  Please update to clarify.

::: layout/style/Declaration.cpp:121
(Diff revision 8)
>  {
>    MOZ_ASSERT(mData, "must call only while compressed");
>    MOZ_ASSERT(HasImportantData(), "must only be called for Declarations with "
>                                   "important data");
> -  if (mImportantVariables && mImportantVariables->Count() != 0) {
> +  if (mVariables && mVariables->Count() != 0 && mVariables->HasImportant()) {
> +    // Some custom properties might be inherited. All variables are.

Here too.

::: layout/style/Declaration.cpp:1812
(Diff revision 8)
> -  }
>  
> -  if (!aIsImportant && !aOverrideImportant &&
> -      mImportantVariables && mImportantVariables->Has(aName)) {
> +  // Hope we don't get too many declarations...
> +  size_t szDeclID = mVariables->Add(aName, aType, aValue, aIsImportant,
> +                                    aOverrideImportant, aContext);
> +  if (szDeclID > (uint32_t) -1) {

I don't think this will be effective on 32-bit builds, since the condition will always be false.  However, nsTArrays cannot grow to be longer than 2**31 elements anyway, so I think we can just safely cast here and not bother checking.

::: layout/style/Declaration.cpp:1829
(Diff revision 8)
> -    if (!mImportantVariables) {
> -      mImportantVariables = new CSSVariableDeclarations;
> +  AutoTArray<uint32_t, 8> oldOrder = mOrder;
> +  mOrder.Clear();

Do:

  nsTArray<uin32_t> oldOrder;
  mOrder.SwapElements(oldOrder);

If we spilled over mOrder's 8 element auto buffer into a dynamically allocated buffer, we can avoid copying it and just update oldOrder to point to that buffer.

::: layout/style/Declaration.cpp:1835
(Diff revision 8)
> -      mImportantVariables = new CSSVariableDeclarations;
> -    }
> -    variables = mImportantVariables;
> -  } else {
> -    if (mImportantVariables) {
> -      mImportantVariables->Remove(aName);
> +  mOrder.Clear();
> +  for (uint32_t id : oldOrder) {
> +    if (id >= eCSSProperty_COUNT) {
> +      uint32_t declID = id - eCSSProperty_COUNT;
> +      nsAutoString declName;
> +      mVariables->GetDeclName(declID, declName);

Don't we need to null check mVariables here too?  Can we just return early at the top of the function if mVariables is null?

::: layout/style/Declaration.cpp:1852
(Diff revision 8)
> +  return mVariables ? mVariables->Get(aName, type, value, important, context) && important
> +                    : false;

Might be nice to have a method on CSSVariableDeclarations that can look up a variable's importance without having to copy all the other aspects of the declaration.

::: layout/style/nsDOMCSSAttrDeclaration.cpp:130
(Diff revision 8)
> +  CSSParsingEnvironment env;
> +  GetCSSParsingEnvironment(env);
> +
>    // cannot fail
> -  RefPtr<css::Declaration> decl = new css::Declaration();
> +  RefPtr<css::Declaration> decl =
> +    new css::Declaration(CSSVariableRegistrationsOfDocument(env.mDocument));

If we no longer pass CSSVariableRegistrations to the Declaration constructor, we should be able to drop the changes to nsDOMCSS{,Attr}Declaration.{h,cpp} and nsSVGElement.cpp.

        Attachment #8773507 -
        Flags: review?(cam) → review-

    
    

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

https://reviewboard.mozilla.org/r/66180/#review67466

r=me with these things addressed.

From the commit message:

> In this case, the problem was that nsStyleContext::CalcStyleDifference compared
> mVariables in nsStyleVariables but not mHasUninherited. So even if one of a
> parent style struct's variables became uninherited, CalcStyleDifference would
> return that they were the same, and as a result not restyle child style
> contexts. So nsStyleContext.cpp:960 now compares mHasUninherited as well.

Do you have a test case that breaks without this?  If so, please include it with your tests patch (if you haven't already).  I think we can be a bit more specific here about how to handle changes in variables.  If the two mVariables compare equals, but mHasUninherited does not match, then I think we can still proceed with |compare = false|, since the actual variable values are the same and so all the other structs shouldn't have their properties affected by variable changes.  However we do need to ensure the aEqualStructs bit is not set if mHasUninherited doesn't match, as that will cause problems with some optimizations that RestyleManager does.

::: layout/style/nsRuleNode.cpp:10344
(Diff revision 5)
>  
>    COMPUTE_END_RESET(Effects, effects)
>  }
>  
> +static CSSComputedValue::SingleTerm
> +ComputeCSSValueTerm(const nsCSSValue& aValue, const nsAString& aExpr,

We don't seem to use aExpr.  Don't bother passing it in?

::: layout/style/CSSVariableResolver.h:111
(Diff revision 8)
> -    bool mInStack;
> -    size_t mIndex;
> -    size_t mLowLink;
>    };
>  
> -  /**
> +  void PutInherited(const nsAString& aVariableName,

Can you add some comments explaining the difference between PutInherited, PutSpecifiedUnparsed and PutSpecifiedParsed (i.e. something like the previous comment that mentioned where these functions are intended to be called from)?

Can you (in this or in the previous patch that added it) document CSSVariableDeclarations::Declaration::mParsed?  I take it it means "whether we parsed the string into an nsCSSValue because we had a registered custom property and the value didn't have any variable references".  Is that right?

::: layout/style/CSSVariableResolver.h:139
(Diff revision 8)
> +  };
> +
> +  bool HandleVisitNode(size_t aID, VisitNodeResult aResult);
>  
>    // Helper functions for Resolve.
> -  void RemoveCycles(size_t aID);
> +  VisitNodeResult VisitNode(size_t aID, bool& aHasUninherited);

Should HandleVisitNode go down here under the "Helper functions for Resolve" too?

::: layout/style/CSSVariableResolver.h:141
(Diff revision 8)
>    // A mapping of variable names to an ID that indexes into mVariables
>    // and mReferences.

This comment needs updating if we've removed mReferences.

::: layout/style/CSSVariableResolver.h:156
(Diff revision 8)
>    // The object to output the resolved variables into.
>    CSSVariableValues* mOutput;
>  
> +  RefPtr<const CSSVariableRegistrations> mRegistrations;
> +
> +  nsTArray<bool> mVisited;

May as well put a |bool mVisited| on Variable itself?

::: layout/style/CSSVariableResolver.h:164
(Diff revision 8)
>    // Whether Resolve has been called.
>    bool mResolved;
>  #endif
> +
> +private:
> +  inline size_t

No need to say "inline", since that's implied by the method body being here.

::: layout/style/CSSVariableResolver.cpp:32
(Diff revision 8)
> -  {
> -    size_t id;
> -    if (mResolver.mVariableIDs.Get(aVariableName, &id)) {
> -      mReferences[id] = true;
>      } else {
> -      mReferencesNonExistentVariable = true;
> +      forceUseInitial = true;

When do we get in here?  Can you add a comment?

::: layout/style/CSSVariableResolver.cpp
(Diff revision 8)
> -
> -  // The only time we would be worried about having a null aInherited is
> -  // for the root, but in that case nsRuleNode::ComputeVariablesData will
> -  // happen to pass in whatever we're using as mOutput for aInherited,
> -  // which will initially be empty.

I think this comment is still true.  Is there value in removing it?

::: layout/style/CSSVariableResolver.cpp:83
(Diff revision 8)
> -  for (size_t id = 0; id < n; id++) {
> -    data.Reset();
> -    if (!mVariables[id].mWasInherited &&
> +        // This variable was specified or inherited. It's initial value will be
> +        // populated, if at all (we don't want to if there is a compute-time
> +        // error) in the VisitNodeResult::UseInitial branch below.

s/It's/Its/

s/it at all/if it needs to be/ ?

::: layout/style/CSSVariableResolver.cpp:89
(Diff revision 8)
> -                                              &data)) {
> -        // Convert the boolean array of dependencies in |data| to a list
> -        // of dependencies.
> +        // All custom properties initially have their initial value (even those
> +        // never specified or declared anywhere!) If we are supposed to load an
> +        // initial value, then, we don't actually want to set anything.

Is this the case that handles custom properties registered with type "*" with an empty token stream?  If so, can you assert the type?

Might be clearer to extend the second sentence of the comment with something like "... set anything, since the absence of a declaration represents this invalid initial value"?

::: layout/style/CSSVariableResolver.cpp:93
(Diff revision 8)
> -        // in the resolver, so that we can ensure we still resolve its value
> -        // in ResolveVariable, even though its mReferences list is empty.
> -        mVariables[id].mReferencesNonExistentVariable =
> -          data.ReferencesNonExistentVariable();
>        } else {
> -        MOZ_ASSERT(false, "EnumerateVariableReferences should not have failed "
> +        const CSSVariableRegistration* registration = iter.UserData();

Maybe pull out this |registration| declaration to just before the if statement, so you can use it in the first branch's condition?

::: layout/style/CSSVariableResolver.cpp:164
(Diff revision 8)
> +  // At this point we know we're going to do something with the specified
> +  // declaration (even if it turns out to be erroneous).
> +  // We have to set aHasUninherited if this is an uninherited custom property so
> +  // that our children don't inherit our decision.
> +  if (registration && !registration->mInherited) {
> +    aHasUninherited = true;

Nit: I wonder if we can use the term "non-inherited" rather than "uninherited".  The latter sounds to me like something is done to undo the inheritance, and the former seems to be used in existing code.

::: layout/style/CSSVariableResolver.cpp:184
(Diff revision 8)
> +    return VisitNodeResult::Done;
> +  }
> +
> +  if (entry.mType != CSSVariableDeclarations::Type::TokenStream) {
> +    switch (entry.mType) {
> +    case CSSVariableDeclarations::Type::Initial:

Nit: indent cases one level.

::: layout/style/CSSVariableResolver.cpp:192
(Diff revision 8)
> +        // Custom properties are always uninherited, and unset => inherited if
> +        // the property is inherited.

Do you mean "Unregistered custom properties are always inherited"?

::: layout/style/CSSVariableResolver.cpp:210
(Diff revision 8)
> +  for (size_t refID = 0; refID < entry.mRefs.Length(); refID++) {
> +    const nsString& ref = entry.mRefs[refID];

for (const nsString& ref : entry.mRefs) {

?

::: layout/style/CSSVariableResolver.cpp:240
(Diff revision 8)
> +    // A declaration can be invalid at computed-value time if it contains a
> +    // var() that references a custom property with its initial value, as

"... references a token-stream typed custom property ..."?

::: layout/style/CSSVariableResolver.cpp:259
(Diff revision 8)
> +    }
> +    return VisitNodeResult::UseInitial;
> +  }
> +
> +  if (!registration) {
> +    // Variables that aren't custom properties are token stream values.

I think this terminology distinction here is confusing.  I believe variables are the abstract thing that can be referenced by var()s, and custom properties are the way to provide their values.  So here I think we want to say:

  Unregistered custom properties take token stream values.

::: layout/style/CSSVariableResolver.cpp:270
(Diff revision 8)
> +    nsCSSValue value = nsCSSValue(tokenStream);
> +    mOutput->Put(var.mName, expr, value, CSSValueType::TokenStream,

nsCSSValue value(tokenStream);

should be sufficient.

::: layout/style/CSSVariableResolver.cpp:287
(Diff revision 8)
> +                               entry.mContext.mSheetURI,
> +                               entry.mContext.mBaseURI,
> +                               entry.mContext.mSheetPrincipal,
> +                               value, type)) {
> +    // Type of expanded value is incorrect. This expansion doesn't work!
> +    // Try to fall back to the inherited value.

I guess this comment should say "Try to fall back to the inherited or initial value, depending on whether the property is registered as inherited".  But also, I think you can remove the "Try to" since there's no way this can fail?

::: layout/style/CSSVariableResolver.cpp:310
(Diff revision 8)
> -                         nsCSSTokenSerializationType aFirstToken,
> +                                  nsCSSTokenSerializationType aFirstToken,
> -                         nsCSSTokenSerializationType aLastToken,
> +                                  nsCSSTokenSerializationType aLastToken)
> -                         bool aWasInherited)
>  {
> -  MOZ_ASSERT(!mResolved);
> +  size_t id = LookupOrAddVariable(aVariableName);
> +  mVariables[id].mInherited =

Should we assert that mVariables[id].mInherited is currently None (and in PutSpecified{Parsed,Unparsed}, that mSpecified is None)?

::: layout/style/CSSVariableResolver.cpp:329
(Diff revision 8)
> +             auto maybe = static_cast<Maybe<Entry>*>(aData);
> +             maybe->ptr()->mRefs.AppendElement(aName);
> +           }, &(mVariables[id].mSpecified))) {

Perhaps pass in mVariables[id].mSpecified.ptr() as the aData argument, so we don't have to deal with the Maybe inside the lambda?

::: layout/style/nsCSSParser.cpp:2119
(Diff revision 8)
> -                                                             mBaseURI,
> +                                                                mBaseURI,
> -                                                             mSheetPrincipal));
> +                                                                mSheetPrincipal));

(Ah, I thought you left these misaligned in the previous patch deliberately, to stay below 80 columns.)

::: layout/style/nsRuleNode.cpp:2445
(Diff revision 8)
>      detail = eRulePartialMixed; // Treat as though some data is specified to avoid
>                                  // the optimizations and force data computation.
>  
>    if (detail == eRuleNone && startStruct) {
> +    MOZ_ASSERT(aSID != eStyleStruct_Variables,
> +               "It shouldn't be possible to be here for the variables style, "

s/variables style/Variables struct/

::: layout/style/nsRuleNode.cpp:2472
(Diff revision 8)
> +    // Note: for variables, this means that we didn't specify anything at all
> +    // (see CheckVariablesCallback).

Mention in this comment that it's because we store both inherited and non-inherited custom properties in the Variables struct?

::: layout/style/nsRuleNode.cpp:2501
(Diff revision 8)
> +      const void* parentStruct = parentContext->StyleData(aSID);
> +      bool share = true;
> +
> +      if (aSID == eStyleStruct_Variables) {
> +        const nsStyleVariables* parentVariables = static_cast<const
> +          nsStyleVariables*>(parentStruct);
> +        if (parentVariables->mHasUninherited) {
> +          share = false;
> +        }
> +      }
> +
> +      if (share) {

Nit: just to keep things a bit more compact in here, how about formatting like this:

  const void* parentStruct = ...;
  if (aSID != eStyleStruct_Variables ||
      static_cast<const nsStyleVariables*>(parentStruct)->mHasUninherited) {
    // We have a parent, and so ...
    aContext->AddStyleBit(...
    ...
  }

and then extend the comment to say mention that for the Variables struct, we additionally need to check that we don't have any non-inherited custom properties stored in it.

::: layout/style/nsRuleNode.cpp:10177
(Diff revision 8)
> -  MOZ_ASSERT(aRuleData->mVariables,
> -             "shouldn't be in ComputeVariablesData if there were no variable "
> -             "declarations specified");
> -
> -  CSSVariableResolver resolver(&variables->mVariables);
> -  resolver.Resolve(&parentVariables->mVariables,
> -                   aRuleData->mVariables);
> +  CSSVariableDeclarations* specified;
> +  if (!aRuleData->mVariables) {
> +    // We must have been called because our parent set some uninherited custom
> +    // properties.
> +    specified = new CSSVariableDeclarations(nullptr);
> +  } else {
> +    specified = aRuleData->mVariables;
> +  }

I think CSSVariableResolver::Resolve doesn't need to require that aSpecified is not null.  I think the existing code just asserts that (and doesn't null check aSpecified before the AddVariablesToResolver call) just because previously it could never be null.

So how about we just make Resolve() accept a null aSpecified, and then not worry about creating an empty CSSVariableDeclarations and deleting it later, here.

::: layout/style/nsRuleNode.cpp:10190
(Diff revision 8)
> +
> +  const CSSVariableRegistrations* registrations =
> +    CSSVariableRegistrationsOfStyleContext(aContext);
> +  CSSVariableResolver resolver(&variables->mVariables, registrations);
> +
> +  resolver.Resolve(&parentVariables->mVariables, specified, variables->mHasUninherited);

Please wrap to keep to 80 columns.

        Attachment #8773510 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8781032 [details]
Bug 1273706 - Part 26: Implement lazy computation of computed CSS registered property values on CSSVariableValues.

https://reviewboard.mozilla.org/r/71532/#review71356

::: layout/style/CSSVariableValues.h:67
(Diff revision 3)
> +   * @param aName The variable name (sans "--" prefix).
> +   * @return True iff |aName| corresponds to a registered custom property.
> +   * Computed values of unregistered variables are just their token stream
> +   * values, which can be retrieved using Get.
> +   */
> +  bool Compute(const nsAString& aName, const CSSComputedValue** aValue) const;

Can we just return the |const CSSComputedValue*| instead of setting it through an outparam?  Then return null on failure.

::: layout/style/CSSVariableValues.h:194
(Diff revision 3)
> +
> +  /**
> +   * nsStyleContext used to compute variable values.
> +   * It's a regular pointer because we expect that this is owned by the style context:
> +   * nsStyleContext -> nsStyleVariables -> CSSVariableValues
> +   */
> +  nsStyleContext* mStyleContext;
> +
> +  /**
> +   * nsPresContext used to compute variable values.
> +   */
> +  RefPtr<nsPresContext> mPresContext;

Is there a way we can avoid storing these pointers?  For the style context one, I am slightly worried that work such as bug 1188721 will allow the style context to be destroyed if we share the Variables struct among other style contexts.

Can we pass these two things in to Compute(), which is the function that needs mStyleContext and mPresContext, instead?  There looks to be only two places where we call Compute().  In both nsComputedDOMStyle::DoGetCustomProperty and ExtractComputedCustomValue, we have the style context pointer.  And from that we can get the pres context.

So perhaps just add a single nsStyleContext* parameter to Compute() and get the pres context from it.

::: layout/style/CSSVariableValues.cpp:137
(Diff revision 3)
> +      nsTArray<CSSComputedValue::SingleTerm> outTerms;
> +      while (inTerms) {
> +        auto term = ComputeCSSValueTerm(inTerms->mValue, type, mStyleContext,
> +                                        mPresContext);
> +        MOZ_ASSERT(term.GetType() == type);
> +        outTerms.AppendElement(term);

Can you |outTerms.AppendElement(Move(term));| here?

::: layout/style/nsRuleNode.cpp:10230
(Diff revision 3)
> +namespace mozilla {
> +
> +CSSComputedValue::SingleTerm
> +ComputeCSSValueTerm(const nsCSSValue& aValue, CSSValueType aType,
> +                    nsStyleContext* aStyleContext,
> +                    nsPresContext* aPresContext)

Could drop the aPresContext and get it from aStyleContext here too.

::: layout/style/nsRuleNode.cpp:10242
(Diff revision 3)
> +  // We don't actually keep track of these. The Variables struct is always set
> +  // to uncacheable. In the future, this could be optimized.
> +  RuleNodeCacheConditions conditions;
> +
> +  switch (aType) {
> +  case CSSValueType::Number:

Nit: indent cases.

::: layout/style/nsRuleNode.cpp:10245
(Diff revision 3)
> +
> +  switch (aType) {
> +  case CSSValueType::Number:
> +    if (aValue.GetUnit() == eCSSUnit_Calc) {
> +      mozilla::css::ReduceNumberCalcOps ops;
> +      float number = mozilla::css::ComputeCalc(aValue, ops);

Just |css::ComputeCalc|.

::: layout/style/nsRuleNode.cpp:10253
(Diff revision 3)
> +    // This is the messiest case, but it's actually pretty simple.
> +    // It just looks messy because of the overlap between values that can be
> +    // <length>, <percentage>, and <length-percentage>.

We might be able to shorten this by taking advantage of the fact that all combinations of aType and aValue.GetUnit() produce an nsStyleCoord, and that aType alone determines which CSSComputedvalue constructor we use.  Something like this?

  nsStyleCoord coord;

  if (aValue.IsLengthUnit()) {
    // call nsRuleNode::CalcLength and set coord
  } else if (aValue.GetUnit() == eCSSUnit_Percent) {
    // set coord
  } else {
    // call ComputeCalc and set coord
  }

  switch (aType) {
    case CSSValueType::Length:
      return CSSComputedValue::Length(coord);
    case CSSValueType::Percentage:
      return CSSComputedValue::Percentage(coord);
    case CSSValueType::LengthPercentage:
      return CSSComputedValue::LengthPercentage(coord);
  }

WDYT?

::: layout/style/nsRuleNode.cpp:10262
(Diff revision 3)
> +      nscoord computed = nsRuleNode::CalcLength(aValue, aStyleContext,
> +                                                aPresContext, conditions);
> +      nsStyleCoord coord;
> +      coord.SetCoordValue(computed);
> +      switch (aType) {
> +      case CSSValueType::Length:

Nit: indent cases.

::: layout/style/nsRuleNode.cpp:10288
(Diff revision 3)
> +    if (aValue.GetUnit() == eCSSUnit_Calc) {
> +      LengthPercentPairCalcOps ops(aStyleContext, aPresContext, conditions);
> +      nsRuleNode::ComputedCalc computed = ComputeCalc(aValue, ops);
> +      switch (aType) {
> +      case CSSValueType::Length: {
> +        MOZ_ASSERT(computed.mPercent == 0);

Assert |!ops.mHasPercent|?

::: layout/style/nsRuleNode.cpp:10342
(Diff revision 3)
> +  case CSSValueType::Integer:
> +    if (aValue.GetUnit() == eCSSUnit_Calc) {
> +      mozilla::css::ReduceIntegerCalcOps ops;
> +      int integer = mozilla::css::ComputeCalc(aValue, ops);
> +      return CSSComputedValue::Integer(nsStyleCoord(integer,
> +                                                    eStyleUnit_Integer));
> +    } else {
> +      return CSSComputedValue::Integer(nsStyleCoord(aValue.GetIntValue(),
> +                                                    eStyleUnit_Integer));
> +    }

Perhaps this could be merged with the Length/Percentage/LengthPercentage cases above?

::: layout/style/nsRuleNode.cpp:10364
(Diff revision 3)
> +    for (size_t argID = 0; argID < input->Count() - 1; argID++) {
> +      const nsCSSValue& arg = input->Item(1 + argID);
> +      // <transform-function> arguments can currently be <length>s,
> +      // <number>s, and <angle>s.

Should we really be computing down the argument values of the <transform-function>?  The spec says the computed value of a <transform-function> should be the same as the specified value.

Is there a reason this whole case for TransformFunction can't be just:

  case CSSvalueType::TransformFunction:
    return CSSComputedvalue::TransformFunction(aValue);

?

    
    

    
  
Comment on attachment 8781032 [details]
Bug 1273706 - Part 26: Implement lazy computation of computed CSS registered property values on CSSVariableValues.

https://reviewboard.mozilla.org/r/71532/#review71364

r=me with those things I mentioned in the previous comment.

        Attachment #8781032 -
        Flags: review?(cam) → review+

    
    

    
  
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #319)
> ::: dom/webidl/PropertyDescriptorDict.webidl:13
> (Diff revision 8)
> > +// Renamed from PropertyDescriptor to avoid conflicting with a JS class of the
> > +// same name.
> 
> Does this need to be spec feedback?  Or is the name of dictionaries not
> exposed to the Web?
> 
> (Maybe this is a question for heycam.)

Dictionary names are indeed not exposed to the web.  (The PropertyDescriptor class Jonathan refers to is a Gecko C++ struct name, not a class defined by the JS spec, so I don't think we need to submit spec feedback for this.)

    
    

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

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

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

Yep, that's OK.

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

Sounds good, thanks.

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

Yes, I think so.  This API is only used for tests.

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

That makes sense, what you have in the latest patch looks good.  Thanks!

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

OK, don't worry about it then.

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

I think it's safe to remove it.

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

OK, that makes sense.  I guess it depends on the context whether eCSSPropertyExtra_variable really means "an unregistered custom property" or just "any custom property", e.g. from the return value of nsCSSProps::LookupProperty.

    
    

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

https://reviewboard.mozilla.org/r/66174/#review71280

> Can you explain why we need to record immutability on the Declaration?  I feel like we might not need this.  Is it just as an optimization, to skip the RecomputeValidities work if we know that the Declaration can't have tried to modify our contents?
> 
> On Declaration we have IsImmutable / EnsureMutable that we use to implement copy-on-write semantics, but we don't have similar methods on CSSVariableDeclarations.  Since all modifications of CSSVariableDeclarations should come from methods on Declaration, I think we can just rely on its handling of mImmutable for us.

Oh OK, that makes sense. Wasn't intended as an optimization, I was just being paranoid about the comment:

  // Someone cares about this declaration's contents, so don't let it
  // change from under them.  See e.g. bug 338679.

I checked and it looks like you are right. I will get rid of it, and it simplifies the code too. Thanks.

    
    

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

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

r=me with these changes.

::: dom/animation/AnimValuesStyleRule.cpp:60
(Diff revision 8)
> +      // URL values need a principal to be set in the parser.
> +      // Otherwise we should be able to supply a context of all nulls (not
> +      // uninitialized!) because this is already a computed value.

Do we have some assertions somewhere that if we do go to compute a URL value, our context has been properly initialized?

::: dom/animation/AnimValuesStyleRule.cpp:112
(Diff revision 8)
> +      nsAutoString name;
> +      pair.mProperty.AsCustom()->ToString(name);
> +      str.AppendLiteral("--");
> +      AppendUTF16toUTF8(name, str);

ToString() already includes the "--", so no need to add it to str.

::: dom/animation/KeyframeEffect.cpp:851
(Diff revision 8)
>    nsAutoString invalidPacedProperty;
> +  const CSSVariableRegistrations* registrations =
> +    CSSVariableRegistrationsOfDocument(doc);
>    KeyframeEffectParams effectOptions =
> -    KeyframeEffectParamsFromUnion(aOptions, invalidPacedProperty, aRv);
> +    KeyframeEffectParamsFromUnion(aOptions, invalidPacedProperty,
> +                                  Move(registrations), aRv);

Doesn't make sense to use Move() here now that we're passing in a raw pointer.

::: dom/animation/KeyframeEffect.cpp:964
(Diff revision 8)
> -    printf("%s\n", nsCSSProps::GetStringValue(p.mProperty).get());
> +    if (p.mProperty.IsFixed()) {
> +      printf("%s\n", nsCSSProps::GetStringValue(p.mProperty.AsFixed()).get());
> +    } else {
> +      nsAutoString name;
> +      p.mProperty.AsCustom()->ToString(name);
> +      printf("--%s\n", NS_ConvertUTF16toUTF8(name).get());

Here too, no need for the additional "--".

::: dom/animation/KeyframeEffect.cpp:1147
(Diff revision 8)
> +        nsString utf16Name;
> +        propertyValue.mProperty.ToString(utf16Name);
> +        name = ToNewUTF8String(utf16Name);

This now leaks, since ToNewUTF8String allocates a new object.  Instead, do |name = NS_ConvertUTF16toUTF8(utf16name)|.

::: dom/animation/KeyframeEffect.cpp:1155
(Diff revision 8)
> -      const char* name = nsCSSProps::PropertyIDLName(propertyValue.mProperty);
> +      // nsCSSValue::AppendToString does not accept shorthand properties, but
> -
> -      // nsCSSValue::AppendToString does not accept shorthands properties but
>        // works with token stream values if we pass eCSSProperty_UNKNOWN as
>        // the property.
> +      // It works with variables if we supply eCSSProperty_UNKNOQN, because for

eCSSProperty_UNKNOWN

::: dom/animation/KeyframeEffect.cpp:1157
(Diff revision 8)
> -      // nsCSSValue::AppendToString does not accept shorthands properties but
>        // works with token stream values if we pass eCSSProperty_UNKNOWN as
>        // the property.
> +      // It works with variables if we supply eCSSProperty_UNKNOQN, because for
> +      // all types we might set the nsCSSValue to, we don't run into a case
> +      // where we need to know the specific proprty.

property

::: dom/animation/KeyframeEffectParams.h:36
(Diff revision 8)
> +        nsAutoString name, unprefixedName;
> +        mPacedProperty.AsCustom()->ToString(unprefixedName);
> +        name.AppendLiteral("--");
> +        name.Append(unprefixedName);
> +        aSpacing.Append(name);

No double "--" here too.

::: dom/animation/KeyframeUtils.cpp:12
(Diff revision 8)
>  
>  #include "mozilla/AnimationUtils.h"
> +#include "mozilla/CSSVariableRegistration.h"
> +#include "mozilla/CSSVariableRegistrations.h"
>  #include "mozilla/ErrorResult.h"
> +#include "mozilla/CSSPropertySet.h"

Nit: Move this up a few lines now that it's renamed.

::: dom/animation/KeyframeUtils.cpp:1046
(Diff revision 8)
> +    CSSVariableRegistration* registration;
> +    nsString name;
> +    aProperty.AsCustom()->ToString(name);
> +    if (registrations && registrations->mData.Get(name, &registration)) {

Does registrations->mData store property names with their "--" prefixes or not?  I think it's without, but ToString() will include it.

(I wonder if all of the places we store things based on the "--"-stripped name we should change to storing based on the atom.  Originally I stripped the "--" because it seemed wasteful to always be comparing those two initial characters, but if we need to atomize these strings anyway it might be better just to key things off the atoms.  File a followup for this?)

::: dom/base/nsDOMWindowUtils.cpp:2698
(Diff revision 8)
> +    property = CSSProperty(eCSSProperty_UNKNOWN);
> +  } else {
> +    property = CSSProperty(fixed);
>    }
>  
> -  MOZ_ASSERT(property == eCSSProperty_UNKNOWN ||
> +  MOZ_ASSERT(!property.IsShorthand(), "should not have shorthand");

property.IsShorthand() will assert since it calls nsCSSProps::IsShorthand(), which will assert on eCSSProperty_UNKNOWN.  So I think we make the assertion here be:

  property == eCSSProperty_UNKNOWN ||
  !property.IsShorthand()

::: layout/style/StyleAnimationValue.cpp:2964
(Diff revision 8)
> -    nsStyleStructID sid = nsCSSProps::kSIDTable[aProperty];
> +    nsStyleStructID sid =
> +      aProperty.IsFixed() ? nsCSSProps::kSIDTable[aProperty.AsFixed()]
> +                          : eStyleStruct_Variables;

We can replace this with

  nsStyleStructID sid = aProperty.GetStyleStructID();

like you've done elsewhere.

::: layout/style/StyleAnimationValue.cpp:3258
(Diff revision 8)
>      case eUnit_UnparsedString: {
>        RefPtr<nsCSSValueTokenStream> value = new nsCSSValueTokenStream;
> -      value->mPropertyID = aProperty;
> +      // 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;
>        aComputedValue.GetStringValue(value->mTokenStream);
>        aSpecifiedValue.SetTokenStreamValue(value);
>        break;

Why don't we need to fill in any other information on the nsCSSValueTokenStream?  I guess we don't have it, because eUnit_UnparsedString stores an nsStringBuffer.  Is it that when animating custom properties, we never need that additional information (base URI, principal, etc.)?  If so, can you please document that here, because it's not obvious.  I assume as well that we won't get into this path here for animation of fixed properties (as they will never have eUnit_UnparsedString values).  If that's right, can you please assert in here that |aProperty.IsCustom()|?

::: layout/style/nsAnimationManager.cpp:1028
(Diff revision 8)
> +    CSSProperty prop(cssprop);
>      if (!aAnimatedProperties.HasProperty(prop)) {
>        continue;
>      }
>  
> +    // If we hit either of these assertion, it probably means we are fetching a

*assertions

::: layout/style/nsAnimationManager.cpp:1041
(Diff revision 8)
> +  // Fill in missing keyframe values for custom properties.
> +  // It's possible for custom properties to have no appropriate value we can
> +  // fill in (i.e. if CSS.registerProperty is called with no initialValue).
> +  // Then we should remove this property from the animation altogether.

After discussing with Brian, I think we should allow animations that involve an underlying "invalid token stream" value.  So instead of dropping such animations when we detect them here, can we make AppendProperty succeed?

I filed https://github.com/w3c/css-houdini-drafts/issues/285 for that to be clear in the spec.

I'm happy to keep the changes to make AppendProperty return a boolean, though, rather than the old way of checking for Null to determine whether we encountered value that StyleAnimationValue didn't know about.

::: layout/style/nsComputedDOMStyle.cpp:6356
(Diff revision 8)
> +    } else {
> +      nsString name;
> +      prop.AsCustom()->ToString(name);
> +      property->SetString(name);
> +    }

I think we need to handle the name of the custom property here in the same way that we do for unknown properties, i.e. to use AppendEscapedCSSIdent on them so that the returned string is valid CSS.

        Attachment #8773513 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8781033 [details]
Bug 1273706 - Part 30: Add preliminary tests for the Properties & Values API implementation.

https://reviewboard.mozilla.org/r/71534/#review71632

These tests look good.  I have some comments, and suggestions for some additional tests, which I'd like to see another revision of the patch for.  Thanks!

::: layout/reftests/css-properties-and-values/properties-and-values-01a.html:7
(Diff revision 3)
> +CSS.registerProperty({name: "--a", syntax: "<color>", inherits: true, initialValue: "green"});
> +CSS.registerProperty({name: "--b", syntax: "<color>", inherits: false, initialValue: "red"});

For reftests, unless there is a specific need to use other colours, it's good to try to use green for passing things and red for failing things.  Here our last subtest uses red for success, which may confuse humans.

So ideally I think this test would be structured in a way that it produced three sentences that all should be green.

I would also suggest writing these tests in a way that they can be submitted upstream, like the web-platform-tests you've added, but I am not sure where Houdini specs are meant to go or whether these should be reftests or scripted tests.  So here is fine for now.

::: layout/reftests/css-properties-and-values/reftest.list:1
(Diff revision 3)
> +default-preferences pref(layout.css.properties_and_values.enabled,true)

Rather than setting the pref here, let's set it in testing/profiles/prefs_genera.js.  That will cause it to be set for both reftests and the web-platform-tests.

::: testing/web-platform/tests/css-houdini/properties-and-values.html:1
(Diff revision 3)
> +<!doctype html>

Some other tests I'd like to see are:

* test for the default value of |inherits| and of |syntax|
* test that |name| is required (there might one that I missed)
* test that the used declarations change appropriately after property registration/unregistration, including with some !important vs non-!important tests
* test that a registerProperty / unregisterProperty call can cause a transition to be triggered

::: testing/web-platform/tests/css-houdini/properties-and-values.html:3
(Diff revision 3)
> +<!doctype html>
> +<meta charset=utf-8>
> +<title></title>

Please include a useful title, and add a <link rel=help> pointing to the spec section that this test relates to.

I think this should really be split up into multiple test files.  I think a logical grouping would be:

* one for the animation tests
* one for the family tests
* one for the "initial value computational idempotency" tests
* one for the syntax exception tests
* one for the property name validity tests
* one for the register/unregister exception tests
* one for the computed value tests

You can have a helper .js file to reuse expectFail/expectSucced across tests.

Then you'll be able to point to more specific sections of the spec in each test file, from the <link rel=help>.

::: testing/web-platform/tests/css-houdini/properties-and-values.html:74
(Diff revision 3)
> +<a id="link" href="/"></a>
> +
> +<script>
> +let baseURL = document.getElementById("link").href;
> +
> +let animTests = [

Here are some additional animation tests I'd like to see:

* animation of a <number>- and <length>-typed custom property with calc() values
* animation of <color>-typed custom property, including something with currentColor
* animation of <image>-typed custom property (marked as an expected fail, if we indeed don't support that)
* animation of non-px <length> values (and <length>+ values) that tests how they compute down to px values
* animation between px, % and calc() values for <length-percentage>
* animation involving invalid token stream values
* animation of custom properties that are referenced by unanimated fixed properties, e.g. animate a --color and use it in background-color

::: testing/web-platform/tests/css-houdini/properties-and-values.html:249
(Diff revision 3)
> +  // Transform function arguments should be computed.
> +  { registrations: [
> +      { name: "--a",
> +        syntax: "<transform-function>",
> +        inherits: true,
> +      },
> +    ],
> +    parent: "font-size: 10px;",
> +    child: "--a: translateX(5em);",
> +    expected: {
> +      "--a": "translateX(50px)",
> +    },
> +  },

Per a comment on the previous patch, I think we shouldn't be computing transform function arguments down.

::: testing/web-platform/tests/css-houdini/properties-and-values.html:324
(Diff revision 3)
> +    },
> +  },
> +];
> +
> +
> +let idempotentPassTests = [

Is a value of 0% computationally idempotent?  Should be testing that (and a calc() that has 0% in it) in idempotentPassTests or idempotentFailTests, whichever it is.  Or file an issue and add a comment.

::: testing/web-platform/tests/css-houdini/properties-and-values.html:342
(Diff revision 3)
> +  { syntax: "<transform-function>", initialValue: "translateX(5em)" },
> +  { syntax: "<integer> | <length>+", initialValue: "5px 0 -5em 0 12px" },
> +  { syntax: "<color> | <transform-function>+", initialValue: "translateX(-10em)" },
> +];
> +
> +let invalidSyntaxTests = [

Should we add seemingly valid initialValues where we can, to ensure we throw for invalid syntax and not for an invalid "" initial value?

::: testing/web-platform/tests/css-houdini/properties-and-values.html:344
(Diff revision 3)
> +  { syntax: "<color> | <transform-function>+", initialValue: "translateX(-10em)" },
> +];
> +
> +let invalidSyntaxTests = [
> +  { syntax: "<number>+ <color>" },
> +  { syntax: "<i-don't-exist>+" },

Depending on how the spec eventually defines how to parse the syntax string, the apostrophe might open a CSS string.  How about calling it "<i-do-not-exist>" just to avoid it testing something other than you intend.

::: testing/web-platform/tests/css-houdini/properties-and-values.html:353
(Diff revision 3)
> +  // We don't support revert yet.
> +  //{ syntax: "revert" },

Rather than comment it out, leave it and add failure expectations to the .ini file.

::: testing/web-platform/tests/css-houdini/properties-and-values.html:358
(Diff revision 3)
> +  // We don't support revert yet.
> +  //{ syntax: "revert" },
> +  { syntax: "* worn out places" },
> +  { syntax: "* | worn | out | faces" },
> +  { syntax: "*+" },
> +];

How about one for "a | | b", i.e. checking that between the "|" operator we have something.

::: testing/web-platform/tests/css-houdini/properties-and-values.html:362
(Diff revision 3)
> +  { syntax: "<color>" },
> +  { syntax: " <color> " },
> +  { syntax: "<color>  +" },
> +
> +  // Allow some CSS keywords, just not the CSS-wide keywords or revert.
> +  { syntax: "center" },

Isn't the initialValue required when syntax isn't "*"?  Well, I guess the spec doesn't really say what to do when initialValue is not specified in that case, but I read it as not being valid.  Otherwise there's no obvious initial value to use, and I don't think we want to use the special invalid value.

Raised: https://github.com/w3c/css-houdini-drafts/issues/286

::: testing/web-platform/tests/css-houdini/properties-and-values.html:376
(Diff revision 3)
> +  { syntax: "<color>", initialValue: "5px" },
> +  { syntax: "<transform-function>", initialValue: "red" },
> +  { syntax: "<transform-function>+", initialValue: " " },
> +  { syntax: "<resolution>", initialValue: "calc(0)" },
> +  { syntax: "<custom-ident>", initialValue: "5px" },
> +  { syntax: "to-me", initialValue: "who-are-you" },

How about a test like { syntax: "a", initialValue: "A" }, to verify that idents are case sensitive.

::: testing/web-platform/tests/css-houdini/properties-and-values.html:390
(Diff revision 3)
> +let syntaxPassTests = [
> +  { syntax: "*", initialValue: "all around me are familiar faces" },
> +  { syntax: "*", initialValue: "translateX(5px) 5em 73" },
> +  { syntax: "<number>", initialValue: "5" },
> +  { syntax: "<number> | <color>", initialValue: "red" },
> +  { syntax: "<number>+", initialValue: "4 8 15 16 23 42" },

:thumbsup:

::: testing/web-platform/tests/css-houdini/properties-and-values.html:418
(Diff revision 3)
> +
> +let validNameTests = [
> +  { name: "--a" },
> +  { name: "--A" },
> +  { name: "--ç" },
> +  { name: "--\\n" },

As with other PropertyDescriptor fields, it's not defined how to parse the name so it's not clear that we should accept this or not.  Maybe link to https://github.com/w3c/css-houdini-drafts/issues/112 somewhere?

::: testing/web-platform/tests/css-houdini/properties-and-values.html:423
(Diff revision 3)
> +let computedValueTests = [
> +  // Parsing & computing (incl. serializing) lists of idents.
> +  {
> +    syntax: "a+ | <custom-ident>+",
> +    declared: "a a a a b c d",
> +    expected: "a a a a b c d",
> +  },

I think we need initialValues on all these tests too, and ideally with a value different from declared.

::: testing/web-platform/tests/css-houdini/properties-and-values.html:525
(Diff revision 3)
> +    declared: "calc(1 * 2 + 3 / 4)",
> +    expected: "2.75",
> +  },
> +  { syntax: "<number>",
> +    declared: "calc(1 / 0)",
> +    expected: "",

Shouldn't this be whatever you give for the initialValue?  Same for the other subtests where you're checking |expected: ""|.

::: testing/web-platform/tests/css-houdini/properties-and-values.html:561
(Diff revision 3)
> +  { syntax: "<image>",
> +    declared: "radial-gradient(2em at 60px 50% , #000000 0%, #000000 14px, rgba(0, 0, 0, 0.3) 18px, rgba(0, 0, 0, 0) 4em)",
> +    expected: "radial-gradient(32px at 60px 50%, rgb(0, 0, 0) 0%, rgb(0, 0, 0) 14px, rgba(0, 0, 0, 0.3) 18px, transparent 64px)",
> +    // This is the serialization result we get from calling getComputedStyle
> +    // and getPropertyValue on a background-image property set to this value.
> +  },

I'm a bit wary of adding upstreamed tests that rely on particular serializations, since CSS specs still are a bit lax on defining how serialization happens.

We could avoid this issue by checking that the serialization matches the serialization of a fixed property that accepts the same syntax.  That's probably not supported by the specs either, but much more likely to be correct.

Unfortunately I don't think all of the types here can be used in a fixed property (like <resolution> and the list-typed ones).  Not sure what to do about that -- these tests are useful to have.  I guess the same applies to the animation tests, which also use getComputedStyle.

::: testing/web-platform/tests/css-houdini/properties-and-values.html:771
(Diff revision 3)
> +// But for a custom property without an initial value, it's not possible to
> +// fill in a value.

Per a previous comment, I think this should be allowed, and we should test it.

        Attachment #8781033 -
        Flags: review?(cam) → review-

    
    

    
  
Comment on attachment 8781032 [details]
Bug 1273706 - Part 26: Implement lazy computation of computed CSS registered property values on CSSVariableValues.

https://reviewboard.mozilla.org/r/71532/#review71356

> Should we really be computing down the argument values of the <transform-function>?  The spec says the computed value of a <transform-function> should be the same as the specified value.
> 
> Is there a reason this whole case for TransformFunction can't be just:
> 
>   case CSSvalueType::TransformFunction:
>     return CSSComputedvalue::TransformFunction(aValue);
> 
> ?

I was confused about this while implementing, so I filed an issue: https://github.com/w3c/css-houdini-drafts/issues/250 and it looks like the spec is going to change. WDYT?
Blocks: 1298517

    
    

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

https://reviewboard.mozilla.org/r/66174/#review71280

> mRegistrations is only used within AddVariableDeclaration, to pass it to the CSSVariableDeclarations constructor.
> 
> Instead of storing the CSSVariableRegistrations pointer on every Declaration, can we instead just pass it in to AddVariableDeclarations?  There are only a few callers of AddVariableDeclarations: two in nsCSSParser, where we can just call GetVariableRegistrations; and BuildStyleRule in StyleAnimationValue.cpp, where we're currently creating a Declaration with no CSSVariableRegistrations anyway.

This sounds very reasonable, and it does reduce the number of places we need to change code. Thanks for pointing this out!

> Doesn't matter too much, but it seems to be the pattern to use |#ifdef DEBUG_BLAH| rather than |#ifdef BLAH_DEBUG| for component-specific debug code.

Didn't know that. Thanks!

> Can we call RecomputeSingleValidity here, since we only need to ensure that we've updated validities for variable declarations aID corresponds to?

Yes, you are right.

> Hmm, yes that does seem unfortunate that we have to parse the string three times. :(  Please file a follow-up bug to reduce the amount of parsing we do here.

I managed to get rid of the initial ParseVariableDeclaration, as discussed on IRC! I will make a follow-up bug.

> Since we are careful to only add a single variable declaration (the winning one from this CSSVariableDeclarations, and never overwriting an existing one) to aRuleData->mVariables, I wonder if we should not call Has(), which will perform validity updates, but instead poke at mVariableIDs and mUsedDeclarations on aRuleData->mVariables directly?  Actually I think we can just do aRuleData->mVariables->mVariableIDs->Contains(name), since we know that aRuleData->mVariables->mUsedDeclarations would (if we updated validity) have a value other than -1 at every element.

Nice catch. Thanks!

> Nit which you can ignore since this will change in a later patch: switch cases should be indented a level.

Thanks! I should have checked the style guide.

> The size of the Declaration objects themselves should already be counted in the mDeclarations.ShallowSizeOfExcludingThis() call, as that includes the array buffer allocation.

Oops. Thanks.

> I don't think we want to measure mRegistrations on every CSSVariableDeclarations object, as it's shared between them.  Instead, we would just measure it somewhere in nsDocument::DocAddSizeOfExcludingThis, maybe accounting it against &aWindowSizes->mStyleSheetsSize or mDOMOtherSize.

That makes sense. Thanks.

> DeclPriorityGte is cheaper than the ComputeValidity call.  Should we do it first?  Also, should we iterate mDeclarations in reverse, so that we don't have to call ComputeValidity for all variable declarations, and instead at most have to call it twice (once we encounter a first non-important declaration, then second when we encounter an important declaration)?

Good idea. Thanks!

> Per above hopefully we can remove this.

I put it in an #ifdef DEBUG and use it right now to assert that we always call AddVariableDeclaration with the same CSSVariableRegistrations. Does this sound OK?

> Not sure what the distinction you are drawing between custom properties and variables is here.  Is this comment out of date now that we are storing non-registered and registered custom properties in the same style struct?  Please update to clarify.

Sorry, at the time I wrote the comment I wasn't clear. I meant to write 'registered custom properties' vs 'unregistered custom properties / variables'.

> I don't think this will be effective on 32-bit builds, since the condition will always be false.  However, nsTArrays cannot grow to be longer than 2**31 elements anyway, so I think we can just safely cast here and not bother checking.

Alright, thanks. I have put a comment to that effect.

> Don't we need to null check mVariables here too?  Can we just return early at the top of the function if mVariables is null?

Yep, that makes more sense.

> Might be nice to have a method on CSSVariableDeclarations that can look up a variable's importance without having to copy all the other aspects of the declaration.

You are right. Added!

> If we no longer pass CSSVariableRegistrations to the Declaration constructor, we should be able to drop the changes to nsDOMCSS{,Attr}Declaration.{h,cpp} and nsSVGElement.cpp.

Yep! Thanks for suggesting it.

    
    

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

https://reviewboard.mozilla.org/r/66180/#review67466

The reftests properties-and-values-02.html and properties-and-values-03.html were what I used to test this.

Thanks for the suggestion about CalcStyleDifference. I will do that, and add your rationale as a comment.

> Can you add some comments explaining the difference between PutInherited, PutSpecifiedUnparsed and PutSpecifiedParsed (i.e. something like the previous comment that mentioned where these functions are intended to be called from)?
> 
> Can you (in this or in the previous patch that added it) document CSSVariableDeclarations::Declaration::mParsed?  I take it it means "whether we parsed the string into an nsCSSValue because we had a registered custom property and the value didn't have any variable references".  Is that right?

Yep, that is right. I have added the comment to part 20 and will add comments here. Thanks!

> Should HandleVisitNode go down here under the "Helper functions for Resolve" too?

Yep! I have added comments to elaborate more on this. Thanks!

> May as well put a |bool mVisited| on Variable itself?

That makes more sense. Thanks.

> When do we get in here?  Can you add a comment?

Thanks for pointing this out. If we correctly set initial values on the root, we should only run into cases in which forceUseInitial is unnecessary, because we will just set the invalid initial value. Here is the comment I am writing:

      // We can get here if:
      // (1) have a variable declaration that specifies 'inherit' (or 'unset'
      //     and this is an unregistered or registered inherited custom
      //     property)
      // (2) this is a registered inherited custom property that is not
      //     specified
      // (3) this is an unregistered custom property that is not specified (all
      //     unregistered custom properties are inherited)
      // *and* there is no inherited value! That can happen if this is a
      // registered custom property whose syntax is "*" with no initial value,
      // or if this is an unregistered custom property whose ancestors never
      // declare a value.
      // In that case, we need to use the initial value, *and* the initial
      // value is the special invalid value -- which means we actually don't do
      // anything.

> I think this comment is still true.  Is there value in removing it?

Sorry. I will add it back.

> s/It's/Its/
> 
> s/it at all/if it needs to be/ ?

Just realized this shouldn't actually be necessary due to part 24. Thanks for the realization. Sorry.

> Is this the case that handles custom properties registered with type "*" with an empty token stream?  If so, can you assert the type?
> 
> Might be clearer to extend the second sentence of the comment with something like "... set anything, since the absence of a declaration represents this invalid initial value"?

As noted in the previous comment, it turned out that that code isn't necessary because of part 24. Thanks for pointing out the possible confusion.

> Nit: I wonder if we can use the term "non-inherited" rather than "uninherited".  The latter sounds to me like something is done to undo the inheritance, and the former seems to be used in existing code.

That sounds good to me. I will use sed.

> Do you mean "Unregistered custom properties are always inherited"?

Yes. Sorry!

> for (const nsString& ref : entry.mRefs) {
> 
> ?

That is better. Thanks!

> "... references a token-stream typed custom property ..."?

This is actually a quote from CSS Variables. I will add what you mentioned in [...] because it makes sense to clarify it. Thanks!

> I think this terminology distinction here is confusing.  I believe variables are the abstract thing that can be referenced by var()s, and custom properties are the way to provide their values.  So here I think we want to say:
> 
>   Unregistered custom properties take token stream values.

Yes, sorry. As you noticed, I made an incorrect distinction in a bunch of comments. Thanks for pointing it out.

> I guess this comment should say "Try to fall back to the inherited or initial value, depending on whether the property is registered as inherited".  But also, I think you can remove the "Try to" since there's no way this can fail?

Yes. Thanks!

> Should we assert that mVariables[id].mInherited is currently None (and in PutSpecified{Parsed,Unparsed}, that mSpecified is None)?

Yes. That makes sense.

> Perhaps pass in mVariables[id].mSpecified.ptr() as the aData argument, so we don't have to deal with the Maybe inside the lambda?

That is a good idea.

> (Ah, I thought you left these misaligned in the previous patch deliberately, to stay below 80 columns.)

Haha, sorry. Also, not sure how the fix ended up in this patch. I think it should be fixed.

> Mention in this comment that it's because we store both inherited and non-inherited custom properties in the Variables struct?

Sounds good.

> Nit: just to keep things a bit more compact in here, how about formatting like this:
> 
>   const void* parentStruct = ...;
>   if (aSID != eStyleStruct_Variables ||
>       static_cast<const nsStyleVariables*>(parentStruct)->mHasUninherited) {
>     // We have a parent, and so ...
>     aContext->AddStyleBit(...
>     ...
>   }
> 
> and then extend the comment to say mention that for the Variables struct, we additionally need to check that we don't have any non-inherited custom properties stored in it.

Thanks! Nice!

> I think CSSVariableResolver::Resolve doesn't need to require that aSpecified is not null.  I think the existing code just asserts that (and doesn't null check aSpecified before the AddVariablesToResolver call) just because previously it could never be null.
> 
> So how about we just make Resolve() accept a null aSpecified, and then not worry about creating an empty CSSVariableDeclarations and deleting it later, here.

Yes, that sounds more reasonable. Thanks.

    
    

    
  
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

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

I have resolved this by adding:

    case eUnit_UnparsedString: {
      // Only custom properties should uncompute from unparsed strings. We
      // don't want to set mPropertyID otherwise, because it is used to track
      // token streams from shorthand property declarations.
      MOZ_ASSERT(aProperty == eCSSPropertyExtra_variable);

... which becomes:

    case eUnit_UnparsedString: {
      // Only custom properties should uncompute from unparsed strings. We
      // don't want to set mPropertyID otherwise, because it is used to track
      // token streams from shorthand property declarations.
      MOZ_ASSERT(aProperty == eCSSPropertyExtra_variable ||
                 aProperty.IsCustom());

... with the animations patch, which changes aProperty to be a CSSProperty. Does this seem OK?

    
    

    
  
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/#review70852

> probably also assert that mTerms.IsEmpty()?

Yep, you are right.

    
    

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

https://reviewboard.mozilla.org/r/66170/#review71202

> This duplicates a bunch of code that exists elsewhere.
> 
> I think you should remove this function, and also remove all calls to it, and instead, at the *beginning* of ParseSingleTypedValue (before any type checks or GetToken calls, do:
> 
> if (ParseSingleTokenVariant(aValue, VARIANT_INHERIT, nullptr)) {
>   return true;
> }

Thanks for pointing this out. This was a bad mistake on my part. ParseTypedValue should never get any CSS-wide keywords as inputs. I have added a comment to note that.

We use ParseTypedValue in the following places:

- dom/animation/KeyframeUtils.cpp, where we parse JS objects representing keyframe animations. Not exactly clear whether we should accept CSS-wide keywords, but the case for regular properties does not. I've sent an email to birtles about that.
- layout/style/CSS.cpp, which disallows CSS-wide keywords per the spec as being not 'computationally idempotent'
- layout/style/CSSVariableDeclarations.cpp, which uses it to type-check, but only on values that have CSSVariableDeclarations::Type::TokenStream (inherit has type 'Inherit', etc.). The type comes from ParseVariableDeclaration
- layout/style/CSSVariableResolver.cpp, which has resolved 'inherit', 'initial', and 'unset' before

> You need an UngetToken() call here if this fails, since parsing functions shouldn't consume tokens that they don't actually parse.  (Otherwise they'll accept errors that they shouldn't in some particular cases.)

Thanks for pointing this out. I looked at the code, and there were indeed missing UngetTokens in places. But I just realized that because ParseSingleTransform calls ParseFunction, which skips until the closing parenthesis on errors, it seems like in order to try parsing again, we have to restore the saved input state anyways, at least in this case. It seems like if we restore the saved input state at the start of every try, then, there isn't a point in calling UngetToken.

Right now the loop looks like this:

    CSSParserInputState state;
    SaveInputState(state);
    
    nsCSSValue result;
    
    for (const CSSVariableSyntax::Term& term : aSyntax.GetTerms()) {
      bool ok = false;
      if (term.IsList()) {
        nsCSSValueList* item = nullptr;
        for (;;) {
          nsCSSValue value;
          if (!GetToken(/* aSkipWS = */ true)) {
            // If at EOF, stop.
            break;
          }
          UngetToken();
          if (!ParseSingleTypedValue(term, value)) {
            ok = false;
            break;
          }
          ...
        }
      } else {
        ok = ParseSingleTypedValue(term, result);
        // If we are in a disjunction (<A> | <B> | ...) and this one failed, we
        // need to keep trying.
      }
      ...
      RestoreSavedInputState(state);
      result.Reset();
    }

... and because of the RestoreSavedInputState, I've removed UngetToken from ParseSingleTypedValue.

Am I understanding this correctly & does this seem OK?

> This means that you allow lists to be terminated only by EOF.  That seems wrong; it means they can't occur within a style sheet.  (It's OK for the ParseTypedValue API introduced in that patch -- but is that really going to be the only path?  I suppose maybe it is since it's all reparsing of old token streams?)
> 
> Fixing this probably means making ParseSingleTypedValue return CSSParseResult rather than boolean.
> 
> Or if it is what is intended, it requires very clear comments on the limits of the function.

Sorry, this is indeed what is intended. You are right that this is all reparsing of old token streams. I've added comments on the CSSParserImpl ParseTypedValue to clarify this.

    
    

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

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

> It looks like we don't use this constructor.  Can we remove it?

Yes, you're right. Sorry.

    
    

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

https://reviewboard.mozilla.org/r/66178/#review71220

> Does this need to be spec feedback?  Or is the name of dictionaries not exposed to the Web?
> 
> (Maybe this is a question for heycam.)

heycam has replied (on Bugzilla):
Dictionary names are indeed not exposed to the web.  (The PropertyDescriptor class Jonathan refers to is a Gecko C++ struct name, not a class defined by the JS spec, so I don't think we need to submit spec feedback for this.)

> Do we put the initial values in the spec's WebIDL here?  Or is there a reason we're not supposed to?

Wasn't aware of that feature. Thanks!

    
    

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

https://reviewboard.mozilla.org/r/66184/#review71254

> Passing these parameters as nonzero requires justification.  I think the use of eRestyle_Subtree is probably correct since changing the registrations causes changes in the computed values of custom properties.  However, NS_STYLE_HINT_REFLOW should almost certainly be nsChangeHint(0) instead.
> 
> And the name of the function should be RebuildStyleDataForRegisteredProperties or something more specific, since it is passing particular parameters to RebuildAllStyleData.
> 
> 
> 
> Second, and even more importantly, this call is unnecessarily synchronous, which means this will do multiple rebuilds for multiple registrations.  You should instead call PostRebuildAllStyleDataEvent.

Thanks for the suggestion. Added the changes & a comment.

> I don't think having a null pres shell justifies throwing an exception here.  Not currently having a presentation means there's no style to rebuild.  This function should just return void.

I am changing it to return NS_OK unless we fail to get the parsing info, which allows for behavior in line with what CSS::Supports does when we fail to get the parsing info in CSS::RegisterProperty and CSS::UnregisterProperty.

> This seems like a strong statement to make for all functions.  I think you should have assertions that this is only operating on a limited set of known functions that are allowed in the values of typed properties.

I ended up modifying ParseTypedValue to take an aDisallowRelativeDimensions parameter, which we use to set VARIANT_ABSOLUTE_DIMENSION on the variants and pass to ParseSingleTransform. THis appears to have the desired effect.

> You also need to check that the scanner has consumed all of its input!

Thanks.

> I think you actually need to use the identifier from the token so that you process CSS escaping correctly (and same for unregister!), but please see what happens on https://github.com/w3c/css-houdini-drafts/issues/283 which I just filed.

I've added a FIXME comment pointing to that issue and use token.mIdent now.

> Is all of this really needed?  Why not just look up the name in the existing registrations?

I thought that it made more sense to return a SyntaxError if the name was invalid. But I checked the spec and you are right, so now I will throw a NotFoundError.

I'm factoring out the code to get the unescaped name from token.mIdent (it looks like nsCSSScanner does this?) and share it in both cases.

    
    

    
  
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/#review70852

> probably also assert that mTerms.IsEmpty()?

Yep, you are right.

    
    

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

https://reviewboard.mozilla.org/r/66170/#review71202

> Thanks for pointing this out. This was a bad mistake on my part. ParseTypedValue should never get any CSS-wide keywords as inputs. I have added a comment to note that.
> 
> We use ParseTypedValue in the following places:
> 
> - dom/animation/KeyframeUtils.cpp, where we parse JS objects representing keyframe animations. Not exactly clear whether we should accept CSS-wide keywords, but the case for regular properties does not. I've sent an email to birtles about that.
> - layout/style/CSS.cpp, which disallows CSS-wide keywords per the spec as being not 'computationally idempotent'
> - layout/style/CSSVariableDeclarations.cpp, which uses it to type-check, but only on values that have CSSVariableDeclarations::Type::TokenStream (inherit has type 'Inherit', etc.). The type comes from ParseVariableDeclaration
> - layout/style/CSSVariableResolver.cpp, which has resolved 'inherit', 'initial', and 'unset' before

I emailed birtles to ask about allowing inherit/initial/unset in the keyframes argument for Web Animations. He pointed out that we allow these for regular properties. Accordingly, I will modify ParseTypedValue to accept inherit/initial/unset, and add assertions where we do not expect them.

    
    

    
  
Comment on attachment 8781032 [details]
Bug 1273706 - Part 26: Implement lazy computation of computed CSS registered property values on CSSVariableValues.

https://reviewboard.mozilla.org/r/71532/#review71356

> Can we just return the |const CSSComputedValue*| instead of setting it through an outparam?  Then return null on failure.

Sounds good.

> Is there a way we can avoid storing these pointers?  For the style context one, I am slightly worried that work such as bug 1188721 will allow the style context to be destroyed if we share the Variables struct among other style contexts.
> 
> Can we pass these two things in to Compute(), which is the function that needs mStyleContext and mPresContext, instead?  There looks to be only two places where we call Compute().  In both nsComputedDOMStyle::DoGetCustomProperty and ExtractComputedCustomValue, we have the style context pointer.  And from that we can get the pres context.
> 
> So perhaps just add a single nsStyleContext* parameter to Compute() and get the pres context from it.

Thanks for pointing that out. Do you think this means we need special consideration for a different nsStyleContext being passed to Compute() after we have already computed the value?

I will change it to store the nsStyleContext that was used to compute each Variable, but to use the one given to Compute() in order to compute. Then I will compute the value if either the style context passed to Compute differs or the value wasn't previously computed.

Then even if the style context is destroyed, we won't access free'd data or anything, because we will still always just use the nsStyleContext given as the argument.

Does this sound OK?

> We might be able to shorten this by taking advantage of the fact that all combinations of aType and aValue.GetUnit() produce an nsStyleCoord, and that aType alone determines which CSSComputedvalue constructor we use.  Something like this?
> 
>   nsStyleCoord coord;
> 
>   if (aValue.IsLengthUnit()) {
>     // call nsRuleNode::CalcLength and set coord
>   } else if (aValue.GetUnit() == eCSSUnit_Percent) {
>     // set coord
>   } else {
>     // call ComputeCalc and set coord
>   }
> 
>   switch (aType) {
>     case CSSValueType::Length:
>       return CSSComputedValue::Length(coord);
>     case CSSValueType::Percentage:
>       return CSSComputedValue::Percentage(coord);
>     case CSSValueType::LengthPercentage:
>       return CSSComputedValue::LengthPercentage(coord);
>   }
> 
> WDYT?

Good idea, thanks.

    
    

    
  
Comment on attachment 8777231 [details]
Bug 1273706 - Part 27: Add CSSPropertySet.

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

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

Thanks!

    
  
Alias: css-properties-values-api
Summary: implement CSS Properties and Values API → [META] implement CSS Properties and Values API
Keywords: meta

    
    

    
  
Over his winter break, Jonathan unbitrotted these patches (no small task!), and addressed some of the pending feedback.  (Current patch stack is at https://bitbucket.org/jyc/firefox-mq-properties-and-values/overview )

I plan on shepherding these patches into the tree in the near future (auditing the changes to be sure they're sane, making sure all review feedback was/is addressed, etc.)

Since we're at 340+ comments on this bug here, I'll try to avoid posting much more here -- and instead I'll probably split this bug's patch queue into several spun-off helper-bugs, and land the patches there.  I'll do my best to make a clear mapping between the (tweaked/audited/landed) patches on those helper-bugs & the reviewed patches here.

    
    

    
  
(In reply to Daniel Holbert [:dholbert] from comment #343)
> Over his winter break, Jonathan unbitrotted these patches (no small task!),
> and addressed some of the pending feedback.  (Current patch stack is at
> https://bitbucket.org/jyc/firefox-mq-properties-and-values/overview )
> 
> I plan on shepherding these patches into the tree in the near future
> (auditing the changes to be sure they're sane, making sure all review
> feedback was/is addressed, etc.)
> 
> Since we're at 340+ comments on this bug here, I'll try to avoid posting
> much more here -- and instead I'll probably split this bug's patch queue
> into several spun-off helper-bugs, and land the patches there.  I'll do my
> best to make a clear mapping between the (tweaked/audited/landed) patches on
> those helper-bugs & the reviewed patches here.

Thanks for the update. Can you please provide the URLs here of the following/helper-bugs when they are created, so the community still can around, if someone is interested.
Depends on: 1370344

    
    

    
  
(In reply to Serg from comment #344)
> Thanks for the update. Can you please provide the URLs here of the
> following/helper-bugs when they are created, so the community still can
> around, if someone is interested.

Hi Serg! Here is a quick update (sorry for the delay!):

We are swapping out Gecko's style system for Servo's over the next couple of months, so we want to minimize unnecessary changes to the Gecko's style-system at this point. Unfortunately this means that most of the code that was tracked here will probably not land, although some might still end up landing as "glue code."

We're going to instead reimplement this in Servo. There is a new bug tracking the Servo side of implementing things here: https://github.com/servo/servo/issues/17155, and Bug #1370344 on Bugzilla tracks the Firefox side of things.

Ideally this will be done by the end of the summer (but last year showed I'm very bad at estimating how long things might take)!

    
    

    
  
(In reply to Jonathan Chan [:jyc] from comment #345)
> (In reply to Serg from comment #344)
> > Thanks for the update. Can you please provide the URLs here of the
> > following/helper-bugs when they are created, so the community still can
> > around, if someone is interested.
> 
> Hi Serg! Here is a quick update (sorry for the delay!):
> 
> We are swapping out Gecko's style system for Servo's over the next couple of
> months, so we want to minimize unnecessary changes to the Gecko's
> style-system at this point. Unfortunately this means that most of the code
> that was tracked here will probably not land, although some might still end
> up landing as "glue code."
> 
> We're going to instead reimplement this in Servo. There is a new bug
> tracking the Servo side of implementing things here:
> https://github.com/servo/servo/issues/17155, and Bug #1370344 on Bugzilla
> tracks the Firefox side of things.
> 
> Ideally this will be done by the end of the summer (but last year showed I'm
> very bad at estimating how long things might take)!

Thanks for the update.
Will switch to check the Servo bug-tracker and builds.

    
  
Summary: [META] implement CSS Properties and Values API → [META] implement Houdini CSS Properties and Values API

    
  
Blocks: all-css-modules
Type: defect → enhancement
Whiteboard: [layout:backlog:2020]
Blocks: 1634403
Depends on: 1684605

    
    

    
  
The bug assignee didn't login in Bugzilla in the last 7 months.

:emilio, could you have a look please?

For more information, please visit auto_nag documentation.


Assignee: jyc → nobody
Flags: needinfo?(emilio)
Flags: needinfo?(emilio)

    
  
Severity: normal → S3

    
  
Blocks: interop-2023
Depends on: 1821552

    
  
Depends on: 1840587
Depends on: 1840987
Depends on: 1841021
No longer depends on: 1840987
Depends on: 1843988
Depends on: 1846516
Depends on: 1846488
Depends on: 1846625
Depends on: 1846628
Depends on: 1846632
Depends on: 1846635
Depends on: 1833538

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: