[webkit-reviews] review denied: [Bug 98712] Make CSS variable names case-insensitive. : [Attachment 167676] Patch

Wed Oct 10 16:32:10 PDT 2012

Tony Chang <tony at chromium.org> has denied Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 98712: Make CSS variable names case-insensitive.
https://bugs.webkit.org/show_bug.cgi?id=98712

Attachment 167676: Patch
https://bugs.webkit.org/attachment.cgi?id=167676&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=167676&action=review

>>>> Source/WebCore/ChangeLog:9
>>>> +	      making variable definitions consistent with other property names,
which are also case insensitive.
>>> 
>>> Is this the only rationale?
>> 
>> My understanding is that this is the intent of the CSS Variables spec - See
http://dev.w3.org/csswg/css-variables/#defining-variables "ISSUE 1".
> 
> Yup, we resolved to be case-insensitive of *some* kind.  At minimum, we'll be
ascii-case-insensitive.  (And I think we'll probably end at that.)

It would be nice if you could either include a link to the spec or to the
mailing list in the ChangeLog.

> Source/WebCore/css/CSSParser.cpp:3037
>      ASSERT(name.length() > 12);
> -    AtomicString variableName = name.is8Bit() ?
AtomicString(name.characters8() + 12, name.length() - 12) :
AtomicString(name.characters16() + 12, name.length() - 12);
> +    AtomicString variableName = name.substring(12, name.length() -
12).lower();

Where does 12 come from? Can we use a const char* and sizeof - 1?

> Source/WebCore/css/CSSParserValues.h:101
> +    String substring(unsigned position, unsigned length) const
> +    {
> +	   ASSERT(m_length >= position + length);
> +	   return is8Bit() ? String(m_data.characters8 + position, length) :
String(m_data.characters16 + position, length);
> +    }

I don't think this helper function is useful since you only use it in one place
and it uses String instead of AtomicString.  This is less efficient than just
calling lower() on name, then creating an AtomicString in
storeVariableDeclaration (it allocates an extra string).

> LayoutTests/fast/css/variables/case-insensitive-expected.html:3
> +<!DOCTYPE html>
> +<html>
> +<style>

Can we just make this into a script test?  reftests run slower than script
tests.