Consider merging https://chromium.googlesource.com/chromium/blink/+/cdf054eda8bdc38052421f32c62738dc7aa5e097 This will ensure that types are enforced when passing arguments in various css functions. Having a compile time check make sure that no mistakes will be done over the type. The parser was also patched to directly use CSSValueID over integers. This patch also fixes various call sites abusing the usage of integers.
Also https://chromium.googlesource.com/chromium/blink/+/4723596eb342119f3cab6ca04be783233c6c2e40
Created attachment 205018 [details] Patch
Comment on attachment 205018 [details] Patch Attachment 205018 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/945270
Created attachment 205020 [details] attempt to fix efl build
Comment on attachment 205020 [details] attempt to fix efl build View in context: https://bugs.webkit.org/attachment.cgi?id=205020&action=review > Source/WebCore/css/CSSParser.cpp:5646 > + addProperty(CSSPropertyFontWeight, cssValuePool().createIdentifierValue(static_cast<CSSValueID>(CSSValue100 + weight / 100 - 1)), important); I reeeeaaally don't like this. I would make it a separate function, with pre and post assertions, and a comment. > Source/WebCore/css/CSSParser.cpp:12236 > - return 0; // illegal character > + return CSSValueInvalid; // illegal character Fix the comment. > Source/WebCore/css/CSSValuePool.cpp:55 > - if (ident <= 0 || ident >= numCSSValueKeywords) > + if (ident <= 0) > return CSSPrimitiveValue::createIdentifier(ident); 1) Why would that happen with valid CSSValueID? Shouldn't you instead have two constructors?: one taking an int, the other taking a CSSValueID. > Source/WebCore/css/CSSValuePool.cpp:57 > if (!m_identifierValueCache[ident]) 2) Because of this, you should have a Release Assert for the range of ident!
Created attachment 205030 [details] with benjamin's comments
Comment on attachment 205030 [details] with benjamin's comments View in context: https://bugs.webkit.org/attachment.cgi?id=205030&action=review LGTM > Source/WebCore/css/CSSValuePool.cpp:58 > + ASSERT(ident < numCSSValueKeywords); > if (!m_identifierValueCache[ident]) You should have a full bound checking release assert here.
Created attachment 205031 [details] Patch for landing
Comment on attachment 205031 [details] Patch for landing Clearing flags on attachment: 205031 Committed r151754: <http://trac.webkit.org/changeset/151754>
All reviewed patches have been landed. Closing bug.