276954 - [FIXr]Create nsLayoutUtils::GetRealFrameFor

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 271422 comment 12.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Add some layout utils methods to hide the casting.

Note that I didn't just put them in the header to avoid having to include
nsIFrame.h and nsPlaceholderFrame.h (the latter not being exported, I'd have
had to munge LOCAL_INCLUDES in places that include nsLayoutUtils.h).  I think
this should build fine as long as callers that use the methods I added include
nsPlaceholderFrame.h and nsIFrame.h themselves (and I did test that this builds
with gcc).

The other option, if this fails to compile, is to make these methods
_IMPL_NS_LAYOUT, as dbaron suggested, and hope that all places that define that
also have the right dirs in LOCAL_INCLUDES.... or just change LOCAL_INCLUDES
appropriately.

I really do want to have these be inline, though, since I'm basically just
replacing a cast with one of the methods in question.

Comment 2

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

Are you sure those are getting inlined? I would think not.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Not completely sure, no.  How would I tell?

I can go the _IMPL_NS_LAYOUT route (and put the bodies in the header too)
instead if you prefer...

Comment 4

[Boris Zbarsky [:bzbarsky]]

So it looks like various content code defines _IMPL_NS_LAYOUT....  Should I just
add layout/generic to the relevant LOCAL_INCLUDES, or export
nsPlaceholderFrame.h?  I guess the former is better, eh?

Comment 5

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

Another option is to just define them as static members of nsPlaceholderFrame.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm...  Yeah, that sounds like a reasonable idea.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: With that change

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 180018 [details] [diff] [review]
With that change

Requesting 1.8 approval.  This is a very safe change; just makes us not copy
this casting code all over.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 180018 [details] [diff] [review]
With that change

a=brendan for 1.8b2.

/be

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fixed

Comment 11

[[e-mail]]

Some of these calls need null-checking:
###!!! ASSERTION: Must have a frame to work with: 'aFrame', file
../generic/nsPlaceholderFrame.h, line 99
which in my case was called from nsFocusIterator.cpp line 564

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix for Neil's crash

All the other places already null-checked; dunno how I missed this one.  :(

Comment 13

[Boris Zbarsky [:bzbarsky]]

Checked in the crash fix.