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


Attachments
patch (6.93 KB, patch)
2012-06-30 12:27 PDT, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
longsonr: review+
Details | Diff | Splinter Review

Description Jonathan Watt [:jwatt] (back in October - email directly if necessary) 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 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-06-30 12:27:38 PDT
Created attachment 638136 [details] [diff] [review]
patch
Comment 2 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 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 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 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 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 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 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 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-07-18 11:01:59 PDT
Checked in the test as:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9aa4f137061e
Comment 7 Ed Morley [:emorley] 2012-07-19 07:34:22 PDT
https://hg.mozilla.org/mozilla-central/rev/9aa4f137061e
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-08-09 08:26:48 PDT
Should this bug have been closed?
Comment 9 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-08-13 05:46:00 PDT
Yes. Thanks, Ryan.
Comment 10 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 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.