Bug 16090 – carto.net Dock example redraws *way* too often

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

carto.net Dock example redraws *way* too often

Run Quartz Debug to see what I mean.  It seems setpoints() is called every 50ms.  It refreshes the positions of each of the images.  The images are having attributeChanged() called on them, and because of the evilness that is notifyAttributeChanged, they are redrawing even though x/y/width/height never changed!

[Eric Seidel (no email)]

This is another bug caused by bug 15394.

[Eric Seidel (no email)]

Now that bug 15394 is fixed, we can start to fix all the cases where attribute values are set to the same value and thus we can ignore the change.  This is part of what the MappedAttribute* system takes care of for HTML automagically.

[Dirk Schulze]

Have a patch for this problem but need to write a test first.

[Dirk Schulze]

Created attachment 70848 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 70848 [details]
Patch

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

Great patch, still needs some love.

> WebCore/ChangeLog:12
> +        Also added caching of the repaint rect to avoid mutliple calculations.

typo: multiple.

> WebCore/ChangeLog:21
> +        (WebCore::RenderSVGImage::repaintRectInLocalCoordinates): Give back the stored repaint rect.

s/Give back../Return the cached repaint rect./

> WebCore/rendering/RenderSVGImage.cpp:72
> +        m_needsBoundariesUpdate = true;

This is wrong. You don't need to recalculate the boundaries. m_localBounds won't change.
You only have to notify the parents about the bbox change, because they will map the "local bounds" through the "local transforms".

> WebCore/rendering/RenderSVGImage.cpp:87
> +    bool a = repainter.repaintAfterLayout();
> +    UNUSED_PARAM(a);

Just ignore the parameter.

> WebCore/rendering/RenderSVGImage.cpp:88
> +    m_needsBoundariesUpdate = false;

This should only be set in the if (m_needsBoundariesUpdate) branch, no need to do it every time.

> WebCore/rendering/RenderSVGImage.h:76
>      FloatRect m_localBounds;

Please name it "m_objectBoundingBox" for consistency.

> LayoutTests/svg/custom/repaint-on-constant-size-change.svg:21
> +            }, 50);

This indicates a flaw test. As images don't load immediately, you should wait for the load event to be fired, before doing your actual testing.

[Dirk Schulze]

Sorry, forgot to delete my debugging code.

[Dirk Schulze]

Created attachment 70857 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 70857 [details]
Patch

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

> WebCore/rendering/RenderSVGImage.cpp:80
> +    if (m_needsBoundariesUpdate) {

Please move this section, right after the if (m_needsTRansformUpate) section, as it's done for the other renderers as well.

> WebCore/rendering/RenderSVGImage.cpp:100
> +        m_needsBoundariesUpdate = true;

Okay, I don't see the need for m_needsBoundariesUpdate. The name is misleading, as you already updated the boundaries... just the cached repaint rect updates are missing.
Why don't you just update them here?, and call RenderSVGModelObject::setNeedsBoundariesUpdate(), before calling setNeedsLayout(true)?

> LayoutTests/svg/custom/repaint-on-constant-size-change.svg:3
> +    <image externalResourcesRequired="true" onload="loadEventFired()" xlink:href="../../css2.1/support/swatch-green.png" id="target" x="0" y="0" width="200" height="100" />

Either remove id="target", or use getElementById() in your script.

> LayoutTests/svg/custom/repaint-on-constant-size-change.svg:6
> +	function loadEventFired() {

Indentation.

[Eric Seidel (no email)]

It looks like you're using a repaint test, but not in the way they're designed.  There should be a lighter area showing where the repaint happened.  There is no lighter area for your result.

[Nikolas Zimmermann]

(In reply to comment #9)
> It looks like you're using a repaint test, but not in the way they're designed.  There should be a lighter area showing where the repaint happened.  There is no lighter area for your result.

That's the exepcted result, to see that we're _not_ repainting anymore, if the attribute value is the same :-)
It shows a light area on my unpatchted webkit.

[Dirk Schulze]

Created attachment 70881 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 70881 [details]
Patch

Looks great. Btw, preserveAspectRatio has the same problem.... You might want to add another bug report, and also handle this in updateFromElement.
If you prefer, include it in this patch, or land it as is now and fix it afterwards.

[Dirk Schulze]

Comment on attachment 70881 [details]
Patch

Clearing flags on attachment: 70881

Committed r69874: <http://trac.webkit.org/changeset/69874>

[Dirk Schulze]

All reviewed patches have been landed.  Closing bug.