Closed Bug 480880 Opened 13 years ago Closed 13 years ago

[FIX]InvalidateCanvasIfNeeded is busted, generally speaking

Categories

(Core :: Web Painting, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1b3

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(3 files, 3 obsolete files)

Attached file Reftest (obsolete) —
We call InvalidateCanvasIfNeeded after constructing new frames but before processing pseudo-frames.  That means we can miss newly added frames (reftest coming up that shows that).

I was wondering whether there's a reason we have this complicated function, complete with looking at frames, looking for backgrounds, etc.  Can't we just look at the content we're given in ContentInserted/ContentAppended/ContentRemoved and if it's the <body> kid of a root parent just invalidate the canvas?  That seems like a rare-enough situation that the rare cases when we don't really need to invalidate don't matter that much, I'd think...
Attached file Reference
I agree, we should just invalidate when you append a <body> to the document root element, or when you append a root element to an empty document.
Do we only need to handle DOM mutations here?  Or would a reframe of the <body>s frames due to a display change or whatnot need to hit this code too?

That is, does this check need to live in frame constructor, or can it live in presshell?
Reframes of BODY aren't anything special. Not sure about reframes of the root element though...
OK, did some testing and in fact reframes of <body> have the same problem.  Will add a reftest for that.  So the check really does need to live in the frame constructor.
Target Milestone: --- → mozilla1.9.1b3
Attached patch Proposed patch (obsolete) — Splinter Review
This seems to fix the issue.  Note that of the tests, only 1c, 1d, 1e pass without this patch.  In particular, restyling the root element used to not invalidate the canvas!  This was because RecreateFramesForContent didn't call ContentRemoved in that case.  I changed that, then realized that to be safe we should invalidate in ReconstructDocElementHierarchyInternal as well, since that has an external caller (nsPresShell).  I can undo the change to RecreateFramesForContent and try to write some reftests that actually add/remove the root element without messing up the reftest-wait stuff, but I sort of like having fewer special-cased codepaths.

This applies on top of the patches for bug 482889.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #367495 - Flags: superreview?(roc)
Attachment #367495 - Flags: review?(roc)
Summary: InvalidateCanvasIfNeeded is busted, generally speaking → [FIX]InvalidateCanvasIfNeeded is busted, generally speaking
+InvalidateCanvasIfNeeded(nsIPresShell* presShell, nsIContent* node)

Should rename these parameters to a* I guess.

You're removed the main call to ReconstructDocElementHierarchy and replaced it with the simple remove/add of the root element. That's a bit scary but it seems fine as far as I can tell; anyway, if you're going to remove that call, why not remove the others as well? (Replace them with calls to RecreateFramesForContent I guess.)
(I'd like to get rid of ReconstructDocElementHierarchyInternal because having it do things a bit differently is actually causing problems with nsSVGRenderingObserver and other places.)
> Should rename these parameters to a* I guess.

I could, but then I have to touch more lines of the method.  I guess I'll do that.

How about I not touch the call to ReconstructDocElementHierarchy here, since it should work now in any case, but perhaps get rid of it as part of bug 481806 or a prelude to it?  I agree that it's scary, and that it seems fine, but I'm happier having a separate changeset on a different day in terms of regression-hunting for it.
So I'll rebuild without that change and retest, then post that patch.
(In reply to comment #10)
> > Should rename these parameters to a* I guess.
> 
> I could, but then I have to touch more lines of the method.  I guess I'll do
> that.

This is why I argued forcefully to ditch that naming convention. But I lost :-(.

> How about I not touch the call to ReconstructDocElementHierarchy here, since it
> should work now in any case, but perhaps get rid of it as part of bug 481806 or
> a prelude to it?  I agree that it's scary, and that it seems fine, but I'm
> happier having a separate changeset on a different day in terms of
> regression-hunting for it.

Sounds good.
Attached patch With that change (obsolete) — Splinter Review
Attachment #367495 - Attachment is obsolete: true
Attachment #367527 - Flags: superreview?(roc)
Attachment #367527 - Flags: review?(roc)
Attachment #367495 - Flags: superreview?(roc)
Attachment #367495 - Flags: review?(roc)
This patch still removes the call to ReconstructDocElementHierarchy from nsCSSFrameConstructor::RecreateFramesForContent.
Attachment #367527 - Attachment is obsolete: true
Attachment #367529 - Flags: superreview?(roc)
Attachment #367529 - Flags: review?(roc)
Attachment #367527 - Flags: superreview?(roc)
Attachment #367527 - Flags: review?(roc)
Comment on attachment 367529 [details] [diff] [review]
Now with qref goodness

I pass the test!
Attachment #367529 - Flags: superreview?(roc)
Attachment #367529 - Flags: superreview+
Attachment #367529 - Flags: review?(roc)
Attachment #367529 - Flags: review+
Pushed http://hg.mozilla.org/mozilla-central/rev/a49759b39e1e
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.