Bug 233651 – Follow-up fixes for modern units in legacy CSS

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

[Brent Fulgham]

This patch corrects a few items that Darin pointed out after the patch for Bug 22977 landed.

> https://bugs.webkit.org/attachment.cgi?id=437335&action=review
> 
> > Source/WebCore/css/DeprecatedCSSOMPrimitiveValue.cpp:94
> > +    case CSSUnitType::CSS_X:                            return 37;
> 
> Please don’t add this. We don’t need to expose modern units in the legacy
> CSS primitive value object model. Also, this patch has no test coverage for
> this. Just return CSS_UNKNOWN for tis case.
> 
> If you have a different strategy in mind, please add new test cases
> demonstrating what we intend, and consider adding a constant to the IDL file.
> 
> > Source/WebCore/css/DeprecatedCSSOMPrimitiveValue.cpp:136
> > +    case 37:                return m_value->getFloatValue(CSSUnitType::CSS_X);
> 
> Please don’t add this. Same reason as above.

This patch resolves these two issues.

[Radar WebKit Bug Importer]

<rdar://problem/85878291>

[Sam Sneddon [:gsnedders]]

Created attachment 449336 [details]
Patch

[Darin Adler]

Comment on attachment 449336 [details]
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.

[Sam Sneddon [:gsnedders]]

Created attachment 449380 [details]
Patch

[Sam Sneddon [:gsnedders]]

Created attachment 449383 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 449383 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValue.cpp:1125
> +    convertedValue *= *factor;

factor.value() might make this a bit more readable.

> Source/WebCore/css/CSSPrimitiveValue.cpp:1131
> +    convertedValue /= *factor;

Error here. You want if (!factor.value()). I would also use .value() on the next line.

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

Maybe write without the "we". "Only values expose in DOM Level 2 Style should be included".

[Simon Fraser (smfr)]

(In reply to Simon Fraser (smfr) from comment #6)
> Comment on attachment 449383 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449383&action=review
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:1125
> > +    convertedValue *= *factor;
> 
> factor.value() might make this a bit more readable.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:1131
> > +    convertedValue /= *factor;
> 
> Error here. You want if (!factor.value()). I would also use .value() on the
> next line.

I re-read and see that we didn't zero check before. This code would be clearer if we:
i) used a new variable for the second factor
ii) used .has_value() and .value()

[Sam Sneddon [:gsnedders]]

Created attachment 449518 [details]
Patch

[EWS]

Committed r288245 (246198@main): <https://commits.webkit.org/246198@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449518 [details].