[Webkit-unassigned] [Bug 75091] [SVG] Text does not show after scripted scaling.

[bugzilla-daemon at webkit.org]

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





--- Comment #8 from Nikolas Zimmermann <zimmermann at kde.org>  2012-02-15 15:08:17 PST ---
(From update of attachment 127235)
View in context: https://bugs.webkit.org/attachment.cgi?id=127235&action=review

Pre-review: Very interesting patch, looking forward to the next version :-)

> Source/WebCore/css/CSSStyleSelector.h:-38
> -enum ESmartMinimumForFontSize { DoNotUseSmartMinimumForFontSize, UseSmartMinimumForFontFize };

You could land this separated, but it's not a must :-)

> Source/WebCore/rendering/svg/RenderSVGTransformableContainer.cpp:42
>  
> +bool RenderSVGTransformableContainer::transformToRootChanged()
> +{
> +    RenderObject* ancestor = parent();

Interessting approach, though very differently from what we do for eg. setNeedsBoundariesUpdate.

void RenderObject::setNeedsBoundariesUpdate()
{
    if (RenderObject* renderer = parent())
        renderer->setNeedsBoundariesUpdate();
}

We could implement setTransformToRootChanged the same way. Default impl: pass to parent, and in RenderSVGTransformableContainer::setTransformToRootChanged, you can set it to true. Then just look at each place where we call setNeedsTransformUpdate, and also call setTransformToRootChanged, to make sure each ancestor is notified about the change. That would avoid the need to crawl the tree again when doing layout(), as it's already done eg. from SVGStyedTransformableElement::svgAttributeChanged, if 'transform' changed. (Of course there are more places, just an example).

What do you think?

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:249
> +static inline bool transformToRootChanged(RenderObject* start)

You probably want to share this in eg. SVGRenderSupport, or a more specific class.

> LayoutTests/svg/text/text-rescale.html:20
> +    function runRepaintTest() {

This function needs to be named repaintTest(), otherwise you're overriding the function from repaint.js.
You're currently not using layoutTestController.display() at all - you're not tracking repainting, thus no grey rect is painted over the view after the initial painting.
Using reftests together with repaint tests, thus doensn't make much sense, otherwise you'd need to paint the gray overlay rects and white highlight rects yourself in the expected.html file!

I'd advice to make this a pixel test using repaint.js only, and not make it a reftest. Whenever we want to test repainting reliable, this is the way to go.
For any static testcase reftests are the way to go in the future.

> LayoutTests/svg/text/text-rescale.html:22
> +        if (window.layoutTestController)
> +            layoutTestController.waitUntilDone();

This is not needed anymore, once you fix the repaintTest() name clash.

> LayoutTests/svg/text/text-rescale.html:30
> +            if (window.layoutTestController)
> +                layoutTestController.notifyDone();
> +        }, 1);

Ditto.

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