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.
<rdar://problem/85878291>
Created attachment 449336 [details] Patch
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.
Created attachment 449380 [details] Patch
Created attachment 449383 [details] Patch
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".
(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()
Created attachment 449518 [details] Patch
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].