836957 - Remove unnecessary GetSkipSides() specializations

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1 v1: Remove unneeded GetSkipSides impls that are entirely header files

Comment 2

[Daniel Holbert [:dholbert]]

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):

./tables/nsTableCellFrame.h
./tables/nsTableRowGroupFrame.h
./tables/nsTableRowFrame.h
./tables/nsTableColGroupFrame.h
./tables/nsTableFrame.h
./generic/nsImageFrame.h
./generic/nsFirstLetterFrame.h
./generic/nsFrameSetFrame.h
./generic/nsGfxScrollFrame.h
./generic/nsGfxScrollFrame.h
./generic/nsSubDocumentFrame.h
./generic/nsCanvasFrame.h
./generic/nsBlockFrame.h
./generic/nsInlineFrame.h
./forms/nsFileControlFrame.h
./forms/nsListControlFrame.h
./forms/nsHTMLButtonControlFrame.h

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)

Comment 3

[Daniel Holbert [:dholbert]]

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.

Comment 4

[Daniel Holbert [:dholbert]]

(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.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: part 2 v1: Remove unneeded GetSkipSides impls that are in .cpp files

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: part 3 v1: Mark remaining methods as MOZ_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.)

Comment 7

[David Baron :dbaron:]

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 8

[Mats Palmgren (inactive)]

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).

Comment 9

[Daniel Holbert [:dholbert]]

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)

Comment 10

[Daniel Holbert [:dholbert]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=8d0adb9f1844

Landed:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/69a6396e7d99
 https://hg.mozilla.org/integration/mozilla-inbound/rev/2c6a7b66968f
 https://hg.mozilla.org/integration/mozilla-inbound/rev/b40b5c2a7592

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/69a6396e7d99
https://hg.mozilla.org/mozilla-central/rev/2c6a7b66968f
https://hg.mozilla.org/mozilla-central/rev/b40b5c2a7592