Closed
Bug 837013
Opened 11 years ago
Closed 11 years ago
De-virtualize virtual nsTableFrame methods that don't have any overrides
Categories
(Core :: Layout: Tables, defect)
Core
Layout: Tables
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: dholbert, Assigned: glennal.buford)
References
()
Details
(Whiteboard: [mentor=dholbert][lang=c++])
Attachments
(1 file, 1 obsolete file)
6.96 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
Reporter | ||
Comment 2•11 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
Should I remove the virtual if nsTableFrame is overriding the method from some super class?
Reporter | ||
Comment 4•11 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.
That's why I thought I'd ask, as it's a matter of taste in most cases. Thanks!
Removed virtual from methods that are, in fact, not virtual.
Attachment #709500 -
Attachment is patch: true
Reporter | ||
Comment 7•11 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!
Attachment #709500 -
Attachment is obsolete: true
Attachment #709521 -
Flags: review?(dholbert)
Reporter | ||
Comment 9•11 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•11 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•11 years ago
|
||
It is my first! And thanks for being so helpful!
Reporter | ||
Comment 12•11 years ago
|
||
You're very welcome -- thanks for the patch!
Comment 13•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/baa19c2db2af
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in
before you can comment on or make changes to this bug.
Description
•