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!
This is another bug caused by bug 15394.
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.
Have a patch for this problem but need to write a test first.
Created attachment 70848 [details] Patch
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.
Sorry, forgot to delete my debugging code.
Created attachment 70857 [details] Patch
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.
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.
(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.
Created attachment 70881 [details] Patch
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.
Comment on attachment 70881 [details] Patch Clearing flags on attachment: 70881 Committed r69874: <http://trac.webkit.org/changeset/69874>
All reviewed patches have been landed. Closing bug.