511803 - Store transform function name as eCSSKeyword_* in nsCSSValue::Array

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Ryo Kawaguchi]

This is a follow-up bug for bug 113577.

Currently, a CSS transform function such as "skewX" and "rotate" is stored in nsCSSValue of type eCSSUnit_Function, and the first element of nsCSSValue::Array stores the function name as nsString.

bug 113577 has introduced a new mechanism for storing CSS functions using eCSSKeyword_* (enum) as the first element of nsCSSValue::Array.
Two helper functions are available for the purpose:

// Initializes as a function value with the specified function id.
Array* nsCSSValue::InitFunction(nsCSSKeyword aFunctionId, PRUint32 aNumArgs);
// Checks if this is a function value with the specified function id.
PRBool nsCSSValue::EqualsFunction(nsCSSKeyword aFunctionId) const;

This bug will use this new mechanism for CSS transform functions to unify the way to store CSS function values (also to achieve slight performance and memory usage improvements).

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This was originally followup from bug 113577 comment 44 bug 113577 comment 51.

We hit it again in bug 872678.

I have a patch in progress.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The patch I'm going to post here is expected to make one behavioral change, which I think is an improvement anyway, but I wanted to run it by you.

It should change serialization of the functions in font-variant-alternates (i.e., stylistic(), styleset(), character-variant(), swash(), ornaments(), annotation()) to convert to the canonical lowercase rather than preserving the case that the author used.  This will match what we do for other functions (although for some of the transform functions, e.g., translateX(), the canonical case is mixed case).

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: Fix serialization of font-variant-alternates values with more than one function. (Tested in patch 2.)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: Convert all eCSSUnit_Function storage to use nsCSSKeyword. This leads to one behavior change, which is case canonicalization for font-variant-alternates function values.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And note that this removes a bunch of hashtable lookups from performance-sensitive code (in particular, animation of transforms, including on the compositor thread; see bug 872678).

Comment 6

[John Daggett (:jtd)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #2)
> The patch I'm going to post here is expected to make one behavioral change,
> which I think is an improvement anyway, but I wanted to run it by you.
> 
> It should change serialization of the functions in font-variant-alternates
> (i.e., stylistic(), styleset(), character-variant(), swash(), ornaments(),
> annotation()) to convert to the canonical lowercase rather than preserving
> the case that the author used.  This will match what we do for other
> functions (although for some of the transform functions, e.g., translateX(),
> the canonical case is mixed case).

This makes complete sense to me.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

try run: https://tbpl.mozilla.org/?tree=Try&rev=a0c0927a136e

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 752559 [details] [diff] [review]
patch 2:  Convert all eCSSUnit_Function storage to use nsCSSKeyword.  This leads to one behavior change, which is case canonicalization for font-variant-alternates function values.

r=me

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ea47adb68738
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3e67e9e31f0e

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ea47adb68738
https://hg.mozilla.org/mozilla-central/rev/3e67e9e31f0e