Remove unnecessary GetSkipSides() specializations

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Assignee

Description

7 years ago
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

Comment 1

7 years ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee

Updated

7 years ago
Attachment #708834 - Flags: review?(matspal)
Assignee

Comment 2

7 years ago
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)
Assignee

Comment 3

7 years ago
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
Assignee

Updated

7 years ago
Attachment #708834 - Attachment description: fix v1 → part 1 v1: Remove unneeded GetSkipSides impls that are entirely header files
Assignee

Comment 4

7 years ago
(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.)
Assignee

Comment 6

7 years ago
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+

Updated

7 years ago
Attachment #708842 - Flags: review?(matspal) → review+

Updated

7 years ago
Attachment #708845 - Flags: review?(matspal) → review+
Assignee

Comment 9

6 years ago
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)

Updated

6 years ago
Depends on: 958249
You need to log in before you can comment on or make changes to this bug.