[webkit-reviews] review granted: [Bug 233651] Follow-up fixes for modern units in legacy CSS : [Attachment 449336] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Sneddon [:gsnedders]
<gsnedders at apple.com>'s request for review:
Bug 233651: Follow-up fixes for modern units in legacy CSS
https://bugs.webkit.org/show_bug.cgi?id=233651

Attachment 449336: Patch

https://bugs.webkit.org/attachment.cgi?id=449336&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 449336
  --> https://bugs.webkit.org/attachment.cgi?id=449336
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449336&action=review

Thank you, so glad you are doing this!

As Myles likes to say, r=mews

> Source/WebCore/ChangeLog:19
> +	   (WebCore::DeprecatedCSSOMPrimitiveValue::primitiveType const): 
Remove post-DOM Level 2 Style values.

Extra space after colon here.

> Source/WebCore/css/CSSUnits.cpp:44
> +    case CSSUnitType::CSS_EMS:

We can also consider renaming these in future. There’s no reason any more that
the enumeration inside the CSS code needs to name these ALL_CAPS nor do they
need a CSS_ prefix since they are already scoped and will have a CSSUnitType::
prefix. And it would be good to be clear that CSSUnitType::CSS_ATTR and
CSS_ATTR need not be the same constant.

> Source/WebCore/css/DeprecatedCSSOMPrimitiveValue.idl:52
> +    // Do not add new units here; this is deprecated and we shouldn't expose
anything not in DOM Level 2 Style.

I think we should put this comment in the .h file too.