nsTableFrame only has one derived class, nsMathMLmtableFrame, as shown by this search:
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);
> 354 /** helper to get the cell spacing X style value */
> 355 virtual nscoord GetCellSpacingX();
> 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.
nsTableFrame class definition:
nsMathMLmtableFrame class definition:
Hi, I'd like to take this bug.
Great! I've marked you as the assignee.
Let me know if you have any questions!
Should I remove the virtual if nsTableFrame is overriding the method from some super class?
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.
That's why I thought I'd ask, as it's a matter of taste in most cases. Thanks!
Created attachment 709500 [details] [diff] [review]
Removed virtual from methods that are, in fact, not virtual.
Comment on attachment 709500 [details] [diff] [review]
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:
(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.
Created attachment 709521 [details] [diff] [review]
Comment on attachment 709521 [details] [diff] [review]
I'd suggest adding "...from nsTableFrame" to the commit message -- I'll make that tweak and check this in.
Landed on mozilla-inbound:
(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!
It is my first! And thanks for being so helpful!
You're very welcome -- thanks for the patch!