Last Comment Bug 769942 - DLBI broke invalidation of foreignObjects in filters
: DLBI broke invalidation of foreignObjects in filters
: regression
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla17
Assigned To: Jonathan Watt [:jwatt]
: Jet Villegas (:jet)
Depends on:
Blocks: dlbi
  Show dependency treegraph
Reported: 2012-06-30 12:24 PDT by Jonathan Watt [:jwatt]
Modified: 2012-08-13 05:55 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (6.93 KB, patch)
2012-06-30 12:27 PDT, Jonathan Watt [:jwatt]
longsonr: review+
Details | Diff | Splinter Review

Description User image Jonathan Watt [:jwatt] 2012-06-30 12:24:43 PDT
The DLBI patches broke invalidation of foreignObjects that are under an SVG filter, so it seems we don't have tests for that. Basically what nsSVGForeignObjectFrame::GetInvalidRegion does is wrong. It simply transforms the fO's bounds into nsSVGOuterSVGFrame space and invalidates that area, completely ignoring any filters along the parent chain.

This seems to have happened because the nsSVGUtils::FindFilterInvalidation call in the original patch got changed to an nsSVGUtils::GetPostFilterVisualOverflowRect call after I killed off FindFilterInvalidation. Unfortunately, calling GetPostFilterVisualOverflowRect on the bounds transformed into nsSVGOuterSVGFrame space wasn't the right thing to replace it with though, since that skips filters along the way, only taking account of any filter that may be on the outer-<svg> itself. We should call nsSVGUtils::InvalidateBounds on the nsSVGForeignObjectFrame instead.
Comment 1 User image Jonathan Watt [:jwatt] 2012-06-30 12:27:38 PDT
Created attachment 638136 [details] [diff] [review]
Comment 2 User image Jonathan Watt [:jwatt] 2012-06-30 12:30:12 PDT
I'm leaving on vacation for a week in a few hours, so I'll probably not be able to land this. In fact it might be a good idea to wait and see if DLBI sticks before landing this so that if it needs to be backed out it doesn't add to the pain.

Matt, if you do get backed out, feel free to just directly integrate the changes and test from this patch directly into your own patch, rather than committing it as a separate patch. (If that makes things easier.)
Comment 3 User image Robert Longson 2012-06-30 12:43:02 PDT
Can we get into InvalidateIfChildrenDirty if the foreignObject doesn't have any children? Do we have anything with a foreignObject that doesn't have children where we get an invalidation coming through?
Comment 4 User image Jonathan Watt [:jwatt] 2012-06-30 12:49:01 PDT
The 'kid' in InvalidateIfChildrenDirty is the anonymous child that is always there to wrap any real children that may exist.
Comment 5 User image Jonathan Watt [:jwatt] 2012-06-30 12:49:37 PDT
To be clear, the anonymous child is always there, whether or not there are any real descendants.
Comment 6 User image Jonathan Watt [:jwatt] 2012-07-18 11:01:59 PDT
Checked in the test as:
Comment 7 User image Ed Morley [:emorley] 2012-07-19 07:34:22 PDT
Comment 8 User image Ryan VanderMeulen [:RyanVM] 2012-08-09 08:26:48 PDT
Should this bug have been closed?
Comment 9 User image Jonathan Watt [:jwatt] 2012-08-13 05:46:00 PDT
Yes. Thanks, Ryan.
Comment 10 User image Jonathan Watt [:jwatt] 2012-08-13 05:48:43 PDT
To be clear, the regression test is what Matt wanted checked in. He will/has just incorporated the fixes from the patch into his own patches as appropriate.

Note You need to log in before you can comment on or make changes to this bug.