| Summary: | Converting time, angle and frequency units in CSS calc() function | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tibor Mészáros <mtiborinf> | ||||||
| Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | benjamin, cdumez, commit-queue, darin, kling, koivisto, zalan | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Tibor Mészáros
2014-11-04 08:13:29 PST
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. |