802628 - Fix broken invalidation in ReflowSVG implementations

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

SVG still has its own invalidation mechanisms after landing of DLBI (bug 539356). There are a bunch of SVG invalidation bugs that were caused by the landing of DLBI, mostly because DLBI and the existing SVG invalidation mechanisms don't play nicely together. Ultimately we want SVG to solely use DLBI for invalidation, and it's probably easier to implement that than to fix all the DLBI SVG invalidation regressions individually. This bug is for getting rid of the SVG invalidation mechanisms and converting SVG to use DLBI.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: Un-inline nsSVGUtils::InvalidateAndScheduleReflowSVG

Simply to make the real patches easier to read.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: Fix broken invalidation in ReflowSVG implementation

One of the optimization in the old SVG invalidation code was that it wouldn't bother invalidating a frame if it knew the parent frame would be invalidating since the parents overflow area would include its children. In an active layers world that's not valid since we can have active layers between the outermost dirty container and dirty children. We need to invalidate all leaf SVG frames that are dirty, and thus there's no point in invalidating SVG containers (which never paint).

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: Fix broken invalidation in ReflowSVG implementation

Err, this is the correct patch for that.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 692796 [details] [diff] [review]
Fix broken invalidation in ReflowSVG implementation

Review of attachment 692796 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/nsSVGGlyphFrame.cpp
@@ +547,5 @@
>  
>    mState &= ~(NS_FRAME_FIRST_REFLOW | NS_FRAME_IS_DIRTY |
>                NS_FRAME_HAS_DIRTY_CHILDREN);
>  
> +  InvalidateFrame();

Don't we still want to at least check NS_FRAME_IS_DIRTY, and NS_FRAME_FIRST_REFLOW on the parent?

Ideally though, we wouldn't ever call InvalidateFrame from reflow. The only time we should need to invalidate during reflow is if the size/overflow changes, and DLBI should pick those up automatically (using geometry comparison).

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: Fix broken invalidation in ReflowSVG implementation

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Don't we still want to at least check NS_FRAME_IS_DIRTY, and
> NS_FRAME_FIRST_REFLOW on the parent?

For leaf frames, we know that NS_FRAME_IS_DIRTY is set because otherwise we wouldn't have ended up in ReflowSVG. I guess I can keep the NS_FRAME_FIRST_REFLOW check in order to do less work.

> Ideally though, we wouldn't ever call InvalidateFrame from reflow. The only
> time we should need to invalidate during reflow is if the size/overflow
> changes, and DLBI should pick those up automatically (using geometry
> comparison).

Right. Removal of the InvalidateFrame calls comes later as a follow-up. Right now I'm trying to stay close to what the current code does though to minimize risk for branch landings.

Comment 6

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd33e059b4d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/b910863e5aa8

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bd33e059b4d6
https://hg.mozilla.org/mozilla-central/rev/b910863e5aa8

Comment 8

[Jonathan Watt [:jwatt]]

Comment on attachment 692793 [details] [diff] [review]
Un-inline nsSVGUtils::InvalidateAndScheduleReflowSVG

Requesting approval on these two patches in order to fix bug 786894.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 776054
User impact if declined: dynamic SVG (scripted/animated) will frequently be broken
Testing completed (on m-c, etc.): passed m-i and landed on m-c now
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

Comment 9

[bhavana bajaj [:bajaj]]

Comment on attachment 692793 [details] [diff] [review]
Un-inline nsSVGUtils::InvalidateAndScheduleReflowSVG

Approving on aurora/beta as its a low risk patch for an SVG regression only on FF18/19.

Comment 10

[Jonathan Watt [:jwatt]]

Landed both part 1 and 2 on beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/76c63f26ba46
https://hg.mozilla.org/releases/mozilla-beta/rev/f887c6fc51d7

Aurora is currently closed, so I'll land on it later.

Comment 11

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4fa6c5187555
https://hg.mozilla.org/releases/mozilla-aurora/rev/a723f289a56d

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/76c63f26ba46
https://hg.mozilla.org/releases/mozilla-b2g18/rev/f887c6fc51d7

Comment 13

[Jonathan Watt [:jwatt]]

To make regression tracking cleaner given the patches landed on branches I'm closing this bug and have opened bug 827915 for the remaining work.