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

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

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.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

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.

Comment 2

[Daniel Holbert [: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.)

Comment 3

[Daniel Holbert [:dholbert]]

(Or maybe the sorts of changes that would require rendering-observer-invalidation will send additional notifications which will make that invalidation happen elsewhere?)

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch

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:

https://tbpl.mozilla.org/?tree=Try&rev=824862ef0e45

Comment 5

[Jonathan Watt [:jwatt]]

Removed the include directive locally.

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: patch

Actually this probably feels better. (The nsChangeHint_RepaintFrame and nsChangeHint_UpdateEffects hints are passed together when an effect changes.)

Comment 7

[Jonathan Watt [:jwatt]]

https://tbpl.mozilla.org/?tree=Try&rev=e6c90bebeb32

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 604251 [details] [diff] [review]
patch

Seems reasonable. r=me

Comment 9

[Robert Longson [:longsonr]]

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.

Comment 10

[Jonathan Watt [:jwatt]]

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.

Comment 11

[Robert Longson [:longsonr]]

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.

Comment 12

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/9d97a3d7788a