De-virtualize virtual nsTableFrame methods that don't have any overrides

RESOLVED FIXED in mozilla21

Status

()

Core
Layout: Tables
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dholbert, Assigned: Glenna)

Tracking

Trunk
mozilla21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=dholbert][lang=c++], URL)

Attachments

(1 attachment, 1 obsolete attachment)

6.96 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
nsTableFrame only has one derived class, nsMathMLmtableFrame, as shown by this search:
  https://mxr.mozilla.org/mozilla-central/search?string=public%20nsTableFrame

nsTableFrame declares a lot of new methods as 'virtual' that don't actually need to be virtual, because nsMathMLmtableFrame doesn't override them -- e.g.:
> 352   virtual int32_t GetColumnWidth(int32_t aColIndex);
> 353 
> 354   /** helper to get the cell spacing X style value */
> 355   virtual nscoord GetCellSpacingX();
> 356 
> 357   /** helper to get the cell spacing Y style value */
> 358   virtual nscoord GetCellSpacingY();

Filing this bug on dropping the 'virtual' keyword from any nsTableFrame methods that are marked as 'virtual' but aren't overridden.

HANDY LINKS:
nsTableFrame class definition:
 https://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.h

nsMathMLmtableFrame class definition:
 https://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmtableFrame.h#73
(Reporter)

Updated

4 years ago
(Assignee)

Comment 1

4 years ago
Hi, I'd like to take this bug.
(Reporter)

Comment 2

4 years ago
Great! I've marked you as the assignee.

Let me know if you have any questions!
Assignee: nobody → glennal.buford
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
Should I remove the virtual if nsTableFrame is overriding the method from some super class?
(Reporter)

Comment 4

4 years ago
I'd lean towards leaving the "virtual" in that case -- while it's technically unnecessary (since the virtual-ness is determined by the super class), it's also a useful reminder that the method *is* virtual.
(Assignee)

Comment 5

4 years ago
That's why I thought I'd ask, as it's a matter of taste in most cases. Thanks!
(Assignee)

Comment 6

4 years ago
Created attachment 709500 [details] [diff] [review]
proposed patch

Removed virtual from methods that are, in fact, not virtual.
(Assignee)

Updated

4 years ago
Attachment #709500 - Attachment is patch: true
(Reporter)

Comment 7

4 years ago
Comment on attachment 709500 [details] [diff] [review]
proposed patch

Looks good!

Just one thing I noticed to fix -- could you fix up the indentation on subsequent lines, in chunks like this:
>-  virtual int32_t  GetEffectiveRowSpan(int32_t                 aStartRowIndex,
>+  int32_t  GetEffectiveRowSpan(int32_t                 aStartRowIndex,
>                                        const nsTableCellFrame& aCell) const;
so that the arguments will still line up like they did before?

Also, add a commit message to the patch headers, as described here:
  https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
(I'd call these methods "unnecessarily virtual", rather than "not virtual" as your current patch title calls them.  Note that they *are* virtual, there's just no reason for them to be.

Once you've done those things, please re-post the patch and request review from me (by toggling the "review" flag to "?", and then typing dholbert into the field that appears after that.

Thanks!
(Assignee)

Comment 8

4 years ago
Created attachment 709521 [details] [diff] [review]
patch (v2)
Attachment #709500 - Attachment is obsolete: true
Attachment #709521 - Flags: review?(dholbert)
(Reporter)

Comment 9

4 years ago
Comment on attachment 709521 [details] [diff] [review]
patch (v2)

Looks great!

I'd suggest adding "...from nsTableFrame" to the commit message -- I'll make that tweak and check this in.
Attachment #709521 - Flags: review?(dholbert) → review+
(Reporter)

Comment 10

4 years ago
Landed on mozilla-inbound:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/baa19c2db2af
(Every day or so, someone merges mozilla-inbound over to mozilla-central; when that happens, they'll close this bug.)

Congrats on your first (I think?) checked-in mozilla patch!
(Assignee)

Comment 11

4 years ago
It is my first! And thanks for being so helpful!
(Reporter)

Comment 12

4 years ago
You're very welcome -- thanks for the patch!
https://hg.mozilla.org/mozilla-central/rev/baa19c2db2af
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.