Bug 117481 – Make sure to use CSSValueID and CSSPropertyID rather than integers

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Also https://chromium.googlesource.com/chromium/blink/+/4723596eb342119f3cab6ca04be783233c6c2e40

[Alexis Menard (darktears)]

Created attachment 205018 [details]
Patch

[EFL EWS Bot]

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

[Alexis Menard (darktears)]

Created attachment 205020 [details]
attempt to fix efl build

[Benjamin Poulain]

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!

[Alexis Menard (darktears)]

Created attachment 205030 [details]
with benjamin's comments

[Benjamin Poulain]

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.

[Alexis Menard (darktears)]

Created attachment 205031 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 205031 [details]
Patch for landing

Clearing flags on attachment: 205031

Committed r151754: <http://trac.webkit.org/changeset/151754>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.