[FIXr]Create nsLayoutUtils::GetRealFrameFor

RESOLVED FIXED in mozilla1.8beta2

Status

()

Core
Layout: Misc Code
P1
normal
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla1.8beta2
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Priority: -- → P1
Target Milestone: --- → mozilla1.8beta
Created attachment 179732 [details] [diff] [review]
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.
Attachment #179732 - Flags: superreview?(roc)
Attachment #179732 - Flags: review?(roc)
Summary: Create nsLayoutUtils::GetRealFrameFor → [FIX]Create nsLayoutUtils::GetRealFrameFor
Target Milestone: mozilla1.8beta1 → mozilla1.8beta2
Are you sure those are getting inlined? I would think not.
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...
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?
Another option is to just define them as static members of nsPlaceholderFrame.
Hmm...  Yeah, that sounds like a reasonable idea.
Attachment #179732 - Flags: superreview?(roc)
Attachment #179732 - Flags: review?(roc)
Created attachment 180018 [details] [diff] [review]
With that change
Attachment #179732 - Attachment is obsolete: true
Attachment #180018 - Flags: superreview?(roc)
Attachment #180018 - Flags: review?(roc)
Attachment #180018 - Flags: superreview?(roc)
Attachment #180018 - Flags: superreview+
Attachment #180018 - Flags: review?(roc)
Attachment #180018 - Flags: review+
Summary: [FIX]Create nsLayoutUtils::GetRealFrameFor → [FIXr]Create nsLayoutUtils::GetRealFrameFor
Target Milestone: mozilla1.8beta2 → mozilla1.8beta3
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.
Attachment #180018 - Flags: approval1.8b2?
Comment on attachment 180018 [details] [diff] [review]
With that change

a=brendan for 1.8b2.

/be
Attachment #180018 - Flags: approval1.8b2? → approval1.8b2+
Fixed
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.8beta3 → mozilla1.8beta2

Comment 11

13 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 180812 [details] [diff] [review]
Fix for Neil's crash

All the other places already null-checked; dunno how I missed this one.  :(
Attachment #180812 - Flags: superreview?(roc)
Attachment #180812 - Flags: review?(roc)

Updated

13 years ago
Blocks: 290400
Attachment #180812 - Flags: superreview?(roc)
Attachment #180812 - Flags: superreview+
Attachment #180812 - Flags: review?(roc)
Attachment #180812 - Flags: review+
Attachment #180812 - Flags: approval1.8b2+
Checked in the crash fix.
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.