Closed Bug 836957 Opened 11 years ago Closed 11 years ago

Remove unnecessary GetSkipSides() specializations


(Core :: Layout, defect)

Not set





(Reporter: dholbert, Assigned: dholbert)




(3 files)

nsIFrame has:
> 2433   virtual int GetSkipSides() const { return 0; }

A bunch of MathML frames, as well as nsTableOuterFrame, have their own overrides for that method, which also return 0.

These overrides can just be removed -- the classes can just inherit the nsIFrame impl rather than overriding it with the same code.
Assignee: nobody → dholbert
Attachment #708834 - Flags: review?(matspal)
For reference -- after this patch is applied, here are the remaining header files that have declarations of GetSkipSides() specializations (excluding nsIFrame.h since that's the root class):


Note that none of those (except for nsIFrame.h) are in the inheritance hierarchy for the frame classes that my patch touches. So, we won't be inheriting any of those implementations -- we'll get the implementation that nsIFrame provides.

The modified MathML classes inherit from nsIFrame via this chain:
 nsMathMLContainerFrame --> nsContainerFrame --> nsSplittableFrame --> nsFrame --> nsIFrame
(and nsTableOuterFrame inherits from nsContainerFrame, too)
Actually, most of those implementations are just "return 0;" in the .cpp file, too.

I'll attach a second patch to rip those ones out, too.
Summary: Remove unnecessary GetSkipSides() specializations in MathML and nsTableOuterFrame → Remove unnecessary GetSkipSides() specializations
Attachment #708834 - Attachment description: fix v1 → part 1 v1: Remove unneeded GetSkipSides impls that are entirely header files
(Based on contextual comments, I'm guessing nsSomethingContainerFrame (nsHTMLContainerFrame?) used to have a pure-virtual GetSkipSides() impl that all of these classes had to override.)
This marks the few remaining (a.k.a. non-trivial) implementations as MOZ_OVERRIDE.

This patch also removes a number of contextual comments that mention GetSkipSides being an "abstract method on nsContainerFrame", because that's doubly incorrect -- it's not abstract, and nsContainerFrame doesn't declare or implement it.)
Attachment #708845 - Flags: review?(matspal)
Just for reference:  any frame class that's splittable ought to have a nontrivial implementation of GetSkipSides.  It might be worth mentioning that in the comment in nsIFrame.h, and perhaps also checking that it's true.
Comment on attachment 708834 [details] [diff] [review]
part 1 v1: Remove unneeded GetSkipSides impls that are entirely header files

It looks like nsMathMLmtdInnerFrame inherits nsBlockFrame::GetSkipSides
so removing its override is a functional change.  I think this change
is correct though.

Same comment for nsFileControlFrame in part 2 (and in this case
I don't think it can be split so nsBlockFrame::GetSkipSides() will
always return zero).
Attachment #708834 - Flags: review?(matspal) → review+
Attachment #708842 - Flags: review?(matspal) → review+
Attachment #708845 - Flags: review?(matspal) → review+
Thanks for catching those two sort-of functional changes! As you said, I think the change gives us the correct behavior in both cases.

FWIW, I tried to come up with a test for the nsMathMLmtdInnerFrame behavior-change, but I couldn't get it to break with unpatched mozilla-central (from print-previewing a really tall <mtd> element).  I didn't actually hit any calls to that class's GetSkipSides method when the mtd was split across pages (though I did get some calls to nsBlockFrame::GetSkipSides() for its continuations, which were of type nsBlockFrame, not nsMathMLmtdInnerFrame)
Depends on: 958249
You need to log in before you can comment on or make changes to this bug.