235264 - Clean up Invalidate API

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We need to convert users of the old nsFrame::Invalidate to use
nsIFrame::Invalidate (eliminating the prescontext parameter). Many call sites
also check to see if the rectangle is empty; I've removed these checks in favour
of a check inside nsIFrame;:Invalidate. I've also located several copies of code
equivalent to Invalidate and replaced them with calls to nsIFrame::Invalidate or
just deleted them if not used. I've also audited many of the callers of
Invalidate and converted some of them to invalidate the proper outline rect if
that seems necessary (more work to prepare for proper CSS2 'outline' support
where the outline is outside the boder).

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 142000 [details] [diff] [review]
fix

This should be fairly straightforward ... at most sites the new code is either
equivalent or invalidates at least as much as before

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsIFrame::GetOutlineRect needs to handle GetOutlineWidth returning false
(although, actually, it may never happen, so maybe an assertion would suffice).

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 142000 [details] [diff] [review]
fix

In nsIFrame.h, it should be clearer that GetOutlineRect returns a rect
that is relative to the frame itself, not the parent (as GetRect does).

You can remove the |rect| variable  at the second change in
nsHTMLFrame.cpp (and replace rect.x and rect.y).

I'd think UpdateViewsForTree might not need to union things into the
bounds when they have views, or might not need to invalidate all the
views separately, but that could probably be an improvement in a later
patch.

Are you sure you can remove the painting suppressed check in
nsBox::Redraw?	That's probably called from places other than where you
expect...

In nsTreeBodyFrame::Invalidate, why not consider outline?

Same file, could you change " ? PR_TRUE : PR_FALSE" to " != nsnull"?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Are you sure you can remove the painting suppressed check in
> nsBox::Redraw? That's probably called from places other than where you
> expect...

I'm removing it from here because when Redraw calls frame->Invalidate, we'll do
the check there.

> In nsTreeBodyFrame::Invalidate, why not consider outline?

Hmm, sure, I'll do that.

The rest of your comments are handled ... I put an assertion that
GetOutlineWidth returns true. To handle the false case, we need a new
CalcOutlineFor method in nsStyleOutline and I think that deserves a separate
patch (perhaps as part of the "turn real outlines on" patch that should be
doable soon).

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Or, as I proposed in bug 236175 comment 15, we need a better system...)

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in.