Closed Bug 973805 Opened 6 years ago Closed 6 years ago
Object Frame .h, ns Image Frame .h, and ns Image Control Frame .cpp could use some more MOZ _OVERRIDE annotations
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.)
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
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.
Status: NEW → ASSIGNED
Severity: normal → enhancement
Priority: -- → P4
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.)
(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): https://hg.mozilla.org/integration/mozilla-inbound/rev/82299ce5b67d https://hg.mozilla.org/integration/mozilla-inbound/rev/e2ce7a9a1e91 Thanks again for the patches!
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
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
Status: ASSIGNED → RESOLVED
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.