[Webkit-unassigned] [Bug 27160] Implement vw/vh/vm (viewport sizes) from CSS 3 Values and Units

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27160





--- Comment #37 from Luke Macpherson <macpherson at chromium.org>  2012-02-15 17:00:46 PST ---
(From update of attachment 127238)
View in context: https://bugs.webkit.org/attachment.cgi?id=127238&action=review

This patch is looking a lot better to me. My only concern right now is that there appear to be a lot of places where calcValue/calcMinValue should be able to handle viewport units, but the viewport size is not being passed in. You would probably pick this up if you were using more interesting test cases. I suggest running through "grep -r calcValue\( Source/WebCore/" and seeing all the other ways that Lengths are used. There are also probably existing test cases that could be modified simply by adding the appropriate units, and that could save you some work.

> Source/WebCore/css/CSSStyleSelector.cpp:5768
> +Length viewportRelativeLength(CSSPrimitiveValue* primitiveValue)

Now that this has been so effectively simplified, could this be made a method on CSSPrimitiveValue? I'm trying to kill the CSSPrimitiveValue::primitiveType() method at the moment (it should currently only be used in SVG code), and would also like to merge all CSSPrimitiveValue to Length conversions into a single point soon.

> Source/WebCore/platform/Length.h:248
> +        return type() == ViewportRelativeWidth || type() == ViewportRelativeHeight || type() == ViewportRelativeMin; 

This can simply be return type() >= ViewportRelativeWidth;

> Source/WebCore/rendering/RenderBox.cpp:1753
> +        logicalWidthResult = computeBorderBoxLogicalWidth(logicalWidth.calcValue(availableLogicalWidth, document()->viewportSize()));

Is these the only place that viewport based units are used? I see 125 references to calcValue() in the codebase - why can't all of them potentially be viewport based?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1078
> +    LayoutUnit xPosition = fillLayer->xPosition().calcMinValue(positioningAreaSize.width() - geometry.tileSize().width(), IntSize(), true);

Can fillLayer->xPosition() return a viewport relative size? If you need to assume this I think you should add an assertion.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1084
> +    LayoutUnit yPosition = fillLayer->yPosition().calcMinValue(positioningAreaSize.height() - geometry.tileSize().height(), IntSize(), true);

Can fillLayer->yPosition() return a viewport relative size? If you need to assume this I think you should add an assertion.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.