[webkit-reviews] review granted: [Bug 85580] Implement CSS Variables Standard. : [Attachment 146161] Rename cssText().

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 85580: Implement CSS Variables Standard.
https://bugs.webkit.org/show_bug.cgi?id=85580

Attachment 146161: Rename cssText().
https://bugs.webkit.org/attachment.cgi?id=146161&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=146161&action=review


I think this is in a good place for a first iteration. Antti, it sounds like
you don't have strong objections at this point. If you disagree, feel free to
R- my R+.

(In reply to comment #78)
> (In reply to comment #76)
> > In general I still don't like all this round-tripping to strings and back.
Why can't we just make a copy that replaces the found variable references with
parsed values?
> 
> It would be nice to do that, but it is difficult for a few reasons:
> 1) The logic for validating expressions is deep inside the CSSParser /
CSSGrammar.y, so it is difficult to determine that a rule will be valid without
re-parsing, other than to completely duplicate that logic.
> 2) It may not even be possible to parse a variable correctly until you know
the context in which it is used - for example how do you know whether to parse
something as a font name or an ident unless you know the context in which it
occurs?
> 3) Shorthand expansion happens inside the parser - eg. border: var(x); will
expand to a different set of property/value pairs to apply depending on the
value of x (which could be a list value).
> 
> We might get to the point where you can validate a variable without
re-parsing eventually, but it would take a lot of work to get there, and I
don't think we want to make the kind of changes to the rest of the CSS engine
that would be required to do it at this stage in the life of CSS variables.

I think this is fine for now. Can you add FIXMEs to all these round-tripping
cases to get to this point though?

> Source/WebCore/ChangeLog:10
> +	   As an overview,

Nit: No need for this line. This bit of the ChangeLog description is always
meant to be an overview. :)

> Source/WebCore/ChangeLog:12
> +	   That HashMap is copy-on-write, and unless variables is simply a
pointer to the parent's definitions.

There's some grammar problem here. I'm not sure if you have extra words or are
missing a clause after "unless...definitions".

> Source/WebCore/css/CSSParser.cpp:1187
> +#if ENABLE(CSS_VARIABLES)
> +static inline void filterProperties(bool important, const
CSSParser::ParsedPropertyVector& input, Vector<CSSProperty, 256>& output,
size_t& unusedEntries, BitArray<numCSSProperties>& seenProperties,
HashSet<AtomicString>& seenVariables)
> +#else
>  static inline void filterProperties(bool important, const
CSSParser::ParsedPropertyVector& input, Vector<CSSProperty, 256>& output,
size_t& unusedEntries, BitArray<numCSSProperties>& seenProperties)
> +#endif

This is fine. I'd also be OK with always having this take a seenVariables
argument and just passing in 0 if CSS_VARIABLES is not enabled.