Last Comment Bug 837013 - De-virtualize virtual nsTableFrame methods that don't have any overrides
: De-virtualize virtual nsTableFrame methods that don't have any overrides
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla21
Assigned To: Glenna
Depends on:
  Show dependency treegraph
Reported: 2013-01-31 18:31 PST by Daniel Holbert [:dholbert]
Modified: 2013-02-04 04:15 PST (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

proposed patch (6.02 KB, patch)
2013-02-03 10:18 PST, Glenna
no flags Details | Diff | Splinter Review
patch (v2) (6.96 KB, patch)
2013-02-03 13:32 PST, Glenna
dholbert: review+
Details | Diff | Splinter Review

Description User image Daniel Holbert [:dholbert] 2013-01-31 18:31:40 PST
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);
> 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.

nsTableFrame class definition:

nsMathMLmtableFrame class definition:
Comment 1 User image Glenna 2013-02-03 09:13:59 PST
Hi, I'd like to take this bug.
Comment 2 User image Daniel Holbert [:dholbert] 2013-02-03 09:20:56 PST
Great! I've marked you as the assignee.

Let me know if you have any questions!
Comment 3 User image Glenna 2013-02-03 09:46:50 PST
Should I remove the virtual if nsTableFrame is overriding the method from some super class?
Comment 4 User image Daniel Holbert [:dholbert] 2013-02-03 09:49:27 PST
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.
Comment 5 User image Glenna 2013-02-03 09:50:16 PST
That's why I thought I'd ask, as it's a matter of taste in most cases. Thanks!
Comment 6 User image Glenna 2013-02-03 10:18:23 PST
Created attachment 709500 [details] [diff] [review]
proposed patch

Removed virtual from methods that are, in fact, not virtual.
Comment 7 User image Daniel Holbert [:dholbert] 2013-02-03 12:38:52 PST
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:
(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.

Comment 8 User image Glenna 2013-02-03 13:32:48 PST
Created attachment 709521 [details] [diff] [review]
patch (v2)
Comment 9 User image Daniel Holbert [:dholbert] 2013-02-03 13:52:25 PST
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.
Comment 10 User image Daniel Holbert [:dholbert] 2013-02-03 13:58:29 PST
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!
Comment 11 User image Glenna 2013-02-03 14:01:59 PST
It is my first! And thanks for being so helpful!
Comment 12 User image Daniel Holbert [:dholbert] 2013-02-03 14:03:57 PST
You're very welcome -- thanks for the patch!
Comment 13 User image Ryan VanderMeulen [:RyanVM] 2013-02-04 04:15:42 PST

Note You need to log in before you can comment on or make changes to this bug.