Bug 80874

Summary:

Implement a fast path when setting CSS properties with keywords from JS.

Product:

WebKit

Reporter:

Alexis Menard (darktears) <menard>

Component:

New Bugs

Assignee:

Alexis Menard (darktears) <menard>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, darin, dglazkov, kling, koivisto, macpherson, tonikitoo, tony, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
CPU Monitor while browsing Google Maps...	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	none
Patch for landing	none

Alexis Menard (darktears)

Reported 2012-03-12 13:05:55 PDT

Implement a fast path when setting CSS properties with keywords from JS.

Attachments
Patch (51.40 KB, patch) 2012-03-12 13:09 PDT, Alexis Menard (darktears)	no flags	Details Formatted Diff Diff
Patch (51.46 KB, patch) 2012-03-12 14:27 PDT, Alexis Menard (darktears)	no flags	Details Formatted Diff Diff
Patch (47.58 KB, patch) 2012-03-13 11:22 PDT, Alexis Menard (darktears)	no flags	Details Formatted Diff Diff
Patch (47.47 KB, patch) 2012-03-13 11:33 PDT, Alexis Menard (darktears)	no flags	Details Formatted Diff Diff
CPU Monitor while browsing Google Maps... (142.24 KB, image/png) 2012-03-13 12:33 PDT, Alexis Menard (darktears)	no flags	Details
Patch (44.18 KB, patch) 2012-03-13 17:30 PDT, Alexis Menard (darktears)	no flags	Details Formatted Diff Diff
Patch (44.19 KB, patch) 2012-03-14 06:03 PDT, Alexis Menard (darktears)	no flags	Details Formatted Diff Diff
Patch (44.26 KB, patch) 2012-03-14 15:35 PDT, Alexis Menard (darktears)	no flags	Details Formatted Diff Diff
Patch (47.13 KB, patch) 2012-03-19 13:30 PDT, Alexis Menard (darktears)	no flags	Details Formatted Diff Diff
Patch (47.65 KB, patch) 2012-03-19 15:53 PDT, Alexis Menard (darktears)	no flags	Details Formatted Diff Diff
Patch (52.09 KB, patch) 2012-03-20 04:20 PDT, Alexis Menard (darktears)	no flags	Details Formatted Diff Diff
Patch for landing (52.09 KB, patch) 2012-03-20 04:27 PDT, Alexis Menard (darktears)	no flags	Details Formatted Diff Diff
Patch for landing (52.09 KB, patch) 2012-03-20 07:04 PDT, Alexis Menard (darktears)	no flags	Details Formatted Diff Diff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.

Alexis Menard (darktears)

Comment 1 2012-03-12 13:09:42 PDT

Created attachment 131388 [details] Patch

Alexis Menard (darktears)

Comment 2 2012-03-12 13:16:04 PDT

Consider this benchmark : <html> <head> <script src="../resources/runner.js"></script> <style> #test { background: green; height: 200px; width: 200px; border: 20px solid blue; padding-top: 10px; color: yellow; position: static; } </style> </head> <body> <div id="test"></div> </body> <script> var div = document.getElementById("test"); PerfTestRunner.run(function() { div.style.border = ""; div.style.border = "20px dotted red"; div.style.paddingTop = ""; div.style.paddingTop = "30px"; div.style.position = ""; div.style.position = "static"; }, 100000); </script> </html> On my machine without the patch it executes with an average of 670. On my machine with the patch it executes with an average of 542. For comparison : Opera 11.61 average 877 Firefox 10 average 1936 IE10 (Win 8 consumer preview) 2824 I think the patch is worth it. Feel free to provide feedback (I'm sure the classes/variable names suck for example).

WebKit Review Bot

Comment 3 2012-03-12 14:06:17 PDT

Comment on attachment 131388 [details] Patch Attachment 131388 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11947063 New failing tests: fast/text/international/text-combine-parser-test.html fast/text/international/spaces-combined-in-vertical-text.html fast/dynamic/text-combine.html

Alexis Menard (darktears)

Comment 4 2012-03-12 14:27:17 PDT

Created attachment 131412 [details] Patch

Alexis Menard (darktears)

Comment 5 2012-03-12 14:28:20 PDT

(In reply to comment #4) > Created an attachment (id=131412) [details] > Patch Should fix the regressions.

Andreas Kling

Comment 6 2012-03-12 14:31:27 PDT

Comment on attachment 131388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131388&action=review Dumping comments. > Source/WebCore/css/CSSParser.cpp:575 > +class CSSPropertyParser { CSSPropertyParser is not an appropriate name for this class, as it's really only an information container. KeywordPropertyMapEntry or similar would be more descriptive. > Source/WebCore/css/CSSParser.cpp:579 > + CSSPropertyParser() > + : m_acceptedKeywords(0) > + , m_length(0) Missing member initializations in this constructor. > Source/WebCore/css/CSSParser.cpp:593 > + void setRange(int min, int max) { m_hasRange = true; m_minKeyword = min; m_maxKeyword = max; } This should be done via a constructor rather than a function call. > Source/WebCore/css/CSSParser.cpp:611 > +typedef HashMap<int, CSSPropertyParser> propertiesMap; 'propertiesMap' is not an appropriate type name. I would suggest something like "KeywordPropertyMap" > Source/WebCore/css/CSSParser.cpp:617 > + static const int positionKeywords[] = { CSSValueStatic, CSSValueRelative, CSSValueAbsolute, CSSValueFixed }; > + SET_PROPERTY_PARSER_MAP_ENTRY(map, CSSPropertyPosition, positionKeywords); This blob of code is very ugly. We are trading readability for performance in this patch, so we need to make sure the new code is as understandable as possible. For starters, these should be a single line only, it should be possible with a template. > Source/WebCore/css/CSSParser.cpp:815 > + cssString.characters = const_cast<UChar*>(string.characters()); It's unfortunate that we have to call characters() here, as that may trigger 8-to-16-bit string conversion. Not specific to this patch, just noting it. > Source/WebCore/css/CSSParser.cpp:824 > + RefPtr<CSSValuePool> cssValuePool = document ? document->cssValuePool() : 0; There's no need to ref the CSSValuePool here. > Source/WebCore/css/CSSParser.cpp:850 > + for (unsigned i = 0 ; !validPrimitive && i < property.length(); ++i) { Style, space after 0.

Andreas Kling

Comment 7 2012-03-12 14:49:53 PDT

Is this affecting real-world web pages? I'm surprised that these types of property assignments would be a bottle neck.

Alexis Menard (darktears)

Comment 8 2012-03-12 15:03:26 PDT

(In reply to comment #7) > Is this affecting real-world web pages? I'm surprised that these types of property assignments would be a bottle neck. Google maps is setting a lot properties(CSSPropertyDisplay, CSSPropertyPosition, CSSPropertyVisibility) from JS everytime you zoom or move the mouse to move the map.

Benjamin Poulain

Comment 9 2012-03-12 16:57:46 PDT

I suspect this might also help on Dromaeo, that is worth testing.

Luke Macpherson

Comment 10 2012-03-12 18:19:34 PDT

Comment on attachment 131412 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131412&action=review > Source/WebCore/css/CSSParser.cpp:576 > +class CSSPropertyParser { I would re-write this class along the lines of CSSStyleApplyProperty's ApplyPropertyExpanding. It's more verbose, but you can loose all the member variables, and produce optimal code for each case. > Source/WebCore/css/CSSParser.cpp:599 > + unsigned length() const { return m_length; } Can loose all these getters and wrap the range checking logic into this class. This interface leaks all the internals unnecessarily. > Source/WebCore/css/CSSParser.cpp:609 > +#define SET_PROPERTY_PARSER_MAP_ENTRY(map, propID, array) \ Drop the macro, it's cleaner. :) > Source/WebCore/css/CSSParser.cpp:856 > + } This section (846-856) should be a function on CSSPropertyParser, and the acceptedKeywords, minKeyword and maxKeyword functions should go away. Then 843-856 becomes: if (!propertyForID(propertyId)->testId(valueID)) return false; // or similar > Source/WebCore/css/CSSParser.cpp:1335 > + } Oh look, that code is repeated here. All the more reason to factor it out.

Alexis Menard (darktears)

Comment 11 2012-03-12 18:29:13 PDT

Dromaeo perf tests : https://gist.github.com/2025940 green is with the patch. jslib-modify-jquery and cssquery-jquery seems to improve (they seem to hit the new code path according to my quick debug).

Alexis Menard (darktears)

Comment 12 2012-03-13 11:20:28 PDT

(In reply to comment #11) > Dromaeo perf tests : https://gist.github.com/2025940 > > green is with the patch. > > jslib-modify-jquery and cssquery-jquery seems to improve (they seem to hit the new code path according to my quick debug). https://gist.github.com/2030135 with the chromium port. Dromaeo/jslib-modify-jquery.html shows up as an improvement.

Alexis Menard (darktears)

Comment 13 2012-03-13 11:22:45 PDT

Created attachment 131677 [details] Patch

Alexis Menard (darktears)

Comment 14 2012-03-13 11:33:33 PDT

Created attachment 131683 [details] Patch

Alexis Menard (darktears)

Comment 15 2012-03-13 12:33:59 PDT

Created attachment 131694 [details] CPU Monitor while browsing Google Maps... Attached a screenshot of Instruments showing the CPU usage while browsing Google Maps. I did the same manipulation (zooming and panning) with and without the patch. The RUN1 (bottom in the image) is with the patch. It seems that the CPU usage is slightly better.

Luke Macpherson

Comment 16 2012-03-13 17:19:30 PDT

Comment on attachment 131683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131683&action=review > Source/WebCore/css/CSSParser.cpp:482 > +static inline bool isKeywordPropertyID(int propertyId) Would be better to ditch this switch completely and just call map.contains(propertyId). Then you don't have to keep both functions up-to-date. (I am assuming the set of properties listed is identical). > Source/WebCore/css/CSSParser.cpp:608 > +}; Looks much better, thanks. > Source/WebCore/css/CSSParser.cpp:610 > +typedef HashMap<int, PropertyMapEntry> KeywordPropertyMap; I assume that since PropertyMapEntry is non-virtual, this is equivalent to a function pointer now, so this should be fine. > Source/WebCore/css/CSSParser.cpp:614 > + if (map.isEmpty()) { It would probably make sense to initialize this map in a separate function rather than the lookup code, and even better if you can guarantee the map is initialized before propertyForID is called.

Alexis Menard (darktears)

Comment 17 2012-03-13 17:30:22 PDT

Created attachment 131759 [details] Patch

Alexis Menard (darktears)

Comment 18 2012-03-13 17:31:12 PDT

(In reply to comment #16) > (From update of attachment 131683 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131683&action=review > > > Source/WebCore/css/CSSParser.cpp:482 > > +static inline bool isKeywordPropertyID(int propertyId) > > Would be better to ditch this switch completely and just call map.contains(propertyId). Then you don't have to keep both functions up-to-date. (I am assuming the set of properties listed is identical). I thought the same, uploaded it already :). > > > Source/WebCore/css/CSSParser.cpp:608 > > +}; > > Looks much better, thanks. > > > Source/WebCore/css/CSSParser.cpp:610 > > +typedef HashMap<int, PropertyMapEntry> KeywordPropertyMap; > > I assume that since PropertyMapEntry is non-virtual, this is equivalent to a function pointer now, so this should be fine. > > > Source/WebCore/css/CSSParser.cpp:614 > > + if (map.isEmpty()) { > > It would probably make sense to initialize this map in a separate function rather than the lookup code, and even better if you can guarantee the map is initialized before propertyForID is called. Will look into that.

Benjamin Poulain

Comment 19 2012-03-13 22:20:44 PDT

Comment on attachment 131759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131759&action=review I really like the idea. > Source/WebCore/css/CSSParser.cpp:491 > +class PropertyMapEntry { > +public: > + typedef bool (*MatchFunction)(int); > + PropertyMapEntry() : m_match(0) { } > + bool isValid() const { return m_match; } > + PropertyMapEntry(MatchFunction match) : m_match(match) { } > + bool match(int id) const { ASSERT(m_match); return (*m_match)(id); } > +private: > + MatchFunction m_match; > +}; Let's add some spaces in here to improve readability. Please also put the two constructor together. > Source/WebCore/css/CSSParser.cpp:517 > +typedef HashMap<int, PropertyMapEntry> KeywordPropertyMap; I am tempted to ask you to replace this and KeywordPropertyMapEntry by a switch(). :) It could be easier to read, and does not need any initialization. I would also enjoy seeing MatchFunction go away because function pointers are a bit harder to debug. Maybe as a separate patch, you could add a benchmark for CSSStyleProperty? Setting every CSS property to a valid value.

Alexis Menard (darktears)

Comment 20 2012-03-14 05:44:20 PDT

(In reply to comment #19) > (From update of attachment 131759 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131759&action=review > > I really like the idea. > > > Source/WebCore/css/CSSParser.cpp:491 > > +class PropertyMapEntry { > > +public: > > + typedef bool (*MatchFunction)(int); > > + PropertyMapEntry() : m_match(0) { } > > + bool isValid() const { return m_match; } > > + PropertyMapEntry(MatchFunction match) : m_match(match) { } > > + bool match(int id) const { ASSERT(m_match); return (*m_match)(id); } > > +private: > > + MatchFunction m_match; > > +}; > > Let's add some spaces in here to improve readability. > Please also put the two constructor together. > > > Source/WebCore/css/CSSParser.cpp:517 > > +typedef HashMap<int, PropertyMapEntry> KeywordPropertyMap; > > I am tempted to ask you to replace this and KeywordPropertyMapEntry by a switch(). :) > It could be easier to read, and does not need any initialization. > It appears that https://gist.github.com/2036206 is slower than this implementation :(. > I would also enjoy seeing MatchFunction go away because function pointers are a bit harder to debug. Well the problem is that I have to use PropertyMapEntry as a base class so I can put any template variants in the map. I tried to put the match function as virtual but I'm not allocating the entries in the map on the heap and I couldn't make it work. > > > Maybe as a separate patch, you could add a benchmark for CSSStyleProperty? Setting every CSS property to a valid value. I will add a benchmark which read and write css properties from JS (so we're covered). Separate patch though.

Alexis Menard (darktears)

Comment 21 2012-03-14 05:53:06 PDT

(In reply to comment #18) > (In reply to comment #16) > > (From update of attachment 131683 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=131683&action=review > > > > > Source/WebCore/css/CSSParser.cpp:482 > > > +static inline bool isKeywordPropertyID(int propertyId) > > > > Would be better to ditch this switch completely and just call map.contains(propertyId). Then you don't have to keep both functions up-to-date. (I am assuming the set of properties listed is identical). > > I thought the same, uploaded it already :). > > > > > > Source/WebCore/css/CSSParser.cpp:608 > > > +}; > > > > Looks much better, thanks. > > > > > Source/WebCore/css/CSSParser.cpp:610 > > > +typedef HashMap<int, PropertyMapEntry> KeywordPropertyMap; > > > > I assume that since PropertyMapEntry is non-virtual, this is equivalent to a function pointer now, so this should be fine. > > > > > Source/WebCore/css/CSSParser.cpp:614 > > > + if (map.isEmpty()) { > > > > It would probably make sense to initialize this map in a separate function rather than the lookup code, and even better if you can guarantee the map is initialized before propertyForID is called. > > Will look into that. The problem is that the lookup functions are static so the only solution I see is to fill that map when the first CSSParser instance is created...

Alexis Menard (darktears)

Comment 22 2012-03-14 06:01:12 PDT

(In reply to comment #21) > (In reply to comment #18) > > (In reply to comment #16) > > > (From update of attachment 131683 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=131683&action=review > > > > > > > Source/WebCore/css/CSSParser.cpp:482 > > > > +static inline bool isKeywordPropertyID(int propertyId) > > > > > > Would be better to ditch this switch completely and just call map.contains(propertyId). Then you don't have to keep both functions up-to-date. (I am assuming the set of properties listed is identical). > > > > I thought the same, uploaded it already :). > > > > > > > > > Source/WebCore/css/CSSParser.cpp:608 > > > > +}; > > > > > > Looks much better, thanks. > > > > > > > Source/WebCore/css/CSSParser.cpp:610 > > > > +typedef HashMap<int, PropertyMapEntry> KeywordPropertyMap; > > > > > > I assume that since PropertyMapEntry is non-virtual, this is equivalent to a function pointer now, so this should be fine. > > > > > > > Source/WebCore/css/CSSParser.cpp:614 > > > > + if (map.isEmpty()) { > > > > > > It would probably make sense to initialize this map in a separate function rather than the lookup code, and even better if you can guarantee the map is initialized before propertyForID is called. > > > > Will look into that. > > The problem is that the lookup functions are static so the only solution I see is to fill that map when the first CSSParser instance is created... which will be the case as bool parseValue(int propId, bool important); is using it and is called whenever a CSSParser instance is created and the parsing started. I think it's fine the way it is today.

Alexis Menard (darktears)

Comment 23 2012-03-14 06:03:06 PDT

Created attachment 131832 [details] Patch

Tony Chang

Comment 24 2012-03-14 15:05:51 PDT

Comment on attachment 131832 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131832&action=review > Source/WebCore/css/CSSParser.cpp:487 > + PropertyMapEntry(MatchFunction match) : m_match(match) { } explicit > Source/WebCore/css/CSSParser.cpp:512 > + if (behavior == MatchRange && id >= one && id <= two) > + return true; indent is weird > Source/WebCore/css/CSSParser.cpp:539 > + map.set(CSSPropertyDisplay, KeywordPropertyMapEntry<MatchRange, CSSValueInline, CSSValueWebkitInlineFlexbox, CSSValueNone, CSSValueWebkitGrid, CSSValueWebkitInlineGrid>()); WebkitGrid and WebkitInlineGrid come after WebkitInlineFlexbox, so you could just extend the range to WebkitInlineGrid. > Source/WebCore/css/CSSParser.cpp:541 > + map.set(CSSPropertyDisplay, KeywordPropertyMapEntry<MatchRange, CSSValueInline, CSSValueWebkitInlineFlexbox, CSSValueNone>()); indent is weird > Source/WebCore/css/CSSParser.cpp:627 > + if (!string.length()) isEmpty() > Source/WebCore/css/CSSParser.cpp:649 > + if (cssValuePool) > + value = cssValuePool->createInheritedValue(); > + else > + value = CSSInheritedValue::create(); Nit: Use a ternary operator? > Source/WebCore/css/CSSParser.cpp:657 > + if (cssValuePool) > + value = cssValuePool->createExplicitInitialValue(); > + else > + value = CSSInitialValue::createExplicit(); Nit: Use a ternary operator? > Source/WebCore/css/CSSParser.cpp:668 > + if (document) > + value = cssValuePool->createIdentifierValue(valueID); > + else > + value = CSSPrimitiveValue::createIdentifier(valueID); Nit: Use a ternary operator? > Source/WebCore/css/CSSParser.cpp:-1008 > - case CSSPropertyEmptyCells: // show | hide | inherit > - if (id == CSSValueShow Does the switch statement compile if SVG is not enabled? I think clang will complain about a switch with missing values. You probably need to add these with an ASSERT_NOT_REACHED().

Alexis Menard (darktears)

Comment 25 2012-03-14 15:35:47 PDT

Created attachment 131940 [details] Patch

Benjamin Poulain

Comment 26 2012-03-14 16:01:24 PDT

I tried on a quick hand made benchmark: -Before patch: 552.5 -After patch: 369 I wish you could also do a shortcut for -webkit-transform after this patch :)

Tony Chang

Comment 27 2012-03-14 17:38:52 PDT

This looks good to me, but I defer the final review to Antti or someone else. View in context: https://bugs.webkit.org/attachment.cgi?id=131940&action=review > Source/WebCore/css/CSSParser.cpp:2232 > #if ENABLE(SVG) > default: > return parseSVGValue(propId, important); > +#else > + default: > + ASSERT_NOT_REACHED(); > + return false; Nit: I would pull the default: out of the #if.

Antti Koivisto

Comment 28 2012-03-19 11:45:40 PDT

Why does this use complex and borderline unreadable HashMap/template/function pointer setup instead of a simple and readable switch statement? Presumably the performance gains are from skipping the css parser construction and cssyyparse, not from these tricks.

Alexis Menard (darktears)

Comment 29 2012-03-19 13:30:54 PDT

Created attachment 132649 [details] Patch

Antti Koivisto

Comment 30 2012-03-19 14:08:43 PDT

Comment on attachment 132649 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132649&action=review Looking better but r- for various style and other issues. > Source/WebCore/css/CSSParser.cpp:484 > +static inline bool propertyMatchKeyword(int propertyId, int id) > +{ The name here is not great. Maybe isValidKeywordPropertyAndValue() or similar? What 'id'? It should be called 'valueID'. I think we generally capitalize ID so 'propertyID'. > Source/WebCore/css/CSSParser.cpp:489 > + case CSSPropertyBorderCollapse: // collapse | separate | inherit > + if (id == CSSValueCollapse || id == CSSValueSeparate) > + return true; > + break; Double indentation in this switch > Source/WebCore/css/CSSParser.cpp:899 > + CSSParserString cssString; > + cssString.characters = const_cast<UChar*>(string.characters()); > + cssString.length = string.length(); > + int valueID = cssValueKeywordID(cssString); This could be an inline function. Surprised that there is not one already. > Source/WebCore/css/CSSParser.cpp:909 > + if (cssValuePool) > + value = cssValuePool ? cssValuePool->createInheritedValue() : CSSInheritedValue::create(); did you forget that if() there? > Source/WebCore/css/CSSParser.cpp:924 > + declaration->addParsedProperty(CSSProperty(propertyId, value.release(), important)); > + return true; > + } > + if (valueID == CSSValueInitial) { > + value = cssValuePool ? cssValuePool->createExplicitInitialValue() : CSSInitialValue::createExplicit(); > + declaration->addParsedProperty(CSSProperty(propertyId, value.release(), important)); > + return true; > + } > + > + if (!propertyMatchKeyword(propertyId, valueID)) > + return false; > + > + value = document ? cssValuePool->createIdentifierValue(valueID) : CSSPrimitiveValue::createIdentifier(valueID); > + declaration->addParsedProperty(CSSProperty(propertyId, value.release(), important)); > + return true; The code might look nicer and be shorter if factored to create CSSValues in branches and then do declaration->addParsedProperty() once at the end. Basically not using early returns for inherit and initial. > Source/WebCore/css/CSSParser.cpp:1381 > bool validPrimitive = false; > RefPtr<CSSValue> parsedValue; > > + if (id && isKeywordPropertyID(propId)) { > + if (!propertyMatchKeyword(propId, id)) You don't need to invoke isKeywordPropertyID() here at all, propertyMatchKeyword() returns false for non-keyword properties. if() here should be before the initialization of those variables. They are not needed if the branch is taken. Indentation. > Source/WebCore/css/CSSParser.cpp:1383 > + return false; > + parsedValue = parseValidPrimitive(id, value); Why does this call to parseValidPrimitive? You know it is an identifier value, you can just do cssValuePool()->createIdentifierValue(id). > Source/WebCore/css/CSSParser.cpp:1390 > + if (!m_valueList->current() || inShorthand()) { > + addProperty(propId, parsedValue.release(), important); > + return true; > + } > + return false; Would be nicer to switch these branches around as you won't need the block at all.

Alexis Menard (darktears)

Comment 31 2012-03-19 15:53:47 PDT

Created attachment 132693 [details] Patch

Alexis Menard (darktears)

Comment 32 2012-03-19 16:04:26 PDT

(In reply to comment #30) > (From update of attachment 132649 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132649&action=review > > Looking better but r- for various style and other issues. > > > Source/WebCore/css/CSSParser.cpp:484 > > +static inline bool propertyMatchKeyword(int propertyId, int id) > > +{ > > The name here is not great. Maybe isValidKeywordPropertyAndValue() or similar? > > What 'id'? It should be called 'valueID'. I think we generally capitalize ID so 'propertyID'. > > > Source/WebCore/css/CSSParser.cpp:489 > > + case CSSPropertyBorderCollapse: // collapse | separate | inherit > > + if (id == CSSValueCollapse || id == CSSValueSeparate) > > + return true; > > + break; > > Double indentation in this switch > > > Source/WebCore/css/CSSParser.cpp:899 > > + CSSParserString cssString; > > + cssString.characters = const_cast<UChar*>(string.characters()); > > + cssString.length = string.length(); > > + int valueID = cssValueKeywordID(cssString); > > This could be an inline function. Surprised that there is not one already. I will do a separate patch, there are some other candidates. > > > Source/WebCore/css/CSSParser.cpp:909 > > + if (cssValuePool) > > + value = cssValuePool ? cssValuePool->createInheritedValue() : CSSInheritedValue::create(); > > did you forget that if() there? > > > Source/WebCore/css/CSSParser.cpp:924 > > + declaration->addParsedProperty(CSSProperty(propertyId, value.release(), important)); > > + return true; > > + } > > + if (valueID == CSSValueInitial) { > > + value = cssValuePool ? cssValuePool->createExplicitInitialValue() : CSSInitialValue::createExplicit(); > > + declaration->addParsedProperty(CSSProperty(propertyId, value.release(), important)); > > + return true; > > + } > > + > > + if (!propertyMatchKeyword(propertyId, valueID)) > > + return false; > > + > > + value = document ? cssValuePool->createIdentifierValue(valueID) : CSSPrimitiveValue::createIdentifier(valueID); > > + declaration->addParsedProperty(CSSProperty(propertyId, value.release(), important)); > > + return true; > > The code might look nicer and be shorter if factored to create CSSValues in branches and then do declaration->addParsedProperty() once at the end. Basically not using early returns for inherit and initial. > > > Source/WebCore/css/CSSParser.cpp:1381 > > bool validPrimitive = false; > > RefPtr<CSSValue> parsedValue; > > > > + if (id && isKeywordPropertyID(propId)) { > > + if (!propertyMatchKeyword(propId, id)) > > You don't need to invoke isKeywordPropertyID() here at all, propertyMatchKeyword() returns false for non-keyword properties. Well yes but then a case like display: circle will continue to the switch case. > > if() here should be before the initialization of those variables. They are not needed if the branch is taken. > > Indentation. > > > Source/WebCore/css/CSSParser.cpp:1383 > > + return false; > > + parsedValue = parseValidPrimitive(id, value); > > Why does this call to parseValidPrimitive? You know it is an identifier value, you can just do cssValuePool()->createIdentifierValue(id). > > > Source/WebCore/css/CSSParser.cpp:1390 > > + if (!m_valueList->current() || inShorthand()) { > > + addProperty(propId, parsedValue.release(), important); > > + return true; > > + } > > + return false; > > Would be nicer to switch these branches around as you won't need the block at all.

Antti Koivisto

Comment 33 2012-03-20 01:57:09 PDT

Comment on attachment 132693 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132693&action=review r=me, with a few minor comments > Source/WebCore/css/CSSParser.cpp:792 > + case CSSPropertyWordWrap: // normal | break-word > + if (valueID == CSSValueNormal || valueID == CSSValueBreakWord) > + return true; > + break; > + default: > + return false; > + } You should ASSERT_NOT_REACHED() in default case. > Source/WebCore/css/CSSParser.cpp:919 > + if (valueID == CSSValueInherit) > + value = cssValuePool ? cssValuePool->createInheritedValue() : CSSInheritedValue::create(); > + if (valueID == CSSValueInitial) > + value = cssValuePool ? cssValuePool->createExplicitInitialValue() : CSSInitialValue::createExplicit(); > + > + if (!value && !isValidKeywordPropertyAndValue(propertyId, valueID)) > + return false; > + > + if (!value) > + value = document ? cssValuePool->createIdentifierValue(valueID) : CSSPrimitiveValue::createIdentifier(valueID); This can be still improved. Use 'else if's. You won't need the !value tests. > Source/WebCore/css/CSSParser.cpp:1374 > + if (id && isKeywordPropertyID(propId)) { Can the id really be 0? ASSERT(id) perhaps?

Antti Koivisto

Comment 34 2012-03-20 02:21:50 PDT

Comment on attachment 132693 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132693&action=review Switched to r- for concerns over invalid value handling and breaking the no-svg build. >> Source/WebCore/css/CSSParser.cpp:1374 >> + if (id && isKeywordPropertyID(propId)) { > > Can the id really be 0? ASSERT(id) perhaps? Actually the id test is wrong. You will end up in the main switch which won't know how to handle the propId. If id==CSSValueInvalid can actually happen then it needs to be handled within isKeywordPropertyID() branch maybe by having isValidKeywordPropertyAndValue() test it. > Source/WebCore/css/CSSParser.cpp:-979 > - case CSSPropertyPosition: // static | relative | absolute | fixed | inherit > - if (id == CSSValueStatic In non-SVG build the switch won't have default: case so it won't build after your change. I think you should keep these properties in the main switch, together without implementation, and ASSERT_NOT_REACHED.

Alexis Menard (darktears)

Comment 35 2012-03-20 04:20:59 PDT

Created attachment 132795 [details] Patch

Antti Koivisto

Comment 36 2012-03-20 04:23:37 PDT

Comment on attachment 132795 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132795&action=review > Source/WebCore/css/CSSParser.cpp:1387 > + addProperty(propId, cssValuePool()->createIdentifierValue(id), important); two spaces here

Alexis Menard (darktears)

Comment 37 2012-03-20 04:27:54 PDT

Created attachment 132797 [details] Patch for landing

Antti Koivisto

Comment 38 2012-03-20 04:50:41 PDT

Comment on attachment 132797 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=132797&action=review > Source/WebCore/css/CSSParser.cpp:924 > + value = cssValuePool ? cssValuePool->createExplicitInitialValue() : CSSInitialValue::createExplicit(); > + else if (isValidKeywordPropertyAndValue(propertyId, valueID)) > + value = document ? cssValuePool->createIdentifierValue(valueID) : CSSPrimitiveValue::createIdentifier(valueID); Should be cssValuePool instead of document here.

Alexis Menard (darktears)

Comment 39 2012-03-20 07:04:14 PDT

Created attachment 132821 [details] Patch for landing

WebKit Review Bot

Comment 40 2012-03-20 07:46:44 PDT

Comment on attachment 132821 [details] Patch for landing Clearing flags on attachment: 132821 Committed r111394: <http://trac.webkit.org/changeset/111394>

WebKit Review Bot

Comment 41 2012-03-20 07:46:53 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.