Jonathan Watt [:jwatt] Assignee
	Description • 13 years ago

Bug 667324 discovered that nsSVGForeignObjectFrame::PaintSVG's aDirtyRect may be null because when nsSVGUtils::PaintFrameWithEffects is called it is sometimes explicitly passed nsnull as its aDirtyRect (to mean "paint everything"). This is an issue in nsSVGForeignObjectFrame::PaintSVG though, since we need to pass down an nsRegion _reference_ (not pointer) to nsLayoutUtils::PaintFrame. Right now the code in nsSVGForeignObjectFrame::PaintSVG dereferences aDirtyRect despite the fact that it may be nsnull.

One thing to note is that if aDirtyRect is null, it means that the fO is under a marker, mask or pattern. We may want to just return early and refuse to paint in these cases, or we may wish to pass a maximally sized nsRegion to nsLayoutUtils::PaintFrame to get it to paint everything. In the latter case it's not clear to me what we should pass down. A region the size of the kid's visible region??

	Robert Longson [:longsonr]
	Comment 1 • 13 years ago

Really this is the same as bug 620274.

dirtyRect can only be null for a foreignObject that's a child of a pattern or a mask. In this case IsDisabled() will return false and save us from the dereference but also prevent the foreignObject from being included in the mask or pattern.

The implmentation of IsDisabled() is incorrect as it uses the bounding rectangle where it should be using the values of the content width and height attributes (and possibly the currentCTM in case that's singular). For mask/pattern children the mRect of the foreignObject will always be x,y,width,height=0. In this case we can also detect that with IsValidCoveredRect which will return false.

Once IsDisabled is fixed then we'll need to fix it so that we only dereference dirtyRect if it's not null. We only do that in one place whereas we need to do it in more than one. Other fixes may be required to get foreignObject in mask/pattern to work properly.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 2 • 13 years ago

(In reply to Jonathan Watt [:jwatt] from comment #0)
> One thing to note is that if aDirtyRect is null, it means that the fO is
> under a marker, mask or pattern. We may want to just return early and refuse
> to paint in these cases, or we may wish to pass a maximally sized nsRegion
> to nsLayoutUtils::PaintFrame to get it to paint everything. In the latter
> case it's not clear to me what we should pass down. A region the size of the
> kid's visible region??

Just use the visual overflow area of the frame --- that's "everything".

	Jonathan Watt [:jwatt] Assignee
	Comment 3 • 13 years ago

(In reply to Robert Longson from comment #1)
> Really this is the same as bug 620274.

Hmm, although it's a little confusing that that bug's summary claims checking aDirtyRect is unnecessary, whereas this bug claims it is. ;-)

> Once IsDisabled is fixed then we'll need to fix it so that we only
> dereference dirtyRect if it's not null.

Let's just fix it now.

	Jonathan Watt [:jwatt] Assignee
	Comment 5 • 13 years ago

Meh. Forgot to qref.

	Joe Drew (not getting mail)
	Comment 7 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/b945ae00f5f5