Bug 16090 - carto.net Dock example redraws *way* too often
Summary: carto.net Dock example redraws *way* too often
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Dirk Schulze
URL: http://www.carto.net/papers/svg/dock/...
Depends on: 15394
  Show dependency treegraph
Reported: 2007-11-21 22:30 PST by Eric Seidel (no email)
Modified: 2010-10-15 13:03 PDT (History)
3 users (show)

See Also:

Patch (25.72 KB, patch)
2010-10-15 04:31 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (27.50 KB, patch)
2010-10-15 05:48 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (26.98 KB, patch)
2010-10-15 10:28 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2007-11-21 22:30:05 PST
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!
Comment 1 Eric Seidel (no email) 2007-12-15 03:32:53 PST
This is another bug caused by bug 15394.
Comment 2 Eric Seidel (no email) 2008-02-03 15:26:27 PST
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.
Comment 3 Dirk Schulze 2010-10-14 05:55:27 PDT
Have a patch for this problem but need to write a test first.
Comment 4 Dirk Schulze 2010-10-15 04:31:09 PDT
Created attachment 70848 [details]
Comment 5 Nikolas Zimmermann 2010-10-15 04:49:10 PDT
Comment on attachment 70848 [details]

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.
Comment 6 Dirk Schulze 2010-10-15 05:14:04 PDT
Sorry, forgot to delete my debugging code.
Comment 7 Dirk Schulze 2010-10-15 05:48:29 PDT
Created attachment 70857 [details]
Comment 8 Nikolas Zimmermann 2010-10-15 05:54:26 PDT
Comment on attachment 70857 [details]

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() {

Comment 9 Eric Seidel (no email) 2010-10-15 06:11:19 PDT
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.
Comment 10 Nikolas Zimmermann 2010-10-15 06:36:36 PDT
(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.
Comment 11 Dirk Schulze 2010-10-15 10:28:58 PDT
Created attachment 70881 [details]
Comment 12 Nikolas Zimmermann 2010-10-15 11:30:08 PDT
Comment on attachment 70881 [details]

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.
Comment 13 Dirk Schulze 2010-10-15 13:02:54 PDT
Comment on attachment 70881 [details]

Clearing flags on attachment: 70881

Committed r69874: <http://trac.webkit.org/changeset/69874>
Comment 14 Dirk Schulze 2010-10-15 13:03:05 PDT
All reviewed patches have been landed.  Closing bug.