Bug 229777 - Support the `x` resolution unit
Summary: Support the `x` resolution unit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Sneddon [:gsnedders]
URL:
Keywords: InRadar
Depends on:
Blocks: 78087 233651
  Show dependency treegraph
 
Reported: 2021-09-01 16:31 PDT by Sam Sneddon [:gsnedders]
Modified: 2021-11-30 12:00 PST (History)
3 users (show)

See Also:


Attachments
Patch (38.40 KB, patch)
2021-09-03 21:04 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Sneddon [:gsnedders] 2021-09-01 16:31:41 PDT
https://drafts.csswg.org/css-values-4/#x

This is identical to dppx, but allows for nicer statements like `resolution: 2x`.
Comment 1 Sam Sneddon [:gsnedders] 2021-09-03 21:04:05 PDT
Created attachment 437335 [details]
Patch

This might depend on bug 229776, not quite sure if it'll apply cleanly without
Comment 2 Sam Sneddon [:gsnedders] 2021-09-03 21:09:42 PDT
Ah yes, the updates to LayoutTests/imported/w3c/web-platform-tests/css/mediaqueries/test_media_queries-expected.txt depend on bug 229776. Could in principle land this without, but the gain is much smaller.
Comment 3 Radar WebKit Bug Importer 2021-09-08 16:32:22 PDT
<rdar://problem/82897690>
Comment 4 EWS 2021-09-14 10:08:12 PDT
Committed r282396 (241657@main): <https://commits.webkit.org/241657@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437335 [details].
Comment 5 Darin Adler 2021-09-14 11:22:06 PDT
Comment on attachment 437335 [details]
Patch

View in context: 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.
Comment 6 Brent Fulgham 2021-11-30 12:00:28 PST
(In reply to Darin Adler from comment #5)
> Comment on attachment 437335 [details]
> Patch
> 
> View in context:
> 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.

These comments will be addressed in Bug 233651.