Use RenderBlock::layout() in RenderSlider

[Kent Tamura]

Use RenderBlock::layout() in RenderSlider

[Kent Tamura]

Created attachment 95152 [details]
Patch

[Kent Tamura]

Comment on attachment 95152 [details]
Patch

Oops, the patch doesn't respect baseSize in RenderSlider::layout().

[WebKit Review Bot]

Comment on attachment 95152 [details]
Patch

Attachment 95152 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8741362

New failing tests:
fast/multicol/client-rects.html

[WebKit Review Bot]

Created attachment 95154 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Kent Tamura]

Created attachment 95155 [details]
Patch 2

[Dimitri Glazkov (Google)]

Comment on attachment 95155 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=95155&action=review

I think this patch might be a tactical win, but not much more. We should still use CSS machinery here.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:-90
> -    // FIXME: Move the logic of positioning the thumb here.

I don' think this should be removed. It's still the right place to position.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:139
> +inline static bool hasVerticalAppearance(HTMLInputElement* input)
> +{
> +    RenderStyle* sliderStyle = input->renderer()->style();
> +    return sliderStyle->appearance() == SliderVerticalPart || sliderStyle->appearance() == MediaVolumeSliderPart;
> +}
> +
> +// Returns a value between 0 and 1.
> +inline static double sliderPosition(HTMLInputElement* element)
> +{
> +    StepRange range(element);
> +    return range.proportionFromValue(range.valueFromElement(element));
> +}
> +
> +void SliderThumbElement::setPositionDuringLayout()
> +{
> +    HTMLInputElement* input = hostInput();
> +    ASSERT(input);
> +    if (!input || !input->renderBox() || !renderBox())
> +        return;
> +
> +    RenderBox* hostBox = input->renderBox();
> +    RenderBox* box = renderBox();
> +    double fraction = sliderPosition(input);
> +    IntRect hostRect = hostBox->contentBoxRect();
> +    double x, y;
> +    if (hasVerticalAppearance(input)) {
> +        x = hostRect.x() + (hostRect.width() - box->width()) / 2;
> +        y = hostRect.y() + nextafter((hostRect.height() - box->height()) + 1, 0) * (1 - fraction);
> +    } else {
> +        x = hostRect.x() + nextafter((hostRect.width() - box->width()) + 1, 0) * fraction;
> +        y = hostRect.y() + (hostRect.height() - box->height()) / 2;
> +    }
> +    box->setX(clampToInteger(x));
> +    box->setY(clampToInteger(y));
> +}

I don't understand why you moved such RenderObject-heavy logic into an Element? Seems like you're mixing render tree and DOM tree logic here. Can this live in RenderSlider?

> Source/WebCore/html/shadow/SliderThumbElement.cpp:164
> +        // Ditto.

This comment is in danger of losing context. You should explicitly refer to the comment above.

> Source/WebCore/rendering/RenderSlider.cpp:117
> +        thumbElement->setPositionDuringLayout();

You might still need LayoutStateMaintainer here. You're changing coordinates, but not accounting for any transforms.

[Kent Tamura]

I understand CSS works.