clean up IsFrameOfType

RESOLVED FIXED in mozilla1.9alpha3

Status

()

Core
Layout: Misc Code
P1
normal
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9alpha3
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(1 attachment)

(Assignee)

Description

12 years ago
I propose (as partly discussed about a week ago in dev-tech-layout) cleaning up IsFrameOfType in the manner described below.  This should happen either:
 * on the reflow branch, or
 * after the reflow branch lands,
since doing otherwise would cause a lot of merge conflicts.

I propose that we:

Make IsFrameOfType a public non-virtual method on nsIFrame, which calls GetClassBits, a virtual method.

Define GetClassBits on frame classes as a public *inline* virtual method with only the differences relative to its superclass.  This should allow the compiler to generate virtual methods for each frame class that simply return a constant rather than actually call methods on the parent.

While doing this, we should also ensure we also have a |Super| typedef on each frame class.
(Assignee)

Updated

12 years ago
Blocks: 343308
(Assignee)

Comment 1

11 years ago
So one advantage of the current way of doing things is that some of the type determinations can be more expensive than just class constants.  For example, I was thinking of adding an IsFrameOfType state for HasIntrinsicRatio, which is true for all nsImageFrame, all nsHTMLCanvasFrame, and *some* nsSVGOuterSVGFrame.  The current setup allows that; the proposed one does not.

Maybe this should just be WONTFIX?
(Assignee)

Comment 2

11 years ago
That said, if we stick to the current approach, we should still probably call the superclass methods reliably -- which probably means making all the implementations inline.
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9alpha3
Version: 1.8 Branch → Trunk
(Assignee)

Comment 3

11 years ago
Created attachment 256258 [details] [diff] [review]
patch

So here's a patch to make all IsFrameOfType implementations inline virtual functions, and make them all call their base class IsFrameOfType so that it will be easier to add new bits in the future.

The basic pattern is simple.  If a frame class wants to add bits, it does:

  return base::IsFrameOfType(aFlags & ~(nsIFrame::bit_I_implement));

If a frame wants to remove bits (relatively rare), it does:

  if (aFlags & (nsIFrame::bit_I_dont_implement))
    return PR_FALSE;
  return base::IsFrameOfType(aFlags);

This patch is not intended to change any IsFrameOfType results.  The one possible mistake that I caught is the override in nsMathMLContainerFrame that undoes the eBidiInlineContainer bit implemented by nsInlineFrame.  I'll file a separate bug on determining whether this should be changed.

I also attempted to make the style a little more consistent:
 * using nsIFrame:: throughout, for clarity (since that points to where the
   constants are defined, which might not be obvious) and resistance to
   shadowing
 * using parentheses throughout to group bits, even if only one bit (so that
   it's more obvious where to add new bits, which is easy to mess up with
   the ~ case)

While writing the patch, I checked all base classes to make sure I wasn't adding bits that weren't previously advertised (keeping a mental cache as I went, since lots of the base classes were quite common).

I also promoted a few IsFrameOfType methods to common base classes:  nsGfxCheckboxControlFrame and nsGfxRadioControlFrame to nsFormControlFrame, and nsSVGPathGeometryFrame and half of nsSVGGlyphFrame (which still needs an override for one of the bits) to nsSVGGeometryFrame.

I ran the full reftests, and the results are as expected.

Finally (after running the reftests), I added some debugging code to make frame classes that don't call up to nsFrame::IsFrameOfType assert:  I added two special constants to the enum in nsIFrame.h, and added the assertion at the start of nsFrame::Init.

I'm hoping to get reasonably quick review on this since the patch may have a rather short shelf life.
Attachment #256258 - Flags: superreview?(roc)
Attachment #256258 - Flags: review?(roc)
(Assignee)

Updated

11 years ago
Blocks: 371481
(Assignee)

Comment 4

11 years ago
(In reply to comment #3)
> The one
> possible mistake that I caught is the override in nsMathMLContainerFrame that
> undoes the eBidiInlineContainer bit implemented by nsInlineFrame.  I'll file a
> separate bug on determining whether this should be changed.

Filed bug 371481.
(Assignee)

Updated

11 years ago
Priority: -- → P1
Whiteboard: [patch]
Note that we have frame impls that don't call nsFrame::Init.... :(
Comment on attachment 256258 [details] [diff] [review]
patch

I feel like I should have something to say about a patch this size, but I don't...
Attachment #256258 - Flags: superreview?(roc)
Attachment #256258 - Flags: superreview+
Attachment #256258 - Flags: review?(roc)
Attachment #256258 - Flags: review+
(Assignee)

Comment 7

11 years ago
(In reply to comment #5)
> Note that we have frame impls that don't call nsFrame::Init.... :(

I sort of figured that, but the same would probably be true for pretty much any entry point (although maybe nsFrame::Destroy is more reliable -- except that somebody could write a frame class that is always leaked), and I didn't want to scatter the same assertion everywhere.  The point is to help people catch mistakes, not guarantee that they're never made.
(Assignee)

Comment 8

11 years ago
Checked in to trunk about an hour ago.  I think that's it for this bug; marking fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.