Closed
Bug 480880
Opened 16 years ago
Closed 16 years ago
[FIX]InvalidateCanvasIfNeeded is busted, generally speaking
Categories
(Core :: Web Painting, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b3
People
(Reporter: bzbarsky, Assigned: bzbarsky)
Details
Attachments
(3 files, 3 obsolete files)
|
(deleted),
text/html
|
Details | |
|
(deleted),
text/html
|
Details | |
|
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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...
|
Assignee | |
Comment 1•16 years ago
|
||
Attachment #364834 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 2•16 years ago
|
||
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.
|
|
Assignee | |
Comment 4•16 years ago
|
||
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...
|
|
Assignee | |
Comment 6•16 years ago
|
||
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
|
|
Assignee | |
Comment 7•16 years ago
|
||
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)
|
|
Assignee | |
Updated•16 years ago
|
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.)
|
|
Assignee | |
Comment 10•16 years ago
|
||
> 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.
|
|
Assignee | |
Comment 11•16 years ago
|
||
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.
|
|
Assignee | |
Comment 13•16 years ago
|
||
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.
|
|
Assignee | |
Comment 15•16 years ago
|
||
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+
|
|
Assignee | |
Comment 17•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
|
||
Updated•6 years ago
|
Component: Layout: View Rendering → Layout: Web Painting
You need to log in
before you can comment on or make changes to this bug.
Description
•