973805 - nsObjectFrame.h, nsImageFrame.h, and nsImageControlFrame.cpp could use some more MOZ_OVERRIDE annotations

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P4)

Description

[Daniel Holbert [:dholbert]]

I noticed while reviewing bug 919806 that nsObjectFrame.h is missing MOZ_OVERRIDE annotations on most of the methods that should have it.
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.h

(I haven't looked in detail, but probably all of its methods that are labeled 'virtual' want to be MOZ_OVERRIDE.)

Comment 1

[Daniel Holbert [:dholbert]]

Same goes for nsImageFrame.h.

(Didn't notice any others, but I wasn't looking too closely.)

Comment 2

[Arnaud Sourioux [:Six]]

Hi,

i will do a new pass of my MOZ_OVERRIDE script to see if it catches them now.
Otherwise i will fix it to get them.

Comment 3

[Daniel Holbert [:dholbert]]

Thanks!

Comment 4

[Daniel Holbert [:dholbert]]

Arnaud realized that his scripts from bug 733186 didn't auto-annotate these methods because their parent-class is obscured behind a layer of "#define". e.g.:

> 37 #define nsObjectFrameSuper nsFrame
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.h#37

grep finds 4 instances of this in layout:
> layout/generic/nsObjectFrame.h:37:#define nsObjectFrameSuper nsFrame
> layout/generic/nsImageFrame.h:57:#define ImageFrameSuper nsSplittableFrame
> layout/generic/nsBlockFrame.h:73:#define nsBlockFrameSuper nsContainerFrame
> layout/forms/nsImageControlFrame.cpp:25:#define nsImageControlFrameSuper nsImageFrame

We should probably fix all of those to use typedef. (in part because it's more declarative, and in part because it's easier for code-analysis tools to understand, at least for the library that Arnaud's auto-MOZ_OVERRIDE script uses.)

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> grep finds 4 instances of this in layout:
> > layout/generic/nsBlockFrame.h:73:#define nsBlockFrameSuper nsContainerFrame

[looks like this one is sufficiently MOZ_OVERRIDE'd, from bug 900242]

> > layout/forms/nsImageControlFrame.cpp:25:#define nsImageControlFrameSuper nsImageFrame

[Arnaud says his script found a few MOZ_OVERRIDE opportunities here after the typedef switch, so I'm expanding the bug-summary slightly to cover this file as well]

Comment 6

[Arnaud Sourioux [:Six]]

Attachment: part 2: moz_override_973805.patch

This patch adds MOZ_OVERRIDE to the 3 mentionned files:
- nsObjectFrame.h
- nsImageFrame.h
- nsImageControlFrame.cpp

Comment 7

[Arnaud Sourioux [:Six]]

Attachment: part 1: typedef_instead_define.patch

Change the #define directives by typedefs in:
- layout/generic/nsObjectFrame.h
- layout/generic/nsImageFrame.h
- layout/generic/nsBlockFrame.h
- layout/forms/nsImageControlFrame.cpp

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8377831 [details] [diff] [review]
part 2: moz_override_973805.patch

>Bug 973805: Adds MOZ_OVERRIDE in layout/ forms/nsImageControlFrame.cpp, generic/nsImageFrame.h, generic/nsObjectFrame.h r=dholbert

Looks good!

Just one nit: on bugs with multiple patches, it's nice to put "part 1", "part 2", etc. in the patches' commit messages.

No need to necessarily re-post the patches just to fix that, though; someone could fix that when landing (or not, since it doesn't matter too much).

Comment 9

[Daniel Holbert [:dholbert]]

I verified that this builds locally; given the scope/type of this patch, I don't think we need a Try run on top of that.

So, I pushed (with "part 1" "part 2" commit message tweaks per comment 8):
  https://hg.mozilla.org/integration/mozilla-inbound/rev/82299ce5b67d
  https://hg.mozilla.org/integration/mozilla-inbound/rev/e2ce7a9a1e91

Thanks again for the patches!

Comment 10

[Arnaud Sourioux [:Six]]

Okay,
anyway just for the record it has passed the build: https://tbpl.mozilla.org/?tree=Try&rev=e20d0670e4fd

Thanks for the commit message tip

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/82299ce5b67d
https://hg.mozilla.org/mozilla-central/rev/e2ce7a9a1e91