Implement a fast path when setting CSS properties with keywords from JS.
Created attachment 131388 [details] Patch
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).
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
Created attachment 131412 [details] Patch
(In reply to comment #4) > Created an attachment (id=131412) [details] > Patch Should fix the regressions.
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.
Is this affecting real-world web pages? I'm surprised that these types of property assignments would be a bottle neck.
(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.
I suspect this might also help on Dromaeo, that is worth testing.
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.
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).
(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.
Created attachment 131677 [details] Patch
Created attachment 131683 [details] Patch
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.
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.
Created attachment 131759 [details] Patch
(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.
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.
(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.
(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...
(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.
Created attachment 131832 [details] Patch
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().
Created attachment 131940 [details] Patch
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 :)
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.
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.
Created attachment 132649 [details] Patch
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.
Created attachment 132693 [details] Patch
(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.
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?
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.
Created attachment 132795 [details] Patch
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
Created attachment 132797 [details] Patch for landing
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.
Created attachment 132821 [details] Patch for landing
Comment on attachment 132821 [details] Patch for landing Clearing flags on attachment: 132821 Committed r111394: <http://trac.webkit.org/changeset/111394>
All reviewed patches have been landed. Closing bug.