Summary: | Changing svg transform attribute to the same value triggers repaint | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Patrick R. Gansterer <paroga> | ||||||
Component: | SVG | Assignee: | Patrick R. Gansterer <paroga> | ||||||
Status: | RESOLVED WONTFIX | ||||||||
Severity: | Normal | CC: | buildbot, darin, dglazkov, eric, gustavo.noronha, gustavo, krit, rwlbuis, webkit.review.bot, xan.lopez, zimmermann | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | 55829, 55771 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Patrick R. Gansterer
2011-03-03 07:42:21 PST
Created attachment 84772 [details]
Patch
Comment on attachment 84772 [details]
Patch
Are you sure it's worth the gain for the common-case, where the attribute indeed changed to a new value? Looks like an expensive test for a corner case...
Created attachment 84879 [details] Alternative Patch (depends on bug 55829) (In reply to comment #2) > Are you sure it's worth the gain for the common-case, where the attribute indeed changed to a new value? Looks like an expensive test for a corner case... I'm not 100% happy with the patch either, but I was my first try to fix this problem. I also added a a change comparison in the setAttribute directly, but many unit LayoutTest failed. This patch disables the whole svgAttributeChanged method, if the attribute stays the same but requires bug 55829. Attachment 84879 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8105082 Attachment 84879 [details] did not build on win: Build output: http://queues.webkit.org/results/8101158 Attachment 84879 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8106078 Attachment 84879 [details] did not build on qt: Build output: http://queues.webkit.org/results/8098361 Attachment 84879 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8103166 Attachment 84879 [details] did not build on mac: Build output: http://queues.webkit.org/results/8105104 svgAttributeChanged is kinda a disaster anyway. I can't remember why it's needed. (In reply to comment #10) > svgAttributeChanged is kinda a disaster anyway. I can't remember why it's needed. You're saying this since ages, it's not a disaster in anyway, but just fine. I explained several times why it's needed (unify SVG DOM _and_ XML DOM changes). Comment on attachment 84772 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84772&action=review I don't understand why the code was moved from svgAttributeChanged to parseMappedAttribute. Your ChangeLog should probably explain this. > Source/WebCore/ChangeLog:5 > + Chaning svg transform attribute to the same value triggers repaint Typo: Chaning Comment on attachment 84879 [details] Alternative Patch (depends on bug 55829) View in context: https://bugs.webkit.org/attachment.cgi?id=84879&action=review > Source/WebCore/svg/SVGElement.cpp:386 > - if (attr->name() != HTMLNames::styleAttr) > + if (type != ChangedAttributeToSameValue && attr->name() != HTMLNames::styleAttr) Does HTMLElement or Element do this? Just noticed this nearly 12-month-old bug. Nico: Thoughts on this change? You're much more familiar with SVG these days than I. Comment on attachment 84879 [details] Alternative Patch (depends on bug 55829) With no response, and no answer to Adam's question, marking this r-. Feel free to mark this r? again with questions answered. We do no longer have AttributeChangedType. Transforms should be handled by CSS in the future anyway. |