Bug 233651

Summary: Follow-up fixes for modern units in legacy CSS
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: CSSAssignee: Sam Sneddon [:gsnedders] <gsnedders>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, darin, esprehn+autocc, ews-watchlist, glenn, gsnedders, gyuyoung.kim, kondapallykalyan, macpherson, menard, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 229777    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch ews-feeder: commit-queue-

Brent Fulgham
Reported 2021-11-30 11:58:37 PST
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.
Attachments
Patch (31.54 KB, patch)
2022-01-17 08:35 PST, Sam Sneddon [:gsnedders] 		no flags
DetailsFormatted DiffDiff
Patch (37.88 KB, patch)
2022-01-18 04:45 PST, Sam Sneddon [:gsnedders] 		no flags
DetailsFormatted DiffDiff
Patch (38.03 KB, patch)
2022-01-18 06:31 PST, Sam Sneddon [:gsnedders] 		no flags
DetailsFormatted DiffDiff
Patch (38.18 KB, patch)
2022-01-19 14:14 PST, Sam Sneddon [:gsnedders] 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2021-11-30 11:59:07 PST
<rdar://problem/85878291>
Sam Sneddon [:gsnedders]
Comment 2 2022-01-17 08:35:09 PST
Created attachment 449336 [details] Patch
Darin Adler
Comment 3 2022-01-17 09:53:23 PST
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]
Comment 4 2022-01-18 04:45:42 PST
Created attachment 449380 [details] Patch
Sam Sneddon [:gsnedders]
Comment 5 2022-01-18 06:31:14 PST
Created attachment 449383 [details] Patch
Simon Fraser (smfr)
Comment 6 2022-01-18 08:24:42 PST
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)
Comment 7 2022-01-18 08:38:55 PST
(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]
Comment 8 2022-01-19 14:14:23 PST
Created attachment 449518 [details] Patch
EWS
Comment 9 2022-01-19 15:14:51 PST
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].
Note You need to log in before you can comment on or make changes to this bug.