[webkit-reviews] review denied: [Bug 29084] getComputedStyle returns percentage values for left / right / top / bottom : [Attachment 190170] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Mar 8 13:15:01 PST 2013


Eric Seidel <eric at webkit.org> has denied Tim 'mithro' Ansell
<mithro at mithis.com>'s request for review:
Bug 29084: getComputedStyle returns percentage values for left / right / top /
bottom
https://bugs.webkit.org/show_bug.cgi?id=29084

Attachment 190170: Patch
https://bugs.webkit.org/attachment.cgi?id=190170&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=190170&action=review


OK.  This change looks great.  My only concern is about testing.  There are a
lot of cases here.  I'm not sure you covered position: sticky.	You shoudl be
aware of the many types of lenghts:
enum LengthType {
    Auto, Relative, Percent, Fixed,
    Intrinsic, MinIntrinsic,
    MinContent, MaxContent, FillAvailable, FitContent,
    Calculated,
    ViewportPercentageWidth, ViewportPercentageHeight, ViewportPercentageMin,
ViewportPercentageMax,
    Undefined
};

You shouldn't need to care about those, but its good to knwo it exists.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:641
> +    // See
http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSview-getComputedStyle

Maybe you meant this?
http://dev.w3.org/csswg/cssom/#widl-Window-getComputedStyle-CSSStyleDeclaration
-Element-elt

If the property applies to a positioned element and the resolved value of the
'display' property is not none, the resolved value is the used value. Otherwise
the resolved value is the computed value.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:645
> +	       return zoomAdjustedPixelValueForLength(style->left(), style);

I might have moved these switch statements into helper functions.  Then this
code looks like:

if (!renderView || !renderer || !renderer->isBox())
    return zoomAdjustedPixelValueForLength(lengthForProperty(propertyId),
style);

One could do similar for the other switches, presumably:
relativePositionForProperty(propertyId, box);
and
positionForProperty(propertyId, box, containingBlock);

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:-661
> -	   else if (l.isViewportPercentage())

Do we have test coverage for this behavior?  You appear to be removing it in
the display: none case.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:659
> +    if (box->isRelPositioned() || !containingBlock) {

I don't believe we can hit this case?  You will always have a containing block
if you're in the DOM tree, I think?  Unless we're talking about <html
style='position: relative'>?  If the element isn't in the tree, it can't have a
renderer.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:687
> +    return 0;

We need tests requesting top/right/bottom/left on elements which are not
positioned.


More information about the webkit-reviews mailing list