According to the standard, calc() should be able to handle angle, time and frequency values as well: http://www.w3.org/TR/css3-values/#calc
Created attachment 240930 [details] Patch This patch will merge Blink commit https://codereview.chromium.org/345903005/ (Patch by Renata Hodovan) which is based on http://trac.webkit.org/changeset/168685, http://trac.webkit.org/changeset/170544 (Patch by Martin Hodovan).
Could you possibly have a little bit of free time to review this patch?
ping?
Could you have a little free time to review this patch?
Comment on attachment 240930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240930&action=review > LayoutTests/ChangeLog:11 > + * css3/calc/calc-errors-expected.txt: Most of these tests are passing without your change. This only one that doesn't is: FAIL Tests calc() with time units. assert_equals: calc(4s + 1s) should equal to 5s expected "5s" but got "0.005s" This does look like a bug. But otherwise, converting time, angle and frequency units in CSS calc() function is already working. Right now, it is not really clear what this patch is doing. I would focus on fixing what actually doesn't work (e.g. the case above) instead of refactoring. > LayoutTests/css3/calc/calc-errors-expected.txt:1 > +unclosed calc Could you explain what changes you did to this test and why? It is not in the Changelog and not obvious to me at least. > Source/WebCore/css/CSSCalculationValue.h:97 > + unsigned short primitiveType() const { return m_expression->primitiveType(); } Do we really need this? CSSPrimitiveValue::primitiveType() already handles calculated values. > Source/WebCore/css/CSSPrimitiveValue.cpp:540 > + switch (isCalculated() ? cssCalcValue()->primitiveType() : m_primitiveUnitType) { Can't you call CSSPrimitiveValue::primitiveType() here?
Comment on attachment 240930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240930&action=review > LayoutTests/css3/calc/calc-with-time-angle-and-frequency-values-expected.txt:2 > +PASS Tests calc() with time units. We need more example that are currently failing. As I said before, most of these seem to be passing without fix. >> Source/WebCore/css/CSSCalculationValue.h:97 >> + unsigned short primitiveType() const { return m_expression->primitiveType(); } > > Do we really need this? CSSPrimitiveValue::primitiveType() already handles calculated values. It seems this is needed after call, but it should probably be called from CSSPrimitiveValue::primitiveType(). >> Source/WebCore/css/CSSPrimitiveValue.cpp:540 >> + switch (isCalculated() ? cssCalcValue()->primitiveType() : m_primitiveUnitType) { > > Can't you call CSSPrimitiveValue::primitiveType() here? Hmm, ok, so CSSPrimitiveValue::primitiveType()'s handling of calculated values is actually not great. It does not really check the unit inside the calculated expression. Instead, it relies on the category and returns CSS_MS if category is CalcTime. We should fix CSSPrimitiveValue::primitiveType(). > Source/WebCore/css/CSSPrimitiveValue.h:-262 > - if (timeUnit == Seconds && m_primitiveUnitType == CSS_S) We could potentially keep the same implementation if we call primitiveType() instead of accessing m_primitiveUnitType directly.
Comment on attachment 240930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240930&action=review > LayoutTests/ChangeLog:3 > + Converting time, angle and frequency units in CSS calc() function What part of the change fixes frequency unit?
Created attachment 244818 [details] Alternative patch As there was no activity for a while, I am uploading another patch with more failing test cases and that updates CSSPrimitiveValue::primitiveType() instead of updating call sites to use CSSCalcValue::primitiveType() directly (more robust this way).
Comment on attachment 244818 [details] Alternative patch Clearing flags on attachment: 244818 Committed r178627: <http://trac.webkit.org/changeset/178627>
All reviewed patches have been landed. Closing bug.