98712 – Make CSS variable names case-insensitive.

RESOLVED FIXED 98712

Make CSS variable names case-insensitive.

https://bugs.webkit.org/show_bug.cgi?id=98712

Summary Make CSS variable names case-insensitive.

Luke Macpherson

Reported 2012-10-08 20:10:01 PDT

Make CSS variable names case-insensitive.

Attachments
Patch (7.88 KB, patch) 2012-10-08 20:23 PDT, Luke Macpherson	no flags	Details Formatted Diff Diff
Patch (8.10 KB, patch) 2012-10-10 18:36 PDT, Luke Macpherson	no flags	Details Formatted Diff Diff
Patch (9.96 KB, patch) 2012-10-11 18:49 PDT, Luke Macpherson	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Luke Macpherson

Comment 1 2012-10-08 20:23:40 PDT

Created attachment 167676 [details] Patch

Alexey Proskuryakov

Comment 2 2012-10-09 09:42:38 PDT

Comment on attachment 167676 [details] Patch 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?

Luke Macpherson

Comment 3 2012-10-09 17:04:15 PDT

Comment on attachment 167676 [details] Patch 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".

Tab Atkins

Comment 4 2012-10-09 17:13:22 PDT

(In reply to comment #3) > (From update of attachment 167676 [details]) > 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.)

Tony Chang

Comment 5 2012-10-10 16:32:50 PDT

Comment on attachment 167676 [details] Patch 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.

Tab Atkins

Comment 6 2012-10-10 16:42:58 PDT

(In reply to comment #5) > Where does 12 come from? Can we use a const char* and sizeof - 1? It's the length of "-webkit-var-", which is a required prefix on all var property names right now. Agreed, though, that this can be done more understandably.

Luke Macpherson

Comment 7 2012-10-10 18:28:19 PDT

Comment on attachment 167676 [details] Patch 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. Added spec link. >> Source/WebCore/css/CSSParser.cpp:3037 >> + AtomicString variableName = name.substring(12, name.length() - 12).lower(); > > Where does 12 come from? Can we use a const char* and sizeof - 1? Done. >> Source/WebCore/css/CSSParserValues.h:101 >> + } > > 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). I want to avoid adding intermediate strings to the atomic string table (ie. the non-case-normalized and non-shortened strings). This operation could normally be done in place, but everywhere else I look CSSParserStrings coming in from the tokenizer are treated as immutable, so I am assuming that modifying the underlying string is not expected. >> LayoutTests/fast/css/variables/case-insensitive-expected.html:3 >> +<style> > > Can we just make this into a script test? reftests run slower than script tests. http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree "Most test cases can be easily transformed into ref-tests and it is the preferred way to add new tests." IMHO ref tests are ideal for testing CSS variables because they provide very clear documentation of the expected behavior of variable resolution.

Luke Macpherson

Comment 8 2012-10-10 18:36:02 PDT

Created attachment 168114 [details] Patch

Tony Chang

Comment 9 2012-10-11 10:05:10 PDT

Comment on attachment 168114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168114&action=review > Source/WebCore/css/CSSParser.cpp:3041 > + AtomicString variableName = name.substring(prefixLength, name.length() - prefixLength).lower(); What I'm suggesting is: String lowerName = name.lower(); AtomicString variableName = lowerName.is8Bit() ? AtomicString(lowerName.characters8() + prefixLength, lowerName.length() - prefixLength) : AtomicString(lowerName.characters16() + prefixLength, lowerName.length() - prefixLength); This creates 1 extra String and 1 AtomicString with the variable name. Your code creates 2 Strings and 1 AtomicString. You could make a static inline helper function for this if you wanted. It doesn't seem like it's worth adding to CSSParserValues.h since it's only used in one place.

Tony Chang

Comment 10 2012-10-11 10:09:47 PDT

(In reply to comment #7) > > Can we just make this into a script test? reftests run slower than script tests. > > http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree "Most test cases can be easily transformed into ref-tests and it is the preferred way to add new tests." IMHO ref tests are ideal for testing CSS variables because they provide very clear documentation of the expected behavior of variable resolution. I think that sentence means the preferred way to add new pixel tests. It's under the heading "Pixel Test Tips". Also, I think it's more clear to have the test write "PASS getComputedStyle(div).backgroundColor is green".

Ryosuke Niwa

Comment 11 2012-10-11 16:10:10 PDT

(In reply to comment #9) > (From update of attachment 168114 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=168114&action=review > > > Source/WebCore/css/CSSParser.cpp:3041 > > + AtomicString variableName = name.substring(prefixLength, name.length() - prefixLength).lower(); > > What I'm suggesting is: > String lowerName = name.lower(); > AtomicString variableName = lowerName.is8Bit() ? AtomicString(lowerName.characters8() + prefixLength, lowerName.length() - prefixLength) : AtomicString(lowerName.characters16() + prefixLength, lowerName.length() - prefixLength); > > This creates 1 extra String and 1 AtomicString with the variable name. Your code creates 2 Strings and 1 AtomicString. You could make a static inline helper function for this if you wanted. It doesn't seem like it's worth adding to CSSParserValues.h since it's only used in one place. Why don't we just do AtomicString(name.lower().substringSharingImpl(prefixLength).impl()); ?

Luke Macpherson

Comment 12 2012-10-11 16:11:50 PDT

Comment on attachment 168114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168114&action=review >> Source/WebCore/css/CSSParser.cpp:3041 >> + AtomicString variableName = name.substring(prefixLength, name.length() - prefixLength).lower(); > > What I'm suggesting is: > String lowerName = name.lower(); > AtomicString variableName = lowerName.is8Bit() ? AtomicString(lowerName.characters8() + prefixLength, lowerName.length() - prefixLength) : AtomicString(lowerName.characters16() + prefixLength, lowerName.length() - prefixLength); > > This creates 1 extra String and 1 AtomicString with the variable name. Your code creates 2 Strings and 1 AtomicString. You could make a static inline helper function for this if you wanted. It doesn't seem like it's worth adding to CSSParserValues.h since it's only used in one place. name is const &, and CSSParserString::lower() returns void and mutates in place.

Luke Macpherson

Comment 13 2012-10-11 18:49:47 PDT

Created attachment 168332 [details] Patch

WebKit Review Bot

Comment 14 2012-10-15 10:09:27 PDT

Comment on attachment 168332 [details] Patch Clearing flags on attachment: 168332 Committed r131313: <http://trac.webkit.org/changeset/131313>

WebKit Review Bot

Comment 15 2012-10-15 10:09:31 PDT

All reviewed patches have been landed. Closing bug.

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

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component New Bugs

Assignee

Luke Macpherson

Reported

2012-10-08 20:10 PDT

Modified

2012-10-15 10:09 PDT History

CC List

6 users Show

URL

Keywords

Depends on

Blocks