Closed Bug 734218 Opened 8 years ago Closed 8 years ago

nsChangeHint_RepaintFrame should only invalidate the bounds of SVG frames, not update them


(Core :: SVG, defect)

Not set





(Reporter: jwatt, Assigned: jwatt)


(Keywords: perf)


(1 file, 2 obsolete files)

It seems that we are currently calling nsSVGUtils::UpdateGraphic on SVG frames that are being restyled with the nsChangeHint_RepaintFrame hint. nsChangeHint_RepaintFrame is supposed to indicate a simple repaint though, and should not require the expensive UpdateCoveredRegion call in nsSVGUtils::UpdateGraphic.
Attached patch patch (obsolete) — Splinter Review
This patch changes the nsSVGUtils::UpdateGraphic call to an nsSVGUtils::InvalidateCoveredRegion call. The only issue with this is that when markers change, we don't currently have any code to update the covered regions of frames that reference the markers (markers are the one SVG effect that are included in covered regions). (We have basically been depending on the broken use of nsSVGUtils::UpdateGraphic to do this.) I've fixed that in the patch too.
Attachment #604156 - Flags: review?(dholbert)
UpdateGraphic calls :InvalidateRenderingObservers(aFrame), too.  That seems like something we might still need to do, but maybe we don't?  (or maybe it'll still get called somehow?  It doesn't look like InvalidateCoveredRegion directly calls it.)
(Or maybe the sorts of changes that would require rendering-observer-invalidation will send additional notifications which will make that invalidation happen elsewhere?)
Attached patch patch (obsolete) — Splinter Review
Oops, I remembered InvalidateRenderingObservers in bug 734079, but I forgot it here. :/ Yeah, we need that, and in fact we need to invalidate the old area too, not just the new. So might as well use nsSVGUtils::UpdateGraphic (but only when we end up in nsSVGEffects::UpdateEffects while handling the nsChangeHint_UpdateEffects hint).

Pushed this to Try to make sure we pass everything:
Attachment #604156 - Attachment is obsolete: true
Attachment #604228 - Flags: review?(dholbert)
Attachment #604156 - Flags: review?(dholbert)
Removed the include directive locally.
Attached patch patchSplinter Review
Actually this probably feels better. (The nsChangeHint_RepaintFrame and nsChangeHint_UpdateEffects hints are passed together when an effect changes.)
Attachment #604228 - Attachment is obsolete: true
Attachment #604251 - Flags: review?(dholbert)
Attachment #604228 - Flags: review?(dholbert)
Comment on attachment 604251 [details] [diff] [review]

Seems reasonable. r=me
Attachment #604251 - Flags: review?(dholbert) → review+
If you invalidate rendering observers you'll get an infinite loop won't you? We update shape with a gradient or pattern, resulting in us doing an update effects change hint which then when if it calls InvalidateRenderingObservers will result in another update. This is async so you won't run out of stack.
The pre-patch code was already doing the rendering observer invalidation since it called nsSVGUtils::UpdateGraphic. The patch just makes us do less work in the case that the nsChangeHint_UpdateEffects hint is not passsed.

The details of how SVG effects change notifications and invalidations work are not entirely clear to me just yet, so I'm not sure if there is indeed a (pre-existing) issue.
dynamic-filter-contents-01b.svg is the sort of thing I was worried about but that seems to work OK. Looks like we remove the rendering observer on the element itself before we update then it gets added back on paint thus avoiding looping.
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.