Closed Bug 973805 Opened 6 years ago Closed 6 years ago

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


(Core :: Layout, enhancement, P4)






(Reporter: dholbert, Assigned: Six)




(2 files)

I noticed while reviewing bug 919806 that nsObjectFrame.h is missing MOZ_OVERRIDE annotations on most of the methods that should have it.

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

(Didn't notice any others, but I wasn't looking too closely.)
OS: Linux → All
Hardware: x86_64 → All
Summary: nsObjectFrame.h needs MOZ_OVERRIDE labelling → nsObjectFrame.h & nsImageFrame.h could use some more MOZ_OVERRIDE annotations

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.
Assignee: nobody → six.dsn
Severity: normal → enhancement
Priority: -- → P4
Depends on: 733186
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

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.)
(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]
Summary: nsObjectFrame.h & nsImageFrame.h could use some more MOZ_OVERRIDE annotations → nsObjectFrame.h, nsImageFrame.h, and nsImageControlFrame.cpp could use some more MOZ_OVERRIDE annotations
This patch adds MOZ_OVERRIDE to the 3 mentionned files:
- nsObjectFrame.h
- nsImageFrame.h
- nsImageControlFrame.cpp
Attachment #8377831 - Flags: review?(dholbert)
Change the #define directives by typedefs in:
- layout/generic/nsObjectFrame.h
- layout/generic/nsImageFrame.h
- layout/generic/nsBlockFrame.h
- layout/forms/nsImageControlFrame.cpp
Attachment #8377839 - Flags: review?(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).
Attachment #8377831 - Flags: review?(dholbert) → review+
Attachment #8377839 - Flags: review?(dholbert) → review+
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):

Thanks again for the patches!
Flags: in-testsuite-
Attachment #8377839 - Attachment description: typedef_instead_define.patch → part 1: typedef_instead_define.patch
Attachment #8377831 - Attachment description: moz_override_973805.patch → part 2: moz_override_973805.patch
anyway just for the record it has passed the build:

Thanks for the commit message tip
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.