Closed
Bug 973805
Opened 11 years ago
Closed 11 years ago
nsObjectFrame.h, nsImageFrame.h, and nsImageControlFrame.cpp could use some more MOZ_OVERRIDE annotations
Categories
(Core :: Layout, enhancement, P4)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: dholbert, Assigned: Six)
References
Details
Attachments
(2 files)
10.89 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
3.08 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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.)
Reporter | ||
Comment 1•11 years ago
|
||
Same goes for nsImageFrame.h.
(Didn't notice any others, but I wasn't looking too closely.)
Reporter | ||
Updated•11 years ago
|
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
Assignee | ||
Comment 2•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → six.dsn
Reporter | ||
Comment 3•11 years ago
|
||
Thanks!
Reporter | ||
Updated•11 years ago
|
Severity: normal → enhancement
Priority: -- → P4
Reporter | ||
Comment 4•11 years ago
|
||
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.)
Reporter | ||
Comment 5•11 years ago
|
||
(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
Assignee | ||
Comment 6•11 years ago
|
||
This patch adds MOZ_OVERRIDE to the 3 mentionned files:
- nsObjectFrame.h
- nsImageFrame.h
- nsImageControlFrame.cpp
Attachment #8377831 -
Flags: review?(dholbert)
Assignee | ||
Comment 7•11 years ago
|
||
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)
Reporter | ||
Comment 8•11 years ago
|
||
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+
Reporter | ||
Updated•11 years ago
|
Attachment #8377839 -
Flags: review?(dholbert) → review+
Reporter | ||
Comment 9•11 years ago
|
||
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!
Flags: in-testsuite-
Reporter | ||
Updated•11 years ago
|
Attachment #8377839 -
Attachment description: typedef_instead_define.patch → part 1: typedef_instead_define.patch
Reporter | ||
Updated•11 years ago
|
Attachment #8377831 -
Attachment description: moz_override_973805.patch → part 2: moz_override_973805.patch
Assignee | ||
Comment 10•11 years ago
|
||
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•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/82299ce5b67d
https://hg.mozilla.org/mozilla-central/rev/e2ce7a9a1e91
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•