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
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] proposed patch Removed virtual from methods that are, in fact, not virtual.
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!
Created attachment 709521 [details] [diff] [review] patch (v2)
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.
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!
It is my first! And thanks for being so helpful!
You're very welcome -- thanks for the patch!